On Mon, 19 Jun 2023 13:05:02 +0200
Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:
> On Mon, 19 Jun 2023 12:44:06 +0200
> Christian König <christian.koenig@xxxxxxx> wrote:
>
> > Am 19.06.23 um 12:12 schrieb Boris Brezillon:
> > > [SNIP]
> > > Note that the drm_exec_until_all_locked() helper I introduced is taking
> > > an expression, so in theory, you don't have to define a separate
> > > function.
> > >
> > > drm_exec_until_all_locked(&exec, {
> > > /* inlined-code */
> > > int ret;
> > >
> > > ret = blabla()
> > > if (ret)
> > > goto error;
> > >
> > > ...
> > >
> > > error:
> > > /* return value. */
> > > ret;
> > > });
> > >
> > > This being said, as soon as you have several failure paths,
> > > it makes things a lot easier/controllable if you make it a function,
> > > and I honestly don't think the readability would suffer from having a
> > > function defined just above the user. My main concern with the original
> > > approach was the risk of calling continue/break_if_contended() in the
> > > wrong place, and also the fact you can't really externalize things to
> > > a function if you're looking for a cleaner split. At least with
> > > drm_exec_until_all_locked() you can do both.
> >
> > Yeah, but that means that you can't use return inside your code block
> > and instead has to define an error label for handling "normal"
> > contention which is what I'm trying to avoid here.
> >
> > How about:
> >
> > #define drm_exec_until_all_locked(exec) \
> > __drm_exec_retry: if (drm_exec_cleanup(exec))
> >
> >
> > #define drm_exec_retry_on_contention(exec) \
> > if (unlikely(drm_exec_is_contended(exec))) \
> > goto __drm_exec_retry
> >
> >
> > And then use it like:
> >
> > drm_exec_until_all_locked(exec)
> > {
> > ret = drm_exec_prepare_obj(exec, obj);
> > drm_exec_retry_on_contention(exec);
> > }
>
> That would work, and I was about to suggest extending my proposal with
> a drm_exec_retry_on_contention() to support both use cases. The only
> downside is the fact you might be able to break out of a loop that has
> local variables, which will leak stack space.
Nevermind, brain fart on my end. It shouldn't leak any stack space, so
yeah, I think that's a good compromise.
