Re: [RFC PATCH v3 16/21] drm/exec: Introduce an evict mode

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Wed, 2024-05-22 at 15:28 +0200, Christian König wrote:
> Am 21.05.24 um 09:16 schrieb Thomas Hellström:
> > Locking for eviction is in some way different from locking for
> > submission:
> > 
> > 1) We can't lock objects that are already locked for submission,
> > hence DRM_EXEC_IGNORE_DUPLICATES must be unset.
> > 2) We must be able to re-lock objects locked for eviction,
> > either for submission or for yet another eviction, in
> > particular objects sharing a single resv must be considered.
> 
> Yeah, I was already thinking about that as well.
> 
> My idea so far was to have a separate function for locking eviction
> BOs. 
> This function would then use trylock or blocking depending on some
> setting.

Downstream i915 also has a separate locking function for this. I'm fine
with that as well. Probably the most sane choice.


> 
> > 3) There is no point to keep a contending object after the
> > transaction restart. We don't know whether we actually want to use
> > it again.
> 
> Well that isn't true as far as I know.
> 
> If we don't use trylock we still need to lock the object after
> rollback 
> to make sure that we waited for it to become available.

Yes, the transaction restart mentioned above is *after* the relaxation,
so the rollback becomes:

unlock_all
lock_contending_lock.
unlock_contending_lock.
drop_contending lock.


/Thomas


