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

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

Hi Christian,

On Thu,  4 May 2023 13:51:47 +0200
"Christian König" <ckoenig.leichtzumerken@gmail.com> wrote:

> This adds the infrastructure for an execution context for GEM buffers
> which is similar to the existing TTMs execbuf util and intended to replace
> it in the long term.
> 
> The basic functionality is that we abstracts the necessary loop to lock
> many different GEM buffers with automated deadlock and duplicate handling.

As many other drivers do already, we are considering using drm_exec()
for our resv locking in the PowerVR driver, so we might have more
questions/comments in the coming days/weeks, but I already have a
couple right now (see below).

> v3: drop duplicate tracking, radeon is really the only one needing that

I think we'd actually be interested in duplicate tracking. Is there any
way we can make it an optional feature through some extra helpers/flags?
Doesn't have to be done in this patch series, I'm just wondering if this
is something we can share as well.

[...]

> +/**
> + * 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;

I guess we could even add static checks to make sure
drm_exec_try_prepare_obj() is called inside a
drm_exec_while_not_all_locked() loop.

> + *		ret = drm_exec_prepare_obj(&exec, boB, 1);
> + *		drm_exec_continue_on_contention(&exec);
> + *		if (ret)
> + *			goto error;
> + *	}
> + *
> + *	drm_exec_for_each_locked_object(&exec, index, obj) {
> + *		dma_resv_add_fence(obj->resv, fence, DMA_RESV_USAGE_READ);
> + *		...
> + *	}
> + *	drm_exec_fini(&exec);
> + *
> + * See struct dma_exec for more details.
> + */

[...]

> +/**
> + * drm_exec_prepare_array - helper to prepare an array of objects
> + * @exec: the drm_exec object with the state
> + * @objects: array of GEM object to prepare
> + * @num_objects: number of GEM objects in the array
> + * @num_fences: number of fences to reserve on each GEM object
> + *
> + * Prepares all GEM objects in an array, handles contention but aports on first
> + * error otherwise. Reserves @num_fences on each GEM object after locking it.
> + *
> + * Returns: -EALREADY when object is already locked, -ENOMEM when memory
> + * allocation failed and zero for success.
> + */
> +int drm_exec_prepare_array(struct drm_exec *exec,
> +			   struct drm_gem_object **objects,
> +			   unsigned int num_objects,
> +			   unsigned int num_fences)
> +{
> +	int ret;
> +
> +	drm_exec_while_not_all_locked(exec) {
> +		for (unsigned int i = 0; i < num_objects; ++i) {
> +			ret = drm_exec_prepare_obj(exec, objects[i],
> +						   num_fences);
> +			drm_exec_break_on_contention(exec);

I had a hard time understanding what the intent was here: we do want the
locking to keep going on contention (reset and retry), but we need to
break out of the inner loop for this to happen, which is what this
drm_exec_break_on_contention() is doing. My misunderstanding was coming
from the fact I was expecting drm_exec_break_on_contention() to stop
the process of preparing objects. Maybe it's just me, but I think it'd
be less confusing if we were getting rid of
drm_exec_{break,continue}_on_contention and have the loop slightly
adjusted:

	unsigned int obj_ptr = 0;

	drm_exec_while_not_all_locked(exec) {
		int ret;

		/* We acquired/prepared all objects, we can leave the loop now. */
		if (obj_ptr == num_objects)
			break;

		ret = drm_exec_try_prepare_obj_or_retry(exec, objects[obj_ptr++],
							num_fences);
		if (ret)
			return ret;
	}

	return 0;

Of course, this is just my personal view on this, and none of these
comments should be considered as blockers, but I thought I'd share
my concerns anyway.

Thanks again for your work!

Regards,

Boris