* [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.