Re: [PATCH v3 5/5] drm/arm/malidp: Added the late system pm functions

[Rafael J. Wysocki]

On 5/14/2018 12:01 PM, Ayan Halder wrote:
> On Wed, Apr 25, 2018 at 01:49:35PM +0200, Daniel Vetter wrote:
> Hi Daniel,
> > On Wed, Apr 25, 2018 at 1:26 PM, Liviu Dudau <liviu.dudau@xxxxxxx> wrote:
> > > On Wed, Apr 25, 2018 at 09:17:22AM +0200, Daniel Vetter wrote:
> > > > On Tue, Apr 24, 2018 at 07:12:47PM +0100, Ayan Kumar Halder wrote:
> > > > > malidp_pm_suspend_late checks if the runtime status is not suspended
> > > > > and if so, invokes malidp_runtime_pm_suspend which disables the
> > > > > display engine/core interrupts and the clocks. It sets the runtime status
> > > > > as suspended.
> > > > >
> > > > > The difference between suspend() and suspend_late() is as follows:-
> > > > > 1. suspend() makes the device quiescent. In our case, we invoke the DRM
> > > > > helper which disables the CRTC. This would have invoked runtime pm
> > > > > suspend but the system suspend process disables runtime pm.
> > > > > 2. suspend_late() It continues the suspend operations of the drm device
> > > > > which was started by suspend(). In our case, it performs the same functionality
> > > > > as runtime_suspend().
> > > > >
> > > > > The complimentary functions are resume() and resume_early(). In the case of
> > > > > resume_early(), we invoke malidp_runtime_pm_resume() which enables the clocks
> > > > > and the interrupts. It sets the runtime status as active. If the device was
> > > > > in runtime suspend mode before system suspend was called, pm_runtime_work()
> > > > > will put the device back in runtime suspended mode( after the complete system
> > > > > has been resumed).
> > > > >
> > > > > Signed-off-by: Ayan Kumar Halder <ayan.halder@xxxxxxx>
> > > > Afaiui we still haven't bottomed out on the discussion on v1. Did you get
> > > > hold of Rafael?
> > > No, there was no reply from him. Lets try again:
> > >
> > > Rafael, we are debating on what the proper approach is for handling the
> > > suspend/resume callbacks for a DRM driver that is likely to not be
> > > runtime suspended when the power-down happens (because we are driving
> > > the display output). We are using in this patch the LATE_SYSTEM_SLEEP_PM_OPS
> > > in order to do the work that we also do during runtime suspend, which is
> > > turning off the output and the clocks driving it. The reason for doing
> > > that is because the PM core takes a runtime reference during system
> > > suspend for all devices that are not already runtime suspended, so our
> > > runtime_pm_suspend() hook is never called.
> > >
> > > Daniel's argument is that we should not be doing this from LATE hooks,
> > > but from the normal suspend hooks, however kernel doc seems to suggest
> > > otherwise.
> > For more context: I thought the reason behind the recommendation to
> > stuff the rpm callbacks into the late/early hooks was to solve
> > cross-device ordering issues. That way everyone shuts down the device
> > functionality in the normal hooks, but only powers them off in the
> > late hook (to allow other drivers to keep using the clock/i2c
> > master/whatever). But we now have device_link to solve that since a
> > while, so I'm not sure the recommendation to stuff the rpm hooks into
> > late/early callbacks is still correct.
> > -Daniel
> It has been more than two weeks and we have not got any response from
> Rafael. Can you ping him personally or suggest any way by which ask
> him to respond?

It is in my queue though, sorry for the delay.

It would help if you resent the series with a CC to 
linux-pm@xxxxxxxxxxxxxxx as it would be easier for me to review it then.

Thanks,
Rafael