[PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block

[PATCH v2] 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.

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

Since power is lost on suspend implement PM_SLEEP_OPS using
drm_mode_config_helper_suspend/resume.

The mxsfb_plane_atomic_update function can get called before
mxsfb_pipe_enable while the block is not yet powered. When this happens
the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank
until a refresh.

Fix this by not writing gem->paddr if the block is not enabled and
instead delaying the write until the next mxsfb_crtc_mode_set_nofb call.
At that point also update cur_buf to avoid an initial corrupt frame
after resume.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

---

The purpose of this patch is to prepare for enabling power gating on
DISPLAY power domains.

Changes since v1:
* Drop mxsfb_runtime_suspend/mxsfb_runtime_resume, calling
pm_runtime_get/put in pipe enable/disable is enough.
* Use drm_mode_config_helper_suspend/resume instead of attempting to
track state manually.
* Don't touch NEXT_BUF if atomic_update called with crtc disabled
* Also update CUR_BUF on enable, this avoids initial corrupt frames.

Previous discussion: https://lkml.org/lkml/2018/7/17/329
---
 drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 51 +++++++++++++++++++++++-------
 drivers/gpu/drm/mxsfb/mxsfb_drv.c  | 25 +++++++++++++++
 drivers/gpu/drm/mxsfb/mxsfb_drv.h  |  2 ++
 3 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
index 0abe77675b76..10153da77c40 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,35 +284,47 @@ 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)
 {
+	if (mxsfb->enabled)
+		return;
+
 	mxsfb_crtc_mode_set_nofb(mxsfb);
 	mxsfb_enable_controller(mxsfb);
+
+	mxsfb->enabled = true;
 }
 
 void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb)
 {
+	if (!mxsfb->enabled)
+		return;
+
 	mxsfb_disable_controller(mxsfb);
+
+	mxsfb->enabled = false;
 }
 
 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 +335,15 @@ 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)
+	if (!mxsfb->enabled)
 		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);
+	}
 }
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index dd1dd58e4956..a5269fccbed9 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)
 {
@@ -412,17 +416,38 @@ static int mxsfb_remove(struct platform_device *pdev)
 	drm_dev_unref(drm);
 
 	return 0;
 }
 
+#ifdef CONFIG_PM
+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-drm",
 		.of_match_table	= mxsfb_dt_ids,
+		.pm		= &mxsfb_pm_ops,
 	},
 };
 
 module_platform_driver(mxsfb_platform_driver);
 
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
index 5d0883fc805b..e539d4b05c48 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
@@ -36,10 +36,12 @@ struct mxsfb_drm_private {
 
 	struct drm_simple_display_pipe	pipe;
 	struct drm_connector		connector;
 	struct drm_panel		*panel;
 	struct drm_fbdev_cma		*fbdev;
+
+	bool enabled;
 };
 
 int mxsfb_setup_crtc(struct drm_device *dev);
 int mxsfb_create_output(struct drm_device *dev);
 
-- 
2.17.1

