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

[PATCH v2 1/6] 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>
Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
Reviewed-by: Marek Vasut <marex@denx.de>
---
v2: Some more rewording.
---
 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..07e343e01f3e 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:
+	 * The P_SIZE and T_SIZE bitfields are only documented 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/6] drm: lcdif: don't clear unrelated bits in CTRLDESCL0_5 when setting up format

[Lucas Stach]

The CTRLDESCL0_5 register also holds other bits that are not related to the
format, which should not be overwritten when the format is set up. Use a
proper RMW access in lcdif_set_formats().

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

diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index 07e343e01f3e..e277592e5fa5 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -166,6 +166,7 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
 	const u32 format = plane_state->fb->format->format;
 	bool in_yuv = false;
 	bool out_yuv = false;
+	u32 ctrl_desc_5;
 
 	switch (bus_format) {
 	case MEDIA_BUS_FMT_RGB565_1X16:
@@ -186,52 +187,49 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
 		break;
 	}
 
+	ctrl_desc_5 = readl(lcdif->base + LCDC_V8_CTRLDESCL0_5) &
+		      ~(CTRLDESCL0_5_BPP_MASK | CTRLDESCL0_5_YUV_FORMAT_MASK);
+
 	switch (format) {
 	/* RGB Formats */
 	case DRM_FORMAT_RGB565:
-		writel(CTRLDESCL0_5_BPP_16_RGB565,
-		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
+		ctrl_desc_5 |= CTRLDESCL0_5_BPP_16_RGB565;
 		break;
 	case DRM_FORMAT_RGB888:
-		writel(CTRLDESCL0_5_BPP_24_RGB888,
-		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
+		ctrl_desc_5 |= CTRLDESCL0_5_BPP_24_RGB888;
 		break;
 	case DRM_FORMAT_XRGB1555:
-		writel(CTRLDESCL0_5_BPP_16_ARGB1555,
-		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
+		ctrl_desc_5 |= CTRLDESCL0_5_BPP_16_ARGB1555;
 		break;
 	case DRM_FORMAT_XRGB4444:
-		writel(CTRLDESCL0_5_BPP_16_ARGB4444,
-		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
+		ctrl_desc_5 |= CTRLDESCL0_5_BPP_16_ARGB4444;
 		break;
 	case DRM_FORMAT_XBGR8888:
-		writel(CTRLDESCL0_5_BPP_32_ABGR8888,
-		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
+		ctrl_desc_5 |= CTRLDESCL0_5_BPP_32_ABGR8888;
 		break;
 	case DRM_FORMAT_XRGB8888:
-		writel(CTRLDESCL0_5_BPP_32_ARGB8888,
-		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
+		ctrl_desc_5 |= CTRLDESCL0_5_BPP_32_ARGB8888;
 		break;
 
 	/* YUV Formats */
 	case DRM_FORMAT_YUYV:
-		writel(CTRLDESCL0_5_BPP_YCbCr422 | CTRLDESCL0_5_YUV_FORMAT_VY2UY1,
-		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
+		ctrl_desc_5 |= CTRLDESCL0_5_BPP_YCbCr422 |
+			       CTRLDESCL0_5_YUV_FORMAT_VY2UY1;
 		in_yuv = true;
 		break;
 	case DRM_FORMAT_YVYU:
-		writel(CTRLDESCL0_5_BPP_YCbCr422 | CTRLDESCL0_5_YUV_FORMAT_UY2VY1,
-		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
+		ctrl_desc_5 |= CTRLDESCL0_5_BPP_YCbCr422 |
+			       CTRLDESCL0_5_YUV_FORMAT_UY2VY1;
 		in_yuv = true;
 		break;
 	case DRM_FORMAT_UYVY:
-		writel(CTRLDESCL0_5_BPP_YCbCr422 | CTRLDESCL0_5_YUV_FORMAT_Y2VY1U,
-		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
+		ctrl_desc_5 |= CTRLDESCL0_5_BPP_YCbCr422 |
+			       CTRLDESCL0_5_YUV_FORMAT_Y2VY1U;
 		in_yuv = true;
 		break;
 	case DRM_FORMAT_VYUY:
-		writel(CTRLDESCL0_5_BPP_YCbCr422 | CTRLDESCL0_5_YUV_FORMAT_Y2UY1V,
-		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
+		ctrl_desc_5 |= CTRLDESCL0_5_BPP_YCbCr422 |
+			       CTRLDESCL0_5_YUV_FORMAT_Y2UY1V;
 		in_yuv = true;
 		break;
 
@@ -240,6 +238,8 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
 		break;
 	}
 
+	writel(ctrl_desc_5, lcdif->base + LCDC_V8_CTRLDESCL0_5);
+
 	/*
 	 * The CSC differentiates between "YCbCr" and "YUV", but the reference
 	 * manual doesn't detail how they differ. Experiments showed that the
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/6] drm: lcdif: rework runtime PM handling in the atomic commit

[Lucas Stach]

drm_atomic_helper_commit_tail_rpm makes it hard for drivers to follow
the documented encoder/bridge enable flow, as it commits all CRTC enables
before the planes are fully set up, so drivers that can't enable the
display link without valid plane setup either need to do the plane setup
in the CRTC enable or violate the flow by enabling the display link after
the planes have been set up. Neither of those options seem like a good
idea.

For devices that only do coarse-grained runtime PM for the whole display
controller and not per CRTC, like the i.MX LCDIF, we can handle hardware
wakeup and suspend in the atomic_commit_tail. Add a commit tail which
follows the more conventional atomic commit flow of first diabling any
unused CRTCs, setting up all the active plane state, then enable all
active display pipes and also handles the device runtime PM at the
appropriate times.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
v2: new patch
---
 drivers/gpu/drm/mxsfb/lcdif_drv.c | 22 +++++++++++++++++++++-
 drivers/gpu/drm/mxsfb/lcdif_kms.c | 12 ++++++++++--
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c b/drivers/gpu/drm/mxsfb/lcdif_drv.c
index 18de2f17e249..205f6855fb1b 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
@@ -36,8 +36,28 @@ static const struct drm_mode_config_funcs lcdif_mode_config_funcs = {
 	.atomic_commit		= drm_atomic_helper_commit,
 };
 
+void lcdif_commit_tail(struct drm_atomic_state *old_state)
+{
+	struct drm_device *drm = old_state->dev;
+
+	pm_runtime_get_sync(drm->dev);
+
+	drm_atomic_helper_commit_modeset_disables(drm, old_state);
+	drm_atomic_helper_commit_planes(drm, old_state,
+					DRM_PLANE_COMMIT_ACTIVE_ONLY);
+	drm_atomic_helper_commit_modeset_enables(drm, old_state);
+
+	drm_atomic_helper_fake_vblank(old_state);
+	drm_atomic_helper_commit_hw_done(old_state);
+	drm_atomic_helper_wait_for_vblanks(drm, old_state);
+
+	pm_runtime_put(drm->dev);
+
+	drm_atomic_helper_cleanup_planes(drm, old_state);
+}
+
 static const struct drm_mode_config_helper_funcs lcdif_mode_config_helpers = {
-	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+	.atomic_commit_tail = lcdif_commit_tail,
 };
 
 static const struct drm_encoder_funcs lcdif_encoder_funcs = {
diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index e277592e5fa5..ccee5e28f236 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -540,7 +540,11 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	clk_set_rate(lcdif->clk, m->crtc_clock * 1000);
 
-	pm_runtime_get_sync(drm->dev);
+	/*
+	 * Update the RPM usage count, actual resume already happened in
+	 * lcdif_commit_tail wrapping all the atomic update.
+	 */
+	pm_runtime_get_noresume(drm->dev);
 
 	lcdif_crtc_mode_set_nofb(new_cstate, new_pstate);
 
@@ -576,7 +580,11 @@ static void lcdif_crtc_atomic_disable(struct drm_crtc *crtc,
 	}
 	spin_unlock_irq(&drm->event_lock);
 
-	pm_runtime_put_sync(drm->dev);
+	/*
+	 * Update the RPM usage count, actual suspend happens in
+	 * lcdif_commit_tail wrapping all the atomic update.
+	 */
+	pm_runtime_put(drm->dev);
 }
 
 static void lcdif_crtc_atomic_destroy_state(struct drm_crtc *crtc,
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/6] 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>
Reviewed-by: Marek Vasut <marex@denx.de>
---
v2: no changes
---
 drivers/gpu/drm/mxsfb/lcdif_kms.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index ccee5e28f236..4e9284b0d12e 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -536,7 +536,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);
 
@@ -548,14 +547,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);
-	}
 	lcdif_enable_controller(lcdif);
 
 	drm_crtc_vblank_on(crtc);
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/6] 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>
Reviewed-by: Marek Vasut <marex@denx.de>
---
v2: no changes
---
 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 4e9284b0d12e..daf54ff2b7bd 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);
-
-	/*
-	 * The P_SIZE and T_SIZE bitfields are only documented 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)
@@ -694,6 +681,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);
 	}
+
+	/*
+	 * The P_SIZE and T_SIZE bitfields are only documented 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 6/6] 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>
Reviewed-by: Marek Vasut <marex@denx.de>
---
v2: fix indentation
---
 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 205f6855fb1b..69a2a9323257 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 daf54ff2b7bd..34386e4b31c4 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -652,18 +652,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/6] drm: lcdif: rework runtime PM handling in the atomic commit

[kernel test robot]

Hi Lucas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.6-rc2 next-20230921]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lucas-Stach/drm-lcdif-don-t-clear-unrelated-bits-in-CTRLDESCL0_5-when-setting-up-format/20230922-040438
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20230921200312.3989073-3-l.stach%40pengutronix.de
patch subject: [PATCH v2 3/6] drm: lcdif: rework runtime PM handling in the atomic commit
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230922/202309220530.84SlbdTU-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230922/202309220530.84SlbdTU-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309220530.84SlbdTU-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/mxsfb/lcdif_drv.c:39:6: warning: no previous prototype for 'lcdif_commit_tail' [-Wmissing-prototypes]
      39 | void lcdif_commit_tail(struct drm_atomic_state *old_state)
         |      ^~~~~~~~~~~~~~~~~


vim +/lcdif_commit_tail +39 drivers/gpu/drm/mxsfb/lcdif_drv.c

    38	
  > 39	void lcdif_commit_tail(struct drm_atomic_state *old_state)
    40	{
    41		struct drm_device *drm = old_state->dev;
    42	
    43		pm_runtime_get_sync(drm->dev);
    44	
    45		drm_atomic_helper_commit_modeset_disables(drm, old_state);
    46		drm_atomic_helper_commit_planes(drm, old_state,
    47						DRM_PLANE_COMMIT_ACTIVE_ONLY);
    48		drm_atomic_helper_commit_modeset_enables(drm, old_state);
    49	
    50		drm_atomic_helper_fake_vblank(old_state);
    51		drm_atomic_helper_commit_hw_done(old_state);
    52		drm_atomic_helper_wait_for_vblanks(drm, old_state);
    53	
    54		pm_runtime_put(drm->dev);
    55	
    56		drm_atomic_helper_cleanup_planes(drm, old_state);
    57	}
    58	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 3/6] drm: lcdif: rework runtime PM handling in the atomic commit

[Ying Liu]

On Friday, September 22, 2023 4:03 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> drm_atomic_helper_commit_tail_rpm makes it hard for drivers to follow
> the documented encoder/bridge enable flow, as it commits all CRTC enables
> before the planes are fully set up, so drivers that can't enable the
> display link without valid plane setup either need to do the plane setup
> in the CRTC enable or violate the flow by enabling the display link after
> the planes have been set up. Neither of those options seem like a good
> idea.
> 
> For devices that only do coarse-grained runtime PM for the whole display
> controller and not per CRTC, like the i.MX LCDIF, we can handle hardware
> wakeup and suspend in the atomic_commit_tail. Add a commit tail which
> follows the more conventional atomic commit flow of first diabling any
> unused CRTCs, setting up all the active plane state, then enable all
> active display pipes and also handles the device runtime PM at the
> appropriate times.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> v2: new patch
> ---
>  drivers/gpu/drm/mxsfb/lcdif_drv.c | 22 +++++++++++++++++++++-
>  drivers/gpu/drm/mxsfb/lcdif_kms.c | 12 ++++++++++--
>  2 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> index 18de2f17e249..205f6855fb1b 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> @@ -36,8 +36,28 @@ static const struct drm_mode_config_funcs
> lcdif_mode_config_funcs = {
>  	.atomic_commit		= drm_atomic_helper_commit,
>  };
> 
> +void lcdif_commit_tail(struct drm_atomic_state *old_state)
> +{
> +	struct drm_device *drm = old_state->dev;
> +
> +	pm_runtime_get_sync(drm->dev);

Here, pixel clock lcdif->clk is enabled via lcdif_rpm_resume(), and then ...

> +
> +	drm_atomic_helper_commit_modeset_disables(drm, old_state);
> +	drm_atomic_helper_commit_planes(drm, old_state,
> +
> 	DRM_PLANE_COMMIT_ACTIVE_ONLY);
> +	drm_atomic_helper_commit_modeset_enables(drm, old_state);

... here, clk_set_rate() is called for lcdif->clk via lcdif_crtc_atomic_enable().
Set rate with clock enabled?

Another concern is lcdif_reset_block() is called via lcdif_crtc_mode_set_nofb()
here, while plane is already set up, which means plane settings are potentially
reset.

With this patch series, display shows constant color by running modetest to
change fb pixel format.  However, doing page flip with "-v" option seems fine.
Also, seems the issue doesn't reproduce without fbdev emulation.

Regards,
Liu Ying

> +
> +	drm_atomic_helper_fake_vblank(old_state);
> +	drm_atomic_helper_commit_hw_done(old_state);
> +	drm_atomic_helper_wait_for_vblanks(drm, old_state);
> +
> +	pm_runtime_put(drm->dev);
> +
> +	drm_atomic_helper_cleanup_planes(drm, old_state);
> +}
> +
>  static const struct drm_mode_config_helper_funcs
> lcdif_mode_config_helpers = {
> -	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> +	.atomic_commit_tail = lcdif_commit_tail,
>  };
> 
>  static const struct drm_encoder_funcs lcdif_encoder_funcs = {
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index e277592e5fa5..ccee5e28f236 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -540,7 +540,11 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc
> *crtc,
> 
>  	clk_set_rate(lcdif->clk, m->crtc_clock * 1000);
> 
> -	pm_runtime_get_sync(drm->dev);
> +	/*
> +	 * Update the RPM usage count, actual resume already happened in
> +	 * lcdif_commit_tail wrapping all the atomic update.
> +	 */
> +	pm_runtime_get_noresume(drm->dev);
> 
>  	lcdif_crtc_mode_set_nofb(new_cstate, new_pstate);
> 
> @@ -576,7 +580,11 @@ static void lcdif_crtc_atomic_disable(struct drm_crtc
> *crtc,
>  	}
>  	spin_unlock_irq(&drm->event_lock);
> 
> -	pm_runtime_put_sync(drm->dev);
> +	/*
> +	 * Update the RPM usage count, actual suspend happens in
> +	 * lcdif_commit_tail wrapping all the atomic update.
> +	 */
> +	pm_runtime_put(drm->dev);
>  }
> 
>  static void lcdif_crtc_atomic_destroy_state(struct drm_crtc *crtc,
> --
> 2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/6] drm: lcdif: rework runtime PM handling in the atomic commit

[Lucas Stach]

Am Freitag, dem 22.09.2023 um 09:51 +0000 schrieb Ying Liu:
> On Friday, September 22, 2023 4:03 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > drm_atomic_helper_commit_tail_rpm makes it hard for drivers to follow
> > the documented encoder/bridge enable flow, as it commits all CRTC enables
> > before the planes are fully set up, so drivers that can't enable the
> > display link without valid plane setup either need to do the plane setup
> > in the CRTC enable or violate the flow by enabling the display link after
> > the planes have been set up. Neither of those options seem like a good
> > idea.
> > 
> > For devices that only do coarse-grained runtime PM for the whole display
> > controller and not per CRTC, like the i.MX LCDIF, we can handle hardware
> > wakeup and suspend in the atomic_commit_tail. Add a commit tail which
> > follows the more conventional atomic commit flow of first diabling any
> > unused CRTCs, setting up all the active plane state, then enable all
> > active display pipes and also handles the device runtime PM at the
> > appropriate times.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> > v2: new patch
> > ---
> >  drivers/gpu/drm/mxsfb/lcdif_drv.c | 22 +++++++++++++++++++++-
> >  drivers/gpu/drm/mxsfb/lcdif_kms.c | 12 ++++++++++--
> >  2 files changed, 31 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > index 18de2f17e249..205f6855fb1b 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > @@ -36,8 +36,28 @@ static const struct drm_mode_config_funcs
> > lcdif_mode_config_funcs = {
> >  	.atomic_commit		= drm_atomic_helper_commit,
> >  };
> > 
> > +void lcdif_commit_tail(struct drm_atomic_state *old_state)
> > +{
> > +	struct drm_device *drm = old_state->dev;
> > +
> > +	pm_runtime_get_sync(drm->dev);
> 
> Here, pixel clock lcdif->clk is enabled via lcdif_rpm_resume(), and then ...
> 
> > +
> > +	drm_atomic_helper_commit_modeset_disables(drm, old_state);
> > +	drm_atomic_helper_commit_planes(drm, old_state,
> > +
> > 	DRM_PLANE_COMMIT_ACTIVE_ONLY);
> > +	drm_atomic_helper_commit_modeset_enables(drm, old_state);
> 
> ... here, clk_set_rate() is called for lcdif->clk via lcdif_crtc_atomic_enable().
> Set rate with clock enabled?
> 
Yea, I don't like the pixel clock enable/disable in the runtime PM
handling, but wanted to minimize the changes for now and I don't think
there is any issue with changing the rate of a already enabled clock.
Might be better to move the pixel clock enable/disable in the
atomic_enable/disable to clear any doubt.

> Another concern is lcdif_reset_block() is called via lcdif_crtc_mode_set_nofb()
> here, while plane is already set up, which means plane settings are potentially
> reset.
> 
I thought so as well, but the documentation states that only internal
state is reset, not the user visible registers. My testing seemed to
indicate that the plane state is unaffected by the reset, but...

> With this patch series, display shows constant color by running modetest to
> change fb pixel format.  However, doing page flip with "-v" option seems fine.
> Also, seems the issue doesn't reproduce without fbdev emulation.
> 
... this seems to contradict this. I'll dig some more into this. I
don't even know if this reset is required at all at this point, as it
seems this is a leftover from the mxsfb code. I can't find any
mandatory reset in the i.MX8MP reference manual.

Regards,
Lucas

> Regards,
> Liu Ying
> 
> > +
> > +	drm_atomic_helper_fake_vblank(old_state);
> > +	drm_atomic_helper_commit_hw_done(old_state);
> > +	drm_atomic_helper_wait_for_vblanks(drm, old_state);
> > +
> > +	pm_runtime_put(drm->dev);
> > +
> > +	drm_atomic_helper_cleanup_planes(drm, old_state);
> > +}
> > +
> >  static const struct drm_mode_config_helper_funcs
> > lcdif_mode_config_helpers = {
> > -	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> > +	.atomic_commit_tail = lcdif_commit_tail,
> >  };
> > 
> >  static const struct drm_encoder_funcs lcdif_encoder_funcs = {
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > index e277592e5fa5..ccee5e28f236 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > @@ -540,7 +540,11 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc
> > *crtc,
> > 
> >  	clk_set_rate(lcdif->clk, m->crtc_clock * 1000);
> > 
> > -	pm_runtime_get_sync(drm->dev);
> > +	/*
> > +	 * Update the RPM usage count, actual resume already happened in
> > +	 * lcdif_commit_tail wrapping all the atomic update.
> > +	 */
> > +	pm_runtime_get_noresume(drm->dev);
> > 
> >  	lcdif_crtc_mode_set_nofb(new_cstate, new_pstate);
> > 
> > @@ -576,7 +580,11 @@ static void lcdif_crtc_atomic_disable(struct drm_crtc
> > *crtc,
> >  	}
> >  	spin_unlock_irq(&drm->event_lock);
> > 
> > -	pm_runtime_put_sync(drm->dev);
> > +	/*
> > +	 * Update the RPM usage count, actual suspend happens in
> > +	 * lcdif_commit_tail wrapping all the atomic update.
> > +	 */
> > +	pm_runtime_put(drm->dev);
> >  }
> > 
> >  static void lcdif_crtc_atomic_destroy_state(struct drm_crtc *crtc,
> > --
> > 2.39.2
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel