[PATCH v2 0/5] drm: Provide framebuffer vmap helpers

[PATCH v2 0/5] drm: Provide framebuffer vmap helpers

[Thomas Zimmermann]

Add the new helpers drm_gem_fb_vmap() and drm_gem_fb_vunmap(), which
provide vmap/vunmap for all BOs of a framebuffer. Convert shadow-
plane helpers, gud and vkms.

Callers of GEM vmap and vunmap functions used to do the minumum work
or get some detail wrong. Therefore shadow-plane helpers were intro-
duced to implement the details for all callers. The vmapping code in
the shadow-plane helpers is also useful for gud and vkms. So it makes
sense to provide rsp helpers. Simply call drm_gem_fb_vmap() to
retrieve mappings of all of a framebuffer's BOs.

Future work: besides the mapping's addresses, drm_gem_fb_vmap() should
also return the mappings with the frambuffer data offset added. These
are the addresses were the actual image data is located. A follow-up
set of patches will implement this feature.

v2:
	* update commit message for first patch (Maxime)
	* fix error handling after DRM_FORMAT_MAX_PLANES changes
	  (kernel test robot)
	* fix includes (kernel test robot)
	* use [static N] notations for array parameters

Thomas Zimmermann (5):
  drm: Define DRM_FORMAT_MAX_PLANES
  drm/gem: Provide drm_gem_fb_{vmap,vunmap}()
  drm/gem: Clear mapping addresses for unused framebuffer planes
  drm/gud: Map framebuffer BOs with drm_gem_fb_vmap()
  drm/vkms: Map output framebuffer BOs with drm_gem_fb_vmap()

 drivers/gpu/drm/drm_gem_atomic_helper.c      | 37 +-------
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 96 ++++++++++++++++++--
 drivers/gpu/drm/gud/gud_pipe.c               | 10 +-
 drivers/gpu/drm/vkms/vkms_composer.c         |  2 +-
 drivers/gpu/drm/vkms/vkms_drv.h              |  6 +-
 drivers/gpu/drm/vkms/vkms_writeback.c        | 21 ++---
 include/drm/drm_fourcc.h                     | 13 ++-
 include/drm/drm_framebuffer.h                |  8 +-
 include/drm/drm_gem_atomic_helper.h          |  3 +-
 include/drm/drm_gem_framebuffer_helper.h     |  6 ++
 10 files changed, 133 insertions(+), 69 deletions(-)


base-commit: 2bda1ca4d4acb4892556fec3a7ea1f02afcd40bb
prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
--
2.32.0

[PATCH v2 1/5] drm: Define DRM_FORMAT_MAX_PLANES

[Thomas Zimmermann]

DRM uses a magic number of 4 for the maximum number of planes per color
format. Declare this constant via DRM_FORMAT_MAX_PLANES and update the
related code. Some code depends on the length of arrays that are now
declared with DRM_FORMAT_MAX_PLANES. Convert it from '4' to ARRAY_SIZE.

