Hi Christian,
On Thu, 4 May 2023 13:51:47 +0200
"Christian König" <ckoenig.leichtzumerken@xxxxxxxxx> 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
