devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/4] media: venus: enable venus on qcs615
@ 2024-12-17  9:17 Renjiang Han
  2024-12-17  9:17 ` [PATCH v5 1/4] dt-bindings: media: add support for video hardware Renjiang Han
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Renjiang Han @ 2024-12-17  9:17 UTC (permalink / raw)
  To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Bjorn Andersson, Konrad Dybcio
  Cc: linux-media, linux-arm-msm, devicetree, linux-kernel,
	Stanimir Varbanov, 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 v5:
- 1. Remove extra blank lines in yaml files.
- 2. Add new variables in the driver while keeping the order of the
original variables. And remove unnecessary variable initialization.
- 3. Update commit message.
- 4. Update the order of nodes in the device tree.
- Link to v4: https://lore.kernel.org/r/20241213-add-venus-for-qcs615-v4-0-7e2c9a72d309@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: qcs615: add venus node
      arm64: dts: qcom: qcs615-ride: enable venus node

 .../bindings/media/qcom,sc7180-venus.yaml          |  7 +-
 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     | 53 +++++++++----
 4 files changed, 135 insertions(+), 15 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>


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

* [PATCH v5 1/4] dt-bindings: media: add support for video hardware
  2024-12-17  9:17 [PATCH v5 0/4] media: venus: enable venus on qcs615 Renjiang Han
@ 2024-12-17  9:17 ` Renjiang Han
  2024-12-17 12:40   ` Krzysztof Kozlowski
  2024-12-17  9:17 ` [PATCH v5 2/4] media: venus: core: use opp-table for the frequency Renjiang Han
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Renjiang Han @ 2024-12-17  9:17 UTC (permalink / raw)
  To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Bjorn Andersson, Konrad Dybcio
  Cc: linux-media, linux-arm-msm, devicetree, linux-kernel,
	Stanimir Varbanov, 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 | 7 ++++++-
 1 file changed, 6 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..de628709fe43b1f3261820b89c65e6eb4dd352e2 100644
--- a/Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml
+++ b/Documentation/devicetree/bindings/media/qcom,sc7180-venus.yaml
@@ -18,7 +18,12 @@ 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


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

* [PATCH v5 2/4] media: venus: core: use opp-table for the frequency
  2024-12-17  9:17 [PATCH v5 0/4] media: venus: enable venus on qcs615 Renjiang Han
  2024-12-17  9:17 ` [PATCH v5 1/4] dt-bindings: media: add support for video hardware Renjiang Han
@ 2024-12-17  9:17 ` Renjiang Han
  2024-12-17  9:17 ` [PATCH v5 3/4] arm64: dts: qcom: qcs615: add venus node Renjiang Han
  2024-12-17  9:17 ` [PATCH v5 4/4] arm64: dts: qcom: qcs615-ride: enable " Renjiang Han
  3 siblings, 0 replies; 14+ messages in thread
From: Renjiang Han @ 2024-12-17  9:17 UTC (permalink / raw)
  To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Bjorn Andersson, Konrad Dybcio
  Cc: linux-media, linux-arm-msm, devicetree, linux-kernel,
	Stanimir Varbanov, 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.

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

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 33a5a659c0ada0ca97eebb5522c5f349f95c49c7..b61c0ad152878870b7223efa274e137d3636433b 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -43,14 +43,20 @@ 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;
+	struct device *dev = core->dev;
+	struct dev_pm_opp *opp;
 	unsigned long freq;
 	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;
 	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);
@@ -1078,7 +1091,9 @@ static int load_scale_v4(struct venus_inst *inst)
 	unsigned int num_rows = core->res->freq_tbl_size;
 	struct device *dev = core->dev;
 	unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0;
+	unsigned long max_freq = ULONG_MAX;
 	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


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

* [PATCH v5 3/4] arm64: dts: qcom: qcs615: add venus node
  2024-12-17  9:17 [PATCH v5 0/4] media: venus: enable venus on qcs615 Renjiang Han
  2024-12-17  9:17 ` [PATCH v5 1/4] dt-bindings: media: add support for video hardware Renjiang Han
  2024-12-17  9:17 ` [PATCH v5 2/4] media: venus: core: use opp-table for the frequency Renjiang Han
@ 2024-12-17  9:17 ` Renjiang Han
  2024-12-17  9:38   ` Bryan O'Donoghue
  2024-12-17  9:17 ` [PATCH v5 4/4] arm64: dts: qcom: qcs615-ride: enable " Renjiang Han
  3 siblings, 1 reply; 14+ messages in thread
From: Renjiang Han @ 2024-12-17  9:17 UTC (permalink / raw)
  To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Bjorn Andersson, Konrad Dybcio
  Cc: linux-media, linux-arm-msm, devicetree, linux-kernel,
	Stanimir Varbanov, 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..e92a0d90a4606a450f160c1ed9ee84c16b9c22cf 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";
+
+			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>;
+				};
+			};
+
+			video-decoder {
+				compatible = "venus-decoder";
+			};
+
+			video-encoder {
+				compatible = "venus-encoder";
+			};
+		};
+
 		videocc: clock-controller@ab00000 {
 			compatible = "qcom,qcs615-videocc";
 			reg = <0 0xab00000 0 0x10000>;

-- 
2.34.1


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

* [PATCH v5 4/4] arm64: dts: qcom: qcs615-ride: enable venus node
  2024-12-17  9:17 [PATCH v5 0/4] media: venus: enable venus on qcs615 Renjiang Han
                   ` (2 preceding siblings ...)
  2024-12-17  9:17 ` [PATCH v5 3/4] arm64: dts: qcom: qcs615: add venus node Renjiang Han
@ 2024-12-17  9:17 ` Renjiang Han
  3 siblings, 0 replies; 14+ messages in thread
From: Renjiang Han @ 2024-12-17  9:17 UTC (permalink / raw)
  To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue,
	Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Bjorn Andersson, Konrad Dybcio
  Cc: linux-media, linux-arm-msm, devicetree, linux-kernel,
	Stanimir Varbanov, 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


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

* Re: [PATCH v5 3/4] arm64: dts: qcom: qcs615: add venus node
  2024-12-17  9:17 ` [PATCH v5 3/4] arm64: dts: qcom: qcs615: add venus node Renjiang Han
@ 2024-12-17  9:38   ` Bryan O'Donoghue
  2024-12-17  9:54     ` Renjiang Han
  0 siblings, 1 reply; 14+ messages in thread
From: Bryan O'Donoghue @ 2024-12-17  9:38 UTC (permalink / raw)
  To: Renjiang Han, Stanimir Varbanov, Vikash Garodia,
	Bryan O'Donoghue, Mauro Carvalho Chehab, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio
  Cc: linux-media, linux-arm-msm, devicetree, linux-kernel,
	Stanimir Varbanov

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

I gave you feedback on this in v4.

Could you please provide some commentary on why you're persisting with 
this ?

- Driver configuration should not live in dts
- A patchset exists to mitigate this
- If you don't want to use that series, what do you propose
   to resolve this ?

Please don't just ignore feedback, either act on it or add to your 
commit log _why_ you didn't act on it.

---
bod

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

* Re: [PATCH v5 3/4] arm64: dts: qcom: qcs615: add venus node
  2024-12-17  9:38   ` Bryan O'Donoghue
@ 2024-12-17  9:54     ` Renjiang Han
  2024-12-17 11:01       ` Bryan O'Donoghue
  2024-12-17 12:09       ` Dmitry Baryshkov
  0 siblings, 2 replies; 14+ messages in thread
From: Renjiang Han @ 2024-12-17  9:54 UTC (permalink / raw)
  To: Bryan O'Donoghue, Stanimir Varbanov, Vikash Garodia,
	Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Bjorn Andersson, Konrad Dybcio
  Cc: linux-media, linux-arm-msm, devicetree, linux-kernel,
	Stanimir Varbanov


On 12/17/2024 5:38 PM, Bryan O'Donoghue wrote:
> On 17/12/2024 09:17, Renjiang Han wrote:
>> +
>> +            video-decoder {
>> +                compatible = "venus-decoder";
>> +            };
>> +
>> +            video-encoder {
>> +                compatible = "venus-encoder";
>> +            };
>
> I gave you feedback on this in v4.
>
> Could you please provide some commentary on why you're persisting with 
> this ?
>
> - Driver configuration should not live in dts
> - A patchset exists to mitigate this
> - If you don't want to use that series, what do you propose
>   to resolve this ?
>
> Please don't just ignore feedback, either act on it or add to your 
> commit log _why_ you didn't act on it.
>
> ---
> bod

  Thanks for your review. You pointed it out correctly. As replied in v4,

  I also think your change is a good change, but your change involves many

  platforms. I am not sure if other guys have comments and when it will

  pass the review. Currently, it only refers to SC7180 and falls back

  QCS615 to SC7180. I have verified it on both SC7180 and QCS615 platforms.

  I think when your change review passes, we only need to remove the

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

-- 
Best Regards,
Renjiang


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

* Re: [PATCH v5 3/4] arm64: dts: qcom: qcs615: add venus node
  2024-12-17  9:54     ` Renjiang Han
