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

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

On Mon, 2026-07-13 at 15:14 +0200, Christian König wrote:
> 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.

Yeah, IIRC the main argument was that for various reasons neither the
WW mutex nor the dma_resv had a refcount, so either one had to use an
ops (like my rfc) or attach a refcount to the drm_resv or ww_mutex,
like the series under discussion.

> 
> 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.

OK, so xe has always seen the dma_buf_map() as the dma-buf equivalent
of ttm_bo_validate() + dma_mapping. The main reason for that is the
pairing with what was previously known as move_notify. If the map was
invalidated, you'd release it and when you needed it again, you called
dma_buf_map(), and if p2p was requested, the exporter moved the dma_buf
to vram if all attachments supported it.

Pinning would create the awkwardness of having to pin and then
immediately unpin again.

But regardless of what dma_buf interface we use for this, or perhaps
agree on a new interface, the functionality will be needed, and since
with refcounted dma_resv, this is more of a move than a
reimplementation, I think we should do it now or agree to do it as a
follow-up.

Thanks,
Thomas



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