From: Viresh Kumar <viresh.kumar@linaro.org>
To: Dmitry Osipenko <digetx@gmail.com>
Cc: Thierry Reding <thierry.reding@gmail.com>,
Jonathan Hunter <jonathanh@nvidia.com>,
Ulf Hansson <ulf.hansson@linaro.org>,
"Rafael J. Wysocki" <rjw@rjwysocki.net>,
Kevin Hilman <khilman@kernel.org>,
Viresh Kumar <vireshk@kernel.org>,
Stephen Boyd <sboyd@kernel.org>, Nishanth Menon <nm@ti.com>,
linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org,
linux-pm@vger.kernel.org
Subject: Re: [PATCH v10 5/8] soc/tegra: pmc: Implement dev_get_performance_state() callback
Date: Wed, 1 Sep 2021 12:46:55 +0530 [thread overview]
Message-ID: <20210901071655.gf6qg7piisddg2a3@vireshk-i7> (raw)
In-Reply-To: <7f4f5ab0-cf23-3e27-211e-4dcd8903f96f@gmail.com>
On 01-09-21, 09:57, Dmitry Osipenko wrote:
> 01.09.2021 09:10, Viresh Kumar пишет:
> > So you create the OPP table and just after that you remove it ? Just
> > to get the current OPP and pstate ? This doesn't look okay.
> >
> > Moreover, this routine must be implemented with the expectation that
> > the genpd core can call it anytime it wants, not just at the
> > beginning. And so if the table is already setup by someone else then,
> > then this all will just fail.
>
> This is not doable using the current OPP API, it doesn't allow to
> re-use initialized OPP table.
That isn't correct, you can call dev_pm_opp_of_add_table() as many
times as you want. It will just increase the refcount and return the
next time.
Yes, you can call the APIs like set-clk-name or supported-hw, since
they are supposed to be set only once.
> The current limitation is okay because genpd core doesn't call
> routine anytime.
Yeah, but is broken by design. People can make changes to genpd core
later on to call it as many times and they aren't required to have a
look at all the users to see who abused the API and who didn't.
> > Can you give the sequence in which the whole thing works out with
> > respect to the OPP core? who calls
> > devm_tegra_core_dev_init_opp_table() and when, and when exactly will
> > this routine get called, etc ?
> >
>
> gr3d_probe(struct platform_device *pdev)
Thanks for this.
> {
> gr3d_init_power(dev)
> {
> static const char * const opp_genpd_names[] = { "3d0", "3d1", NULL };
>
> devm_pm_opp_attach_genpd(dev, opp_genpd_names)
> {
> dev_pm_opp_attach_genpd(dev, names, virt_devs)
> {
> // takes and holds table reference
> opp_table = _add_opp_table(dev, false);
>
> while (*name) {
> // first attachment succeeds,
> // second fails with "tegra-gr3d 54180000.gr3d: failed to set OPP clk: -EBUSY"
> dev_pm_domain_attach_by_name(dev, *name)
> {
> tegra_pmc_pd_dev_get_performance_state(dev)
> {
> dev_pm_opp_set_clkname(dev, NULL);
> dev_pm_opp_of_add_table(dev);
What you end up doing here is pretty much like
devm_tegra_core_dev_init_opp_table_simple(), right ?
> opp = dev_pm_opp_get_current(dev);
> dev_pm_opp_of_remove_table(dev);
> dev_pm_opp_put_clkname(opp_table);
You shouldn't be required to do this at all.
> ...
> }
> // opp_table->clk = ERR_PTR(-EINVAL) after the first attachment
> }
> }
> }
> }
> }
>
> devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);
Can we make the call at the beginning ? before calling
gr3d_init_power() ? I mean you should only be required to initialize
the OPP table just once.
If not, then what about calling
devm_tegra_core_dev_init_opp_table_simple() from here and from
tegra_pmc_pd_dev_get_performance_state() as well ?
And update devm_tegra_core_dev_init_opp_table_simple() to call
dev_pm_opp_get_opp_table() first and return early if the OPP table is
already initialized ?
--
viresh
next prev parent reply other threads:[~2021-09-01 7:17 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-31 13:54 [PATCH v10 0/8] NVIDIA Tegra power management patches for 5.16 Dmitry Osipenko
2021-08-31 13:54 ` [PATCH v10 1/8] opp: Add dev_pm_opp_get_current() Dmitry Osipenko
2021-09-01 4:39 ` Viresh Kumar
2021-09-01 5:43 ` Dmitry Osipenko
2021-09-01 6:05 ` Viresh Kumar
2021-08-31 13:54 ` [PATCH v10 2/8] opp: Allow dev_pm_opp_set_clkname() to replace released clock Dmitry Osipenko
2021-09-01 4:42 ` Viresh Kumar
2021-09-01 5:46 ` Dmitry Osipenko
2021-09-01 6:02 ` Viresh Kumar
2021-08-31 13:54 ` [PATCH v10 3/8] opp: Change type of dev_pm_opp_attach_genpd(names) argument Dmitry Osipenko
2021-09-01 4:41 ` Viresh Kumar
2021-09-01 5:44 ` Dmitry Osipenko
2021-09-01 5:48 ` Viresh Kumar
2021-08-31 13:54 ` [PATCH v10 4/8] PM: domains: Add dev_get_performance_state() callback Dmitry Osipenko
2021-09-01 16:59 ` Ulf Hansson
2021-09-02 8:42 ` Dmitry Osipenko
2021-08-31 13:54 ` [PATCH v10 5/8] soc/tegra: pmc: Implement " Dmitry Osipenko
2021-09-01 6:10 ` Viresh Kumar
2021-09-01 6:57 ` Dmitry Osipenko
2021-09-01 7:16 ` Viresh Kumar [this message]
2021-09-01 9:04 ` Dmitry Osipenko
2021-08-31 13:54 ` [PATCH v10 6/8] soc/tegra: Add devm_tegra_core_dev_init_opp_table_simple() Dmitry Osipenko
2021-08-31 13:54 ` [PATCH v10 7/8] gpu: host1x: Add host1x_channel_stop() Dmitry Osipenko
2021-08-31 13:54 ` [PATCH v10 8/8] drm/tegra: gr3d: Support generic power domain and runtime PM Dmitry Osipenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210901071655.gf6qg7piisddg2a3@vireshk-i7 \
--to=viresh.kumar@linaro.org \
--cc=digetx@gmail.com \
--cc=jonathanh@nvidia.com \
--cc=khilman@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=nm@ti.com \
--cc=rjw@rjwysocki.net \
--cc=sboyd@kernel.org \
--cc=thierry.reding@gmail.com \
--cc=ulf.hansson@linaro.org \
--cc=vireshk@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).