[PATCH 0/5] imx-lcdif modeset changes

[PATCH 0/5] imx-lcdif modeset changes

[Lucas Stach]

This changes the modeset flow in the imx-lcdif driver to work
better with runtime PM and get rid of duplicate hardware setup.

This series is a result of looking a bit more into the issue and
feedback received on some earlier patches [1].

Regards,
Lucas

[1] https://lore.kernel.org/all/CAOcKUNWMU6tjWwtnU+COggrr--G19EvZaHrxAp0-0i5dK394jg@mail.gmail.com/

Lucas Stach (5):
  drm: lcdif: improve burst size configuration comment
  drm: lcdif: move controller enable into atomic_flush
  drm: lcdif: remove superfluous setup of framebuffer DMA address
  drm: lcdif: move pitch setup to plane atomic update
  drm: lcdif: force modeset when FB format changes

 drivers/gpu/drm/mxsfb/lcdif_drv.c | 18 +++++++-
 drivers/gpu/drm/mxsfb/lcdif_kms.c | 68 +++++++++++++++++--------------
 2 files changed, 55 insertions(+), 31 deletions(-)

-- 
2.39.2

[PATCH 1/5] drm: lcdif: improve burst size configuration comment

[Lucas Stach]

The comment regarding AXI bust size configuration is a bit hard
to read. Improve the wording somewhat.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/mxsfb/lcdif_kms.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index 2541d2de4e45..f5bfe8b52920 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -329,12 +329,12 @@ static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags)
 	       lcdif->base + LCDC_V8_CTRLDESCL0_1);
 
 	/*
-	 * Undocumented P_SIZE and T_SIZE register but those written in the
-	 * downstream kernel those registers control the AXI burst size. As of
-	 * now there are two known values:
+	 * Undocumented P_SIZE and T_SIZE bitfields written in the downstream
+	 * driver. Those bitfields control the AXI burst size. As of now there
+	 * are two known values:
 	 *  1 - 128Byte
 	 *  2 - 256Byte
-	 * Downstream set it to 256B burst size to improve the memory
+	 * Downstream sets this to 256B burst size to improve the memory access
 	 * efficiency so set it here too.
 	 */
 	ctrl = CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) |
-- 
2.39.2

[PATCH 2/5] drm: lcdif: move controller enable into atomic_flush

[Lucas Stach]

Allow drm_atomic_helper_commit_tail_rpm to setup all the plane
state before the scanout is started.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/mxsfb/lcdif_kms.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index f5bfe8b52920..4acf6914a8d1 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -505,6 +505,8 @@ static int lcdif_crtc_atomic_check(struct drm_crtc *crtc,
 static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
 				    struct drm_atomic_state *state)
 {
+	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
+									  crtc);
 	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev);
 	struct drm_pending_vblank_event *event;
 	u32 reg;
@@ -513,6 +515,9 @@ static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
 	reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
 	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
 
+	if (drm_atomic_crtc_needs_modeset(crtc_state))
+		lcdif_enable_controller(lcdif);
+
 	event = crtc->state->event;
 	crtc->state->event = NULL;
 
@@ -552,7 +557,6 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc,
 		writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
 		       lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
 	}
-	lcdif_enable_controller(lcdif);
 
 	drm_crtc_vblank_on(crtc);
 }
-- 
2.39.2

[PATCH 3/5] drm: lcdif: remove superfluous setup of framebuffer DMA address

[Lucas Stach]

Now that the plane state is fully programmed into the hardware before
the scanout is started there is no need to program the plane framebuffer
DMA address from the CRTC atomic_enable anymore.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/mxsfb/lcdif_kms.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index 4acf6914a8d1..33a082366b25 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -541,7 +541,6 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc,
 									    crtc->primary);
 	struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode;
 	struct drm_device *drm = lcdif->drm;
-	dma_addr_t paddr;
 
 	clk_set_rate(lcdif->clk, m->crtc_clock * 1000);
 
@@ -549,15 +548,6 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	lcdif_crtc_mode_set_nofb(new_cstate, new_pstate);
 
-	/* Write cur_buf as well to avoid an initial corrupt frame */
-	paddr = drm_fb_dma_get_gem_addr(new_pstate->fb, new_pstate, 0);
-	if (paddr) {
-		writel(lower_32_bits(paddr),
-		       lcdif->base + LCDC_V8_CTRLDESCL_LOW0_4);
-		writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
-		       lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
-	}
-
 	drm_crtc_vblank_on(crtc);
 }
 
-- 
2.39.2

[PATCH 4/5] drm: lcdif: move pitch setup to plane atomic update

[Lucas Stach]

The buffer pitch may change when switching the buffer on a
atomic update. As the register is double buffered it can be
safely changed while the display is active.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/mxsfb/lcdif_kms.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index 33a082366b25..3ebf55d06027 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -327,19 +327,6 @@ static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags)
 	writel(CTRLDESCL0_1_HEIGHT(m->vdisplay) |
 	       CTRLDESCL0_1_WIDTH(m->hdisplay),
 	       lcdif->base + LCDC_V8_CTRLDESCL0_1);
-
-	/*
-	 * Undocumented P_SIZE and T_SIZE bitfields written in the downstream
-	 * driver. Those bitfields control the AXI burst size. As of now there
-	 * are two known values:
-	 *  1 - 128Byte
-	 *  2 - 256Byte
-	 * Downstream sets this to 256B burst size to improve the memory access
-	 * efficiency so set it here too.
-	 */
-	ctrl = CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) |
-	       CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state->fb->pitches[0]);
-	writel(ctrl, lcdif->base + LCDC_V8_CTRLDESCL0_3);
 }
 
 static void lcdif_enable_controller(struct lcdif_drm_private *lcdif)
@@ -689,6 +676,19 @@ static void lcdif_plane_primary_atomic_update(struct drm_plane *plane,
 		writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
 		       lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
 	}
