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

From: Brian Starkey <brian.starkey@arm.com>
To: daniel@ffwll.ch
Cc: Ayan Halder <ayan.halder@arm.com>,
	Liviu Dudau <liviu.dudau@arm.com>,
	Mali DP Maintainers <malidp@foss.arm.com>,
	Dave Airlie <airlied@linux.ie>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	nd <nd@arm.com>
Subject: Re: [PATCH 8/8] drm/arm/malidp: Added the late system pm functions
Date: Mon, 9 Apr 2018 17:15:08 +0100	[thread overview]
Message-ID: <20180409161226.GA6726@e107564-lin.cambridge.arm.com> (raw)
In-Reply-To: <20180409082337.GG31310@phenom.ffwll.local>

Hi Daniel,

On Mon, Apr 09, 2018 at 10:23:37AM +0200, Daniel Vetter wrote:
>On Fri, Apr 06, 2018 at 08:02:16PM +0100, Ayan Halder wrote:
>> On Tue, Mar 27, 2018 at 01:09:36PM +0200, Daniel Vetter wrote:
>> > On Tue, Mar 27, 2018 at 11:59 AM, Ayan Halder <ayan.halder@arm.com> wrote:
>> > > On Tue, Mar 27, 2018 at 10:29:03AM +0200, Daniel Vetter wrote:
>> > >> On Mon, Mar 26, 2018 at 06:03:20PM +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. Subsequently, malidp_pm_resume_early will invoke
>> > >> > malidp_runtime_pm_resume which enables the clocks and the interrupts
>> > >> > (previously disabled) and sets the runtime status as active.
>> > >> >
>> > >> > Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
>> > >> > Change-Id: I5f8c3d28f076314a1c9da2a46760a9c37039ccda
>> > >>
>> > >> Why exactly do you need late/early hooks? If you have dependencies with
>> > >> other devices, pls consider adding device_links instead. This here
>> > >> shouldn't be necessary.
>> > >> -Daniel
>> > > We need to late/early hooks to disable malidp interrupts and the
>> > > clocks.
>> >
>> > Yes, but why this ordering constraint? Why can't you just disable the
>> > interrupts/clocks in the normal suspend code. I see that the patch
>> > does this, I want to understand why it does it.
>> > -Daniel
>> Apologies for my delayed response on this.
>>
>> With reference to https://lwn.net/Articles/505683/ :-
>> 1. "suspend() should leave the device in a quiescent state." We invoke
>> drm_mode_config_helper_suspend() which deactivates the crtc. I
>> understand that this is the quiescent state.
>>
>> 2. "suspend_late() can often be the same as runtime_suspend()."  We
>> invoke runtime suspend/resume calls in late/early hooks.
>
>This article is from 2012. That's not really recommended best practices
>anymore. device_links have only been added a few years ago, so ofc an
>article from 2012 can't tell you that you should use those instead :-)
>
>That's why I brought this up, we have much better ways to handle device
>dependencies now.
>

We aren't trying to manage any device dependencies here, I don't know
where that idea came from?

The kernel-doc on drm-next this afternoon says effectively the same
thing:

   * @suspend: Executed before putting the system into a sleep state in which the
   *      contents of main memory are preserved.  The exact action to perform
   *      depends on the device's subsystem (PM domain, device type, class or bus
   *      type), but generally the device must be quiescent after subsystem-level
   *      @suspend() has returned, so that it doesn't do any I/O or DMA.
   *      Subsystem-level @suspend() is executed for all devices after invoking
   *      subsystem-level @prepare() for all of them.

(i.e. suspend() makes the device quiescent).

   * @suspend_late: Continue operations started by @suspend().  For a number of
   *      devices @suspend_late() may point to the same callback routine as the
   *      runtime suspend callback.

(suggests suspend_late() be assigned to the same function as runtime
suspend).

As for why, my understanding is like so:

For ->suspend(), we use the DRM helper, which disables the CRTC.
Normally disabling the CRTC would be enough to also invoke our
pm_runtime callback to do the final clock disable etc., however when a
system suspend is in-progress, the core forcibly takes a runtime
reference on all devices - preventing any pm_runtime paths from
running.

This means that after the CRTC is disabled in ->suspend(), our normal
pm_runtime path will not be invoked, and so the things done in
malidp_runtime_pm_suspend() will never happen.

We were just following the advice in the kernel-doc to deal with this.

The alternative would be to call malidp_runtime_pm_{suspend,resume}
from the "not late" hooks, but I'd ask why?

>Also, you still haven't explained what exactly the dependency is.

Because there isn't one :-)

Thanks,
-Brian

>-Daniel
>
>>
>> > >> > ---
>> > >> >  drivers/gpu/drm/arm/malidp_drv.c | 17 +++++++++++++++++
>> > >> >  1 file changed, 17 insertions(+)
>> > >> >
>> > >> > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
>> > >> > index bd44a6d..f6124d8 100644
>> > >> > --- a/drivers/gpu/drm/arm/malidp_drv.c
>> > >> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
>> > >> > @@ -766,8 +766,25 @@ static int __maybe_unused malidp_pm_resume(struct device *dev)
>> > >> >     return 0;
>> > >> >  }
>> > >> >
>> > >> > +static int __maybe_unused malidp_pm_suspend_late(struct device *dev)
>> > >> > +{
>> > >> > +   if (!pm_runtime_status_suspended(dev)) {
>> > >> > +           malidp_runtime_pm_suspend(dev);
>> > >> > +           pm_runtime_set_suspended(dev);
>> > >> > +   }
>> > >> > +   return 0;
>> > >> > +}
>> > >> > +
>> > >> > +static int __maybe_unused malidp_pm_resume_early(struct device *dev)
>> > >> > +{
>> > >> > +   malidp_runtime_pm_resume(dev);
>> > >> > +   pm_runtime_set_active(dev);
>> > >> > +   return 0;
>> > >> > +}
>> > >> > +
>> > >> >  static const struct dev_pm_ops malidp_pm_ops = {
>> > >> >     SET_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend, malidp_pm_resume) \
>> > >> > +   SET_LATE_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend_late, malidp_pm_resume_early) \
>> > >> >     SET_RUNTIME_PM_OPS(malidp_runtime_pm_suspend, malidp_runtime_pm_resume, NULL)
>> > >> >  };
>> > >> >
>> > >> > --
>> > >> > 2.7.4
>> > >> >
>> > >> > _______________________________________________
>> > >> > dri-devel mailing list
>> > >> > dri-devel@lists.freedesktop.org
>> > >> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> > >>
>> > >> --
>> > >> Daniel Vetter
>> > >> Software Engineer, Intel Corporation
>> > >> http://blog.ffwll.ch
>> > >> _______________________________________________
>> > >> dri-devel mailing list
>> > >> dri-devel@lists.freedesktop.org
>> > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>> > > _______________________________________________
>> > > dri-devel mailing list
>> > > dri-devel@lists.freedesktop.org
>> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >
>> >
>> >
>> > --
>> > Daniel Vetter
>> > Software Engineer, Intel Corporation
>> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch