[PATCH 0/6 RFC] drm: Move vmap out of commit tail for SHMEM drivers

[PATCH 0/6 RFC] drm: Move vmap out of commit tail for SHMEM drivers

[Thomas Zimmermann]

Several SHMEM-based drivers use the BO as shadow buffer for the real
framebuffer memory. Damage handling requires a vmap of the BO memory.
Vmap/vunmap can acquire the dma-buf reservation lock, which is not
allowed in commit tails.

This patchset introduces a set of helpers that implement vmap/vunmap
in the plane's prepare_fb and cleanup_fb; and provide the mapping's
address to commit-tail functions. Wrapper sfor simple KMS are provides,
as all of the involved drivers are built upon simple-KMS helpers.

Patch 1 adds the support for private plane state to the simple-KMS
helper library.

Patch 2 provides plane state for SHMEM-based shadow planes. It's a
DRM plane with BO mappings into kernel address space. The involved
helpers take care of the details.

Patches 3 to 6 convert each involved driver to the new shadow-plane
helpers. The vmap operations are being removed from commit-tail
functions. An additional benefit is that vmap errors are now detected
before the commit starts. The original commit-tail functions could not
handle failed vmap operations.

I smoke-tested the code by running fbdev, Xorg and weston with the
converted mgag200 driver.

Thomas Zimmermann (6):
  drm/simple-kms: Add plane-state helpers
  drm/shmem-helper: Add additional KMS helpers
  drm/mgag200: Move vmap out of commit tail
  drm/cirrus: Move vmap out of commit tail
  drm/gm12u320: Move vmap out of commit tail
  drm/udl: Move vmap out of commit tail

 drivers/gpu/drm/Kconfig                    |   7 +
 drivers/gpu/drm/Makefile                   |   1 +
 drivers/gpu/drm/drm_gem_shmem_kms_helper.c | 159 +++++++++++++++++++++
 drivers/gpu/drm/drm_simple_kms_helper.c    |  40 +++++-
 drivers/gpu/drm/mgag200/Kconfig            |   1 +
 drivers/gpu/drm/mgag200/mgag200_mode.c     |  25 ++--
 drivers/gpu/drm/tiny/Kconfig               |   6 +-
 drivers/gpu/drm/tiny/cirrus.c              |  45 +++---
 drivers/gpu/drm/tiny/gm12u320.c            |  30 ++--
 drivers/gpu/drm/udl/Kconfig                |   1 +
 drivers/gpu/drm/udl/udl_modeset.c          |  36 ++---
 include/drm/drm_gem_shmem_kms_helper.h     |  56 ++++++++
 include/drm/drm_simple_kms_helper.h        |  28 ++++
 13 files changed, 353 insertions(+), 82 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_gem_shmem_kms_helper.c
 create mode 100644 include/drm/drm_gem_shmem_kms_helper.h


base-commit: 1e37c3960fd3910f3f65cd6927a6ebab8e8efc26
--
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/6] drm/simple-kms: Add plane-state helpers

[Thomas Zimmermann]

Just like regular plane-state helpers, drivers can use these new
callbacks to create and destroy private plane state.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_simple_kms_helper.c | 40 +++++++++++++++++++++++--
 include/drm/drm_simple_kms_helper.h     | 28 +++++++++++++++++
 2 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index 6ce8f5cd1eb5..0c5bb0f98fa0 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -253,13 +253,47 @@ static const struct drm_plane_helper_funcs drm_simple_kms_plane_helper_funcs = {
 	.atomic_update = drm_simple_kms_plane_atomic_update,
 };
 
+static void drm_simple_kms_plane_reset(struct drm_plane *plane)
+{
+	struct drm_simple_display_pipe *pipe;
+
+	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
+	if (!pipe->funcs || !pipe->funcs->reset_plane)
+		return drm_atomic_helper_plane_reset(plane);
+
+	return pipe->funcs->reset_plane(pipe);
+}
+
+static struct drm_plane_state *drm_simple_kms_plane_duplicate_state(struct drm_plane *plane)
+{
+	struct drm_simple_display_pipe *pipe;
+
+	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
+	if (!pipe->funcs || !pipe->funcs->duplicate_plane_state)
+		return drm_atomic_helper_plane_duplicate_state(plane);
+
+	return pipe->funcs->duplicate_plane_state(pipe, plane->state);
+}
+
+static void drm_simple_kms_plane_destroy_state(struct drm_plane *plane,
+					       struct drm_plane_state *state)
+{
+	struct drm_simple_display_pipe *pipe;
+
+	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
+	if (!pipe->funcs || !pipe->funcs->destroy_plane_state)
+		drm_atomic_helper_plane_destroy_state(plane, state);
+	else
+		pipe->funcs->destroy_plane_state(pipe, state);
+}
+
 static const struct drm_plane_funcs drm_simple_kms_plane_funcs = {
 	.update_plane		= drm_atomic_helper_update_plane,
 	.disable_plane		= drm_atomic_helper_disable_plane,
 	.destroy		= drm_plane_cleanup,
-	.reset			= drm_atomic_helper_plane_reset,
-	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
-	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
+	.reset			= drm_simple_kms_plane_reset,
+	.atomic_duplicate_state	= drm_simple_kms_plane_duplicate_state,
+	.atomic_destroy_state	= drm_simple_kms_plane_destroy_state,
 	.format_mod_supported   = drm_simple_kms_format_mod_supported,
 };
 
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
index e6dbf3161c2f..0c1a2e07caf2 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -149,6 +149,34 @@ struct drm_simple_display_pipe_funcs {
 	 * more details.
 	 */
 	void (*disable_vblank)(struct drm_simple_display_pipe *pipe);
+
+	/**
+	 * @reset_plane:
+	 *
+	 * Optional, called by &drm_plane_funcs.reset. Please read the
+	 * documentation for the &drm_plane_funcs.reset hook for more details.
+	 */
+	void (*reset_plane)(struct drm_simple_display_pipe *pipe);
+
+	/**
+	 * @duplicate_plane_state:
+	 *
+	 * Optional, called by &drm_plane_funcs.atomic_duplicate_state.  Please
+	 * read the documentation for the &drm_plane_funcs.atomic_duplicate_state
+	 * hook for more details.
+	 */
+	struct drm_plane_state * (*duplicate_plane_state)(struct drm_simple_display_pipe *pipe,
+							  struct drm_plane_state *plane_state);
+
+	/**
+	 * @destroy_plane_state:
+	 *
+	 * Optional, called by &drm_plane_funcs.atomic_destroy_state.  Please
+	 * read the documentation for the &drm_plane_funcs.atomic_destroy_state
+	 * hook for more details.
+	 */
+	void (*destroy_plane_state)(struct drm_simple_display_pipe *pipe,
+				    struct drm_plane_state *plane_state);
 };
 
 /**
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/6] drm/shmem-helper: Add additional KMS helpers

[Thomas Zimmermann]

Several drivers use GEM SHMEM buffer objects as shadow buffers for
the actual framebuffer memory. Right now, drivers do these vmap
operations in their commit tail, which is actually not allowed by the
locking rules for the dma-buf reservation lock. The involved SHMEM
BO has to be vmapped in the plane's prepare_fb callback and vunmapped
in cleanup_fb.

This patch introduces a DRM library that implements KMS helpers for
GEM SHMEM buffer objects. The first set of helpers is the plane state
for shadow planes. The provided implementations for prepare_fb and
cleanup_fb vmap and vunmap all BOs of struct drm_plane_state.fb. The
mappings are afterwards available in the plane's commit-tail functions.

All rsp drivers use the simple KMS helpers, so we add the plane callbacks
and wrappers for simple KMS.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/Kconfig                    |   7 +
 drivers/gpu/drm/Makefile                   |   1 +
 drivers/gpu/drm/drm_gem_shmem_kms_helper.c | 159 +++++++++++++++++++++
 include/drm/drm_gem_shmem_kms_helper.h     |  56 ++++++++
 4 files changed, 223 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_gem_shmem_kms_helper.c
 create mode 100644 include/drm/drm_gem_shmem_kms_helper.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 8bf103de1594..b8d8b00ab5d4 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -214,6 +214,13 @@ config DRM_GEM_SHMEM_HELPER
 	help
 	  Choose this if you need the GEM shmem helper functions
 
+config DRM_GEM_SHMEM_KMS_HELPER
+	bool
+	depends on DRM_GEM_SHMEM_HELPER
+	help
+	help
+	  Choose this if you need the GEM SHMEM helper functions for KMS
+
 config DRM_SCHED
 	tristate
 	depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 926adef289db..37a73dee5baf 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -53,6 +53,7 @@ drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
 drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
 drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
+drm_kms_helper-$(CONFIG_DRM_GEM_SHMEM_KMS_HELPER) += drm_gem_shmem_kms_helper.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
diff --git a/drivers/gpu/drm/drm_gem_shmem_kms_helper.c b/drivers/gpu/drm/drm_gem_shmem_kms_helper.c
new file mode 100644
index 000000000000..8843c5837f98
--- /dev/null
+++ b/drivers/gpu/drm/drm_gem_shmem_kms_helper.c
@@ -0,0 +1,159 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <drm/drm_atomic_state_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_gem_shmem_kms_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+
+/*
+ * Helpers for struct drm_plane_funcs
+ *
+ */
+
+static struct drm_plane_state *
+drm_gem_shmem_duplicate_shadow_plane_state(struct drm_plane *plane,
+					   struct drm_plane_state *plane_state)
+{
+	struct drm_gem_shmem_shadow_plane_state *new_shadow_plane_state;
+
+	if (!plane_state)
+		return NULL;
+
+	new_shadow_plane_state = kzalloc(sizeof(*new_shadow_plane_state), GFP_KERNEL);
+	if (!new_shadow_plane_state)
+		return NULL;
+	__drm_atomic_helper_plane_duplicate_state(plane, &new_shadow_plane_state->base);
+
+	return &new_shadow_plane_state->base;
+}
+
+static void drm_gem_shmem_destroy_shadow_plane_state(struct drm_plane *plane,
+						     struct drm_plane_state *plane_state)
+{
+	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
+		to_drm_gem_shmem_shadow_plane_state(plane_state);
+
+	__drm_atomic_helper_plane_destroy_state(&shadow_plane_state->base);
+	kfree(shadow_plane_state);
+}
+
+static void drm_gem_shmem_reset_shadow_plane(struct drm_plane *plane)
+{
+	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state;
+
+	if (plane->state) {
+		drm_gem_shmem_destroy_shadow_plane_state(plane, plane->state);
+		plane->state = NULL; /* must be set to NULL here */
+	}
+
+	shadow_plane_state = kzalloc(sizeof(*shadow_plane_state), GFP_KERNEL);
+	if (!shadow_plane_state)
+		return;
+	__drm_atomic_helper_plane_reset(plane, &shadow_plane_state->base);
+}
+
+/*
+ * Helpers for struct drm_plane_helper_funcs
+ */
+
+static int drm_gem_shmem_prepare_shadow_fb(struct drm_plane *plane,
+					   struct drm_plane_state *plane_state)
+{
+	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
+		to_drm_gem_shmem_shadow_plane_state(plane_state);
+	struct drm_framebuffer *fb = plane_state->fb;
+	struct drm_gem_object *obj;
+	struct dma_buf_map map;
+	int ret;
+	size_t i;
+
+	if (!fb)
+		return 0;
+
+	ret = drm_gem_fb_prepare_fb(plane, plane_state);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ARRAY_SIZE(shadow_plane_state->map); ++i) {
+		obj = drm_gem_fb_get_obj(fb, i);
+		if (!obj)
+			continue;
+		ret = drm_gem_shmem_vmap(obj, &map);
+		if (ret)
+			goto err_drm_gem_shmem_vunmap;
+		shadow_plane_state->map[i] = map;
+	}
+
+	return 0;
+
+err_drm_gem_shmem_vunmap:
+	while (i) {
+		--i;
+		obj = drm_gem_fb_get_obj(fb, i);
+		if (!obj)
+			continue;
+		drm_gem_shmem_vunmap(obj, &shadow_plane_state->map[i]);
+	}
+	return ret;
+}
+
+static void drm_gem_shmem_cleanup_shadow_fb(struct drm_plane *plane,
+					    struct drm_plane_state *plane_state)
+{
+	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
+		to_drm_gem_shmem_shadow_plane_state(plane_state);
+	struct drm_framebuffer *fb = plane_state->fb;
+	size_t i = ARRAY_SIZE(shadow_plane_state->map);
+	struct drm_gem_object *obj;
+
+	if (!fb)
+		return;
+
+	while (i) {
+		--i;
+		obj = drm_gem_fb_get_obj(fb, i);
+		if (!obj)
+			continue;
+		drm_gem_shmem_vunmap(obj, &shadow_plane_state->map[i]);
+	}
+}
+
+/*
+ * Simple KMS helpers
+ */
+
+int drm_gem_shmem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe,
+					       struct drm_plane_state *plane_state)
+{
+	return drm_gem_shmem_prepare_shadow_fb(&pipe->plane, plane_state);
+}
+EXPORT_SYMBOL(drm_gem_shmem_simple_kms_prepare_shadow_fb);
+
+void drm_gem_shmem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
+						struct drm_plane_state *plane_state)
+{
+	drm_gem_shmem_cleanup_shadow_fb(&pipe->plane, plane_state);
+}
+EXPORT_SYMBOL(drm_gem_shmem_simple_kms_cleanup_shadow_fb);
+
+void drm_gem_shmem_simple_kms_reset_shadow_plane(struct drm_simple_display_pipe *pipe)
+{
+	drm_gem_shmem_reset_shadow_plane(&pipe->plane);
+}
+EXPORT_SYMBOL(drm_gem_shmem_simple_kms_reset_shadow_plane);
+
+struct drm_plane_state *
+drm_gem_shmem_simple_kms_duplicate_shadow_plane_state(struct drm_simple_display_pipe *pipe,
+						      struct drm_plane_state *plane_state)
+{
+	return drm_gem_shmem_duplicate_shadow_plane_state(&pipe->plane, plane_state);
+}
+EXPORT_SYMBOL(drm_gem_shmem_simple_kms_duplicate_shadow_plane_state);
+
+void drm_gem_shmem_simple_kms_destroy_shadow_plane_state(struct drm_simple_display_pipe *pipe,
+							 struct drm_plane_state *plane_state)
+{
+	drm_gem_shmem_destroy_shadow_plane_state(&pipe->plane, plane_state);
+}
+EXPORT_SYMBOL(drm_gem_shmem_simple_kms_destroy_shadow_plane_state);
diff --git a/include/drm/drm_gem_shmem_kms_helper.h b/include/drm/drm_gem_shmem_kms_helper.h
new file mode 100644
index 000000000000..bd42c9c0a39e
--- /dev/null
+++ b/include/drm/drm_gem_shmem_kms_helper.h
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __DRM_GEM_SHMEM_KMS_HELPER_H__
+#define __DRM_GEM_SHMEM_KMS_HELPER_H__
+
+#include <linux/dma-buf-map.h>
+
+#include <drm/drm_plane.h>
+
+struct drm_simple_display_pipe;
+
+struct drm_gem_shmem_shadow_plane_state {
+	struct drm_plane_state base;
+
+	/* Transitional state - do not export or duplicate */
+
+	struct dma_buf_map map[4];
+};
+
+static inline struct drm_gem_shmem_shadow_plane_state *
+to_drm_gem_shmem_shadow_plane_state(struct drm_plane_state *state)
+{
+	return container_of(state, struct drm_gem_shmem_shadow_plane_state, base);
+}
+
+/*
+ * Simple KMS helpers
+ */
+
+int drm_gem_shmem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe,
+					       struct drm_plane_state *plane_state);
+void drm_gem_shmem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
+						struct drm_plane_state *plane_state);
+void drm_gem_shmem_simple_kms_reset_shadow_plane(struct drm_simple_display_pipe *pipe);
+struct drm_plane_state *
+drm_gem_shmem_simple_kms_duplicate_shadow_plane_state(struct drm_simple_display_pipe *pipe,
+						      struct drm_plane_state *plane_state);
+void
+drm_gem_shmem_simple_kms_destroy_shadow_plane_state(struct drm_simple_display_pipe *pipe,
+						    struct drm_plane_state *plane_state);
+
+/**
+ * DRM_GEM_SHMEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS -
+ *	Initializes struct drm_simple_display_pipe_funcs for SHMEM shadow planes
+ *
+ * Drivers may use GEM SHMEM BOs as shadow buffers over the framebuffer memory. This
+ * macro initializes struct drm_simple_display_pipe_funcs to use the rsp helper functions.
+ */
+#define DRM_GEM_SHMEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS \
+	.prepare_fb = drm_gem_shmem_simple_kms_prepare_shadow_fb, \
+	.cleanup_fb = drm_gem_shmem_simple_kms_cleanup_shadow_fb, \
+	.reset_plane = drm_gem_shmem_simple_kms_reset_shadow_plane, \
+	.duplicate_plane_state = drm_gem_shmem_simple_kms_duplicate_shadow_plane_state, \
+	.destroy_plane_state   = drm_gem_shmem_simple_kms_destroy_shadow_plane_state
+
+#endif /* __DRM_GEM_SHMEM_KMS_HELPER_H__ */
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/6] drm/mgag200: Move vmap out of commit tail

