[PATCH 0/6] SM8350 and SC8280XP venus support

[PATCH 0/6] SM8350 and SC8280XP venus support

[Konrad Dybcio]

SM8350 and SC8280XP both implement IRIS2, with the bigger SoC having
a bit of a beefier unit, capable of reaching a higher core clock.

Rebased atop Stan's venus-for-6.6.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (6):
      media: dt-bindings: Document SC8280XP/SM8350 Venus
      media: venus: core: Remove trailing commas from of match entries
      media: venus: hfi_venus: Support only updating certain bits with presets
      media: platform: venus: Add optional LLCC path
      media: venus: core: Add SM8350 resource struct
      media: venus: core: Add SC8280XP resource struct

 .../bindings/media/qcom,sm8350-venus.yaml          | 149 +++++++++++++++++++++
 drivers/media/platform/qcom/venus/core.c           | 119 ++++++++++++++--
 drivers/media/platform/qcom/venus/core.h           |   4 +
 drivers/media/platform/qcom/venus/hfi_venus.c      |  15 ++-
 drivers/media/platform/qcom/venus/pm_helpers.c     |   3 +
 5 files changed, 279 insertions(+), 11 deletions(-)
---
base-commit: 210fefeb11b4bf5d4c5597f126425c2d3fea1aa9
change-id: 20230731-topic-8280_venus-8f506ae723a6

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

[PATCH 1/6] media: dt-bindings: Document SC8280XP/SM8350 Venus

[Konrad Dybcio]

Both of these SoCs implement an IRIS2 block, with SC8280XP being able
to clock it a bit higher.

Document it.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 .../bindings/media/qcom,sm8350-venus.yaml          | 149 +++++++++++++++++++++
 1 file changed, 149 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/qcom,sm8350-venus.yaml b/Documentation/devicetree/bindings/media/qcom,sm8350-venus.yaml
new file mode 100644
index 000000000000..8a31bce27c18
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/qcom,sm8350-venus.yaml
@@ -0,0 +1,149 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/qcom,sm8350-venus.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SM8350 Venus video encode and decode accelerators
+
+maintainers:
+  - Konrad Dybcio <konradybcio@kernel.org>
+
+description: |
+  The Venus Iris2 IP is a video encode and decode accelerator present
+  on Qualcomm platforms
+
+allOf:
+  - $ref: qcom,venus-common.yaml#
+
+properties:
+  compatible:
+    enum:
+      - qcom,sc8280xp-venus
+      - qcom,sm8350-venus
+
+  clocks:
+    maxItems: 3
+
+  clock-names:
+    items:
+      - const: iface
+      - const: core
+      - const: vcodec0_core
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    items:
+      - const: core
+
+  power-domains:
+    maxItems: 3
+
+  power-domain-names:
+    items:
+      - const: venus
+      - const: vcodec0
+      - const: mx
+
+  interconnects:
+    maxItems: 3
+
+  interconnect-names:
+    items:
+      - const: cpu-cfg
+      - const: video-mem
+      - const: video-llcc
+
+  operating-points-v2: true
+  opp-table:
+    type: object
+
+  iommus:
+    maxItems: 1
+
+  video-decoder:
+    type: object
+
+    properties:
+      compatible:
+        const: venus-decoder
+
+    required:
+      - compatible
+
+    additionalProperties: false
+
+  video-encoder:
+    type: object
+
+    properties:
+      compatible:
+        const: venus-encoder
+
+    required:
+      - compatible
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - power-domain-names
+  - iommus
+  - video-decoder
+  - video-encoder
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/qcom,gcc-sm8350.h>
+    #include <dt-bindings/clock/qcom,sm8350-videocc.h>
+    #include <dt-bindings/interconnect/qcom,sm8350.h>
+    #include <dt-bindings/power/qcom-rpmpd.h>
+
+    venus: video-codec@aa00000 {
+        compatible = "qcom,sm8350-venus";
+        reg = <0x0aa00000 0x100000>;
+        interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
+
+        clocks = <&gcc GCC_VIDEO_AXI0_CLK>,
+                 <&videocc VIDEO_CC_MVS0C_CLK>,
+                 <&videocc VIDEO_CC_MVS0_CLK>;
+        clock-names = "iface",
+                      "core",
+                      "vcodec0_core";
+
+        resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
+        reset-names = "core";
+
+        power-domains = <&videocc MVS0C_GDSC>,
+                        <&videocc MVS0_GDSC>,
+                        <&rpmhpd SM8350_MX>;
+        power-domain-names = "venus",
+                             "vcodec0",
+                             "mx";
+
+        interconnects = <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_VENUS_CFG 0>,
+                        <&mmss_noc MASTER_VIDEO_P0 0 &mc_virt SLAVE_EBI1 0>,
+                        <&mmss_noc MASTER_VIDEO_P0 0 &gem_noc SLAVE_LLCC 0>;
+        interconnect-names = "cpu-cfg",
+                             "video-mem",
+                             "video-llcc";
+
+        operating-points-v2 = <&venus_opp_table>;
+        iommus = <&apps_smmu 0x2100 0x400>;
+        memory-region = <&pil_video_mem>;
+
+        status = "disabled";
+
+        video-decoder {
+            compatible = "venus-decoder";
+        };
+
+        video-encoder {
+            compatible = "venus-encoder";
+        };
+    };

-- 
2.41.0

[PATCH 2/6] media: venus: core: Remove trailing commas from of match entries

[Konrad Dybcio]

Even though it has zero effect on functionality, remove them for coherency
with other drivers.

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

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 3cc38881d4f6..0af45faec247 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -884,14 +884,14 @@ static const struct venus_resources sc7280_res = {
 };
 
 static const struct of_device_id venus_dt_match[] = {
-	{ .compatible = "qcom,msm8916-venus", .data = &msm8916_res, },
-	{ .compatible = "qcom,msm8996-venus", .data = &msm8996_res, },
-	{ .compatible = "qcom,sdm660-venus", .data = &sdm660_res, },
-	{ .compatible = "qcom,sdm845-venus", .data = &sdm845_res, },
-	{ .compatible = "qcom,sdm845-venus-v2", .data = &sdm845_res_v2, },
-	{ .compatible = "qcom,sc7180-venus", .data = &sc7180_res, },
-	{ .compatible = "qcom,sc7280-venus", .data = &sc7280_res, },
-	{ .compatible = "qcom,sm8250-venus", .data = &sm8250_res, },
+	{ .compatible = "qcom,msm8916-venus", .data = &msm8916_res },
+	{ .compatible = "qcom,msm8996-venus", .data = &msm8996_res },
+	{ .compatible = "qcom,sdm660-venus", .data = &sdm660_res },
+	{ .compatible = "qcom,sdm845-venus", .data = &sdm845_res },
+	{ .compatible = "qcom,sdm845-venus-v2", .data = &sdm845_res_v2 },
+	{ .compatible = "qcom,sc7180-venus", .data = &sc7180_res },
+	{ .compatible = "qcom,sc7280-venus", .data = &sc7280_res },
+	{ .compatible = "qcom,sm8250-venus", .data = &sm8250_res },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, venus_dt_match);

-- 
2.41.0

[PATCH 3/6] media: venus: hfi_venus: Support only updating certain bits with presets

[Konrad Dybcio]

