Re: [PATCH 10/12] drm/exec: add drm_exec_lock_resv function

["Christian König" <christian.koenig@amd.com>]

On 7/13/26 13:57, Thomas Hellström wrote:
> On Fri, 2026-07-10 at 20:52 +0200, Christian König wrote:
>> Restructure the drm_exec object to work with dma_resv references
>> instead
>> of GEM object references.
>>
>> Add the new function dma_exec_lock_resv() to lock individual dma_resv
>> objects and so allow higher level implementations to handle
>> contention
>> purely on dma_resv objects.
>>
>> WIP! Don't commit like that!
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
> 
> Here, IMO we should move a dma-resv based implementation to dma-buf to
> facilitate passing it also through dma_buf_map(),

I still don't see why that would be necessary?

dma_buf_map() just maps the current location of the buffer, it has no requirement to force the buffer into VRAM.

At least on amdgpu we always validate buffer during dma_buf_map() with VRAM|GTT, so we never cause any eviction at all.
 
> And if wanting to avoid rewriting all users of drm_exec, Make drm_exec
> a thin wrapper on top.

DMA-buf looks like the wrong place for this since it only works on exported buffers and that should be the absolute minority.

We could have a dma-resv contention tracking helper, but I still don't see for what that would be good for?

Regards,
Christian.