> 
> Regards,
> Christian.
> 
> > So introduce a drm_exec evict mode, and for now instead of
> > explicitly setting it using a function call or implement separate
> > locking functions that use evict mode, assume evict mode if
> > there is a snapshot registered. This can easily be changed later.
> > 
> > To keep track of resvs locked for eviction, use a pointer set
> > implemented by an xarray. This is probably not the most efficient
> > data structure but used as an easy-to-implement first approach.
> > If the set is empty (evict mode never used), the performance-
> > and memory usage impact will be very small.
> > 
> > TODO: Probably want to implement the set using an open addressing
> > hash table.
> > 
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: Somalapuram Amaranath <Amaranath.Somalapuram@amd.com>
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Cc: <dri-devel@lists.freedesktop.org>
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > ---
> >   drivers/gpu/drm/drm_exec.c | 77
> > ++++++++++++++++++++++++++++++++++----
> >   include/drm/drm_exec.h     | 15 ++++++++
> >   2 files changed, 85 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_exec.c
> > b/drivers/gpu/drm/drm_exec.c
> > index 9eea5d0d3a98..ea79d96f5439 100644
> > --- a/drivers/gpu/drm/drm_exec.c
> > +++ b/drivers/gpu/drm/drm_exec.c
> > @@ -65,6 +65,10 @@ static void drm_exec_unlock_all(struct drm_exec
> > *exec)
> >   
> >   	drm_gem_object_put(exec->prelocked);
> >   	exec->prelocked = NULL;
> > +
> > +	/* garbage collect */
> > +	xa_destroy(&exec->resv_set);
> > +	xa_init(&exec->resv_set);
> >   }
> >   
> >   /**
> > @@ -92,6 +96,8 @@ void drm_exec_init(struct drm_exec *exec, u32
> > flags, unsigned nr)
> >   	exec->contended = DRM_EXEC_DUMMY;
> >   	exec->prelocked = NULL;
> >   	exec->snap = NULL;
> > +	exec->drop_contended = false;
> > +	xa_init(&exec->resv_set);
> >   }
> >   EXPORT_SYMBOL(drm_exec_init);
> >   
> > @@ -110,6 +116,7 @@ void drm_exec_fini(struct drm_exec *exec)
> >   		drm_gem_object_put(exec->contended);
> >   		ww_acquire_fini(&exec->ticket);
> >   	}
> > +	xa_destroy(&exec->resv_set);
> >   }
> >   EXPORT_SYMBOL(drm_exec_fini);
> >   
> > @@ -139,6 +146,30 @@ bool drm_exec_cleanup(struct drm_exec *exec)
> >   }
> >   EXPORT_SYMBOL(drm_exec_cleanup);
> >   
> > +static unsigned long drm_exec_resv_to_key(const struct dma_resv
> > *resv)
> > +{
> > +	return (unsigned long)resv / __alignof__(typeof(*resv));
> > +}
> > +
> > +static void
> > +drm_exec_resv_set_erase(struct drm_exec *exec, unsigned long key)
> > +{
> > +	if (xa_load(&exec->resv_set, key))
> > +		xa_erase(&exec->resv_set, key);
> > +}
> > +
> > +static bool drm_exec_in_evict_mode(struct drm_exec *exec)
> > +{
> > +	return !!exec->snap;
> > +}
> > +
> > +static void drm_exec_set_evict_mode(struct drm_exec *exec,
> > +				    struct drm_exec_snapshot
> > *snap)
> > +{
> > +	exec->snap = snap;
> > +	exec->flags &= ~DRM_EXEC_IGNORE_DUPLICATES;
> > +}
> > +
> >   /* Track the locked object in the array */
> >   static int drm_exec_obj_locked(struct drm_exec *exec,
> >   			       struct drm_gem_object *obj,
> > @@ -161,6 +192,14 @@ static int drm_exec_obj_locked(struct drm_exec
> > *exec,
> >   	drm_gem_object_get(obj);
> >   	exec->objects[exec->num_objects++] = obj;
> >   
> > +	/*
> > +	 * Errors here are not fatal, It means the object we
> > locked
> > +	 * for eviction can't be locked again. If that is
> > problematic
> > +	 * we may need to reconsider this.
> > +	 */
> > +	if (drm_exec_in_evict_mode(exec))
> > +		(void)xa_store(&exec->resv_set,
> > drm_exec_resv_to_key(obj->resv),
> > +			       obj->resv, gfp | __GFP_NOWARN);
> >   	return 0;
> >   }
> >   
> > @@ -184,6 +223,9 @@ static int drm_exec_lock_contended(struct
> > drm_exec *exec)
> >   		dma_resv_lock_slow(obj->resv, &exec->ticket);
> >   	}
> >   
> > +	if (exec->drop_contended)
> > +		goto error_unlock;
> > +
> >   	ret = drm_exec_obj_locked(exec, obj, GFP_KERNEL);
> >   	if (unlikely(ret))
> >   		goto error_unlock;
> > @@ -245,10 +287,19 @@ int drm_exec_trylock_obj(struct drm_exec
> > *exec, struct drm_gem_object *obj)
> >   	}
> >   
> >   	if (!dma_resv_trylock_ctx(obj->resv, &exec->ticket)) {
> > -		if (dma_resv_locking_ctx(obj->resv) == &exec-
> > >ticket)
> > -			return (exec->flags &
> > DRM_EXEC_IGNORE_DUPLICATES) ? 0 : -EALREADY;
> > -		else
> > +		if (dma_resv_locking_ctx(obj->resv) == &exec-
> > >ticket) {
> > +			unsigned long key =
> > drm_exec_resv_to_key(obj->resv);
> > +
> > +			if (exec->flags &
> > DRM_EXEC_IGNORE_DUPLICATES ||
> > +			    xa_load(&exec->resv_set, key)) {
> > +				if (!drm_exec_in_evict_mode(exec))
> > +					drm_exec_resv_set_erase(ex
> > ec, key);
> > +				return 0;
> > +			}
> > +			return -EALREADY;
> > +		} else {
> >   			return -EBUSY;
> > +		}
> >   	}
> >   
> >   	ret = drm_exec_obj_locked(exec, obj, GFP_ATOMIC |
> > __GFP_NOWARN);
> > @@ -288,12 +339,20 @@ int drm_exec_lock_obj(struct drm_exec *exec,
> > struct drm_gem_object *obj)
> >   	if (unlikely(ret == -EDEADLK)) {
> >   		drm_gem_object_get(obj);
> >   		exec->contended = obj;
> > +		exec->drop_contended =
> > drm_exec_in_evict_mode(exec);
> >   		return -EDEADLK;
> >   	}
> >   
> > -	if (unlikely(ret == -EALREADY) &&
> > -	    exec->flags & DRM_EXEC_IGNORE_DUPLICATES)
> > -		return 0;
> > +	if (unlikely(ret == -EALREADY)) {
> > +		unsigned long key = drm_exec_resv_to_key(obj-
> > >resv);
> > +
> > +		if (exec->flags & DRM_EXEC_IGNORE_DUPLICATES ||
> > +		    xa_load(&exec->resv_set, key)) {
> > +			if (!drm_exec_in_evict_mode(exec))
> > +				drm_exec_resv_set_erase(exec,
> > key);
> > +			return 0;
> > +		}
> > +	}
> >   
> >   	if (unlikely(ret))
> >   		return ret;
> > @@ -324,6 +383,7 @@ void drm_exec_unlock_obj(struct drm_exec *exec,
> > struct drm_gem_object *obj)
> >   
> >   	for (i = exec->num_objects; i--;) {
> >   		if (exec->objects[i] == obj) {
> > +			drm_exec_resv_set_erase(exec,
> > drm_exec_resv_to_key(obj->resv));
> >   			dma_resv_unlock(obj->resv);
> >   			for (++i; i < exec->num_objects; ++i)
> >   				exec->objects[i - 1] = exec-
> > >objects[i];
> > @@ -415,12 +475,14 @@ void drm_exec_restore(struct drm_exec *exec,
> > struct drm_exec_snapshot *snap)
> >   		if (index + 1 == snap->num_locked)
> >   			break;
> >   
> > +		xa_erase(&exec->resv_set,
> > drm_exec_resv_to_key(obj->resv));
> >   		dma_resv_unlock(obj->resv);
> >   		drm_gem_object_put(obj);
> >   		exec->objects[index] = NULL;
> >   	}
> >   
> >   	exec->num_objects = snap->num_locked;
> > +	exec->flags = snap->flags;
> >   
> >   	if (!exec->prelocked)
> >   		exec->prelocked = snap->prelocked;
> > @@ -443,8 +505,9 @@ void drm_exec_snapshot(struct drm_exec *exec,
> > struct drm_exec_snapshot *snap)
> >   	snap->prelocked = exec->prelocked;
> >   	if (snap->prelocked)
> >   		drm_gem_object_get(snap->prelocked);
> > +	snap->flags = exec->flags;
> >   	snap->saved_snap = exec->snap;
> > -	exec->snap = snap;
> > +	drm_exec_set_evict_mode(exec, snap);
> >   }
> >   EXPORT_SYMBOL(drm_exec_snapshot);
> >   
> > diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> > index 0ce4d749511b..0b6d5ac0c092 100644
> > --- a/include/drm/drm_exec.h
> > +++ b/include/drm/drm_exec.h
> > @@ -5,6 +5,7 @@
> >   
> >   #include <linux/compiler.h>
> >   #include <linux/ww_mutex.h>
> > +#include <linux/xarray.h>
> >   
> >   #define DRM_EXEC_INTERRUPTIBLE_WAIT	BIT(0)
> >   #define DRM_EXEC_IGNORE_DUPLICATES	BIT(1)
> > @@ -53,6 +54,17 @@ struct drm_exec {
> >   	 * @snap: Pointer to the last snapshot taken or NULL if
> > none.
> >   	 */
> >   	struct drm_exec_snapshot *snap;
> > +
> > +	/**
> > +	 * @resv_set: Set of pointers to locked objects in evict
> > mode.
> > +	 */
> > +	struct xarray resv_set;
> > +
> > +	/**
> > +	 * @drop_contended: Drop the contended object after WW
> > transaction
> > +	 * relaxation.
> > +	 */
> > +	bool drop_contended;
> >   };
> >   
> >   /**
> > @@ -67,6 +79,9 @@ struct drm_exec_snapshot {
> >   
> >   	/** @num_locked: Number of locked objects at snapshot
> > time. */
> >   	unsigned long num_locked;
> > +
> > +	/** @flags: The drm_exec flags at snapshot time. */
> > +	u32 flags;
> >   };
> >   
> >   int drm_exec_handle_contended(struct drm_exec *exec);
>