Re: [PATCH 1/6] drm: execution context for GEM buffers v7
On Sun, 23 Jul 2023 at 03:36, Nathan Chancellor wrote:
>
> Hi Christian,
>
> On Tue, Jul 11, 2023 at 03:31:17PM +0200, Christian König 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.
> >
> > v2: drop xarray and use dynamic resized array instead, the locking
> > overhead is unnecessary and measurable.
> > v3: drop duplicate tracking, radeon is really the only one needing that.
> > v4: fixes issues pointed out by Danilo, some typos in comments and a
> > helper for lock arrays of GEM objects.
> > v5: some suggestions by Boris Brezillon, especially just use one retry
> > macro, drop loop in prepare_array, use flags instead of bool
> > v6: minor changes suggested by Thomas, Boris and Danilo
> > v7: minor typos pointed out by checkpatch.pl fixed
> >
> > Signed-off-by: Christian König
> > Reviewed-by: Boris Brezillon
> > Reviewed-by: Danilo Krummrich
> > Tested-by: Danilo Krummrich
>
>
>
> > diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> > new file mode 100644
> > index ..73205afec162
> > --- /dev/null
> > +++ b/include/drm/drm_exec.h
>
>
>
> > + * Since labels can't be defined local to the loops body we use a jump
> > pointer
> > + * to make sure that the retry is only used from within the loops body.
> > + */
> > +#define drm_exec_until_all_locked(exec) \
> > + for (void *__drm_exec_retry_ptr; ({ \
> > + __label__ __drm_exec_retry; \
> > +__drm_exec_retry:\
> > + __drm_exec_retry_ptr = &&__drm_exec_retry; \
> > + (void)__drm_exec_retry_ptr; \
> > + drm_exec_cleanup(exec); \
> > + });)
> > +
> > +/**
> > + * drm_exec_retry_on_contention - restart the loop to grap all locks
> > + * @exec: drm_exec object
> > + *
> > + * Control flow helper to continue when a contention was detected and we
> > need to
> > + * clean up and re-start the loop to prepare all GEM objects.
> > + */
> > +#define drm_exec_retry_on_contention(exec) \
> > + do {\
> > + if (unlikely(drm_exec_is_contended(exec))) \
> > + goto *__drm_exec_retry_ptr; \
> > + } while (0)
>
> This construct of using a label as a value and goto to jump into a GNU
> C statement expression is explicitly documented in GCC's manual [1] as
> undefined behavior:
>
> "Jumping into a statement expression with a computed goto (see Labels as
> Values [2]) has undefined behavior. "
>
> A recent change in clang [3] to prohibit this altogether points this out, so
> builds using clang-17 and newer will break with this change applied:
>
> drivers/gpu/drm/tests/drm_exec_test.c:41:3: error: cannot jump from this
> indirect goto statement to one of its possible targets
> 41 | drm_exec_retry_on_contention();
> | ^
LKFT also noticed these build failures on arm and arm64 with clang nightly.
> include/drm/drm_exec.h:96:4: note: expanded from macro
> 'drm_exec_retry_on_contention'
> 96 | goto *__drm_exec_retry_ptr; \
> | ^
> drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: possible target of
> indirect goto statement
> 39 | drm_exec_until_all_locked() {
> | ^
> include/drm/drm_exec.h:79:33: note: expanded from macro
> 'drm_exec_until_all_locked'
> 79 | __label__ __drm_exec_retry; \
> | ^
> drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: jump enters a statement
> expression
>
> It seems like if this construct works, it is by chance, although I am
> not sure if there is another solution.
Thanks for looking into this problem.
>
> [1]: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
> [2]: https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html
> [3]:
> https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067
>
> Cheers,
> Nathan
- Naresh
Re: [PATCH 1/6] drm: execution context for GEM buffers v7
Hi Christian,
On Tue, Jul 11, 2023 at 03:31:17PM +0200, Christian König 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.
>
> v2: drop xarray and use dynamic resized array instead, the locking
> overhead is unnecessary and measurable.
> v3: drop duplicate tracking, radeon is really the only one needing that.
> v4: fixes issues pointed out by Danilo, some typos in comments and a
> helper for lock arrays of GEM objects.
> v5: some suggestions by Boris Brezillon, especially just use one retry
> macro, drop loop in prepare_array, use flags instead of bool
> v6: minor changes suggested by Thomas, Boris and Danilo
> v7: minor typos pointed out by checkpatch.pl fixed
>
> Signed-off-by: Christian König
> Reviewed-by: Boris Brezillon
> Reviewed-by: Danilo Krummrich
> Tested-by: Danilo Krummrich
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> new file mode 100644
> index ..73205afec162
> --- /dev/null
> +++ b/include/drm/drm_exec.h
> + * Since labels can't be defined local to the loops body we use a jump
> pointer
> + * to make sure that the retry is only used from within the loops body.
> + */
> +#define drm_exec_until_all_locked(exec) \
> + for (void *__drm_exec_retry_ptr; ({ \
> + __label__ __drm_exec_retry; \
> +__drm_exec_retry:\
> + __drm_exec_retry_ptr = &&__drm_exec_retry; \
> + (void)__drm_exec_retry_ptr; \
> + drm_exec_cleanup(exec); \
> + });)
> +
> +/**
> + * drm_exec_retry_on_contention - restart the loop to grap all locks
> + * @exec: drm_exec object
> + *
> + * Control flow helper to continue when a contention was detected and we
> need to
> + * clean up and re-start the loop to prepare all GEM objects.
> + */
> +#define drm_exec_retry_on_contention(exec) \
> + do {\
> + if (unlikely(drm_exec_is_contended(exec))) \
> + goto *__drm_exec_retry_ptr; \
> + } while (0)
This construct of using a label as a value and goto to jump into a GNU
C statement expression is explicitly documented in GCC's manual [1] as
undefined behavior:
"Jumping into a statement expression with a computed goto (see Labels as
Values [2]) has undefined behavior. "
A recent change in clang [3] to prohibit this altogether points this out, so
builds using clang-17 and newer will break with this change applied:
drivers/gpu/drm/tests/drm_exec_test.c:41:3: error: cannot jump from this
indirect goto statement to one of its possible targets
41 | drm_exec_retry_on_contention();
| ^
include/drm/drm_exec.h:96:4: note: expanded from macro
'drm_exec_retry_on_contention'
96 | goto *__drm_exec_retry_ptr; \
| ^
drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: possible target of indirect
goto statement
39 | drm_exec_until_all_locked() {
| ^
include/drm/drm_exec.h:79:33: note: expanded from macro
'drm_exec_until_all_locked'
79 | __label__ __drm_exec_retry; \
| ^
drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: jump enters a statement
expression
It seems like if this construct works, it is by chance, although I am
not sure if there is another solution.
[1]: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
[2]: https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html
[3]:
https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067
Cheers,
Nathan
[PATCH 1/6] drm: execution context for GEM buffers v7
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.
v2: drop xarray and use dynamic resized array instead, the locking
overhead is unnecessary and measurable.
v3: drop duplicate tracking, radeon is really the only one needing that.
v4: fixes issues pointed out by Danilo, some typos in comments and a
helper for lock arrays of GEM objects.
v5: some suggestions by Boris Brezillon, especially just use one retry
macro, drop loop in prepare_array, use flags instead of bool
v6: minor changes suggested by Thomas, Boris and Danilo
v7: minor typos pointed out by checkpatch.pl fixed
Signed-off-by: Christian König
Reviewed-by: Boris Brezillon
Reviewed-by: Danilo Krummrich
Tested-by: Danilo Krummrich
---
Documentation/gpu/drm-mm.rst | 12 ++
drivers/gpu/drm/Kconfig | 6 +
drivers/gpu/drm/Makefile | 2 +
drivers/gpu/drm/drm_exec.c | 333 +++
include/drm/drm_exec.h | 123 +
5 files changed, 476 insertions(+)
create mode 100644 drivers/gpu/drm/drm_exec.c
create mode 100644 include/drm/drm_exec.h
diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
index a79fd3549ff8..a52e6f4117d6 100644
--- a/Documentation/gpu/drm-mm.rst
+++ b/Documentation/gpu/drm-mm.rst
@@ -493,6 +493,18 @@ DRM Sync Objects
.. kernel-doc:: drivers/gpu/drm/drm_syncobj.c
:export:
+DRM Execution context
+=
+
+.. kernel-doc:: drivers/gpu/drm/drm_exec.c
+ :doc: Overview
+
+.. kernel-doc:: include/drm/drm_exec.h
+ :internal:
+
+.. kernel-doc:: drivers/gpu/drm/drm_exec.c
+ :export:
+
GPU Scheduler
=
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index cc9c9947fdef..c0b4063a3ee6 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -194,6 +194,12 @@ config DRM_TTM
GPU memory types. Will be enabled automatically if a device driver
uses it.
+config DRM_EXEC
+ tristate
+ depends on DRM
+ help
+ Execution context for command submissions
+
config DRM_BUDDY
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1c0f5204e47b..021b3f0ac152 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -78,6 +78,8 @@ obj-$(CONFIG_DRM_PANEL_ORIENTATION_QUIRKS) +=
drm_panel_orientation_quirks.o
#
# Memory-management helpers
#
+#
+obj-$(CONFIG_DRM_EXEC) += drm_exec.o
obj-$(CONFIG_DRM_BUDDY) += drm_buddy.o
diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c
new file mode 100644
index ..ff69cf0fb42a
--- /dev/null
+++ b/drivers/gpu/drm/drm_exec.c
@@ -0,0 +1,333 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+#include
+#include
+#include
+
+/**
+ * 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(, DRM_EXEC_INTERRUPTIBLE_WAIT);
+ * drm_exec_until_all_locked() {
+ * ret = drm_exec_prepare_obj(, boA, 1);
+ * drm_exec_retry_on_contention();
+ * if (ret)
+ * goto error;
+ *
+ * ret = drm_exec_prepare_obj(, boB, 1);
+ * drm_exec_retry_on_contention();
+ * if (ret)
+ * goto error;
+ * }
+ *
+ * drm_exec_for_each_locked_object(, index, obj) {
+ * dma_resv_add_fence(obj->resv, fence, DMA_RESV_USAGE_READ);
+ * ...
+ * }
+ * drm_exec_fini();
+ *
+ * See struct dma_exec for more details.
+ */
+
+/* Dummy value used to initially enter the retry loop */
+#define DRM_EXEC_DUMMY ((void *)~0)
+
+/* Unlock all objects and drop references */
+static void drm_exec_unlock_all(struct drm_exec *exec)
+{
+ struct drm_gem_object *obj;
+ unsigned long index;
+
+ drm_exec_for_each_locked_object(exec, index, obj) {
+ dma_resv_unlock(obj->resv);
+ drm_gem_object_put(obj);
+ }
+
+ drm_gem_object_put(exec->prelocked);
+ exec->prelocked = NULL;
+}
+
+/**
+ * drm_exec_init - initialize a drm_exec object
+ * @exec: the
