Messages in this thread | | | Date | Tue, 31 Mar 2020 19:35:21 +0530 | From | kalyan_t@codeauro ... | Subject | Re: [Freedreno] [PATCH] drm/msm/dpu: ensure device suspend happens during PM sleep |
| |
On 2020-03-31 00:25, Doug Anderson wrote: > Hi, > > On Mon, Mar 30, 2020 at 2:04 AM Kalyan Thota <kalyan_t@codeaurora.org> > wrote: >> >> "The PM core always increments the runtime usage counter >> before calling the ->suspend() callback and decrements it >> after calling the ->resume() callback" >> >> DPU and DSI are managed as runtime devices. When >> suspend is triggered, PM core adds a refcount on all the >> devices and calls device suspend, since usage count is >> already incremented, runtime suspend was not getting called >> and it kept the clocks on which resulted in target not >> entering into XO shutdown. >> >> Add changes to manage runtime devices during pm sleep. >> >> Changes in v1: >> - Remove unnecessary checks in the function >> _dpu_kms_disable_dpu (Rob Clark). >> >> Changes in v2: >> - Avoid using suspend_late to reset the usagecount >> as suspend_late might not be called during suspend >> call failures (Doug). >> >> Signed-off-by: Kalyan Thota <kalyan_t@codeaurora.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 33 >> +++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/msm/msm_drv.c | 4 ++++ >> drivers/gpu/drm/msm/msm_kms.h | 2 ++ >> 3 files changed, 39 insertions(+) > > I am still 100% baffled by your patch and I never did quite understand > your response to my previous comments [1]. I think you're saying that > the problem you were facing is that if you call "suspend" but never > called "runtime_suspend" that the device stays active. Is that right? > If that's true, did you try something like this suggestion I made? > > SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > pm_runtime_force_resume) > > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> index ce19f1d..2343cbd 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> @@ -26,6 +26,7 @@ >> #include "dpu_encoder.h" >> #include "dpu_plane.h" >> #include "dpu_crtc.h" >> +#include "dsi.h" >> >> #define CREATE_TRACE_POINTS >> #include "dpu_trace.h" >> @@ -325,6 +326,37 @@ static void dpu_kms_disable_commit(struct msm_kms >> *kms) >> pm_runtime_put_sync(&dpu_kms->pdev->dev); >> } >> >> +static void _dpu_kms_disable_dpu(struct msm_kms *kms) >> +{ >> + struct dpu_kms *dpu_kms = to_dpu_kms(kms); >> + struct drm_device *dev = dpu_kms->dev; >> + struct msm_drm_private *priv = dev->dev_private; >> + struct msm_dsi *dsi; >> + int i; >> + >> + dpu_kms_disable_commit(kms); >> + >> + for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) { >> + if (!priv->dsi[i]) >> + continue; >> + dsi = priv->dsi[i]; >> + pm_runtime_put_sync(&dsi->pdev->dev); >> + } >> + pm_runtime_put_sync(dev->dev); >> + >> + /* Increment the usagecount without triggering a resume */ >> + pm_runtime_get_noresume(dev->dev); >> + >> + pm_runtime_get_noresume(&dpu_kms->pdev->dev); >> + >> + for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) { >> + if (!priv->dsi[i]) >> + continue; >> + dsi = priv->dsi[i]; >> + pm_runtime_get_noresume(&dsi->pdev->dev); >> + } >> +} > > My pm_runtime knowledge is pretty weak sometimes, but the above > function looks crazy. Maybe it's just me not understanding, but can > you please summarize what you're trying to accomplish? > -- I was trying to get the runtime callbacks via controlling the device usage_count Since the usage_count was already incremented by PM core, i was decrementing and incrementing (without resume) so that callbacks are triggered.
I have taken your suggestion on forcing the suspend instead of managing it via usage_count. i'll follow it up in the next patchset.
> -Doug > > [1] > https://lore.kernel.org/r/114130f68c494f83303c51157e2c5bfa@codeaurora.org > _______________________________________________ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno
| |