[PATCH v3 0/8] drm/gpuvm: two pass locking for exec

[PATCH v3 0/8] drm/gpuvm: two pass locking for exec

[Matthew Brost]

Two processes share a set of buffers, and each has buffers of its own
which the other never sees. Say process A has mapped

  S        the shared buffers, also mapped by B
  P        buffers private to A

and B has mapped S plus private buffers of its own. The overlap is
exactly S, and the work each process wants to do on its own buffers is
independent of the other.

A submits. Its exec locks the dma-resv of everything it has mapped, S
and P both, then finds something in P has been evicted and migrates it
back in. B submits, and blocks on S for as long as that migration takes,
even though the migration is of a buffer belonging to A which B has
never seen.

So the stall does not come from the overlapping set. The buffers in S
are resident, and neither exec has anything to do to them beyond
attaching a fence. They are held only because an exec locks everything
it has mapped in one go, and they stay held until the slowest unrelated
thing in that transaction is done.

Which buffers get evicted is a separate matter, and one which already
has answers: eviction heuristics which leave shared buffers alone, or
one process' allocations outranking another's. This is what is left once
those work.

Where this tends to show up is compositors and presentation, which is
also where userspace has worked hardest to avoid it. Wayland explicit
sync exists so that a compositor is not latched onto its clients'
rendering, waiting on fences it never asked for. The locking above
reintroduces that coupling anyway, in the kernel, and does it under
memory pressure, which is where a missed frame is least welcome and the
cause is hardest to see.

The fix is to stop coupling "lock the VM" to "validate it". Instead of
locking everything and then validating, lock the private buffers and the
evicted external ones, validate those, and only then lock the rest, all
within the same drm_exec transaction.

Patch 1 lets a driver split the locking of an exec that way, patches 2
and 3 use it in Xe and Panthor, whose panthor_vm_bo_validate() swaps
pages back in under those same shared locks. It is opt-in, and drivers
which do not ask for it are unaffected.

Patches 5 and 6 do the same for MSM. Its VM_BIND submit path locks every
BO mapped in the VM and then validates the evicted ones, which means
getting their pages and mapping them again, with the shared BOs locked
throughout. Patch 4 switches VM_BIND VMs to DRM_GPUVM_RESV_PROTECTED,
which two pass locking requires, and patch 5 splits the submit's locking
into the two passes. Kernel managed VMs and the legacy submit path are
left alone.

The MSM part should resolve exactly the problem Anna Maniscalco
presented at XDC, render jobs stalling the compositor on shared buffers,
which is currently worked around with a downstream hack in MSM for
SteamOS [1][2] (the talk starts at 5:30).

Nouveau support for DRM_GPUVM_RESV_PROTECTED and two pass locking added
in patches 7 and 8.

All driver updated side from Xe untested.

[1] https://indico.freedesktop.org/event/12/contributions/627/
[2] https://www.youtube.com/watch?v=j5W5ErEMnvM&t=330s

Matt

Cc: Abhinav Kumar <abhinav.kumar@linux.dev>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Anna Maniscalco <anna.maniscalco2000@gmail.com>
Cc: Antonino Maniscalco <antomani103@gmail.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Dmitry Baryshkov <lumag@kernel.org>
Cc: Jessica Zhang <jesszhan0024@gmail.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Marijn Suijten <marijn.suijten@somainline.org>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Rob Clark <robin.clark@oss.qualcomm.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Steven Price <steven.price@arm.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Assisted-by: LLM

---
v3:
 - Address Danilo's comments of GPUVM patch
 - Update MSM to reject submit_bo on VM_BIND context (Sashiko)
 - Also update Nouevau

Matthew Brost (8):
  drm/gpuvm: allow locking external objects in two passes
  drm/xe: lock the resident BOs of an exec last
  drm/panthor: lock the resident BOs of a submit last
  drm/msm: reject a submit_bo table on VM_BIND contexts
  drm/msm: use DRM_GPUVM_RESV_PROTECTED for VM_BIND VMs
  drm/msm: lock the resident BOs of a VM_BIND submit last
  drm/nouveau: use DRM_GPUVM_RESV_PROTECTED
  drm/nouveau: lock the resident BOs of an exec last

 Documentation/gpu/drm-mm.rst           |   6 +
 drivers/gpu/drm/drm_gpuvm.c            | 505 ++++++++++++++++++++++++-
 drivers/gpu/drm/msm/msm_gem_submit.c   |  90 ++++-
 drivers/gpu/drm/msm/msm_gem_vma.c      |  16 +-
 drivers/gpu/drm/nouveau/nouveau_exec.c |  27 +-
 drivers/gpu/drm/nouveau/nouveau_uvmm.c |  49 ++-
 drivers/gpu/drm/panthor/panthor_mmu.c  |  55 ++-
 drivers/gpu/drm/xe/xe_exec.c           |  23 +-
 drivers/gpu/drm/xe/xe_vm.c             |  43 ++-
 drivers/gpu/drm/xe/xe_vm.h             |   3 +-
 include/drm/drm_gpuvm.h                | 139 ++++++-
 11 files changed, 882 insertions(+), 74 deletions(-)

-- 
2.34.1

[PATCH v3 1/8] drm/gpuvm: allow locking external objects in two passes

[Matthew Brost]

Today a driver locks every drm_gem_object its drm_gpuvm has mappings of
in a single drm_exec transaction, validates and rebinds whatever needs
it, submits and unlocks. The dma-resv lock of every mapped object is
therefore held for as long as the slowest validation in the transaction
takes.

That is fine as long as the objects are private, but it is not fine for
objects shared between processes. The processes are of course related,
that is why they share a buffer, but the work which holds the lock is
not: a client stalls the compositor it is presenting to while it faults
in or migrates some large buffer of its own, one the compositor has no
interest in and will never touch. The deadline is missed because of a
non overlapping set of objects.

Keeping the shared, about to be presented buffers resident is a
different problem, and one which already has its own answers: driver
side eviction heuristics which decline to evict shared objects, or a
compositor whose allocations simply outrank everyone else's. This is
what is left even once those work. The objects being held hostage are
not ones the transaction has anything slow to do on. Only the evicted
objects need validating; the rest are already resident, and holding
their dma-resv while some other object is migrated buys nothing. They
are worked on in the end, of course, having a fence attached and their
mappings rebound, but none of that has to wait on a migration.

So let a driver set drm_gpuvm_exec::two_pass and have
drm_gpuvm_exec_lock() acquire its locks in two steps:

  DRM_GPUVM_EXEC_PASS_EARLY validates what the transaction already
  holds. The GPUVM's own dma-resv is locked from the start, so that is
  every private object, and the driver validates the evicted ones. The
  pass also opportunistically locks the external objects which are
  evicted, since those need validating anyway, and the driver validates
  those too. The resident external objects are left unlocked, then

  DRM_GPUVM_EXEC_PASS_LATE locks everything else, i.e. exactly what the
  early pass left out. The driver validates anything which raced with
  the early pass and does whatever needs every lock held, such as
  attaching its job's fence.

Both passes share one drm_exec transaction. The early pass keeps
everything it locked and the late pass only ever adds to it, so there is
no window in which another thread can undo the early pass' work, and no
recheck or retry logic is needed. The extra.fn callback is invoked once
per pass, with drm_gpuvm_exec::pass telling it which one it is in.

Only the external objects are divided up like this. The passes have to
agree on which objects belong to which, and the GPUVM's common dma-resv
is what gives that, so it is held throughout.

Since the early pass leaves the objects it did not lock alone, they are
the late pass' problem, and drm_gpuvm_exec_pass_validate() must skip
them or it would validate an object the transaction does not hold the
dma-resv of. Such an object can be sitting on the evicted list while
that happens, which is why drm_gpuvm_exec_pass_has_evicted() exists: a
driver looping until nothing is evicted would otherwise spin in the
early pass on an object that pass is never going to validate. It keys
off drm_gpuvm_bo::lock_skipped, which the early pass latches, rather
than off drm_gpuvm_bo::evicted, which can change at any time. The late
pass consumes that same latched value to derive what to prepare, which
is what keeps the two passes an exact partition even if an object is
evicted in between; re-preparing an object the transaction already holds
would fail with -EALREADY.

The early pass reads drm_gpuvm_bo::evicted without holding the object's
dma-resv, that being the lock it is trying not to take. The race is
benign: an object evicted just after being skipped is validated by the
late pass instead, exactly as if it had been evicted a moment later
still.

Two pass locking requires a DRM_GPUVM_RESV_PROTECTED drm_gpuvm. The late
pass has to prepare precisely the complement of what the early pass
locked, and only the GPUVM's common dma-resv, which the transaction
holds across both passes, keeps the external object list from changing
underneath.

Splitting the locking only pays off when there is validation to keep the
resident objects unlocked for. With nothing evicted it would just walk
the external object list a second time for nothing, so
drm_gpuvm_exec_pass_needs_split() is consulted once the VM resv is held
and the sequence collapses back to a single pass when it says no. That
check is advisory: both answers are functionally correct, so a stale one
only costs the optimization.

Making it O(1) needs a count of the evicted external objects, since
those are the ones drm_gpuvm_bo_evict() cannot put on the evicted list,
lacking the GPUVM's common dma-resv to do it under. The object's own
dma-resv is held there, so the transition is stable and an atomic_t is
enough. It is decremented again wherever a drm_gpuvm_bo is taken off the
lists, including the deferred cleanup path used by immediate mode.

DRM_GPUVM_EXEC_PASS_ALL is zero and two_pass defaults to false, so a
zero initialised drm_gpuvm_exec and the existing
drm_gpuvm_prepare_objects() and drm_gpuvm_validate() keep the
traditional behaviour, and no existing driver changes behaviour.

Cc: Abhinav Kumar <abhinav.kumar@linux.dev>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Anna Maniscalco <anna.maniscalco2000@gmail.com>
Cc: Antonino Maniscalco <antomani103@gmail.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Dmitry Baryshkov <lumag@kernel.org>
Cc: Jessica Zhang <jesszhan0024@gmail.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Marijn Suijten <marijn.suijten@somainline.org>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Rob Clark <robin.clark@oss.qualcomm.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Steven Price <steven.price@arm.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Assisted-by: LLM
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
---
v3:
 - Pair the lockless READ_ONCE() of drm_gpuvm_bo::evicted with a
   WRITE_ONCE() in drm_gpuvm_bo_evict() and document why it is safe
   (Danilo)
 - Rename the DOC section to "GPUVM EXEC two-pass locking" and describe
   it in terms of preparing and validating objects (Danilo)
 - Use drm_WARN_ON_ONCE() (Danilo)
 - Consolidate the DRM_GPUVM_RESV_PROTECTED checks into
   drm_gpuvm_exec_pass_supported(); drm_gpuvm_exec_lock() now warns and
   falls back to a single pass rather than returning -EOPNOTSUPP (Danilo)
 - Use the drm_gpuvm_exec_pass_ prefix for all new functions (Danilo)
---
 Documentation/gpu/drm-mm.rst |   6 +
 drivers/gpu/drm/drm_gpuvm.c  | 505 +++++++++++++++++++++++++++++++++--
 include/drm/drm_gpuvm.h      | 139 +++++++++-
 3 files changed, 629 insertions(+), 21 deletions(-)

diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
index 2dea94f77d52..7eca0966448d 100644
--- a/Documentation/gpu/drm-mm.rst
+++ b/Documentation/gpu/drm-mm.rst
@@ -510,6 +510,12 @@ Locking
 .. kernel-doc:: drivers/gpu/drm/drm_gpuvm.c
    :doc: Locking
 
+GPUVM EXEC two-pass locking
+---------------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_gpuvm.c
+   :doc: GPUVM EXEC two-pass locking
+
 Examples
 --------
 
diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
index d1c80ad3dead..6eaa4aef8093 100644
--- a/drivers/gpu/drm/drm_gpuvm.c
+++ b/drivers/gpu/drm/drm_gpuvm.c
@@ -527,6 +527,93 @@
  * hence do not require internal locking.
  */
 
+/**
+ * DOC: GPUVM EXEC two-pass locking
+ *
+ * This is about how the &drm_gem_objects a &drm_gpuvm has mappings of are
+ * prepared, i.e. have their dma-resv locked and fence slots reserved, through
+ * drm_gpuvm_exec_lock() or drm_gpuvm_prepare_objects(), and how the evicted
+ * ones among them are validated, through drm_gpuvm_validate(), within that
+ * same &drm_exec transaction.
+ *
+ * By default a driver prepares every &drm_gem_object a &drm_gpuvm has
+ * mappings of in a single &drm_exec transaction, validates and rebinds
+ * whatever needs it, submits its job and unlocks. That is simple and correct,
+ * but it means the dma-resv lock of every mapped object is held for as long
+ * as the validation of the slowest object takes.
+ *
+ * That is a problem when some of those objects are shared with other
+ * processes, for example the buffers a compositor is about to present. The
+ * processes are of course related, that is why they share a buffer, but the
+ * work holding the lock is not: a client stalls the compositor it presents
+ * to while faulting in or migrating some large buffer of its own, one the
+ * compositor will never touch.
+ *
+ * Keeping the shared buffers themselves resident is a separate problem with
+ * separate answers, such as driver side heuristics which decline to evict
+ * shared objects, or simply a compositor whose allocations outrank everyone
+ * else's. What is left even once they work is this: a resident shared object
+ * needs no validating, yet its dma-resv is held for the duration of the
+ * validation of unrelated objects.
+ *
+ * The observation which fixes that is that only the evicted objects need any
+ * work done on them. Everything else is already resident, so holding its
+ * dma-resv throughout buys nothing. A driver can therefore set
+ * &drm_gpuvm_exec.two_pass and have the single &drm_exec transaction acquire
+ * its locks in two steps:
+ *
+ * 1) %DRM_GPUVM_EXEC_PASS_EARLY validates what the transaction already
+ *    holds. The &drm_gpuvm's own dma-resv is locked from the start, so that
+ *    is every private object, and the driver validates the evicted ones from
+ *    its &drm_gpuvm_exec.extra callback. On top of that the pass
+ *    opportunistically locks the external objects which are evicted, since
+ *    those have to be validated anyway, and the callback validates them too.
+ *    The resident external objects are left unlocked.
+ *
+ * 2) %DRM_GPUVM_EXEC_PASS_LATE locks everything else, i.e. exactly what the
+ *    early pass left out. The callback runs again, now able to touch every
+ *    mapped object, and validates anything which raced with the early pass
+ *    before the driver submits.
+ *
+ * The important part is what does not happen in between: the early pass keeps
+ * everything it locked, so there is no window in which another thread can
+ * undo its work, and no recheck or retry logic is needed. The resident
+ * objects are simply locked last, once the expensive work is already done.
+ *
+ * The late pass can still find something to validate, since an object it had
+ * not locked yet may have been evicted while the early pass was running. That
+ * is handled the way it is today, by validating it with every lock held; it
+ * is just no longer the common case.
+ *
+ * Setting &drm_gpuvm_exec.two_pass only asks for two passes, it does not
+ * force them. Splitting the transaction is pointless when nothing is evicted,
+ * as the early pass would lock nothing and validate nothing, so
+ * drm_gpuvm_exec_lock() consults drm_gpuvm_exec_pass_needs_split() once the
+ * GPUVM's dma-resv is held and falls back to a single
+ * %DRM_GPUVM_EXEC_PASS_ALL pass if there is no work for an early pass to do.
+ * Drivers which drive the passes themselves rather than through
+ * drm_gpuvm_exec_lock() should do the same.
+ *
+ * That check is advisory. An object may be evicted right after it answers
+ * false, in which case the single pass validates it with every lock held,
+ * exactly as it would have without two-pass locking. Both answers are always
+ * correct; a stale one only costs the optimization.
+ *
+ * drm_gpuvm_exec_pass_validate() must be used instead of
+ * drm_gpuvm_validate(), so that an object the early pass did not lock is not
+ * validated behind its dma-resv lock's back. Such an object is simply left to
+ * the late pass, which does hold it. It can still be sitting on the evicted
+ * list while that happens, so the usual "loop until nothing is evicted"
+ * termination condition must use drm_gpuvm_exec_pass_has_evicted() rather
+ * than a plain emptiness test on the evicted list, or the early pass would
+ * spin on an object it is never going to validate.
+ *
+ * Two-pass locking requires a %DRM_GPUVM_RESV_PROTECTED &drm_gpuvm. The late
+ * pass has to prepare precisely the complement of what the early pass locked,
+ * and only the GPUVM's common dma-resv, which the transaction holds across
+ * both passes, keeps the external object list from changing underneath.
+ */
+
 /**
  * DOC: Examples
  *
@@ -1104,6 +1191,7 @@ drm_gpuvm_init(struct drm_gpuvm *gpuvm, const char *name,
 
 	INIT_LIST_HEAD(&gpuvm->extobj.list);
 	spin_lock_init(&gpuvm->extobj.lock);
+	atomic_set(&gpuvm->extobj.num_evicted, 0);
 
 	INIT_LIST_HEAD(&gpuvm->evict.list);
 	spin_lock_init(&gpuvm->evict.lock);
@@ -1220,16 +1308,105 @@ drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
 }
 EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_vm);
 
+/*
+ * Everything two-pass locking keys off is protected by the GPUVM's common
+ * dma-resv: the external object list not changing between the passes, and
+ * &drm_gpuvm_bo.lock_skipped being latched by one pass and consumed by the
+ * next. That lock is held for the whole &drm_exec transaction, so assert it
+ * wherever a pass is acted upon.
+ *
+ * %DRM_GPUVM_EXEC_PASS_ALL is exempt. It is the single pass behaviour which
+ * predates this, and !%DRM_GPUVM_RESV_PROTECTED drivers legitimately reach it
+ * without holding the common dma-resv, using the internal spinlocks instead.
+ */
+#ifdef CONFIG_LOCKDEP
+static void
+drm_gpuvm_pass_assert_held(struct drm_gpuvm *gpuvm,
+			   enum drm_gpuvm_exec_pass pass)
+{
+	if (pass != DRM_GPUVM_EXEC_PASS_ALL)
+		drm_gpuvm_resv_assert_held(gpuvm);
+}
+#else
+static void
+drm_gpuvm_pass_assert_held(struct drm_gpuvm *gpuvm,
+			   enum drm_gpuvm_exec_pass pass)
+{
+}
+#endif
+
+/*
+ * Two-pass locking requires a %DRM_GPUVM_RESV_PROTECTED &drm_gpuvm, see
+ * "GPUVM EXEC two-pass locking". This is the one place checking for it, so
+ * that it can simply go away once every &drm_gpuvm is.
+ *
+ * Returns: true if @pass can be used with @gpuvm. %DRM_GPUVM_EXEC_PASS_ALL
+ * always can.
+ */
+static bool
+drm_gpuvm_exec_pass_supported(struct drm_gpuvm *gpuvm,
+			      enum drm_gpuvm_exec_pass pass)
+{
+	if (pass == DRM_GPUVM_EXEC_PASS_ALL)
+		return true;
+
+	return !drm_WARN_ON_ONCE(gpuvm->drm, !drm_gpuvm_resv_protected(gpuvm));
+}
+
+/*
+ * Decide whether @pass prepares @vm_bo, and maintain @vm_bo->lock_skipped,
+ * which records whether the transaction is missing this object's dma-resv.
+ *
+ * The early pass latches its decision there so that
+ * drm_gpuvm_exec_pass_validate() keys off a value which cannot change under
+ * it, even though @vm_bo->evicted can. The late pass then consumes that
+ * latched value rather than re-reading @vm_bo->evicted, which is what makes
+ * it prepare exactly the objects the early pass left out, no more and no less.
+ */
+static bool
+drm_gpuvm_prepare_skip(struct drm_gpuvm_bo *vm_bo,
+		       enum drm_gpuvm_exec_pass pass)
+{
+	drm_gpuvm_pass_assert_held(vm_bo->vm, pass);
+
+	switch (pass) {
+	case DRM_GPUVM_EXEC_PASS_EARLY:
+		/*
+		 * Lockless hint, pairs with WRITE_ONCE() in drm_gpuvm_bo_evict().
+		 * A stale value is harmless: the decision is latched in lock_skipped
+		 * and anything acting on evicted re-reads it under the object's resv.
+		 */
+		vm_bo->lock_skipped = !READ_ONCE(vm_bo->evicted);
+		break;
+	case DRM_GPUVM_EXEC_PASS_LATE:
+		/* Already locked by the early pass, must not lock it twice. */
+		if (!vm_bo->lock_skipped)
+			return true;
+
+		vm_bo->lock_skipped = false;
+		break;
+	case DRM_GPUVM_EXEC_PASS_ALL:
+		vm_bo->lock_skipped = false;
+		break;
+	}
+
+	return vm_bo->lock_skipped;
+}
+
 static int
 __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
 			    struct drm_exec *exec,
-			    unsigned int num_fences)
+			    unsigned int num_fences,
+			    enum drm_gpuvm_exec_pass pass)
 {
 	struct drm_gpuvm_bo *vm_bo;
 	LIST_HEAD(extobjs);
 	int ret = 0;
 
 	for_each_vm_bo_in_list(gpuvm, extobj, &extobjs, vm_bo) {
+		if (drm_gpuvm_prepare_skip(vm_bo, pass))
+			continue;
+
 		ret = exec_prepare_obj(exec, vm_bo->obj, num_fences);
 		if (ret)
 			break;
@@ -1244,7 +1421,8 @@ __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
 static int
 drm_gpuvm_prepare_objects_locked(struct drm_gpuvm *gpuvm,
 				 struct drm_exec *exec,
-				 unsigned int num_fences)
+				 unsigned int num_fences,
+				 enum drm_gpuvm_exec_pass pass)
 {
 	struct drm_gpuvm_bo *vm_bo;
 	int ret = 0;
@@ -1254,6 +1432,9 @@ drm_gpuvm_prepare_objects_locked(struct drm_gpuvm *gpuvm,
 		if (drm_gpuvm_bo_is_zombie(vm_bo))
 			continue;
 
+		if (drm_gpuvm_prepare_skip(vm_bo, pass))
+			continue;
+
 		ret = exec_prepare_obj(exec, vm_bo->obj, num_fences);
 		if (ret)
 			break;
@@ -1293,13 +1474,54 @@ drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
 			  struct drm_exec *exec,
 			  unsigned int num_fences)
 {
+	return drm_gpuvm_exec_pass_prepare_objects(gpuvm, exec, num_fences,
+						   DRM_GPUVM_EXEC_PASS_ALL);
+}
+EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_objects);
+
+/**
+ * drm_gpuvm_exec_pass_prepare_objects() - prepare the associated BOs of a pass
+ * @gpuvm: the &drm_gpuvm
+ * @exec: the &drm_exec locking context
+ * @num_fences: the amount of &dma_fences to reserve
+ * @pass: the &enum drm_gpuvm_exec_pass to prepare for
+ *
+ * Same as drm_gpuvm_prepare_objects(), except that @pass selects which
+ * external objects are prepared. With %DRM_GPUVM_EXEC_PASS_EARLY the resident
+ * &drm_gpuvm_bos are left alone, so that their dma-resv locks are only taken
+ * by the %DRM_GPUVM_EXEC_PASS_LATE call which follows in the same &drm_exec
+ * transaction.
+ *
+ * The two passes must be used as a pair and in that order, since the late one
+ * derives what to prepare from what the early one recorded.
+ *
+ * Anything other than %DRM_GPUVM_EXEC_PASS_ALL requires a
+ * %DRM_GPUVM_RESV_PROTECTED @gpuvm, whose common dma-resv, held across both
+ * passes, is what keeps the external object list stable between them.
+ *
+ * Returns: 0 on success, negative error code on failure, -EOPNOTSUPP if @pass
+ * is not %DRM_GPUVM_EXEC_PASS_ALL and @gpuvm is not
+ * %DRM_GPUVM_RESV_PROTECTED.
+ */
+int
+drm_gpuvm_exec_pass_prepare_objects(struct drm_gpuvm *gpuvm,
+				    struct drm_exec *exec,
+				    unsigned int num_fences,
+				    enum drm_gpuvm_exec_pass pass)
+{
+	if (!drm_gpuvm_exec_pass_supported(gpuvm, pass))
+		return -EOPNOTSUPP;
+
+	drm_gpuvm_pass_assert_held(gpuvm, pass);
+
 	if (drm_gpuvm_resv_protected(gpuvm))
 		return drm_gpuvm_prepare_objects_locked(gpuvm, exec,
-							num_fences);
+							num_fences, pass);
 	else
-		return __drm_gpuvm_prepare_objects(gpuvm, exec, num_fences);
+		return __drm_gpuvm_prepare_objects(gpuvm, exec, num_fences,
+						   pass);
 }
-EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_objects);
+EXPORT_SYMBOL_GPL(drm_gpuvm_exec_pass_prepare_objects);
 
 /**
  * drm_gpuvm_prepare_range() - prepare all BOs mapped within a given range
@@ -1338,6 +1560,32 @@ drm_gpuvm_prepare_range(struct drm_gpuvm *gpuvm, struct drm_exec *exec,
 }
 EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_range);
 
+/*
+ * Prepare the external objects belonging to @pass and let the driver do its
+ * per pass work. Contention is left for the caller to act on, since only it
+ * can restart the drm_exec transaction.
+ */
+static int
+drm_gpuvm_exec_do_pass(struct drm_gpuvm_exec *vm_exec,
+		       enum drm_gpuvm_exec_pass pass)
+{
+	int ret;
+
+	drm_gpuvm_pass_assert_held(vm_exec->vm, pass);
+
+	vm_exec->pass = pass;
+
+	ret = drm_gpuvm_exec_pass_prepare_objects(vm_exec->vm, &vm_exec->exec,
+						  vm_exec->num_fences, pass);
+	if (ret)
+		return ret;
+
+	if (vm_exec->extra.fn)
+		return vm_exec->extra.fn(vm_exec);
+
+	return 0;
+}
+
 /**
  * drm_gpuvm_exec_lock() - lock all dma-resv of all associated BOs
  * @vm_exec: the &drm_gpuvm_exec wrapper
@@ -1350,6 +1598,24 @@ EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_range);
  * dma-resv in the context of the &drm_gpuvm_exec instance. Typically, drivers
  * would call drm_exec_prepare_obj() from within this callback.
  *
+ * If struct drm_gpuvm_exec::two_pass is set the locking may be split in two,
+ * see &enum drm_gpuvm_exec_pass, and @fn is called once per pass with struct
+ * drm_gpuvm_exec::pass telling it which one it is in. The split is skipped,
+ * and @fn called once with %DRM_GPUVM_EXEC_PASS_ALL, when there is nothing
+ * evicted for it to help with; see drm_gpuvm_exec_pass_needs_split(). A
+ * driver setting two_pass therefore has to handle all three passes. Such a
+ * callback has to be written with that in mind: preparing the same object in
+ * both passes fails with -EALREADY. Both passes share the one &drm_exec
+ * transaction, so the late pass only ever adds locks to what the early pass
+ * already holds. This requires a %DRM_GPUVM_RESV_PROTECTED &drm_gpuvm, whose
+ * common dma-resv keeps the external object list stable across the two
+ * passes; on any other two_pass warns and a single pass is used.
+ *
+ * Note that ww_mutex backoff can still restart the whole transaction from the
+ * early pass, in which case the contended object is locked up front and the
+ * early pass does run holding it. That is a rare fallback, not the common
+ * path it is trying to avoid.
+ *
  * Returns: 0 on success, negative error code on failure.
  */
 int
@@ -1368,17 +1634,22 @@ drm_gpuvm_exec_lock(struct drm_gpuvm_exec *vm_exec)
 		if (ret)
 			goto err;
 
-		ret = drm_gpuvm_prepare_objects(gpuvm, exec, num_fences);
-		drm_exec_retry_on_contention(exec);
-		if (ret)
-			goto err;
-
-		if (vm_exec->extra.fn) {
-			ret = vm_exec->extra.fn(vm_exec);
+		if (vm_exec->two_pass && drm_gpuvm_exec_pass_needs_split(gpuvm)) {
+			ret = drm_gpuvm_exec_do_pass(vm_exec,
+						     DRM_GPUVM_EXEC_PASS_EARLY);
 			drm_exec_retry_on_contention(exec);
 			if (ret)
 				goto err;
+
+			ret = drm_gpuvm_exec_do_pass(vm_exec,
+						     DRM_GPUVM_EXEC_PASS_LATE);
+		} else {
+			ret = drm_gpuvm_exec_do_pass(vm_exec,
+						     DRM_GPUVM_EXEC_PASS_ALL);
 		}
+		drm_exec_retry_on_contention(exec);
+		if (ret)
+			goto err;
 	}
 
 	return 0;
@@ -1410,6 +1681,12 @@ fn_lock_array(struct drm_gpuvm_exec *vm_exec)
  * Acquires all dma-resv locks of all &drm_gem_objects the given &drm_gpuvm
  * contains mappings of, plus the ones given through @objs.
  *
+ * Two-pass locking is not supported here: @objs are not tracked by the
+ * &drm_gpuvm, so there is no way to tell which pass each of them belongs in.
+ * A driver wanting both has to open code this using
+ * drm_gpuvm_exec_lock() and a &drm_gpuvm_exec.extra callback which keys off
+ * &drm_gpuvm_exec.pass.
+ *
  * Returns: 0 on success, negative error code on failure.
  */
 int
@@ -1422,6 +1699,9 @@ drm_gpuvm_exec_lock_array(struct drm_gpuvm_exec *vm_exec,
 		unsigned int num_objs;
 	} args;
 
+	if (drm_WARN_ON_ONCE(vm_exec->vm->drm, vm_exec->two_pass))
+		return -EOPNOTSUPP;
+
 	args.objs = objs;
 	args.num_objs = num_objs;
 
@@ -1469,8 +1749,23 @@ drm_gpuvm_exec_lock_range(struct drm_gpuvm_exec *vm_exec,
 }
 EXPORT_SYMBOL_GPL(drm_gpuvm_exec_lock_range);
 
+/*
+ * An object the current pass deliberately did not lock must not be validated,
+ * it simply stays on the evicted list until a pass which does lock it comes
+ * along.
+ */
+static bool
+drm_gpuvm_validate_skip(struct drm_gpuvm_bo *vm_bo,
+			enum drm_gpuvm_exec_pass pass)
+{
+	drm_gpuvm_pass_assert_held(vm_bo->vm, pass);
+
+	return pass != DRM_GPUVM_EXEC_PASS_ALL && vm_bo->lock_skipped;
+}
+
 static int
