Hi Christian,
On Tue, Jul 11, 2023 at 03:31:17PM +0200, Christian König wrote:
> This adds the infrastructure for an execution context for GEM buffers
> which is similar to the existing TTMs execbuf util and intended to replace
> it in the long term.
>
> The basic functionality is that we abstracts the necessary loop to lock
> many different GEM buffers with automated deadlock and duplicate handling.
>
> v2: drop xarray and use dynamic resized array instead, the locking
> overhead is unnecessary and measurable.
> v3: drop duplicate tracking, radeon is really the only one needing that.
> v4: fixes issues pointed out by Danilo, some typos in comments and a
> helper for lock arrays of GEM objects.
> v5: some suggestions by Boris Brezillon, especially just use one retry
> macro, drop loop in prepare_array, use flags instead of bool
> v6: minor changes suggested by Thomas, Boris and Danilo
> v7: minor typos pointed out by checkpatch.pl fixed
>
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> Reviewed-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> Reviewed-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> Tested-by: Danilo Krummrich <dakr@xxxxxxxxxx>
<snip>
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> new file mode 100644
> index 000000000000..73205afec162
> --- /dev/null
> +++ b/include/drm/drm_exec.h
<snip>
> + * Since labels can't be defined local to the loops body we use a jump pointer
> + * to make sure that the retry is only used from within the loops body.
> + */
> +#define drm_exec_until_all_locked(exec) \
> + for (void *__drm_exec_retry_ptr; ({ \
> + __label__ __drm_exec_retry; \
> +__drm_exec_retry: \
> + __drm_exec_retry_ptr = &&__drm_exec_retry; \
> + (void)__drm_exec_retry_ptr; \
> + drm_exec_cleanup(exec); \
> + });)
> +
> +/**
> + * drm_exec_retry_on_contention - restart the loop to grap all locks
> + * @exec: drm_exec object
> + *
> + * 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.
> + */
> +#define drm_exec_retry_on_contention(exec) \
> + do { \
> + if (unlikely(drm_exec_is_contended(exec))) \
> + goto *__drm_exec_retry_ptr; \
> + } while (0)
This construct of using a label as a value and goto to jump into a GNU
C statement expression is explicitly documented in GCC's manual [1] as
undefined behavior:
"Jumping into a statement expression with a computed goto (see Labels as
Values [2]) has undefined behavior. "
A recent change in clang [3] to prohibit this altogether points this out, so
builds using clang-17 and newer will break with this change applied:
drivers/gpu/drm/tests/drm_exec_test.c:41:3: error: cannot jump from this indirect goto statement to one of its possible targets
41 | drm_exec_retry_on_contention(&exec);
| ^
include/drm/drm_exec.h:96:4: note: expanded from macro 'drm_exec_retry_on_contention'
96 | goto *__drm_exec_retry_ptr; \
| ^
drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: possible target of indirect goto statement
39 | drm_exec_until_all_locked(&exec) {
| ^
include/drm/drm_exec.h:79:33: note: expanded from macro 'drm_exec_until_all_locked'
79 | __label__ __drm_exec_retry; \
| ^
drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: jump enters a statement expression
It seems like if this construct works, it is by chance, although I am
not sure if there is another solution.
[1]: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
[2]: https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html
[3]: https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067
Cheers,
Nathan
