Quoting Mika Kuoppala (2017-08-17 15:10:02)
> Chris Wilson <[email protected]> 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 <[email protected]>
> > Cc: Tvrtko Ursulin <[email protected]>
> > Cc: Joonas Lahtinen <[email protected]>
> > Cc: Mika Kuoppala <[email protected]>
> > ---
> >  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
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to