[Thomas Zimmermann]

Vmap operations may acquire the dmabuf reservation lock, which is not
allowed within atomic commit-tail functions. Therefore move vmap and
vunmap from the damage handler into prepare_fb and cleanup_fb callbacks.

The mapping is provided as GEM SHMEM shadow plane. The enable and prepare
callbacks use the established mapping for damage handling.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/Kconfig        |  1 +
 drivers/gpu/drm/mgag200/mgag200_mode.c | 25 ++++++++++---------------
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig
index eec59658a938..b4e5a1eb57ce 100644
--- a/drivers/gpu/drm/mgag200/Kconfig
+++ b/drivers/gpu/drm/mgag200/Kconfig
@@ -3,6 +3,7 @@ config DRM_MGAG200
 	tristate "Matrox G200"
 	depends on DRM && PCI && MMU
 	select DRM_GEM_SHMEM_HELPER
+	select DRM_GEM_SHMEM_KMS_HELPER
 	select DRM_KMS_HELPER
 	help
 	 This is a KMS driver for Matrox G200 chips. It supports the original
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 1dfc42170059..70040a02ee98 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -18,6 +18,7 @@
 #include <drm/drm_format_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_kms_helper.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
@@ -1549,22 +1550,12 @@ mgag200_simple_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
 
 static void
 mgag200_handle_damage(struct mga_device *mdev, struct drm_framebuffer *fb,
-		      struct drm_rect *clip)
+		      struct drm_rect *clip, const struct dma_buf_map *map)
 {
-	struct drm_device *dev = &mdev->base;
-	struct dma_buf_map map;
-	void *vmap;
-	int ret;
-
-	ret = drm_gem_shmem_vmap(fb->obj[0], &map);
-	if (drm_WARN_ON(dev, ret))
-		return; /* BUG: SHMEM BO should always be vmapped */
-	vmap = map.vaddr; /* TODO: Use mapping abstraction properly */
+	void *vmap = map->vaddr; /* TODO: Use mapping abstraction properly */
 
 	drm_fb_memcpy_dstclip(mdev->vram, vmap, fb, clip);
 
-	drm_gem_shmem_vunmap(fb->obj[0], &map);
-
 	/* Always scanout image at VRAM offset 0 */
 	mgag200_set_startadd(mdev, (u32)0);
 	mgag200_set_offset(mdev, fb);
@@ -1580,6 +1571,8 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 	struct mga_device *mdev = to_mga_device(dev);
 	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
 	struct drm_framebuffer *fb = plane_state->fb;
+	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
+		to_drm_gem_shmem_shadow_plane_state(plane_state);
 	struct drm_rect fullscreen = {
 		.x1 = 0,
 		.x2 = fb->width,
@@ -1608,7 +1601,7 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 	mga_crtc_load_lut(crtc);
 	mgag200_enable_display(mdev);
 
-	mgag200_handle_damage(mdev, fb, &fullscreen);
+	mgag200_handle_damage(mdev, fb, &fullscreen, &shadow_plane_state->map[0]);
 }
 
 static void
@@ -1649,6 +1642,8 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
 	struct drm_device *dev = plane->dev;
 	struct mga_device *mdev = to_mga_device(dev);
 	struct drm_plane_state *state = plane->state;
+	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
+		to_drm_gem_shmem_shadow_plane_state(state);
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_rect damage;
 
@@ -1656,7 +1651,7 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
 		return;
 
 	if (drm_atomic_helper_damage_merged(old_state, state, &damage))
-		mgag200_handle_damage(mdev, fb, &damage);
+		mgag200_handle_damage(mdev, fb, &damage, &shadow_plane_state->map[0]);
 }
 
 static const struct drm_simple_display_pipe_funcs
@@ -1666,7 +1661,7 @@ mgag200_simple_display_pipe_funcs = {
 	.disable    = mgag200_simple_display_pipe_disable,
 	.check	    = mgag200_simple_display_pipe_check,
 	.update	    = mgag200_simple_display_pipe_update,
-	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
+	DRM_GEM_SHMEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
 };
 
 static const uint32_t mgag200_simple_display_pipe_formats[] = {
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/6] drm/cirrus: Move vmap out of commit tail

[Thomas Zimmermann]

Vmap operations may acquire the dmabuf reservation lock, which is not
allowed within atomic commit-tail functions. Therefore move vmap and
vunmap from the damage handler into prepare_fb and cleanup_fb callbacks.

