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

From: Ulf Hansson <ulf.hansson@linaro.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Dmitry Osipenko <digetx@gmail.com>,
	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 13:46:43 +0100	[thread overview]
Message-ID: <CAPDyKFr7SD_A9sKD2+90XfjP09T+PCLE=8qvn=M-yPEPXvBhsg@mail.gmail.com> (raw)
In-Reply-To: <20210118110506.linvsoca7jbl42iq@vireshk-i7>

On Mon, 18 Jan 2021 at 12:05, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 18-01-21, 11:59, Ulf Hansson wrote:
> > 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?
>
> That will work but I am wondering if there is a way to avoid the
> unnecessary propagation if we can somehow find out if someone above in
> hierarchy supports pstates or not ?

You seem to be worried about latency/overhead while doing the
propagation upwards in the hierarchy. That seems like a reasonable
concern to me, especially as the genpd lock is taken at each level.

However, to mitigate this can be rather messy. In principle, we would
need to walk the hierarchy upwards, each time a new subdomain is added
in genpd_add_subdomain(). While doing this, we would also need to keep
track on what level we set to continue the propagation of the
performance states for. Even if this can be done in non-latency
sensitive paths, I don't think it's worth it because of complexity (I
haven't even thought of what happens when removing a subdomain).

So, maybe we should simply just stick to the existing code, forcing
the parent to have a ->set_performance() callback assigned if
propagation should continue?

Kind regards
Uffe