@ 2024-12-17 11:01       ` Bryan O'Donoghue
  2024-12-18  6:09         ` Renjiang Han
  2024-12-17 12:09       ` Dmitry Baryshkov
  1 sibling, 1 reply; 14+ messages in thread
From: Bryan O'Donoghue @ 2024-12-17 11:01 UTC (permalink / raw)
  To: Renjiang Han, Stanimir Varbanov, Vikash Garodia,
	Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Bjorn Andersson, Konrad Dybcio
  Cc: linux-media, linux-arm-msm, devicetree, linux-kernel,
	Stanimir Varbanov

On 17/12/2024 09:54, Renjiang Han wrote:
> 
>   I think when your change review passes, we only need to remove the
> 
>   video-decoder and video-encoder nodes in the device tree.

Yes but the _point_ is we shouldn't be adding in driver configuration to 
dts.

Which means I don't think your patch can be applied until we resolve in 
impasse in venus.

---
bod

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

* Re: [PATCH v5 3/4] arm64: dts: qcom: qcs615: add venus node
  2024-12-17  9:54     ` Renjiang Han
  2024-12-17 11:01       ` Bryan O'Donoghue
@ 2024-12-17 12:09       ` Dmitry Baryshkov
  2024-12-18  6:13         ` Renjiang Han
  1 sibling, 1 reply; 14+ messages in thread
From: Dmitry Baryshkov @ 2024-12-17 12:09 UTC (permalink / raw)
  To: Renjiang Han
  Cc: Bryan O'Donoghue, Stanimir Varbanov, Vikash Garodia,
	Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Bjorn Andersson, Konrad Dybcio, linux-media,
	linux-arm-msm, devicetree, linux-kernel, Stanimir Varbanov

On Tue, Dec 17, 2024 at 05:54:57PM +0800, Renjiang Han wrote:
> 
> On 12/17/2024 5:38 PM, Bryan O'Donoghue wrote:
> > On 17/12/2024 09:17, Renjiang Han wrote:
> > > +
> > > +            video-decoder {
> > > +                compatible = "venus-decoder";
> > > +            };
> > > +
> > > +            video-encoder {
> > > +                compatible = "venus-encoder";
> > > +            };
> > 
> > I gave you feedback on this in v4.
> > 
> > Could you please provide some commentary on why you're persisting with
> > this ?
> > 
> > - Driver configuration should not live in dts
> > - A patchset exists to mitigate this
> > - If you don't want to use that series, what do you propose
> >   to resolve this ?
> > 
> > Please don't just ignore feedback, either act on it or add to your
> > commit log _why_ you didn't act on it.
> > 
> > ---
> > bod
> 
>  Thanks for your review. You pointed it out correctly. As replied in v4,
> 
>  I also think your change is a good change, but your change involves many
> 
>  platforms.

You can help it by reviewing it and then providing a Tested-by tag for
it.

P.S. Something is wrong with your emails, I see a lot of lines separated
by empty lines. It makes it harder to read.


-- 
With best wishes
Dmitry

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

* Re: [PATCH v5 1/4] dt-bindings: media: add support for video hardware
  2024-12-17  9:17 ` [PATCH v5 1/4] dt-bindings: media: add support for video hardware Renjiang Han
