Re: [PATCH v3 12/17] media: venus: core: Add support for opp tables/perf voting

[Rajendra Nayak]

Hey Stan,

On 4/29/2020 8:06 PM, Stanimir Varbanov wrote:
> Hi Rajendra,
>
> Thanks for the patch!
[]..

> >   #include <linux/pm_runtime.h>
> >   #include <media/videobuf2-v4l2.h>
> >   #include <media/v4l2-mem2mem.h>
> > @@ -214,6 +215,20 @@ static int venus_probe(struct platform_device *pdev)
> >   	if (!core->pm_ops)
> >   		return -ENODEV;
> >   
> > +	core->opp_table = dev_pm_opp_set_clkname(dev, "core");
>
> Should we set opp clkname if opp_of_add_table fails? We have platforms
> which don't have opp tables in Venus DT node. We have to be backward
> compatible for them.

so the way its designed, you are expected to call dev_pm_opp_set_clkname()
*before* adding any OPPs from the OPP table.
As for backward compatibility its already handled by the OPP core now [1]
which makes sure dev_pm_opp_set_rate() is equivalent to a clk_set_rate()
in case of a missing OPP table.

> > +	if (IS_ERR(core->opp_table))
> > +		return PTR_ERR(core->opp_table);
> > +
> > +	if (core->res->opp_pmdomain) {
> > +		ret = dev_pm_opp_of_add_table(dev);
> > +		if (!ret) {
> > +			core->has_opp_table = true;
> > +		} else if (ret != -ENODEV) {
>
> Is it possible dev_pm_opp_of_add_table() to return EPROBE_DEFER?

Nope, it does not, I had checked.

> > +			dev_err(dev, "Invalid OPP table in Device tree\n");
>
> ... if so, please drop dev_err.
>
> > +			return ret;
> > +		}
> > +	}
[]..

> > --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> > +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> > @@ -9,6 +9,7 @@
> >   #include <linux/iopoll.h>
> >   #include <linux/kernel.h>
> >   #include <linux/pm_domain.h>
> > +#include <linux/pm_opp.h>
> >   #include <linux/pm_runtime.h>
> >   #include <linux/types.h>
> >   #include <media/v4l2-mem2mem.h>
> > @@ -66,10 +67,9 @@ static void core_clks_disable(struct venus_core *core)
> >   
> >   static int core_clks_set_rate(struct venus_core *core, unsigned long freq)
> >   {
> > -	struct clk *clk = core->clks[0];
> >   	int ret;
> >   
> > -	ret = clk_set_rate(clk, freq);
> > +	ret = dev_pm_opp_set_rate(core->dev, freq);
>
> Is this will work for legacy platforms without OPP tables?

yes, see [1] which is already merged in mainline.

> Also what about the other clocks (vicodec0,1) in this function.

We continue to use clk_set_rate() for those. The performance state
is associated with only the core clk.

> >   	if (ret)
> >   		return ret;
> >   
> > @@ -740,13 +740,15 @@ static int venc_power_v4(struct device *dev, int on)
> >   
> >   static int vcodec_domains_get(struct device *dev)
> >   {
> > +	struct opp_table *opp_table;
> > +	struct device **opp_virt_dev;
> >   	struct venus_core *core = dev_get_drvdata(dev);
> >   	const struct venus_resources *res = core->res;
> >   	struct device *pd;
> >   	unsigned int i;
> >   
> >   	if (!res->vcodec_pmdomains_num)
> > -		return -ENODEV;
> > +		goto skip_pmdomains;
> >   
> >   	for (i = 0; i < res->vcodec_pmdomains_num; i++) {
> >   		pd = dev_pm_domain_attach_by_name(dev,
> > @@ -763,6 +765,24 @@ static int vcodec_domains_get(struct device *dev)
> >   	if (!core->pd_dl_venus)
> >   		return -ENODEV;
> >   
> > +skip_pmdomains:
> > +	if (!res->opp_pmdomain || !core->has_opp_table)
> > +		return 0;
> > +
> > +	/* Attach the power domain for setting performance state */
> > +	opp_table = dev_pm_opp_attach_genpd(dev, res->opp_pmdomain, &opp_virt_dev);
> > +	if (IS_ERR(opp_table)) {
> > +		return PTR_ERR(opp_table);
> > +	} else if (opp_virt_dev) {
> > +		core->opp_pmdomain = *opp_virt_dev;
> > +		core->opp_dl_venus = device_link_add(dev, core->opp_pmdomain,
> > +						     DL_FLAG_RPM_ACTIVE |
> > +						     DL_FLAG_PM_RUNTIME |
> > +						     DL_FLAG_STATELESS);
> > +		if (!core->opp_dl_venus)
> > +			return -ENODEV;
>
> I think as you return ENODEV you have to detach opp domain here because
> vcodec_domains_put() is not called in error path.

Ok, I'll fix that up.

Thanks for the review.

[1] https://lkml.org/lkml/2020/4/8/413

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation