Re: [PATCH 01/13] drm: execution context for GEM buffers v4

[Boris Brezillon <boris.brezillon@collabora.com>]

Hello Thomas,

On Mon, 19 Jun 2023 10:59:16 +0200
Thomas Hellström (Intel) <thomas_os@shipmail.org> 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