linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/5] drm/mxsfb: Fix runtime PM for unpowering lcdif block
@ 2018-08-08 16:08 Leonard Crestez
  2018-08-08 16:08 ` [PATCH v4 1/5] drm/mxsfb: Move axi clk enable/disable to crtc enable/disable Leonard Crestez
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Leonard Crestez @ 2018-08-08 16:08 UTC (permalink / raw)
  To: Stefan Agner, Marek Vasut, Shawn Guo
  Cc: Philipp Zabel, Robert Chiras, Fabio Estevam, Anson Huang,
	dri-devel, Dong Aisheng, linux-imx, kernel, linux-kernel

Adding lcdif nodes to a power domain currently does work, it results in
black/corrupted screens or hangs. While the driver does enable runtime
pm it does not deal correctly with the block being unpowered.

Changes since v3:
 * Don't set initial fb in mode_set_nofb, do it in crtc_enable
 * Simplify AXI clk handling to allow setting fb in crtc_enable
 * No functional changes

Last 3 patches are identical to v3 with review tags attached.

Link to v3: https://lkml.org/lkml/2018/8/6/728

Leonard Crestez (5):
  drm/mxsfb: Move axi clk enable/disable to crtc enable/disable
  drm/mxsfb: Fix initial corrupt frame when activating display
  drm/mxsfb: Add pm_runtime calls to pipe_enable/disable
  drm/mxsfb: Add PM_SLEEP support
  drm/mxsfb: Switch to drm_atomic_helper_commit_tail_rpm

 drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 53 +++++++++++++++++++-----------
 drivers/gpu/drm/mxsfb/mxsfb_drv.c  | 40 ++++++++++++++++++++++
 2 files changed, 74 insertions(+), 19 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v4 1/5] drm/mxsfb: Move axi clk enable/disable to crtc enable/disable
  2018-08-08 16:08 [PATCH v4 0/5] drm/mxsfb: Fix runtime PM for unpowering lcdif block Leonard Crestez
@ 2018-08-08 16:08 ` Leonard Crestez
  2018-08-08 18:57   ` Stefan Agner
  2018-08-08 16:08 ` [PATCH v4 2/5] drm/mxsfb: Fix initial corrupt frame when activating display Leonard Crestez
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Leonard Crestez @ 2018-08-08 16:08 UTC (permalink / raw)
  To: Stefan Agner, Marek Vasut, Shawn Guo
  Cc: Philipp Zabel, Robert Chiras, Fabio Estevam, Anson Huang,
	dri-devel, Dong Aisheng, linux-imx, kernel, linux-kernel

The main axi clk is disabled at the end of mxsfb_crtc_mode_set_nofb and
immediately reenabled in mxsfb_enable_controller.

Avoid this by moving the handling of axi clk one level up to
mxsfb_crtc_enable. Do the same for mxsfb_crtc_disable for simmetry

This shouldn't have any functional effect.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
index 0abe77675b76..e4fcbb65b969 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
@@ -127,11 +127,10 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
 	u32 reg;
 
 	if (mxsfb->clk_disp_axi)
 		clk_prepare_enable(mxsfb->clk_disp_axi);
 	clk_prepare_enable(mxsfb->clk);
-	mxsfb_enable_axi_clk(mxsfb);
 
 	/* If it was disabled, re-enable the mode again */
 	writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET);
 
 	/* Enable the SYNC signals first, then the DMA engine */
@@ -157,12 +156,10 @@ static void mxsfb_disable_controller(struct mxsfb_drm_private *mxsfb)
 
 	reg = readl(mxsfb->base + LCDC_VDCTRL4);
 	reg &= ~VDCTRL4_SYNC_SIGNALS_ON;
 	writel(reg, mxsfb->base + LCDC_VDCTRL4);
 
-	mxsfb_disable_axi_clk(mxsfb);
-
 	clk_disable_unprepare(mxsfb->clk);
 	if (mxsfb->clk_disp_axi)
 		clk_disable_unprepare(mxsfb->clk_disp_axi);
 }
 
@@ -206,11 +203,10 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb)
 	/*
 	 * It seems, you can't re-program the controller if it is still
 	 * running. This may lead to shifted pictures (FIFO issue?), so
 	 * first stop the controller and drain its FIFOs.
 	 */
