media: venus: Add QCM2290 support with AR50_LITE core

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml line 152

Quoted 7 lines · Jorge Ramirez-Ortiz

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +7 -0

+        video-decoder {
+            compatible = "venus-decoder";
+        };
+
+        video-encoder {
+            compatible = "venus-encoder";
+        };

Not necessary, drop.

---
bod

Add a schema for the venus video encoder/decoder on the qcm2290.

Signed-off-by Jorge Ramirez-Ortiz <[email protected]>

---
 .../bindings/media/qcom,qcm2290-venus.yaml    | 153 ++++++++++++++++++
 1 file changed, 153 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +2 -0

new file mode 100644
index 000000000000..ffa72f1e27f3
@@ -0,0 +1,153 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +6 -0

+  The Venus AR50_LITE IP is a video encode and decode accelerator present
+  on Qualcomm platforms
+
+allOf:
+  - $ref: qcom,venus-common.yaml#
+

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +34 -0

+    items:
+      - const: venus
+      - const: vcodec0
+      - const: cx
+
+  clocks:
+    maxItems: 6
+
+  clock-names:
+    items:
+      - const: core
+      - const: iface
+      - const: bus
+      - const: throttle
+      - const: vcodec0_core
+      - const: vcodec0_bus
+
+  iommus:
+    minItems: 1
+    maxItems: 5
+
+  interconnects:
+    maxItems: 2
+
+  interconnect-names:
+    items:
+      - const: video-mem
+      - const: cpu-cfg
+
+  operating-points-v2: true
+  opp-table:
+    type: object
+
+  video-decoder:

Quoted 20 lines · Jorge Ramirez-Ortiz

Add a schema for the venus video encoder/decoder on the qcm2290.

Signed-off-by Jorge Ramirez-Ortiz <[email protected]>

---
 .../bindings/media/qcom,qcm2290-venus.yaml    | 153 ++++++++++++++++++
 1 file changed, 153 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +6 -0

new file mode 100644
index 000000000000..ffa72f1e27f3
@@ -0,0 +1,153 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/qcom,qcm2290-venus.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml line 12

Quoted 6 lines

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +6 -0

+title: Qualcomm QCM2290 Venus video encode and decode accelerators
+
+maintainers:
+  - Stanimir Varbanov <[email protected]>
+
+description: |

Do not need '|' unless you need to preserve formatting.

Quoted 11 lines

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +10 -0

+  The Venus AR50_LITE IP is a video encode and decode accelerator present
+  on Qualcomm platforms
+
+allOf:
+  - $ref: qcom,venus-common.yaml#
+
+properties:
+  compatible:
+    const: qcom,qcm2290-venus
+

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml line 28

Quoted 6 lines

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +6 -0

+  power-domains:
+    minItems: 2
+    maxItems: 3
+
+  power-domain-names:
+    minItems: 2

Why is this flexible? Either you have two or three. Not mixed.

Quoted 11 lines

Quoted 11 lines

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +10 -0

+  The Venus AR50_LITE IP is a video encode and decode accelerator present
+  on Qualcomm platforms
+
+allOf:
+  - $ref: qcom,venus-common.yaml#
+
+properties:
+  compatible:
+    const: qcom,qcm2290-venus
+

Quoted 8 lines

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml line 28

Quoted 6 lines

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +6 -0

+  power-domains:
+    minItems: 2
+    maxItems: 3
+
+  power-domain-names:
+    minItems: 2

Why is this flexible? Either you have two or three. Not mixed.

please check 5b380f242f360256c96e96adabeb7ce9ec784306

Quoted 20 lines · Jorge Ramirez

Quoted 19 lines

Quoted 7 lines

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +6 -0

+  The Venus AR50_LITE IP is a video encode and decode accelerator present
+  on Qualcomm platforms
+
+allOf:
+  - $ref: qcom,venus-common.yaml#
+

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml line 28

Quoted 10 lines

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +10 -0

+properties:
+  compatible:
+    const: qcom,qcm2290-venus
+
+  power-domains:
+    minItems: 2
+    maxItems: 3
+
+  power-domain-names:
+    minItems: 2

Why is this flexible? Either you have two or three. Not mixed.

Quoted 1 line

please check 5b380f242f360256c96e96adabeb7ce9ec784306

This does not explain why this is optional HERE. You cannot use for a new platform an argument that some existing platform was changed in ABI-preserving way.

BTW, also subject prefixes needs fixing. For DTS: it is never "arch".

For this patch: wrong order (see DT submitting patches).

Best regards,
Krzysztof

Quoted 21 lines · Krzysztof Kozlowski

Quoted 20 lines · Jorge Ramirez

Quoted 19 lines

Quoted 7 lines

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +6 -0

+  The Venus AR50_LITE IP is a video encode and decode accelerator present
+  on Qualcomm platforms
+
+allOf:
+  - $ref: qcom,venus-common.yaml#
+

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml line 28

Quoted 10 lines

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +10 -0

+properties:
+  compatible:
+    const: qcom,qcm2290-venus
+
+  power-domains:
+    minItems: 2
+    maxItems: 3
+
+  power-domain-names:
+    minItems: 2

Why is this flexible? Either you have two or three. Not mixed.

Quoted 5 lines

Quoted 1 line

please check 5b380f242f360256c96e96adabeb7ce9ec784306

This does not explain why this is optional HERE. You cannot use for a new platform an argument that some existing platform was changed in ABI-preserving way.

thanks for quick the follow up.

but bear with me please because I dont follow - why can the same logic be used - it being applicable - and therefore result in a definition similar to those other platforms?

Quoted 28 lines · Jorge Ramirez

Quoted 26 lines · Krzysztof Kozlowski

Quoted 20 lines · Jorge Ramirez

Quoted 11 lines

Quoted 11 lines

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +10 -0

+  The Venus AR50_LITE IP is a video encode and decode accelerator present
+  on Qualcomm platforms
+
+allOf:
+  - $ref: qcom,venus-common.yaml#
+
+properties:
+  compatible:
+    const: qcom,qcm2290-venus
+

Quoted 8 lines

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml line 28

Quoted 6 lines

Documentation/devicetree/bindings/media/qcom,qcm2290-venus.yaml new file +6 -0

+  power-domains:
+    minItems: 2
+    maxItems: 3
+
+  power-domain-names:
+    minItems: 2

Why is this flexible? Either you have two or three. Not mixed.

Quoted 1 line

please check 5b380f242f360256c96e96adabeb7ce9ec784306

This does not explain why this is optional HERE. You cannot use for a new platform an argument that some existing platform was changed in ABI-preserving way.

Quoted 5 lines

thanks for quick the follow up.

but bear with me please because I dont follow - why can the same logic be used - it being applicable - and therefore result in a definition similar to those other platforms?

Because this platform either has 2 or 3, not both. Unless that's not true, but then please share some arguments.

Best regards,
Krzysztof

Quoted 29 lines · Krzysztof Kozlowski

Quoted 28 lines · Jorge Ramirez

Quoted 26 lines · Krzysztof Kozlowski

Quoted 20 lines · Jorge Ramirez

Quoted 11 lines

Quoted 11 lines

(quoted diff) +10 -0

+  The Venus AR50_LITE IP is a video encode and decode accelerator present
+  on Qualcomm platforms
+
+allOf:
+  - $ref: qcom,venus-common.yaml#
+
+properties:
+  compatible:
+    const: qcom,qcm2290-venus
+

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+  power-domains:
+    minItems: 2
+    maxItems: 3
+
+  power-domain-names:
+    minItems: 2

Why is this flexible? Either you have two or three. Not mixed.

Quoted 1 line

please check 5b380f242f360256c96e96adabeb7ce9ec784306

This does not explain why this is optional HERE. You cannot use for a new platform an argument that some existing platform was changed in ABI-preserving way.

Quoted 8 lines

Quoted 5 lines

thanks for quick the follow up.

but bear with me please because I dont follow - why can the same logic be used - it being applicable - and therefore result in a definition similar to those other platforms?

Because this platform either has 2 or 3, not both. Unless that's not true, but then please share some arguments.

as with every other venus schema with more than 1 power domain, the argument is the same one that I have shared with you a couple of messages back (DVFS).

verbatim:
Venus needs to vote for the performance state of a power domain (cx) to be able to support DVFS. This 'cx' power domain is controlled by rpm and is a common power domain (scalable) not specific to venus alone. This is optional in the sense that, leaving this power domain out does not really impact the functionality but just makes the platform a little less power efficient.

Seeing all these venus schemas follow the same pattern, it seems to me that this is the correct way of implementing the above.

You seem to disagree. please could you explain?

Quoted 43 lines · Jorge Ramirez

Quoted 29 lines · Krzysztof Kozlowski

Quoted 28 lines · Jorge Ramirez

Quoted 26 lines · Krzysztof Kozlowski

Quoted 20 lines · Jorge Ramirez

Quoted 11 lines

Quoted 11 lines

(quoted diff) +10 -0

+  The Venus AR50_LITE IP is a video encode and decode accelerator present
+  on Qualcomm platforms
+
+allOf:
+  - $ref: qcom,venus-common.yaml#
+
+properties:
+  compatible:
+    const: qcom,qcm2290-venus
+

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+  power-domains:
+    minItems: 2
+    maxItems: 3
+
+  power-domain-names:
+    minItems: 2

Why is this flexible? Either you have two or three. Not mixed.

Quoted 1 line

please check 5b380f242f360256c96e96adabeb7ce9ec784306

This does not explain why this is optional HERE. You cannot use for a new platform an argument that some existing platform was changed in ABI-preserving way.

Quoted 8 lines

Quoted 5 lines

thanks for quick the follow up.

but bear with me please because I dont follow - why can the same logic be used - it being applicable - and therefore result in a definition similar to those other platforms?

Because this platform either has 2 or 3, not both. Unless that's not true, but then please share some arguments.

as with every other venus schema with more than 1 power domain, the argument is the same one that I have shared with you a couple of messages back (DVFS).

Quoted 7 lines

verbatim:
Venus needs to vote for the performance state of a power domain (cx) to be able to support DVFS. This 'cx' power domain is controlled by rpm and is a common power domain (scalable) not specific to venus alone. This is optional in the sense that, leaving this power domain out does not really impact the functionality but just makes the platform a little less power efficient.

That's not definition of optional. The domain is needed for this device, the device is one way or another having its rails routed to that domain. It is not optional.

Quoted 1 line

Quoted 2 lines

Seeing all these venus schemas follow the same pattern, it seems to me that this is the correct way of implementing the above.

No for the reason I mentioned earlier.

Quoted 3 lines

You seem to disagree. please could you explain?

I already explained. You add new device, so argument to preserve ABI, which was accepted THAT TIME, is not valid. You do not have ABI.

Best regards,
Krzysztof

Quoted 57 lines · Krzysztof Kozlowski

Quoted 43 lines · Jorge Ramirez

Quoted 29 lines · Krzysztof Kozlowski

Quoted 28 lines · Jorge Ramirez

Quoted 26 lines · Krzysztof Kozlowski

Quoted 20 lines · Jorge Ramirez

Quoted 11 lines

Quoted 11 lines

(quoted diff) +10 -0

+  The Venus AR50_LITE IP is a video encode and decode accelerator present
+  on Qualcomm platforms
+
+allOf:
+  - $ref: qcom,venus-common.yaml#
+
+properties:
+  compatible:
+    const: qcom,qcm2290-venus
+

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+  power-domains:
+    minItems: 2
+    maxItems: 3
+
+  power-domain-names:
+    minItems: 2

Why is this flexible? Either you have two or three. Not mixed.

Quoted 1 line

please check 5b380f242f360256c96e96adabeb7ce9ec784306

This does not explain why this is optional HERE. You cannot use for a new platform an argument that some existing platform was changed in ABI-preserving way.

Quoted 8 lines

Quoted 5 lines

thanks for quick the follow up.

but bear with me please because I dont follow - why can the same logic be used - it being applicable - and therefore result in a definition similar to those other platforms?

Because this platform either has 2 or 3, not both. Unless that's not true, but then please share some arguments.

as with every other venus schema with more than 1 power domain, the argument is the same one that I have shared with you a couple of messages back (DVFS).

Quoted 7 lines

verbatim:
Venus needs to vote for the performance state of a power domain (cx) to be able to support DVFS. This 'cx' power domain is controlled by rpm and is a common power domain (scalable) not specific to venus alone. This is optional in the sense that, leaving this power domain out does not really impact the functionality but just makes the platform a little less power efficient.

That's not definition of optional. The domain is needed for this device, the device is one way or another having its rails routed to that domain. It is not optional.

Quoted 1 line

Quoted 4 lines

Quoted 2 lines

Seeing all these venus schemas follow the same pattern, it seems to me that this is the correct way of implementing the above.

No for the reason I mentioned earlier.

So just to close this story up, were these two commits wrongly reviewed and signed off then ? Please do notice they were also - just like this one - new additions and not a change in an ABI preserving way as you characterize them.

e48b839b6699c2268e545360e06962bb76ff5b8d
8d3a1cb32124eaeb3f2efe4889de214d3b658d8d

Quoted 68 lines · Jorge Ramirez

Quoted 57 lines · Krzysztof Kozlowski

Quoted 43 lines · Jorge Ramirez

Quoted 29 lines · Krzysztof Kozlowski

Quoted 28 lines · Jorge Ramirez

Quoted 26 lines · Krzysztof Kozlowski

Quoted 20 lines · Jorge Ramirez

Quoted 11 lines

Quoted 11 lines

(quoted diff) +10 -0

+  The Venus AR50_LITE IP is a video encode and decode accelerator present
+  on Qualcomm platforms
+
+allOf:
+  - $ref: qcom,venus-common.yaml#
+
+properties:
+  compatible:
+    const: qcom,qcm2290-venus
+

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+  power-domains:
+    minItems: 2
+    maxItems: 3
+
+  power-domain-names:
+    minItems: 2

Why is this flexible? Either you have two or three. Not mixed.

Quoted 1 line

please check 5b380f242f360256c96e96adabeb7ce9ec784306

This does not explain why this is optional HERE. You cannot use for a new platform an argument that some existing platform was changed in ABI-preserving way.

Quoted 8 lines

Quoted 5 lines

thanks for quick the follow up.

but bear with me please because I dont follow - why can the same logic be used - it being applicable - and therefore result in a definition similar to those other platforms?

Because this platform either has 2 or 3, not both. Unless that's not true, but then please share some arguments.

as with every other venus schema with more than 1 power domain, the argument is the same one that I have shared with you a couple of messages back (DVFS).

Quoted 7 lines

verbatim:
Venus needs to vote for the performance state of a power domain (cx) to be able to support DVFS. This 'cx' power domain is controlled by rpm and is a common power domain (scalable) not specific to venus alone. This is optional in the sense that, leaving this power domain out does not really impact the functionality but just makes the platform a little less power efficient.

That's not definition of optional. The domain is needed for this device, the device is one way or another having its rails routed to that domain. It is not optional.

Quoted 1 line

Quoted 4 lines

Quoted 2 lines

Seeing all these venus schemas follow the same pattern, it seems to me that this is the correct way of implementing the above.

No for the reason I mentioned earlier.

So just to close this story up, were these two commits wrongly reviewed and signed off then ? Please do notice they were also - just like this one - new additions and not a change in an ABI preserving way as you characterize them.

Quoted 2 lines

e48b839b6699c2268e545360e06962bb76ff5b8d
8d3a1cb32124eaeb3f2efe4889de214d3b658d8d

I was waiting for this argument: there was something similar some years ago (but even months ago...) and it got reviewed, so I can do the same.

You can even go further back. Take commits for DT bindings from 2013 and use that against our new review. So many different things were accepted in 2013.

You can take any driver code from 2013. Huh, people actually do! People still send .owner=THIS_MODULE. In 2013 this was reviewed and accepted, so I can send it, right?

And then people are not happy that they patches receive too much detailed review or review takes too much time or whatever other reason... Yeah if any review you ever give will be some day used against you, you would think 10 times and be 10 times more picky then necessary.

This is like an ultimate, super, triple combo argument against reviewers and maintainers to discredit their work. I will not play such games.

Quoted 69 lines · Krzysztof Kozlowski

Quoted 68 lines · Jorge Ramirez

Quoted 57 lines · Krzysztof Kozlowski

Quoted 43 lines · Jorge Ramirez

Quoted 29 lines · Krzysztof Kozlowski

Quoted 28 lines · Jorge Ramirez

Quoted 26 lines · Krzysztof Kozlowski

Quoted 20 lines · Jorge Ramirez

Quoted 11 lines

Quoted 11 lines

(quoted diff) +10 -0

+  The Venus AR50_LITE IP is a video encode and decode accelerator present
+  on Qualcomm platforms
+
+allOf:
+  - $ref: qcom,venus-common.yaml#
+
+properties:
+  compatible:
+    const: qcom,qcm2290-venus
+

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+  power-domains:
+    minItems: 2
+    maxItems: 3
+
+  power-domain-names:
+    minItems: 2

Why is this flexible? Either you have two or three. Not mixed.

Quoted 1 line

please check 5b380f242f360256c96e96adabeb7ce9ec784306

This does not explain why this is optional HERE. You cannot use for a new platform an argument that some existing platform was changed in ABI-preserving way.

Quoted 8 lines

Quoted 5 lines

thanks for quick the follow up.

but bear with me please because I dont follow - why can the same logic be used - it being applicable - and therefore result in a definition similar to those other platforms?

Because this platform either has 2 or 3, not both. Unless that's not true, but then please share some arguments.

as with every other venus schema with more than 1 power domain, the argument is the same one that I have shared with you a couple of messages back (DVFS).

Quoted 7 lines

verbatim:
Venus needs to vote for the performance state of a power domain (cx) to be able to support DVFS. This 'cx' power domain is controlled by rpm and is a common power domain (scalable) not specific to venus alone. This is optional in the sense that, leaving this power domain out does not really impact the functionality but just makes the platform a little less power efficient.

That's not definition of optional. The domain is needed for this device, the device is one way or another having its rails routed to that domain. It is not optional.

Quoted 1 line

Quoted 4 lines

Quoted 2 lines

Seeing all these venus schemas follow the same pattern, it seems to me that this is the correct way of implementing the above.

No for the reason I mentioned earlier.

So just to close this story up, were these two commits wrongly reviewed and signed off then ? Please do notice they were also - just like this one - new additions and not a change in an ABI preserving way as you characterize them.

Quoted 5 lines

Quoted 2 lines

e48b839b6699c2268e545360e06962bb76ff5b8d
8d3a1cb32124eaeb3f2efe4889de214d3b658d8d

I was waiting for this argument: there was something similar some years ago (but even months ago...) and it got reviewed, so I can do the same.

Add DT entries for the qcm2290 venus encoder/decoder.

Signed-off-by Jorge Ramirez-Ortiz <[email protected]>

---
  arch/arm64/boot/dts/qcom/qcm2290.dtsi | 53 +++++++++++++++++++++++++++
  1 file changed, 53 insertions(+)

arch/arm64/boot/dts/qcom/qcm2290.dtsi +41 -0

index f49ac1c1f8a3..af2c1f66fe07 100644
@@ -1628,6 +1628,59 @@ adreno_smmu: iommu@59a0000 {
  			#iommu-cells = <2>;
  		};
 
