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 <[email protected]> > Signed-off-by: Christian König <[email protected]> > --- > 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
