[Intel-gfx] [PATCH 1/5] drm/i915: Round up object allocations

[Ben Widawsky]

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 at 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 at 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