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

["Danilo Krummrich" <dakr@kernel.org>]

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.