+
+	/*
+	 * Undocumented P_SIZE and T_SIZE bitfields written in the downstream
+	 * driver. Those bitfields control the AXI burst size. As of now there
+	 * are two known values:
+	 *  1 - 128Byte
+	 *  2 - 256Byte
+	 * Downstream sets this to 256B burst size to improve the memory access
+	 * efficiency so set it here too.
+	 */
+	writel(CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) |
+	       CTRLDESCL0_3_PITCH(new_pstate->fb->pitches[0]),
+	       lcdif->base + LCDC_V8_CTRLDESCL0_3);
 }
 
 static bool lcdif_format_mod_supported(struct drm_plane *plane,
-- 
2.39.2

[PATCH 5/5] drm: lcdif: force modeset when FB format changes

[Lucas Stach]

Force a modeset if the new FB has a different format than the
currently active one. While it might be possible to change between
compatible formats without a full modeset as the format control is
also supposed to be double buffered, the colorspace conversion is
not, so when the CSC changes we need a modeset.

For now just always force a modeset when the FB format changes to
properly reconfigure all parts of the device for the new format.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/mxsfb/lcdif_drv.c | 18 +++++++++++++++++-
 drivers/gpu/drm/mxsfb/lcdif_kms.c | 26 ++++++++++++++++++++------
 2 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c b/drivers/gpu/drm/mxsfb/lcdif_drv.c
index 18de2f17e249..b74f0cf1e240 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
@@ -30,9 +30,25 @@
 #include "lcdif_drv.h"
 #include "lcdif_regs.h"
 
+static int lcdif_atomic_check(struct drm_device *dev,
+				struct drm_atomic_state *state)
+{
+	int ret;
+
+	ret = drm_atomic_helper_check(dev, state);
+	if (ret)
+		return ret;
+
+	/*
+	 * Check modeset again in case crtc_state->mode_changed is
+	 * updated in plane's ->atomic_check callback.
+	 */
+	return drm_atomic_helper_check_modeset(dev, state);
+}
+
 static const struct drm_mode_config_funcs lcdif_mode_config_funcs = {
 	.fb_create		= drm_gem_fb_create,
-	.atomic_check		= drm_atomic_helper_check,
+	.atomic_check		= lcdif_atomic_check,
 	.atomic_commit		= drm_atomic_helper_commit,
 };
 
diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index 3ebf55d06027..8167604bd3f8 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -647,18 +647,32 @@ static const struct drm_crtc_funcs lcdif_crtc_funcs = {
 static int lcdif_plane_atomic_check(struct drm_plane *plane,
 				    struct drm_atomic_state *state)
 {
-	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state,
-									     plane);
+	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
+									   plane);
+	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
+									   plane);
 	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(plane->dev);
 	struct drm_crtc_state *crtc_state;
+	int ret;
+
+	/* always okay to disable the plane */
+	if (!new_state->fb)
+		return 0;
 
 	crtc_state = drm_atomic_get_new_crtc_state(state,
 						   &lcdif->crtc);
 
-	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
-						   DRM_PLANE_NO_SCALING,
-						   DRM_PLANE_NO_SCALING,
-						   false, true);
+	ret = drm_atomic_helper_check_plane_state(new_state, crtc_state,
+						  DRM_PLANE_NO_SCALING,
+						  DRM_PLANE_NO_SCALING,
+						  false, true);
+	if (ret)
+		return ret;
+
+	if (old_state->fb && new_state->fb->format != old_state->fb->format)
+		crtc_state->mode_changed = true;
+
+	return 0;
 }
 
 static void lcdif_plane_primary_atomic_update(struct drm_plane *plane,
-- 
2.39.2

Re: [PATCH 1/5] drm: lcdif: improve burst size configuration comment

[Marco Felsch]

On 23-09-20, Lucas Stach wrote:
> The comment regarding AXI bust size configuration is a bit hard
> to read. Improve the wording somewhat.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

Re: [PATCH 1/5] drm: lcdif: improve burst size configuration comment

[Marek Vasut]

On 9/20/23 12:31, Lucas Stach wrote:
> The comment regarding AXI bust size configuration is a bit hard
> to read. Improve the wording somewhat.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>   drivers/gpu/drm/mxsfb/lcdif_kms.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index 2541d2de4e45..f5bfe8b52920 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -329,12 +329,12 @@ static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags)
>   	       lcdif->base + LCDC_V8_CTRLDESCL0_1);
>   
>   	/*
> -	 * Undocumented P_SIZE and T_SIZE register but those written in the
> -	 * downstream kernel those registers control the AXI burst size. As of
> -	 * now there are two known values:
> +	 * Undocumented P_SIZE and T_SIZE bitfields written in the downstream

s@written@documented only@ or so, since yeah, those fields are only used 
in the downstream driver and miss in the TRM .

> +	 * driver. Those bitfields control the AXI burst size. As of now there
> +	 * are two known values:
>   	 *  1 - 128Byte
>   	 *  2 - 256Byte
> -	 * Downstream set it to 256B burst size to improve the memory
> +	 * Downstream sets this to 256B burst size to improve the memory access
>   	 * efficiency so set it here too.

With that fixed:

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 2/5] drm: lcdif: move controller enable into atomic_flush

[Marek Vasut]

On 9/20/23 12:31, Lucas Stach wrote:
> Allow drm_atomic_helper_commit_tail_rpm to setup all the plane
> state before the scanout is started.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>   drivers/gpu/drm/mxsfb/lcdif_kms.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index f5bfe8b52920..4acf6914a8d1 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -505,6 +505,8 @@ static int lcdif_crtc_atomic_check(struct drm_crtc *crtc,
>   static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
>   				    struct drm_atomic_state *state)
>   {
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +									  crtc);
>   	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev);
>   	struct drm_pending_vblank_event *event;
>   	u32 reg;
> @@ -513,6 +515,9 @@ static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
>   	reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
>   	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
>   
> +	if (drm_atomic_crtc_needs_modeset(crtc_state))
> +		lcdif_enable_controller(lcdif);
> +
>   	event = crtc->state->event;
>   	crtc->state->event = NULL;
>   
> @@ -552,7 +557,6 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc,
>   		writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
>   		       lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
>   	}
> -	lcdif_enable_controller(lcdif);
>   
>   	drm_crtc_vblank_on(crtc);
>   }

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 3/5] drm: lcdif: remove superfluous setup of framebuffer DMA address

[Marek Vasut]

On 9/20/23 12:31, Lucas Stach wrote:
> Now that the plane state is fully programmed into the hardware before
> the scanout is started there is no need to program the plane framebuffer
> DMA address from the CRTC atomic_enable anymore.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>   drivers/gpu/drm/mxsfb/lcdif_kms.c | 10 ----------
>   1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index 4acf6914a8d1..33a082366b25 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -541,7 +541,6 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc,
>   									    crtc->primary);
>   	struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode;
>   	struct drm_device *drm = lcdif->drm;
> -	dma_addr_t paddr;
>   
>   	clk_set_rate(lcdif->clk, m->crtc_clock * 1000);
>   
> @@ -549,15 +548,6 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc,
>   
>   	lcdif_crtc_mode_set_nofb(new_cstate, new_pstate);
>   
> -	/* Write cur_buf as well to avoid an initial corrupt frame */
> -	paddr = drm_fb_dma_get_gem_addr(new_pstate->fb, new_pstate, 0);
> -	if (paddr) {
> -		writel(lower_32_bits(paddr),
> -		       lcdif->base + LCDC_V8_CTRLDESCL_LOW0_4);
> -		writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
> -		       lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
> -	}
> -
>   	drm_crtc_vblank_on(crtc);
>   }

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 4/5] drm: lcdif: move pitch setup to plane atomic update

[Marek Vasut]

On 9/20/23 12:31, Lucas Stach wrote:
> The buffer pitch may change when switching the buffer on a
> atomic update. As the register is double buffered it can be
> safely changed while the display is active.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>   drivers/gpu/drm/mxsfb/lcdif_kms.c | 26 +++++++++++++-------------
>   1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index 33a082366b25..3ebf55d06027 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -327,19 +327,6 @@ static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags)
>   	writel(CTRLDESCL0_1_HEIGHT(m->vdisplay) |
>   	       CTRLDESCL0_1_WIDTH(m->hdisplay),
>   	       lcdif->base + LCDC_V8_CTRLDESCL0_1);
> -
> -	/*
> -	 * Undocumented P_SIZE and T_SIZE bitfields written in the downstream
> -	 * driver. Those bitfields control the AXI burst size. As of now there
> -	 * are two known values:
> -	 *  1 - 128Byte
> -	 *  2 - 256Byte
> -	 * Downstream sets this to 256B burst size to improve the memory access
> -	 * efficiency so set it here too.
> -	 */
> -	ctrl = CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) |
> -	       CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state->fb->pitches[0]);
> -	writel(ctrl, lcdif->base + LCDC_V8_CTRLDESCL0_3);
>   }
>   
>   static void lcdif_enable_controller(struct lcdif_drm_private *lcdif)
> @@ -689,6 +676,19 @@ static void lcdif_plane_primary_atomic_update(struct drm_plane *plane,
>   		writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
>   		       lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
>   	}
> +
> +	/*
> +	 * Undocumented P_SIZE and T_SIZE bitfields written in the downstream
> +	 * driver. Those bitfields control the AXI burst size. As of now there
> +	 * are two known values:
> +	 *  1 - 128Byte
> +	 *  2 - 256Byte
> +	 * Downstream sets this to 256B burst size to improve the memory access
> +	 * efficiency so set it here too.
> +	 */
> +	writel(CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) |
> +	       CTRLDESCL0_3_PITCH(new_pstate->fb->pitches[0]),
> +	       lcdif->base + LCDC_V8_CTRLDESCL0_3);
>   }
>   
>   static bool lcdif_format_mod_supported(struct drm_plane *plane,

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 5/5] drm: lcdif: force modeset when FB format changes

[Marek Vasut]

On 9/20/23 12:31, Lucas Stach wrote:
> Force a modeset if the new FB has a different format than the
> currently active one. While it might be possible to change between
> compatible formats without a full modeset as the format control is
> also supposed to be double buffered, the colorspace conversion is
> not, so when the CSC changes we need a modeset.
> 
> For now just always force a modeset when the FB format changes to
> properly reconfigure all parts of the device for the new format.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>   drivers/gpu/drm/mxsfb/lcdif_drv.c | 18 +++++++++++++++++-
>   drivers/gpu/drm/mxsfb/lcdif_kms.c | 26 ++++++++++++++++++++------
>   2 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> index 18de2f17e249..b74f0cf1e240 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> @@ -30,9 +30,25 @@
>   #include "lcdif_drv.h"
>   #include "lcdif_regs.h"
>   
> +static int lcdif_atomic_check(struct drm_device *dev,
> +				struct drm_atomic_state *state)
> +{
> +	int ret;
> +
> +	ret = drm_atomic_helper_check(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Check modeset again in case crtc_state->mode_changed is
> +	 * updated in plane's ->atomic_check callback.
> +	 */
> +	return drm_atomic_helper_check_modeset(dev, state);
> +}
> +
>   static const struct drm_mode_config_funcs lcdif_mode_config_funcs = {
>   	.fb_create		= drm_gem_fb_create,
> -	.atomic_check		= drm_atomic_helper_check,
> +	.atomic_check		= lcdif_atomic_check,
>   	.atomic_commit		= drm_atomic_helper_commit,
>   };
>   
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index 3ebf55d06027..8167604bd3f8 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -647,18 +647,32 @@ static const struct drm_crtc_funcs lcdif_crtc_funcs = {
>   static int lcdif_plane_atomic_check(struct drm_plane *plane,
>   				    struct drm_atomic_state *state)
>   {
> -	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state,
> -									     plane);
> +	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
> +									   plane);
> +	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
> +									   plane);
>   	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(plane->dev);
>   	struct drm_crtc_state *crtc_state;
> +	int ret;
> +
> +	/* always okay to disable the plane */
> +	if (!new_state->fb)
> +		return 0;
>   
>   	crtc_state = drm_atomic_get_new_crtc_state(state,
>   						   &lcdif->crtc);
>   
> -	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> -						   DRM_PLANE_NO_SCALING,
> -						   DRM_PLANE_NO_SCALING,
> -						   false, true);
> +	ret = drm_atomic_helper_check_plane_state(new_state, crtc_state,
> +						  DRM_PLANE_NO_SCALING,
> +						  DRM_PLANE_NO_SCALING,
> +						  false, true);
> +	if (ret)
> +		return ret;
> +
> +	if (old_state->fb && new_state->fb->format != old_state->fb->format)
> +		crtc_state->mode_changed = true;
> +
> +	return 0;
>   }
>   
>   static void lcdif_plane_primary_atomic_update(struct drm_plane *plane,

Reviewed-by: Marek Vasut <marex@denx.de>

RE: [PATCH 0/5] imx-lcdif modeset changes

[Ying Liu]

Hi Lucas,

Thank you for the patch series.

On Wednesday, September 20, 2023 6:31 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> This changes the modeset flow in the imx-lcdif driver to work
> better with runtime PM and get rid of duplicate hardware setup.

I'm assuming the duplicate hardware setup is calling
lcdif_plane_primary_atomic_update() in lcdif_crtc_atomic_enable()
as I mentioned in [1].  If that's the case, it looks like the call is
inevitable due to CRTC/bridge enablement sequence.  See detail
comments for patch 2/5.

Regards,
Liu Ying

>
> This series is a result of looking a bit more into the issue and
> feedback received on some earlier patches [1].
>
> Regards,
> Lucas
>
> [1]
> https://lore.k/
> ernel.org%2Fall%2FCAOcKUNWMU6tjWwtnU%2BCOggrr--G19EvZaHrxAp0-
> 0i5dK394jg%40mail.gmail.com%2F&data=05%7C01%7Cvictor.liu%40nxp.com
> %7C3dedd482af2a4a3d9d3f08dbb9c4bfe8%7C686ea1d3bc2b4c6fa92cd99c5c
> 301635%7C0%7C0%7C638308026921385573%7CUnknown%7CTWFpbGZsb3d
> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C3000%7C%7C%7C&sdata=%2BxMk6cO%2Fxpoip2xChoFcHtkhLWNmi%2
> FngtNm%2BBu2kr6M%3D&reserved=0
>
> Lucas Stach (5):
>   drm: lcdif: improve burst size configuration comment
>   drm: lcdif: move controller enable into atomic_flush
>   drm: lcdif: remove superfluous setup of framebuffer DMA address
>   drm: lcdif: move pitch setup to plane atomic update
>   drm: lcdif: force modeset when FB format changes
>
>  drivers/gpu/drm/mxsfb/lcdif_drv.c | 18 +++++++-
>  drivers/gpu/drm/mxsfb/lcdif_kms.c | 68 +++++++++++++++++--------------
>  2 files changed, 55 insertions(+), 31 deletions(-)
>
> --
> 2.39.2

RE: [PATCH 2/5] drm: lcdif: move controller enable into atomic_flush

[Ying Liu]

Hi,

On Wednesday, September 20, 2023 6:31 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> Allow drm_atomic_helper_commit_tail_rpm to setup all the plane
> state before the scanout is started.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/mxsfb/lcdif_kms.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index f5bfe8b52920..4acf6914a8d1 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -505,6 +505,8 @@ static int lcdif_crtc_atomic_check(struct drm_crtc
> *crtc,
>  static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
>  				    struct drm_atomic_state *state)
>  {
> +	struct drm_crtc_state *crtc_state =
> drm_atomic_get_new_crtc_state(state,
> +									  crtc);
>  	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev);
>  	struct drm_pending_vblank_event *event;
>  	u32 reg;
> @@ -513,6 +515,9 @@ static void lcdif_crtc_atomic_flush(struct drm_crtc
> *crtc,
>  	reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
>  	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> 
> +	if (drm_atomic_crtc_needs_modeset(crtc_state))
> +		lcdif_enable_controller(lcdif);

Moving lcdif_enable_controller() function call from atomic_enable to
atomic_flush would change CRTC vs bridge enablement order, which
effectively makes bridge enablement happen prior to CRTC enablement,
see drm_atomic_helper_commit_tail_rpm() detail implementation.  The
reversed order potentially causes malfunctions of the bridge.

BTW, even if it's ok to move the function call, it would be better to call
lcdif_enable_controller() before CTRLDESCL0_5_SHADOW_LOAD_EN is
set so that the original sequence is kept.  Also, LCDIF chapter in SoC RMs
indicates that the shadow load enablement is the last step in
"Software flow diagram".

Regards,
Liu Ying

> +
>  	event = crtc->state->event;
>  	crtc->state->event = NULL;
> 
> @@ -552,7 +557,6 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc
> *crtc,
> 
> 	writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
>  		       lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
>  	}
> -	lcdif_enable_controller(lcdif);
> 
>  	drm_crtc_vblank_on(crtc);
>  }
> --
> 2.39.2

