[1/3] media: venus: pm_helper: Commonize v3/v4 pmdomains and clocks management

Commit Message

AngeloGioacchino Del Regno Sept. 26, 2020, 12:54 p.m. UTC

From: AngeloGioacchino Del Regno <[e-mail]>

Most of the SoCs that are featuring Venus HFI3 also need to scale the
clocks and also need to bringup power domains, very very similarly to
SoCs (like SDM845 and SC7180) featuring HFI4 Venus, which has *yes*
differences, but not in that specific "field".

So, reduce code duplication (most v3 functions are a subset of v4),
achieve better power consumption and slightly simplify the code by
merging the v3 and v4 functions that are responsible for bringing
up and tearing down clocks and pm domains.

While at it, also add a small quirk to the video encoder functions:
since not all SoCs have got two (or more) vcodec instances, it is
incorrect to *always* manage vcodec1_clks (and itsrelative pmdomain)
there.
To get video encoders working fine - and to avoid duplicating clocks
and pmdomains declarations between vcodec0 and 1 - check for the
number of vcodecs and currently assume that:
  1. If Venus has only one vcodec (vcodec_num), then we use vcodec0
     for both video decoder and encoder;
  2. If it has more than one vcodec, then we use vcodec1 for the
     video encoder and vcodec0 for the decoder.

Signed-off-by: AngeloGioacchino Del Regno <[e-mail]>
---
 .../media/platform/qcom/venus/pm_helpers.c    | 525 +++++++++---------
 1 file changed, 277 insertions(+), 248 deletions(-)

Comments

kernel test robot Sept. 26, 2020, 4:16 p.m. UTC | #1

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on next-20200925]
[cannot apply to v5.9-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/kholk11-gmail-com/Qualcomm-Venus-enablement-for-SDM630-636-660/20200926-205558
base:   git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-a002-20200925 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project a83eb048cb9a75da7a07a9d5318bbdbf54885c87)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/144283cb3c0b5f0dbd7083ea6eec2b5f049ca9c2
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review kholk11-gmail-com/Qualcomm-Venus-enablement-for-SDM630-636-660/20200926-205558
        git checkout 144283cb3c0b5f0dbd7083ea6eec2b5f049ca9c2
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>):

>> drivers/media/platform/qcom/venus/pm_helpers.c:496:12: warning: address of array 'core->vcodec1_clks' will always evaluate to 'true' [-Wpointer-bool-conversion]
           if (core->vcodec1_clks) {
           ~~  ~~~~~~^~~~~~~~~~~~
   1 warning generated.

vim +496 drivers/media/platform/qcom/venus/pm_helpers.c

   476	
   477	static int core_get_v4(struct device *dev)
   478	{
   479		struct venus_core *core = dev_get_drvdata(dev);
   480		const struct venus_resources *res = core->res;
   481		int ret;
   482	
   483		ret = core_clks_get(core);
   484		if (ret)
   485			return ret;
   486	
   487		if (!res->vcodec_pmdomains_num)
   488			legacy_binding = true;
   489	
   490		dev_info(dev, "%s legacy binding\n", legacy_binding ? "" : "non");
   491	
   492		ret = vcodec_clks_get(core, dev, core->vcodec0_clks, res->vcodec0_clks);
   493		if (ret)
   494			return ret;
   495	
 > 496		if (core->vcodec1_clks) {
   497			ret = vcodec_clks_get(core, dev,
   498					      core->vcodec1_clks, res->vcodec1_clks);
   499			if (ret)
   500				return ret;
   501		}
   502	
   503		if (legacy_binding)
   504			return 0;
   505	
   506		core->opp_table = dev_pm_opp_set_clkname(dev, "core");
   507		if (IS_ERR(core->opp_table))
   508			return PTR_ERR(core->opp_table);
   509	
   510		if (core->res->opp_pmdomain) {
   511			ret = dev_pm_opp_of_add_table(dev);
   512			if (!ret) {
   513				core->has_opp_table = true;
   514			} else if (ret != -ENODEV) {
   515				dev_err(dev, "invalid OPP table in device tree\n");
   516				dev_pm_opp_put_clkname(core->opp_table);
   517				return ret;
   518			}
   519		}
   520	
   521		ret = vcodec_domains_get(dev);
   522		if (ret) {
   523			if (core->has_opp_table)
   524				dev_pm_opp_of_remove_table(dev);
   525			dev_pm_opp_put_clkname(core->opp_table);
   526			return ret;
   527		}
   528	
   529		return 0;
   530	}
   531	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Patch

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 57877eacecf0..fda480b26636 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -130,6 +130,100 @@  static void vcodec_clks_disable(struct venus_core *core, struct clk **clks)
 		clk_disable_unprepare(clks[i]);
 }
 