-	mxsfb_enable_axi_clk(mxsfb);
 
 	/* Mandatory eLCDIF reset as per the Reference Manual */
 	err = mxsfb_reset_block(mxsfb->base);
 	if (err)
 		return;
@@ -267,23 +263,23 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb)
 	       SET_VERT_WAIT_CNT(m->crtc_vtotal - m->crtc_vsync_start),
 	       mxsfb->base + LCDC_VDCTRL3);
 
 	writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay),
 	       mxsfb->base + LCDC_VDCTRL4);
-
-	mxsfb_disable_axi_clk(mxsfb);
 }
 
 void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb)
 {
+	mxsfb_enable_axi_clk(mxsfb);
 	mxsfb_crtc_mode_set_nofb(mxsfb);
 	mxsfb_enable_controller(mxsfb);
 }
 
 void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb)
 {
 	mxsfb_disable_controller(mxsfb);
+	mxsfb_disable_axi_clk(mxsfb);
 }
 
 void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb,
 			       struct drm_plane_state *state)
 {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v4 2/5] drm/mxsfb: Fix initial corrupt frame when activating display
  2018-08-08 16:08 [PATCH v4 0/5] drm/mxsfb: Fix runtime PM for unpowering lcdif block Leonard Crestez
  2018-08-08 16:08 ` [PATCH v4 1/5] drm/mxsfb: Move axi clk enable/disable to crtc enable/disable Leonard Crestez
@ 2018-08-08 16:08 ` Leonard Crestez
  2018-08-08 18:58   ` Stefan Agner
  2018-08-08 16:09 ` [PATCH v4 3/5] drm/mxsfb: Add pm_runtime calls to pipe_enable/disable Leonard Crestez
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Leonard Crestez @ 2018-08-08 16:08 UTC (permalink / raw)
  To: Stefan Agner, Marek Vasut, Shawn Guo
  Cc: Philipp Zabel, Robert Chiras, Fabio Estevam, Anson Huang,
	dri-devel, Dong Aisheng, linux-imx, kernel, linux-kernel

LCDIF will repeatedly display data from CUR_BUF and set CUR_BUF to
NEXT_BUF when done. Since we are only ever writing to NEXT_BUF the
display will show an initial corrupt frame.

Fix by writing the FB paddr to both CUR_BUF and NEXT_BUF when
activating the CRTC.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 45 +++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
index e4fcbb65b969..24b1f0c1432e 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
@@ -191,10 +191,25 @@ static int mxsfb_reset_block(void __iomem *reset_addr)
 		return ret;
 
 	return clear_poll_bit(reset_addr, MODULE_CLKGATE);
 }
 
+static dma_addr_t mxsfb_get_fb_paddr(struct mxsfb_drm_private *mxsfb)
+{
+	struct drm_framebuffer *fb = mxsfb->pipe.plane.state->fb;
+	struct drm_gem_cma_object *gem;
+
+	if (!fb)
+		return 0;
+
+	gem = drm_fb_cma_get_gem_obj(fb, 0);
+	if (!gem)
+		return 0;
+
+	return gem->paddr;
+}
+
 static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb)
 {
 	struct drm_display_mode *m = &mxsfb->pipe.crtc.state->adjusted_mode;
 	const u32 bus_flags = mxsfb->connector.display_info.bus_flags;
 	u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
@@ -267,12 +282,22 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb)
 	       mxsfb->base + LCDC_VDCTRL4);
 }
 
 void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb)
 {
+	dma_addr_t paddr;
+
 	mxsfb_enable_axi_clk(mxsfb);
 	mxsfb_crtc_mode_set_nofb(mxsfb);
+
+	/* Write cur_buf as well to avoid an initial corrupt frame */
+	paddr = mxsfb_get_fb_paddr(mxsfb);
+	if (paddr) {
+		writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf);
+		writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
+	}
+
 	mxsfb_enable_controller(mxsfb);
 }
 
 void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb)
 {
@@ -283,16 +308,12 @@ void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb)
 void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb,
 			       struct drm_plane_state *state)
 {
 	struct drm_simple_display_pipe *pipe = &mxsfb->pipe;
 	struct drm_crtc *crtc = &pipe->crtc;
-	struct drm_framebuffer *fb = pipe->plane.state->fb;
 	struct drm_pending_vblank_event *event;
-	struct drm_gem_cma_object *gem;
-
-	if (!crtc)
-		return;
+	dma_addr_t paddr;
 
 	spin_lock_irq(&crtc->dev->event_lock);
 	event = crtc->state->event;
 	if (event) {
 		crtc->state->event = NULL;
@@ -303,14 +324,12 @@ void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb,
 			drm_crtc_send_vblank_event(crtc, event);
 		}
 	}
 	spin_unlock_irq(&crtc->dev->event_lock);
 
-	if (!fb)
-		return;
-
-	gem = drm_fb_cma_get_gem_obj(fb, 0);
-
-	mxsfb_enable_axi_clk(mxsfb);
-	writel(gem->paddr, mxsfb->base + mxsfb->devdata->next_buf);
-	mxsfb_disable_axi_clk(mxsfb);
+	paddr = mxsfb_get_fb_paddr(mxsfb);
+	if (paddr) {
+		mxsfb_enable_axi_clk(mxsfb);
+		writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
+		mxsfb_disable_axi_clk(mxsfb);
+	}
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v4 3/5] drm/mxsfb: Add pm_runtime calls to pipe_enable/disable
  2018-08-08 16:08 [PATCH v4 0/5] drm/mxsfb: Fix runtime PM for unpowering lcdif block Leonard Crestez
  2018-08-08 16:08 ` [PATCH v4 1/5] drm/mxsfb: Move axi clk enable/disable to crtc enable/disable Leonard Crestez
  2018-08-08 16:08 ` [PATCH v4 2/5] drm/mxsfb: Fix initial corrupt frame when activating display Leonard Crestez
@ 2018-08-08 16:09 ` Leonard Crestez
  2018-08-08 16:09 ` [PATCH v4 4/5] drm/mxsfb: Add PM_SLEEP support Leonard Crestez
  2018-08-08 16:09 ` [PATCH v4 5/5] drm/mxsfb: Switch to drm_atomic_helper_commit_tail_rpm Leonard Crestez
  4 siblings, 0 replies; 9+ messages in thread
From: Leonard Crestez @ 2018-08-08 16:09 UTC (permalink / raw)
  To: Stefan Agner, Marek Vasut, Shawn Guo
  Cc: Philipp Zabel, Robert Chiras, Fabio Estevam, Anson Huang,
	dri-devel, Dong Aisheng, linux-imx, kernel, linux-kernel

Adding lcdif nodes to a power domain currently results in
black/corrupted screens or hangs because power is not correctly enabled
when required.

Ensure power is on when display is active by adding
pm_runtime_get/put_sync to mxsfb_pipe_enable/disable.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reviewed-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/mxsfb/mxsfb_drv.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index ffe5137ccaf8..68d79f5dc0d3 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -101,23 +101,27 @@ static const struct drm_mode_config_funcs mxsfb_mode_config_funcs = {
 static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe,
 			      struct drm_crtc_state *crtc_state,
 			      struct drm_plane_state *plane_state)
 {
 	struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe);
+	struct drm_device *drm = pipe->plane.dev;
 
+	pm_runtime_get_sync(drm->dev);
 	drm_panel_prepare(mxsfb->panel);
 	mxsfb_crtc_enable(mxsfb);
 	drm_panel_enable(mxsfb->panel);
 }
 
 static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe)
 {
 	struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe);
+	struct drm_device *drm = pipe->plane.dev;
 
 	drm_panel_disable(mxsfb->panel);
 	mxsfb_crtc_disable(mxsfb);
 	drm_panel_unprepare(mxsfb->panel);
+	pm_runtime_put_sync(drm->dev);
 }
 
 static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe,
 			      struct drm_plane_state *plane_state)
 {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v4 4/5] drm/mxsfb: Add PM_SLEEP support
  2018-08-08 16:08 [PATCH v4 0/5] drm/mxsfb: Fix runtime PM for unpowering lcdif block Leonard Crestez
                   ` (2 preceding siblings ...)
  2018-08-08 16:09 ` [PATCH v4 3/5] drm/mxsfb: Add pm_runtime calls to pipe_enable/disable Leonard Crestez
@ 2018-08-08 16:09 ` Leonard Crestez
  2018-08-08 16:09 ` [PATCH v4 5/5] drm/mxsfb: Switch to drm_atomic_helper_commit_tail_rpm Leonard Crestez
  4 siblings, 0 replies; 9+ messages in thread
From: Leonard Crestez @ 2018-08-08 16:09 UTC (permalink / raw)
  To: Stefan Agner, Marek Vasut, Shawn Guo
  Cc: Philipp Zabel, Robert Chiras, Fabio Estevam, Anson Huang,
	dri-devel, Dong Aisheng, linux-imx, kernel, linux-kernel

Since power to the lcdif block can be lost on suspend implement
PM_SLEEP_OPS using drm_mode_config_helper_suspend/resume to save/restore
the current mode.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reviewed-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/mxsfb/mxsfb_drv.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index 68d79f5dc0d3..d797dfd40d98 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -416,17 +416,38 @@ static int mxsfb_remove(struct platform_device *pdev)
 	drm_dev_unref(drm);
 
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int mxsfb_suspend(struct device *dev)
+{
+       struct drm_device *drm = dev_get_drvdata(dev);
+
+       return drm_mode_config_helper_suspend(drm);
+}
+
+static int mxsfb_resume(struct device *dev)
+{
+       struct drm_device *drm = dev_get_drvdata(dev);
+
+       return drm_mode_config_helper_resume(drm);
+}
+#endif
+
+static const struct dev_pm_ops mxsfb_pm_ops = {
+       SET_SYSTEM_SLEEP_PM_OPS(mxsfb_suspend, mxsfb_resume)
+};
+
 static struct platform_driver mxsfb_platform_driver = {
 	.probe		= mxsfb_probe,
 	.remove		= mxsfb_remove,
 	.id_table	= mxsfb_devtype,
 	.driver	= {
 		.name		= "mxsfb",
 		.of_match_table	= mxsfb_dt_ids,
+		.pm		= &mxsfb_pm_ops,
 	},
 };
 
 module_platform_driver(mxsfb_platform_driver);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v4 5/5] drm/mxsfb: Switch to drm_atomic_helper_commit_tail_rpm
  2018-08-08 16:08 [PATCH v4 0/5] drm/mxsfb: Fix runtime PM for unpowering lcdif block Leonard Crestez
                   ` (3 preceding siblings ...)
  2018-08-08 16:09 ` [PATCH v4 4/5] drm/mxsfb: Add PM_SLEEP support Leonard Crestez
@ 2018-08-08 16:09 ` Leonard Crestez
  4 siblings, 0 replies; 9+ messages in thread