The mapping is provided as GEM SHMEM shadow plane. The enable and prepare
callbacks use the established mapping for damage handling.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/Kconfig  |  3 ++-
 drivers/gpu/drm/tiny/cirrus.c | 45 +++++++++++++++--------------------
 2 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index 2b6414f0fa75..e0aef6cf8e26 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -3,8 +3,9 @@
 config DRM_CIRRUS_QEMU
 	tristate "Cirrus driver for QEMU emulated device"
 	depends on DRM && PCI && MMU
-	select DRM_KMS_HELPER
 	select DRM_GEM_SHMEM_HELPER
+	select DRM_GEM_SHMEM_KMS_HELPER
+	select DRM_KMS_HELPER
 	help
 	 This is a KMS driver for emulated cirrus device in qemu.
 	 It is *NOT* intended for real cirrus devices. This requires
diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
index a043e602199e..aac627615b2d 100644
--- a/drivers/gpu/drm/tiny/cirrus.c
+++ b/drivers/gpu/drm/tiny/cirrus.c
@@ -33,8 +33,9 @@
 #include <drm/drm_file.h>
 #include <drm/drm_format_helper.h>
 #include <drm/drm_fourcc.h>
-#include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_gem_shmem_kms_helper.h>
 #include <drm/drm_ioctl.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_modeset_helper_vtables.h>
@@ -311,22 +312,15 @@ static int cirrus_mode_set(struct cirrus_device *cirrus,
 	return 0;
 }
 