v2:
	* mention usage of ARRAY_SIZE() in the commit message (Maxime)
	* also fix error handling in drm_gem_fb_init_with_funcs()
	  (kernel test robot)
	* include <drm/drm_fourcc.h> for DRM_FORMAT_MAX_PLANES

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 19 +++++++++++--------
 include/drm/drm_fourcc.h                     | 13 +++++++++----
 include/drm/drm_framebuffer.h                |  8 ++++----
 include/drm/drm_gem_atomic_helper.h          |  3 ++-
 4 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index 67bc9edc1d98..421e029a6b3e 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -48,7 +48,7 @@
 struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
 					  unsigned int plane)
 {
-	if (plane >= 4)
+	if (plane >= ARRAY_SIZE(fb->obj))
 		return NULL;
 
 	return fb->obj[plane];
@@ -62,7 +62,8 @@ drm_gem_fb_init(struct drm_device *dev,
 		 struct drm_gem_object **obj, unsigned int num_planes,
 		 const struct drm_framebuffer_funcs *funcs)
 {
-	int ret, i;
+	unsigned int i;
+	int ret;
 
 	drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
 
@@ -86,9 +87,9 @@ drm_gem_fb_init(struct drm_device *dev,
  */
 void drm_gem_fb_destroy(struct drm_framebuffer *fb)
 {
-	int i;
+	size_t i;
 
-	for (i = 0; i < 4; i++)
+	for (i = 0; i < ARRAY_SIZE(fb->obj); i++)
 		drm_gem_object_put(fb->obj[i]);
 
 	drm_framebuffer_cleanup(fb);
@@ -145,8 +146,9 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev,
 			       const struct drm_framebuffer_funcs *funcs)
 {
 	const struct drm_format_info *info;
-	struct drm_gem_object *objs[4];
-	int ret, i;
+	struct drm_gem_object *objs[DRM_FORMAT_MAX_PLANES];
+	unsigned int i;
+	int ret;
 
 	info = drm_get_format_info(dev, mode_cmd);
 	if (!info) {
@@ -187,9 +189,10 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev,
 	return 0;
 
 err_gem_object_put:
-	for (i--; i >= 0; i--)
+	while (i > 0) {
+		--i;
 		drm_gem_object_put(objs[i]);
-
+	}
 	return ret;
 }
 EXPORT_SYMBOL_GPL(drm_gem_fb_init_with_funcs);
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 3b138d4ae67e..22aa64d07c79 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -25,6 +25,11 @@
 #include <linux/types.h>
 #include <uapi/drm/drm_fourcc.h>
 
+/**
+ * DRM_FORMAT_MAX_PLANES - maximum number of planes a DRM format can have
+ */
+#define DRM_FORMAT_MAX_PLANES	4u
+
 /*
  * DRM formats are little endian.  Define host endian variants for the
  * most common formats here, to reduce the #ifdefs needed in drivers.
@@ -78,7 +83,7 @@ struct drm_format_info {
 		 * triplet @char_per_block, @block_w, @block_h for better
 		 * describing the pixel format.
 		 */
-		u8 cpp[4];
+		u8 cpp[DRM_FORMAT_MAX_PLANES];
 
 		/**
 		 * @char_per_block:
@@ -104,7 +109,7 @@ struct drm_format_info {
 		 * information from their drm_mode_config.get_format_info hook
 		 * if they want the core to be validating the pitch.
 		 */
-		u8 char_per_block[4];
+		u8 char_per_block[DRM_FORMAT_MAX_PLANES];
 	};
 
 	/**
@@ -113,7 +118,7 @@ struct drm_format_info {
 	 * Block width in pixels, this is intended to be accessed through
 	 * drm_format_info_block_width()
 	 */
-	u8 block_w[4];
+	u8 block_w[DRM_FORMAT_MAX_PLANES];
 
 	/**
 	 * @block_h:
@@ -121,7 +126,7 @@ struct drm_format_info {
 	 * Block height in pixels, this is intended to be accessed through
 	 * drm_format_info_block_height()
 	 */
-	u8 block_h[4];
+	u8 block_h[DRM_FORMAT_MAX_PLANES];
 
 	/** @hsub: Horizontal chroma subsampling factor */
 	u8 hsub;
diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
index be658ebbec72..f67c5b7bcb68 100644
--- a/include/drm/drm_framebuffer.h
+++ b/include/drm/drm_framebuffer.h
@@ -27,12 +27,12 @@
 #include <linux/list.h>
 #include <linux/sched.h>
 
+#include <drm/drm_fourcc.h>
 #include <drm/drm_mode_object.h>
 
 struct drm_clip_rect;
 struct drm_device;
 struct drm_file;
-struct drm_format_info;
 struct drm_framebuffer;
 struct drm_gem_object;
 
@@ -147,7 +147,7 @@ struct drm_framebuffer {
 	 * @pitches: Line stride per buffer. For userspace created object this
 	 * is copied from drm_mode_fb_cmd2.
 	 */
-	unsigned int pitches[4];
+	unsigned int pitches[DRM_FORMAT_MAX_PLANES];
 	/**
 	 * @offsets: Offset from buffer start to the actual pixel data in bytes,
 	 * per buffer. For userspace created object this is copied from
@@ -165,7 +165,7 @@ struct drm_framebuffer {
 	 * data (even for linear buffers). Specifying an x/y pixel offset is
 	 * instead done through the source rectangle in &struct drm_plane_state.
 	 */
-	unsigned int offsets[4];
+	unsigned int offsets[DRM_FORMAT_MAX_PLANES];
 	/**
 	 * @modifier: Data layout modifier. This is used to describe
 	 * tiling, or also special layouts (like compression) of auxiliary
@@ -210,7 +210,7 @@ struct drm_framebuffer {
 	 * This is used by the GEM framebuffer helpers, see e.g.
 	 * drm_gem_fb_create().
 	 */
-	struct drm_gem_object *obj[4];
+	struct drm_gem_object *obj[DRM_FORMAT_MAX_PLANES];
 };
 
 #define obj_to_fb(x) container_of(x, struct drm_framebuffer, base)
diff --git a/include/drm/drm_gem_atomic_helper.h b/include/drm/drm_gem_atomic_helper.h
index d82c23622156..f9f8b6f0494a 100644
--- a/include/drm/drm_gem_atomic_helper.h
+++ b/include/drm/drm_gem_atomic_helper.h
@@ -5,6 +5,7 @@
 
 #include <linux/dma-buf-map.h>
 
+#include <drm/drm_fourcc.h>
 #include <drm/drm_plane.h>
 
 struct drm_simple_display_pipe;
@@ -40,7 +41,7 @@ struct drm_shadow_plane_state {
 	 * The memory mappings stored in map should be established in the plane's
 	 * prepare_fb callback and removed in the cleanup_fb callback.
 	 */
-	struct dma_buf_map map[4];
+	struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
 };
 
 /**
-- 
2.32.0

[PATCH v2 2/5] drm/gem: Provide drm_gem_fb_{vmap,vunmap}()

[Thomas Zimmermann]

Move framebuffer vmap code from shadow-buffered plane state into the new
interfaces drm_gem_fb_vmap() and drm_gem_fb_vunmap(). These functions
provide mappings of a framebuffer's BOs into kernel address space. No
functional changes.

v2:
	* using [static N] for array parameters enables compile-time checks
	* include <drm/drm_fourcc.h> for DRM_FORMAT_MAX_PLANES (kernel
	  test robot)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_atomic_helper.c      | 37 +---------
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 73 ++++++++++++++++++++
 include/drm/drm_gem_framebuffer_helper.h     |  6 ++
 3 files changed, 82 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
index 26af09b959d4..b1cc19e47165 100644
--- a/drivers/gpu/drm/drm_gem_atomic_helper.c
+++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
@@ -330,10 +330,7 @@ int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *p
 {
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_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;
@@ -342,27 +339,7 @@ int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *p
 	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_vmap(obj, &map);
-		if (ret)
-			goto err_drm_gem_vunmap;
-		shadow_plane_state->map[i] = map;
-	}
-
-	return 0;
-
-err_drm_gem_vunmap:
-	while (i) {
-		--i;
-		obj = drm_gem_fb_get_obj(fb, i);
-		if (!obj)
-			continue;
-		drm_gem_vunmap(obj, &shadow_plane_state->map[i]);
-	}
-	return ret;
+	return drm_gem_fb_vmap(fb, shadow_plane_state->map);
 }
 EXPORT_SYMBOL(drm_gem_prepare_shadow_fb);
 
@@ -374,25 +351,17 @@ EXPORT_SYMBOL(drm_gem_prepare_shadow_fb);
  * This function implements struct &drm_plane_helper_funcs.cleanup_fb.
  * This function unmaps all buffer objects of the plane's framebuffer.
  *
- * See drm_gem_prepare_shadow_fb() for more inforamtion.
+ * See drm_gem_prepare_shadow_fb() for more information.
  */
 void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state)
 {
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_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_vunmap(obj, &shadow_plane_state->map[i]);
-	}
+	drm_gem_fb_vunmap(fb, shadow_plane_state->map);
 }
 EXPORT_SYMBOL(drm_gem_cleanup_shadow_fb);
 
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index 421e029a6b3e..243affbad437 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -15,6 +15,8 @@
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_modeset_helper.h>
 
+#include "drm_internal.h"
+
 #define AFBC_HEADER_SIZE		16
 #define AFBC_TH_LAYOUT_ALIGNMENT	8
 #define AFBC_HDR_ALIGN			64
@@ -309,6 +311,77 @@ drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file,
 }
 EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_dirty);
 
