Re: [PATCH v6 5/6] media: venus: core: Add support for opp tables/perf voting

[Rajendra Nayak]

On 6/18/2020 8:24 PM, Stanimir Varbanov wrote:
> Hi Rajendra,
>
> On 6/15/20 3:02 PM, Rajendra Nayak wrote:
> > Add support to add OPP tables and perf voting on the OPP powerdomain.
> > This is needed so venus votes on the corresponding performance state
> > for the OPP powerdomain along with setting the core clock rate.
> >
> > Signed-off-by: Rajendra Nayak <rnayak@xxxxxxxxxxxxxx>
> > Reviewed-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> > Cc: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx>
> > Cc: linux-media@xxxxxxxxxxxxxxx
> > ---
> > No functional change in v6, rebased over 5.8-rc1
> > Bindings update to add optional PD https://lore.kernel.org/patchwork/patch/1241077/
> >
> >   drivers/media/platform/qcom/venus/core.c       | 43 +++++++++++++++++---
> >   drivers/media/platform/qcom/venus/core.h       |  5 +++
> >   drivers/media/platform/qcom/venus/pm_helpers.c | 54 ++++++++++++++++++++++++--
> >   3 files changed, 92 insertions(+), 10 deletions(-)
>
> <cut>
>
> >   
> > @@ -740,13 +740,16 @@ static int venc_power_v4(struct device *dev, int on)
> >   
> >   static int vcodec_domains_get(struct device *dev)
> >   {
> > +	int ret;
> > +	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,7 +766,41 @@ static int vcodec_domains_get(struct device *dev)
> >   	if (!core->pd_dl_venus)
> >   		return -ENODEV;
> >   
> > +skip_pmdomains:
> > +	if (!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)) {
> > +		ret = PTR_ERR(opp_table);
> > +		goto opp_attach_err;
> > +	}
> > +
> > +	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) {
> > +		ret = -ENODEV;
> > +		goto opp_dl_add_err;
> > +	}
> > +
> >   	return 0;
> > +
> > +opp_dl_add_err:
> > +	dev_pm_domain_detach(core->opp_pmdomain, true);
> > +opp_attach_err:
> > +	if (core->pd_dl_venus) {
> > +		device_link_del(core->pd_dl_venus);
> > +		for (i = 0; i < res->vcodec_pmdomains_num; i++) {
> > +			if (IS_ERR_OR_NULL(core->pmdomains[i]))
> > +				continue;
> > +			dev_pm_domain_detach(core->pmdomains[i], true);
> > +		}
> > +	}
> > +	return ret;
> >   }
> >   
> >   static void vcodec_domains_put(struct device *dev)
> > @@ -773,7 +810,7 @@ static void vcodec_domains_put(struct device *dev)
> >   	unsigned int i;
> >   
> >   	if (!res->vcodec_pmdomains_num)
> > -		return;
> > +		goto skip_pmdomains;
> >   
> >   	if (core->pd_dl_venus)
> >   		device_link_del(core->pd_dl_venus);
> > @@ -783,6 +820,15 @@ static void vcodec_domains_put(struct device *dev)
> >   			continue;
> >   		dev_pm_domain_detach(core->pmdomains[i], true);
> >   	}
> > +
> > +skip_pmdomains:
> > +	if (!res->opp_pmdomain)
> > +		return;
> > +
> > +	if (core->opp_dl_venus)
> > +		device_link_del(core->opp_dl_venus);
> > +
> > +	dev_pm_domain_detach(core->opp_pmdomain, true);
>
> Without corresponding changes in venus DT node we call pm_domain_detach
> with core->opp_pmdomain = NULL which triggers NULL pointer dereference.
>
> I guess you should check:
>
> 	if (core->has_opp_table)
> 		dev_pm_domain_detach(core->opp_pmdomain, true);
>
> or
>
> 	if (core->opp_pmdomain)
> 		dev_pm_domain_detach(core->opp_pmdomain, true);
>
>
> ... not sure which one is more appropriate or both are.

Thanks, I'll fix that up when I repost. Sorry I was out for a while so
haven't been able to reproduce the rpmh timeout issue you reported.
Will spend some time on it this week and see if I can reproduce it.


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