On Wed, 14 Jun 2023 14:30:53 +0200
Christian König <christian.koenig@xxxxxxx> wrote:
> Am 14.06.23 um 14:23 schrieb Boris Brezillon:
> > 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.
>
> You can still capture the -EALREADY error and act appropriately in your
> driver.
>
> For radeon it just means ignoring the error code and going ahead, but
> that behavior doesn't seem to be desired in most cases.
>
> Initially I though we need to separately track how many and how often
> BOs are duplicated, but there is simply no use for this.
>
> >
> > [...]
> >
> >> +/**
> >> + * 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 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.
>
> Interesting idea, but how would somebody do that?
There are probably better/cleaner ways, but the below diff
seems to catch cases where drm_exec_try_prepare_obj() is
called in a context where break/continue are allowed, but
that's not inside a drm_exec_while_not_all_locked() section.
What's missing though is a way to detect when it's called
from an inner loop.
---
diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
index 7c7481ed088a..1f4e0e1a7783 100644
--- a/include/drm/drm_exec.h
+++ b/include/drm/drm_exec.h
@@ -69,8 +69,10 @@ struct drm_exec {
*
* At the beginning of the loop it is guaranteed that no GEM object is locked.
*/
+#define __in_drm_exec_while_not_all_locked false
#define drm_exec_while_not_all_locked(exec) \
- while (drm_exec_cleanup(exec))
+ for (const bool __in_drm_exec_while_not_all_locked = true; \
+ drm_exec_cleanup(exec); )
/**
* drm_exec_continue_on_contention - continue the loop when we need to cleanup
@@ -83,6 +85,25 @@ struct drm_exec {
if (unlikely(drm_exec_is_contended(exec))) \
continue
+/**
+ * drm_exec_try_prepare_obj - Try prepare an object and retry on contention
+ * @exec: drm_exec object
+ * @obj: GEM object to prepare
+ * @num_fence: number of fence slots to reserve
+ *
+ * Wrapper around drm_exec_prepare_obj() that automatically retries on
+ * contention by going back to the head of the drm_exec_while_not_all_locked()
+ * loop.
+ */
+#define drm_exec_try_prepare_obj(exec, obj, num_fences) \
+ ({ \
+ int __ret = drm_exec_prepare_obj(exec, obj, num_fences); \
+ static_assert(__in_drm_exec_while_not_all_locked == true); \
+ if (unlikely(drm_exec_is_contended(exec))) \
+ continue; \
+ __ret; \
+ })
+
/**
* drm_exec_break_on_contention - break a subordinal loop on contention
* @exec: drm_exec object
