linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/20] Venus cleanups
@ 2024-02-09 21:09 Konrad Dybcio
  2024-02-09 21:09 ` [PATCH v2 01/20] media: venus: pm_helpers: Only set rate of the core clock in core_clks_enable Konrad Dybcio
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Konrad Dybcio @ 2024-02-09 21:09 UTC (permalink / raw)
  To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue,
	Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab,
	Dikshita Agarwal, Philipp Zabel
  Cc: Marijn Suijten, Stanimir Varbanov, Mauro Carvalho Chehab,
	linux-media, linux-arm-msm, linux-kernel, Konrad Dybcio

With the driver supporting multiple generations of hardware, some mold
has definitely grown over the code..

This series attempts to amend this situation a bit by commonizing some
code paths and fixing some bugs while at it.

Only tested on SM8250.

Definitely needs testing on:

- SDM845 with old bindings
- SDM845 with new bindings or 7180
- MSM8916
- MSM8996

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Changes in v2:
- Fix "set but unused" warning in "Drop cache properties in resource struct"
- Fix modular build with "Commonize vdec_get()"
- Rebase
- Test again on 8250, since nobody else tested other platforms since the last
  submission (or at least hasn't reported that), I'm assuming nobody cares
- Needs to be tested atop [1] and similar, it's in latest -next already
- Link to v1: https://lore.kernel.org/r/20230911-topic-mars-v1-0-a7d38bf87bdb@linaro.org

[1] https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/commit/?h=for-next&id=d2cd22c9c384aa50c0b4530e842bd078427e6279

---
Konrad Dybcio (20):
      media: venus: pm_helpers: Only set rate of the core clock in core_clks_enable
      media: venus: pm_helpers: Rename core_clks_get to venus_clks_get
      media: venus: pm_helpers: Add kerneldoc to venus_clks_get()
      media: venus: core: Set OPP clkname in a common code path
      media: venus: pm_helpers: Kill dead code
      media: venus: pm_helpers: Move reset acquisition to common code
      media: venus: core: Constify all members of the resource struct
      media: venus: core: Deduplicate OPP genpd names
      media: venus: core: Get rid of vcodec_num
      media: venus: core: Drop cache properties in resource struct
      media: venus: core: Use GENMASK for dma_mask
      media: venus: core: Remove cp_start
      media: venus: pm_helpers: Commonize core_power
      media: venus: pm_helpers: Remove pm_ops->core_put
      media: venus: core: Define a pointer to core->res
      media: venus: pm_helpers: Simplify vcodec clock handling
      media: venus: pm_helpers: Commonize getting clocks and GenPDs
      media: venus: pm_helpers: Commonize vdec_get()
      media: venus: pm_helpers: Commonize venc_get()
      media: venus: pm_helpers: Use reset_bulk API

 drivers/media/platform/qcom/venus/core.c       | 139 ++++-------
 drivers/media/platform/qcom/venus/core.h       |  66 +++--
 drivers/media/platform/qcom/venus/firmware.c   |   3 +-
 drivers/media/platform/qcom/venus/hfi_venus.c  |  10 +-
 drivers/media/platform/qcom/venus/pm_helpers.c | 323 +++++++++----------------
 drivers/media/platform/qcom/venus/pm_helpers.h |  10 +-
 drivers/media/platform/qcom/venus/vdec.c       |   9 +-
 drivers/media/platform/qcom/venus/venc.c       |   9 +-
 8 files changed, 213 insertions(+), 356 deletions(-)
---
base-commit: 445a555e0623387fa9b94e68e61681717e70200a
change-id: 20230911-topic-mars-e60bb2269411

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 01/20] media: venus: pm_helpers: Only set rate of the core clock in core_clks_enable
  2024-02-09 21:09 [PATCH v2 00/20] Venus cleanups Konrad Dybcio
@ 2024-02-09 21:09 ` Konrad Dybcio
  2024-02-09 21:09 ` [PATCH v2 02/20] media: venus: pm_helpers: Rename core_clks_get to venus_clks_get Konrad Dybcio
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Konrad Dybcio @ 2024-02-09 21:09 UTC (permalink / raw)
  To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue,
	Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab,
	Dikshita Agarwal, Philipp Zabel
  Cc: Marijn Suijten, Stanimir Varbanov, Mauro Carvalho Chehab,
	linux-media, linux-arm-msm, linux-kernel, Konrad Dybcio

Commit c22b1a29497c ("media: venus: core,pm: Vote for min clk freq
during venus boot") intended to up the rate of the Venus core clock
from the XO minimum to something more reasonable, based on the per-
SoC frequency table.

Unfortunately, it ended up calling set_rate with that same argument
on all clocks in res->clks. Fix that using the OPP API.

Fixes: c22b1a29497c ("media: venus: core,pm: Vote for min clk freq during venus boot")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 502822059498..8bd0ce4ce69d 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -41,24 +41,23 @@ static int core_clks_get(struct venus_core *core)
 static int core_clks_enable(struct venus_core *core)
 {
 	const struct venus_resources *res = core->res;
-	const struct freq_tbl *freq_tbl = core->res->freq_tbl;
-	unsigned int freq_tbl_size = core->res->freq_tbl_size;
-	unsigned long freq;
+	struct dev_pm_opp *opp;
+	unsigned long freq = 0;
 	unsigned int i;
 	int ret;
 
-	if (!freq_tbl)
-		return -EINVAL;
+	if (core->has_opp_table) {
+		opp = dev_pm_opp_find_freq_ceil(core->dev, &freq);
+		if (IS_ERR(opp))
+			return PTR_ERR(opp);
+		dev_pm_opp_put(opp);
 
-	freq = freq_tbl[freq_tbl_size - 1].freq;
+		ret = dev_pm_opp_set_rate(core->dev, freq);
+		if (ret)
+			return ret;
+	}
 
 	for (i = 0; i < res->clks_num; i++) {
-		if (IS_V6(core)) {
-			ret = clk_set_rate(core->clks[i], freq);
-			if (ret)
-				goto err;
-		}
-
 		ret = clk_prepare_enable(core->clks[i]);
 		if (ret)
 			goto err;

-- 
2.43.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 02/20] media: venus: pm_helpers: Rename core_clks_get to venus_clks_get
  2024-02-09 21:09 [PATCH v2 00/20] Venus cleanups Konrad Dybcio
  2024-02-09 21:09 ` [PATCH v2 01/20] media: venus: pm_helpers: Only set rate of the core clock in core_clks_enable Konrad Dybcio
