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

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

[Leonard Crestez]

Adding imx6sl/sx lcdif nodes in 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.

Fix by adding pm_runtime_get/put_sync to mxsfb_pipe_enable/disable.

The mxsfb_plane_atomic_update function can get called before
mxsfb_pipe_enable while the block is not powered. When this happens the
write to LCDIF_NEXT_BUF is lost causing random corrupted pixels on
unblank.

Fix this by splitting the writing of gem->paddr to nextbuf into a
mxsfb_update_hw_next_buf functiona and also calling it from
mxsfb_crtc_mode_set_nofb.

Also add fields to mxsfb_drv to keep track of enabled/suspended states.

Signed-off-by: Robert Chiras <robert.chiras@nxp.com>
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

---
 drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 38 +++++++++++-----
 drivers/gpu/drm/mxsfb/mxsfb_drv.c  | 72 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/mxsfb/mxsfb_drv.h  |  3 ++
 3 files changed, 100 insertions(+), 13 deletions(-)

This was initially written by Robert for imx8m but I tested it also
works on imx6sx/imx6sl to DISPMIX power domain.

Tested on imx6sl-evk and imx6sx-sdb with SEIKO 43WVF1G panel by
blanking and unblanking via sysfs and suspend/resume

Testing requires a modified config (to enable MXFSB_DRM):
CONFIG_DRM_MXSFB=y
CONFIG_DRM_PANEL_SEIKO_43WVF1G=y
CONFIG_FB_MXS=n

It also requires dts changes to enable the DISPMIX power domain.  The
dts changes might break drivers so this patch attempts to fix the lcdif
driver first.

Patch 2 is a RFC of imx6sl changes, imx6sx is a bit more complicated
and it also interacts with PCI.

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
index 0abe77675b76..cce2ec1c80ae 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
@@ -194,10 +194,25 @@ static int mxsfb_reset_block(void __iomem *reset_addr)
 		return ret;
 
 	return clear_poll_bit(reset_addr, MODULE_CLKGATE);
 }
 
+static void mxsfb_update_hw_next_buf(struct mxsfb_drm_private *mxsfb)
+{
+	struct drm_framebuffer *fb = mxsfb->pipe.plane.state->fb;
+	struct drm_gem_cma_object *gem;
+
+	if (!fb)
+		return;
+
+	gem = drm_fb_cma_get_gem_obj(fb, 0);
+	if (!gem)
+		return;
+
+	writel(gem->paddr, mxsfb->base + mxsfb->devdata->next_buf);
+}
+
 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;
@@ -268,35 +283,41 @@ 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);
 
+	mxsfb_update_hw_next_buf(mxsfb);
 	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;
 
 	spin_lock_irq(&crtc->dev->event_lock);
 	event = crtc->state->event;
 	if (event) {
 		crtc->state->event = NULL;
@@ -307,14 +328,9 @@ 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_update_hw_next_buf(mxsfb);
 	mxsfb_disable_axi_clk(mxsfb);
 }
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index 5cae8db9dcd4..c889cac2e275 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -100,23 +100,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 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)
 {
@@ -175,10 +179,11 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags)
 	if (!mxsfb)
 		return -ENOMEM;
 
 	drm->dev_private = mxsfb;
 	mxsfb->devdata = &mxsfb_devdata[pdev->id_entry->driver_data];
+	platform_set_drvdata(pdev, drm);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	mxsfb->base = devm_ioremap_resource(drm->dev, res);
 	if (IS_ERR(mxsfb->base))
 		return PTR_ERR(mxsfb->base);
@@ -256,12 +261,10 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags)
 		mxsfb->fbdev = NULL;
 		dev_err(drm->dev, "Failed to init FB CMA area\n");
 		goto err_cma;
 	}
 
-	platform_set_drvdata(pdev, drm);
-
 	drm_helper_hpd_irq_event(drm);
 
 	return 0;
 
 err_cma:
@@ -417,17 +420,82 @@ static int mxsfb_remove(struct platform_device *pdev)
 	drm_dev_unref(drm);
 
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int mxsfb_runtime_suspend(struct device *dev)
+{
+	struct drm_device *drm = dev_get_drvdata(dev);
+	struct mxsfb_drm_private *mxsfb = drm->dev_private;
+
+	if (!drm->registered)
+		return 0;
+
+	if (mxsfb->enabled) {
+		mxsfb_crtc_disable(mxsfb);
+		mxsfb->suspended = true;
+	}
+
+	return 0;
+}
+
+static int mxsfb_runtime_resume(struct device *dev)
+{
+	struct drm_device *drm = dev_get_drvdata(dev);
+	struct mxsfb_drm_private *mxsfb = drm->dev_private;
+
+	if (!drm->registered || !mxsfb->suspended)
+		return 0;
+
+	mxsfb_crtc_enable(mxsfb);
+	mxsfb->suspended = false;
+
+	return 0;
+}
+
+static int mxsfb_suspend(struct device *dev)
+{
+	struct drm_device *drm = dev_get_drvdata(dev);
+	struct mxsfb_drm_private *mxsfb = drm->dev_private;
+
+	if (mxsfb->enabled) {
+		mxsfb_crtc_disable(mxsfb);
+		mxsfb->suspended = true;
+	}
+
+	return 0;
+}
+
+static int mxsfb_resume(struct device *dev)
+{
+	struct drm_device *drm = dev_get_drvdata(dev);
+	struct mxsfb_drm_private *mxsfb = drm->dev_private;
+
+	if (!mxsfb->suspended)
+		return 0;
+
+	mxsfb_crtc_enable(mxsfb);
+	mxsfb->suspended = false;
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops mxsfb_pm_ops = {
+	SET_RUNTIME_PM_OPS(mxsfb_runtime_suspend, mxsfb_runtime_resume, NULL)
+	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);
 
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
index 5d0883fc805b..d8ef4a053f0e 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
@@ -36,10 +36,13 @@ 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;
+	bool                            suspended;
 };
 
 int mxsfb_setup_crtc(struct drm_device *dev);
 int mxsfb_create_output(struct drm_device *dev);
 
-- 
2.17.0

[RFC 2/2] ARM: dts: imx6sl: Convert gpc to new bindings

[Leonard Crestez]

With old bindings imx_gpc_onecell_data always sets num_domains to 2 so
the DISPMIX domain can't actually be referenced. The pd is still defined
and pm core shuts it down as "unused" so display can't work.

Converting to new gpc bindings by adding pgc nodes, also reference
referencing the newly-defined &pu_disp domain from &lcdif.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 arch/arm/boot/dts/imx6sl.dtsi | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

There are erratas regarding dispmix on 6sl so this might be wrong

diff --git a/arch/arm/boot/dts/imx6sl.dtsi b/arch/arm/boot/dts/imx6sl.dtsi
index ab6a7e2e7e8f..9982874fa39e 100644
--- a/arch/arm/boot/dts/imx6sl.dtsi
+++ b/arch/arm/boot/dts/imx6sl.dtsi
@@ -694,14 +694,42 @@
 				reg = <0x020dc000 0x4000>;
 				interrupt-controller;
 				#interrupt-cells = <3>;
 				interrupts = <0 89 IRQ_TYPE_LEVEL_HIGH>;
 				interrupt-parent = <&intc>;
-				pu-supply = <&reg_pu>;
-				clocks = <&clks IMX6SL_CLK_GPU2D_OVG>,
-					 <&clks IMX6SL_CLK_GPU2D_PODF>;
+				clocks = <&clks IMX6SL_CLK_IPG>;
+				clock-names = "ipg";
 				#power-domain-cells = <1>;
+
+				pgc {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					power-domain@0 {
+						reg = <0>;
+						#power-domain-cells = <0>;
+					};
+
+					pd_pu: power-domain@1 {
+						reg = <1>;
+						#power-domain-cells = <0>;
+						power-supply = <&reg_pu>;
+						clocks = <&clks IMX6SL_CLK_GPU2D_OVG>,
+						         <&clks IMX6SL_CLK_GPU2D_PODF>;
+					};
+
+					pd_disp: power-domain@2 {
+						reg = <2>;
+						#power-domain-cells = <0>;
+						clocks = <&clks IMX6SL_CLK_IPG>,
+							 <&clks IMX6SL_CLK_LCDIF_AXI>,
+							 <&clks IMX6SL_CLK_LCDIF_PIX>,
+							 <&clks IMX6SL_CLK_EPDC_AXI>,
+							 <&clks IMX6SL_CLK_EPDC_PIX>,
+							 <&clks IMX6SL_CLK_PXP_AXI>;
+					};
+				};
 			};
 
 			gpr: iomuxc-gpr@20e0000 {
 				compatible = "fsl,imx6sl-iomuxc-gpr",
 					     "fsl,imx6q-iomuxc-gpr", "syscon";
@@ -752,10 +780,11 @@
 				clocks = <&clks IMX6SL_CLK_LCDIF_PIX>,
 					 <&clks IMX6SL_CLK_LCDIF_AXI>,
 					 <&clks IMX6SL_CLK_DUMMY>;
 				clock-names = "pix", "axi", "disp_axi";
 				status = "disabled";
+				power-domains = <&pd_disp>;
 			};
 
 			dcp: dcp@20fc000 {
 				compatible = "fsl,imx6sl-dcp", "fsl,imx28-dcp";
 				reg = <0x020fc000 0x4000>;
-- 
2.17.0

Re: [RFC 2/2] ARM: dts: imx6sl: Convert gpc to new bindings

[Lucas Stach]

Am Dienstag, den 05.06.2018, 20:11 +0300 schrieb Leonard Crestez:
> With old bindings imx_gpc_onecell_data always sets num_domains to 2 so
> the DISPMIX domain can't actually be referenced. The pd is still defined
> and pm core shuts it down as "unused" so display can't work.
> 
> Converting to new gpc bindings by adding pgc nodes, also reference
> referencing the newly-defined &pu_disp domain from &lcdif.
> 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  arch/arm/boot/dts/imx6sl.dtsi | 35 ++++++++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> There are erratas regarding dispmix on 6sl so this might be wrong
> 
> diff --git a/arch/arm/boot/dts/imx6sl.dtsi b/arch/arm/boot/dts/imx6sl.dtsi
> index ab6a7e2e7e8f..9982874fa39e 100644
> --- a/arch/arm/boot/dts/imx6sl.dtsi
> +++ b/arch/arm/boot/dts/imx6sl.dtsi
> @@ -694,14 +694,42 @@
> >  				reg = <0x020dc000 0x4000>;
> >  				interrupt-controller;
> >  				#interrupt-cells = <3>;
> >  				interrupts = <0 89 IRQ_TYPE_LEVEL_HIGH>;
> >  				interrupt-parent = <&intc>;
> > -				pu-supply = <&reg_pu>;
> > -				clocks = <&clks IMX6SL_CLK_GPU2D_OVG>,
> > -					 <&clks IMX6SL_CLK_GPU2D_PODF>;
> > +				clocks = <&clks IMX6SL_CLK_IPG>;
> > +				clock-names = "ipg";
> >  				#power-domain-cells = <1>;

The above power-domain-cells needs to go away when using the new
binding.

Regards,
Lucas

> +				pgc {
> > +					#address-cells = <1>;
> > +					#size-cells = <0>;
> +
> > +					power-domain@0 {
> > +						reg = <0>;
> > +						#power-domain-cells = <0>;
> > +					};
> +
> > > +					pd_pu: power-domain@1 {
> > +						reg = <1>;
> > +						#power-domain-cells = <0>;
> > +						power-supply = <&reg_pu>;
> > +						clocks = <&clks IMX6SL_CLK_GPU2D_OVG>,
> > +						         <&clks IMX6SL_CLK_GPU2D_PODF>;
> > +					};
> +
> > > +					pd_disp: power-domain@2 {
> > +						reg = <2>;
> > +						#power-domain-cells = <0>;
> > +						clocks = <&clks IMX6SL_CLK_IPG>,
> > +							 <&clks IMX6SL_CLK_LCDIF_AXI>,
> > +							 <&clks IMX6SL_CLK_LCDIF_PIX>,
> > +							 <&clks IMX6SL_CLK_EPDC_AXI>,
> > +							 <&clks IMX6SL_CLK_EPDC_PIX>,
> > +							 <&clks IMX6SL_CLK_PXP_AXI>;
> > +					};
> > +				};
> >  			};
>  
> > >  			gpr: iomuxc-gpr@20e0000 {
> >  				compatible = "fsl,imx6sl-iomuxc-gpr",
> >  					     "fsl,imx6q-iomuxc-gpr", "syscon";
> @@ -752,10 +780,11 @@
> >  				clocks = <&clks IMX6SL_CLK_LCDIF_PIX>,
> >  					 <&clks IMX6SL_CLK_LCDIF_AXI>,
> >  					 <&clks IMX6SL_CLK_DUMMY>;
> >  				clock-names = "pix", "axi", "disp_axi";
> >  				status = "disabled";
> > +				power-domains = <&pd_disp>;
> >  			};
>  
> > >  			dcp: dcp@20fc000 {
> >  				compatible = "fsl,imx6sl-dcp", "fsl,imx28-dcp";
> >  				reg = <0x020fc000 0x4000>;

Re: [RFC 2/2] ARM: dts: imx6sl: Convert gpc to new bindings

[Leonard Crestez]

On Wed, 2018-06-06 at 11:24 +0200, Lucas Stach wrote:
> Am Dienstag, den 05.06.2018, 20:11 +0300 schrieb Leonard Crestez:
> > With old bindings imx_gpc_onecell_data always sets num_domains to 2 so
> > the DISPMIX domain can't actually be referenced. The pd is still defined
> > and pm core shuts it down as "unused" so display can't work.
> > 
> > Converting to new gpc bindings by adding pgc nodes, also reference
> > referencing the newly-defined &pu_disp domain from &lcdif.
> > 
> > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > 
> > ---
> >  arch/arm/boot/dts/imx6sl.dtsi | 35 ++++++++++++++++++++++++++++++++---
> >  1 file changed, 32 insertions(+), 3 deletions(-)
> > 
> > There are erratas regarding dispmix on 6sl so this might be wrong
> > 
> > diff --git a/arch/arm/boot/dts/imx6sl.dtsi b/arch/arm/boot/dts/imx6sl.dtsi
> > index ab6a7e2e7e8f..9982874fa39e 100644
> > --- a/arch/arm/boot/dts/imx6sl.dtsi
> > +++ b/arch/arm/boot/dts/imx6sl.dtsi
> > @@ -694,14 +694,42 @@
> > >  				reg = <0x020dc000 0x4000>;
> > >  				interrupt-controller;
> > >  				#interrupt-cells = <3>;
> > >  				interrupts = <0 89 IRQ_TYPE_LEVEL_HIGH>;
> > >  				interrupt-parent = <&intc>;
> > > -				pu-supply = <&reg_pu>;
> > > -				clocks = <&clks IMX6SL_CLK_GPU2D_OVG>,
> > > -					 <&clks IMX6SL_CLK_GPU2D_PODF>;
> > > +				clocks = <&clks IMX6SL_CLK_IPG>;
> > > +				clock-names = "ipg";
> > >  				#power-domain-cells = <1>;
> 
> The above power-domain-cells needs to go away when using the new
> binding.

Yes, I fixed this for next version.

However for imx6sl there is a very nasty errata published and I've been
unable to get confirmation of a fix in later versions so it should
probably receive the PGC_DOMAIN_FLAG_NO_PD treatment.

See ERR006287 in https://www.nxp.com/docs/en/errata/IMX6SLCE.pdf

Any comments on the first patch for mxsfb? It's far more interesting.

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

[Stefan Agner]

On 05.06.2018 19:11, Leonard Crestez wrote:
> Adding imx6sl/sx lcdif nodes in 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.
> 
> Fix by adding pm_runtime_get/put_sync to mxsfb_pipe_enable/disable.
> 
> The mxsfb_plane_atomic_update function can get called before
> mxsfb_pipe_enable while the block is not powered. When this happens the
> write to LCDIF_NEXT_BUF is lost causing random corrupted pixels on
> unblank.
> 
> Fix this by splitting the writing of gem->paddr to nextbuf into a
> mxsfb_update_hw_next_buf functiona and also calling it from
> mxsfb_crtc_mode_set_nofb.
> 
> Also add fields to mxsfb_drv to keep track of enabled/suspended states.
> 
> Signed-off-by: Robert Chiras <robert.chiras@nxp.com>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

That patch seems to go in the right direction.

One comment below.

> 
> ---
>  drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 38 +++++++++++-----
>  drivers/gpu/drm/mxsfb/mxsfb_drv.c  | 72 +++++++++++++++++++++++++++++-
>  drivers/gpu/drm/mxsfb/mxsfb_drv.h  |  3 ++
>  3 files changed, 100 insertions(+), 13 deletions(-)
> 
> This was initially written by Robert for imx8m but I tested it also
> works on imx6sx/imx6sl to DISPMIX power domain.
> 
> Tested on imx6sl-evk and imx6sx-sdb with SEIKO 43WVF1G panel by
> blanking and unblanking via sysfs and suspend/resume
> 
> Testing requires a modified config (to enable MXFSB_DRM):
> CONFIG_DRM_MXSFB=y
> CONFIG_DRM_PANEL_SEIKO_43WVF1G=y
> CONFIG_FB_MXS=n
> 
> It also requires dts changes to enable the DISPMIX power domain.  The
> dts changes might break drivers so this patch attempts to fix the lcdif
> driver first.
> 
> Patch 2 is a RFC of imx6sl changes, imx6sx is a bit more complicated
> and it also interacts with PCI.
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> index 0abe77675b76..cce2ec1c80ae 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> @@ -194,10 +194,25 @@ static int mxsfb_reset_block(void __iomem *reset_addr)
>  		return ret;
>  
>  	return clear_poll_bit(reset_addr, MODULE_CLKGATE);
>  }
>  
> +static void mxsfb_update_hw_next_buf(struct mxsfb_drm_private *mxsfb)
> +{
> +	struct drm_framebuffer *fb = mxsfb->pipe.plane.state->fb;
> +	struct drm_gem_cma_object *gem;
> +
> +	if (!fb)
> +		return;
> +
> +	gem = drm_fb_cma_get_gem_obj(fb, 0);
> +	if (!gem)
> +		return;
> +
> +	writel(gem->paddr, mxsfb->base + mxsfb->devdata->next_buf);
> +}
> +
>  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;
> @@ -268,35 +283,41 @@ 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);
>  
> +	mxsfb_update_hw_next_buf(mxsfb);
>  	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;

Is this state keeping really necessary?

--
Stefan

>  }
>  
>  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;
>  
>  	spin_lock_irq(&crtc->dev->event_lock);
>  	event = crtc->state->event;
>  	if (event) {
>  		crtc->state->event = NULL;
> @@ -307,14 +328,9 @@ 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_update_hw_next_buf(mxsfb);
>  	mxsfb_disable_axi_clk(mxsfb);
>  }
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> index 5cae8db9dcd4..c889cac2e275 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> @@ -100,23 +100,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 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)
>  {
> @@ -175,10 +179,11 @@ static int mxsfb_load(struct drm_device *drm,
> unsigned long flags)
>  	if (!mxsfb)
>  		return -ENOMEM;
>  
>  	drm->dev_private = mxsfb;
>  	mxsfb->devdata = &mxsfb_devdata[pdev->id_entry->driver_data];
> +	platform_set_drvdata(pdev, drm);
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	mxsfb->base = devm_ioremap_resource(drm->dev, res);
>  	if (IS_ERR(mxsfb->base))
>  		return PTR_ERR(mxsfb->base);
> @@ -256,12 +261,10 @@ static int mxsfb_load(struct drm_device *drm,
> unsigned long flags)
>  		mxsfb->fbdev = NULL;
>  		dev_err(drm->dev, "Failed to init FB CMA area\n");
>  		goto err_cma;
>  	}
>  
> -	platform_set_drvdata(pdev, drm);
> -
>  	drm_helper_hpd_irq_event(drm);
>  
>  	return 0;
>  
>  err_cma:
> @@ -417,17 +420,82 @@ static int mxsfb_remove(struct platform_device *pdev)
>  	drm_dev_unref(drm);
>  
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM
> +static int mxsfb_runtime_suspend(struct device *dev)
> +{
> +	struct drm_device *drm = dev_get_drvdata(dev);
> +	struct mxsfb_drm_private *mxsfb = drm->dev_private;
> +
> +	if (!drm->registered)
> +		return 0;
> +
> +	if (mxsfb->enabled) {
> +		mxsfb_crtc_disable(mxsfb);
> +		mxsfb->suspended = true;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mxsfb_runtime_resume(struct device *dev)
> +{
> +	struct drm_device *drm = dev_get_drvdata(dev);
> +	struct mxsfb_drm_private *mxsfb = drm->dev_private;
> +
> +	if (!drm->registered || !mxsfb->suspended)
> +		return 0;
> +
> +	mxsfb_crtc_enable(mxsfb);
> +	mxsfb->suspended = false;
> +
> +	return 0;
> +}
> +
> +static int mxsfb_suspend(struct device *dev)
> +{
> +	struct drm_device *drm = dev_get_drvdata(dev);
> +	struct mxsfb_drm_private *mxsfb = drm->dev_private;
> +
> +	if (mxsfb->enabled) {
> +		mxsfb_crtc_disable(mxsfb);
> +		mxsfb->suspended = true;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mxsfb_resume(struct device *dev)
> +{
> +	struct drm_device *drm = dev_get_drvdata(dev);
> +	struct mxsfb_drm_private *mxsfb = drm->dev_private;
> +
> +	if (!mxsfb->suspended)
> +		return 0;
> +
> +	mxsfb_crtc_enable(mxsfb);
> +	mxsfb->suspended = false;
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops mxsfb_pm_ops = {
> +	SET_RUNTIME_PM_OPS(mxsfb_runtime_suspend, mxsfb_runtime_resume, NULL)
> +	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);
>  
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> index 5d0883fc805b..d8ef4a053f0e 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> @@ -36,10 +36,13 @@ 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;
> +	bool                            suspended;
>  };
>  
>  int mxsfb_setup_crtc(struct drm_device *dev);
>  int mxsfb_create_output(struct drm_device *dev);

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

[Leonard Crestez]

On Fri, 2018-07-13 at 12:57 +0200, Stefan Agner wrote:
> On 05.06.2018 19:11, Leonard Crestez wrote:
> > Adding imx6sl/sx lcdif nodes in 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.
> > 
> > Fix by adding pm_runtime_get/put_sync to mxsfb_pipe_enable/disable.
> > 
> > The mxsfb_plane_atomic_update function can get called before
> > mxsfb_pipe_enable while the block is not powered. When this happens the
> > write to LCDIF_NEXT_BUF is lost causing random corrupted pixels on
> > unblank.
> > 
> > Fix this by splitting the writing of gem->paddr to nextbuf into a
> > mxsfb_update_hw_next_buf functiona and also calling it from
> > mxsfb_crtc_mode_set_nofb.
> > 
> > Also add fields to mxsfb_drv to keep track of enabled/suspended states.

> > +static void mxsfb_update_hw_next_buf(struct mxsfb_drm_private *mxsfb)
> > +{
> > +	struct drm_framebuffer *fb = mxsfb->pipe.plane.state->fb;
> > +	struct drm_gem_cma_object *gem;
> > +
> > +	if (!fb)
> > +		return;
> > +
> > +	gem = drm_fb_cma_get_gem_obj(fb, 0);
> > +	if (!gem)
> > +		return;
> > +
> > +	writel(gem->paddr, mxsfb->base + mxsfb->devdata->next_buf);
> > +}
> > +
> >  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;
> > @@ -268,35 +283,41 @@ 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);
> >  
> > +	mxsfb_update_hw_next_buf(mxsfb);
> >  	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;
> 
> Is this state keeping really necessary?

Sort of. The code is indeed a bit complicated, there are 3 possible
states: "disabled", "enabled", "enabled but suspended"

When the device resumes it should enable the controller only if it was
enabled before (not if screen is blanked). After some digging I found
that suspend/resume could be better implemented using
drm_mode_config_helper_suspend/resume. This should automatically
remember the crtc state "properly" without keeping state in the driver.

One notable issue is that the lcdif block is only powered while the
crtc is enabled but mxsfb_plane_atomic_update can be called outside
that. So we need to check if the CRTC is enabled inside
mxsfb_plane_atomic_update and avoid writing to hardware. Instead the HW
write for next is delayed for the next mxsfb_crtc_mode_set_nofb.

Does this make sense? Being unable to update the plane if the crtc is
not enabled seems like it would be a common limitation of simple
display controllers.

Writing the FB paddr from mxsfb_crtc_mode_set_nofb feels wrong and I'm
not sure it behaves correctly: when the display is resumed it seems to
output an initial frame of incorrect data. It's possible the enable
sequence needs to be adjusted.