Messages in this thread | | | Subject | Re: [PATCH v3 12/17] media: venus: core: Add support for opp tables/perf voting | From | Rajendra Nayak <> | Date | Wed, 29 Apr 2020 20:40:42 +0530 |
| |
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
| |