[PATCH v4 0/4] media: venus: enable venus on qcs615

[PATCH v4 0/4] media: venus: enable venus on qcs615

[Renjiang Han]

QCS615 uses the same video core as SC7180, so reuse the same resource
data of SC7180 for QCS615 to enable video functionality.

Validated this series on QCS615 and SC7180.

Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
---
Changes in v4:
- 1. Remove qcom,qcs615-venus.yaml and use qcom,sc7180-venus.yaml for
qcs615 dt-bindings.
- 2. Add "qcom,qcs615-venus" compatible into qcom,sc7180-venus.yaml.
- 3. Remove qcs615 resource from the driver and use sc7180 resource for
the qcs615.
- 4. Use the frequency in the opp-table in devicetree for the driver.
For compatibility, if getting data from the opp table fails, the data
in the frequency table will be used.
- 5. Keep the reverse Christmas tree order coding style.
- 6. Add "qcom,sc7180-venus" compatible in devicetree.
- 7. Update cover letter message.
- Link to v3: https://lore.kernel.org/r/20241125-add-venus-for-qcs615-v3-0-5a376b97a68e@quicinc.com

Changes in v3:
- 1. Remove the ‘|’ after 'description' in the qcom,qcs615-venus.yaml.
- 2. Add a blank line before 'opp-table' in the qcom,qcs615-venus.yaml.
- 3. Put ‘additionalProperties’ before ‘properties’ in the
qcom,qcs615-venus.yaml.
- 4. Update the subject of qcom,qcs615-venus.yaml patch.
- Link to v2: https://lore.kernel.org/r/20241112-add-venus-for-qcs615-v2-0-e67947f957af@quicinc.com

Changes in v2:
- 1. The change-id of DT and driver are removed.
- 2. Add qcom,qcs615-venus.yaml files to explain DT.
- 3. The order of driver's commit and DT's commit is adjusted. Place the
driver's commit before the DT's commit.
- 4. Extends driver's commit message.
- 5. Split DT's commit into two commits. Add the venus node to the
qcs615.dtsi file. Then in the qcs615-ride.dts file enable the venus node.
- 6. Modify alignment, sort, upper and lower case letters issue.
- 7. Update cover letter message description.
- Link to v1: https://lore.kernel.org/r/20241008-add_qcs615_video-v1-0-436ce07bfc63@quicinc.com

---
Renjiang Han (4):
      dt-bindings: media: add support for video hardware
      media: venus: core: use opp-table for the frequency
      arm64: dts: qcom: add venus node for the qcs615
      arm64: dts: qcom: qcs615-ride: enable venus node

 .../bindings/media/qcom,sc7180-venus.yaml          |  8 +-
 arch/arm64/boot/dts/qcom/qcs615-ride.dts           |  4 +
 arch/arm64/boot/dts/qcom/qcs615.dtsi               | 86 ++++++++++++++++++++++
 drivers/media/platform/qcom/venus/pm_helpers.c     | 67 +++++++++++------
 4 files changed, 143 insertions(+), 22 deletions(-)
---
base-commit: 3e42dc9229c5950e84b1ed705f94ed75ed208228
change-id: 20241213-add-venus-for-qcs615-9176992189e5
prerequisite-message-id: <20241108-qcs615-mm-dt-nodes-v1-0-b2669cac0624@quicinc.com>
prerequisite-patch-id: bcb1328b70868bb9c87c0e4c48e5c9d38853bc60
prerequisite-patch-id: 8844a4661902eb44406639a3b7344416a0c88ed9
prerequisite-message-id: <20241108-qcs615-mm-clockcontroller-v3-0-7d3b2d235fdf@quicinc.com>
prerequisite-patch-id: 748a4e51bbedae9c6ebdbd642b2fd1badf958788
prerequisite-patch-id: 72a894a3b19fdbd431e1cec9397365bc5b27abfe
prerequisite-patch-id: da2b7a74f1afd58833c6a9a4544a0e271720641f
prerequisite-patch-id: 40b79fe0b9101f5db3bddad23551c1123572aee5
prerequisite-patch-id: cb93e5798f6bfe8cc3044c4ce973e3ae5f20dc6b
prerequisite-patch-id: 13b0dbf97ac1865d241791afb4b46a28ca499523
prerequisite-patch-id: 807019bedabd47c04f7ac78e9461d0b5a6e9131b
prerequisite-patch-id: 8e2e841401fefbd96d78dd4a7c47514058c83bf2
prerequisite-patch-id: 125bb8cb367109ba22cededf6e78754579e1ed03
prerequisite-patch-id: b3cc42570d5826a4704f7702e7b26af9a0fe57b0
prerequisite-patch-id: df8e2fdd997cbf6c0a107f1871ed9e2caaa97582

Best regards,
-- 
Renjiang Han <quic_renjiang@quicinc.com>

[PATCH v4 1/4] dt-bindings: media: add support for video hardware

[Renjiang Han]

Add qcom,qcs615-venus compatible into qcom,sc7180-venus.yaml for the
video, and let qcom,qcs615-venus fallback to qcom,sc7180-venus on
QCS615 platform.

Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
---
 Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml b/Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml
index 5cec1d077cda77817f6d876109defcb0abbfeb2c..6dee45b7366578e51319b575e5dd2587dc84baeb 100644
--- a/Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml
+++ b/Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml
@@ -18,7 +18,13 @@ allOf:
 
 properties:
   compatible:
-    const: qcom,sc7180-venus
+    oneOf:
+      - items:
+          - enum:
+              - qcom,qcs615-venus
+          - const: qcom,sc7180-venus
+
+      - const: qcom,sc7180-venus
 
   power-domains:
     minItems: 2

-- 
2.34.1

[PATCH v4 2/4] media: venus: core: use opp-table for the frequency

[Renjiang Han]

Get frequency value from the opp-table of devicetree for the v4 core.
For compatibility, if getting data from the opp table fails, the data
in the frequency table will be used.

The order of variable definitions is adjusted only to keep the reverse
Christmas tree order coding style.

Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 67 ++++++++++++++++++--------
 1 file changed, 46 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 33a5a659c0ada0ca97eebb5522c5f349f95c49c7..a5c3f9ad2088d8c80247b52d5c1b8e053f499bfe 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -40,17 +40,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;
+	const struct venus_resources *res = core->res;
+	struct device *dev = core->dev;
+	unsigned long freq = 0;
+	struct dev_pm_opp *opp;
 	unsigned int i;
 	int ret;
 