-static int cirrus_fb_blit_rect(struct drm_framebuffer *fb,
+static int cirrus_fb_blit_rect(struct drm_framebuffer *fb, const struct dma_buf_map *map,
 			       struct drm_rect *rect)
 {
 	struct cirrus_device *cirrus = to_cirrus(fb->dev);
-	struct dma_buf_map map;
-	void *vmap;
-	int idx, ret;
+	void *vmap = map->vaddr; /* TODO: Use mapping abstraction properly */
+	int idx;
 
-	ret = -ENODEV;
 	if (!drm_dev_enter(&cirrus->dev, &idx))
-		goto out;
-
-	ret = drm_gem_shmem_vmap(fb->obj[0], &map);
-	if (ret)
-		goto out_dev_exit;
-	vmap = map.vaddr; /* TODO: Use mapping abstraction properly */
+		return -ENODEV;
 
 	if (cirrus->cpp == fb->format->cpp[0])
 		drm_fb_memcpy_dstclip(cirrus->vram,
@@ -345,16 +339,12 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb,
 	else
 		WARN_ON_ONCE("cpp mismatch");
 
-	drm_gem_shmem_vunmap(fb->obj[0], &map);
-	ret = 0;
-
-out_dev_exit:
 	drm_dev_exit(idx);
-out:
-	return ret;
+
+	return 0;
 }
 
-static int cirrus_fb_blit_fullscreen(struct drm_framebuffer *fb)
+static int cirrus_fb_blit_fullscreen(struct drm_framebuffer *fb, const struct dma_buf_map *map)
 {
 	struct drm_rect fullscreen = {
 		.x1 = 0,
@@ -362,7 +352,7 @@ static int cirrus_fb_blit_fullscreen(struct drm_framebuffer *fb)
 		.y1 = 0,
 		.y2 = fb->height,
 	};
-	return cirrus_fb_blit_rect(fb, &fullscreen);
+	return cirrus_fb_blit_rect(fb, map, &fullscreen);
 }
 
 static int cirrus_check_size(int width, int height,
@@ -441,9 +431,11 @@ static void cirrus_pipe_enable(struct drm_simple_display_pipe *pipe,
 			       struct drm_plane_state *plane_state)
 {
 	struct cirrus_device *cirrus = to_cirrus(pipe->crtc.dev);
+	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
+		to_drm_gem_shmem_shadow_plane_state(plane_state);
 
 	cirrus_mode_set(cirrus, &crtc_state->mode, plane_state->fb);
-	cirrus_fb_blit_fullscreen(plane_state->fb);
+	cirrus_fb_blit_fullscreen(plane_state->fb, &shadow_plane_state->map[0]);
 }
 
 static void cirrus_pipe_update(struct drm_simple_display_pipe *pipe,
@@ -451,16 +443,16 @@ static void cirrus_pipe_update(struct drm_simple_display_pipe *pipe,
 {
 	struct cirrus_device *cirrus = to_cirrus(pipe->crtc.dev);
 	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
+		to_drm_gem_shmem_shadow_plane_state(state);
 	struct drm_crtc *crtc = &pipe->crtc;
 	struct drm_rect rect;
 
-	if (pipe->plane.state->fb &&
-	    cirrus->cpp != cirrus_cpp(pipe->plane.state->fb))
-		cirrus_mode_set(cirrus, &crtc->mode,
-				pipe->plane.state->fb);
+	if (state->fb && cirrus->cpp != cirrus_cpp(state->fb))
+		cirrus_mode_set(cirrus, &crtc->mode, state->fb);
 
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
-		cirrus_fb_blit_rect(pipe->plane.state->fb, &rect);
+		cirrus_fb_blit_rect(state->fb, &shadow_plane_state->map[0], &rect);
 }
 
 static const struct drm_simple_display_pipe_funcs cirrus_pipe_funcs = {
@@ -468,6 +460,7 @@ static const struct drm_simple_display_pipe_funcs cirrus_pipe_funcs = {
 	.check	    = cirrus_pipe_check,
 	.enable	    = cirrus_pipe_enable,
 	.update	    = cirrus_pipe_update,
+	DRM_GEM_SHMEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
 };
 
 static const uint32_t cirrus_formats[] = {
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/6] drm/gm12u320: Move vmap out of commit tail

[Thomas Zimmermann]

Vmap operations may acquire the dmabuf reservation lock, which is not
allowed within atomic commit-tail functions. Therefore move vmap and
vunmap from the damage handler into prepare_fb and cleanup_fb callbacks.

The mapping is provided as GEM SHMEM shadow plane. The enable and prepare
callbacks use the established mapping for damage handling.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/Kconfig    |  3 ++-
 drivers/gpu/drm/tiny/gm12u320.c | 30 +++++++++++++++---------------
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index e0aef6cf8e26..faf3926fd4fa 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -23,8 +23,9 @@ config DRM_CIRRUS_QEMU
 config DRM_GM12U320
 	tristate "GM12U320 driver for USB projectors"
 	depends on DRM && USB
-	select DRM_KMS_HELPER
 	select DRM_GEM_SHMEM_HELPER
+	select DRM_GEM_SHMEM_KMS_HELPER
+	select DRM_KMS_HELPER
 	help
 	 This is a KMS driver for projectors which use the GM12U320 chipset
 	 for video transfer over USB2/3, such as the Acer C120 mini projector.
diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
index 33f65f4626e5..1d20949f429a 100644
--- a/drivers/gpu/drm/tiny/gm12u320.c
+++ b/drivers/gpu/drm/tiny/gm12u320.c
@@ -16,8 +16,9 @@
 #include <drm/drm_file.h>
 #include <drm/drm_format_helper.h>
 #include <drm/drm_fourcc.h>
-#include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_gem_shmem_kms_helper.h>
 #include <drm/drm_ioctl.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_modeset_helper_vtables.h>
@@ -94,6 +95,7 @@ struct gm12u320_device {
 		struct drm_rect          rect;
 		int frame;
 		int draw_status_timeout;
+		struct dma_buf_map src_map;
 	} fb_update;
 };
 
@@ -250,7 +252,6 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
 {
 	int block, dst_offset, len, remain, ret, x1, x2, y1, y2;
 	struct drm_framebuffer *fb;
-	struct dma_buf_map map;
 	void *vaddr;
 	u8 *src;
 
@@ -264,20 +265,14 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
 	x2 = gm12u320->fb_update.rect.x2;
 	y1 = gm12u320->fb_update.rect.y1;
 	y2 = gm12u320->fb_update.rect.y2;
-
-	ret = drm_gem_shmem_vmap(fb->obj[0], &map);
-	if (ret) {
-		GM12U320_ERR("failed to vmap fb: %d\n", ret);
-		goto put_fb;
-	}
-	vaddr = map.vaddr; /* TODO: Use mapping abstraction properly */
+	vaddr = gm12u320->fb_update.src_map.vaddr; /* TODO: Use mapping abstraction properly */
 
 	if (fb->obj[0]->import_attach) {
 		ret = dma_buf_begin_cpu_access(
 			fb->obj[0]->import_attach->dmabuf, DMA_FROM_DEVICE);
 		if (ret) {
 			GM12U320_ERR("dma_buf_begin_cpu_access err: %d\n", ret);
-			goto vunmap;
+			goto put_fb;
 		}
 	}
 
@@ -321,8 +316,6 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
 		if (ret)
 			GM12U320_ERR("dma_buf_end_cpu_access err: %d\n", ret);
 	}
-vunmap:
-	drm_gem_shmem_vunmap(fb->obj[0], &map);
 put_fb:
 	drm_framebuffer_put(fb);
 	gm12u320->fb_update.fb = NULL;
@@ -410,7 +403,7 @@ static void gm12u320_fb_update_work(struct work_struct *work)
 		GM12U320_ERR("Frame update error: %d\n", ret);
 }
 
-static void gm12u320_fb_mark_dirty(struct drm_framebuffer *fb,
+static void gm12u320_fb_mark_dirty(struct drm_framebuffer *fb, const struct dma_buf_map *map,
 				   struct drm_rect *dirty)
 {
 	struct gm12u320_device *gm12u320 = to_gm12u320(fb->dev);
@@ -424,6 +417,7 @@ static void gm12u320_fb_mark_dirty(struct drm_framebuffer *fb,
 		drm_framebuffer_get(fb);
 		gm12u320->fb_update.fb = fb;
 		gm12u320->fb_update.rect = *dirty;
+		gm12u320->fb_update.src_map = *map;
 		wakeup = true;
 	} else {
 		struct drm_rect *rect = &gm12u320->fb_update.rect;
@@ -452,6 +446,7 @@ static void gm12u320_stop_fb_update(struct gm12u320_device *gm12u320)
 	mutex_lock(&gm12u320->fb_update.lock);
 	old_fb = gm12u320->fb_update.fb;
 	gm12u320->fb_update.fb = NULL;
+	dma_buf_map_clear(&gm12u320->fb_update.src_map);
 	mutex_unlock(&gm12u320->fb_update.lock);
 
 	drm_framebuffer_put(old_fb);
@@ -564,9 +559,11 @@ static void gm12u320_pipe_enable(struct drm_simple_display_pipe *pipe,
 {
 	struct drm_rect rect = { 0, 0, GM12U320_USER_WIDTH, GM12U320_HEIGHT };
 	struct gm12u320_device *gm12u320 = to_gm12u320(pipe->crtc.dev);
+	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
+		to_drm_gem_shmem_shadow_plane_state(plane_state);
 
 	gm12u320->fb_update.draw_status_timeout = FIRST_FRAME_TIMEOUT;
-	gm12u320_fb_mark_dirty(plane_state->fb, &rect);
+	gm12u320_fb_mark_dirty(plane_state->fb, &shadow_plane_state->map[0], &rect);
 }
 
 static void gm12u320_pipe_disable(struct drm_simple_display_pipe *pipe)
@@ -580,16 +577,19 @@ static void gm12u320_pipe_update(struct drm_simple_display_pipe *pipe,
 				 struct drm_plane_state *old_state)
 {
 	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
+		to_drm_gem_shmem_shadow_plane_state(state);
 	struct drm_rect rect;
 
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
-		gm12u320_fb_mark_dirty(pipe->plane.state->fb, &rect);
+		gm12u320_fb_mark_dirty(state->fb, &shadow_plane_state->map[0], &rect);
 }
 
 static const struct drm_simple_display_pipe_funcs gm12u320_pipe_funcs = {
 	.enable	    = gm12u320_pipe_enable,
 	.disable    = gm12u320_pipe_disable,
 	.update	    = gm12u320_pipe_update,
+	DRM_GEM_SHMEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
 };
 
 static const uint32_t gm12u320_pipe_formats[] = {
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/6] drm/udl: Move vmap out of commit tail

[Thomas Zimmermann]

Vmap operations may acquire the dmabuf reservation lock, which is not
allowed within atomic commit-tail functions. Therefore move vmap and
vunmap from the damage handler into prepare_fb and cleanup_fb callbacks.

The mapping is provided as GEM SHMEM shadow plane. The enable and prepare
callbacks use the established mapping for damage handling.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/Kconfig       |  1 +
 drivers/gpu/drm/udl/udl_modeset.c | 36 +++++++++++++------------------
 2 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/udl/Kconfig b/drivers/gpu/drm/udl/Kconfig
index 1f497d8f1ae5..1b46d93ca61c 100644
--- a/drivers/gpu/drm/udl/Kconfig
+++ b/drivers/gpu/drm/udl/Kconfig
@@ -5,6 +5,7 @@ config DRM_UDL
 	depends on USB
 	depends on USB_ARCH_HAS_HCD
 	select DRM_GEM_SHMEM_HELPER
+	select DRM_GEM_SHMEM_KMS_HELPER
 	select DRM_KMS_HELPER
 	help
 	  This is a KMS driver for the USB displaylink video adapters.
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 9d34ec9d03f6..974dffe86a44 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -17,6 +17,7 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_gem_shmem_kms_helper.h>
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_vblank.h>
 
@@ -266,18 +267,17 @@ static int udl_aligned_damage_clip(struct drm_rect *clip, int x, int y,
 	return 0;
 }
 
-static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
-			     int width, int height)
+static int udl_handle_damage(struct drm_framebuffer *fb, const struct dma_buf_map *map,
+			     int x, int y, int width, int height)
 {
 	struct drm_device *dev = fb->dev;
 	struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
+	void *vaddr = map->vaddr; /* TODO: Use mapping abstraction properly */
 	int i, ret, tmp_ret;
 	char *cmd;
 	struct urb *urb;
 	struct drm_rect clip;
 	int log_bpp;
-	struct dma_buf_map map;
-	void *vaddr;
 
 	ret = udl_log_cpp(fb->format->cpp[0]);
 	if (ret < 0)
@@ -297,17 +297,10 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 			return ret;
 	}
 
-	ret = drm_gem_shmem_vmap(fb->obj[0], &map);
-	if (ret) {
-		DRM_ERROR("failed to vmap fb\n");
-		goto out_dma_buf_end_cpu_access;
-	}
-	vaddr = map.vaddr; /* TODO: Use mapping abstraction properly */
-
 	urb = udl_get_urb(dev);
 	if (!urb) {
 		ret = -ENOMEM;
-		goto out_drm_gem_shmem_vunmap;
+		goto out_dma_buf_end_cpu_access;
 	}
 	cmd = urb->transfer_buffer;
 
@@ -320,7 +313,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 				       &cmd, byte_offset, dev_byte_offset,
 				       byte_width);
 		if (ret)
-			goto out_drm_gem_shmem_vunmap;
+			goto out_dma_buf_end_cpu_access;
 	}
 
 	if (cmd > (char *)urb->transfer_buffer) {
@@ -336,8 +329,6 @@ static int udl_handle_damage(struct drm_framebuffer *fb, int x, int y,
 
 	ret = 0;
 
-out_drm_gem_shmem_vunmap:
-	drm_gem_shmem_vunmap(fb->obj[0], &map);
 out_dma_buf_end_cpu_access:
 	if (import_attach) {
 		tmp_ret = dma_buf_end_cpu_access(import_attach->dmabuf,
@@ -375,6 +366,8 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 	struct drm_framebuffer *fb = plane_state->fb;
 	struct udl_device *udl = to_udl(dev);
 	struct drm_display_mode *mode = &crtc_state->mode;
+	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
+		to_drm_gem_shmem_shadow_plane_state(plane_state);
 	char *buf;
 	char *wrptr;
 	int color_depth = UDL_COLOR_DEPTH_16BPP;
@@ -400,7 +393,7 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	udl->mode_buf_len = wrptr - buf;
 
-	udl_handle_damage(fb, 0, 0, fb->width, fb->height);
+	udl_handle_damage(fb, &shadow_plane_state->map[0], 0, 0, fb->width, fb->height);
 
 	if (!crtc_state->mode_changed)
 		return;
@@ -435,6 +428,8 @@ udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
 			       struct drm_plane_state *old_plane_state)
 {
 	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
+		to_drm_gem_shmem_shadow_plane_state(state);
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_rect rect;
 
@@ -442,17 +437,16 @@ udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
 		return;
 
 	if (drm_atomic_helper_damage_merged(old_plane_state, state, &rect))
-		udl_handle_damage(fb, rect.x1, rect.y1, rect.x2 - rect.x1,
-				  rect.y2 - rect.y1);
+		udl_handle_damage(fb, &shadow_plane_state->map[0], rect.x1, rect.y1,
+				  rect.x2 - rect.x1, rect.y2 - rect.y1);
 }
 
-static const
-struct drm_simple_display_pipe_funcs udl_simple_display_pipe_funcs = {
+static const struct drm_simple_display_pipe_funcs udl_simple_display_pipe_funcs = {
 	.mode_valid = udl_simple_display_pipe_mode_valid,
 	.enable = udl_simple_display_pipe_enable,
 	.disable = udl_simple_display_pipe_disable,
 	.update = udl_simple_display_pipe_update,
-	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
+	DRM_GEM_SHMEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
 };
 
 /*
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/6] drm/shmem-helper: Add additional KMS helpers

[Daniel Vetter]

On Wed, Feb 03, 2021 at 02:10:42PM +0100, Thomas Zimmermann wrote:
> Several drivers use GEM SHMEM buffer objects as shadow buffers for
> the actual framebuffer memory. Right now, drivers do these vmap
> operations in their commit tail, which is actually not allowed by the
> locking rules for the dma-buf reservation lock. The involved SHMEM
> BO has to be vmapped in the plane's prepare_fb callback and vunmapped
> in cleanup_fb.
> 
> This patch introduces a DRM library that implements KMS helpers for
> GEM SHMEM buffer objects. The first set of helpers is the plane state
> for shadow planes. The provided implementations for prepare_fb and
> cleanup_fb vmap and vunmap all BOs of struct drm_plane_state.fb. The
> mappings are afterwards available in the plane's commit-tail functions.
> 
> All rsp drivers use the simple KMS helpers, so we add the plane callbacks
> and wrappers for simple KMS.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/Kconfig                    |   7 +
>  drivers/gpu/drm/Makefile                   |   1 +
>  drivers/gpu/drm/drm_gem_shmem_kms_helper.c | 159 +++++++++++++++++++++
>  include/drm/drm_gem_shmem_kms_helper.h     |  56 ++++++++
>  4 files changed, 223 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_gem_shmem_kms_helper.c
>  create mode 100644 include/drm/drm_gem_shmem_kms_helper.h
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 8bf103de1594..b8d8b00ab5d4 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -214,6 +214,13 @@ config DRM_GEM_SHMEM_HELPER
>  	help
>  	  Choose this if you need the GEM shmem helper functions
>  
> +config DRM_GEM_SHMEM_KMS_HELPER
> +	bool
> +	depends on DRM_GEM_SHMEM_HELPER
> +	help
> +	help
> +	  Choose this if you need the GEM SHMEM helper functions for KMS

I think we should just stuff this into simple pipe helpers directly. Also
needs some kerneldoc and ideally intro section of some sorts, but I guess
that was just missing for RFC.

Thinking a bit bigger and looking at the first patch, I wonder whether we
shouldn't outright integrate this simple pipe helpers. Adding all the
hooks for plane_state (instead of a simple_pipe_state or something like
that) feels a bit icky to me. But on the driver side the integration with
just the single macro line is very neat, so that's redeeming feature.

Note doing a drm_simple_display_pipe_state would be a bit tricky to do,
since we'd need custome duplicate_state functions to make sure there's
only ever exactly one:

struct drm_simple_display_pipe_state {
	struct drm_crtc_state crtc_state;
	struct drm_plane_state plane_state;

	struct dma_buf_map map[4];
};

But that's a bit a bigger step, maybe when we also need to subclass crtc
stuff we can look into this. Or maybe that's then too much feature creep,
dunno. Implemenation shouldn't be too tricky:
- crtc state just duplicates itself (including plane_state duplication).
  In release it also cleans up the plane state.
- plane state grabs the crtc state instead, and then does a cast. destroy
  hook does nothing (to avoid touching freed memory, since we always dupe
  the crtc state when the plane state exists we know the crtc destroy hook
  will clean things up).

That means drm_atomic_state has 2 pointers pointing into the same
allocation, but that should be all fine.

Aside from this bikeshed here pondering a bit how this best first into our
simple pipe helpers I think this all looks very clean.
-Daniel

> +
>  config DRM_SCHED
>  	tristate
>  	depends on DRM
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 926adef289db..37a73dee5baf 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -53,6 +53,7 @@ drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
>  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
>  drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
> +drm_kms_helper-$(CONFIG_DRM_GEM_SHMEM_KMS_HELPER) += drm_gem_shmem_kms_helper.o
>  
>  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
>  obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> diff --git a/drivers/gpu/drm/drm_gem_shmem_kms_helper.c b/drivers/gpu/drm/drm_gem_shmem_kms_helper.c
> new file mode 100644
> index 000000000000..8843c5837f98
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_gem_shmem_kms_helper.c
> @@ -0,0 +1,159 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <drm/drm_atomic_state_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_gem_shmem_helper.h>
> +#include <drm/drm_gem_shmem_kms_helper.h>
> +#include <drm/drm_simple_kms_helper.h>
> +
> +/*
> + * Helpers for struct drm_plane_funcs
> + *
> + */
> +
> +static struct drm_plane_state *
> +drm_gem_shmem_duplicate_shadow_plane_state(struct drm_plane *plane,
> +					   struct drm_plane_state *plane_state)
> +{
> +	struct drm_gem_shmem_shadow_plane_state *new_shadow_plane_state;
> +
> +	if (!plane_state)
> +		return NULL;
> +
> +	new_shadow_plane_state = kzalloc(sizeof(*new_shadow_plane_state), GFP_KERNEL);
> +	if (!new_shadow_plane_state)
> +		return NULL;
> +	__drm_atomic_helper_plane_duplicate_state(plane, &new_shadow_plane_state->base);
> +
> +	return &new_shadow_plane_state->base;
> +}
> +
> +static void drm_gem_shmem_destroy_shadow_plane_state(struct drm_plane *plane,
> +						     struct drm_plane_state *plane_state)
> +{
> +	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
> +		to_drm_gem_shmem_shadow_plane_state(plane_state);
> +
> +	__drm_atomic_helper_plane_destroy_state(&shadow_plane_state->base);
> +	kfree(shadow_plane_state);
> +}
> +
> +static void drm_gem_shmem_reset_shadow_plane(struct drm_plane *plane)
> +{
> +	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state;
> +
> +	if (plane->state) {
> +		drm_gem_shmem_destroy_shadow_plane_state(plane, plane->state);
> +		plane->state = NULL; /* must be set to NULL here */
> +	}
> +
> +	shadow_plane_state = kzalloc(sizeof(*shadow_plane_state), GFP_KERNEL);
> +	if (!shadow_plane_state)
> +		return;
> +	__drm_atomic_helper_plane_reset(plane, &shadow_plane_state->base);
> +}
> +
> +/*
> + * Helpers for struct drm_plane_helper_funcs
> + */
> +
> +static int drm_gem_shmem_prepare_shadow_fb(struct drm_plane *plane,
> +					   struct drm_plane_state *plane_state)
> +{
> +	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
> +		to_drm_gem_shmem_shadow_plane_state(plane_state);
> +	struct drm_framebuffer *fb = plane_state->fb;
> +	struct drm_gem_object *obj;
> +	struct dma_buf_map map;
> +	int ret;
> +	size_t i;
> +
> +	if (!fb)
> +		return 0;
> +
> +	ret = drm_gem_fb_prepare_fb(plane, plane_state);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(shadow_plane_state->map); ++i) {
> +		obj = drm_gem_fb_get_obj(fb, i);
> +		if (!obj)
> +			continue;
> +		ret = drm_gem_shmem_vmap(obj, &map);
> +		if (ret)
> +			goto err_drm_gem_shmem_vunmap;
> +		shadow_plane_state->map[i] = map;
> +	}
> +
> +	return 0;
> +
> +err_drm_gem_shmem_vunmap:
> +	while (i) {
> +		--i;
> +		obj = drm_gem_fb_get_obj(fb, i);
> +		if (!obj)
> +			continue;
> +		drm_gem_shmem_vunmap(obj, &shadow_plane_state->map[i]);
> +	}
> +	return ret;
> +}
> +
> +static void drm_gem_shmem_cleanup_shadow_fb(struct drm_plane *plane,
> +					    struct drm_plane_state *plane_state)
> +{
> +	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
> +		to_drm_gem_shmem_shadow_plane_state(plane_state);
> +	struct drm_framebuffer *fb = plane_state->fb;
> +	size_t i = ARRAY_SIZE(shadow_plane_state->map);
> +	struct drm_gem_object *obj;
> +
> +	if (!fb)
> +		return;
> +
> +	while (i) {
> +		--i;
> +		obj = drm_gem_fb_get_obj(fb, i);
> +		if (!obj)
> +			continue;
> +		drm_gem_shmem_vunmap(obj, &shadow_plane_state->map[i]);
> +	}
> +}
> +
> +/*
> + * Simple KMS helpers
> + */
> +
> +int drm_gem_shmem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe,
> +					       struct drm_plane_state *plane_state)
> +{
> +	return drm_gem_shmem_prepare_shadow_fb(&pipe->plane, plane_state);
> +}
> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_prepare_shadow_fb);
> +
> +void drm_gem_shmem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
> +						struct drm_plane_state *plane_state)
> +{
> +	drm_gem_shmem_cleanup_shadow_fb(&pipe->plane, plane_state);
> +}
> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_cleanup_shadow_fb);
> +
> +void drm_gem_shmem_simple_kms_reset_shadow_plane(struct drm_simple_display_pipe *pipe)
> +{
> +	drm_gem_shmem_reset_shadow_plane(&pipe->plane);
> +}
> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_reset_shadow_plane);
> +
> +struct drm_plane_state *
> +drm_gem_shmem_simple_kms_duplicate_shadow_plane_state(struct drm_simple_display_pipe *pipe,
> +						      struct drm_plane_state *plane_state)
> +{
> +	return drm_gem_shmem_duplicate_shadow_plane_state(&pipe->plane, plane_state);
> +}
> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_duplicate_shadow_plane_state);
> +
> +void drm_gem_shmem_simple_kms_destroy_shadow_plane_state(struct drm_simple_display_pipe *pipe,
> +							 struct drm_plane_state *plane_state)
> +{
> +	drm_gem_shmem_destroy_shadow_plane_state(&pipe->plane, plane_state);
> +}
> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_destroy_shadow_plane_state);
> diff --git a/include/drm/drm_gem_shmem_kms_helper.h b/include/drm/drm_gem_shmem_kms_helper.h
> new file mode 100644
> index 000000000000..bd42c9c0a39e
> --- /dev/null
> +++ b/include/drm/drm_gem_shmem_kms_helper.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __DRM_GEM_SHMEM_KMS_HELPER_H__
> +#define __DRM_GEM_SHMEM_KMS_HELPER_H__
> +
> +#include <linux/dma-buf-map.h>
> +
> +#include <drm/drm_plane.h>
> +
> +struct drm_simple_display_pipe;
> +
> +struct drm_gem_shmem_shadow_plane_state {
> +	struct drm_plane_state base;
> +
> +	/* Transitional state - do not export or duplicate */
> +
> +	struct dma_buf_map map[4];
> +};
> +
> +static inline struct drm_gem_shmem_shadow_plane_state *
> +to_drm_gem_shmem_shadow_plane_state(struct drm_plane_state *state)
> +{
> +	return container_of(state, struct drm_gem_shmem_shadow_plane_state, base);
> +}
> +
> +/*
> + * Simple KMS helpers
> + */
> +
> +int drm_gem_shmem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe,
> +					       struct drm_plane_state *plane_state);
> +void drm_gem_shmem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
> +						struct drm_plane_state *plane_state);
> +void drm_gem_shmem_simple_kms_reset_shadow_plane(struct drm_simple_display_pipe *pipe);
> +struct drm_plane_state *
> +drm_gem_shmem_simple_kms_duplicate_shadow_plane_state(struct drm_simple_display_pipe *pipe,
> +						      struct drm_plane_state *plane_state);
> +void
> +drm_gem_shmem_simple_kms_destroy_shadow_plane_state(struct drm_simple_display_pipe *pipe,
> +						    struct drm_plane_state *plane_state);
> +
> +/**
> + * DRM_GEM_SHMEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS -
> + *	Initializes struct drm_simple_display_pipe_funcs for SHMEM shadow planes
> + *
> + * Drivers may use GEM SHMEM BOs as shadow buffers over the framebuffer memory. This
> + * macro initializes struct drm_simple_display_pipe_funcs to use the rsp helper functions.
> + */
> +#define DRM_GEM_SHMEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS \
> +	.prepare_fb = drm_gem_shmem_simple_kms_prepare_shadow_fb, \
> +	.cleanup_fb = drm_gem_shmem_simple_kms_cleanup_shadow_fb, \
> +	.reset_plane = drm_gem_shmem_simple_kms_reset_shadow_plane, \
> +	.duplicate_plane_state = drm_gem_shmem_simple_kms_duplicate_shadow_plane_state, \
> +	.destroy_plane_state   = drm_gem_shmem_simple_kms_destroy_shadow_plane_state
> +
> +#endif /* __DRM_GEM_SHMEM_KMS_HELPER_H__ */
> -- 
> 2.30.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/6] drm/shmem-helper: Add additional KMS helpers

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 13659 bytes --]

