diff mbox series

[v2,4/4] arm64: dts: rockchip: Add rkvdec2 Video Decoder on rk3588(s)

Message ID 20240619150029.59730-5-detlev.casanova@collabora.com (mailing list archive)
State New
Headers show
Series media: rockchip: Add rkvdec2 driver | expand

Commit Message

Detlev Casanova June 19, 2024, 2:57 p.m. UTC
Add the rkvdec2 Video Decoder to the RK3588s devicetree.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 50 +++++++++++++++++++++++
 1 file changed, 50 insertions(+)

Comments

Jonas Karlman June 19, 2024, 3:28 p.m. UTC | #1
Hi Detlev,

On 2024-06-19 16:57, Detlev Casanova wrote:
> Add the rkvdec2 Video Decoder to the RK3588s devicetree.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 50 +++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> index 6ac5ac8b48ab..7690632f57f1 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> @@ -2596,6 +2596,16 @@ system_sram2: sram@ff001000 {
>  		ranges = <0x0 0x0 0xff001000 0xef000>;
>  		#address-cells = <1>;
>  		#size-cells = <1>;
> +
> +		vdec0_sram: rkvdec-sram@0 {
> +			reg = <0x0 0x78000>;
> +			pool;
> +		};
> +
> +		vdec1_sram: rkvdec-sram@1 {
> +			reg = <0x78000 0x77000>;
> +			pool;
> +		};
>  	};
>  
>  	pinctrl: pinctrl {
> @@ -2665,6 +2675,46 @@ gpio4: gpio@fec50000 {
>  			#interrupt-cells = <2>;
>  		};
>  	};
> +
> +	vdec0: video-decoder@fdc38100 {
> +		compatible = "rockchip,rk3588-vdec";
> +		reg = <0x0 0xfdc38100 0x0 0x500>;
> +		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
> +		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>, <&cru CLK_RKVDEC0_CA>,
> +			 <&cru CLK_RKVDEC0_CORE>, <&cru CLK_RKVDEC0_HEVC_CA>;
> +		clock-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
> +		assigned-clocks = <&cru ACLK_RKVDEC0>, <&cru CLK_RKVDEC0_CORE>,
> +				  <&cru CLK_RKVDEC0_CA>, <&cru CLK_RKVDEC0_HEVC_CA>;
> +		assigned-clock-rates = <800000000>, <600000000>,
> +				       <600000000>, <1000000000>;
> +		resets = <&cru SRST_A_RKVDEC0>, <&cru SRST_H_RKVDEC0>, <&cru SRST_RKVDEC0_CA>,
> +			 <&cru SRST_RKVDEC0_CORE>, <&cru SRST_RKVDEC0_HEVC_CA>;
> +		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
> +			      "rst_core", "rst_hevc_cabac";
> +		power-domains = <&power RK3588_PD_RKVDEC0>;
> +		sram = <&vdec0_sram>;
> +		status = "okay";
> +	};
> +
> +	vdec1: video-decoder@fdc40100 {
> +		compatible = "rockchip,rk3588-vdec";
> +		reg = <0x0 0xfdc40100 0x0 0x500>;
> +		interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH 0>;
> +		clocks = <&cru ACLK_RKVDEC1>, <&cru HCLK_RKVDEC1>, <&cru CLK_RKVDEC1_CA>,
> +			 <&cru CLK_RKVDEC1_CORE>, <&cru CLK_RKVDEC1_HEVC_CA>;
> +		clock-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
> +		assigned-clocks = <&cru ACLK_RKVDEC1>, <&cru CLK_RKVDEC1_CORE>,
> +				  <&cru CLK_RKVDEC1_CA>, <&cru CLK_RKVDEC1_HEVC_CA>;
> +		assigned-clock-rates = <800000000>, <600000000>,
> +				       <600000000>, <1000000000>;
> +		resets = <&cru SRST_A_RKVDEC1>, <&cru SRST_H_RKVDEC1>, <&cru SRST_RKVDEC1_CA>,
> +			 <&cru SRST_RKVDEC1_CORE>, <&cru SRST_RKVDEC1_HEVC_CA>;
> +		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
> +			      "rst_core", "rst_hevc_cabac";
> +		power-domains = <&power RK3588_PD_RKVDEC1>;
> +		sram = <&vdec1_sram>;
> +		status = "okay";
> +	};

This is still missing the iommus, please add the iommus, they should be
supported/same as the one used for e.g. VOP2:

  compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";

The VOP2 MMUs does have one extra mmu_cfg_mode flag in AUTO_GATING,
compared to the VDPU381 MMUs, however only the AV1D MMU should be
special on RK3588.

Please add the iommus :-)

Regards,
Jonas