+
+/**
+ * drm_gem_fb_vmap - maps all framebuffer BOs into kernel address space
+ * @fb: the framebuffer
+ * @map: returns the mapping's address for each BO
+ *
+ * This function maps all buffer objects of the given framebuffer into
+ * kernel address space and stores them in struct dma_buf_map. If the
+ * mapping operation fails for one of the BOs, the function unmaps the
+ * already established mappings automatically.
+ *
+ * See drm_gem_fb_vunmap() for unmapping.
+ *
+ * Returns:
+ * 0 on success, or a negative errno code otherwise.
+ */
+int drm_gem_fb_vmap(struct drm_framebuffer *fb,
+		    struct dma_buf_map map[static DRM_FORMAT_MAX_PLANES])
+{
+	struct drm_gem_object *obj;
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < DRM_FORMAT_MAX_PLANES; ++i) {
+		obj = drm_gem_fb_get_obj(fb, i);
+		if (!obj)
+			continue;
+		ret = drm_gem_vmap(obj, &map[i]);
+		if (ret)
+			goto err_drm_gem_vunmap;
+	}
+
+	return 0;
+
+err_drm_gem_vunmap:
+	while (i) {
+		--i;
+		obj = drm_gem_fb_get_obj(fb, i);
+		if (!obj)
+			continue;
+		drm_gem_vunmap(obj, &map[i]);
+	}
+	return ret;
+}
+EXPORT_SYMBOL(drm_gem_fb_vmap);
+
+/**
+ * drm_gem_fb_vunmap - unmaps framebuffer BOs from kernel address space
+ * @fb: the framebuffer
+ * @map: mapping addresses as returned by drm_gem_fb_vmap()
+ *
+ * This function unmaps all buffer objects of the given framebuffer.
+ *
+ * See drm_gem_fb_vmap() for more information.
+ */
+void drm_gem_fb_vunmap(struct drm_framebuffer *fb,
+		       struct dma_buf_map map[static DRM_FORMAT_MAX_PLANES])
+{
+	unsigned int i = DRM_FORMAT_MAX_PLANES;
+	struct drm_gem_object *obj;
+
+	while (i) {
+		--i;
+		obj = drm_gem_fb_get_obj(fb, i);
+		if (!obj)
+			continue;
+		drm_gem_vunmap(obj, &map[i]);
+	}
+}
+EXPORT_SYMBOL(drm_gem_fb_vunmap);
+
 /**
  * drm_gem_fb_begin_cpu_access - prepares GEM buffer objects for CPU access
  * @fb: the framebuffer
diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
index 5705722f0855..ff2024dd7b77 100644
--- a/include/drm/drm_gem_framebuffer_helper.h
+++ b/include/drm/drm_gem_framebuffer_helper.h
@@ -4,6 +4,8 @@
 #include <linux/dma-buf.h>
 #include <linux/dma-buf-map.h>
 
+#include <drm/drm_fourcc.h>
+
 struct drm_afbc_framebuffer;
 struct drm_device;
 struct drm_fb_helper_surface_size;
@@ -37,6 +39,10 @@ struct drm_framebuffer *
 drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file,
 			     const struct drm_mode_fb_cmd2 *mode_cmd);
 
+int drm_gem_fb_vmap(struct drm_framebuffer *fb,
+		    struct dma_buf_map map[static DRM_FORMAT_MAX_PLANES]);
+void drm_gem_fb_vunmap(struct drm_framebuffer *fb,
+		       struct dma_buf_map map[static DRM_FORMAT_MAX_PLANES]);
 int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir);
 void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir);
 
-- 
2.32.0

[PATCH v2 3/5] drm/gem: Clear mapping addresses for unused framebuffer planes

[Thomas Zimmermann]

Set the returned mapping address to NULL if a framebuffer plane does
not have a BO associated with it. Likewise, ignore mappings of NULL
during framebuffer unmap operations. Allows users of the functions to
perform unmap operations of certain BOs by themselfes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index 243affbad437..02928607a716 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -336,8 +336,10 @@ int drm_gem_fb_vmap(struct drm_framebuffer *fb,
 
 	for (i = 0; i < DRM_FORMAT_MAX_PLANES; ++i) {
 		obj = drm_gem_fb_get_obj(fb, i);
-		if (!obj)
+		if (!obj) {
+			dma_buf_map_clear(&map[i]);
 			continue;
+		}
 		ret = drm_gem_vmap(obj, &map[i]);
 		if (ret)
 			goto err_drm_gem_vunmap;
@@ -377,6 +379,8 @@ void drm_gem_fb_vunmap(struct drm_framebuffer *fb,
 		obj = drm_gem_fb_get_obj(fb, i);
 		if (!obj)
 			continue;
+		if (dma_buf_map_is_null(&map[i]))
+			continue;
 		drm_gem_vunmap(obj, &map[i]);
 	}
 }
-- 
2.32.0

[PATCH v2 4/5] drm/gud: Map framebuffer BOs with drm_gem_fb_vmap()

[Thomas Zimmermann]

Abstract the framebuffer details by mapping its BOs with a call
to drm_gem_fb_vmap(). Unmap with drm_gem_fb_vunmap().

The call to drm_gem_fb_vmap() ensures that all BOs are mapped
correctly. Gud still only supports single-plane formats.

No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Acked-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/gud/gud_pipe.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index 4d7a26b68a2e..7e009f562b30 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -14,8 +14,8 @@
 #include <drm/drm_format_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_framebuffer.h>
+#include <drm/drm_gem.h>
 #include <drm/drm_gem_framebuffer_helper.h>
-#include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_print.h>
 #include <drm/drm_rect.h>
 #include <drm/drm_simple_kms_helper.h>
@@ -152,7 +152,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
 {
 	struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
 	u8 compression = gdrm->compression;
-	struct dma_buf_map map;
+	struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
 	void *vaddr, *buf;
 	size_t pitch, len;
 	int ret = 0;
@@ -162,11 +162,11 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
 	if (len > gdrm->bulk_len)
 		return -E2BIG;
 
-	ret = drm_gem_shmem_vmap(fb->obj[0], &map);
+	ret = drm_gem_fb_vmap(fb, map);
 	if (ret)
 		return ret;
 
-	vaddr = map.vaddr + fb->offsets[0];
+	vaddr = map[0].vaddr + fb->offsets[0];
 
 	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
 	if (ret)
@@ -225,7 +225,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
 end_cpu_access:
 	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 vunmap:
-	drm_gem_shmem_vunmap(fb->obj[0], &map);
+	drm_gem_fb_vunmap(fb, map);
 
 	return ret;
 }
-- 
2.32.0

[PATCH v2 5/5] drm/vkms: Map output framebuffer BOs with drm_gem_fb_vmap()

[Thomas Zimmermann]

Abstract the framebuffer details by mappings its BOs with a call
to drm_gem_fb_vmap(). Unmap with drm_gem_fb_vunamp().

Before, the output address with stored as raw pointer in the priv
field of struct drm_writeback_job. Introduce the new type
struct vkms_writeback_job, which holds the output mappings addresses
while the writeback job is active.

The patchset also cleans up some internal casting an setup of the
output addresses. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/vkms/vkms_composer.c  |  2 +-
 drivers/gpu/drm/vkms/vkms_drv.h       |  6 +++++-
 drivers/gpu/drm/vkms/vkms_writeback.c | 21 ++++++++++-----------
 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index ead8fff81f30..49f109c3a2b3 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -257,7 +257,7 @@ void vkms_composer_worker(struct work_struct *work)
 		return;
 
 	if (wb_pending)
-		vaddr_out = crtc_state->active_writeback;
+		vaddr_out = crtc_state->active_writeback->map[0].vaddr;
 
 	ret = compose_active_planes(&vaddr_out, primary_composer,
 				    crtc_state);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 8c731b6dcba7..8bc9e3f52e1f 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -20,6 +20,10 @@
 #define XRES_MAX  8192
 #define YRES_MAX  8192
 
+struct vkms_writeback_job {
+	struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
+};
+
 struct vkms_composer {
 	struct drm_framebuffer fb;
 	struct drm_rect src, dst;
@@ -57,7 +61,7 @@ struct vkms_crtc_state {
 	int num_active_planes;
 	/* stack of active planes for crc computation, should be in z order */
 	struct vkms_plane_state **active_planes;