Re: [PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block

[Leonard Crestez]

On Tue, 2018-07-17 at 13:48 +0300, Leonard Crestez wrote:
> 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.
> 
> Ensure power is on when required by adding pm_runtime_get/put_sync to
> mxsfb_pipe_enable/disable.
> 
> Since power is lost on suspend implement PM_SLEEP_OPS using
> drm_mode_config_helper_suspend/resume.
> 
> The mxsfb_plane_atomic_update function can get called before
> mxsfb_pipe_enable while the block is not yet powered. When this happens
> the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank
> until a refresh.
> 
> Fix this by not writing gem->paddr if the block is not enabled and
> instead delaying the write until the next mxsfb_crtc_mode_set_nofb call.
> At that point also update cur_buf to avoid an initial corrupt frame
> after resume.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

This is a gentle reminder that this patch was sent two weeks ago
without any reply. Since v1 went ~1 month without a reply it makes
sense to send an explicit ping.

Can somebody please take a look at this?

> ---
> 
> The purpose of this patch is to prepare for enabling power gating on
> DISPLAY power domains.
> 
> Changes since v1:
> * Drop mxsfb_runtime_suspend/mxsfb_runtime_resume, calling
> pm_runtime_get/put in pipe enable/disable is enough.
> * Use drm_mode_config_helper_suspend/resume instead of attempting to
> track state manually.
> * Don't touch NEXT_BUF if atomic_update called with crtc disabled
> * Also update CUR_BUF on enable, this avoids initial corrupt frames.
> 
> Previous discussion: https://lkml.org/lkml/2018/7/17/329
> ---
>  drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 51 +++++++++++++++++++++++-------
>  drivers/gpu/drm/mxsfb/mxsfb_drv.c  | 25 +++++++++++++++
>  drivers/gpu/drm/mxsfb/mxsfb_drv.h  |  2 ++
>  3 files changed, 67 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> index 0abe77675b76..10153da77c40 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,35 +284,47 @@ 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)
>  {
> +	if (mxsfb->enabled)
> +		return;
> +
>  	mxsfb_crtc_mode_set_nofb(mxsfb);
>  	mxsfb_enable_controller(mxsfb);
> +
> +	mxsfb->enabled = true;
>  }
>  
>  void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb)
>  {
> +	if (!mxsfb->enabled)
> +		return;
> +
>  	mxsfb_disable_controller(mxsfb);
> +
> +	mxsfb->enabled = false;
>  }
>  
>  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 +335,15 @@ 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)
> +	if (!mxsfb->enabled)
>  		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);
> +	}
>  }
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> index dd1dd58e4956..a5269fccbed9 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)
>  {
> @@ -412,17 +416,38 @@ static int mxsfb_remove(struct platform_device *pdev)
>  	drm_dev_unref(drm);
>  
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM
> +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-drm",
>  		.of_match_table	= mxsfb_dt_ids,
> +		.pm		= &mxsfb_pm_ops,
>  	},
>  };
>  
>  module_platform_driver(mxsfb_platform_driver);
>  
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> index 5d0883fc805b..e539d4b05c48 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> @@ -36,10 +36,12 @@ struct mxsfb_drm_private {
>  
>  	struct drm_simple_display_pipe	pipe;
>  	struct drm_connector		connector;
>  	struct drm_panel		*panel;
>  	struct drm_fbdev_cma		*fbdev;
> +
> +	bool enabled;
>  };
>  
>  int mxsfb_setup_crtc(struct drm_device *dev);
>  int mxsfb_create_output(struct drm_device *dev);
>  

Re: [PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block

[Philipp Zabel]

On Tue, 2018-07-17 at 13:48 +0300, Leonard Crestez wrote:
> 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.
> 
> Ensure power is on when required by adding pm_runtime_get/put_sync to
> mxsfb_pipe_enable/disable.
> 
> Since power is lost on suspend implement PM_SLEEP_OPS using
> drm_mode_config_helper_suspend/resume.
> 
> The mxsfb_plane_atomic_update function can get called before
> mxsfb_pipe_enable while the block is not yet powered. When this happens
> the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank
> until a refresh.

Why does this happen?

> Fix this by not writing gem->paddr if the block is not enabled and
> instead delaying the write until the next mxsfb_crtc_mode_set_nofb call.
> At that point also update cur_buf to avoid an initial corrupt frame
> after resume.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Tested-by: Philipp Zabel <p.zabel@pengutronix.de>

on imx6ull-evk, this fixes the initial corrupt frame.

regards
Philipp

> 
> ---
> 
> The purpose of this patch is to prepare for enabling power gating on
> DISPLAY power domains.
> 
> Changes since v1:
> * Drop mxsfb_runtime_suspend/mxsfb_runtime_resume, calling
> pm_runtime_get/put in pipe enable/disable is enough.
> * Use drm_mode_config_helper_suspend/resume instead of attempting to
> track state manually.
> * Don't touch NEXT_BUF if atomic_update called with crtc disabled
> * Also update CUR_BUF on enable, this avoids initial corrupt frames.
> 
> Previous discussion: https://lkml.org/lkml/2018/7/17/329
> ---
>  drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 51 +++++++++++++++++++++++-------
>  drivers/gpu/drm/mxsfb/mxsfb_drv.c  | 25 +++++++++++++++
>  drivers/gpu/drm/mxsfb/mxsfb_drv.h  |  2 ++
>  3 files changed, 67 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> index 0abe77675b76..10153da77c40 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,35 +284,47 @@ 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)
>  {
> +	if (mxsfb->enabled)
> +		return;
> +
>  	mxsfb_crtc_mode_set_nofb(mxsfb);
>  	mxsfb_enable_controller(mxsfb);
> +
> +	mxsfb->enabled = true;
>  }
>  
>  void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb)
>  {
> +	if (!mxsfb->enabled)
> +		return;
> +
>  	mxsfb_disable_controller(mxsfb);
> +
> +	mxsfb->enabled = false;
>  }
>  
>  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 +335,15 @@ 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)
> +	if (!mxsfb->enabled)
>  		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);
> +	}
>  }
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> index dd1dd58e4956..a5269fccbed9 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)
>  {
> @@ -412,17 +416,38 @@ static int mxsfb_remove(struct platform_device *pdev)
>  	drm_dev_unref(drm);
>  
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM
> +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-drm",
>  		.of_match_table	= mxsfb_dt_ids,
> +		.pm		= &mxsfb_pm_ops,
>  	},
>  };
>  
>  module_platform_driver(mxsfb_platform_driver);
>  
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> index 5d0883fc805b..e539d4b05c48 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> @@ -36,10 +36,12 @@ struct mxsfb_drm_private {
>  
>  	struct drm_simple_display_pipe	pipe;
>  	struct drm_connector		connector;
>  	struct drm_panel		*panel;
>  	struct drm_fbdev_cma		*fbdev;
> +
> +	bool enabled;
>  };
>  
>  int mxsfb_setup_crtc(struct drm_device *dev);
>  int mxsfb_create_output(struct drm_device *dev);
>  

Re: [PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block

[Leonard Crestez]

On Tue, 2018-07-31 at 13:54 +0200, Philipp Zabel wrote:
> On Tue, 2018-07-17 at 13:48 +0300, Leonard Crestez wrote:
> > 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.
> > 
> > Ensure power is on when required by adding pm_runtime_get/put_sync to
> > mxsfb_pipe_enable/disable.
> > 
> > Since power is lost on suspend implement PM_SLEEP_OPS using
> > drm_mode_config_helper_suspend/resume.
> > 
> > The mxsfb_plane_atomic_update function can get called before
> > mxsfb_pipe_enable while the block is not yet powered. When this happens
> > the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank
> > until a refresh.
> 
> Why does this happen?

I'm not sure what you're asking but register writes to unpowered or
unclocked blocks are not expected to "just work". Here the write is
ignored/lost but I think on imx8 it can even cause a bus error.

The approach here is to only set the framebuffer address as part of
activating the display.

> > Fix this by not writing gem->paddr if the block is not enabled and
> > instead delaying the write until the next mxsfb_crtc_mode_set_nofb call.
> > At that point also update cur_buf to avoid an initial corrupt frame
> > after resume.
> > 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> 
> Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> on imx6ull-evk, this fixes the initial corrupt frame.

Cool!

Re: [PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block

[Stefan Agner]

On 17.07.2018 12:48, Leonard Crestez wrote:
> 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.
> 
> Ensure power is on when required by adding pm_runtime_get/put_sync to
> mxsfb_pipe_enable/disable.
> 
> Since power is lost on suspend implement PM_SLEEP_OPS using
> drm_mode_config_helper_suspend/resume.
> 
> The mxsfb_plane_atomic_update function can get called before
> mxsfb_pipe_enable while the block is not yet powered. When this happens
> the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank
> until a refresh.
> 
> Fix this by not writing gem->paddr if the block is not enabled and
> instead delaying the write until the next mxsfb_crtc_mode_set_nofb call.
> At that point also update cur_buf to avoid an initial corrupt frame
> after resume.

You seem to do two things in a single patch. I know they are related,
but it still seems better if you split it up:

1. Introduce mxsfb_get_fb_paddr and set framebuffer pointers properly
(this seems to have a positive effect even without PM as reported by
Philipp)
2. Add PM callbacks

I think in a follow up patch we can also use runtime pm to
enable/disable the AXI clock. Add driver level runtime pm functions
which enable disable the clocks.

> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> 
> ---
> 
> The purpose of this patch is to prepare for enabling power gating on
> DISPLAY power domains.
> 
> Changes since v1:
> * Drop mxsfb_runtime_suspend/mxsfb_runtime_resume, calling
> pm_runtime_get/put in pipe enable/disable is enough.
> * Use drm_mode_config_helper_suspend/resume instead of attempting to
> track state manually.
> * Don't touch NEXT_BUF if atomic_update called with crtc disabled
> * Also update CUR_BUF on enable, this avoids initial corrupt frames.
> 
> Previous discussion: https://lkml.org/lkml/2018/7/17/329
> ---
>  drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 51 +++++++++++++++++++++++-------
>  drivers/gpu/drm/mxsfb/mxsfb_drv.c  | 25 +++++++++++++++
>  drivers/gpu/drm/mxsfb/mxsfb_drv.h  |  2 ++
>  3 files changed, 67 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> index 0abe77675b76..10153da77c40 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,35 +284,47 @@ 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)
>  {
> +	if (mxsfb->enabled)
> +		return;
> +
>  	mxsfb_crtc_mode_set_nofb(mxsfb);
>  	mxsfb_enable_controller(mxsfb);
> +
> +	mxsfb->enabled = true;
>  }
>  
>  void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb)
>  {
> +	if (!mxsfb->enabled)
> +		return;
> +
>  	mxsfb_disable_controller(mxsfb);
> +
> +	mxsfb->enabled = false;
>  }
>  
>  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 +335,15 @@ 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)
> +	if (!mxsfb->enabled)
>  		return;
>  

I think this is the wrong thing to do.

The simple KMS helper callback ->update() is called by the
->atomic_update() callback of drm_plane_helper_funcs.

And the documentation of atomic_update() states:
https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms-helpers.html#c.drm_plane_helper_funcs

"Note that the power state of the display pipe when this function is
called depends upon the exact helpers and calling sequence the driver
has picked. See drm_atomic_helper_commit_planes() for a discussion of
the tradeoffs and variants of plane commit helpers."

The documentation of drm_atomic_helper_commit_planes() then has more
information:
https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms-helpers.html#c.drm_atomic_helper_commit_planes

Bottom line, we should be using drm_atomic_helper_commit_tail_rpm() for
runtime pm...

So adding something like:

static const struct drm_mode_config_helper_funcs
mxsfb_drm_mode_config_helpers = {
	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
};

And add something like this in mxsfb_load:

	drm->mode_config.funcs		= &mxsfb_mode_config_funcs;
+	dev->mode_config.helper_private = &mxsfb_drm_mode_config_helpers;
...

Should make the stack not calling update while the pipe is disabled.

With that you do not have to keep state locally and can always apply the
new state in ->enable().

> -	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);
> +	}
>  }
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> index dd1dd58e4956..a5269fccbed9 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)
>  {
> @@ -412,17 +416,38 @@ static int mxsfb_remove(struct platform_device *pdev)
>  	drm_dev_unref(drm);
>  
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM

This should be CONFIG_PM_SLEEP afaict.

--
Stefan

> +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-drm",
>  		.of_match_table	= mxsfb_dt_ids,
> +		.pm		= &mxsfb_pm_ops,
>  	},
>  };
>  
>  module_platform_driver(mxsfb_platform_driver);
>  
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> index 5d0883fc805b..e539d4b05c48 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> @@ -36,10 +36,12 @@ struct mxsfb_drm_private {
>  
>  	struct drm_simple_display_pipe	pipe;
>  	struct drm_connector		connector;
>  	struct drm_panel		*panel;
>  	struct drm_fbdev_cma		*fbdev;
> +
> +	bool enabled;
>  };
>  
>  int mxsfb_setup_crtc(struct drm_device *dev);
>  int mxsfb_create_output(struct drm_device *dev);

Re: [PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block

[Philipp Zabel]

On Tue, 2018-07-31 at 12:17 +0000, Leonard Crestez wrote:
> On Tue, 2018-07-31 at 13:54 +0200, Philipp Zabel wrote:
> > On Tue, 2018-07-17 at 13:48 +0300, Leonard Crestez wrote:
> > > 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.
> > > 
> > > Ensure power is on when required by adding pm_runtime_get/put_sync to
> > > mxsfb_pipe_enable/disable.
> > > 
> > > Since power is lost on suspend implement PM_SLEEP_OPS using
> > > drm_mode_config_helper_suspend/resume.
> > > 
> > > The mxsfb_plane_atomic_update function can get called before
> > > mxsfb_pipe_enable while the block is not yet powered. When this happens
> > > the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank
> > > until a refresh.
> > 
> > Why does this happen?
> 
> I'm not sure what you're asking but register writes to unpowered or
> unclocked blocks are not expected to "just work". Here the write is
> ignored/lost but I think on imx8 it can even cause a bus error.
> 
> The approach here is to only set the framebuffer address as part of
> activating the display.

I wonder why atomic update is called at all while the pipe is not
enabled.

regards
Philipp

Re: [PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block

[Stefan Agner]

On 02.08.2018 12:17, Philipp Zabel wrote:
> On Tue, 2018-07-31 at 12:17 +0000, Leonard Crestez wrote:
>> On Tue, 2018-07-31 at 13:54 +0200, Philipp Zabel wrote:
>> > On Tue, 2018-07-17 at 13:48 +0300, Leonard Crestez wrote:
>> > > 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.
>> > >
>> > > Ensure power is on when required by adding pm_runtime_get/put_sync to
>> > > mxsfb_pipe_enable/disable.
>> > >
>> > > Since power is lost on suspend implement PM_SLEEP_OPS using
>> > > drm_mode_config_helper_suspend/resume.
>> > >
>> > > The mxsfb_plane_atomic_update function can get called before
>> > > mxsfb_pipe_enable while the block is not yet powered. When this happens
>> > > the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank
>> > > until a refresh.
>> >
>> > Why does this happen?
>>
>> I'm not sure what you're asking but register writes to unpowered or
>> unclocked blocks are not expected to "just work". Here the write is
>> ignored/lost but I think on imx8 it can even cause a bus error.
>>
>> The approach here is to only set the framebuffer address as part of
>> activating the display.
> 
> I wonder why atomic update is called at all while the pipe is not
> enabled.

It can be made to behave differently (see also my review).

However, the default seems also a bit unfortunate to me.

--
Stefan

Re: [PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block

[Leonard Crestez]

On Thu, 2018-08-02 at 13:06 +0200, Stefan Agner wrote:
> On 02.08.2018 12:17, Philipp Zabel wrote:
> > On Tue, 2018-07-31 at 12:17 +0000, Leonard Crestez wrote:
> > > On Tue, 2018-07-31 at 13:54 +0200, Philipp Zabel wrote:
> > > > On Tue, 2018-07-17 at 13:48 +0300, Leonard Crestez wrote:
> > > > > 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.
> > > > > 
> > > > > Ensure power is on when required by adding pm_runtime_get/put_sync to
> > > > > mxsfb_pipe_enable/disable.
> > > > > 
> > > > > Since power is lost on suspend implement PM_SLEEP_OPS using
> > > > > drm_mode_config_helper_suspend/resume.
> > > > > 
> > > > > The mxsfb_plane_atomic_update function can get called before
> > > > > mxsfb_pipe_enable while the block is not yet powered. When this happens
> > > > > the write to LCDIF_NEXT_BUF is lost causing corrupt display on unblank
> > > > > until a refresh.
> > > > 
> > > > Why does this happen?
> > > 
> > > I'm not sure what you're asking but register writes to unpowered or
> > > unclocked blocks are not expected to "just work". Here the write is
> > > ignored/lost but I think on imx8 it can even cause a bus error.
> > > 
> > > The approach here is to only set the framebuffer address as part of
> > > activating the display.
> > 
> > I wonder why atomic update is called at all while the pipe is not
> > enabled.
> 
> It can be made to behave differently (see also my review).

Setting the framebuffer before enabling the crtc makes sense to me,
otherwise an initial corrupt frame would be impossible to avoid.

It's just that it slightly complicates PM for simple devices. Maybe
drm_simple_display_pipe could have prepare/unprepare callbacks for
PM+modeset separate from just crtc enable/disable?

Re: [PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block

[Philipp Zabel]

Hi Stefan,

On Wed, 2018-08-01 at 12:00 +0200, Stefan Agner wrote:
[...]
> > @@ -307,14 +335,15 @@ 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)
> > +	if (!mxsfb->enabled)
> >  		return;
> >  
> 
> I think this is the wrong thing to do.
> 
> The simple KMS helper callback ->update() is called by the
> ->atomic_update() callback of drm_plane_helper_funcs.
> 
> And the documentation of atomic_update() states:
> https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms-helpers.html#c.drm_plane_helper_funcs
> 
> "Note that the power state of the display pipe when this function is
> called depends upon the exact helpers and calling sequence the driver
> has picked. See drm_atomic_helper_commit_planes() for a discussion of
> the tradeoffs and variants of plane commit helpers."
> 
> The documentation of drm_atomic_helper_commit_planes() then has more
> information:
> https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms-helpers.html#c.drm_atomic_helper_commit_planes
> 
> Bottom line, we should be using drm_atomic_helper_commit_tail_rpm() for
> runtime pm...
> 
> So adding something like:
> 
> static const struct drm_mode_config_helper_funcs
> mxsfb_drm_mode_config_helpers = {
> 	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> };
> 
> And add something like this in mxsfb_load:
> 
> 	drm->mode_config.funcs		= &mxsfb_mode_config_funcs;
> +	dev->mode_config.helper_private = &mxsfb_drm_mode_config_helpers;
> ...
> 
> Should make the stack not calling update while the pipe is disabled.
> 
> With that you do not have to keep state locally and can always apply the
> new state in ->enable().

Yes, thank you for the explanation. That is exactly what I would have
expected.

regards
Philipp