On some platforms (like SM8350) we're expected to only touch certain bits
(such as 0 and 4 corresponding to mask 0x11). Add support for doing so.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.h      |  1 +
 drivers/media/platform/qcom/venus/hfi_venus.c | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index d996abd339e1..2999c24392f5 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -38,6 +38,7 @@ struct freq_tbl {
 struct reg_val {
 	u32 reg;
 	u32 value;
+	u32 mask;
 };
 
 struct bw_tbl {
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 19fc6575a489..d4bad66f7293 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -349,10 +349,19 @@ static void venus_set_registers(struct venus_hfi_device *hdev)
 	const struct venus_resources *res = hdev->core->res;
 	const struct reg_val *tbl = res->reg_tbl;
 	unsigned int count = res->reg_tbl_size;
-	unsigned int i;
+	unsigned int i, val;
+
+	for (i = 0; i < count; i++) {
+		val = tbl[i].value;
 
-	for (i = 0; i < count; i++)
-		writel(tbl[i].value, hdev->core->base + tbl[i].reg);
+		/* In some cases, we only want to update certain bits */
+		if (tbl[i].mask) {
+			val = readl(hdev->core->base + tbl[i].reg);
+			val = (val & ~tbl[i].mask) | (tbl[i].value & tbl[i].mask);
+		}
+
+		writel(val, hdev->core->base + tbl[i].reg);
+	}
 }
 
 static void venus_soft_int(struct venus_hfi_device *hdev)

-- 
2.41.0

[PATCH 4/6] media: platform: venus: Add optional LLCC path

[Konrad Dybcio]

Some newer SoCs (such as SM8350) have a third interconnect path. Add
it and make it optional.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c       | 19 +++++++++++++++++++
 drivers/media/platform/qcom/venus/core.h       |  3 +++
 drivers/media/platform/qcom/venus/pm_helpers.c |  3 +++
 3 files changed, 25 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 0af45faec247..db363061748f 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -302,6 +302,15 @@ static int venus_probe(struct platform_device *pdev)
 	if (IS_ERR(core->cpucfg_path))
 		return PTR_ERR(core->cpucfg_path);
 
+	core->llcc_path = devm_of_icc_get(dev, "video-llcc");
+	if (IS_ERR(core->llcc_path)) {
+		/* LLCC path is optional */
+		if (PTR_ERR(core->llcc_path) == -ENODATA)
+			core->llcc_path = NULL;
+		else
+			return PTR_ERR(core->llcc_path);
+	}
+
 	core->irq = platform_get_irq(pdev, 0);
 	if (core->irq < 0)
 		return core->irq;
@@ -479,12 +488,18 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
 	if (ret)
 		goto err_cpucfg_path;
 
+	ret = icc_set_bw(core->llcc_path, 0, 0);
+	if (ret)
+		goto err_llcc_path;
+
 	ret = icc_set_bw(core->video_path, 0, 0);
 	if (ret)
 		goto err_video_path;
 
 	return ret;
 
+err_llcc_path:
+	icc_set_bw(core->video_path, kbps_to_icc(20000), 0);
 err_video_path:
 	icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
 err_cpucfg_path:
@@ -504,6 +519,10 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	ret = icc_set_bw(core->llcc_path, kbps_to_icc(20000), 0);
+	if (ret)
+		return ret;
+
 	ret = icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
 	if (ret)
 		return ret;
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 2999c24392f5..45ed1551c2db 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -65,6 +65,7 @@ struct venus_resources {
 	unsigned int bw_tbl_enc_size;
 	const struct bw_tbl *bw_tbl_dec;
 	unsigned int bw_tbl_dec_size;
+	bool has_llcc_path;
 	const struct reg_val *reg_tbl;
 	unsigned int reg_tbl_size;
 	const struct hfi_ubwc_config *ubwc_conf;
@@ -134,6 +135,7 @@ struct venus_format {
  * @vcodec1_clks: an array of vcodec1 struct clk pointers
  * @video_path: an interconnect handle to video to/from memory path
  * @cpucfg_path: an interconnect handle to cpu configuration path
+ * @llcc_path: an interconnect handle to video to/from llcc path
  * @has_opp_table: does OPP table exist
  * @pmdomains:	an array of pmdomains struct device pointers
  * @opp_dl_venus: an device-link for device OPP
@@ -186,6 +188,7 @@ struct venus_core {
 	struct clk *vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
 	struct icc_path *video_path;
 	struct icc_path *cpucfg_path;
+	struct icc_path *llcc_path;
 	bool has_opp_table;
 	struct device *pmdomains[VIDC_PMDOMAINS_NUM_MAX];
 	struct device_link *opp_dl_venus;
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 48c9084bb4db..79392ff8fa06 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -237,6 +237,9 @@ static int load_scale_bw(struct venus_core *core)
 	dev_dbg(core->dev, VDBGL "total: avg_bw: %u, peak_bw: %u\n",
 		total_avg, total_peak);
 
+	if (core->res->has_llcc_path)
+		icc_set_bw(core->llcc_path, total_avg, total_peak);
+
 	return icc_set_bw(core->video_path, total_avg, total_peak);
 }
 

-- 
2.41.0

[PATCH 5/6] media: venus: core: Add SM8350 resource struct

[Konrad Dybcio]

Add SM8350 configuration data and related compatible.

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

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index db363061748f..5f285ae75e9d 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -844,6 +844,44 @@ static const struct venus_resources sm8250_res = {
 	.fwname = "qcom/vpu-1.0/venus.mdt",
 };
 
+static const struct reg_val sm8350_reg_preset[] = {
+	{ 0xb0088, 0, 0x11 },
+};
+
+static const struct venus_resources sm8350_res = {
+	.freq_tbl = sm8250_freq_table,
+	.freq_tbl_size = ARRAY_SIZE(sm8250_freq_table),
+	.reg_tbl = sm8350_reg_preset,
+	.reg_tbl_size = ARRAY_SIZE(sm8350_reg_preset),
+	.bw_tbl_enc = sm8250_bw_table_enc,
+	.bw_tbl_enc_size = ARRAY_SIZE(sm8250_bw_table_enc),
+	.bw_tbl_dec = sm8250_bw_table_dec,
+	.bw_tbl_dec_size = ARRAY_SIZE(sm8250_bw_table_dec),
+	.clks = { "core", "iface" },
+	.clks_num = 2,
+	.resets = { "core" },
+	.resets_num = 1,
+	.vcodec0_clks = { "vcodec0_core" },
+	.vcodec_clks_num = 1,
+	.vcodec_pmdomains = { "venus", "vcodec0" },
+	.vcodec_pmdomains_num = 2,
+	.opp_pmdomain = (const char *[]) { "mx", NULL },
+	.vcodec_num = 1,
+	.max_load = 7833600, /* 7680x4320@60fps */
+	.hfi_version = HFI_VERSION_6XX,
+	.vpu_version = VPU_VERSION_IRIS2,
+	.num_vpp_pipes = 4,
+	.vmem_id = VIDC_RESOURCE_NONE,
+	.vmem_size = 0,
+	.vmem_addr = 0,
+	.dma_mask = GENMASK(31, 29) - 1,
+	.cp_start = 0,
+	.cp_size = 0x25800000,
+	.cp_nonpixel_start = 0x1000000,
+	.cp_nonpixel_size = 0x24800000,
+	.fwname = "qcom/vpu-2.0/venus.mbn",
+};
+
 static const struct freq_tbl sc7280_freq_table[] = {
 	{ 0, 460000000 },
 	{ 0, 424000000 },
@@ -911,6 +949,7 @@ static const struct of_device_id venus_dt_match[] = {
 	{ .compatible = "qcom,sc7180-venus", .data = &sc7180_res },
 	{ .compatible = "qcom,sc7280-venus", .data = &sc7280_res },
 	{ .compatible = "qcom,sm8250-venus", .data = &sm8250_res },
+	{ .compatible = "qcom,sm8350-venus", .data = &sm8350_res },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, venus_dt_match);

-- 
2.41.0

[PATCH 6/6] media: venus: core: Add SC8280XP resource struct

[Konrad Dybcio]

Add SC8280XP configuration data and related compatible.

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

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 5f285ae75e9d..32591b624a36 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -940,6 +940,50 @@ static const struct venus_resources sc7280_res = {
 	.fwname = "qcom/vpu-2.0/venus.mbn",
 };
 
+static const struct freq_tbl sc8280xp_freq_table[] = {
+	{ 0, 239999999 },
+	{ 0, 338000000 },
+	{ 0, 366000000 },
+	{ 0, 444000000 },
+	{ 0, 533000000 },
+	{ 0, 560000000 },
+};
+
+static const struct venus_resources sc8280xp_res = {
+	.freq_tbl = sc8280xp_freq_table,
+	.freq_tbl_size = ARRAY_SIZE(sc8280xp_freq_table),
+	.reg_tbl = sm8350_reg_preset,
+	.reg_tbl_size = ARRAY_SIZE(sm8350_reg_preset),
+	.bw_tbl_enc = sm8250_bw_table_enc,
+	.bw_tbl_enc_size = ARRAY_SIZE(sm8250_bw_table_enc),
+	.bw_tbl_dec = sm8250_bw_table_dec,
+	.bw_tbl_dec_size = ARRAY_SIZE(sm8250_bw_table_dec),
+	.clks = { "core", "iface" },
+	.clks_num = 2,
+	.resets = { "core" },
+	.resets_num = 1,
+	.vcodec0_clks = { "vcodec0_core" },
+	.vcodec_clks_num = 1,
+	.vcodec_pmdomains = { "venus", "vcodec0" },
+	.vcodec_pmdomains_num = 2,
+	.opp_pmdomain = (const char *[]) { "mx", NULL },
+	.vcodec_num = 1,
+	.max_load = 7833600, /* 7680x4320@60fps */
+	.hfi_version = HFI_VERSION_6XX,
+	.vpu_version = VPU_VERSION_IRIS2,
+	.num_vpp_pipes = 4,
+	.vmem_id = VIDC_RESOURCE_NONE,
+	.vmem_size = 0,
+	.vmem_addr = 0,
+	.dma_mask = GENMASK(31, 29) - 1,
+	.cp_start = 0,
+	.cp_size = 0x25800000,
+	.cp_nonpixel_start = 0x1000000,
+	.cp_nonpixel_size = 0x24800000,
+	.fwname = "qcom/vpu-2.0/venus.mbn",
+};
+
+
 static const struct of_device_id venus_dt_match[] = {
 	{ .compatible = "qcom,msm8916-venus", .data = &msm8916_res },
 	{ .compatible = "qcom,msm8996-venus", .data = &msm8996_res },
@@ -948,6 +992,7 @@ static const struct of_device_id venus_dt_match[] = {
 	{ .compatible = "qcom,sdm845-venus-v2", .data = &sdm845_res_v2 },
 	{ .compatible = "qcom,sc7180-venus", .data = &sc7180_res },
 	{ .compatible = "qcom,sc7280-venus", .data = &sc7280_res },
+	{ .compatible = "qcom,sc8280xp-venus", .data = &sc8280xp_res },
 	{ .compatible = "qcom,sm8250-venus", .data = &sm8250_res },
 	{ .compatible = "qcom,sm8350-venus", .data = &sm8350_res },
 	{ }

-- 
2.41.0

Re: [PATCH 3/6] media: venus: hfi_venus: Support only updating certain bits with presets

[Bryan O'Donoghue]

On 04/08/2023 21:09, Konrad Dybcio wrote:
> On some platforms (like SM8350) we're expected to only touch certain bits
> (such as 0 and 4 corresponding to mask 0x11). Add support for doing so.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/core.h      |  1 +
>   drivers/media/platform/qcom/venus/hfi_venus.c | 15 ++++++++++++---
>   2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index d996abd339e1..2999c24392f5 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -38,6 +38,7 @@ struct freq_tbl {
>   struct reg_val {
>   	u32 reg;
>   	u32 value;
> +	u32 mask;
>   };
>   
>   struct bw_tbl {
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index 19fc6575a489..d4bad66f7293 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -349,10 +349,19 @@ static void venus_set_registers(struct venus_hfi_device *hdev)
>   	const struct venus_resources *res = hdev->core->res;
>   	const struct reg_val *tbl = res->reg_tbl;
>   	unsigned int count = res->reg_tbl_size;
> -	unsigned int i;
> +	unsigned int i, val;

u32 val;

> +
> +	for (i = 0; i < count; i++) {
> +		val = tbl[i].value;

struct reg_val looks like this

struct reg_val {
         u32 reg;
         u32 value;
};

val should be declared a u32

> -	for (i = 0; i < count; i++)
> -		writel(tbl[i].value, hdev->core->base + tbl[i].reg);
> +		/* In some cases, we only want to update certain bits */

I'll trust this is a legitimate and true statement.

> +		if (tbl[i].mask) {
> +			val = readl(hdev->core->base + tbl[i].reg);
> +			val = (val & ~tbl[i].mask) | (tbl[i].value & tbl[i].mask);

feels like something regmap_update_bits() already does though, I prefer 
this because there's less code in it.

> +		}
> +
> +		writel(val, hdev->core->base + tbl[i].reg);
> +	}

With the val declaration fix

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

Re: [PATCH 3/6] media: venus: hfi_venus: Support only updating certain bits with presets

[Bryan O'Donoghue]

On 04/08/2023 21:50, Bryan O'Donoghue wrote:
> On 04/08/2023 21:09, Konrad Dybcio wrote:
>> On some platforms (like SM8350) we're expected to only touch certain bits
>> (such as 0 and 4 corresponding to mask 0x11). Add support for doing so.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/media/platform/qcom/venus/core.h      |  1 +
>>   drivers/media/platform/qcom/venus/hfi_venus.c | 15 ++++++++++++---
>>   2 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h 
>> b/drivers/media/platform/qcom/venus/core.h
>> index d996abd339e1..2999c24392f5 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -38,6 +38,7 @@ struct freq_tbl {
>>   struct reg_val {
>>       u32 reg;
>>       u32 value;
>> +    u32 mask;
>>   };
>>   struct bw_tbl {
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c 
>> b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index 19fc6575a489..d4bad66f7293 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -349,10 +349,19 @@ static void venus_set_registers(struct 
>> venus_hfi_device *hdev)
>>       const struct venus_resources *res = hdev->core->res;
>>       const struct reg_val *tbl = res->reg_tbl;
>>       unsigned int count = res->reg_tbl_size;
>> -    unsigned int i;
>> +    unsigned int i, val;
> 
> u32 val;
> 
>> +
>> +    for (i = 0; i < count; i++) {
>> +        val = tbl[i].value;
> 
> struct reg_val looks like this
> 
> struct reg_val {
>          u32 reg;
>          u32 value;
> };
> 
> val should be declared a u32
> 
>> -    for (i = 0; i < count; i++)
>> -        writel(tbl[i].value, hdev->core->base + tbl[i].reg);
>> +        /* In some cases, we only want to update certain bits */
> 
> I'll trust this is a legitimate and true statement.
> 
>> +        if (tbl[i].mask) {
>> +            val = readl(hdev->core->base + tbl[i].reg);
>> +            val = (val & ~tbl[i].mask) | (tbl[i].value & tbl[i].mask);
> 
> feels like something regmap_update_bits() already does though, I prefer 
> this because there's less code in it.
> 
>> +        }
>> +
>> +        writel(val, hdev->core->base + tbl[i].reg);
>> +    }
> 
> With the val declaration fix
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

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

Re: [PATCH 4/6] media: platform: venus: Add optional LLCC path

[Bryan O'Donoghue]

On 04/08/2023 21:09, Konrad Dybcio wrote:
> Some newer SoCs (such as SM8350) have a third interconnect path. Add
> it and make it optional.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/core.c       | 19 +++++++++++++++++++
>   drivers/media/platform/qcom/venus/core.h       |  3 +++
>   drivers/media/platform/qcom/venus/pm_helpers.c |  3 +++
>   3 files changed, 25 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 0af45faec247..db363061748f 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -302,6 +302,15 @@ static int venus_probe(struct platform_device *pdev)
>   	if (IS_ERR(core->cpucfg_path))
>   		return PTR_ERR(core->cpucfg_path);
>   
> +	core->llcc_path = devm_of_icc_get(dev, "video-llcc");
> +	if (IS_ERR(core->llcc_path)) {
> +		/* LLCC path is optional */
> +		if (PTR_ERR(core->llcc_path) == -ENODATA)
> +			core->llcc_path = NULL;
> +		else
> +			return PTR_ERR(core->llcc_path);
> +	}
> +
>   	core->irq = platform_get_irq(pdev, 0);
>   	if (core->irq < 0)
>   		return core->irq;
> @@ -479,12 +488,18 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
>   	if (ret)
>   		goto err_cpucfg_path;
>   
> +	ret = icc_set_bw(core->llcc_path, 0, 0);
> +	if (ret)
> +		goto err_llcc_path;
> +
>   	ret = icc_set_bw(core->video_path, 0, 0);
>   	if (ret)
>   		goto err_video_path;
>   
>   	return ret;
>   
> +err_llcc_path:
> +	icc_set_bw(core->video_path, kbps_to_icc(20000), 0);
>   err_video_path:
>   	icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
>   err_cpucfg_path:
> @@ -504,6 +519,10 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
>   	if (ret)
>   		return ret;
>   
> +	ret = icc_set_bw(core->llcc_path, kbps_to_icc(20000), 0);
> +	if (ret)
> +		return ret;
> +

I would scream if someone left me this comment but...

In probe we have

video_path =
cpu_cfgpath =

llc_path =

suspend

icc_set_bw(cpu_cfgpath,);
icc_set_bw(llc_path,);
icc_set_bw(video_path,);

resume
icc_set_bw(video_path,);
icc_set_bw(llc_path,);
icc_set_bw(cpu_cfgpath,);

it would be nice to have probe match the ordering ...

>   	ret = icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
>   	if (ret)
>   		return ret;
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 2999c24392f5..45ed1551c2db 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -65,6 +65,7 @@ struct venus_resources {
>   	unsigned int bw_tbl_enc_size;
>   	const struct bw_tbl *bw_tbl_dec;
>   	unsigned int bw_tbl_dec_size;
> +	bool has_llcc_path;

Why do you need this bool, you can get for llc_path == NULL

---
bod

Re: [PATCH 2/6] media: venus: core: Remove trailing commas from of match entries

[Bryan O'Donoghue]

On 04/08/2023 21:09, Konrad Dybcio wrote:
> Even though it has zero effect on functionality, remove them for coherency
> with other drivers.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/core.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 3cc38881d4f6..0af45faec247 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -884,14 +884,14 @@ static const struct venus_resources sc7280_res = {
>   };
>   
>   static const struct of_device_id venus_dt_match[] = {
> -	{ .compatible = "qcom,msm8916-venus", .data = &msm8916_res, },
> -	{ .compatible = "qcom,msm8996-venus", .data = &msm8996_res, },
> -	{ .compatible = "qcom,sdm660-venus", .data = &sdm660_res, },
> -	{ .compatible = "qcom,sdm845-venus", .data = &sdm845_res, },
> -	{ .compatible = "qcom,sdm845-venus-v2", .data = &sdm845_res_v2, },
> -	{ .compatible = "qcom,sc7180-venus", .data = &sc7180_res, },
> -	{ .compatible = "qcom,sc7280-venus", .data = &sc7280_res, },
> -	{ .compatible = "qcom,sm8250-venus", .data = &sm8250_res, },
> +	{ .compatible = "qcom,msm8916-venus", .data = &msm8916_res },
> +	{ .compatible = "qcom,msm8996-venus", .data = &msm8996_res },
> +	{ .compatible = "qcom,sdm660-venus", .data = &sdm660_res },
> +	{ .compatible = "qcom,sdm845-venus", .data = &sdm845_res },
> +	{ .compatible = "qcom,sdm845-venus-v2", .data = &sdm845_res_v2 },
> +	{ .compatible = "qcom,sc7180-venus", .data = &sc7180_res },
> +	{ .compatible = "qcom,sc7280-venus", .data = &sc7280_res },
> +	{ .compatible = "qcom,sm8250-venus", .data = &sm8250_res },
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(of, venus_dt_match);


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

Re: [PATCH 4/6] media: platform: venus: Add optional LLCC path

[Bryan O'Donoghue]

On 04/08/2023 22:04, Bryan O'Donoghue wrote:
> you can get for llc_path == NULL

[sic] You can test.

---
bod

Re: [PATCH 5/6] media: venus: core: Add SM8350 resource struct

[Bryan O'Donoghue]

On 04/08/2023 21:09, Konrad Dybcio wrote:
> +	.freq_tbl = sm8250_freq_table,
> +	.freq_tbl_size = ARRAY_SIZE(sm8250_freq_table),
> +	.reg_tbl = sm8350_reg_preset,
> +	.reg_tbl_size = ARRAY_SIZE(sm8350_reg_preset),
> +	.bw_tbl_enc = sm8250_bw_table_enc,
> +	.bw_tbl_enc_size = ARRAY_SIZE(sm8250_bw_table_enc),
> +	.bw_tbl_dec = sm8250_bw_table_dec,
> +	.bw_tbl_dec_size = ARRAY_SIZE(sm8250_bw_table_dec),

The very same freq and bandwidth tables ?

---
bod

Re: [PATCH 6/6] media: venus: core: Add SC8280XP resource struct

[Bryan O'Donoghue]

On 04/08/2023 21:09, Konrad Dybcio wrote:
> +	.freq_tbl = sc8280xp_freq_table,
> +	.freq_tbl_size = ARRAY_SIZE(sc8280xp_freq_table),

Would it not be more legitimate and logical to have 8350 use 8280xp's 
frequency table, instead of 8250 ?

---
bod

Re: [PATCH 6/6] media: venus: core: Add SC8280XP resource struct

[Konrad Dybcio]

On 4.08.2023 23:10, Bryan O'Donoghue wrote:
> On 04/08/2023 21:09, Konrad Dybcio wrote:
>> +    .freq_tbl = sc8280xp_freq_table,
>> +    .freq_tbl_size = ARRAY_SIZE(sc8280xp_freq_table),
> 
> Would it not be more legitimate and logical to have 8350 use 8280xp's frequency table, instead of 8250 ?
top freq is higher on 8280

Konrad

Re: [PATCH 6/6] media: venus: core: Add SC8280XP resource struct

[Bryan O'Donoghue]

On 04/08/2023 22:10, Konrad Dybcio wrote:
>> Would it not be more legitimate and logical to have 8350 use 8280xp's frequency table, instead of 8250 ?
> top freq is higher on 8280

Still though its a bit suspicious 8350 doesn't have its own table.

Are we missing the downstream reference ?

---
bod

Re: [PATCH 6/6] media: venus: core: Add SC8280XP resource struct

[Konrad Dybcio]

On 4.08.2023 23:12, Bryan O'Donoghue wrote:
> On 04/08/2023 22:10, Konrad Dybcio wrote:
>>> Would it not be more legitimate and logical to have 8350 use 8280xp's frequency table, instead of 8250 ?
>> top freq is higher on 8280
> 
> Still though its a bit suspicious 8350 doesn't have its own table.
> 
> Are we missing the downstream reference ?
8250:
qcom,allowed-clock-rates = <239999999 338000000 366000000 444000000>;

8350:
qcom,allowed-clock-rates = <239999999 338000000 366000000 444000000>;

(identical)

Konrad

Re: [PATCH 5/6] media: venus: core: Add SM8350 resource struct

[Konrad Dybcio]

On 4.08.2023 23:08, Bryan O'Donoghue wrote:
> On 04/08/2023 21:09, Konrad Dybcio wrote:
>> +    .freq_tbl = sm8250_freq_table,
>> +    .freq_tbl_size = ARRAY_SIZE(sm8250_freq_table),
>> +    .reg_tbl = sm8350_reg_preset,
>> +    .reg_tbl_size = ARRAY_SIZE(sm8350_reg_preset),
>> +    .bw_tbl_enc = sm8250_bw_table_enc,
>> +    .bw_tbl_enc_size = ARRAY_SIZE(sm8250_bw_table_enc),
>> +    .bw_tbl_dec = sm8250_bw_table_dec,
>> +    .bw_tbl_dec_size = ARRAY_SIZE(sm8250_bw_table_dec),
> 
> The very same freq and bandwidth tables ?
yep!

Konrad

Re: [PATCH 6/6] media: venus: core: Add SC8280XP resource struct

[Bryan O'Donoghue]

On 04/08/2023 22:17, Konrad Dybcio wrote:
> On 4.08.2023 23:12, Bryan O'Donoghue wrote:
>> On 04/08/2023 22:10, Konrad Dybcio wrote:
>>>> Would it not be more legitimate and logical to have 8350 use 8280xp's frequency table, instead of 8250 ?
>>> top freq is higher on 8280
>>
>> Still though its a bit suspicious 8350 doesn't have its own table.
>>
>> Are we missing the downstream reference ?
> 8250:
> qcom,allowed-clock-rates = <239999999 338000000 366000000 444000000>;
> 
> 8350:
> qcom,allowed-clock-rates = <239999999 338000000 366000000 444000000>;
> 
> (identical)
> 
> Konrad

Fair enough

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

Re: [PATCH 1/6] media: dt-bindings: Document SC8280XP/SM8350 Venus

[Krzysztof Kozlowski]

On 04/08/2023 22:09, Konrad Dybcio wrote:
> Both of these SoCs implement an IRIS2 block, with SC8280XP being able
> to clock it a bit higher.
> 

...

> +
> +  iommus:
> +    maxItems: 1
> +
> +  video-decoder:
> +    type: object
> +
> +    properties:
> +      compatible:
> +        const: venus-decoder

That's not how compatibles are constructed... missing vendor prefix, SoC
or IP block name.

> +
> +    required:
> +      - compatible
> +
> +    additionalProperties: false

Why do you need this child node? Child nodes without properties are
usually useless.

> +
> +  video-encoder:
> +    type: object
> +
> +    properties:
> +      compatible:
> +        const: venus-encoder
> +
> +    required:
> +      - compatible
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - power-domain-names
> +  - iommus
> +  - video-decoder
> +  - video-encoder
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,gcc-sm8350.h>
> +    #include <dt-bindings/clock/qcom,sm8350-videocc.h>
> +    #include <dt-bindings/interconnect/qcom,sm8350.h>
> +    #include <dt-bindings/power/qcom-rpmpd.h>
> +
> +    venus: video-codec@aa00000 {
> +        compatible = "qcom,sm8350-venus";
> +        reg = <0x0aa00000 0x100000>;
> +        interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
> +
> +        clocks = <&gcc GCC_VIDEO_AXI0_CLK>,
> +                 <&videocc VIDEO_CC_MVS0C_CLK>,
> +                 <&videocc VIDEO_CC_MVS0_CLK>;
> +        clock-names = "iface",
> +                      "core",
> +                      "vcodec0_core";
> +
> +        resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
> +        reset-names = "core";
> +
> +        power-domains = <&videocc MVS0C_GDSC>,
> +                        <&videocc MVS0_GDSC>,
> +                        <&rpmhpd SM8350_MX>;
> +        power-domain-names = "venus",
> +                             "vcodec0",
> +                             "mx";
> +
> +        interconnects = <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_VENUS_CFG 0>,
> +                        <&mmss_noc MASTER_VIDEO_P0 0 &mc_virt SLAVE_EBI1 0>,
> +                        <&mmss_noc MASTER_VIDEO_P0 0 &gem_noc SLAVE_LLCC 0>;
> +        interconnect-names = "cpu-cfg",
> +                             "video-mem",
> +                             "video-llcc";
> +
> +        operating-points-v2 = <&venus_opp_table>;
> +        iommus = <&apps_smmu 0x2100 0x400>;
> +        memory-region = <&pil_video_mem>;
> +
> +        status = "disabled";

Drop status.

> +
> +        video-decoder {

Best regards,
Krzysztof

Re: [PATCH 4/6] media: platform: venus: Add optional LLCC path

[Johan Hovold]

On Fri, Aug 04, 2023 at 10:09:11PM +0200, Konrad Dybcio wrote:

> @@ -479,12 +488,18 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
>  	if (ret)
>  		goto err_cpucfg_path;
>  
> +	ret = icc_set_bw(core->llcc_path, 0, 0);
> +	if (ret)
> +		goto err_llcc_path;
> +
>  	ret = icc_set_bw(core->video_path, 0, 0);
>  	if (ret)
>  		goto err_video_path;
>  
>  	return ret;
>  
> +err_llcc_path:
> +	icc_set_bw(core->video_path, kbps_to_icc(20000), 0);

This looks wrong; you should not try to restore the video path bw which
you have not yet updated here.

Also error labels should be named after what they do, not after where
you jump from (e.g. to avoid mistakes like the above). Perhaps you can
clean up the existing labels before adding the new one.

>  err_video_path:
>  	icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
>  err_cpucfg_path:

Johan

Re: [PATCH 1/6] media: dt-bindings: Document SC8280XP/SM8350 Venus

[Konrad Dybcio]

On 5.08.2023 21:29, Krzysztof Kozlowski wrote:
> On 04/08/2023 22:09, Konrad Dybcio wrote:
>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able
>> to clock it a bit higher.
>>
> 
> ...
> 
>> +
>> +  iommus:
>> +    maxItems: 1
>> +
>> +  video-decoder:
>> +    type: object
>> +
>> +    properties:
>> +      compatible:
>> +        const: venus-decoder
> 
> That's not how compatibles are constructed... missing vendor prefix, SoC
> or IP block name.
> 
>> +
>> +    required:
>> +      - compatible
>> +
>> +    additionalProperties: false
> 
> Why do you need this child node? Child nodes without properties are
> usually useless.
For both comments: I aligned with what was there..

The driver abuses these compats to probe enc/dec submodules, even though
every Venus implementation (to my knowledge) is implicitly enc/dec capable..

Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec
devices from the venus core probe and get rid of this fake stuff?

Konrad

Re: [PATCH 4/6] media: platform: venus: Add optional LLCC path

[Konrad Dybcio]

On 7.08.2023 12:43, Johan Hovold wrote:
> On Fri, Aug 04, 2023 at 10:09:11PM +0200, Konrad Dybcio wrote:
> 
>> @@ -479,12 +488,18 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
>>  	if (ret)
>>  		goto err_cpucfg_path;
>>  
>> +	ret = icc_set_bw(core->llcc_path, 0, 0);
>> +	if (ret)
>> +		goto err_llcc_path;
>> +
>>  	ret = icc_set_bw(core->video_path, 0, 0);
>>  	if (ret)
>>  		goto err_video_path;
>>  
>>  	return ret;
>>  
>> +err_llcc_path:
>> +	icc_set_bw(core->video_path, kbps_to_icc(20000), 0);
> 
> This looks wrong; you should not try to restore the video path bw which
> you have not yet updated here.
Oh whoops :D

> 
> Also error labels should be named after what they do, not after where
> you jump from (e.g. to avoid mistakes like the above). Perhaps you can
> clean up the existing labels before adding the new one.
Ack, I wouldn't mind giving this some cleanup.

Konrad

Re: [PATCH 1/6] media: dt-bindings: Document SC8280XP/SM8350 Venus

[Krzysztof Kozlowski]

On 07/08/2023 14:41, Konrad Dybcio wrote:
> On 5.08.2023 21:29, Krzysztof Kozlowski wrote:
>> On 04/08/2023 22:09, Konrad Dybcio wrote:
>>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able
>>> to clock it a bit higher.
>>>
>>
>> ...
>>
>>> +
>>> +  iommus:
>>> +    maxItems: 1
>>> +
>>> +  video-decoder:
>>> +    type: object
>>> +
>>> +    properties:
>>> +      compatible:
>>> +        const: venus-decoder
>>
>> That's not how compatibles are constructed... missing vendor prefix, SoC
>> or IP block name.
>>
>>> +
>>> +    required:
>>> +      - compatible
>>> +
>>> +    additionalProperties: false
>>
>> Why do you need this child node? Child nodes without properties are
>> usually useless.
> For both comments: I aligned with what was there..
> 
> The driver abuses these compats to probe enc/dec submodules, even though
> every Venus implementation (to my knowledge) is implicitly enc/dec capable..

Holy crap, I see...

> 
> Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec
> devices from the venus core probe and get rid of this fake stuff?

Few devices (qcom,msm8996-venus.yaml, sdm660, sdm845) have clocks there,
so we actually could stay with these subnodes, just correct the
compatibles to a list with correct prefixes:

qcom,sc8280xp-venus-decoder + qcom,venus-decoder

Best regards,
Krzysztof

Re: [PATCH 1/6] media: dt-bindings: Document SC8280XP/SM8350 Venus

[Konrad Dybcio]

On 7.08.2023 16:04, Krzysztof Kozlowski wrote:
> On 07/08/2023 14:41, Konrad Dybcio wrote:
>> On 5.08.2023 21:29, Krzysztof Kozlowski wrote:
>>> On 04/08/2023 22:09, Konrad Dybcio wrote:
>>>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able
>>>> to clock it a bit higher.
>>>>
>>>
>>> ...
>>>
>>>> +
>>>> +  iommus:
>>>> +    maxItems: 1
>>>> +
>>>> +  video-decoder:
>>>> +    type: object
>>>> +
>>>> +    properties:
>>>> +      compatible:
>>>> +        const: venus-decoder
>>>
>>> That's not how compatibles are constructed... missing vendor prefix, SoC
>>> or IP block name.
>>>
>>>> +
>>>> +    required:
>>>> +      - compatible
>>>> +
>>>> +    additionalProperties: false
>>>
>>> Why do you need this child node? Child nodes without properties are
>>> usually useless.
>> For both comments: I aligned with what was there..
>>
>> The driver abuses these compats to probe enc/dec submodules, even though
>> every Venus implementation (to my knowledge) is implicitly enc/dec capable..
> 
> Holy crap, I see...
> 
>>
>> Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec
>> devices from the venus core probe and get rid of this fake stuff?
> 
> Few devices (qcom,msm8996-venus.yaml, sdm660, sdm845) have clocks there,
> so we actually could stay with these subnodes, just correct the
> compatibles to a list with correct prefixes:
> 
> qcom,sc8280xp-venus-decoder + qcom,venus-decoder
Hm.. looks like pre-845-v2 (with the v2 being "v2 binding" and not
"v2 chip" or "v2 hardware") these were used to look up clocks but
then they were moved to the root node.

I am not quite sure if it makes sense to distinguish e.g.
sc8280xp-venus-decoder within sc8280xp-venus..

Perhaps deprecating the "8916 way" (clocks under subnodes), adding
some boilerplate to look up clocks/pds in both places and converting
everybody to the "7180 way" way of doing things (clocks under venus),
and then getting rid of venus encoder/decoder completely (by calling
device creation from venus probe) would be better. WDYT?

Konrad

Re: [PATCH 1/6] media: dt-bindings: Document SC8280XP/SM8350 Venus

[Krzysztof Kozlowski]

On 07/08/2023 17:02, Konrad Dybcio wrote:
> On 7.08.2023 16:04, Krzysztof Kozlowski wrote:
>> On 07/08/2023 14:41, Konrad Dybcio wrote:
>>> On 5.08.2023 21:29, Krzysztof Kozlowski wrote:
>>>> On 04/08/2023 22:09, Konrad Dybcio wrote:
>>>>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able
>>>>> to clock it a bit higher.
>>>>>
>>>>
>>>> ...
>>>>
>>>>> +
>>>>> +  iommus:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  video-decoder:
>>>>> +    type: object
>>>>> +
>>>>> +    properties:
>>>>> +      compatible:
>>>>> +        const: venus-decoder
>>>>
>>>> That's not how compatibles are constructed... missing vendor prefix, SoC
>>>> or IP block name.
>>>>
>>>>> +
>>>>> +    required:
>>>>> +      - compatible
>>>>> +
>>>>> +    additionalProperties: false
>>>>
>>>> Why do you need this child node? Child nodes without properties are
>>>> usually useless.
>>> For both comments: I aligned with what was there..
>>>
>>> The driver abuses these compats to probe enc/dec submodules, even though
>>> every Venus implementation (to my knowledge) is implicitly enc/dec capable..
>>
>> Holy crap, I see...
>>
>>>
>>> Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec
>>> devices from the venus core probe and get rid of this fake stuff?
>>
>> Few devices (qcom,msm8996-venus.yaml, sdm660, sdm845) have clocks there,
>> so we actually could stay with these subnodes, just correct the
>> compatibles to a list with correct prefixes:
>>
>> qcom,sc8280xp-venus-decoder + qcom,venus-decoder
> Hm.. looks like pre-845-v2 (with the v2 being "v2 binding" and not
> "v2 chip" or "v2 hardware") these were used to look up clocks but
> then they were moved to the root node.
> 
> I am not quite sure if it makes sense to distinguish e.g.
> sc8280xp-venus-decoder within sc8280xp-venus..>
> Perhaps deprecating the "8916 way" (clocks under subnodes), adding
> some boilerplate to look up clocks/pds in both places and converting
> everybody to the "7180 way" way of doing things (clocks under venus),
> and then getting rid of venus encoder/decoder completely (by calling
> device creation from venus probe) would be better. WDYT?

Yes, this makes more sense. I think this is controlled by
"legacy_binding" variable (see
drivers/media/platform/qcom/venus/pm_helpers.c).

Once all bindings are converted to new one (clocks in main/parent node),
then we can drop the children and instantiate devices as MFD.

Best regards,
Krzysztof

Re: [PATCH 1/6] media: dt-bindings: Document SC8280XP/SM8350 Venus

[Bryan O'Donoghue]

On 07/08/2023 16:02, Konrad Dybcio wrote:
> On 7.08.2023 16:04, Krzysztof Kozlowski wrote:
>> On 07/08/2023 14:41, Konrad Dybcio wrote:
>>> On 5.08.2023 21:29, Krzysztof Kozlowski wrote:
>>>> On 04/08/2023 22:09, Konrad Dybcio wrote:
>>>>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able
>>>>> to clock it a bit higher.
>>>>>
>>>>
>>>> ...
>>>>
>>>>> +
>>>>> +  iommus:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  video-decoder:
>>>>> +    type: object
>>>>> +
>>>>> +    properties:
>>>>> +      compatible:
>>>>> +        const: venus-decoder
>>>>
>>>> That's not how compatibles are constructed... missing vendor prefix, SoC
>>>> or IP block name.
>>>>
>>>>> +
>>>>> +    required:
>>>>> +      - compatible
>>>>> +
>>>>> +    additionalProperties: false
>>>>
>>>> Why do you need this child node? Child nodes without properties are
>>>> usually useless.
>>> For both comments: I aligned with what was there..
>>>
>>> The driver abuses these compats to probe enc/dec submodules, even though
>>> every Venus implementation (to my knowledge) is implicitly enc/dec capable..
>>
>> Holy crap, I see...
>>
>>>
>>> Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec
>>> devices from the venus core probe and get rid of this fake stuff?
>>
>> Few devices (qcom,msm8996-venus.yaml, sdm660, sdm845) have clocks there,
>> so we actually could stay with these subnodes, just correct the
>> compatibles to a list with correct prefixes:
>>
>> qcom,sc8280xp-venus-decoder + qcom,venus-decoder
> Hm.. looks like pre-845-v2 (with the v2 being "v2 binding" and not
> "v2 chip" or "v2 hardware") these were used to look up clocks but
> then they were moved to the root node.
> 
> I am not quite sure if it makes sense to distinguish e.g.
> sc8280xp-venus-decoder within sc8280xp-venus..
> 
> Perhaps deprecating the "8916 way" (clocks under subnodes), adding
> some boilerplate to look up clocks/pds in both places and converting
> everybody to the "7180 way" way of doing things (clocks under venus),
> and then getting rid of venus encoder/decoder completely (by calling
> device creation from venus probe) would be better. WDYT?
> 
> Konrad

As I understand it though, for some classes of venus hardware - earlier, 
it was possible to have two encoders or two decoders and it really 
didn't - perhaps still doesn't matter which order they are declared in.

That's the logic behind having a compat string that assigns either 
encoder or decoder to one of the logical blocks.

You can have any mixture of
- encoder
- decoder

- encoder
- encoder

- decoder
- decoder

- decoder
- encoder

- encoder

- decoder

I think it should *still* be the case - whether it is a practical 
reality or not, that any of those mapping can be selected and supported.

---
bod

Re: [PATCH 1/6] media: dt-bindings: Document SC8280XP/SM8350 Venus

[Konrad Dybcio]

On 7.08.2023 20:44, Bryan O'Donoghue wrote:
> On 07/08/2023 16:02, Konrad Dybcio wrote:
>> On 7.08.2023 16:04, Krzysztof Kozlowski wrote:
>>> On 07/08/2023 14:41, Konrad Dybcio wrote:
>>>> On 5.08.2023 21:29, Krzysztof Kozlowski wrote:
>>>>> On 04/08/2023 22:09, Konrad Dybcio wrote:
>>>>>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able
>>>>>> to clock it a bit higher.
>>>>>>
>>>>>
>>>>> ...
>>>>>
>>>>>> +
>>>>>> +  iommus:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  video-decoder:
>>>>>> +    type: object
>>>>>> +
>>>>>> +    properties:
>>>>>> +      compatible:
>>>>>> +        const: venus-decoder
>>>>>
>>>>> That's not how compatibles are constructed... missing vendor prefix, SoC
>>>>> or IP block name.
>>>>>
>>>>>> +
>>>>>> +    required:
>>>>>> +      - compatible
>>>>>> +
>>>>>> +    additionalProperties: false
>>>>>
>>>>> Why do you need this child node? Child nodes without properties are
>>>>> usually useless.
>>>> For both comments: I aligned with what was there..
>>>>
>>>> The driver abuses these compats to probe enc/dec submodules, even though
>>>> every Venus implementation (to my knowledge) is implicitly enc/dec capable..
>>>
>>> Holy crap, I see...
>>>
>>>>
>>>> Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec
>>>> devices from the venus core probe and get rid of this fake stuff?
>>>
>>> Few devices (qcom,msm8996-venus.yaml, sdm660, sdm845) have clocks there,
>>> so we actually could stay with these subnodes, just correct the
>>> compatibles to a list with correct prefixes:
>>>
>>> qcom,sc8280xp-venus-decoder + qcom,venus-decoder
>> Hm.. looks like pre-845-v2 (with the v2 being "v2 binding" and not
>> "v2 chip" or "v2 hardware") these were used to look up clocks but
>> then they were moved to the root node.
>>
>> I am not quite sure if it makes sense to distinguish e.g.
>> sc8280xp-venus-decoder within sc8280xp-venus..
>>
>> Perhaps deprecating the "8916 way" (clocks under subnodes), adding
>> some boilerplate to look up clocks/pds in both places and converting
>> everybody to the "7180 way" way of doing things (clocks under venus),
>> and then getting rid of venus encoder/decoder completely (by calling
>> device creation from venus probe) would be better. WDYT?
>>
>> Konrad
> 
> As I understand it though, for some classes of venus hardware - earlier, it was possible to have two encoders or two decoders and it really didn't - perhaps still doesn't matter which order they are declared in.
> 
> That's the logic behind having a compat string that assigns either encoder or decoder to one of the logical blocks.
> 
> You can have any mixture of
> - encoder
> - decoder
> 
> - encoder
> - encoder
> 
> - decoder
> - decoder
> 
> - decoder
> - encoder
> 
> - encoder
> 
> - decoder
> 
> I think it should *still* be the case - whether it is a practical reality or not, that any of those mapping can be selected and supported.
That can be taken care of with match data.

Konrad

Re: [PATCH 1/6] media: dt-bindings: Document SC8280XP/SM8350 Venus

[Bryan O'Donoghue]

On 07/08/2023 19:45, Konrad Dybcio wrote:
> That can be taken care of with match data.
> 
> Konrad

Well perhaps.

I'm just sticking my oar in, to elucidate.

The compat sub-nodes aren't just a random choice with no logic. They 
exist to select between what you assign the blocks to be, encoder, 
decoder or any admixture thereof.

A functionality we want to maintain.

---
bod

Re: [PATCH 1/6] media: dt-bindings: Document SC8280XP/SM8350 Venus

[Konrad Dybcio]

On 7.08.2023 20:49, Bryan O'Donoghue wrote:
> On 07/08/2023 19:45, Konrad Dybcio wrote:
>> That can be taken care of with match data.
>>
>> Konrad
> 
> Well perhaps.
> 
> I'm just sticking my oar in, to elucidate.
> 
> The compat sub-nodes aren't just a random choice with no logic. They exist to select between what you assign the blocks to be, encoder, decoder or any admixture thereof.
> 
> A functionality we want to maintain.
Surely something like a modparam would be more suitable here?

Konrad

Re: [PATCH 1/6] media: dt-bindings: Document SC8280XP/SM8350 Venus

[Bryan O'Donoghue]

On 07/08/2023 19:55, Konrad Dybcio wrote:
> On 7.08.2023 20:49, Bryan O'Donoghue wrote:
>> On 07/08/2023 19:45, Konrad Dybcio wrote:
>>> That can be taken care of with match data.
>>>
>>> Konrad
>>
>> Well perhaps.
>>
>> I'm just sticking my oar in, to elucidate.
>>
>> The compat sub-nodes aren't just a random choice with no logic. They exist to select between what you assign the blocks to be, encoder, decoder or any admixture thereof.
>>
>> A functionality we want to maintain.
> Surely something like a modparam would be more suitable here?
> 
> Konrad

Hmm.

Well from earlier in the thread the question "why do we have these 
compat strings" is because we can have any combination of 
encoder/decoder assigned.

If there's a cogent argument _still_ to be made to transition to some 
new way of assignment then fine so long as we don't break that basic 
flexibility.

Though my own €0.02 is that a module parameter is more of a PITA than a 
compat string.

OTOH I could make the argument, that the high probability is most people 
- probably all, just instantiate a single encoder and decoder and aren't 
aware of or using the inbuilt flexibility.

@stan probably has the right idea what to do.

---
bod

Re: [PATCH 1/6] media: dt-bindings: Document SC8280XP/SM8350 Venus

[Konrad Dybcio]

On 7.08.2023 21:05, Bryan O'Donoghue wrote:
> On 07/08/2023 19:55, Konrad Dybcio wrote:
>> On 7.08.2023 20:49, Bryan O'Donoghue wrote:
>>> On 07/08/2023 19:45, Konrad Dybcio wrote:
>>>> That can be taken care of with match data.
>>>>
>>>> Konrad
>>>
>>> Well perhaps.
>>>
>>> I'm just sticking my oar in, to elucidate.
>>>
>>> The compat sub-nodes aren't just a random choice with no logic. They exist to select between what you assign the blocks to be, encoder, decoder or any admixture thereof.
>>>
>>> A functionality we want to maintain.
>> Surely something like a modparam would be more suitable here?
>>
>> Konrad
> 
> Hmm.
> 
> Well from earlier in the thread the question "why do we have these compat strings" is because we can have any combination of encoder/decoder assigned.
> 
> If there's a cogent argument _still_ to be made to transition to some new way of assignment then fine so long as we don't break that basic flexibility.
> 
> Though my own €0.02 is that a module parameter is more of a PITA than a compat string.
> 
> OTOH I could make the argument, that the high probability is most people - probably all, just instantiate a single encoder and decoder and aren't aware of or using the inbuilt flexibility.
> 
> @stan probably has the right idea what to do.
Actually..

Has anybody tested this, ever, with the mainline driver?

Do we have anyone using this?

Is anybody willing to maintain that, test for regressions and
fix them in a reasonable amount of time?


If we don't have at least 2x "yes" here, I don't think it makes sense
to worry about it..

Konrad

Re: [PATCH 1/6] media: dt-bindings: Document SC8280XP/SM8350 Venus

[Bryan O'Donoghue]

On 09/08/2023 13:15, Konrad Dybcio wrote:
>> Hmm.
>>
>> Well from earlier in the thread the question "why do we have these compat strings" is because we can have any combination of encoder/decoder assigned.
>>
>> If there's a cogent argument_still_  to be made to transition to some new way of assignment then fine so long as we don't break that basic flexibility.
>>
>> Though my own €0.02 is that a module parameter is more of a PITA than a compat string.
>>
>> OTOH I could make the argument, that the high probability is most people - probably all, just instantiate a single encoder and decoder and aren't aware of or using the inbuilt flexibility.
>>
>> @stan probably has the right idea what to do.
> Actually..
> 
> Has anybody tested this, ever, with the mainline driver?

I assume Stan has.

> Do we have anyone using this?
Can't say.

> Is anybody willing to maintain that, test for regressions and
> fix them in a reasonable amount of time?
> 
> 
> If we don't have at least 2x "yes" here, I don't think it makes sense
> to worry about it..

Hmm.

We decide if we are encoding or decoding when we init a session and the 
blocks are symmetrical. The hw blocks themselves are not bound to a 
particular encode/decode mode.

Having two parallel encoders or decoders is exactly the same effort as 
having a parallel encoder/decoder.

We don't test parallel encoding/decoding but we should. I'd not be 
surprised to find there are bugs but, that's not a reason to exclude 
rather to find and fix bugs.

---
bod

Re: [PATCH 4/6] media: platform: venus: Add optional LLCC path

[Konrad Dybcio]

On 4.08.2023 23:04, Bryan O'Donoghue wrote:
> On 04/08/2023 21:09, Konrad Dybcio wrote:
>> Some newer SoCs (such as SM8350) have a third interconnect path. Add
>> it and make it optional.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
[...]

> I would scream if someone left me this comment but...
> 
> In probe we have
> 
> video_path =
> cpu_cfgpath =
> 
> llc_path =
> 
> suspend
> 
> icc_set_bw(cpu_cfgpath,);
> icc_set_bw(llc_path,);
> icc_set_bw(video_path,);
> 
> resume
> icc_set_bw(video_path,);
> icc_set_bw(llc_path,);
> icc_set_bw(cpu_cfgpath,);
suspend == resume[::-1] is totally the right thing, but I'll
reorder things in probe for your viewing pleasure

Konrad

Re: [PATCH 4/6] media: platform: venus: Add optional LLCC path

[Konrad Dybcio]

On 4.08.2023 23:06, Bryan O'Donoghue wrote:
> On 04/08/2023 22:04, Bryan O'Donoghue wrote:
>> you can get for llc_path == NULL
> 
> [sic] You can test.
Even better, I can just throw it into icc APIs as-is, as they
nullcheck internally

Konrad

Re: [PATCH 6/6] media: venus: core: Add SC8280XP resource struct

[Bryan O'Donoghue]

On 04/08/2023 21:09, Konrad Dybcio wrote:
> Add SC8280XP configuration data and related compatible.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/core.c | 45 ++++++++++++++++++++++++++++++++
>   1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 5f285ae75e9d..32591b624a36 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c

Reviewing this series, I think my input here has not been helpful or 
correct.

1. Declaring encoders/decoders in dts or yaml is wrong, accepted.

2. We can make a platform choice to hard-code that here in the
    platform declarations.

3. Remove the requirement from yaml for sc8280xp to declare decoder
    encoder

3. Profit.

Existing dtb all, literally all do the same thing first block decoder, 
second block encoder.

Rather than perform extensive surgery to venus to remediate the original 
yaml sin - hard-code decoder/encoder into platform code and deprecate 
the legacy over time.

Yes that means fixing to block 0 as decoder and block 1 as encoder but 
that is the defacto situation we have now, we may as well make it dejure.

---
bod