On Sun, 23 Jul 2023 at 03:36, Nathan Chancellor <nathan@xxxxxxxxxx> wrote:
>
> 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);
> | ^
LKFT also noticed these build failures on arm and arm64 with clang nightly.
> 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.
Thanks for looking into this problem.
>
> [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
- Naresh
