[Intel-gfx] [PATCH 21/27] drm/i915: Move context management under GEM

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-26 14:57:24)
> 
> On 25/09/2019 11:01, Chris Wilson wrote:
> >   void i915_gem_context_release(struct kref *ref)
> >   {
> >       struct i915_gem_context *ctx = container_of(ref, typeof(*ctx), ref);
> > -     struct drm_i915_private *i915 = ctx->i915;
> > +     struct i915_gem_contexts *gc = &ctx->i915->gem.contexts;
> >   
> >       trace_i915_context_free(ctx);
> > -     if (llist_add(&ctx->free_link, &i915->contexts.free_list))
> > -             queue_work(i915->wq, &i915->contexts.free_work);
> > +     if (llist_add(&ctx->free_link, &gc->free_list))
> > +             queue_work(ctx->i915->wq, &gc->free_work);
> 
> At first I thought gc was some sort of garbage collection list. But it 
> is a GEM contexts list. :) This hunk looks completely avoidable, I don't 
> see it brings much benefit on balance since in turn it adds ctx->i915 
> twice . But as you wish..

Since it's not taking struct_mutex it doesn't need i915->wq any more,
good point.

> > @@ -464,11 +454,11 @@ static void __apply_ppgtt(struct intel_context *ce, void *vm)
> >   static struct i915_address_space *
> >   __set_ppgtt(struct i915_gem_context *ctx, struct i915_address_space *vm)
> >   {
> > -     struct i915_address_space *old = ctx->vm;
> > +     struct i915_address_space *old = rcu_dereference_protected(ctx->vm, 1);
> >   
> >       GEM_BUG_ON(old && i915_vm_is_4lvl(vm) != i915_vm_is_4lvl(old));
> >   
> > -     ctx->vm = i915_vm_open(vm);
> > +     rcu_assign_pointer(ctx->vm, i915_vm_open(vm));
> 
> Are updaters here serialized? Lost track if struct mutex is still held 
> in set_ppgtt at this point in the series or not.. Thinking of "old = 
> rcu_dereference_protected.." and here.

Yes, ctx->mutex; I was being very lazy in not marking it up; especially
in the selftests where it was so much more dontcare.

> > @@ -653,8 +633,8 @@ static int gem_context_register(struct i915_gem_context *ctx,
> >       int ret;
> >   
> >       ctx->file_priv = fpriv;
> > -     if (ctx->vm)
> > -             ctx->vm->file = fpriv;
> > +     if (rcu_access_pointer(ctx->vm))
> > +             rcu_dereference_protected(ctx->vm, true)->file = fpriv;
> 
> Here rcu accesses are just to satisfy sparse?

Total eyesore, yup. I'm thinking I just declare ctx->vm to be protected
by ctx->mutex and make it so.
-Chris