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 <[email protected]>
> Reviewed-by: Matthias Kaehlcke <[email protected]>
> Cc: Stanimir Varbanov <[email protected]>
> Cc: [email protected]
> ---
> 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.
--
regards,
Stan
