Date:   Thu, 6 Apr 2023 12:59:10 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Abel Vesa <abel.vesa@...aro.org>
Cc:     Saravana Kannan <saravanak@...gle.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Kevin Hilman <khilman@...nel.org>, Pavel Machek <pavel@....cz>,
        Len Brown <len.brown@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Mike Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, linux-pm@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
        Doug Anderson <dianders@...omium.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        Android Kernel Team <kernel-team@...roid.com>
Subject: Re: [PATCH v3 0/4] Allow genpd providers to power off domains on sync state

On Thu, 6 Apr 2023 at 11:26, Abel Vesa <abel.vesa@...aro.org> wrote:
>
> On 23-04-05 16:11:18, Ulf Hansson wrote:
> > Abel, Saravana,
> >
> > On Fri, 31 Mar 2023 at 06:59, Abel Vesa <abel.vesa@...aro.org> wrote:
> > >
> > > On 23-03-30 12:50:44, Saravana Kannan wrote:
> > > > On Thu, Mar 30, 2023 at 4:27 AM Abel Vesa <abel.vesa@...aro.org> wrote:
> > > > >
> > > > > On 23-03-27 17:17:28, Saravana Kannan wrote:
> > > > > > On Mon, Mar 27, 2023 at 12:38 PM Abel Vesa <abel.vesa@...aro.org> wrote:
> > > > > > >
> > > > > > > There have been already a couple of tries to make the genpd "disable
> > > > > > > unused" late initcall skip the powering off of domains that might be
> > > > > > > needed until later on (i.e. until some consumer probes). The conclusion
> > > > > > > was that the provider could return -EBUSY from the power_off callback
> > > > > > > until the provider's sync state has been reached. This patch series tries
> > > > > > > to provide a proof-of-concept that is working on Qualcomm platforms.
> > > > > >
> > > > > > I'm giving my thoughts in the cover letter instead of spreading it
> > > > > > around all the patches so that there's context between the comments.
> > > > > >
> > > > > > 1) Why can't all the logic in this patch series be implemented at the
> > > > > > framework level? And then allow the drivers to opt into this behavior
> > > > > > by setting the sync_state() callback.
> > > > > >
> > > > > > That way, you can land it only for QC drivers by setting up
> > > > > > sync_state() callback only for QC drivers, but actually have the same
> > > > > > code function correctly for non-QC drivers too. And then once we have
> > > > > > this functionality working properly for QC drivers for one kernel
> > > > > > version (or two), we'll just have the framework set the device's
> > > > > > driver's sync_state() if it doesn't have one already.
> > > > >
> > > > > I think Ulf has already NACK'ed that approach here:
> > > > > [1] https://lore.kernel.org/lkml/CAPDyKFon35wcQ+5kx3QZb-awN_S_q8y1Sir-G+GoxkCvpN=iiA@mail.gmail.com/
> > > >
> > > > I would have NACK'ed that too because that's an incomplete fix. As I
> > > > said further below, the fix needs to be at the aggregation level where
> > > > you aggregate all the current consumer requests. In there, you need to
> > > > add in the "state at boot" input that gets cleared out after a
> > > > sync_state() call is received for that power domain.
> > > >
> > >
> > > So, just to make sure I understand your point. You would rather have the
> > > genpd_power_off check if 'state at boot' is 'on' and return busy and
> > > then clear then, via a generic genpd sync state you would mark 'state at
> > > boot' as 'off' and queue up a power off request for each PD from there.
> > > And as for 'state at boot' it would check the enable bit through
> > > provider.
> > >
> > > Am I right so far?
> >
> > I am not sure I completely follow what you are suggesting here.
>
> Please have a look at this:
> https://git.kernel.org/pub/scm/linux/kernel/git/abelvesa/linux.git/commit/?h=qcom/genpd/ignore_unused_until_sync_state&id=4f9e6140dfe77884012383f8ba2140cadb62ca4a
>
> Keep in mind that is WIP for now. Once I have something, I'll post it on
> mailing list. Right now, there is a missing piece mentioned in that
> commit message.

I had a quick look and it seems rather promising, but I will have a
closer look when you post it.

>
> >
> > Although, let me point out that there is no requirement from the genpd
> > API point of view, that the provider needs to be a driver. This means
> > that the sync_state callback may not even be applicable for all genpd
> > providers.
>
> Yes, I'm considering that case too.

Good.

>
> >
> > In other words, it looks to me that we may need some new genpd helper
> > functions, no matter what. More importantly, it looks like we need an
> > opt-in behaviour, unless we can figure out a common way for genpd to
> > understand whether the sync_state thing is going to be applicable or
> > not. Maybe Saravana has some ideas around this?
> >
> > Note that, I don't object to extending genpd to be more clever and to
> > share common code, of course. We could, for example, make
> > genpd_power_off() to bail out earlier, rather than calling the
> > ->power_off() callback and waiting for it to return -EBUSY. Both of
> > you have pointed this out to me, in some of the earlier
> > replies/discussions too.
>
> The above link basically does this. I hope this is what Saravana has in
> mind as well.

Okay, let's see what Saravana thinks.

Maybe it's easier to post an RFC, based upon the above and continue
the discussion around that?

Kind regards
Uffe

Powered by blists - more mailing lists