From: Leonard Crestez @ 2018-08-08 16:09 UTC (permalink / raw)
  To: Stefan Agner, Marek Vasut, Shawn Guo
  Cc: Philipp Zabel, Robert Chiras, Fabio Estevam, Anson Huang,
	dri-devel, Dong Aisheng, linux-imx, kernel, linux-kernel

The lcdif block is only powered on when display is active so plane
updates when not enabled are not valid. Writing to an unpowered IP block
is mostly ignored but can trigger bus errors on some chips.

Prevent this situation by switching to drm_atomic_helper_commit_tail_rpm
and having the drm core ensure atomic_plane_update is only called while
the crtc is active. This avoids having to keep track of "enabled" bits
inside the mxsfb driver.

This also requires handling the vblank event for disable from
mxsfb_pipe_update.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Suggested-by: Stefan Agner <stefan@agner.ch>
Reviewed-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/mxsfb/mxsfb_drv.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index d797dfd40d98..5777e730085b 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -96,10 +96,14 @@ static const struct drm_mode_config_funcs mxsfb_mode_config_funcs = {
 	.fb_create		= drm_gem_fb_create,
 	.atomic_check		= drm_atomic_helper_check,
 	.atomic_commit		= drm_atomic_helper_commit,
 };
 
+static const struct drm_mode_config_helper_funcs mxsfb_mode_config_helpers = {
+	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+};
+
 static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe,
 			      struct drm_crtc_state *crtc_state,
 			      struct drm_plane_state *plane_state)
 {
 	struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe);
@@ -113,15 +117,25 @@ static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe)
 {
 	struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe);
 	struct drm_device *drm = pipe->plane.dev;
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct drm_pending_vblank_event *event;
 
 	drm_panel_disable(mxsfb->panel);
 	mxsfb_crtc_disable(mxsfb);
 	drm_panel_unprepare(mxsfb->panel);
 	pm_runtime_put_sync(drm->dev);
+
+	spin_lock_irq(&drm->event_lock);
+	event = crtc->state->event;
+	if (event) {
+		crtc->state->event = NULL;
+		drm_crtc_send_vblank_event(crtc, event);
+	}
+	spin_unlock_irq(&drm->event_lock);
 }
 
 static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe,
 			      struct drm_plane_state *plane_state)
 {
@@ -232,10 +246,11 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags)
 	drm->mode_config.min_width	= MXSFB_MIN_XRES;
 	drm->mode_config.min_height	= MXSFB_MIN_YRES;
 	drm->mode_config.max_width	= MXSFB_MAX_XRES;
 	drm->mode_config.max_height	= MXSFB_MAX_YRES;
 	drm->mode_config.funcs		= &mxsfb_mode_config_funcs;
+	drm->mode_config.helper_private	= &mxsfb_mode_config_helpers;
 
 	drm_mode_config_reset(drm);
 
 	pm_runtime_get_sync(drm->dev);
 	ret = drm_irq_install(drm, platform_get_irq(pdev, 0));
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 1/5] drm/mxsfb: Move axi clk enable/disable to crtc enable/disable
  2018-08-08 16:08 ` [PATCH v4 1/5] drm/mxsfb: Move axi clk enable/disable to crtc enable/disable Leonard Crestez
@ 2018-08-08 18:57   ` Stefan Agner
  2018-08-13 13:19     ` Leonard Crestez
  0 siblings, 1 reply; 9+ messages in thread
From: Stefan Agner @ 2018-08-08 18:57 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Marek Vasut, Shawn Guo, Philipp Zabel, Robert Chiras,
	Fabio Estevam, Anson Huang, dri-devel, Dong Aisheng, linux-imx,
	kernel, linux-kernel

On 08.08.2018 18:08, Leonard Crestez wrote:
> The main axi clk is disabled at the end of mxsfb_crtc_mode_set_nofb and
> immediately reenabled in mxsfb_enable_controller.
> 
> Avoid this by moving the handling of axi clk one level up to
> mxsfb_crtc_enable. Do the same for mxsfb_crtc_disable for simmetry
> 
> This shouldn't have any functional effect.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Thanks, looks good!

Reviewed-by: Stefan Agner <stefan@agner.ch>

> ---
>  drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> index 0abe77675b76..e4fcbb65b969 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> @@ -127,11 +127,10 @@ static void mxsfb_enable_controller(struct
> mxsfb_drm_private *mxsfb)
>  	u32 reg;
>  
>  	if (mxsfb->clk_disp_axi)
>  		clk_prepare_enable(mxsfb->clk_disp_axi);
>  	clk_prepare_enable(mxsfb->clk);
> -	mxsfb_enable_axi_clk(mxsfb);
>  
>  	/* If it was disabled, re-enable the mode again */
>  	writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET);
>  
>  	/* Enable the SYNC signals first, then the DMA engine */
> @@ -157,12 +156,10 @@ static void mxsfb_disable_controller(struct
> mxsfb_drm_private *mxsfb)
>  
>  	reg = readl(mxsfb->base + LCDC_VDCTRL4);
>  	reg &= ~VDCTRL4_SYNC_SIGNALS_ON;
>  	writel(reg, mxsfb->base + LCDC_VDCTRL4);
>  
> -	mxsfb_disable_axi_clk(mxsfb);
> -
>  	clk_disable_unprepare(mxsfb->clk);
>  	if (mxsfb->clk_disp_axi)
>  		clk_disable_unprepare(mxsfb->clk_disp_axi);
>  }
>  
> @@ -206,11 +203,10 @@ static void mxsfb_crtc_mode_set_nofb(struct
> mxsfb_drm_private *mxsfb)
>  	/*
>  	 * It seems, you can't re-program the controller if it is still
>  	 * running. This may lead to shifted pictures (FIFO issue?), so
>  	 * first stop the controller and drain its FIFOs.
>  	 */
> -	mxsfb_enable_axi_clk(mxsfb);
>  
>  	/* Mandatory eLCDIF reset as per the Reference Manual */
>  	err = mxsfb_reset_block(mxsfb->base);
>  	if (err)
>  		return;
> @@ -267,23 +263,23 @@ static void mxsfb_crtc_mode_set_nofb(struct
> mxsfb_drm_private *mxsfb)
>  	       SET_VERT_WAIT_CNT(m->crtc_vtotal - m->crtc_vsync_start),
>  	       mxsfb->base + LCDC_VDCTRL3);
>  
>  	writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay),
>  	       mxsfb->base + LCDC_VDCTRL4);
> -
> -	mxsfb_disable_axi_clk(mxsfb);
>  }
>  
>  void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb)
>  {
> +	mxsfb_enable_axi_clk(mxsfb);
>  	mxsfb_crtc_mode_set_nofb(mxsfb);
>  	mxsfb_enable_controller(mxsfb);
>  }
>  
>  void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb)
>  {
>  	mxsfb_disable_controller(mxsfb);
> +	mxsfb_disable_axi_clk(mxsfb);
>  }
>  
>  void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb,
>  			       struct drm_plane_state *state)
>  {

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 2/5] drm/mxsfb: Fix initial corrupt frame when activating display
  2018-08-08 16:08 ` [PATCH v4 2/5] drm/mxsfb: Fix initial corrupt frame when activating display Leonard Crestez