-	void *active_writeback;
+	struct vkms_writeback_job *active_writeback;
 
 	/* below four are protected by vkms_output.composer_lock */
 	bool crc_pending;
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index 0935686475a0..765bb85ba76c 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -65,21 +65,23 @@ static int vkms_wb_connector_get_modes(struct drm_connector *connector)
 static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
 			       struct drm_writeback_job *job)
 {
-	struct drm_gem_object *gem_obj;
-	struct dma_buf_map map;
+	struct vkms_writeback_job *vkmsjob;
 	int ret;
 
 	if (!job->fb)
 		return 0;
 
-	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
-	ret = drm_gem_shmem_vmap(gem_obj, &map);
+	vkmsjob = kzalloc(sizeof(*vkmsjob), GFP_KERNEL);
+	if (!vkmsjob)
+		return -ENOMEM;
+
+	ret = drm_gem_fb_vmap(job->fb, vkmsjob->map);
 	if (ret) {
 		DRM_ERROR("vmap failed: %d\n", ret);
 		return ret;
 	}
 
-	job->priv = map.vaddr;
+	job->priv = vkmsjob;
 
 	return 0;
 }
@@ -87,18 +89,15 @@ static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
 static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
 				struct drm_writeback_job *job)
 {
-	struct drm_gem_object *gem_obj;
+	struct vkms_writeback_job *vkmsjob = job->priv;
 	struct vkms_device *vkmsdev;
-	struct dma_buf_map map;
 
 	if (!job->fb)
 		return;
 
-	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
-	dma_buf_map_set_vaddr(&map, job->priv);
-	drm_gem_shmem_vunmap(gem_obj, &map);
+	drm_gem_fb_vunmap(job->fb, vkmsjob->map);
 
-	vkmsdev = drm_device_to_vkms_device(gem_obj->dev);
+	vkmsdev = drm_device_to_vkms_device(job->fb->dev);
 	vkms_set_composer(&vkmsdev->output, false);
 }
 
-- 
2.32.0

Re: [PATCH v2 1/5] drm: Define DRM_FORMAT_MAX_PLANES

[Sam Ravnborg]

Hi Thomas,

On Sun, Jul 25, 2021 at 07:44:34PM +0200, Thomas Zimmermann wrote:
> DRM uses a magic number of 4 for the maximum number of planes per color
> format. Declare this constant via DRM_FORMAT_MAX_PLANES and update the
> related code. Some code depends on the length of arrays that are now
> declared with DRM_FORMAT_MAX_PLANES. Convert it from '4' to ARRAY_SIZE.
> 
> v2:
> 	* mention usage of ARRAY_SIZE() in the commit message (Maxime)
> 	* also fix error handling in drm_gem_fb_init_with_funcs()
> 	  (kernel test robot)
> 	* include <drm/drm_fourcc.h> for DRM_FORMAT_MAX_PLANES
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

