Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 14 May 2018 11:01:26 +0100
From:   Ayan Halder <ayan.halder@arm.com>
To:     Daniel Vetter <daniel@ffwll.ch>
Cc:     Liviu Dudau <liviu.dudau@arm.com>,
        Brian Starkey <brian.starkey@arm.com>,
        Dave Airlie <airlied@linux.ie>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>, nd@arm.com
Subject: Re: [PATCH v3 5/5] drm/arm/malidp: Added the late system pm functions
Message-ID: <20180514100126.GA24437@arm.com>
References: <1524593567-5559-1-git-send-email-ayan.halder@arm.com>
 <1524593567-5559-6-git-send-email-ayan.halder@arm.com>
 <20180425071722.GN25142@phenom.ffwll.local>
 <20180425112604.GP14661@e110455-lin.cambridge.arm.com>
 <CAKMK7uGeptRT+oeTQQQ4U6LgAVQg2SOofTrqDo9MfFwRkKhj4A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAKMK7uGeptRT+oeTQQQ4U6LgAVQg2SOofTrqDo9MfFwRkKhj4A@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
NoDisclaimer: True
Received-SPF: None (protection.outlook.com: arm.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 May 2018 10:01:35.9148
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 9e49902e-6ea6-4109-58bd-08d5b981ae4f
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f34e5979-57d9-4aaa-ad4d-b122a662184d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB7PR08MB3034
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@arm.com> 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@arm.com>
> >> >
> >>
> >> 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?
> >
> > Best regards,
> > Liviu
> >
> >
> >
> >> -Daniel
> >>
> >> > ---
> >> > Changes in v3:-
> >> > - Rebased on top of earlier v3 patches,
> >> >
> >> > Changes in v2:-
> >> > - Removed the change id and modified the commit message
> >> > ---
> >> >  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 82221ea..c53b46a 100644
> >> > --- a/drivers/gpu/drm/arm/malidp_drv.c
> >> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> >> > @@ -768,8 +768,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
> >
> > --
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ??\_(???)_/??
> > _______________________________________________
> > 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