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

[Chris Wilson]

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

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