Re: [PATCH] drm/exec: don't immediately add prelocked obj to array of locked objs

[vitaly prosyak <vprosyak@amd.com>]

Hi Christian,

Thanks for the fix. Tested under the same conditions that previously
reproduced the crash at dma-resv.c:329; the crash did not reproduce
with this patch
2026-10-03T16:00:58-04:00 Intel-system kernel: kernel BUG at drivers/dma-buf/dma-resv.c:329!
2026-10-03T16:00:58-04:00 Intel-system kernel: Oops: invalid opcode: 0000 [#1] SMP NOPTI
2026-10-03T16:00:58-04:00 Intel-system kernel: CPU: 11 UID: 0 PID: 477 Comm: kworker/11:2 Not tainted 7.1.0+ #83 PREEMPT(full) 
2026-10-03T16:00:58-04:00 Intel-system kernel: Hardware name: ASUS System Product Name/ROG MAXIMUS Z790 HERO, BIOS 2703 10/17/2024
2026-10-03T16:00:58-04:00 Intel-system kernel: Workqueue: events amdgpu_userq_restore_worker [amdgpu]
2026-10-03T16:00:58-04:00 Intel-system kernel: RIP: 0010:dma_resv_add_fence+0x2d6/0x2f0
2026-10-03T16:00:58-04:00 Intel-system kernel: Code: 44 8b 88 30 0a 00 00 48 05 18 0d 00 00 50 4c 8b 45 08 48 8b 75 b8 e8 f9 01 35 ff 58 41 8b 45 14 41 39 45 10 0f 82 d2 fe ff ff <0f> 0b 31 d2 45 31 ff e9 ba fe ff ff e8 c9 a5 5c 00 66 0f 1f 84 00
2026-10-03T16:00:58-04:00 Intel-system kernel: RSP: 0018:ffffcb8682e5fb28 EFLAGS: 00010246
2026-10-03T16:00:58-04:00 Intel-system kernel: RAX: 0000000000000000 RBX: 0000000000000003 RCX: 0000000000000000
2026-10-03T16:00:58-04:00 Intel-system kernel: RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
2026-10-03T16:00:58-04:00 Intel-system kernel: RBP: ffffcb8682e5fb78 R08: 0000000000000000 R09: 0000000000000000
2026-10-03T16:00:58-04:00 Intel-system kernel: R10: 0000000000000000 R11: 0000000000000000 R12: ffff88edd40ca500
2026-10-03T16:00:58-04:00 Intel-system kernel: R13: ffff88ed91d83f00 R14: ffffffffc1c3ee60 R15: 0000000000000000
2026-10-03T16:00:58-04:00 Intel-system kernel: FS:  0000000000000000(0000) GS:ffff88f53e2bc000(0000) knlGS:0000000000000000
2026-10-03T16:00:58-04:00 Intel-system kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
2026-10-03T16:00:58-04:00 Intel-system kernel: CR2: 0000756822dd7000 CR3: 000000012e1d4006 CR4: 0000000000f72ef0
2026-10-03T16:00:58-04:00 Intel-system kernel: PKRU: 55555554
2026-10-03T16:00:58-04:00 Intel-system kernel: Call Trace:
2026-10-03T16:00:58-04:00 Intel-system kernel:  <TASK>
2026-10-03T16:00:58-04:00 Intel-system kernel:  amdgpu_evf_mgr_attach_fence+0x179/0x330 [amdgpu]
2026-10-03T16:00:58-04:00 Intel-system kernel:  amdgpu_evf_mgr_rearm+0x1ff/0x2b0 [amdgpu]
2026-10-03T16:00:58-04:00 Intel-system kernel:  amdgpu_userq_vm_validate_and_restore_queue+0x8f0/0xc80 [amdgpu]
2026-10-03T16:00:58-04:00 Intel-system kernel:  amdgpu_userq_restore_worker+0x64/0xb0 [amdgpu]
2026-10-03T16:00:58-04:00 Intel-system kernel:  process_one_work+0x22e/0x780
2026-10-03T16:00:58-04:00 Intel-system kernel:  worker_thread+0x1db/0x3d0
2026-10-03T16:00:58-04:00 Intel-system kernel:  ? __pfx_worker_thread+0x10/0x10
2026-10-03T16:00:58-04:00 Intel-system kernel:  kthread+0xfe/0x140
2026-10-03T16:00:58-04:00 Intel-system kernel:  ? __pfx_kthread+0x10/0x10
2026-10-03T16:00:58-04:00 Intel-system kernel:  ret_from_fork+0x3bd/0x470
2026-10-03T16:00:58-04:00 Intel-system kernel:  ? __pfx_kthread+0x10/0x10
2026-10-03T16:00:58-04:00 Intel-system kernel:  ret_from_fork_asm+0x1a/0x30
2026-10-03T16:00:58-04:00 Intel-system kernel:  </TASK>


My patch preserved the fence-slot request across retries. Your approach
instead excludes the implicit prelocked BO from iteration until explicitly
requested again, avoiding the extra reservation-count bookkeeping.

A few minor wording corrections:

- "keeping the prelocked object of the array" should be
  "keeping the prelocked object out of the array".
- "Should the prelocked object never been mentioned again" should be
  "If the prelocked object is never mentioned again".
- "might never given to" should be "might never be passed to".
- "unecessarily" should be "unnecessarily".

Tested-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
Reviewed-by: Vitaly Prosyak <vitaly.prosyak@amd.com>

Thanks,
Vitaly

On 2026-10-07 08:46, Christian König wrote:
> When a contention is detected the drm_exec object grabs a reference to
> the contended object, unlocks all other objects and pre-locks the
> contended one before anything else.
>
> The problem is that it is possible that this pre-locked object was about
> to be destroyed and drm_exec is now holding the last reference to it.
>
> So we never actually see this pre-locked object again to be explicitly
> locked and so also never reserve a slot for a dma_fence object on it.
>
> Should the driver now use drm_exec_for_each_locked_object() to add a new
> fence to all locked objects we eventually see a warning or even a
> BUG_ON() to prevent random memory corruption.
>
> Fix this by keeping the prelocked object of the array of locked objs
> until it was explicitly mentioned by the driver again.
>
> Should the prelocked object never been mentioned again by the driver
> just unlock and drop the reference at the end of the locking loop.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reported-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
> Fixes: 09593216bff1 ("drm: execution context for GEM buffers v7")
> CC: stable@vger.kernel.org # v6.6+
> ---
>  drivers/gpu/drm/drm_exec.c | 73 ++++++++++++++++++++++----------------
>  include/drm/drm_exec.h     | 15 ++++----
>  2 files changed, 52 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
> index 2453ec41360f4..a910b0ad17ce4 100644
> --- a/drivers/gpu/drm/drm_exec.c
> +++ b/drivers/gpu/drm/drm_exec.c
> @@ -48,6 +48,16 @@
>   * See struct dma_exec for more details.
>   */
>  
> +/* Unlock and put the prelocked obj */
> +static void drm_exec_drop_prelocked(struct drm_exec *exec)
> +{
> +	if (exec->prelocked) {
> +		dma_resv_unlock(exec->prelocked->resv);
> +		drm_gem_object_put(exec->prelocked);
> +		exec->prelocked = NULL;
> +	}
> +}
> +
>  /* Unlock all objects and drop references */
>  static void drm_exec_unlock_all(struct drm_exec *exec)
>  {
> @@ -58,8 +68,7 @@ static void drm_exec_unlock_all(struct drm_exec *exec)
>  		drm_gem_object_put(obj);
>  	}
>  
> -	drm_gem_object_put(exec->prelocked);
> -	exec->prelocked = NULL;
> +	drm_exec_drop_prelocked(exec);
>  }
>  
>  /**
> @@ -118,6 +127,14 @@ EXPORT_SYMBOL(drm_exec_fini);
>  bool drm_exec_cleanup(struct drm_exec *exec)
>  {
>  	if (likely(!exec->contended)) {
> +
> +		/*
> +		 * The contended and prelocked object might never given to
> +		 * drm_exec_lock_obj() again. This can happen when the object
> +		 * is about to be cleaned up and we now hold the last reference
> +		 * to it. Make sure that we don't unecessarily keep it around.
> +		 */
> +		drm_exec_drop_prelocked(exec);
>  		ww_acquire_done(&exec->ticket);
>  		return false;
>  	}
> @@ -175,16 +192,16 @@ static int drm_exec_lock_contended(struct drm_exec *exec)
>  		dma_resv_lock_slow(obj->resv, &exec->ticket);
>  	}
>  
> -	ret = drm_exec_obj_locked(exec, obj);
> -	if (unlikely(ret))
> -		goto error_unlock;
> +	/*
> +	 * It is perfectly possible that we see another contention before the
> +	 * first one is fully handled.
> +	 */
> +	drm_exec_drop_prelocked(exec);
>  
> +	/* We move the reference from contended to prelocked here */
>  	exec->prelocked = obj;
>  	return 0;
>  
> -error_unlock:
> -	dma_resv_unlock(obj->resv);
> -
>  error_dropref:
>  	drm_gem_object_put(obj);
>  	return ret;
> @@ -212,35 +229,31 @@ int drm_exec_lock_obj(struct drm_exec *exec, struct drm_gem_object *obj)
>  	if (exec->prelocked == obj) {
>  		drm_gem_object_put(exec->prelocked);
>  		exec->prelocked = NULL;
> -		return 0;
> -	}
> +	} else {
> +		if (exec->flags & DRM_EXEC_INTERRUPTIBLE_WAIT)
> +			ret = dma_resv_lock_interruptible(obj->resv,
> +							  &exec->ticket);
> +		else
> +			ret = dma_resv_lock(obj->resv, &exec->ticket);
> +
> +		if (unlikely(ret == -EDEADLK)) {
> +			drm_gem_object_get(obj);
> +			exec->contended = obj;
> +			return -EDEADLK;
> +		}
>  
> -	if (exec->flags & DRM_EXEC_INTERRUPTIBLE_WAIT)
> -		ret = dma_resv_lock_interruptible(obj->resv, &exec->ticket);
> -	else
> -		ret = dma_resv_lock(obj->resv, &exec->ticket);
> +		if (unlikely(ret == -EALREADY) &&
> +		    exec->flags & DRM_EXEC_IGNORE_DUPLICATES)
> +			return 0;
>  
> -	if (unlikely(ret == -EDEADLK)) {
> -		drm_gem_object_get(obj);
> -		exec->contended = obj;
> -		return -EDEADLK;
> +		if (unlikely(ret))
> +			return ret;
>  	}
>  
> -	if (unlikely(ret == -EALREADY) &&
> -	    exec->flags & DRM_EXEC_IGNORE_DUPLICATES)
> -		return 0;
> -
> -	if (unlikely(ret))
> -		return ret;
> -
>  	ret = drm_exec_obj_locked(exec, obj);
>  	if (ret)
> -		goto error_unlock;
> -
> -	return 0;
> +		dma_resv_unlock(obj->resv);
>  
> -error_unlock:
> -	dma_resv_unlock(obj->resv);
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_exec_lock_obj);
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> index cc2937185a9f7..c92d69a2c793d 100644
> --- a/include/drm/drm_exec.h
> +++ b/include/drm/drm_exec.h
> @@ -73,13 +73,15 @@ drm_exec_obj(struct drm_exec *exec, unsigned long index)
>  
>  /* Helper for drm_exec_for_each_locked_object(). Internal use only. */
>  #define __drm_exec_for_each_locked_object(exec, obj, __index)		\
> -	for (unsigned long __index = 0; ((obj) = drm_exec_obj(exec, __index)); ++__index)
> +	for (unsigned long __index = 0; ((obj) = drm_exec_obj(exec, __index)); \
> +	     ++__index)
>  /**
>   * drm_exec_for_each_locked_object - iterate over all the locked objects
>   * @exec: drm_exec object
>   * @obj: the current GEM object
>   *
> - * Iterate over all the locked GEM objects inside the drm_exec object.
> + * Iterate over all the explicitly locked GEM objects inside the drm_exec
> + * container, except for the contended and implicit prelocked one.
>   */
>  #define drm_exec_for_each_locked_object(exec, obj)			\
>  	__drm_exec_for_each_locked_object(exec, obj, __UNIQUE_ID(drm_exec))
> @@ -94,12 +96,13 @@ drm_exec_obj(struct drm_exec *exec, unsigned long index)
>   * @exec: drm_exec object
>   * @obj: the current GEM object
>   *
> - * Iterate over all the locked GEM objects inside the drm_exec object in
> - * reverse locking order. Note that the internal index may wrap around,
> - * but that will be caught by drm_exec_obj(), returning a NULL object.
> + * Iterate over all the explicitly locked GEM objects inside the drm_exec
> + * container in reverse locking order. Note that the internal index may wrap
> + * around, but that will be caught by drm_exec_obj(), returning a NULL object.
>   */
>  #define drm_exec_for_each_locked_object_reverse(exec, obj)		\
> -	__drm_exec_for_each_locked_object_reverse(exec, obj, __UNIQUE_ID(drm_exec))
> +	__drm_exec_for_each_locked_object_reverse(exec, obj,		\
> +						  __UNIQUE_ID(drm_exec))
>  
>  /**
>   * drm_exec_until_all_locked - loop until all GEM objects are locked