| Message ID | 20240118183924.144221-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/lcdif: Do not disable clock on already suspended hardware | expand |
On 1/18/24 19:39, Marek Vasut wrote: > In case the LCDIF is enabled in DT but unused, the clock used by the > LCDIF are not enabled. Those clock may even have a use count of 0 in > case there are no other users of those clock. This can happen e.g. in > case the LCDIF drives HDMI bridge which has no panel plugged into the > HDMI connector. > > Do not attempt to disable clock in the suspend callback and re-enable > clock in the resume callback unless the LCDIF is enabled and was in > use before the system entered suspend, otherwise the driver might end > up trying to disable clock which are already disabled with use count > 0, and would trigger a warning from clock core about this condition. > > Note that the lcdif_rpm_suspend() and lcdif_rpm_resume() functions > internally perform the clock disable and enable operations and act > as runtime PM hooks too. > > Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: David Airlie <airlied@gmail.com> > Cc: Fabio Estevam <festevam@gmail.com> > Cc: Liu Ying <victor.liu@nxp.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: NXP Linux Team <linux-imx@nxp.com> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Stefan Agner <stefan@agner.ch> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: dri-devel@lists.freedesktop.org > Cc: linux-arm-kernel@lists.infradead.org > --- > drivers/gpu/drm/mxsfb/lcdif_drv.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c b/drivers/gpu/drm/mxsfb/lcdif_drv.c > index ea10bf81582e9..0f895b8a99d62 100644 > --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c > +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c > @@ -343,6 +343,9 @@ static int __maybe_unused lcdif_suspend(struct device *dev) > if (ret) > return ret; > > + if (pm_runtime_suspended(dev)) > + return 0; > + > return lcdif_rpm_suspend(dev); > } > > @@ -350,7 +353,8 @@ static int __maybe_unused lcdif_resume(struct device *dev) > { > struct drm_device *drm = dev_get_drvdata(dev); > > - lcdif_rpm_resume(dev); > + if (!pm_runtime_suspended(dev)) > + lcdif_rpm_resume(dev); > > return drm_mode_config_helper_resume(drm); > } Is this OK to pick up ? Some AB/RB/input would be appreciated, thanks !
On 1/18/24 19:39, Marek Vasut wrote: > In case the LCDIF is enabled in DT but unused, the clock used by the > LCDIF are not enabled. Those clock may even have a use count of 0 in > case there are no other users of those clock. This can happen e.g. in > case the LCDIF drives HDMI bridge which has no panel plugged into the > HDMI connector. > > Do not attempt to disable clock in the suspend callback and re-enable > clock in the resume callback unless the LCDIF is enabled and was in > use before the system entered suspend, otherwise the driver might end > up trying to disable clock which are already disabled with use count > 0, and would trigger a warning from clock core about this condition. > > Note that the lcdif_rpm_suspend() and lcdif_rpm_resume() functions > internally perform the clock disable and enable operations and act > as runtime PM hooks too. > > Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: David Airlie <airlied@gmail.com> > Cc: Fabio Estevam <festevam@gmail.com> > Cc: Liu Ying <victor.liu@nxp.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: NXP Linux Team <linux-imx@nxp.com> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Stefan Agner <stefan@agner.ch> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: dri-devel@lists.freedesktop.org > Cc: linux-arm-kernel@lists.infradead.org > --- > drivers/gpu/drm/mxsfb/lcdif_drv.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c b/drivers/gpu/drm/mxsfb/lcdif_drv.c > index ea10bf81582e9..0f895b8a99d62 100644 > --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c > +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c > @@ -343,6 +343,9 @@ static int __maybe_unused lcdif_suspend(struct device *dev) > if (ret) > return ret; > > + if (pm_runtime_suspended(dev)) > + return 0; > + > return lcdif_rpm_suspend(dev); > } > > @@ -350,7 +353,8 @@ static int __maybe_unused lcdif_resume(struct device *dev) > { > struct drm_device *drm = dev_get_drvdata(dev); > > - lcdif_rpm_resume(dev); > + if (!pm_runtime_suspended(dev)) > + lcdif_rpm_resume(dev); > > return drm_mode_config_helper_resume(drm); > } Moving Victor to To: , it would be good to get some input on this as it fixes suspend/resume on MX8MP with HDMI .
On Friday, January 19, 2024 2:39 AM, Marek Vasut <marex@denx.de> wrote: > In case the LCDIF is enabled in DT but unused, the clock used by the Nit: s/clock/clocks/ > LCDIF are not enabled. Those clock may even have a use count of 0 in Ditto. > case there are no other users of those clock. This can happen e.g. in Ditto. > case the LCDIF drives HDMI bridge which has no panel plugged into the > HDMI connector. > > Do not attempt to disable clock in the suspend callback and re-enable > clock in the resume callback unless the LCDIF is enabled and was in > use before the system entered suspend, otherwise the driver might end > up trying to disable clock which are already disabled with use count Ditto. With the nitpicks addressed: Reviewed-by: Liu Ying <victor.liu@nxp.com> Regards, Liu Ying > 0, and would trigger a warning from clock core about this condition. > > Note that the lcdif_rpm_suspend() and lcdif_rpm_resume() functions > internally perform the clock disable and enable operations and act > as runtime PM hooks too. > > Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: David Airlie <airlied@gmail.com> > Cc: Fabio Estevam <festevam@gmail.com> > Cc: Liu Ying <victor.liu@nxp.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: NXP Linux Team <linux-imx@nxp.com> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Stefan Agner <stefan@agner.ch> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: dri-devel@lists.freedesktop.org > Cc: linux-arm-kernel@lists.infradead.org > --- > drivers/gpu/drm/mxsfb/lcdif_drv.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c > b/drivers/gpu/drm/mxsfb/lcdif_drv.c > index ea10bf81582e9..0f895b8a99d62 100644 > --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c > +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c > @@ -343,6 +343,9 @@ static int __maybe_unused lcdif_suspend(struct > device *dev) > if (ret) > return ret; > > + if (pm_runtime_suspended(dev)) > + return 0; > + > return lcdif_rpm_suspend(dev); > } > > @@ -350,7 +353,8 @@ static int __maybe_unused lcdif_resume(struct > device *dev) > { > struct drm_device *drm = dev_get_drvdata(dev); > > - lcdif_rpm_resume(dev); > + if (!pm_runtime_suspended(dev)) > + lcdif_rpm_resume(dev); > > return drm_mode_config_helper_resume(drm); > } > -- > 2.43.0
diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c b/drivers/gpu/drm/mxsfb/lcdif_drv.c index ea10bf81582e9..0f895b8a99d62 100644 --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c @@ -343,6 +343,9 @@ static int __maybe_unused lcdif_suspend(struct device *dev) if (ret) return ret; + if (pm_runtime_suspended(dev)) + return 0; + return lcdif_rpm_suspend(dev); } @@ -350,7 +353,8 @@ static int __maybe_unused lcdif_resume(struct device *dev) { struct drm_device *drm = dev_get_drvdata(dev); - lcdif_rpm_resume(dev); + if (!pm_runtime_suspended(dev)) + lcdif_rpm_resume(dev); return drm_mode_config_helper_resume(drm); }
In case the LCDIF is enabled in DT but unused, the clock used by the LCDIF are not enabled. Those clock may even have a use count of 0 in case there are no other users of those clock. This can happen e.g. in case the LCDIF drives HDMI bridge which has no panel plugged into the HDMI connector. Do not attempt to disable clock in the suspend callback and re-enable clock in the resume callback unless the LCDIF is enabled and was in use before the system entered suspend, otherwise the driver might end up trying to disable clock which are already disabled with use count 0, and would trigger a warning from clock core about this condition. Note that the lcdif_rpm_suspend() and lcdif_rpm_resume() functions internally perform the clock disable and enable operations and act as runtime PM hooks too. Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Daniel Vetter <daniel@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Fabio Estevam <festevam@gmail.com> Cc: Liu Ying <victor.liu@nxp.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: NXP Linux Team <linux-imx@nxp.com> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> Cc: Sascha Hauer <s.hauer@pengutronix.de> Cc: Shawn Guo <shawnguo@kernel.org> Cc: Stefan Agner <stefan@agner.ch> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: dri-devel@lists.freedesktop.org Cc: linux-arm-kernel@lists.infradead.org --- drivers/gpu/drm/mxsfb/lcdif_drv.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)