-	if (!freq_tbl)
-		return -EINVAL;
-
-	freq = freq_tbl[freq_tbl_size - 1].freq;
+	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
+	if (IS_ERR(opp)) {
+		if (!freq_tbl)
+			return -EINVAL;
+		freq = freq_tbl[freq_tbl_size - 1].freq;
+	} else {
+		dev_pm_opp_put(opp);
+	}
 
 	for (i = 0; i < res->clks_num; i++) {
 		if (IS_V6(core)) {
@@ -627,12 +633,15 @@ min_loaded_core(struct venus_inst *inst, u32 *min_coreid, u32 *min_load, bool lo
 
 static int decide_core(struct venus_inst *inst)
 {
+	const struct freq_tbl *freq_tbl = inst->core->res->freq_tbl;
 	const u32 ptype = HFI_PROPERTY_CONFIG_VIDEOCORES_USAGE;
-	struct venus_core *core = inst->core;
-	u32 min_coreid, min_load, cur_inst_load;
 	u32 min_lp_coreid, min_lp_load, cur_inst_lp_load;
+	u32 min_coreid, min_load, cur_inst_load;
+	struct venus_core *core = inst->core;
 	struct hfi_videocores_usage_type cu;
-	unsigned long max_freq;
+	unsigned long max_freq = ULONG_MAX;
+	struct device *dev = core->dev;
+	struct dev_pm_opp *opp;
 	int ret = 0;
 
 	if (legacy_binding) {
@@ -655,7 +664,11 @@ static int decide_core(struct venus_inst *inst)
 	cur_inst_lp_load *= inst->clk_data.low_power_freq;
 	/*TODO : divide this inst->load by work_route */
 
-	max_freq = core->res->freq_tbl[0].freq;
+	opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
+	if (IS_ERR(opp))
+		max_freq = freq_tbl[0].freq;
+	else
+		dev_pm_opp_put(opp);
 
 	min_loaded_core(inst, &min_coreid, &min_load, false);
 	min_loaded_core(inst, &min_lp_coreid, &min_lp_load, true);
@@ -1073,12 +1086,14 @@ static unsigned long calculate_inst_freq(struct venus_inst *inst,
 
 static int load_scale_v4(struct venus_inst *inst)
 {
+	const struct freq_tbl *table = inst->core->res->freq_tbl;
+	unsigned int num_rows = inst->core->res->freq_tbl_size;
+	unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0;
 	struct venus_core *core = inst->core;
-	const struct freq_tbl *table = core->res->freq_tbl;
-	unsigned int num_rows = core->res->freq_tbl_size;
+	unsigned long max_freq = ULONG_MAX;
 	struct device *dev = core->dev;
-	unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0;
 	unsigned long filled_len = 0;
+	struct dev_pm_opp *opp;
 	int i, ret = 0;
 
 	for (i = 0; i < inst->num_input_bufs; i++)
@@ -1104,19 +1119,29 @@ static int load_scale_v4(struct venus_inst *inst)
 
 	freq = max(freq_core1, freq_core2);
 
-	if (freq > table[0].freq) {
-		dev_dbg(dev, VDBGL "requested clock rate: %lu scaling clock rate : %lu\n",
-			freq, table[0].freq);
+	opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
+	if (IS_ERR(opp))
+		max_freq = table[0].freq;
+	else
+		dev_pm_opp_put(opp);
 
-		freq = table[0].freq;
+	if (freq > max_freq) {
+		dev_dbg(dev, VDBGL "requested clock rate: %lu scaling clock rate : %lu\n",
+			freq, max_freq);
+		freq = max_freq;
 		goto set_freq;
 	}
 
-	for (i = num_rows - 1 ; i >= 0; i--) {
-		if (freq <= table[i].freq) {
-			freq = table[i].freq;
-			break;
+	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
+	if (IS_ERR(opp)) {
+		for (i = num_rows - 1 ; i >= 0; i--) {
+			if (freq <= table[i].freq) {
+				freq = table[i].freq;
+				break;
+			}
 		}
+	} else {
+		dev_pm_opp_put(opp);
 	}
 
 set_freq:

-- 
2.34.1

[PATCH v4 3/4] arm64: dts: qcom: add venus node for the qcs615

[Renjiang Han]

Add venus node into devicetree for the qcs615 video and fallback
qcs615 to sc7180 due to the same video core.

Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
---
 arch/arm64/boot/dts/qcom/qcs615.dtsi | 86 ++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
index 37a189e0834d2f4b75ed9deb6fff73da163cb3a3..c08da80c7fd8fa8c69aff04b14784b821ce3ea13 100644
--- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
@@ -427,6 +427,11 @@ smem_region: smem@86000000 {
 			no-map;
 			hwlocks = <&tcsr_mutex 3>;
 		};
+
+		pil_video_mem: pil-video@93400000 {
+			reg = <0x0 0x93400000 0x0 0x500000>;
+			no-map;
+		};
 	};
 
 	soc: soc@0 {
@@ -2806,6 +2811,87 @@ gem_noc: interconnect@9680000 {
 			qcom,bcm-voters = <&apps_bcm_voter>;
 		};
 
+		venus: video-codec@aa00000 {
+			compatible = "qcom,qcs615-venus", "qcom,sc7180-venus";
+			reg = <0x0 0x0aa00000 0x0 0x100000>;
+			interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
+
+			clocks = <&videocc VIDEO_CC_VENUS_CTL_CORE_CLK>,
+				 <&videocc VIDEO_CC_VENUS_AHB_CLK>,
+				 <&videocc VIDEO_CC_VENUS_CTL_AXI_CLK>,
+				 <&videocc VIDEO_CC_VCODEC0_CORE_CLK>,
+				 <&videocc VIDEO_CC_VCODEC0_AXI_CLK>;
+			clock-names = "core",
+				      "iface",
+				      "bus",
+				      "vcodec0_core",
+				      "vcodec0_bus";
+
+			power-domains = <&videocc VENUS_GDSC>,
+					<&videocc VCODEC0_GDSC>,
+					<&rpmhpd RPMHPD_CX>;
+			power-domain-names = "venus",
+					     "vcodec0",
+					     "cx";
+
+			operating-points-v2 = <&venus_opp_table>;
+
+			interconnects = <&mmss_noc MASTER_VIDEO_P0 QCOM_ICC_TAG_ALWAYS
+					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
+					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
+					 &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ALWAYS>;
+			interconnect-names = "video-mem",
+					     "cpu-cfg";
+
+			iommus = <&apps_smmu 0xe40 0x20>;
+
+			memory-region = <&pil_video_mem>;
+
+			status = "disabled";
+
+			video-decoder {
+				compatible = "venus-decoder";
+			};
+
+			video-encoder {
+				compatible = "venus-encoder";
+			};
+
+			venus_opp_table: opp-table {
+				compatible = "operating-points-v2";
+
+				opp-133330000 {
+					opp-hz = /bits/ 64 <133330000>;
+					required-opps = <&rpmhpd_opp_low_svs>;
+				};
+
+				opp-240000000 {
+					opp-hz = /bits/ 64 <240000000>;
+					required-opps = <&rpmhpd_opp_svs>;
+				};
+
+				opp-300000000 {
+					opp-hz = /bits/ 64 <300000000>;
+					required-opps = <&rpmhpd_opp_svs_l1>;
+				};
+
+				opp-380000000 {
+					opp-hz = /bits/ 64 <380000000>;
+					required-opps = <&rpmhpd_opp_nom>;
+				};
+
+				opp-410000000 {
+					opp-hz = /bits/ 64 <410000000>;
+					required-opps = <&rpmhpd_opp_turbo>;
+				};
+
+				opp-460000000 {
+					opp-hz = /bits/ 64 <460000000>;
+					required-opps = <&rpmhpd_opp_turbo_l1>;
+				};
+			};
+		};
+
 		videocc: clock-controller@ab00000 {
 			compatible = "qcom,qcs615-videocc";
 			reg = <0 0xab00000 0 0x10000>;

-- 
2.34.1

[PATCH v4 4/4] arm64: dts: qcom: qcs615-ride: enable venus node

[Renjiang Han]

Enable the venus node so that the video codec will start working.

Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
---
 arch/arm64/boot/dts/qcom/qcs615-ride.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
index a25928933e2b66241258e418c6e5bc36c306101e..de954ede27f0942397d982f9aa725e59a8de9657 100644
--- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts
+++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
@@ -237,6 +237,10 @@ &usb_1_dwc3 {
 	dr_mode = "peripheral";
 };
 
+&venus {
+	status = "okay";
+};
+
 &watchdog {
 	clocks = <&sleep_clk>;
 };

-- 
2.34.1

Re: [PATCH v4 3/4] arm64: dts: qcom: add venus node for the qcs615

[Bryan O'Donoghue]

On 13/12/2024 09:56, Renjiang Han wrote:
> +			video-decoder {
> +				compatible = "venus-decoder";
> +			};
> +
> +			video-encoder {
> +				compatible = "venus-encoder";
> +			};

To be honest, I'd prefer not to continue on doing this.

Adding a compat string to existing yaml might work-around the issue but, 
it doesn't really _account_ for the issue.

I've posted a series to fix the problem

20241209-media-staging-24-11-25-rb3-hw-compat-string-v5-0-ef7e5f85f302@linaro.org

Either that or deeper rewrite of venus to remove the above dependencies 
should go ahead.

I'm not in favour of willfully continuing to do the wrong thing, 
especially when per above, there's a working solution for it.

---
bod

Re: [PATCH v4 1/4] dt-bindings: media: add support for video hardware

[Krzysztof Kozlowski]

On Fri, Dec 13, 2024 at 03:26:46PM +0530, Renjiang Han wrote:
> Add qcom,qcs615-venus compatible into qcom,sc7180-venus.yaml for the
> video, and let qcom,qcs615-venus fallback to qcom,sc7180-venus on
> QCS615 platform.
> 
> Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
> ---
>  Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml b/Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml
> index 5cec1d077cda77817f6d876109defcb0abbfeb2c..6dee45b7366578e51319b575e5dd2587dc84baeb 100644
> --- a/Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml
> @@ -18,7 +18,13 @@ allOf:
>  
>  properties:
>    compatible:
> -    const: qcom,sc7180-venus
> +    oneOf:
> +      - items:
> +          - enum:
> +              - qcom,qcs615-venus
> +          - const: qcom,sc7180-venus
> +

Drop blank line.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v4 3/4] arm64: dts: qcom: add venus node for the qcs615

[Bjorn Andersson]

On Fri, Dec 13, 2024 at 03:26:48PM +0530, Renjiang Han wrote:

Subject should be prefixed per the file being changed, i.e:
"arm64: dts: qcom: qcs615: Add Venus"

> Add venus node into devicetree for the qcs615 video and fallback
> qcs615 to sc7180 due to the same video core.
> 
> Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/qcs615.dtsi | 86 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> index 37a189e0834d2f4b75ed9deb6fff73da163cb3a3..c08da80c7fd8fa8c69aff04b14784b821ce3ea13 100644
> --- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> @@ -427,6 +427,11 @@ smem_region: smem@86000000 {
>  			no-map;
>  			hwlocks = <&tcsr_mutex 3>;
>  		};
> +
> +		pil_video_mem: pil-video@93400000 {
> +			reg = <0x0 0x93400000 0x0 0x500000>;
> +			no-map;
> +		};
>  	};
>  
>  	soc: soc@0 {
> @@ -2806,6 +2811,87 @@ gem_noc: interconnect@9680000 {
>  			qcom,bcm-voters = <&apps_bcm_voter>;
>  		};
>  
> +		venus: video-codec@aa00000 {
> +			compatible = "qcom,qcs615-venus", "qcom,sc7180-venus";
> +			reg = <0x0 0x0aa00000 0x0 0x100000>;
> +			interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
> +
> +			clocks = <&videocc VIDEO_CC_VENUS_CTL_CORE_CLK>,
> +				 <&videocc VIDEO_CC_VENUS_AHB_CLK>,
> +				 <&videocc VIDEO_CC_VENUS_CTL_AXI_CLK>,
> +				 <&videocc VIDEO_CC_VCODEC0_CORE_CLK>,
> +				 <&videocc VIDEO_CC_VCODEC0_AXI_CLK>;
> +			clock-names = "core",
> +				      "iface",
> +				      "bus",
> +				      "vcodec0_core",
> +				      "vcodec0_bus";
> +
> +			power-domains = <&videocc VENUS_GDSC>,
> +					<&videocc VCODEC0_GDSC>,
> +					<&rpmhpd RPMHPD_CX>;
> +			power-domain-names = "venus",
> +					     "vcodec0",
> +					     "cx";
> +
> +			operating-points-v2 = <&venus_opp_table>;
> +
> +			interconnects = <&mmss_noc MASTER_VIDEO_P0 QCOM_ICC_TAG_ALWAYS
> +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> +					 &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ALWAYS>;
> +			interconnect-names = "video-mem",
> +					     "cpu-cfg";
> +
> +			iommus = <&apps_smmu 0xe40 0x20>;
> +
> +			memory-region = <&pil_video_mem>;
> +
> +			status = "disabled";
> +
> +			video-decoder {
> +				compatible = "venus-decoder";
> +			};
> +
> +			video-encoder {
> +				compatible = "venus-encoder";
> +			};
> +
> +			venus_opp_table: opp-table {

'o' < 'v', so this should come above video-decoder.

Regards,
Bjorn

> +				compatible = "operating-points-v2";
> +
> +				opp-133330000 {
> +					opp-hz = /bits/ 64 <133330000>;
> +					required-opps = <&rpmhpd_opp_low_svs>;
> +				};
> +
> +				opp-240000000 {
> +					opp-hz = /bits/ 64 <240000000>;
> +					required-opps = <&rpmhpd_opp_svs>;
> +				};
> +
> +				opp-300000000 {
> +					opp-hz = /bits/ 64 <300000000>;
> +					required-opps = <&rpmhpd_opp_svs_l1>;
> +				};
> +
> +				opp-380000000 {
> +					opp-hz = /bits/ 64 <380000000>;
> +					required-opps = <&rpmhpd_opp_nom>;
> +				};
> +
> +				opp-410000000 {
> +					opp-hz = /bits/ 64 <410000000>;
> +					required-opps = <&rpmhpd_opp_turbo>;
> +				};
> +
> +				opp-460000000 {
> +					opp-hz = /bits/ 64 <460000000>;
> +					required-opps = <&rpmhpd_opp_turbo_l1>;
> +				};
> +			};
> +		};
> +
>  		videocc: clock-controller@ab00000 {
>  			compatible = "qcom,qcs615-videocc";
>  			reg = <0 0xab00000 0 0x10000>;
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 2/4] media: venus: core: use opp-table for the frequency

[Bjorn Andersson]

On Fri, Dec 13, 2024 at 03:26:47PM +0530, Renjiang Han wrote:
> Get frequency value from the opp-table of devicetree for the v4 core.
> For compatibility, if getting data from the opp table fails, the data
> in the frequency table will be used.
> 
> The order of variable definitions is adjusted only to keep the reverse
> Christmas tree order coding style.
> 

1) Do the best you can to add your variables while trying to maintain
the order, but if it's not possible better leave it than making it hard
to parse logical change from shuffling of code.

2) This comment is useful during review, but not necessarily so in the
git history, so I'd suggest to keep it below the '---' line.

> Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
> ---
>  drivers/media/platform/qcom/venus/pm_helpers.c | 67 ++++++++++++++++++--------
>  1 file changed, 46 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 33a5a659c0ada0ca97eebb5522c5f349f95c49c7..a5c3f9ad2088d8c80247b52d5c1b8e053f499bfe 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -40,17 +40,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;
> +	const struct venus_resources *res = core->res;
> +	struct device *dev = core->dev;
> +	unsigned long freq = 0;

Is it really necessary to initialize this? I'd expect that
dev_pm_opp_find_freq_ceil() would either initialize freq or return a
failure, in which case you assign freq.

Perhaps the compiler isn't clever enough to see this?

> +	struct dev_pm_opp *opp;
>  	unsigned int i;
>  	int ret;
>  
> -	if (!freq_tbl)
> -		return -EINVAL;
> -
> -	freq = freq_tbl[freq_tbl_size - 1].freq;
> +	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> +	if (IS_ERR(opp)) {
> +		if (!freq_tbl)
> +			return -EINVAL;
> +		freq = freq_tbl[freq_tbl_size - 1].freq;
> +	} else {
> +		dev_pm_opp_put(opp);
> +	}
>  
>  	for (i = 0; i < res->clks_num; i++) {
>  		if (IS_V6(core)) {
> @@ -627,12 +633,15 @@ min_loaded_core(struct venus_inst *inst, u32 *min_coreid, u32 *min_load, bool lo
>  
>  static int decide_core(struct venus_inst *inst)
>  {
> +	const struct freq_tbl *freq_tbl = inst->core->res->freq_tbl;
>  	const u32 ptype = HFI_PROPERTY_CONFIG_VIDEOCORES_USAGE;
> -	struct venus_core *core = inst->core;
> -	u32 min_coreid, min_load, cur_inst_load;
>  	u32 min_lp_coreid, min_lp_load, cur_inst_lp_load;
> +	u32 min_coreid, min_load, cur_inst_load;
> +	struct venus_core *core = inst->core;
>  	struct hfi_videocores_usage_type cu;
> -	unsigned long max_freq;
> +	unsigned long max_freq = ULONG_MAX;
> +	struct device *dev = core->dev;
> +	struct dev_pm_opp *opp;

Here the line shuffling makes it hard to determine what is part of the
logical change and what is just style changes...

Regards,
Bjorn

>  	int ret = 0;
>  
>  	if (legacy_binding) {
> @@ -655,7 +664,11 @@ static int decide_core(struct venus_inst *inst)
>  	cur_inst_lp_load *= inst->clk_data.low_power_freq;
>  	/*TODO : divide this inst->load by work_route */
>  
> -	max_freq = core->res->freq_tbl[0].freq;
> +	opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
> +	if (IS_ERR(opp))
> +		max_freq = freq_tbl[0].freq;
> +	else
> +		dev_pm_opp_put(opp);
>  
>  	min_loaded_core(inst, &min_coreid, &min_load, false);
>  	min_loaded_core(inst, &min_lp_coreid, &min_lp_load, true);
> @@ -1073,12 +1086,14 @@ static unsigned long calculate_inst_freq(struct venus_inst *inst,
>  
>  static int load_scale_v4(struct venus_inst *inst)
>  {
> +	const struct freq_tbl *table = inst->core->res->freq_tbl;
> +	unsigned int num_rows = inst->core->res->freq_tbl_size;
> +	unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0;
>  	struct venus_core *core = inst->core;
> -	const struct freq_tbl *table = core->res->freq_tbl;
> -	unsigned int num_rows = core->res->freq_tbl_size;
> +	unsigned long max_freq = ULONG_MAX;
>  	struct device *dev = core->dev;
> -	unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0;
>  	unsigned long filled_len = 0;
> +	struct dev_pm_opp *opp;
>  	int i, ret = 0;
>  
>  	for (i = 0; i < inst->num_input_bufs; i++)
> @@ -1104,19 +1119,29 @@ static int load_scale_v4(struct venus_inst *inst)
>  
>  	freq = max(freq_core1, freq_core2);
>  
> -	if (freq > table[0].freq) {
> -		dev_dbg(dev, VDBGL "requested clock rate: %lu scaling clock rate : %lu\n",
> -			freq, table[0].freq);
> +	opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
> +	if (IS_ERR(opp))
> +		max_freq = table[0].freq;
> +	else
> +		dev_pm_opp_put(opp);
>  
> -		freq = table[0].freq;
> +	if (freq > max_freq) {
> +		dev_dbg(dev, VDBGL "requested clock rate: %lu scaling clock rate : %lu\n",
> +			freq, max_freq);
> +		freq = max_freq;
>  		goto set_freq;
>  	}
>  
> -	for (i = num_rows - 1 ; i >= 0; i--) {
> -		if (freq <= table[i].freq) {
> -			freq = table[i].freq;
> -			break;
> +	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> +	if (IS_ERR(opp)) {
> +		for (i = num_rows - 1 ; i >= 0; i--) {
> +			if (freq <= table[i].freq) {
> +				freq = table[i].freq;
> +				break;
> +			}
>  		}
> +	} else {
> +		dev_pm_opp_put(opp);
>  	}
>  
>  set_freq:
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 1/4] dt-bindings: media: add support for video hardware

[Renjiang Han]

On 12/16/2024 3:52 PM, Krzysztof Kozlowski wrote:
> On Fri, Dec 13, 2024 at 03:26:46PM +0530, Renjiang Han wrote:
>> Add qcom,qcs615-venus compatible into qcom,sc7180-venus.yaml for the
>> video, and let qcom,qcs615-venus fallback to qcom,sc7180-venus on
>> QCS615 platform.
>>
>> Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
>> ---
>>   Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml b/Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml
>> index 5cec1d077cda77817f6d876109defcb0abbfeb2c..6dee45b7366578e51319b575e5dd2587dc84baeb 100644
>> --- a/Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml
>> +++ b/Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml
>> @@ -18,7 +18,13 @@ allOf:
>>   
>>   properties:
>>     compatible:
>> -    const: qcom,sc7180-venus
>> +    oneOf:
>> +      - items:
>> +          - enum:
>> +              - qcom,qcs615-venus
>> +          - const: qcom,sc7180-venus
>> +
> Drop blank line.
OK, thanks for pointing it out.
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> Best regards,
> Krzysztof
>
-- 
Best Regards,
Renjiang

Re: [PATCH v4 2/4] media: venus: core: use opp-table for the frequency

[Renjiang Han]

On 12/17/2024 2:21 AM, Bjorn Andersson wrote:
> On Fri, Dec 13, 2024 at 03:26:47PM +0530, Renjiang Han wrote:
>> Get frequency value from the opp-table of devicetree for the v4 core.
>> For compatibility, if getting data from the opp table fails, the data
>> in the frequency table will be used.
>>
>> The order of variable definitions is adjusted only to keep the reverse
>> Christmas tree order coding style.
>>
> 1) Do the best you can to add your variables while trying to maintain
> the order, but if it's not possible better leave it than making it hard
> to parse logical change from shuffling of code.
OK, thanks for pointing it out.
>
> 2) This comment is useful during review, but not necessarily so in the
> git history, so I'd suggest to keep it below the '---' line.

  Do you mean that the message about my code style changes only needs

  to be added to the cover letter?
>
>> Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/venus/pm_helpers.c | 67 ++++++++++++++++++--------
>>   1 file changed, 46 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index 33a5a659c0ada0ca97eebb5522c5f349f95c49c7..a5c3f9ad2088d8c80247b52d5c1b8e053f499bfe 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -40,17 +40,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;
>> +	const struct venus_resources *res = core->res;
>> +	struct device *dev = core->dev;
>> +	unsigned long freq = 0;
> Is it really necessary to initialize this? I'd expect that
> dev_pm_opp_find_freq_ceil() would either initialize freq or return a
> failure, in which case you assign freq.

  Thanks for your review. There is really no need to initialize freq.

  The default value of freq is 0 here. dev_pm_opp_find_freq_ceil() will

  query the matching value from the opp table based on this default value.
>
> Perhaps the compiler isn't clever enough to see this?
>
>> +	struct dev_pm_opp *opp;
>>   	unsigned int i;
>>   	int ret;
>>   
>> -	if (!freq_tbl)
>> -		return -EINVAL;
>> -
>> -	freq = freq_tbl[freq_tbl_size - 1].freq;
>> +	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
>> +	if (IS_ERR(opp)) {
>> +		if (!freq_tbl)
>> +			return -EINVAL;
>> +		freq = freq_tbl[freq_tbl_size - 1].freq;
>> +	} else {
>> +		dev_pm_opp_put(opp);
>> +	}
>>   
>>   	for (i = 0; i < res->clks_num; i++) {
>>   		if (IS_V6(core)) {
>> @@ -627,12 +633,15 @@ min_loaded_core(struct venus_inst *inst, u32 *min_coreid, u32 *min_load, bool lo
>>   
>>   static int decide_core(struct venus_inst *inst)
>>   {
>> +	const struct freq_tbl *freq_tbl = inst->core->res->freq_tbl;
>>   	const u32 ptype = HFI_PROPERTY_CONFIG_VIDEOCORES_USAGE;
>> -	struct venus_core *core = inst->core;
>> -	u32 min_coreid, min_load, cur_inst_load;
>>   	u32 min_lp_coreid, min_lp_load, cur_inst_lp_load;
>> +	u32 min_coreid, min_load, cur_inst_load;
>> +	struct venus_core *core = inst->core;
>>   	struct hfi_videocores_usage_type cu;
>> -	unsigned long max_freq;
>> +	unsigned long max_freq = ULONG_MAX;
>> +	struct device *dev = core->dev;
>> +	struct dev_pm_opp *opp;
> Here the line shuffling makes it hard to determine what is part of the
> logical change and what is just style changes...
>
> Regards,
> Bjorn

  You mean I only need to add variable definitions basing on the original

  order of variable definitions, and I don't need to modify the order of

  variable definitions specifically for the reverse Christmas tree code

  style. Is that right?
>
>>   	int ret = 0;
>>   
>>   	if (legacy_binding) {
>> @@ -655,7 +664,11 @@ static int decide_core(struct venus_inst *inst)
>>   	cur_inst_lp_load *= inst->clk_data.low_power_freq;
>>   	/*TODO : divide this inst->load by work_route */
>>   
>> -	max_freq = core->res->freq_tbl[0].freq;
>> +	opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
>> +	if (IS_ERR(opp))
>> +		max_freq = freq_tbl[0].freq;
>> +	else
>> +		dev_pm_opp_put(opp);
>>   
>>   	min_loaded_core(inst, &min_coreid, &min_load, false);
>>   	min_loaded_core(inst, &min_lp_coreid, &min_lp_load, true);
>> @@ -1073,12 +1086,14 @@ static unsigned long calculate_inst_freq(struct venus_inst *inst,
>>   
>>   static int load_scale_v4(struct venus_inst *inst)
>>   {
>> +	const struct freq_tbl *table = inst->core->res->freq_tbl;
>> +	unsigned int num_rows = inst->core->res->freq_tbl_size;
>> +	unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0;
>>   	struct venus_core *core = inst->core;
>> -	const struct freq_tbl *table = core->res->freq_tbl;
>> -	unsigned int num_rows = core->res->freq_tbl_size;
>> +	unsigned long max_freq = ULONG_MAX;
>>   	struct device *dev = core->dev;
>> -	unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0;
>>   	unsigned long filled_len = 0;
>> +	struct dev_pm_opp *opp;
>>   	int i, ret = 0;
>>   
>>   	for (i = 0; i < inst->num_input_bufs; i++)
>> @@ -1104,19 +1119,29 @@ static int load_scale_v4(struct venus_inst *inst)
>>   
>>   	freq = max(freq_core1, freq_core2);
>>   
>> -	if (freq > table[0].freq) {
>> -		dev_dbg(dev, VDBGL "requested clock rate: %lu scaling clock rate : %lu\n",
>> -			freq, table[0].freq);
>> +	opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
>> +	if (IS_ERR(opp))
>> +		max_freq = table[0].freq;
>> +	else
>> +		dev_pm_opp_put(opp);
>>   
>> -		freq = table[0].freq;
>> +	if (freq > max_freq) {
>> +		dev_dbg(dev, VDBGL "requested clock rate: %lu scaling clock rate : %lu\n",
>> +			freq, max_freq);
>> +		freq = max_freq;
>>   		goto set_freq;
>>   	}
>>   
>> -	for (i = num_rows - 1 ; i >= 0; i--) {
>> -		if (freq <= table[i].freq) {
>> -			freq = table[i].freq;
>> -			break;
>> +	opp = dev_pm_opp_find_freq_ceil(dev, &freq);
>> +	if (IS_ERR(opp)) {
>> +		for (i = num_rows - 1 ; i >= 0; i--) {
>> +			if (freq <= table[i].freq) {
>> +				freq = table[i].freq;
>> +				break;
>> +			}
>>   		}
>> +	} else {
>> +		dev_pm_opp_put(opp);
>>   	}
>>   
>>   set_freq:
>>
>> -- 
>> 2.34.1
>>
-- 
Best Regards,
Renjiang

Re: [PATCH v4 3/4] arm64: dts: qcom: add venus node for the qcs615

[Renjiang Han]

On 12/13/2024 7:31 PM, Bryan O'Donoghue wrote:
> On 13/12/2024 09:56, Renjiang Han wrote:
>> +            video-decoder {
>> +                compatible = "venus-decoder";
>> +            };
>> +
>> +            video-encoder {
>> +                compatible = "venus-encoder";
>> +            };
>
> To be honest, I'd prefer not to continue on doing this.
>
> Adding a compat string to existing yaml might work-around the issue 
> but, it doesn't really _account_ for the issue.
>
> I've posted a series to fix the problem
>
> 20241209-media-staging-24-11-25-rb3-hw-compat-string-v5-0-ef7e5f85f302@linaro.org 
>
>
> Either that or deeper rewrite of venus to remove the above 
> dependencies should go ahead.
>
> I'm not in favour of willfully continuing to do the wrong thing, 
> especially when per above, there's a working solution for it.
>
> ---
> bod

Thanks for your review. Your change is a good way. But your change is a

  big change, involving many platforms, I am not sure if other guys have

  comments. Currently, my change only refers to SC7180 and falls back QCS615

  to SC7180. Now I have verified my patches on SC7180 and QCS615. I think if

  your change has passed the review, then we only need to remove the

  video-decoder and video-encoder nodes in this device tree.

-- 
Best Regards,
Renjiang