>  };
>  
>  #include "rk3588s-pinctrl.dtsi"
Heiko Stuebner June 19, 2024, 3:34 p.m. UTC | #2
Am Mittwoch, 19. Juni 2024, 16:57:21 CEST schrieb Detlev Casanova:
> Add the rkvdec2 Video Decoder to the RK3588s devicetree.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 50 +++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> index 6ac5ac8b48ab..7690632f57f1 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> @@ -2596,6 +2596,16 @@ system_sram2: sram@ff001000 {
>  		ranges = <0x0 0x0 0xff001000 0xef000>;
>  		#address-cells = <1>;
>  		#size-cells = <1>;
> +
> +		vdec0_sram: rkvdec-sram@0 {
> +			reg = <0x0 0x78000>;
> +			pool;
> +		};
> +
> +		vdec1_sram: rkvdec-sram@1 {
> +			reg = <0x78000 0x77000>;
> +			pool;
> +		};
>  	};
>  
>  	pinctrl: pinctrl {
> @@ -2665,6 +2675,46 @@ gpio4: gpio@fec50000 {
>  			#interrupt-cells = <2>;
>  		};
>  	};
> +
> +	vdec0: video-decoder@fdc38100 {
> +		compatible = "rockchip,rk3588-vdec";
> +		reg = <0x0 0xfdc38100 0x0 0x500>;
> +		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
> +		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>, <&cru CLK_RKVDEC0_CA>,
> +			 <&cru CLK_RKVDEC0_CORE>, <&cru CLK_RKVDEC0_HEVC_CA>;
> +		clock-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
> +		assigned-clocks = <&cru ACLK_RKVDEC0>, <&cru CLK_RKVDEC0_CORE>,
> +				  <&cru CLK_RKVDEC0_CA>, <&cru CLK_RKVDEC0_HEVC_CA>;
> +		assigned-clock-rates = <800000000>, <600000000>,
> +				       <600000000>, <1000000000>;
> +		resets = <&cru SRST_A_RKVDEC0>, <&cru SRST_H_RKVDEC0>, <&cru SRST_RKVDEC0_CA>,
> +			 <&cru SRST_RKVDEC0_CORE>, <&cru SRST_RKVDEC0_HEVC_CA>;
> +		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
> +			      "rst_core", "rst_hevc_cabac";
> +		power-domains = <&power RK3588_PD_RKVDEC0>;
> +		sram = <&vdec0_sram>;
> +		status = "okay";

okay is the default status, so is not needed when the node is always-on

> +	};
> +
> +	vdec1: video-decoder@fdc40100 {
> +		compatible = "rockchip,rk3588-vdec";
> +		reg = <0x0 0xfdc40100 0x0 0x500>;
> +		interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH 0>;
> +		clocks = <&cru ACLK_RKVDEC1>, <&cru HCLK_RKVDEC1>, <&cru CLK_RKVDEC1_CA>,
> +			 <&cru CLK_RKVDEC1_CORE>, <&cru CLK_RKVDEC1_HEVC_CA>;
> +		clock-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
> +		assigned-clocks = <&cru ACLK_RKVDEC1>, <&cru CLK_RKVDEC1_CORE>,
> +				  <&cru CLK_RKVDEC1_CA>, <&cru CLK_RKVDEC1_HEVC_CA>;
> +		assigned-clock-rates = <800000000>, <600000000>,
> +				       <600000000>, <1000000000>;
> +		resets = <&cru SRST_A_RKVDEC1>, <&cru SRST_H_RKVDEC1>, <&cru SRST_RKVDEC1_CA>,
> +			 <&cru SRST_RKVDEC1_CORE>, <&cru SRST_RKVDEC1_HEVC_CA>;
> +		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
> +			      "rst_core", "rst_hevc_cabac";
> +		power-domains = <&power RK3588_PD_RKVDEC1>;
> +		sram = <&vdec1_sram>;
> +		status = "okay";

same

> +	};
>  };
>  
>  #include "rk3588s-pinctrl.dtsi"
>
Alex Bee June 19, 2024, 5:19 p.m. UTC | #3
Am 19.06.24 um 17:28 schrieb Jonas Karlman:
> Hi Detlev,
>
> On 2024-06-19 16:57, Detlev Casanova wrote:
>> Add the rkvdec2 Video Decoder to the RK3588s devicetree.
>>
>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>> ---
>>   arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 50 +++++++++++++++++++++++
>>   1 file changed, 50 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> index 6ac5ac8b48ab..7690632f57f1 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> @@ -2596,6 +2596,16 @@ system_sram2: sram@ff001000 {
>>   		ranges = <0x0 0x0 0xff001000 0xef000>;
>>   		#address-cells = <1>;
>>   		#size-cells = <1>;
>> +
>> +		vdec0_sram: rkvdec-sram@0 {
>> +			reg = <0x0 0x78000>;
>> +			pool;
>> +		};
>> +
>> +		vdec1_sram: rkvdec-sram@1 {
>> +			reg = <0x78000 0x77000>;
>> +			pool;
>> +		};
>>   	};
>>   
>>   	pinctrl: pinctrl {
>> @@ -2665,6 +2675,46 @@ gpio4: gpio@fec50000 {
>>   			#interrupt-cells = <2>;
>>   		};
>>   	};
>> +
>> +	vdec0: video-decoder@fdc38100 {
>> +		compatible = "rockchip,rk3588-vdec";
>> +		reg = <0x0 0xfdc38100 0x0 0x500>;
>> +		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
>> +		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>, <&cru CLK_RKVDEC0_CA>,
>> +			 <&cru CLK_RKVDEC0_CORE>, <&cru CLK_RKVDEC0_HEVC_CA>;
>> +		clock-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
>> +		assigned-clocks = <&cru ACLK_RKVDEC0>, <&cru CLK_RKVDEC0_CORE>,
>> +				  <&cru CLK_RKVDEC0_CA>, <&cru CLK_RKVDEC0_HEVC_CA>;
>> +		assigned-clock-rates = <800000000>, <600000000>,
>> +				       <600000000>, <1000000000>;
>> +		resets = <&cru SRST_A_RKVDEC0>, <&cru SRST_H_RKVDEC0>, <&cru SRST_RKVDEC0_CA>,
>> +			 <&cru SRST_RKVDEC0_CORE>, <&cru SRST_RKVDEC0_HEVC_CA>;
>> +		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
>> +			      "rst_core", "rst_hevc_cabac";
>> +		power-domains = <&power RK3588_PD_RKVDEC0>;
>> +		sram = <&vdec0_sram>;
>> +		status = "okay";
>> +	};
>> +
>> +	vdec1: video-decoder@fdc40100 {
>> +		compatible = "rockchip,rk3588-vdec";
>> +		reg = <0x0 0xfdc40100 0x0 0x500>;
>> +		interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH 0>;
>> +		clocks = <&cru ACLK_RKVDEC1>, <&cru HCLK_RKVDEC1>, <&cru CLK_RKVDEC1_CA>,
>> +			 <&cru CLK_RKVDEC1_CORE>, <&cru CLK_RKVDEC1_HEVC_CA>;
>> +		clock-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
>> +		assigned-clocks = <&cru ACLK_RKVDEC1>, <&cru CLK_RKVDEC1_CORE>,
>> +				  <&cru CLK_RKVDEC1_CA>, <&cru CLK_RKVDEC1_HEVC_CA>;
>> +		assigned-clock-rates = <800000000>, <600000000>,
>> +				       <600000000>, <1000000000>;
>> +		resets = <&cru SRST_A_RKVDEC1>, <&cru SRST_H_RKVDEC1>, <&cru SRST_RKVDEC1_CA>,
>> +			 <&cru SRST_RKVDEC1_CORE>, <&cru SRST_RKVDEC1_HEVC_CA>;
>> +		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
>> +			      "rst_core", "rst_hevc_cabac";
>> +		power-domains = <&power RK3588_PD_RKVDEC1>;
>> +		sram = <&vdec1_sram>;
>> +		status = "okay";
>> +	};
> This is still missing the iommus, please add the iommus, they should be
> supported/same as the one used for e.g. VOP2:
>
>    compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
>
> The VOP2 MMUs does have one extra mmu_cfg_mode flag in AUTO_GATING,
> compared to the VDPU381 MMUs, however only the AV1D MMU should be
> special on RK3588.
>
> Please add the iommus :-)
When looking add the vendor DT/iommu driver I'm seeing serval quirks
applied for vdec's iommus. Since it's rightly frowned upon adding such
boolean-quirk-properties to upstream devicetrees, we'd at least need
additional (fallback-) compatibles, even if it works with the iommu driver
as is (what I doubt, but haven't tested). We need to be able to apply those
quirks later without changing the devicetree (as usual) and I'm sure RK
devs haven't added these quirks for the personal amusement. If Detlev says
iommu is out of scope for this series (which is valid), I'd say it's fine
to leave them out for now (as no binding exists) and the HW works
(obviously) fine without them.

> Regards,
> Jonas
>
>>   };
>>   
>>   #include "rk3588s-pinctrl.dtsi"
Jonas Karlman June 19, 2024, 6:06 p.m. UTC | #4
Hi Alex,

On 2024-06-19 19:19, Alex Bee wrote:
> 
> Am 19.06.24 um 17:28 schrieb Jonas Karlman:
>> Hi Detlev,
>>
>> On 2024-06-19 16:57, Detlev Casanova wrote:
>>> Add the rkvdec2 Video Decoder to the RK3588s devicetree.
>>>
>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>>> ---
>>>   arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 50 +++++++++++++++++++++++
>>>   1 file changed, 50 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>>> index 6ac5ac8b48ab..7690632f57f1 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>>> @@ -2596,6 +2596,16 @@ system_sram2: sram@ff001000 {
>>>   		ranges = <0x0 0x0 0xff001000 0xef000>;
>>>   		#address-cells = <1>;
>>>   		#size-cells = <1>;
>>> +
>>> +		vdec0_sram: rkvdec-sram@0 {
>>> +			reg = <0x0 0x78000>;
>>> +			pool;
>>> +		};
>>> +
>>> +		vdec1_sram: rkvdec-sram@1 {
>>> +			reg = <0x78000 0x77000>;
>>> +			pool;
>>> +		};
>>>   	};
>>>   
>>>   	pinctrl: pinctrl {
>>> @@ -2665,6 +2675,46 @@ gpio4: gpio@fec50000 {
>>>   			#interrupt-cells = <2>;
>>>   		};
>>>   	};
>>> +
>>> +	vdec0: video-decoder@fdc38100 {
>>> +		compatible = "rockchip,rk3588-vdec";
>>> +		reg = <0x0 0xfdc38100 0x0 0x500>;
>>> +		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
>>> +		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>, <&cru CLK_RKVDEC0_CA>,
>>> +			 <&cru CLK_RKVDEC0_CORE>, <&cru CLK_RKVDEC0_HEVC_CA>;
>>> +		clock-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
>>> +		assigned-clocks = <&cru ACLK_RKVDEC0>, <&cru CLK_RKVDEC0_CORE>,
>>> +				  <&cru CLK_RKVDEC0_CA>, <&cru CLK_RKVDEC0_HEVC_CA>;
>>> +		assigned-clock-rates = <800000000>, <600000000>,
>>> +				       <600000000>, <1000000000>;
>>> +		resets = <&cru SRST_A_RKVDEC0>, <&cru SRST_H_RKVDEC0>, <&cru SRST_RKVDEC0_CA>,
>>> +			 <&cru SRST_RKVDEC0_CORE>, <&cru SRST_RKVDEC0_HEVC_CA>;
>>> +		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
>>> +			      "rst_core", "rst_hevc_cabac";
>>> +		power-domains = <&power RK3588_PD_RKVDEC0>;
>>> +		sram = <&vdec0_sram>;
>>> +		status = "okay";
>>> +	};
>>> +
>>> +	vdec1: video-decoder@fdc40100 {
>>> +		compatible = "rockchip,rk3588-vdec";
>>> +		reg = <0x0 0xfdc40100 0x0 0x500>;
>>> +		interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH 0>;
>>> +		clocks = <&cru ACLK_RKVDEC1>, <&cru HCLK_RKVDEC1>, <&cru CLK_RKVDEC1_CA>,
>>> +			 <&cru CLK_RKVDEC1_CORE>, <&cru CLK_RKVDEC1_HEVC_CA>;
>>> +		clock-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
>>> +		assigned-clocks = <&cru ACLK_RKVDEC1>, <&cru CLK_RKVDEC1_CORE>,
>>> +				  <&cru CLK_RKVDEC1_CA>, <&cru CLK_RKVDEC1_HEVC_CA>;
>>> +		assigned-clock-rates = <800000000>, <600000000>,
>>> +				       <600000000>, <1000000000>;
>>> +		resets = <&cru SRST_A_RKVDEC1>, <&cru SRST_H_RKVDEC1>, <&cru SRST_RKVDEC1_CA>,
>>> +			 <&cru SRST_RKVDEC1_CORE>, <&cru SRST_RKVDEC1_HEVC_CA>;
>>> +		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
>>> +			      "rst_core", "rst_hevc_cabac";
>>> +		power-domains = <&power RK3588_PD_RKVDEC1>;
>>> +		sram = <&vdec1_sram>;
>>> +		status = "okay";
>>> +	};
>> This is still missing the iommus, please add the iommus, they should be
>> supported/same as the one used for e.g. VOP2:
>>
>>    compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
>>
>> The VOP2 MMUs does have one extra mmu_cfg_mode flag in AUTO_GATING,
>> compared to the VDPU381 MMUs, however only the AV1D MMU should be
>> special on RK3588.
>>
>> Please add the iommus :-)
> When looking add the vendor DT/iommu driver I'm seeing serval quirks
> applied for vdec's iommus. Since it's rightly frowned upon adding such
> boolean-quirk-properties to upstream devicetrees, we'd at least need
> additional (fallback-) compatibles, even if it works with the iommu driver
> as is (what I doubt, but haven't tested). We need to be able to apply those
> quirks later without changing the devicetree (as usual) and I'm sure RK
> devs haven't added these quirks for the personal amusement.

Based on what I investigated the hw should work similar, and the quirks
mostly seem related to optimizations and sw quirks, like do not zap each
line, keep it alive even when pm runtime say it is not in use and other
quirks that seem to be more of sw nature on how to best utilize the hw.

> If Detlev says
> iommu is out of scope for this series (which is valid), I'd say it's fine
> to leave them out for now (as no binding exists) and the HW works
> (obviously) fine without them.

Sure, use of MMU can be added later.

Regards,
Jonas

> 
>> Regards,
>> Jonas
>>
>>>   };
>>>   
>>>   #include "rk3588s-pinctrl.dtsi"
Krzysztof Kozlowski June 20, 2024, 10:24 a.m. UTC | #5
On 19/06/2024 16:57, Detlev Casanova wrote:
> Add the rkvdec2 Video Decoder to the RK3588s devicetree.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 50 +++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> index 6ac5ac8b48ab..7690632f57f1 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> @@ -2596,6 +2596,16 @@ system_sram2: sram@ff001000 {
>  		ranges = <0x0 0x0 0xff001000 0xef000>;
>  		#address-cells = <1>;
>  		#size-cells = <1>;
> +
> +		vdec0_sram: rkvdec-sram@0 {
> +			reg = <0x0 0x78000>;
> +			pool;
> +		};
> +
> +		vdec1_sram: rkvdec-sram@1 {
> +			reg = <0x78000 0x77000>;
> +			pool;
> +		};
>  	};
>  
>  	pinctrl: pinctrl {
> @@ -2665,6 +2675,46 @@ gpio4: gpio@fec50000 {
>  			#interrupt-cells = <2>;
>  		};
>  	};
> +
> +	vdec0: video-decoder@fdc38100 {
> +		compatible = "rockchip,rk3588-vdec";
> +		reg = <0x0 0xfdc38100 0x0 0x500>;
> +		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
> +		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>, <&cru CLK_RKVDEC0_CA>,
> +			 <&cru CLK_RKVDEC0_CORE>, <&cru CLK_RKVDEC0_HEVC_CA>;
> +		clock-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
> +		assigned-clocks = <&cru ACLK_RKVDEC0>, <&cru CLK_RKVDEC0_CORE>,
> +				  <&cru CLK_RKVDEC0_CA>, <&cru CLK_RKVDEC0_HEVC_CA>;
> +		assigned-clock-rates = <800000000>, <600000000>,
> +				       <600000000>, <1000000000>;
> +		resets = <&cru SRST_A_RKVDEC0>, <&cru SRST_H_RKVDEC0>, <&cru SRST_RKVDEC0_CA>,
> +			 <&cru SRST_RKVDEC0_CORE>, <&cru SRST_RKVDEC0_HEVC_CA>;
> +		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
> +			      "rst_core", "rst_hevc_cabac";
> +		power-domains = <&power RK3588_PD_RKVDEC0>;
> +		sram = <&vdec0_sram>;
> +		status = "okay";

Should not be needed. Where did you disable it?

> +	};
> +
> +	vdec1: video-decoder@fdc40100 {
> +		compatible = "rockchip,rk3588-vdec";
> +		reg = <0x0 0xfdc40100 0x0 0x500>;
> +		interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH 0>;
> +		clocks = <&cru ACLK_RKVDEC1>, <&cru HCLK_RKVDEC1>, <&cru CLK_RKVDEC1_CA>,
> +			 <&cru CLK_RKVDEC1_CORE>, <&cru CLK_RKVDEC1_HEVC_CA>;
> +		clock-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
> +		assigned-clocks = <&cru ACLK_RKVDEC1>, <&cru CLK_RKVDEC1_CORE>,
> +				  <&cru CLK_RKVDEC1_CA>, <&cru CLK_RKVDEC1_HEVC_CA>;
> +		assigned-clock-rates = <800000000>, <600000000>,
> +				       <600000000>, <1000000000>;
> +		resets = <&cru SRST_A_RKVDEC1>, <&cru SRST_H_RKVDEC1>, <&cru SRST_RKVDEC1_CA>,
> +			 <&cru SRST_RKVDEC1_CORE>, <&cru SRST_RKVDEC1_HEVC_CA>;
> +		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
> +			      "rst_core", "rst_hevc_cabac";
> +		power-domains = <&power RK3588_PD_RKVDEC1>;
> +		sram = <&vdec1_sram>;
> +		status = "okay";

Should not be needed. Where did you disable it?



Best regards,
Krzysztof
Detlev Casanova June 20, 2024, 1:31 p.m. UTC | #6
Hi Jonas, Alex,

On Wednesday, June 19, 2024 2:06:40 P.M. EDT Jonas Karlman wrote:
> Hi Alex,
> 
> On 2024-06-19 19:19, Alex Bee wrote:
> > Am 19.06.24 um 17:28 schrieb Jonas Karlman:
> >> Hi Detlev,
> >> 
> >> On 2024-06-19 16:57, Detlev Casanova wrote:
> >>> Add the rkvdec2 Video Decoder to the RK3588s devicetree.
> >>> 
> >>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> >>> ---
> >>> 
> >>>   arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 50 +++++++++++++++++++++++
> >>>   1 file changed, 50 insertions(+)
> >>> 
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >>> b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi index
> >>> 6ac5ac8b48ab..7690632f57f1 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >>> @@ -2596,6 +2596,16 @@ system_sram2: sram@ff001000 {
> >>> 
> >>>   		ranges = <0x0 0x0 0xff001000 0xef000>;
> >>>   		#address-cells = <1>;
> >>>   		#size-cells = <1>;
> >>> 
> >>> +
> >>> +		vdec0_sram: rkvdec-sram@0 {
> >>> +			reg = <0x0 0x78000>;
> >>> +			pool;
> >>> +		};
> >>> +
> >>> +		vdec1_sram: rkvdec-sram@1 {
> >>> +			reg = <0x78000 0x77000>;
> >>> +			pool;
> >>> +		};
> >>> 
> >>>   	};
> >>>   	
> >>>   	pinctrl: pinctrl {
> >>> 
> >>> @@ -2665,6 +2675,46 @@ gpio4: gpio@fec50000 {
> >>> 
> >>>   			#interrupt-cells = <2>;
> >>>   		
> >>>   		};
> >>>   	
> >>>   	};
> >>> 
> >>> +
> >>> +	vdec0: video-decoder@fdc38100 {
> >>> +		compatible = "rockchip,rk3588-vdec";
> >>> +		reg = <0x0 0xfdc38100 0x0 0x500>;
> >>> +		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
> >>> +		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>, 
<&cru
> >>> CLK_RKVDEC0_CA>, +			 <&cru 
CLK_RKVDEC0_CORE>, <&cru
> >>> CLK_RKVDEC0_HEVC_CA>;
> >>> +		clock-names = "axi", "ahb", "cabac", "core", 
"hevc_cabac";
> >>> +		assigned-clocks = <&cru ACLK_RKVDEC0>, <&cru 
CLK_RKVDEC0_CORE>,
> >>> +				  <&cru CLK_RKVDEC0_CA>, <&cru 
CLK_RKVDEC0_HEVC_CA>;
> >>> +		assigned-clock-rates = <800000000>, <600000000>,
> >>> +				       <600000000>, <1000000000>;
> >>> +		resets = <&cru SRST_A_RKVDEC0>, <&cru SRST_H_RKVDEC0>, 
<&cru
> >>> SRST_RKVDEC0_CA>, +			 <&cru 
SRST_RKVDEC0_CORE>, <&cru
> >>> SRST_RKVDEC0_HEVC_CA>;
> >>> +		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
> >>> +			      "rst_core", "rst_hevc_cabac";
> >>> +		power-domains = <&power RK3588_PD_RKVDEC0>;
> >>> +		sram = <&vdec0_sram>;
> >>> +		status = "okay";
> >>> +	};
> >>> +
> >>> +	vdec1: video-decoder@fdc40100 {
> >>> +		compatible = "rockchip,rk3588-vdec";
> >>> +		reg = <0x0 0xfdc40100 0x0 0x500>;
> >>> +		interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH 0>;
> >>> +		clocks = <&cru ACLK_RKVDEC1>, <&cru HCLK_RKVDEC1>, 
<&cru
> >>> CLK_RKVDEC1_CA>, +			 <&cru 
CLK_RKVDEC1_CORE>, <&cru
> >>> CLK_RKVDEC1_HEVC_CA>;
> >>> +		clock-names = "axi", "ahb", "cabac", "core", 
"hevc_cabac";
> >>> +		assigned-clocks = <&cru ACLK_RKVDEC1>, <&cru 
CLK_RKVDEC1_CORE>,
> >>> +				  <&cru CLK_RKVDEC1_CA>, <&cru 
CLK_RKVDEC1_HEVC_CA>;
> >>> +		assigned-clock-rates = <800000000>, <600000000>,
> >>> +				       <600000000>, <1000000000>;
> >>> +		resets = <&cru SRST_A_RKVDEC1>, <&cru SRST_H_RKVDEC1>, 
<&cru
> >>> SRST_RKVDEC1_CA>, +			 <&cru 
SRST_RKVDEC1_CORE>, <&cru
> >>> SRST_RKVDEC1_HEVC_CA>;
> >>> +		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
> >>> +			      "rst_core", "rst_hevc_cabac";
> >>> +		power-domains = <&power RK3588_PD_RKVDEC1>;
> >>> +		sram = <&vdec1_sram>;
> >>> +		status = "okay";
> >>> +	};
> >> 
> >> This is still missing the iommus, please add the iommus, they should be
> >> 
> >> supported/same as the one used for e.g. VOP2:
> >>    compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
> >> 
> >> The VOP2 MMUs does have one extra mmu_cfg_mode flag in AUTO_GATING,
> >> compared to the VDPU381 MMUs, however only the AV1D MMU should be
> >> special on RK3588.
> >> 
> >> Please add the iommus :-)
> > 
> > When looking add the vendor DT/iommu driver I'm seeing serval quirks
> > applied for vdec's iommus. Since it's rightly frowned upon adding such
> > boolean-quirk-properties to upstream devicetrees, we'd at least need
> > additional (fallback-) compatibles, even if it works with the iommu driver
> > as is (what I doubt, but haven't tested). We need to be able to apply
> > those
> > quirks later without changing the devicetree (as usual) and I'm sure RK
> > devs haven't added these quirks for the personal amusement.
> 
> Based on what I investigated the hw should work similar, and the quirks
> mostly seem related to optimizations and sw quirks, like do not zap each
> line, keep it alive even when pm runtime say it is not in use and other
> quirks that seem to be more of sw nature on how to best utilize the hw.

I did some testing with the IOMMU but unfortunately, I'm only getting page 
fault errors. This may be something I'm doing wrong, but it clearly needs more 
investigation.

> > If Detlev says
> > iommu is out of scope for this series (which is valid), I'd say it's fine
> > to leave them out for now (as no binding exists) and the HW works
> > (obviously) fine without them.
> 
> Sure, use of MMU can be added later.

I'd rather go for that for now. I'll add that IMMU support is missing in the 
TODO file.

> Regards,
> Jonas
> 
> >> Regards,
> >> Jonas
> >> 
> >>>   };
> >>>   
> >>>   #include "rk3588s-pinctrl.dtsi"
Jonas Karlman June 24, 2024, 9:16 a.m. UTC | #7
Hi Detlev and Alex,

