Re: [PATCH v10 5/8] soc/tegra: pmc: Implement dev_get_performance_state() callback

01.09.2021 10:16, Viresh Kumar пишет:

On 01-09-21, 09:57, Dmitry Osipenko wrote:

quoted

01.09.2021 09:10, Viresh Kumar пишет:

quoted

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.

quoted

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.

quoted

quoted

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.

quoted

{
	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 ?

Yes

quoted

							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.

quoted

							...
						}
						// 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.

This breaks dev_pm_opp_set_supported_hw() which isn't allowed to be
called for a populated OPP table, set/put_hw also isn't refcounted.

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 ?

This doesn't work for devm_pm_opp_register_set_opp_helper() that is used
by gr3d_probe() because set_opp_helper() should be invoked only before
table is populated and it's already populated for the case of a
single-domain h/w since domain is attached before driver is probed.

But it's a good idea to use dev_pm_opp_get_opp_table(). I got it to work
by adding dev_pm_opp_get_opp_table() to
tegra_pmc_pd_dev_get_performance_state() and moving
devm_tegra_core_dev_init_opp_table_simple() before gr3d_init_power().

Thank you for the suggestion, I'll change it in v11.