[Intel-gfx] [PATCH 2/7] drm/i915: Check context status before looking up our obj/vma

[Chris Wilson]

Quoting Mika Kuoppala (2017-08-17 15:10:02)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > Since we keep the context around across the slow lookup where we may
> > drop the struct_mutex, we should double check that the context is still
> > valid upon reacquisition.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 359d5dc6d8df..044fb1205554 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -679,13 +679,6 @@ static int eb_select_context(struct i915_execbuffer *eb)
> >       if (unlikely(!ctx))
> >               return -ENOENT;
> >  
> > -     if (unlikely(i915_gem_context_is_banned(ctx))) {
> > -             DRM_DEBUG("Context %u tried to submit while banned\n",
> > -                       ctx->user_handle);
> > -             i915_gem_context_put(ctx);
> > -             return -EIO;
> > -     }
> > -
> >       eb->ctx = ctx;
> >       eb->vm = ctx->ppgtt ? &ctx->ppgtt->base : &eb->i915->ggtt.base;
> >  
> > @@ -707,6 +700,12 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
> >       int slow_pass = -1;
> >       int err;
> >  
> > +     if (unlikely(i915_gem_context_is_closed(eb->ctx)))
> > +             return -ENOENT;
> > +
> > +     if (unlikely(i915_gem_context_is_banned(eb->ctx)))
> > +             return -EIO;
> > +
> 
> We are referring the lut before the context has been validated.
> Not that it matters but for style, please consider assigning
> the lut after the context check.

~o~ Not feeling it. If it was checking for an invalid pointer, then yes
rearranging the code to avoid the appearance of the early dereference
makes sense, but as it is we are just creating an alias for part of the
ctx. Should look at whether gcc likes a few more locals...
-Chris