+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)
+		goto skip_pmdomains;
+
+	for (i = 0; i < res->vcodec_pmdomains_num; i++) {
+		pd = dev_pm_domain_attach_by_name(dev,
+						  res->vcodec_pmdomains[i]);
+		if (IS_ERR(pd))
+			return PTR_ERR(pd);
+		core->pmdomains[i] = pd;
+	}
+
+	core->pd_dl_venus = device_link_add(dev, core->pmdomains[0],
+					    DL_FLAG_PM_RUNTIME |
+					    DL_FLAG_STATELESS |
+					    DL_FLAG_RPM_ACTIVE);
+	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)
+{
+	struct venus_core *core = dev_get_drvdata(dev);
+	const struct venus_resources *res = core->res;
+	unsigned int i;
+
+	if (!res->vcodec_pmdomains_num)
+		goto skip_pmdomains;
+
+	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);
+	}
+
+skip_pmdomains:
+	if (!core->has_opp_table)
+		return;
+
+	if (core->opp_dl_venus)
+		device_link_del(core->opp_dl_venus);
+
+	dev_pm_domain_detach(core->opp_pmdomain, true);
+}
+
+
 static u32 load_per_instance(struct venus_inst *inst)
 {
 	u32 mbs;
@@ -308,19 +402,17 @@  vcodec_control_v3(struct venus_core *core, u32 session_type, bool enable)
 		writel(1, ctrl);
 }
 
