| Message ID | 1406193474-13695-1-git-send-email-shawn.guo@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shawn, Am Donnerstag, den 24.07.2014, 17:17 +0800 schrieb Shawn Guo: > HDMI currently stops working after a system suspend/resume cycle. It > turns out that the cause is the imx-hdmi encoder .dpms hook doesn't get > called from anywhere across suspend/resume cycle. > > The patch follows what exynos drm driver does to walk the list of > connectors and call their .dpms function from suspend/resume hook. And > the connectors' .dpms function will in turn filter down to the .dpms > hooks of encoders and CRTCs. > > With this change, HDMI can continue working after a suspend/resume > cycle. > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com> > --- > Tested with HDMI and LVDS. It'd be great if someone can help test TVE > to ensure the patch doesn't break anything. Tested-by: Philipp Zabel <p.zabel@pengutronix.de> on i.MX53-QSB with VGA via TVE. regards Philipp
Am Donnerstag, den 24.07.2014, 17:17 +0800 schrieb Shawn Guo: > HDMI currently stops working after a system suspend/resume cycle. It > turns out that the cause is the imx-hdmi encoder .dpms hook doesn't get > called from anywhere across suspend/resume cycle. > > The patch follows what exynos drm driver does to walk the list of > connectors and call their .dpms function from suspend/resume hook. And > the connectors' .dpms function will in turn filter down to the .dpms > hooks of encoders and CRTCs. > > With this change, HDMI can continue working after a suspend/resume > cycle. > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com> > --- > Tested with HDMI and LVDS. It'd be great if someone can help test TVE > to ensure the patch doesn't break anything. > > drivers/staging/imx-drm/imx-drm-core.c | 39 ++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c > index def8280d7ee6..b0ea1f0ed32f 100644 > --- a/drivers/staging/imx-drm/imx-drm-core.c > +++ b/drivers/staging/imx-drm/imx-drm-core.c > @@ -696,6 +696,44 @@ static int imx_drm_platform_remove(struct platform_device *pdev) > return 0; > } > > +#if CONFIG_PM_SLEEP use #ifdef > +static int imx_drm_suspend(struct device *dev) > +{ > + struct drm_device *drm_dev = dev_get_drvdata(dev); > + struct drm_connector *connector; > + > + drm_kms_helper_poll_disable(drm_dev); > + That's ok. > + drm_modeset_lock_all(drm_dev); > + list_for_each_entry(connector, &drm_dev->mode_config.connector_list, head) { > + if (connector->funcs->dpms) > + connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF); > + } Don't touch DPMS state here. See below. > + drm_modeset_unlock_all(drm_dev); > + > + return 0; > +} > + > +static int imx_drm_resume(struct device *dev) > +{ > + struct drm_device *drm_dev = dev_get_drvdata(dev); > + struct drm_connector *connector; > + > + drm_modeset_lock_all(drm_dev); > + list_for_each_entry(connector, &drm_dev->mode_config.connector_list, head) { > + if (connector->funcs->dpms) > + connector->funcs->dpms(connector, DRM_MODE_DPMS_ON); > + } > + drm_modeset_unlock_all(drm_dev); > + This forcefully enables all connectors, which might not have been the state before suspend. Have a look at simply using drm_helper_resume_force_mode(), which should do the right thing. > + drm_kms_helper_poll_enable(drm_dev); > + > + return 0; > +} > +#endif > + > +static SIMPLE_DEV_PM_OPS(imx_drm_pm_ops, imx_drm_suspend, imx_drm_resume); > + > static const struct of_device_id imx_drm_dt_ids[] = { > { .compatible = "fsl,imx-display-subsystem", }, > { /* sentinel */ }, > @@ -708,6 +746,7 @@ static struct platform_driver imx_drm_pdrv = { > .driver = { > .owner = THIS_MODULE, > .name = "imx-drm", > + .pm = &imx_drm_pm_ops, > .of_match_table = imx_drm_dt_ids, > }, > }; Regards, Lucas
On Thu, Jul 24, 2014 at 11:47:55AM +0200, Lucas Stach wrote: > Am Donnerstag, den 24.07.2014, 17:17 +0800 schrieb Shawn Guo: > > HDMI currently stops working after a system suspend/resume cycle. It > > turns out that the cause is the imx-hdmi encoder .dpms hook doesn't get > > called from anywhere across suspend/resume cycle. > > > > The patch follows what exynos drm driver does to walk the list of > > connectors and call their .dpms function from suspend/resume hook. And > > the connectors' .dpms function will in turn filter down to the .dpms > > hooks of encoders and CRTCs. > > > > With this change, HDMI can continue working after a suspend/resume > > cycle. > > > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com> > > --- > > Tested with HDMI and LVDS. It'd be great if someone can help test TVE > > to ensure the patch doesn't break anything. > > > > drivers/staging/imx-drm/imx-drm-core.c | 39 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > > > diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c > > index def8280d7ee6..b0ea1f0ed32f 100644 > > --- a/drivers/staging/imx-drm/imx-drm-core.c > > +++ b/drivers/staging/imx-drm/imx-drm-core.c > > @@ -696,6 +696,44 @@ static int imx_drm_platform_remove(struct platform_device *pdev) > > return 0; > > } > > > > +#if CONFIG_PM_SLEEP > > use #ifdef > > > +static int imx_drm_suspend(struct device *dev) > > +{ > > + struct drm_device *drm_dev = dev_get_drvdata(dev); > > + struct drm_connector *connector; > > + > > + drm_kms_helper_poll_disable(drm_dev); > > + > > That's ok. > > > + drm_modeset_lock_all(drm_dev); > > + list_for_each_entry(connector, &drm_dev->mode_config.connector_list, head) { > > + if (connector->funcs->dpms) > > + connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF); > > + } > > Don't touch DPMS state here. See below. In which case, how else does the hardware get placed into a low power mode on suspend? DRM has nothing provided, and this is left up to each DRM driver to implement (probably because it tends to be very driver specific.) i915 for example calls the CRTC DPMS function with DRM_MODE_DPMS_OFF. This provides a similar mechanism, but also informs the connector, any bridge, and encoder associated with the connector as well as the CRTC to place themselves into a low power mode (which is what DRM_MODE_DPMS_OFF should be doing anyway.) > > +static int imx_drm_resume(struct device *dev) > > +{ > > + struct drm_device *drm_dev = dev_get_drvdata(dev); > > + struct drm_connector *connector; > > + > > + drm_modeset_lock_all(drm_dev); > > + list_for_each_entry(connector, &drm_dev->mode_config.connector_list, head) { > > + if (connector->funcs->dpms) > > + connector->funcs->dpms(connector, DRM_MODE_DPMS_ON); > > + } > > + drm_modeset_unlock_all(drm_dev); > > + > This forcefully enables all connectors, which might not have been the > state before suspend. Have a look at simply using > drm_helper_resume_force_mode(), which should do the right thing. Yes, we agree there.
On 07/24/2014 11:47 AM, Lucas Stach wrote: > Am Donnerstag, den 24.07.2014, 17:17 +0800 schrieb Shawn Guo: >> HDMI currently stops working after a system suspend/resume cycle. It >> turns out that the cause is the imx-hdmi encoder .dpms hook doesn't get >> called from anywhere across suspend/resume cycle. >> >> The patch follows what exynos drm driver does to walk the list of >> connectors and call their .dpms function from suspend/resume hook. And >> the connectors' .dpms function will in turn filter down to the .dpms >> hooks of encoders and CRTCs. >> >> With this change, HDMI can continue working after a suspend/resume >> cycle. >> >> Signed-off-by: Shawn Guo <shawn.guo@freescale.com> >> --- >> Tested with HDMI and LVDS. It'd be great if someone can help test TVE >> to ensure the patch doesn't break anything. >> >> drivers/staging/imx-drm/imx-drm-core.c | 39 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c >> index def8280d7ee6..b0ea1f0ed32f 100644 >> --- a/drivers/staging/imx-drm/imx-drm-core.c >> +++ b/drivers/staging/imx-drm/imx-drm-core.c >> @@ -696,6 +696,44 @@ static int imx_drm_platform_remove(struct platform_device *pdev) >> return 0; >> } >> >> +#if CONFIG_PM_SLEEP > > use #ifdef ...or remove #if/#ifdef and mark as __maybe_unused Marc
On Thu, Jul 24, 2014 at 10:54:30AM +0100, Russell King - ARM Linux wrote: > On Thu, Jul 24, 2014 at 11:47:55AM +0200, Lucas Stach wrote: > > Am Donnerstag, den 24.07.2014, 17:17 +0800 schrieb Shawn Guo: > > > HDMI currently stops working after a system suspend/resume cycle. It > > > turns out that the cause is the imx-hdmi encoder .dpms hook doesn't get > > > called from anywhere across suspend/resume cycle. > > > > > > The patch follows what exynos drm driver does to walk the list of > > > connectors and call their .dpms function from suspend/resume hook. And > > > the connectors' .dpms function will in turn filter down to the .dpms > > > hooks of encoders and CRTCs. > > > > > > With this change, HDMI can continue working after a suspend/resume > > > cycle. > > > > > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com> > > > --- > > > Tested with HDMI and LVDS. It'd be great if someone can help test TVE > > > to ensure the patch doesn't break anything. > > > > > > drivers/staging/imx-drm/imx-drm-core.c | 39 ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 39 insertions(+) > > > > > > diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c > > > index def8280d7ee6..b0ea1f0ed32f 100644 > > > --- a/drivers/staging/imx-drm/imx-drm-core.c > > > +++ b/drivers/staging/imx-drm/imx-drm-core.c > > > @@ -696,6 +696,44 @@ static int imx_drm_platform_remove(struct platform_device *pdev) > > > return 0; > > > } > > > > > > +#if CONFIG_PM_SLEEP > > > > use #ifdef > > > > > +static int imx_drm_suspend(struct device *dev) > > > +{ > > > + struct drm_device *drm_dev = dev_get_drvdata(dev); > > > + struct drm_connector *connector; > > > + > > > + drm_kms_helper_poll_disable(drm_dev); > > > + > > > > That's ok. > > > > > + drm_modeset_lock_all(drm_dev); > > > + list_for_each_entry(connector, &drm_dev->mode_config.connector_list, head) { > > > + if (connector->funcs->dpms) > > > + connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF); > > > + } > > > > Don't touch DPMS state here. See below. > > In which case, how else does the hardware get placed into a low power > mode on suspend? > > DRM has nothing provided, and this is left up to each DRM driver to > implement (probably because it tends to be very driver specific.) > i915 for example calls the CRTC DPMS function with DRM_MODE_DPMS_OFF. > > This provides a similar mechanism, but also informs the connector, any > bridge, and encoder associated with the connector as well as the CRTC > to place themselves into a low power mode (which is what > DRM_MODE_DPMS_OFF should be doing anyway.) Well you need to call internal functions to make sure you can restore the state again. Not sure any more how that all works with the crtc helpers and whether they restore dpms state properly at all. i915 uses something completely different nowadays. -Daniel
On Thu, Jul 24, 2014 at 12:38:59PM +0200, Daniel Vetter wrote: > > > > +static int imx_drm_suspend(struct device *dev) > > > > +{ > > > > + struct drm_device *drm_dev = dev_get_drvdata(dev); > > > > + struct drm_connector *connector; > > > > + > > > > + drm_kms_helper_poll_disable(drm_dev); > > > > + > > > > > > That's ok. > > > > > > > + drm_modeset_lock_all(drm_dev); > > > > + list_for_each_entry(connector, &drm_dev->mode_config.connector_list, head) { > > > > + if (connector->funcs->dpms) > > > > + connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF); > > > > + } > > > > > > Don't touch DPMS state here. See below. > > > > In which case, how else does the hardware get placed into a low power > > mode on suspend? > > > > DRM has nothing provided, and this is left up to each DRM driver to > > implement (probably because it tends to be very driver specific.) > > i915 for example calls the CRTC DPMS function with DRM_MODE_DPMS_OFF. > > > > This provides a similar mechanism, but also informs the connector, any > > bridge, and encoder associated with the connector as well as the CRTC > > to place themselves into a low power mode (which is what > > DRM_MODE_DPMS_OFF should be doing anyway.) > > Well you need to call internal functions to make sure you can restore the > state again. Not sure any more how that all works with the crtc helpers > and whether they restore dpms state properly at all. i915 uses something > completely different nowadays. Is it okay to do what exynos driver does, i.e. saving/restoring the dpms state before/after calling connector .dpms function? list_for_each_entry(connector, &drm_dev->mode_config.connector_list, head) { int old_dpms = connector->dpms; if (connector->funcs->dpms) connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF); /* Set the old mode back to the connector for resume */ connector->dpms = old_dpms; } Shawn
On Thu, Jul 24, 2014 at 11:47:55AM +0200, Lucas Stach wrote: > > +static int imx_drm_resume(struct device *dev) > > +{ > > + struct drm_device *drm_dev = dev_get_drvdata(dev); > > + struct drm_connector *connector; > > + > > + drm_modeset_lock_all(drm_dev); > > + list_for_each_entry(connector, &drm_dev->mode_config.connector_list, head) { > > + if (connector->funcs->dpms) > > + connector->funcs->dpms(connector, DRM_MODE_DPMS_ON); > > + } > > + drm_modeset_unlock_all(drm_dev); > > + > This forcefully enables all connectors, which might not have been the > state before suspend. Have a look at simply using > drm_helper_resume_force_mode(), which should do the right thing. With a bit more testing, I found that simply calling drm_helper_resume_force_mode() from .resume hook can get HDMI back to work. So it seems the issue is not caused by that hdmi .dpms function isn't being called but some mode setting states gets lost and isn't being restored. It can fix the HDMI issue we're facing right now, and I will send V2 with this change. But it doesn't address Russell's concern, the HDMI hardware isn't actually put into low power mode since hdmi .dpms function isn't called anywhere during suspend/resume cycle. Shawn
On Thu, Jul 24, 2014 at 11:38:52AM +0200, Philipp Zabel wrote: > Hi Shawn, > > Am Donnerstag, den 24.07.2014, 17:17 +0800 schrieb Shawn Guo: > > HDMI currently stops working after a system suspend/resume cycle. It > > turns out that the cause is the imx-hdmi encoder .dpms hook doesn't get > > called from anywhere across suspend/resume cycle. > > > > The patch follows what exynos drm driver does to walk the list of > > connectors and call their .dpms function from suspend/resume hook. And > > the connectors' .dpms function will in turn filter down to the .dpms > > hooks of encoders and CRTCs. > > > > With this change, HDMI can continue working after a suspend/resume > > cycle. > > > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com> > > --- > > Tested with HDMI and LVDS. It'd be great if someone can help test TVE > > to ensure the patch doesn't break anything. > > Tested-by: Philipp Zabel <p.zabel@pengutronix.de> > > on i.MX53-QSB with VGA via TVE. Thanks much, Philipp. Shawn
On Thu, Jul 24, 2014 at 11:56:32AM +0200, Marc Kleine-Budde wrote: > >> @@ -696,6 +696,44 @@ static int imx_drm_platform_remove(struct platform_device *pdev) > >> return 0; > >> } > >> > >> +#if CONFIG_PM_SLEEP > > > > use #ifdef > > ...or remove #if/#ifdef and mark as __maybe_unused I personally do not like the idea of __maybe_unused. Will there be a compile error if I call some helper functions which are only available with CONFIG_PM_SLEEP? Shawn
On Thu, Jul 24, 2014 at 10:59:36PM +0800, Shawn Guo wrote: > On Thu, Jul 24, 2014 at 12:38:59PM +0200, Daniel Vetter wrote: > > > > > +static int imx_drm_suspend(struct device *dev) > > > > > +{ > > > > > + struct drm_device *drm_dev = dev_get_drvdata(dev); > > > > > + struct drm_connector *connector; > > > > > + > > > > > + drm_kms_helper_poll_disable(drm_dev); > > > > > + > > > > > > > > That's ok. > > > > > > > > > + drm_modeset_lock_all(drm_dev); > > > > > + list_for_each_entry(connector, &drm_dev->mode_config.connector_list, head) { > > > > > + if (connector->funcs->dpms) > > > > > + connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF); > > > > > + } > > > > > > > > Don't touch DPMS state here. See below. > > > > > > In which case, how else does the hardware get placed into a low power > > > mode on suspend? > > > > > > DRM has nothing provided, and this is left up to each DRM driver to > > > implement (probably because it tends to be very driver specific.) > > > i915 for example calls the CRTC DPMS function with DRM_MODE_DPMS_OFF. > > > > > > This provides a similar mechanism, but also informs the connector, any > > > bridge, and encoder associated with the connector as well as the CRTC > > > to place themselves into a low power mode (which is what > > > DRM_MODE_DPMS_OFF should be doing anyway.) > > > > Well you need to call internal functions to make sure you can restore the > > state again. Not sure any more how that all works with the crtc helpers > > and whether they restore dpms state properly at all. i915 uses something > > completely different nowadays. > > Is it okay to do what exynos driver does, i.e. saving/restoring the dpms > state before/after calling connector .dpms function? > > list_for_each_entry(connector, &drm_dev->mode_config.connector_list, head) { > int old_dpms = connector->dpms; > > if (connector->funcs->dpms) > connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF); > > /* Set the old mode back to the connector for resume */ > connector->dpms = old_dpms; > } I'm not sure whether that will get you correct behavior since iirc the crtc helpers aren't terribly good at restoring the right dpms state. drm_helper_resume_force_mode simple does a modeset, and that has an implicit dpms on when enabling the crtc. Someone might want to tackle this, but thus far it doesn't seem to have been too annoying. I think the best way would be to do this as part of atomic modeset, where we can supply the desired dpms state directly. -Daniel
diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c index def8280d7ee6..b0ea1f0ed32f 100644 --- a/drivers/staging/imx-drm/imx-drm-core.c +++ b/drivers/staging/imx-drm/imx-drm-core.c @@ -696,6 +696,44 @@ static int imx_drm_platform_remove(struct platform_device *pdev) return 0; } +#if CONFIG_PM_SLEEP +static int imx_drm_suspend(struct device *dev) +{ + struct drm_device *drm_dev = dev_get_drvdata(dev); + struct drm_connector *connector; + + drm_kms_helper_poll_disable(drm_dev); + + drm_modeset_lock_all(drm_dev); + list_for_each_entry(connector, &drm_dev->mode_config.connector_list, head) { + if (connector->funcs->dpms) + connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF); + } + drm_modeset_unlock_all(drm_dev); + + return 0; +} + +static int imx_drm_resume(struct device *dev) +{ + struct drm_device *drm_dev = dev_get_drvdata(dev); + struct drm_connector *connector; + + drm_modeset_lock_all(drm_dev); + list_for_each_entry(connector, &drm_dev->mode_config.connector_list, head) { + if (connector->funcs->dpms) + connector->funcs->dpms(connector, DRM_MODE_DPMS_ON); + } + drm_modeset_unlock_all(drm_dev); + + drm_kms_helper_poll_enable(drm_dev); + + return 0; +} +#endif + +static SIMPLE_DEV_PM_OPS(imx_drm_pm_ops, imx_drm_suspend, imx_drm_resume); + static const struct of_device_id imx_drm_dt_ids[] = { { .compatible = "fsl,imx-display-subsystem", }, { /* sentinel */ }, @@ -708,6 +746,7 @@ static struct platform_driver imx_drm_pdrv = { .driver = { .owner = THIS_MODULE, .name = "imx-drm", + .pm = &imx_drm_pm_ops, .of_match_table = imx_drm_dt_ids, }, };
HDMI currently stops working after a system suspend/resume cycle. It turns out that the cause is the imx-hdmi encoder .dpms hook doesn't get called from anywhere across suspend/resume cycle. The patch follows what exynos drm driver does to walk the list of connectors and call their .dpms function from suspend/resume hook. And the connectors' .dpms function will in turn filter down to the .dpms hooks of encoders and CRTCs. With this change, HDMI can continue working after a suspend/resume cycle. Signed-off-by: Shawn Guo <shawn.guo@freescale.com> --- Tested with HDMI and LVDS. It'd be great if someone can help test TVE to ensure the patch doesn't break anything. drivers/staging/imx-drm/imx-drm-core.c | 39 ++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)