Quoting Tvrtko Ursulin (2020-03-20 13:47:39)
>
> On 20/03/2020 13:01, Chris Wilson wrote:
> > As we store the handle lookup inside a radix tree, we do not need the
> > gem_context->mutex except until we need to insert our lookup into the
> > common radix tree. This takes a small bit of rearranging to ensure that
> > the lut we insert into the tree is ready prior to actually inserting it
> > (as soon as it is exposed via the radixtree, it is visible to any other
> > submission).
> >
> > v2: For brownie points, remove the goto spaghetti.
> > v3: Tighten up the closed-handle checks.
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 136 +++++++++++-------
> > 1 file changed, 87 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > index c1179c00bc61..876fc2e124b9 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > @@ -481,7 +481,7 @@ eb_add_vma(struct i915_execbuffer *eb,
> >
> > GEM_BUG_ON(i915_vma_is_closed(vma));
> >
> > - ev->vma = i915_vma_get(vma);
> > + ev->vma = vma;
> > ev->exec = entry;
> > ev->flags = entry->flags;
> >
> > @@ -728,77 +728,117 @@ static int eb_select_context(struct i915_execbuffer *eb)
> > return 0;
> > }
> >
> > -static int eb_lookup_vmas(struct i915_execbuffer *eb)
> > +static int __eb_add_lut(struct i915_execbuffer *eb,
> > + u32 handle, struct i915_vma *vma)
> > {
> > - struct radix_tree_root *handles_vma = &eb->gem_context->handles_vma;
> > - struct drm_i915_gem_object *obj;
> > - unsigned int i, batch;
> > + struct i915_gem_context *ctx = eb->gem_context;
> > + struct i915_lut_handle *lut;
> > int err;
> >
> > - if (unlikely(i915_gem_context_is_closed(eb->gem_context)))
> > - return -ENOENT;
> > + lut = i915_lut_handle_alloc();
> > + if (unlikely(!lut))
> > + return -ENOMEM;
> >
> > - INIT_LIST_HEAD(&eb->relocs);
> > - INIT_LIST_HEAD(&eb->unbound);
> > + i915_vma_get(vma);
> > + if (!atomic_fetch_inc(&vma->open_count))
> > + i915_vma_reopen(vma);
> > + lut->handle = handle;
> > + lut->ctx = ctx;
> > +
> > + /* Check that the context hasn't been closed in the meantime */
> > + err = -EINTR;
> > + if (!mutex_lock_interruptible(&ctx->mutex)) {
> > + err = -ENOENT;
> > + if (likely(!i915_gem_context_is_closed(ctx)))
> > + err = radix_tree_insert(&ctx->handles_vma, handle, vma);
> > + if (err == 0) { /* And nor has this handle */
> > + struct drm_i915_gem_object *obj = vma->obj;
> > +
> > + i915_gem_object_lock(obj);
>
> Does this fit into Maarten's rework - nesting of object_lock under
> ctx->mutex I mean?
This is the current lock nesting, and should generally remain so. One
context will peek at multiple objects and we should be holding an object
lock to peek at a context.
As for the rework, it holds a bunch of exclusive locks far beyond their
scope causing an even worse BKL, and that will have to be reworked.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
