[PATCH 3/5] drm/exec: provide trylock interface for eviction

[Thomas Hellström]

On Wed, 2024-07-03 at 15:26 +0200, Christian König wrote:
> The TTM eviction path has some additional requirements which make it
> necessary to trylock an object and then eventually keep or drop the
> lock
> again.
> 
> Signed-off-by: Christian König <christian.koenig at amd.com>
> ---
>  drivers/gpu/drm/drm_exec.c | 77
> ++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_exec.h     |  5 +++
>  2 files changed, 82 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
> index 220df336fbd9..b81bf5a92d97 100644
> --- a/drivers/gpu/drm/drm_exec.c
> +++ b/drivers/gpu/drm/drm_exec.c
> @@ -336,5 +336,82 @@ int drm_exec_prepare_array(struct drm_exec
> *exec,
>  }
>  EXPORT_SYMBOL(drm_exec_prepare_array);
>  
> +/**
> + * drm_exec_trylock_obj - try to lock a GEM object
> + * @exec: the drm_exec object with the state
> + * @obj: the GEM object to trylock
> + *
> + * Try to lock a GEM object but don't grab a reference yet.
> + *
> + * Since we can't handle contention here it's illegal to trylock the
> first
> + * object.
> + *
> + * This function is suposed to be used from atomic context and we
> don't
> + * know if the GEM object will actually be used or not. So we don't
> grab a
> + * reference yet.

With the pending LRU walker the *need* for atomic context here is gone.

> + *
> + * Returns: True if the object could be locked, false otherwise.
> + */
> +bool drm_exec_trylock_obj(struct drm_exec *exec, struct
> drm_gem_object *obj)
> +{
> +	if (WARN_ON(!exec->num_objects))
> +		return false;

I think we were in the middle of the discussion here about how to
handle this. IIRC the last suggestion was to 

if (exec->contended)
     return false;

and provide a 

drm_exec_sanitize_for_trylock() function that could be used to pre-lock
the contended lock (and perhaps provide any needed memory to register
so that a lock in atomic context could be made)

The use-case I'm worried about moving forward is, again, bo creation
where I think a push out of the validation will make the conversion of
drm_exec buffer object creation in drivers much more complicated than
it already is. Or perhaps I'm misunderstanding how that was supposed to
work.

/Thomas


> +
> +	if (exec->prelocked == obj)
> +		return true;
> +
> +	return dma_resv_trylock_ctx(obj->resv, &exec->ticket);
> +}
> +EXPORT_SYMBOL(drm_exec_trylock_obj);
> +
> +/**
> + * drm_exec_keep_trylocked_obj - keep the trylocked obj
> + * @exec: the drm_exec object with the state
> + * @obj: the GEM object to trylock
> + *
> + * Keep a trylocked object in the drm_exec state object. Grabs a
> reference to
> + * the object and adds it to the container of locked objects.
> + */

So these could be dropped.


> +int drm_exec_keep_trylocked_obj(struct drm_exec *exec,
> +				struct drm_gem_object *obj)
> +{
> +	int ret;
> +
> +	ret = drm_exec_obj_locked(exec, obj);
> +	if (ret) {
> +		dma_resv_unlock(obj->resv);
> +		return ret;
> +	}
> +
> +	if (exec->prelocked == obj) {
> +		drm_gem_object_put(exec->prelocked);
> +		exec->prelocked = NULL;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_exec_keep_trylocked_obj);
> +
> +/**
> + * drm_exec_drop_trylocked_obj - drop the trylocked obj
> + * @exec: the drm_exec object with the state
> + * @obj: the GEM object to trylock
> + *
> + * Used to drop a trylocked object in the drm_exec state object,
> drop the
> + * reservation lock again and cleanup all references.
> + */
> +void drm_exec_drop_trylocked_obj(struct drm_exec *exec,
> +				 struct drm_gem_object *obj)
> +{
> +	/*
> +	 * We can't drop the reference of prelocked objects since we
> might still
> +	 * be in atomic context. Additionally it makes sense to keep
> the
> +	 * prelocked object around since we might need it again
> later on.
> +	 */
> +	if (exec->prelocked != obj)
> +		dma_resv_unlock(obj->resv);
> +}
> +EXPORT_SYMBOL(drm_exec_drop_trylocked_obj);
> +
>  MODULE_DESCRIPTION("DRM execution context");
>  MODULE_LICENSE("Dual MIT/GPL");
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> index aa786b828a0a..a3943057a3e8 100644
> --- a/include/drm/drm_exec.h
> +++ b/include/drm/drm_exec.h
> @@ -146,5 +146,10 @@ int drm_exec_prepare_array(struct drm_exec
> *exec,
>  			   struct drm_gem_object **objects,
>  			   unsigned int num_objects,
>  			   unsigned int num_fences);
> +bool drm_exec_trylock_obj(struct drm_exec *exec, struct
> drm_gem_object *obj);
> +int drm_exec_keep_trylocked_obj(struct drm_exec *exec,
> +				struct drm_gem_object *obj);
> +void drm_exec_drop_trylocked_obj(struct drm_exec *exec,
> +				struct drm_gem_object *obj);
>  
>  #endif