@ 2018-08-08 18:58   ` Stefan Agner
  0 siblings, 0 replies; 9+ messages in thread
From: Stefan Agner @ 2018-08-08 18:58 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Marek Vasut, Shawn Guo, Philipp Zabel, Robert Chiras,
	Fabio Estevam, Anson Huang, dri-devel, Dong Aisheng, linux-imx,
	kernel, linux-kernel

On 08.08.2018 18:08, Leonard Crestez wrote:
> LCDIF will repeatedly display data from CUR_BUF and set CUR_BUF to
> NEXT_BUF when done. Since we are only ever writing to NEXT_BUF the
> display will show an initial corrupt frame.
> 
> Fix by writing the FB paddr to both CUR_BUF and NEXT_BUF when
> activating the CRTC.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Tested-by: Philipp Zabel <p.zabel@pengutronix.de>

Reviewed-by: Stefan Agner <stefan@agner.ch>

> ---
>  drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 45 +++++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> index e4fcbb65b969..24b1f0c1432e 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> @@ -191,10 +191,25 @@ static int mxsfb_reset_block(void __iomem *reset_addr)
>  		return ret;
>  
>  	return clear_poll_bit(reset_addr, MODULE_CLKGATE);
>  }
>  
> +static dma_addr_t mxsfb_get_fb_paddr(struct mxsfb_drm_private *mxsfb)
> +{
> +	struct drm_framebuffer *fb = mxsfb->pipe.plane.state->fb;
> +	struct drm_gem_cma_object *gem;
> +
> +	if (!fb)
> +		return 0;
> +
> +	gem = drm_fb_cma_get_gem_obj(fb, 0);
> +	if (!gem)
> +		return 0;
> +
> +	return gem->paddr;
> +}
> +
>  static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb)
>  {
>  	struct drm_display_mode *m = &mxsfb->pipe.crtc.state->adjusted_mode;
>  	const u32 bus_flags = mxsfb->connector.display_info.bus_flags;
>  	u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
> @@ -267,12 +282,22 @@ static void mxsfb_crtc_mode_set_nofb(struct
> mxsfb_drm_private *mxsfb)
>  	       mxsfb->base + LCDC_VDCTRL4);
>  }
>  
>  void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb)
>  {
> +	dma_addr_t paddr;
> +
>  	mxsfb_enable_axi_clk(mxsfb);
>  	mxsfb_crtc_mode_set_nofb(mxsfb);
> +
> +	/* Write cur_buf as well to avoid an initial corrupt frame */
> +	paddr = mxsfb_get_fb_paddr(mxsfb);
> +	if (paddr) {
> +		writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf);
> +		writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
> +	}
> +
>  	mxsfb_enable_controller(mxsfb);
>  }
>  
>  void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb)
>  {
> @@ -283,16 +308,12 @@ void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb)
>  void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb,
>  			       struct drm_plane_state *state)
>  {
>  	struct drm_simple_display_pipe *pipe = &mxsfb->pipe;
>  	struct drm_crtc *crtc = &pipe->crtc;
> -	struct drm_framebuffer *fb = pipe->plane.state->fb;
>  	struct drm_pending_vblank_event *event;
> -	struct drm_gem_cma_object *gem;
> -
> -	if (!crtc)
> -		return;
> +	dma_addr_t paddr;
>  
>  	spin_lock_irq(&crtc->dev->event_lock);
>  	event = crtc->state->event;
>  	if (event) {
>  		crtc->state->event = NULL;
> @@ -303,14 +324,12 @@ void mxsfb_plane_atomic_update(struct
> mxsfb_drm_private *mxsfb,
>  			drm_crtc_send_vblank_event(crtc, event);
>  		}
>  	}
>  	spin_unlock_irq(&crtc->dev->event_lock);
>  
> -	if (!fb)
> -		return;
> -
> -	gem = drm_fb_cma_get_gem_obj(fb, 0);
> -
> -	mxsfb_enable_axi_clk(mxsfb);
> -	writel(gem->paddr, mxsfb->base + mxsfb->devdata->next_buf);
> -	mxsfb_disable_axi_clk(mxsfb);
> +	paddr = mxsfb_get_fb_paddr(mxsfb);
> +	if (paddr) {
> +		mxsfb_enable_axi_clk(mxsfb);
> +		writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
> +		mxsfb_disable_axi_clk(mxsfb);
> +	}
>  }

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 1/5] drm/mxsfb: Move axi clk enable/disable to crtc enable/disable
  2018-08-08 18:57   ` Stefan Agner
@ 2018-08-13 13:19     ` Leonard Crestez
  0 siblings, 0 replies; 9+ messages in thread
From: Leonard Crestez @ 2018-08-13 13:19 UTC (permalink / raw)
  To: stefan, marex
  Cc: dl-linux-imx, A.s. Dong, linux-kernel, Robert Chiras,
	Fabio Estevam, p.zabel, dri-devel, shawnguo, Anson Huang, kernel

On Wed, 2018-08-08 at 20:57 +0200, Stefan Agner wrote:
> On 08.08.2018 18:08, Leonard Crestez wrote:
> > The main axi clk is disabled at the end of mxsfb_crtc_mode_set_nofb and
> > immediately reenabled in mxsfb_enable_controller.
> > 
> > Avoid this by moving the handling of axi clk one level up to
> > mxsfb_crtc_enable. Do the same for mxsfb_crtc_disable for simmetry
> > 
> > This shouldn't have any functional effect.
> > 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> 
> Thanks, looks good!
> 
> Reviewed-by: Stefan Agner <stefan@agner.ch>

So what is next now that this entire series was reviewed? My guess is
that somebody needs to push it to drm-misc-next.

However 4.18 was just released and I'm not sure how this interacts with
drm. As far as I can tell drm-misc-next is always open and a patch
accepted now will make it into 4.20, based on this graph:

https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-misc.html#merge-timeline

--
Regards,
Leonard

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-08-13 13:19 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-08 16:08 [PATCH v4 0/5] drm/mxsfb: Fix runtime PM for unpowering lcdif block Leonard Crestez
2018-08-08 16:08 ` [PATCH v4 1/5] drm/mxsfb: Move axi clk enable/disable to crtc enable/disable Leonard Crestez
2018-08-08 18:57   ` Stefan Agner
2018-08-13 13:19     ` Leonard Crestez
2018-08-08 16:08 ` [PATCH v4 2/5] drm/mxsfb: Fix initial corrupt frame when activating display Leonard Crestez
2018-08-08 18:58   ` Stefan Agner
2018-08-08 16:09 ` [PATCH v4 3/5] drm/mxsfb: Add pm_runtime calls to pipe_enable/disable Leonard Crestez
2018-08-08 16:09 ` [PATCH v4 4/5] drm/mxsfb: Add PM_SLEEP support Leonard Crestez
2018-08-08 16:09 ` [PATCH v4 5/5] drm/mxsfb: Switch to drm_atomic_helper_commit_tail_rpm Leonard Crestez

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).