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