Hi

Am 03.02.21 um 15:01 schrieb Daniel Vetter:
> On Wed, Feb 03, 2021 at 02:10:42PM +0100, Thomas Zimmermann wrote:
>> Several drivers use GEM SHMEM buffer objects as shadow buffers for
>> the actual framebuffer memory. Right now, drivers do these vmap
>> operations in their commit tail, which is actually not allowed by the
>> locking rules for the dma-buf reservation lock. The involved SHMEM
>> BO has to be vmapped in the plane's prepare_fb callback and vunmapped
>> in cleanup_fb.
>>
>> This patch introduces a DRM library that implements KMS helpers for
>> GEM SHMEM buffer objects. The first set of helpers is the plane state
>> for shadow planes. The provided implementations for prepare_fb and
>> cleanup_fb vmap and vunmap all BOs of struct drm_plane_state.fb. The
>> mappings are afterwards available in the plane's commit-tail functions.
>>
>> All rsp drivers use the simple KMS helpers, so we add the plane callbacks
>> and wrappers for simple KMS.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/Kconfig                    |   7 +
>>   drivers/gpu/drm/Makefile                   |   1 +
>>   drivers/gpu/drm/drm_gem_shmem_kms_helper.c | 159 +++++++++++++++++++++
>>   include/drm/drm_gem_shmem_kms_helper.h     |  56 ++++++++
>>   4 files changed, 223 insertions(+)
>>   create mode 100644 drivers/gpu/drm/drm_gem_shmem_kms_helper.c
>>   create mode 100644 include/drm/drm_gem_shmem_kms_helper.h
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 8bf103de1594..b8d8b00ab5d4 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -214,6 +214,13 @@ config DRM_GEM_SHMEM_HELPER
>>   	help
>>   	  Choose this if you need the GEM shmem helper functions
>>   
>> +config DRM_GEM_SHMEM_KMS_HELPER
>> +	bool
>> +	depends on DRM_GEM_SHMEM_HELPER
>> +	help
>> +	help
>> +	  Choose this if you need the GEM SHMEM helper functions for KMS
> 
> I think we should just stuff this into simple pipe helpers directly. Also
> needs some kerneldoc and ideally intro section of some sorts, but I guess
> that was just missing for RFC.

The missing docs is why it's an RFC. i was concerned about the 
additional simple-pipe callbacks, but I'm glad you're OK with them. I 
thought about simple_pipe state (as you mentioned below) or 
drm_private_state, but found it all too complex for this simple problem.

> 
> Thinking a bit bigger and looking at the first patch, I wonder whether we
> shouldn't outright integrate this simple pipe helpers. Adding all the
> hooks for plane_state (instead of a simple_pipe_state or something like
> that) feels a bit icky to me. But on the driver side the integration with
> just the single macro line is very neat, so that's redeeming feature.