> 
> Thanks,
> Thomas
> 
> 
> 
>> ---
>>  drivers/gpu/drm/drm_exec.c | 75 ++++++++++++++++++++++--------------
>> --
>>  drivers/gpu/drm/drm_gem.c  |  2 +
>>  include/drm/drm_exec.h     |  9 +++--
>>  3 files changed, 50 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
>> index fa923852fae45..382bf7bcd5ff3 100644
>> --- a/drivers/gpu/drm/drm_exec.c
>> +++ b/drivers/gpu/drm/drm_exec.c
>> @@ -58,8 +58,11 @@ static void drm_exec_unlock_all(struct drm_exec
>> *exec)
>>  		drm_gem_object_put(obj);
>>  	}
>>  
>> -	drm_gem_object_put(exec->prelocked);
>> -	exec->prelocked = NULL;
>> +	if (exec->prelocked) {
>> +		dma_resv_unlock(exec->prelocked);
>> +		dma_resv_put(exec->prelocked);
>> +		exec->prelocked = NULL;
>> +	}
>>  }
>>  
>>  /**
>> @@ -101,7 +104,7 @@ void drm_exec_fini(struct drm_exec *exec)
>>  	drm_exec_unlock_all(exec);
>>  	kvfree(exec->objects);
>>  	if (exec->contended != DRM_EXEC_DUMMY) {
>> -		drm_gem_object_put(exec->contended);
>> +		dma_resv_put(exec->contended);
>>  		ww_acquire_fini(&exec->ticket);
>>  	}
>>  }
>> @@ -158,50 +161,41 @@ static int drm_exec_obj_locked(struct drm_exec
>> *exec,
>>  /* Make sure the contended object is locked first */
>>  static int drm_exec_lock_contended(struct drm_exec *exec)
>>  {
>> -	struct drm_gem_object *obj = exec->contended;
>> +	struct dma_resv *resv = exec->contended;
>>  	int ret;
>>  
>> -	if (likely(!obj))
>> +	if (likely(!resv))
>>  		return 0;
>>  
>>  	/* Always cleanup the contention so that error handling can
>> kick in */
>>  	exec->contended = NULL;
>>  	if (exec->flags & DRM_EXEC_INTERRUPTIBLE_WAIT) {
>> -		ret = dma_resv_lock_slow_interruptible(obj->resv,
>> -						       &exec-
>>> ticket);
>> +		ret = dma_resv_lock_slow_interruptible(resv, &exec-
>>> ticket);
>>  		if (unlikely(ret))
>>  			goto error_dropref;
>>  	} else {
>> -		dma_resv_lock_slow(obj->resv, &exec->ticket);
>> +		dma_resv_lock_slow(resv, &exec->ticket);
>>  	}
>>  
>> -	ret = drm_exec_obj_locked(exec, obj);
>> -	if (unlikely(ret))
>> -		goto error_unlock;
>> -
>> -	exec->prelocked = obj;
>> +	exec->prelocked = resv;
>>  	return 0;
>>  
>> -error_unlock:
>> -	dma_resv_unlock(obj->resv);
>> -
>>  error_dropref:
>> -	drm_gem_object_put(obj);
>> +	dma_resv_put(resv);
>>  	return ret;
>>  }
>>  
>>  /**
>> - * drm_exec_lock_obj - lock a GEM object for use
>> + * drm_exec_lock_resv - lock a dma_resv object
>>   * @exec: the drm_exec object with the state
>> - * @obj: the GEM object to lock
>> + * @resv: the dma_resv object to lock
>>   *
>> - * Lock a GEM object for use and grab a reference to it.
>> + * Lock a dma_resv object for use or grab a reference to it on
>> contention.
>>   *
>>   * Returns: -EDEADLK if a contention is detected, -EALREADY when
>> object is
>> - * already locked (can be suppressed by setting the
>> DRM_EXEC_IGNORE_DUPLICATES
>> - * flag), -ENOMEM when memory allocation failed and zero for
>> success.
>> + * already locked, -ENOMEM when memory allocation failed and zero
>> for success.
>>   */
>> -int drm_exec_lock_obj(struct drm_exec *exec, struct drm_gem_object
>> *obj)
>> +int drm_exec_lock_resv(struct drm_exec *exec, struct dma_resv *resv)
>>  {
>>  	int ret;
>>  
>> @@ -209,22 +203,39 @@ int drm_exec_lock_obj(struct drm_exec *exec,
>> struct drm_gem_object *obj)
>>  	if (unlikely(ret))
>>  		return ret;
>>  
>> -	if (exec->prelocked == obj) {
>> -		drm_gem_object_put(exec->prelocked);
>> +	if (exec->prelocked == resv) {
>> +		dma_resv_put(exec->prelocked);
>>  		exec->prelocked = NULL;
>>  		return 0;
>>  	}
>>  
>>  	if (exec->flags & DRM_EXEC_INTERRUPTIBLE_WAIT)
>> -		ret = dma_resv_lock_interruptible(obj->resv, &exec-
>>> ticket);
>> +		ret = dma_resv_lock_interruptible(resv, &exec-
>>> ticket);
>>  	else
>> -		ret = dma_resv_lock(obj->resv, &exec->ticket);
>> +		ret = dma_resv_lock(resv, &exec->ticket);
>>  
>> -	if (unlikely(ret == -EDEADLK)) {
>> -		drm_gem_object_get(obj);
>> -		exec->contended = obj;
>> -		return -EDEADLK;
>> -	}
>> +	if (unlikely(ret == -EDEADLK))
>> +		exec->contended = dma_resv_get(resv);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(drm_exec_lock_resv);
>> +
>> +/**
>> + * drm_exec_lock_obj - lock a GEM object for use
>> + * @exec: the drm_exec object with the state
>> + * @obj: the GEM object to lock
>> + *
>> + * Lock a GEM object for use and grab a reference to it.
>> + *
>> + * Returns: -EDEADLK if a contention is detected, -EALREADY when
>> object is
>> + * already locked (can be suppressed by setting the
>> DRM_EXEC_IGNORE_DUPLICATES
>> + * flag), -ENOMEM when memory allocation failed and zero for
>> success.
>> + */
>> +int drm_exec_lock_obj(struct drm_exec *exec, struct drm_gem_object
>> *obj)
>> +{
>> +	int ret;
>> +
>> +	ret = drm_exec_lock_resv(exec, obj->resv);
>>  
>>  	if (unlikely(ret == -EALREADY) &&
>>  	    exec->flags & DRM_EXEC_IGNORE_DUPLICATES)
>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> index bbcbd25f014f0..f5cf9ad596a67 100644
>> --- a/drivers/gpu/drm/drm_gem.c
>> +++ b/drivers/gpu/drm/drm_gem.c
>> @@ -229,6 +229,8 @@ void drm_gem_private_object_init(struct
>> drm_device *dev,
>>  	obj->size = size;
>>  	mutex_init(&obj->gpuva.lock);
>>  	dma_resv_init(&obj->_resv);
>> +
>> +	/* TODO: This needs to go away for drm_exec to work
>> correctly!!! */
>>  	if (!obj->resv)
>>  		obj->resv = dma_resv_get(&obj->_resv);
>>  
>> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
>> index 8725ba92ff916..9daedb676d7b1 100644
>> --- a/include/drm/drm_exec.h
>> +++ b/include/drm/drm_exec.h
>> @@ -47,14 +47,14 @@ struct drm_exec {
>>  	struct drm_gem_object	**objects;
>>  
>>  	/**
>> -	 * @contended: contended GEM object we backed off for
>> +	 * @contended: contended dma_resv object we backed off for
>>  	 */
>> -	struct drm_gem_object	*contended;
>> +	struct dma_resv		*contended;
>>  
>>  	/**
>> -	 * @prelocked: already locked GEM object due to contention
>> +	 * @prelocked: already locked dma_resv object due to
>> contention
>>  	 */
>> -	struct drm_gem_object *prelocked;
>> +	struct dma_resv		*prelocked;
>>  };
>>  
>>  /**
>> @@ -175,6 +175,7 @@ static inline struct ww_acquire_ctx
>> *drm_exec_ticket(struct drm_exec *exec)
>>  void drm_exec_init(struct drm_exec *exec, u32 flags, unsigned nr);
>>  void drm_exec_fini(struct drm_exec *exec);
>>  bool drm_exec_cleanup(struct drm_exec *exec);
>> +int drm_exec_lock_resv(struct drm_exec *exec, struct dma_resv
>> *resv);
>>  int drm_exec_lock_obj(struct drm_exec *exec, struct drm_gem_object
>> *obj);
>>  void drm_exec_unlock_obj(struct drm_exec *exec, struct
>> drm_gem_object *obj);
>>  int drm_exec_prepare_obj(struct drm_exec *exec, struct
>> drm_gem_object *obj,