Hi Tony,
On 26/04/2021 17:12, Tony Lindgren wrote:
On resume, dispc pm_runtime_force_resume() is not enabling the hardware
as we pass the pm_runtime_need_not_resume() test as the device is suspended
with no child devices.
As the resume continues, omap_atomic_comit_tail() calls dispc_runtime_get()
that calls rpm_resume() enabling the hardware, and increasing child_count
for it's parent device.
But at this point device_complete() has not yet been called for dispc. So
when omap_atomic_comit_tail() calls dispc_runtime_get(), it won't idle
Is that supposed to be dispc_runtime_put()?
the hardware, and the clocks are left on after resume.
This can be easily seen for example after suspending Beagleboard-X15 with
no displays connected, and by reading the CM_DSS_DSS_CLKCTRL register at
0x4a009120 after resume. After a suspend and resume cycle, it shows a
value of 0x00040102 instead of 0x00070000 like it should.
Let's fix the issue by calling dispc_runtime_suspend() and
dispc_runtime_resume() directly from dispc_suspend() and dispc_resume().
This leaves out the PM runtime related issues for system suspend.
See also earlier commit 88d26136a256 ("PM: Prevent runtime suspend during
system resume") and commit ca8199f13498 ("drm/msm/dpu: ensure device
suspend happens during PM sleep") for more information.
Fixes: ecfdedd7da5d ("drm/omap: force runtime PM suspend on system suspend")
Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
Why is this only needed for dispc, and not the other dss submodules
which were handled in ecfdedd7da5d?
I have to say I'm pretty confused (maybe partly because it's been a
while since I debugged this =). Aren't the
pm_runtime_force_suspend/resume made explicitly for this use case? At
least that is how I read the documentation.
If I understand right, this is only an issue when the dss was not
enabled before the system suspend? And as the dispc is not enabled at
suspend, pm_runtime_force_suspend and pm_runtime_force_resume don't
really do anything. At resume, the DRM resume functionality causes
omapdrm to call pm_runtime_get and put, and this somehow causes the dss
to stay enabled.
I think I'm missing something here, but this patch feels like a hack
fix. But continuing with the hack mindset, as the PM apparently needs
DSS to be enabled at suspend for it to work correctly, lets give that to
the PM. This seems to work also:
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
b/drivers/gpu/drm/omapdrm/omap_drv.c
index 28bbad1353ee..0fd9d80d3e12 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -695,6 +695,8 @@ static int omap_drm_suspend(struct device *dev)
struct omap_drm_private *priv = dev_get_drvdata(dev);
struct drm_device *drm_dev = priv->ddev;
+ dispc_runtime_get(priv->dispc);
+
return drm_mode_config_helper_suspend(drm_dev);
}
@@ -705,6 +707,8 @@ static int omap_drm_resume(struct device *dev)
drm_mode_config_helper_resume(drm_dev);
+ dispc_runtime_put(priv->dispc);
+
return omap_gem_resume(drm_dev);
}
#endif
But I don't think that helps with the other dss submodules either.
Tomi
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
