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

[Natalie Vock]

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.v...@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);