@ 2024-02-09 21:09 ` Konrad Dybcio
  2024-02-09 21:09 ` [PATCH v2 03/20] media: venus: pm_helpers: Add kerneldoc to venus_clks_get() Konrad Dybcio
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Konrad Dybcio @ 2024-02-09 21:09 UTC (permalink / raw)
  To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue,
	Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab,
	Dikshita Agarwal, Philipp Zabel
  Cc: Marijn Suijten, Stanimir Varbanov, Mauro Carvalho Chehab,
	linux-media, linux-arm-msm, linux-kernel, Konrad Dybcio

"core" is used in multiple contexts when talking about Venus, rename
the function to save on confusion.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 8bd0ce4ce69d..ac7c83404c6e 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -23,7 +23,7 @@
 
 static bool legacy_binding;
 
-static int core_clks_get(struct venus_core *core)
+static int venus_clks_get(struct venus_core *core)
 {
 	const struct venus_resources *res = core->res;
 	struct device *dev = core->dev;
@@ -294,7 +294,7 @@ static int core_get_v1(struct venus_core *core)
 {
 	int ret;
 
-	ret = core_clks_get(core);
+	ret = venus_clks_get(core);
 	if (ret)
 		return ret;
 
@@ -961,7 +961,7 @@ static int core_get_v4(struct venus_core *core)
 	const struct venus_resources *res = core->res;
 	int ret;
 
-	ret = core_clks_get(core);
+	ret = venus_clks_get(core);
 	if (ret)
 		return ret;
 

-- 
2.43.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 03/20] media: venus: pm_helpers: Add kerneldoc to venus_clks_get()
  2024-02-09 21:09 [PATCH v2 00/20] Venus cleanups Konrad Dybcio
  2024-02-09 21:09 ` [PATCH v2 01/20] media: venus: pm_helpers: Only set rate of the core clock in core_clks_enable Konrad Dybcio
  2024-02-09 21:09 ` [PATCH v2 02/20] media: venus: pm_helpers: Rename core_clks_get to venus_clks_get Konrad Dybcio
@ 2024-02-09 21:09 ` Konrad Dybcio
  2024-02-09 21:09 ` [PATCH v2 04/20] media: venus: core: Set OPP clkname in a common code path Konrad Dybcio
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Konrad Dybcio @ 2024-02-09 21:09 UTC (permalink / raw)
  To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue,
	Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab,
	Dikshita Agarwal, Philipp Zabel
  Cc: Marijn Suijten, Stanimir Varbanov, Mauro Carvalho Chehab,
	linux-media, linux-arm-msm, linux-kernel, Konrad Dybcio

To make it easier to understand the various clock requirements within
this driver, add kerneldoc to venus_clk_get() explaining the fluff.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index ac7c83404c6e..ea0a7d4601e2 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -23,6 +23,34 @@
 
 static bool legacy_binding;
 
+/**
+ * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
+ * @core: A pointer to the venus core resource
+ *
+ * The Venus block (depending on the generation) can be split into a couple
+ * of clock domains: one for "main logic" and one for each video core (0-2pcs).
+ *
+ * MSM8916 (and possibly other HFIv1 users) only feature the "main logic"
+ * domain, so this function is the only kind if clk_get necessary there.
+ *
+ * MSM8996 (and other HFIv3 users) feature two video cores, with core0 being
+ * statically proclaimed a decoder and core1 an encoder, with both having
+ * their own clock domains.
+ *
+ * SDM845 features two video cores, each one of which may or may not be
+ * subdivided into 2 enc/dec threads.
+ *
+ * Other SoCs either feature a single video core (with its own clock domain)
+ * or 1 video core and 1 CVP (Computer Vision Processor) core. In both cases
+ * we treat it the same (CVP only happens to live near-by Venus on the SoC).
+ *
+ * Due to unfortunate developments in the past, we have to support bindings
+ * (MSM8996, SDM660, SDM845) that require specifying the clocks and
+ * power-domains associated with a video core domain in a bogus subnode,
+ * which means that additional fluff is necessary..
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
 static int venus_clks_get(struct venus_core *core)
 {
 	const struct venus_resources *res = core->res;

-- 
2.43.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 04/20] media: venus: core: Set OPP clkname in a common code path
  2024-02-09 21:09 [PATCH v2 00/20] Venus cleanups Konrad Dybcio
                   ` (2 preceding siblings ...)
  2024-02-09 21:09 ` [PATCH v2 03/20] media: venus: pm_helpers: Add kerneldoc to venus_clks_get() Konrad Dybcio
@ 2024-02-09 21:09 ` Konrad Dybcio
  2024-02-09 21:09 ` [PATCH v2 05/20] media: venus: pm_helpers: Kill dead code Konrad Dybcio
  2024-02-09 21:10 ` [PATCH v2 00/20] Venus cleanups Konrad Dybcio
  5 siblings, 0 replies; 12+ messages in thread
From: Konrad Dybcio @ 2024-02-09 21:09 UTC (permalink / raw)
  To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue,
	Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab,
	Dikshita Agarwal, Philipp Zabel
  Cc: Marijn Suijten, Stanimir Varbanov, Mauro Carvalho Chehab,
	linux-media, linux-arm-msm, linux-kernel, Konrad Dybcio

Calling devm_pm_opp_set_clkname() is repeated for all HFI versions in
pm_ops->core_power.

Move it to the common codepath.

This also lets us get rid of core_get_v1.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c       |  5 +++++
 drivers/media/platform/qcom/venus/pm_helpers.c | 23 ++---------------------
 2 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index ce206b709754..5ab3c414ec0f 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -14,6 +14,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/pm_domain.h>
@@ -319,6 +320,10 @@ static int venus_probe(struct platform_device *pdev)
 	if (!core->pm_ops)
 		return -ENODEV;
 
+	ret = devm_pm_opp_set_clkname(dev, "core");
+	if (ret)
+		return ret;
+
 	if (core->pm_ops->core_get) {
 		ret = core->pm_ops->core_get(core);
 		if (ret)
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index ea0a7d4601e2..1ba65345a5e2 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -318,21 +318,6 @@ static int load_scale_v1(struct venus_inst *inst)
 	return ret;
 }
 
-static int core_get_v1(struct venus_core *core)
-{
-	int ret;
-
-	ret = venus_clks_get(core);
-	if (ret)
-		return ret;
-
-	ret = devm_pm_opp_set_clkname(core->dev, "core");
-	if (ret)
-		return ret;
-
-	return 0;
-}
-
 static void core_put_v1(struct venus_core *core)
 {
 }
@@ -350,7 +335,7 @@ static int core_power_v1(struct venus_core *core, int on)
 }
 
 static const struct venus_pm_ops pm_ops_v1 = {
-	.core_get = core_get_v1,
+	.core_get = venus_clks_get,
 	.core_put = core_put_v1,
 	.core_power = core_power_v1,
 	.load_scale = load_scale_v1,
@@ -423,7 +408,7 @@ static int venc_power_v3(struct device *dev, int on)
 }
 
 static const struct venus_pm_ops pm_ops_v3 = {
-	.core_get = core_get_v1,
+	.core_get = venus_clks_get,
 	.core_put = core_put_v1,
 	.core_power = core_power_v1,
 	.vdec_get = vdec_get_v3,
@@ -1013,10 +998,6 @@ static int core_get_v4(struct venus_core *core)
 	if (legacy_binding)
 		return 0;
 
-	ret = devm_pm_opp_set_clkname(dev, "core");
-	if (ret)
-		return ret;
-
 	ret = vcodec_domains_get(core);
 	if (ret)
 		return ret;

-- 
2.43.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 05/20] media: venus: pm_helpers: Kill dead code
  2024-02-09 21:09 [PATCH v2 00/20] Venus cleanups Konrad Dybcio
                   ` (3 preceding siblings ...)
  2024-02-09 21:09 ` [PATCH v2 04/20] media: venus: core: Set OPP clkname in a common code path Konrad Dybcio
@ 2024-02-09 21:09 ` Konrad Dybcio
  2024-02-09 21:10 ` [PATCH v2 00/20] Venus cleanups Konrad Dybcio
  5 siblings, 0 replies; 12+ messages in thread
From: Konrad Dybcio @ 2024-02-09 21:09 UTC (permalink / raw)
  To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue,
	Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab,
	Dikshita Agarwal, Philipp Zabel
  Cc: Marijn Suijten, Stanimir Varbanov, Mauro Carvalho Chehab,
	linux-media, linux-arm-msm, linux-kernel, Konrad Dybcio

A situation like:

if (!foo)
	goto bar;

for (i = 0; i < foo; i++)
	...1...

bar:
	...2...

is totally identical to:

for (i = 0; i < 0; i++) // === if (0)
	...1...

...2...

Get rid of such boilerplate.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 1ba65345a5e2..7193075e8c04 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -878,14 +878,10 @@ static int vcodec_domains_get(struct venus_core *core)
 		.pd_flags = PD_FLAG_NO_DEV_LINK,
 	};
 
-	if (!res->vcodec_pmdomains_num)
-		goto skip_pmdomains;
-
 	ret = dev_pm_domain_attach_list(dev, &vcodec_data, &core->pmdomains);
 	if (ret < 0)
 		return ret;
 
-skip_pmdomains:
 	if (!core->res->opp_pmdomain)
 		return 0;
 
@@ -928,9 +924,6 @@ static int core_resets_reset(struct venus_core *core)
 	unsigned int i;
 	int ret;
 
-	if (!res->resets_num)
-		return 0;
-
 	for (i = 0; i < res->resets_num; i++) {
 		ret = reset_control_assert(core->resets[i]);
 		if (ret)
@@ -953,9 +946,6 @@ static int core_resets_get(struct venus_core *core)
 	unsigned int i;
 	int ret;
 
-	if (!res->resets_num)
-		return 0;
-
 	for (i = 0; i < res->resets_num; i++) {
 		core->resets[i] =
 			devm_reset_control_get_exclusive(dev, res->resets[i]);

-- 
2.43.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 03/20] media: venus: pm_helpers: Add kerneldoc to venus_clks_get()
  2024-02-09 21:09 Konrad Dybcio
@ 2024-02-09 21:09 ` Konrad Dybcio
  2024-03-06 12:20   ` Bryan O'Donoghue
  0 siblings, 1 reply; 12+ messages in thread
From: Konrad Dybcio @ 2024-02-09 21:09 UTC (permalink / raw)
  To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue,
	Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab,
	Dikshita Agarwal, Philipp Zabel
  Cc: Marijn Suijten, Stanimir Varbanov, Mauro Carvalho Chehab,
	linux-media, linux-arm-msm, linux-kernel, Konrad Dybcio

To make it easier to understand the various clock requirements within
this driver, add kerneldoc to venus_clk_get() explaining the fluff.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index ac7c83404c6e..ea0a7d4601e2 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -23,6 +23,34 @@
 
 static bool legacy_binding;
 
+/**
+ * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
+ * @core: A pointer to the venus core resource
+ *
+ * The Venus block (depending on the generation) can be split into a couple
+ * of clock domains: one for "main logic" and one for each video core (0-2pcs).
+ *
+ * MSM8916 (and possibly other HFIv1 users) only feature the "main logic"
+ * domain, so this function is the only kind if clk_get necessary there.
+ *
+ * MSM8996 (and other HFIv3 users) feature two video cores, with core0 being
+ * statically proclaimed a decoder and core1 an encoder, with both having
+ * their own clock domains.
+ *
+ * SDM845 features two video cores, each one of which may or may not be
+ * subdivided into 2 enc/dec threads.
+ *
+ * Other SoCs either feature a single video core (with its own clock domain)
+ * or 1 video core and 1 CVP (Computer Vision Processor) core. In both cases
+ * we treat it the same (CVP only happens to live near-by Venus on the SoC).
+ *
+ * Due to unfortunate developments in the past, we have to support bindings
+ * (MSM8996, SDM660, SDM845) that require specifying the clocks and
+ * power-domains associated with a video core domain in a bogus subnode,
+ * which means that additional fluff is necessary..
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
 static int venus_clks_get(struct venus_core *core)
 {
 	const struct venus_resources *res = core->res;

-- 
2.43.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 00/20] Venus cleanups
  2024-02-09 21:09 [PATCH v2 00/20] Venus cleanups Konrad Dybcio
                   ` (4 preceding siblings ...)
  2024-02-09 21:09 ` [PATCH v2 05/20] media: venus: pm_helpers: Kill dead code Konrad Dybcio
@ 2024-02-09 21:10 ` Konrad Dybcio
  5 siblings, 0 replies; 12+ messages in thread
From: Konrad Dybcio @ 2024-02-09 21:10 UTC (permalink / raw)
  To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue,
	Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab,
	Dikshita Agarwal, Philipp Zabel
  Cc: Marijn Suijten, Stanimir Varbanov, Mauro Carvalho Chehab,
	linux-media, linux-arm-msm, linux-kernel

On 9.02.2024 22:09, Konrad Dybcio wrote:
> With the driver supporting multiple generations of hardware, some mold
> has definitely grown over the code..
> 
> This series attempts to amend this situation a bit by commonizing some
> code paths and fixing some bugs while at it.
> 
> Only tested on SM8250.
> 
> Definitely needs testing on:
> 
> - SDM845 with old bindings
> - SDM845 with new bindings or 7180
> - MSM8916
> - MSM8996
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---

Apologies for sending this twice. The other submission should be looked at.

Konrad

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 03/20] media: venus: pm_helpers: Add kerneldoc to venus_clks_get()
  2024-02-09 21:09 ` [PATCH v2 03/20] media: venus: pm_helpers: Add kerneldoc to venus_clks_get() Konrad Dybcio
@ 2024-03-06 12:20   ` Bryan O'Donoghue
  2024-03-26 21:23     ` Konrad Dybcio
  0 siblings, 1 reply; 12+ messages in thread
From: Bryan O'Donoghue @ 2024-03-06 12:20 UTC (permalink / raw)
  To: Konrad Dybcio, Stanimir Varbanov, Vikash Garodia,
	Bryan O'Donoghue, Andy Gross, Bjorn Andersson,
	Mauro Carvalho Chehab, Dikshita Agarwal, Philipp Zabel
  Cc: Marijn Suijten, Stanimir Varbanov, Mauro Carvalho Chehab,
	linux-media, linux-arm-msm, linux-kernel

On 09/02/2024 21:09, Konrad Dybcio wrote:
> To make it easier to understand the various clock requirements within
> this driver, add kerneldoc to venus_clk_get() explaining the fluff.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index ac7c83404c6e..ea0a7d4601e2 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -23,6 +23,34 @@
>   
>   static bool legacy_binding;
>   
> +/**
> + * venus_clks_get() - Get Venus clocks that are not bound to a vcodec

Get non-codec Venus clocks.

> + * @core: A pointer to the venus core resource
> + *
> + * The Venus block (depending on the generation) can be split into a couple
> + * of clock domains: one for "main logic" and one for each video core (0-2pcs).

(0-2pcs) is hard for me to decode => zero to two parts?

Why are we double quoting "main logic" feels a bit "Dr Evil"

Suggest hyphenating which would do the same thing:

'one clock for the core-logic||main-logic'

> + *
> + * MSM8916 (and possibly other HFIv1 users) only feature the "main logic"
> + * domain, so this function is the only kind if clk_get necessary there.
> + *
> + * MSM8996 (and other HFIv3 users) feature two video cores, with core0 being
> + * statically proclaimed a decoder and core1 an encoder, with both having
> + * their own clock domains.

"statically defined" not "statically proclaimed"

> + *
> + * SDM845 features two video cores, each one of which may or may not be
> + * subdivided into 2 enc/dec threads.

"into two encoder/decoder threads."


> + *
> + * Other SoCs either feature a single video core (with its own clock domain)
> + * or 1 video core and 1 CVP (Computer Vision Processor) core. In both cases
> + * we treat it the same (CVP only happens to live near-by Venus on the SoC).

One not 1

> + *
> + * Due to unfortunate developments in the past, we have to support bindings
> + * (MSM8996, SDM660, SDM845) that require specifying the clocks and
> + * power-domains associated with a video core domain in a bogus subnode,
> + * which means that additional fluff is necessary..

"We need to support legacy bindings"

"sub-node"

> + *
> + * Return: 0 on success, negative errno on failure.
> + */
>   static int venus_clks_get(struct venus_core *core)
>   {
>   	const struct venus_resources *res = core->res;
> 

With that fixed.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 03/20] media: venus: pm_helpers: Add kerneldoc to venus_clks_get()
  2024-03-06 12:20   ` Bryan O'Donoghue
@ 2024-03-26 21:23     ` Konrad Dybcio
  2024-03-27  9:55       ` Bryan O'Donoghue
  0 siblings, 1 reply; 12+ messages in thread
From: Konrad Dybcio @ 2024-03-26 21:23 UTC (permalink / raw)
  To: Bryan O'Donoghue, Stanimir Varbanov, Vikash Garodia,
	Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab,
	Dikshita Agarwal, Philipp Zabel
  Cc: Marijn Suijten, Stanimir Varbanov, Mauro Carvalho Chehab,
	linux-media, linux-arm-msm, linux-kernel

On 6.03.2024 1:20 PM, Bryan O'Donoghue wrote:
> On 09/02/2024 21:09, Konrad Dybcio wrote:
>> To make it easier to understand the various clock requirements within
>> this driver, add kerneldoc to venus_clk_get() explaining the fluff.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index ac7c83404c6e..ea0a7d4601e2 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -23,6 +23,34 @@
>>     static bool legacy_binding;
>>   +/**
>> + * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
> 
> Get non-codec Venus clocks.

No, this is not necessarily the case.. these may still include
vcodec clocks, that are specified under the root venus node (which
is the only way we'd like to keep)

I applied the rest of your suggestions, do I keep your rb?

Konrad

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 03/20] media: venus: pm_helpers: Add kerneldoc to venus_clks_get()
  2024-03-26 21:23     ` Konrad Dybcio
@ 2024-03-27  9:55       ` Bryan O'Donoghue
  2024-03-27 17:23         ` Konrad Dybcio
  0 siblings, 1 reply; 12+ messages in thread
From: Bryan O'Donoghue @ 2024-03-27  9:55 UTC (permalink / raw)
  To: Konrad Dybcio, Bryan O'Donoghue, Stanimir Varbanov,
	Vikash Garodia, Andy Gross, Bjorn Andersson,
	Mauro Carvalho Chehab, Dikshita Agarwal, Philipp Zabel
  Cc: Marijn Suijten, Stanimir Varbanov, Mauro Carvalho Chehab,
	linux-media, linux-arm-msm, linux-kernel

On 26/03/2024 21:23, Konrad Dybcio wrote:
> On 6.03.2024 1:20 PM, Bryan O'Donoghue wrote:
>> On 09/02/2024 21:09, Konrad Dybcio wrote:
>>> To make it easier to understand the various clock requirements within
>>> this driver, add kerneldoc to venus_clk_get() explaining the fluff.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>    drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
>>>    1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>>> index ac7c83404c6e..ea0a7d4601e2 100644
>>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>>> @@ -23,6 +23,34 @@
>>>      static bool legacy_binding;
>>>    +/**
>>> + * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
>>
>> Get non-codec Venus clocks.
> 
> No, this is not necessarily the case.. these may still include
> vcodec clocks, that are specified under the root venus node (which
> is the only way we'd like to keep)
> 
> I applied the rest of your suggestions, do I keep your rb?
> 
> Konrad
> 

Sure

BTW, I plan to test this series when I can - do you have a working tree ?

---
bod

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 03/20] media: venus: pm_helpers: Add kerneldoc to venus_clks_get()
  2024-03-27  9:55       ` Bryan O'Donoghue
@ 2024-03-27 17:23         ` Konrad Dybcio
  0 siblings, 0 replies; 12+ messages in thread
From: Konrad Dybcio @ 2024-03-27 17:23 UTC (permalink / raw)
  To: Bryan O'Donoghue, Bryan O'Donoghue, Stanimir Varbanov,
	Vikash Garodia, Andy Gross, Bjorn Andersson,
	Mauro Carvalho Chehab, Dikshita Agarwal, Philipp Zabel
  Cc: Marijn Suijten, Stanimir Varbanov, Mauro Carvalho Chehab,
	linux-media, linux-arm-msm, linux-kernel

On 27.03.2024 10:55 AM, Bryan O'Donoghue wrote:
> On 26/03/2024 21:23, Konrad Dybcio wrote:
>> On 6.03.2024 1:20 PM, Bryan O'Donoghue wrote:
>>> On 09/02/2024 21:09, Konrad Dybcio wrote:
>>>> To make it easier to understand the various clock requirements within
>>>> this driver, add kerneldoc to venus_clk_get() explaining the fluff.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>    drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
>>>>    1 file changed, 28 insertions(+)
>>>>
>>>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>>>> index ac7c83404c6e..ea0a7d4601e2 100644
>>>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>>>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>>>> @@ -23,6 +23,34 @@
>>>>      static bool legacy_binding;
>>>>    +/**
>>>> + * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
>>>
>>> Get non-codec Venus clocks.
>>
>> No, this is not necessarily the case.. these may still include
>> vcodec clocks, that are specified under the root venus node (which
>> is the only way we'd like to keep)
>>
>> I applied the rest of your suggestions, do I keep your rb?
>>
>> Konrad
>>
> 
> Sure
> 
> BTW, I plan to test this series when I can - do you have a working tree ?

next + the patchset in question is a working tree..

Konrad

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2024-03-27 17:23 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-09 21:09 [PATCH v2 00/20] Venus cleanups Konrad Dybcio
2024-02-09 21:09 ` [PATCH v2 01/20] media: venus: pm_helpers: Only set rate of the core clock in core_clks_enable Konrad Dybcio
2024-02-09 21:09 ` [PATCH v2 02/20] media: venus: pm_helpers: Rename core_clks_get to venus_clks_get Konrad Dybcio
2024-02-09 21:09 ` [PATCH v2 03/20] media: venus: pm_helpers: Add kerneldoc to venus_clks_get() Konrad Dybcio
2024-02-09 21:09 ` [PATCH v2 04/20] media: venus: core: Set OPP clkname in a common code path Konrad Dybcio
2024-02-09 21:09 ` [PATCH v2 05/20] media: venus: pm_helpers: Kill dead code Konrad Dybcio
2024-02-09 21:10 ` [PATCH v2 00/20] Venus cleanups Konrad Dybcio
  -- strict thread matches above, loose matches on Subject: below --
2024-02-09 21:09 Konrad Dybcio
2024-02-09 21:09 ` [PATCH v2 03/20] media: venus: pm_helpers: Add kerneldoc to venus_clks_get() Konrad Dybcio
2024-03-06 12:20   ` Bryan O'Donoghue
2024-03-26 21:23     ` Konrad Dybcio
2024-03-27  9:55       ` Bryan O'Donoghue
2024-03-27 17:23         ` Konrad Dybcio

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).