-static int vdec_get_v3(struct device *dev)
-{
-	struct venus_core *core = dev_get_drvdata(dev);
-
-	return vcodec_clks_get(core, dev, core->vcodec0_clks,
-			       core->res->vcodec0_clks);
-}
-
 static int vdec_power_v3(struct device *dev, int on)
 {
 	struct venus_core *core = dev_get_drvdata(dev);
 	int ret = 0;
 
+	if (!legacy_binding && on == POWER_ON) {
+		ret = pm_runtime_get_sync(core->pmdomains[1]);
+		if (ret < 0)
+			return ret;
+	}
+
 	vcodec_control_v3(core, VIDC_SESSION_TYPE_DEC, true);
 
 	if (on == POWER_ON)
@@ -330,40 +422,202 @@  static int vdec_power_v3(struct device *dev, int on)
 
 	vcodec_control_v3(core, VIDC_SESSION_TYPE_DEC, false);
 
-	return ret;
-}
-
-static int venc_get_v3(struct device *dev)
-{
-	struct venus_core *core = dev_get_drvdata(dev);
+	if (!legacy_binding && on == POWER_OFF) {
+		ret = pm_runtime_put_sync(core->pmdomains[1]);
+		if (ret < 0)
+			return ret;
+	}
 
-	return vcodec_clks_get(core, dev, core->vcodec1_clks,
-			       core->res->vcodec1_clks);
+	return ret;
 }
 
 static int venc_power_v3(struct device *dev, int on)
 {
 	struct venus_core *core = dev_get_drvdata(dev);
-	int ret = 0;
+	const struct venus_resources *res = core->res;
+	struct clk **vcodec_clks;
+	int idx, ret = 0;
+
+	/*
+	 * Some SoCs have got only one vcodec instance and all of them
+	 * have got one global and one pmdomain per instance.
+	 */
+	if (res->vcodec_num > 1) {
+		vcodec_clks = core->vcodec1_clks;
+		idx = 2;
+	} else {
+		vcodec_clks = core->vcodec0_clks;
+		idx = 1;
+	}
+
+	if (!legacy_binding && on == POWER_ON) {
+		ret = pm_runtime_get_sync(core->pmdomains[idx]);
+		if (ret < 0)
+			return ret;
+	}
 
 	vcodec_control_v3(core, VIDC_SESSION_TYPE_ENC, true);
 
 	if (on == POWER_ON)
-		ret = vcodec_clks_enable(core, core->vcodec1_clks);
+		ret = vcodec_clks_enable(core, vcodec_clks);
 	else
-		vcodec_clks_disable(core, core->vcodec1_clks);
+		vcodec_clks_disable(core, vcodec_clks);
 
 	vcodec_control_v3(core, VIDC_SESSION_TYPE_ENC, false);
 
+	if (!legacy_binding && on == POWER_OFF) {
+		ret = pm_runtime_put_sync(core->pmdomains[idx]);
+		if (ret < 0)
+			return ret;
+	}
+
+	return ret;
+}
+
+static int core_get_v4(struct device *dev)
+{
+	struct venus_core *core = dev_get_drvdata(dev);
+	const struct venus_resources *res = core->res;
+	int ret;
+
+	ret = core_clks_get(core);
+	if (ret)
+		return ret;
+
+	if (!res->vcodec_pmdomains_num)
+		legacy_binding = true;
+
+	dev_info(dev, "%s legacy binding\n", legacy_binding ? "" : "non");
+
+	ret = vcodec_clks_get(core, dev, core->vcodec0_clks, res->vcodec0_clks);
+	if (ret)
+		return ret;
+
+	if (core->vcodec1_clks) {
+		ret = vcodec_clks_get(core, dev,
+				      core->vcodec1_clks, res->vcodec1_clks);
+		if (ret)
+			return ret;
+	}
+
+	if (legacy_binding)
+		return 0;
+
+	core->opp_table = dev_pm_opp_set_clkname(dev, "core");
+	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) {
+			dev_err(dev, "invalid OPP table in device tree\n");
+			dev_pm_opp_put_clkname(core->opp_table);
+			return ret;
+		}
+	}
+
+	ret = vcodec_domains_get(dev);
+	if (ret) {
+		if (core->has_opp_table)
+			dev_pm_opp_of_remove_table(dev);
+		dev_pm_opp_put_clkname(core->opp_table);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void core_put_v4(struct device *dev)
+{
+	struct venus_core *core = dev_get_drvdata(dev);
+
+	if (legacy_binding)
+		return;
+
+	vcodec_domains_put(dev);
+
+	if (core->has_opp_table)
+		dev_pm_opp_of_remove_table(dev);
+	if (core->opp_table)
+		dev_pm_opp_put_clkname(core->opp_table);
+
+}
+
+static int core_power_v4(struct device *dev, int on)
+{
+	struct venus_core *core = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (on == POWER_ON) {
+		ret = core_clks_enable(core);
+	} else {
+		/* Drop the performance state vote */
+		if (core->opp_pmdomain)
+			dev_pm_opp_set_rate(dev, 0);
+
+		core_clks_disable(core);
+	}
+
 	return ret;
 }
 
+static int vdec_get_v4(struct device *dev)
+{
+	struct venus_core *core = dev_get_drvdata(dev);
+
+	if (!legacy_binding)
+		return 0;
+
+	return vcodec_clks_get(core, dev, core->vcodec0_clks,
+			       core->res->vcodec0_clks);
+}
+
+static void vdec_put_v4(struct device *dev)
+{
+	struct venus_core *core = dev_get_drvdata(dev);
+	unsigned int i;
+
+	if (!legacy_binding)
+		return;
+
+	for (i = 0; i < core->res->vcodec_clks_num; i++)
+		core->vcodec0_clks[i] = NULL;
+}
+
+static int venc_get_v4(struct device *dev)
+{
+	struct venus_core *core = dev_get_drvdata(dev);
+
+	if (!legacy_binding)
+		return 0;
+
+	return vcodec_clks_get(core, dev, core->vcodec1_clks,
+			       core->res->vcodec1_clks);
+}
+
+static void venc_put_v4(struct device *dev)
+{
+	struct venus_core *core = dev_get_drvdata(dev);
+	unsigned int i;
+
+	if (!legacy_binding)
+		return;
+
+	for (i = 0; i < core->res->vcodec_clks_num; i++)
+		core->vcodec1_clks[i] = NULL;
+}
+
 static const struct venus_pm_ops pm_ops_v3 = {
-	.core_get = core_get_v1,
-	.core_power = core_power_v1,
-	.vdec_get = vdec_get_v3,
+	.core_get = core_get_v4,
+	.core_put = core_put_v4,
+	.core_power = core_power_v4,
+	.vdec_get = vdec_get_v4,
+	.vdec_put = vdec_put_v4,
 	.vdec_power = vdec_power_v3,
-	.venc_get = venc_get_v3,
+	.venc_get = venc_get_v4,
+	.venc_put = venc_put_v4,
 	.venc_power = venc_power_v3,
 	.load_scale = load_scale_v1,
 };
@@ -652,29 +906,6 @@  static int coreid_power_v4(struct venus_inst *inst, int on)
 	return ret;
 }
 
-static int vdec_get_v4(struct device *dev)
-{
-	struct venus_core *core = dev_get_drvdata(dev);
-
-	if (!legacy_binding)
-		return 0;
-
-	return vcodec_clks_get(core, dev, core->vcodec0_clks,
-			       core->res->vcodec0_clks);
-}
-
-static void vdec_put_v4(struct device *dev)
-{
-	struct venus_core *core = dev_get_drvdata(dev);
-	unsigned int i;
-
-	if (!legacy_binding)
-		return;
-
-	for (i = 0; i < core->res->vcodec_clks_num; i++)
-		core->vcodec0_clks[i] = NULL;
-}
-
 static int vdec_power_v4(struct device *dev, int on)
 {
 	struct venus_core *core = dev_get_drvdata(dev);
@@ -697,29 +928,6 @@  static int vdec_power_v4(struct device *dev, int on)
 	return ret;
 }
 