I do disagree with integrating the shadow-plane code into simple-pipe. 
Right now it's SHMEM-depended and CMA-based simple-pipe drivers would 
probably not want to depend on this. The other reason is that I can 
certainly generalize the shadow planes away from SHMEM helpers; and use 
them for these vbox and ast patchsets as well. These drivers use VRAM 
helpers and full KMS helpers. I guess shadow planes could then be moved 
into drm_gem_framebuffer.c. There's other plane-helper code there already.

Unfortunately, I only realized this after sending out the patch set. :)

Best regards
Thomas

> 
> Note doing a drm_simple_display_pipe_state would be a bit tricky to do,
> since we'd need custome duplicate_state functions to make sure there's
> only ever exactly one:
> 
> struct drm_simple_display_pipe_state {
> 	struct drm_crtc_state crtc_state;
> 	struct drm_plane_state plane_state;
> 
> 	struct dma_buf_map map[4];
> };
> 
> But that's a bit a bigger step, maybe when we also need to subclass crtc
> stuff we can look into this. Or maybe that's then too much feature creep,
> dunno. Implemenation shouldn't be too tricky:
> - crtc state just duplicates itself (including plane_state duplication).
>    In release it also cleans up the plane state.
> - plane state grabs the crtc state instead, and then does a cast. destroy
>    hook does nothing (to avoid touching freed memory, since we always dupe
>    the crtc state when the plane state exists we know the crtc destroy hook
>    will clean things up).
> 
> That means drm_atomic_state has 2 pointers pointing into the same
> allocation, but that should be all fine.
> 
> Aside from this bikeshed here pondering a bit how this best first into our
> simple pipe helpers I think this all looks very clean.
> -Daniel
> 
>> +
>>   config DRM_SCHED
>>   	tristate
>>   	depends on DRM
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 926adef289db..37a73dee5baf 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -53,6 +53,7 @@ drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>>   drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
>>   drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
>>   drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
>> +drm_kms_helper-$(CONFIG_DRM_GEM_SHMEM_KMS_HELPER) += drm_gem_shmem_kms_helper.o
>>   
>>   obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
>>   obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
>> diff --git a/drivers/gpu/drm/drm_gem_shmem_kms_helper.c b/drivers/gpu/drm/drm_gem_shmem_kms_helper.c
>> new file mode 100644
>> index 000000000000..8843c5837f98
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_gem_shmem_kms_helper.c
>> @@ -0,0 +1,159 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <drm/drm_atomic_state_helper.h>
>> +#include <drm/drm_gem_framebuffer_helper.h>
>> +#include <drm/drm_gem_shmem_helper.h>
>> +#include <drm/drm_gem_shmem_kms_helper.h>
>> +#include <drm/drm_simple_kms_helper.h>
>> +
>> +/*
>> + * Helpers for struct drm_plane_funcs
>> + *
>> + */
>> +
>> +static struct drm_plane_state *
>> +drm_gem_shmem_duplicate_shadow_plane_state(struct drm_plane *plane,
>> +					   struct drm_plane_state *plane_state)
>> +{
>> +	struct drm_gem_shmem_shadow_plane_state *new_shadow_plane_state;
>> +
>> +	if (!plane_state)
>> +		return NULL;
>> +
>> +	new_shadow_plane_state = kzalloc(sizeof(*new_shadow_plane_state), GFP_KERNEL);
>> +	if (!new_shadow_plane_state)
>> +		return NULL;
>> +	__drm_atomic_helper_plane_duplicate_state(plane, &new_shadow_plane_state->base);
>> +
>> +	return &new_shadow_plane_state->base;
>> +}
>> +
>> +static void drm_gem_shmem_destroy_shadow_plane_state(struct drm_plane *plane,
>> +						     struct drm_plane_state *plane_state)
>> +{
>> +	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
>> +		to_drm_gem_shmem_shadow_plane_state(plane_state);
>> +
>> +	__drm_atomic_helper_plane_destroy_state(&shadow_plane_state->base);
>> +	kfree(shadow_plane_state);
>> +}
>> +
>> +static void drm_gem_shmem_reset_shadow_plane(struct drm_plane *plane)
>> +{
>> +	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state;
>> +
>> +	if (plane->state) {
>> +		drm_gem_shmem_destroy_shadow_plane_state(plane, plane->state);
>> +		plane->state = NULL; /* must be set to NULL here */
>> +	}
>> +
>> +	shadow_plane_state = kzalloc(sizeof(*shadow_plane_state), GFP_KERNEL);
>> +	if (!shadow_plane_state)
>> +		return;
>> +	__drm_atomic_helper_plane_reset(plane, &shadow_plane_state->base);
>> +}
>> +
>> +/*
>> + * Helpers for struct drm_plane_helper_funcs
>> + */
>> +
>> +static int drm_gem_shmem_prepare_shadow_fb(struct drm_plane *plane,
>> +					   struct drm_plane_state *plane_state)
>> +{
>> +	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
>> +		to_drm_gem_shmem_shadow_plane_state(plane_state);
>> +	struct drm_framebuffer *fb = plane_state->fb;
>> +	struct drm_gem_object *obj;
>> +	struct dma_buf_map map;
>> +	int ret;
>> +	size_t i;
>> +
>> +	if (!fb)
>> +		return 0;
>> +
>> +	ret = drm_gem_fb_prepare_fb(plane, plane_state);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(shadow_plane_state->map); ++i) {
>> +		obj = drm_gem_fb_get_obj(fb, i);
>> +		if (!obj)
>> +			continue;
>> +		ret = drm_gem_shmem_vmap(obj, &map);
>> +		if (ret)
>> +			goto err_drm_gem_shmem_vunmap;
>> +		shadow_plane_state->map[i] = map;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_drm_gem_shmem_vunmap:
>> +	while (i) {
>> +		--i;
>> +		obj = drm_gem_fb_get_obj(fb, i);
>> +		if (!obj)
>> +			continue;
>> +		drm_gem_shmem_vunmap(obj, &shadow_plane_state->map[i]);
>> +	}
>> +	return ret;
>> +}
>> +
>> +static void drm_gem_shmem_cleanup_shadow_fb(struct drm_plane *plane,
>> +					    struct drm_plane_state *plane_state)
>> +{
>> +	struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
>> +		to_drm_gem_shmem_shadow_plane_state(plane_state);
>> +	struct drm_framebuffer *fb = plane_state->fb;
>> +	size_t i = ARRAY_SIZE(shadow_plane_state->map);
>> +	struct drm_gem_object *obj;
>> +
>> +	if (!fb)
>> +		return;
>> +
>> +	while (i) {
>> +		--i;
>> +		obj = drm_gem_fb_get_obj(fb, i);
>> +		if (!obj)
>> +			continue;
>> +		drm_gem_shmem_vunmap(obj, &shadow_plane_state->map[i]);
>> +	}
>> +}
>> +
>> +/*
>> + * Simple KMS helpers
>> + */
>> +
>> +int drm_gem_shmem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe,
>> +					       struct drm_plane_state *plane_state)
>> +{
>> +	return drm_gem_shmem_prepare_shadow_fb(&pipe->plane, plane_state);
>> +}
>> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_prepare_shadow_fb);
>> +
>> +void drm_gem_shmem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
>> +						struct drm_plane_state *plane_state)
>> +{
>> +	drm_gem_shmem_cleanup_shadow_fb(&pipe->plane, plane_state);
>> +}
>> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_cleanup_shadow_fb);
>> +
>> +void drm_gem_shmem_simple_kms_reset_shadow_plane(struct drm_simple_display_pipe *pipe)
>> +{
>> +	drm_gem_shmem_reset_shadow_plane(&pipe->plane);
>> +}
>> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_reset_shadow_plane);
>> +
>> +struct drm_plane_state *
>> +drm_gem_shmem_simple_kms_duplicate_shadow_plane_state(struct drm_simple_display_pipe *pipe,
>> +						      struct drm_plane_state *plane_state)
>> +{
>> +	return drm_gem_shmem_duplicate_shadow_plane_state(&pipe->plane, plane_state);
>> +}
>> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_duplicate_shadow_plane_state);
>> +
>> +void drm_gem_shmem_simple_kms_destroy_shadow_plane_state(struct drm_simple_display_pipe *pipe,
>> +							 struct drm_plane_state *plane_state)
>> +{
>> +	drm_gem_shmem_destroy_shadow_plane_state(&pipe->plane, plane_state);
>> +}
>> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_destroy_shadow_plane_state);
>> diff --git a/include/drm/drm_gem_shmem_kms_helper.h b/include/drm/drm_gem_shmem_kms_helper.h
>> new file mode 100644
>> index 000000000000..bd42c9c0a39e
>> --- /dev/null
>> +++ b/include/drm/drm_gem_shmem_kms_helper.h
>> @@ -0,0 +1,56 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef __DRM_GEM_SHMEM_KMS_HELPER_H__
>> +#define __DRM_GEM_SHMEM_KMS_HELPER_H__
>> +
>> +#include <linux/dma-buf-map.h>
>> +
>> +#include <drm/drm_plane.h>
>> +
>> +struct drm_simple_display_pipe;
>> +
>> +struct drm_gem_shmem_shadow_plane_state {
>> +	struct drm_plane_state base;
>> +
>> +	/* Transitional state - do not export or duplicate */
>> +
>> +	struct dma_buf_map map[4];
>> +};
>> +
>> +static inline struct drm_gem_shmem_shadow_plane_state *
>> +to_drm_gem_shmem_shadow_plane_state(struct drm_plane_state *state)
>> +{
>> +	return container_of(state, struct drm_gem_shmem_shadow_plane_state, base);
>> +}
>> +
>> +/*
>> + * Simple KMS helpers
>> + */
>> +
>> +int drm_gem_shmem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe,
>> +					       struct drm_plane_state *plane_state);
>> +void drm_gem_shmem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
>> +						struct drm_plane_state *plane_state);
>> +void drm_gem_shmem_simple_kms_reset_shadow_plane(struct drm_simple_display_pipe *pipe);
>> +struct drm_plane_state *
>> +drm_gem_shmem_simple_kms_duplicate_shadow_plane_state(struct drm_simple_display_pipe *pipe,
>> +						      struct drm_plane_state *plane_state);
>> +void
>> +drm_gem_shmem_simple_kms_destroy_shadow_plane_state(struct drm_simple_display_pipe *pipe,
>> +						    struct drm_plane_state *plane_state);
>> +
>> +/**
>> + * DRM_GEM_SHMEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS -
>> + *	Initializes struct drm_simple_display_pipe_funcs for SHMEM shadow planes
>> + *
>> + * Drivers may use GEM SHMEM BOs as shadow buffers over the framebuffer memory. This
>> + * macro initializes struct drm_simple_display_pipe_funcs to use the rsp helper functions.
>> + */
>> +#define DRM_GEM_SHMEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS \
>> +	.prepare_fb = drm_gem_shmem_simple_kms_prepare_shadow_fb, \
>> +	.cleanup_fb = drm_gem_shmem_simple_kms_cleanup_shadow_fb, \
>> +	.reset_plane = drm_gem_shmem_simple_kms_reset_shadow_plane, \
>> +	.duplicate_plane_state = drm_gem_shmem_simple_kms_duplicate_shadow_plane_state, \
>> +	.destroy_plane_state   = drm_gem_shmem_simple_kms_destroy_shadow_plane_state
>> +
>> +#endif /* __DRM_GEM_SHMEM_KMS_HELPER_H__ */
>> -- 
>> 2.30.0
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/6] drm/shmem-helper: Add additional KMS helpers

