On Fri Aug 14, 2026 at 9:32 AM CEST, Matthew Brost wrote:
> 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.
That's a nice optimization!
> 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.
I only read this after I stumbled across this in the code below. Leaving this
as-is is still a data race per LKMM and I'd expect KCSAN to flag it.
In any case, please also add the corresponding WRITE_ONCE() as well as a comment
that explains why in this specific case it is OK to read the value without the
dma-resv lock held and why ordering is not an issue.
> Two pass locking requires a DRM_GPUVM_RESV_PROTECTED drm_gpuvm.
This is unfortunate, I don't want to have any second class citizens.
That said, I think I can make nouveau switch to DRM_GPUVM_RESV_PROTECTED. With
this, only MSM is left, and it says
* 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:
which is probably similar to why nouveau didn't do it in the first place. I
could have a look after LPC so we can get rid of !DRM_GPUVM_RESV_PROTECTED
entirely, which I think would be great.
> Assisted-by: GitHub_Copilot:claude-opus-5
Please use Assisted-by: LLM instead.
> +/**
> + * DOC: Two pass locking
I think this should say that this is about "preparing" objects and object
validation.
Maybe "GPUVM EXEC two-pass locking"?
> +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:
> + vm_bo->lock_skipped = !READ_ONCE(vm_bo->evicted);
Ick! Not a huge fan of this, but I guess it makes sense. AFAICS this can race
with drm_gpuvm_bo_evict() though.
> +int
> +drm_gpuvm_prepare_objects_pass(struct drm_gpuvm *gpuvm,
> + struct drm_exec *exec,
> + unsigned int num_fences,
> + enum drm_gpuvm_exec_pass pass)
> +{
> + if (pass != DRM_GPUVM_EXEC_PASS_ALL &&
> + drm_WARN_ON(gpuvm->drm, !drm_gpuvm_resv_protected(gpuvm)))
drm_WARN_ON_ONCE() seems to make more sense here.
> @@ -1360,6 +1603,10 @@ drm_gpuvm_exec_lock(struct drm_gpuvm_exec *vm_exec)
> unsigned int num_fences = vm_exec->num_fences;
> int ret;
>
> + if (vm_exec->two_pass &&
> + drm_WARN_ON(gpuvm->drm, !drm_gpuvm_resv_protected(gpuvm)))
Same here...
> @@ -1422,6 +1680,9 @@ drm_gpuvm_exec_lock_array(struct drm_gpuvm_exec
> *vm_exec,
> unsigned int num_objs;
> } args;
>
> + if (drm_WARN_ON(vm_exec->vm->drm, vm_exec->two_pass))
...and here.
> +int
> +drm_gpuvm_validate_pass(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 (pass != DRM_GPUVM_EXEC_PASS_ALL &&
> + drm_WARN_ON(gpuvm->drm, !drm_gpuvm_resv_protected(gpuvm)))
That's really a lot of !drm_gpuvm_resv_protected() checks needed. :(
> +bool
> +drm_gpuvm_has_evicted(struct drm_gpuvm *gpuvm, enum drm_gpuvm_exec_pass pass)
The name is a bit confusing, as the scope of the function is limited to an
exec_pass, but not the VM in general.
Even though a bit verbose, I think drm_gpuvm_exec_pass_has_evicted() is better.
I'd probably also use the drm_gpuvm_exec_pass prefiy consistently for the
functions newly introduced for the feature.
> @@ -680,6 +799,18 @@ struct drm_gpuvm_bo {
> */
> 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_validate_pass()
> + * 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;
That's a bit nasty, but it works and I can't think of a cleaner solution.
