On Thu, Jun 17, 2021 at 08:30:07AM +0200, Thomas Hellström wrote:
> Since the ww transaction endpoint easily end up far out-of-scope of
> the objects on the ww object list, particularly for contending lock
> objects, make sure we reference objects on the list so they don't
> disappear under us.
>
> This comes with a performance penalty so it's been debated whether this
> is really needed. But I think this is motivated by the fact that locking
> is typically difficult to get right, and whatever we can do to make it
> simpler for developers moving forward should be done, unless the
> performance impact is far too high.
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> Reviewed-by: Matthew Auld <matthew.auld@xxxxxxxxx>
Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
I've looked the past 2-3 weeks in-depth at our execbuf code. That has
definitely gone way too far into "very clevery" territory, and safe is so
much better than clever.
If there's a fundamental performance issue, we need to fix this in a
fundamental way. E.g. for this one here a possible solution could be
VM_BIND, at least in the fastpath, where we don't need to look-up any
objects, nor refcount them, nor anything else (at least that's the goal).
Only some per vm/request book-keeping and done.
Also I think we can easily claw this back once we get to the cleanup part
of this work: i915_vma_pin has a bunch of atomics (and lots of locks in
slow-paths) of its own, which are largely redundant now that object state
is protected by dma_resv_lock. Once that's cleaned up we can pay our
atomic inc/dec here with the removed atomic ops from the vma side I think.
Anyway just figured I drop some thoughts and my ack on the direction
you're pushing here.
-Daniel
> ---
> drivers/gpu/drm/i915/gem/i915_gem_object.h | 8 ++++++--
> drivers/gpu/drm/i915/i915_gem.c | 4 ++++
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index d66aa00d023a..241666931945 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -169,13 +169,17 @@ static inline int __i915_gem_object_lock(struct drm_i915_gem_object *obj,
> else
> ret = dma_resv_lock(obj->base.resv, ww ? &ww->ctx : NULL);
>
> - if (!ret && ww)
> + if (!ret && ww) {
> + i915_gem_object_get(obj);
> list_add_tail(&obj->obj_link, &ww->obj_list);
> + }
> if (ret == -EALREADY)
> ret = 0;
>
> - if (ret == -EDEADLK)
> + if (ret == -EDEADLK) {
> + i915_gem_object_get(obj);
> ww->contended = obj;
> + }
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6a0a3f0e36e1..c62dcd0e341a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1222,6 +1222,7 @@ static void i915_gem_ww_ctx_unlock_all(struct i915_gem_ww_ctx *ww)
> while ((obj = list_first_entry_or_null(&ww->obj_list, struct drm_i915_gem_object, obj_link))) {
> list_del(&obj->obj_link);
> i915_gem_object_unlock(obj);
> + i915_gem_object_put(obj);
> }
> }
>
> @@ -1229,6 +1230,7 @@ void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj)
> {
> list_del(&obj->obj_link);
> i915_gem_object_unlock(obj);
> + i915_gem_object_put(obj);
> }
>
> void i915_gem_ww_ctx_fini(struct i915_gem_ww_ctx *ww)
> @@ -1253,6 +1255,8 @@ int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ww)
>
> if (!ret)
> list_add_tail(&ww->contended->obj_link, &ww->obj_list);
> + else
> + i915_gem_object_put(ww->contended);
>
> ww->contended = NULL;
>
> --
> 2.31.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
