Re: [PATCH 01/10] drm/exec: Add helper to bypass IGNORE_DUPLICATES flag

[Natalie Vock <nat@pixelcluster.dev>]

On 7/7/26 14:54, Christian König wrote:
> On 7/7/26 14:41, Thomas Hellström wrote:
>> On Tue, 2026-07-07 at 14:28 +0200, Christian König wrote:
>>> On 7/7/26 14:09, Thomas Hellström wrote:
>>>> On Fri, 2026-07-03 at 18:31 +0200, Natalie Vock wrote:
>>>>> TTM is about to switch to drm_exec for locking objects
>>>>> in the LRU list. When we're done processing the object, we want
>>>>> to
>>>>> unlock it only if the caller doesn't already hold that lock. If
>>>>> DRM_EXEC_IGNORE_DUPLICATES is set on the exec object (which
>>>>> callers
>>>>> may
>>>>> require for unrelated reasons), we have no way of knowing whether
>>>>> the
>>>>> lock is already held.
>>>>>
>>>>> To remedy this, add a separate helper that forcefully bypasses
>>>>> the
>>>>> IGNORE_DUPLICATES flag for only a single locking operation.
>>>>>
>>>>> Signed-off-by: Natalie Vock <natalie.vock@gmx.de>
>>>>
>>>> The first, more complete attempt to try to tackle the exhaustive
>>>> eviction introduced a drm_exec snapshot ability instead.
>>>>
>>>> https://lists.freedesktop.org/archives/intel-xe/2024-May/035820.html
>>>>
>>>> The idea was that one would want to incrementally lock more buffer
>>>> objects until the validation succeeded, rather than dropping each
>>>> single lock after processing the eviction. That's actually what
>>>> guarantees forward progress. Restoring the snapshot unlocked all
>>>> locks
>>>> that we grabbed in the process, and would work also for single
>>>> locks.
>>>
>>> That's actually not what this patch here tries to solve.
>>>
>>> The problem is rather since we don't remove the BOs from the LRU list
>>> that we try to evict some which are actually part of our working set.
>>>
>>> So when we lock for eviction we can't ignore duplicates no matter
>>> what the global flag says.
>>
>> Ah yes, I see that now when I've gotten to patch 9. But then I think
>> the commit message is a bit misleading. It talks about unlocking an
>> already processed object?
> 
> Oh, good point! Yeah the problem isn't the unlocking but that we evict an object from the working set.
> 
> The commit message clearly needs to be fixed.

The problem is actually both. Evicting an object from the working set 
definitely is potentially unsafe, but I considered that a minor thing. 
Unlocking the already processed objects is a much, much bigger problem IMO.

The worst breakage is for AMDGPU's per-VM buffers whose resv is just the 
VM's own reservation object: If we encounter another per-VM buffer from 
our own VM on the LRU, we may or may not decide to evict it, but in any 
case we leave the VM completely unlocked for the remainder of the 
submission, including in pagetable updates and all that, which more or 
less completely smashes the state of the entire VM if multiple PT update 
operations end up racing with each other.

This was the breakage I observed myself, so that's what I described in 
the commit message. In any case, I can also change the commit message to 
primarily point at self-eviction and/or expand my explanation on why 
unlocking causes issues.

Best,
Natalie

> 
> Christian.
> 
>>
>> Thanks,
>> Thomas
>>
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Thanks,
>>>> Thomas
>>>>
>>>>
>>>>> ---
>>>>>   drivers/gpu/drm/drm_exec.c | 52
>>>>> ++++++++++++++++++++++++++++++++++--
>>>>> ----------
>>>>>   include/drm/drm_exec.h     |  2 ++
>>>>>   2 files changed, 41 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_exec.c
>>>>> b/drivers/gpu/drm/drm_exec.c
>>>>> index 7988f5e7d56a3..91de6b4d29df8 100644
>>>>> --- a/drivers/gpu/drm/drm_exec.c
>>>>> +++ b/drivers/gpu/drm/drm_exec.c
>>>>> @@ -190,18 +190,9 @@ static int drm_exec_lock_contended(struct
>>>>> drm_exec *exec)
>>>>>   	return ret;
>>>>>   }
>>>>>   
>>>>> -/**
>>>>> - * 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)
>>>>> +static int __drm_exec_lock_obj(struct drm_exec *exec,
>>>>> +			       struct drm_gem_object *obj,
>>>>> +			       bool always_report_duplicates)
>>>>>   {
>>>>>   	int ret;
>>>>>   
>>>>> @@ -226,7 +217,7 @@ int drm_exec_lock_obj(struct drm_exec *exec,
>>>>> struct drm_gem_object *obj)
>>>>>   		return -EDEADLK;
>>>>>   	}
>>>>>   
>>>>> -	if (unlikely(ret == -EALREADY) &&
>>>>> +	if (unlikely(ret == -EALREADY) &&
>>>>> !always_report_duplicates
>>>>> &&
>>>>>   	    exec->flags & DRM_EXEC_IGNORE_DUPLICATES)
>>>>>   		return 0;
>>>>>   
>>>>> @@ -243,8 +234,43 @@ int drm_exec_lock_obj(struct drm_exec *exec,
>>>>> struct drm_gem_object *obj)
>>>>>   	dma_resv_unlock(obj->resv);
>>>>>   	return ret;
>>>>>   }
>>>>> +
>>>>> +/**
>>>>> + * 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)
>>>>> +{
>>>>> +	return __drm_exec_lock_obj(exec, obj, false);
>>>>> +}
>>>>>   EXPORT_SYMBOL(drm_exec_lock_obj);
>>>>>   
>>>>> +/**
>>>>> + * drm_exec_lock_obj_report_dup - lock a GEM object for use, but
>>>>> always report duplicates
>>>>> + * @exec: the drm_exec object with the state
>>>>> + * @obj: the GEM object to lock
>>>>> + *
>>>>> + * Like drm_exec_lock_obj, lock a GEM object for use and grab a
>>>>> reference to it.
>>>>> + * Unlike drm_exec_lock_obj, DRM_EXEC_IGNORE_DUPLICATES is
>>>>> ignored
>>>>> and duplicates are
>>>>> + * always reported.
>>>>> + *
>>>>> + * Returns: -EDEADLK if a contention is detected, -EALREADY when
>>>>> object is
>>>>> + * already locked, -ENOMEM when memory allocation failed and
>>>>> zero
>>>>> for success.
>>>>> + */
>>>>> +int drm_exec_lock_obj_report_dup(struct drm_exec *exec,
>>>>> +				 struct drm_gem_object *obj)
>>>>> +{
>>>>> +	return __drm_exec_lock_obj(exec, obj, false);
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_exec_lock_obj_report_dup);
>>>>> +
>>>>>   /**
>>>>>    * drm_exec_unlock_obj - unlock a GEM object in this exec
>>>>> context
>>>>>    * @exec: the drm_exec object with the state
>>>>> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
>>>>> index 8725ba92ff916..ff80dd2b72240 100644
>>>>> --- a/include/drm/drm_exec.h
>>>>> +++ b/include/drm/drm_exec.h
>>>>> @@ -176,6 +176,8 @@ 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_obj(struct drm_exec *exec, struct
>>>>> drm_gem_object
>>>>> *obj);
>>>>> +int drm_exec_lock_obj_report_dup(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,
>>>>>   			 unsigned int num_fences);
>