| Message ID | 1434718786-28121-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 19, 2015 at 01:59:46PM +0100, Chris Wilson wrote: > Internal requirement for the alignment is that it must be a > power-of-two, so enforce rejection at the user interface to execbuffer > (which allows the caller to specify a stricter-than-expected alignment > criterion). > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> That sounds right in general and I could find at least one instance where we rely on alignment being a power of two (eb_vma_misplaced() and vma->node.start & (entry->alignment - 1)) so: Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
On Mon, Jun 22, 2015 at 12:03:08PM +0100, Damien Lespiau wrote: > On Fri, Jun 19, 2015 at 01:59:46PM +0100, Chris Wilson wrote: > > Internal requirement for the alignment is that it must be a > > power-of-two, so enforce rejection at the user interface to execbuffer > > (which allows the caller to specify a stricter-than-expected alignment > > criterion). > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > That sounds right in general and I could find at least one instance > where we rely on alignment being a power of two (eb_vma_misplaced() and > vma->node.start & (entry->alignment - 1)) > > so: > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> Needs a nasty igt I think ... Do we have? Applied meanwhile. -Daniel > > -- > Damien > > > --- > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > index f4fb2bd33753..b14733908d8d 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > @@ -1189,6 +1189,9 @@ validate_exec_list(const struct drm_i915_gem_execbuffer2 *args, > > if (exec[i].flags & invalid_flags) > > return -EINVAL; > > > > + if (exec[i].alignment && !is_power_of_2(exec[i].alignment)) > > + return -EINVAL; > > + > > /* pad_to_size was once a reserved field, so sanitize it */ > > if (exec[i].flags & EXEC_OBJECT_PAD_TO_SIZE) { > > if (offset_in_page(exec[i].pad_to_size)) > > -- > > 2.1.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jun 22, 2015 at 04:08:20PM +0200, Daniel Vetter wrote: > On Mon, Jun 22, 2015 at 12:03:08PM +0100, Damien Lespiau wrote: > > On Fri, Jun 19, 2015 at 01:59:46PM +0100, Chris Wilson wrote: > > > Internal requirement for the alignment is that it must be a > > > power-of-two, so enforce rejection at the user interface to execbuffer > > > (which allows the caller to specify a stricter-than-expected alignment > > > criterion). > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > That sounds right in general and I could find at least one instance > > where we rely on alignment being a power of two (eb_vma_misplaced() and > > vma->node.start & (entry->alignment - 1)) > > > > so: > > > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> > > Needs a nasty igt I think ... Do we have? Applied meanwhile. Sure, we can demonstrate a bug in the current code that would not realign an object to the arbitrary alignment requested by the user. -Chris
On Mon, Jun 22, 2015 at 03:28:26PM +0100, Chris Wilson wrote: > On Mon, Jun 22, 2015 at 04:08:20PM +0200, Daniel Vetter wrote: > > On Mon, Jun 22, 2015 at 12:03:08PM +0100, Damien Lespiau wrote: > > > On Fri, Jun 19, 2015 at 01:59:46PM +0100, Chris Wilson wrote: > > > > Internal requirement for the alignment is that it must be a > > > > power-of-two, so enforce rejection at the user interface to execbuffer > > > > (which allows the caller to specify a stricter-than-expected alignment > > > > criterion). > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > That sounds right in general and I could find at least one instance > > > where we rely on alignment being a power of two (eb_vma_misplaced() and > > > vma->node.start & (entry->alignment - 1)) > > > > > > so: > > > > > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> > > > > Needs a nasty igt I think ... Do we have? Applied meanwhile. > > Sure, we can demonstrate a bug in the current code that would not > realign an object to the arbitrary alignment requested by the user. igt/gem_exec_alignment exercises said bug. -Chris
On Mon, Jun 22, 2015 at 03:28:26PM +0100, Chris Wilson wrote: > On Mon, Jun 22, 2015 at 04:08:20PM +0200, Daniel Vetter wrote: > > On Mon, Jun 22, 2015 at 12:03:08PM +0100, Damien Lespiau wrote: > > > On Fri, Jun 19, 2015 at 01:59:46PM +0100, Chris Wilson wrote: > > > > Internal requirement for the alignment is that it must be a > > > > power-of-two, so enforce rejection at the user interface to execbuffer > > > > (which allows the caller to specify a stricter-than-expected alignment > > > > criterion). > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > That sounds right in general and I could find at least one instance > > > where we rely on alignment being a power of two (eb_vma_misplaced() and > > > vma->node.start & (entry->alignment - 1)) > > > > > > so: > > > > > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> > > > > Needs a nasty igt I think ... Do we have? Applied meanwhile. > > Sure, we can demonstrate a bug in the current code that would not > realign an object to the arbitrary alignment requested by the user. Just checking that the kernel rejects non-pot alignment should be good enough. No need imo to write a _that_ nasty igt ;-) -Daniel
On Mon, Jun 22, 2015 at 05:32:49PM +0200, Daniel Vetter wrote: > On Mon, Jun 22, 2015 at 03:28:26PM +0100, Chris Wilson wrote: > > On Mon, Jun 22, 2015 at 04:08:20PM +0200, Daniel Vetter wrote: > > > On Mon, Jun 22, 2015 at 12:03:08PM +0100, Damien Lespiau wrote: > > > > On Fri, Jun 19, 2015 at 01:59:46PM +0100, Chris Wilson wrote: > > > > > Internal requirement for the alignment is that it must be a > > > > > power-of-two, so enforce rejection at the user interface to execbuffer > > > > > (which allows the caller to specify a stricter-than-expected alignment > > > > > criterion). > > > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > > > That sounds right in general and I could find at least one instance > > > > where we rely on alignment being a power of two (eb_vma_misplaced() and > > > > vma->node.start & (entry->alignment - 1)) > > > > > > > > so: > > > > > > > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> > > > > > > Needs a nasty igt I think ... Do we have? Applied meanwhile. > > > > Sure, we can demonstrate a bug in the current code that would not > > realign an object to the arbitrary alignment requested by the user. > > Just checking that the kernel rejects non-pot alignment should be good > enough. No need imo to write a _that_ nasty igt ;-) Nah, we will probably fix our pot requirement (given a usecase), so just that the kernel fails to adhere to the user's request without throwing an error is the bug. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index f4fb2bd33753..b14733908d8d 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1189,6 +1189,9 @@ validate_exec_list(const struct drm_i915_gem_execbuffer2 *args, if (exec[i].flags & invalid_flags) return -EINVAL; + if (exec[i].alignment && !is_power_of_2(exec[i].alignment)) + return -EINVAL; + /* pad_to_size was once a reserved field, so sanitize it */ if (exec[i].flags & EXEC_OBJECT_PAD_TO_SIZE) { if (offset_in_page(exec[i].pad_to_size))
Internal requirement for the alignment is that it must be a power-of-two, so enforce rejection at the user interface to execbuffer (which allows the caller to specify a stricter-than-expected alignment criterion). Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +++ 1 file changed, 3 insertions(+)