[Intel-gfx] [PATCH 02/23] drm/i915/gem: Split the context's obj:vma lut into its own mutex

[Chris Wilson]

Quoting Andi Shyti (2020-07-02 23:09:44)
> Hi Chris,
> 
> > @@ -1312,11 +1314,11 @@ static int set_ppgtt(struct drm_i915_file_private *file_priv,
> >       if (vm == rcu_access_pointer(ctx->vm))
> >               goto unlock;
> >  
> > +     old = __set_ppgtt(ctx, vm);
> > +
> >       /* Teardown the existing obj:vma cache, it will have to be rebuilt. */
> >       lut_close(ctx);
> >  
> > -     old = __set_ppgtt(ctx, vm);
> > -
> >       /*
> >        * We need to flush any requests using the current ppgtt before
> >        * we release it as the requests do not hold a reference themselves,
> > @@ -1330,6 +1332,7 @@ static int set_ppgtt(struct drm_i915_file_private *file_priv,
> >       if (err) {
> >               i915_vm_close(__set_ppgtt(ctx, old));
> >               i915_vm_close(old);
> > +             lut_close(ctx); /* rebuild the old obj:vma cache */
> 
> I don't really understand this but it doesn't hurt

Yeah; another testcase required for racing set-vm against execbuf.
Outcome unknown, all that we have to avoid are explosions. Userspace is
allowed to shoot itself in the foot, but is not allowed to shoot anyone
else.
 
> > diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_context.c b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> > index aa0d06cf1903..51b5a3421b40 100644
> > --- a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> > +++ b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> > @@ -23,6 +23,8 @@ mock_context(struct drm_i915_private *i915,
> >       INIT_LIST_HEAD(&ctx->link);
> >       ctx->i915 = i915;
> >  
> > +     mutex_init(&ctx->mutex);
> > +
> >       spin_lock_init(&ctx->stale.lock);
> >       INIT_LIST_HEAD(&ctx->stale.engines);
> >  
> > @@ -35,7 +37,7 @@ mock_context(struct drm_i915_private *i915,
> >       RCU_INIT_POINTER(ctx->engines, e);
> >  
> >       INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
> > -     mutex_init(&ctx->mutex);
> > +     mutex_init(&ctx->lut_mutex);
> 
> ...and I don't really understand why moved the first
> init(&ctx->mutex) above, is it just aesthetic?

Yup. The ctx->mutex is the broader one, so I felt it deserved to be
higher. Whereas here we are setting up the lut [handles_vma] so was the
natural spot to place the ctx->lut_mutex; and I wanted some distance
between the pair to keep the confusion at bay.
-Chris