Re: [PATCH] drm/msm/dpu: ensure device suspend happens during PM sleep

From: Doug Anderson <dianders@chromium.org>
To: Rob Clark <robdclark@gmail.com>
Cc: Kalyan Thota <kalyan_t@codeaurora.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	freedreno <freedreno@lists.freedesktop.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	Sean Paul <seanpaul@chromium.org>,
	"Kristian H. Kristensen" <hoegsberg@chromium.org>,
	Jeykumar Sankaran <jsanka@codeaurora.org>,
	mkrishn@codeaurora.org, travitej@codeaurora.org,
	nganji@codeaurora.org
Subject: Re: [PATCH] drm/msm/dpu: ensure device suspend happens during PM sleep
Date: Wed, 25 Mar 2020 08:50:05 -0700	[thread overview]
Message-ID: <CAD=FV=VxeCUEEFi9T0Jand3EWkaQTLnQkT3v5yjyjLi4yDeQ-w@mail.gmail.com> (raw)
In-Reply-To: <CAF6AEGs5saoU3FeO++S+YD=Js499HB2CjK8neYCXAZmCjgy2nQ@mail.gmail.com>

Hi,

On Wed, Mar 25, 2020 at 8:40 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Mar 24, 2020 at 7:35 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Sun, Mar 22, 2020 at 11:14 PM 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).
> >
> > I'm wondering what happened with my feedback on v1, AKA:
> >
> > https://lore.kernel.org/r/CAD=FV=VxzEV40g+ieuEN+7o=34+wM8MHO8o7T5zA1Yosx7SVWg@mail.gmail.com
> >
> > Maybe you didn't see it?  ...or if you or Rob think I'm way off base
> > (always possible) then please tell me so.
> >
>
> At least w/ the current patch, disable_dpu should not be called for
> screen-off (although I'd hope if all the screens are off the device
> would suspend).

OK, that's good.

> But I won't claim to be a pm expert.. so not really
> sure if this is the best approach or not.  I don't think our
> arrangement of sub-devices under a parent is completely abnormal, so
> it does feel like there should be a simpler solution..

I think the other arguments about asymmetry are still valid and I've
fixed bugs around this type of thing in the past.  For instance, see
commit f7ccbed656f7 ("drm/rockchip: Suspend DP late").

-Doug