Re: [PATCH v3 1/3] PM: domains: Make set_performance_state() callback optional

From: Dmitry Osipenko <digetx@gmail.com>
To: Ulf Hansson <ulf.hansson@linaro.org>,
	Viresh Kumar <viresh.kumar@linaro.org>
Cc: Thierry Reding <thierry.reding@gmail.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Peter Geis <pgwipeout@gmail.com>,
	Nicolas Chauvet <kwizart@gmail.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Kevin Hilman <khilman@kernel.org>,
	Peter De Schrijver <pdeschrijver@nvidia.com>,
	Viresh Kumar <vireshk@kernel.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Matt Merhar <mattmerhar@protonmail.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-tegra <linux-tegra@vger.kernel.org>,
	Linux PM <linux-pm@vger.kernel.org>
Subject: Re: [PATCH v3 1/3] PM: domains: Make set_performance_state() callback optional
Date: Mon, 18 Jan 2021 21:44:43 +0300	[thread overview]
Message-ID: <1e54674e-000d-d3a1-6123-715f8f445726@gmail.com> (raw)
In-Reply-To: <CAPDyKFquCGUSTvcCpmN0vm1eGEz9B_hYSNm7wojhgwuXT=jkEQ@mail.gmail.com>

18.01.2021 13:59, Ulf Hansson пишет:
> On Mon, 18 Jan 2021 at 08:28, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>
>> On 18-01-21, 04:13, Dmitry Osipenko wrote:
>>> Make set_performance_state() callback optional in order to remove the
>>> need from power domain drivers to implement a dummy callback. If callback
>>> isn't implemented by a GENPD driver, then the performance state is passed
>>> to the parent domain.
>>>
>>> Tested-by: Peter Geis <pgwipeout@gmail.com>
>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>>> Tested-by: Matt Merhar <mattmerhar@protonmail.com>
>>> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/base/power/domain.c | 11 +++++------
>>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>> index 9a14eedacb92..a3e1bfc233d4 100644
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -339,9 +339,11 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
>>>                       goto err;
>>>       }
>>>
>>> -     ret = genpd->set_performance_state(genpd, state);
>>> -     if (ret)
>>> -             goto err;
>>> +     if (genpd->set_performance_state) {
>>> +             ret = genpd->set_performance_state(genpd, state);
>>> +             if (ret)
>>> +                     goto err;
>>> +     }
>>
>> Earlier in this routine we also have this:
>>
>> if (!parent->set_performance_state)
>>         continue;
>>
>> Should we change that too ?
> 
> Good point! I certainly overlooked that when reviewing. We need to
> reevaluate the new state when propagating to the parent(s).
> 
> To me, it looks like when doing the propagation we must check if the
> parent has the ->set_performance_state() callback assigned. If so, we
> should call dev_pm_opp_xlate_performance_state(), but otherwise just
> use the value of "state", when doing the reevaluation.
> 
> Does it make sense?

I played a tad with the power domains by creating a couple dummy domains
and putting them into a parent->child chain. Yours suggestion works well
for the case where intermediate domain doesn't implement the
set_performance_state() callback, i.e. the performance state is
propagated through the whole chain instead of stopping on the domain
that doesn't implement the callback.

I'll make a v4, thanks.