@ 2024-12-17 12:40   ` Krzysztof Kozlowski
  2024-12-17 12:40     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 14+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-17 12:40 UTC (permalink / raw)
  To: Renjiang Han, Stanimir Varbanov, Vikash Garodia,
	Bryan O'Donoghue, Mauro Carvalho Chehab, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio
  Cc: linux-media, linux-arm-msm, devicetree, linux-kernel,
	Stanimir Varbanov

On 17/12/2024 10:17, 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>
> ---
Read that message fully.

<form letter>
This is a friendly reminder during the review process.

It looks like you received a tag and forgot to add it.

If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.

https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577

If a tag was not added on purpose, please state why and what changed.
</form letter>

Best regards,
Krzysztof

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

* Re: [PATCH v5 1/4] dt-bindings: media: add support for video hardware
  2024-12-17 12:40   ` Krzysztof Kozlowski
@ 2024-12-17 12:40     ` Krzysztof Kozlowski
  2024-12-18  2:30       ` Renjiang Han
  0 siblings, 1 reply; 14+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-17 12:40 UTC (permalink / raw)
  To: Renjiang Han, Stanimir Varbanov, Vikash Garodia,
	Bryan O'Donoghue, Mauro Carvalho Chehab, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio
  Cc: linux-media, linux-arm-msm, devicetree, linux-kernel,
	Stanimir Varbanov

On 17/12/2024 13:40, Krzysztof Kozlowski wrote:
> On 17/12/2024 10:17, 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>
>> ---
> Read that message fully.

I meant, message in our previous discussion.

> 
> <form letter>
> This is a friendly reminder during the review process.
> 
> It looks like you received a tag and forgot to add it.
> 
> If you do not know the process, here is a short explanation:
> Please add Acked-by/Reviewed-by/Tested-by tags when posting new
> versions, under or above your Signed-off-by tag. Tag is "received", when
> provided in a message replied to you on the mailing list. Tools like b4
> can help here. However, there's no need to repost patches *only* to add
> the tags. The upstream maintainer will do that for tags received on the
> version they apply.
> 
> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
> 
> If a tag was not added on purpose, please state why and what changed.
> </form letter>

Best regards,
Krzysztof

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

* Re: [PATCH v5 1/4] dt-bindings: media: add support for video hardware
  2024-12-17 12:40     ` Krzysztof Kozlowski
@ 2024-12-18  2:30       ` Renjiang Han
  0 siblings, 0 replies; 14+ messages in thread
From: Renjiang Han @ 2024-12-18  2:30 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Stanimir Varbanov, Vikash Garodia,
	Bryan O'Donoghue, Mauro Carvalho Chehab, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio
  Cc: linux-media, linux-arm-msm, devicetree, linux-kernel,
	Stanimir Varbanov


On 12/17/2024 8:40 PM, Krzysztof Kozlowski wrote:
> On 17/12/2024 13:40, Krzysztof Kozlowski wrote:
>> On 17/12/2024 10:17, 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>
>>> ---
>> Read that message fully.
> I meant, message in our previous discussion.
>
>> <form letter>
>> This is a friendly reminder during the review process.
>>
>> It looks like you received a tag and forgot to add it.
>>
>> If you do not know the process, here is a short explanation:
>> Please add Acked-by/Reviewed-by/Tested-by tags when posting new
>> versions, under or above your Signed-off-by tag. Tag is "received", when
>> provided in a message replied to you on the mailing list. Tools like b4
>> can help here. However, there's no need to repost patches *only* to add
>> the tags. The upstream maintainer will do that for tags received on the
>> version they apply.
>>
>> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
>>
>> If a tag was not added on purpose, please state why and what changed.
>> </form letter>
> Best regards,
> Krzysztof

  OK, thanks for your reminder. I forgot to add the Reviewed-by tag,

  that's my mistake. I'll add it in the next patch version.

-- 
Best Regards,
Renjiang


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

* Re: [PATCH v5 3/4] arm64: dts: qcom: qcs615: add venus node
  2024-12-17 11:01       ` Bryan O'Donoghue
@ 2024-12-18  6:09         ` Renjiang Han
  0 siblings, 0 replies; 14+ messages in thread
