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

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

On 7/13/26 14:19, Thomas Hellström wrote:
> On Mon, 2026-07-13 at 14:07 +0200, Christian König wrote:
>> 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.
> 
> For fast interconnects xe wants to avoid pinning in VRAM and needs a
> more aggressive validation. If you have a bunch of processes using WW
> transactions to lock out others from VRAM allocation with one process
> not participating that wouldn't work out well.
> 
>>  
>>> 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?
> 
> See the above. The problem is that the WW transaction always starts
> with a drm_exec on the importer side and it needs to be the same
> structure that holds the contended lock on rollback.
> 
> What are the issues you are seeing?

Mhm, there had been quite some discussion on that topic in the past and so far the result was usually that this shouldn't be necessary.

On the other hand with a reference counted dma_resv object it should be technically doable and the past discussions have already been years ago with quite a bunch of different requirements.

For dma_buf_map() I still don't see the use case, cause that function is really "give me access to the current location of the buffer" it should not move the buffer. If the exporter needs the buffer in a different location later on it can still do so.

But for dma_buf_pin() it might be perfectly valid to kick out other buffers and then I agree that definately needs to ww locking context.

Regards,
Christian.

> 
> /Thomas
> 
> 
>>
>> 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,