On 2024-06-20 15:31, Detlev Casanova wrote:
> Hi Jonas, Alex,
> 
> On Wednesday, June 19, 2024 2:06:40 P.M. EDT Jonas Karlman wrote:
>> Hi Alex,
>>
>> On 2024-06-19 19:19, Alex Bee wrote:
>>> Am 19.06.24 um 17:28 schrieb Jonas Karlman:
>>>> Hi Detlev,
>>>>
>>>> On 2024-06-19 16:57, Detlev Casanova wrote:
>>>>> Add the rkvdec2 Video Decoder to the RK3588s devicetree.
>>>>>
>>>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>>>>> ---
>>>>>
>>>>>   arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 50 +++++++++++++++++++++++
>>>>>   1 file changed, 50 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>>>>> b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi index
>>>>> 6ac5ac8b48ab..7690632f57f1 100644
>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>>>>> @@ -2596,6 +2596,16 @@ system_sram2: sram@ff001000 {
>>>>>
>>>>>   		ranges = <0x0 0x0 0xff001000 0xef000>;
>>>>>   		#address-cells = <1>;
>>>>>   		#size-cells = <1>;
>>>>>
>>>>> +
>>>>> +		vdec0_sram: rkvdec-sram@0 {
>>>>> +			reg = <0x0 0x78000>;
>>>>> +			pool;
>>>>> +		};
>>>>> +
>>>>> +		vdec1_sram: rkvdec-sram@1 {
>>>>> +			reg = <0x78000 0x77000>;
>>>>> +			pool;
>>>>> +		};
>>>>>
>>>>>   	};
>>>>>   	
>>>>>   	pinctrl: pinctrl {
>>>>>
>>>>> @@ -2665,6 +2675,46 @@ gpio4: gpio@fec50000 {
>>>>>
>>>>>   			#interrupt-cells = <2>;
>>>>>   		
>>>>>   		};
>>>>>   	
>>>>>   	};
>>>>>
>>>>> +
>>>>> +	vdec0: video-decoder@fdc38100 {
>>>>> +		compatible = "rockchip,rk3588-vdec";
>>>>> +		reg = <0x0 0xfdc38100 0x0 0x500>;
>>>>> +		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
>>>>> +		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>, 
> <&cru
>>>>> CLK_RKVDEC0_CA>, +			 <&cru 
> CLK_RKVDEC0_CORE>, <&cru
>>>>> CLK_RKVDEC0_HEVC_CA>;
>>>>> +		clock-names = "axi", "ahb", "cabac", "core", 
> "hevc_cabac";
>>>>> +		assigned-clocks = <&cru ACLK_RKVDEC0>, <&cru 
> CLK_RKVDEC0_CORE>,
>>>>> +				  <&cru CLK_RKVDEC0_CA>, <&cru 
> CLK_RKVDEC0_HEVC_CA>;
>>>>> +		assigned-clock-rates = <800000000>, <600000000>,
>>>>> +				       <600000000>, <1000000000>;
>>>>> +		resets = <&cru SRST_A_RKVDEC0>, <&cru SRST_H_RKVDEC0>, 
> <&cru
>>>>> SRST_RKVDEC0_CA>, +			 <&cru 
> SRST_RKVDEC0_CORE>, <&cru
>>>>> SRST_RKVDEC0_HEVC_CA>;
>>>>> +		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
>>>>> +			      "rst_core", "rst_hevc_cabac";
>>>>> +		power-domains = <&power RK3588_PD_RKVDEC0>;
>>>>> +		sram = <&vdec0_sram>;
>>>>> +		status = "okay";
>>>>> +	};
>>>>> +
>>>>> +	vdec1: video-decoder@fdc40100 {
>>>>> +		compatible = "rockchip,rk3588-vdec";
>>>>> +		reg = <0x0 0xfdc40100 0x0 0x500>;
>>>>> +		interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH 0>;
>>>>> +		clocks = <&cru ACLK_RKVDEC1>, <&cru HCLK_RKVDEC1>, 
> <&cru
>>>>> CLK_RKVDEC1_CA>, +			 <&cru 
> CLK_RKVDEC1_CORE>, <&cru
>>>>> CLK_RKVDEC1_HEVC_CA>;
>>>>> +		clock-names = "axi", "ahb", "cabac", "core", 
> "hevc_cabac";
>>>>> +		assigned-clocks = <&cru ACLK_RKVDEC1>, <&cru 
> CLK_RKVDEC1_CORE>,
>>>>> +				  <&cru CLK_RKVDEC1_CA>, <&cru 
> CLK_RKVDEC1_HEVC_CA>;
>>>>> +		assigned-clock-rates = <800000000>, <600000000>,
>>>>> +				       <600000000>, <1000000000>;
>>>>> +		resets = <&cru SRST_A_RKVDEC1>, <&cru SRST_H_RKVDEC1>, 
> <&cru
>>>>> SRST_RKVDEC1_CA>, +			 <&cru 
> SRST_RKVDEC1_CORE>, <&cru
>>>>> SRST_RKVDEC1_HEVC_CA>;
>>>>> +		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
>>>>> +			      "rst_core", "rst_hevc_cabac";
>>>>> +		power-domains = <&power RK3588_PD_RKVDEC1>;
>>>>> +		sram = <&vdec1_sram>;
>>>>> +		status = "okay";
>>>>> +	};
>>>>
>>>> This is still missing the iommus, please add the iommus, they should be
>>>>
>>>> supported/same as the one used for e.g. VOP2:
>>>>    compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
>>>>
>>>> The VOP2 MMUs does have one extra mmu_cfg_mode flag in AUTO_GATING,
>>>> compared to the VDPU381 MMUs, however only the AV1D MMU should be
>>>> special on RK3588.
>>>>
>>>> Please add the iommus :-)
>>>
>>> When looking add the vendor DT/iommu driver I'm seeing serval quirks
>>> applied for vdec's iommus. Since it's rightly frowned upon adding such
>>> boolean-quirk-properties to upstream devicetrees, we'd at least need
>>> additional (fallback-) compatibles, even if it works with the iommu driver
>>> as is (what I doubt, but haven't tested). We need to be able to apply
>>> those
>>> quirks later without changing the devicetree (as usual) and I'm sure RK
>>> devs haven't added these quirks for the personal amusement.
>>
>> Based on what I investigated the hw should work similar, and the quirks
>> mostly seem related to optimizations and sw quirks, like do not zap each
>> line, keep it alive even when pm runtime say it is not in use and other
>> quirks that seem to be more of sw nature on how to best utilize the hw.
> 
> I did some testing with the IOMMU but unfortunately, I'm only getting page 
> fault errors. This may be something I'm doing wrong, but it clearly needs more 
> investigation.

I re-tested and the addition of sram seem to now cause page faults, the
sram also need to be mapped in the iommu.

However, doing more testing revealed that use of iommu present the same
issue as seen with hevc on rk3399, after a fail fluster tests continue
to fail until a reset.

Seeing how this issue was very similar I re-tested on rk3399 without
iommu and cma=1G and could observe that there was no longer any need to
reset after a failed test. Interestingly the score also went up from
135 to 137/147.

Digging some more revealed that the iommu also is reset during the
internal rkvdec soft reset on error, leaving the iommu with dte_addr=0
and paging in disabled state.

Ensuring that the iommu was reconfigured after a failure fixed the issue
observed on rk3399 and I now also get 137/147 hevc fluster score using
the iommu.

Will send out a rkvdec hevc v2 series after some more testing.

Guessing there is a similar need to reconfigure iommu on rk3588, and my
initial tests also showed promising result, however more tests are
needed.

Regards,
Jonas

> 
>>> If Detlev says
>>> iommu is out of scope for this series (which is valid), I'd say it's fine
>>> to leave them out for now (as no binding exists) and the HW works
>>> (obviously) fine without them.
>>
>> Sure, use of MMU can be added later.
> 
> I'd rather go for that for now. I'll add that IMMU support is missing in the 
> TODO file.
> 
>> Regards,
>> Jonas
>>
>>>> Regards,
>>>> Jonas
>>>>
>>>>>   };
>>>>>   
>>>>>   #include "rk3588s-pinctrl.dtsi"
>
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
index 6ac5ac8b48ab..7690632f57f1 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -2596,6 +2596,16 @@  system_sram2: sram@ff001000 {
 		ranges = <0x0 0x0 0xff001000 0xef000>;
 		#address-cells = <1>;
 		#size-cells = <1>;
+
+		vdec0_sram: rkvdec-sram@0 {
+			reg = <0x0 0x78000>;
+			pool;
+		};
+
+		vdec1_sram: rkvdec-sram@1 {
+			reg = <0x78000 0x77000>;
+			pool;
+		};
 	};
 
 	pinctrl: pinctrl {
@@ -2665,6 +2675,46 @@  gpio4: gpio@fec50000 {
 			#interrupt-cells = <2>;
 		};
 	};
