Hello Thomas,
On Mon, 19 Jun 2023 10:59:16 +0200
Thomas Hellström (Intel) <thomas_os@xxxxxxxxxxxx> wrote:
> >>>>
> >>>>> +/**
> >>>>> + * DOC: Overview
> >>>>> + *
> >>>>> + * This component mainly abstracts the retry loop necessary for locking
> >>>>> + * multiple GEM objects while preparing hardware operations (e.g. command
> >>>>> + * submissions, page table updates etc..).
> >>>>> + *
> >>>>> + * If a contention is detected while locking a GEM object the cleanup procedure
> >>>>> + * unlocks all previously locked GEM objects and locks the contended one first
> >>>>> + * before locking any further objects.
> >>>>> + *
> >>>>> + * After an object is locked fences slots can optionally be reserved on the
> >>>>> + * dma_resv object inside the GEM object.
> >>>>> + *
> >>>>> + * A typical usage pattern should look like this::
> >>>>> + *
> >>>>> + * struct drm_gem_object *obj;
> >>>>> + * struct drm_exec exec;
> >>>>> + * unsigned long index;
> >>>>> + * int ret;
> >>>>> + *
> >>>>> + * drm_exec_init(&exec, true);
> >>>>> + * drm_exec_while_not_all_locked(&exec) {
> >>>>> + * ret = drm_exec_prepare_obj(&exec, boA, 1);
> >>>>> + * drm_exec_continue_on_contention(&exec);
> >>>>> + * if (ret)
> >>>>> + * goto error;
> >>>>> + *
> >>>> Have you considered defining a drm_exec_try_prepare_obj_or_retry()
> >>>> combining drm_exec_prepare_obj() and drm_exec_continue_on_contention()?
> >>>>
> >>>> #define drm_exec_try_prepare_obj_or_retry(exec, obj, num_fences) \
> >>>> ({ \
> >>>> int __ret = drm_exec_prepare_obj(exec, bo, num_fences); \
> >>>> if (unlikely(drm_exec_is_contended(exec))) \
> >>>> continue; \
> >>>> __ret; \
> >>>> })
> >>>>
> >>>> This way the following pattern
> >>>>
> >>>> ret = drm_exec_prepare_obj(&exec, boA, 1);
> >>>> drm_exec_continue_on_contention(&exec);
> >>>> if (ret)
> >>>> goto error;
> >>>>
> >>>> can be turned into something more conventional:
> >>>>
> >>>> ret = drm_exec_try_prepare_obj_or_retry(&exec, boA, 1);
> >>>> if (ret)
> >>>> goto error;
> >>> Yeah, I was considering that as well. But then abandoned it as to
> >>> complicated.
> >>>
> >>> I really need to find some time to work on that anyway.
> > I've been playing with drm_exec for a couple weeks now, and I wanted
> > to share something I hacked to try and make the API simpler and
> > more robust against misuse (see the below diff, which is a slightly
> > adjusted version of your work).
>
> It would be good if we could have someone taking charge of this series
> and address all review comments, I see some of my comments getting lost,
> we have multiple submitters and I can't find a dri-devel patchwork entry
> for this.
My bad, I wasn't intending to submit a new version. I just added a
diff to show what I had in mind. This being said, it'd be great if we
could make some progress on this series, because we have quite a few
drivers depending on it now.
>
> >
> > In this version, the user is no longer in control of the retry
> > loop. Instead, it provides an expression (a call to a
> > sub-function) to be re-evaluated each time a contention is
> > detected. IMHO, this makes the 'prepare-objs' functions easier to
> > apprehend, and avoids any mistake like calling
> > drm_exec_continue_on_contention() in an inner loop, or breaking
> > out of the drm_exec_while_all_locked() loop unintentionally.
>
> In i915 we've had a very similar helper to this, and while I agree this
> newer version would probably help make code cleaner, but OTOH there also
> are some places where the short drm_exec_while_all_locked() -likeblock
> don't really motivate a separate function. Porting i915 to the current
> version will take some work, For the xe driver both versions would work
> fine.
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.
Regards,
Boris