[Daniel Vetter]

On Wed, Feb 3, 2021 at 3:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 03.02.21 um 15:01 schrieb Daniel Vetter:
> > On Wed, Feb 03, 2021 at 02:10:42PM +0100, Thomas Zimmermann wrote:
> >> Several drivers use GEM SHMEM buffer objects as shadow buffers for
> >> the actual framebuffer memory. Right now, drivers do these vmap
> >> operations in their commit tail, which is actually not allowed by the
> >> locking rules for the dma-buf reservation lock. The involved SHMEM
> >> BO has to be vmapped in the plane's prepare_fb callback and vunmapped
> >> in cleanup_fb.
> >>
> >> This patch introduces a DRM library that implements KMS helpers for
> >> GEM SHMEM buffer objects. The first set of helpers is the plane state
> >> for shadow planes. The provided implementations for prepare_fb and
> >> cleanup_fb vmap and vunmap all BOs of struct drm_plane_state.fb. The
> >> mappings are afterwards available in the plane's commit-tail functions.
> >>
> >> All rsp drivers use the simple KMS helpers, so we add the plane callbacks
> >> and wrappers for simple KMS.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> ---
> >>   drivers/gpu/drm/Kconfig                    |   7 +
> >>   drivers/gpu/drm/Makefile                   |   1 +
> >>   drivers/gpu/drm/drm_gem_shmem_kms_helper.c | 159 +++++++++++++++++++++
> >>   include/drm/drm_gem_shmem_kms_helper.h     |  56 ++++++++
> >>   4 files changed, 223 insertions(+)
> >>   create mode 100644 drivers/gpu/drm/drm_gem_shmem_kms_helper.c
> >>   create mode 100644 include/drm/drm_gem_shmem_kms_helper.h
> >>
> >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> >> index 8bf103de1594..b8d8b00ab5d4 100644
> >> --- a/drivers/gpu/drm/Kconfig
> >> +++ b/drivers/gpu/drm/Kconfig
> >> @@ -214,6 +214,13 @@ config DRM_GEM_SHMEM_HELPER
> >>      help
> >>        Choose this if you need the GEM shmem helper functions
> >>
> >> +config DRM_GEM_SHMEM_KMS_HELPER
> >> +    bool
> >> +    depends on DRM_GEM_SHMEM_HELPER
> >> +    help
> >> +    help
> >> +      Choose this if you need the GEM SHMEM helper functions for KMS
> >
> > I think we should just stuff this into simple pipe helpers directly. Also
> > needs some kerneldoc and ideally intro section of some sorts, but I guess
> > that was just missing for RFC.
>
> The missing docs is why it's an RFC. i was concerned about the
> additional simple-pipe callbacks, but I'm glad you're OK with them. I
> thought about simple_pipe state (as you mentioned below) or
> drm_private_state, but found it all too complex for this simple problem.

Yeah I think for this it's overkill, and it shouldn't be hard to add a
simple_pipe_state later on. Imo once you need a private state your
driver has proably outgrown simple pipe helpers.

> > Thinking a bit bigger and looking at the first patch, I wonder whether we
> > shouldn't outright integrate this simple pipe helpers. Adding all the
> > hooks for plane_state (instead of a simple_pipe_state or something like
> > that) feels a bit icky to me. But on the driver side the integration with
> > just the single macro line is very neat, so that's redeeming feature.
>
> I do disagree with integrating the shadow-plane code into simple-pipe.
> Right now it's SHMEM-depended and CMA-based simple-pipe drivers would
> probably not want to depend on this. The other reason is that I can
> certainly generalize the shadow planes away from SHMEM helpers; and use
> them for these vbox and ast patchsets as well. These drivers use VRAM
> helpers and full KMS helpers. I guess shadow planes could then be moved
> into drm_gem_framebuffer.c. There's other plane-helper code there already.
>
> Unfortunately, I only realized this after sending out the patch set. :)

Yeah I also noticed we have the current simple gem fb helper in there
already, so maybe just stuff it in there. Whether the simple glue for
shme/gem/cma/whatever integration is in drm_simple_pipe.c or
drm_gem_framebuffer.c kinda doesn't matter much. Just yet another
library felt a bit like overkill.

Cheers, Daniel

>
> Best regards
> Thomas
>
> >
> > Note doing a drm_simple_display_pipe_state would be a bit tricky to do,
> > since we'd need custome duplicate_state functions to make sure there's
> > only ever exactly one:
> >
> > struct drm_simple_display_pipe_state {
> >       struct drm_crtc_state crtc_state;
> >       struct drm_plane_state plane_state;
> >
> >       struct dma_buf_map map[4];
> > };
> >
> > But that's a bit a bigger step, maybe when we also need to subclass crtc
> > stuff we can look into this. Or maybe that's then too much feature creep,
> > dunno. Implemenation shouldn't be too tricky:
> > - crtc state just duplicates itself (including plane_state duplication).
> >    In release it also cleans up the plane state.
> > - plane state grabs the crtc state instead, and then does a cast. destroy
> >    hook does nothing (to avoid touching freed memory, since we always dupe
> >    the crtc state when the plane state exists we know the crtc destroy hook
> >    will clean things up).
> >
> > That means drm_atomic_state has 2 pointers pointing into the same
> > allocation, but that should be all fine.
> >
> > Aside from this bikeshed here pondering a bit how this best first into our
> > simple pipe helpers I think this all looks very clean.
> > -Daniel
> >
> >> +
> >>   config DRM_SCHED
> >>      tristate
> >>      depends on DRM
> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> >> index 926adef289db..37a73dee5baf 100644
> >> --- a/drivers/gpu/drm/Makefile
> >> +++ b/drivers/gpu/drm/Makefile
> >> @@ -53,6 +53,7 @@ drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> >>   drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> >>   drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> >>   drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
> >> +drm_kms_helper-$(CONFIG_DRM_GEM_SHMEM_KMS_HELPER) += drm_gem_shmem_kms_helper.o
> >>
> >>   obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> >>   obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_kms_helper.c b/drivers/gpu/drm/drm_gem_shmem_kms_helper.c
> >> new file mode 100644
> >> index 000000000000..8843c5837f98
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/drm_gem_shmem_kms_helper.c
> >> @@ -0,0 +1,159 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +#include <drm/drm_atomic_state_helper.h>
> >> +#include <drm/drm_gem_framebuffer_helper.h>
> >> +#include <drm/drm_gem_shmem_helper.h>
> >> +#include <drm/drm_gem_shmem_kms_helper.h>
> >> +#include <drm/drm_simple_kms_helper.h>
> >> +
> >> +/*
> >> + * Helpers for struct drm_plane_funcs
> >> + *
> >> + */
> >> +
> >> +static struct drm_plane_state *
> >> +drm_gem_shmem_duplicate_shadow_plane_state(struct drm_plane *plane,
> >> +                                       struct drm_plane_state *plane_state)
> >> +{
> >> +    struct drm_gem_shmem_shadow_plane_state *new_shadow_plane_state;
> >> +
> >> +    if (!plane_state)
> >> +            return NULL;
> >> +
> >> +    new_shadow_plane_state = kzalloc(sizeof(*new_shadow_plane_state), GFP_KERNEL);
> >> +    if (!new_shadow_plane_state)
> >> +            return NULL;
> >> +    __drm_atomic_helper_plane_duplicate_state(plane, &new_shadow_plane_state->base);
> >> +
> >> +    return &new_shadow_plane_state->base;
> >> +}
> >> +
> >> +static void drm_gem_shmem_destroy_shadow_plane_state(struct drm_plane *plane,
> >> +                                                 struct drm_plane_state *plane_state)
> >> +{
> >> +    struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
> >> +            to_drm_gem_shmem_shadow_plane_state(plane_state);
> >> +
> >> +    __drm_atomic_helper_plane_destroy_state(&shadow_plane_state->base);
> >> +    kfree(shadow_plane_state);
> >> +}
> >> +
> >> +static void drm_gem_shmem_reset_shadow_plane(struct drm_plane *plane)
> >> +{
> >> +    struct drm_gem_shmem_shadow_plane_state *shadow_plane_state;
> >> +
> >> +    if (plane->state) {
> >> +            drm_gem_shmem_destroy_shadow_plane_state(plane, plane->state);
> >> +            plane->state = NULL; /* must be set to NULL here */
> >> +    }
> >> +
> >> +    shadow_plane_state = kzalloc(sizeof(*shadow_plane_state), GFP_KERNEL);
> >> +    if (!shadow_plane_state)
> >> +            return;
> >> +    __drm_atomic_helper_plane_reset(plane, &shadow_plane_state->base);
> >> +}
> >> +
> >> +/*
> >> + * Helpers for struct drm_plane_helper_funcs
> >> + */
> >> +
> >> +static int drm_gem_shmem_prepare_shadow_fb(struct drm_plane *plane,
> >> +                                       struct drm_plane_state *plane_state)
> >> +{
> >> +    struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
> >> +            to_drm_gem_shmem_shadow_plane_state(plane_state);
> >> +    struct drm_framebuffer *fb = plane_state->fb;
> >> +    struct drm_gem_object *obj;
> >> +    struct dma_buf_map map;
> >> +    int ret;
> >> +    size_t i;
> >> +
> >> +    if (!fb)
> >> +            return 0;
> >> +
> >> +    ret = drm_gem_fb_prepare_fb(plane, plane_state);
> >> +    if (ret)
> >> +            return ret;
> >> +
> >> +    for (i = 0; i < ARRAY_SIZE(shadow_plane_state->map); ++i) {
> >> +            obj = drm_gem_fb_get_obj(fb, i);
> >> +            if (!obj)
> >> +                    continue;
> >> +            ret = drm_gem_shmem_vmap(obj, &map);
> >> +            if (ret)
> >> +                    goto err_drm_gem_shmem_vunmap;
> >> +            shadow_plane_state->map[i] = map;
> >> +    }
> >> +
> >> +    return 0;
> >> +
> >> +err_drm_gem_shmem_vunmap:
> >> +    while (i) {
> >> +            --i;
> >> +            obj = drm_gem_fb_get_obj(fb, i);
> >> +            if (!obj)
> >> +                    continue;
> >> +            drm_gem_shmem_vunmap(obj, &shadow_plane_state->map[i]);
> >> +    }
> >> +    return ret;
> >> +}
> >> +
> >> +static void drm_gem_shmem_cleanup_shadow_fb(struct drm_plane *plane,
> >> +                                        struct drm_plane_state *plane_state)
> >> +{
> >> +    struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
> >> +            to_drm_gem_shmem_shadow_plane_state(plane_state);
> >> +    struct drm_framebuffer *fb = plane_state->fb;
> >> +    size_t i = ARRAY_SIZE(shadow_plane_state->map);
> >> +    struct drm_gem_object *obj;
> >> +
> >> +    if (!fb)
> >> +            return;
> >> +
> >> +    while (i) {
> >> +            --i;
> >> +            obj = drm_gem_fb_get_obj(fb, i);
> >> +            if (!obj)
> >> +                    continue;
> >> +            drm_gem_shmem_vunmap(obj, &shadow_plane_state->map[i]);
> >> +    }
> >> +}
> >> +
> >> +/*
> >> + * Simple KMS helpers
> >> + */
> >> +
> >> +int drm_gem_shmem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe,
> >> +                                           struct drm_plane_state *plane_state)
> >> +{
> >> +    return drm_gem_shmem_prepare_shadow_fb(&pipe->plane, plane_state);
> >> +}
> >> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_prepare_shadow_fb);
> >> +
> >> +void drm_gem_shmem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
> >> +                                            struct drm_plane_state *plane_state)
> >> +{
> >> +    drm_gem_shmem_cleanup_shadow_fb(&pipe->plane, plane_state);
> >> +}
> >> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_cleanup_shadow_fb);
> >> +
> >> +void drm_gem_shmem_simple_kms_reset_shadow_plane(struct drm_simple_display_pipe *pipe)
> >> +{
> >> +    drm_gem_shmem_reset_shadow_plane(&pipe->plane);
> >> +}
> >> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_reset_shadow_plane);
> >> +
> >> +struct drm_plane_state *
> >> +drm_gem_shmem_simple_kms_duplicate_shadow_plane_state(struct drm_simple_display_pipe *pipe,
> >> +                                                  struct drm_plane_state *plane_state)
> >> +{
> >> +    return drm_gem_shmem_duplicate_shadow_plane_state(&pipe->plane, plane_state);
> >> +}
> >> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_duplicate_shadow_plane_state);
> >> +
> >> +void drm_gem_shmem_simple_kms_destroy_shadow_plane_state(struct drm_simple_display_pipe *pipe,
> >> +                                                     struct drm_plane_state *plane_state)
> >> +{
> >> +    drm_gem_shmem_destroy_shadow_plane_state(&pipe->plane, plane_state);
> >> +}
> >> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_destroy_shadow_plane_state);
> >> diff --git a/include/drm/drm_gem_shmem_kms_helper.h b/include/drm/drm_gem_shmem_kms_helper.h
> >> new file mode 100644
> >> index 000000000000..bd42c9c0a39e
> >> --- /dev/null
> >> +++ b/include/drm/drm_gem_shmem_kms_helper.h
> >> @@ -0,0 +1,56 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +
> >> +#ifndef __DRM_GEM_SHMEM_KMS_HELPER_H__
> >> +#define __DRM_GEM_SHMEM_KMS_HELPER_H__
> >> +
> >> +#include <linux/dma-buf-map.h>
> >> +
> >> +#include <drm/drm_plane.h>
> >> +
> >> +struct drm_simple_display_pipe;
> >> +
> >> +struct drm_gem_shmem_shadow_plane_state {
> >> +    struct drm_plane_state base;
> >> +
> >> +    /* Transitional state - do not export or duplicate */
> >> +
> >> +    struct dma_buf_map map[4];
> >> +};
> >> +
> >> +static inline struct drm_gem_shmem_shadow_plane_state *
> >> +to_drm_gem_shmem_shadow_plane_state(struct drm_plane_state *state)
> >> +{
> >> +    return container_of(state, struct drm_gem_shmem_shadow_plane_state, base);
> >> +}
> >> +
> >> +/*
> >> + * Simple KMS helpers
> >> + */
> >> +
> >> +int drm_gem_shmem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe,
> >> +                                           struct drm_plane_state *plane_state);
> >> +void drm_gem_shmem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
> >> +                                            struct drm_plane_state *plane_state);
> >> +void drm_gem_shmem_simple_kms_reset_shadow_plane(struct drm_simple_display_pipe *pipe);
> >> +struct drm_plane_state *
> >> +drm_gem_shmem_simple_kms_duplicate_shadow_plane_state(struct drm_simple_display_pipe *pipe,
> >> +                                                  struct drm_plane_state *plane_state);
> >> +void
> >> +drm_gem_shmem_simple_kms_destroy_shadow_plane_state(struct drm_simple_display_pipe *pipe,
> >> +                                                struct drm_plane_state *plane_state);
> >> +
> >> +/**
> >> + * DRM_GEM_SHMEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS -
> >> + *  Initializes struct drm_simple_display_pipe_funcs for SHMEM shadow planes
> >> + *
> >> + * Drivers may use GEM SHMEM BOs as shadow buffers over the framebuffer memory. This
> >> + * macro initializes struct drm_simple_display_pipe_funcs to use the rsp helper functions.
> >> + */
> >> +#define DRM_GEM_SHMEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS \
> >> +    .prepare_fb = drm_gem_shmem_simple_kms_prepare_shadow_fb, \
> >> +    .cleanup_fb = drm_gem_shmem_simple_kms_cleanup_shadow_fb, \
> >> +    .reset_plane = drm_gem_shmem_simple_kms_reset_shadow_plane, \
> >> +    .duplicate_plane_state = drm_gem_shmem_simple_kms_duplicate_shadow_plane_state, \
> >> +    .destroy_plane_state   = drm_gem_shmem_simple_kms_destroy_shadow_plane_state
> >> +
> >> +#endif /* __DRM_GEM_SHMEM_KMS_HELPER_H__ */
> >> --
> >> 2.30.0
> >>
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/6] drm/cirrus: Move vmap out of commit tail