-static int venc_get_v4(struct device *dev)
-{
-	struct venus_core *core = dev_get_drvdata(dev);
-
-	if (!legacy_binding)
-		return 0;
-
-	return vcodec_clks_get(core, dev, core->vcodec1_clks,
-			       core->res->vcodec1_clks);
-}
-
-static void venc_put_v4(struct device *dev)
-{
-	struct venus_core *core = dev_get_drvdata(dev);
-	unsigned int i;
-
-	if (!legacy_binding)
-		return;
-
-	for (i = 0; i < core->res->vcodec_clks_num; i++)
-		core->vcodec1_clks[i] = NULL;
-}
-
 static int venc_power_v4(struct device *dev, int on)
 {
 	struct venus_core *core = dev_get_drvdata(dev);
@@ -742,185 +950,6 @@  static int venc_power_v4(struct device *dev, int on)
 	return ret;
 }
 
-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)
-		goto skip_pmdomains;
-
-	for (i = 0; i < res->vcodec_pmdomains_num; i++) {
-		pd = dev_pm_domain_attach_by_name(dev,
-						  res->vcodec_pmdomains[i]);
-		if (IS_ERR(pd))
-			return PTR_ERR(pd);
-		core->pmdomains[i] = pd;
-	}
-
-	core->pd_dl_venus = device_link_add(dev, core->pmdomains[0],
-					    DL_FLAG_PM_RUNTIME |
-					    DL_FLAG_STATELESS |
-					    DL_FLAG_RPM_ACTIVE);
-	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)
-{
-	struct venus_core *core = dev_get_drvdata(dev);
-	const struct venus_resources *res = core->res;
-	unsigned int i;
-
-	if (!res->vcodec_pmdomains_num)
-		goto skip_pmdomains;
-
-	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);
-	}
-
-skip_pmdomains:
-	if (!core->has_opp_table)
-		return;
-
-	if (core->opp_dl_venus)
-		device_link_del(core->opp_dl_venus);
-
-	dev_pm_domain_detach(core->opp_pmdomain, true);
-}
-
-static int core_get_v4(struct device *dev)
-{
-	struct venus_core *core = dev_get_drvdata(dev);
-	const struct venus_resources *res = core->res;
-	int ret;
-
-	ret = core_clks_get(core);
-	if (ret)
-		return ret;
-
-	if (!res->vcodec_pmdomains_num)
-		legacy_binding = true;
-
-	dev_info(dev, "%s legacy binding\n", legacy_binding ? "" : "non");
-
-	ret = vcodec_clks_get(core, dev, core->vcodec0_clks, res->vcodec0_clks);
-	if (ret)
-		return ret;
-
-	ret = vcodec_clks_get(core, dev, core->vcodec1_clks, res->vcodec1_clks);
-	if (ret)
-		return ret;
-
-	if (legacy_binding)
-		return 0;
-
-	core->opp_table = dev_pm_opp_set_clkname(dev, "core");
-	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) {
-			dev_err(dev, "invalid OPP table in device tree\n");
-			dev_pm_opp_put_clkname(core->opp_table);
-			return ret;
-		}
-	}
-
-	ret = vcodec_domains_get(dev);
-	if (ret) {
-		if (core->has_opp_table)
-			dev_pm_opp_of_remove_table(dev);
-		dev_pm_opp_put_clkname(core->opp_table);
-		return ret;
-	}
-
-	return 0;
-}
-
-static void core_put_v4(struct device *dev)
-{
-	struct venus_core *core = dev_get_drvdata(dev);
-
-	if (legacy_binding)
-		return;
-
-	vcodec_domains_put(dev);
-
-	if (core->has_opp_table)
-		dev_pm_opp_of_remove_table(dev);
-	if (core->opp_table)
-		dev_pm_opp_put_clkname(core->opp_table);
-
-}
-
-static int core_power_v4(struct device *dev, int on)
-{
-	struct venus_core *core = dev_get_drvdata(dev);
-	int ret = 0;
-
-	if (on == POWER_ON) {
-		ret = core_clks_enable(core);
-	} else {
-		/* Drop the performance state vote */
-		if (core->opp_pmdomain)
-			dev_pm_opp_set_rate(dev, 0);
-
-		core_clks_disable(core);
-	}
-
-	return ret;
-}
-
 static unsigned long calculate_inst_freq(struct venus_inst *inst,
 					 unsigned long filled_len)
 {