* [PATCH 1/5] drm/i915: Round up object allocations [not found] <1390533674-18402-1-git-send-email-benjamin.widawsky@intel.com> @ 2014-01-24 3:21 ` Ben Widawsky 2014-01-25 20:28 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Ben Widawsky @ 2014-01-24 3:21 UTC (permalink / raw) To: Internal GFX Development, Intel GFX; +Cc: Ben Widawsky, Ben Widawsky DRM gets very mad when you request an object which occupies a partial page. As a DRM driver, i915 never really wants to anger DRM, and would always just want the rounding done for us. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 024e454..8cd1134 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, struct address_space *mapping; gfp_t mask; + size = round_up(size, PAGE_SIZE); + obj = i915_gem_object_alloc(dev); if (obj == NULL) return NULL; -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] drm/i915: Round up object allocations 2014-01-24 3:21 ` [PATCH 1/5] drm/i915: Round up object allocations Ben Widawsky @ 2014-01-25 20:28 ` Daniel Vetter 2014-01-26 5:49 ` Ben Widawsky 0 siblings, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2014-01-25 20:28 UTC (permalink / raw) To: Ben Widawsky; +Cc: Intel GFX, Ben Widawsky, Internal GFX Development On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: > DRM gets very mad when you request an object which occupies a partial > page. As a DRM driver, i915 never really wants to anger DRM, and would > always just want the rounding done for us. > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 024e454..8cd1134 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > struct address_space *mapping; > gfp_t mask; > > + size = round_up(size, PAGE_SIZE); > + Nope, if there's some code that doesn't do page-aligend bo allocations it needs to be fixed there. If you want throw a WARN_ON and early return in here. -Daniel > obj = i915_gem_object_alloc(dev); > if (obj == NULL) > return NULL; > -- > 1.8.5.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] drm/i915: Round up object allocations 2014-01-25 20:28 ` Daniel Vetter @ 2014-01-26 5:49 ` Ben Widawsky 2014-01-26 9:17 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Ben Widawsky @ 2014-01-26 5:49 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel GFX, Ben Widawsky On Sat, Jan 25, 2014 at 09:28:24PM +0100, Daniel Vetter wrote: > On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: > > DRM gets very mad when you request an object which occupies a partial > > page. As a DRM driver, i915 never really wants to anger DRM, and would > > always just want the rounding done for us. > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 024e454..8cd1134 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > > struct address_space *mapping; > > gfp_t mask; > > > > + size = round_up(size, PAGE_SIZE); > > + > > Nope, if there's some code that doesn't do page-aligend bo allocations it > needs to be fixed there. If you want throw a WARN_ON and early return in > here. > -Daniel Why? -- Ben Widawsky, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] drm/i915: Round up object allocations 2014-01-26 5:49 ` Ben Widawsky @ 2014-01-26 9:17 ` Daniel Vetter 2014-01-26 9:57 ` Chris Wilson 0 siblings, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2014-01-26 9:17 UTC (permalink / raw) To: Ben Widawsky; +Cc: Intel GFX, Ben Widawsky On Sun, Jan 26, 2014 at 6:49 AM, Ben Widawsky <ben@bwidawsk.net> wrote: > On Sat, Jan 25, 2014 at 09:28:24PM +0100, Daniel Vetter wrote: >> On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: >> > DRM gets very mad when you request an object which occupies a partial >> > page. As a DRM driver, i915 never really wants to anger DRM, and would >> > always just want the rounding done for us. >> > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> >> > --- >> > drivers/gpu/drm/i915/i915_gem.c | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> > index 024e454..8cd1134 100644 >> > --- a/drivers/gpu/drm/i915/i915_gem.c >> > +++ b/drivers/gpu/drm/i915/i915_gem.c >> > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, >> > struct address_space *mapping; >> > gfp_t mask; >> > >> > + size = round_up(size, PAGE_SIZE); >> > + >> >> Nope, if there's some code that doesn't do page-aligend bo allocations it >> needs to be fixed there. If you want throw a WARN_ON and early return in >> here. >> -Daniel > > Why? Because allocating a non-page aligned gem bo is a bug. All current in-kernel allocations are already aligned. I've thought that we also reject unaligned request from userspace but apparently we help out since forever (i.e. gem was merged). Might be worth a shot to turn that into an -EINVAL if libdrm does the right rounding ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] drm/i915: Round up object allocations 2014-01-26 9:17 ` Daniel Vetter @ 2014-01-26 9:57 ` Chris Wilson 2014-01-26 10:01 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Chris Wilson @ 2014-01-26 9:57 UTC (permalink / raw) To: Daniel Vetter; +Cc: Ben Widawsky, Intel GFX, Ben Widawsky On Sun, Jan 26, 2014 at 10:17:57AM +0100, Daniel Vetter wrote: > On Sun, Jan 26, 2014 at 6:49 AM, Ben Widawsky <ben@bwidawsk.net> wrote: > > On Sat, Jan 25, 2014 at 09:28:24PM +0100, Daniel Vetter wrote: > >> On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: > >> > DRM gets very mad when you request an object which occupies a partial > >> > page. As a DRM driver, i915 never really wants to anger DRM, and would > >> > always just want the rounding done for us. > >> > > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > >> > --- > >> > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > >> > 1 file changed, 2 insertions(+) > >> > > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >> > index 024e454..8cd1134 100644 > >> > --- a/drivers/gpu/drm/i915/i915_gem.c > >> > +++ b/drivers/gpu/drm/i915/i915_gem.c > >> > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > >> > struct address_space *mapping; > >> > gfp_t mask; > >> > > >> > + size = round_up(size, PAGE_SIZE); > >> > + > >> > >> Nope, if there's some code that doesn't do page-aligend bo allocations it > >> needs to be fixed there. If you want throw a WARN_ON and early return in > >> here. > >> -Daniel > > > > Why? > > Because allocating a non-page aligned gem bo is a bug. All current > in-kernel allocations are already aligned. I've thought that we also > reject unaligned request from userspace but apparently we help out > since forever (i.e. gem was merged). Might be worth a shot to turn > that into an -EINVAL if libdrm does the right rounding ... We already have an -EINVAL guard on our create ioctl(s). -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] drm/i915: Round up object allocations 2014-01-26 9:57 ` Chris Wilson @ 2014-01-26 10:01 ` Daniel Vetter 2014-01-26 10:07 ` Chris Wilson 0 siblings, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2014-01-26 10:01 UTC (permalink / raw) To: Chris Wilson, Daniel Vetter, Ben Widawsky, Intel GFX, Ben Widawsky On Sun, Jan 26, 2014 at 09:57:08AM +0000, Chris Wilson wrote: > On Sun, Jan 26, 2014 at 10:17:57AM +0100, Daniel Vetter wrote: > > On Sun, Jan 26, 2014 at 6:49 AM, Ben Widawsky <ben@bwidawsk.net> wrote: > > > On Sat, Jan 25, 2014 at 09:28:24PM +0100, Daniel Vetter wrote: > > >> On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: > > >> > DRM gets very mad when you request an object which occupies a partial > > >> > page. As a DRM driver, i915 never really wants to anger DRM, and would > > >> > always just want the rounding done for us. > > >> > > > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > >> > --- > > >> > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > >> > 1 file changed, 2 insertions(+) > > >> > > > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > >> > index 024e454..8cd1134 100644 > > >> > --- a/drivers/gpu/drm/i915/i915_gem.c > > >> > +++ b/drivers/gpu/drm/i915/i915_gem.c > > >> > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > > >> > struct address_space *mapping; > > >> > gfp_t mask; > > >> > > > >> > + size = round_up(size, PAGE_SIZE); > > >> > + > > >> > > >> Nope, if there's some code that doesn't do page-aligend bo allocations it > > >> needs to be fixed there. If you want throw a WARN_ON and early return in > > >> here. > > >> -Daniel > > > > > > Why? > > > > Because allocating a non-page aligned gem bo is a bug. All current > > in-kernel allocations are already aligned. I've thought that we also > > reject unaligned request from userspace but apparently we help out > > since forever (i.e. gem was merged). Might be worth a shot to turn > > that into an -EINVAL if libdrm does the right rounding ... > > We already have an -EINVAL guard on our create ioctl(s). Only for size == 0, not for non-aligned size, at least afaics (no coffee yet ...). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] drm/i915: Round up object allocations 2014-01-26 10:01 ` Daniel Vetter @ 2014-01-26 10:07 ` Chris Wilson 2014-01-26 19:00 ` Ben Widawsky 0 siblings, 1 reply; 10+ messages in thread From: Chris Wilson @ 2014-01-26 10:07 UTC (permalink / raw) To: Daniel Vetter; +Cc: Ben Widawsky, Intel GFX, Ben Widawsky On Sun, Jan 26, 2014 at 11:01:50AM +0100, Daniel Vetter wrote: > On Sun, Jan 26, 2014 at 09:57:08AM +0000, Chris Wilson wrote: > > On Sun, Jan 26, 2014 at 10:17:57AM +0100, Daniel Vetter wrote: > > > On Sun, Jan 26, 2014 at 6:49 AM, Ben Widawsky <ben@bwidawsk.net> wrote: > > > > On Sat, Jan 25, 2014 at 09:28:24PM +0100, Daniel Vetter wrote: > > > >> On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: > > > >> > DRM gets very mad when you request an object which occupies a partial > > > >> > page. As a DRM driver, i915 never really wants to anger DRM, and would > > > >> > always just want the rounding done for us. > > > >> > > > > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > >> > --- > > > >> > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > > >> > 1 file changed, 2 insertions(+) > > > >> > > > > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > > >> > index 024e454..8cd1134 100644 > > > >> > --- a/drivers/gpu/drm/i915/i915_gem.c > > > >> > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > >> > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > > > >> > struct address_space *mapping; > > > >> > gfp_t mask; > > > >> > > > > >> > + size = round_up(size, PAGE_SIZE); > > > >> > + > > > >> > > > >> Nope, if there's some code that doesn't do page-aligend bo allocations it > > > >> needs to be fixed there. If you want throw a WARN_ON and early return in > > > >> here. > > > >> -Daniel > > > > > > > > Why? > > > > > > Because allocating a non-page aligned gem bo is a bug. All current > > > in-kernel allocations are already aligned. I've thought that we also > > > reject unaligned request from userspace but apparently we help out > > > since forever (i.e. gem was merged). Might be worth a shot to turn > > > that into an -EINVAL if libdrm does the right rounding ... > > > > We already have an -EINVAL guard on our create ioctl(s). > > Only for size == 0, not for non-aligned size, at least afaics (no coffee > yet ...). The lack of caffeine was mine. I say if (blah(PAGE_SIZE)) return -EINVAL and was happy. But as it happens it still implies that the BUG_ON in drm/drm_gem.c is relevant as is and we shouldn't be papering over it. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] drm/i915: Round up object allocations 2014-01-26 10:07 ` Chris Wilson @ 2014-01-26 19:00 ` Ben Widawsky 2014-01-26 19:59 ` Daniel Vetter 2014-01-26 20:03 ` Chris Wilson 0 siblings, 2 replies; 10+ messages in thread From: Ben Widawsky @ 2014-01-26 19:00 UTC (permalink / raw) To: Chris Wilson, Daniel Vetter, Ben Widawsky, Intel GFX On Sun, Jan 26, 2014 at 10:07:45AM +0000, Chris Wilson wrote: > On Sun, Jan 26, 2014 at 11:01:50AM +0100, Daniel Vetter wrote: > > On Sun, Jan 26, 2014 at 09:57:08AM +0000, Chris Wilson wrote: > > > On Sun, Jan 26, 2014 at 10:17:57AM +0100, Daniel Vetter wrote: > > > > On Sun, Jan 26, 2014 at 6:49 AM, Ben Widawsky <ben@bwidawsk.net> wrote: > > > > > On Sat, Jan 25, 2014 at 09:28:24PM +0100, Daniel Vetter wrote: > > > > >> On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: > > > > >> > DRM gets very mad when you request an object which occupies a partial > > > > >> > page. As a DRM driver, i915 never really wants to anger DRM, and would > > > > >> > always just want the rounding done for us. > > > > >> > > > > > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > > >> > --- > > > > >> > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > > > >> > 1 file changed, 2 insertions(+) > > > > >> > > > > > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > > > >> > index 024e454..8cd1134 100644 > > > > >> > --- a/drivers/gpu/drm/i915/i915_gem.c > > > > >> > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > > >> > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > > > > >> > struct address_space *mapping; > > > > >> > gfp_t mask; > > > > >> > > > > > >> > + size = round_up(size, PAGE_SIZE); > > > > >> > + > > > > >> > > > > >> Nope, if there's some code that doesn't do page-aligend bo allocations it > > > > >> needs to be fixed there. If you want throw a WARN_ON and early return in > > > > >> here. > > > > >> -Daniel > > > > > > > > > > Why? > > > > > > > > Because allocating a non-page aligned gem bo is a bug. All current > > > > in-kernel allocations are already aligned. I've thought that we also > > > > reject unaligned request from userspace but apparently we help out > > > > since forever (i.e. gem was merged). Might be worth a shot to turn > > > > that into an -EINVAL if libdrm does the right rounding ... > > > > > > We already have an -EINVAL guard on our create ioctl(s). > > > > Only for size == 0, not for non-aligned size, at least afaics (no coffee > > yet ...). > > The lack of caffeine was mine. I say if (blah(PAGE_SIZE)) return -EINVAL > and was happy. > > But as it happens it still implies that the BUG_ON in drm/drm_gem.c is > relevant as is and we shouldn't be papering over it. > -Chris > I was referring to internal, driver callers. If all internal callers round up, we may as well do it for them. If the earlier assertion is that we block bad sizes already at the IOCTL hanlder... then what's the problem again? -- Ben Widawsky, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] drm/i915: Round up object allocations 2014-01-26 19:00 ` Ben Widawsky @ 2014-01-26 19:59 ` Daniel Vetter 2014-01-26 20:03 ` Chris Wilson 1 sibling, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2014-01-26 19:59 UTC (permalink / raw) To: Ben Widawsky; +Cc: Ben Widawsky, Intel GFX On Sun, Jan 26, 2014 at 11:00:52AM -0800, Ben Widawsky wrote: > On Sun, Jan 26, 2014 at 10:07:45AM +0000, Chris Wilson wrote: > > On Sun, Jan 26, 2014 at 11:01:50AM +0100, Daniel Vetter wrote: > > > On Sun, Jan 26, 2014 at 09:57:08AM +0000, Chris Wilson wrote: > > > > On Sun, Jan 26, 2014 at 10:17:57AM +0100, Daniel Vetter wrote: > > > > > On Sun, Jan 26, 2014 at 6:49 AM, Ben Widawsky <ben@bwidawsk.net> wrote: > > > > > > On Sat, Jan 25, 2014 at 09:28:24PM +0100, Daniel Vetter wrote: > > > > > >> On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: > > > > > >> > DRM gets very mad when you request an object which occupies a partial > > > > > >> > page. As a DRM driver, i915 never really wants to anger DRM, and would > > > > > >> > always just want the rounding done for us. > > > > > >> > > > > > > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > > > >> > --- > > > > > >> > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > > > > >> > 1 file changed, 2 insertions(+) > > > > > >> > > > > > > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > > > > >> > index 024e454..8cd1134 100644 > > > > > >> > --- a/drivers/gpu/drm/i915/i915_gem.c > > > > > >> > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > > > >> > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > > > > > >> > struct address_space *mapping; > > > > > >> > gfp_t mask; > > > > > >> > > > > > > >> > + size = round_up(size, PAGE_SIZE); > > > > > >> > + > > > > > >> > > > > > >> Nope, if there's some code that doesn't do page-aligend bo allocations it > > > > > >> needs to be fixed there. If you want throw a WARN_ON and early return in > > > > > >> here. > > > > > >> -Daniel > > > > > > > > > > > > Why? > > > > > > > > > > Because allocating a non-page aligned gem bo is a bug. All current > > > > > in-kernel allocations are already aligned. I've thought that we also > > > > > reject unaligned request from userspace but apparently we help out > > > > > since forever (i.e. gem was merged). Might be worth a shot to turn > > > > > that into an -EINVAL if libdrm does the right rounding ... > > > > > > > > We already have an -EINVAL guard on our create ioctl(s). > > > > > > Only for size == 0, not for non-aligned size, at least afaics (no coffee > > > yet ...). > > > > The lack of caffeine was mine. I say if (blah(PAGE_SIZE)) return -EINVAL > > and was happy. > > > > But as it happens it still implies that the BUG_ON in drm/drm_gem.c is > > relevant as is and we shouldn't be papering over it. > > -Chris > > > > I was referring to internal, driver callers. If all internal callers > round up, we may as well do it for them. If the earlier assertion is > that we block bad sizes already at the IOCTL hanlder... then what's the > problem again? create2_ioctl should imo reject non-page-aligned requests. I guess we could dig through libdrm history and check whether that's also possible for the current create ioctl. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] drm/i915: Round up object allocations 2014-01-26 19:00 ` Ben Widawsky 2014-01-26 19:59 ` Daniel Vetter @ 2014-01-26 20:03 ` Chris Wilson 1 sibling, 0 replies; 10+ messages in thread From: Chris Wilson @ 2014-01-26 20:03 UTC (permalink / raw) To: Ben Widawsky; +Cc: Ben Widawsky, Intel GFX On Sun, Jan 26, 2014 at 11:00:52AM -0800, Ben Widawsky wrote: > On Sun, Jan 26, 2014 at 10:07:45AM +0000, Chris Wilson wrote: > > On Sun, Jan 26, 2014 at 11:01:50AM +0100, Daniel Vetter wrote: > > > On Sun, Jan 26, 2014 at 09:57:08AM +0000, Chris Wilson wrote: > > > > On Sun, Jan 26, 2014 at 10:17:57AM +0100, Daniel Vetter wrote: > > > > > On Sun, Jan 26, 2014 at 6:49 AM, Ben Widawsky <ben@bwidawsk.net> wrote: > > > > > > On Sat, Jan 25, 2014 at 09:28:24PM +0100, Daniel Vetter wrote: > > > > > >> On Thu, Jan 23, 2014 at 07:21:10PM -0800, Ben Widawsky wrote: > > > > > >> > DRM gets very mad when you request an object which occupies a partial > > > > > >> > page. As a DRM driver, i915 never really wants to anger DRM, and would > > > > > >> > always just want the rounding done for us. > > > > > >> > > > > > > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > > > >> > --- > > > > > >> > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > > > > >> > 1 file changed, 2 insertions(+) > > > > > >> > > > > > > >> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > > > > >> > index 024e454..8cd1134 100644 > > > > > >> > --- a/drivers/gpu/drm/i915/i915_gem.c > > > > > >> > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > > > >> > @@ -4168,6 +4168,8 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > > > > > >> > struct address_space *mapping; > > > > > >> > gfp_t mask; > > > > > >> > > > > > > >> > + size = round_up(size, PAGE_SIZE); > > > > > >> > + > > > > > >> > > > > > >> Nope, if there's some code that doesn't do page-aligend bo allocations it > > > > > >> needs to be fixed there. If you want throw a WARN_ON and early return in > > > > > >> here. > > > > > >> -Daniel > > > > > > > > > > > > Why? > > > > > > > > > > Because allocating a non-page aligned gem bo is a bug. All current > > > > > in-kernel allocations are already aligned. I've thought that we also > > > > > reject unaligned request from userspace but apparently we help out > > > > > since forever (i.e. gem was merged). Might be worth a shot to turn > > > > > that into an -EINVAL if libdrm does the right rounding ... > > > > > > > > We already have an -EINVAL guard on our create ioctl(s). > > > > > > Only for size == 0, not for non-aligned size, at least afaics (no coffee > > > yet ...). > > > > The lack of caffeine was mine. I say if (blah(PAGE_SIZE)) return -EINVAL > > and was happy. > > > > But as it happens it still implies that the BUG_ON in drm/drm_gem.c is > > relevant as is and we shouldn't be papering over it. > > -Chris > > > > I was referring to internal, driver callers. If all internal callers > round up, we may as well do it for them. If the earlier assertion is > that we block bad sizes already at the IOCTL hanlder... then what's the > problem again? You are suggesting we write: size = PAGE_ALIGN(size); BUG_ON(size & ~PAGE_MASK); If you want to change something remove the BUG_ON, do the rounding and update all callers to not worry about page alignment. Tell me how many call sites require alteration? It seems to be an almost universal assumption that the size is page-aligned (despite my own attempts to get subpage allocations into the ABI). -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-01-26 20:03 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1390533674-18402-1-git-send-email-benjamin.widawsky@intel.com>
2014-01-24 3:21 ` [PATCH 1/5] drm/i915: Round up object allocations Ben Widawsky
2014-01-25 20:28 ` Daniel Vetter
2014-01-26 5:49 ` Ben Widawsky
2014-01-26 9:17 ` Daniel Vetter
2014-01-26 9:57 ` Chris Wilson
2014-01-26 10:01 ` Daniel Vetter
2014-01-26 10:07 ` Chris Wilson
2014-01-26 19:00 ` Ben Widawsky
2014-01-26 19:59 ` Daniel Vetter
2014-01-26 20:03 ` Chris Wilson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.