Re: [PATCH 04/12] drm/i915: Don't unconditionally try to deref aliasing ppgtt

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Nov 25, 2013 at 09:08:50PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 25, 2013 at 10:10:28AM -0800, Ben Widawsky wrote:
> > On Mon, Nov 25, 2013 at 06:06:18PM +0000, Chris Wilson wrote:
> > > On Mon, Nov 25, 2013 at 09:54:35AM -0800, Ben Widawsky wrote:
> > > > Since the beginning, the functions which try to properly reference the
> > > > aliasing PPGTT have deferences a potentially null aliasing_ppgtt member.
> > > > Since the accessors are meant to be global, this will not do.
> > > > 
> > > > Introduced originally in:
> > > > commit a70a3148b0c61cb7c588ea650db785b261b378a3
> > > > Author: Ben Widawsky <ben@bwidawsk.net>
> > > > Date:   Wed Jul 31 16:59:56 2013 -0700
> > > > 
> > > >     drm/i915: Make proper functions for VMs
> > > > 
> > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > > index 40d9dcf..bc5c865 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > @@ -4971,7 +4971,8 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> > > >  	struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> > > >  	struct i915_vma *vma;
> > > >  
> > > > -	if (vm == &dev_priv->mm.aliasing_ppgtt->base)
> > > > +	if (!dev_priv->mm.aliasing_ppgtt ||
> > > > +	    vm == &dev_priv->mm.aliasing_ppgtt->base)
> > > 
> > > Where's the dereference? gcc is smarter than your average bear.
> > > -Chris
> > 
> > I had assumed: dev_priv->mm.aliasing_ppgtt->base in cases when
> > aliasing_ppgtt was NULL. Given that I never actually hit this, I agree
> > GCC must be doing something.
> 
> Sounds like another discussion on the implementation of offsetof().
> 
> > 
> > Is such behavior documented somewhere? (forgive the lazy)
> 
> IIRC the C standard does say that for &*foo it's as if both & and *
> weren't there (and IIRC the same for &foo[x]), but doesn't really
> say that kind of thing for &foo->bar. I guess it's a gray area,
> and just happens work that way on certain compilers.
> 
> In this particular case I think there's one slight issue. If
> aliasing_pggtt == NULL, and someone passes in vm == NULL by
> accident, then it'll take the branch and use ggtt because
> &aliasing_ppgtt->base will be NULL too (due to base being at
> offset 0 in the struct). Now, I don't know if a NULL
> vm could survive this far into the code, but it it can, then it
> might make debugging a bit more "fun".

If you pass in a NULL vm then you get to keep both pieces. And wrt
portability atm you can pick any compiler for the linux kernel as long as
it's gcc compatible. So imo nothing to really worry about.

Aside: We really should start to ditch all these (vm, obj) pair lookups
and move to using vmas everywhere ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch