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