[PATCH v3 0/4] drm/mxsfb: Fix runtime PM for unpowering lcdif block

[PATCH v3 0/4] drm/mxsfb: Fix runtime PM for unpowering lcdif block

[Leonard Crestez]

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 v2:
 * Split into multiple commits.
 * Use #ifdef CONFIG_PM_SLEEP.
 * Switch to drm_atomic_helper_commit_tail_rpm

Patch 1 was split because it's useful alone.

Patches 2/3 could maybe be squashed.

I'm not sure patch 4 handles vblanks correctly. It does not replace
patch 1 because there is still a small window between enabling the pipe
and the first atomic_update and also somebody needs to write to
NEXT_BUF.

Link to v2: https://lkml.org/lkml/2018/7/17/414

 drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 42 +++++++++++++++++++++---------
 drivers/gpu/drm/mxsfb/mxsfb_drv.c  | 40 ++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 13 deletions(-)

-- 
2.17.1

[PATCH v3 1/4] drm/mxsfb: Fix initial corrupt frame when activating display

[Leonard Crestez]

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>
---
 drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 42 +++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
index 0abe77675b76..db3ff5bde122 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
@@ -194,15 +194,31 @@ 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;
+	dma_addr_t paddr;
 	int err;
 
 	/*
 	 * It seems, you can't re-program the controller if it is still
 	 * running. This may lead to shifted pictures (FIFO issue?), so
@@ -268,10 +284,16 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb)
 	       mxsfb->base + LCDC_VDCTRL3);
 
 	writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay),
 	       mxsfb->base + LCDC_VDCTRL4);
 
+	/* Update 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_disable_axi_clk(mxsfb);
 }
 
 void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb)
 {
@@ -287,16 +309,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;
@@ -307,14 +325,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

Re: [PATCH v3 1/4] drm/mxsfb: Fix initial corrupt frame when activating display

[Philipp Zabel]

On Mon, 2018-08-06 at 22:31 +0300, 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>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
on imx6ull-evk.

regards
Philipp

Re: [PATCH v3 1/4] drm/mxsfb: Fix initial corrupt frame when activating display

[Stefan Agner]

On 06.08.2018 21:31, 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.

Really like how this patch set evolves! Only some minor thing below...

> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 42 +++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> index 0abe77675b76..db3ff5bde122 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> @@ -194,15 +194,31 @@ 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;
> +	dma_addr_t paddr;
>  	int err;
>  
>  	/*
>  	 * It seems, you can't re-program the controller if it is still
>  	 * running. This may lead to shifted pictures (FIFO issue?), so
> @@ -268,10 +284,16 @@ static void mxsfb_crtc_mode_set_nofb(struct
> mxsfb_drm_private *mxsfb)
>  	       mxsfb->base + LCDC_VDCTRL3);
>  
>  	writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay),
>  	       mxsfb->base + LCDC_VDCTRL4);
>  
> +	/* Update 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);
> +	}

Traditionally mxsfb_crtc_mode_set_nofb() was used as a callback to
mode_set_nofb. And that callback should not fiddle with the fb (as the
name says).

So I rather prefer to move the new bits to mxsfb_crtc_enable(), between
mxsfb_crtc_mode_set_nofb() and mxsfb_enable_controller(). It is only
cosmetics, but I'd rather prefer to have it in the right place.

Currently the AXI clock is disabled between those functions... I suggest
to enable the AXI clock once in mxsfb_crtc_enable(). This puts an end to
the unnecessary enable/disable/enable sequence in mxsfb_crtc_enable().

So I suggest to create a patch "drm/mxsfb: simplify AXI clock handling"

--- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
@@ -129,7 +129,6 @@ static void mxsfb_enable_controller(struct
mxsfb_drm_private *mxsfb)
        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);
@@ -159,8 +158,6 @@ static void mxsfb_disable_controller(struct
mxsfb_drm_private *mxsfb)
        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);
@@ -208,7 +205,6 @@ static void mxsfb_crtc_mode_set_nofb(struct
mxsfb_drm_private *mxsfb)
         * 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);
@@ -269,12 +265,11 @@ static void mxsfb_crtc_mode_set_nofb(struct
mxsfb_drm_private *mxsfb)
 
        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);
 }
@@ -282,6 +277,7 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private
*mxsfb)
 void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb)
 {
        mxsfb_disable_controller(mxsfb);
+       mxsfb_disable_axi_clk(mxsfb);
 }
 

Then move the above section between mxsfb_crtc_mode_set_nofb() and
mxsfb_enable_controller().

--
Stefan

>  	mxsfb_disable_axi_clk(mxsfb);
>  }
>  
>  void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb)
>  {
> @@ -287,16 +309,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;
> @@ -307,14 +325,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);
> +	}
>  }

[PATCH v3 2/4] drm/mxsfb: Add pm_runtime calls to pipe_enable/disable

[Leonard Crestez]

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>
---
 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

Re: [PATCH v3 2/4] drm/mxsfb: Add pm_runtime calls to pipe_enable/disable

[Stefan Agner]

On 06.08.2018 21:31, Leonard Crestez wrote:
> 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)
>  {

[PATCH v3 3/4] drm/mxsfb: Add PM_SLEEP support

[Leonard Crestez]

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>
---
 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

Re: [PATCH v3 3/4] drm/mxsfb: Add PM_SLEEP support

[Stefan Agner]

On 06.08.2018 21:31, Leonard Crestez wrote:
> 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);

[PATCH v3 4/4] drm/mxsfb: Switch to drm_atomic_helper_commit_tail_rpm

[Leonard Crestez]

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>
---
 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

Re: [PATCH v3 4/4] drm/mxsfb: Switch to drm_atomic_helper_commit_tail_rpm

[Stefan Agner]

On 06.08.2018 21:31, Leonard Crestez wrote:
> 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.

Hm, I don't think this is a new requirement. Simple KMS Helper Reference
clearly states that it should be called from update:
https://www.kernel.org/doc/html/v4.17/gpu/drm-kms-helpers.html#simple-kms-helper-reference

Probably using drm_atomic_helper_commit_tail_rpm just exacerbates an
issue which we haven't seen before...

Since I think it is a general fix, I'd rather prefer have it in a
separate commit.

> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Suggested-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)
>  {

Shouldn't that be in mxsfb_pipe_update?

--
Stefan

>  	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));

Re: [PATCH v3 4/4] drm/mxsfb: Switch to drm_atomic_helper_commit_tail_rpm

[Leonard Crestez]

On Tue, 2018-08-07 at 21:01 +0200, Stefan Agner wrote:
> On 06.08.2018 21:31, Leonard Crestez wrote:
> > 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~~ **mxsfb_pipe_disable**.
> 
> Hm, I don't think this is a new requirement. Simple KMS Helper Reference
> clearly states that it should be called from update.
> 
> Probably using drm_atomic_helper_commit_tail_rpm just exacerbates an
> issue which we haven't seen before...
> 
> Since I think it is a general fix, I'd rather prefer have it in a
> separate commit.

I wrote the commit message wrong, what I meant is that it requires
handling the vblank event from *disable*.

Switching to atomic_helper_commit_tail_rpm means atomic_update is no
longer called when !state->active so nobody dispatches the last vblank
event for disabling the crtc. This causes a warning in
drm_atomic_helper_commit_hw_done on disable.

Looking through the docs there seems to be a lot of complexity behind
vblank events so maybe I'm missing something.

Re: [PATCH v3 4/4] drm/mxsfb: Switch to drm_atomic_helper_commit_tail_rpm

[Stefan Agner]

On 08.08.2018 10:00, Leonard Crestez wrote:
> On Tue, 2018-08-07 at 21:01 +0200, Stefan Agner wrote:
>> On 06.08.2018 21:31, Leonard Crestez wrote:
>> > 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~~ **mxsfb_pipe_disable**.
>>
>> Hm, I don't think this is a new requirement. Simple KMS Helper Reference
>> clearly states that it should be called from update.
>>
>> Probably using drm_atomic_helper_commit_tail_rpm just exacerbates an
>> issue which we haven't seen before...
>>
>> Since I think it is a general fix, I'd rather prefer have it in a
>> separate commit.
> 
> I wrote the commit message wrong, what I meant is that it requires
> handling the vblank event from *disable*.
> 
> Switching to atomic_helper_commit_tail_rpm means atomic_update is no
> longer called when !state->active so nobody dispatches the last vblank
> event for disabling the crtc. This causes a warning in
> drm_atomic_helper_commit_hw_done on disable.

Hm, I see, atomic_helper_commit_tail_rpm() uses
DRM_PLANE_COMMIT_ACTIVE_ONLY, which leads to update() not being called
for Simple KMS (since simple KMS uses the plane atomic_update() hook).

I was looking in other drivers such as
drivers/gpu/drm/pl111/pl111_display.c and was wondering why they do not
need that in disable. But it makes sense since they do not use
atomic_helper_commit_tail_rpm(), update does get called. Also note that
pl111_display_update() uses drm_crtc_send_vblank_event() too if the crtc
gets disabled:

		if (crtc->state->active && drm_crtc_vblank_get(crtc) == 0)
			drm_crtc_arm_vblank_event(crtc, event);
		else
			drm_crtc_send_vblank_event(crtc, event);


The other users of atomic_helper_commit_tail_rpm(), exynos and sun4i,
call drm_crtc_send_vblank_event() in the CRTC atomic_disable() hook. The
Simple KMS equivalent of that is the disable hook.

So it is really the change to _rpm() which requires to add
drm_crtc_send_vblank_event() in disable. Having this two changes in a
single commit indeed makes sense and is correct, sorry about the noise!
Just fix the commit, and than I am fine with this.

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

--
Stefan

> 
> Looking through the docs there seems to be a lot of complexity behind
> vblank events so maybe I'm missing something.