Re: [PATCH] drm/drm_exec: avoid indirect goto

[Christian König]

Adding a few more people to comment and review.

Thanks Mike for the pointers, going to add them to the patch before pushing it to drm-misc-fixes.

@Thomas, Maxime, Simona, Dave can anybody give me an rb or comment?

Thanks,
Christian.

On 7/4/26 10:41, Christian König wrote:
> The drm_exec component uses a variable with scope limited to the for() and
> an indirect goto to allow instantiating multiple macros in the same
> function.
> 
> This unfortunately doesn't work well with certain compilers when the
> indirect goto can't be lowered to a direct jump.
> 
> Switch the indirect goto to a direct goto, the drawback is that we now
> can't use the dma_exec_until_all_locked() macro in the same function
> multiple times.
> 
> The is currently only one user of this and only as a hacky workaround
> which is about to be removed.
> 
> So document that the __label__ statement should be used when the macro is
> used multiple times and fix the tests and the only use case where that is
> necessary.
> 
> Suggested-by: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c |  2 ++
>  drivers/gpu/drm/tests/drm_exec_test.c  | 24 ++++++++++++------
>  include/drm/drm_exec.h                 | 34 ++++++++++++++------------
>  3 files changed, 36 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index fee4c94c2585..fc28d0fdad37 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -3011,6 +3011,8 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
>  	is_compute_context = vm->is_compute_context;
>  
>  	if (is_compute_context) {
> +		__label__ drm_exec_retry;
> +
>  		/* Release the root PD lock since svm_range_restore_pages
>  		 * might try to take it.
>  		 * TODO: rework svm_range_restore_pages so that this isn't
> diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c
> index 2fc47f3b463b..7a374e462348 100644
> --- a/drivers/gpu/drm/tests/drm_exec_test.c
> +++ b/drivers/gpu/drm/tests/drm_exec_test.c
> @@ -180,19 +180,27 @@ static void test_multiple_loops(struct kunit *test)
>  {
>  	struct drm_exec exec;
>  
> -	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT, 0);
> -	drm_exec_until_all_locked(&exec)
>  	{
> -		break;
> +		__label__ drm_exec_retry;
> +
> +		drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT, 0);
> +		drm_exec_until_all_locked(&exec)
> +		{
> +			break;
> +		}
> +		drm_exec_fini(&exec);
>  	}
> -	drm_exec_fini(&exec);
>  
> -	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT, 0);
> -	drm_exec_until_all_locked(&exec)
>  	{
> -		break;
> +		__label__ drm_exec_retry;
> +
> +		drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT, 0);
> +		drm_exec_until_all_locked(&exec)
> +		{
> +			break;
> +		}
> +		drm_exec_fini(&exec);
>  	}
> -	drm_exec_fini(&exec);
>  	KUNIT_SUCCEED(test);
>  }
>  
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> index 8725ba92ff91..cc2937185a9f 100644
> --- a/include/drm/drm_exec.h
> +++ b/include/drm/drm_exec.h
> @@ -101,17 +101,6 @@ drm_exec_obj(struct drm_exec *exec, unsigned long index)
>  #define drm_exec_for_each_locked_object_reverse(exec, obj)		\
>  	__drm_exec_for_each_locked_object_reverse(exec, obj, __UNIQUE_ID(drm_exec))
>  
> -/*
> - * Helper to drm_exec_until_all_locked(). Don't use directly.
> - *
> - * Since labels can't be defined local to the loop's body we use a jump pointer
> - * to make sure that the retry is only used from within the loop's body.
> - */
> -#define __drm_exec_until_all_locked(exec, _label)			 \
> -_label:									 \
> -	for (void *const __maybe_unused __drm_exec_retry_ptr = &&_label; \
> -	     drm_exec_cleanup(exec);)
> -
>  /**
>   * drm_exec_until_all_locked - loop until all GEM objects are locked
>   * @exec: drm_exec object
> @@ -119,9 +108,18 @@ _label:									 \
>   * Core functionality of the drm_exec object. Loops until all GEM objects are
>   * locked and no more contention exists. At the beginning of the loop it is
>   * guaranteed that no GEM object is locked.
> + *
> + * A global label name drm_exec_retry is used, if you need to use more than one
> + * instance of this macro in the same function the label needs to be made local
> + * to the block with the __label__ keyword.
>   */
>  #define drm_exec_until_all_locked(exec)					\
> -	__drm_exec_until_all_locked(exec, __UNIQUE_ID(drm_exec))
> +	for (bool const __maybe_unused __drm_exec_loop = false;		\
> +	     drm_exec_cleanup(exec);)					\
> +		if (false) {						\
> +drm_exec_retry: __maybe_unused;						\
> +			continue;					\
> +		} else
>  
>  /**
>   * drm_exec_retry_on_contention - restart the loop to grap all locks
> @@ -129,12 +127,14 @@ _label:									 \
>   *
>   * Control flow helper to continue when a contention was detected and we need to
>   * clean up and re-start the loop to prepare all GEM objects.
> + * The __drm_exec_loop check exists to prevent usage outside of an
> + * drm_exec_until_all_locked() loop.
>   */
>  #define drm_exec_retry_on_contention(exec)			\
>  	do {							\
>  		if (unlikely(drm_exec_is_contended(exec)))	\
> -			goto *__drm_exec_retry_ptr;		\
> -	} while (0)
> +			goto drm_exec_retry;			\
> +	} while (__drm_exec_loop)
>  
>  /**
>   * drm_exec_is_contended - check for contention
> @@ -154,12 +154,14 @@ static inline bool drm_exec_is_contended(struct drm_exec *exec)
>   *
>   * Unconditionally retry the loop to lock all objects. For consistency,
>   * the exec object needs to be newly initialized.
> + * The __drm_exec_loop check exists to prevent usage outside of an
> + * drm_exec_until_all_locked() loop.
>   */
>  #define drm_exec_retry(_exec)					\
>  	do {							\
>  		WARN_ON((_exec)->contended != DRM_EXEC_DUMMY);	\
> -		goto *__drm_exec_retry_ptr;			\
> -	} while (0)
> +		goto drm_exec_retry;				\
> +	} while (__drm_exec_loop)
>  
>  /**
>   * drm_exec_ticket - return the ww_acquire_ctx for this exec context