-__drm_gpuvm_validate(struct drm_gpuvm *gpuvm, struct drm_exec *exec)
+__drm_gpuvm_validate(struct drm_gpuvm *gpuvm, struct drm_exec *exec,
+		     enum drm_gpuvm_exec_pass pass)
 {
 	const struct drm_gpuvm_ops *ops = gpuvm->ops;
 	struct drm_gpuvm_bo *vm_bo;
@@ -1478,6 +1773,9 @@ __drm_gpuvm_validate(struct drm_gpuvm *gpuvm, struct drm_exec *exec)
 	int ret = 0;
 
 	for_each_vm_bo_in_list(gpuvm, evict, &evict, vm_bo) {
+		if (drm_gpuvm_validate_skip(vm_bo, pass))
+			continue;
+
 		ret = ops->vm_bo_validate(vm_bo, exec);
 		if (ret)
 			break;
@@ -1490,7 +1788,8 @@ __drm_gpuvm_validate(struct drm_gpuvm *gpuvm, struct drm_exec *exec)
 }
 
 static int
-drm_gpuvm_validate_locked(struct drm_gpuvm *gpuvm, struct drm_exec *exec)
+drm_gpuvm_validate_locked(struct drm_gpuvm *gpuvm, struct drm_exec *exec,
+			  enum drm_gpuvm_exec_pass pass)
 {
 	const struct drm_gpuvm_ops *ops = gpuvm->ops;
 	struct drm_gpuvm_bo *vm_bo, *next;
@@ -1503,6 +1802,9 @@ drm_gpuvm_validate_locked(struct drm_gpuvm *gpuvm, struct drm_exec *exec)
 		if (drm_gpuvm_bo_is_zombie(vm_bo))
 			continue;
 
+		if (drm_gpuvm_validate_skip(vm_bo, pass))
+			continue;
+
 		ret = ops->vm_bo_validate(vm_bo, exec);
 		if (ret)
 			break;
@@ -1527,18 +1829,146 @@ drm_gpuvm_validate_locked(struct drm_gpuvm *gpuvm, struct drm_exec *exec)
  */
 int
 drm_gpuvm_validate(struct drm_gpuvm *gpuvm, struct drm_exec *exec)
+{
+	return drm_gpuvm_exec_pass_validate(gpuvm, exec, DRM_GPUVM_EXEC_PASS_ALL);
+}
+EXPORT_SYMBOL_GPL(drm_gpuvm_validate);
+
+/**
+ * drm_gpuvm_exec_pass_validate() - validate the BOs of a pass marked as evicted
+ * @gpuvm: the &drm_gpuvm to validate evicted BOs
+ * @exec: the &drm_exec instance used for locking the GPUVM
+ * @pass: the &enum drm_gpuvm_exec_pass being validated
+ *
+ * Same as drm_gpuvm_validate(), except that the &drm_gpuvm_bos which the
+ * matching drm_gpuvm_exec_pass_prepare_objects() call did not lock are left
+ * alone. They stay on the evicted list for the %DRM_GPUVM_EXEC_PASS_LATE
+ * pass, which locks them, to deal with.
+ *
+ * Anything other than %DRM_GPUVM_EXEC_PASS_ALL requires a
+ * %DRM_GPUVM_RESV_PROTECTED @gpuvm, as for
+ * drm_gpuvm_exec_pass_prepare_objects().
+ *
+ * Returns: 0 on success, negative error code on failure, -EOPNOTSUPP if @pass
+ * is not %DRM_GPUVM_EXEC_PASS_ALL and @gpuvm is not
+ * %DRM_GPUVM_RESV_PROTECTED.
+ */
+int
+drm_gpuvm_exec_pass_validate(struct drm_gpuvm *gpuvm, struct drm_exec *exec,
+			     enum drm_gpuvm_exec_pass pass)
 {
 	const struct drm_gpuvm_ops *ops = gpuvm->ops;
 
 	if (unlikely(!ops || !ops->vm_bo_validate))
 		return -EOPNOTSUPP;
 
+	if (!drm_gpuvm_exec_pass_supported(gpuvm, pass))
+		return -EOPNOTSUPP;
+
+	drm_gpuvm_pass_assert_held(gpuvm, pass);
+
 	if (drm_gpuvm_resv_protected(gpuvm))
-		return drm_gpuvm_validate_locked(gpuvm, exec);
+		return drm_gpuvm_validate_locked(gpuvm, exec, pass);
 	else
-		return __drm_gpuvm_validate(gpuvm, exec);
+		return __drm_gpuvm_validate(gpuvm, exec, pass);
 }
-EXPORT_SYMBOL_GPL(drm_gpuvm_validate);
+EXPORT_SYMBOL_GPL(drm_gpuvm_exec_pass_validate);
+
+/**
+ * drm_gpuvm_exec_pass_needs_split() - whether splitting the locking is worth it
+ * @gpuvm: the &drm_gpuvm to query
+ *
+ * Two-pass locking only pays off when there is validation to be done, since
+ * the point of it is to keep the resident external objects unlocked while
+ * that runs. If nothing is evicted there is no such work, and the split would
+ * only walk the external object list a second time to no purpose.
+ *
+ * This is advisory and O(1). It does not hold the dma-resv of the external
+ * objects, so the answer can be stale by the time the caller acts on it, and
+ * a &drm_gpuvm_bo which is destroyed while evicted keeps it pessimistic
+ * until then. That is fine, because both answers are correct: a
+ * single pass behaves exactly as it did before two-pass locking existed, and
+ * a two-pass sequence with nothing evicted simply finds nothing to do in its
+ * early pass.
+ *
+ * Requires a %DRM_GPUVM_RESV_PROTECTED @gpuvm with its common dma-resv held,
+ * i.e. call it after drm_gpuvm_prepare_vm().
+ *
+ * Returns: true if the caller should use %DRM_GPUVM_EXEC_PASS_EARLY and
+ * %DRM_GPUVM_EXEC_PASS_LATE, false if it should use a single
+ * %DRM_GPUVM_EXEC_PASS_ALL.
+ */
+bool
+drm_gpuvm_exec_pass_needs_split(struct drm_gpuvm *gpuvm)
+{
+	drm_gpuvm_resv_assert_held(gpuvm);
+
+	if (!drm_gpuvm_exec_pass_supported(gpuvm, DRM_GPUVM_EXEC_PASS_EARLY))
+		return false;
+
+	/*
+	 * Evicted private objects are already on the evicted list, having the
+	 * GPUVM's common dma-resv to be added under. Evicted external objects
+	 * are not, drm_gpuvm_bo_evict() cannot put them there, so they are
+	 * counted instead.
+	 */
+	return !list_empty(&gpuvm->evict.list) ||
+	       atomic_read(&gpuvm->extobj.num_evicted);
+}
+EXPORT_SYMBOL_GPL(drm_gpuvm_exec_pass_needs_split);
+
+/**
+ * drm_gpuvm_exec_pass_has_evicted() - whether a pass has evicted BOs left
+ * @gpuvm: the &drm_gpuvm to query
+ * @pass: the &enum drm_gpuvm_exec_pass being validated
+ *
+ * Drivers typically loop over validation and rebinding until nothing is
+ * evicted anymore. Since drm_gpuvm_exec_pass_validate() leaves the objects it
+ * did not lock on the evicted list, %DRM_GPUVM_EXEC_PASS_EARLY must not use a
+ * plain emptiness test for that loop condition or it would never terminate.
+ * %DRM_GPUVM_EXEC_PASS_LATE holds every lock the transaction will ever hold,
+ * so it behaves like %DRM_GPUVM_EXEC_PASS_ALL here.
+ *
+ * This is more accurate than testing the evicted list for emptiness even for
+ * %DRM_GPUVM_EXEC_PASS_ALL, since zombie &drm_gpuvm_bos sit on that list
+ * without ever being validated.
+ *
+ * For a &DRM_GPUVM_RESV_PROTECTED GPUVM the caller must hold its common
+ * dma-resv lock, otherwise the evicted list's internal lock is taken. As
+ * everywhere else, anything other than %DRM_GPUVM_EXEC_PASS_ALL requires the
+ * former.
+ *
+ * Return: true if @pass still has an evicted &drm_gpuvm_bo to validate.
+ */
+bool
+drm_gpuvm_exec_pass_has_evicted(struct drm_gpuvm *gpuvm,
+				enum drm_gpuvm_exec_pass pass)
+{
+	struct drm_gpuvm_bo *vm_bo;
+	bool ret = false;
+
+	if (!drm_gpuvm_resv_protected(gpuvm))
+		spin_lock(&gpuvm->evict.lock);
+	else
+		drm_gpuvm_resv_assert_held(gpuvm);
+
+	list_for_each_entry(vm_bo, &gpuvm->evict.list, list.entry.evict) {
+		if (drm_gpuvm_bo_is_zombie(vm_bo))
+			continue;
+
+		if (drm_gpuvm_validate_skip(vm_bo, pass))
+			continue;
+
+		ret = true;
+		break;
+	}
+
+	if (!drm_gpuvm_resv_protected(gpuvm))
+		spin_unlock(&gpuvm->evict.lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(drm_gpuvm_exec_pass_has_evicted);
 
 /**
  * drm_gpuvm_resv_add_fence - add fence to private and all extobj
@@ -1597,6 +2027,7 @@ drm_gpuvm_bo_create(struct drm_gpuvm *gpuvm,
 	drm_gem_object_get(obj);
 
 	kref_init(&vm_bo->kref);
+	vm_bo->lock_skipped = false;
 	INIT_LIST_HEAD(&vm_bo->list.gpuva);
 	INIT_LIST_HEAD(&vm_bo->list.entry.gem);
 
@@ -1644,6 +2075,21 @@ drm_gpuvm_bo_destroy_not_in_lists_kref(struct kref *kref)
 	drm_gpuvm_bo_destroy_not_in_lists(vm_bo);
 }
 
+/*
+ * Drop a &drm_gpuvm_bo out of &drm_gpuvm.extobj.num_evicted, which counts the
+ * evicted external objects for drm_gpuvm_exec_pass_needs_split(). Only those
+ * are counted, everything else being discoverable from the evicted list.
+ */
+static void
+drm_gpuvm_bo_uncount_evicted(struct drm_gpuvm_bo *vm_bo)
+{
+	struct drm_gpuvm *gpuvm = vm_bo->vm;
+
+	if (drm_gpuvm_resv_protected(gpuvm) && vm_bo->evicted &&
+	    drm_gpuvm_is_extobj(gpuvm, vm_bo->obj))
+		atomic_dec(&gpuvm->extobj.num_evicted);
+}
+
 static void
 drm_gpuvm_bo_destroy(struct kref *kref)
 {
@@ -1655,6 +2101,8 @@ drm_gpuvm_bo_destroy(struct kref *kref)
 	if (!lock)
 		drm_gpuvm_resv_assert_held(gpuvm);
 
+	drm_gpuvm_bo_uncount_evicted(vm_bo);
+
 	drm_gpuvm_bo_list_del(vm_bo, extobj, lock);
 	drm_gpuvm_bo_list_del(vm_bo, evict, lock);
 
@@ -1789,6 +2237,7 @@ drm_gpuvm_bo_deferred_cleanup(struct drm_gpuvm *gpuvm)
 	if (drm_gpuvm_resv_protected(gpuvm)) {
 		dma_resv_lock(drm_gpuvm_resv(gpuvm), NULL);
 		llist_for_each_entry(vm_bo, bo_defer, list.entry.bo_defer) {
+			drm_gpuvm_bo_uncount_evicted(vm_bo);
 			drm_gpuvm_bo_list_del(vm_bo, extobj, false);
 			drm_gpuvm_bo_list_del(vm_bo, evict, false);
 		}
@@ -1959,6 +2408,11 @@ EXPORT_SYMBOL_GPL(drm_gpuvm_bo_extobj_add);
  * @evict: indicates whether the object is evicted
  *
  * Adds a &drm_gpuvm_bo to or removes it from the &drm_gpuvm's evicted list.
+ *
+ * An external object of a %DRM_GPUVM_RESV_PROTECTED &drm_gpuvm is the
+ * exception: the evicted list is protected by the GPUVM's common dma-resv
+ * there, which this does not hold, so such an object is only accounted for
+ * and is put on the list later, by drm_gpuvm_prepare_objects().
  */
 void
 drm_gpuvm_bo_evict(struct drm_gpuvm_bo *vm_bo, bool evict)
@@ -1966,16 +2420,29 @@ drm_gpuvm_bo_evict(struct drm_gpuvm_bo *vm_bo, bool evict)
 	struct drm_gpuvm *gpuvm = vm_bo->vm;
 	struct drm_gem_object *obj = vm_bo->obj;
 	bool lock = !drm_gpuvm_resv_protected(gpuvm);
+	bool was_evicted = vm_bo->evicted;
 
 	dma_resv_assert_held(obj->resv);
-	vm_bo->evicted = evict;
+	/*
+	 * Pairs with the READ_ONCE() in drm_gpuvm_prepare_skip(), which reads
+	 * this without the object's dma-resv held.
+	 */
+	WRITE_ONCE(vm_bo->evicted, evict);
 
 	/* Can't add external objects to the evicted list directly if not using
 	 * internal spinlocks, since in this case the evicted list is protected
 	 * with the VM's common dma-resv lock.
 	 */
-	if (drm_gpuvm_is_extobj(gpuvm, obj) && !lock)
+	if (drm_gpuvm_is_extobj(gpuvm, obj) && !lock) {
+		/*
+		 * Count them instead, so drm_gpuvm_exec_pass_needs_split() can tell
+		 * whether any are evicted without walking the list. The
+		 * object's dma-resv is held, so the transition is stable.
+		 */
+		if (evict != was_evicted)
+			atomic_add(evict ? 1 : -1, &gpuvm->extobj.num_evicted);
 		return;
+	}
 
 	if (evict)
 		drm_gpuvm_bo_list_add(vm_bo, evict, lock);
diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
index 38221d83285b..33a69d771eee 100644
--- a/include/drm/drm_gpuvm.h
+++ b/include/drm/drm_gpuvm.h
@@ -308,6 +308,18 @@ struct drm_gpuvm {
 		 * @extobj.lock: spinlock to protect the extobj list
 		 */
 		spinlock_t lock;
+
+		/**
+		 * @extobj.num_evicted: number of entries of the extobj list
+		 * which are evicted.
+		 *
+		 * Only maintained for a %DRM_GPUVM_RESV_PROTECTED &drm_gpuvm,
+		 * where an evicted external object cannot be put on the
+		 * evicted list, and only so that drm_gpuvm_exec_pass_needs_split()
+		 * does not have to walk the list to find out. Never used to
+		 * decide anything that has to be exact.
+		 */
+		atomic_t num_evicted;
 	} extobj;
 
 	/**
@@ -521,6 +533,76 @@ __drm_gpuva_next(struct drm_gpuva *va)
 #define drm_gpuvm_for_each_va_safe(va__, next__, gpuvm__) \
 	list_for_each_entry_safe(va__, next__, &(gpuvm__)->rb.list, rb.entry)
 
+/**
+ * enum drm_gpuvm_exec_pass - the pass of a &drm_gpuvm locking sequence
+ *
+ * Most drivers lock all the &drm_gem_objects a &drm_gpuvm has mappings of in
+ * a single &drm_exec transaction and use %DRM_GPUVM_EXEC_PASS_ALL, which is
+ * the default.
+ *
+ * A driver may instead ask for the locking to be split in two passes, by
+ * setting &drm_gpuvm_exec.two_pass, in order to bound the time it holds the
+ * dma-resv lock of objects it does not have to validate. Only the evicted
+ * objects need that, so only those are locked by the early pass, and
+ * everything else is locked by the late pass once the validation is already
+ * done.
+ *
+ * The passes only divide up the external objects. The &drm_gpuvm's own
+ * dma-resv is held for the whole transaction, so private objects, which share
+ * it, are available to the driver in the early pass and are validated there.
+ *
+ * Both passes run inside a single &drm_exec transaction: the early pass keeps
+ * everything it locked, and the late pass only ever adds to that. Nothing is
+ * unlocked in between, so work done in the early pass is still valid when the
+ * driver submits at the end of the late pass.
+ */
+enum drm_gpuvm_exec_pass {
+	/**
+	 * @DRM_GPUVM_EXEC_PASS_ALL: Prepare every external object. This is the
+	 * behaviour of a single pass locking sequence. Its value is zero so
+	 * that a zero initialised &drm_gpuvm_exec keeps that behaviour.
+	 */
+	DRM_GPUVM_EXEC_PASS_ALL = 0,
+
+	/**
+	 * @DRM_GPUVM_EXEC_PASS_EARLY: Validate everything the transaction
+	 * already holds, and opportunistically take on the external objects
+	 * which are worth taking on.
+	 *
+	 * The &drm_gpuvm's dma-resv is held from the start of the transaction,
+	 * so every private object is locked before this pass begins. The
+	 * driver validates all of the evicted ones here.
+	 *
+	 * External objects are not locked yet, and this pass only prepares the
+	 * ones which are evicted, that is the ones which are going to need
+	 * validating anyway, for the driver to validate as well. The resident
+	 * ones are deliberately left for later: they are still worked on
+	 * before the transaction ends, having a fence attached or their
+	 * mappings rebound, but none of that has to wait behind a migration,
+	 * so there is no reason to hold their dma-resv while one is going on.
+	 */
+	DRM_GPUVM_EXEC_PASS_EARLY,
+
+	/**
+	 * @DRM_GPUVM_EXEC_PASS_LATE: Lock everything else, and pick up
+	 * whatever raced with the early pass.
+	 *
+	 * This prepares exactly the external objects the early pass left out,
+	 * so that the transaction ends up holding the same set of locks a
+	 * %DRM_GPUVM_EXEC_PASS_ALL one would have. That set is the complement
+	 * of what the early pass actually did, which is not the same thing as
+	 * whatever happens to be resident by now: an object can be evicted
+	 * while the early pass is doing its slow work, and re-preparing an
+	 * object the transaction already holds would fail with -EALREADY.
+	 *
+	 * The driver validates again here. Usually there is nothing left to
+	 * do, but an object which was resident when the early pass skipped it
+	 * may have been evicted since, and this is the pass which holds its
+	 * dma-resv and can deal with it.
+	 */
+	DRM_GPUVM_EXEC_PASS_LATE,
+};
+
 /**
  * struct drm_gpuvm_exec - &drm_gpuvm abstraction of &drm_exec
  *
@@ -544,6 +626,32 @@ struct drm_gpuvm_exec {
 	 */
 	struct drm_gpuvm *vm;
 
+	/**
+	 * @two_pass: split the locking into an early and a late pass; see
+	 * &enum drm_gpuvm_exec_pass. Only supported for a
+	 * %DRM_GPUVM_RESV_PROTECTED &drm_gpuvm, since the two passes have to
+	 * agree on which external objects exist and only the GPUVM's common
+	 * dma-resv, held across both, gives that. Setting it on any other
+	 * &drm_gpuvm warns, and a single pass is used.
+	 *
+	 * This is a request, not a guarantee: the split is skipped when
+	 * drm_gpuvm_exec_pass_needs_split() says it would not help, in which
+	 * case the callback sees a single %DRM_GPUVM_EXEC_PASS_ALL.
+	 *
+	 * Only drm_gpuvm_exec_lock() acts on this. drm_gpuvm_exec_lock_array()
+	 * rejects it and drm_gpuvm_exec_lock_range() ignores it, neither
+	 * having a way to assign the objects it is given to a pass.
+	 */
+	bool two_pass;
+
+	/**
+	 * @pass: the pass currently being prepared. Set by drm_gpuvm_exec_lock()
+	 * before each call to @extra.fn, so that the callback can tell which
+	 * objects it may touch. Always %DRM_GPUVM_EXEC_PASS_ALL unless
+	 * @two_pass is set.
+	 */
+	enum drm_gpuvm_exec_pass pass;
+
 	/**
 	 * @num_fences: the number of fences to reserve for the &dma_resv of the
 	 * locked &drm_gem_objects
@@ -576,6 +684,11 @@ int drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
 			      struct drm_exec *exec,
 			      unsigned int num_fences);
 
+int drm_gpuvm_exec_pass_prepare_objects(struct drm_gpuvm *gpuvm,
+					struct drm_exec *exec,
+					unsigned int num_fences,
+					enum drm_gpuvm_exec_pass pass);
+
 int drm_gpuvm_prepare_range(struct drm_gpuvm *gpuvm,
 			    struct drm_exec *exec,
 			    u64 addr, u64 range,
@@ -606,6 +719,13 @@ drm_gpuvm_exec_unlock(struct drm_gpuvm_exec *vm_exec)
 }
 
 int drm_gpuvm_validate(struct drm_gpuvm *gpuvm, struct drm_exec *exec);
+int drm_gpuvm_exec_pass_validate(struct drm_gpuvm *gpuvm,
+				 struct drm_exec *exec,
+				 enum drm_gpuvm_exec_pass pass);
+bool drm_gpuvm_exec_pass_needs_split(struct drm_gpuvm *gpuvm);
+
+bool drm_gpuvm_exec_pass_has_evicted(struct drm_gpuvm *gpuvm,
+				     enum drm_gpuvm_exec_pass pass);
 void drm_gpuvm_resv_add_fence(struct drm_gpuvm *gpuvm,
 			      struct drm_exec *exec,
 			      struct dma_fence *fence,
@@ -642,7 +762,8 @@ drm_gpuvm_exec_resv_add_fence(struct drm_gpuvm_exec *vm_exec,
 static inline int
 drm_gpuvm_exec_validate(struct drm_gpuvm_exec *vm_exec)
 {
-	return drm_gpuvm_validate(vm_exec->vm, &vm_exec->exec);
+	return drm_gpuvm_exec_pass_validate(vm_exec->vm, &vm_exec->exec,
+					    vm_exec->pass);
 }
 
 /**
@@ -676,10 +797,24 @@ struct drm_gpuvm_bo {
 
 	/**
 	 * @evicted: Indicates whether the &drm_gem_object is evicted; field
-	 * protected by the &drm_gem_object's dma-resv lock.
+	 * protected by the &drm_gem_object's dma-resv lock. Only written
+	 * through drm_gpuvm_bo_evict(), with WRITE_ONCE(), since
+	 * %DRM_GPUVM_EXEC_PASS_EARLY reads it without that lock held.
 	 */
 	bool evicted;
 
+	/**
+	 * @lock_skipped: Indicates that the current &drm_exec transaction does
+	 * not hold this &drm_gpuvm_bo's dma-resv, because
+	 * %DRM_GPUVM_EXEC_PASS_EARLY skipped it as not needing validation.
+	 * Unlike @evicted this is stable for the duration of a locking
+	 * sequence, which is what makes it safe for
+	 * drm_gpuvm_exec_pass_validate() to key off, and what tells
+	 * %DRM_GPUVM_EXEC_PASS_LATE which objects are still missing. Field
+	 * protected the same way as the &drm_gpuvm's extobj list.
+	 */
+	bool lock_skipped;
+
 	/**
 	 * @kref: The reference count for this &drm_gpuvm_bo.
 	 */
-- 
2.34.1

[PATCH v3 2/8] drm/xe: lock the resident BOs of an exec last

[Matthew Brost]

xe_exec_ioctl() locks the dma-resv of every BO mapped in the VM in one
drm_exec transaction, then validates, rebinds and submits. Any migration
or fault-in a client needs therefore happens while it holds the dma-resv
of every object it has mapped, including the ones shared with other
processes. A client faulting in a large buffer of its own stalls whoever
else has those shared objects mapped, so the compositor it is presenting
to can miss a deadline over a set of BOs it has nothing to do with.

Most of those objects are not ones the exec has to validate. Make the
exec transaction two pass, so that it locks the evicted BOs first,
validates them, and only then locks the resident ones. It ends up
holding exactly the locks it holds today, it just takes the ones it does
not have to validate last, once the expensive work is already done.

Only the external BOs are actually split between the passes. The VM's
dma-resv is held from the start, as before, so the evicted private BOs
are validated in the early pass too, without anything extra being
locked for them.

Nothing is unlocked in between the passes, so this needs no recheck and
no fallback. The late pass can still find something to validate, since a
BO it had not locked yet may have been evicted meanwhile; that is handled
the way it is today, with every lock held.

Two details are worth pointing out. xe_vm_rebind() rebinds the whole
rebind list in one go and attaches a fence to the dma-resv of every BO
on it, so it needs all of them locked; the early pass deliberately does
not hold the resident ones, so it leaves the rebind to the late pass
entirely. That is also the better order, since rebinding allocates page
tables and can therefore evict the very BOs the early pass is trying to
leave alone. And the sched job's fence slot is reserved in the late pass
only, that being the one which holds every lock the transaction is going
to hold, so it is still reserved exactly once per object.

A concern with splitting the passes is that validating in the early pass
could evict the very BOs the late pass is about to lock, moving the work
back under the full set of locks. Xe is immune to this by construction:
__xe_bo_validate() brackets its ttm_bo_validate() call with
xe_vm_set_validating(), and xe_bo_eviction_valuable() walks the
drm_gpuvm_bos of any eviction candidate and refuses the ones bound to a VM
the current task is validating. The early pass therefore cannot evict a BO
mapped in the VM it is validating, whether or not the late pass was going
to lock it. That guard predates this patch; self-eviction is pointless
work in a single pass too.

While at it, xe_gpuvm_validate() is changed to clear the evicted state
with drm_gpuvm_bo_evict() rather than by assigning drm_gpuvm_bo::evicted
behind GPUVM's back, so that the bookkeeping GPUVM now does there is not
bypassed.

Cc: Abhinav Kumar <abhinav.kumar@linux.dev>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Anna Maniscalco <anna.maniscalco2000@gmail.com>
Cc: Antonino Maniscalco <antomani103@gmail.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Dmitry Baryshkov <lumag@kernel.org>
Cc: Jessica Zhang <jesszhan0024@gmail.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Marijn Suijten <marijn.suijten@somainline.org>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Rob Clark <robin.clark@oss.qualcomm.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Steven Price <steven.price@arm.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Assisted-by: LLM
Reviewed-by: Francois Dugast <francois.dugast@intel.com>
---
v3:
 - Follow the drm_gpuvm_exec_pass_ function renames (Danilo)
---
 drivers/gpu/drm/xe/xe_exec.c | 23 ++++++++++++++++---
 drivers/gpu/drm/xe/xe_vm.c   | 43 ++++++++++++++++++++++++++++++------
 drivers/gpu/drm/xe/xe_vm.h   |  3 ++-
 3 files changed, 58 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_exec.c b/drivers/gpu/drm/xe/xe_exec.c
index d5293bc33a67..abe522c19ec9 100644
--- a/drivers/gpu/drm/xe/xe_exec.c
+++ b/drivers/gpu/drm/xe/xe_exec.c
@@ -79,8 +79,10 @@
  *	<----------------------------------------------------------------------|
  *	Lock global VM lock in read mode                                       |
  *	Pin userptrs (also finds userptr invalidated since last exec)          |
- *	Lock exec (VM dma-resv lock, external BOs dma-resv locks)              |
+ *	Lock exec early pass (VM and evicted external BOs dma-resv locks)      |
  *	Validate BOs that have been evicted                                    |
+ *	Lock exec late pass (the external BOs left out above)                  |
+ *	Validate any BO evicted since the early pass looked at it              |
  *	Create job                                                             |
  *	Rebind invalidated userptrs + evicted BOs (non-compute-mode)           |
  *	Add rebind fence dependency to job                                     |
@@ -95,15 +97,22 @@
 /*
  * Add validation and rebinding to the drm_exec locking loop, since both can
  * trigger eviction which may require sleeping dma_resv locks.
+ *
+ * Called once per pass, see xe_exec_ioctl(). The fence slot is intended for
+ * the exec sched job and is only reserved in the pass which holds every lock
+ * the transaction is going to hold, so that it is reserved exactly once.
  */
 static int xe_exec_fn(struct drm_gpuvm_exec *vm_exec)
 {
 	struct xe_vm *vm = container_of(vm_exec->vm, struct xe_vm, gpuvm);
+	unsigned int num_fences;
 	int ret;
 
-	/* The fence slot added here is intended for the exec sched job. */
+	num_fences = vm_exec->pass == DRM_GPUVM_EXEC_PASS_EARLY ? 0 : 1;
+
 	xe_vm_set_validation_exec(vm, &vm_exec->exec);
-	ret = xe_vm_validate_rebind(vm, &vm_exec->exec, 1);
+	ret = xe_vm_validate_rebind(vm, &vm_exec->exec, num_fences,
+				    vm_exec->pass);
 	xe_vm_set_validation_exec(vm, NULL);
 	return ret;
 }
@@ -268,6 +277,14 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	if (!xe_vm_in_lr_mode(vm)) {
 		vm_exec.vm = &vm->gpuvm;
 		vm_exec.flags = DRM_EXEC_INTERRUPTIBLE_WAIT;
+		/*
+		 * Only the evicted BOs need validating, so lock those first,
+		 * validate them, and only then lock the resident ones. A
+		 * client faulting in a huge buffer of its own then no longer
+		 * holds, for the duration of that, the dma-resv of a BO it
+		 * shares with the compositor it presents to.
+		 */
+		vm_exec.two_pass = true;
 		err = xe_validation_exec_lock(&ctx, &vm_exec, &xe->val);
 		if (err)
 			goto err_unlock_list;
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 2422bfde9177..0befae97008c 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -357,7 +357,7 @@ static int xe_gpuvm_validate(struct drm_gpuvm_bo *vm_bo, struct drm_exec *exec)
 
 	/* Skip re-populating purged BOs, rebind maps scratch pages. */
 	if (xe_bo_is_purged(bo)) {
-		vm_bo->evicted = false;
+		drm_gpuvm_bo_evict(vm_bo, false);
 		return 0;
 	}
 
@@ -368,7 +368,7 @@ static int xe_gpuvm_validate(struct drm_gpuvm_bo *vm_bo, struct drm_exec *exec)
 	if (ret)
 		return ret;
 
-	vm_bo->evicted = false;
+	drm_gpuvm_bo_evict(vm_bo, false);
 	return 0;
 }
 
@@ -377,31 +377,59 @@ static int xe_gpuvm_validate(struct drm_gpuvm_bo *vm_bo, struct drm_exec *exec)
  * @vm: The vm for which we are rebinding.
  * @exec: The struct drm_exec with the locked GEM objects.
  * @num_fences: The number of fences to reserve for the operation, not
- * including rebinds and validations.
+ * including rebinds and validations. Zero reserves none, which is what the
+ * %DRM_GPUVM_EXEC_PASS_EARLY pass wants.
+ * @pass: The &enum drm_gpuvm_exec_pass @exec was locked for.
  *
  * Validates all evicted gem objects and rebinds their vmas. Note that
  * rebindings may cause evictions and hence the validation-rebind
  * sequence is rerun until there are no more objects to validate.
  *
+ * In the %DRM_GPUVM_EXEC_PASS_EARLY pass only the validation is done, and
+ * only for the objects whose dma-resv @exec holds. The rest, along with the
+ * rebind and the fence reservation, is left to the
+ * %DRM_GPUVM_EXEC_PASS_LATE pass of the same transaction, which locks
+ * everything.
+ *
  * Return: 0 on success, negative error code on error. In particular,
  * may return -EINTR or -ERESTARTSYS if interrupted, and -EDEADLK if
  * the drm_exec transaction needs to be restarted.
  */
 int xe_vm_validate_rebind(struct xe_vm *vm, struct drm_exec *exec,
-			  unsigned int num_fences)
+			  unsigned int num_fences,
+			  enum drm_gpuvm_exec_pass pass)
 {
 	struct drm_gem_object *obj;
 	int ret;
 
 	do {
-		ret = drm_gpuvm_validate(&vm->gpuvm, exec);
+		ret = drm_gpuvm_exec_pass_validate(&vm->gpuvm, exec, pass);
 		if (ret)
 			return ret;
 
+		/*
+		 * xe_vm_rebind() rebinds the whole rebind list in one go and
+		 * attaches a fence to the dma-resv of every BO on it, so it
+		 * needs all of them locked. The early pass deliberately does
+		 * not lock the resident ones, so leave the rebind to the late
+		 * pass, which holds everything.
+		 */
+		if (pass == DRM_GPUVM_EXEC_PASS_EARLY)
+			continue;
+
 		ret = xe_vm_rebind(vm, false);
 		if (ret)
 			return ret;
-	} while (!list_empty(&vm->gpuvm.evict.list));
+	} while (drm_gpuvm_exec_pass_has_evicted(&vm->gpuvm, pass));
+
+	/*
+	 * The early pass reserves nothing. It attaches no fence itself, and
+	 * the objects it locks are still locked in the late pass, whose own
+	 * reservation below walks every object the transaction has
+	 * accumulated and so covers them too.
+	 */
+	if (!num_fences)
+		return 0;
 
 	drm_exec_for_each_locked_object(exec, obj) {
 		ret = dma_resv_reserve_fences(obj->resv, num_fences);
@@ -446,7 +474,8 @@ static int xe_preempt_work_begin(struct drm_exec *exec, struct xe_vm *vm,
 	 * The fence reservation here is intended for the new preempt fences
 	 * we attach at the end of the rebind work.
 	 */
-	return xe_vm_validate_rebind(vm, exec, vm->preempt.num_exec_queues);
+	return xe_vm_validate_rebind(vm, exec, vm->preempt.num_exec_queues,
+				     DRM_GPUVM_EXEC_PASS_ALL);
 }
 
 static bool vm_suspend_rebind_worker(struct xe_vm *vm)
diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
index 89c4f95984a2..e4f184752840 100644
--- a/drivers/gpu/drm/xe/xe_vm.h
+++ b/drivers/gpu/drm/xe/xe_vm.h
@@ -282,7 +282,8 @@ static inline void xe_vm_reactivate_rebind(struct xe_vm *vm)
 int xe_vm_lock_vma(struct drm_exec *exec, struct xe_vma *vma);
 
 int xe_vm_validate_rebind(struct xe_vm *vm, struct drm_exec *exec,
-			  unsigned int num_fences);
+			  unsigned int num_fences,
+			  enum drm_gpuvm_exec_pass pass);
 
 struct dma_fence *xe_vm_bind_kernel_bo(struct xe_vm *vm, struct xe_bo *bo,
 				       struct xe_exec_queue *q, u64 addr,
-- 
2.34.1

[PATCH v3 3/8] drm/panthor: lock the resident BOs of a submit last

[Matthew Brost]

panthor_vm_prepare_mapped_bos_resvs() locks every external object mapped
in the VM and then validates the evicted ones. Validation here means
panthor_vm_bo_validate(), which swaps the BO's pages back in and restores
its VMAs. That is slow, and an external object is one which can be shared
with another process, so the whole of it happens while holding dma-resv
locks other processes may be waiting on.

Nothing is gained by holding those. A resident object needs no swapping
in; only the evicted ones do. Split the locking into the two passes
gpuvm now understands: the early pass takes just the evicted external
objects and swaps them in, and the late pass takes the ones which were
resident and are therefore normally ready to use as they are. Private
objects are covered by the VM resv, which is held from the start, so
evicted ones are still validated in the early pass.

The split is only worth it when there is something to validate, so
drm_gpuvm_exec_pass_needs_split() decides, and a submit with nothing
evicted keeps doing exactly what it does today in a single pass.

Both passes run in the same drm_exec transaction, so nothing is unlocked
in between and the late pass only ever adds locks. They take disjoint
sets of objects, so passing slot_count to both still reserves it exactly
once per object.

The early pass reads the evicted state without the object's dma-resv,
that being the lock it is trying not to take. The race is benign: an
object evicted right after the early pass skipped it is picked up by the
late pass instead, which is why that pass still validates.

Validation here allocates pages, which can recurse into panthor's own
shrinker, so it is worth being explicit about what the early pass can
evict. There is no deadlock: drm_gem_lru_scan() acquires the resv with
ww_mutex_trylock() and skips what it cannot get. VM-exclusive BOs share
the VM resv, which is held across both passes, so those are always
skipped. External objects are not held by the early pass, though, so
reclaim can evict one while the early pass validates something else.

That is handled, and is why the late pass validates rather than only
locking: it picks up anything evicted after the early pass looked at it.
The cost is that the swapin for such a BO happens under the full set of
locks, i.e. it degrades to the current behaviour for that one object.

Xe avoids this by refusing to evict BOs bound to a VM the current task is
validating (xe_bo_eviction_valuable() and xe_vm_is_validating()). Panthor
has no equivalent guard. Adding one would make the split more effective
under memory pressure, but it is not needed for correctness, so it is left
as a follow up.

Cc: Abhinav Kumar <abhinav.kumar@linux.dev>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Anna Maniscalco <anna.maniscalco2000@gmail.com>
Cc: Antonino Maniscalco <antomani103@gmail.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Dmitry Baryshkov <lumag@kernel.org>
Cc: Jessica Zhang <jesszhan0024@gmail.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Marijn Suijten <marijn.suijten@somainline.org>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Rob Clark <robin.clark@oss.qualcomm.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Steven Price <steven.price@arm.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Assisted-by: LLM
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
---
v3:
 - Follow the drm_gpuvm_exec_pass_ function renames (Danilo)
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 55 ++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index d75d575473da..d4b968ab094e 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -3258,6 +3258,26 @@ int panthor_vm_unmap_range(struct panthor_vm *vm, u64 va, u64 size)
  * need to reserve a slot on all BOs mapped to a VM and update this slot with
  * the job fence after its submission.
  *
+ * When something is evicted the locks are taken in two passes; when nothing
+ * is, a single pass is used, as before. The early pass only takes the external
+ * objects which actually need validating, i.e. the evicted ones, and swaps
+ * them back in. Private objects are covered by the VM resv, which is held
+ * from the start, so they are validated here too. The late pass then takes
+ * the external objects the early pass left out, which were resident and so
+ * normally need no swapping in; it still validates, since one of them may
+ * have been evicted in the meantime.
+ *
+ * The point is that panthor_vm_bo_validate() swaps pages back in, which is
+ * slow, and an external object is one which can be shared with another
+ * process. Doing that while holding the resv of a resident shared BO would
+ * stall whoever else needs it, for no benefit, since a resident object is
+ * ready to use as it is.
+ *
+ * Both passes run in the same drm_exec transaction: nothing is unlocked in
+ * between and the late pass only ever adds locks. The passes take disjoint
+ * sets of objects, so reserving @slot_count in each still reserves it
+ * exactly once per object.
+ *
  * Return: 0 on success, a negative error code otherwise.
  */
 int panthor_vm_prepare_mapped_bos_resvs(struct drm_exec *exec, struct panthor_vm *vm,
@@ -3270,11 +3290,42 @@ int panthor_vm_prepare_mapped_bos_resvs(struct drm_exec *exec, struct panthor_vm
 	if (ret)
 		return ret;
 
-	ret = drm_gpuvm_prepare_objects(&vm->base, exec, slot_count);
+	/*
+	 * With nothing evicted there is no validation to keep the resident
+	 * objects unlocked for, so do not pay for the second walk.
+	 */
+	if (!drm_gpuvm_exec_pass_needs_split(&vm->base)) {
+		ret = drm_gpuvm_prepare_objects(&vm->base, exec, slot_count);
+		if (ret)
+			return ret;
+
+		return drm_gpuvm_validate(&vm->base, exec);
+	}
+
+	ret = drm_gpuvm_exec_pass_prepare_objects(&vm->base, exec,
+						  slot_count,
+						  DRM_GPUVM_EXEC_PASS_EARLY);
+	if (ret)
+		return ret;
+
+	ret = drm_gpuvm_exec_pass_validate(&vm->base, exec,
+					   DRM_GPUVM_EXEC_PASS_EARLY);
 	if (ret)
 		return ret;
 
-	return drm_gpuvm_validate(&vm->base, exec);
+	ret = drm_gpuvm_exec_pass_prepare_objects(&vm->base, exec,
+						  slot_count,
+						  DRM_GPUVM_EXEC_PASS_LATE);
+	if (ret)
+		return ret;
+
+	/*
+	 * Objects the early pass skipped were resident then, but another
+	 * process may have evicted one since. Now that everything is locked,
+	 * pick up whatever is left.
+	 */
+	return drm_gpuvm_exec_pass_validate(&vm->base, exec,
+					    DRM_GPUVM_EXEC_PASS_LATE);
 }
 
 unsigned long
-- 
2.34.1

[PATCH v3 4/8] drm/msm: reject a submit_bo table on VM_BIND contexts

[Matthew Brost]

The uapi says a VM_BIND context must not pass a submit_bo table to
MSM_GEM_SUBMIT and that one will be rejected, but nothing checks
nr_bos. A VM_BIND context which passes one anyway runs the legacy BO
handling against its userspace managed VM:

 - submit_lock_objects_vmbind() only locks the objects mapped in the
   VM, yet submit_pin_objects() calls msm_gem_get_vma_locked() on every
   submit BO. For a BO not mapped in the VM that walks and modifies the
   object's gpuva list without its resv held, and has the kernel
   allocate a VMA spanning [0, U64_MAX) in a VM whose address space
   belongs to userspace.

 - Every submit BO holds a vm_bo reference which msm_submit_retire()
   drops with only the object's resv held. If userspace unmaps the BO
   with VM_BIND while the submit is in flight, that is the last
   reference, and drm_gpuvm_bo_destroy() runs without the VM's resv.

Reject nr_bos != 0 on VM_BIND contexts, as documented. Mesa only passes
a submit_bo table when VM_BIND is not enabled.

Fixes: 2e6a8a1fe2b2 ("drm/msm: Add VM_BIND ioctl")
Cc: Abhinav Kumar <abhinav.kumar@linux.dev>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Anna Maniscalco <anna.maniscalco2000@gmail.com>
Cc: Antonino Maniscalco <antomani103@gmail.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Dmitry Baryshkov <lumag@kernel.org>
Cc: Jessica Zhang <jesszhan0024@gmail.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Marijn Suijten <marijn.suijten@somainline.org>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Rob Clark <robin.clark@oss.qualcomm.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Steven Price <steven.price@arm.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: stable@vger.kernel.org
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Assisted-by: LLM
---
v3:
 - New patch (Sashiko)
---
 drivers/gpu/drm/msm/msm_gem_submit.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 5862db05297a..1215b388cb40 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -598,6 +598,12 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 		goto out_post_unlock;
 	}
 
+	/* The resident set of a VM_BIND context comes from its VM_BIND ops */
+	if (msm_context_is_vmbind(ctx) && args->nr_bos) {
+		ret = UERR(EINVAL, dev, "submit_bo table not allowed with VM_BIND");
+		goto out_post_unlock;
+	}
+
 	ring = gpu->rb[queue->ring_nr];
 
 	if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
-- 
2.34.1

[PATCH v3 5/8] drm/msm: use DRM_GPUVM_RESV_PROTECTED for VM_BIND VMs

[Matthew Brost]

msm_gem_vm_create() creates every drm_gpuvm without
DRM_GPUVM_RESV_PROTECTED, on the grounds that it makes
drm_gpuvm_bo_evict() lose track of evicted external objects. It does not:
drm_gpuvm_bo_evict() still sets drm_gpuvm_bo::evicted on an extobj, and
drm_gpuvm_prepare_objects() moves any such extobj onto the evicted list
before drm_gpuvm_validate() looks at it. The VM_BIND submit path always
calls the two in that order.

Userspace managed VMs already touch the extobj and evicted lists only
with the VM's resv held: VMAs are created and linked by VM_BIND under
the VM resv, msm_gem_vma_close() asserts it, and every drm_gpuvm_bo_put()
which can drop the last reference of a VM_BIND vm_bo runs with it held,
via msm_gem_lock_vm_and_obj(), with_vm_locks() or the object free path.
The internal spinlocks buy nothing there, so set
DRM_GPUVM_RESV_PROTECTED for those VMs.

Kernel managed VMs are left alone. The legacy submit path holds a vm_bo
reference per BO and drops it in msm_submit_retire() with only the
object's resv held, which could be the last reference once the VMA is
gone. VM_BIND contexts never get there, the previous patch having made
MSM_GEM_SUBMIT reject a submit_bo table from them.

This is also what two pass locking in drm_gpuvm requires, which a
following patch makes use of.

Cc: Abhinav Kumar <abhinav.kumar@linux.dev>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Anna Maniscalco <anna.maniscalco2000@gmail.com>
Cc: Antonino Maniscalco <antomani103@gmail.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Dmitry Baryshkov <lumag@kernel.org>
Cc: Jessica Zhang <jesszhan0024@gmail.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Marijn Suijten <marijn.suijten@somainline.org>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Rob Clark <robin.clark@oss.qualcomm.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Steven Price <steven.price@arm.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Assisted-by: LLM
---
v3:
 - Rely on VM_BIND contexts not being able to pass a submit_bo table,
   now enforced by the previous patch (Sashiko)
---
 drivers/gpu/drm/msm/msm_gem_vma.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index 1badec3caa7b..c2b2415e86c9 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -818,11 +818,19 @@ msm_gem_vm_create(struct drm_device *drm, struct msm_mmu *mmu, const char *name,
 		  u64 va_start, u64 va_size, bool managed)
 {
 	/*
-	 * We mostly want to use DRM_GPUVM_RESV_PROTECTED, except that
-	 * makes drm_gpuvm_bo_evict() a no-op for extobjs (ie. we loose
-	 * tracking that an extobj is evicted) :facepalm:
+	 * Userspace managed (VM_BIND) VMs only ever touch the gpuvm's extobj
+	 * and evicted lists with the VM's resv held, so use
+	 * DRM_GPUVM_RESV_PROTECTED for those.  drm_gpuvm_bo_evict() cannot
+	 * put an extobj on the evicted list there, but it records the
+	 * eviction and drm_gpuvm_prepare_objects() moves it onto the list
+	 * before drm_gpuvm_validate() runs, so nothing is lost.
+	 *
+	 * Kernel managed VMs keep the internal spinlocks, since the legacy
+	 * submit path can drop the last vm_bo reference with only the
+	 * object's resv held (see msm_submit_retire()).  VM_BIND contexts
+	 * cannot reach that path, as they may not pass a submit_bo table.
 	 */
-	enum drm_gpuvm_flags flags = 0;
+	enum drm_gpuvm_flags flags = managed ? 0 : DRM_GPUVM_RESV_PROTECTED;
 	struct msm_gem_vm *vm;
 	struct drm_gem_object *dummy_gem;
 	int ret = 0;
-- 
2.34.1

[PATCH v3 6/8] drm/msm: lock the resident BOs of a VM_BIND submit last

[Matthew Brost]

A VM_BIND submit locks the resv of every BO mapped in the VM, then
validates the evicted ones, which means getting their pages and mapping
them again. That is slow, and external objects can be shared with other
processes, so all of it happens while holding resv locks other processes
may be waiting on, for BOs which needed no work at all.

Use the two pass locking gpuvm now provides. The early pass locks only
the evicted external objects and validates them, along with the evicted
private ones, which the VM resv held from the start covers. The late
pass locks the external objects which were resident, and still validates
in case one of them was evicted meanwhile. When nothing is evicted,
drm_gpuvm_exec_pass_needs_split() says so and the submit keeps using a
single pass.

Both passes run in the same drm_exec transaction, nothing is unlocked in
between, and they take disjoint sets of objects, so reserving one fence
slot in each still reserves it exactly once per object.

This moves validation from after drm_sched_job_arm() and fence
attachment into the locking loop, ahead of everything else, which is
also where a failure is easiest to unwind. The order does not matter to
the shrinker: it skips any BO mapped in a VM whose resv it cannot
trylock, and the submit holds the VM resv throughout, so a BO mapped in
this VM cannot be evicted while it is locked, whether or not a fence is
attached to it yet. The same means the early pass can never evict a BO
the late pass is about to lock, the property Xe gets from
xe_vm_set_validating().

Cc: Abhinav Kumar <abhinav.kumar@linux.dev>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Anna Maniscalco <anna.maniscalco2000@gmail.com>
Cc: Antonino Maniscalco <antomani103@gmail.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Dmitry Baryshkov <lumag@kernel.org>
Cc: Jessica Zhang <jesszhan0024@gmail.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Marijn Suijten <marijn.suijten@somainline.org>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Rob Clark <robin.clark@oss.qualcomm.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Steven Price <steven.price@arm.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Assisted-by: LLM
---
v3:
 - Follow the drm_gpuvm_exec_pass_ function renames (Danilo)
---
 drivers/gpu/drm/msm/msm_gem_submit.c | 84 ++++++++++++++++++++++------
 1 file changed, 66 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 1215b388cb40..4e454345567a 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -266,6 +266,71 @@ static int submit_lookup_cmds(struct msm_gem_submit *submit,
 	return ret;
 }
 
+/*
+ * Lock and validate every BO mapped in a VM_BIND VM.  Unlike the legacy path,
+ * where submit_pin_objects() only validates the BOs userspace attached to the
+ * submit, userspace does not tell us which BOs a VM_BIND submit uses, so the
+ * entire VM has to be validated.
+ *
+ * When something is evicted, the locks are taken in two passes.  The early
+ * pass locks only the external objects which need validating, i.e. the
+ * evicted ones, and validates them along with the evicted private objects,
+ * which the VM resv held from the start already covers.  The late pass then
+ * locks the external objects which were resident.  Validation means getting
+ * pages and mapping them, which is slow, and an external object can be shared
+ * with another process, so there is no point in stalling that process on the
+ * resv of a resident BO for the duration of it.  The late pass still
+ * validates, in case one of those BOs got evicted meanwhile.
+ *
+ * Both passes run in the same drm_exec transaction, nothing is unlocked in
+ * between, and they take disjoint sets of objects, so reserving one fence
+ * slot in each reserves it exactly once per object.
+ *
+ * The shrinker cannot evict a BO the early pass is about to validate, nor one
+ * it has validated already: it only evicts a BO after trylocking the resv of
+ * every VM the BO is mapped in, and the VM resv is held throughout.
+ */
+static int submit_prepare_vm_objects(struct msm_gem_submit *submit)
+{
+	struct drm_gpuvm *vm = submit->vm;
+	struct drm_exec *exec = &submit->exec;
+	int ret;
+
+	ret = drm_gpuvm_prepare_vm(vm, exec, 1);
+	if (ret)
+		return ret;
+
+	/*
+	 * With nothing evicted there is no validation to keep the resident
+	 * objects unlocked for, so do not pay for the second walk.
+	 */
+	if (!drm_gpuvm_exec_pass_needs_split(vm)) {
+		ret = drm_gpuvm_prepare_objects(vm, exec, 1);
+		if (ret)
+			return ret;
+
+		return drm_gpuvm_validate(vm, exec);
+	}
+
+	ret = drm_gpuvm_exec_pass_prepare_objects(vm, exec, 1,
+						  DRM_GPUVM_EXEC_PASS_EARLY);
+	if (ret)
+		return ret;
+
+	ret = drm_gpuvm_exec_pass_validate(vm, exec,
+					   DRM_GPUVM_EXEC_PASS_EARLY);
+	if (ret)
+		return ret;
+
+	ret = drm_gpuvm_exec_pass_prepare_objects(vm, exec, 1,
+						  DRM_GPUVM_EXEC_PASS_LATE);
+	if (ret)
+		return ret;
+
+	return drm_gpuvm_exec_pass_validate(vm, exec,
+					    DRM_GPUVM_EXEC_PASS_LATE);
+}
+
 static int submit_lock_objects_vmbind(struct msm_gem_submit *submit)
 {
 	unsigned flags = DRM_EXEC_INTERRUPTIBLE_WAIT | DRM_EXEC_IGNORE_DUPLICATES;
@@ -276,12 +341,7 @@ static int submit_lock_objects_vmbind(struct msm_gem_submit *submit)
 	submit->has_exec = true;
 
 	drm_exec_until_all_locked (&submit->exec) {
-		ret = drm_gpuvm_prepare_vm(submit->vm, exec, 1);
-		drm_exec_retry_on_contention(exec);
-		if (ret)
-			break;
-
-		ret = drm_gpuvm_prepare_objects(submit->vm, exec, 1);
+		ret = submit_prepare_vm_objects(submit);
 		drm_exec_retry_on_contention(exec);
 		if (ret)
 			break;
@@ -790,18 +850,6 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
 
 	submit_attach_object_fences(submit);
 
-	if (msm_context_is_vmbind(ctx)) {
-		/*
-		 * If we are not using VM_BIND, submit_pin_vmas() will validate
-		 * just the BOs attached to the submit.  In that case we don't
-		 * need to validate the _entire_ vm, because userspace tracked
-		 * what BOs are associated with the submit.
-		 */
-		ret = drm_gpuvm_validate(submit->vm, &submit->exec);
-		if (ret)
-			goto out;
-	}
-
 	/* The scheduler owns a ref now: */
 	msm_gem_submit_get(submit);
 
-- 
2.34.1

[PATCH v3 7/8] drm/nouveau: use DRM_GPUVM_RESV_PROTECTED

[Matthew Brost]

nouveau creates its drm_gpuvm without DRM_GPUVM_RESV_PROTECTED, so the
extobj and evicted lists are protected by internal spinlocks. Every
nouveau path but three already touches them with the VM's resv held:
drm_gpuvm_exec_lock() and drm_gpuvm_validate() on exec, and TTM moves of
private BOs, which share the VM's resv. The remaining three are:

 - nouveau_uvmm_bind_job_submit() adds the vm_bo of a MAP op to the
   extobj list holding no resv at all. Move that into
   bind_lock_validate(), which now locks the VM's resv as well.

 - bind_link_gpuvas() unlinks the GPUVAs of UNMAP and REMAP ops, which
   can drop the last reference of their vm_bo. It runs within the bind
   job's drm_exec transaction, which the above makes hold the VM's resv.

 - nouveau_uvmm_bind_job_cleanup() and nouveau_uvmm_fini() can drop the
   last reference of a vm_bo holding only the object's resv. Lock the
   VM's resv along with it, through a new
   nouveau_uvmm_lock_vm_and_obj().

The bind job's fence now always lands in the VM's resv, as BOOKKEEP.
This already happened whenever a bind job mapped a private BO.

With that the internal spinlocks buy nothing, so set
DRM_GPUVM_RESV_PROTECTED. This is also what two-pass locking in
drm_gpuvm requires, which a following patch makes use of.

Cc: Abhinav Kumar <abhinav.kumar@linux.dev>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Anna Maniscalco <anna.maniscalco2000@gmail.com>
Cc: Antonino Maniscalco <antomani103@gmail.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Dmitry Baryshkov <lumag@kernel.org>
Cc: Jessica Zhang <jesszhan0024@gmail.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Marijn Suijten <marijn.suijten@somainline.org>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Rob Clark <robin.clark@oss.qualcomm.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Steven Price <steven.price@arm.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Assisted-by: LLM
---
v3:
 - New patch (Danilo)
---
 drivers/gpu/drm/nouveau/nouveau_uvmm.c | 49 ++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
index 2026fe6b48c6..dae612e56d91 100644
--- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
@@ -1187,6 +1187,27 @@ bind_validate_region(struct nouveau_job *job)
 	return 0;
 }
 
+/*
+ * Lock the VM's common dma-resv together with the one of @obj, as needed to
+ * drop what may be the last reference of a &drm_gpuvm_bo.
+ */
+static void
+nouveau_uvmm_lock_vm_and_obj(struct nouveau_uvmm *uvmm, struct drm_exec *exec,
+			     struct drm_gem_object *obj)
+{
+	int ret;
+
+	drm_exec_init(exec, DRM_EXEC_IGNORE_DUPLICATES, 2);
+	drm_exec_until_all_locked(exec) {
+		ret = drm_exec_lock_obj(exec, drm_gpuvm_resv_obj(&uvmm->base));
+		if (!ret)
+			ret = drm_exec_lock_obj(exec, obj);
+		drm_exec_retry_on_contention(exec);
+		if (drm_WARN_ON(uvmm->base.drm, ret))
+			break;
+	}
+}
+
 static void
 bind_link_gpuvas(struct bind_job_op *bop)
 {
@@ -1224,12 +1245,24 @@ bind_lock_validate(struct nouveau_job *job, struct drm_exec *exec,
 		   unsigned int num_fences)
 {
 	struct nouveau_uvmm_bind_job *bind_job = to_uvmm_bind_job(job);
+	struct nouveau_uvmm *uvmm = nouveau_cli_uvmm(job->cli);
 	struct bind_job_op *op;
 	int ret;
 
+	/* The VM's dma-resv protects its extobj and evicted lists, and must be
+	 * held by bind_link_gpuvas() in case it drops the last reference of a
+	 * &drm_gpuvm_bo.
+	 */
+	ret = drm_gpuvm_prepare_vm(&uvmm->base, exec, num_fences);
+	if (ret)
+		return ret;
+
 	list_for_each_op(op, &bind_job->ops) {
 		struct drm_gpuva_op *va_op;
 
+		if (op->op == OP_MAP)
+			drm_gpuvm_bo_extobj_add(op->vm_bo);
+
 		if (!op->ops)
 			continue;
 
@@ -1288,8 +1321,6 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job,
 			dma_resv_unlock(obj->resv);
 			if (IS_ERR(op->vm_bo))
 				return PTR_ERR(op->vm_bo);
-
-			drm_gpuvm_bo_extobj_add(op->vm_bo);
 		}
 
 		ret = bind_validate_op(job, op);
@@ -1603,9 +1634,11 @@ nouveau_uvmm_bind_job_cleanup(struct nouveau_job *job)
 			drm_gpuva_ops_free(&uvmm->base, op->ops);
 
 		if (!IS_ERR_OR_NULL(op->vm_bo)) {
-			dma_resv_lock(obj->resv, NULL);
+			struct drm_exec exec;
+
+			nouveau_uvmm_lock_vm_and_obj(uvmm, &exec, obj);
 			drm_gpuvm_bo_put(op->vm_bo);
-			dma_resv_unlock(obj->resv);
+			drm_exec_fini(&exec);
 		}
 
 		if (obj)
@@ -1941,7 +1974,8 @@ nouveau_uvmm_ioctl_vm_init(struct drm_device *dev,
 	mt_init_flags(&uvmm->region_mt, MT_FLAGS_LOCK_EXTERN);
 	mt_set_external_lock(&uvmm->region_mt, &uvmm->mutex);
 
-	drm_gpuvm_init(&uvmm->base, cli->name, 0, drm, r_obj,
+	drm_gpuvm_init(&uvmm->base, cli->name, DRM_GPUVM_RESV_PROTECTED,
+		       drm, r_obj,
 		       NOUVEAU_VA_SPACE_START,
 		       NOUVEAU_VA_SPACE_END,
 		       init->kernel_managed_addr,
@@ -1978,6 +2012,7 @@ nouveau_uvmm_fini(struct nouveau_uvmm *uvmm)
 	struct nouveau_uvma_region *reg;
 	struct nouveau_cli *cli = uvmm->vmm.cli;
 	struct drm_gpuva *va, *next;
+	struct drm_exec exec;
 
 	nouveau_uvmm_lock(uvmm);
 	drm_gpuvm_for_each_va_safe(va, next, &uvmm->base) {
@@ -1989,9 +2024,9 @@ nouveau_uvmm_fini(struct nouveau_uvmm *uvmm)
 
 		drm_gpuva_remove(va);
 
-		dma_resv_lock(obj->resv, NULL);
+		nouveau_uvmm_lock_vm_and_obj(uvmm, &exec, obj);
 		drm_gpuva_unlink(va);
-		dma_resv_unlock(obj->resv);
+		drm_exec_fini(&exec);
 
 		nouveau_uvma_unmap(uvma);
 		nouveau_uvma_vmm_put(uvma);
-- 
2.34.1

[PATCH v3 8/8] drm/nouveau: lock the resident BOs of an exec last

[Matthew Brost]

nouveau_exec_job_submit() locks every BO mapped in the VM, then
validates the evicted ones while holding all of those locks. A client
paging a large working set back in therefore keeps the dma-resv of every
BO it shares, e.g. with the compositor it presents to, locked for as
long as that takes, stalling the other side for no reason.

Use the two-pass locking drm_gpuvm now provides. Validation moves from
after drm_gpuvm_exec_lock() into its per-pass extra.fn callback, so the
early pass validates the evicted BOs while the resident external ones
are still unlocked, and the late pass locks those and validates
anything that was evicted in the meantime. drm_gpuvm_exec_lock() falls
back to a single pass when nothing is evicted.

Validation now runs under the uvmm mutex, like it already does for bind
jobs in bind_lock_validate().

Cc: Abhinav Kumar <abhinav.kumar@linux.dev>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Anna Maniscalco <anna.maniscalco2000@gmail.com>
Cc: Antonino Maniscalco <antomani103@gmail.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Dmitry Baryshkov <lumag@kernel.org>
Cc: Jessica Zhang <jesszhan0024@gmail.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Marijn Suijten <marijn.suijten@somainline.org>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Rob Clark <robin.clark@oss.qualcomm.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Steven Price <steven.price@arm.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Assisted-by: LLM
---
v3:
 - New patch (Danilo)
---
 drivers/gpu/drm/nouveau/nouveau_exec.c | 27 +++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_exec.c b/drivers/gpu/drm/nouveau/nouveau_exec.c
index eea6619cffaa..b9ab7da125b1 100644
--- a/drivers/gpu/drm/nouveau/nouveau_exec.c
+++ b/drivers/gpu/drm/nouveau/nouveau_exec.c
@@ -85,6 +85,18 @@
  * the corresponding VM_BIND jobs they depend on - attached to them.
  */
 
+/*
+ * Called once per pass of drm_gpuvm_exec_lock(), validating whatever that pass
+ * locked. The early pass validates the evicted BOs before the resident ones
+ * are locked, so that a BO shared with another client, e.g. a compositor, is
+ * not held locked while this job's evicted BOs are moved back in.
+ */
+static int
+nouveau_exec_job_validate(struct drm_gpuvm_exec *vme)
+{
+	return drm_gpuvm_exec_validate(vme);
+}
+
 static int
 nouveau_exec_job_submit(struct nouveau_job *job,
 			struct drm_gpuvm_exec *vme)
@@ -99,21 +111,14 @@ nouveau_exec_job_submit(struct nouveau_job *job,
 	if (ret)
 		return ret;
 
+	vme->two_pass = true;
+	vme->extra.fn = nouveau_exec_job_validate;
+
 	nouveau_uvmm_lock(uvmm);
 	ret = drm_gpuvm_exec_lock(vme);
-	if (ret) {
-		nouveau_uvmm_unlock(uvmm);
-		return ret;
-	}
 	nouveau_uvmm_unlock(uvmm);
 
-	ret = drm_gpuvm_exec_validate(vme);
-	if (ret) {
-		drm_gpuvm_exec_unlock(vme);
-		return ret;
-	}
-
-	return 0;
+	return ret;
 }
 
 static void
-- 
2.34.1

Re: [PATCH v3 7/8] drm/nouveau: use DRM_GPUVM_RESV_PROTECTED

[Matthew Brost]

On Thu, Oct 01, 2026 at 03:06:31PM -0700, Matthew Brost wrote:
> nouveau creates its drm_gpuvm without DRM_GPUVM_RESV_PROTECTED, so the
> extobj and evicted lists are protected by internal spinlocks. Every
> nouveau path but three already touches them with the VM's resv held:
> drm_gpuvm_exec_lock() and drm_gpuvm_validate() on exec, and TTM moves of
> private BOs, which share the VM's resv. The remaining three are:
> 
>  - nouveau_uvmm_bind_job_submit() adds the vm_bo of a MAP op to the
>    extobj list holding no resv at all. Move that into
>    bind_lock_validate(), which now locks the VM's resv as well.
> 
>  - bind_link_gpuvas() unlinks the GPUVAs of UNMAP and REMAP ops, which
>    can drop the last reference of their vm_bo. It runs within the bind
>    job's drm_exec transaction, which the above makes hold the VM's resv.
> 
>  - nouveau_uvmm_bind_job_cleanup() and nouveau_uvmm_fini() can drop the
>    last reference of a vm_bo holding only the object's resv. Lock the
>    VM's resv along with it, through a new
>    nouveau_uvmm_lock_vm_and_obj().
> 
> The bind job's fence now always lands in the VM's resv, as BOOKKEEP.
> This already happened whenever a bind job mapped a private BO.
> 
> With that the internal spinlocks buy nothing, so set
> DRM_GPUVM_RESV_PROTECTED. This is also what two-pass locking in
> drm_gpuvm requires, which a following patch makes use of.
> 
> Cc: Abhinav Kumar <abhinav.kumar@linux.dev>
> Cc: Alice Ryhl <aliceryhl@google.com>
> Cc: Anna Maniscalco <anna.maniscalco2000@gmail.com>
> Cc: Antonino Maniscalco <antomani103@gmail.com>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Dmitry Baryshkov <lumag@kernel.org>
> Cc: Jessica Zhang <jesszhan0024@gmail.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Marijn Suijten <marijn.suijten@somainline.org>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Rob Clark <robin.clark@oss.qualcomm.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Shuah Khan <skhan@linuxfoundation.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Assisted-by: LLM
> ---
> v3:
>  - New patch (Danilo)
> ---
>  drivers/gpu/drm/nouveau/nouveau_uvmm.c | 49 ++++++++++++++++++++++----
>  1 file changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index 2026fe6b48c6..dae612e56d91 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -1187,6 +1187,27 @@ bind_validate_region(struct nouveau_job *job)
>  	return 0;
>  }
>  
> +/*
> + * Lock the VM's common dma-resv together with the one of @obj, as needed to
> + * drop what may be the last reference of a &drm_gpuvm_bo.
> + */
> +static void
> +nouveau_uvmm_lock_vm_and_obj(struct nouveau_uvmm *uvmm, struct drm_exec *exec,
> +			     struct drm_gem_object *obj)
> +{
> +	int ret;
> +
> +	drm_exec_init(exec, DRM_EXEC_IGNORE_DUPLICATES, 2);
> +	drm_exec_until_all_locked(exec) {
> +		ret = drm_exec_lock_obj(exec, drm_gpuvm_resv_obj(&uvmm->base));
> +		if (!ret)
> +			ret = drm_exec_lock_obj(exec, obj);
> +		drm_exec_retry_on_contention(exec);
> +		if (drm_WARN_ON(uvmm->base.drm, ret))

Sashiko flagged that -ENOMEM is a possible return from drm_exec_lock_obj
if a kmalloc fails and that is correct.

My plan is to change this and open coded WW transaction which can't
fail.

Not going to post now as I don't want to spam the list.

Matt

> +			break;
> +	}
> +}
> +
>  static void
>  bind_link_gpuvas(struct bind_job_op *bop)
>  {
> @@ -1224,12 +1245,24 @@ bind_lock_validate(struct nouveau_job *job, struct drm_exec *exec,
>  		   unsigned int num_fences)
>  {
>  	struct nouveau_uvmm_bind_job *bind_job = to_uvmm_bind_job(job);
> +	struct nouveau_uvmm *uvmm = nouveau_cli_uvmm(job->cli);
>  	struct bind_job_op *op;
>  	int ret;
>  
> +	/* The VM's dma-resv protects its extobj and evicted lists, and must be
> +	 * held by bind_link_gpuvas() in case it drops the last reference of a
> +	 * &drm_gpuvm_bo.
> +	 */
> +	ret = drm_gpuvm_prepare_vm(&uvmm->base, exec, num_fences);
> +	if (ret)
> +		return ret;
> +
>  	list_for_each_op(op, &bind_job->ops) {
>  		struct drm_gpuva_op *va_op;
>  
> +		if (op->op == OP_MAP)
> +			drm_gpuvm_bo_extobj_add(op->vm_bo);
> +
>  		if (!op->ops)
>  			continue;
>  
> @@ -1288,8 +1321,6 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job,
>  			dma_resv_unlock(obj->resv);
>  			if (IS_ERR(op->vm_bo))
>  				return PTR_ERR(op->vm_bo);
> -
> -			drm_gpuvm_bo_extobj_add(op->vm_bo);
>  		}
>  
>  		ret = bind_validate_op(job, op);
> @@ -1603,9 +1634,11 @@ nouveau_uvmm_bind_job_cleanup(struct nouveau_job *job)
>  			drm_gpuva_ops_free(&uvmm->base, op->ops);
>  
>  		if (!IS_ERR_OR_NULL(op->vm_bo)) {
> -			dma_resv_lock(obj->resv, NULL);
> +			struct drm_exec exec;
> +
> +			nouveau_uvmm_lock_vm_and_obj(uvmm, &exec, obj);
>  			drm_gpuvm_bo_put(op->vm_bo);
> -			dma_resv_unlock(obj->resv);
> +			drm_exec_fini(&exec);
>  		}
>  
>  		if (obj)
> @@ -1941,7 +1974,8 @@ nouveau_uvmm_ioctl_vm_init(struct drm_device *dev,
>  	mt_init_flags(&uvmm->region_mt, MT_FLAGS_LOCK_EXTERN);
>  	mt_set_external_lock(&uvmm->region_mt, &uvmm->mutex);
>  
> -	drm_gpuvm_init(&uvmm->base, cli->name, 0, drm, r_obj,
> +	drm_gpuvm_init(&uvmm->base, cli->name, DRM_GPUVM_RESV_PROTECTED,
> +		       drm, r_obj,
>  		       NOUVEAU_VA_SPACE_START,
>  		       NOUVEAU_VA_SPACE_END,
>  		       init->kernel_managed_addr,
> @@ -1978,6 +2012,7 @@ nouveau_uvmm_fini(struct nouveau_uvmm *uvmm)
>  	struct nouveau_uvma_region *reg;
>  	struct nouveau_cli *cli = uvmm->vmm.cli;
>  	struct drm_gpuva *va, *next;
> +	struct drm_exec exec;
>  
>  	nouveau_uvmm_lock(uvmm);
>  	drm_gpuvm_for_each_va_safe(va, next, &uvmm->base) {
> @@ -1989,9 +2024,9 @@ nouveau_uvmm_fini(struct nouveau_uvmm *uvmm)
>  
>  		drm_gpuva_remove(va);
>  
> -		dma_resv_lock(obj->resv, NULL);
> +		nouveau_uvmm_lock_vm_and_obj(uvmm, &exec, obj);
>  		drm_gpuva_unlink(va);
> -		dma_resv_unlock(obj->resv);
> +		drm_exec_fini(&exec);
>  
>  		nouveau_uvma_unmap(uvma);
>  		nouveau_uvma_vmm_put(uvma);
> -- 
> 2.34.1
> 

Re: [PATCH v3 7/8] drm/nouveau: use DRM_GPUVM_RESV_PROTECTED

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/nouveau: silently ignored lock failure causes unprotected list access
--

commit 8fab8734d6eb64d1f1dd2d598d76c6f17c6efddd
Author: Matthew Brost <matthew.brost@intel.com>
Subject: drm/nouveau: use DRM_GPUVM_RESV_PROTECTED

This commit sets DRM_GPUVM_RESV_PROTECTED for the nouveau drm_gpuvm. It
introduces nouveau_uvmm_lock_vm_and_obj() to lock the VM's dma-resv together
with the object's dma-resv when dropping a reference to a drm_gpuvm_bo.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index fc125fd44a9b7..13e11905b5f97 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
[ ... ]
> @@ -1184,6 +1184,27 @@ bind_validate_region(struct nouveau_job *job)
>  	return 0;
>  }
>  
> +/*
> + * Lock the VM's common dma-resv together with the one of @obj, as needed to
> + * drop what may be the last reference of a &drm_gpuvm_bo.
> + */
> +static void
> +nouveau_uvmm_lock_vm_and_obj(struct nouveau_uvmm *uvmm, struct drm_exec *exec,
> +			     struct drm_gem_object *obj)
> +{
> +	int ret;
> +
> +	drm_exec_init(exec, DRM_EXEC_IGNORE_DUPLICATES, 2);
> +	drm_exec_until_all_locked(exec) {
> +		ret = drm_exec_lock_obj(exec, drm_gpuvm_resv_obj(&uvmm->base));
> +		if (!ret)
> +			ret = drm_exec_lock_obj(exec, obj);
> +		drm_exec_retry_on_contention(exec);
> +		if (drm_WARN_ON(uvmm->base.drm, ret))
> +			break;

[Severity: High]
If drm_exec_lock_obj() fails with an error like -ENOMEM (which can occur
during internal memory allocations), the loop hits this break and returns.

Does this mean the function returns without fully acquiring the necessary
locks, while hiding the failure from the caller because of the void return
type?

> +	}
> +}
[ ... ]
> @@ -1594,9 +1625,11 @@ nouveau_uvmm_bind_job_cleanup(struct nouveau_job *job)
>  			drm_gpuva_ops_free(&uvmm->base, op->ops);
>  
>  		if (!IS_ERR_OR_NULL(op->vm_bo)) {
> -			dma_resv_lock(obj->resv, NULL);
> +			struct drm_exec exec;
> +
> +			nouveau_uvmm_lock_vm_and_obj(uvmm, &exec, obj);
>  			drm_gpuvm_bo_put(op->vm_bo);

[Severity: High]
If nouveau_uvmm_lock_vm_and_obj() fails and returns without holding the
dma_resv locks, will calling drm_gpuvm_bo_put() here result in unprotected
concurrent list operations?

The caller assumes the locks are securely held. Functions like
drm_gpuvm_bo_put() can modify obj->gpuva.list and gpuvm->extobj.list.

Under memory pressure on a shared GEM object, could this lead to list
corruption or a use-after-free?

> -			dma_resv_unlock(obj->resv);
> +			drm_exec_fini(&exec);
>  		}
>  
>  		if (obj)
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001220632.3190896-1-matthew.brost@intel.com?part=7

Re: [PATCH v3 7/8] drm/nouveau: use DRM_GPUVM_RESV_PROTECTED

[Matthew Brost]

On Thu, Oct 01, 2026 at 05:06:30PM -0700, Matthew Brost wrote:
> On Thu, Oct 01, 2026 at 03:06:31PM -0700, Matthew Brost wrote:
> > nouveau creates its drm_gpuvm without DRM_GPUVM_RESV_PROTECTED, so the
> > extobj and evicted lists are protected by internal spinlocks. Every
> > nouveau path but three already touches them with the VM's resv held:
> > drm_gpuvm_exec_lock() and drm_gpuvm_validate() on exec, and TTM moves of
> > private BOs, which share the VM's resv. The remaining three are:
> > 
> >  - nouveau_uvmm_bind_job_submit() adds the vm_bo of a MAP op to the
> >    extobj list holding no resv at all. Move that into
> >    bind_lock_validate(), which now locks the VM's resv as well.
> > 
> >  - bind_link_gpuvas() unlinks the GPUVAs of UNMAP and REMAP ops, which
> >    can drop the last reference of their vm_bo. It runs within the bind
> >    job's drm_exec transaction, which the above makes hold the VM's resv.
> > 
> >  - nouveau_uvmm_bind_job_cleanup() and nouveau_uvmm_fini() can drop the
> >    last reference of a vm_bo holding only the object's resv. Lock the
> >    VM's resv along with it, through a new
> >    nouveau_uvmm_lock_vm_and_obj().
> > 
> > The bind job's fence now always lands in the VM's resv, as BOOKKEEP.
> > This already happened whenever a bind job mapped a private BO.
> > 
> > With that the internal spinlocks buy nothing, so set
> > DRM_GPUVM_RESV_PROTECTED. This is also what two-pass locking in
> > drm_gpuvm requires, which a following patch makes use of.
> > 
> > Cc: Abhinav Kumar <abhinav.kumar@linux.dev>
> > Cc: Alice Ryhl <aliceryhl@google.com>
> > Cc: Anna Maniscalco <anna.maniscalco2000@gmail.com>
> > Cc: Antonino Maniscalco <antomani103@gmail.com>
> > Cc: Boris Brezillon <boris.brezillon@collabora.com>
> > Cc: Danilo Krummrich <dakr@kernel.org>
> > Cc: David Airlie <airlied@gmail.com>
> > Cc: Dmitry Baryshkov <lumag@kernel.org>
> > Cc: Jessica Zhang <jesszhan0024@gmail.com>
> > Cc: Jonathan Corbet <corbet@lwn.net>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Marijn Suijten <marijn.suijten@somainline.org>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Randy Dunlap <rdunlap@infradead.org>
> > Cc: Rob Clark <robin.clark@oss.qualcomm.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: Shuah Khan <skhan@linuxfoundation.org>
> > Cc: Simona Vetter <simona@ffwll.ch>
> > Cc: Steven Price <steven.price@arm.com>
> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > Assisted-by: LLM
> > ---
> > v3:
> >  - New patch (Danilo)
> > ---
> >  drivers/gpu/drm/nouveau/nouveau_uvmm.c | 49 ++++++++++++++++++++++----
> >  1 file changed, 42 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > index 2026fe6b48c6..dae612e56d91 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > @@ -1187,6 +1187,27 @@ bind_validate_region(struct nouveau_job *job)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Lock the VM's common dma-resv together with the one of @obj, as needed to
> > + * drop what may be the last reference of a &drm_gpuvm_bo.
> > + */
> > +static void
> > +nouveau_uvmm_lock_vm_and_obj(struct nouveau_uvmm *uvmm, struct drm_exec *exec,
> > +			     struct drm_gem_object *obj)
> > +{
> > +	int ret;
> > +
> > +	drm_exec_init(exec, DRM_EXEC_IGNORE_DUPLICATES, 2);
> > +	drm_exec_until_all_locked(exec) {
> > +		ret = drm_exec_lock_obj(exec, drm_gpuvm_resv_obj(&uvmm->base));
> > +		if (!ret)
> > +			ret = drm_exec_lock_obj(exec, obj);
> > +		drm_exec_retry_on_contention(exec);
> > +		if (drm_WARN_ON(uvmm->base.drm, ret))
> 
> Sashiko flagged that -ENOMEM is a possible return from drm_exec_lock_obj
> if a kmalloc fails and that is correct.
> 
> My plan is to change this and open coded WW transaction which can't
> fail.
> 
> Not going to post now as I don't want to spam the list.
> 

Or this patch merges - then is a moot point:
https://patchwork.freedesktop.org/series/175390/

Matt

> Matt
> 
> > +			break;
> > +	}
> > +}
> > +
> >  static void
> >  bind_link_gpuvas(struct bind_job_op *bop)
> >  {
> > @@ -1224,12 +1245,24 @@ bind_lock_validate(struct nouveau_job *job, struct drm_exec *exec,
> >  		   unsigned int num_fences)
> >  {
> >  	struct nouveau_uvmm_bind_job *bind_job = to_uvmm_bind_job(job);
> > +	struct nouveau_uvmm *uvmm = nouveau_cli_uvmm(job->cli);
> >  	struct bind_job_op *op;
> >  	int ret;
> >  
> > +	/* The VM's dma-resv protects its extobj and evicted lists, and must be
> > +	 * held by bind_link_gpuvas() in case it drops the last reference of a
> > +	 * &drm_gpuvm_bo.
> > +	 */
> > +	ret = drm_gpuvm_prepare_vm(&uvmm->base, exec, num_fences);
> > +	if (ret)
> > +		return ret;
> > +
> >  	list_for_each_op(op, &bind_job->ops) {
> >  		struct drm_gpuva_op *va_op;
> >  
> > +		if (op->op == OP_MAP)
> > +			drm_gpuvm_bo_extobj_add(op->vm_bo);
> > +
> >  		if (!op->ops)
> >  			continue;
> >  
> > @@ -1288,8 +1321,6 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job,
> >  			dma_resv_unlock(obj->resv);
> >  			if (IS_ERR(op->vm_bo))
> >  				return PTR_ERR(op->vm_bo);
> > -
> > -			drm_gpuvm_bo_extobj_add(op->vm_bo);
> >  		}
> >  
> >  		ret = bind_validate_op(job, op);
> > @@ -1603,9 +1634,11 @@ nouveau_uvmm_bind_job_cleanup(struct nouveau_job *job)
> >  			drm_gpuva_ops_free(&uvmm->base, op->ops);
> >  
> >  		if (!IS_ERR_OR_NULL(op->vm_bo)) {
> > -			dma_resv_lock(obj->resv, NULL);
> > +			struct drm_exec exec;
> > +
> > +			nouveau_uvmm_lock_vm_and_obj(uvmm, &exec, obj);
> >  			drm_gpuvm_bo_put(op->vm_bo);
> > -			dma_resv_unlock(obj->resv);
> > +			drm_exec_fini(&exec);
> >  		}
> >  
> >  		if (obj)
> > @@ -1941,7 +1974,8 @@ nouveau_uvmm_ioctl_vm_init(struct drm_device *dev,
> >  	mt_init_flags(&uvmm->region_mt, MT_FLAGS_LOCK_EXTERN);
> >  	mt_set_external_lock(&uvmm->region_mt, &uvmm->mutex);
> >  
> > -	drm_gpuvm_init(&uvmm->base, cli->name, 0, drm, r_obj,
> > +	drm_gpuvm_init(&uvmm->base, cli->name, DRM_GPUVM_RESV_PROTECTED,
> > +		       drm, r_obj,
> >  		       NOUVEAU_VA_SPACE_START,
> >  		       NOUVEAU_VA_SPACE_END,
> >  		       init->kernel_managed_addr,
> > @@ -1978,6 +2012,7 @@ nouveau_uvmm_fini(struct nouveau_uvmm *uvmm)
> >  	struct nouveau_uvma_region *reg;
> >  	struct nouveau_cli *cli = uvmm->vmm.cli;
> >  	struct drm_gpuva *va, *next;
> > +	struct drm_exec exec;
> >  
> >  	nouveau_uvmm_lock(uvmm);
> >  	drm_gpuvm_for_each_va_safe(va, next, &uvmm->base) {
> > @@ -1989,9 +2024,9 @@ nouveau_uvmm_fini(struct nouveau_uvmm *uvmm)
> >  
> >  		drm_gpuva_remove(va);
> >  
> > -		dma_resv_lock(obj->resv, NULL);
> > +		nouveau_uvmm_lock_vm_and_obj(uvmm, &exec, obj);
> >  		drm_gpuva_unlink(va);
> > -		dma_resv_unlock(obj->resv);
> > +		drm_exec_fini(&exec);
> >  
> >  		nouveau_uvma_unmap(uvma);
> >  		nouveau_uvma_vmm_put(uvma);
> > -- 
> > 2.34.1
> > 

Re: [PATCH v3 1/8] drm/gpuvm: allow locking external objects in two passes

[Anna Maniscalco]

On 10/2/26 12:06 AM, Matthew Brost wrote:
> Today a driver locks every drm_gem_object its drm_gpuvm has mappings of
> in a single drm_exec transaction, validates and rebinds whatever needs
> it, submits and unlocks. The dma-resv lock of every mapped object is
> therefore held for as long as the slowest validation in the transaction
> takes.
>
> That is fine as long as the objects are private, but it is not fine for
> objects shared between processes. The processes are of course related,
> that is why they share a buffer, but the work which holds the lock is
> not: a client stalls the compositor it is presenting to while it faults
> in or migrates some large buffer of its own, one the compositor has no
> interest in and will never touch. The deadline is missed because of a
> non overlapping set of objects.
>
> Keeping the shared, about to be presented buffers resident is a
> different problem, and one which already has its own answers: driver
> side eviction heuristics which decline to evict shared objects, or a
> compositor whose allocations simply outrank everyone else's. This is
> what is left even once those work. The objects being held hostage are
> not ones the transaction has anything slow to do on. Only the evicted
> objects need validating; the rest are already resident, and holding
> their dma-resv while some other object is migrated buys nothing. They
> are worked on in the end, of course, having a fence attached and their
> mappings rebound, but none of that has to wait on a migration.
>
> So let a driver set drm_gpuvm_exec::two_pass and have
> drm_gpuvm_exec_lock() acquire its locks in two steps:
>
>    DRM_GPUVM_EXEC_PASS_EARLY validates what the transaction already
>    holds. The GPUVM's own dma-resv is locked from the start, so that is
>    every private object, and the driver validates the evicted ones. The
>    pass also opportunistically locks the external objects which are
>    evicted, since those need validating anyway, and the driver validates
>    those too. The resident external objects are left unlocked, then
>
>    DRM_GPUVM_EXEC_PASS_LATE locks everything else, i.e. exactly what the
>    early pass left out. The driver validates anything which raced with
>    the early pass and does whatever needs every lock held, such as
>    attaching its job's fence.
>
> Both passes share one drm_exec transaction. The early pass keeps
> everything it locked and the late pass only ever adds to it, so there is
> no window in which another thread can undo the early pass' work, and no
> recheck or retry logic is needed. The extra.fn callback is invoked once
> per pass, with drm_gpuvm_exec::pass telling it which one it is in.
>
> Only the external objects are divided up like this. The passes have to
> agree on which objects belong to which, and the GPUVM's common dma-resv
> is what gives that, so it is held throughout.
>
> Since the early pass leaves the objects it did not lock alone, they are
> the late pass' problem, and drm_gpuvm_exec_pass_validate() must skip
> them or it would validate an object the transaction does not hold the
> dma-resv of. Such an object can be sitting on the evicted list while
> that happens, which is why drm_gpuvm_exec_pass_has_evicted() exists: a
> driver looping until nothing is evicted would otherwise spin in the
> early pass on an object that pass is never going to validate. It keys
> off drm_gpuvm_bo::lock_skipped, which the early pass latches, rather
> than off drm_gpuvm_bo::evicted, which can change at any time. The late
> pass consumes that same latched value to derive what to prepare, which
> is what keeps the two passes an exact partition even if an object is
> evicted in between; re-preparing an object the transaction already holds
> would fail with -EALREADY.
>
> The early pass reads drm_gpuvm_bo::evicted without holding the object's
> dma-resv, that being the lock it is trying not to take. The race is
> benign: an object evicted just after being skipped is validated by the
> late pass instead, exactly as if it had been evicted a moment later
> still.
>
> Two pass locking requires a DRM_GPUVM_RESV_PROTECTED drm_gpuvm. The late
> pass has to prepare precisely the complement of what the early pass
> locked, and only the GPUVM's common dma-resv, which the transaction
> holds across both passes, keeps the external object list from changing
> underneath.
>
> Splitting the locking only pays off when there is validation to keep the
> resident objects unlocked for. With nothing evicted it would just walk
> the external object list a second time for nothing, so
> drm_gpuvm_exec_pass_needs_split() is consulted once the VM resv is held
> and the sequence collapses back to a single pass when it says no. That
> check is advisory: both answers are functionally correct, so a stale one
> only costs the optimization.
>
> Making it O(1) needs a count of the evicted external objects, since
> those are the ones drm_gpuvm_bo_evict() cannot put on the evicted list,
> lacking the GPUVM's common dma-resv to do it under. The object's own
> dma-resv is held there, so the transition is stable and an atomic_t is
> enough. It is decremented again wherever a drm_gpuvm_bo is taken off the
> lists, including the deferred cleanup path used by immediate mode.
>
> DRM_GPUVM_EXEC_PASS_ALL is zero and two_pass defaults to false, so a
> zero initialised drm_gpuvm_exec and the existing
> drm_gpuvm_prepare_objects() and drm_gpuvm_validate() keep the
> traditional behaviour, and no existing driver changes behaviour.
Thanks for working on solving this problem, this looks cleaner than what 
I had in my presentation!
>
> Cc: Abhinav Kumar <abhinav.kumar@linux.dev>
> Cc: Alice Ryhl <aliceryhl@google.com>
> Cc: Anna Maniscalco <anna.maniscalco2000@gmail.com>
> Cc: Antonino Maniscalco <antomani103@gmail.com>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Dmitry Baryshkov <lumag@kernel.org>
> Cc: Jessica Zhang <jesszhan0024@gmail.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Marijn Suijten <marijn.suijten@somainline.org>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Rob Clark <robin.clark@oss.qualcomm.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Shuah Khan <skhan@linuxfoundation.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Assisted-by: LLM
> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> ---
> v3:
>   - Pair the lockless READ_ONCE() of drm_gpuvm_bo::evicted with a
>     WRITE_ONCE() in drm_gpuvm_bo_evict() and document why it is safe
>     (Danilo)
>   - Rename the DOC section to "GPUVM EXEC two-pass locking" and describe
>     it in terms of preparing and validating objects (Danilo)
>   - Use drm_WARN_ON_ONCE() (Danilo)
>   - Consolidate the DRM_GPUVM_RESV_PROTECTED checks into
>     drm_gpuvm_exec_pass_supported(); drm_gpuvm_exec_lock() now warns and
>     falls back to a single pass rather than returning -EOPNOTSUPP (Danilo)
>   - Use the drm_gpuvm_exec_pass_ prefix for all new functions (Danilo)
> ---
>   Documentation/gpu/drm-mm.rst |   6 +
>   drivers/gpu/drm/drm_gpuvm.c  | 505 +++++++++++++++++++++++++++++++++--
>   include/drm/drm_gpuvm.h      | 139 +++++++++-
>   3 files changed, 629 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
> index 2dea94f77d52..7eca0966448d 100644
> --- a/Documentation/gpu/drm-mm.rst
> +++ b/Documentation/gpu/drm-mm.rst
> @@ -510,6 +510,12 @@ Locking
>   .. kernel-doc:: drivers/gpu/drm/drm_gpuvm.c
>      :doc: Locking
>   
> +GPUVM EXEC two-pass locking
> +---------------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_gpuvm.c
> +   :doc: GPUVM EXEC two-pass locking
> +
>   Examples
>   --------
>   
> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> index d1c80ad3dead..6eaa4aef8093 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -527,6 +527,93 @@
>    * hence do not require internal locking.
>    */
>   
> +/**
> + * DOC: GPUVM EXEC two-pass locking
> + *
> + * This is about how the &drm_gem_objects a &drm_gpuvm has mappings of are
> + * prepared, i.e. have their dma-resv locked and fence slots reserved, through
> + * drm_gpuvm_exec_lock() or drm_gpuvm_prepare_objects(), and how the evicted
> + * ones among them are validated, through drm_gpuvm_validate(), within that
> + * same &drm_exec transaction.
> + *
> + * By default a driver prepares every &drm_gem_object a &drm_gpuvm has
> + * mappings of in a single &drm_exec transaction, validates and rebinds
> + * whatever needs it, submits its job and unlocks. That is simple and correct,
> + * but it means the dma-resv lock of every mapped object is held for as long
> + * as the validation of the slowest object takes.
> + *
> + * That is a problem when some of those objects are shared with other
> + * processes, for example the buffers a compositor is about to present. The
> + * processes are of course related, that is why they share a buffer, but the
> + * work holding the lock is not: a client stalls the compositor it presents
> + * to while faulting in or migrating some large buffer of its own, one the
> + * compositor will never touch.
> + *
> + * Keeping the shared buffers themselves resident is a separate problem with
> + * separate answers, such as driver side heuristics which decline to evict
> + * shared objects, or simply a compositor whose allocations outrank everyone
> + * else's. What is left even once they work is this: a resident shared object
> + * needs no validating, yet its dma-resv is held for the duration of the
> + * validation of unrelated objects.
> + *
> + * The observation which fixes that is that only the evicted objects need any
> + * work done on them. Everything else is already resident, so holding its
> + * dma-resv throughout buys nothing. A driver can therefore set
> + * &drm_gpuvm_exec.two_pass and have the single &drm_exec transaction acquire
> + * its locks in two steps:
> + *
> + * 1) %DRM_GPUVM_EXEC_PASS_EARLY validates what the transaction already
> + *    holds. The &drm_gpuvm's own dma-resv is locked from the start, so that
> + *    is every private object, and the driver validates the evicted ones from
> + *    its &drm_gpuvm_exec.extra callback. On top of that the pass
> + *    opportunistically locks the external objects which are evicted, since
> + *    those have to be validated anyway, and the callback validates them too.
> + *    The resident external objects are left unlocked.
> + *
> + * 2) %DRM_GPUVM_EXEC_PASS_LATE locks everything else, i.e. exactly what the
> + *    early pass left out. The callback runs again, now able to touch every
> + *    mapped object, and validates anything which raced with the early pass
> + *    before the driver submits.
> + *
> + * The important part is what does not happen in between: the early pass keeps
> + * everything it locked, so there is no window in which another thread can
> + * undo its work, and no recheck or retry logic is needed. The resident
> + * objects are simply locked last, once the expensive work is already done.
> + *
> + * The late pass can still find something to validate, since an object it had
> + * not locked yet may have been evicted while the early pass was running. That
> + * is handled the way it is today, by validating it with every lock held; it
> + * is just no longer the common case.
> + *
> + * Setting &drm_gpuvm_exec.two_pass only asks for two passes, it does not
> + * force them. Splitting the transaction is pointless when nothing is evicted,
> + * as the early pass would lock nothing and validate nothing, so
> + * drm_gpuvm_exec_lock() consults drm_gpuvm_exec_pass_needs_split() once the
> + * GPUVM's dma-resv is held and falls back to a single
> + * %DRM_GPUVM_EXEC_PASS_ALL pass if there is no work for an early pass to do.
> + * Drivers which drive the passes themselves rather than through
> + * drm_gpuvm_exec_lock() should do the same.
> + *
> + * That check is advisory. An object may be evicted right after it answers
> + * false, in which case the single pass validates it with every lock held,
> + * exactly as it would have without two-pass locking. Both answers are always
> + * correct; a stale one only costs the optimization.
> + *
> + * drm_gpuvm_exec_pass_validate() must be used instead of
> + * drm_gpuvm_validate(), so that an object the early pass did not lock is not
> + * validated behind its dma-resv lock's back. Such an object is simply left to
> + * the late pass, which does hold it. It can still be sitting on the evicted
> + * list while that happens, so the usual "loop until nothing is evicted"
> + * termination condition must use drm_gpuvm_exec_pass_has_evicted() rather
> + * than a plain emptiness test on the evicted list, or the early pass would
> + * spin on an object it is never going to validate.
> + *
> + * Two-pass locking requires a %DRM_GPUVM_RESV_PROTECTED &drm_gpuvm. The late
> + * pass has to prepare precisely the complement of what the early pass locked,
> + * and only the GPUVM's common dma-resv, which the transaction holds across
> + * both passes, keeps the external object list from changing underneath.
> + */
> +
>   /**
>    * DOC: Examples
>    *
> @@ -1104,6 +1191,7 @@ drm_gpuvm_init(struct drm_gpuvm *gpuvm, const char *name,
>   
>   	INIT_LIST_HEAD(&gpuvm->extobj.list);
>   	spin_lock_init(&gpuvm->extobj.lock);
> +	atomic_set(&gpuvm->extobj.num_evicted, 0);
>   
>   	INIT_LIST_HEAD(&gpuvm->evict.list);
>   	spin_lock_init(&gpuvm->evict.lock);
> @@ -1220,16 +1308,105 @@ drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
>   }
>   EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_vm);
>   
> +/*
> + * Everything two-pass locking keys off is protected by the GPUVM's common
> + * dma-resv: the external object list not changing between the passes, and
> + * &drm_gpuvm_bo.lock_skipped being latched by one pass and consumed by the
> + * next. That lock is held for the whole &drm_exec transaction, so assert it
> + * wherever a pass is acted upon.
> + *
> + * %DRM_GPUVM_EXEC_PASS_ALL is exempt. It is the single pass behaviour which
> + * predates this, and !%DRM_GPUVM_RESV_PROTECTED drivers legitimately reach it
> + * without holding the common dma-resv, using the internal spinlocks instead.
> + */
> +#ifdef CONFIG_LOCKDEP
> +static void
> +drm_gpuvm_pass_assert_held(struct drm_gpuvm *gpuvm,
> +			   enum drm_gpuvm_exec_pass pass)
> +{
> +	if (pass != DRM_GPUVM_EXEC_PASS_ALL)
> +		drm_gpuvm_resv_assert_held(gpuvm);
> +}
> +#else
> +static void
> +drm_gpuvm_pass_assert_held(struct drm_gpuvm *gpuvm,
> +			   enum drm_gpuvm_exec_pass pass)
> +{
> +}
> +#endif
> +
> +/*
> + * Two-pass locking requires a %DRM_GPUVM_RESV_PROTECTED &drm_gpuvm, see
> + * "GPUVM EXEC two-pass locking". This is the one place checking for it, so
> + * that it can simply go away once every &drm_gpuvm is.
> + *
> + * Returns: true if @pass can be used with @gpuvm. %DRM_GPUVM_EXEC_PASS_ALL
> + * always can.
> + */
> +static bool
> +drm_gpuvm_exec_pass_supported(struct drm_gpuvm *gpuvm,
> +			      enum drm_gpuvm_exec_pass pass)
> +{
> +	if (pass == DRM_GPUVM_EXEC_PASS_ALL)
> +		return true;
> +
> +	return !drm_WARN_ON_ONCE(gpuvm->drm, !drm_gpuvm_resv_protected(gpuvm));
> +}
> +
> +/*
> + * Decide whether @pass prepares @vm_bo, and maintain @vm_bo->lock_skipped,
> + * which records whether the transaction is missing this object's dma-resv.
> + *
> + * The early pass latches its decision there so that
> + * drm_gpuvm_exec_pass_validate() keys off a value which cannot change under
> + * it, even though @vm_bo->evicted can. The late pass then consumes that
> + * latched value rather than re-reading @vm_bo->evicted, which is what makes
> + * it prepare exactly the objects the early pass left out, no more and no less.
> + */
> +static bool
> +drm_gpuvm_prepare_skip(struct drm_gpuvm_bo *vm_bo,
> +		       enum drm_gpuvm_exec_pass pass)
> +{
> +	drm_gpuvm_pass_assert_held(vm_bo->vm, pass);
> +
> +	switch (pass) {
> +	case DRM_GPUVM_EXEC_PASS_EARLY:
> +		/*
> +		 * Lockless hint, pairs with WRITE_ONCE() in drm_gpuvm_bo_evict().
> +		 * A stale value is harmless: the decision is latched in lock_skipped
> +		 * and anything acting on evicted re-reads it under the object's resv.
> +		 */
> +		vm_bo->lock_skipped = !READ_ONCE(vm_bo->evicted);
> +		break;
> +	case DRM_GPUVM_EXEC_PASS_LATE:
> +		/* Already locked by the early pass, must not lock it twice. */
> +		if (!vm_bo->lock_skipped)
> +			return true;
> +
> +		vm_bo->lock_skipped = false;
> +		break;
> +	case DRM_GPUVM_EXEC_PASS_ALL:
> +		vm_bo->lock_skipped = false;
> +		break;
> +	}
> +
> +	return vm_bo->lock_skipped;
> +}
> +
>   static int
>   __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
>   			    struct drm_exec *exec,
> -			    unsigned int num_fences)
> +			    unsigned int num_fences,
> +			    enum drm_gpuvm_exec_pass pass)
>   {
>   	struct drm_gpuvm_bo *vm_bo;
>   	LIST_HEAD(extobjs);
>   	int ret = 0;
>   
>   	for_each_vm_bo_in_list(gpuvm, extobj, &extobjs, vm_bo) {
> +		if (drm_gpuvm_prepare_skip(vm_bo, pass))
> +			continue;
> +
>   		ret = exec_prepare_obj(exec, vm_bo->obj, num_fences);
>   		if (ret)
>   			break;
> @@ -1244,7 +1421,8 @@ __drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
>   static int
>   drm_gpuvm_prepare_objects_locked(struct drm_gpuvm *gpuvm,
>   				 struct drm_exec *exec,
> -				 unsigned int num_fences)
> +				 unsigned int num_fences,
> +				 enum drm_gpuvm_exec_pass pass)
>   {
>   	struct drm_gpuvm_bo *vm_bo;
>   	int ret = 0;
> @@ -1254,6 +1432,9 @@ drm_gpuvm_prepare_objects_locked(struct drm_gpuvm *gpuvm,
>   		if (drm_gpuvm_bo_is_zombie(vm_bo))
>   			continue;
>   
> +		if (drm_gpuvm_prepare_skip(vm_bo, pass))
> +			continue;
> +
>   		ret = exec_prepare_obj(exec, vm_bo->obj, num_fences);
>   		if (ret)
>   			break;
> @@ -1293,13 +1474,54 @@ drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
>   			  struct drm_exec *exec,
>   			  unsigned int num_fences)
>   {
> +	return drm_gpuvm_exec_pass_prepare_objects(gpuvm, exec, num_fences,
> +						   DRM_GPUVM_EXEC_PASS_ALL);
> +}
> +EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_objects);
> +
> +/**
> + * drm_gpuvm_exec_pass_prepare_objects() - prepare the associated BOs of a pass
> + * @gpuvm: the &drm_gpuvm
> + * @exec: the &drm_exec locking context
> + * @num_fences: the amount of &dma_fences to reserve
> + * @pass: the &enum drm_gpuvm_exec_pass to prepare for
> + *
> + * Same as drm_gpuvm_prepare_objects(), except that @pass selects which
> + * external objects are prepared. With %DRM_GPUVM_EXEC_PASS_EARLY the resident
> + * &drm_gpuvm_bos are left alone, so that their dma-resv locks are only taken
> + * by the %DRM_GPUVM_EXEC_PASS_LATE call which follows in the same &drm_exec
> + * transaction.
> + *
> + * The two passes must be used as a pair and in that order, since the late one
> + * derives what to prepare from what the early one recorded.
> + *
> + * Anything other than %DRM_GPUVM_EXEC_PASS_ALL requires a
> + * %DRM_GPUVM_RESV_PROTECTED @gpuvm, whose common dma-resv, held across both
> + * passes, is what keeps the external object list stable between them.
> + *
> + * Returns: 0 on success, negative error code on failure, -EOPNOTSUPP if @pass
> + * is not %DRM_GPUVM_EXEC_PASS_ALL and @gpuvm is not
> + * %DRM_GPUVM_RESV_PROTECTED.
> + */
> +int
> +drm_gpuvm_exec_pass_prepare_objects(struct drm_gpuvm *gpuvm,
> +				    struct drm_exec *exec,
> +				    unsigned int num_fences,
> +				    enum drm_gpuvm_exec_pass pass)
> +{
> +	if (!drm_gpuvm_exec_pass_supported(gpuvm, pass))
> +		return -EOPNOTSUPP;
> +
> +	drm_gpuvm_pass_assert_held(gpuvm, pass);
> +
>   	if (drm_gpuvm_resv_protected(gpuvm))
>   		return drm_gpuvm_prepare_objects_locked(gpuvm, exec,
> -							num_fences);
> +							num_fences, pass);
>   	else
> -		return __drm_gpuvm_prepare_objects(gpuvm, exec, num_fences);
> +		return __drm_gpuvm_prepare_objects(gpuvm, exec, num_fences,
> +						   pass);
>   }
> -EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_objects);
> +EXPORT_SYMBOL_GPL(drm_gpuvm_exec_pass_prepare_objects);
>   
>   /**
>    * drm_gpuvm_prepare_range() - prepare all BOs mapped within a given range
> @@ -1338,6 +1560,32 @@ drm_gpuvm_prepare_range(struct drm_gpuvm *gpuvm, struct drm_exec *exec,
>   }
>   EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_range);
>   
> +/*
> + * Prepare the external objects belonging to @pass and let the driver do its
> + * per pass work. Contention is left for the caller to act on, since only it
> + * can restart the drm_exec transaction.
> + */
> +static int
> +drm_gpuvm_exec_do_pass(struct drm_gpuvm_exec *vm_exec,
> +		       enum drm_gpuvm_exec_pass pass)
> +{
> +	int ret;
> +
> +	drm_gpuvm_pass_assert_held(vm_exec->vm, pass);
> +
> +	vm_exec->pass = pass;
> +
> +	ret = drm_gpuvm_exec_pass_prepare_objects(vm_exec->vm, &vm_exec->exec,
> +						  vm_exec->num_fences, pass);
> +	if (ret)
> +		return ret;
> +
> +	if (vm_exec->extra.fn)
> +		return vm_exec->extra.fn(vm_exec);
> +
> +	return 0;
> +}
> +
>   /**
>    * drm_gpuvm_exec_lock() - lock all dma-resv of all associated BOs
>    * @vm_exec: the &drm_gpuvm_exec wrapper
> @@ -1350,6 +1598,24 @@ EXPORT_SYMBOL_GPL(drm_gpuvm_prepare_range);
>    * dma-resv in the context of the &drm_gpuvm_exec instance. Typically, drivers
>    * would call drm_exec_prepare_obj() from within this callback.
>    *
> + * If struct drm_gpuvm_exec::two_pass is set the locking may be split in two,
> + * see &enum drm_gpuvm_exec_pass, and @fn is called once per pass with struct
> + * drm_gpuvm_exec::pass telling it which one it is in. The split is skipped,
> + * and @fn called once with %DRM_GPUVM_EXEC_PASS_ALL, when there is nothing
> + * evicted for it to help with; see drm_gpuvm_exec_pass_needs_split(). A
> + * driver setting two_pass therefore has to handle all three passes. Such a
> + * callback has to be written with that in mind: preparing the same object in
> + * both passes fails with -EALREADY. Both passes share the one &drm_exec
> + * transaction, so the late pass only ever adds locks to what the early pass
> + * already holds. This requires a %DRM_GPUVM_RESV_PROTECTED &drm_gpuvm, whose
> + * common dma-resv keeps the external object list stable across the two
> + * passes; on any other two_pass warns and a single pass is used.
> + *
> + * Note that ww_mutex backoff can still restart the whole transaction from the
> + * early pass, in which case the contended object is locked up front and the
> + * early pass does run holding it. That is a rare fallback, not the common
> + * path it is trying to avoid.
> + *
>    * Returns: 0 on success, negative error code on failure.
>    */
>   int
> @@ -1368,17 +1634,22 @@ drm_gpuvm_exec_lock(struct drm_gpuvm_exec *vm_exec)
>   		if (ret)
>   			goto err;
>   
> -		ret = drm_gpuvm_prepare_objects(gpuvm, exec, num_fences);
> -		drm_exec_retry_on_contention(exec);
> -		if (ret)
> -			goto err;
> -
> -		if (vm_exec->extra.fn) {
> -			ret = vm_exec->extra.fn(vm_exec);
> +		if (vm_exec->two_pass && drm_gpuvm_exec_pass_needs_split(gpuvm)) {
> +			ret = drm_gpuvm_exec_do_pass(vm_exec,
> +						     DRM_GPUVM_EXEC_PASS_EARLY);
>   			drm_exec_retry_on_contention(exec);
>   			if (ret)
>   				goto err;
> +
> +			ret = drm_gpuvm_exec_do_pass(vm_exec,
> +						     DRM_GPUVM_EXEC_PASS_LATE);
> +		} else {
> +			ret = drm_gpuvm_exec_do_pass(vm_exec,
> +						     DRM_GPUVM_EXEC_PASS_ALL);
>   		}
> +		drm_exec_retry_on_contention(exec);
> +		if (ret)
> +			goto err;
>   	}
>   
>   	return 0;
> @@ -1410,6 +1681,12 @@ fn_lock_array(struct drm_gpuvm_exec *vm_exec)
>    * Acquires all dma-resv locks of all &drm_gem_objects the given &drm_gpuvm
>    * contains mappings of, plus the ones given through @objs.
>    *
> + * Two-pass locking is not supported here: @objs are not tracked by the
> + * &drm_gpuvm, so there is no way to tell which pass each of them belongs in.
> + * A driver wanting both has to open code this using
> + * drm_gpuvm_exec_lock() and a &drm_gpuvm_exec.extra callback which keys off
> + * &drm_gpuvm_exec.pass.
> + *
>    * Returns: 0 on success, negative error code on failure.
>    */
>   int
> @@ -1422,6 +1699,9 @@ drm_gpuvm_exec_lock_array(struct drm_gpuvm_exec *vm_exec,
>   		unsigned int num_objs;
>   	} args;
>   
> +	if (drm_WARN_ON_ONCE(vm_exec->vm->drm, vm_exec->two_pass))
> +		return -EOPNOTSUPP;
> +
>   	args.objs = objs;
>   	args.num_objs = num_objs;
>   
> @@ -1469,8 +1749,23 @@ drm_gpuvm_exec_lock_range(struct drm_gpuvm_exec *vm_exec,
>   }
>   EXPORT_SYMBOL_GPL(drm_gpuvm_exec_lock_range);
>   
> +/*
> + * An object the current pass deliberately did not lock must not be validated,
> + * it simply stays on the evicted list until a pass which does lock it comes
> + * along.
> + */
> +static bool
> +drm_gpuvm_validate_skip(struct drm_gpuvm_bo *vm_bo,
> +			enum drm_gpuvm_exec_pass pass)
> +{
> +	drm_gpuvm_pass_assert_held(vm_bo->vm, pass);
> +
> +	return pass != DRM_GPUVM_EXEC_PASS_ALL && vm_bo->lock_skipped;
> +}
> +
>   static int
> -__drm_gpuvm_validate(struct drm_gpuvm *gpuvm, struct drm_exec *exec)
> +__drm_gpuvm_validate(struct drm_gpuvm *gpuvm, struct drm_exec *exec,
> +		     enum drm_gpuvm_exec_pass pass)
>   {
>   	const struct drm_gpuvm_ops *ops = gpuvm->ops;
>   	struct drm_gpuvm_bo *vm_bo;
> @@ -1478,6 +1773,9 @@ __drm_gpuvm_validate(struct drm_gpuvm *gpuvm, struct drm_exec *exec)
>   	int ret = 0;
>   
>   	for_each_vm_bo_in_list(gpuvm, evict, &evict, vm_bo) {
> +		if (drm_gpuvm_validate_skip(vm_bo, pass))
> +			continue;
> +
>   		ret = ops->vm_bo_validate(vm_bo, exec);
>   		if (ret)
>   			break;
> @@ -1490,7 +1788,8 @@ __drm_gpuvm_validate(struct drm_gpuvm *gpuvm, struct drm_exec *exec)
>   }
>   
>   static int
> -drm_gpuvm_validate_locked(struct drm_gpuvm *gpuvm, struct drm_exec *exec)
> +drm_gpuvm_validate_locked(struct drm_gpuvm *gpuvm, struct drm_exec *exec,
> +			  enum drm_gpuvm_exec_pass pass)
>   {
>   	const struct drm_gpuvm_ops *ops = gpuvm->ops;
>   	struct drm_gpuvm_bo *vm_bo, *next;
> @@ -1503,6 +1802,9 @@ drm_gpuvm_validate_locked(struct drm_gpuvm *gpuvm, struct drm_exec *exec)
>   		if (drm_gpuvm_bo_is_zombie(vm_bo))
>   			continue;
>   
> +		if (drm_gpuvm_validate_skip(vm_bo, pass))
> +			continue;
> +
>   		ret = ops->vm_bo_validate(vm_bo, exec);
>   		if (ret)
>   			break;
> @@ -1527,18 +1829,146 @@ drm_gpuvm_validate_locked(struct drm_gpuvm *gpuvm, struct drm_exec *exec)
>    */
>   int
>   drm_gpuvm_validate(struct drm_gpuvm *gpuvm, struct drm_exec *exec)
> +{
> +	return drm_gpuvm_exec_pass_validate(gpuvm, exec, DRM_GPUVM_EXEC_PASS_ALL);
> +}
> +EXPORT_SYMBOL_GPL(drm_gpuvm_validate);
> +
> +/**
> + * drm_gpuvm_exec_pass_validate() - validate the BOs of a pass marked as evicted
> + * @gpuvm: the &drm_gpuvm to validate evicted BOs
> + * @exec: the &drm_exec instance used for locking the GPUVM
> + * @pass: the &enum drm_gpuvm_exec_pass being validated
> + *
> + * Same as drm_gpuvm_validate(), except that the &drm_gpuvm_bos which the
> + * matching drm_gpuvm_exec_pass_prepare_objects() call did not lock are left
> + * alone. They stay on the evicted list for the %DRM_GPUVM_EXEC_PASS_LATE
> + * pass, which locks them, to deal with.
> + *
> + * Anything other than %DRM_GPUVM_EXEC_PASS_ALL requires a
> + * %DRM_GPUVM_RESV_PROTECTED @gpuvm, as for
> + * drm_gpuvm_exec_pass_prepare_objects().
> + *
> + * Returns: 0 on success, negative error code on failure, -EOPNOTSUPP if @pass
> + * is not %DRM_GPUVM_EXEC_PASS_ALL and @gpuvm is not
> + * %DRM_GPUVM_RESV_PROTECTED.
> + */
> +int
> +drm_gpuvm_exec_pass_validate(struct drm_gpuvm *gpuvm, struct drm_exec *exec,
> +			     enum drm_gpuvm_exec_pass pass)
>   {
>   	const struct drm_gpuvm_ops *ops = gpuvm->ops;
>   
>   	if (unlikely(!ops || !ops->vm_bo_validate))
>   		return -EOPNOTSUPP;
>   
> +	if (!drm_gpuvm_exec_pass_supported(gpuvm, pass))
> +		return -EOPNOTSUPP;
> +
> +	drm_gpuvm_pass_assert_held(gpuvm, pass);
> +
>   	if (drm_gpuvm_resv_protected(gpuvm))
> -		return drm_gpuvm_validate_locked(gpuvm, exec);
> +		return drm_gpuvm_validate_locked(gpuvm, exec, pass);
>   	else
> -		return __drm_gpuvm_validate(gpuvm, exec);
> +		return __drm_gpuvm_validate(gpuvm, exec, pass);
>   }
> -EXPORT_SYMBOL_GPL(drm_gpuvm_validate);
> +EXPORT_SYMBOL_GPL(drm_gpuvm_exec_pass_validate);
> +
> +/**
> + * drm_gpuvm_exec_pass_needs_split() - whether splitting the locking is worth it
> + * @gpuvm: the &drm_gpuvm to query
> + *
> + * Two-pass locking only pays off when there is validation to be done, since
> + * the point of it is to keep the resident external objects unlocked while
> + * that runs. If nothing is evicted there is no such work, and the split would
> + * only walk the external object list a second time to no purpose.
> + *
> + * This is advisory and O(1). It does not hold the dma-resv of the external
> + * objects, so the answer can be stale by the time the caller acts on it, and
> + * a &drm_gpuvm_bo which is destroyed while evicted keeps it pessimistic
> + * until then. That is fine, because both answers are correct: a
> + * single pass behaves exactly as it did before two-pass locking existed, and
> + * a two-pass sequence with nothing evicted simply finds nothing to do in its
> + * early pass.
> + *
> + * Requires a %DRM_GPUVM_RESV_PROTECTED @gpuvm with its common dma-resv held,
> + * i.e. call it after drm_gpuvm_prepare_vm().
> + *
> + * Returns: true if the caller should use %DRM_GPUVM_EXEC_PASS_EARLY and
> + * %DRM_GPUVM_EXEC_PASS_LATE, false if it should use a single
> + * %DRM_GPUVM_EXEC_PASS_ALL.
> + */
> +bool
> +drm_gpuvm_exec_pass_needs_split(struct drm_gpuvm *gpuvm)
> +{
> +	drm_gpuvm_resv_assert_held(gpuvm);
> +
> +	if (!drm_gpuvm_exec_pass_supported(gpuvm, DRM_GPUVM_EXEC_PASS_EARLY))
> +		return false;
> +
> +	/*
> +	 * Evicted private objects are already on the evicted list, having the
> +	 * GPUVM's common dma-resv to be added under. Evicted external objects
> +	 * are not, drm_gpuvm_bo_evict() cannot put them there, so they are
> +	 * counted instead.
> +	 */
> +	return !list_empty(&gpuvm->evict.list) ||
> +	       atomic_read(&gpuvm->extobj.num_evicted);
> +}
> +EXPORT_SYMBOL_GPL(drm_gpuvm_exec_pass_needs_split);
So the idea is that this can return false positives but, since everything is
done under the gpuvm lock which is taken by the shrinker as well, then 
it should never
return false negatives? Then I wonder why it mentions that running 
single pass
when two-pass should run will still be correct, it is obviously true but are
there cases where it can happen?

Everything looks good otherwise so might be able to give my Rb.

> +
> +/**
> + * drm_gpuvm_exec_pass_has_evicted() - whether a pass has evicted BOs left
> + * @gpuvm: the &drm_gpuvm to query
> + * @pass: the &enum drm_gpuvm_exec_pass being validated
> + *
> + * Drivers typically loop over validation and rebinding until nothing is
> + * evicted anymore. Since drm_gpuvm_exec_pass_validate() leaves the objects it
> + * did not lock on the evicted list, %DRM_GPUVM_EXEC_PASS_EARLY must not use a
> + * plain emptiness test for that loop condition or it would never terminate.
> + * %DRM_GPUVM_EXEC_PASS_LATE holds every lock the transaction will ever hold,
> + * so it behaves like %DRM_GPUVM_EXEC_PASS_ALL here.
> + *
> + * This is more accurate than testing the evicted list for emptiness even for
> + * %DRM_GPUVM_EXEC_PASS_ALL, since zombie &drm_gpuvm_bos sit on that list
> + * without ever being validated.
> + *
> + * For a &DRM_GPUVM_RESV_PROTECTED GPUVM the caller must hold its common
> + * dma-resv lock, otherwise the evicted list's internal lock is taken. As
> + * everywhere else, anything other than %DRM_GPUVM_EXEC_PASS_ALL requires the
> + * former.
> + *
> + * Return: true if @pass still has an evicted &drm_gpuvm_bo to validate.
> + */
> +bool
> +drm_gpuvm_exec_pass_has_evicted(struct drm_gpuvm *gpuvm,
> +				enum drm_gpuvm_exec_pass pass)
> +{
> +	struct drm_gpuvm_bo *vm_bo;
> +	bool ret = false;
> +
> +	if (!drm_gpuvm_resv_protected(gpuvm))
> +		spin_lock(&gpuvm->evict.lock);
> +	else
> +		drm_gpuvm_resv_assert_held(gpuvm);
> +
> +	list_for_each_entry(vm_bo, &gpuvm->evict.list, list.entry.evict) {
> +		if (drm_gpuvm_bo_is_zombie(vm_bo))
> +			continue;
> +
> +		if (drm_gpuvm_validate_skip(vm_bo, pass))
> +			continue;
> +
> +		ret = true;
> +		break;
> +	}
> +
> +	if (!drm_gpuvm_resv_protected(gpuvm))
> +		spin_unlock(&gpuvm->evict.lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(drm_gpuvm_exec_pass_has_evicted);
>   
>   /**
>    * drm_gpuvm_resv_add_fence - add fence to private and all extobj
> @@ -1597,6 +2027,7 @@ drm_gpuvm_bo_create(struct drm_gpuvm *gpuvm,
>   	drm_gem_object_get(obj);
>   
>   	kref_init(&vm_bo->kref);
> +	vm_bo->lock_skipped = false;
>   	INIT_LIST_HEAD(&vm_bo->list.gpuva);
>   	INIT_LIST_HEAD(&vm_bo->list.entry.gem);
>   
> @@ -1644,6 +2075,21 @@ drm_gpuvm_bo_destroy_not_in_lists_kref(struct kref *kref)
>   	drm_gpuvm_bo_destroy_not_in_lists(vm_bo);
>   }
>   
> +/*
> + * Drop a &drm_gpuvm_bo out of &drm_gpuvm.extobj.num_evicted, which counts the
> + * evicted external objects for drm_gpuvm_exec_pass_needs_split(). Only those
> + * are counted, everything else being discoverable from the evicted list.
> + */
> +static void
> +drm_gpuvm_bo_uncount_evicted(struct drm_gpuvm_bo *vm_bo)
> +{
> +	struct drm_gpuvm *gpuvm = vm_bo->vm;
> +
> +	if (drm_gpuvm_resv_protected(gpuvm) && vm_bo->evicted &&
> +	    drm_gpuvm_is_extobj(gpuvm, vm_bo->obj))
> +		atomic_dec(&gpuvm->extobj.num_evicted);
> +}
> +
>   static void
>   drm_gpuvm_bo_destroy(struct kref *kref)
>   {
> @@ -1655,6 +2101,8 @@ drm_gpuvm_bo_destroy(struct kref *kref)
>   	if (!lock)
>   		drm_gpuvm_resv_assert_held(gpuvm);
>   
> +	drm_gpuvm_bo_uncount_evicted(vm_bo);
> +
>   	drm_gpuvm_bo_list_del(vm_bo, extobj, lock);
>   	drm_gpuvm_bo_list_del(vm_bo, evict, lock);
>   
> @@ -1789,6 +2237,7 @@ drm_gpuvm_bo_deferred_cleanup(struct drm_gpuvm *gpuvm)
>   	if (drm_gpuvm_resv_protected(gpuvm)) {
>   		dma_resv_lock(drm_gpuvm_resv(gpuvm), NULL);
>   		llist_for_each_entry(vm_bo, bo_defer, list.entry.bo_defer) {
> +			drm_gpuvm_bo_uncount_evicted(vm_bo);
>   			drm_gpuvm_bo_list_del(vm_bo, extobj, false);
>   			drm_gpuvm_bo_list_del(vm_bo, evict, false);
>   		}
> @@ -1959,6 +2408,11 @@ EXPORT_SYMBOL_GPL(drm_gpuvm_bo_extobj_add);
>    * @evict: indicates whether the object is evicted
>    *
>    * Adds a &drm_gpuvm_bo to or removes it from the &drm_gpuvm's evicted list.
> + *
> + * An external object of a %DRM_GPUVM_RESV_PROTECTED &drm_gpuvm is the
> + * exception: the evicted list is protected by the GPUVM's common dma-resv
> + * there, which this does not hold, so such an object is only accounted for
> + * and is put on the list later, by drm_gpuvm_prepare_objects().
>    */
>   void
>   drm_gpuvm_bo_evict(struct drm_gpuvm_bo *vm_bo, bool evict)
> @@ -1966,16 +2420,29 @@ drm_gpuvm_bo_evict(struct drm_gpuvm_bo *vm_bo, bool evict)
>   	struct drm_gpuvm *gpuvm = vm_bo->vm;
>   	struct drm_gem_object *obj = vm_bo->obj;
>   	bool lock = !drm_gpuvm_resv_protected(gpuvm);
> +	bool was_evicted = vm_bo->evicted;
>   
>   	dma_resv_assert_held(obj->resv);
> -	vm_bo->evicted = evict;
> +	/*
> +	 * Pairs with the READ_ONCE() in drm_gpuvm_prepare_skip(), which reads
> +	 * this without the object's dma-resv held.
> +	 */
> +	WRITE_ONCE(vm_bo->evicted, evict);
>   
>   	/* Can't add external objects to the evicted list directly if not using
>   	 * internal spinlocks, since in this case the evicted list is protected
>   	 * with the VM's common dma-resv lock.
>   	 */
> -	if (drm_gpuvm_is_extobj(gpuvm, obj) && !lock)
> +	if (drm_gpuvm_is_extobj(gpuvm, obj) && !lock) {
> +		/*
> +		 * Count them instead, so drm_gpuvm_exec_pass_needs_split() can tell
> +		 * whether any are evicted without walking the list. The
> +		 * object's dma-resv is held, so the transition is stable.
> +		 */
> +		if (evict != was_evicted)
> +			atomic_add(evict ? 1 : -1, &gpuvm->extobj.num_evicted);
>   		return;
> +	}
>   
>   	if (evict)
>   		drm_gpuvm_bo_list_add(vm_bo, evict, lock);
> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
> index 38221d83285b..33a69d771eee 100644
> --- a/include/drm/drm_gpuvm.h
> +++ b/include/drm/drm_gpuvm.h
> @@ -308,6 +308,18 @@ struct drm_gpuvm {
>   		 * @extobj.lock: spinlock to protect the extobj list
>   		 */
>   		spinlock_t lock;
> +
> +		/**
> +		 * @extobj.num_evicted: number of entries of the extobj list
> +		 * which are evicted.
> +		 *
> +		 * Only maintained for a %DRM_GPUVM_RESV_PROTECTED &drm_gpuvm,
> +		 * where an evicted external object cannot be put on the
> +		 * evicted list, and only so that drm_gpuvm_exec_pass_needs_split()
> +		 * does not have to walk the list to find out. Never used to
> +		 * decide anything that has to be exact.
> +		 */
> +		atomic_t num_evicted;
>   	} extobj;
>   
>   	/**
> @@ -521,6 +533,76 @@ __drm_gpuva_next(struct drm_gpuva *va)
>   #define drm_gpuvm_for_each_va_safe(va__, next__, gpuvm__) \
>   	list_for_each_entry_safe(va__, next__, &(gpuvm__)->rb.list, rb.entry)
>   
> +/**
> + * enum drm_gpuvm_exec_pass - the pass of a &drm_gpuvm locking sequence
> + *
> + * Most drivers lock all the &drm_gem_objects a &drm_gpuvm has mappings of in
> + * a single &drm_exec transaction and use %DRM_GPUVM_EXEC_PASS_ALL, which is
> + * the default.
> + *
> + * A driver may instead ask for the locking to be split in two passes, by
> + * setting &drm_gpuvm_exec.two_pass, in order to bound the time it holds the
> + * dma-resv lock of objects it does not have to validate. Only the evicted
> + * objects need that, so only those are locked by the early pass, and
> + * everything else is locked by the late pass once the validation is already
> + * done.
> + *
> + * The passes only divide up the external objects. The &drm_gpuvm's own
> + * dma-resv is held for the whole transaction, so private objects, which share
> + * it, are available to the driver in the early pass and are validated there.
> + *
> + * Both passes run inside a single &drm_exec transaction: the early pass keeps
> + * everything it locked, and the late pass only ever adds to that. Nothing is
> + * unlocked in between, so work done in the early pass is still valid when the
> + * driver submits at the end of the late pass.
> + */
> +enum drm_gpuvm_exec_pass {
> +	/**
> +	 * @DRM_GPUVM_EXEC_PASS_ALL: Prepare every external object. This is the
> +	 * behaviour of a single pass locking sequence. Its value is zero so
> +	 * that a zero initialised &drm_gpuvm_exec keeps that behaviour.
> +	 */
> +	DRM_GPUVM_EXEC_PASS_ALL = 0,
> +
> +	/**
> +	 * @DRM_GPUVM_EXEC_PASS_EARLY: Validate everything the transaction
> +	 * already holds, and opportunistically take on the external objects
> +	 * which are worth taking on.
> +	 *
> +	 * The &drm_gpuvm's dma-resv is held from the start of the transaction,
> +	 * so every private object is locked before this pass begins. The
> +	 * driver validates all of the evicted ones here.
> +	 *
> +	 * External objects are not locked yet, and this pass only prepares the
> +	 * ones which are evicted, that is the ones which are going to need
> +	 * validating anyway, for the driver to validate as well. The resident
> +	 * ones are deliberately left for later: they are still worked on
> +	 * before the transaction ends, having a fence attached or their
> +	 * mappings rebound, but none of that has to wait behind a migration,
> +	 * so there is no reason to hold their dma-resv while one is going on.
> +	 */
> +	DRM_GPUVM_EXEC_PASS_EARLY,
> +
> +	/**
> +	 * @DRM_GPUVM_EXEC_PASS_LATE: Lock everything else, and pick up
> +	 * whatever raced with the early pass.
> +	 *
> +	 * This prepares exactly the external objects the early pass left out,
> +	 * so that the transaction ends up holding the same set of locks a
> +	 * %DRM_GPUVM_EXEC_PASS_ALL one would have. That set is the complement
> +	 * of what the early pass actually did, which is not the same thing as
> +	 * whatever happens to be resident by now: an object can be evicted
> +	 * while the early pass is doing its slow work, and re-preparing an
> +	 * object the transaction already holds would fail with -EALREADY.
> +	 *
> +	 * The driver validates again here. Usually there is nothing left to
> +	 * do, but an object which was resident when the early pass skipped it
> +	 * may have been evicted since, and this is the pass which holds its
> +	 * dma-resv and can deal with it.
> +	 */
> +	DRM_GPUVM_EXEC_PASS_LATE,
> +};
> +
>   /**
>    * struct drm_gpuvm_exec - &drm_gpuvm abstraction of &drm_exec
>    *
> @@ -544,6 +626,32 @@ struct drm_gpuvm_exec {
>   	 */
>   	struct drm_gpuvm *vm;
>   
> +	/**
> +	 * @two_pass: split the locking into an early and a late pass; see
> +	 * &enum drm_gpuvm_exec_pass. Only supported for a
> +	 * %DRM_GPUVM_RESV_PROTECTED &drm_gpuvm, since the two passes have to
> +	 * agree on which external objects exist and only the GPUVM's common
> +	 * dma-resv, held across both, gives that. Setting it on any other
> +	 * &drm_gpuvm warns, and a single pass is used.
> +	 *
> +	 * This is a request, not a guarantee: the split is skipped when
> +	 * drm_gpuvm_exec_pass_needs_split() says it would not help, in which
> +	 * case the callback sees a single %DRM_GPUVM_EXEC_PASS_ALL.
> +	 *
> +	 * Only drm_gpuvm_exec_lock() acts on this. drm_gpuvm_exec_lock_array()
> +	 * rejects it and drm_gpuvm_exec_lock_range() ignores it, neither
> +	 * having a way to assign the objects it is given to a pass.
> +	 */
> +	bool two_pass;
> +
> +	/**
> +	 * @pass: the pass currently being prepared. Set by drm_gpuvm_exec_lock()
> +	 * before each call to @extra.fn, so that the callback can tell which
> +	 * objects it may touch. Always %DRM_GPUVM_EXEC_PASS_ALL unless
> +	 * @two_pass is set.
> +	 */
> +	enum drm_gpuvm_exec_pass pass;
> +
>   	/**
>   	 * @num_fences: the number of fences to reserve for the &dma_resv of the
>   	 * locked &drm_gem_objects
> @@ -576,6 +684,11 @@ int drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
>   			      struct drm_exec *exec,
>   			      unsigned int num_fences);
>   
> +int drm_gpuvm_exec_pass_prepare_objects(struct drm_gpuvm *gpuvm,
> +					struct drm_exec *exec,
> +					unsigned int num_fences,
> +					enum drm_gpuvm_exec_pass pass);
> +
>   int drm_gpuvm_prepare_range(struct drm_gpuvm *gpuvm,
>   			    struct drm_exec *exec,
>   			    u64 addr, u64 range,
> @@ -606,6 +719,13 @@ drm_gpuvm_exec_unlock(struct drm_gpuvm_exec *vm_exec)
>   }
>   
>   int drm_gpuvm_validate(struct drm_gpuvm *gpuvm, struct drm_exec *exec);
> +int drm_gpuvm_exec_pass_validate(struct drm_gpuvm *gpuvm,
> +				 struct drm_exec *exec,
> +				 enum drm_gpuvm_exec_pass pass);
> +bool drm_gpuvm_exec_pass_needs_split(struct drm_gpuvm *gpuvm);
> +
> +bool drm_gpuvm_exec_pass_has_evicted(struct drm_gpuvm *gpuvm,
> +				     enum drm_gpuvm_exec_pass pass);
>   void drm_gpuvm_resv_add_fence(struct drm_gpuvm *gpuvm,
>   			      struct drm_exec *exec,
>   			      struct dma_fence *fence,
> @@ -642,7 +762,8 @@ drm_gpuvm_exec_resv_add_fence(struct drm_gpuvm_exec *vm_exec,
>   static inline int
>   drm_gpuvm_exec_validate(struct drm_gpuvm_exec *vm_exec)
>   {
> -	return drm_gpuvm_validate(vm_exec->vm, &vm_exec->exec);
> +	return drm_gpuvm_exec_pass_validate(vm_exec->vm, &vm_exec->exec,
> +					    vm_exec->pass);
>   }
>   
>   /**
> @@ -676,10 +797,24 @@ struct drm_gpuvm_bo {
>   
>   	/**
>   	 * @evicted: Indicates whether the &drm_gem_object is evicted; field
> -	 * protected by the &drm_gem_object's dma-resv lock.
> +	 * protected by the &drm_gem_object's dma-resv lock. Only written
> +	 * through drm_gpuvm_bo_evict(), with WRITE_ONCE(), since
> +	 * %DRM_GPUVM_EXEC_PASS_EARLY reads it without that lock held.
>   	 */
>   	bool evicted;
>   
> +	/**
> +	 * @lock_skipped: Indicates that the current &drm_exec transaction does
> +	 * not hold this &drm_gpuvm_bo's dma-resv, because
> +	 * %DRM_GPUVM_EXEC_PASS_EARLY skipped it as not needing validation.
> +	 * Unlike @evicted this is stable for the duration of a locking
> +	 * sequence, which is what makes it safe for
> +	 * drm_gpuvm_exec_pass_validate() to key off, and what tells
> +	 * %DRM_GPUVM_EXEC_PASS_LATE which objects are still missing. Field
> +	 * protected the same way as the &drm_gpuvm's extobj list.
> +	 */
> +	bool lock_skipped;
> +
>   	/**
>   	 * @kref: The reference count for this &drm_gpuvm_bo.
>   	 */


Best regards,
-- 
Anna Maniscalco <anna.maniscalco2000@gmail.com>

Re: [PATCH v3 6/8] drm/msm: lock the resident BOs of a VM_BIND submit last

[Anna Maniscalco]

On 10/2/26 12:06 AM, Matthew Brost wrote:
> A VM_BIND submit locks the resv of every BO mapped in the VM, then
> validates the evicted ones, which means getting their pages and mapping
> them again. That is slow, and external objects can be shared with other
> processes, so all of it happens while holding resv locks other processes
> may be waiting on, for BOs which needed no work at all.
>
> Use the two pass locking gpuvm now provides. The early pass locks only
> the evicted external objects and validates them, along with the evicted
> private ones, which the VM resv held from the start covers. The late
> pass locks the external objects which were resident, and still validates
> in case one of them was evicted meanwhile. When nothing is evicted,
> drm_gpuvm_exec_pass_needs_split() says so and the submit keeps using a
> single pass.
>
> Both passes run in the same drm_exec transaction, nothing is unlocked in
> between, and they take disjoint sets of objects, so reserving one fence
> slot in each still reserves it exactly once per object.
>
> This moves validation from after drm_sched_job_arm() and fence
> attachment into the locking loop, ahead of everything else, which is
> also where a failure is easiest to unwind. The order does not matter to
> the shrinker: it skips any BO mapped in a VM whose resv it cannot
> trylock, and the submit holds the VM resv throughout, so a BO mapped in
> this VM cannot be evicted while it is locked, whether or not a fence is
> attached to it yet. The same means the early pass can never evict a BO
> the late pass is about to lock, the property Xe gets from
> xe_vm_set_validating().
>
> Cc: Abhinav Kumar <abhinav.kumar@linux.dev>
> Cc: Alice Ryhl <aliceryhl@google.com>
> Cc: Anna Maniscalco <anna.maniscalco2000@gmail.com>
> Cc: Antonino Maniscalco <antomani103@gmail.com>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Dmitry Baryshkov <lumag@kernel.org>
> Cc: Jessica Zhang <jesszhan0024@gmail.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Marijn Suijten <marijn.suijten@somainline.org>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Rob Clark <robin.clark@oss.qualcomm.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Shuah Khan <skhan@linuxfoundation.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Assisted-by: LLM
> ---
> v3:
>   - Follow the drm_gpuvm_exec_pass_ function renames (Danilo)
> ---
>   drivers/gpu/drm/msm/msm_gem_submit.c | 84 ++++++++++++++++++++++------
>   1 file changed, 66 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 1215b388cb40..4e454345567a 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -266,6 +266,71 @@ static int submit_lookup_cmds(struct msm_gem_submit *submit,
>   	return ret;
>   }
>   
> +/*
> + * Lock and validate every BO mapped in a VM_BIND VM.  Unlike the legacy path,
> + * where submit_pin_objects() only validates the BOs userspace attached to the
> + * submit, userspace does not tell us which BOs a VM_BIND submit uses, so the
> + * entire VM has to be validated.
> + *
> + * When something is evicted, the locks are taken in two passes.  The early
> + * pass locks only the external objects which need validating, i.e. the
> + * evicted ones, and validates them along with the evicted private objects,
> + * which the VM resv held from the start already covers.  The late pass then
> + * locks the external objects which were resident.  Validation means getting
> + * pages and mapping them, which is slow, and an external object can be shared
> + * with another process, so there is no point in stalling that process on the
> + * resv of a resident BO for the duration of it.  The late pass still
> + * validates, in case one of those BOs got evicted meanwhile.
> + *
> + * Both passes run in the same drm_exec transaction, nothing is unlocked in
> + * between, and they take disjoint sets of objects, so reserving one fence
> + * slot in each reserves it exactly once per object.
> + *
> + * The shrinker cannot evict a BO the early pass is about to validate, nor one
> + * it has validated already: it only evicts a BO after trylocking the resv of
> + * every VM the BO is mapped in, and the VM resv is held throughout.
> + */
> +static int submit_prepare_vm_objects(struct msm_gem_submit *submit)
> +{
> +	struct drm_gpuvm *vm = submit->vm;
> +	struct drm_exec *exec = &submit->exec;
> +	int ret;
> +
> +	ret = drm_gpuvm_prepare_vm(vm, exec, 1);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * With nothing evicted there is no validation to keep the resident
> +	 * objects unlocked for, so do not pay for the second walk.
> +	 */
> +	if (!drm_gpuvm_exec_pass_needs_split(vm)) {
> +		ret = drm_gpuvm_prepare_objects(vm, exec, 1);
> +		if (ret)
> +			return ret;
> +
> +		return drm_gpuvm_validate(vm, exec);
> +	}
> +
> +	ret = drm_gpuvm_exec_pass_prepare_objects(vm, exec, 1,
> +						  DRM_GPUVM_EXEC_PASS_EARLY);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_gpuvm_exec_pass_validate(vm, exec,
> +					   DRM_GPUVM_EXEC_PASS_EARLY);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_gpuvm_exec_pass_prepare_objects(vm, exec, 1,
> +						  DRM_GPUVM_EXEC_PASS_LATE);
> +	if (ret)
> +		return ret;
> +
> +	return drm_gpuvm_exec_pass_validate(vm, exec,
> +					    DRM_GPUVM_EXEC_PASS_LATE);
> +}
> +
>   static int submit_lock_objects_vmbind(struct msm_gem_submit *submit)
>   {
>   	unsigned flags = DRM_EXEC_INTERRUPTIBLE_WAIT | DRM_EXEC_IGNORE_DUPLICATES;
> @@ -276,12 +341,7 @@ static int submit_lock_objects_vmbind(struct msm_gem_submit *submit)
>   	submit->has_exec = true;
>   
>   	drm_exec_until_all_locked (&submit->exec) {
> -		ret = drm_gpuvm_prepare_vm(submit->vm, exec, 1);
> -		drm_exec_retry_on_contention(exec);
> -		if (ret)
> -			break;
> -
> -		ret = drm_gpuvm_prepare_objects(submit->vm, exec, 1);
> +		ret = submit_prepare_vm_objects(submit);
>   		drm_exec_retry_on_contention(exec);
>   		if (ret)
>   			break;
> @@ -790,18 +850,6 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>   
>   	submit_attach_object_fences(submit);
>   
> -	if (msm_context_is_vmbind(ctx)) {
> -		/*
> -		 * If we are not using VM_BIND, submit_pin_vmas() will validate
> -		 * just the BOs attached to the submit.  In that case we don't
> -		 * need to validate the _entire_ vm, because userspace tracked
> -		 * what BOs are associated with the submit.
> -		 */
> -		ret = drm_gpuvm_validate(submit->vm, &submit->exec);
> -		if (ret)
> -			goto out;
> -	}
> -
>   	/* The scheduler owns a ref now: */
>   	msm_gem_submit_get(submit);
>   

This and the two other msm patches are:

Reviewed-by: Anna Maniscalco <anna.maniscalco2000@gmail.com>

Best regards,
-- 
Anna Maniscalco <anna.maniscalco2000@gmail.com>

Re: [PATCH v3 3/8] drm/panthor: lock the resident BOs of a submit last

[Boris Brezillon]

On Thu,  1 Oct 2026 15:06:27 -0700
Matthew Brost <matthew.brost@intel.com> wrote:

> panthor_vm_prepare_mapped_bos_resvs() locks every external object mapped
> in the VM and then validates the evicted ones. Validation here means
> panthor_vm_bo_validate(), which swaps the BO's pages back in and restores
> its VMAs. That is slow, and an external object is one which can be shared
> with another process, so the whole of it happens while holding dma-resv
> locks other processes may be waiting on.
> 
> Nothing is gained by holding those. A resident object needs no swapping
> in; only the evicted ones do. Split the locking into the two passes
> gpuvm now understands: the early pass takes just the evicted external
> objects and swaps them in, and the late pass takes the ones which were
> resident and are therefore normally ready to use as they are. Private
> objects are covered by the VM resv, which is held from the start, so
> evicted ones are still validated in the early pass.
> 
> The split is only worth it when there is something to validate, so
> drm_gpuvm_exec_pass_needs_split() decides, and a submit with nothing
> evicted keeps doing exactly what it does today in a single pass.
> 
> Both passes run in the same drm_exec transaction, so nothing is unlocked
> in between and the late pass only ever adds locks. They take disjoint
> sets of objects, so passing slot_count to both still reserves it exactly
> once per object.
> 
> The early pass reads the evicted state without the object's dma-resv,
> that being the lock it is trying not to take. The race is benign: an
> object evicted right after the early pass skipped it is picked up by the
> late pass instead, which is why that pass still validates.
> 
> Validation here allocates pages, which can recurse into panthor's own
> shrinker, so it is worth being explicit about what the early pass can
> evict. There is no deadlock: drm_gem_lru_scan() acquires the resv with
> ww_mutex_trylock() and skips what it cannot get. VM-exclusive BOs share
> the VM resv, which is held across both passes, so those are always
> skipped. External objects are not held by the early pass, though, so
> reclaim can evict one while the early pass validates something else.
> 
> That is handled, and is why the late pass validates rather than only
> locking: it picks up anything evicted after the early pass looked at it.
> The cost is that the swapin for such a BO happens under the full set of
> locks, i.e. it degrades to the current behaviour for that one object.
> 
> Xe avoids this by refusing to evict BOs bound to a VM the current task is
> validating (xe_bo_eviction_valuable() and xe_vm_is_validating()). Panthor
> has no equivalent guard. Adding one would make the split more effective
> under memory pressure, but it is not needed for correctness, so it is left
> as a follow up.
> 
> Cc: Abhinav Kumar <abhinav.kumar@linux.dev>
> Cc: Alice Ryhl <aliceryhl@google.com>
> Cc: Anna Maniscalco <anna.maniscalco2000@gmail.com>
> Cc: Antonino Maniscalco <antomani103@gmail.com>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Dmitry Baryshkov <lumag@kernel.org>
> Cc: Jessica Zhang <jesszhan0024@gmail.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Marijn Suijten <marijn.suijten@somainline.org>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Rob Clark <robin.clark@oss.qualcomm.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Shuah Khan <skhan@linuxfoundation.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Assisted-by: LLM
> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> ---
> v3:
>  - Follow the drm_gpuvm_exec_pass_ function renames (Danilo)
> ---
>  drivers/gpu/drm/panthor/panthor_mmu.c | 55 ++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index d75d575473da..d4b968ab094e 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -3258,6 +3258,26 @@ int panthor_vm_unmap_range(struct panthor_vm *vm, u64 va, u64 size)
>   * need to reserve a slot on all BOs mapped to a VM and update this slot with
>   * the job fence after its submission.
>   *
> + * When something is evicted the locks are taken in two passes; when nothing
> + * is, a single pass is used, as before. The early pass only takes the external
> + * objects which actually need validating, i.e. the evicted ones, and swaps
> + * them back in. Private objects are covered by the VM resv, which is held
> + * from the start, so they are validated here too. The late pass then takes
> + * the external objects the early pass left out, which were resident and so
> + * normally need no swapping in; it still validates, since one of them may
> + * have been evicted in the meantime.
> + *
> + * The point is that panthor_vm_bo_validate() swaps pages back in, which is
> + * slow, and an external object is one which can be shared with another
> + * process. Doing that while holding the resv of a resident shared BO would
> + * stall whoever else needs it, for no benefit, since a resident object is
> + * ready to use as it is.
> + *
> + * Both passes run in the same drm_exec transaction: nothing is unlocked in
> + * between and the late pass only ever adds locks. The passes take disjoint
> + * sets of objects, so reserving @slot_count in each still reserves it
> + * exactly once per object.
> + *
>   * Return: 0 on success, a negative error code otherwise.
>   */
>  int panthor_vm_prepare_mapped_bos_resvs(struct drm_exec *exec, struct panthor_vm *vm,
> @@ -3270,11 +3290,42 @@ int panthor_vm_prepare_mapped_bos_resvs(struct drm_exec *exec, struct panthor_vm
>  	if (ret)
>  		return ret;
>  
> -	ret = drm_gpuvm_prepare_objects(&vm->base, exec, slot_count);
> +	/*
> +	 * With nothing evicted there is no validation to keep the resident
> +	 * objects unlocked for, so do not pay for the second walk.
> +	 */
> +	if (!drm_gpuvm_exec_pass_needs_split(&vm->base)) {
> +		ret = drm_gpuvm_prepare_objects(&vm->base, exec, slot_count);
> +		if (ret)
> +			return ret;
> +
> +		return drm_gpuvm_validate(&vm->base, exec);
> +	}
> +
> +	ret = drm_gpuvm_exec_pass_prepare_objects(&vm->base, exec,
> +						  slot_count,
> +						  DRM_GPUVM_EXEC_PASS_EARLY);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_gpuvm_exec_pass_validate(&vm->base, exec,
> +					   DRM_GPUVM_EXEC_PASS_EARLY);
>  	if (ret)
>  		return ret;
>  
> -	return drm_gpuvm_validate(&vm->base, exec);
> +	ret = drm_gpuvm_exec_pass_prepare_objects(&vm->base, exec,
> +						  slot_count,
> +						  DRM_GPUVM_EXEC_PASS_LATE);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Objects the early pass skipped were resident then, but another
> +	 * process may have evicted one since. Now that everything is locked,
> +	 * pick up whatever is left.
> +	 */
> +	return drm_gpuvm_exec_pass_validate(&vm->base, exec,
> +					    DRM_GPUVM_EXEC_PASS_LATE);
>  }
>  
>  unsigned long

Re: [PATCH v3 7/8] drm/nouveau: use DRM_GPUVM_RESV_PROTECTED

[Liviu Dudau]

On Thu, Oct 01, 2026 at 03:06:31PM -0700, Matthew Brost wrote:
> nouveau creates its drm_gpuvm without DRM_GPUVM_RESV_PROTECTED, so the
> extobj and evicted lists are protected by internal spinlocks. Every
> nouveau path but three already touches them with the VM's resv held:
> drm_gpuvm_exec_lock() and drm_gpuvm_validate() on exec, and TTM moves of
> private BOs, which share the VM's resv. The remaining three are:
> 
>  - nouveau_uvmm_bind_job_submit() adds the vm_bo of a MAP op to the
>    extobj list holding no resv at all. Move that into
>    bind_lock_validate(), which now locks the VM's resv as well.
> 
>  - bind_link_gpuvas() unlinks the GPUVAs of UNMAP and REMAP ops, which
>    can drop the last reference of their vm_bo. It runs within the bind
>    job's drm_exec transaction, which the above makes hold the VM's resv.
> 
>  - nouveau_uvmm_bind_job_cleanup() and nouveau_uvmm_fini() can drop the
>    last reference of a vm_bo holding only the object's resv. Lock the
>    VM's resv along with it, through a new
>    nouveau_uvmm_lock_vm_and_obj().
> 
> The bind job's fence now always lands in the VM's resv, as BOOKKEEP.
> This already happened whenever a bind job mapped a private BO.
> 
> With that the internal spinlocks buy nothing, so set
> DRM_GPUVM_RESV_PROTECTED. This is also what two-pass locking in
> drm_gpuvm requires, which a following patch makes use of.
> 
> Cc: Abhinav Kumar <abhinav.kumar@linux.dev>
> Cc: Alice Ryhl <aliceryhl@google.com>
> Cc: Anna Maniscalco <anna.maniscalco2000@gmail.com>
> Cc: Antonino Maniscalco <antomani103@gmail.com>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Dmitry Baryshkov <lumag@kernel.org>
> Cc: Jessica Zhang <jesszhan0024@gmail.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Marijn Suijten <marijn.suijten@somainline.org>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Rob Clark <robin.clark@oss.qualcomm.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Shuah Khan <skhan@linuxfoundation.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Assisted-by: LLM
> ---
> v3:
>  - New patch (Danilo)

Bit confused about this. If Danilo contributed this patch should it not carry his S-o-b as well?

Best regards,
Liviu

> ---
>  drivers/gpu/drm/nouveau/nouveau_uvmm.c | 49 ++++++++++++++++++++++----
>  1 file changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index 2026fe6b48c6..dae612e56d91 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -1187,6 +1187,27 @@ bind_validate_region(struct nouveau_job *job)
>  	return 0;
>  }
>  
> +/*
> + * Lock the VM's common dma-resv together with the one of @obj, as needed to
> + * drop what may be the last reference of a &drm_gpuvm_bo.
> + */
> +static void
> +nouveau_uvmm_lock_vm_and_obj(struct nouveau_uvmm *uvmm, struct drm_exec *exec,
> +			     struct drm_gem_object *obj)
> +{
> +	int ret;
> +
> +	drm_exec_init(exec, DRM_EXEC_IGNORE_DUPLICATES, 2);
> +	drm_exec_until_all_locked(exec) {
> +		ret = drm_exec_lock_obj(exec, drm_gpuvm_resv_obj(&uvmm->base));
> +		if (!ret)
> +			ret = drm_exec_lock_obj(exec, obj);
> +		drm_exec_retry_on_contention(exec);
> +		if (drm_WARN_ON(uvmm->base.drm, ret))
> +			break;
> +	}
> +}
> +
>  static void
>  bind_link_gpuvas(struct bind_job_op *bop)
>  {
> @@ -1224,12 +1245,24 @@ bind_lock_validate(struct nouveau_job *job, struct drm_exec *exec,
>  		   unsigned int num_fences)
>  {
>  	struct nouveau_uvmm_bind_job *bind_job = to_uvmm_bind_job(job);
> +	struct nouveau_uvmm *uvmm = nouveau_cli_uvmm(job->cli);
>  	struct bind_job_op *op;
>  	int ret;
>  
> +	/* The VM's dma-resv protects its extobj and evicted lists, and must be
> +	 * held by bind_link_gpuvas() in case it drops the last reference of a
> +	 * &drm_gpuvm_bo.
> +	 */
> +	ret = drm_gpuvm_prepare_vm(&uvmm->base, exec, num_fences);
> +	if (ret)
> +		return ret;
> +
>  	list_for_each_op(op, &bind_job->ops) {
>  		struct drm_gpuva_op *va_op;
>  
> +		if (op->op == OP_MAP)
> +			drm_gpuvm_bo_extobj_add(op->vm_bo);
> +
>  		if (!op->ops)
>  			continue;
>  
> @@ -1288,8 +1321,6 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job,
>  			dma_resv_unlock(obj->resv);
>  			if (IS_ERR(op->vm_bo))
>  				return PTR_ERR(op->vm_bo);
> -
> -			drm_gpuvm_bo_extobj_add(op->vm_bo);
>  		}
>  
>  		ret = bind_validate_op(job, op);
> @@ -1603,9 +1634,11 @@ nouveau_uvmm_bind_job_cleanup(struct nouveau_job *job)
>  			drm_gpuva_ops_free(&uvmm->base, op->ops);
>  
>  		if (!IS_ERR_OR_NULL(op->vm_bo)) {
> -			dma_resv_lock(obj->resv, NULL);
> +			struct drm_exec exec;
> +
> +			nouveau_uvmm_lock_vm_and_obj(uvmm, &exec, obj);
>  			drm_gpuvm_bo_put(op->vm_bo);
> -			dma_resv_unlock(obj->resv);
> +			drm_exec_fini(&exec);
>  		}
>  
>  		if (obj)
> @@ -1941,7 +1974,8 @@ nouveau_uvmm_ioctl_vm_init(struct drm_device *dev,
>  	mt_init_flags(&uvmm->region_mt, MT_FLAGS_LOCK_EXTERN);
>  	mt_set_external_lock(&uvmm->region_mt, &uvmm->mutex);
>  
> -	drm_gpuvm_init(&uvmm->base, cli->name, 0, drm, r_obj,
> +	drm_gpuvm_init(&uvmm->base, cli->name, DRM_GPUVM_RESV_PROTECTED,
> +		       drm, r_obj,
>  		       NOUVEAU_VA_SPACE_START,
>  		       NOUVEAU_VA_SPACE_END,
>  		       init->kernel_managed_addr,
> @@ -1978,6 +2012,7 @@ nouveau_uvmm_fini(struct nouveau_uvmm *uvmm)
>  	struct nouveau_uvma_region *reg;
>  	struct nouveau_cli *cli = uvmm->vmm.cli;
>  	struct drm_gpuva *va, *next;
> +	struct drm_exec exec;
>  
>  	nouveau_uvmm_lock(uvmm);
>  	drm_gpuvm_for_each_va_safe(va, next, &uvmm->base) {
> @@ -1989,9 +2024,9 @@ nouveau_uvmm_fini(struct nouveau_uvmm *uvmm)
>  
>  		drm_gpuva_remove(va);
>  
> -		dma_resv_lock(obj->resv, NULL);
> +		nouveau_uvmm_lock_vm_and_obj(uvmm, &exec, obj);
>  		drm_gpuva_unlink(va);
> -		dma_resv_unlock(obj->resv);
> +		drm_exec_fini(&exec);
>  
>  		nouveau_uvma_unmap(uvma);
>  		nouveau_uvma_vmm_put(uvma);
> -- 
> 2.34.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