+
+	vdec0: video-decoder@fdc38100 {
+		compatible = "rockchip,rk3588-vdec";
+		reg = <0x0 0xfdc38100 0x0 0x500>;
+		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>, <&cru CLK_RKVDEC0_CA>,
+			 <&cru CLK_RKVDEC0_CORE>, <&cru CLK_RKVDEC0_HEVC_CA>;
+		clock-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
+		assigned-clocks = <&cru ACLK_RKVDEC0>, <&cru CLK_RKVDEC0_CORE>,
+				  <&cru CLK_RKVDEC0_CA>, <&cru CLK_RKVDEC0_HEVC_CA>;
+		assigned-clock-rates = <800000000>, <600000000>,
+				       <600000000>, <1000000000>;
+		resets = <&cru SRST_A_RKVDEC0>, <&cru SRST_H_RKVDEC0>, <&cru SRST_RKVDEC0_CA>,
+			 <&cru SRST_RKVDEC0_CORE>, <&cru SRST_RKVDEC0_HEVC_CA>;
+		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
+			      "rst_core", "rst_hevc_cabac";
+		power-domains = <&power RK3588_PD_RKVDEC0>;
+		sram = <&vdec0_sram>;
+		status = "okay";
+	};
+
+	vdec1: video-decoder@fdc40100 {
+		compatible = "rockchip,rk3588-vdec";
+		reg = <0x0 0xfdc40100 0x0 0x500>;
+		interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_RKVDEC1>, <&cru HCLK_RKVDEC1>, <&cru CLK_RKVDEC1_CA>,
+			 <&cru CLK_RKVDEC1_CORE>, <&cru CLK_RKVDEC1_HEVC_CA>;
+		clock-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
+		assigned-clocks = <&cru ACLK_RKVDEC1>, <&cru CLK_RKVDEC1_CORE>,
+				  <&cru CLK_RKVDEC1_CA>, <&cru CLK_RKVDEC1_HEVC_CA>;
+		assigned-clock-rates = <800000000>, <600000000>,
+				       <600000000>, <1000000000>;
+		resets = <&cru SRST_A_RKVDEC1>, <&cru SRST_H_RKVDEC1>, <&cru SRST_RKVDEC1_CA>,
+			 <&cru SRST_RKVDEC1_CORE>, <&cru SRST_RKVDEC1_HEVC_CA>;
+		reset-names = "rst_axi", "rst_ahb", "rst_cabac",
+			      "rst_core", "rst_hevc_cabac";
+		power-domains = <&power RK3588_PD_RKVDEC1>;
+		sram = <&vdec1_sram>;
+		status = "okay";
+	};
 };
 
 #include "rk3588s-pinctrl.dtsi"