One nit below.
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 19 +++++++++++--------
>  include/drm/drm_fourcc.h                     | 13 +++++++++----
>  include/drm/drm_framebuffer.h                |  8 ++++----
>  include/drm/drm_gem_atomic_helper.h          |  3 ++-
>  4 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index 67bc9edc1d98..421e029a6b3e 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -48,7 +48,7 @@
>  struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
>  					  unsigned int plane)
>  {
> -	if (plane >= 4)
> +	if (plane >= ARRAY_SIZE(fb->obj))
>  		return NULL;
>  
>  	return fb->obj[plane];
> @@ -62,7 +62,8 @@ drm_gem_fb_init(struct drm_device *dev,
>  		 struct drm_gem_object **obj, unsigned int num_planes,
>  		 const struct drm_framebuffer_funcs *funcs)
>  {
> -	int ret, i;
> +	unsigned int i;
> +	int ret;
>  
>  	drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
>
This change looks accidental/unrelated.
But I guess it is to be consistent in use of unsigned int when
iterating the array.

> @@ -86,9 +87,9 @@ drm_gem_fb_init(struct drm_device *dev,
>   */
>  void drm_gem_fb_destroy(struct drm_framebuffer *fb)
>  {
> -	int i;
> +	size_t i;
>  
> -	for (i = 0; i < 4; i++)
> +	for (i = 0; i < ARRAY_SIZE(fb->obj); i++)
>  		drm_gem_object_put(fb->obj[i]);
>  
>  	drm_framebuffer_cleanup(fb);
> @@ -145,8 +146,9 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev,
>  			       const struct drm_framebuffer_funcs *funcs)
>  {
>  	const struct drm_format_info *info;
> -	struct drm_gem_object *objs[4];
> -	int ret, i;
> +	struct drm_gem_object *objs[DRM_FORMAT_MAX_PLANES];
> +	unsigned int i;
> +	int ret;
>  
>  	info = drm_get_format_info(dev, mode_cmd);
>  	if (!info) {
> @@ -187,9 +189,10 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev,
>  	return 0;
>  
>  err_gem_object_put:
> -	for (i--; i >= 0; i--)
> +	while (i > 0) {
> +		--i;
>  		drm_gem_object_put(objs[i]);
> -
> +	}
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_fb_init_with_funcs);
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index 3b138d4ae67e..22aa64d07c79 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -25,6 +25,11 @@
>  #include <linux/types.h>
>  #include <uapi/drm/drm_fourcc.h>
>  
> +/**
> + * DRM_FORMAT_MAX_PLANES - maximum number of planes a DRM format can have
> + */
> +#define DRM_FORMAT_MAX_PLANES	4u
> +
>  /*
>   * DRM formats are little endian.  Define host endian variants for the
>   * most common formats here, to reduce the #ifdefs needed in drivers.
> @@ -78,7 +83,7 @@ struct drm_format_info {
>  		 * triplet @char_per_block, @block_w, @block_h for better
>  		 * describing the pixel format.
>  		 */
> -		u8 cpp[4];
> +		u8 cpp[DRM_FORMAT_MAX_PLANES];
>  
>  		/**
>  		 * @char_per_block:
> @@ -104,7 +109,7 @@ struct drm_format_info {
>  		 * information from their drm_mode_config.get_format_info hook
>  		 * if they want the core to be validating the pitch.
>  		 */
> -		u8 char_per_block[4];
> +		u8 char_per_block[DRM_FORMAT_MAX_PLANES];
>  	};
>  
>  	/**
> @@ -113,7 +118,7 @@ struct drm_format_info {
>  	 * Block width in pixels, this is intended to be accessed through
>  	 * drm_format_info_block_width()
>  	 */
> -	u8 block_w[4];
> +	u8 block_w[DRM_FORMAT_MAX_PLANES];
>  
>  	/**
>  	 * @block_h:
> @@ -121,7 +126,7 @@ struct drm_format_info {
>  	 * Block height in pixels, this is intended to be accessed through
>  	 * drm_format_info_block_height()
>  	 */
> -	u8 block_h[4];
> +	u8 block_h[DRM_FORMAT_MAX_PLANES];
>  
>  	/** @hsub: Horizontal chroma subsampling factor */
>  	u8 hsub;
> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> index be658ebbec72..f67c5b7bcb68 100644
> --- a/include/drm/drm_framebuffer.h
> +++ b/include/drm/drm_framebuffer.h
> @@ -27,12 +27,12 @@
>  #include <linux/list.h>
>  #include <linux/sched.h>
>  
> +#include <drm/drm_fourcc.h>
>  #include <drm/drm_mode_object.h>
>  
>  struct drm_clip_rect;
>  struct drm_device;
>  struct drm_file;
> -struct drm_format_info;
>  struct drm_framebuffer;
>  struct drm_gem_object;
>  
> @@ -147,7 +147,7 @@ struct drm_framebuffer {
>  	 * @pitches: Line stride per buffer. For userspace created object this
>  	 * is copied from drm_mode_fb_cmd2.
>  	 */
> -	unsigned int pitches[4];
> +	unsigned int pitches[DRM_FORMAT_MAX_PLANES];
>  	/**
>  	 * @offsets: Offset from buffer start to the actual pixel data in bytes,
>  	 * per buffer. For userspace created object this is copied from
> @@ -165,7 +165,7 @@ struct drm_framebuffer {
>  	 * data (even for linear buffers). Specifying an x/y pixel offset is
>  	 * instead done through the source rectangle in &struct drm_plane_state.
>  	 */
> -	unsigned int offsets[4];
> +	unsigned int offsets[DRM_FORMAT_MAX_PLANES];
>  	/**
>  	 * @modifier: Data layout modifier. This is used to describe
>  	 * tiling, or also special layouts (like compression) of auxiliary
> @@ -210,7 +210,7 @@ struct drm_framebuffer {
>  	 * This is used by the GEM framebuffer helpers, see e.g.
>  	 * drm_gem_fb_create().
>  	 */
> -	struct drm_gem_object *obj[4];
> +	struct drm_gem_object *obj[DRM_FORMAT_MAX_PLANES];
>  };
>  
>  #define obj_to_fb(x) container_of(x, struct drm_framebuffer, base)
> diff --git a/include/drm/drm_gem_atomic_helper.h b/include/drm/drm_gem_atomic_helper.h
> index d82c23622156..f9f8b6f0494a 100644
> --- a/include/drm/drm_gem_atomic_helper.h
> +++ b/include/drm/drm_gem_atomic_helper.h
> @@ -5,6 +5,7 @@
>  
>  #include <linux/dma-buf-map.h>
>  
> +#include <drm/drm_fourcc.h>
>  #include <drm/drm_plane.h>
>  
>  struct drm_simple_display_pipe;
> @@ -40,7 +41,7 @@ struct drm_shadow_plane_state {
>  	 * The memory mappings stored in map should be established in the plane's
>  	 * prepare_fb callback and removed in the cleanup_fb callback.
>  	 */
> -	struct dma_buf_map map[4];
> +	struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
>  };
>  
>  /**
> -- 
> 2.32.0

Re: [PATCH v2 2/5] drm/gem: Provide drm_gem_fb_{vmap,vunmap}()

[Sam Ravnborg]

On Sun, Jul 25, 2021 at 07:44:35PM +0200, Thomas Zimmermann wrote:
> Move framebuffer vmap code from shadow-buffered plane state into the new
> interfaces drm_gem_fb_vmap() and drm_gem_fb_vunmap(). These functions
> provide mappings of a framebuffer's BOs into kernel address space. No
> functional changes.
> 
> v2:
> 	* using [static N] for array parameters enables compile-time checks
> 	* include <drm/drm_fourcc.h> for DRM_FORMAT_MAX_PLANES (kernel
> 	  test robot)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Re: [PATCH v2 3/5] drm/gem: Clear mapping addresses for unused framebuffer planes

[Sam Ravnborg]

On Sun, Jul 25, 2021 at 07:44:36PM +0200, Thomas Zimmermann wrote:
> Set the returned mapping address to NULL if a framebuffer plane does
> not have a BO associated with it. Likewise, ignore mappings of NULL
> during framebuffer unmap operations. Allows users of the functions to
> perform unmap operations of certain BOs by themselfes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> ---
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index 243affbad437..02928607a716 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -336,8 +336,10 @@ int drm_gem_fb_vmap(struct drm_framebuffer *fb,
>  
>  	for (i = 0; i < DRM_FORMAT_MAX_PLANES; ++i) {
>  		obj = drm_gem_fb_get_obj(fb, i);
> -		if (!obj)
> +		if (!obj) {
> +			dma_buf_map_clear(&map[i]);
>  			continue;
> +		}
>  		ret = drm_gem_vmap(obj, &map[i]);
>  		if (ret)
>  			goto err_drm_gem_vunmap;
> @@ -377,6 +379,8 @@ void drm_gem_fb_vunmap(struct drm_framebuffer *fb,
>  		obj = drm_gem_fb_get_obj(fb, i);
>  		if (!obj)
>  			continue;
> +		if (dma_buf_map_is_null(&map[i]))
> +			continue;
>  		drm_gem_vunmap(obj, &map[i]);
>  	}
>  }
> -- 
> 2.32.0

Re: [PATCH v2 5/5] drm/vkms: Map output framebuffer BOs with drm_gem_fb_vmap()

[Sam Ravnborg]

On Sun, Jul 25, 2021 at 07:44:38PM +0200, Thomas Zimmermann wrote:
> Abstract the framebuffer details by mappings its BOs with a call
> to drm_gem_fb_vmap(). Unmap with drm_gem_fb_vunamp().
> 
> Before, the output address with stored as raw pointer in the priv
> field of struct drm_writeback_job. Introduce the new type
> struct vkms_writeback_job, which holds the output mappings addresses
> while the writeback job is active.
> 
> The patchset also cleans up some internal casting an setup of the
> output addresses. No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c  |  2 +-
>  drivers/gpu/drm/vkms/vkms_drv.h       |  6 +++++-
>  drivers/gpu/drm/vkms/vkms_writeback.c | 21 ++++++++++-----------
>  3 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index ead8fff81f30..49f109c3a2b3 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -257,7 +257,7 @@ void vkms_composer_worker(struct work_struct *work)
>  		return;
>  
>  	if (wb_pending)
> -		vaddr_out = crtc_state->active_writeback;
> +		vaddr_out = crtc_state->active_writeback->map[0].vaddr;
>  
>  	ret = compose_active_planes(&vaddr_out, primary_composer,
>  				    crtc_state);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 8c731b6dcba7..8bc9e3f52e1f 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -20,6 +20,10 @@
>  #define XRES_MAX  8192
>  #define YRES_MAX  8192
>  
> +struct vkms_writeback_job {
> +	struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
> +};
> +
>  struct vkms_composer {
>  	struct drm_framebuffer fb;
>  	struct drm_rect src, dst;
> @@ -57,7 +61,7 @@ struct vkms_crtc_state {
>  	int num_active_planes;
>  	/* stack of active planes for crc computation, should be in z order */
>  	struct vkms_plane_state **active_planes;
> -	void *active_writeback;
> +	struct vkms_writeback_job *active_writeback;
>  
>  	/* below four are protected by vkms_output.composer_lock */
>  	bool crc_pending;
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index 0935686475a0..765bb85ba76c 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -65,21 +65,23 @@ static int vkms_wb_connector_get_modes(struct drm_connector *connector)
>  static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
>  			       struct drm_writeback_job *job)
>  {
> -	struct drm_gem_object *gem_obj;
> -	struct dma_buf_map map;
> +	struct vkms_writeback_job *vkmsjob;
>  	int ret;
>  
>  	if (!job->fb)
>  		return 0;
>  
> -	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> -	ret = drm_gem_shmem_vmap(gem_obj, &map);
> +	vkmsjob = kzalloc(sizeof(*vkmsjob), GFP_KERNEL);
> +	if (!vkmsjob)
> +		return -ENOMEM;
> +
> +	ret = drm_gem_fb_vmap(job->fb, vkmsjob->map);
>  	if (ret) {
>  		DRM_ERROR("vmap failed: %d\n", ret);
>  		return ret;
>  	}
>  
> -	job->priv = map.vaddr;
> +	job->priv = vkmsjob;
>  
>  	return 0;
>  }
> @@ -87,18 +89,15 @@ static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
>  static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
>  				struct drm_writeback_job *job)
>  {
> -	struct drm_gem_object *gem_obj;
> +	struct vkms_writeback_job *vkmsjob = job->priv;
>  	struct vkms_device *vkmsdev;
> -	struct dma_buf_map map;
>  
>  	if (!job->fb)
>  		return;
>  
> -	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> -	dma_buf_map_set_vaddr(&map, job->priv);
> -	drm_gem_shmem_vunmap(gem_obj, &map);
> +	drm_gem_fb_vunmap(job->fb, vkmsjob->map);
>  
> -	vkmsdev = drm_device_to_vkms_device(gem_obj->dev);
> +	vkmsdev = drm_device_to_vkms_device(job->fb->dev);
>  	vkms_set_composer(&vkmsdev->output, false);
>  }
>  
> -- 
> 2.32.0

Re: [PATCH v2 4/5] drm/gud: Map framebuffer BOs with drm_gem_fb_vmap()

[Sam Ravnborg]

On Sun, Jul 25, 2021 at 07:44:37PM +0200, Thomas Zimmermann wrote:
> Abstract the framebuffer details by mapping its BOs with a call
> to drm_gem_fb_vmap(). Unmap with drm_gem_fb_vunmap().
> 
> The call to drm_gem_fb_vmap() ensures that all BOs are mapped
> correctly. Gud still only supports single-plane formats.
> 
> No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Acked-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
> ---
>  drivers/gpu/drm/gud/gud_pipe.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> index 4d7a26b68a2e..7e009f562b30 100644
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -14,8 +14,8 @@
>  #include <drm/drm_format_helper.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> -#include <drm/drm_gem_shmem_helper.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_rect.h>
>  #include <drm/drm_simple_kms_helper.h>
> @@ -152,7 +152,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
>  {
>  	struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
>  	u8 compression = gdrm->compression;
> -	struct dma_buf_map map;
> +	struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
>  	void *vaddr, *buf;
>  	size_t pitch, len;
>  	int ret = 0;
> @@ -162,11 +162,11 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
>  	if (len > gdrm->bulk_len)
>  		return -E2BIG;
>  
> -	ret = drm_gem_shmem_vmap(fb->obj[0], &map);
> +	ret = drm_gem_fb_vmap(fb, map);
>  	if (ret)
>  		return ret;
>  
> -	vaddr = map.vaddr + fb->offsets[0];
> +	vaddr = map[0].vaddr + fb->offsets[0];
>  
>  	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>  	if (ret)
> @@ -225,7 +225,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
>  end_cpu_access:
>  	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>  vunmap:
> -	drm_gem_shmem_vunmap(fb->obj[0], &map);
> +	drm_gem_fb_vunmap(fb, map);
>  
>  	return ret;
>  }
> -- 
> 2.32.0

Re: [PATCH v2 1/5] drm: Define DRM_FORMAT_MAX_PLANES

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 7898 bytes --]

Hi

Am 25.07.21 um 21:49 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Sun, Jul 25, 2021 at 07:44:34PM +0200, Thomas Zimmermann wrote:
>> DRM uses a magic number of 4 for the maximum number of planes per color
>> format. Declare this constant via DRM_FORMAT_MAX_PLANES and update the
>> related code. Some code depends on the length of arrays that are now
>> declared with DRM_FORMAT_MAX_PLANES. Convert it from '4' to ARRAY_SIZE.
>>
>> v2:
>> 	* mention usage of ARRAY_SIZE() in the commit message (Maxime)
>> 	* also fix error handling in drm_gem_fb_init_with_funcs()
>> 	  (kernel test robot)
>> 	* include <drm/drm_fourcc.h> for DRM_FORMAT_MAX_PLANES
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> One nit below.
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Thanks a lot for your reviews.

> 
>> ---
>>   drivers/gpu/drm/drm_gem_framebuffer_helper.c | 19 +++++++++++--------
>>   include/drm/drm_fourcc.h                     | 13 +++++++++----
>>   include/drm/drm_framebuffer.h                |  8 ++++----
>>   include/drm/drm_gem_atomic_helper.h          |  3 ++-
>>   4 files changed, 26 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> index 67bc9edc1d98..421e029a6b3e 100644
>> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> @@ -48,7 +48,7 @@
>>   struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
>>   					  unsigned int plane)
>>   {
>> -	if (plane >= 4)
>> +	if (plane >= ARRAY_SIZE(fb->obj))
>>   		return NULL;
>>   
>>   	return fb->obj[plane];
>> @@ -62,7 +62,8 @@ drm_gem_fb_init(struct drm_device *dev,
>>   		 struct drm_gem_object **obj, unsigned int num_planes,
>>   		 const struct drm_framebuffer_funcs *funcs)
>>   {
>> -	int ret, i;
>> +	unsigned int i;
>> +	int ret;
>>   
>>   	drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
>>
> This change looks accidental/unrelated.
> But I guess it is to be consistent in use of unsigned int when
> iterating the array.

Indeed. The current code uses a mixture of signed and unsigned types in 
several places. DRM_FORMAT_MAX_PLANES is defined as unsigned, so I 
updated all the related code accordingly.

Best regards
Thomas

> 
>> @@ -86,9 +87,9 @@ drm_gem_fb_init(struct drm_device *dev,
>>    */
>>   void drm_gem_fb_destroy(struct drm_framebuffer *fb)
>>   {
>> -	int i;
>> +	size_t i;
>>   
>> -	for (i = 0; i < 4; i++)
>> +	for (i = 0; i < ARRAY_SIZE(fb->obj); i++)
>>   		drm_gem_object_put(fb->obj[i]);
>>   
>>   	drm_framebuffer_cleanup(fb);
>> @@ -145,8 +146,9 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev,
>>   			       const struct drm_framebuffer_funcs *funcs)
>>   {
>>   	const struct drm_format_info *info;
>> -	struct drm_gem_object *objs[4];
>> -	int ret, i;
>> +	struct drm_gem_object *objs[DRM_FORMAT_MAX_PLANES];
>> +	unsigned int i;
>> +	int ret;
>>   
>>   	info = drm_get_format_info(dev, mode_cmd);
>>   	if (!info) {
>> @@ -187,9 +189,10 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev,
>>   	return 0;
>>   
>>   err_gem_object_put:
>> -	for (i--; i >= 0; i--)
>> +	while (i > 0) {
>> +		--i;
>>   		drm_gem_object_put(objs[i]);
>> -
>> +	}
>>   	return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(drm_gem_fb_init_with_funcs);
>> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
>> index 3b138d4ae67e..22aa64d07c79 100644
>> --- a/include/drm/drm_fourcc.h
>> +++ b/include/drm/drm_fourcc.h
>> @@ -25,6 +25,11 @@
>>   #include <linux/types.h>
>>   #include <uapi/drm/drm_fourcc.h>
>>   
>> +/**
>> + * DRM_FORMAT_MAX_PLANES - maximum number of planes a DRM format can have
>> + */
>> +#define DRM_FORMAT_MAX_PLANES	4u
>> +
>>   /*
>>    * DRM formats are little endian.  Define host endian variants for the
>>    * most common formats here, to reduce the #ifdefs needed in drivers.
>> @@ -78,7 +83,7 @@ struct drm_format_info {
>>   		 * triplet @char_per_block, @block_w, @block_h for better
>>   		 * describing the pixel format.
>>   		 */
>> -		u8 cpp[4];
>> +		u8 cpp[DRM_FORMAT_MAX_PLANES];
>>   
>>   		/**
>>   		 * @char_per_block:
>> @@ -104,7 +109,7 @@ struct drm_format_info {
>>   		 * information from their drm_mode_config.get_format_info hook
>>   		 * if they want the core to be validating the pitch.
>>   		 */
>> -		u8 char_per_block[4];
>> +		u8 char_per_block[DRM_FORMAT_MAX_PLANES];
>>   	};
>>   
>>   	/**
>> @@ -113,7 +118,7 @@ struct drm_format_info {
>>   	 * Block width in pixels, this is intended to be accessed through
>>   	 * drm_format_info_block_width()
>>   	 */
>> -	u8 block_w[4];
>> +	u8 block_w[DRM_FORMAT_MAX_PLANES];
>>   
>>   	/**
>>   	 * @block_h:
>> @@ -121,7 +126,7 @@ struct drm_format_info {
>>   	 * Block height in pixels, this is intended to be accessed through
>>   	 * drm_format_info_block_height()
>>   	 */
>> -	u8 block_h[4];
>> +	u8 block_h[DRM_FORMAT_MAX_PLANES];
>>   
>>   	/** @hsub: Horizontal chroma subsampling factor */
>>   	u8 hsub;
>> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
>> index be658ebbec72..f67c5b7bcb68 100644
>> --- a/include/drm/drm_framebuffer.h
>> +++ b/include/drm/drm_framebuffer.h
>> @@ -27,12 +27,12 @@
>>   #include <linux/list.h>
>>   #include <linux/sched.h>
>>   
>> +#include <drm/drm_fourcc.h>
>>   #include <drm/drm_mode_object.h>
>>   
>>   struct drm_clip_rect;
>>   struct drm_device;
>>   struct drm_file;
>> -struct drm_format_info;
>>   struct drm_framebuffer;
>>   struct drm_gem_object;
>>   
>> @@ -147,7 +147,7 @@ struct drm_framebuffer {
>>   	 * @pitches: Line stride per buffer. For userspace created object this
>>   	 * is copied from drm_mode_fb_cmd2.
>>   	 */
>> -	unsigned int pitches[4];
>> +	unsigned int pitches[DRM_FORMAT_MAX_PLANES];
>>   	/**
>>   	 * @offsets: Offset from buffer start to the actual pixel data in bytes,
>>   	 * per buffer. For userspace created object this is copied from
>> @@ -165,7 +165,7 @@ struct drm_framebuffer {
>>   	 * data (even for linear buffers). Specifying an x/y pixel offset is
>>   	 * instead done through the source rectangle in &struct drm_plane_state.
>>   	 */
>> -	unsigned int offsets[4];
>> +	unsigned int offsets[DRM_FORMAT_MAX_PLANES];
>>   	/**
>>   	 * @modifier: Data layout modifier. This is used to describe
>>   	 * tiling, or also special layouts (like compression) of auxiliary
>> @@ -210,7 +210,7 @@ struct drm_framebuffer {
>>   	 * This is used by the GEM framebuffer helpers, see e.g.
>>   	 * drm_gem_fb_create().
>>   	 */
>> -	struct drm_gem_object *obj[4];
>> +	struct drm_gem_object *obj[DRM_FORMAT_MAX_PLANES];
>>   };
>>   
>>   #define obj_to_fb(x) container_of(x, struct drm_framebuffer, base)
>> diff --git a/include/drm/drm_gem_atomic_helper.h b/include/drm/drm_gem_atomic_helper.h
>> index d82c23622156..f9f8b6f0494a 100644
>> --- a/include/drm/drm_gem_atomic_helper.h
>> +++ b/include/drm/drm_gem_atomic_helper.h
>> @@ -5,6 +5,7 @@
>>   
>>   #include <linux/dma-buf-map.h>
>>   
>> +#include <drm/drm_fourcc.h>
>>   #include <drm/drm_plane.h>
>>   
>>   struct drm_simple_display_pipe;
>> @@ -40,7 +41,7 @@ struct drm_shadow_plane_state {
>>   	 * The memory mappings stored in map should be established in the plane's
>>   	 * prepare_fb callback and removed in the cleanup_fb callback.
>>   	 */
>> -	struct dma_buf_map map[4];
>> +	struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
>>   };
>>   
>>   /**
>> -- 
>> 2.32.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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 5/5] drm/vkms: Map output framebuffer BOs with drm_gem_fb_vmap()

[Rodrigo Siqueira]

On 07/25, Thomas Zimmermann wrote:
> Abstract the framebuffer details by mappings its BOs with a call
> to drm_gem_fb_vmap(). Unmap with drm_gem_fb_vunamp().
> 
> Before, the output address with stored as raw pointer in the priv
> field of struct drm_writeback_job. Introduce the new type
> struct vkms_writeback_job, which holds the output mappings addresses
> while the writeback job is active.
> 
> The patchset also cleans up some internal casting an setup of the
> output addresses. No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c  |  2 +-
>  drivers/gpu/drm/vkms/vkms_drv.h       |  6 +++++-
>  drivers/gpu/drm/vkms/vkms_writeback.c | 21 ++++++++++-----------
>  3 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index ead8fff81f30..49f109c3a2b3 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -257,7 +257,7 @@ void vkms_composer_worker(struct work_struct *work)
>  		return;
>  
>  	if (wb_pending)
> -		vaddr_out = crtc_state->active_writeback;
> +		vaddr_out = crtc_state->active_writeback->map[0].vaddr;
>  
>  	ret = compose_active_planes(&vaddr_out, primary_composer,
>  				    crtc_state);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 8c731b6dcba7..8bc9e3f52e1f 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -20,6 +20,10 @@
>  #define XRES_MAX  8192
>  #define YRES_MAX  8192
>  
> +struct vkms_writeback_job {
> +	struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
> +};
> +
>  struct vkms_composer {
>  	struct drm_framebuffer fb;
>  	struct drm_rect src, dst;
> @@ -57,7 +61,7 @@ struct vkms_crtc_state {
>  	int num_active_planes;
>  	/* stack of active planes for crc computation, should be in z order */
>  	struct vkms_plane_state **active_planes;
> -	void *active_writeback;
> +	struct vkms_writeback_job *active_writeback;
>  
>  	/* below four are protected by vkms_output.composer_lock */
>  	bool crc_pending;
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index 0935686475a0..765bb85ba76c 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -65,21 +65,23 @@ static int vkms_wb_connector_get_modes(struct drm_connector *connector)
>  static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
>  			       struct drm_writeback_job *job)
>  {
> -	struct drm_gem_object *gem_obj;
> -	struct dma_buf_map map;
> +	struct vkms_writeback_job *vkmsjob;
>  	int ret;
>  
>  	if (!job->fb)
>  		return 0;
>  
> -	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> -	ret = drm_gem_shmem_vmap(gem_obj, &map);
> +	vkmsjob = kzalloc(sizeof(*vkmsjob), GFP_KERNEL);
> +	if (!vkmsjob)
> +		return -ENOMEM;
> +
> +	ret = drm_gem_fb_vmap(job->fb, vkmsjob->map);
>  	if (ret) {
>  		DRM_ERROR("vmap failed: %d\n", ret);
>  		return ret;
>  	}
>  
> -	job->priv = map.vaddr;
> +	job->priv = vkmsjob;
>  
>  	return 0;
>  }
> @@ -87,18 +89,15 @@ static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
>  static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
>  				struct drm_writeback_job *job)
>  {
> -	struct drm_gem_object *gem_obj;
> +	struct vkms_writeback_job *vkmsjob = job->priv;
>  	struct vkms_device *vkmsdev;
> -	struct dma_buf_map map;
>  
>  	if (!job->fb)
>  		return;
>  
> -	gem_obj = drm_gem_fb_get_obj(job->fb, 0);
> -	dma_buf_map_set_vaddr(&map, job->priv);
> -	drm_gem_shmem_vunmap(gem_obj, &map);
> +	drm_gem_fb_vunmap(job->fb, vkmsjob->map);
>  
> -	vkmsdev = drm_device_to_vkms_device(gem_obj->dev);
> +	vkmsdev = drm_device_to_vkms_device(job->fb->dev);
>  	vkms_set_composer(&vkmsdev->output, false);
>  }
>  
> -- 
> 2.32.0
>

Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> 

-- 
Rodrigo Siqueira
https://siqueira.tech