RE: [PATCH 5/5] drm: lcdif: force modeset when FB format changes

[Ying Liu]

Hi,

On Wednesday, September 20, 2023 6:31 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> Force a modeset if the new FB has a different format than the
> currently active one. While it might be possible to change between
> compatible formats without a full modeset as the format control is
> also supposed to be double buffered, the colorspace conversion is
> not, so when the CSC changes we need a modeset.
> 
> For now just always force a modeset when the FB format changes to
> properly reconfigure all parts of the device for the new format.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/mxsfb/lcdif_drv.c | 18 +++++++++++++++++-
>  drivers/gpu/drm/mxsfb/lcdif_kms.c | 26 ++++++++++++++++++++------
>  2 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> index 18de2f17e249..b74f0cf1e240 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> @@ -30,9 +30,25 @@
>  #include "lcdif_drv.h"
>  #include "lcdif_regs.h"
> 
> +static int lcdif_atomic_check(struct drm_device *dev,
> +				struct drm_atomic_state *state)

" checkpatch.pl --strict" complains:
CHECK: Alignment should match open parenthesis
#31: FILE: drivers/gpu/drm/mxsfb/lcdif_drv.c:34:
+static int lcdif_atomic_check(struct drm_device *dev,
+                               struct drm_atomic_state *state)

> +{
> +	int ret;
> +
> +	ret = drm_atomic_helper_check(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Check modeset again in case crtc_state->mode_changed is
> +	 * updated in plane's ->atomic_check callback.
> +	 */
> +	return drm_atomic_helper_check_modeset(dev, state);

This additional check looks fine, but it's done for every commit.
Is it ok to move it to lcdif_plane_atomic_check() where mode_changed
is set for those commits in question?

Regards,
Liu Ying

> +}
> +
>  static const struct drm_mode_config_funcs lcdif_mode_config_funcs = {
>  	.fb_create		= drm_gem_fb_create,
> -	.atomic_check		= drm_atomic_helper_check,
> +	.atomic_check		= lcdif_atomic_check,
>  	.atomic_commit		= drm_atomic_helper_commit,
>  };
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index 3ebf55d06027..8167604bd3f8 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -647,18 +647,32 @@ static const struct drm_crtc_funcs lcdif_crtc_funcs
> = {
>  static int lcdif_plane_atomic_check(struct drm_plane *plane,
>  				    struct drm_atomic_state *state)
>  {
> -	struct drm_plane_state *plane_state =
> drm_atomic_get_new_plane_state(state,
> -
> plane);
> +	struct drm_plane_state *new_state =
> drm_atomic_get_new_plane_state(state,
> +
> plane);
> +	struct drm_plane_state *old_state =
> drm_atomic_get_old_plane_state(state,
> +
> plane);
>  	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(plane->dev);
>  	struct drm_crtc_state *crtc_state;
> +	int ret;
> +
> +	/* always okay to disable the plane */
> +	if (!new_state->fb)
> +		return 0;
> 
>  	crtc_state = drm_atomic_get_new_crtc_state(state,
>  						   &lcdif->crtc);
> 
> -	return drm_atomic_helper_check_plane_state(plane_state,
> crtc_state,
> -						   DRM_PLANE_NO_SCALING,
> -						   DRM_PLANE_NO_SCALING,
> -						   false, true);
> +	ret = drm_atomic_helper_check_plane_state(new_state, crtc_state,
> +						  DRM_PLANE_NO_SCALING,
> +						  DRM_PLANE_NO_SCALING,
> +						  false, true);
> +	if (ret)
> +		return ret;
> +
> +	if (old_state->fb && new_state->fb->format != old_state->fb->format)
> +		crtc_state->mode_changed = true;
> +
> +	return 0;
>  }
> 
>  static void lcdif_plane_primary_atomic_update(struct drm_plane *plane,
> --
> 2.39.2

Re: [PATCH 2/5] drm: lcdif: move controller enable into atomic_flush

[Lucas Stach]

Am Donnerstag, dem 21.09.2023 um 07:13 +0000 schrieb Ying Liu:
> Hi,
> 
> On Wednesday, September 20, 2023 6:31 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> > Allow drm_atomic_helper_commit_tail_rpm to setup all the plane
> > state before the scanout is started.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/gpu/drm/mxsfb/lcdif_kms.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > index f5bfe8b52920..4acf6914a8d1 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > @@ -505,6 +505,8 @@ static int lcdif_crtc_atomic_check(struct drm_crtc
> > *crtc,
> >  static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
> >  				    struct drm_atomic_state *state)
> >  {
> > +	struct drm_crtc_state *crtc_state =
> > drm_atomic_get_new_crtc_state(state,
> > +									  crtc);
> >  	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev);
> >  	struct drm_pending_vblank_event *event;
> >  	u32 reg;
> > @@ -513,6 +515,9 @@ static void lcdif_crtc_atomic_flush(struct drm_crtc
> > *crtc,
> >  	reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
> >  	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> > 
> > +	if (drm_atomic_crtc_needs_modeset(crtc_state))
> > +		lcdif_enable_controller(lcdif);
> 
> Moving lcdif_enable_controller() function call from atomic_enable to
> atomic_flush would change CRTC vs bridge enablement order, which
> effectively makes bridge enablement happen prior to CRTC enablement,
> see drm_atomic_helper_commit_tail_rpm() detail implementation.  The
> reversed order potentially causes malfunctions of the bridge.
> 
This has nothing to do with the bridge after the LCDIF controller. The
RPM commit_tail implementation enables the CRTC before all the plane
state is set up. To avoid having to program the plane state in the CRTC
enable this series defers the actual controller enable to the last step
of the atomic commit. This way the plane state is programmed the usual
way via the atomic update_plane callback and we don't need to duplicate
this setup.

> BTW, even if it's ok to move the function call, it would be better to call
> lcdif_enable_controller() before CTRLDESCL0_5_SHADOW_LOAD_EN is
> set so that the original sequence is kept.  Also, LCDIF chapter in SoC RMs
> indicates that the shadow load enablement is the last step in
> "Software flow diagram".

This flow chart shows how the double buffered update should work, it
doesn't show the initial controller enable sequence. Without the shadow
load enable bit being set before the controller enable I could observe
sporadic issues on the first frame where the DMA engine would read the
wrong memory address.

Regards,
Lucas

> 
> Regards,
> Liu Ying
> 
> > +
> >  	event = crtc->state->event;
> >  	crtc->state->event = NULL;
> > 
> > @@ -552,7 +557,6 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc
> > *crtc,
> > 
> > 	writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
> >  		       lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
> >  	}
> > -	lcdif_enable_controller(lcdif);
> > 
> >  	drm_crtc_vblank_on(crtc);
> >  }
> > --
> > 2.39.2
> 

Re: [PATCH 5/5] drm: lcdif: force modeset when FB format changes

[Lucas Stach]

Am Donnerstag, dem 21.09.2023 um 07:34 +0000 schrieb Ying Liu:
> Hi,
> 
> On Wednesday, September 20, 2023 6:31 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> > Force a modeset if the new FB has a different format than the
> > currently active one. While it might be possible to change between
> > compatible formats without a full modeset as the format control is
> > also supposed to be double buffered, the colorspace conversion is
> > not, so when the CSC changes we need a modeset.
> > 
> > For now just always force a modeset when the FB format changes to
> > properly reconfigure all parts of the device for the new format.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/gpu/drm/mxsfb/lcdif_drv.c | 18 +++++++++++++++++-
> >  drivers/gpu/drm/mxsfb/lcdif_kms.c | 26 ++++++++++++++++++++------
> >  2 files changed, 37 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > index 18de2f17e249..b74f0cf1e240 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > @@ -30,9 +30,25 @@
> >  #include "lcdif_drv.h"
> >  #include "lcdif_regs.h"
> > 
> > +static int lcdif_atomic_check(struct drm_device *dev,
> > +				struct drm_atomic_state *state)
> 
> " checkpatch.pl --strict" complains:
> CHECK: Alignment should match open parenthesis
> #31: FILE: drivers/gpu/drm/mxsfb/lcdif_drv.c:34:
> +static int lcdif_atomic_check(struct drm_device *dev,
> +                               struct drm_atomic_state *state)
> 
Right, I'll fix that.

> > +{
> > +	int ret;
> > +
> > +	ret = drm_atomic_helper_check(dev, state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Check modeset again in case crtc_state->mode_changed is
> > +	 * updated in plane's ->atomic_check callback.
> > +	 */
> > +	return drm_atomic_helper_check_modeset(dev, state);
> 
> This additional check looks fine, but it's done for every commit.
> Is it ok to move it to lcdif_plane_atomic_check() where mode_changed
> is set for those commits in question?

Potentially yes, as we only have a single plane in the LCDIF, but it
would be a deviation of how every other DRM driver is implementing this
check. If there are multiple planes than this call must happen after
all of them checked the state, so this is the most logical place to
have this check. Doing this check on every commit doesn't seem to hurt
other drivers, so I'm not really keen on doing things differently here.

Regards,
Lucas
 
> 
> Regards,
> Liu Ying
> 
> > +}
> > +
> >  static const struct drm_mode_config_funcs lcdif_mode_config_funcs = {
> >  	.fb_create		= drm_gem_fb_create,
> > -	.atomic_check		= drm_atomic_helper_check,
> > +	.atomic_check		= lcdif_atomic_check,
> >  	.atomic_commit		= drm_atomic_helper_commit,
> >  };
> > 
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > index 3ebf55d06027..8167604bd3f8 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > @@ -647,18 +647,32 @@ static const struct drm_crtc_funcs lcdif_crtc_funcs
> > = {
> >  static int lcdif_plane_atomic_check(struct drm_plane *plane,
> >  				    struct drm_atomic_state *state)
> >  {
> > -	struct drm_plane_state *plane_state =
> > drm_atomic_get_new_plane_state(state,
> > -
> > plane);
> > +	struct drm_plane_state *new_state =
> > drm_atomic_get_new_plane_state(state,
> > +
> > plane);
> > +	struct drm_plane_state *old_state =
> > drm_atomic_get_old_plane_state(state,
> > +
> > plane);
> >  	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(plane->dev);
> >  	struct drm_crtc_state *crtc_state;
> > +	int ret;
> > +
> > +	/* always okay to disable the plane */
> > +	if (!new_state->fb)
> > +		return 0;
> > 
> >  	crtc_state = drm_atomic_get_new_crtc_state(state,
> >  						   &lcdif->crtc);
> > 
> > -	return drm_atomic_helper_check_plane_state(plane_state,
> > crtc_state,
> > -						   DRM_PLANE_NO_SCALING,
> > -						   DRM_PLANE_NO_SCALING,
> > -						   false, true);
> > +	ret = drm_atomic_helper_check_plane_state(new_state, crtc_state,
> > +						  DRM_PLANE_NO_SCALING,
> > +						  DRM_PLANE_NO_SCALING,
> > +						  false, true);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (old_state->fb && new_state->fb->format != old_state->fb->format)
> > +		crtc_state->mode_changed = true;
> > +
> > +	return 0;
> >  }
> > 
> >  static void lcdif_plane_primary_atomic_update(struct drm_plane *plane,
> > --
> > 2.39.2
> 

RE: [PATCH 2/5] drm: lcdif: move controller enable into atomic_flush

[Ying Liu]

On Thursday, September 21, 2023 3:55 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Donnerstag, dem 21.09.2023 um 07:13 +0000 schrieb Ying Liu:
> > Hi,
> >
> > On Wednesday, September 20, 2023 6:31 PM Lucas Stach
> <l.stach@pengutronix.de> wrote:
> > > Allow drm_atomic_helper_commit_tail_rpm to setup all the plane
> > > state before the scanout is started.
> > >
> > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > ---
> > >  drivers/gpu/drm/mxsfb/lcdif_kms.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > index f5bfe8b52920..4acf6914a8d1 100644
> > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > @@ -505,6 +505,8 @@ static int lcdif_crtc_atomic_check(struct drm_crtc
> > > *crtc,
> > >  static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
> > >  				    struct drm_atomic_state *state)
> > >  {
> > > +	struct drm_crtc_state *crtc_state =
> > > drm_atomic_get_new_crtc_state(state,
> > > +									  crtc);
> > >  	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev);
> > >  	struct drm_pending_vblank_event *event;
> > >  	u32 reg;
> > > @@ -513,6 +515,9 @@ static void lcdif_crtc_atomic_flush(struct drm_crtc
> > > *crtc,
> > >  	reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
> > >  	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> > >
> > > +	if (drm_atomic_crtc_needs_modeset(crtc_state))
> > > +		lcdif_enable_controller(lcdif);
> >
> > Moving lcdif_enable_controller() function call from atomic_enable to
> > atomic_flush would change CRTC vs bridge enablement order, which
> > effectively makes bridge enablement happen prior to CRTC enablement,
> > see drm_atomic_helper_commit_tail_rpm() detail implementation.  The
> > reversed order potentially causes malfunctions of the bridge.
> >
> This has nothing to do with the bridge after the LCDIF controller. The

IMHO, the reserved CRTC vs bridge enablement order is relevant for
the overall display pipeline.

> RPM commit_tail implementation enables the CRTC before all the plane
> state is set up. To avoid having to program the plane state in the CRTC
> enable this series defers the actual controller enable to the last step
> of the atomic commit. This way the plane state is programmed the usual
> way via the atomic update_plane callback and we don't need to duplicate
> this setup.

I understand the patch avoids some duplications via deferral controller
enablement time point, but the reversed CRTC vs bridge enablement order
is the concern here.

> 
> > BTW, even if it's ok to move the function call, it would be better to call
> > lcdif_enable_controller() before CTRLDESCL0_5_SHADOW_LOAD_EN is
> > set so that the original sequence is kept.  Also, LCDIF chapter in SoC RMs
> > indicates that the shadow load enablement is the last step in
> > "Software flow diagram".
> 
> This flow chart shows how the double buffered update should work, it
> doesn't show the initial controller enable sequence. Without the shadow

You are right.  The downstream driver also enables shadow load before
the entire controller.

Regards,
Liu Ying

> load enable bit being set before the controller enable I could observe
> sporadic issues on the first frame where the DMA engine would read the
> wrong memory address.
> 
> Regards,
> Lucas
> 
> >
> > Regards,
> > Liu Ying
> >
> > > +
> > >  	event = crtc->state->event;
> > >  	crtc->state->event = NULL;
> > >
> > > @@ -552,7 +557,6 @@ static void lcdif_crtc_atomic_enable(struct
> drm_crtc
> > > *crtc,
> > >
> > > 	writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
> > >  		       lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
> > >  	}
> > > -	lcdif_enable_controller(lcdif);
> > >
> > >  	drm_crtc_vblank_on(crtc);
> > >  }
> > > --
> > > 2.39.2
> >

RE: [PATCH 5/5] drm: lcdif: force modeset when FB format changes

[Ying Liu]

On Thursday, September 21, 2023 3:59 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Donnerstag, dem 21.09.2023 um 07:34 +0000 schrieb Ying Liu:
> > Hi,
> >
> > On Wednesday, September 20, 2023 6:31 PM Lucas Stach
> <l.stach@pengutronix.de> wrote:
> > > Force a modeset if the new FB has a different format than the
> > > currently active one. While it might be possible to change between
> > > compatible formats without a full modeset as the format control is
> > > also supposed to be double buffered, the colorspace conversion is
> > > not, so when the CSC changes we need a modeset.
> > >
> > > For now just always force a modeset when the FB format changes to
> > > properly reconfigure all parts of the device for the new format.
> > >
> > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > ---
> > >  drivers/gpu/drm/mxsfb/lcdif_drv.c | 18 +++++++++++++++++-
> > >  drivers/gpu/drm/mxsfb/lcdif_kms.c | 26 ++++++++++++++++++++------
> > >  2 files changed, 37 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > > b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > > index 18de2f17e249..b74f0cf1e240 100644
> > > --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > > +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > > @@ -30,9 +30,25 @@
> > >  #include "lcdif_drv.h"
> > >  #include "lcdif_regs.h"
> > >
> > > +static int lcdif_atomic_check(struct drm_device *dev,
> > > +				struct drm_atomic_state *state)
> >
> > " checkpatch.pl --strict" complains:
> > CHECK: Alignment should match open parenthesis
> > #31: FILE: drivers/gpu/drm/mxsfb/lcdif_drv.c:34:
> > +static int lcdif_atomic_check(struct drm_device *dev,
> > +                               struct drm_atomic_state *state)
> >
> Right, I'll fix that.
> 
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = drm_atomic_helper_check(dev, state);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * Check modeset again in case crtc_state->mode_changed is
> > > +	 * updated in plane's ->atomic_check callback.
> > > +	 */
> > > +	return drm_atomic_helper_check_modeset(dev, state);
> >
> > This additional check looks fine, but it's done for every commit.
> > Is it ok to move it to lcdif_plane_atomic_check() where mode_changed
> > is set for those commits in question?
> 
> Potentially yes, as we only have a single plane in the LCDIF, but it
> would be a deviation of how every other DRM driver is implementing this

malidp_crtc_atomic_check_gamma() calls it too.

> check. If there are multiple planes than this call must happen after
> all of them checked the state, so this is the most logical place to
> have this check. Doing this check on every commit doesn't seem to hurt
> other drivers, so I'm not really keen on doing things differently here.

Up to you.  No strong opinion.

Regards,
Liu Ying

> 
> Regards,
> Lucas
> 
> >
> > Regards,
> > Liu Ying
> >
> > > +}
> > > +
> > >  static const struct drm_mode_config_funcs lcdif_mode_config_funcs = {
> > >  	.fb_create		= drm_gem_fb_create,
> > > -	.atomic_check		= drm_atomic_helper_check,
> > > +	.atomic_check		= lcdif_atomic_check,
> > >  	.atomic_commit		= drm_atomic_helper_commit,
> > >  };
> > >
> > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > index 3ebf55d06027..8167604bd3f8 100644
> > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > @@ -647,18 +647,32 @@ static const struct drm_crtc_funcs
> lcdif_crtc_funcs
> > > = {
> > >  static int lcdif_plane_atomic_check(struct drm_plane *plane,
> > >  				    struct drm_atomic_state *state)
> > >  {
> > > -	struct drm_plane_state *plane_state =
> > > drm_atomic_get_new_plane_state(state,
> > > -
> > > plane);
> > > +	struct drm_plane_state *new_state =
> > > drm_atomic_get_new_plane_state(state,
> > > +
> > > plane);
> > > +	struct drm_plane_state *old_state =
> > > drm_atomic_get_old_plane_state(state,
> > > +
> > > plane);
> > >  	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(plane->dev);
> > >  	struct drm_crtc_state *crtc_state;
> > > +	int ret;
> > > +
> > > +	/* always okay to disable the plane */
> > > +	if (!new_state->fb)
> > > +		return 0;
> > >
> > >  	crtc_state = drm_atomic_get_new_crtc_state(state,
> > >  						   &lcdif->crtc);
> > >
> > > -	return drm_atomic_helper_check_plane_state(plane_state,
> > > crtc_state,
> > > -						   DRM_PLANE_NO_SCALING,
> > > -						   DRM_PLANE_NO_SCALING,
> > > -						   false, true);
> > > +	ret = drm_atomic_helper_check_plane_state(new_state, crtc_state,
> > > +						  DRM_PLANE_NO_SCALING,
> > > +						  DRM_PLANE_NO_SCALING,
> > > +						  false, true);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (old_state->fb && new_state->fb->format != old_state->fb->format)
> > > +		crtc_state->mode_changed = true;
> > > +
> > > +	return 0;
> > >  }
> > >
> > >  static void lcdif_plane_primary_atomic_update(struct drm_plane *plane,
> > > --
> > > 2.39.2
> >

Re: [PATCH 2/5] drm: lcdif: move controller enable into atomic_flush

[Lucas Stach]

Am Donnerstag, dem 21.09.2023 um 08:18 +0000 schrieb Ying Liu:
> On Thursday, September 21, 2023 3:55 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> > Am Donnerstag, dem 21.09.2023 um 07:13 +0000 schrieb Ying Liu:
> > > Hi,
> > > 
> > > On Wednesday, September 20, 2023 6:31 PM Lucas Stach
> > <l.stach@pengutronix.de> wrote:
> > > > Allow drm_atomic_helper_commit_tail_rpm to setup all the plane
> > > > state before the scanout is started.
> > > > 
> > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > > ---
> > > >  drivers/gpu/drm/mxsfb/lcdif_kms.c | 6 +++++-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > index f5bfe8b52920..4acf6914a8d1 100644
> > > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > @@ -505,6 +505,8 @@ static int lcdif_crtc_atomic_check(struct drm_crtc
> > > > *crtc,
> > > >  static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
> > > >  				    struct drm_atomic_state *state)
> > > >  {
> > > > +	struct drm_crtc_state *crtc_state =
> > > > drm_atomic_get_new_crtc_state(state,
> > > > +									  crtc);
> > > >  	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev);
> > > >  	struct drm_pending_vblank_event *event;
> > > >  	u32 reg;
> > > > @@ -513,6 +515,9 @@ static void lcdif_crtc_atomic_flush(struct drm_crtc
> > > > *crtc,
> > > >  	reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
> > > >  	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> > > > 
> > > > +	if (drm_atomic_crtc_needs_modeset(crtc_state))
> > > > +		lcdif_enable_controller(lcdif);
> > > 
> > > Moving lcdif_enable_controller() function call from atomic_enable to
> > > atomic_flush would change CRTC vs bridge enablement order, which
> > > effectively makes bridge enablement happen prior to CRTC enablement,
> > > see drm_atomic_helper_commit_tail_rpm() detail implementation.  The
> > > reversed order potentially causes malfunctions of the bridge.
> > > 
> > This has nothing to do with the bridge after the LCDIF controller. The
> 
> IMHO, the reserved CRTC vs bridge enablement order is relevant for
> the overall display pipeline.
> 
Ah, I see what you mean now. Moving the controller enable into flush is
violating the documented flow that bridges might assume the display
link to be running in their enable callback. I don't think any of the
bridges connected to the LCDIF really care about this, but it's bad
nevertheless.

This seems to be a generic issue with the RPM commit tail. Enabling the
display link in the CRTC atomic_enable without all the plane state
being set up in the HW sounds wrong and I think we also don't want to
hoist (duplicate) all the plane setup into the CRTC enable.

I'll look into this some more to see if we can do something better that
doesn't violate the bridge chain constraints.

Regards,
Lucas

> > RPM commit_tail implementation enables the CRTC before all the plane
> > state is set up. To avoid having to program the plane state in the CRTC
> > enable this series defers the actual controller enable to the last step
> > of the atomic commit. This way the plane state is programmed the usual
> > way via the atomic update_plane callback and we don't need to duplicate
> > this setup.
> 
> I understand the patch avoids some duplications via deferral controller
> enablement time point, but the reversed CRTC vs bridge enablement order
> is the concern here.
> 
> > 
> > > BTW, even if it's ok to move the function call, it would be better to call
> > > lcdif_enable_controller() before CTRLDESCL0_5_SHADOW_LOAD_EN is
> > > set so that the original sequence is kept.  Also, LCDIF chapter in SoC RMs
> > > indicates that the shadow load enablement is the last step in
> > > "Software flow diagram".
> > 
> > This flow chart shows how the double buffered update should work, it
> > doesn't show the initial controller enable sequence. Without the shadow
> 
> You are right.  The downstream driver also enables shadow load before
> the entire controller.
> 
> Regards,
> Liu Ying
> 
> > load enable bit being set before the controller enable I could observe
> > sporadic issues on the first frame where the DMA engine would read the
> > wrong memory address.
> > 
> > Regards,
> > Lucas
> > 
> > > 
> > > Regards,
> > > Liu Ying
> > > 
> > > > +
> > > >  	event = crtc->state->event;
> > > >  	crtc->state->event = NULL;
> > > > 
> > > > @@ -552,7 +557,6 @@ static void lcdif_crtc_atomic_enable(struct
> > drm_crtc
> > > > *crtc,
> > > > 
> > > > 	writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
> > > >  		       lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
> > > >  	}
> > > > -	lcdif_enable_controller(lcdif);
> > > > 
> > > >  	drm_crtc_vblank_on(crtc);
> > > >  }
> > > > --
> > > > 2.39.2
> > > 
> 

RE: [PATCH 2/5] drm: lcdif: move controller enable into atomic_flush

[Ying Liu]

On Thursday, September 21, 2023 4:57 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Donnerstag, dem 21.09.2023 um 08:18 +0000 schrieb Ying Liu:
> > On Thursday, September 21, 2023 3:55 PM Lucas Stach
> <l.stach@pengutronix.de> wrote:
> > > Am Donnerstag, dem 21.09.2023 um 07:13 +0000 schrieb Ying Liu:
> > > > Hi,
> > > >
> > > > On Wednesday, September 20, 2023 6:31 PM Lucas Stach
> > > <l.stach@pengutronix.de> wrote:
> > > > > Allow drm_atomic_helper_commit_tail_rpm to setup all the plane
> > > > > state before the scanout is started.
> > > > >
> > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > > > ---
> > > > >  drivers/gpu/drm/mxsfb/lcdif_kms.c | 6 +++++-
> > > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > > b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > > index f5bfe8b52920..4acf6914a8d1 100644
> > > > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > > @@ -505,6 +505,8 @@ static int lcdif_crtc_atomic_check(struct
> drm_crtc
> > > > > *crtc,
> > > > >  static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
> > > > >  				    struct drm_atomic_state *state)
> > > > >  {
> > > > > +	struct drm_crtc_state *crtc_state =
> > > > > drm_atomic_get_new_crtc_state(state,
> > > > > +
> 	  crtc);
> > > > >  	struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev);
> > > > >  	struct drm_pending_vblank_event *event;
> > > > >  	u32 reg;
> > > > > @@ -513,6 +515,9 @@ static void lcdif_crtc_atomic_flush(struct
> drm_crtc
> > > > > *crtc,
> > > > >  	reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
> > > > >  	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> > > > >
> > > > > +	if (drm_atomic_crtc_needs_modeset(crtc_state))
> > > > > +		lcdif_enable_controller(lcdif);
> > > >
> > > > Moving lcdif_enable_controller() function call from atomic_enable to
> > > > atomic_flush would change CRTC vs bridge enablement order, which
> > > > effectively makes bridge enablement happen prior to CRTC enablement,
> > > > see drm_atomic_helper_commit_tail_rpm() detail implementation.  The
> > > > reversed order potentially causes malfunctions of the bridge.
> > > >
> > > This has nothing to do with the bridge after the LCDIF controller. The
> >
> > IMHO, the reserved CRTC vs bridge enablement order is relevant for
> > the overall display pipeline.
> >
> Ah, I see what you mean now. Moving the controller enable into flush is
> violating the documented flow that bridges might assume the display
> link to be running in their enable callback. I don't think any of the
> bridges connected to the LCDIF really care about this, but it's bad
> nevertheless.

Not sure if the first bridges(all in SoCs for now) connected to LCDIF care
about the order.  But, it's really about the overall display pipeline including
wild out-of-SoC bridges in a chain.

> 
> This seems to be a generic issue with the RPM commit tail. Enabling the
> display link in the CRTC atomic_enable without all the plane state
> being set up in the HW sounds wrong and I think we also don't want to

Right, seems to be a generic issue.

Regards,
Liu Ying

> hoist (duplicate) all the plane setup into the CRTC enable.
> 
> I'll look into this some more to see if we can do something better that
> doesn't violate the bridge chain constraints.
> 
> Regards,
> Lucas
> 
> > > RPM commit_tail implementation enables the CRTC before all the plane
> > > state is set up. To avoid having to program the plane state in the CRTC
> > > enable this series defers the actual controller enable to the last step
> > > of the atomic commit. This way the plane state is programmed the usual
> > > way via the atomic update_plane callback and we don't need to duplicate
> > > this setup.
> >
> > I understand the patch avoids some duplications via deferral controller
> > enablement time point, but the reversed CRTC vs bridge enablement order
> > is the concern here.
> >
> > >
> > > > BTW, even if it's ok to move the function call, it would be better to call
> > > > lcdif_enable_controller() before CTRLDESCL0_5_SHADOW_LOAD_EN is
> > > > set so that the original sequence is kept.  Also, LCDIF chapter in SoC RMs
> > > > indicates that the shadow load enablement is the last step in
> > > > "Software flow diagram".
> > >
> > > This flow chart shows how the double buffered update should work, it
> > > doesn't show the initial controller enable sequence. Without the shadow
> >
> > You are right.  The downstream driver also enables shadow load before
> > the entire controller.
> >
> > Regards,
> > Liu Ying
> >
> > > load enable bit being set before the controller enable I could observe
> > > sporadic issues on the first frame where the DMA engine would read the
> > > wrong memory address.
> > >
> > > Regards,
> > > Lucas
> > >
> > > >
> > > > Regards,
> > > > Liu Ying
> > > >
> > > > > +
> > > > >  	event = crtc->state->event;
> > > > >  	crtc->state->event = NULL;
> > > > >
> > > > > @@ -552,7 +557,6 @@ static void lcdif_crtc_atomic_enable(struct
> > > drm_crtc
> > > > > *crtc,
> > > > >
> > > > > 	writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
> > > > >  		       lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
> > > > >  	}
> > > > > -	lcdif_enable_controller(lcdif);
> > > > >
> > > > >  	drm_crtc_vblank_on(crtc);
> > > > >  }
> > > > > --
> > > > > 2.39.2
> > > >
> >