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

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

On Wed, 14 Jun 2023 14:30:53 +0200
Christian König <christian.koenig@amd.com> 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@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.  
> 
> 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