+		venus: video-codec@5a00000 {
+			compatible = "qcom,qcm2290-venus";
+			reg = <0 0x5a00000 0 0xff000>;
+			interrupts = <GIC_SPI 225 IRQ_TYPE_LEVEL_HIGH>;
+
+			power-domains = <&gcc GCC_VENUS_GDSC>,
+					<&gcc GCC_VCODEC0_GDSC>,
+					<&rpmpd QCM2290_VDDCX>;
+			power-domain-names = "venus", "vcodec0", "cx";
+			operating-points-v2 = <&venus_opp_table>;
+
+			clocks = <&gcc GCC_VIDEO_VENUS_CTL_CLK>,
+				 <&gcc GCC_VIDEO_AHB_CLK>,
+				 <&gcc GCC_VENUS_CTL_AXI_CLK>,
+				 <&gcc GCC_VIDEO_THROTTLE_CORE_CLK>,
+				 <&gcc GCC_VIDEO_VCODEC0_SYS_CLK>,
+				 <&gcc GCC_VCODEC0_AXI_CLK>;
+			clock-names = "core", "iface", "bus", "throttle",
+				      "vcodec0_core", "vcodec0_bus";
+
+			memory-region = <&pil_video_mem>;
+			iommus = <&apps_smmu 0x860 0x0>,
+				 <&apps_smmu 0x880 0x0>,
+				 <&apps_smmu 0x861 0x04>,
+				 <&apps_smmu 0x863 0x0>,
+				 <&apps_smmu 0x804 0xE0>;
+
+			interconnects = <&mmnrt_virt MASTER_VIDEO_P0 0 &bimc SLAVE_EBI1 0>,
+					<&bimc MASTER_APPSS_PROC 0 &config_noc SLAVE_VENUS_CFG 0>;
+			interconnect-names = "video-mem", "cpu-cfg";
+
+			venus_opp_table: opp-table {
+				compatible = "operating-points-v2";
+				opp-133000000 {
+					opp-hz = /bits/ 64 <133000000>;
+					required-opps = <&rpmpd_opp_low_svs>;
+				};
+
+				opp-240000000 {
+					opp-hz = /bits/ 64 <240000000>;
+					required-opps = <&rpmpd_opp_svs>;

Quoted 59 lines · Jorge Ramirez-Ortiz

Add DT entries for the qcm2290 venus encoder/decoder.

Signed-off-by Jorge Ramirez-Ortiz <[email protected]>

---
  arch/arm64/boot/dts/qcom/qcm2290.dtsi | 53 +++++++++++++++++++++++++++
  1 file changed, 53 insertions(+)

arch/arm64/boot/dts/qcom/qcm2290.dtsi +45 -0

index f49ac1c1f8a3..af2c1f66fe07 100644
@@ -1628,6 +1628,59 @@ adreno_smmu: iommu@59a0000 {
  			#iommu-cells = <2>;
  		};
+		venus: video-codec@5a00000 {
+			compatible = "qcom,qcm2290-venus";
+			reg = <0 0x5a00000 0 0xff000>;
+			interrupts = <GIC_SPI 225 IRQ_TYPE_LEVEL_HIGH>;
+
+			power-domains = <&gcc GCC_VENUS_GDSC>,
+					<&gcc GCC_VCODEC0_GDSC>,
+					<&rpmpd QCM2290_VDDCX>;
+			power-domain-names = "venus", "vcodec0", "cx";
+			operating-points-v2 = <&venus_opp_table>;
+
+			clocks = <&gcc GCC_VIDEO_VENUS_CTL_CLK>,
+				 <&gcc GCC_VIDEO_AHB_CLK>,
+				 <&gcc GCC_VENUS_CTL_AXI_CLK>,
+				 <&gcc GCC_VIDEO_THROTTLE_CORE_CLK>,
+				 <&gcc GCC_VIDEO_VCODEC0_SYS_CLK>,
+				 <&gcc GCC_VCODEC0_AXI_CLK>;
+			clock-names = "core", "iface", "bus", "throttle",
+				      "vcodec0_core", "vcodec0_bus";
+
+			memory-region = <&pil_video_mem>;
+			iommus = <&apps_smmu 0x860 0x0>,
+				 <&apps_smmu 0x880 0x0>,
+				 <&apps_smmu 0x861 0x04>,
+				 <&apps_smmu 0x863 0x0>,
+				 <&apps_smmu 0x804 0xE0>;
+
+			interconnects = <&mmnrt_virt MASTER_VIDEO_P0 0 &bimc SLAVE_EBI1 0>,
+					<&bimc MASTER_APPSS_PROC 0 &config_noc SLAVE_VENUS_CFG 0>;
+			interconnect-names = "video-mem", "cpu-cfg";
+
+			venus_opp_table: opp-table {
+				compatible = "operating-points-v2";
+				opp-133000000 {
+					opp-hz = /bits/ 64 <133000000>;
+					required-opps = <&rpmpd_opp_low_svs>;
+				};
+
+				opp-240000000 {
+					opp-hz = /bits/ 64 <240000000>;
+					required-opps = <&rpmpd_opp_svs>;
+				};
+			};
+
+			video-decoder {

arch/arm64/boot/dts/qcom/qcm2290.dtsi adreno_smmu: iommu@59a0000 { · line 1681

Quoted 6 lines

arch/arm64/boot/dts/qcom/qcm2290.dtsi +6 -0

+				compatible = "venus-decoder";
+			};
+
+			video-encoder {
+				compatible = "venus-encoder";
+			};

These should be dropped in favour of static config in the driver.

arch/arm64/boot/dts/qcom/qcm2290.dtsi adreno_smmu: iommu@59a0000 { · line 1686

Quoted 5 lines

arch/arm64/boot/dts/qcom/qcm2290.dtsi +2 -0

+		};
+
  		mdss: display-subsystem@5e00000 {
  			compatible = "qcom,qcm2290-mdss";
  			reg = <0x0 0x05e00000 0x0 0x1000>;

---
bod

Add DT entries for the qcm2290 venus encoder/decoder.

Signed-off-by Jorge Ramirez-Ortiz <[email protected]>

---
 arch/arm64/boot/dts/qcom/qcm2290.dtsi | 53 +++++++++++++++++++++++++++

 1 file changed, 53 insertions(+)

arch/arm64/boot/dts/qcom/qcm2290.dtsi +36 -0

index f49ac1c1f8a3..af2c1f66fe07 100644
@@ -1628,6 +1628,59 @@ adreno_smmu: iommu@59a0000 {
 			#iommu-cells = <2>;
 		};
 
+		venus: video-codec@5a00000 {
+			compatible = "qcom,qcm2290-venus";
+			reg = <0 0x5a00000 0 0xff000>;
+			interrupts = <GIC_SPI 225 IRQ_TYPE_LEVEL_HIGH>;
+
+			power-domains = <&gcc GCC_VENUS_GDSC>,
+					<&gcc GCC_VCODEC0_GDSC>,
+					<&rpmpd QCM2290_VDDCX>;
+			power-domain-names = "venus", "vcodec0", "cx";
+			operating-points-v2 = <&venus_opp_table>;
+
+			clocks = <&gcc GCC_VIDEO_VENUS_CTL_CLK>,
+				 <&gcc GCC_VIDEO_AHB_CLK>,
+				 <&gcc GCC_VENUS_CTL_AXI_CLK>,
+				 <&gcc GCC_VIDEO_THROTTLE_CORE_CLK>,
+				 <&gcc GCC_VIDEO_VCODEC0_SYS_CLK>,
+				 <&gcc GCC_VCODEC0_AXI_CLK>;
+			clock-names = "core", "iface", "bus", "throttle",
+				      "vcodec0_core", "vcodec0_bus";
+
+			memory-region = <&pil_video_mem>;
+			iommus = <&apps_smmu 0x860 0x0>,
+				 <&apps_smmu 0x880 0x0>,
+				 <&apps_smmu 0x861 0x04>,
+				 <&apps_smmu 0x863 0x0>,
+				 <&apps_smmu 0x804 0xE0>;
+
+			interconnects = <&mmnrt_virt MASTER_VIDEO_P0 0 &bimc SLAVE_EBI1 0>,
+					<&bimc MASTER_APPSS_PROC 0 &config_noc SLAVE_VENUS_CFG 0>;
+			interconnect-names = "video-mem", "cpu-cfg";
+
+			venus_opp_table: opp-table {
+				compatible = "operating-points-v2";
+				opp-133000000 {
+					opp-hz = /bits/ 64 <133000000>;
+					required-opps = <&rpmpd_opp_low_svs>;

Quoted 5 lines · Jorge Ramirez-Ortiz

Add DT entries for the qcm2290 venus encoder/decoder.

Signed-off-by Jorge Ramirez-Ortiz <[email protected]>

---
 arch/arm64/boot/dts/qcom/qcm2290.dtsi | 53 +++++++++++++++++++++++++++

DTS cannot be a dependency for driver. Order your patches correctly (see dts coding style, soc maintainer rules or internal guidelines).

Quoted 50 lines

 1 file changed, 53 insertions(+)

arch/arm64/boot/dts/qcom/qcm2290.dtsi +40 -0

index f49ac1c1f8a3..af2c1f66fe07 100644
@@ -1628,6 +1628,59 @@ adreno_smmu: iommu@59a0000 {
 			#iommu-cells = <2>;
 		};
 
+		venus: video-codec@5a00000 {
+			compatible = "qcom,qcm2290-venus";
+			reg = <0 0x5a00000 0 0xff000>;
+			interrupts = <GIC_SPI 225 IRQ_TYPE_LEVEL_HIGH>;
+
+			power-domains = <&gcc GCC_VENUS_GDSC>,
+					<&gcc GCC_VCODEC0_GDSC>,
+					<&rpmpd QCM2290_VDDCX>;
+			power-domain-names = "venus", "vcodec0", "cx";
+			operating-points-v2 = <&venus_opp_table>;
+
+			clocks = <&gcc GCC_VIDEO_VENUS_CTL_CLK>,
+				 <&gcc GCC_VIDEO_AHB_CLK>,
+				 <&gcc GCC_VENUS_CTL_AXI_CLK>,
+				 <&gcc GCC_VIDEO_THROTTLE_CORE_CLK>,
+				 <&gcc GCC_VIDEO_VCODEC0_SYS_CLK>,
+				 <&gcc GCC_VCODEC0_AXI_CLK>;
+			clock-names = "core", "iface", "bus", "throttle",
+				      "vcodec0_core", "vcodec0_bus";
+
+			memory-region = <&pil_video_mem>;
+			iommus = <&apps_smmu 0x860 0x0>,
+				 <&apps_smmu 0x880 0x0>,
+				 <&apps_smmu 0x861 0x04>,
+				 <&apps_smmu 0x863 0x0>,
+				 <&apps_smmu 0x804 0xE0>;
+
+			interconnects = <&mmnrt_virt MASTER_VIDEO_P0 0 &bimc SLAVE_EBI1 0>,
+					<&bimc MASTER_APPSS_PROC 0 &config_noc SLAVE_VENUS_CFG 0>;
+			interconnect-names = "video-mem", "cpu-cfg";
+
+			venus_opp_table: opp-table {
+				compatible = "operating-points-v2";
+				opp-133000000 {
+					opp-hz = /bits/ 64 <133000000>;
+					required-opps = <&rpmpd_opp_low_svs>;
+				};
+
+				opp-240000000 {
+					opp-hz = /bits/ 64 <240000000>;

arch/arm64/boot/dts/qcom/qcm2290.dtsi adreno_smmu: iommu@59a0000 { · line 1676

Quoted 6 lines

arch/arm64/boot/dts/qcom/qcm2290.dtsi +6 -0

+					required-opps = <&rpmpd_opp_svs>;
+				};
+			};
+
+			video-decoder {
+				compatible = "venus-decoder";

Don't add deprecated properties.

Best regards,
Krzysztof

The AR50_LITE is a streamlined variant of the AR50 video core, designed for power and cost-efficient platforms.

It supports hardware-accelerated decoding of H.264, HEVC, and VP9 formats.

Signed-off-by Jorge Ramirez-Ortiz <[email protected]>

---
  drivers/media/platform/qcom/venus/core.c      | 11 ++-
  drivers/media/platform/qcom/venus/core.h      |  5 +-
  drivers/media/platform/qcom/venus/firmware.c  |  8 +-
  drivers/media/platform/qcom/venus/helpers.c   | 81 +++++++++++++++++++
  drivers/media/platform/qcom/venus/helpers.h   |  2 +
  .../media/platform/qcom/venus/hfi_helper.h    | 10 ++-
  drivers/media/platform/qcom/venus/hfi_venus.c | 14 ++--
  .../media/platform/qcom/venus/pm_helpers.c    |  1 +
  drivers/media/platform/qcom/venus/vdec.c      | 15 ++--
  9 files changed, 123 insertions(+), 24 deletions(-)

drivers/media/platform/qcom/venus/core.c +0 -0

index d305d74bb152..736ef53d988d 100644

drivers/media/platform/qcom/venus/core.c +7 -1

  		core->cpu_base = core->base + CPU_BASE_V6;
  		core->cpu_cs_base = core->base + CPU_CS_BASE_V6;
  		core->cpu_ic_base = core->base + CPU_IC_BASE_V6;
  		core->wrapper_base = core->base + WRAPPER_BASE_V6;
  		core->wrapper_tz_base = core->base + WRAPPER_TZ_BASE_V6;
-		core->aon_base = core->base + AON_BASE_V6;
+		if (IS_AR50_LITE(core)) {
+			core->vbif_base = NULL;
+			core->aon_base = NULL;
+		} else {
+			core->vbif_base = core->base + VBIF_BASE;
+			core->aon_base = core->base + AON_BASE_V6;
+		}
  	} else {
  		core->vbif_base = core->base + VBIF_BASE;
  		core->cpu_base = core->base + CPU_BASE;

drivers/media/platform/qcom/venus/core.h +4 -1

index b412e0c5515a..122441f9600a 100644
@@ -382,6 +382,7 @@ enum venus_inst_modes {
   * @lock:	instance lock
   * @core:	a reference to the core struct
   * @clk_data:	clock data per core ID
+ * @eosbufs:	a lit of EOS buffers
   * @dpbbufs:	a list of decoded picture buffers
   * @internalbufs:	a list of internal bufferes
   * @registeredbufs:	a list of registered capture bufferes

@@ -450,6 +451,7 @@ struct venus_inst {
  	struct mutex lock;
  	struct venus_core *core;
  	struct clock_data clk_data;
+	struct list_head eosbufs;
  	struct list_head dpbbufs;
  	struct list_head internalbufs;
  	struct list_head registeredbufs;

@@ -520,7 +522,8 @@ struct venus_inst {
  #define IS_V1(core)	((core)->res->hfi_version == HFI_VERSION_1XX)
  #define IS_V3(core)	((core)->res->hfi_version == HFI_VERSION_3XX)
  #define IS_V4(core)	((core)->res->hfi_version == HFI_VERSION_4XX)
-#define IS_V6(core)	((core)->res->hfi_version == HFI_VERSION_6XX)
+#define IS_V6(core)     (((core)->res->hfi_version == HFI_VERSION_6XX) || \
+			 ((core)->res->hfi_version == HFI_VERSION_6XX_LITE))
 
  #define IS_AR50(core)		((core)->res->vpu_version == VPU_VERSION_AR50)
  #define IS_AR50_LITE(core)	((core)->res->vpu_version == VPU_VERSION_AR50_LITE)

drivers/media/platform/qcom/venus/firmware.c +0 -0

index 66a18830e66d..f8dcef0426ac 100644

drivers/media/platform/qcom/venus/firmware.c +3 -3

  		wrapper_base = core->wrapper_tz_base;
  	else
  		wrapper_base = core->wrapper_base;

@@ -42,7 +42,7 @@ static void venus_reset_cpu(struct venus_core *core)
  	writel(fw_size, wrapper_base + WRAPPER_NONPIX_START_ADDR);
  	writel(fw_size, wrapper_base + WRAPPER_NONPIX_END_ADDR);
 
-	if (IS_IRIS2_1(core)) {
+	if (IS_IRIS2_1(core) || IS_AR50_LITE(core)) {
  		/* Bring XTSS out of reset */
  		writel(0, wrapper_base + WRAPPER_TZ_XTSS_SW_RESET);
  	} else {

@@ -68,7 +68,7 @@ int venus_set_hw_state(struct venus_core *core, bool resume)
  	if (resume) {
  		venus_reset_cpu(core);
  	} else {
-		if (IS_IRIS2_1(core))
+		if (IS_IRIS2_1(core) || IS_AR50_LITE(core))
  			writel(WRAPPER_XTSS_SW_RESET_BIT,
  			       core->wrapper_tz_base + WRAPPER_TZ_XTSS_SW_RESET);
  		else

@@ -181,7 +181,7 @@ static int venus_shutdown_no_tz(struct venus_core *core)
  	void __iomem *wrapper_base = core->wrapper_base;
  	void __iomem *wrapper_tz_base = core->wrapper_tz_base;
 
-	if (IS_IRIS2_1(core)) {
+	if (IS_IRIS2_1(core) || IS_AR50_LITE(core)) {
  		/* Assert the reset to XTSS */
  		reg = readl(wrapper_tz_base + WRAPPER_TZ_XTSS_SW_RESET);
  		reg |= WRAPPER_XTSS_SW_RESET_BIT;

drivers/media/platform/qcom/venus/helpers.c +13 -0

index 8295542e1a7c..ae89369c6a07 100644
@@ -230,6 +230,80 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
  }
  EXPORT_SYMBOL_GPL(venus_helper_alloc_dpb_bufs);
 
+static void free_eos_buf(struct venus_inst *inst, struct intbuf *buf)
+{
+	list_del_init(&buf->list);
+	dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da,
+		       buf->attrs);
+	kfree(buf);
+}
+
+int venus_helper_free_eos_bufs(struct venus_inst *inst)
+{
+	struct intbuf *buf, *n;
+
+	list_for_each_entry_safe(buf, n, &inst->eosbufs, list) {

drivers/media/platform/qcom/venus/helpers.c +40 -0

+
+int venus_helper_alloc_eos_buf(struct venus_inst *inst,
+			       struct hfi_frame_data *data)
+{
+	struct venus_core *core = inst->core;
+	struct device *dev = core->dev;
+	struct intbuf *buf;
+	int ret = 0;
+
+	memset(data, 0, sizeof(*data));
+
+	data->buffer_type = HFI_BUFFER_INPUT;
+	data->flags = HFI_BUFFERFLAG_EOS;
+
+	if (IS_AR50_LITE(inst->core)) {
+		/* We must send valid sizes and addresses */
+		buf = kzalloc(sizeof(*buf), GFP_KERNEL);
+		if (!buf) {
+			ret = -ENOMEM;
+			goto fail;
+		}
+
+		buf->type = HFI_BUFFER_INPUT;
+		buf->size = SZ_4K;
+		buf->attrs = DMA_ATTR_NO_KERNEL_MAPPING;
+		buf->va = dma_alloc_attrs(dev, buf->size, &buf->da, GFP_KERNEL,
+					  buf->attrs);
+		if (!buf->va) {
+			ret = -ENOMEM;
+			goto fail;
+		}
+
+		list_add_tail(&buf->list, &inst->eosbufs);
+
+		data->alloc_len = buf->size;
+		data->device_addr = buf->da;
+
+	} else if (IS_V6(inst->core) &&
+		   is_fw_rev_or_older(inst->core, 1, 0, 87)) {
+		data->device_addr = 0;

drivers/media/platform/qcom/venus/helpers.c +8 -0

+
  static int intbufs_set_buffer(struct venus_inst *inst, u32 type)
  {
  	struct venus_core *core = inst->core;

@@ -630,6 +704,13 @@ static int platform_get_bufreq(struct venus_inst *inst, u32 buftype,
  	if (!hfi_plat || !hfi_plat->bufreq)
  		return -EINVAL;
 
+	/* Firmware buffer requirements for internal buffers only */
+	if (IS_AR50_LITE(inst->core))
+		if ((buftype != HFI_BUFFER_INPUT) &&
+		    (buftype != HFI_BUFFER_OUTPUT) &&
+		    (buftype != HFI_BUFFER_OUTPUT2))
+			return -EINVAL;
+
  	params.version = version;
  	params.num_vpp_pipes = inst->core->res->num_vpp_pipes;
 

drivers/media/platform/qcom/venus/helpers.h +2 -0

index 358e4f39c9c0..bf55fe3b8747 100644
@@ -58,6 +58,8 @@ int venus_helper_get_out_fmts(struct venus_inst *inst, u32 fmt, u32 *out_fmt,
  bool venus_helper_check_format(struct venus_inst *inst, u32 v4l2_pixfmt);
  int venus_helper_alloc_dpb_bufs(struct venus_inst *inst);
  int venus_helper_free_dpb_bufs(struct venus_inst *inst);
+int venus_helper_alloc_eos_buf(struct venus_inst *inst, struct hfi_frame_data *data);
+int venus_helper_free_eos_bufs(struct venus_inst *inst);
  int venus_helper_intbufs_alloc(struct venus_inst *inst);
  int venus_helper_intbufs_free(struct venus_inst *inst);
  int venus_helper_intbufs_realloc(struct venus_inst *inst);

drivers/media/platform/qcom/venus/hfi_helper.h +7 -3

index f44059f19505..128ddf8e3cd5 100644
@@ -397,13 +397,16 @@
  #define HFI_BUFFER_INTERNAL_PERSIST_1		0x5
  #define HFI_BUFFER_INTERNAL_SCRATCH(ver)	\
  	(((ver) == HFI_VERSION_4XX ||		\
-	(ver) == HFI_VERSION_6XX) ? 0x6 : 0x1000001)
+	(ver) == HFI_VERSION_6XX || (ver) == HFI_VERSION_6XX_LITE) \
+	? 0x6 : 0x1000001)
  #define HFI_BUFFER_INTERNAL_SCRATCH_1(ver)	\
  	(((ver) == HFI_VERSION_4XX ||		\
-	(ver) == HFI_VERSION_6XX) ? 0x7 : 0x1000005)
+	(ver) == HFI_VERSION_6XX || (ver) == HFI_VERSION_6XX_LITE) \
+	? 0x7 : 0x1000005)
  #define HFI_BUFFER_INTERNAL_SCRATCH_2(ver)	\
  	(((ver) == HFI_VERSION_4XX ||		\
-	(ver) == HFI_VERSION_6XX) ? 0x8 : 0x1000006)
+	(ver) == HFI_VERSION_6XX || (ver) == HFI_VERSION_6XX_LITE) \
+	? 0x8 : 0x1000006)
  #define HFI_BUFFER_EXTRADATA_INPUT(ver)		\
  	(((ver) == HFI_VERSION_4XX) ? 0xc : 0x1000002)
  #define HFI_BUFFER_EXTRADATA_OUTPUT(ver)	\

@@ -561,6 +564,7 @@ enum hfi_version {
  	HFI_VERSION_3XX,
  	HFI_VERSION_4XX,
  	HFI_VERSION_6XX,
+	HFI_VERSION_6XX_LITE,
  };
 
  struct hfi_buffer_info {

drivers/media/platform/qcom/venus/hfi_venus.c +9 -5

index b5f2ea879950..302776bf8fe6 100644
@@ -497,7 +497,7 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
  	if (count >= max_tries)
  		ret = -ETIMEDOUT;
 
-	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {
+	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core) || IS_AR50_LITE(hdev->core)) {
  		writel(0x1, cpu_cs_base + CPU_CS_H2XSOFTINTEN_V6);
  		writel(0x0, cpu_cs_base + CPU_CS_X2RPMH_V6);
  	}

@@ -565,6 +565,9 @@ static int venus_halt_axi(struct venus_hfi_device *hdev)
  	u32 mask_val;
  	int ret;
 
+	if (IS_AR50_LITE(hdev->core))
+		return 0;
+
  	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {
  		writel(0x3, cpu_cs_base + CPU_CS_X2RPMH_V6);
 

@@ -1134,7 +1137,8 @@ static irqreturn_t venus_isr(struct venus_core *core)
  	wrapper_base = hdev->core->wrapper_base;
 
  	status = readl(wrapper_base + WRAPPER_INTR_STATUS);
-	if (IS_IRIS2(core) || IS_IRIS2_1(core)) {
+
+	if (IS_IRIS2(core) || IS_IRIS2_1(core) || IS_AR50_LITE(core)) {
  		if (status & WRAPPER_INTR_STATUS_A2H_MASK ||
  		    status & WRAPPER_INTR_STATUS_A2HWD_MASK_V6 ||
  		    status & CPU_CS_SCIACMDARG0_INIT_IDLE_MSG_MASK)

@@ -1146,7 +1150,7 @@ static irqreturn_t venus_isr(struct venus_core *core)
  			hdev->irq_status = status;
  	}
  	writel(1, cpu_cs_base + CPU_CS_A2HSOFTINTCLR);
-	if (!(IS_IRIS2(core) || IS_IRIS2_1(core)))
+	if (!(IS_IRIS2(core) || IS_IRIS2_1(core) || IS_AR50_LITE(core)))
  		writel(status, wrapper_base + WRAPPER_INTR_CLEAR);
 
  	return IRQ_WAKE_THREAD;

@@ -1531,7 +1535,7 @@ static bool venus_cpu_and_video_core_idle(struct venus_hfi_device *hdev)
  	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
  	u32 ctrl_status, cpu_status;
 
-	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
+	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core) || IS_AR50_LITE(hdev->core))
  		cpu_status = readl(wrapper_tz_base + WRAPPER_TZ_CPU_STATUS_V6);
  	else
  		cpu_status = readl(wrapper_base + WRAPPER_CPU_STATUS);

@@ -1551,7 +1555,7 @@ static bool venus_cpu_idle_and_pc_ready(struct venus_hfi_device *hdev)
  	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
  	u32 ctrl_status, cpu_status;
 
-	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
+	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core) || IS_AR50_LITE(hdev->core))
  		cpu_status = readl(wrapper_tz_base + WRAPPER_TZ_CPU_STATUS_V6);
  	else
  		cpu_status = readl(wrapper_base + WRAPPER_CPU_STATUS);

drivers/media/platform/qcom/venus/pm_helpers.c +1 -0

index 409aa9bd0b5d..5d9dfe3fd043 100644
@@ -1168,6 +1168,7 @@ const struct venus_pm_ops *venus_pm_get(enum hfi_version version)
  		return &pm_ops_v3;
  	case HFI_VERSION_4XX:
  	case HFI_VERSION_6XX:
+	case HFI_VERSION_6XX_LITE:
  		return &pm_ops_v4;
  	}
 

drivers/media/platform/qcom/venus/vdec.c +6 -8

index 99ce5fd41577..87c7901b280e 100644
@@ -550,7 +550,7 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd)
  {
  	struct venus_inst *inst = to_inst(file);
  	struct vb2_queue *dst_vq;
-	struct hfi_frame_data fdata = {0};
+	struct hfi_frame_data fdata;
  	int ret;
 
  	ret = v4l2_m2m_ioctl_try_decoder_cmd(file, fh, cmd);

@@ -561,18 +561,15 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd)
 
  	if (cmd->cmd == V4L2_DEC_CMD_STOP) {
  		/*
-		 * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on
+		 * Implement V4L2_DEC_CMD_STOP by enqueue a buffer on
  		 * decoder input to signal EOS.
  		 */
  		if (!(inst->streamon_out && inst->streamon_cap))
  			goto unlock;
 
-		fdata.buffer_type = HFI_BUFFER_INPUT;
-		fdata.flags |= HFI_BUFFERFLAG_EOS;
-		if (IS_V6(inst->core) && is_fw_rev_or_older(inst->core, 1, 0, 87))
-			fdata.device_addr = 0;
-		else
-			fdata.device_addr = 0xdeadb000;
+		ret = venus_helper_alloc_eos_buf(inst, &fdata);
+		if (ret)
+			goto unlock;
 
  		ret = hfi_session_process_buf(inst, &fdata);
 

@@ -1332,6 +1329,7 @@ static void vdec_session_release(struct venus_inst *inst)
  		hfi_session_abort(inst);
 
  	venus_helper_free_dpb_bufs(inst);
+	venus_helper_free_eos_bufs(inst);
  	venus_pm_load_scale(inst);
  	INIT_LIST_HEAD(&inst->registeredbufs);
  	mutex_unlock(&inst->lock);

Quoted 19 lines · Jorge Ramirez-Ortiz

The AR50_LITE is a streamlined variant of the AR50 video core, designed for power and cost-efficient platforms.

It supports hardware-accelerated decoding of H.264, HEVC, and VP9 formats.

Signed-off-by Jorge Ramirez-Ortiz <[email protected]>

---
  drivers/media/platform/qcom/venus/core.c      | 11 ++-
  drivers/media/platform/qcom/venus/core.h      |  5 +-
  drivers/media/platform/qcom/venus/firmware.c  |  8 +-
  drivers/media/platform/qcom/venus/helpers.c   | 81 +++++++++++++++++++
  drivers/media/platform/qcom/venus/helpers.h   |  2 +
  .../media/platform/qcom/venus/hfi_helper.h    | 10 ++-
  drivers/media/platform/qcom/venus/hfi_venus.c | 14 ++--
  .../media/platform/qcom/venus/pm_helpers.c    |  1 +
  drivers/media/platform/qcom/venus/vdec.c      | 15 ++--
  9 files changed, 123 insertions(+), 24 deletions(-)

drivers/media/platform/qcom/venus/core.c static int venus_enumerate_codecs(struct venus_core *core, u32 type) · line 256

Quoted 10 lines

drivers/media/platform/qcom/venus/core.c +1 -2

index d305d74bb152..736ef53d988d 100644
@@ -254,14 +254,19 @@ static int venus_enumerate_codecs(struct venus_core *core, u32 type)
  static void venus_assign_register_offsets(struct venus_core *core)
  {
-	if (IS_IRIS2(core) || IS_IRIS2_1(core)) {
-		core->vbif_base = core->base + VBIF_BASE;
+	if (IS_IRIS2(core) || IS_IRIS2_1(core) || IS_AR50_LITE(core)) {

Is there a property that IS_IRIS_2_1 || IS_AR50_LITE that we could use instead of expanding this IS_THING list ?

Quoted 52 lines

drivers/media/platform/qcom/venus/core.c +7 -1

  		core->cpu_base = core->base + CPU_BASE_V6;
  		core->cpu_cs_base = core->base + CPU_CS_BASE_V6;
  		core->cpu_ic_base = core->base + CPU_IC_BASE_V6;
  		core->wrapper_base = core->base + WRAPPER_BASE_V6;
  		core->wrapper_tz_base = core->base + WRAPPER_TZ_BASE_V6;
-		core->aon_base = core->base + AON_BASE_V6;
+		if (IS_AR50_LITE(core)) {
+			core->vbif_base = NULL;
+			core->aon_base = NULL;
+		} else {
+			core->vbif_base = core->base + VBIF_BASE;
+			core->aon_base = core->base + AON_BASE_V6;
+		}
  	} else {
  		core->vbif_base = core->base + VBIF_BASE;
  		core->cpu_base = core->base + CPU_BASE;

drivers/media/platform/qcom/venus/core.h +4 -1

index b412e0c5515a..122441f9600a 100644
@@ -382,6 +382,7 @@ enum venus_inst_modes {
   * @lock:	instance lock
   * @core:	a reference to the core struct
   * @clk_data:	clock data per core ID
+ * @eosbufs:	a lit of EOS buffers
   * @dpbbufs:	a list of decoded picture buffers
   * @internalbufs:	a list of internal bufferes
   * @registeredbufs:	a list of registered capture bufferes

@@ -450,6 +451,7 @@ struct venus_inst {
  	struct mutex lock;
  	struct venus_core *core;
  	struct clock_data clk_data;
+	struct list_head eosbufs;
  	struct list_head dpbbufs;
  	struct list_head internalbufs;
  	struct list_head registeredbufs;

@@ -520,7 +522,8 @@ struct venus_inst {
  #define IS_V1(core)	((core)->res->hfi_version == HFI_VERSION_1XX)
  #define IS_V3(core)	((core)->res->hfi_version == HFI_VERSION_3XX)
  #define IS_V4(core)	((core)->res->hfi_version == HFI_VERSION_4XX)
-#define IS_V6(core)	((core)->res->hfi_version == HFI_VERSION_6XX)
+#define IS_V6(core)     (((core)->res->hfi_version == HFI_VERSION_6XX) || \
+			 ((core)->res->hfi_version == HFI_VERSION_6XX_LITE))
  #define IS_AR50(core)		((core)->res->vpu_version == VPU_VERSION_AR50)
  #define IS_AR50_LITE(core)	((core)->res->vpu_version == VPU_VERSION_AR50_LITE)

drivers/media/platform/qcom/venus/firmware.c +0 -0

index 66a18830e66d..f8dcef0426ac 100644

drivers/media/platform/qcom/venus/firmware.c static void venus_reset_cpu(struct venus_core *core) · line 32

Quoted 4 lines

drivers/media/platform/qcom/venus/firmware.c +1 -1

@@ -30,7 +30,7 @@ static void venus_reset_cpu(struct venus_core *core)
  	u32 fw_size = core->fw.mapped_mem_size;
  	void __iomem *wrapper_base;
-	if (IS_IRIS2_1(core))
+	if (IS_IRIS2_1(core) || IS_AR50_LITE(core))

For example here.

Quoted 53 lines

drivers/media/platform/qcom/venus/firmware.c +3 -3

  		wrapper_base = core->wrapper_tz_base;
  	else
  		wrapper_base = core->wrapper_base;

@@ -42,7 +42,7 @@ static void venus_reset_cpu(struct venus_core *core)
  	writel(fw_size, wrapper_base + WRAPPER_NONPIX_START_ADDR);
  	writel(fw_size, wrapper_base + WRAPPER_NONPIX_END_ADDR);
-	if (IS_IRIS2_1(core)) {
+	if (IS_IRIS2_1(core) || IS_AR50_LITE(core)) {
  		/* Bring XTSS out of reset */
  		writel(0, wrapper_base + WRAPPER_TZ_XTSS_SW_RESET);
  	} else {

@@ -68,7 +68,7 @@ int venus_set_hw_state(struct venus_core *core, bool resume)
  	if (resume) {
  		venus_reset_cpu(core);
  	} else {
-		if (IS_IRIS2_1(core))
+		if (IS_IRIS2_1(core) || IS_AR50_LITE(core))
  			writel(WRAPPER_XTSS_SW_RESET_BIT,
  			       core->wrapper_tz_base + WRAPPER_TZ_XTSS_SW_RESET);
  		else

@@ -181,7 +181,7 @@ static int venus_shutdown_no_tz(struct venus_core *core)
  	void __iomem *wrapper_base = core->wrapper_base;
  	void __iomem *wrapper_tz_base = core->wrapper_tz_base;
-	if (IS_IRIS2_1(core)) {
+	if (IS_IRIS2_1(core) || IS_AR50_LITE(core)) {
  		/* Assert the reset to XTSS */
  		reg = readl(wrapper_tz_base + WRAPPER_TZ_XTSS_SW_RESET);
  		reg |= WRAPPER_XTSS_SW_RESET_BIT;

drivers/media/platform/qcom/venus/helpers.c +18 -0

index 8295542e1a7c..ae89369c6a07 100644
@@ -230,6 +230,80 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
  }
  EXPORT_SYMBOL_GPL(venus_helper_alloc_dpb_bufs);
+static void free_eos_buf(struct venus_inst *inst, struct intbuf *buf)
+{
+	list_del_init(&buf->list);
+	dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da,
+		       buf->attrs);
+	kfree(buf);
+}
+
+int venus_helper_free_eos_bufs(struct venus_inst *inst)
+{
+	struct intbuf *buf, *n;
+
+	list_for_each_entry_safe(buf, n, &inst->eosbufs, list) {
+		free_eos_buf(inst, buf);
+	}
+
+	if (list_empty(&inst->eosbufs))
+		INIT_LIST_HEAD(&inst->eosbufs);

drivers/media/platform/qcom/venus/helpers.c int venus_helper_alloc_dpb_bufs(struct venus_inst *inst) · line 255

Quoted 5 lines

drivers/media/platform/qcom/venus/helpers.c +5 -0

+
+	return 0;
+
+}
+EXPORT_SYMBOL_GPL(venus_helper_free_eos_bufs);

These EOS buf things look like they are not specific to ar50, could you add these in a preceding patch to this ?

Quoted 47 lines

drivers/media/platform/qcom/venus/helpers.c +46 -0

+
+int venus_helper_alloc_eos_buf(struct venus_inst *inst,
+			       struct hfi_frame_data *data)
+{
+	struct venus_core *core = inst->core;
+	struct device *dev = core->dev;
+	struct intbuf *buf;
+	int ret = 0;
+
+	memset(data, 0, sizeof(*data));
+
+	data->buffer_type = HFI_BUFFER_INPUT;
+	data->flags = HFI_BUFFERFLAG_EOS;
+
+	if (IS_AR50_LITE(inst->core)) {
+		/* We must send valid sizes and addresses */
+		buf = kzalloc(sizeof(*buf), GFP_KERNEL);
+		if (!buf) {
+			ret = -ENOMEM;
+			goto fail;
+		}
+
+		buf->type = HFI_BUFFER_INPUT;
+		buf->size = SZ_4K;
+		buf->attrs = DMA_ATTR_NO_KERNEL_MAPPING;
+		buf->va = dma_alloc_attrs(dev, buf->size, &buf->da, GFP_KERNEL,
+					  buf->attrs);
+		if (!buf->va) {
+			ret = -ENOMEM;
+			goto fail;
+		}
+
+		list_add_tail(&buf->list, &inst->eosbufs);
+
+		data->alloc_len = buf->size;
+		data->device_addr = buf->da;
+
+	} else if (IS_V6(inst->core) &&
+		   is_fw_rev_or_older(inst->core, 1, 0, 87)) {
+		data->device_addr = 0;
+	} else {
+		data->device_addr = 0xdeadb000;
+	}
+
+	return 0;
+fail:

drivers/media/platform/qcom/venus/helpers.c int venus_helper_alloc_dpb_bufs(struct venus_inst *inst) · line 305

Quoted 4 lines

drivers/media/platform/qcom/venus/helpers.c +4 -0

+	kfree(buf);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(venus_helper_alloc_eos_buf);

I think the series would be nicer if the EOS buf decomposition stuff went into a patch that preceded this one, and then you add the IS_AR50_LITE() in the next patch progressively.

Quoted 164 lines

drivers/media/platform/qcom/venus/helpers.c +8 -0

+
  static int intbufs_set_buffer(struct venus_inst *inst, u32 type)
  {
  	struct venus_core *core = inst->core;

@@ -630,6 +704,13 @@ static int platform_get_bufreq(struct venus_inst *inst, u32 buftype,
  	if (!hfi_plat || !hfi_plat->bufreq)
  		return -EINVAL;
+	/* Firmware buffer requirements for internal buffers only */
+	if (IS_AR50_LITE(inst->core))
+		if ((buftype != HFI_BUFFER_INPUT) &&
+		    (buftype != HFI_BUFFER_OUTPUT) &&
+		    (buftype != HFI_BUFFER_OUTPUT2))
+			return -EINVAL;
+
  	params.version = version;
  	params.num_vpp_pipes = inst->core->res->num_vpp_pipes;

drivers/media/platform/qcom/venus/helpers.h +2 -0

index 358e4f39c9c0..bf55fe3b8747 100644
@@ -58,6 +58,8 @@ int venus_helper_get_out_fmts(struct venus_inst *inst, u32 fmt, u32 *out_fmt,
  bool venus_helper_check_format(struct venus_inst *inst, u32 v4l2_pixfmt);
  int venus_helper_alloc_dpb_bufs(struct venus_inst *inst);
  int venus_helper_free_dpb_bufs(struct venus_inst *inst);
+int venus_helper_alloc_eos_buf(struct venus_inst *inst, struct hfi_frame_data *data);
+int venus_helper_free_eos_bufs(struct venus_inst *inst);
  int venus_helper_intbufs_alloc(struct venus_inst *inst);
  int venus_helper_intbufs_free(struct venus_inst *inst);
  int venus_helper_intbufs_realloc(struct venus_inst *inst);

drivers/media/platform/qcom/venus/hfi_helper.h +7 -3

index f44059f19505..128ddf8e3cd5 100644
@@ -397,13 +397,16 @@
  #define HFI_BUFFER_INTERNAL_PERSIST_1		0x5
  #define HFI_BUFFER_INTERNAL_SCRATCH(ver)	\
  	(((ver) == HFI_VERSION_4XX ||		\
-	(ver) == HFI_VERSION_6XX) ? 0x6 : 0x1000001)
+	(ver) == HFI_VERSION_6XX || (ver) == HFI_VERSION_6XX_LITE) \
+	? 0x6 : 0x1000001)
  #define HFI_BUFFER_INTERNAL_SCRATCH_1(ver)	\
  	(((ver) == HFI_VERSION_4XX ||		\
-	(ver) == HFI_VERSION_6XX) ? 0x7 : 0x1000005)
+	(ver) == HFI_VERSION_6XX || (ver) == HFI_VERSION_6XX_LITE) \
+	? 0x7 : 0x1000005)
  #define HFI_BUFFER_INTERNAL_SCRATCH_2(ver)	\
  	(((ver) == HFI_VERSION_4XX ||		\
-	(ver) == HFI_VERSION_6XX) ? 0x8 : 0x1000006)
+	(ver) == HFI_VERSION_6XX || (ver) == HFI_VERSION_6XX_LITE) \
+	? 0x8 : 0x1000006)
  #define HFI_BUFFER_EXTRADATA_INPUT(ver)		\
  	(((ver) == HFI_VERSION_4XX) ? 0xc : 0x1000002)
  #define HFI_BUFFER_EXTRADATA_OUTPUT(ver)	\

@@ -561,6 +564,7 @@ enum hfi_version {
  	HFI_VERSION_3XX,
  	HFI_VERSION_4XX,
  	HFI_VERSION_6XX,
+	HFI_VERSION_6XX_LITE,
  };
  struct hfi_buffer_info {

drivers/media/platform/qcom/venus/hfi_venus.c +9 -5

index b5f2ea879950..302776bf8fe6 100644
@@ -497,7 +497,7 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
  	if (count >= max_tries)
  		ret = -ETIMEDOUT;
-	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {
+	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core) || IS_AR50_LITE(hdev->core)) {
  		writel(0x1, cpu_cs_base + CPU_CS_H2XSOFTINTEN_V6);
  		writel(0x0, cpu_cs_base + CPU_CS_X2RPMH_V6);
  	}

@@ -565,6 +565,9 @@ static int venus_halt_axi(struct venus_hfi_device *hdev)
  	u32 mask_val;
  	int ret;
+	if (IS_AR50_LITE(hdev->core))
+		return 0;
+
  	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {
  		writel(0x3, cpu_cs_base + CPU_CS_X2RPMH_V6);

@@ -1134,7 +1137,8 @@ static irqreturn_t venus_isr(struct venus_core *core)
  	wrapper_base = hdev->core->wrapper_base;
  	status = readl(wrapper_base + WRAPPER_INTR_STATUS);
-	if (IS_IRIS2(core) || IS_IRIS2_1(core)) {
+
+	if (IS_IRIS2(core) || IS_IRIS2_1(core) || IS_AR50_LITE(core)) {
  		if (status & WRAPPER_INTR_STATUS_A2H_MASK ||
  		    status & WRAPPER_INTR_STATUS_A2HWD_MASK_V6 ||
  		    status & CPU_CS_SCIACMDARG0_INIT_IDLE_MSG_MASK)

@@ -1146,7 +1150,7 @@ static irqreturn_t venus_isr(struct venus_core *core)
  			hdev->irq_status = status;
  	}
  	writel(1, cpu_cs_base + CPU_CS_A2HSOFTINTCLR);
-	if (!(IS_IRIS2(core) || IS_IRIS2_1(core)))
+	if (!(IS_IRIS2(core) || IS_IRIS2_1(core) || IS_AR50_LITE(core)))
  		writel(status, wrapper_base + WRAPPER_INTR_CLEAR);
  	return IRQ_WAKE_THREAD;

@@ -1531,7 +1535,7 @@ static bool venus_cpu_and_video_core_idle(struct venus_hfi_device *hdev)
  	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
  	u32 ctrl_status, cpu_status;
-	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
+	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core) || IS_AR50_LITE(hdev->core))
  		cpu_status = readl(wrapper_tz_base + WRAPPER_TZ_CPU_STATUS_V6);
  	else
  		cpu_status = readl(wrapper_base + WRAPPER_CPU_STATUS);

@@ -1551,7 +1555,7 @@ static bool venus_cpu_idle_and_pc_ready(struct venus_hfi_device *hdev)
  	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
  	u32 ctrl_status, cpu_status;
-	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
+	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core) || IS_AR50_LITE(hdev->core))
  		cpu_status = readl(wrapper_tz_base + WRAPPER_TZ_CPU_STATUS_V6);
  	else
  		cpu_status = readl(wrapper_base + WRAPPER_CPU_STATUS);

drivers/media/platform/qcom/venus/pm_helpers.c +1 -0

index 409aa9bd0b5d..5d9dfe3fd043 100644
@@ -1168,6 +1168,7 @@ const struct venus_pm_ops *venus_pm_get(enum hfi_version version)
  		return &pm_ops_v3;
  	case HFI_VERSION_4XX:
  	case HFI_VERSION_6XX:
+	case HFI_VERSION_6XX_LITE:
  		return &pm_ops_v4;
  	}

drivers/media/platform/qcom/venus/vdec.c +6 -8

index 99ce5fd41577..87c7901b280e 100644
@@ -550,7 +550,7 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd)
  {
  	struct venus_inst *inst = to_inst(file);
  	struct vb2_queue *dst_vq;
-	struct hfi_frame_data fdata = {0};
+	struct hfi_frame_data fdata;
  	int ret;
  	ret = v4l2_m2m_ioctl_try_decoder_cmd(file, fh, cmd);

@@ -561,18 +561,15 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd)
  	if (cmd->cmd == V4L2_DEC_CMD_STOP) {
  		/*
-		 * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on
+		 * Implement V4L2_DEC_CMD_STOP by enqueue a buffer on
  		 * decoder input to signal EOS.
  		 */
  		if (!(inst->streamon_out && inst->streamon_cap))
  			goto unlock;
-		fdata.buffer_type = HFI_BUFFER_INPUT;
-		fdata.flags |= HFI_BUFFERFLAG_EOS;
-		if (IS_V6(inst->core) && is_fw_rev_or_older(inst->core, 1, 0, 87))
-			fdata.device_addr = 0;
-		else
-			fdata.device_addr = 0xdeadb000;
+		ret = venus_helper_alloc_eos_buf(inst, &fdata);
+		if (ret)
+			goto unlock;
  		ret = hfi_session_process_buf(inst, &fdata);

@@ -1332,6 +1329,7 @@ static void vdec_session_release(struct venus_inst *inst)
  		hfi_session_abort(inst);
  	venus_helper_free_dpb_bufs(inst);
+	venus_helper_free_eos_bufs(inst);
  	venus_pm_load_scale(inst);
  	INIT_LIST_HEAD(&inst->registeredbufs);
  	mutex_unlock(&inst->lock);

drivers/media/platform/qcom/venus/vdec.c static int vdec_open(struct file *file) · line 1685

Quoted 6 lines

drivers/media/platform/qcom/venus/vdec.c +1 -0

@@ -1682,6 +1680,7 @@ static int vdec_open(struct file *file)
  	if (!inst)
  		return -ENOMEM;
+	INIT_LIST_HEAD(&inst->eosbufs);
  	INIT_LIST_HEAD(&inst->dpbbufs);
  	INIT_LIST_HEAD(&inst->registeredbufs);
  	INIT_LIST_HEAD(&inst->internalbufs);

Otherwise LGTM.

---
bod

Quoted 20 lines · Bryan O'Donoghue

Quoted 19 lines · Jorge Ramirez-Ortiz

The AR50_LITE is a streamlined variant of the AR50 video core, designed for power and cost-efficient platforms.

It supports hardware-accelerated decoding of H.264, HEVC, and VP9 formats.

Signed-off-by Jorge Ramirez-Ortiz <[email protected]>

---
   drivers/media/platform/qcom/venus/core.c      | 11 ++-
   drivers/media/platform/qcom/venus/core.h      |  5 +-
   drivers/media/platform/qcom/venus/firmware.c  |  8 +-
   drivers/media/platform/qcom/venus/helpers.c   | 81 +++++++++++++++++++
   drivers/media/platform/qcom/venus/helpers.h   |  2 +
   .../media/platform/qcom/venus/hfi_helper.h    | 10 ++-
   drivers/media/platform/qcom/venus/hfi_venus.c | 14 ++--
   .../media/platform/qcom/venus/pm_helpers.c    |  1 +
   drivers/media/platform/qcom/venus/vdec.c      | 15 ++--
   9 files changed, 123 insertions(+), 24 deletions(-)

Quoted 13 lines

drivers/media/platform/qcom/venus/core.c static int venus_enumerate_codecs(struct venus_core *core, u32 type) · line 256

Quoted 10 lines

drivers/media/platform/qcom/venus/core.c +1 -2

index d305d74bb152..736ef53d988d 100644
@@ -254,14 +254,19 @@ static int venus_enumerate_codecs(struct venus_core *core, u32 type)
   static void venus_assign_register_offsets(struct venus_core *core)
   {
-	if (IS_IRIS2(core) || IS_IRIS2_1(core)) {
-		core->vbif_base = core->base + VBIF_BASE;
+	if (IS_IRIS2(core) || IS_IRIS2_1(core) || IS_AR50_LITE(core)) {

Is there a property that IS_IRIS_2_1 || IS_AR50_LITE that we could use instead of expanding this IS_THING list ?

I know what you mean but I think looking for possible comonalities between cores will just make the partitioning more obscure.

we could perhaps define a different type of macro ie, something like IS_CORE(core, IRIS2, IRIS2_1, AR50_LITE) ? shall I go ahead with this type of macro instead?

Quoted 112 lines

Quoted 52 lines

drivers/media/platform/qcom/venus/core.c +7 -1

   		core->cpu_base = core->base + CPU_BASE_V6;
   		core->cpu_cs_base = core->base + CPU_CS_BASE_V6;
   		core->cpu_ic_base = core->base + CPU_IC_BASE_V6;
   		core->wrapper_base = core->base + WRAPPER_BASE_V6;
   		core->wrapper_tz_base = core->base + WRAPPER_TZ_BASE_V6;
-		core->aon_base = core->base + AON_BASE_V6;
+		if (IS_AR50_LITE(core)) {
+			core->vbif_base = NULL;
+			core->aon_base = NULL;
+		} else {
+			core->vbif_base = core->base + VBIF_BASE;
+			core->aon_base = core->base + AON_BASE_V6;
+		}
   	} else {
   		core->vbif_base = core->base + VBIF_BASE;
   		core->cpu_base = core->base + CPU_BASE;

drivers/media/platform/qcom/venus/core.h +4 -1

index b412e0c5515a..122441f9600a 100644
@@ -382,6 +382,7 @@ enum venus_inst_modes {
    * @lock:	instance lock
    * @core:	a reference to the core struct
    * @clk_data:	clock data per core ID
+ * @eosbufs:	a lit of EOS buffers
    * @dpbbufs:	a list of decoded picture buffers
    * @internalbufs:	a list of internal bufferes
    * @registeredbufs:	a list of registered capture bufferes

@@ -450,6 +451,7 @@ struct venus_inst {
   	struct mutex lock;
   	struct venus_core *core;
   	struct clock_data clk_data;
+	struct list_head eosbufs;
   	struct list_head dpbbufs;
   	struct list_head internalbufs;
   	struct list_head registeredbufs;

@@ -520,7 +522,8 @@ struct venus_inst {
   #define IS_V1(core)	((core)->res->hfi_version == HFI_VERSION_1XX)
   #define IS_V3(core)	((core)->res->hfi_version == HFI_VERSION_3XX)
   #define IS_V4(core)	((core)->res->hfi_version == HFI_VERSION_4XX)
-#define IS_V6(core)	((core)->res->hfi_version == HFI_VERSION_6XX)
+#define IS_V6(core)     (((core)->res->hfi_version == HFI_VERSION_6XX) || \
+			 ((core)->res->hfi_version == HFI_VERSION_6XX_LITE))
   #define IS_AR50(core)		((core)->res->vpu_version == VPU_VERSION_AR50)
   #define IS_AR50_LITE(core)	((core)->res->vpu_version == VPU_VERSION_AR50_LITE)

drivers/media/platform/qcom/venus/firmware.c +0 -0

index 66a18830e66d..f8dcef0426ac 100644

drivers/media/platform/qcom/venus/firmware.c static void venus_reset_cpu(struct venus_core *core) · line 32

Quoted 4 lines

drivers/media/platform/qcom/venus/firmware.c +1 -1

@@ -30,7 +30,7 @@ static void venus_reset_cpu(struct venus_core *core)
   	u32 fw_size = core->fw.mapped_mem_size;
   	void __iomem *wrapper_base;
-	if (IS_IRIS2_1(core))
+	if (IS_IRIS2_1(core) || IS_AR50_LITE(core))

For example here.

Quoted 53 lines

drivers/media/platform/qcom/venus/firmware.c +3 -3

   		wrapper_base = core->wrapper_tz_base;
   	else
   		wrapper_base = core->wrapper_base;

@@ -42,7 +42,7 @@ static void venus_reset_cpu(struct venus_core *core)
   	writel(fw_size, wrapper_base + WRAPPER_NONPIX_START_ADDR);
   	writel(fw_size, wrapper_base + WRAPPER_NONPIX_END_ADDR);
-	if (IS_IRIS2_1(core)) {
+	if (IS_IRIS2_1(core) || IS_AR50_LITE(core)) {
   		/* Bring XTSS out of reset */
   		writel(0, wrapper_base + WRAPPER_TZ_XTSS_SW_RESET);
   	} else {

@@ -68,7 +68,7 @@ int venus_set_hw_state(struct venus_core *core, bool resume)
   	if (resume) {
   		venus_reset_cpu(core);
   	} else {
-		if (IS_IRIS2_1(core))
+		if (IS_IRIS2_1(core) || IS_AR50_LITE(core))
   			writel(WRAPPER_XTSS_SW_RESET_BIT,
   			       core->wrapper_tz_base + WRAPPER_TZ_XTSS_SW_RESET);
   		else

@@ -181,7 +181,7 @@ static int venus_shutdown_no_tz(struct venus_core *core)
   	void __iomem *wrapper_base = core->wrapper_base;
   	void __iomem *wrapper_tz_base = core->wrapper_tz_base;
-	if (IS_IRIS2_1(core)) {
+	if (IS_IRIS2_1(core) || IS_AR50_LITE(core)) {
   		/* Assert the reset to XTSS */
   		reg = readl(wrapper_tz_base + WRAPPER_TZ_XTSS_SW_RESET);
   		reg |= WRAPPER_XTSS_SW_RESET_BIT;

drivers/media/platform/qcom/venus/helpers.c +18 -0

index 8295542e1a7c..ae89369c6a07 100644
@@ -230,6 +230,80 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
   }
   EXPORT_SYMBOL_GPL(venus_helper_alloc_dpb_bufs);
+static void free_eos_buf(struct venus_inst *inst, struct intbuf *buf)
+{
+	list_del_init(&buf->list);
+	dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da,
+		       buf->attrs);
+	kfree(buf);
+}
+
+int venus_helper_free_eos_bufs(struct venus_inst *inst)
+{
+	struct intbuf *buf, *n;
+
+	list_for_each_entry_safe(buf, n, &inst->eosbufs, list) {
+		free_eos_buf(inst, buf);
+	}
+
+	if (list_empty(&inst->eosbufs))
+		INIT_LIST_HEAD(&inst->eosbufs);

Quoted 8 lines

drivers/media/platform/qcom/venus/helpers.c int venus_helper_alloc_dpb_bufs(struct venus_inst *inst) · line 255

Quoted 5 lines

drivers/media/platform/qcom/venus/helpers.c +5 -0

+
+	return 0;
+
+}
+EXPORT_SYMBOL_GPL(venus_helper_free_eos_bufs);

These EOS buf things look like they are not specific to ar50, could you add these in a preceding patch to this ?

um, previous cores seemed to tolerate passing empty pointers to the VPU firmware without issue. Unfortunately, with the AR50_LITE, this results in segmentation faults in the _firmware_. The purpose of this function is to add support for AR50_LITE while preserving the existing behaviour for other cores...introducing a dedicated function felt cleaner than further complicating the existing conditional.
Given that the previous conditional was just an if/else I don’t really see a lot of benefit in refactoring the earlier conditionals _before_ adding the AR50_LITE logic. Would you be OK if we keep it as proposed?

Quoted 48 lines

Quoted 48 lines

drivers/media/platform/qcom/venus/helpers.c +46 -0

+
+int venus_helper_alloc_eos_buf(struct venus_inst *inst,
+			       struct hfi_frame_data *data)
+{
+	struct venus_core *core = inst->core;
+	struct device *dev = core->dev;
+	struct intbuf *buf;
+	int ret = 0;
+
+	memset(data, 0, sizeof(*data));
+
+	data->buffer_type = HFI_BUFFER_INPUT;
+	data->flags = HFI_BUFFERFLAG_EOS;
+
+	if (IS_AR50_LITE(inst->core)) {
+		/* We must send valid sizes and addresses */
+		buf = kzalloc(sizeof(*buf), GFP_KERNEL);
+		if (!buf) {
+			ret = -ENOMEM;
+			goto fail;
+		}
+
+		buf->type = HFI_BUFFER_INPUT;
+		buf->size = SZ_4K;
+		buf->attrs = DMA_ATTR_NO_KERNEL_MAPPING;
+		buf->va = dma_alloc_attrs(dev, buf->size, &buf->da, GFP_KERNEL,
+					  buf->attrs);
+		if (!buf->va) {
+			ret = -ENOMEM;
+			goto fail;
+		}
+
+		list_add_tail(&buf->list, &inst->eosbufs);
+
+		data->alloc_len = buf->size;
+		data->device_addr = buf->da;
+
+	} else if (IS_V6(inst->core) &&
+		   is_fw_rev_or_older(inst->core, 1, 0, 87)) {
+		data->device_addr = 0;
+	} else {
+		data->device_addr = 0xdeadb000;
+	}
+
+	return 0;
+fail:

Quoted 8 lines

drivers/media/platform/qcom/venus/helpers.c int venus_helper_alloc_dpb_bufs(struct venus_inst *inst) · line 305

Quoted 4 lines

drivers/media/platform/qcom/venus/helpers.c +4 -0

+	kfree(buf);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(venus_helper_alloc_eos_buf);

I think the series would be nicer if the EOS buf decomposition stuff went into a patch that preceded this one, and then you add the IS_AR50_LITE() in the next patch progressively.

see above

Quoted 175 lines

Quoted 164 lines

drivers/media/platform/qcom/venus/helpers.c +8 -0

+
   static int intbufs_set_buffer(struct venus_inst *inst, u32 type)
   {
   	struct venus_core *core = inst->core;

@@ -630,6 +704,13 @@ static int platform_get_bufreq(struct venus_inst *inst, u32 buftype,
   	if (!hfi_plat || !hfi_plat->bufreq)
   		return -EINVAL;
+	/* Firmware buffer requirements for internal buffers only */
+	if (IS_AR50_LITE(inst->core))
+		if ((buftype != HFI_BUFFER_INPUT) &&
+		    (buftype != HFI_BUFFER_OUTPUT) &&
+		    (buftype != HFI_BUFFER_OUTPUT2))
+			return -EINVAL;
+
   	params.version = version;
   	params.num_vpp_pipes = inst->core->res->num_vpp_pipes;

drivers/media/platform/qcom/venus/helpers.h +2 -0

index 358e4f39c9c0..bf55fe3b8747 100644
@@ -58,6 +58,8 @@ int venus_helper_get_out_fmts(struct venus_inst *inst, u32 fmt, u32 *out_fmt,
   bool venus_helper_check_format(struct venus_inst *inst, u32 v4l2_pixfmt);
   int venus_helper_alloc_dpb_bufs(struct venus_inst *inst);
   int venus_helper_free_dpb_bufs(struct venus_inst *inst);
+int venus_helper_alloc_eos_buf(struct venus_inst *inst, struct hfi_frame_data *data);
+int venus_helper_free_eos_bufs(struct venus_inst *inst);
   int venus_helper_intbufs_alloc(struct venus_inst *inst);
   int venus_helper_intbufs_free(struct venus_inst *inst);
   int venus_helper_intbufs_realloc(struct venus_inst *inst);

drivers/media/platform/qcom/venus/hfi_helper.h +7 -3

index f44059f19505..128ddf8e3cd5 100644
@@ -397,13 +397,16 @@
   #define HFI_BUFFER_INTERNAL_PERSIST_1		0x5
   #define HFI_BUFFER_INTERNAL_SCRATCH(ver)	\
   	(((ver) == HFI_VERSION_4XX ||		\
-	(ver) == HFI_VERSION_6XX) ? 0x6 : 0x1000001)
+	(ver) == HFI_VERSION_6XX || (ver) == HFI_VERSION_6XX_LITE) \
+	? 0x6 : 0x1000001)
   #define HFI_BUFFER_INTERNAL_SCRATCH_1(ver)	\
   	(((ver) == HFI_VERSION_4XX ||		\
-	(ver) == HFI_VERSION_6XX) ? 0x7 : 0x1000005)
+	(ver) == HFI_VERSION_6XX || (ver) == HFI_VERSION_6XX_LITE) \
+	? 0x7 : 0x1000005)
   #define HFI_BUFFER_INTERNAL_SCRATCH_2(ver)	\
   	(((ver) == HFI_VERSION_4XX ||		\
-	(ver) == HFI_VERSION_6XX) ? 0x8 : 0x1000006)
+	(ver) == HFI_VERSION_6XX || (ver) == HFI_VERSION_6XX_LITE) \
+	? 0x8 : 0x1000006)
   #define HFI_BUFFER_EXTRADATA_INPUT(ver)		\
   	(((ver) == HFI_VERSION_4XX) ? 0xc : 0x1000002)
   #define HFI_BUFFER_EXTRADATA_OUTPUT(ver)	\

@@ -561,6 +564,7 @@ enum hfi_version {
   	HFI_VERSION_3XX,
   	HFI_VERSION_4XX,
   	HFI_VERSION_6XX,
+	HFI_VERSION_6XX_LITE,
   };
   struct hfi_buffer_info {

drivers/media/platform/qcom/venus/hfi_venus.c +9 -5

index b5f2ea879950..302776bf8fe6 100644
@@ -497,7 +497,7 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
   	if (count >= max_tries)
   		ret = -ETIMEDOUT;
-	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {
+	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core) || IS_AR50_LITE(hdev->core)) {
   		writel(0x1, cpu_cs_base + CPU_CS_H2XSOFTINTEN_V6);
   		writel(0x0, cpu_cs_base + CPU_CS_X2RPMH_V6);
   	}

@@ -565,6 +565,9 @@ static int venus_halt_axi(struct venus_hfi_device *hdev)
   	u32 mask_val;
   	int ret;
+	if (IS_AR50_LITE(hdev->core))
+		return 0;
+
   	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {
   		writel(0x3, cpu_cs_base + CPU_CS_X2RPMH_V6);

@@ -1134,7 +1137,8 @@ static irqreturn_t venus_isr(struct venus_core *core)
   	wrapper_base = hdev->core->wrapper_base;
   	status = readl(wrapper_base + WRAPPER_INTR_STATUS);
-	if (IS_IRIS2(core) || IS_IRIS2_1(core)) {
+
+	if (IS_IRIS2(core) || IS_IRIS2_1(core) || IS_AR50_LITE(core)) {
   		if (status & WRAPPER_INTR_STATUS_A2H_MASK ||
   		    status & WRAPPER_INTR_STATUS_A2HWD_MASK_V6 ||
   		    status & CPU_CS_SCIACMDARG0_INIT_IDLE_MSG_MASK)

@@ -1146,7 +1150,7 @@ static irqreturn_t venus_isr(struct venus_core *core)
   			hdev->irq_status = status;
   	}
   	writel(1, cpu_cs_base + CPU_CS_A2HSOFTINTCLR);
-	if (!(IS_IRIS2(core) || IS_IRIS2_1(core)))
+	if (!(IS_IRIS2(core) || IS_IRIS2_1(core) || IS_AR50_LITE(core)))
   		writel(status, wrapper_base + WRAPPER_INTR_CLEAR);
   	return IRQ_WAKE_THREAD;

@@ -1531,7 +1535,7 @@ static bool venus_cpu_and_video_core_idle(struct venus_hfi_device *hdev)
   	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
   	u32 ctrl_status, cpu_status;
-	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
+	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core) || IS_AR50_LITE(hdev->core))
   		cpu_status = readl(wrapper_tz_base + WRAPPER_TZ_CPU_STATUS_V6);
   	else
   		cpu_status = readl(wrapper_base + WRAPPER_CPU_STATUS);

@@ -1551,7 +1555,7 @@ static bool venus_cpu_idle_and_pc_ready(struct venus_hfi_device *hdev)
   	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
   	u32 ctrl_status, cpu_status;
-	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
+	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core) || IS_AR50_LITE(hdev->core))
   		cpu_status = readl(wrapper_tz_base + WRAPPER_TZ_CPU_STATUS_V6);
   	else
   		cpu_status = readl(wrapper_base + WRAPPER_CPU_STATUS);

drivers/media/platform/qcom/venus/pm_helpers.c +1 -0

index 409aa9bd0b5d..5d9dfe3fd043 100644
@@ -1168,6 +1168,7 @@ const struct venus_pm_ops *venus_pm_get(enum hfi_version version)
   		return &pm_ops_v3;
   	case HFI_VERSION_4XX:
   	case HFI_VERSION_6XX:
+	case HFI_VERSION_6XX_LITE:
   		return &pm_ops_v4;
   	}

drivers/media/platform/qcom/venus/vdec.c +6 -8

index 99ce5fd41577..87c7901b280e 100644
@@ -550,7 +550,7 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd)
   {
   	struct venus_inst *inst = to_inst(file);
   	struct vb2_queue *dst_vq;
-	struct hfi_frame_data fdata = {0};
+	struct hfi_frame_data fdata;
   	int ret;
   	ret = v4l2_m2m_ioctl_try_decoder_cmd(file, fh, cmd);

@@ -561,18 +561,15 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd)
   	if (cmd->cmd == V4L2_DEC_CMD_STOP) {
   		/*
-		 * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on
+		 * Implement V4L2_DEC_CMD_STOP by enqueue a buffer on
   		 * decoder input to signal EOS.
   		 */
   		if (!(inst->streamon_out && inst->streamon_cap))
   			goto unlock;
-		fdata.buffer_type = HFI_BUFFER_INPUT;
-		fdata.flags |= HFI_BUFFERFLAG_EOS;
-		if (IS_V6(inst->core) && is_fw_rev_or_older(inst->core, 1, 0, 87))
-			fdata.device_addr = 0;
-		else
-			fdata.device_addr = 0xdeadb000;
+		ret = venus_helper_alloc_eos_buf(inst, &fdata);
+		if (ret)
+			goto unlock;
   		ret = hfi_session_process_buf(inst, &fdata);

@@ -1332,6 +1329,7 @@ static void vdec_session_release(struct venus_inst *inst)
   		hfi_session_abort(inst);
   	venus_helper_free_dpb_bufs(inst);
+	venus_helper_free_eos_bufs(inst);
   	venus_pm_load_scale(inst);
   	INIT_LIST_HEAD(&inst->registeredbufs);
   	mutex_unlock(&inst->lock);

drivers/media/platform/qcom/venus/vdec.c static int vdec_open(struct file *file) · line 1685

Quoted 6 lines

drivers/media/platform/qcom/venus/vdec.c +1 -0

@@ -1682,6 +1680,7 @@ static int vdec_open(struct file *file)
   	if (!inst)
   		return -ENOMEM;
+	INIT_LIST_HEAD(&inst->eosbufs);
   	INIT_LIST_HEAD(&inst->dpbbufs);
   	INIT_LIST_HEAD(&inst->registeredbufs);
   	INIT_LIST_HEAD(&inst->internalbufs);

Otherwise LGTM.

---
bod

Quoted 35 lines · Jorge Ramirez

Quoted 20 lines · Bryan O'Donoghue

Quoted 19 lines · Jorge Ramirez-Ortiz

The AR50_LITE is a streamlined variant of the AR50 video core, designed for power and cost-efficient platforms.

It supports hardware-accelerated decoding of H.264, HEVC, and VP9 formats.

Signed-off-by Jorge Ramirez-Ortiz <[email protected]>

---
   drivers/media/platform/qcom/venus/core.c      | 11 ++-
   drivers/media/platform/qcom/venus/core.h      |  5 +-
   drivers/media/platform/qcom/venus/firmware.c  |  8 +-
   drivers/media/platform/qcom/venus/helpers.c   | 81 +++++++++++++++++++
   drivers/media/platform/qcom/venus/helpers.h   |  2 +
   .../media/platform/qcom/venus/hfi_helper.h    | 10 ++-
   drivers/media/platform/qcom/venus/hfi_venus.c | 14 ++--
   .../media/platform/qcom/venus/pm_helpers.c    |  1 +
   drivers/media/platform/qcom/venus/vdec.c      | 15 ++--
   9 files changed, 123 insertions(+), 24 deletions(-)

Quoted 13 lines

drivers/media/platform/qcom/venus/core.c static int venus_enumerate_codecs(struct venus_core *core, u32 type) · line 256

Quoted 10 lines

drivers/media/platform/qcom/venus/core.c +1 -2

index d305d74bb152..736ef53d988d 100644
@@ -254,14 +254,19 @@ static int venus_enumerate_codecs(struct venus_core *core, u32 type)
   static void venus_assign_register_offsets(struct venus_core *core)
   {
-	if (IS_IRIS2(core) || IS_IRIS2_1(core)) {
-		core->vbif_base = core->base + VBIF_BASE;
+	if (IS_IRIS2(core) || IS_IRIS2_1(core) || IS_AR50_LITE(core)) {

Is there a property that IS_IRIS_2_1 || IS_AR50_LITE that we could use instead of expanding this IS_THING list ?

Quoted 6 lines

I know what you mean but I think looking for possible comonalities between cores will just make the partitioning more obscure.

we could perhaps define a different type of macro ie, something like IS_CORE(core, IRIS2, IRIS2_1, AR50_LITE) ? shall I go ahead with this type of macro instead?

It feels like a nit-picky point to be making but, I'm just thinking about adding another say two or three SoCs that need this conditional.

In that case this statement would start to look long and ungainley. It would be nice to come up with some common conditional for it.

Quoted 122 lines

Quoted 113 lines

Quoted 53 lines

drivers/media/platform/qcom/venus/core.c +7 -1

   		core->cpu_base = core->base + CPU_BASE_V6;
   		core->cpu_cs_base = core->base + CPU_CS_BASE_V6;
   		core->cpu_ic_base = core->base + CPU_IC_BASE_V6;
   		core->wrapper_base = core->base + WRAPPER_BASE_V6;
   		core->wrapper_tz_base = core->base + WRAPPER_TZ_BASE_V6;
-		core->aon_base = core->base + AON_BASE_V6;
+		if (IS_AR50_LITE(core)) {
+			core->vbif_base = NULL;
+			core->aon_base = NULL;
+		} else {
+			core->vbif_base = core->base + VBIF_BASE;
+			core->aon_base = core->base + AON_BASE_V6;
+		}
   	} else {
   		core->vbif_base = core->base + VBIF_BASE;
   		core->cpu_base = core->base + CPU_BASE;

drivers/media/platform/qcom/venus/core.h +4 -1

index b412e0c5515a..122441f9600a 100644
@@ -382,6 +382,7 @@ enum venus_inst_modes {
    * @lock:	instance lock
    * @core:	a reference to the core struct
    * @clk_data:	clock data per core ID
+ * @eosbufs:	a lit of EOS buffers
    * @dpbbufs:	a list of decoded picture buffers
    * @internalbufs:	a list of internal bufferes
    * @registeredbufs:	a list of registered capture bufferes

@@ -450,6 +451,7 @@ struct venus_inst {
   	struct mutex lock;
   	struct venus_core *core;
   	struct clock_data clk_data;
+	struct list_head eosbufs;
   	struct list_head dpbbufs;
   	struct list_head internalbufs;
   	struct list_head registeredbufs;

@@ -520,7 +522,8 @@ struct venus_inst {
   #define IS_V1(core)	((core)->res->hfi_version == HFI_VERSION_1XX)
   #define IS_V3(core)	((core)->res->hfi_version == HFI_VERSION_3XX)
   #define IS_V4(core)	((core)->res->hfi_version == HFI_VERSION_4XX)
-#define IS_V6(core)	((core)->res->hfi_version == HFI_VERSION_6XX)
+#define IS_V6(core)     (((core)->res->hfi_version == HFI_VERSION_6XX) || \
+			 ((core)->res->hfi_version == HFI_VERSION_6XX_LITE))
   #define IS_AR50(core)		((core)->res->vpu_version == VPU_VERSION_AR50)
   #define IS_AR50_LITE(core)	((core)->res->vpu_version == VPU_VERSION_AR50_LITE)

drivers/media/platform/qcom/venus/firmware.c +0 -0

index 66a18830e66d..f8dcef0426ac 100644

drivers/media/platform/qcom/venus/firmware.c static void venus_reset_cpu(struct venus_core *core) · line 32

Quoted 4 lines

drivers/media/platform/qcom/venus/firmware.c +1 -1

@@ -30,7 +30,7 @@ static void venus_reset_cpu(struct venus_core *core)
   	u32 fw_size = core->fw.mapped_mem_size;
   	void __iomem *wrapper_base;
-	if (IS_IRIS2_1(core))
+	if (IS_IRIS2_1(core) || IS_AR50_LITE(core))

For example here.

Quoted 53 lines

drivers/media/platform/qcom/venus/firmware.c +3 -3

   		wrapper_base = core->wrapper_tz_base;
   	else
   		wrapper_base = core->wrapper_base;

@@ -42,7 +42,7 @@ static void venus_reset_cpu(struct venus_core *core)
   	writel(fw_size, wrapper_base + WRAPPER_NONPIX_START_ADDR);
   	writel(fw_size, wrapper_base + WRAPPER_NONPIX_END_ADDR);
-	if (IS_IRIS2_1(core)) {
+	if (IS_IRIS2_1(core) || IS_AR50_LITE(core)) {
   		/* Bring XTSS out of reset */
   		writel(0, wrapper_base + WRAPPER_TZ_XTSS_SW_RESET);
   	} else {

@@ -68,7 +68,7 @@ int venus_set_hw_state(struct venus_core *core, bool resume)
   	if (resume) {
   		venus_reset_cpu(core);
   	} else {
-		if (IS_IRIS2_1(core))
+		if (IS_IRIS2_1(core) || IS_AR50_LITE(core))
   			writel(WRAPPER_XTSS_SW_RESET_BIT,
   			       core->wrapper_tz_base + WRAPPER_TZ_XTSS_SW_RESET);
   		else

@@ -181,7 +181,7 @@ static int venus_shutdown_no_tz(struct venus_core *core)
   	void __iomem *wrapper_base = core->wrapper_base;
   	void __iomem *wrapper_tz_base = core->wrapper_tz_base;
-	if (IS_IRIS2_1(core)) {
+	if (IS_IRIS2_1(core) || IS_AR50_LITE(core)) {
   		/* Assert the reset to XTSS */
   		reg = readl(wrapper_tz_base + WRAPPER_TZ_XTSS_SW_RESET);
   		reg |= WRAPPER_XTSS_SW_RESET_BIT;

drivers/media/platform/qcom/venus/helpers.c +18 -0

index 8295542e1a7c..ae89369c6a07 100644
@@ -230,6 +230,80 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst)
   }
   EXPORT_SYMBOL_GPL(venus_helper_alloc_dpb_bufs);
+static void free_eos_buf(struct venus_inst *inst, struct intbuf *buf)
+{
+	list_del_init(&buf->list);
+	dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da,
+		       buf->attrs);
+	kfree(buf);
+}
+
+int venus_helper_free_eos_bufs(struct venus_inst *inst)
+{
+	struct intbuf *buf, *n;
+
+	list_for_each_entry_safe(buf, n, &inst->eosbufs, list) {
+		free_eos_buf(inst, buf);
+	}
+
+	if (list_empty(&inst->eosbufs))
+		INIT_LIST_HEAD(&inst->eosbufs);

Quoted 8 lines

drivers/media/platform/qcom/venus/helpers.c int venus_helper_alloc_dpb_bufs(struct venus_inst *inst) · line 255

Quoted 5 lines

drivers/media/platform/qcom/venus/helpers.c +5 -0

+
+	return 0;
+
+}
+EXPORT_SYMBOL_GPL(venus_helper_free_eos_bufs);

These EOS buf things look like they are not specific to ar50, could you add these in a preceding patch to this ?

Quoted 9 lines

um, previous cores seemed to tolerate passing empty pointers to the VPU firmware without issue. Unfortunately, with the AR50_LITE, this results in segmentation faults in the _firmware_. The purpose of this function is to add support for AR50_LITE while preserving the existing behaviour for other cores...introducing a dedicated function felt cleaner than further complicating the existing conditional.
Given that the previous conditional was just an if/else I don’t really see a lot of benefit in refactoring the earlier conditionals _before_ adding the AR50_LITE logic. Would you be OK if we keep it as proposed?

ok

Quoted 57 lines

Quoted 48 lines

Quoted 48 lines

drivers/media/platform/qcom/venus/helpers.c +46 -0

+
+int venus_helper_alloc_eos_buf(struct venus_inst *inst,
+			       struct hfi_frame_data *data)
+{
+	struct venus_core *core = inst->core;
+	struct device *dev = core->dev;
+	struct intbuf *buf;
+	int ret = 0;
+
+	memset(data, 0, sizeof(*data));
+
+	data->buffer_type = HFI_BUFFER_INPUT;
+	data->flags = HFI_BUFFERFLAG_EOS;
+
+	if (IS_AR50_LITE(inst->core)) {
+		/* We must send valid sizes and addresses */
+		buf = kzalloc(sizeof(*buf), GFP_KERNEL);
+		if (!buf) {
+			ret = -ENOMEM;
+			goto fail;
+		}
+
+		buf->type = HFI_BUFFER_INPUT;
+		buf->size = SZ_4K;
+		buf->attrs = DMA_ATTR_NO_KERNEL_MAPPING;
+		buf->va = dma_alloc_attrs(dev, buf->size, &buf->da, GFP_KERNEL,
+					  buf->attrs);
+		if (!buf->va) {
+			ret = -ENOMEM;
+			goto fail;
+		}
+
+		list_add_tail(&buf->list, &inst->eosbufs);
+
+		data->alloc_len = buf->size;
+		data->device_addr = buf->da;
+
+	} else if (IS_V6(inst->core) &&
+		   is_fw_rev_or_older(inst->core, 1, 0, 87)) {
+		data->device_addr = 0;
+	} else {
+		data->device_addr = 0xdeadb000;
+	}
+
+	return 0;
+fail:

Quoted 8 lines

drivers/media/platform/qcom/venus/helpers.c int venus_helper_alloc_dpb_bufs(struct venus_inst *inst) · line 305

Quoted 4 lines

drivers/media/platform/qcom/venus/helpers.c +4 -0

+	kfree(buf);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(venus_helper_alloc_eos_buf);

I think the series would be nicer if the EOS buf decomposition stuff went into a patch that preceded this one, and then you add the IS_AR50_LITE() in the next patch progressively.

Quoted 177 lines

see above

Quoted 175 lines

Quoted 164 lines

drivers/media/platform/qcom/venus/helpers.c +8 -0

+
   static int intbufs_set_buffer(struct venus_inst *inst, u32 type)
   {
   	struct venus_core *core = inst->core;

@@ -630,6 +704,13 @@ static int platform_get_bufreq(struct venus_inst *inst, u32 buftype,
   	if (!hfi_plat || !hfi_plat->bufreq)
   		return -EINVAL;
+	/* Firmware buffer requirements for internal buffers only */
+	if (IS_AR50_LITE(inst->core))
+		if ((buftype != HFI_BUFFER_INPUT) &&
+		    (buftype != HFI_BUFFER_OUTPUT) &&
+		    (buftype != HFI_BUFFER_OUTPUT2))
+			return -EINVAL;
+
   	params.version = version;
   	params.num_vpp_pipes = inst->core->res->num_vpp_pipes;

drivers/media/platform/qcom/venus/helpers.h +2 -0

index 358e4f39c9c0..bf55fe3b8747 100644
@@ -58,6 +58,8 @@ int venus_helper_get_out_fmts(struct venus_inst *inst, u32 fmt, u32 *out_fmt,
   bool venus_helper_check_format(struct venus_inst *inst, u32 v4l2_pixfmt);
   int venus_helper_alloc_dpb_bufs(struct venus_inst *inst);
   int venus_helper_free_dpb_bufs(struct venus_inst *inst);
+int venus_helper_alloc_eos_buf(struct venus_inst *inst, struct hfi_frame_data *data);
+int venus_helper_free_eos_bufs(struct venus_inst *inst);
   int venus_helper_intbufs_alloc(struct venus_inst *inst);
   int venus_helper_intbufs_free(struct venus_inst *inst);
   int venus_helper_intbufs_realloc(struct venus_inst *inst);

drivers/media/platform/qcom/venus/hfi_helper.h +7 -3

index f44059f19505..128ddf8e3cd5 100644
@@ -397,13 +397,16 @@
   #define HFI_BUFFER_INTERNAL_PERSIST_1		0x5
   #define HFI_BUFFER_INTERNAL_SCRATCH(ver)	\
   	(((ver) == HFI_VERSION_4XX ||		\
-	(ver) == HFI_VERSION_6XX) ? 0x6 : 0x1000001)
+	(ver) == HFI_VERSION_6XX || (ver) == HFI_VERSION_6XX_LITE) \
+	? 0x6 : 0x1000001)
   #define HFI_BUFFER_INTERNAL_SCRATCH_1(ver)	\
   	(((ver) == HFI_VERSION_4XX ||		\
-	(ver) == HFI_VERSION_6XX) ? 0x7 : 0x1000005)
+	(ver) == HFI_VERSION_6XX || (ver) == HFI_VERSION_6XX_LITE) \
+	? 0x7 : 0x1000005)
   #define HFI_BUFFER_INTERNAL_SCRATCH_2(ver)	\
   	(((ver) == HFI_VERSION_4XX ||		\
-	(ver) == HFI_VERSION_6XX) ? 0x8 : 0x1000006)
+	(ver) == HFI_VERSION_6XX || (ver) == HFI_VERSION_6XX_LITE) \
+	? 0x8 : 0x1000006)
   #define HFI_BUFFER_EXTRADATA_INPUT(ver)		\
   	(((ver) == HFI_VERSION_4XX) ? 0xc : 0x1000002)
   #define HFI_BUFFER_EXTRADATA_OUTPUT(ver)	\

@@ -561,6 +564,7 @@ enum hfi_version {
   	HFI_VERSION_3XX,
   	HFI_VERSION_4XX,
   	HFI_VERSION_6XX,
+	HFI_VERSION_6XX_LITE,
   };
   struct hfi_buffer_info {

drivers/media/platform/qcom/venus/hfi_venus.c +9 -5

index b5f2ea879950..302776bf8fe6 100644
@@ -497,7 +497,7 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
   	if (count >= max_tries)
   		ret = -ETIMEDOUT;
-	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {
+	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core) || IS_AR50_LITE(hdev->core)) {
   		writel(0x1, cpu_cs_base + CPU_CS_H2XSOFTINTEN_V6);
   		writel(0x0, cpu_cs_base + CPU_CS_X2RPMH_V6);
   	}

@@ -565,6 +565,9 @@ static int venus_halt_axi(struct venus_hfi_device *hdev)
   	u32 mask_val;
   	int ret;
+	if (IS_AR50_LITE(hdev->core))
+		return 0;
+
   	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {
   		writel(0x3, cpu_cs_base + CPU_CS_X2RPMH_V6);

@@ -1134,7 +1137,8 @@ static irqreturn_t venus_isr(struct venus_core *core)
   	wrapper_base = hdev->core->wrapper_base;
   	status = readl(wrapper_base + WRAPPER_INTR_STATUS);
-	if (IS_IRIS2(core) || IS_IRIS2_1(core)) {
+
+	if (IS_IRIS2(core) || IS_IRIS2_1(core) || IS_AR50_LITE(core)) {
   		if (status & WRAPPER_INTR_STATUS_A2H_MASK ||
   		    status & WRAPPER_INTR_STATUS_A2HWD_MASK_V6 ||
   		    status & CPU_CS_SCIACMDARG0_INIT_IDLE_MSG_MASK)

@@ -1146,7 +1150,7 @@ static irqreturn_t venus_isr(struct venus_core *core)
   			hdev->irq_status = status;
   	}
   	writel(1, cpu_cs_base + CPU_CS_A2HSOFTINTCLR);
-	if (!(IS_IRIS2(core) || IS_IRIS2_1(core)))
+	if (!(IS_IRIS2(core) || IS_IRIS2_1(core) || IS_AR50_LITE(core)))
   		writel(status, wrapper_base + WRAPPER_INTR_CLEAR);
   	return IRQ_WAKE_THREAD;

@@ -1531,7 +1535,7 @@ static bool venus_cpu_and_video_core_idle(struct venus_hfi_device *hdev)
   	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
   	u32 ctrl_status, cpu_status;
-	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
+	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core) || IS_AR50_LITE(hdev->core))
   		cpu_status = readl(wrapper_tz_base + WRAPPER_TZ_CPU_STATUS_V6);
   	else
   		cpu_status = readl(wrapper_base + WRAPPER_CPU_STATUS);

@@ -1551,7 +1555,7 @@ static bool venus_cpu_idle_and_pc_ready(struct venus_hfi_device *hdev)
   	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
   	u32 ctrl_status, cpu_status;
-	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
+	if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core) || IS_AR50_LITE(hdev->core))
   		cpu_status = readl(wrapper_tz_base + WRAPPER_TZ_CPU_STATUS_V6);
   	else
   		cpu_status = readl(wrapper_base + WRAPPER_CPU_STATUS);

drivers/media/platform/qcom/venus/pm_helpers.c +1 -0

index 409aa9bd0b5d..5d9dfe3fd043 100644
@@ -1168,6 +1168,7 @@ const struct venus_pm_ops *venus_pm_get(enum hfi_version version)
   		return &pm_ops_v3;
   	case HFI_VERSION_4XX:
   	case HFI_VERSION_6XX:
+	case HFI_VERSION_6XX_LITE:
   		return &pm_ops_v4;
   	}

drivers/media/platform/qcom/venus/vdec.c +6 -8

index 99ce5fd41577..87c7901b280e 100644
@@ -550,7 +550,7 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd)
   {
   	struct venus_inst *inst = to_inst(file);
   	struct vb2_queue *dst_vq;
-	struct hfi_frame_data fdata = {0};
+	struct hfi_frame_data fdata;
   	int ret;
   	ret = v4l2_m2m_ioctl_try_decoder_cmd(file, fh, cmd);

@@ -561,18 +561,15 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd)
   	if (cmd->cmd == V4L2_DEC_CMD_STOP) {
   		/*
-		 * Implement V4L2_DEC_CMD_STOP by enqueue an empty buffer on
+		 * Implement V4L2_DEC_CMD_STOP by enqueue a buffer on
   		 * decoder input to signal EOS.
   		 */
   		if (!(inst->streamon_out && inst->streamon_cap))
   			goto unlock;
-		fdata.buffer_type = HFI_BUFFER_INPUT;
-		fdata.flags |= HFI_BUFFERFLAG_EOS;
-		if (IS_V6(inst->core) && is_fw_rev_or_older(inst->core, 1, 0, 87))
-			fdata.device_addr = 0;
-		else
-			fdata.device_addr = 0xdeadb000;
+		ret = venus_helper_alloc_eos_buf(inst, &fdata);
+		if (ret)
+			goto unlock;
   		ret = hfi_session_process_buf(inst, &fdata);

@@ -1332,6 +1329,7 @@ static void vdec_session_release(struct venus_inst *inst)
   		hfi_session_abort(inst);
   	venus_helper_free_dpb_bufs(inst);
+	venus_helper_free_eos_bufs(inst);
   	venus_pm_load_scale(inst);
   	INIT_LIST_HEAD(&inst->registeredbufs);
   	mutex_unlock(&inst->lock);

drivers/media/platform/qcom/venus/vdec.c static int vdec_open(struct file *file) · line 1685

Quoted 6 lines

drivers/media/platform/qcom/venus/vdec.c +1 -0

@@ -1682,6 +1680,7 @@ static int vdec_open(struct file *file)
   	if (!inst)
   		return -ENOMEM;
+	INIT_LIST_HEAD(&inst->eosbufs);
   	INIT_LIST_HEAD(&inst->dpbbufs);
   	INIT_LIST_HEAD(&inst->registeredbufs);
   	INIT_LIST_HEAD(&inst->internalbufs);

Otherwise LGTM.

---
bod

Reviewed-by Bryan O'Donoghue <[email protected]>

Add a qcm2290 compatible binding to the venus core.

Signed-off-by Jorge Ramirez-Ortiz <[email protected]>

---
  drivers/media/platform/qcom/venus/core.c | 39 ++++++++++++++++++++++++
  1 file changed, 39 insertions(+)

drivers/media/platform/qcom/venus/core.c +26 -0

index 736ef53d988d..f1f211ca1ce2 100644
@@ -1062,6 +1062,44 @@ static const struct venus_resources sc7280_res = {
  	.enc_nodename = "video-encoder",
  };
 
+static const struct freq_tbl qcm2290_freq_table[] = {
+	{ 352800, 240000000 },	/* 1920x1088 @ 30 + 1280x720 @ 30 */
+	{ 244800, 133000000 },	/* 1920x1088 @ 30 */
+};
+
+static const struct bw_tbl qcm2290_bw_table_dec[] = {
+	{ 244800, 2128000, 0, 2128000, 0}, /* 1920x1088 @ 30 */
+};
+
+static const struct venus_resources qcm2290_res = {
+	.freq_tbl = qcm2290_freq_table,
+	.freq_tbl_size = ARRAY_SIZE(qcm2290_freq_table),
+	.bw_tbl_dec = qcm2290_bw_table_dec,
+	.bw_tbl_dec_size = ARRAY_SIZE(qcm2290_bw_table_dec),
+	.clks = { "core", "iface", "bus", "throttle" },
+	.clks_num = 4,
+	.vcodec0_clks = { "vcodec0_core", "vcodec0_bus" },
+	.vcodec_clks_num = 2,
+	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
+	.vcodec_pmdomains_num = 2,
+	.opp_pmdomain = (const char *[]) { "cx" },
+	.vcodec_num = 1,
+	.hfi_version = HFI_VERSION_6XX_LITE,
+	.vpu_version = VPU_VERSION_AR50_LITE,
+	.max_load = 352800,
+	.num_vpp_pipes = 1,