[PATCH 1/3] drm: rcar-du: remove unnecessary include

[PATCH 1/3] drm: rcar-du: remove unnecessary include

[Tomi Valkeinen]

From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

rcar_du_regs.h is not needed by rcar_du_drv.c so drop the include.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_drv.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 00ac233a115e..541c202c993a 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -27,7 +27,6 @@
 
 #include "rcar_du_drv.h"
 #include "rcar_du_kms.h"
-#include "rcar_du_regs.h"
 
 /* -----------------------------------------------------------------------------
  * Device Information
-- 
2.34.1

[PATCH 2/3] drm: rcar-du: Fix r8a779a0 color issue.

[Tomi Valkeinen]

From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

The rcar DU driver on r8a779a0 has a bug causing some specific colors
getting converted to transparent colors, which then (usually) show as
black pixels on the screen.

The reason seems to be that the driver sets PnMR_SPIM_ALP bit in
PnMR.SPIM field, which is an illegal setting on r8a779a0. The
PnMR_SPIM_EOR bit also illegal.

Add a new feature flag for this (lack of a) feature and make sure the
bits are zero on r8a779a0.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_drv.c   |  3 ++-
 drivers/gpu/drm/rcar-du/rcar_du_drv.h   |  1 +
 drivers/gpu/drm/rcar-du/rcar_du_plane.c | 16 +++++++++++++---
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 541c202c993a..a2776f1d6f2c 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -506,7 +506,8 @@ static const struct rcar_du_device_info rcar_du_r8a7799x_info = {
 static const struct rcar_du_device_info rcar_du_r8a779a0_info = {
 	.gen = 3,
 	.features = RCAR_DU_FEATURE_CRTC_IRQ
-		  | RCAR_DU_FEATURE_VSP1_SOURCE,
+		  | RCAR_DU_FEATURE_VSP1_SOURCE
+		  | RCAR_DU_FEATURE_NO_BLENDING,
 	.channels_mask = BIT(1) | BIT(0),
 	.routes = {
 		/* R8A779A0 has two MIPI DSI outputs. */
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index bfad7775d9a1..712389c7b3d0 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -31,6 +31,7 @@ struct rcar_du_device;
 #define RCAR_DU_FEATURE_VSP1_SOURCE	BIT(2)	/* Has inputs from VSP1 */
 #define RCAR_DU_FEATURE_INTERLACED	BIT(3)	/* HW supports interlaced */
 #define RCAR_DU_FEATURE_TVM_SYNC	BIT(4)	/* Has TV switch/sync modes */
+#define RCAR_DU_FEATURE_NO_BLENDING	BIT(5)	/* PnMR.SPIM does not have ALP nor EOR bits */
 
 #define RCAR_DU_QUIRK_ALIGN_128B	BIT(0)	/* Align pitches to 128 bytes */
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index 9e1f0cbbf642..2103816807cc 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -506,8 +506,19 @@ static void rcar_du_plane_setup_format_gen3(struct rcar_du_group *rgrp,
 					    unsigned int index,
 					    const struct rcar_du_plane_state *state)
 {
-	rcar_du_plane_write(rgrp, index, PnMR,
-			    PnMR_SPIM_TP_OFF | state->format->pnmr);
+	struct rcar_du_device *rcdu = rgrp->dev;
+	u32 pnmr;
+
+	pnmr = state->format->pnmr;
+
+	if (rcdu->info->features & RCAR_DU_FEATURE_NO_BLENDING) {
+		/* No blending. ALP and EOR are not supported */
+		pnmr &= ~(PnMR_SPIM_ALP | PnMR_SPIM_EOR);
+	}
+
+	pnmr |= PnMR_SPIM_TP_OFF;
+
+	rcar_du_plane_write(rgrp, index, PnMR, pnmr);
 
 	rcar_du_plane_write(rgrp, index, PnDDCR4,
 			    state->format->edf | PnDDCR4_CODE);
@@ -521,7 +532,6 @@ static void rcar_du_plane_setup_format_gen3(struct rcar_du_group *rgrp,
 	 * register to 0 to avoid this.
 	 */
 
-	/* TODO: Check if alpha-blending should be disabled in PnMR. */
 	rcar_du_plane_write(rgrp, index, PnALPHAR, 0);
 }
 
-- 
2.34.1

[PATCH 3/3] drm: rcar-du: fix DSI enable & disable sequence

[Tomi Valkeinen]

From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

The rcar crtc depends on the clock provided from the rcar DSI bridge.
When the DSI bridge is disabled, the clock is stopped, which causes the
crtc disable to timeout.

Also, while I have no issue with the enable, the documentation suggests
to enable the DSI before the crtc so that the crtc has its clock enabled
at enable time. This is also not done by the current driver.

To fix this, we need to keep the DSI bridge enabled until the crtc has
disabled itself, and enable the DSI bridge before crtc enables itself.

Add functions (rcar_mipi_dsi_atomic_early_enable and
rcar_mipi_dsi_atomic_late_disable) to the rcar DSI bridge driver which
the rcar driver can use to enable/disable the DSI clock when needed.
This is similar to what is already done with the rcar LVDS bridge.

Also add a new function, rcar_mipi_dsi_stop_video(), called from
rcar_mipi_dsi_atomic_enable so that the DSI TX gets disabled at the
correct time, even if the clocks are still kept enabled.

And, while possibly not strictly needed, clear clock related enable bits
in rcar_mipi_dsi_atomic_late_disable to mirror the sequence done in
rcar_mipi_dsi_startup() (via rcar_mipi_dsi_atomic_early_enable).

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c    | 25 +++++++++
 drivers/gpu/drm/rcar-du/rcar_du_drv.h     |  2 +
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c |  4 ++
 drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c   | 63 +++++++++++++++++++++--
 drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h   | 32 ++++++++++++
 5 files changed, 121 insertions(+), 5 deletions(-)
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index fd3b94649a01..51fd1d99f4e8 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -29,6 +29,7 @@
 #include "rcar_du_regs.h"
 #include "rcar_du_vsp.h"
 #include "rcar_lvds.h"
+#include "rcar_mipi_dsi.h"
 
 static u32 rcar_du_crtc_read(struct rcar_du_crtc *rcrtc, u32 reg)
 {
@@ -733,6 +734,18 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
 		rcar_cmm_enable(rcrtc->cmm);
 	rcar_du_crtc_get(rcrtc);
 
+	if ((rcdu->info->dsi_clk_mask & BIT(rcrtc->index)) &&
+	    (rstate->outputs &
+	     (BIT(RCAR_DU_OUTPUT_DSI0) | BIT(RCAR_DU_OUTPUT_DSI1)))) {
+		struct drm_bridge *bridge = rcdu->dsi[rcrtc->index];
+
+		/*
+		 * Enable the DSI clock output.
+		 */
+
+		rcar_mipi_dsi_atomic_early_enable(bridge, state);
+	}
+
 	/*
 	 * On D3/E3 the dot clock is provided by the LVDS encoder attached to
 	 * the DU channel. We need to enable its clock output explicitly if
@@ -769,6 +782,18 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
 	rcar_du_crtc_stop(rcrtc);
 	rcar_du_crtc_put(rcrtc);
 
+	if ((rcdu->info->dsi_clk_mask & BIT(rcrtc->index)) &&
+	    (rstate->outputs &
+	     (BIT(RCAR_DU_OUTPUT_DSI0) | BIT(RCAR_DU_OUTPUT_DSI1)))) {
+		struct drm_bridge *bridge = rcdu->dsi[rcrtc->index];
+
+		/*
+		 * Disable the DSI clock output.
+		 */
+
+		rcar_mipi_dsi_atomic_late_disable(bridge);
+	}
+
 	if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) &&
 	    rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) {
 		struct drm_bridge *bridge = rcdu->lvds[rcrtc->index];
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index 712389c7b3d0..5cfa2bb7ad93 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -92,6 +92,7 @@ struct rcar_du_device_info {
 #define RCAR_DU_MAX_GROUPS		DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2)
 #define RCAR_DU_MAX_VSPS		4
 #define RCAR_DU_MAX_LVDS		2
+#define RCAR_DU_MAX_DSI			2
 
 struct rcar_du_device {
 	struct device *dev;
@@ -108,6 +109,7 @@ struct rcar_du_device {
 	struct platform_device *cmms[RCAR_DU_MAX_CRTCS];
 	struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
 	struct drm_bridge *lvds[RCAR_DU_MAX_LVDS];
+	struct drm_bridge *dsi[RCAR_DU_MAX_DSI];
 
 	struct {
 		struct drm_property *colorkey;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
index 60d6be78323b..ac93e08e8af4 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
@@ -84,6 +84,10 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
 		if (output == RCAR_DU_OUTPUT_LVDS0 ||
 		    output == RCAR_DU_OUTPUT_LVDS1)
 			rcdu->lvds[output - RCAR_DU_OUTPUT_LVDS0] = bridge;
+
+		if (output == RCAR_DU_OUTPUT_DSI0 ||
+		    output == RCAR_DU_OUTPUT_DSI1)
+			rcdu->dsi[output - RCAR_DU_OUTPUT_DSI0] = bridge;
 	}
 
 	/*
diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
index 62f7eb84ab01..1ec40e40fd08 100644
--- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
+++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
@@ -542,6 +542,34 @@ static int rcar_mipi_dsi_start_video(struct rcar_mipi_dsi *dsi)
 	return 0;
 }
 
+static void rcar_mipi_dsi_stop_video(struct rcar_mipi_dsi *dsi)
+{
+	u32 status;
+	int ret;
+
+	/* Disable transmission in video mode. */
+	rcar_mipi_dsi_clr(dsi, TXVMCR, TXVMCR_EN_VIDEO);
+
+	ret = read_poll_timeout(rcar_mipi_dsi_read, status,
+				!(status & TXVMSR_ACT),
+				2000, 100000, false, dsi, TXVMSR);
+	if (ret < 0) {
+		dev_err(dsi->dev, "Failed to disable video transmission\n");
+		return;
+	}
+
+	/* Assert video FIFO clear. */
+	rcar_mipi_dsi_set(dsi, TXVMCR, TXVMCR_VFCLR);
+
+	ret = read_poll_timeout(rcar_mipi_dsi_read, status,
+				!(status & TXVMSR_VFRDY),
+				2000, 100000, false, dsi, TXVMSR);
+	if (ret < 0) {
+		dev_err(dsi->dev, "Failed to assert video FIFO clear\n");
+		return;
+	}
+}
+
 /* -----------------------------------------------------------------------------
  * Bridge
  */
@@ -558,7 +586,22 @@ static int rcar_mipi_dsi_attach(struct drm_bridge *bridge,
 static void rcar_mipi_dsi_atomic_enable(struct drm_bridge *bridge,
 					struct drm_bridge_state *old_bridge_state)
 {
-	struct drm_atomic_state *state = old_bridge_state->base.state;
+	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
+
+	rcar_mipi_dsi_start_video(dsi);
+}
+
+static void rcar_mipi_dsi_atomic_disable(struct drm_bridge *bridge,
+					struct drm_bridge_state *old_bridge_state)
+{
+	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
+
+	rcar_mipi_dsi_stop_video(dsi);
+}
+
+void rcar_mipi_dsi_atomic_early_enable(struct drm_bridge *bridge,
+				       struct drm_atomic_state *state)
+{
 	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
 	const struct drm_display_mode *mode;
 	struct drm_connector *connector;
@@ -586,8 +629,6 @@ static void rcar_mipi_dsi_atomic_enable(struct drm_bridge *bridge,
 	if (ret < 0)
 		goto err_dsi_start_hs;
 
-	rcar_mipi_dsi_start_video(dsi);
-
 	return;
 
 err_dsi_start_hs:
@@ -595,15 +636,27 @@ static void rcar_mipi_dsi_atomic_enable(struct drm_bridge *bridge,
 err_dsi_startup:
 	rcar_mipi_dsi_clk_disable(dsi);
 }
+EXPORT_SYMBOL_GPL(rcar_mipi_dsi_atomic_early_enable);
 
-static void rcar_mipi_dsi_atomic_disable(struct drm_bridge *bridge,
-					 struct drm_bridge_state *old_bridge_state)
+void rcar_mipi_dsi_atomic_late_disable(struct drm_bridge *bridge)
 {
 	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
 
+	rcar_mipi_dsi_clr(dsi, VCLKEN, VCLKEN_CKEN);
+
+	/* Disable DOT clock */
+	rcar_mipi_dsi_clr(dsi, VCLKSET, VCLKSET_CKEN);
+
+	/* CFGCLK disable */
+	rcar_mipi_dsi_clr(dsi, CFGCLKSET, CFGCLKSET_CKEN);
+
+	/* LPCLK disable */
+	rcar_mipi_dsi_clr(dsi, LPCLKSET, LPCLKSET_CKEN);
+
 	rcar_mipi_dsi_shutdown(dsi);
 	rcar_mipi_dsi_clk_disable(dsi);
 }
+EXPORT_SYMBOL_GPL(rcar_mipi_dsi_atomic_late_disable);
 
 static enum drm_mode_status
 rcar_mipi_dsi_bridge_mode_valid(struct drm_bridge *bridge,
diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h
new file mode 100644
index 000000000000..1764abf65a34
--- /dev/null
+++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * R-Car DSI Encoder
+ *
+ * Copyright (C) 2022 Renesas Electronics Corporation
+ *
+ * Contact: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
+ */
+
+#ifndef __RCAR_MIPI_DSI_H__
+#define __RCAR_MIPI_DSI_H__
+
+struct drm_bridge;
+struct drm_atomic_state;
+
+#if IS_ENABLED(CONFIG_DRM_RCAR_MIPI_DSI)
+void rcar_mipi_dsi_atomic_early_enable(struct drm_bridge *bridge,
+				       struct drm_atomic_state *state);
+void rcar_mipi_dsi_atomic_late_disable(struct drm_bridge *bridge);
+#else
+static inline void
+rcar_mipi_dsi_atomic_early_enable(struct drm_bridge *bridge,
+				  struct drm_atomic_state *state)
+{
+}
+
+void rcar_mipi_dsi_atomic_late_disable(struct drm_bridge *bridge)
+{
+}
+#endif /* CONFIG_DRM_RCAR_MIPI_DSI */
+
+#endif /* __RCAR_MIPI_DSI_H__ */
-- 
2.34.1

Re: [PATCH 1/3] drm: rcar-du: remove unnecessary include

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Aug 17, 2022 at 04:28:01PM +0300, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> rcar_du_regs.h is not needed by rcar_du_drv.c so drop the include.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> index 00ac233a115e..541c202c993a 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -27,7 +27,6 @@
>  
>  #include "rcar_du_drv.h"
>  #include "rcar_du_kms.h"
> -#include "rcar_du_regs.h"
>  
>  /* -----------------------------------------------------------------------------
>   * Device Information

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] drm: rcar-du: Fix r8a779a0 color issue.

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Aug 17, 2022 at 04:28:02PM +0300, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> The rcar DU driver on r8a779a0 has a bug causing some specific colors
> getting converted to transparent colors, which then (usually) show as
> black pixels on the screen.
> 
> The reason seems to be that the driver sets PnMR_SPIM_ALP bit in
> PnMR.SPIM field, which is an illegal setting on r8a779a0. The
> PnMR_SPIM_EOR bit also illegal.
> 
> Add a new feature flag for this (lack of a) feature and make sure the
> bits are zero on r8a779a0.

Good catch !

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c   |  3 ++-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h   |  1 +
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 16 +++++++++++++---
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> index 541c202c993a..a2776f1d6f2c 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -506,7 +506,8 @@ static const struct rcar_du_device_info rcar_du_r8a7799x_info = {
>  static const struct rcar_du_device_info rcar_du_r8a779a0_info = {
>  	.gen = 3,
>  	.features = RCAR_DU_FEATURE_CRTC_IRQ
> -		  | RCAR_DU_FEATURE_VSP1_SOURCE,
> +		  | RCAR_DU_FEATURE_VSP1_SOURCE
> +		  | RCAR_DU_FEATURE_NO_BLENDING,
>  	.channels_mask = BIT(1) | BIT(0),
>  	.routes = {
>  		/* R8A779A0 has two MIPI DSI outputs. */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> index bfad7775d9a1..712389c7b3d0 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -31,6 +31,7 @@ struct rcar_du_device;
>  #define RCAR_DU_FEATURE_VSP1_SOURCE	BIT(2)	/* Has inputs from VSP1 */
>  #define RCAR_DU_FEATURE_INTERLACED	BIT(3)	/* HW supports interlaced */
>  #define RCAR_DU_FEATURE_TVM_SYNC	BIT(4)	/* Has TV switch/sync modes */
> +#define RCAR_DU_FEATURE_NO_BLENDING	BIT(5)	/* PnMR.SPIM does not have ALP nor EOR bits */
>  
>  #define RCAR_DU_QUIRK_ALIGN_128B	BIT(0)	/* Align pitches to 128 bytes */
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> index 9e1f0cbbf642..2103816807cc 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> @@ -506,8 +506,19 @@ static void rcar_du_plane_setup_format_gen3(struct rcar_du_group *rgrp,
>  					    unsigned int index,
>  					    const struct rcar_du_plane_state *state)
>  {
> -	rcar_du_plane_write(rgrp, index, PnMR,
> -			    PnMR_SPIM_TP_OFF | state->format->pnmr);
> +	struct rcar_du_device *rcdu = rgrp->dev;
> +	u32 pnmr;
> +
> +	pnmr = state->format->pnmr;
> +
> +	if (rcdu->info->features & RCAR_DU_FEATURE_NO_BLENDING) {
> +		/* No blending. ALP and EOR are not supported */
> +		pnmr &= ~(PnMR_SPIM_ALP | PnMR_SPIM_EOR);
> +	}
> +
> +	pnmr |= PnMR_SPIM_TP_OFF;

I'd combine this with the initial pnmr assignment. I can handle this
when applying, no need to resubmit.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +	rcar_du_plane_write(rgrp, index, PnMR, pnmr);
>  
>  	rcar_du_plane_write(rgrp, index, PnDDCR4,
>  			    state->format->edf | PnDDCR4_CODE);
> @@ -521,7 +532,6 @@ static void rcar_du_plane_setup_format_gen3(struct rcar_du_group *rgrp,
>  	 * register to 0 to avoid this.
>  	 */
>  
> -	/* TODO: Check if alpha-blending should be disabled in PnMR. */
>  	rcar_du_plane_write(rgrp, index, PnALPHAR, 0);
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] drm: rcar-du: fix DSI enable & disable sequence

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Aug 17, 2022 at 04:28:03PM +0300, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> The rcar crtc depends on the clock provided from the rcar DSI bridge.
> When the DSI bridge is disabled, the clock is stopped, which causes the
> crtc disable to timeout.
> 
> Also, while I have no issue with the enable, the documentation suggests
> to enable the DSI before the crtc so that the crtc has its clock enabled
> at enable time. This is also not done by the current driver.
> 
> To fix this, we need to keep the DSI bridge enabled until the crtc has
> disabled itself, and enable the DSI bridge before crtc enables itself.
> 
> Add functions (rcar_mipi_dsi_atomic_early_enable and
> rcar_mipi_dsi_atomic_late_disable) to the rcar DSI bridge driver which
> the rcar driver can use to enable/disable the DSI clock when needed.
> This is similar to what is already done with the rcar LVDS bridge.

I had hoped we could avoid that :-(

I wonder if we could instead rely on the pre_enable/post_disable bridge
operations, with a custom commit tail implementation to order those
before/after the CRTC enable/disable respectively. That would be pretty
complex though, so probably not worth it.

Thinking more about this, I wonder why pre_enable is not called before
enabling the CRTC in the DRM atomic helpers. That would make more sense
to me, but I suppose changing it would break too many things ? I think
it should at least be discussed to figure out if it was a historical
oversight or if there was a good reason. It's *really* not nice to poke
holes through the abstraction layers like this.

> Also add a new function, rcar_mipi_dsi_stop_video(), called from
> rcar_mipi_dsi_atomic_enable so that the DSI TX gets disabled at the
> correct time, even if the clocks are still kept enabled.

I think this should be split to a separate patch, possibly before this
one, it addresses a separate issue.

> And, while possibly not strictly needed, clear clock related enable bits
> in rcar_mipi_dsi_atomic_late_disable to mirror the sequence done in
> rcar_mipi_dsi_startup() (via rcar_mipi_dsi_atomic_early_enable).

And this too should be a separate patch, possibly bundled with
rcar_mipi_dsi_stop_video().

Have you checked in the BSP code to see if they do the same at disable
time ?

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c    | 25 +++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h     |  2 +
>  drivers/gpu/drm/rcar-du/rcar_du_encoder.c |  4 ++
>  drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c   | 63 +++++++++++++++++++++--
>  drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h   | 32 ++++++++++++
>  5 files changed, 121 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index fd3b94649a01..51fd1d99f4e8 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -29,6 +29,7 @@
>  #include "rcar_du_regs.h"
>  #include "rcar_du_vsp.h"
>  #include "rcar_lvds.h"
> +#include "rcar_mipi_dsi.h"
>  
>  static u32 rcar_du_crtc_read(struct rcar_du_crtc *rcrtc, u32 reg)
>  {
> @@ -733,6 +734,18 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>  		rcar_cmm_enable(rcrtc->cmm);
>  	rcar_du_crtc_get(rcrtc);
>  

A comment here similar to the LVDS case would be useful. I would
actually move this below the LVDS code, and write

	/*
	 * Similarly, on V3U, ...
	 */

> +	if ((rcdu->info->dsi_clk_mask & BIT(rcrtc->index)) &&
> +	    (rstate->outputs &
> +	     (BIT(RCAR_DU_OUTPUT_DSI0) | BIT(RCAR_DU_OUTPUT_DSI1)))) {
> +		struct drm_bridge *bridge = rcdu->dsi[rcrtc->index];
> +
> +		/*
> +		 * Enable the DSI clock output.
> +		 */
> +
> +		rcar_mipi_dsi_atomic_early_enable(bridge, state);
> +	}

I was wondering if we could merge the DSI and LVDS clock enabling code,
including merging the lvds and dsi fields in rcar_du_device, but it
doesn't seem it will be very clean here.

> +
>  	/*
>  	 * On D3/E3 the dot clock is provided by the LVDS encoder attached to
>  	 * the DU channel. We need to enable its clock output explicitly if
> @@ -769,6 +782,18 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
>  	rcar_du_crtc_stop(rcrtc);
>  	rcar_du_crtc_put(rcrtc);
>  
> +	if ((rcdu->info->dsi_clk_mask & BIT(rcrtc->index)) &&
> +	    (rstate->outputs &
> +	     (BIT(RCAR_DU_OUTPUT_DSI0) | BIT(RCAR_DU_OUTPUT_DSI1)))) {
> +		struct drm_bridge *bridge = rcdu->dsi[rcrtc->index];
> +
> +		/*
> +		 * Disable the DSI clock output.
> +		 */
> +
> +		rcar_mipi_dsi_atomic_late_disable(bridge);
> +	}
> +
>  	if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) &&
>  	    rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) {
>  		struct drm_bridge *bridge = rcdu->lvds[rcrtc->index];
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> index 712389c7b3d0..5cfa2bb7ad93 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -92,6 +92,7 @@ struct rcar_du_device_info {
>  #define RCAR_DU_MAX_GROUPS		DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2)
>  #define RCAR_DU_MAX_VSPS		4
>  #define RCAR_DU_MAX_LVDS		2
> +#define RCAR_DU_MAX_DSI			2
>  
>  struct rcar_du_device {
>  	struct device *dev;
> @@ -108,6 +109,7 @@ struct rcar_du_device {
>  	struct platform_device *cmms[RCAR_DU_MAX_CRTCS];
>  	struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
>  	struct drm_bridge *lvds[RCAR_DU_MAX_LVDS];
> +	struct drm_bridge *dsi[RCAR_DU_MAX_DSI];
>  
>  	struct {
>  		struct drm_property *colorkey;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> index 60d6be78323b..ac93e08e8af4 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> @@ -84,6 +84,10 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
>  		if (output == RCAR_DU_OUTPUT_LVDS0 ||
>  		    output == RCAR_DU_OUTPUT_LVDS1)
>  			rcdu->lvds[output - RCAR_DU_OUTPUT_LVDS0] = bridge;
> +
> +		if (output == RCAR_DU_OUTPUT_DSI0 ||
> +		    output == RCAR_DU_OUTPUT_DSI1)
> +			rcdu->dsi[output - RCAR_DU_OUTPUT_DSI0] = bridge;
>  	}
>  
>  	/*
> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> index 62f7eb84ab01..1ec40e40fd08 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> @@ -542,6 +542,34 @@ static int rcar_mipi_dsi_start_video(struct rcar_mipi_dsi *dsi)
>  	return 0;
>  }
>  
> +static void rcar_mipi_dsi_stop_video(struct rcar_mipi_dsi *dsi)
> +{
> +	u32 status;
> +	int ret;
> +
> +	/* Disable transmission in video mode. */
> +	rcar_mipi_dsi_clr(dsi, TXVMCR, TXVMCR_EN_VIDEO);
> +
> +	ret = read_poll_timeout(rcar_mipi_dsi_read, status,
> +				!(status & TXVMSR_ACT),
> +				2000, 100000, false, dsi, TXVMSR);
> +	if (ret < 0) {
> +		dev_err(dsi->dev, "Failed to disable video transmission\n");
> +		return;
> +	}
> +
> +	/* Assert video FIFO clear. */
> +	rcar_mipi_dsi_set(dsi, TXVMCR, TXVMCR_VFCLR);
> +
> +	ret = read_poll_timeout(rcar_mipi_dsi_read, status,
> +				!(status & TXVMSR_VFRDY),
> +				2000, 100000, false, dsi, TXVMSR);
> +	if (ret < 0) {
> +		dev_err(dsi->dev, "Failed to assert video FIFO clear\n");
> +		return;
> +	}
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Bridge
>   */
> @@ -558,7 +586,22 @@ static int rcar_mipi_dsi_attach(struct drm_bridge *bridge,
>  static void rcar_mipi_dsi_atomic_enable(struct drm_bridge *bridge,
>  					struct drm_bridge_state *old_bridge_state)
>  {
> -	struct drm_atomic_state *state = old_bridge_state->base.state;
> +	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
> +
> +	rcar_mipi_dsi_start_video(dsi);
> +}
> +
> +static void rcar_mipi_dsi_atomic_disable(struct drm_bridge *bridge,
> +					struct drm_bridge_state *old_bridge_state)
> +{
> +	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
> +
> +	rcar_mipi_dsi_stop_video(dsi);
> +}
> +
> +void rcar_mipi_dsi_atomic_early_enable(struct drm_bridge *bridge,
> +				       struct drm_atomic_state *state)
> +{
>  	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
>  	const struct drm_display_mode *mode;
>  	struct drm_connector *connector;
> @@ -586,8 +629,6 @@ static void rcar_mipi_dsi_atomic_enable(struct drm_bridge *bridge,
>  	if (ret < 0)
>  		goto err_dsi_start_hs;
>  
> -	rcar_mipi_dsi_start_video(dsi);
> -
>  	return;
>  
>  err_dsi_start_hs:
> @@ -595,15 +636,27 @@ static void rcar_mipi_dsi_atomic_enable(struct drm_bridge *bridge,
>  err_dsi_startup:
>  	rcar_mipi_dsi_clk_disable(dsi);
>  }
> +EXPORT_SYMBOL_GPL(rcar_mipi_dsi_atomic_early_enable);
>  
> -static void rcar_mipi_dsi_atomic_disable(struct drm_bridge *bridge,
> -					 struct drm_bridge_state *old_bridge_state)
> +void rcar_mipi_dsi_atomic_late_disable(struct drm_bridge *bridge)
>  {
>  	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
>  
> +	rcar_mipi_dsi_clr(dsi, VCLKEN, VCLKEN_CKEN);
> +
> +	/* Disable DOT clock */
> +	rcar_mipi_dsi_clr(dsi, VCLKSET, VCLKSET_CKEN);
> +
> +	/* CFGCLK disable */
> +	rcar_mipi_dsi_clr(dsi, CFGCLKSET, CFGCLKSET_CKEN);
> +
> +	/* LPCLK disable */
> +	rcar_mipi_dsi_clr(dsi, LPCLKSET, LPCLKSET_CKEN);
> +
>  	rcar_mipi_dsi_shutdown(dsi);
>  	rcar_mipi_dsi_clk_disable(dsi);
>  }
> +EXPORT_SYMBOL_GPL(rcar_mipi_dsi_atomic_late_disable);
>  
>  static enum drm_mode_status
>  rcar_mipi_dsi_bridge_mode_valid(struct drm_bridge *bridge,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h
> new file mode 100644
> index 000000000000..1764abf65a34
> --- /dev/null
> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * R-Car DSI Encoder
> + *
> + * Copyright (C) 2022 Renesas Electronics Corporation
> + *
> + * Contact: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> + */
> +
> +#ifndef __RCAR_MIPI_DSI_H__
> +#define __RCAR_MIPI_DSI_H__
> +
> +struct drm_bridge;
> +struct drm_atomic_state;

Alphabetical order please.

> +
> +#if IS_ENABLED(CONFIG_DRM_RCAR_MIPI_DSI)
> +void rcar_mipi_dsi_atomic_early_enable(struct drm_bridge *bridge,
> +				       struct drm_atomic_state *state);
> +void rcar_mipi_dsi_atomic_late_disable(struct drm_bridge *bridge);

It would be nice to have a naming scheme consistent with rcar_lvds.h.
That would mean rcar_mipi_dsi_clk_{enable,disable}(), or renaming the
LVDS functions to match whatever name would be picked here.

We could name the functions pre_enable/post_disable to show what they
should really be, if it wasn't for the DRM atomic helpers calling the
bridge operations at the wrong time.

> +#else
> +static inline void
> +rcar_mipi_dsi_atomic_early_enable(struct drm_bridge *bridge,
> +				  struct drm_atomic_state *state)
> +{
> +}
> +
> +void rcar_mipi_dsi_atomic_late_disable(struct drm_bridge *bridge)
> +{
> +}
> +#endif /* CONFIG_DRM_RCAR_MIPI_DSI */
> +
> +#endif /* __RCAR_MIPI_DSI_H__ */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] drm: rcar-du: fix DSI enable & disable sequence

[Tomi Valkeinen]

Hi,

On 19/08/2022 04:34, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wed, Aug 17, 2022 at 04:28:03PM +0300, Tomi Valkeinen wrote:
>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>
>> The rcar crtc depends on the clock provided from the rcar DSI bridge.
>> When the DSI bridge is disabled, the clock is stopped, which causes the
>> crtc disable to timeout.
>>
>> Also, while I have no issue with the enable, the documentation suggests
>> to enable the DSI before the crtc so that the crtc has its clock enabled
>> at enable time. This is also not done by the current driver.
>>
>> To fix this, we need to keep the DSI bridge enabled until the crtc has
>> disabled itself, and enable the DSI bridge before crtc enables itself.
>>
>> Add functions (rcar_mipi_dsi_atomic_early_enable and
>> rcar_mipi_dsi_atomic_late_disable) to the rcar DSI bridge driver which
>> the rcar driver can use to enable/disable the DSI clock when needed.
>> This is similar to what is already done with the rcar LVDS bridge.
> 
> I had hoped we could avoid that :-(
> 
> I wonder if we could instead rely on the pre_enable/post_disable bridge
> operations, with a custom commit tail implementation to order those
> before/after the CRTC enable/disable respectively. That would be pretty
> complex though, so probably not worth it.

I think so, especially as we already have a similar case for LVDS, and 
these custom calls are quite simple to implement and understand. I fear 
a custom commit implementation would be a much bigger maintenance burden 
for little, if any, benefit.

> Thinking more about this, I wonder why pre_enable is not called before
> enabling the CRTC in the DRM atomic helpers. That would make more sense
> to me, but I suppose changing it would break too many things ? I think
> it should at least be discussed to figure out if it was a historical
> oversight or if there was a good reason. It's *really* not nice to poke
> holes through the abstraction layers like this.

Yes, I'll bring this question to #dri-devel. But I think it's better to 
get the issue fixed with a custom function call as done in this patch, 
and hope that we can do the work in a standard way in the future.

>> Also add a new function, rcar_mipi_dsi_stop_video(), called from
>> rcar_mipi_dsi_atomic_enable so that the DSI TX gets disabled at the
>> correct time, even if the clocks are still kept enabled.
> 
> I think this should be split to a separate patch, possibly before this
> one, it addresses a separate issue.

I agree.

>> And, while possibly not strictly needed, clear clock related enable bits
>> in rcar_mipi_dsi_atomic_late_disable to mirror the sequence done in
>> rcar_mipi_dsi_startup() (via rcar_mipi_dsi_atomic_early_enable).
> 
> And this too should be a separate patch, possibly bundled with
> rcar_mipi_dsi_stop_video().

Yep. I'll have it in a separate patch as they're somewhat different.

> Have you checked in the BSP code to see if they do the same at disable
> time ?

No, they don't seem to do this.

The steps for stopping of the video transmission is described in the 
doc, but there's no mention I can see of clearing those bits (or rather, 
making sure they are cleared before starting the next enable sequence).

>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/rcar-du/rcar_du_crtc.c    | 25 +++++++++
>>   drivers/gpu/drm/rcar-du/rcar_du_drv.h     |  2 +
>>   drivers/gpu/drm/rcar-du/rcar_du_encoder.c |  4 ++
>>   drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c   | 63 +++++++++++++++++++++--
>>   drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h   | 32 ++++++++++++
>>   5 files changed, 121 insertions(+), 5 deletions(-)
>>   create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> index fd3b94649a01..51fd1d99f4e8 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -29,6 +29,7 @@
>>   #include "rcar_du_regs.h"
>>   #include "rcar_du_vsp.h"
>>   #include "rcar_lvds.h"
>> +#include "rcar_mipi_dsi.h"
>>   
>>   static u32 rcar_du_crtc_read(struct rcar_du_crtc *rcrtc, u32 reg)
>>   {
>> @@ -733,6 +734,18 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>>   		rcar_cmm_enable(rcrtc->cmm);
>>   	rcar_du_crtc_get(rcrtc);
>>   
> 
> A comment here similar to the LVDS case would be useful. I would
> actually move this below the LVDS code, and write
> 
> 	/*
> 	 * Similarly, on V3U, ...
> 	 */

Ok.

>> +	if ((rcdu->info->dsi_clk_mask & BIT(rcrtc->index)) &&
>> +	    (rstate->outputs &
>> +	     (BIT(RCAR_DU_OUTPUT_DSI0) | BIT(RCAR_DU_OUTPUT_DSI1)))) {
>> +		struct drm_bridge *bridge = rcdu->dsi[rcrtc->index];
>> +
>> +		/*
>> +		 * Enable the DSI clock output.
>> +		 */
>> +
>> +		rcar_mipi_dsi_atomic_early_enable(bridge, state);
>> +	}
> 
> I was wondering if we could merge the DSI and LVDS clock enabling code,
> including merging the lvds and dsi fields in rcar_du_device, but it
> doesn't seem it will be very clean here.

True, they are quite similar. I didn't want to mix them up as I don't 
have the means to test lvds, nor am I that familiar with the rcar du.

If I'm not mistaken, the difference is that LVDS clock is used for 
non-LVDS output, whereas here the DSI clock is used for DSI output.

>> +
>>   	/*
>>   	 * On D3/E3 the dot clock is provided by the LVDS encoder attached to
>>   	 * the DU channel. We need to enable its clock output explicitly if
>> @@ -769,6 +782,18 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
>>   	rcar_du_crtc_stop(rcrtc);
>>   	rcar_du_crtc_put(rcrtc);
>>   
>> +	if ((rcdu->info->dsi_clk_mask & BIT(rcrtc->index)) &&
>> +	    (rstate->outputs &
>> +	     (BIT(RCAR_DU_OUTPUT_DSI0) | BIT(RCAR_DU_OUTPUT_DSI1)))) {
>> +		struct drm_bridge *bridge = rcdu->dsi[rcrtc->index];
>> +
>> +		/*
>> +		 * Disable the DSI clock output.
>> +		 */
>> +
>> +		rcar_mipi_dsi_atomic_late_disable(bridge);
>> +	}
>> +
>>   	if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) &&
>>   	    rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) {
>>   		struct drm_bridge *bridge = rcdu->lvds[rcrtc->index];
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
>> index 712389c7b3d0..5cfa2bb7ad93 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
>> @@ -92,6 +92,7 @@ struct rcar_du_device_info {
>>   #define RCAR_DU_MAX_GROUPS		DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2)
>>   #define RCAR_DU_MAX_VSPS		4
>>   #define RCAR_DU_MAX_LVDS		2
>> +#define RCAR_DU_MAX_DSI			2
>>   
>>   struct rcar_du_device {
>>   	struct device *dev;
>> @@ -108,6 +109,7 @@ struct rcar_du_device {
>>   	struct platform_device *cmms[RCAR_DU_MAX_CRTCS];
>>   	struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
>>   	struct drm_bridge *lvds[RCAR_DU_MAX_LVDS];
>> +	struct drm_bridge *dsi[RCAR_DU_MAX_DSI];
>>   
>>   	struct {
>>   		struct drm_property *colorkey;
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
>> index 60d6be78323b..ac93e08e8af4 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
>> @@ -84,6 +84,10 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
>>   		if (output == RCAR_DU_OUTPUT_LVDS0 ||
>>   		    output == RCAR_DU_OUTPUT_LVDS1)
>>   			rcdu->lvds[output - RCAR_DU_OUTPUT_LVDS0] = bridge;
>> +
>> +		if (output == RCAR_DU_OUTPUT_DSI0 ||
>> +		    output == RCAR_DU_OUTPUT_DSI1)
>> +			rcdu->dsi[output - RCAR_DU_OUTPUT_DSI0] = bridge;
>>   	}
>>   
>>   	/*
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
>> index 62f7eb84ab01..1ec40e40fd08 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
>> @@ -542,6 +542,34 @@ static int rcar_mipi_dsi_start_video(struct rcar_mipi_dsi *dsi)
>>   	return 0;
>>   }
>>   
>> +static void rcar_mipi_dsi_stop_video(struct rcar_mipi_dsi *dsi)
>> +{
>> +	u32 status;
>> +	int ret;
>> +
>> +	/* Disable transmission in video mode. */
>> +	rcar_mipi_dsi_clr(dsi, TXVMCR, TXVMCR_EN_VIDEO);
>> +
>> +	ret = read_poll_timeout(rcar_mipi_dsi_read, status,
>> +				!(status & TXVMSR_ACT),
>> +				2000, 100000, false, dsi, TXVMSR);
>> +	if (ret < 0) {
>> +		dev_err(dsi->dev, "Failed to disable video transmission\n");
>> +		return;
>> +	}
>> +
>> +	/* Assert video FIFO clear. */
>> +	rcar_mipi_dsi_set(dsi, TXVMCR, TXVMCR_VFCLR);
>> +
>> +	ret = read_poll_timeout(rcar_mipi_dsi_read, status,
>> +				!(status & TXVMSR_VFRDY),
>> +				2000, 100000, false, dsi, TXVMSR);
>> +	if (ret < 0) {
>> +		dev_err(dsi->dev, "Failed to assert video FIFO clear\n");
>> +		return;
>> +	}
>> +}
>> +
>>   /* -----------------------------------------------------------------------------
>>    * Bridge
>>    */
>> @@ -558,7 +586,22 @@ static int rcar_mipi_dsi_attach(struct drm_bridge *bridge,
>>   static void rcar_mipi_dsi_atomic_enable(struct drm_bridge *bridge,
>>   					struct drm_bridge_state *old_bridge_state)
>>   {
>> -	struct drm_atomic_state *state = old_bridge_state->base.state;
>> +	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
>> +
>> +	rcar_mipi_dsi_start_video(dsi);
>> +}
>> +
>> +static void rcar_mipi_dsi_atomic_disable(struct drm_bridge *bridge,
>> +					struct drm_bridge_state *old_bridge_state)
>> +{
>> +	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
>> +
>> +	rcar_mipi_dsi_stop_video(dsi);
>> +}
>> +
>> +void rcar_mipi_dsi_atomic_early_enable(struct drm_bridge *bridge,
>> +				       struct drm_atomic_state *state)
>> +{
>>   	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
>>   	const struct drm_display_mode *mode;
>>   	struct drm_connector *connector;
>> @@ -586,8 +629,6 @@ static void rcar_mipi_dsi_atomic_enable(struct drm_bridge *bridge,
>>   	if (ret < 0)
>>   		goto err_dsi_start_hs;
>>   
>> -	rcar_mipi_dsi_start_video(dsi);
>> -
>>   	return;
>>   
>>   err_dsi_start_hs:
>> @@ -595,15 +636,27 @@ static void rcar_mipi_dsi_atomic_enable(struct drm_bridge *bridge,
>>   err_dsi_startup:
>>   	rcar_mipi_dsi_clk_disable(dsi);
>>   }
>> +EXPORT_SYMBOL_GPL(rcar_mipi_dsi_atomic_early_enable);
>>   
>> -static void rcar_mipi_dsi_atomic_disable(struct drm_bridge *bridge,
>> -					 struct drm_bridge_state *old_bridge_state)
>> +void rcar_mipi_dsi_atomic_late_disable(struct drm_bridge *bridge)
>>   {
>>   	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
>>   
>> +	rcar_mipi_dsi_clr(dsi, VCLKEN, VCLKEN_CKEN);
>> +
>> +	/* Disable DOT clock */
>> +	rcar_mipi_dsi_clr(dsi, VCLKSET, VCLKSET_CKEN);
>> +
>> +	/* CFGCLK disable */
>> +	rcar_mipi_dsi_clr(dsi, CFGCLKSET, CFGCLKSET_CKEN);
>> +
>> +	/* LPCLK disable */
>> +	rcar_mipi_dsi_clr(dsi, LPCLKSET, LPCLKSET_CKEN);
>> +
>>   	rcar_mipi_dsi_shutdown(dsi);
>>   	rcar_mipi_dsi_clk_disable(dsi);
>>   }
>> +EXPORT_SYMBOL_GPL(rcar_mipi_dsi_atomic_late_disable);
>>   
>>   static enum drm_mode_status
>>   rcar_mipi_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h
>> new file mode 100644
>> index 000000000000..1764abf65a34
>> --- /dev/null
>> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * R-Car DSI Encoder
>> + *
>> + * Copyright (C) 2022 Renesas Electronics Corporation
>> + *
>> + * Contact: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> + */
>> +
>> +#ifndef __RCAR_MIPI_DSI_H__
>> +#define __RCAR_MIPI_DSI_H__
>> +
>> +struct drm_bridge;
>> +struct drm_atomic_state;
> 
> Alphabetical order please.

Ok.

>> +
>> +#if IS_ENABLED(CONFIG_DRM_RCAR_MIPI_DSI)
>> +void rcar_mipi_dsi_atomic_early_enable(struct drm_bridge *bridge,
>> +				       struct drm_atomic_state *state);
>> +void rcar_mipi_dsi_atomic_late_disable(struct drm_bridge *bridge);
> 
> It would be nice to have a naming scheme consistent with rcar_lvds.h.
> That would mean rcar_mipi_dsi_clk_{enable,disable}(), or renaming the
> LVDS functions to match whatever name would be picked here.
> 
> We could name the functions pre_enable/post_disable to show what they
> should really be, if it wasn't for the DRM atomic helpers calling the
> bridge operations at the wrong time.

DSI already has rcar_mipi_dsi_clk_enable(). I'll try to figure out a 
suitable common naming.

  Tomi

Re: [PATCH 3/3] drm: rcar-du: fix DSI enable & disable sequence

[Laurent Pinchart]

Hi Tomi,

On Mon, Aug 22, 2022 at 12:02:06PM +0300, Tomi Valkeinen wrote:
> On 19/08/2022 04:34, Laurent Pinchart wrote:
> > On Wed, Aug 17, 2022 at 04:28:03PM +0300, Tomi Valkeinen wrote:
> >> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> >>
> >> The rcar crtc depends on the clock provided from the rcar DSI bridge.
> >> When the DSI bridge is disabled, the clock is stopped, which causes the
> >> crtc disable to timeout.
> >>
> >> Also, while I have no issue with the enable, the documentation suggests
> >> to enable the DSI before the crtc so that the crtc has its clock enabled
> >> at enable time. This is also not done by the current driver.
> >>
> >> To fix this, we need to keep the DSI bridge enabled until the crtc has
> >> disabled itself, and enable the DSI bridge before crtc enables itself.
> >>
> >> Add functions (rcar_mipi_dsi_atomic_early_enable and
> >> rcar_mipi_dsi_atomic_late_disable) to the rcar DSI bridge driver which
> >> the rcar driver can use to enable/disable the DSI clock when needed.
> >> This is similar to what is already done with the rcar LVDS bridge.
> > 
> > I had hoped we could avoid that :-(
> > 
> > I wonder if we could instead rely on the pre_enable/post_disable bridge
> > operations, with a custom commit tail implementation to order those
> > before/after the CRTC enable/disable respectively. That would be pretty
> > complex though, so probably not worth it.
> 
> I think so, especially as we already have a similar case for LVDS, and 
> these custom calls are quite simple to implement and understand. I fear 
> a custom commit implementation would be a much bigger maintenance burden 
> for little, if any, benefit.
> 
> > Thinking more about this, I wonder why pre_enable is not called before
> > enabling the CRTC in the DRM atomic helpers. That would make more sense
> > to me, but I suppose changing it would break too many things ? I think
> > it should at least be discussed to figure out if it was a historical
> > oversight or if there was a good reason. It's *really* not nice to poke
> > holes through the abstraction layers like this.
> 
> Yes, I'll bring this question to #dri-devel. But I think it's better to 
> get the issue fixed with a custom function call as done in this patch, 
> and hope that we can do the work in a standard way in the future.

That's fine. If it turns out that the result of the discussion on
#dri-devel is that this should be fixed in the DRM core, with a simple
and clear path forward, then the next version of this series could be
based on that, otherwise I'm fine with this fix.

> >> Also add a new function, rcar_mipi_dsi_stop_video(), called from
> >> rcar_mipi_dsi_atomic_enable so that the DSI TX gets disabled at the
> >> correct time, even if the clocks are still kept enabled.
> > 
> > I think this should be split to a separate patch, possibly before this
> > one, it addresses a separate issue.
> 
> I agree.
> 
> >> And, while possibly not strictly needed, clear clock related enable bits
> >> in rcar_mipi_dsi_atomic_late_disable to mirror the sequence done in
> >> rcar_mipi_dsi_startup() (via rcar_mipi_dsi_atomic_early_enable).
> > 
> > And this too should be a separate patch, possibly bundled with
> > rcar_mipi_dsi_stop_video().
> 
> Yep. I'll have it in a separate patch as they're somewhat different.
> 
> > Have you checked in the BSP code to see if they do the same at disable
> > time ?
> 
> No, they don't seem to do this.
> 
> The steps for stopping of the video transmission is described in the 
> doc, but there's no mention I can see of clearing those bits (or rather, 
> making sure they are cleared before starting the next enable sequence).
> 
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> >> ---
> >>   drivers/gpu/drm/rcar-du/rcar_du_crtc.c    | 25 +++++++++
> >>   drivers/gpu/drm/rcar-du/rcar_du_drv.h     |  2 +
> >>   drivers/gpu/drm/rcar-du/rcar_du_encoder.c |  4 ++
> >>   drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c   | 63 +++++++++++++++++++++--
> >>   drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h   | 32 ++++++++++++
> >>   5 files changed, 121 insertions(+), 5 deletions(-)
> >>   create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h
> >>
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> index fd3b94649a01..51fd1d99f4e8 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> @@ -29,6 +29,7 @@
> >>   #include "rcar_du_regs.h"
> >>   #include "rcar_du_vsp.h"
> >>   #include "rcar_lvds.h"
> >> +#include "rcar_mipi_dsi.h"
> >>   
> >>   static u32 rcar_du_crtc_read(struct rcar_du_crtc *rcrtc, u32 reg)
> >>   {
> >> @@ -733,6 +734,18 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
> >>   		rcar_cmm_enable(rcrtc->cmm);
> >>   	rcar_du_crtc_get(rcrtc);
> >>   
> > 
> > A comment here similar to the LVDS case would be useful. I would
> > actually move this below the LVDS code, and write
> > 
> > 	/*
> > 	 * Similarly, on V3U, ...
> > 	 */
> 
> Ok.
> 
> >> +	if ((rcdu->info->dsi_clk_mask & BIT(rcrtc->index)) &&
> >> +	    (rstate->outputs &
> >> +	     (BIT(RCAR_DU_OUTPUT_DSI0) | BIT(RCAR_DU_OUTPUT_DSI1)))) {
> >> +		struct drm_bridge *bridge = rcdu->dsi[rcrtc->index];
> >> +
> >> +		/*
> >> +		 * Enable the DSI clock output.
> >> +		 */
> >> +
> >> +		rcar_mipi_dsi_atomic_early_enable(bridge, state);
> >> +	}
> > 
> > I was wondering if we could merge the DSI and LVDS clock enabling code,
> > including merging the lvds and dsi fields in rcar_du_device, but it
> > doesn't seem it will be very clean here.
> 
> True, they are quite similar. I didn't want to mix them up as I don't 
> have the means to test lvds, nor am I that familiar with the rcar du.

I can test it, but I don't think the resulting code would be very clean
anyway.

> If I'm not mistaken, the difference is that LVDS clock is used for 
> non-LVDS output, whereas here the DSI clock is used for DSI output.

That's correct. That's why I initially thought we could avoid enabling
the clock manually for DSI.

> >> +
> >>   	/*
> >>   	 * On D3/E3 the dot clock is provided by the LVDS encoder attached to
> >>   	 * the DU channel. We need to enable its clock output explicitly if
> >> @@ -769,6 +782,18 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
> >>   	rcar_du_crtc_stop(rcrtc);
> >>   	rcar_du_crtc_put(rcrtc);
> >>   
> >> +	if ((rcdu->info->dsi_clk_mask & BIT(rcrtc->index)) &&
> >> +	    (rstate->outputs &
> >> +	     (BIT(RCAR_DU_OUTPUT_DSI0) | BIT(RCAR_DU_OUTPUT_DSI1)))) {
> >> +		struct drm_bridge *bridge = rcdu->dsi[rcrtc->index];
> >> +
> >> +		/*
> >> +		 * Disable the DSI clock output.
> >> +		 */
> >> +
> >> +		rcar_mipi_dsi_atomic_late_disable(bridge);
> >> +	}
> >> +
> >>   	if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) &&
> >>   	    rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) {
> >>   		struct drm_bridge *bridge = rcdu->lvds[rcrtc->index];
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> >> index 712389c7b3d0..5cfa2bb7ad93 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> >> @@ -92,6 +92,7 @@ struct rcar_du_device_info {
> >>   #define RCAR_DU_MAX_GROUPS		DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2)
> >>   #define RCAR_DU_MAX_VSPS		4
> >>   #define RCAR_DU_MAX_LVDS		2
> >> +#define RCAR_DU_MAX_DSI			2
> >>   
> >>   struct rcar_du_device {
> >>   	struct device *dev;
> >> @@ -108,6 +109,7 @@ struct rcar_du_device {
> >>   	struct platform_device *cmms[RCAR_DU_MAX_CRTCS];
> >>   	struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
> >>   	struct drm_bridge *lvds[RCAR_DU_MAX_LVDS];
> >> +	struct drm_bridge *dsi[RCAR_DU_MAX_DSI];
> >>   
> >>   	struct {
> >>   		struct drm_property *colorkey;
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> >> index 60d6be78323b..ac93e08e8af4 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> >> @@ -84,6 +84,10 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> >>   		if (output == RCAR_DU_OUTPUT_LVDS0 ||
> >>   		    output == RCAR_DU_OUTPUT_LVDS1)
> >>   			rcdu->lvds[output - RCAR_DU_OUTPUT_LVDS0] = bridge;
> >> +
> >> +		if (output == RCAR_DU_OUTPUT_DSI0 ||
> >> +		    output == RCAR_DU_OUTPUT_DSI1)
> >> +			rcdu->dsi[output - RCAR_DU_OUTPUT_DSI0] = bridge;
> >>   	}
> >>   
> >>   	/*
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> >> index 62f7eb84ab01..1ec40e40fd08 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> >> @@ -542,6 +542,34 @@ static int rcar_mipi_dsi_start_video(struct rcar_mipi_dsi *dsi)
> >>   	return 0;
> >>   }
> >>   
> >> +static void rcar_mipi_dsi_stop_video(struct rcar_mipi_dsi *dsi)
> >> +{
> >> +	u32 status;
> >> +	int ret;
> >> +
> >> +	/* Disable transmission in video mode. */
> >> +	rcar_mipi_dsi_clr(dsi, TXVMCR, TXVMCR_EN_VIDEO);
> >> +
> >> +	ret = read_poll_timeout(rcar_mipi_dsi_read, status,
> >> +				!(status & TXVMSR_ACT),
> >> +				2000, 100000, false, dsi, TXVMSR);
> >> +	if (ret < 0) {
> >> +		dev_err(dsi->dev, "Failed to disable video transmission\n");
> >> +		return;
> >> +	}
> >> +
> >> +	/* Assert video FIFO clear. */
> >> +	rcar_mipi_dsi_set(dsi, TXVMCR, TXVMCR_VFCLR);
> >> +
> >> +	ret = read_poll_timeout(rcar_mipi_dsi_read, status,
> >> +				!(status & TXVMSR_VFRDY),
> >> +				2000, 100000, false, dsi, TXVMSR);
> >> +	if (ret < 0) {
> >> +		dev_err(dsi->dev, "Failed to assert video FIFO clear\n");
> >> +		return;
> >> +	}
> >> +}
> >> +
> >>   /* -----------------------------------------------------------------------------
> >>    * Bridge
> >>    */
> >> @@ -558,7 +586,22 @@ static int rcar_mipi_dsi_attach(struct drm_bridge *bridge,
> >>   static void rcar_mipi_dsi_atomic_enable(struct drm_bridge *bridge,
> >>   					struct drm_bridge_state *old_bridge_state)
> >>   {
> >> -	struct drm_atomic_state *state = old_bridge_state->base.state;
> >> +	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
> >> +
> >> +	rcar_mipi_dsi_start_video(dsi);
> >> +}
> >> +
> >> +static void rcar_mipi_dsi_atomic_disable(struct drm_bridge *bridge,
> >> +					struct drm_bridge_state *old_bridge_state)
> >> +{
> >> +	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
> >> +
> >> +	rcar_mipi_dsi_stop_video(dsi);
> >> +}
> >> +
> >> +void rcar_mipi_dsi_atomic_early_enable(struct drm_bridge *bridge,
> >> +				       struct drm_atomic_state *state)
> >> +{
> >>   	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
> >>   	const struct drm_display_mode *mode;
> >>   	struct drm_connector *connector;
> >> @@ -586,8 +629,6 @@ static void rcar_mipi_dsi_atomic_enable(struct drm_bridge *bridge,
> >>   	if (ret < 0)
> >>   		goto err_dsi_start_hs;
> >>   
> >> -	rcar_mipi_dsi_start_video(dsi);
> >> -
> >>   	return;
> >>   
> >>   err_dsi_start_hs:
> >> @@ -595,15 +636,27 @@ static void rcar_mipi_dsi_atomic_enable(struct drm_bridge *bridge,
> >>   err_dsi_startup:
> >>   	rcar_mipi_dsi_clk_disable(dsi);
> >>   }
> >> +EXPORT_SYMBOL_GPL(rcar_mipi_dsi_atomic_early_enable);
> >>   
> >> -static void rcar_mipi_dsi_atomic_disable(struct drm_bridge *bridge,
> >> -					 struct drm_bridge_state *old_bridge_state)
> >> +void rcar_mipi_dsi_atomic_late_disable(struct drm_bridge *bridge)
> >>   {
> >>   	struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge);
> >>   
> >> +	rcar_mipi_dsi_clr(dsi, VCLKEN, VCLKEN_CKEN);
> >> +
> >> +	/* Disable DOT clock */
> >> +	rcar_mipi_dsi_clr(dsi, VCLKSET, VCLKSET_CKEN);
> >> +
> >> +	/* CFGCLK disable */
> >> +	rcar_mipi_dsi_clr(dsi, CFGCLKSET, CFGCLKSET_CKEN);
> >> +
> >> +	/* LPCLK disable */
> >> +	rcar_mipi_dsi_clr(dsi, LPCLKSET, LPCLKSET_CKEN);
> >> +
> >>   	rcar_mipi_dsi_shutdown(dsi);
> >>   	rcar_mipi_dsi_clk_disable(dsi);
> >>   }
> >> +EXPORT_SYMBOL_GPL(rcar_mipi_dsi_atomic_late_disable);
> >>   
> >>   static enum drm_mode_status
> >>   rcar_mipi_dsi_bridge_mode_valid(struct drm_bridge *bridge,
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h
> >> new file mode 100644
> >> index 000000000000..1764abf65a34
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.h
> >> @@ -0,0 +1,32 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * R-Car DSI Encoder
> >> + *
> >> + * Copyright (C) 2022 Renesas Electronics Corporation
> >> + *
> >> + * Contact: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> + */
> >> +
> >> +#ifndef __RCAR_MIPI_DSI_H__
> >> +#define __RCAR_MIPI_DSI_H__
> >> +
> >> +struct drm_bridge;
> >> +struct drm_atomic_state;
> > 
> > Alphabetical order please.
> 
> Ok.
> 
> >> +
> >> +#if IS_ENABLED(CONFIG_DRM_RCAR_MIPI_DSI)
> >> +void rcar_mipi_dsi_atomic_early_enable(struct drm_bridge *bridge,
> >> +				       struct drm_atomic_state *state);
> >> +void rcar_mipi_dsi_atomic_late_disable(struct drm_bridge *bridge);
> > 
> > It would be nice to have a naming scheme consistent with rcar_lvds.h.
> > That would mean rcar_mipi_dsi_clk_{enable,disable}(), or renaming the
> > LVDS functions to match whatever name would be picked here.
> > 
> > We could name the functions pre_enable/post_disable to show what they
> > should really be, if it wasn't for the DRM atomic helpers calling the
> > bridge operations at the wrong time.
> 
> DSI already has rcar_mipi_dsi_clk_enable(). I'll try to figure out a 
> suitable common naming.

-- 
Regards,

Laurent Pinchart