| Message ID | 20200609103220.753969-1-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/omap: force runtime PM suspend on system suspend | expand |
* Tomi Valkeinen <tomi.valkeinen@ti.com> [200609 10:33]: > Use suspend_late and resume_early callbacks in DSS submodules to force > runtime PM suspend and resume. > > We use suspend_late callback so that omapdrm's system suspend callback > is called first, as that will disable all the display outputs after > which it's safe to force DSS into suspend. I think we can avoid the pm_runtime_force use if we have omapdrm implement both .suspend and .suspend_late. In that case suspend would only disable the display outputs, then suspend_late would take care of switching off the lights and release the last PM runtime count after the children are done suspending. Looks like we have already something similar done for i915_drv.c, so it should be doable. Maybe the disconnect can be done in .prepare and then .suspend_late is not even needed? And I think at that point the children can just use the standard UNIVERSAL_DEV_PM_OPS without pm_runtime_force usage hopefully :) Regards, Tony
On 09/06/2020 18:12, Tony Lindgren wrote: > * Tomi Valkeinen <tomi.valkeinen@ti.com> [200609 10:33]: >> Use suspend_late and resume_early callbacks in DSS submodules to force >> runtime PM suspend and resume. >> >> We use suspend_late callback so that omapdrm's system suspend callback >> is called first, as that will disable all the display outputs after >> which it's safe to force DSS into suspend. > > I think we can avoid the pm_runtime_force use if we have omapdrm > implement both .suspend and .suspend_late. In that case suspend would > only disable the display outputs, then suspend_late would take care > of switching off the lights and release the last PM runtime count > after the children are done suspending. I'm not sure how that can be done cleanly. omapdrm doesn't really see the DSS submodules. And even if it does, how can it "release the last PM runtime count"? With pm_runtime_force_suspend for each? That would be almost the same as this patch. Also, if omapdrm can do the above, it could do it in the .suspend, after disabling the display outputs. I don't think there's need for suspend_late then. Tomi
* Tomi Valkeinen <tomi.valkeinen@ti.com> [200609 15:38]: > On 09/06/2020 18:12, Tony Lindgren wrote: > > * Tomi Valkeinen <tomi.valkeinen@ti.com> [200609 10:33]: > > > Use suspend_late and resume_early callbacks in DSS submodules to force > > > runtime PM suspend and resume. > > > > > > We use suspend_late callback so that omapdrm's system suspend callback > > > is called first, as that will disable all the display outputs after > > > which it's safe to force DSS into suspend. > > > > I think we can avoid the pm_runtime_force use if we have omapdrm > > implement both .suspend and .suspend_late. In that case suspend would > > only disable the display outputs, then suspend_late would take care > > of switching off the lights and release the last PM runtime count > > after the children are done suspending. > > I'm not sure how that can be done cleanly. omapdrm doesn't really see the > DSS submodules. And even if it does, how can it "release the last PM runtime > count"? With pm_runtime_force_suspend for each? That would be almost the > same as this patch. Well there should not be anything special needed for the children, it should all happen automatically for us. I'm just wondering if this can be all done without the need for all the boilerplate code and adding suspend_late :) > Also, if omapdrm can do the above, it could do it in the .suspend, after > disabling the display outputs. I don't think there's need for suspend_late > then. Yeah so it seems. Can we just diconnect the display outputs in .prepare somewhere? Or is that the wrong place to do it? Regards, Tony
On 09/06/2020 19:10, Tony Lindgren wrote: > * Tomi Valkeinen <tomi.valkeinen@ti.com> [200609 15:38]: >> On 09/06/2020 18:12, Tony Lindgren wrote: >>> * Tomi Valkeinen <tomi.valkeinen@ti.com> [200609 10:33]: >>>> Use suspend_late and resume_early callbacks in DSS submodules to force >>>> runtime PM suspend and resume. >>>> >>>> We use suspend_late callback so that omapdrm's system suspend callback >>>> is called first, as that will disable all the display outputs after >>>> which it's safe to force DSS into suspend. >>> >>> I think we can avoid the pm_runtime_force use if we have omapdrm >>> implement both .suspend and .suspend_late. In that case suspend would >>> only disable the display outputs, then suspend_late would take care >>> of switching off the lights and release the last PM runtime count >>> after the children are done suspending. >> >> I'm not sure how that can be done cleanly. omapdrm doesn't really see the >> DSS submodules. And even if it does, how can it "release the last PM runtime >> count"? With pm_runtime_force_suspend for each? That would be almost the >> same as this patch. > > Well there should not be anything special needed for the children, > it should all happen automatically for us. > > I'm just wondering if this can be all done without the need for > all the boilerplate code and adding suspend_late :) > >> Also, if omapdrm can do the above, it could do it in the .suspend, after >> disabling the display outputs. I don't think there's need for suspend_late >> then. > > Yeah so it seems. Can we just diconnect the display outputs > in .prepare somewhere? Or is that the wrong place to do it? Hmm, yes, perhaps... If omapdrm uses .prepare to disable all the outputs. Then DSS submodules could use UNIVERSAL_DEV_PM_OPS() and have the same functions for system and runtime suspend. Although that has the problem that if the DSS is already runtime suspended when system suspend happens, the PM does not wake DSS up, and thus the suspend callbacks will crash if they access registers. So they need some extra logic there. I'll see tomorrow if I can come up with something like that. Tomi
* Tomi Valkeinen <tomi.valkeinen@ti.com> [200609 16:27]: > On 09/06/2020 19:10, Tony Lindgren wrote: > > Yeah so it seems. Can we just diconnect the display outputs > > in .prepare somewhere? Or is that the wrong place to do it? > > Hmm, yes, perhaps... If omapdrm uses .prepare to disable all the outputs. > Then DSS submodules could use UNIVERSAL_DEV_PM_OPS() and have the same > functions for system and runtime suspend. Yeah that would be nice. And maybe the need for force_suspend also disappears with the ordering issues gone :) > Although that has the problem that if the DSS is already runtime suspended > when system suspend happens, the PM does not wake DSS up, and thus the > suspend callbacks will crash if they access registers. So they need some > extra logic there. > > I'll see tomorrow if I can come up with something like that. OK. This $subject patch works for my test cases for suspend and resume based on a brief test with my fixes branch FYI. Regards, Tony
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c index dbb90f2d2ccd..1c9057d7db7b 100644 --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -4933,9 +4933,25 @@ static int dispc_runtime_resume(struct device *dev) return 0; } +static int dispc_suspend_late(struct device *dev) +{ + pm_runtime_force_suspend(dev); + + return 0; +} + +static int dispc_resume_early(struct device *dev) +{ + pm_runtime_force_resume(dev); + + return 0; +} + static const struct dev_pm_ops dispc_pm_ops = { .runtime_suspend = dispc_runtime_suspend, .runtime_resume = dispc_runtime_resume, + .suspend_late = dispc_suspend_late, + .resume_early = dispc_resume_early, }; struct platform_driver omap_dispchw_driver = { diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c index 79ddfbfd1b58..bae954dc8a5b 100644 --- a/drivers/gpu/drm/omapdrm/dss/dsi.c +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c @@ -5464,9 +5464,25 @@ static int dsi_runtime_resume(struct device *dev) return 0; } +static int dsi_suspend_late(struct device *dev) +{ + pm_runtime_force_suspend(dev); + + return 0; +} + +static int dsi_resume_early(struct device *dev) +{ + pm_runtime_force_resume(dev); + + return 0; +} + static const struct dev_pm_ops dsi_pm_ops = { .runtime_suspend = dsi_runtime_suspend, .runtime_resume = dsi_runtime_resume, + .suspend_late = dsi_suspend_late, + .resume_early = dsi_resume_early, }; struct platform_driver omap_dsihw_driver = { diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c b/drivers/gpu/drm/omapdrm/dss/dss.c index 4d5739fa4a5d..65be4918db0c 100644 --- a/drivers/gpu/drm/omapdrm/dss/dss.c +++ b/drivers/gpu/drm/omapdrm/dss/dss.c @@ -1611,9 +1611,25 @@ static int dss_runtime_resume(struct device *dev) return 0; } +static int dss_suspend_late(struct device *dev) +{ + pm_runtime_force_suspend(dev); + + return 0; +} + +static int dss_resume_early(struct device *dev) +{ + pm_runtime_force_resume(dev); + + return 0; +} + static const struct dev_pm_ops dss_pm_ops = { .runtime_suspend = dss_runtime_suspend, .runtime_resume = dss_runtime_resume, + .suspend_late = dss_suspend_late, + .resume_early = dss_resume_early, }; struct platform_driver omap_dsshw_driver = { diff --git a/drivers/gpu/drm/omapdrm/dss/venc.c b/drivers/gpu/drm/omapdrm/dss/venc.c index 766553bb2f87..3abc7e329121 100644 --- a/drivers/gpu/drm/omapdrm/dss/venc.c +++ b/drivers/gpu/drm/omapdrm/dss/venc.c @@ -942,9 +942,25 @@ static int venc_runtime_resume(struct device *dev) return 0; } +static int venc_suspend_late(struct device *dev) +{ + pm_runtime_force_suspend(dev); + + return 0; +} + +static int venc_resume_early(struct device *dev) +{ + pm_runtime_force_resume(dev); + + return 0; +} + static const struct dev_pm_ops venc_pm_ops = { .runtime_suspend = venc_runtime_suspend, .runtime_resume = venc_runtime_resume, + .suspend_late = venc_suspend_late, + .resume_early = venc_resume_early, }; static const struct of_device_id venc_of_match[] = {
Use suspend_late and resume_early callbacks in DSS submodules to force runtime PM suspend and resume. We use suspend_late callback so that omapdrm's system suspend callback is called first, as that will disable all the display outputs after which it's safe to force DSS into suspend. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- Not fully tested, as I haven't been able to get AM4's system suspend to work. Works with pm_test. drivers/gpu/drm/omapdrm/dss/dispc.c | 16 ++++++++++++++++ drivers/gpu/drm/omapdrm/dss/dsi.c | 16 ++++++++++++++++ drivers/gpu/drm/omapdrm/dss/dss.c | 16 ++++++++++++++++ drivers/gpu/drm/omapdrm/dss/venc.c | 16 ++++++++++++++++ 4 files changed, 64 insertions(+)