[Thomas Zimmermann]

Vmap operations may acquire the dmabuf reservation lock, which is not
allowed within atomic commit-tail functions. Therefore move vmap and
vunmap from the damage handler into prepare_fb and cleanup_fb callbacks.

The mapping is provided as GEM shadow-buffered plane. The functions in
the commit tail use the pre-established mapping for damage handling.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/cirrus.c | 43 ++++++++++++++---------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
index a043e602199e..ad922c3ec681 100644
--- a/drivers/gpu/drm/tiny/cirrus.c
+++ b/drivers/gpu/drm/tiny/cirrus.c
@@ -33,8 +33,9 @@
 #include <drm/drm_file.h>
 #include <drm/drm_format_helper.h>
 #include <drm/drm_fourcc.h>
-#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_ioctl.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_modeset_helper_vtables.h>
@@ -311,22 +312,15 @@ static int cirrus_mode_set(struct cirrus_device *cirrus,
 	return 0;
 }
 
-static int cirrus_fb_blit_rect(struct drm_framebuffer *fb,
+static int cirrus_fb_blit_rect(struct drm_framebuffer *fb, const struct dma_buf_map *map,
 			       struct drm_rect *rect)
 {
 	struct cirrus_device *cirrus = to_cirrus(fb->dev);
-	struct dma_buf_map map;
-	void *vmap;
-	int idx, ret;
+	void *vmap = map->vaddr; /* TODO: Use mapping abstraction properly */
+	int idx;
 
-	ret = -ENODEV;
 	if (!drm_dev_enter(&cirrus->dev, &idx))
-		goto out;
-
-	ret = drm_gem_shmem_vmap(fb->obj[0], &map);
-	if (ret)
-		goto out_dev_exit;
-	vmap = map.vaddr; /* TODO: Use mapping abstraction properly */
+		return -ENODEV;
 
 	if (cirrus->cpp == fb->format->cpp[0])
 		drm_fb_memcpy_dstclip(cirrus->vram,
@@ -345,16 +339,12 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb,
 	else
 		WARN_ON_ONCE("cpp mismatch");
 
-	drm_gem_shmem_vunmap(fb->obj[0], &map);
-	ret = 0;
-
-out_dev_exit:
 	drm_dev_exit(idx);
-out:
-	return ret;
+
+	return 0;
 }
 
-static int cirrus_fb_blit_fullscreen(struct drm_framebuffer *fb)
+static int cirrus_fb_blit_fullscreen(struct drm_framebuffer *fb, const struct dma_buf_map *map)
 {
 	struct drm_rect fullscreen = {
 		.x1 = 0,
@@ -362,7 +352,7 @@ static int cirrus_fb_blit_fullscreen(struct drm_framebuffer *fb)
 		.y1 = 0,
 		.y2 = fb->height,
 	};
-	return cirrus_fb_blit_rect(fb, &fullscreen);
+	return cirrus_fb_blit_rect(fb, map, &fullscreen);
 }
 
 static int cirrus_check_size(int width, int height,
@@ -441,9 +431,10 @@ static void cirrus_pipe_enable(struct drm_simple_display_pipe *pipe,
 			       struct drm_plane_state *plane_state)
 {
 	struct cirrus_device *cirrus = to_cirrus(pipe->crtc.dev);
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
 
 	cirrus_mode_set(cirrus, &crtc_state->mode, plane_state->fb);
-	cirrus_fb_blit_fullscreen(plane_state->fb);
+	cirrus_fb_blit_fullscreen(plane_state->fb, &shadow_plane_state->map[0]);
 }
 
 static void cirrus_pipe_update(struct drm_simple_display_pipe *pipe,
@@ -451,16 +442,15 @@ static void cirrus_pipe_update(struct drm_simple_display_pipe *pipe,
 {
 	struct cirrus_device *cirrus = to_cirrus(pipe->crtc.dev);
 	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
 	struct drm_crtc *crtc = &pipe->crtc;
 	struct drm_rect rect;
 
-	if (pipe->plane.state->fb &&
-	    cirrus->cpp != cirrus_cpp(pipe->plane.state->fb))
-		cirrus_mode_set(cirrus, &crtc->mode,
-				pipe->plane.state->fb);
+	if (state->fb && cirrus->cpp != cirrus_cpp(state->fb))
+		cirrus_mode_set(cirrus, &crtc->mode, state->fb);
 
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
-		cirrus_fb_blit_rect(pipe->plane.state->fb, &rect);
+		cirrus_fb_blit_rect(state->fb, &shadow_plane_state->map[0], &rect);
 }
 
 static const struct drm_simple_display_pipe_funcs cirrus_pipe_funcs = {
@@ -468,6 +458,7 @@ static const struct drm_simple_display_pipe_funcs cirrus_pipe_funcs = {
 	.check	    = cirrus_pipe_check,
 	.enable	    = cirrus_pipe_enable,
 	.update	    = cirrus_pipe_update,
+	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
 };
 
 static const uint32_t cirrus_formats[] = {
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel