On Wed, Aug 2, 2023 at 1:44 AM Boris Brezillon
<boris.brezillon@xxxxxxxxxxxxx> wrote:
>
> On Tue, 1 Aug 2023 13:35:13 -0700
> Nick Desaulniers <ndesaulniers@xxxxxxxxxx> wrote:
>
> > On Mon, Jul 31, 2023 at 5:36 AM Christian König
> > <ckoenig.leichtzumerken@xxxxxxxxx> wrote:
> > >
> > > GCC forbids to jump to labels in loop conditions and a new clang
> > > check stumbled over this.
> > >
> > > So instead using a local label inside the loop condition use an
> > > unique label outside of it.
> > >
> > > Fixes: commit 09593216bff1 ("drm: execution context for GEM buffers v7")
> > > Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/1890
> > > Link: https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067
> > > Reported-by: Nathan Chancellor <nathan@xxxxxxxxxx>
> > > Reported-by: Naresh Kamboju <naresh.kamboju@xxxxxxxxxx>
> > > CC: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> >
> > Works for me; thanks for the patch!
> > Reviewed-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> >
> > I suspect it's possible to change the indirect goto into a direct goto
> > with some further refactoring (macros can take block statements; if
> > drm_exec_until_all_locked accepted a block statement arg then you
> > could introduce a new scope, and a new local label to that scope, then
> > just use direct goto),
>
> Maybe I'm wrong, but this sounds like the version I proposed here [1].
Nearly; here's what I was imagining:
```
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 977e1804718d..3ea8beb159f0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -904,7 +904,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
e->user_invalidated = userpage_invalidated;
}
- drm_exec_until_all_locked(&p->exec) {
+ drm_exec_until_all_locked(&p->exec, {
r = amdgpu_vm_lock_pd(&fpriv->vm, &p->exec, 1 + p->gang_size);
drm_exec_retry_on_contention(&p->exec);
if (unlikely(r))
@@ -928,7 +928,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
if (unlikely(r))
goto out_free_user_pages;
}
- }
+ })
amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
struct mm_struct *usermm;
diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
index 73205afec162..8e32a9b704e7 100644
--- a/include/drm/drm_exec.h
+++ b/include/drm/drm_exec.h
@@ -74,14 +74,13 @@ struct drm_exec {
* 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); \
- });)
+#define drm_exec_until_all_locked(exec, block) \
+ { \
+ __label__ __drm_exec_retry; \
+__drm_exec_retry: \
+ while (drm_exec_cleanup(exec)) \
+ block \
+}
/**
* drm_exec_retry_on_contention - restart the loop to grap all locks
@@ -93,7 +92,7 @@ __drm_exec_retry:
\
#define drm_exec_retry_on_contention(exec) \
do { \
if (unlikely(drm_exec_is_contended(exec))) \
- goto *__drm_exec_retry_ptr; \
+ goto __drm_exec_retry; \
} while (0)
/**
```
(only updated one macro expansion site in
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c, didn't add proper trailing
tabs to macro but you get the gist).
But I think both compilers can optimize out the unnecessary
indirection when it's obvious, so I don't think it matters much, other
than the tastes of whoever has to maintain this.
>
> > but this will probably apply cleaner. (oh, is
> > 09593216bff1 only in next at the moment? The AuthorDate threw me.)
> >
> > There are some curious cases where __attribute__((cleanup())) doesn't
> > mesh well with indirect gotos.
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=37722
> >
> > May not ever be a problem here...
>
> [1]https://patchwork.freedesktop.org/patch/543077/
--
Thanks,
~Nick Desaulniers
