Re: [Intel-gfx] [PATCH v5 01/12] drm/i915: Reference objects on the ww object list

From: Daniel Vetter <daniel@ffwll.ch>
To: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, matthew.auld@intel.com,
	dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v5 01/12] drm/i915: Reference objects on the ww object list
Date: Thu, 17 Jun 2021 21:13:42 +0200	[thread overview]
Message-ID: <YMue5i1Gqoo4ERru@phenom.ffwll.local> (raw)
In-Reply-To: <20210617063018.92802-2-thomas.hellstrom@linux.intel.com>

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@linux.intel.com>
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch