Message ID | 20240630153652.318882-8-wahrenst@gmx.net |
---|---|
State | New |
Headers | show |
Series | ARM: bcm2835: Implement initial S2Idle for Raspberry Pi | expand |
Hi, On Sun, Jun 30, 2024 at 05:36:48PM GMT, Stefan Wahren wrote: > Suspend of VC4 HDMI will likely triggers a warning from > vc4_hdmi_connector_detect_ctx() during poll of connector status. > The power management will prevent the resume and keep the relevant > power domain disabled. > > Since there is no reason to poll the connector status during > suspend, the polling should be disabled during this. > > It not possible to use drm_mode_config_helper_suspend() here, > because the callbacks might be called during bind phase and not all > components are fully initialized. > > Link: https://lore.kernel.org/dri-devel/7003512d-7303-4f41-b0d6-a8af5bf8e497@gmx.net/ > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index b3a42b709718..e80495cea6ac 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -3106,6 +3106,13 @@ static int vc5_hdmi_init_resources(struct drm_device *drm, > static int vc4_hdmi_runtime_suspend(struct device *dev) > { > struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); > + struct drm_device *drm = vc4_hdmi->connector.dev; > + > + /* > + * Don't disable polling if it was never initialized > + */ > + if (drm && drm->mode_config.poll_enabled) > + drm_kms_helper_poll_disable(drm); Does it make sense to add it to runtime_suspend? What if the board boots without a display connected, and only after a while one is connected? Wouldn't that prevent the driver from detecting it? Maxime
Hi Stefan, > Suspend of VC4 HDMI will likely triggers a warning from > vc4_hdmi_connector_detect_ctx() during poll of connector status. > The power management will prevent the resume and keep the relevant > power domain disabled. > > Since there is no reason to poll the connector status during > suspend, the polling should be disabled during this. What about HDMI-CEC? I don't know well enough how CEC integrates at this level but CEC can wake up the device over HDMI from a TV display for example so if this affects that, while it's maybe not required for first pass, I know the rpi is used in a lot of media use cases so the ability to wake up via CEC would certainly be welcomed. > It not possible to use drm_mode_config_helper_suspend() here, > because the callbacks might be called during bind phase and not all > components are fully initialized. > > Link: https://lore.kernel.org/dri-devel/7003512d-7303-4f41-b0d6-a8af5bf8e497@gmx.net/ > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index b3a42b709718..e80495cea6ac 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -3106,6 +3106,13 @@ static int vc5_hdmi_init_resources(struct drm_device *drm, > static int vc4_hdmi_runtime_suspend(struct device *dev) > { > struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); > + struct drm_device *drm = vc4_hdmi->connector.dev; > + > + /* > + * Don't disable polling if it was never initialized > + */ > + if (drm && drm->mode_config.poll_enabled) > + drm_kms_helper_poll_disable(drm); > > clk_disable_unprepare(vc4_hdmi->hsm_clock); > > @@ -3115,6 +3122,7 @@ static int vc4_hdmi_runtime_suspend(struct device *dev) > static int vc4_hdmi_runtime_resume(struct device *dev) > { > struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); > + struct drm_device *drm = vc4_hdmi->connector.dev; > unsigned long __maybe_unused flags; > u32 __maybe_unused value; > unsigned long rate; > @@ -3159,6 +3167,9 @@ static int vc4_hdmi_runtime_resume(struct device *dev) > } > #endif > > + if (drm && drm->mode_config.poll_enabled) > + drm_kms_helper_poll_enable(drm); > + > return 0; > > err_disable_clk: > -- > 2.34.1 >
Hi Maxime, Am 02.07.24 um 15:48 schrieb Maxime Ripard: > Hi, > > On Sun, Jun 30, 2024 at 05:36:48PM GMT, Stefan Wahren wrote: >> Suspend of VC4 HDMI will likely triggers a warning from >> vc4_hdmi_connector_detect_ctx() during poll of connector status. >> The power management will prevent the resume and keep the relevant >> power domain disabled. >> >> Since there is no reason to poll the connector status during >> suspend, the polling should be disabled during this. >> >> It not possible to use drm_mode_config_helper_suspend() here, >> because the callbacks might be called during bind phase and not all >> components are fully initialized. >> >> Link: https://lore.kernel.org/dri-devel/7003512d-7303-4f41-b0d6-a8af5bf8e497@gmx.net/ >> Signed-off-by: Stefan Wahren <wahrenst@gmx.net> >> --- >> drivers/gpu/drm/vc4/vc4_hdmi.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c >> index b3a42b709718..e80495cea6ac 100644 >> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c >> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c >> @@ -3106,6 +3106,13 @@ static int vc5_hdmi_init_resources(struct drm_device *drm, >> static int vc4_hdmi_runtime_suspend(struct device *dev) >> { >> struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); >> + struct drm_device *drm = vc4_hdmi->connector.dev; >> + >> + /* >> + * Don't disable polling if it was never initialized >> + */ >> + if (drm && drm->mode_config.poll_enabled) >> + drm_kms_helper_poll_disable(drm); > Does it make sense to add it to runtime_suspend? i saw that other drm drivers used drm_mode_config_helper_suspend() in the RUNTIME_PM_OPS. But i agree, it should be better moved to SYSTEM_SLEEP_PM_OPS. > What if the board boots without a display connected, and only after a > while one is connected? Wouldn't that prevent the driver from detecting > it? tbh I noticed that HDMI re-detection didn't worked in my setup 6.10-rcX before this series. I need to investigate ... > > Maxime
Hi Peter, Am 02.07.24 um 22:02 schrieb Peter Robinson: > Hi Stefan, > >> Suspend of VC4 HDMI will likely triggers a warning from >> vc4_hdmi_connector_detect_ctx() during poll of connector status. >> The power management will prevent the resume and keep the relevant >> power domain disabled. >> >> Since there is no reason to poll the connector status during >> suspend, the polling should be disabled during this. > What about HDMI-CEC? I don't know well enough how CEC integrates at > this level but CEC can wake up the device over HDMI from a TV display > for example so if this affects that, while it's maybe not required for > first pass, I know the rpi is used in a lot of media use cases so the > ability to wake up via CEC would certainly be welcomed. unfortunately i don't know much about HDMI-CEC, too. The only thing I noticed was that I currently cannot configure vc4 as a wakeup source.
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index b3a42b709718..e80495cea6ac 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -3106,6 +3106,13 @@ static int vc5_hdmi_init_resources(struct drm_device *drm, static int vc4_hdmi_runtime_suspend(struct device *dev) { struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); + struct drm_device *drm = vc4_hdmi->connector.dev; + + /* + * Don't disable polling if it was never initialized + */ + if (drm && drm->mode_config.poll_enabled) + drm_kms_helper_poll_disable(drm); clk_disable_unprepare(vc4_hdmi->hsm_clock); @@ -3115,6 +3122,7 @@ static int vc4_hdmi_runtime_suspend(struct device *dev) static int vc4_hdmi_runtime_resume(struct device *dev) { struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); + struct drm_device *drm = vc4_hdmi->connector.dev; unsigned long __maybe_unused flags; u32 __maybe_unused value; unsigned long rate; @@ -3159,6 +3167,9 @@ static int vc4_hdmi_runtime_resume(struct device *dev) } #endif + if (drm && drm->mode_config.poll_enabled) + drm_kms_helper_poll_enable(drm); + return 0; err_disable_clk:
Suspend of VC4 HDMI will likely triggers a warning from vc4_hdmi_connector_detect_ctx() during poll of connector status. The power management will prevent the resume and keep the relevant power domain disabled. Since there is no reason to poll the connector status during suspend, the polling should be disabled during this. It not possible to use drm_mode_config_helper_suspend() here, because the callbacks might be called during bind phase and not all components are fully initialized. Link: https://lore.kernel.org/dri-devel/7003512d-7303-4f41-b0d6-a8af5bf8e497@gmx.net/ Signed-off-by: Stefan Wahren <wahrenst@gmx.net> --- drivers/gpu/drm/vc4/vc4_hdmi.c | 11 +++++++++++ 1 file changed, 11 insertions(+) -- 2.34.1