From: Renjiang Han @ 2024-12-18  6:09 UTC (permalink / raw)
  To: Bryan O'Donoghue, Stanimir Varbanov, Vikash Garodia,
	Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Bjorn Andersson, Konrad Dybcio
  Cc: linux-media, linux-arm-msm, devicetree, linux-kernel,
	Stanimir Varbanov


On 12/17/2024 7:01 PM, Bryan O'Donoghue wrote:
> On 17/12/2024 09:54, Renjiang Han wrote:
>>
>>   I think when your change review passes, we only need to remove the
>>
>>   video-decoder and video-encoder nodes in the device tree.
>
> Yes but the _point_ is we shouldn't be adding in driver configuration 
> to dts.
>
> Which means I don't think your patch can be applied until we resolve 
> in impasse in venus.
>
> ---
> bod
OK! Thanks. I'll try this patch with your patch and update it with next 
patch version.

-- 
Best Regards,
Renjiang


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

* Re: [PATCH v5 3/4] arm64: dts: qcom: qcs615: add venus node
  2024-12-17 12:09       ` Dmitry Baryshkov
@ 2024-12-18  6:13         ` Renjiang Han
  0 siblings, 0 replies; 14+ messages in thread
From: Renjiang Han @ 2024-12-18  6:13 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Bryan O'Donoghue, Stanimir Varbanov, Vikash Garodia,
	Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Bjorn Andersson, Konrad Dybcio, linux-media,
	linux-arm-msm, devicetree, linux-kernel, Stanimir Varbanov


On 12/17/2024 8:09 PM, Dmitry Baryshkov wrote:
> On Tue, Dec 17, 2024 at 05:54:57PM +0800, Renjiang Han wrote:
>> On 12/17/2024 5:38 PM, Bryan O'Donoghue wrote:
>>> On 17/12/2024 09:17, Renjiang Han wrote:
>>>> +
>>>> +            video-decoder {
>>>> +                compatible = "venus-decoder";
>>>> +            };
>>>> +
>>>> +            video-encoder {
>>>> +                compatible = "venus-encoder";
>>>> +            };
>>> I gave you feedback on this in v4.
>>>
>>> Could you please provide some commentary on why you're persisting with
>>> this ?
>>>
>>> - Driver configuration should not live in dts
>>> - A patchset exists to mitigate this
>>> - If you don't want to use that series, what do you propose
>>>    to resolve this ?
>>>
>>> Please don't just ignore feedback, either act on it or add to your
>>> commit log _why_ you didn't act on it.
>>>
>>> ---
>>> bod
>>   Thanks for your review. You pointed it out correctly. As replied in v4,
>>
>>   I also think your change is a good change, but your change involves many
>>
>>   platforms.
> You can help it by reviewing it and then providing a Tested-by tag for
> it.
OK. I'll try it.
>
> P.S. Something is wrong with your emails, I see a lot of lines separated
> by empty lines. It makes it harder to read.
>
Thanks for your reminder. I'll check my environment.

-- 
Best Regards,
Renjiang


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

end of thread, other threads:[~2024-12-18  6:13 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-17  9:17 [PATCH v5 0/4] media: venus: enable venus on qcs615 Renjiang Han
2024-12-17  9:17 ` [PATCH v5 1/4] dt-bindings: media: add support for video hardware Renjiang Han
2024-12-17 12:40   ` Krzysztof Kozlowski
2024-12-17 12:40     ` Krzysztof Kozlowski
2024-12-18  2:30       ` Renjiang Han
2024-12-17  9:17 ` [PATCH v5 2/4] media: venus: core: use opp-table for the frequency Renjiang Han
2024-12-17  9:17 ` [PATCH v5 3/4] arm64: dts: qcom: qcs615: add venus node Renjiang Han
2024-12-17  9:38   ` Bryan O'Donoghue
2024-12-17  9:54     ` Renjiang Han
2024-12-17 11:01       ` Bryan O'Donoghue
2024-12-18  6:09         ` Renjiang Han
2024-12-17 12:09       ` Dmitry Baryshkov
2024-12-18  6:13         ` Renjiang Han
2024-12-17  9:17 ` [PATCH v5 4/4] arm64: dts: qcom: qcs615-ride: enable " Renjiang Han

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).