Re: [PATCH 25/33] drm/i915: Move object close under its own lock

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Mika Kuoppala (2019-05-22 15:32:34)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index 5016a3e1f863..a9608d9ced6a 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -95,24 +95,42 @@ void i915_lut_handle_free(struct i915_lut_handle *lut)
> >  
> >  static void lut_close(struct i915_gem_context *ctx)
> >  {
> > -     struct i915_lut_handle *lut, *ln;
> >       struct radix_tree_iter iter;
> >       void __rcu **slot;
> >  
> > -     list_for_each_entry_safe(lut, ln, &ctx->handles_list, ctx_link) {
> > -             list_del(&lut->obj_link);
> > -             i915_lut_handle_free(lut);
> > -     }
> > -     INIT_LIST_HEAD(&ctx->handles_list);
> > +     lockdep_assert_held(&ctx->mutex);
> >  
> >       rcu_read_lock();
> >       radix_tree_for_each_slot(slot, &ctx->handles_vma, &iter, 0) {
> >               struct i915_vma *vma = rcu_dereference_raw(*slot);
> > -
> > -             radix_tree_iter_delete(&ctx->handles_vma, &iter, slot);
> > -
> > -             vma->open_count--;
> > -             i915_vma_put(vma);
> > +             struct drm_i915_gem_object *obj = vma->obj;
> > +             struct i915_lut_handle *lut;
> > +             bool found = false;
> > +
> > +             rcu_read_unlock();
> > +             i915_gem_object_lock(obj);
> > +             list_for_each_entry(lut, &obj->lut_list, obj_link) {
> > +                     if (lut->ctx != ctx)
> > +                             continue;
> > +
> > +                     if (lut->handle != iter.index)
> > +                             continue;
> > +
> > +                     list_del(&lut->obj_link);
> > +                     i915_lut_handle_free(lut);
> 
> You could free this after object_unlock if you want. Shrug.
> 
> > +                     found = true;
> > +                     break;
> > +             }
> > +             i915_gem_object_unlock(obj);
> > +             rcu_read_lock();
> > +
> > +             if (found) {
> > +                     radix_tree_iter_delete(&ctx->handles_vma, &iter, slot);
> > +                     if (atomic_dec_and_test(&vma->open_count) &&
> > +                         !i915_vma_is_ggtt(vma))
> > +                             i915_vma_close(vma);
> > +                     i915_gem_object_put(obj);
> 
> I am strugging to pair this with get.

[snip]

> >  #endif /* __I915_GEM_CONTEXT_TYPES_H__ */
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > index 13ab2a2e0099..fa0a880ca4de 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > @@ -801,9 +801,6 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
> >       unsigned int i, batch;
> >       int err;
> >  
> > -     if (unlikely(i915_gem_context_is_closed(eb->gem_context)))
> > -             return -ENOENT;
> > -
> >       if (unlikely(i915_gem_context_is_banned(eb->gem_context)))
> >               return -EIO;
> >  
> > @@ -812,6 +809,12 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
> >  
> >       batch = eb_batch_index(eb);
> >  
> > +     mutex_lock(&eb->gem_context->mutex);
> > +     if (unlikely(i915_gem_context_is_closed(eb->gem_context))) {
> > +             err = -ENOENT;
> > +             goto err_ctx;
> > +     }
> > +
> >       for (i = 0; i < eb->buffer_count; i++) {
> >               u32 handle = eb->exec[i].handle;
> >               struct i915_lut_handle *lut;
> > @@ -846,12 +849,14 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
> >               }
> >  
> >               /* transfer ref to ctx */

It is this reference that we transfer from the object lookup into the
lut, that we must release if we throwaway the lut.

> > -             if (!vma->open_count++)
> > +             if (!atomic_fetch_inc(&vma->open_count))
> >                       i915_vma_reopen(vma);
> > -             list_add(&lut->obj_link, &obj->lut_list);
> > -             list_add(&lut->ctx_link, &eb->gem_context->handles_list);
> > -             lut->ctx = eb->gem_context;
> >               lut->handle = handle;
> > +             lut->ctx = eb->gem_context;
> > +
> > +             i915_gem_object_lock(obj);
> > +             list_add(&lut->obj_link, &obj->lut_list);
> > +             i915_gem_object_unlock(obj);

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx