[PATCH 0/8] drm/mipi-dbi: Convert to shadow-plane helpers

[PATCH 0/8] drm/mipi-dbi: Convert to shadow-plane helpers

[Thomas Zimmermann]

Convert the MIPI-DBI-based drivers to shadow-plane helpers. The
drivers vmap/vunmap GEM buffer memory during the atomic commit.
Shadow-plane helpers automate this process.

Patches 1 to 4 prepare the MIPI code for the change and simplify
the restof the patchset.

Patches 5 to 7 rework the vmap code in the MIPI-DBI drivers and add
shadow-plane helpers. Most of the affected drivers call MIPI-DBI
helpers and get the update automatically. Only ili9225 and st7586
require changes to their source code.

Patch 8 simplifies drm_dev_enter() and _exit(). It's not strictly
needed, but streamlines the driver code and make sense overall.

Testing is welcome, as I don't have any hardware to test these
changes myself.

Thomas Zimmermann (8):
  drm/simple-kms: Remove drm_gem_simple_display_pipe_prepare_fb()
  drm/ili9225: Call MIPI DBI mode_valid helper
  drm/st7586: Call MIPI DBI mode_valid helper
  drm/mipi-dbi: Initialize default driver functions with macro
  drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update
    helpers
  drm/mipi-dbi: Support shadow-plane state
  drm/mipi-dbi: Use shadow-plane mappings
  drm/mipi-dbi: Move drm_dev_{enter,exit}() out from fb_dirty functions

 drivers/gpu/drm/drm_gem_atomic_helper.c      |  31 +---
 drivers/gpu/drm/drm_mipi_dbi.c               | 175 +++++++++++++++----
 drivers/gpu/drm/drm_simple_kms_helper.c      |   2 +-
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c |   6 +-
 drivers/gpu/drm/tiny/hx8357d.c               |   5 +-
 drivers/gpu/drm/tiny/ili9163.c               |   6 +-
 drivers/gpu/drm/tiny/ili9225.c               |  42 +++--
 drivers/gpu/drm/tiny/ili9341.c               |   5 +-
 drivers/gpu/drm/tiny/ili9486.c               |   5 +-
 drivers/gpu/drm/tiny/mi0283qt.c              |   5 +-
 drivers/gpu/drm/tiny/panel-mipi-dbi.c        |   5 +-
 drivers/gpu/drm/tiny/st7586.c                |  45 +++--
 drivers/gpu/drm/tiny/st7735r.c               |   5 +-
 include/drm/drm_gem_atomic_helper.h          |   2 -
 include/drm/drm_mipi_dbi.h                   |  50 +++++-
 include/drm/drm_plane.h                      |   4 +-
 include/drm/drm_simple_kms_helper.h          |   4 +-
 17 files changed, 265 insertions(+), 132 deletions(-)


base-commit: b7598e2b3a3116bb5ddbf756db30a0e5dc0877ea
prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
prerequisite-patch-id: 3f204510fcbf9530d6540bd8e6128cce598988b6
-- 
2.38.1

[PATCH 1/8] drm/simple-kms: Remove drm_gem_simple_display_pipe_prepare_fb()

[Thomas Zimmermann]

The helper drm_gem_simple_display_pipe_prepare_fb() is simple-KMS'
default implementation for prepare_fb. Remove the call from drivers
that set it explicitly. Then inline the helper into the only caller
within simple-kms helpers and remove . No functional changes.

Simple-KMS drivers that implement the prepare_fb callback should call
drm_gem_plane_helper_prepare_fb() directly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_atomic_helper.c      | 31 +-------------------
 drivers/gpu/drm/drm_simple_kms_helper.c      |  2 +-
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c |  1 -
 drivers/gpu/drm/tiny/ili9163.c               |  1 -
 include/drm/drm_gem_atomic_helper.h          |  2 --
 include/drm/drm_plane.h                      |  4 +--
 include/drm/drm_simple_kms_helper.h          |  4 +--
 7 files changed, 6 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
index e42800718f515..5d4b9cd077f7a 100644
--- a/drivers/gpu/drm/drm_gem_atomic_helper.c
+++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
@@ -26,11 +26,8 @@
  * call drm_gem_plane_helper_prepare_fb() from their implementation of
  * struct &drm_plane_helper.prepare_fb . It sets the plane's fence from
  * the framebuffer so that the DRM core can synchronize access automatically.
- *
  * drm_gem_plane_helper_prepare_fb() can also be used directly as
- * implementation of prepare_fb. For drivers based on
- * struct drm_simple_display_pipe, drm_gem_simple_display_pipe_prepare_fb()
- * provides equivalent functionality.
+ * implementation of prepare_fb.
  *
  * .. code-block:: c
  *
@@ -41,11 +38,6 @@
  *		. prepare_fb = drm_gem_plane_helper_prepare_fb,
  *	};
  *
- *	struct drm_simple_display_pipe_funcs driver_pipe_funcs = {
- *		...,
- *		. prepare_fb = drm_gem_simple_display_pipe_prepare_fb,
- *	};
- *
  * A driver using a shadow buffer copies the content of the shadow buffers
  * into the HW's framebuffer memory during an atomic update. This requires
  * a mapping of the shadow buffer into kernel address space. The mappings
@@ -205,27 +197,6 @@ int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane,
 }
 EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb);
 
-/**
- * drm_gem_simple_display_pipe_prepare_fb - prepare_fb helper for &drm_simple_display_pipe
- * @pipe: Simple display pipe
- * @plane_state: Plane state
- *
- * This function uses drm_gem_plane_helper_prepare_fb() to extract the fences
- * from &drm_gem_object.resv and attaches them to the plane state for the atomic
- * helper to wait on. This is necessary to correctly implement implicit
- * synchronization for any buffers shared as a struct &dma_buf. Drivers can use
- * this as their &drm_simple_display_pipe_funcs.prepare_fb callback.
- *
- * See drm_gem_plane_helper_prepare_fb() for a discussion of implicit and
- * explicit fencing in atomic modeset updates.
- */
-int drm_gem_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
-					   struct drm_plane_state *plane_state)
-{
-	return drm_gem_plane_helper_prepare_fb(&pipe->plane, plane_state);
-}
-EXPORT_SYMBOL(drm_gem_simple_display_pipe_prepare_fb);
-
 /*
  * Shadow-buffered Planes
  */
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index 3ef420ec4534a..270523ae36d43 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -267,7 +267,7 @@ static int drm_simple_kms_plane_prepare_fb(struct drm_plane *plane,
 
 		WARN_ON_ONCE(pipe->funcs && pipe->funcs->cleanup_fb);
 
-		return drm_gem_simple_display_pipe_prepare_fb(pipe, state);
+		return drm_gem_plane_helper_prepare_fb(plane, state);
 	}
 
 	return pipe->funcs->prepare_fb(pipe, state);
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
index 384a724f2822e..be088983aa7c7 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -581,7 +581,6 @@ static const struct drm_simple_display_pipe_funcs ili9341_dbi_funcs = {
 	.enable = ili9341_dbi_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
-	.prepare_fb = drm_gem_simple_display_pipe_prepare_fb,
 };
 
 static const struct drm_display_mode ili9341_dbi_mode = {
diff --git a/drivers/gpu/drm/tiny/ili9163.c b/drivers/gpu/drm/tiny/ili9163.c
index ca0451f799625..835ed12792d56 100644
--- a/drivers/gpu/drm/tiny/ili9163.c
+++ b/drivers/gpu/drm/tiny/ili9163.c
@@ -104,7 +104,6 @@ static const struct drm_simple_display_pipe_funcs ili9163_pipe_funcs = {
 	.enable = yx240qv29_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
-	.prepare_fb = drm_gem_simple_display_pipe_prepare_fb,
 };
 
 static const struct drm_display_mode yx240qv29_mode = {
diff --git a/include/drm/drm_gem_atomic_helper.h b/include/drm/drm_gem_atomic_helper.h
index 6970ccb787e23..40b8b039518e0 100644
--- a/include/drm/drm_gem_atomic_helper.h
+++ b/include/drm/drm_gem_atomic_helper.h
@@ -15,8 +15,6 @@ struct drm_simple_display_pipe;
  */
 
 int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state);
-int drm_gem_simple_display_pipe_prepare_fb(struct drm_simple_display_pipe *pipe,
-					   struct drm_plane_state *plane_state);
 
 /*
  * Helpers for planes with shadow buffers
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 447e664e49d50..51291983ea445 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -77,8 +77,8 @@ struct drm_plane_state {
 	 * write this field directly for a driver's implicit fence.
 	 *
 	 * Drivers should store any implicit fence in this from their
-	 * &drm_plane_helper_funcs.prepare_fb callback. See drm_gem_plane_helper_prepare_fb()
-	 * and drm_gem_simple_display_pipe_prepare_fb() for suitable helpers.
+	 * &drm_plane_helper_funcs.prepare_fb callback. See
+	 * drm_gem_plane_helper_prepare_fb() for a suitable helper.
 	 */
 	struct dma_fence *fence;
 
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
index 2298fe3af4cd7..b2486d0737633 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -117,9 +117,9 @@ struct drm_simple_display_pipe_funcs {
 	 * more details.
 	 *
 	 * For GEM drivers who neither have a @prepare_fb nor @cleanup_fb hook
-	 * set drm_gem_simple_display_pipe_prepare_fb() is called automatically
+	 * set, drm_gem_plane_helper_prepare_fb() is called automatically
 	 * to implement this. Other drivers which need additional plane
-	 * processing can call drm_gem_simple_display_pipe_prepare_fb() from
+	 * processing can call drm_gem_plane_helper_prepare_fb() from
 	 * their @prepare_fb hook.
 	 */
 	int (*prepare_fb)(struct drm_simple_display_pipe *pipe,
-- 
2.38.1

[PATCH 2/8] drm/ili9225: Call MIPI DBI mode_valid helper

[Thomas Zimmermann]

MIPI DBI drivers validate each mode against their native resolution.
Add this test to ili9225.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/ili9225.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c
index 815bab2858231..f05a2d25866c1 100644
--- a/drivers/gpu/drm/tiny/ili9225.c
+++ b/drivers/gpu/drm/tiny/ili9225.c
@@ -326,6 +326,7 @@ static int ili9225_dbi_command(struct mipi_dbi *dbi, u8 *cmd, u8 *par,
 }
 
 static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = {
+	.mode_valid	= mipi_dbi_pipe_mode_valid,
 	.enable		= ili9225_pipe_enable,
 	.disable	= ili9225_pipe_disable,
 	.update		= ili9225_pipe_update,
-- 
2.38.1

[PATCH 3/8] drm/st7586: Call MIPI DBI mode_valid helper

[Thomas Zimmermann]

MIPI DBI drivers validate each mode against their native resolution.
Add this test to st7586.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/st7586.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c
index ce57fa9917e51..6bdd23e2a47c7 100644
--- a/drivers/gpu/drm/tiny/st7586.c
+++ b/drivers/gpu/drm/tiny/st7586.c
@@ -263,6 +263,7 @@ static const u32 st7586_formats[] = {
 };
 
 static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
+	.mode_valid	= mipi_dbi_pipe_mode_valid,
 	.enable		= st7586_pipe_enable,
 	.disable	= st7586_pipe_disable,
 	.update		= st7586_pipe_update,
-- 
2.38.1

[PATCH 4/8] drm/mipi-dbi: Initialize default driver functions with macro

[Thomas Zimmermann]

Introduce DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS to initialize MIPI-DBI
helpers to default values and convert drivers. The prepare_fb function
set by some drivers is called implicitly by simple-kms helpers, so leave
it out.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c |  5 +----
 drivers/gpu/drm/tiny/hx8357d.c               |  5 +----
 drivers/gpu/drm/tiny/ili9163.c               |  5 +----
 drivers/gpu/drm/tiny/ili9341.c               |  5 +----
 drivers/gpu/drm/tiny/ili9486.c               |  5 +----
 drivers/gpu/drm/tiny/mi0283qt.c              |  5 +----
 drivers/gpu/drm/tiny/panel-mipi-dbi.c        |  5 +----
 drivers/gpu/drm/tiny/st7735r.c               |  5 +----
 include/drm/drm_mipi_dbi.h                   | 16 ++++++++++++++++
 9 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
index be088983aa7c7..3fdf884b3257f 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -577,10 +577,7 @@ static void ili9341_dbi_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs ili9341_dbi_funcs = {
-	.mode_valid = mipi_dbi_pipe_mode_valid,
-	.enable = ili9341_dbi_enable,
-	.disable = mipi_dbi_pipe_disable,
-	.update = mipi_dbi_pipe_update,
+	DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(ili9341_dbi_enable),
 };
 
 static const struct drm_display_mode ili9341_dbi_mode = {
diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c
index 9f634f720817b..cdc4486e059b5 100644
--- a/drivers/gpu/drm/tiny/hx8357d.c
+++ b/drivers/gpu/drm/tiny/hx8357d.c
@@ -181,10 +181,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = {
-	.mode_valid = mipi_dbi_pipe_mode_valid,
-	.enable = yx240qv29_enable,
-	.disable = mipi_dbi_pipe_disable,
-	.update = mipi_dbi_pipe_update,
+	DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(yx240qv29_enable),
 };
 
 static const struct drm_display_mode yx350hv15_mode = {
diff --git a/drivers/gpu/drm/tiny/ili9163.c b/drivers/gpu/drm/tiny/ili9163.c
index 835ed12792d56..bc4384d410fcc 100644
--- a/drivers/gpu/drm/tiny/ili9163.c
+++ b/drivers/gpu/drm/tiny/ili9163.c
@@ -100,10 +100,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs ili9163_pipe_funcs = {
-	.mode_valid = mipi_dbi_pipe_mode_valid,
-	.enable = yx240qv29_enable,
-	.disable = mipi_dbi_pipe_disable,
-	.update = mipi_dbi_pipe_update,
+	DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(yx240qv29_enable),
 };
 
 static const struct drm_display_mode yx240qv29_mode = {
diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c
index 420f6005a9568..47b61c3bf1457 100644
--- a/drivers/gpu/drm/tiny/ili9341.c
+++ b/drivers/gpu/drm/tiny/ili9341.c
@@ -137,10 +137,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
-	.mode_valid = mipi_dbi_pipe_mode_valid,
-	.enable = yx240qv29_enable,
-	.disable = mipi_dbi_pipe_disable,
-	.update = mipi_dbi_pipe_update,
+	DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(yx240qv29_enable),
 };
 
 static const struct drm_display_mode yx240qv29_mode = {
diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c
index 1bb847466b107..9f735d84d85d4 100644
--- a/drivers/gpu/drm/tiny/ili9486.c
+++ b/drivers/gpu/drm/tiny/ili9486.c
@@ -150,10 +150,7 @@ static void waveshare_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs waveshare_pipe_funcs = {
-	.mode_valid = mipi_dbi_pipe_mode_valid,
-	.enable = waveshare_enable,
-	.disable = mipi_dbi_pipe_disable,
-	.update = mipi_dbi_pipe_update,
+	DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(waveshare_enable),
 };
 
 static const struct drm_display_mode waveshare_mode = {
diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c
index 47df2b5a3048f..01ff43c8ac3ff 100644
--- a/drivers/gpu/drm/tiny/mi0283qt.c
+++ b/drivers/gpu/drm/tiny/mi0283qt.c
@@ -141,10 +141,7 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
-	.mode_valid = mipi_dbi_pipe_mode_valid,
-	.enable = mi0283qt_enable,
-	.disable = mipi_dbi_pipe_disable,
-	.update = mipi_dbi_pipe_update,
+	DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(mi0283qt_enable),
 };
 
 static const struct drm_display_mode mi0283qt_mode = {
diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
index 03a7d569cd568..2ed23ded51997 100644
--- a/drivers/gpu/drm/tiny/panel-mipi-dbi.c
+++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
@@ -212,10 +212,7 @@ static void panel_mipi_dbi_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs panel_mipi_dbi_pipe_funcs = {
-	.mode_valid = mipi_dbi_pipe_mode_valid,
-	.enable = panel_mipi_dbi_enable,
-	.disable = mipi_dbi_pipe_disable,
-	.update = mipi_dbi_pipe_update,
+	DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(panel_mipi_dbi_enable),
 };
 
 DEFINE_DRM_GEM_DMA_FOPS(panel_mipi_dbi_fops);
diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c
index 15d9cf283c66a..477eb36fbb70d 100644
--- a/drivers/gpu/drm/tiny/st7735r.c
+++ b/drivers/gpu/drm/tiny/st7735r.c
@@ -133,10 +133,7 @@ static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs st7735r_pipe_funcs = {
-	.mode_valid	= mipi_dbi_pipe_mode_valid,
-	.enable		= st7735r_pipe_enable,
-	.disable	= mipi_dbi_pipe_disable,
-	.update		= mipi_dbi_pipe_update,
+	DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(st7735r_pipe_enable),
 };
 
 static const struct st7735r_cfg jd_t18003_t01_cfg = {
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index 14eaecb1825c1..8c4ea7956d61d 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -207,4 +207,20 @@ void mipi_dbi_debugfs_init(struct drm_minor *minor);
 static inline void mipi_dbi_debugfs_init(struct drm_minor *minor) {}
 #endif
 
+/**
+ * DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS - Initializes struct drm_simple_display_pipe_funcs
+ *                                          for MIPI-DBI devices
+ * @enable_: Enable-callback implementation
+ *
+ * This macro initializes struct drm_simple_display_pipe_funcs with default
+ * values for MIPI-DBI-based devices. The only callback that depends on the
+ * hardware is @enable, for which the driver has to provide an implementation.
+ * MIPI-based drivers are encouraged to use this macro for initialization.
+ */
+#define DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(enable_) \
+	.mode_valid = mipi_dbi_pipe_mode_valid, \
+	.enable = (enable_), \
+	.disable = mipi_dbi_pipe_disable, \
+	.update = mipi_dbi_pipe_update
+
 #endif /* __LINUX_MIPI_DBI_H */
-- 
2.38.1

[PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers

[Thomas Zimmermann]

Move the vmap/vunmap blocks from the inner fb_dirty helpers into the
MIPI DBI update helpers. The function calls can result in waiting and/or
processing overhead. Reduce the penalties by executing the functions once
in the outer-most function of the pipe update.

This change also prepares for MIPI DBI for shadow-plane helpers. With
shadow-plane helpers, transfer source buffers are mapped into kernel
address space automatically.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_mipi_dbi.c | 60 +++++++++++++++++++---------------
 drivers/gpu/drm/tiny/ili9225.c | 31 ++++++++++++++----
 drivers/gpu/drm/tiny/st7586.c  | 28 +++++++++++-----
 include/drm/drm_mipi_dbi.h     |  6 ++--
 4 files changed, 81 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index a6ac565808765..40e59a3a6481e 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -192,6 +192,7 @@ EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
 /**
  * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
  * @dst: The destination buffer
+ * @src: The source buffer
  * @fb: The source framebuffer
  * @clip: Clipping rectangle of the area to be copied
  * @swap: When true, swap MSB/LSB of 16-bit values
@@ -199,12 +200,13 @@ EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
  * Returns:
  * Zero on success, negative error code on failure.
  */
-int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
+int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *fb,
 		      struct drm_rect *clip, bool swap)
 {
 	struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
-	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
-	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
+	struct iosys_map data[DRM_FORMAT_MAX_PLANES] = {
+		*src,
+	};
 	struct iosys_map dst_map = IOSYS_MAP_INIT_VADDR(dst);
 	int ret;
 
@@ -212,10 +214,6 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
 	if (ret)
 		return ret;
 
-	ret = drm_gem_fb_vmap(fb, map, data);
-	if (ret)
-		goto out_drm_gem_fb_end_cpu_access;
-
 	switch (fb->format->format) {
 	case DRM_FORMAT_RGB565:
 		if (swap)
@@ -232,8 +230,6 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
 		ret = -EINVAL;
 	}
 
-	drm_gem_fb_vunmap(fb, map);
-out_drm_gem_fb_end_cpu_access:
 	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 
 	return ret;
@@ -257,10 +253,9 @@ static void mipi_dbi_set_window_address(struct mipi_dbi_dev *dbidev,
 			 ys & 0xff, (ye >> 8) & 0xff, ye & 0xff);
 }
 
-static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
+static void mipi_dbi_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
+			      struct drm_rect *rect)
 {
-	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
-	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
 	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
 	unsigned int height = rect->y2 - rect->y1;
 	unsigned int width = rect->x2 - rect->x1;
@@ -270,16 +265,9 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
 	bool full;
 	void *tr;
 
-	if (WARN_ON(!fb))
-		return;
-
 	if (!drm_dev_enter(fb->dev, &idx))
 		return;
 
-	ret = drm_gem_fb_vmap(fb, map, data);
-	if (ret)
-		goto err_drm_dev_exit;
-
 	full = width == fb->width && height == fb->height;
 
 	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
@@ -287,11 +275,11 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
 	if (!dbi->dc || !full || swap ||
 	    fb->format->format == DRM_FORMAT_XRGB8888) {
 		tr = dbidev->tx_buf;
-		ret = mipi_dbi_buf_copy(dbidev->tx_buf, fb, rect, swap);
+		ret = mipi_dbi_buf_copy(tr, src, fb, rect, swap);
 		if (ret)
 			goto err_msg;
 	} else {
-		tr = data[0].vaddr; /* TODO: Use mapping abstraction properly */
+		tr = src->vaddr; /* TODO: Use mapping abstraction properly */
 	}
 
 	mipi_dbi_set_window_address(dbidev, rect->x1, rect->x2 - 1, rect->y1,
@@ -303,9 +291,6 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
 	if (ret)
 		drm_err_once(fb->dev, "Failed to update display %d\n", ret);
 
-	drm_gem_fb_vunmap(fb, map);
-
-err_drm_dev_exit:
 	drm_dev_exit(idx);
 }
 
@@ -338,14 +323,27 @@ EXPORT_SYMBOL(mipi_dbi_pipe_mode_valid);
 void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
 			  struct drm_plane_state *old_state)
 {
+	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
+	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
 	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_framebuffer *fb = state->fb;
 	struct drm_rect rect;
+	int ret;
 
 	if (!pipe->crtc.state->active)
 		return;
 
+	if (WARN_ON(!fb))
+		return;
+
+	ret = drm_gem_fb_vmap(fb, map, data);
+	if (ret)
+		return;
+
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
-		mipi_dbi_fb_dirty(state->fb, &rect);
+		mipi_dbi_fb_dirty(&data[0], fb, &rect);
+
+	drm_gem_fb_vunmap(fb, map);
 }
 EXPORT_SYMBOL(mipi_dbi_pipe_update);
 
@@ -373,14 +371,22 @@ void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
 		.y1 = 0,
 		.y2 = fb->height,
 	};
-	int idx;
+	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
+	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
+	int idx, ret;
 
 	if (!drm_dev_enter(&dbidev->drm, &idx))
 		return;
 
-	mipi_dbi_fb_dirty(fb, &rect);
+	ret = drm_gem_fb_vmap(fb, map, data);
+	if (ret)
+		goto err_drm_dev_exit;
+
+	mipi_dbi_fb_dirty(&data[0], fb, &rect);
 	backlight_enable(dbidev->backlight);
 
+	drm_gem_fb_vunmap(fb, map);
+err_drm_dev_exit:
 	drm_dev_exit(idx);
 }
 EXPORT_SYMBOL(mipi_dbi_enable_flush);
diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c
index f05a2d25866c1..0115c4090f9e7 100644
--- a/drivers/gpu/drm/tiny/ili9225.c
+++ b/drivers/gpu/drm/tiny/ili9225.c
@@ -25,6 +25,7 @@
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_dma_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_mipi_dbi.h>
 #include <drm/drm_rect.h>
@@ -76,9 +77,9 @@ static inline int ili9225_command(struct mipi_dbi *dbi, u8 cmd, u16 data)
 	return mipi_dbi_command_buf(dbi, cmd, par, 2);
 }
 
-static void ili9225_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
+static void ili9225_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
+			     struct drm_rect *rect)
 {
-	struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
 	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
 	unsigned int height = rect->y2 - rect->y1;
 	unsigned int width = rect->x2 - rect->x1;
@@ -100,11 +101,11 @@ static void ili9225_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
 	if (!dbi->dc || !full || swap ||
 	    fb->format->format == DRM_FORMAT_XRGB8888) {
 		tr = dbidev->tx_buf;
-		ret = mipi_dbi_buf_copy(dbidev->tx_buf, fb, rect, swap);
+		ret = mipi_dbi_buf_copy(tr, src, fb, rect, swap);
 		if (ret)
 			goto err_msg;
 	} else {
-		tr = dma_obj->vaddr;
+		tr = src->vaddr; /* TODO: Use mapping abstraction properly */
 	}
 
 	switch (dbidev->rotation) {
@@ -162,14 +163,24 @@ static void ili9225_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
 static void ili9225_pipe_update(struct drm_simple_display_pipe *pipe,
 				struct drm_plane_state *old_state)
 {
+	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
+	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
 	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_framebuffer *fb = state->fb;
 	struct drm_rect rect;
+	int ret;
 
 	if (!pipe->crtc.state->active)
 		return;
 
+	ret = drm_gem_fb_vmap(fb, map, data);
+	if (ret)
+		return;
+
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
-		ili9225_fb_dirty(state->fb, &rect);
+		ili9225_fb_dirty(&data[0], fb, &rect);
+
+	drm_gem_fb_vunmap(fb, map);
 }
 
 static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
@@ -186,6 +197,8 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
 		.y1 = 0,
 		.y2 = fb->height,
 	};
+	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
+	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
 	int ret, idx;
 	u8 am_id;
 
@@ -276,7 +289,13 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	ili9225_command(dbi, ILI9225_DISPLAY_CONTROL_1, 0x1017);
 
-	ili9225_fb_dirty(fb, &rect);
+	ret = drm_gem_fb_vmap(fb, map, data);
+	if (ret)
+		goto out_exit;
+
+	ili9225_fb_dirty(&data[0], fb, &rect);
+
+	drm_gem_fb_vunmap(fb, map);
 out_exit:
 	drm_dev_exit(idx);
 }
diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c
index 6bdd23e2a47c7..e773b1f2fd5f3 100644
--- a/drivers/gpu/drm/tiny/st7586.c
+++ b/drivers/gpu/drm/tiny/st7586.c
@@ -92,25 +92,24 @@ static void st7586_xrgb8888_to_gray332(u8 *dst, void *vaddr,
 	kfree(buf);
 }
 
-static int st7586_buf_copy(void *dst, struct drm_framebuffer *fb,
+static int st7586_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *fb,
 			   struct drm_rect *clip)
 {
-	struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
-	void *src = dma_obj->vaddr;
-	int ret = 0;
+	int ret;
 
 	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
 	if (ret)
 		return ret;
 
-	st7586_xrgb8888_to_gray332(dst, src, fb, clip);
+	st7586_xrgb8888_to_gray332(dst, src->vaddr, fb, clip);
 
 	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 
 	return 0;
 }
 
-static void st7586_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
+static void st7586_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
+			    struct drm_rect *rect)
 {
 	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
 	struct mipi_dbi *dbi = &dbidev->dbi;
@@ -125,7 +124,7 @@ static void st7586_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
 
 	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
 
-	ret = st7586_buf_copy(dbidev->tx_buf, fb, rect);
+	ret = st7586_buf_copy(dbidev->tx_buf, src, fb, rect);
 	if (ret)
 		goto err_msg;
 
@@ -154,13 +153,19 @@ static void st7586_pipe_update(struct drm_simple_display_pipe *pipe,
 			       struct drm_plane_state *old_state)
 {
 	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_framebuffer *fb = state->fb;
+	struct drm_gem_dma_object *dma_obj;
+	struct iosys_map src;
 	struct drm_rect rect;
 
 	if (!pipe->crtc.state->active)
 		return;
 
+	dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
+	iosys_map_set_vaddr(&src, dma_obj->vaddr);
+
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
-		st7586_fb_dirty(state->fb, &rect);
+		st7586_fb_dirty(&src, fb, &rect);
 }
 
 static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
@@ -176,6 +181,8 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
 		.y1 = 0,
 		.y2 = fb->height,
 	};
+	struct drm_gem_dma_object *dma_obj;
+	struct iosys_map src;
 	int idx, ret;
 	u8 addr_mode;
 
@@ -235,7 +242,10 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	msleep(100);
 
-	st7586_fb_dirty(fb, &rect);
+	dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
+	iosys_map_set_vaddr(&src, dma_obj->vaddr);
+
+	st7586_fb_dirty(&src, fb, &rect);
 
 	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
 out_exit:
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index 8c4ea7956d61d..36ac8495566b0 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -13,9 +13,10 @@
 #include <drm/drm_simple_kms_helper.h>
 
 struct drm_rect;
-struct spi_device;
 struct gpio_desc;
+struct iosys_map;
 struct regulator;
+struct spi_device;
 
 /**
  * struct mipi_dbi - MIPI DBI interface
@@ -176,8 +177,9 @@ int mipi_dbi_command_read(struct mipi_dbi *dbi, u8 cmd, u8 *val);
 int mipi_dbi_command_buf(struct mipi_dbi *dbi, u8 cmd, u8 *data, size_t len);
 int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, const u8 *data,
 			      size_t len);
-int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
+int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *fb,
 		      struct drm_rect *clip, bool swap);
+
 /**
  * mipi_dbi_command - MIPI DCS command with optional parameter(s)
  * @dbi: MIPI DBI structure
-- 
2.38.1

[PATCH 6/8] drm/mipi-dbi: Support shadow-plane state

[Thomas Zimmermann]

Introduce struct drm_mipi_dbi_plane_state that contains state related to
MIPI DBI. It currently only inherits from struct drm_shadow_plane_state,
so that MIPI DBI drivers can use the vmap'ed GEM-buffer memory. Implement
state helpers, the {begin,end}_fb_access helpers and wire up everything.

With this commit, MIPI DBI drivers can access the GEM object's memory
that is provided by shadow-plane state. The actual changes to drivers
are implemented separately. The new struct drm_mipi_dbi_plane was added
to avoid exposing struct drm_shadow_plane_state directly. The latter is
a detail of the actual implementation and having it in the MIPI driver
interface seems unintuitive.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_mipi_dbi.c | 113 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tiny/ili9225.c |   5 ++
 drivers/gpu/drm/tiny/st7586.c  |   5 ++
 include/drm/drm_mipi_dbi.h     |  30 ++++++++-
 4 files changed, 152 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index 40e59a3a6481e..3030344d25b48 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -436,6 +436,119 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
 }
 EXPORT_SYMBOL(mipi_dbi_pipe_disable);
 
+/**
+ * mipi_dbi_pipe_begin_fb_access - MIPI DBI pipe begin-access helper
+ * @pipe: Display pipe
+ * @plane_state: Plane state
+ *
+ * This function implements struct &drm_simple_display_funcs.begin_fb_access.
+ *
+ * See drm_gem_begin_shadow_fb_access() for details and mipi_dbi_pipe_cleanup_fb()
+ * for cleanup.
+ *
+ * Returns:
+ * 0 on success, or a negative errno code otherwise.
+ */
+int mipi_dbi_pipe_begin_fb_access(struct drm_simple_display_pipe *pipe,
+				  struct drm_plane_state *plane_state)
+{
+	return drm_gem_begin_shadow_fb_access(&pipe->plane, plane_state);
+}
+EXPORT_SYMBOL(mipi_dbi_pipe_begin_fb_access);
+
+/**
+ * mipi_dbi_pipe_end_fb_access - MIPI DBI pipe end-access helper
+ * @pipe: Display pipe
+ * @plane_state: Plane state
+ *
+ * This function implements struct &drm_simple_display_funcs.end_fb_access.
+ *
+ * See mipi_dbi_pipe_begin_fb_access().
+ */
+void mipi_dbi_pipe_end_fb_access(struct drm_simple_display_pipe *pipe,
+				 struct drm_plane_state *plane_state)
+{
+	drm_gem_end_shadow_fb_access(&pipe->plane, plane_state);
+}
+EXPORT_SYMBOL(mipi_dbi_pipe_end_fb_access);
+
+/**
+ * mipi_dbi_pipe_reset_plane - MIPI DBI plane-reset helper
+ * @pipe: Display pipe
+ *
+ * This function implements struct &drm_simple_display_funcs.reset_plane
+ * for MIPI DBI planes.
+ */
+void mipi_dbi_pipe_reset_plane(struct drm_simple_display_pipe *pipe)
+{
+	struct drm_plane *plane = &pipe->plane;
+	struct mipi_dbi_plane_state *mipi_dbi_plane_state;
+
+	if (plane->state) {
+		mipi_dbi_pipe_destroy_plane_state(pipe, plane->state);
+		plane->state = NULL; /* must be set to NULL here */
+	}
+
+	mipi_dbi_plane_state = kzalloc(sizeof(*mipi_dbi_plane_state), GFP_KERNEL);
+	if (!mipi_dbi_plane_state)
+		return;
+	__drm_gem_reset_shadow_plane(plane, &mipi_dbi_plane_state->shadow_plane_state);
+}
+EXPORT_SYMBOL(mipi_dbi_pipe_reset_plane);
+
+/**
+ * mipi_dbi_pipe_duplicate_plane_state - duplicates MIPI DBI plane state
+ * @pipe: Display pipe
+ *
+ * This function implements struct &drm_simple_display_funcs.duplicate_plane_state
+ * for MIPI DBI planes.
+ *
+ * See drm_gem_duplicate_shadow_plane_state() for additional details.
+ *
+ * Returns:
+ * A pointer to a new plane state on success, or NULL otherwise.
+ */
+struct drm_plane_state *mipi_dbi_pipe_duplicate_plane_state(struct drm_simple_display_pipe *pipe)
+{
+	struct drm_plane *plane = &pipe->plane;
+	struct drm_plane_state *plane_state = plane->state;
+	struct mipi_dbi_plane_state *new_mipi_dbi_plane_state;
+	struct drm_shadow_plane_state *new_shadow_plane_state;
+
+	if (!plane_state)
+		return NULL;
+
+	new_mipi_dbi_plane_state = kzalloc(sizeof(*new_mipi_dbi_plane_state), GFP_KERNEL);
+	if (!new_mipi_dbi_plane_state)
+		return NULL;
+	new_shadow_plane_state = &new_mipi_dbi_plane_state->shadow_plane_state;
+
+	__drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
+
+	return &new_shadow_plane_state->base;
+}
+EXPORT_SYMBOL(mipi_dbi_pipe_duplicate_plane_state);
+
+/**
+ * mipi_dbi_pipe_destroy_plane_state - cleans up MIPI DBI plane state
+ * @pipe: Display pipe
+ * @plane_state: Plane state
+ *
+ * This function implements struct drm_simple_display_funcs.destroy_plane_state
+ * for MIPI DBI planes.
+ *
+ * See drm_gem_destroy_shadow_plane_state() for additional details.
+ */
+void mipi_dbi_pipe_destroy_plane_state(struct drm_simple_display_pipe *pipe,
+				       struct drm_plane_state *plane_state)
+{
+	struct mipi_dbi_plane_state *mipi_dbi_plane_state = to_mipi_dbi_plane_state(plane_state);
+
+	__drm_gem_destroy_shadow_plane_state(&mipi_dbi_plane_state->shadow_plane_state);
+	kfree(mipi_dbi_plane_state);
+}
+EXPORT_SYMBOL(mipi_dbi_pipe_destroy_plane_state);
+
 static int mipi_dbi_connector_get_modes(struct drm_connector *connector)
 {
 	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(connector->dev);
diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c
index 0115c4090f9e7..9e55ce28b4552 100644
--- a/drivers/gpu/drm/tiny/ili9225.c
+++ b/drivers/gpu/drm/tiny/ili9225.c
@@ -349,6 +349,11 @@ static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = {
 	.enable		= ili9225_pipe_enable,
 	.disable	= ili9225_pipe_disable,
 	.update		= ili9225_pipe_update,
+	.begin_fb_access = mipi_dbi_pipe_begin_fb_access,
+	.end_fb_access	= mipi_dbi_pipe_end_fb_access,
+	.reset_plane	= mipi_dbi_pipe_reset_plane,
+	.duplicate_plane_state = mipi_dbi_pipe_duplicate_plane_state,
+	.destroy_plane_state = mipi_dbi_pipe_destroy_plane_state,
 };
 
 static const struct drm_display_mode ili9225_mode = {
diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c
index e773b1f2fd5f3..76b13cefc904f 100644
--- a/drivers/gpu/drm/tiny/st7586.c
+++ b/drivers/gpu/drm/tiny/st7586.c
@@ -277,6 +277,11 @@ static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
 	.enable		= st7586_pipe_enable,
 	.disable	= st7586_pipe_disable,
 	.update		= st7586_pipe_update,
+	.begin_fb_access = mipi_dbi_pipe_begin_fb_access,
+	.end_fb_access	= mipi_dbi_pipe_end_fb_access,
+	.reset_plane	= mipi_dbi_pipe_reset_plane,
+	.duplicate_plane_state = mipi_dbi_pipe_duplicate_plane_state,
+	.destroy_plane_state = mipi_dbi_pipe_destroy_plane_state,
 };
 
 static const struct drm_display_mode st7586_mode = {
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index 36ac8495566b0..0213d4aae0326 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -10,6 +10,7 @@
 
 #include <linux/mutex.h>
 #include <drm/drm_device.h>
+#include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_simple_kms_helper.h>
 
 struct drm_rect;
@@ -18,6 +19,19 @@ struct iosys_map;
 struct regulator;
 struct spi_device;
 
+/**
+ * struct mipi_dbi_plane_state - MIPI DBI plane state
+ */
+struct mipi_dbi_plane_state {
+	struct drm_shadow_plane_state shadow_plane_state;
+};
+
+static inline struct mipi_dbi_plane_state *
+to_mipi_dbi_plane_state(struct drm_plane_state *plane_state)
+{
+	return container_of(plane_state, struct mipi_dbi_plane_state, shadow_plane_state.base);
+}
+
 /**
  * struct mipi_dbi - MIPI DBI interface
  */
@@ -164,6 +178,15 @@ void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
 			   struct drm_crtc_state *crtc_state,
 			   struct drm_plane_state *plan_state);
 void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
+int mipi_dbi_pipe_begin_fb_access(struct drm_simple_display_pipe *pipe,
+				  struct drm_plane_state *plane_state);
+void mipi_dbi_pipe_end_fb_access(struct drm_simple_display_pipe *pipe,
+				 struct drm_plane_state *plane_state);
+void mipi_dbi_pipe_reset_plane(struct drm_simple_display_pipe *pipe);
+struct drm_plane_state *mipi_dbi_pipe_duplicate_plane_state(struct drm_simple_display_pipe *pipe);
+void mipi_dbi_pipe_destroy_plane_state(struct drm_simple_display_pipe *pipe,
+				       struct drm_plane_state *plane_state);
+
 void mipi_dbi_hw_reset(struct mipi_dbi *dbi);
 bool mipi_dbi_display_is_on(struct mipi_dbi *dbi);
 int mipi_dbi_poweron_reset(struct mipi_dbi_dev *dbidev);
@@ -223,6 +246,11 @@ static inline void mipi_dbi_debugfs_init(struct drm_minor *minor) {}
 	.mode_valid = mipi_dbi_pipe_mode_valid, \
 	.enable = (enable_), \
 	.disable = mipi_dbi_pipe_disable, \
-	.update = mipi_dbi_pipe_update
+	.update = mipi_dbi_pipe_update, \
+	.begin_fb_access = mipi_dbi_pipe_begin_fb_access, \
+	.end_fb_access = mipi_dbi_pipe_end_fb_access, \
+	.reset_plane = mipi_dbi_pipe_reset_plane, \
+	.duplicate_plane_state = mipi_dbi_pipe_duplicate_plane_state, \
+	.destroy_plane_state = mipi_dbi_pipe_destroy_plane_state
 
 #endif /* __LINUX_MIPI_DBI_H */
-- 
2.38.1

[PATCH 7/8] drm/mipi-dbi: Use shadow-plane mappings

[Thomas Zimmermann]

Use the buffer mappings provided by shadow-plane helpers. As the
mappings are established while the commit can still fail, errors
are now reported correctly to callers.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_mipi_dbi.c | 31 +++++++++++--------------------
 drivers/gpu/drm/tiny/ili9225.c | 28 ++++++++++------------------
 drivers/gpu/drm/tiny/st7586.c  | 22 ++++++++++------------
 3 files changed, 31 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index 3030344d25b48..d45824a65c9fd 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -323,12 +323,13 @@ EXPORT_SYMBOL(mipi_dbi_pipe_mode_valid);
 void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
 			  struct drm_plane_state *old_state)
 {
-	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
-	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
 	struct drm_plane_state *state = pipe->plane.state;
+	struct mipi_dbi_plane_state *mipi_dbi_plane_state =
+		to_mipi_dbi_plane_state(state);
+	struct drm_shadow_plane_state *shadow_plane_state =
+		&mipi_dbi_plane_state->shadow_plane_state;
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_rect rect;
-	int ret;
 
 	if (!pipe->crtc.state->active)
 		return;
@@ -336,14 +337,8 @@ void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
 	if (WARN_ON(!fb))
 		return;
 
-	ret = drm_gem_fb_vmap(fb, map, data);
-	if (ret)
-		return;
-
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
-		mipi_dbi_fb_dirty(&data[0], fb, &rect);
-
-	drm_gem_fb_vunmap(fb, map);
+		mipi_dbi_fb_dirty(&shadow_plane_state->data[0], fb, &rect);
 }
 EXPORT_SYMBOL(mipi_dbi_pipe_update);
 
@@ -364,6 +359,10 @@ void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
 			   struct drm_crtc_state *crtc_state,
 			   struct drm_plane_state *plane_state)
 {
+	struct mipi_dbi_plane_state *mipi_dbi_plane_state =
+		to_mipi_dbi_plane_state(plane_state);
+	struct drm_shadow_plane_state *shadow_plane_state =
+		&mipi_dbi_plane_state->shadow_plane_state;
 	struct drm_framebuffer *fb = plane_state->fb;
 	struct drm_rect rect = {
 		.x1 = 0,
@@ -371,22 +370,14 @@ void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
 		.y1 = 0,
 		.y2 = fb->height,
 	};
-	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
-	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
-	int idx, ret;
+	int idx;
 
 	if (!drm_dev_enter(&dbidev->drm, &idx))
 		return;
 
-	ret = drm_gem_fb_vmap(fb, map, data);
-	if (ret)
-		goto err_drm_dev_exit;
-
-	mipi_dbi_fb_dirty(&data[0], fb, &rect);
+	mipi_dbi_fb_dirty(&shadow_plane_state->data[0], fb, &rect);
 	backlight_enable(dbidev->backlight);
 
-	drm_gem_fb_vunmap(fb, map);
-err_drm_dev_exit:
 	drm_dev_exit(idx);
 }
 EXPORT_SYMBOL(mipi_dbi_enable_flush);
diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c
index 9e55ce28b4552..ba5681b63ffbf 100644
--- a/drivers/gpu/drm/tiny/ili9225.c
+++ b/drivers/gpu/drm/tiny/ili9225.c
@@ -163,24 +163,19 @@ static void ili9225_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
 static void ili9225_pipe_update(struct drm_simple_display_pipe *pipe,
 				struct drm_plane_state *old_state)
 {
-	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
-	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
 	struct drm_plane_state *state = pipe->plane.state;
+	struct mipi_dbi_plane_state *mipi_dbi_plane_state =
+		to_mipi_dbi_plane_state(state);
+	struct drm_shadow_plane_state *shadow_plane_state =
+		&mipi_dbi_plane_state->shadow_plane_state;
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_rect rect;
-	int ret;
 
 	if (!pipe->crtc.state->active)
 		return;
 
-	ret = drm_gem_fb_vmap(fb, map, data);
-	if (ret)
-		return;
-
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
-		ili9225_fb_dirty(&data[0], fb, &rect);
-
-	drm_gem_fb_vunmap(fb, map);
+		ili9225_fb_dirty(&shadow_plane_state->data[0], fb, &rect);
 }
 
 static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
@@ -188,6 +183,10 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
 				struct drm_plane_state *plane_state)
 {
 	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
+	struct mipi_dbi_plane_state *mipi_dbi_plane_state =
+		to_mipi_dbi_plane_state(plane_state);
+	struct drm_shadow_plane_state *shadow_plane_state =
+		&mipi_dbi_plane_state->shadow_plane_state;
 	struct drm_framebuffer *fb = plane_state->fb;
 	struct device *dev = pipe->crtc.dev->dev;
 	struct mipi_dbi *dbi = &dbidev->dbi;
@@ -197,8 +196,6 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
 		.y1 = 0,
 		.y2 = fb->height,
 	};
-	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
-	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
 	int ret, idx;
 	u8 am_id;
 
@@ -289,13 +286,8 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	ili9225_command(dbi, ILI9225_DISPLAY_CONTROL_1, 0x1017);
 
-	ret = drm_gem_fb_vmap(fb, map, data);
-	if (ret)
-		goto out_exit;
-
-	ili9225_fb_dirty(&data[0], fb, &rect);
+	ili9225_fb_dirty(&shadow_plane_state->data[0], fb, &rect);
 
-	drm_gem_fb_vunmap(fb, map);
 out_exit:
 	drm_dev_exit(idx);
 }
diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c
index 76b13cefc904f..ddaa82c2e58ae 100644
--- a/drivers/gpu/drm/tiny/st7586.c
+++ b/drivers/gpu/drm/tiny/st7586.c
@@ -153,19 +153,18 @@ static void st7586_pipe_update(struct drm_simple_display_pipe *pipe,
 			       struct drm_plane_state *old_state)
 {
 	struct drm_plane_state *state = pipe->plane.state;
+	struct mipi_dbi_plane_state *mipi_dbi_plane_state =
+		to_mipi_dbi_plane_state(state);
+	struct drm_shadow_plane_state *shadow_plane_state =
+		&mipi_dbi_plane_state->shadow_plane_state;
 	struct drm_framebuffer *fb = state->fb;
-	struct drm_gem_dma_object *dma_obj;
-	struct iosys_map src;
 	struct drm_rect rect;
 
 	if (!pipe->crtc.state->active)
 		return;
 
-	dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
-	iosys_map_set_vaddr(&src, dma_obj->vaddr);
-
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
-		st7586_fb_dirty(&src, fb, &rect);
+		st7586_fb_dirty(&shadow_plane_state->data[0], fb, &rect);
 }
 
 static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
@@ -173,6 +172,10 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
 			       struct drm_plane_state *plane_state)
 {
 	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
+	struct mipi_dbi_plane_state *mipi_dbi_plane_state =
+		to_mipi_dbi_plane_state(plane_state);
+	struct drm_shadow_plane_state *shadow_plane_state =
+		&mipi_dbi_plane_state->shadow_plane_state;
 	struct drm_framebuffer *fb = plane_state->fb;
 	struct mipi_dbi *dbi = &dbidev->dbi;
 	struct drm_rect rect = {
@@ -181,8 +184,6 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
 		.y1 = 0,
 		.y2 = fb->height,
 	};
-	struct drm_gem_dma_object *dma_obj;
-	struct iosys_map src;
 	int idx, ret;
 	u8 addr_mode;
 
@@ -242,10 +243,7 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	msleep(100);
 
-	dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
-	iosys_map_set_vaddr(&src, dma_obj->vaddr);
-
-	st7586_fb_dirty(&src, fb, &rect);
+	st7586_fb_dirty(&shadow_plane_state->data[0], fb, &rect);
 
 	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
 out_exit:
-- 
2.38.1

[PATCH 8/8] drm/mipi-dbi: Move drm_dev_{enter, exit}() out from fb_dirty functions

[Thomas Zimmermann]

Call drm_dev_enter() and drm_dev_exit() in the outer-most callbacks
of the modesetting pipeline. If drm_dev_enter() fails, the driver can
thus avoid unnecessary work.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_mipi_dbi.c | 13 +++++++------
 drivers/gpu/drm/tiny/ili9225.c | 13 +++++++------
 drivers/gpu/drm/tiny/st7586.c  | 13 +++++++------
 3 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index d45824a65c9fd..fc1c8c536370a 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -261,13 +261,10 @@ static void mipi_dbi_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
 	unsigned int width = rect->x2 - rect->x1;
 	struct mipi_dbi *dbi = &dbidev->dbi;
 	bool swap = dbi->swap_bytes;
-	int idx, ret = 0;
+	int ret = 0;
 	bool full;
 	void *tr;
 
-	if (!drm_dev_enter(fb->dev, &idx))
-		return;
-
 	full = width == fb->width && height == fb->height;
 
 	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
@@ -290,8 +287,6 @@ static void mipi_dbi_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
 err_msg:
 	if (ret)
 		drm_err_once(fb->dev, "Failed to update display %d\n", ret);
-
-	drm_dev_exit(idx);
 }
 
 /**
@@ -330,6 +325,7 @@ void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
 		&mipi_dbi_plane_state->shadow_plane_state;
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_rect rect;
+	int idx;
 
 	if (!pipe->crtc.state->active)
 		return;
@@ -337,8 +333,13 @@ void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
 	if (WARN_ON(!fb))
 		return;
 
+	if (!drm_dev_enter(fb->dev, &idx))
+		return;
+
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
 		mipi_dbi_fb_dirty(&shadow_plane_state->data[0], fb, &rect);
+
+	drm_dev_exit(idx);
 }
 EXPORT_SYMBOL(mipi_dbi_pipe_update);
 
diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c
index ba5681b63ffbf..7ecbb8b141757 100644
--- a/drivers/gpu/drm/tiny/ili9225.c
+++ b/drivers/gpu/drm/tiny/ili9225.c
@@ -87,13 +87,10 @@ static void ili9225_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
 	bool swap = dbi->swap_bytes;
 	u16 x_start, y_start;
 	u16 x1, x2, y1, y2;
-	int idx, ret = 0;
+	int ret = 0;
 	bool full;
 	void *tr;
 
-	if (!drm_dev_enter(fb->dev, &idx))
-		return;
-
 	full = width == fb->width && height == fb->height;
 
 	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
@@ -156,8 +153,6 @@ static void ili9225_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
 err_msg:
 	if (ret)
 		dev_err_once(fb->dev->dev, "Failed to update display %d\n", ret);
-
-	drm_dev_exit(idx);
 }
 
 static void ili9225_pipe_update(struct drm_simple_display_pipe *pipe,
@@ -170,12 +165,18 @@ static void ili9225_pipe_update(struct drm_simple_display_pipe *pipe,
 		&mipi_dbi_plane_state->shadow_plane_state;
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_rect rect;
+	int idx;
 
 	if (!pipe->crtc.state->active)
 		return;
 
+	if (!drm_dev_enter(fb->dev, &idx))
+		return;
+
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
 		ili9225_fb_dirty(&shadow_plane_state->data[0], fb, &rect);
+
+	drm_dev_exit(idx);
 }
 
 static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c
index ddaa82c2e58ae..0f5e532fe5d1d 100644
--- a/drivers/gpu/drm/tiny/st7586.c
+++ b/drivers/gpu/drm/tiny/st7586.c
@@ -113,10 +113,7 @@ static void st7586_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
 {
 	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
 	struct mipi_dbi *dbi = &dbidev->dbi;
-	int start, end, idx, ret = 0;
-
-	if (!drm_dev_enter(fb->dev, &idx))
-		return;
+	int start, end, ret = 0;
 
 	/* 3 pixels per byte, so grow clip to nearest multiple of 3 */
 	rect->x1 = rounddown(rect->x1, 3);
@@ -145,8 +142,6 @@ static void st7586_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
 err_msg:
 	if (ret)
 		dev_err_once(fb->dev->dev, "Failed to update display %d\n", ret);
-
-	drm_dev_exit(idx);
 }
 
 static void st7586_pipe_update(struct drm_simple_display_pipe *pipe,
@@ -159,12 +154,18 @@ static void st7586_pipe_update(struct drm_simple_display_pipe *pipe,
 		&mipi_dbi_plane_state->shadow_plane_state;
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_rect rect;
+	int idx;
 
 	if (!pipe->crtc.state->active)
 		return;
 
+	if (!drm_dev_enter(fb->dev, &idx))
+		return;
+
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
 		st7586_fb_dirty(&shadow_plane_state->data[0], fb, &rect);
+
+	drm_dev_exit(idx);
 }
 
 static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
-- 
2.38.1

Re: [PATCH 0/8] drm/mipi-dbi: Convert to shadow-plane helpers

[Noralf Trønnes]

Den 21.11.2022 11.45, skrev Thomas Zimmermann:
> Convert the MIPI-DBI-based drivers to shadow-plane helpers. The
> drivers vmap/vunmap GEM buffer memory during the atomic commit.
> Shadow-plane helpers automate this process.
> 
> Patches 1 to 4 prepare the MIPI code for the change and simplify
> the restof the patchset.
> 
> Patches 5 to 7 rework the vmap code in the MIPI-DBI drivers and add
> shadow-plane helpers. Most of the affected drivers call MIPI-DBI
> helpers and get the update automatically. Only ili9225 and st7586
> require changes to their source code.
> 
> Patch 8 simplifies drm_dev_enter() and _exit(). It's not strictly
> needed, but streamlines the driver code and make sense overall.
> 
> Testing is welcome, as I don't have any hardware to test these
> changes myself.
> 

I can do a test this weekend.

Btw I've converted drm/gud to the shadow plane helper, I just need to
solve an smtp problem[1] so I can send out the patchset.

[1]
https://lore.kernel.org/dri-devel/1bc45775-0667-01f8-36e1-9f65d3081092@tronnes.org/T/#u

Noralf.

Re: [PATCH 0/8] drm/mipi-dbi: Convert to shadow-plane helpers

[Thomas Zimmermann]

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

Hi

Am 21.11.22 um 13:27 schrieb Noralf Trønnes:
> 
> 
> Den 21.11.2022 11.45, skrev Thomas Zimmermann:
>> Convert the MIPI-DBI-based drivers to shadow-plane helpers. The
>> drivers vmap/vunmap GEM buffer memory during the atomic commit.
>> Shadow-plane helpers automate this process.
>>
>> Patches 1 to 4 prepare the MIPI code for the change and simplify
>> the restof the patchset.
>>
>> Patches 5 to 7 rework the vmap code in the MIPI-DBI drivers and add
>> shadow-plane helpers. Most of the affected drivers call MIPI-DBI
>> helpers and get the update automatically. Only ili9225 and st7586
>> require changes to their source code.
>>
>> Patch 8 simplifies drm_dev_enter() and _exit(). It's not strictly
>> needed, but streamlines the driver code and make sense overall.
>>
>> Testing is welcome, as I don't have any hardware to test these
>> changes myself.
>>
> 
> I can do a test this weekend.

Thanks a lot.

> 
> Btw I've converted drm/gud to the shadow plane helper, I just need to
> solve an smtp problem[1] so I can send out the patchset.

How so?  When I looked at it, the vmap/vunmap happened on a separate 
worker than the commit IIRC.

Best regards
Thomas

> 
> [1]
> https://lore.kernel.org/dri-devel/1bc45775-0667-01f8-36e1-9f65d3081092@tronnes.org/T/#u
> 
> Noralf.

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

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

Re: [PATCH 0/8] drm/mipi-dbi: Convert to shadow-plane helpers

[Noralf Trønnes]

Den 21.11.2022 13.41, skrev Thomas Zimmermann:
> Hi
> 
> Am 21.11.22 um 13:27 schrieb Noralf Trønnes:
>>
>>
>> Den 21.11.2022 11.45, skrev Thomas Zimmermann:
>>> Convert the MIPI-DBI-based drivers to shadow-plane helpers. The
>>> drivers vmap/vunmap GEM buffer memory during the atomic commit.
>>> Shadow-plane helpers automate this process.
>>>
>>> Patches 1 to 4 prepare the MIPI code for the change and simplify
>>> the restof the patchset.
>>>
>>> Patches 5 to 7 rework the vmap code in the MIPI-DBI drivers and add
>>> shadow-plane helpers. Most of the affected drivers call MIPI-DBI
>>> helpers and get the update automatically. Only ili9225 and st7586
>>> require changes to their source code.
>>>
>>> Patch 8 simplifies drm_dev_enter() and _exit(). It's not strictly
>>> needed, but streamlines the driver code and make sense overall.
>>>
>>> Testing is welcome, as I don't have any hardware to test these
>>> changes myself.
>>>
>>
>> I can do a test this weekend.
> 
> Thanks a lot.
> 
>>
>> Btw I've converted drm/gud to the shadow plane helper, I just need to
>> solve an smtp problem[1] so I can send out the patchset.
> 
> How so?  When I looked at it, the vmap/vunmap happened on a separate
> worker than the commit IIRC.
> 

Yes you're right, originally the driver only did flushing asynchronously
in a worker which meant it could access the framebuffer at the same time
as userspace. Later when GNOME got support for one rendering loop per
display, I added a module parameter to enable synchronous flushing
during the commit, it also uses the worker for this but waits for it to
complete.

What I've done in the patchset is to inline the sync flushing and use a
shadow buffer for the async path which still uses the worker, but now it
won't risk reading the framebuffer while userspace writes to it, instead
it reads from the shadow buffer.

Noralf.

> Best regards
> Thomas
> 
>>
>> [1]
>> https://lore.kernel.org/dri-devel/1bc45775-0667-01f8-36e1-9f65d3081092@tronnes.org/T/#u
>>
>> Noralf.
> 

Re: [PATCH 0/8] drm/mipi-dbi: Convert to shadow-plane helpers

[Javier Martinez Canillas]

On 11/21/22 13:27, Noralf Trønnes wrote:
> 
> 
> Den 21.11.2022 11.45, skrev Thomas Zimmermann:
>> Convert the MIPI-DBI-based drivers to shadow-plane helpers. The
>> drivers vmap/vunmap GEM buffer memory during the atomic commit.
>> Shadow-plane helpers automate this process.
>>
>> Patches 1 to 4 prepare the MIPI code for the change and simplify
>> the restof the patchset.
>>
>> Patches 5 to 7 rework the vmap code in the MIPI-DBI drivers and add
>> shadow-plane helpers. Most of the affected drivers call MIPI-DBI
>> helpers and get the update automatically. Only ili9225 and st7586
>> require changes to their source code.
>>
>> Patch 8 simplifies drm_dev_enter() and _exit(). It's not strictly
>> needed, but streamlines the driver code and make sense overall.
>>
>> Testing is welcome, as I don't have any hardware to test these
>> changes myself.
>>
> 
> I can do a test this weekend.
>

I've a ST7735 display so I can also give it a test this weekend.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 1/8] drm/simple-kms: Remove drm_gem_simple_display_pipe_prepare_fb()

[Noralf Trønnes]

Den 21.11.2022 11.45, skrev Thomas Zimmermann:
> The helper drm_gem_simple_display_pipe_prepare_fb() is simple-KMS'
> default implementation for prepare_fb. Remove the call from drivers
> that set it explicitly. Then inline the helper into the only caller
> within simple-kms helpers and remove . No functional changes.
> 
> Simple-KMS drivers that implement the prepare_fb callback should call
> drm_gem_plane_helper_prepare_fb() directly.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

Re: [PATCH 2/8] drm/ili9225: Call MIPI DBI mode_valid helper

[Noralf Trønnes]

Den 21.11.2022 11.45, skrev Thomas Zimmermann:
> MIPI DBI drivers validate each mode against their native resolution.
> Add this test to ili9225.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

Re: [PATCH 3/8] drm/st7586: Call MIPI DBI mode_valid helper

[Noralf Trønnes]

Den 21.11.2022 11.45, skrev Thomas Zimmermann:
> MIPI DBI drivers validate each mode against their native resolution.
> Add this test to st7586.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

Re: [PATCH 4/8] drm/mipi-dbi: Initialize default driver functions with macro

[Noralf Trønnes]

Den 21.11.2022 11.45, skrev Thomas Zimmermann:
> Introduce DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS to initialize MIPI-DBI
> helpers to default values and convert drivers. The prepare_fb function
> set by some drivers is called implicitly by simple-kms helpers, so leave
> it out.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

>  static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = {
> -	.mode_valid = mipi_dbi_pipe_mode_valid,
> -	.enable = yx240qv29_enable,
> -	.disable = mipi_dbi_pipe_disable,
> -	.update = mipi_dbi_pipe_update,
> +	DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(yx240qv29_enable),
>  };

>  static const struct drm_simple_display_pipe_funcs ili9163_pipe_funcs = {
> -	.mode_valid = mipi_dbi_pipe_mode_valid,
> -	.enable = yx240qv29_enable,
> -	.disable = mipi_dbi_pipe_disable,
> -	.update = mipi_dbi_pipe_update,
> +	DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(yx240qv29_enable),
>  };

>  static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
> -	.mode_valid = mipi_dbi_pipe_mode_valid,
> -	.enable = yx240qv29_enable,
> -	.disable = mipi_dbi_pipe_disable,
> -	.update = mipi_dbi_pipe_update,
> +	DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(yx240qv29_enable),
>  };

3 drivers have the same enable function name, ili9163 and hx8357d has
clearly copied from ili9341 which actually supports the yx240qv29 panel.
At least hx8357d managed to update the display mode variable name,
ili9163 didn't. It's not unlikely that I reviewed these drivers...

But that has nothing to do with this patch:

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

Re: [PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers

[Noralf Trønnes]

Den 21.11.2022 11.45, skrev Thomas Zimmermann:
> Move the vmap/vunmap blocks from the inner fb_dirty helpers into the
> MIPI DBI update helpers. The function calls can result in waiting and/or
> processing overhead. Reduce the penalties by executing the functions once
> in the outer-most function of the pipe update.
> 
> This change also prepares for MIPI DBI for shadow-plane helpers. With
> shadow-plane helpers, transfer source buffers are mapped into kernel
> address space automatically.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_mipi_dbi.c | 60 +++++++++++++++++++---------------
>  drivers/gpu/drm/tiny/ili9225.c | 31 ++++++++++++++----
>  drivers/gpu/drm/tiny/st7586.c  | 28 +++++++++++-----
>  include/drm/drm_mipi_dbi.h     |  6 ++--
>  4 files changed, 81 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
> index a6ac565808765..40e59a3a6481e 100644
> --- a/drivers/gpu/drm/drm_mipi_dbi.c
> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
> @@ -192,6 +192,7 @@ EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
>  /**
>   * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
>   * @dst: The destination buffer
> + * @src: The source buffer
>   * @fb: The source framebuffer
>   * @clip: Clipping rectangle of the area to be copied
>   * @swap: When true, swap MSB/LSB of 16-bit values
> @@ -199,12 +200,13 @@ EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
>   * Returns:
>   * Zero on success, negative error code on failure.
>   */
> -int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
> +int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *fb,
>  		      struct drm_rect *clip, bool swap)
>  {
>  	struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
> -	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> -	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
> +	struct iosys_map data[DRM_FORMAT_MAX_PLANES] = {
> +		*src,
> +	};

I would prefer that you used src directly when calling the drm_fb_*
functions, data can be anything and to me it isn't clear what that
contains when I see it (ofc now I know, but next year I've forgotten).
And is the array necessary, this helper will only ever support one color
plane after all?

>  	struct iosys_map dst_map = IOSYS_MAP_INIT_VADDR(dst);
>  	int ret;
>  
> @@ -212,10 +214,6 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>  	if (ret)
>  		return ret;
>  
> -	ret = drm_gem_fb_vmap(fb, map, data);
> -	if (ret)
> -		goto out_drm_gem_fb_end_cpu_access;
> -
>  	switch (fb->format->format) {
>  	case DRM_FORMAT_RGB565:
>  		if (swap)
> @@ -232,8 +230,6 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>  		ret = -EINVAL;
>  	}
>  
> -	drm_gem_fb_vunmap(fb, map);
> -out_drm_gem_fb_end_cpu_access:
>  	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>  
>  	return ret;
> @@ -257,10 +253,9 @@ static void mipi_dbi_set_window_address(struct mipi_dbi_dev *dbidev,
>  			 ys & 0xff, (ye >> 8) & 0xff, ye & 0xff);
>  }
>  
> -static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
> +static void mipi_dbi_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
> +			      struct drm_rect *rect)
>  {
> -	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> -	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>  	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
>  	unsigned int height = rect->y2 - rect->y1;
>  	unsigned int width = rect->x2 - rect->x1;
> @@ -270,16 +265,9 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>  	bool full;
>  	void *tr;
>  
> -	if (WARN_ON(!fb))
> -		return;
> -
>  	if (!drm_dev_enter(fb->dev, &idx))
>  		return;
>  
> -	ret = drm_gem_fb_vmap(fb, map, data);
> -	if (ret)
> -		goto err_drm_dev_exit;
> -
>  	full = width == fb->width && height == fb->height;
>  
>  	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
> @@ -287,11 +275,11 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>  	if (!dbi->dc || !full || swap ||
>  	    fb->format->format == DRM_FORMAT_XRGB8888) {
>  		tr = dbidev->tx_buf;
> -		ret = mipi_dbi_buf_copy(dbidev->tx_buf, fb, rect, swap);
> +		ret = mipi_dbi_buf_copy(tr, src, fb, rect, swap);
>  		if (ret)
>  			goto err_msg;
>  	} else {
> -		tr = data[0].vaddr; /* TODO: Use mapping abstraction properly */
> +		tr = src->vaddr; /* TODO: Use mapping abstraction properly */
>  	}
>  
>  	mipi_dbi_set_window_address(dbidev, rect->x1, rect->x2 - 1, rect->y1,
> @@ -303,9 +291,6 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>  	if (ret)
>  		drm_err_once(fb->dev, "Failed to update display %d\n", ret);
>  
> -	drm_gem_fb_vunmap(fb, map);
> -
> -err_drm_dev_exit:
>  	drm_dev_exit(idx);
>  }
>  
> @@ -338,14 +323,27 @@ EXPORT_SYMBOL(mipi_dbi_pipe_mode_valid);
>  void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
>  			  struct drm_plane_state *old_state)
>  {
> +	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> +	struct iosys_map data[DRM_FORMAT_MAX_PLANES];

These should have been zeroed to avoid UBSAN complaint, but you'll
remove these later so not so important.

>  	struct drm_plane_state *state = pipe->plane.state;
> +	struct drm_framebuffer *fb = state->fb;
>  	struct drm_rect rect;
> +	int ret;
>  
>  	if (!pipe->crtc.state->active)
>  		return;
>  
> +	if (WARN_ON(!fb))
> +		return;
> +
> +	ret = drm_gem_fb_vmap(fb, map, data);
> +	if (ret)
> +		return;
> +
>  	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
> -		mipi_dbi_fb_dirty(state->fb, &rect);
> +		mipi_dbi_fb_dirty(&data[0], fb, &rect);
> +
> +	drm_gem_fb_vunmap(fb, map);
>  }
>  EXPORT_SYMBOL(mipi_dbi_pipe_update);
>  
> @@ -373,14 +371,22 @@ void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
>  		.y1 = 0,
>  		.y2 = fb->height,
>  	};
> -	int idx;
> +	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> +	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
> +	int idx, ret;
>  
>  	if (!drm_dev_enter(&dbidev->drm, &idx))
>  		return;
>  
> -	mipi_dbi_fb_dirty(fb, &rect);
> +	ret = drm_gem_fb_vmap(fb, map, data);
> +	if (ret)
> +		goto err_drm_dev_exit;
> +
> +	mipi_dbi_fb_dirty(&data[0], fb, &rect);
>  	backlight_enable(dbidev->backlight);
>  
> +	drm_gem_fb_vunmap(fb, map);
> +err_drm_dev_exit:
>  	drm_dev_exit(idx);
>  }
>  EXPORT_SYMBOL(mipi_dbi_enable_flush);
> diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c
> index f05a2d25866c1..0115c4090f9e7 100644
> --- a/drivers/gpu/drm/tiny/ili9225.c
> +++ b/drivers/gpu/drm/tiny/ili9225.c
> @@ -25,6 +25,7 @@
>  #include <drm/drm_framebuffer.h>
>  #include <drm/drm_gem_atomic_helper.h>
>  #include <drm/drm_gem_dma_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_managed.h>
>  #include <drm/drm_mipi_dbi.h>
>  #include <drm/drm_rect.h>
> @@ -76,9 +77,9 @@ static inline int ili9225_command(struct mipi_dbi *dbi, u8 cmd, u16 data)
>  	return mipi_dbi_command_buf(dbi, cmd, par, 2);
>  }
>  
> -static void ili9225_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
> +static void ili9225_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
> +			     struct drm_rect *rect)
>  {
> -	struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
>  	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
>  	unsigned int height = rect->y2 - rect->y1;
>  	unsigned int width = rect->x2 - rect->x1;
> @@ -100,11 +101,11 @@ static void ili9225_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>  	if (!dbi->dc || !full || swap ||
>  	    fb->format->format == DRM_FORMAT_XRGB8888) {
>  		tr = dbidev->tx_buf;
> -		ret = mipi_dbi_buf_copy(dbidev->tx_buf, fb, rect, swap);
> +		ret = mipi_dbi_buf_copy(tr, src, fb, rect, swap);
>  		if (ret)
>  			goto err_msg;
>  	} else {
> -		tr = dma_obj->vaddr;
> +		tr = src->vaddr; /* TODO: Use mapping abstraction properly */
>  	}
>  
>  	switch (dbidev->rotation) {
> @@ -162,14 +163,24 @@ static void ili9225_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>  static void ili9225_pipe_update(struct drm_simple_display_pipe *pipe,
>  				struct drm_plane_state *old_state)
>  {
> +	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> +	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>  	struct drm_plane_state *state = pipe->plane.state;
> +	struct drm_framebuffer *fb = state->fb;
>  	struct drm_rect rect;
> +	int ret;
>  
>  	if (!pipe->crtc.state->active)
>  		return;
>  
> +	ret = drm_gem_fb_vmap(fb, map, data);
> +	if (ret)
> +		return;
> +
>  	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
> -		ili9225_fb_dirty(state->fb, &rect);
> +		ili9225_fb_dirty(&data[0], fb, &rect);
> +
> +	drm_gem_fb_vunmap(fb, map);
>  }
>  
>  static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
> @@ -186,6 +197,8 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
>  		.y1 = 0,
>  		.y2 = fb->height,
>  	};
> +	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> +	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>  	int ret, idx;
>  	u8 am_id;
>  
> @@ -276,7 +289,13 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
>  
>  	ili9225_command(dbi, ILI9225_DISPLAY_CONTROL_1, 0x1017);
>  
> -	ili9225_fb_dirty(fb, &rect);
> +	ret = drm_gem_fb_vmap(fb, map, data);
> +	if (ret)
> +		goto out_exit;
> +
> +	ili9225_fb_dirty(&data[0], fb, &rect);
> +
> +	drm_gem_fb_vunmap(fb, map);
>  out_exit:
>  	drm_dev_exit(idx);
>  }
> diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c
> index 6bdd23e2a47c7..e773b1f2fd5f3 100644
> --- a/drivers/gpu/drm/tiny/st7586.c
> +++ b/drivers/gpu/drm/tiny/st7586.c
> @@ -92,25 +92,24 @@ static void st7586_xrgb8888_to_gray332(u8 *dst, void *vaddr,
>  	kfree(buf);
>  }
>  
> -static int st7586_buf_copy(void *dst, struct drm_framebuffer *fb,
> +static int st7586_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *fb,
>  			   struct drm_rect *clip)
>  {
> -	struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
> -	void *src = dma_obj->vaddr;
> -	int ret = 0;
> +	int ret;
>  
>  	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>  	if (ret)
>  		return ret;
>  
> -	st7586_xrgb8888_to_gray332(dst, src, fb, clip);
> +	st7586_xrgb8888_to_gray332(dst, src->vaddr, fb, clip);
>  
>  	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>  
>  	return 0;
>  }
>  
> -static void st7586_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
> +static void st7586_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
> +			    struct drm_rect *rect)
>  {
>  	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
>  	struct mipi_dbi *dbi = &dbidev->dbi;
> @@ -125,7 +124,7 @@ static void st7586_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>  
>  	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
>  
> -	ret = st7586_buf_copy(dbidev->tx_buf, fb, rect);
> +	ret = st7586_buf_copy(dbidev->tx_buf, src, fb, rect);
>  	if (ret)
>  		goto err_msg;
>  
> @@ -154,13 +153,19 @@ static void st7586_pipe_update(struct drm_simple_display_pipe *pipe,
>  			       struct drm_plane_state *old_state)
>  {
>  	struct drm_plane_state *state = pipe->plane.state;
> +	struct drm_framebuffer *fb = state->fb;
> +	struct drm_gem_dma_object *dma_obj;
> +	struct iosys_map src;
>  	struct drm_rect rect;
>  
>  	if (!pipe->crtc.state->active)
>  		return;
>  
> +	dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
> +	iosys_map_set_vaddr(&src, dma_obj->vaddr);
> +

You use drm_gem_fb_vmap() in the other places but here you access the
object directly (and in the next hunk), but again not so important since
it goes away in a later patch.

With the comments considered:

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

>  	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
> -		st7586_fb_dirty(state->fb, &rect);
> +		st7586_fb_dirty(&src, fb, &rect);
>  }
>  
>  static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
> @@ -176,6 +181,8 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
>  		.y1 = 0,
>  		.y2 = fb->height,
>  	};
> +	struct drm_gem_dma_object *dma_obj;
> +	struct iosys_map src;
>  	int idx, ret;
>  	u8 addr_mode;
>  
> @@ -235,7 +242,10 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
>  
>  	msleep(100);
>  
> -	st7586_fb_dirty(fb, &rect);
> +	dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
> +	iosys_map_set_vaddr(&src, dma_obj->vaddr);
> +
> +	st7586_fb_dirty(&src, fb, &rect);
>  
>  	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
>  out_exit:
> diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
> index 8c4ea7956d61d..36ac8495566b0 100644
> --- a/include/drm/drm_mipi_dbi.h
> +++ b/include/drm/drm_mipi_dbi.h
> @@ -13,9 +13,10 @@
>  #include <drm/drm_simple_kms_helper.h>
>  
>  struct drm_rect;
> -struct spi_device;
>  struct gpio_desc;
> +struct iosys_map;
>  struct regulator;
> +struct spi_device;
>  
>  /**
>   * struct mipi_dbi - MIPI DBI interface
> @@ -176,8 +177,9 @@ int mipi_dbi_command_read(struct mipi_dbi *dbi, u8 cmd, u8 *val);
>  int mipi_dbi_command_buf(struct mipi_dbi *dbi, u8 cmd, u8 *data, size_t len);
>  int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, const u8 *data,
>  			      size_t len);
> -int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
> +int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *fb,
>  		      struct drm_rect *clip, bool swap);
> +
>  /**
>   * mipi_dbi_command - MIPI DCS command with optional parameter(s)
>   * @dbi: MIPI DBI structure

Re: [PATCH 6/8] drm/mipi-dbi: Support shadow-plane state

[Noralf Trønnes]

Den 21.11.2022 11.45, skrev Thomas Zimmermann:
> Introduce struct drm_mipi_dbi_plane_state that contains state related to
> MIPI DBI. It currently only inherits from struct drm_shadow_plane_state,
> so that MIPI DBI drivers can use the vmap'ed GEM-buffer memory. Implement
> state helpers, the {begin,end}_fb_access helpers and wire up everything.
> 
> With this commit, MIPI DBI drivers can access the GEM object's memory
> that is provided by shadow-plane state. The actual changes to drivers
> are implemented separately. The new struct drm_mipi_dbi_plane was added
> to avoid exposing struct drm_shadow_plane_state directly. The latter is
> a detail of the actual implementation and having it in the MIPI driver
> interface seems unintuitive.

I don't understand this reasoning. The update functions still uses
drm_shadow_plane_state in order to access ->data[0]. If you want to
avoid exposing it, can't you add an accessor function for ->data[0]
instead? That would actually be useful to me at least since when I first
read the shadow plane code I didn't understand what data really was
referring to. fb_map would have been more clear to me.

Noralf.

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_mipi_dbi.c | 113 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/tiny/ili9225.c |   5 ++
>  drivers/gpu/drm/tiny/st7586.c  |   5 ++
>  include/drm/drm_mipi_dbi.h     |  30 ++++++++-
>  4 files changed, 152 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
> index 40e59a3a6481e..3030344d25b48 100644
> --- a/drivers/gpu/drm/drm_mipi_dbi.c
> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
> @@ -436,6 +436,119 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
>  }
>  EXPORT_SYMBOL(mipi_dbi_pipe_disable);
>  
> +/**
> + * mipi_dbi_pipe_begin_fb_access - MIPI DBI pipe begin-access helper
> + * @pipe: Display pipe
> + * @plane_state: Plane state
> + *
> + * This function implements struct &drm_simple_display_funcs.begin_fb_access.
> + *
> + * See drm_gem_begin_shadow_fb_access() for details and mipi_dbi_pipe_cleanup_fb()
> + * for cleanup.
> + *
> + * Returns:
> + * 0 on success, or a negative errno code otherwise.
> + */
> +int mipi_dbi_pipe_begin_fb_access(struct drm_simple_display_pipe *pipe,
> +				  struct drm_plane_state *plane_state)
> +{
> +	return drm_gem_begin_shadow_fb_access(&pipe->plane, plane_state);
> +}
> +EXPORT_SYMBOL(mipi_dbi_pipe_begin_fb_access);
> +
> +/**
> + * mipi_dbi_pipe_end_fb_access - MIPI DBI pipe end-access helper
> + * @pipe: Display pipe
> + * @plane_state: Plane state
> + *
> + * This function implements struct &drm_simple_display_funcs.end_fb_access.
> + *
> + * See mipi_dbi_pipe_begin_fb_access().
> + */
> +void mipi_dbi_pipe_end_fb_access(struct drm_simple_display_pipe *pipe,
> +				 struct drm_plane_state *plane_state)
> +{
> +	drm_gem_end_shadow_fb_access(&pipe->plane, plane_state);
> +}
> +EXPORT_SYMBOL(mipi_dbi_pipe_end_fb_access);
> +
> +/**
> + * mipi_dbi_pipe_reset_plane - MIPI DBI plane-reset helper
> + * @pipe: Display pipe
> + *
> + * This function implements struct &drm_simple_display_funcs.reset_plane
> + * for MIPI DBI planes.
> + */
> +void mipi_dbi_pipe_reset_plane(struct drm_simple_display_pipe *pipe)
> +{
> +	struct drm_plane *plane = &pipe->plane;
> +	struct mipi_dbi_plane_state *mipi_dbi_plane_state;
> +
> +	if (plane->state) {
> +		mipi_dbi_pipe_destroy_plane_state(pipe, plane->state);
> +		plane->state = NULL; /* must be set to NULL here */
> +	}
> +
> +	mipi_dbi_plane_state = kzalloc(sizeof(*mipi_dbi_plane_state), GFP_KERNEL);
> +	if (!mipi_dbi_plane_state)
> +		return;
> +	__drm_gem_reset_shadow_plane(plane, &mipi_dbi_plane_state->shadow_plane_state);
> +}
> +EXPORT_SYMBOL(mipi_dbi_pipe_reset_plane);
> +
> +/**
> + * mipi_dbi_pipe_duplicate_plane_state - duplicates MIPI DBI plane state
> + * @pipe: Display pipe
> + *
> + * This function implements struct &drm_simple_display_funcs.duplicate_plane_state
> + * for MIPI DBI planes.
> + *
> + * See drm_gem_duplicate_shadow_plane_state() for additional details.
> + *
> + * Returns:
> + * A pointer to a new plane state on success, or NULL otherwise.
> + */
> +struct drm_plane_state *mipi_dbi_pipe_duplicate_plane_state(struct drm_simple_display_pipe *pipe)
> +{
> +	struct drm_plane *plane = &pipe->plane;
> +	struct drm_plane_state *plane_state = plane->state;
> +	struct mipi_dbi_plane_state *new_mipi_dbi_plane_state;
> +	struct drm_shadow_plane_state *new_shadow_plane_state;
> +
> +	if (!plane_state)
> +		return NULL;
> +
> +	new_mipi_dbi_plane_state = kzalloc(sizeof(*new_mipi_dbi_plane_state), GFP_KERNEL);
> +	if (!new_mipi_dbi_plane_state)
> +		return NULL;
> +	new_shadow_plane_state = &new_mipi_dbi_plane_state->shadow_plane_state;
> +
> +	__drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
> +
> +	return &new_shadow_plane_state->base;
> +}
> +EXPORT_SYMBOL(mipi_dbi_pipe_duplicate_plane_state);
> +
> +/**
> + * mipi_dbi_pipe_destroy_plane_state - cleans up MIPI DBI plane state
> + * @pipe: Display pipe
> + * @plane_state: Plane state
> + *
> + * This function implements struct drm_simple_display_funcs.destroy_plane_state
> + * for MIPI DBI planes.
> + *
> + * See drm_gem_destroy_shadow_plane_state() for additional details.
> + */
> +void mipi_dbi_pipe_destroy_plane_state(struct drm_simple_display_pipe *pipe,
> +				       struct drm_plane_state *plane_state)
> +{
> +	struct mipi_dbi_plane_state *mipi_dbi_plane_state = to_mipi_dbi_plane_state(plane_state);
> +
> +	__drm_gem_destroy_shadow_plane_state(&mipi_dbi_plane_state->shadow_plane_state);
> +	kfree(mipi_dbi_plane_state);
> +}
> +EXPORT_SYMBOL(mipi_dbi_pipe_destroy_plane_state);
> +
>  static int mipi_dbi_connector_get_modes(struct drm_connector *connector)
>  {
>  	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(connector->dev);
> diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c
> index 0115c4090f9e7..9e55ce28b4552 100644
> --- a/drivers/gpu/drm/tiny/ili9225.c
> +++ b/drivers/gpu/drm/tiny/ili9225.c
> @@ -349,6 +349,11 @@ static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = {
>  	.enable		= ili9225_pipe_enable,
>  	.disable	= ili9225_pipe_disable,
>  	.update		= ili9225_pipe_update,
> +	.begin_fb_access = mipi_dbi_pipe_begin_fb_access,
> +	.end_fb_access	= mipi_dbi_pipe_end_fb_access,
> +	.reset_plane	= mipi_dbi_pipe_reset_plane,
> +	.duplicate_plane_state = mipi_dbi_pipe_duplicate_plane_state,
> +	.destroy_plane_state = mipi_dbi_pipe_destroy_plane_state,
>  };
>  
>  static const struct drm_display_mode ili9225_mode = {
> diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c
> index e773b1f2fd5f3..76b13cefc904f 100644
> --- a/drivers/gpu/drm/tiny/st7586.c
> +++ b/drivers/gpu/drm/tiny/st7586.c
> @@ -277,6 +277,11 @@ static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
>  	.enable		= st7586_pipe_enable,
>  	.disable	= st7586_pipe_disable,
>  	.update		= st7586_pipe_update,
> +	.begin_fb_access = mipi_dbi_pipe_begin_fb_access,
> +	.end_fb_access	= mipi_dbi_pipe_end_fb_access,
> +	.reset_plane	= mipi_dbi_pipe_reset_plane,
> +	.duplicate_plane_state = mipi_dbi_pipe_duplicate_plane_state,
> +	.destroy_plane_state = mipi_dbi_pipe_destroy_plane_state,
>  };
>  
>  static const struct drm_display_mode st7586_mode = {
> diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
> index 36ac8495566b0..0213d4aae0326 100644
> --- a/include/drm/drm_mipi_dbi.h
> +++ b/include/drm/drm_mipi_dbi.h
> @@ -10,6 +10,7 @@
>  
>  #include <linux/mutex.h>
>  #include <drm/drm_device.h>
> +#include <drm/drm_gem_atomic_helper.h>
>  #include <drm/drm_simple_kms_helper.h>
>  
>  struct drm_rect;
> @@ -18,6 +19,19 @@ struct iosys_map;
>  struct regulator;
>  struct spi_device;
>  
> +/**
> + * struct mipi_dbi_plane_state - MIPI DBI plane state
> + */
> +struct mipi_dbi_plane_state {
> +	struct drm_shadow_plane_state shadow_plane_state;
> +};
> +
> +static inline struct mipi_dbi_plane_state *
> +to_mipi_dbi_plane_state(struct drm_plane_state *plane_state)
> +{
> +	return container_of(plane_state, struct mipi_dbi_plane_state, shadow_plane_state.base);
> +}
> +
>  /**
>   * struct mipi_dbi - MIPI DBI interface
>   */
> @@ -164,6 +178,15 @@ void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
>  			   struct drm_crtc_state *crtc_state,
>  			   struct drm_plane_state *plan_state);
>  void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
> +int mipi_dbi_pipe_begin_fb_access(struct drm_simple_display_pipe *pipe,
> +				  struct drm_plane_state *plane_state);
> +void mipi_dbi_pipe_end_fb_access(struct drm_simple_display_pipe *pipe,
> +				 struct drm_plane_state *plane_state);
> +void mipi_dbi_pipe_reset_plane(struct drm_simple_display_pipe *pipe);
> +struct drm_plane_state *mipi_dbi_pipe_duplicate_plane_state(struct drm_simple_display_pipe *pipe);
> +void mipi_dbi_pipe_destroy_plane_state(struct drm_simple_display_pipe *pipe,
> +				       struct drm_plane_state *plane_state);
> +
>  void mipi_dbi_hw_reset(struct mipi_dbi *dbi);
>  bool mipi_dbi_display_is_on(struct mipi_dbi *dbi);
>  int mipi_dbi_poweron_reset(struct mipi_dbi_dev *dbidev);
> @@ -223,6 +246,11 @@ static inline void mipi_dbi_debugfs_init(struct drm_minor *minor) {}
>  	.mode_valid = mipi_dbi_pipe_mode_valid, \
>  	.enable = (enable_), \
>  	.disable = mipi_dbi_pipe_disable, \
> -	.update = mipi_dbi_pipe_update
> +	.update = mipi_dbi_pipe_update, \
> +	.begin_fb_access = mipi_dbi_pipe_begin_fb_access, \
> +	.end_fb_access = mipi_dbi_pipe_end_fb_access, \
> +	.reset_plane = mipi_dbi_pipe_reset_plane, \
> +	.duplicate_plane_state = mipi_dbi_pipe_duplicate_plane_state, \
> +	.destroy_plane_state = mipi_dbi_pipe_destroy_plane_state
>  
>  #endif /* __LINUX_MIPI_DBI_H */

Re: [PATCH 8/8] drm/mipi-dbi: Move drm_dev_{enter,exit}() out from fb_dirty functions

[Noralf Trønnes]

Den 21.11.2022 11.45, skrev Thomas Zimmermann:
> Call drm_dev_enter() and drm_dev_exit() in the outer-most callbacks
> of the modesetting pipeline. If drm_dev_enter() fails, the driver can
> thus avoid unnecessary work.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Makes sense in order to make the code more readable, the other
*_fb_dirty call sites (*_enable) are already protected.

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

Re: [PATCH 0/8] drm/mipi-dbi: Convert to shadow-plane helpers

[Noralf Trønnes]

Den 21.11.2022 11.45, skrev Thomas Zimmermann:
> Convert the MIPI-DBI-based drivers to shadow-plane helpers. The
> drivers vmap/vunmap GEM buffer memory during the atomic commit.
> Shadow-plane helpers automate this process.
> 
> Patches 1 to 4 prepare the MIPI code for the change and simplify
> the restof the patchset.
> 
> Patches 5 to 7 rework the vmap code in the MIPI-DBI drivers and add
> shadow-plane helpers. Most of the affected drivers call MIPI-DBI
> helpers and get the update automatically. Only ili9225 and st7586
> require changes to their source code.
> 
> Patch 8 simplifies drm_dev_enter() and _exit(). It's not strictly
> needed, but streamlines the driver code and make sense overall.
> 
> Testing is welcome, as I don't have any hardware to test these
> changes myself.
> 

Tested-by: Noralf Trønnes <noralf@tronnes.org> # drm/tiny/mi0283qt

> Thomas Zimmermann (8):
>   drm/simple-kms: Remove drm_gem_simple_display_pipe_prepare_fb()
>   drm/ili9225: Call MIPI DBI mode_valid helper
>   drm/st7586: Call MIPI DBI mode_valid helper
>   drm/mipi-dbi: Initialize default driver functions with macro
>   drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update
>     helpers
>   drm/mipi-dbi: Support shadow-plane state
>   drm/mipi-dbi: Use shadow-plane mappings
>   drm/mipi-dbi: Move drm_dev_{enter,exit}() out from fb_dirty functions
> 
>  drivers/gpu/drm/drm_gem_atomic_helper.c      |  31 +---
>  drivers/gpu/drm/drm_mipi_dbi.c               | 175 +++++++++++++++----
>  drivers/gpu/drm/drm_simple_kms_helper.c      |   2 +-
>  drivers/gpu/drm/panel/panel-ilitek-ili9341.c |   6 +-
>  drivers/gpu/drm/tiny/hx8357d.c               |   5 +-
>  drivers/gpu/drm/tiny/ili9163.c               |   6 +-
>  drivers/gpu/drm/tiny/ili9225.c               |  42 +++--
>  drivers/gpu/drm/tiny/ili9341.c               |   5 +-
>  drivers/gpu/drm/tiny/ili9486.c               |   5 +-
>  drivers/gpu/drm/tiny/mi0283qt.c              |   5 +-
>  drivers/gpu/drm/tiny/panel-mipi-dbi.c        |   5 +-
>  drivers/gpu/drm/tiny/st7586.c                |  45 +++--
>  drivers/gpu/drm/tiny/st7735r.c               |   5 +-
>  include/drm/drm_gem_atomic_helper.h          |   2 -
>  include/drm/drm_mipi_dbi.h                   |  50 +++++-
>  include/drm/drm_plane.h                      |   4 +-
>  include/drm/drm_simple_kms_helper.h          |   4 +-
>  17 files changed, 265 insertions(+), 132 deletions(-)
> 
> 
> base-commit: b7598e2b3a3116bb5ddbf756db30a0e5dc0877ea
> prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
> prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
> prerequisite-patch-id: 3f204510fcbf9530d6540bd8e6128cce598988b6

Re: [PATCH 6/8] drm/mipi-dbi: Support shadow-plane state

[Thomas Zimmermann]

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

Hi

Am 25.11.22 um 18:48 schrieb Noralf Trønnes:
> 
> 
> Den 21.11.2022 11.45, skrev Thomas Zimmermann:
>> Introduce struct drm_mipi_dbi_plane_state that contains state related to
>> MIPI DBI. It currently only inherits from struct drm_shadow_plane_state,
>> so that MIPI DBI drivers can use the vmap'ed GEM-buffer memory. Implement
>> state helpers, the {begin,end}_fb_access helpers and wire up everything.
>>
>> With this commit, MIPI DBI drivers can access the GEM object's memory
>> that is provided by shadow-plane state. The actual changes to drivers
>> are implemented separately. The new struct drm_mipi_dbi_plane was added
>> to avoid exposing struct drm_shadow_plane_state directly. The latter is
>> a detail of the actual implementation and having it in the MIPI driver
>> interface seems unintuitive.
> 
> I don't understand this reasoning. The update functions still uses
> drm_shadow_plane_state in order to access ->data[0]. If you want to
> avoid exposing it, can't you add an accessor function for ->data[0]
> instead? That would actually be useful to me at least since when I first
> read the shadow plane code I didn't understand what data really was
> referring to. fb_map would have been more clear to me.

There's nothing wrong with accessing shadow-plane state directly. I 
simply found it non-intuitive to leave MIPI without it's own plane-state 
structure.  From the perspective of a MIPI-based driver, up-casting to a 
shadow-plane state feels arbitrary. Upcasting to a MIPI plane state 
appears logical.

Anyway, if using the shadow-plane state without the mipi plane state is 
preferred, I'll change the code accordingly.

Best regards
Thomas

> 
> Noralf.
> 
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_mipi_dbi.c | 113 +++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/tiny/ili9225.c |   5 ++
>>   drivers/gpu/drm/tiny/st7586.c  |   5 ++
>>   include/drm/drm_mipi_dbi.h     |  30 ++++++++-
>>   4 files changed, 152 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
>> index 40e59a3a6481e..3030344d25b48 100644
>> --- a/drivers/gpu/drm/drm_mipi_dbi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
>> @@ -436,6 +436,119 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
>>   }
>>   EXPORT_SYMBOL(mipi_dbi_pipe_disable);
>>   
>> +/**
>> + * mipi_dbi_pipe_begin_fb_access - MIPI DBI pipe begin-access helper
>> + * @pipe: Display pipe
>> + * @plane_state: Plane state
>> + *
>> + * This function implements struct &drm_simple_display_funcs.begin_fb_access.
>> + *
>> + * See drm_gem_begin_shadow_fb_access() for details and mipi_dbi_pipe_cleanup_fb()
>> + * for cleanup.
>> + *
>> + * Returns:
>> + * 0 on success, or a negative errno code otherwise.
>> + */
>> +int mipi_dbi_pipe_begin_fb_access(struct drm_simple_display_pipe *pipe,
>> +				  struct drm_plane_state *plane_state)
>> +{
>> +	return drm_gem_begin_shadow_fb_access(&pipe->plane, plane_state);
>> +}
>> +EXPORT_SYMBOL(mipi_dbi_pipe_begin_fb_access);
>> +
>> +/**
>> + * mipi_dbi_pipe_end_fb_access - MIPI DBI pipe end-access helper
>> + * @pipe: Display pipe
>> + * @plane_state: Plane state
>> + *
>> + * This function implements struct &drm_simple_display_funcs.end_fb_access.
>> + *
>> + * See mipi_dbi_pipe_begin_fb_access().
>> + */
>> +void mipi_dbi_pipe_end_fb_access(struct drm_simple_display_pipe *pipe,
>> +				 struct drm_plane_state *plane_state)
>> +{
>> +	drm_gem_end_shadow_fb_access(&pipe->plane, plane_state);
>> +}
>> +EXPORT_SYMBOL(mipi_dbi_pipe_end_fb_access);
>> +
>> +/**
>> + * mipi_dbi_pipe_reset_plane - MIPI DBI plane-reset helper
>> + * @pipe: Display pipe
>> + *
>> + * This function implements struct &drm_simple_display_funcs.reset_plane
>> + * for MIPI DBI planes.
>> + */
>> +void mipi_dbi_pipe_reset_plane(struct drm_simple_display_pipe *pipe)
>> +{
>> +	struct drm_plane *plane = &pipe->plane;
>> +	struct mipi_dbi_plane_state *mipi_dbi_plane_state;
>> +
>> +	if (plane->state) {
>> +		mipi_dbi_pipe_destroy_plane_state(pipe, plane->state);
>> +		plane->state = NULL; /* must be set to NULL here */
>> +	}
>> +
>> +	mipi_dbi_plane_state = kzalloc(sizeof(*mipi_dbi_plane_state), GFP_KERNEL);
>> +	if (!mipi_dbi_plane_state)
>> +		return;
>> +	__drm_gem_reset_shadow_plane(plane, &mipi_dbi_plane_state->shadow_plane_state);
>> +}
>> +EXPORT_SYMBOL(mipi_dbi_pipe_reset_plane);
>> +
>> +/**
>> + * mipi_dbi_pipe_duplicate_plane_state - duplicates MIPI DBI plane state
>> + * @pipe: Display pipe
>> + *
>> + * This function implements struct &drm_simple_display_funcs.duplicate_plane_state
>> + * for MIPI DBI planes.
>> + *
>> + * See drm_gem_duplicate_shadow_plane_state() for additional details.
>> + *
>> + * Returns:
>> + * A pointer to a new plane state on success, or NULL otherwise.
>> + */
>> +struct drm_plane_state *mipi_dbi_pipe_duplicate_plane_state(struct drm_simple_display_pipe *pipe)
>> +{
>> +	struct drm_plane *plane = &pipe->plane;
>> +	struct drm_plane_state *plane_state = plane->state;
>> +	struct mipi_dbi_plane_state *new_mipi_dbi_plane_state;
>> +	struct drm_shadow_plane_state *new_shadow_plane_state;
>> +
>> +	if (!plane_state)
>> +		return NULL;
>> +
>> +	new_mipi_dbi_plane_state = kzalloc(sizeof(*new_mipi_dbi_plane_state), GFP_KERNEL);
>> +	if (!new_mipi_dbi_plane_state)
>> +		return NULL;
>> +	new_shadow_plane_state = &new_mipi_dbi_plane_state->shadow_plane_state;
>> +
>> +	__drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
>> +
>> +	return &new_shadow_plane_state->base;
>> +}
>> +EXPORT_SYMBOL(mipi_dbi_pipe_duplicate_plane_state);
>> +
>> +/**
>> + * mipi_dbi_pipe_destroy_plane_state - cleans up MIPI DBI plane state
>> + * @pipe: Display pipe
>> + * @plane_state: Plane state
>> + *
>> + * This function implements struct drm_simple_display_funcs.destroy_plane_state
>> + * for MIPI DBI planes.
>> + *
>> + * See drm_gem_destroy_shadow_plane_state() for additional details.
>> + */
>> +void mipi_dbi_pipe_destroy_plane_state(struct drm_simple_display_pipe *pipe,
>> +				       struct drm_plane_state *plane_state)
>> +{
>> +	struct mipi_dbi_plane_state *mipi_dbi_plane_state = to_mipi_dbi_plane_state(plane_state);
>> +
>> +	__drm_gem_destroy_shadow_plane_state(&mipi_dbi_plane_state->shadow_plane_state);
>> +	kfree(mipi_dbi_plane_state);
>> +}
>> +EXPORT_SYMBOL(mipi_dbi_pipe_destroy_plane_state);
>> +
>>   static int mipi_dbi_connector_get_modes(struct drm_connector *connector)
>>   {
>>   	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(connector->dev);
>> diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c
>> index 0115c4090f9e7..9e55ce28b4552 100644
>> --- a/drivers/gpu/drm/tiny/ili9225.c
>> +++ b/drivers/gpu/drm/tiny/ili9225.c
>> @@ -349,6 +349,11 @@ static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = {
>>   	.enable		= ili9225_pipe_enable,
>>   	.disable	= ili9225_pipe_disable,
>>   	.update		= ili9225_pipe_update,
>> +	.begin_fb_access = mipi_dbi_pipe_begin_fb_access,
>> +	.end_fb_access	= mipi_dbi_pipe_end_fb_access,
>> +	.reset_plane	= mipi_dbi_pipe_reset_plane,
>> +	.duplicate_plane_state = mipi_dbi_pipe_duplicate_plane_state,
>> +	.destroy_plane_state = mipi_dbi_pipe_destroy_plane_state,
>>   };
>>   
>>   static const struct drm_display_mode ili9225_mode = {
>> diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c
>> index e773b1f2fd5f3..76b13cefc904f 100644
>> --- a/drivers/gpu/drm/tiny/st7586.c
>> +++ b/drivers/gpu/drm/tiny/st7586.c
>> @@ -277,6 +277,11 @@ static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
>>   	.enable		= st7586_pipe_enable,
>>   	.disable	= st7586_pipe_disable,
>>   	.update		= st7586_pipe_update,
>> +	.begin_fb_access = mipi_dbi_pipe_begin_fb_access,
>> +	.end_fb_access	= mipi_dbi_pipe_end_fb_access,
>> +	.reset_plane	= mipi_dbi_pipe_reset_plane,
>> +	.duplicate_plane_state = mipi_dbi_pipe_duplicate_plane_state,
>> +	.destroy_plane_state = mipi_dbi_pipe_destroy_plane_state,
>>   };
>>   
>>   static const struct drm_display_mode st7586_mode = {
>> diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
>> index 36ac8495566b0..0213d4aae0326 100644
>> --- a/include/drm/drm_mipi_dbi.h
>> +++ b/include/drm/drm_mipi_dbi.h
>> @@ -10,6 +10,7 @@
>>   
>>   #include <linux/mutex.h>
>>   #include <drm/drm_device.h>
>> +#include <drm/drm_gem_atomic_helper.h>
>>   #include <drm/drm_simple_kms_helper.h>
>>   
>>   struct drm_rect;
>> @@ -18,6 +19,19 @@ struct iosys_map;
>>   struct regulator;
>>   struct spi_device;
>>   
>> +/**
>> + * struct mipi_dbi_plane_state - MIPI DBI plane state
>> + */
>> +struct mipi_dbi_plane_state {
>> +	struct drm_shadow_plane_state shadow_plane_state;
>> +};
>> +
>> +static inline struct mipi_dbi_plane_state *
>> +to_mipi_dbi_plane_state(struct drm_plane_state *plane_state)
>> +{
>> +	return container_of(plane_state, struct mipi_dbi_plane_state, shadow_plane_state.base);
>> +}
>> +
>>   /**
>>    * struct mipi_dbi - MIPI DBI interface
>>    */
>> @@ -164,6 +178,15 @@ void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
>>   			   struct drm_crtc_state *crtc_state,
>>   			   struct drm_plane_state *plan_state);
>>   void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
>> +int mipi_dbi_pipe_begin_fb_access(struct drm_simple_display_pipe *pipe,
>> +				  struct drm_plane_state *plane_state);
>> +void mipi_dbi_pipe_end_fb_access(struct drm_simple_display_pipe *pipe,
>> +				 struct drm_plane_state *plane_state);
>> +void mipi_dbi_pipe_reset_plane(struct drm_simple_display_pipe *pipe);
>> +struct drm_plane_state *mipi_dbi_pipe_duplicate_plane_state(struct drm_simple_display_pipe *pipe);
>> +void mipi_dbi_pipe_destroy_plane_state(struct drm_simple_display_pipe *pipe,
>> +				       struct drm_plane_state *plane_state);
>> +
>>   void mipi_dbi_hw_reset(struct mipi_dbi *dbi);
>>   bool mipi_dbi_display_is_on(struct mipi_dbi *dbi);
>>   int mipi_dbi_poweron_reset(struct mipi_dbi_dev *dbidev);
>> @@ -223,6 +246,11 @@ static inline void mipi_dbi_debugfs_init(struct drm_minor *minor) {}
>>   	.mode_valid = mipi_dbi_pipe_mode_valid, \
>>   	.enable = (enable_), \
>>   	.disable = mipi_dbi_pipe_disable, \
>> -	.update = mipi_dbi_pipe_update
>> +	.update = mipi_dbi_pipe_update, \
>> +	.begin_fb_access = mipi_dbi_pipe_begin_fb_access, \
>> +	.end_fb_access = mipi_dbi_pipe_end_fb_access, \
>> +	.reset_plane = mipi_dbi_pipe_reset_plane, \
>> +	.duplicate_plane_state = mipi_dbi_pipe_duplicate_plane_state, \
>> +	.destroy_plane_state = mipi_dbi_pipe_destroy_plane_state
>>   
>>   #endif /* __LINUX_MIPI_DBI_H */

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

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

Re: [PATCH 6/8] drm/mipi-dbi: Support shadow-plane state

[Noralf Trønnes]

Den 28.11.2022 13.10, skrev Thomas Zimmermann:
> Hi
> 
> Am 25.11.22 um 18:48 schrieb Noralf Trønnes:
>>
>>
>> Den 21.11.2022 11.45, skrev Thomas Zimmermann:
>>> Introduce struct drm_mipi_dbi_plane_state that contains state related to
>>> MIPI DBI. It currently only inherits from struct drm_shadow_plane_state,
>>> so that MIPI DBI drivers can use the vmap'ed GEM-buffer memory.
>>> Implement
>>> state helpers, the {begin,end}_fb_access helpers and wire up everything.
>>>
>>> With this commit, MIPI DBI drivers can access the GEM object's memory
>>> that is provided by shadow-plane state. The actual changes to drivers
>>> are implemented separately. The new struct drm_mipi_dbi_plane was added
>>> to avoid exposing struct drm_shadow_plane_state directly. The latter is
>>> a detail of the actual implementation and having it in the MIPI driver
>>> interface seems unintuitive.
>>
>> I don't understand this reasoning. The update functions still uses
>> drm_shadow_plane_state in order to access ->data[0]. If you want to
>> avoid exposing it, can't you add an accessor function for ->data[0]
>> instead? That would actually be useful to me at least since when I first
>> read the shadow plane code I didn't understand what data really was
>> referring to. fb_map would have been more clear to me.
> 
> There's nothing wrong with accessing shadow-plane state directly. I
> simply found it non-intuitive to leave MIPI without it's own plane-state
> structure.  From the perspective of a MIPI-based driver, up-casting to a
> shadow-plane state feels arbitrary. Upcasting to a MIPI plane state
> appears logical.
> 
> Anyway, if using the shadow-plane state without the mipi plane state is
> preferred, I'll change the code accordingly.
> 

I prefer to drop this. When I see the subclassed plane state without any
additional state members I'm left wondering why this is done and I start
looking for a TODO.

Noralf.

> Best regards
> Thomas
> 
>>
>> Noralf.
>>
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>   drivers/gpu/drm/drm_mipi_dbi.c | 113 +++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/tiny/ili9225.c |   5 ++
>>>   drivers/gpu/drm/tiny/st7586.c  |   5 ++
>>>   include/drm/drm_mipi_dbi.h     |  30 ++++++++-
>>>   4 files changed, 152 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c
>>> b/drivers/gpu/drm/drm_mipi_dbi.c
>>> index 40e59a3a6481e..3030344d25b48 100644
>>> --- a/drivers/gpu/drm/drm_mipi_dbi.c
>>> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
>>> @@ -436,6 +436,119 @@ void mipi_dbi_pipe_disable(struct
>>> drm_simple_display_pipe *pipe)
>>>   }
>>>   EXPORT_SYMBOL(mipi_dbi_pipe_disable);
>>>   +/**
>>> + * mipi_dbi_pipe_begin_fb_access - MIPI DBI pipe begin-access helper
>>> + * @pipe: Display pipe
>>> + * @plane_state: Plane state
>>> + *
>>> + * This function implements struct
>>> &drm_simple_display_funcs.begin_fb_access.
>>> + *
>>> + * See drm_gem_begin_shadow_fb_access() for details and
>>> mipi_dbi_pipe_cleanup_fb()
>>> + * for cleanup.
>>> + *
>>> + * Returns:
>>> + * 0 on success, or a negative errno code otherwise.
>>> + */
>>> +int mipi_dbi_pipe_begin_fb_access(struct drm_simple_display_pipe *pipe,
>>> +                  struct drm_plane_state *plane_state)
>>> +{
>>> +    return drm_gem_begin_shadow_fb_access(&pipe->plane, plane_state);
>>> +}
>>> +EXPORT_SYMBOL(mipi_dbi_pipe_begin_fb_access);
>>> +
>>> +/**
>>> + * mipi_dbi_pipe_end_fb_access - MIPI DBI pipe end-access helper
>>> + * @pipe: Display pipe
>>> + * @plane_state: Plane state
>>> + *
>>> + * This function implements struct
>>> &drm_simple_display_funcs.end_fb_access.
>>> + *
>>> + * See mipi_dbi_pipe_begin_fb_access().
>>> + */
>>> +void mipi_dbi_pipe_end_fb_access(struct drm_simple_display_pipe *pipe,
>>> +                 struct drm_plane_state *plane_state)
>>> +{
>>> +    drm_gem_end_shadow_fb_access(&pipe->plane, plane_state);
>>> +}
>>> +EXPORT_SYMBOL(mipi_dbi_pipe_end_fb_access);
>>> +
>>> +/**
>>> + * mipi_dbi_pipe_reset_plane - MIPI DBI plane-reset helper
>>> + * @pipe: Display pipe
>>> + *
>>> + * This function implements struct
>>> &drm_simple_display_funcs.reset_plane
>>> + * for MIPI DBI planes.
>>> + */
>>> +void mipi_dbi_pipe_reset_plane(struct drm_simple_display_pipe *pipe)
>>> +{
>>> +    struct drm_plane *plane = &pipe->plane;
>>> +    struct mipi_dbi_plane_state *mipi_dbi_plane_state;
>>> +
>>> +    if (plane->state) {
>>> +        mipi_dbi_pipe_destroy_plane_state(pipe, plane->state);
>>> +        plane->state = NULL; /* must be set to NULL here */
>>> +    }
>>> +
>>> +    mipi_dbi_plane_state = kzalloc(sizeof(*mipi_dbi_plane_state),
>>> GFP_KERNEL);
>>> +    if (!mipi_dbi_plane_state)
>>> +        return;
>>> +    __drm_gem_reset_shadow_plane(plane,
>>> &mipi_dbi_plane_state->shadow_plane_state);
>>> +}
>>> +EXPORT_SYMBOL(mipi_dbi_pipe_reset_plane);
>>> +
>>> +/**
>>> + * mipi_dbi_pipe_duplicate_plane_state - duplicates MIPI DBI plane
>>> state
>>> + * @pipe: Display pipe
>>> + *
>>> + * This function implements struct
>>> &drm_simple_display_funcs.duplicate_plane_state
>>> + * for MIPI DBI planes.
>>> + *
>>> + * See drm_gem_duplicate_shadow_plane_state() for additional details.
>>> + *
>>> + * Returns:
>>> + * A pointer to a new plane state on success, or NULL otherwise.
>>> + */
>>> +struct drm_plane_state *mipi_dbi_pipe_duplicate_plane_state(struct
>>> drm_simple_display_pipe *pipe)
>>> +{
>>> +    struct drm_plane *plane = &pipe->plane;
>>> +    struct drm_plane_state *plane_state = plane->state;
>>> +    struct mipi_dbi_plane_state *new_mipi_dbi_plane_state;
>>> +    struct drm_shadow_plane_state *new_shadow_plane_state;
>>> +
>>> +    if (!plane_state)
>>> +        return NULL;
>>> +
>>> +    new_mipi_dbi_plane_state =
>>> kzalloc(sizeof(*new_mipi_dbi_plane_state), GFP_KERNEL);
>>> +    if (!new_mipi_dbi_plane_state)
>>> +        return NULL;
>>> +    new_shadow_plane_state =
>>> &new_mipi_dbi_plane_state->shadow_plane_state;
>>> +
>>> +    __drm_gem_duplicate_shadow_plane_state(plane,
>>> new_shadow_plane_state);
>>> +
>>> +    return &new_shadow_plane_state->base;
>>> +}
>>> +EXPORT_SYMBOL(mipi_dbi_pipe_duplicate_plane_state);
>>> +
>>> +/**
>>> + * mipi_dbi_pipe_destroy_plane_state - cleans up MIPI DBI plane state
>>> + * @pipe: Display pipe
>>> + * @plane_state: Plane state
>>> + *
>>> + * This function implements struct
>>> drm_simple_display_funcs.destroy_plane_state
>>> + * for MIPI DBI planes.
>>> + *
>>> + * See drm_gem_destroy_shadow_plane_state() for additional details.
>>> + */
>>> +void mipi_dbi_pipe_destroy_plane_state(struct
>>> drm_simple_display_pipe *pipe,
>>> +                       struct drm_plane_state *plane_state)
>>> +{
>>> +    struct mipi_dbi_plane_state *mipi_dbi_plane_state =
>>> to_mipi_dbi_plane_state(plane_state);
>>> +
>>> +   
>>> __drm_gem_destroy_shadow_plane_state(&mipi_dbi_plane_state->shadow_plane_state);
>>> +    kfree(mipi_dbi_plane_state);
>>> +}
>>> +EXPORT_SYMBOL(mipi_dbi_pipe_destroy_plane_state);
>>> +
>>>   static int mipi_dbi_connector_get_modes(struct drm_connector
>>> *connector)
>>>   {
>>>       struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(connector->dev);
>>> diff --git a/drivers/gpu/drm/tiny/ili9225.c
>>> b/drivers/gpu/drm/tiny/ili9225.c
>>> index 0115c4090f9e7..9e55ce28b4552 100644
>>> --- a/drivers/gpu/drm/tiny/ili9225.c
>>> +++ b/drivers/gpu/drm/tiny/ili9225.c
>>> @@ -349,6 +349,11 @@ static const struct
>>> drm_simple_display_pipe_funcs ili9225_pipe_funcs = {
>>>       .enable        = ili9225_pipe_enable,
>>>       .disable    = ili9225_pipe_disable,
>>>       .update        = ili9225_pipe_update,
>>> +    .begin_fb_access = mipi_dbi_pipe_begin_fb_access,
>>> +    .end_fb_access    = mipi_dbi_pipe_end_fb_access,
>>> +    .reset_plane    = mipi_dbi_pipe_reset_plane,
>>> +    .duplicate_plane_state = mipi_dbi_pipe_duplicate_plane_state,
>>> +    .destroy_plane_state = mipi_dbi_pipe_destroy_plane_state,
>>>   };
>>>     static const struct drm_display_mode ili9225_mode = {
>>> diff --git a/drivers/gpu/drm/tiny/st7586.c
>>> b/drivers/gpu/drm/tiny/st7586.c
>>> index e773b1f2fd5f3..76b13cefc904f 100644
>>> --- a/drivers/gpu/drm/tiny/st7586.c
>>> +++ b/drivers/gpu/drm/tiny/st7586.c
>>> @@ -277,6 +277,11 @@ static const struct
>>> drm_simple_display_pipe_funcs st7586_pipe_funcs = {
>>>       .enable        = st7586_pipe_enable,
>>>       .disable    = st7586_pipe_disable,
>>>       .update        = st7586_pipe_update,
>>> +    .begin_fb_access = mipi_dbi_pipe_begin_fb_access,
>>> +    .end_fb_access    = mipi_dbi_pipe_end_fb_access,
>>> +    .reset_plane    = mipi_dbi_pipe_reset_plane,
>>> +    .duplicate_plane_state = mipi_dbi_pipe_duplicate_plane_state,
>>> +    .destroy_plane_state = mipi_dbi_pipe_destroy_plane_state,
>>>   };
>>>     static const struct drm_display_mode st7586_mode = {
>>> diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
>>> index 36ac8495566b0..0213d4aae0326 100644
>>> --- a/include/drm/drm_mipi_dbi.h
>>> +++ b/include/drm/drm_mipi_dbi.h
>>> @@ -10,6 +10,7 @@
>>>     #include <linux/mutex.h>
>>>   #include <drm/drm_device.h>
>>> +#include <drm/drm_gem_atomic_helper.h>
>>>   #include <drm/drm_simple_kms_helper.h>
>>>     struct drm_rect;
>>> @@ -18,6 +19,19 @@ struct iosys_map;
>>>   struct regulator;
>>>   struct spi_device;
>>>   +/**
>>> + * struct mipi_dbi_plane_state - MIPI DBI plane state
>>> + */
>>> +struct mipi_dbi_plane_state {
>>> +    struct drm_shadow_plane_state shadow_plane_state;
>>> +};
>>> +
>>> +static inline struct mipi_dbi_plane_state *
>>> +to_mipi_dbi_plane_state(struct drm_plane_state *plane_state)
>>> +{
>>> +    return container_of(plane_state, struct mipi_dbi_plane_state,
>>> shadow_plane_state.base);
>>> +}
>>> +
>>>   /**
>>>    * struct mipi_dbi - MIPI DBI interface
>>>    */
>>> @@ -164,6 +178,15 @@ void mipi_dbi_enable_flush(struct mipi_dbi_dev
>>> *dbidev,
>>>                  struct drm_crtc_state *crtc_state,
>>>                  struct drm_plane_state *plan_state);
>>>   void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
>>> +int mipi_dbi_pipe_begin_fb_access(struct drm_simple_display_pipe *pipe,
>>> +                  struct drm_plane_state *plane_state);
>>> +void mipi_dbi_pipe_end_fb_access(struct drm_simple_display_pipe *pipe,
>>> +                 struct drm_plane_state *plane_state);
>>> +void mipi_dbi_pipe_reset_plane(struct drm_simple_display_pipe *pipe);
>>> +struct drm_plane_state *mipi_dbi_pipe_duplicate_plane_state(struct
>>> drm_simple_display_pipe *pipe);
>>> +void mipi_dbi_pipe_destroy_plane_state(struct
>>> drm_simple_display_pipe *pipe,
>>> +                       struct drm_plane_state *plane_state);
>>> +
>>>   void mipi_dbi_hw_reset(struct mipi_dbi *dbi);
>>>   bool mipi_dbi_display_is_on(struct mipi_dbi *dbi);
>>>   int mipi_dbi_poweron_reset(struct mipi_dbi_dev *dbidev);
>>> @@ -223,6 +246,11 @@ static inline void mipi_dbi_debugfs_init(struct
>>> drm_minor *minor) {}
>>>       .mode_valid = mipi_dbi_pipe_mode_valid, \
>>>       .enable = (enable_), \
>>>       .disable = mipi_dbi_pipe_disable, \
>>> -    .update = mipi_dbi_pipe_update
>>> +    .update = mipi_dbi_pipe_update, \
>>> +    .begin_fb_access = mipi_dbi_pipe_begin_fb_access, \
>>> +    .end_fb_access = mipi_dbi_pipe_end_fb_access, \
>>> +    .reset_plane = mipi_dbi_pipe_reset_plane, \
>>> +    .duplicate_plane_state = mipi_dbi_pipe_duplicate_plane_state, \
>>> +    .destroy_plane_state = mipi_dbi_pipe_destroy_plane_state
>>>     #endif /* __LINUX_MIPI_DBI_H */
> 

Re: [PATCH 7/8] drm/mipi-dbi: Use shadow-plane mappings

[Noralf Trønnes]

Den 21.11.2022 11.45, skrev Thomas Zimmermann:
> Use the buffer mappings provided by shadow-plane helpers. As the
> mappings are established while the commit can still fail, errors
> are now reported correctly to callers.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

Re: [PATCH 0/8] drm/mipi-dbi: Convert to shadow-plane helpers

[Javier Martinez Canillas]

Hello Thomas,

On 11/21/22 11:45, Thomas Zimmermann wrote:
> Convert the MIPI-DBI-based drivers to shadow-plane helpers. The
> drivers vmap/vunmap GEM buffer memory during the atomic commit.
> Shadow-plane helpers automate this process.
> 
> Patches 1 to 4 prepare the MIPI code for the change and simplify
> the restof the patchset.
> 
> Patches 5 to 7 rework the vmap code in the MIPI-DBI drivers and add
> shadow-plane helpers. Most of the affected drivers call MIPI-DBI
> helpers and get the update automatically. Only ili9225 and st7586
> require changes to their source code.
> 
> Patch 8 simplifies drm_dev_enter() and _exit(). It's not strictly
> needed, but streamlines the driver code and make sense overall.
> 
> Testing is welcome, as I don't have any hardware to test these
> changes myself.
> 
> Thomas Zimmermann (8):
>   drm/simple-kms: Remove drm_gem_simple_display_pipe_prepare_fb()
>   drm/ili9225: Call MIPI DBI mode_valid helper
>   drm/st7586: Call MIPI DBI mode_valid helper
>   drm/mipi-dbi: Initialize default driver functions with macro
>   drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update
>     helpers
>   drm/mipi-dbi: Support shadow-plane state
>   drm/mipi-dbi: Use shadow-plane mappings
>   drm/mipi-dbi: Move drm_dev_{enter,exit}() out from fb_dirty functions
> 

Sorry for the delay. I've tested now this series with the st7735r driver
and everything seems to works correctly.

Tested-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers

[Thomas Zimmermann]

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

Hi

Am 25.11.22 um 18:39 schrieb Noralf Trønnes:
> 
> 
> Den 21.11.2022 11.45, skrev Thomas Zimmermann:
>> Move the vmap/vunmap blocks from the inner fb_dirty helpers into the
>> MIPI DBI update helpers. The function calls can result in waiting and/or
>> processing overhead. Reduce the penalties by executing the functions once
>> in the outer-most function of the pipe update.
>>
>> This change also prepares for MIPI DBI for shadow-plane helpers. With
>> shadow-plane helpers, transfer source buffers are mapped into kernel
>> address space automatically.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_mipi_dbi.c | 60 +++++++++++++++++++---------------
>>   drivers/gpu/drm/tiny/ili9225.c | 31 ++++++++++++++----
>>   drivers/gpu/drm/tiny/st7586.c  | 28 +++++++++++-----
>>   include/drm/drm_mipi_dbi.h     |  6 ++--
>>   4 files changed, 81 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
>> index a6ac565808765..40e59a3a6481e 100644
>> --- a/drivers/gpu/drm/drm_mipi_dbi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
>> @@ -192,6 +192,7 @@ EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
>>   /**
>>    * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
>>    * @dst: The destination buffer
>> + * @src: The source buffer
>>    * @fb: The source framebuffer
>>    * @clip: Clipping rectangle of the area to be copied
>>    * @swap: When true, swap MSB/LSB of 16-bit values
>> @@ -199,12 +200,13 @@ EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
>>    * Returns:
>>    * Zero on success, negative error code on failure.
>>    */
>> -int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>> +int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *fb,
>>   		      struct drm_rect *clip, bool swap)
>>   {
>>   	struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
>> -	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>> -	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>> +	struct iosys_map data[DRM_FORMAT_MAX_PLANES] = {
>> +		*src,
>> +	};
> 
> I would prefer that you used src directly when calling the drm_fb_*
> functions, data can be anything and to me it isn't clear what that
> contains when I see it (ofc now I know, but next year I've forgotten).
> And is the array necessary, this helper will only ever support one color
> plane after all?

Will be fixed.

> 
>>   	struct iosys_map dst_map = IOSYS_MAP_INIT_VADDR(dst);
>>   	int ret;
>>   
>> @@ -212,10 +214,6 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = drm_gem_fb_vmap(fb, map, data);
>> -	if (ret)
>> -		goto out_drm_gem_fb_end_cpu_access;
>> -
>>   	switch (fb->format->format) {
>>   	case DRM_FORMAT_RGB565:
>>   		if (swap)
>> @@ -232,8 +230,6 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>>   		ret = -EINVAL;
>>   	}
>>   
>> -	drm_gem_fb_vunmap(fb, map);
>> -out_drm_gem_fb_end_cpu_access:
>>   	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>>   
>>   	return ret;
>> @@ -257,10 +253,9 @@ static void mipi_dbi_set_window_address(struct mipi_dbi_dev *dbidev,
>>   			 ys & 0xff, (ye >> 8) & 0xff, ye & 0xff);
>>   }
>>   
>> -static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>> +static void mipi_dbi_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
>> +			      struct drm_rect *rect)
>>   {
>> -	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>> -	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>>   	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
>>   	unsigned int height = rect->y2 - rect->y1;
>>   	unsigned int width = rect->x2 - rect->x1;
>> @@ -270,16 +265,9 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>>   	bool full;
>>   	void *tr;
>>   
>> -	if (WARN_ON(!fb))
>> -		return;
>> -
>>   	if (!drm_dev_enter(fb->dev, &idx))
>>   		return;
>>   
>> -	ret = drm_gem_fb_vmap(fb, map, data);
>> -	if (ret)
>> -		goto err_drm_dev_exit;
>> -
>>   	full = width == fb->width && height == fb->height;
>>   
>>   	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
>> @@ -287,11 +275,11 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>>   	if (!dbi->dc || !full || swap ||
>>   	    fb->format->format == DRM_FORMAT_XRGB8888) {
>>   		tr = dbidev->tx_buf;
>> -		ret = mipi_dbi_buf_copy(dbidev->tx_buf, fb, rect, swap);
>> +		ret = mipi_dbi_buf_copy(tr, src, fb, rect, swap);
>>   		if (ret)
>>   			goto err_msg;
>>   	} else {
>> -		tr = data[0].vaddr; /* TODO: Use mapping abstraction properly */
>> +		tr = src->vaddr; /* TODO: Use mapping abstraction properly */
>>   	}
>>   
>>   	mipi_dbi_set_window_address(dbidev, rect->x1, rect->x2 - 1, rect->y1,
>> @@ -303,9 +291,6 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>>   	if (ret)
>>   		drm_err_once(fb->dev, "Failed to update display %d\n", ret);
>>   
>> -	drm_gem_fb_vunmap(fb, map);
>> -
>> -err_drm_dev_exit:
>>   	drm_dev_exit(idx);
>>   }
>>   
>> @@ -338,14 +323,27 @@ EXPORT_SYMBOL(mipi_dbi_pipe_mode_valid);
>>   void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
>>   			  struct drm_plane_state *old_state)
>>   {
>> +	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>> +	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
> 
> These should have been zeroed to avoid UBSAN complaint, but you'll
> remove these later so not so important.

Will be fixed.

But do you know how these warnings happen? I went through the helpers 
before and changed them to only access the format-info's relevant 
planes. No unintialized memory access should be reported.

> 
>>   	struct drm_plane_state *state = pipe->plane.state;
>> +	struct drm_framebuffer *fb = state->fb;
>>   	struct drm_rect rect;
>> +	int ret;
>>   
>>   	if (!pipe->crtc.state->active)
>>   		return;
>>   
>> +	if (WARN_ON(!fb))
>> +		return;
>> +
>> +	ret = drm_gem_fb_vmap(fb, map, data);
>> +	if (ret)
>> +		return;
>> +
>>   	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
>> -		mipi_dbi_fb_dirty(state->fb, &rect);
>> +		mipi_dbi_fb_dirty(&data[0], fb, &rect);
>> +
>> +	drm_gem_fb_vunmap(fb, map);
>>   }
>>   EXPORT_SYMBOL(mipi_dbi_pipe_update);
>>   
>> @@ -373,14 +371,22 @@ void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
>>   		.y1 = 0,
>>   		.y2 = fb->height,
>>   	};
>> -	int idx;
>> +	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>> +	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>> +	int idx, ret;
>>   
>>   	if (!drm_dev_enter(&dbidev->drm, &idx))
>>   		return;
>>   
>> -	mipi_dbi_fb_dirty(fb, &rect);
>> +	ret = drm_gem_fb_vmap(fb, map, data);
>> +	if (ret)
>> +		goto err_drm_dev_exit;
>> +
>> +	mipi_dbi_fb_dirty(&data[0], fb, &rect);
>>   	backlight_enable(dbidev->backlight);
>>   
>> +	drm_gem_fb_vunmap(fb, map);
>> +err_drm_dev_exit:
>>   	drm_dev_exit(idx);
>>   }
>>   EXPORT_SYMBOL(mipi_dbi_enable_flush);
>> diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c
>> index f05a2d25866c1..0115c4090f9e7 100644
>> --- a/drivers/gpu/drm/tiny/ili9225.c
>> +++ b/drivers/gpu/drm/tiny/ili9225.c
>> @@ -25,6 +25,7 @@
>>   #include <drm/drm_framebuffer.h>
>>   #include <drm/drm_gem_atomic_helper.h>
>>   #include <drm/drm_gem_dma_helper.h>
>> +#include <drm/drm_gem_framebuffer_helper.h>
>>   #include <drm/drm_managed.h>
>>   #include <drm/drm_mipi_dbi.h>
>>   #include <drm/drm_rect.h>
>> @@ -76,9 +77,9 @@ static inline int ili9225_command(struct mipi_dbi *dbi, u8 cmd, u16 data)
>>   	return mipi_dbi_command_buf(dbi, cmd, par, 2);
>>   }
>>   
>> -static void ili9225_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>> +static void ili9225_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
>> +			     struct drm_rect *rect)
>>   {
>> -	struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
>>   	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
>>   	unsigned int height = rect->y2 - rect->y1;
>>   	unsigned int width = rect->x2 - rect->x1;
>> @@ -100,11 +101,11 @@ static void ili9225_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>>   	if (!dbi->dc || !full || swap ||
>>   	    fb->format->format == DRM_FORMAT_XRGB8888) {
>>   		tr = dbidev->tx_buf;
>> -		ret = mipi_dbi_buf_copy(dbidev->tx_buf, fb, rect, swap);
>> +		ret = mipi_dbi_buf_copy(tr, src, fb, rect, swap);
>>   		if (ret)
>>   			goto err_msg;
>>   	} else {
>> -		tr = dma_obj->vaddr;
>> +		tr = src->vaddr; /* TODO: Use mapping abstraction properly */
>>   	}
>>   
>>   	switch (dbidev->rotation) {
>> @@ -162,14 +163,24 @@ static void ili9225_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>>   static void ili9225_pipe_update(struct drm_simple_display_pipe *pipe,
>>   				struct drm_plane_state *old_state)
>>   {
>> +	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>> +	struct iosys_map data[DRM_FORMAT_MAX_PLANES];

Will be fixed.

>>   	struct drm_plane_state *state = pipe->plane.state;
>> +	struct drm_framebuffer *fb = state->fb;
>>   	struct drm_rect rect;
>> +	int ret;
>>   
>>   	if (!pipe->crtc.state->active)
>>   		return;
>>   
>> +	ret = drm_gem_fb_vmap(fb, map, data);
>> +	if (ret)
>> +		return;
>> +
>>   	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
>> -		ili9225_fb_dirty(state->fb, &rect);
>> +		ili9225_fb_dirty(&data[0], fb, &rect);
>> +
>> +	drm_gem_fb_vunmap(fb, map);
>>   }
>>   
>>   static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
>> @@ -186,6 +197,8 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
>>   		.y1 = 0,
>>   		.y2 = fb->height,
>>   	};
>> +	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>> +	struct iosys_map data[DRM_FORMAT_MAX_PLANES];

Will be fixed.

>>   	int ret, idx;
>>   	u8 am_id;
>>   
>> @@ -276,7 +289,13 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
>>   
>>   	ili9225_command(dbi, ILI9225_DISPLAY_CONTROL_1, 0x1017);
>>   
>> -	ili9225_fb_dirty(fb, &rect);
>> +	ret = drm_gem_fb_vmap(fb, map, data);
>> +	if (ret)
>> +		goto out_exit;
>> +
>> +	ili9225_fb_dirty(&data[0], fb, &rect);
>> +
>> +	drm_gem_fb_vunmap(fb, map);
>>   out_exit:
>>   	drm_dev_exit(idx);
>>   }
>> diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c
>> index 6bdd23e2a47c7..e773b1f2fd5f3 100644
>> --- a/drivers/gpu/drm/tiny/st7586.c
>> +++ b/drivers/gpu/drm/tiny/st7586.c
>> @@ -92,25 +92,24 @@ static void st7586_xrgb8888_to_gray332(u8 *dst, void *vaddr,
>>   	kfree(buf);
>>   }
>>   
>> -static int st7586_buf_copy(void *dst, struct drm_framebuffer *fb,
>> +static int st7586_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *fb,
>>   			   struct drm_rect *clip)
>>   {
>> -	struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
>> -	void *src = dma_obj->vaddr;
>> -	int ret = 0;
>> +	int ret;
>>   
>>   	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>>   	if (ret)
>>   		return ret;
>>   
>> -	st7586_xrgb8888_to_gray332(dst, src, fb, clip);
>> +	st7586_xrgb8888_to_gray332(dst, src->vaddr, fb, clip);
>>   
>>   	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>>   
>>   	return 0;
>>   }
>>   
>> -static void st7586_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>> +static void st7586_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
>> +			    struct drm_rect *rect)
>>   {
>>   	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
>>   	struct mipi_dbi *dbi = &dbidev->dbi;
>> @@ -125,7 +124,7 @@ static void st7586_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>>   
>>   	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
>>   
>> -	ret = st7586_buf_copy(dbidev->tx_buf, fb, rect);
>> +	ret = st7586_buf_copy(dbidev->tx_buf, src, fb, rect);
>>   	if (ret)
>>   		goto err_msg;
>>   
>> @@ -154,13 +153,19 @@ static void st7586_pipe_update(struct drm_simple_display_pipe *pipe,
>>   			       struct drm_plane_state *old_state)
>>   {
>>   	struct drm_plane_state *state = pipe->plane.state;
>> +	struct drm_framebuffer *fb = state->fb;
>> +	struct drm_gem_dma_object *dma_obj;
>> +	struct iosys_map src;
>>   	struct drm_rect rect;
>>   
>>   	if (!pipe->crtc.state->active)
>>   		return;
>>   
>> +	dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
>> +	iosys_map_set_vaddr(&src, dma_obj->vaddr);
>> +
> 
> You use drm_gem_fb_vmap() in the other places but here you access the
> object directly (and in the next hunk), but again not so important since
> it goes away in a later patch.

I'll update this patch to use drm_gem_fb_vmap() consistently.

> 
> With the comments considered:
> 
> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

Thanks.

Best regards
Thomas

> 
>>   	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
>> -		st7586_fb_dirty(state->fb, &rect);
>> +		st7586_fb_dirty(&src, fb, &rect);
>>   }
>>   
>>   static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
>> @@ -176,6 +181,8 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
>>   		.y1 = 0,
>>   		.y2 = fb->height,
>>   	};
>> +	struct drm_gem_dma_object *dma_obj;
>> +	struct iosys_map src;
>>   	int idx, ret;
>>   	u8 addr_mode;
>>   
>> @@ -235,7 +242,10 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
>>   
>>   	msleep(100);
>>   
>> -	st7586_fb_dirty(fb, &rect);
>> +	dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
>> +	iosys_map_set_vaddr(&src, dma_obj->vaddr);
>> +
>> +	st7586_fb_dirty(&src, fb, &rect);
>>   
>>   	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
>>   out_exit:
>> diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
>> index 8c4ea7956d61d..36ac8495566b0 100644
>> --- a/include/drm/drm_mipi_dbi.h
>> +++ b/include/drm/drm_mipi_dbi.h
>> @@ -13,9 +13,10 @@
>>   #include <drm/drm_simple_kms_helper.h>
>>   
>>   struct drm_rect;
>> -struct spi_device;
>>   struct gpio_desc;
>> +struct iosys_map;
>>   struct regulator;
>> +struct spi_device;
>>   
>>   /**
>>    * struct mipi_dbi - MIPI DBI interface
>> @@ -176,8 +177,9 @@ int mipi_dbi_command_read(struct mipi_dbi *dbi, u8 cmd, u8 *val);
>>   int mipi_dbi_command_buf(struct mipi_dbi *dbi, u8 cmd, u8 *data, size_t len);
>>   int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, const u8 *data,
>>   			      size_t len);
>> -int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>> +int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *fb,
>>   		      struct drm_rect *clip, bool swap);
>> +
>>   /**
>>    * mipi_dbi_command - MIPI DCS command with optional parameter(s)
>>    * @dbi: MIPI DBI structure

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

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

Re: [PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers

[Thomas Zimmermann]

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


>>
>> You use drm_gem_fb_vmap() in the other places but here you access the
>> object directly (and in the next hunk), but again not so important since
>> it goes away in a later patch.
> 
> I'll update this patch to use drm_gem_fb_vmap() consistently.

And after looking at the impact and churn, I rather go with the existing 
code that initializes from the GEM DMA object.

Noralf, is there a reason why most of MIPI DBI uses DMA helpers? In 
terms of flexibility and resource consumption, wouldn't SHMEM helpers be 
a better fit?

Best regards
Thomas

> 
>>
>> With the comments considered:
>>
>> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> 
> Thanks.
> 
> Best regards
> Thomas
> 
>>
>>>       if (drm_atomic_helper_damage_merged(old_state, state, &rect))
>>> -        st7586_fb_dirty(state->fb, &rect);
>>> +        st7586_fb_dirty(&src, fb, &rect);
>>>   }
>>>   static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
>>> @@ -176,6 +181,8 @@ static void st7586_pipe_enable(struct 
>>> drm_simple_display_pipe *pipe,
>>>           .y1 = 0,
>>>           .y2 = fb->height,
>>>       };
>>> +    struct drm_gem_dma_object *dma_obj;
>>> +    struct iosys_map src;
>>>       int idx, ret;
>>>       u8 addr_mode;
>>> @@ -235,7 +242,10 @@ static void st7586_pipe_enable(struct 
>>> drm_simple_display_pipe *pipe,
>>>       msleep(100);
>>> -    st7586_fb_dirty(fb, &rect);
>>> +    dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
>>> +    iosys_map_set_vaddr(&src, dma_obj->vaddr);
>>> +
>>> +    st7586_fb_dirty(&src, fb, &rect);
>>>       mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
>>>   out_exit:
>>> diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
>>> index 8c4ea7956d61d..36ac8495566b0 100644
>>> --- a/include/drm/drm_mipi_dbi.h
>>> +++ b/include/drm/drm_mipi_dbi.h
>>> @@ -13,9 +13,10 @@
>>>   #include <drm/drm_simple_kms_helper.h>
>>>   struct drm_rect;
>>> -struct spi_device;
>>>   struct gpio_desc;
>>> +struct iosys_map;
>>>   struct regulator;
>>> +struct spi_device;
>>>   /**
>>>    * struct mipi_dbi - MIPI DBI interface
>>> @@ -176,8 +177,9 @@ int mipi_dbi_command_read(struct mipi_dbi *dbi, 
>>> u8 cmd, u8 *val);
>>>   int mipi_dbi_command_buf(struct mipi_dbi *dbi, u8 cmd, u8 *data, 
>>> size_t len);
>>>   int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, const 
>>> u8 *data,
>>>                     size_t len);
>>> -int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>>> +int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct 
>>> drm_framebuffer *fb,
>>>                 struct drm_rect *clip, bool swap);
>>> +
>>>   /**
>>>    * mipi_dbi_command - MIPI DCS command with optional parameter(s)
>>>    * @dbi: MIPI DBI structure
> 

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

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

Re: [PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers

[Noralf Trønnes]

Den 02.12.2022 12.27, skrev Thomas Zimmermann:
> Hi
> 
> Am 25.11.22 um 18:39 schrieb Noralf Trønnes:
>>
>>
>> Den 21.11.2022 11.45, skrev Thomas Zimmermann:
>>> Move the vmap/vunmap blocks from the inner fb_dirty helpers into the
>>> MIPI DBI update helpers. The function calls can result in waiting and/or
>>> processing overhead. Reduce the penalties by executing the functions
>>> once
>>> in the outer-most function of the pipe update.
>>>
>>> This change also prepares for MIPI DBI for shadow-plane helpers. With
>>> shadow-plane helpers, transfer source buffers are mapped into kernel
>>> address space automatically.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---

>>> @@ -303,9 +291,6 @@ static void mipi_dbi_fb_dirty(struct
>>> drm_framebuffer *fb, struct drm_rect *rect)
>>>       if (ret)
>>>           drm_err_once(fb->dev, "Failed to update display %d\n", ret);
>>>   -    drm_gem_fb_vunmap(fb, map);
>>> -
>>> -err_drm_dev_exit:
>>>       drm_dev_exit(idx);
>>>   }
>>>   @@ -338,14 +323,27 @@ EXPORT_SYMBOL(mipi_dbi_pipe_mode_valid);
>>>   void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
>>>                 struct drm_plane_state *old_state)
>>>   {
>>> +    struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>>> +    struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>>
>> These should have been zeroed to avoid UBSAN complaint, but you'll
>> remove these later so not so important.
> 
> Will be fixed.
> 
> But do you know how these warnings happen? I went through the helpers
> before and changed them to only access the format-info's relevant
> planes. No unintialized memory access should be reported.
> 

It happens with imported buffers, iosys_map_clear() looks at the
uninitialized boolean variable ->is_iomem:

drm_gem_fb_vmap -> ... -> dma_buf_vmap -> iosys_map_clear

static inline void iosys_map_clear(struct iosys_map *map)
{
	if (map->is_iomem) {
		map->vaddr_iomem = NULL;
		map->is_iomem = false;
	} else {
		map->vaddr = NULL;
	}
}

Noralf.

Re: [PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers

[Noralf Trønnes]

Den 02.12.2022 12.50, skrev Thomas Zimmermann:
> 
>>>
>>> You use drm_gem_fb_vmap() in the other places but here you access the
>>> object directly (and in the next hunk), but again not so important since
>>> it goes away in a later patch.
>>
>> I'll update this patch to use drm_gem_fb_vmap() consistently.
> 
> And after looking at the impact and churn, I rather go with the existing
> code that initializes from the GEM DMA object.
> 
> Noralf, is there a reason why most of MIPI DBI uses DMA helpers? In
> terms of flexibility and resource consumption, wouldn't SHMEM helpers be
> a better fit?
> 

The SHMEM helper didn't exist at the time. The SPI subsystem doesn't
have an interface for scatter/gather transfers and DMA is needed in
order to run at full speed. SPI does convert an is_vmalloc_addr() buffer
to an sg list of pages in spi_map_buf() so it solves the missing
interface that way.

I have never tried to pass a shmem buffer to spi_sync() so I don't know
if it works, but I guess that it will work.

Bare in mind that theses buffers are at most 400k in size so I'm not
sure there's much to gain in term of resources at least.

Noralf.