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

Message ID 20240615015734.1612108-4-detlev.casanova@collabora.com (mailing list archive)
State Superseded
Delegated to: Sebastian Fricke
Headers
Series media: rockchip: Add rkvdec2 driver |

Commit Message

Detlev Casanova June 15, 2024, 1:56 a.m. UTC
  Add the rkvdec2 Video Decoder to the RK3588s devicetree.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 .../boot/dts/rockchip/rk3588-rock-5b.dts      |  4 ++++
 .../boot/dts/rockchip/rk3588s-orangepi-5.dts  |  4 ++++
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 19 +++++++++++++++++++
 3 files changed, 27 insertions(+)
  

Comments

Jonas Karlman June 15, 2024, 8:25 a.m. UTC | #1
Hi Detlev,

On 2024-06-15 03:56, Detlev Casanova wrote:
> Add the rkvdec2 Video Decoder to the RK3588s devicetree.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  .../boot/dts/rockchip/rk3588-rock-5b.dts      |  4 ++++
>  .../boot/dts/rockchip/rk3588s-orangepi-5.dts  |  4 ++++
>  arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 19 +++++++++++++++++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> index c551b676860c..965322c24a65 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> @@ -503,6 +503,10 @@ &pwm1 {
>  	status = "okay";
>  };
>  
> +&rkvdec0 {
> +	status = "okay";
> +};

Enable of rkvdec0 should probably be split out from the patch that adds
the rkvdec0 node to soc dtsi.

Also why is rkvdec0 only enabled on rock-5b and orangepi-5?

> +
>  &saradc {
>  	vref-supply = <&avcc_1v8_s0>;
>  	status = "okay";
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> index feea6b20a6bf..2828fb4c182a 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> @@ -321,6 +321,10 @@ typec5v_pwren: typec5v-pwren {
>  	};
>  };
>  
> +&rkvdec0 {
> +	status = "okay";
> +};
> +
>  &saradc {
>  	vref-supply = <&avcc_1v8_s0>;
>  	status = "okay";
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> index 0fecbf46e127..09672636dcea 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> @@ -3034,6 +3034,9 @@ system_sram2: sram@ff001000 {
>  		ranges = <0x0 0x0 0xff001000 0xef000>;
>  		#address-cells = <1>;
>  		#size-cells = <1>;

Blank line is missing.

> +		rkvdec0_sram: rkvdec-sram@0 {
> +			reg = <0x0 0x78000>;
> +		};
>  	};
>  
>  	pinctrl: pinctrl {
> @@ -3103,6 +3106,22 @@ gpio4: gpio@fec50000 {
>  			#interrupt-cells = <2>;
>  		};
>  	};
> +
> +	rkvdec0: video-decoder@fdc38100 {
> +		compatible = "rockchip,rk3588-vdec2";
> +		reg = <0x0 0xfdc38100 0x0 0x500>;
> +		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
> +		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>, <&cru CLK_RKVDEC0_CORE>,
> +			 <&cru CLK_RKVDEC0_CA>, <&cru CLK_RKVDEC0_HEVC_CA>;
> +		clock-names = "axi", "ahb", "core",
> +			      "cabac", "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>;
> +		power-domains = <&power RK3588_PD_RKVDEC0>;

iommus and resets should probably be added.

> +		status = "disabled";
> +	};

The iommu node for rkvdec0_mmu seem to be missing, is it not required to
be able to use memory >4GiB as decoding buffers?

I would also consider adding the rkvdec1 node(s), if I am understanding
correctly they can both be used in a cluster or completely independent.

Also on RK3582/RK3583 one (or both) of the decoder cores may be marked
as bad, yet the remaining one can still be used independently. The idea
will be that bootloader fixup the DT and disabled/delete-node the bad
core(s).

Regards,
Jonas

>  };
>  
>  #include "rk3588s-pinctrl.dtsi"
  
Detlev Casanova June 15, 2024, 7:55 p.m. UTC | #2
On Saturday, June 15, 2024 4:25:27 A.M. EDT Jonas Karlman wrote:
> Hi Detlev,
> 
> On 2024-06-15 03:56, Detlev Casanova wrote:
> > Add the rkvdec2 Video Decoder to the RK3588s devicetree.
> > 
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >  .../boot/dts/rockchip/rk3588-rock-5b.dts      |  4 ++++
> >  .../boot/dts/rockchip/rk3588s-orangepi-5.dts  |  4 ++++
> >  arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 19 +++++++++++++++++++
> >  3 files changed, 27 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
> > c551b676860c..965322c24a65 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > @@ -503,6 +503,10 @@ &pwm1 {
> > 
> >  	status = "okay";
> >  
> >  };
> > 
> > +&rkvdec0 {
> > +	status = "okay";
> > +};
> 
> Enable of rkvdec0 should probably be split out from the patch that adds
> the rkvdec0 node to soc dtsi.

Ack

> Also why is rkvdec0 only enabled on rock-5b and orangepi-5?

I only could test on those two but I can enable it on all rk3588 devices.

> > +
> > 
> >  &saradc {
> >  
> >  	vref-supply = <&avcc_1v8_s0>;
> >  	status = "okay";
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> > b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts index
> > feea6b20a6bf..2828fb4c182a 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> > @@ -321,6 +321,10 @@ typec5v_pwren: typec5v-pwren {
> > 
> >  	};
> >  
> >  };
> > 
> > +&rkvdec0 {
> > +	status = "okay";
> > +};
> > +
> > 
> >  &saradc {
> >  
> >  	vref-supply = <&avcc_1v8_s0>;
> >  	status = "okay";
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi index
> > 0fecbf46e127..09672636dcea 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > @@ -3034,6 +3034,9 @@ system_sram2: sram@ff001000 {
> > 
> >  		ranges = <0x0 0x0 0xff001000 0xef000>;
> >  		#address-cells = <1>;
> >  		#size-cells = <1>;
> 
> Blank line is missing.
> 
> > +		rkvdec0_sram: rkvdec-sram@0 {
> > +			reg = <0x0 0x78000>;
> > +		};
> > 
> >  	};
> >  	
> >  	pinctrl: pinctrl {
> > 
> > @@ -3103,6 +3106,22 @@ gpio4: gpio@fec50000 {
> > 
> >  			#interrupt-cells = <2>;
> >  		
> >  		};
> >  	
> >  	};
> > 
> > +
> > +	rkvdec0: video-decoder@fdc38100 {
> > +		compatible = "rockchip,rk3588-vdec2";
> > +		reg = <0x0 0xfdc38100 0x0 0x500>;
> > +		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
> > +		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>, 
<&cru
> > CLK_RKVDEC0_CORE>, +			 <&cru 
CLK_RKVDEC0_CA>, <&cru
> > CLK_RKVDEC0_HEVC_CA>;
> > +		clock-names = "axi", "ahb", "core",
> > +			      "cabac", "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>;
> > +		power-domains = <&power RK3588_PD_RKVDEC0>;
> 
> iommus and resets should probably be added.
> 
> > +		status = "disabled";
> > +	};
> 
> The iommu node for rkvdec0_mmu seem to be missing, is it not required to
> be able to use memory >4GiB as decoding buffers?

I need to check if the current rockchip iommu driver will work for this 
decoder. I remember that the iommu code for AV1 was a bit different, not sure 
about this rkvdec.

> I would also consider adding the rkvdec1 node(s), if I am understanding
> correctly they can both be used in a cluster or completely independent.

They can be used independently, yes. I'll add rkvdec1 for rk3588 devices 
(rk3588s only has  1 core)

Regards,
Detlev.

> Also on RK3582/RK3583 one (or both) of the decoder cores may be marked
> as bad, yet the remaining one can still be used independently. The idea
> will be that bootloader fixup the DT and disabled/delete-node the bad
> core(s).
> 
> Regards,
> Jonas
> 
> >  };
> >  
> >  #include "rk3588s-pinctrl.dtsi"
  
Heiko Stübner June 16, 2024, 7:28 a.m. UTC | #3
Am Samstag, 15. Juni 2024, 21:55:54 CEST schrieb Detlev Casanova:
> On Saturday, June 15, 2024 4:25:27 A.M. EDT Jonas Karlman wrote:
> > Hi Detlev,
> > 
> > On 2024-06-15 03:56, Detlev Casanova wrote:
> > > Add the rkvdec2 Video Decoder to the RK3588s devicetree.
> > > 
> > > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > > ---
> > > 
> > >  .../boot/dts/rockchip/rk3588-rock-5b.dts      |  4 ++++
> > >  .../boot/dts/rockchip/rk3588s-orangepi-5.dts  |  4 ++++
> > >  arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 19 +++++++++++++++++++
> > >  3 files changed, 27 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
> > > c551b676860c..965322c24a65 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > @@ -503,6 +503,10 @@ &pwm1 {
> > > 
> > >  	status = "okay";
> > >  
> > >  };
> > > 
> > > +&rkvdec0 {
> > > +	status = "okay";
> > > +};
> > 
> > Enable of rkvdec0 should probably be split out from the patch that adds
> > the rkvdec0 node to soc dtsi.
> 
> Ack
> 
> > Also why is rkvdec0 only enabled on rock-5b and orangepi-5?
> 
> I only could test on those two but I can enable it on all rk3588 devices.
> 
> > > +
> > > 
> > >  &saradc {
> > >  
> > >  	vref-supply = <&avcc_1v8_s0>;
> > >  	status = "okay";
> > > 
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> > > b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts index
> > > feea6b20a6bf..2828fb4c182a 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> > > @@ -321,6 +321,10 @@ typec5v_pwren: typec5v-pwren {
> > > 
> > >  	};
> > >  
> > >  };
> > > 
> > > +&rkvdec0 {
> > > +	status = "okay";
> > > +};
> > > +
> > > 
> > >  &saradc {
> > >  
> > >  	vref-supply = <&avcc_1v8_s0>;
> > >  	status = "okay";
> > > 
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi index
> > > 0fecbf46e127..09672636dcea 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > > @@ -3034,6 +3034,9 @@ system_sram2: sram@ff001000 {
> > > 
> > >  		ranges = <0x0 0x0 0xff001000 0xef000>;
> > >  		#address-cells = <1>;
> > >  		#size-cells = <1>;
> > 
> > Blank line is missing.
> > 
> > > +		rkvdec0_sram: rkvdec-sram@0 {
> > > +			reg = <0x0 0x78000>;
> > > +		};
> > > 
> > >  	};
> > >  	
> > >  	pinctrl: pinctrl {
> > > 
> > > @@ -3103,6 +3106,22 @@ gpio4: gpio@fec50000 {
> > > 
> > >  			#interrupt-cells = <2>;
> > >  		
> > >  		};
> > >  	
> > >  	};
> > > 
> > > +
> > > +	rkvdec0: video-decoder@fdc38100 {
> > > +		compatible = "rockchip,rk3588-vdec2";
> > > +		reg = <0x0 0xfdc38100 0x0 0x500>;
> > > +		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
> > > +		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>, 
> <&cru
> > > CLK_RKVDEC0_CORE>, +			 <&cru 
> CLK_RKVDEC0_CA>, <&cru
> > > CLK_RKVDEC0_HEVC_CA>;
> > > +		clock-names = "axi", "ahb", "core",
> > > +			      "cabac", "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>;
> > > +		power-domains = <&power RK3588_PD_RKVDEC0>;
> > 
> > iommus and resets should probably be added.
> > 
> > > +		status = "disabled";
> > > +	};
> > 
> > The iommu node for rkvdec0_mmu seem to be missing, is it not required to
> > be able to use memory >4GiB as decoding buffers?
> 
> I need to check if the current rockchip iommu driver will work for this 
> decoder. I remember that the iommu code for AV1 was a bit different, not sure 
> about this rkvdec.
> 
> > I would also consider adding the rkvdec1 node(s), if I am understanding
> > correctly they can both be used in a cluster or completely independent.
> 
> They can be used independently, yes. I'll add rkvdec1 for rk3588 devices 
> (rk3588s only has  1 core)

Please check in with Sebastian Reichel about clusters/independent
controllers. He had a lot of fruitful discussions for the VEPU121/VPU121
support he is working on.

Baseline being, while we want the hw to be described correctly wrt the
multiple instances, we don't generally want to expose them individually
to userspace, because that would then require userspace to do all the
scheduling.
  
Jonas Karlman June 16, 2024, 9:17 a.m. UTC | #4
Hi Detlev,

On 2024-06-15 21:55, Detlev Casanova wrote:
> On Saturday, June 15, 2024 4:25:27 A.M. EDT Jonas Karlman wrote:
>> Hi Detlev,
>>
>> On 2024-06-15 03:56, Detlev Casanova wrote:
>>> Add the rkvdec2 Video Decoder to the RK3588s devicetree.
>>>
>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>>> ---
>>>
>>>  .../boot/dts/rockchip/rk3588-rock-5b.dts      |  4 ++++
>>>  .../boot/dts/rockchip/rk3588s-orangepi-5.dts  |  4 ++++
>>>  arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 19 +++++++++++++++++++
>>>  3 files changed, 27 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
>>> c551b676860c..965322c24a65 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> @@ -503,6 +503,10 @@ &pwm1 {
>>>
>>>  	status = "okay";
>>>  
>>>  };
>>>
>>> +&rkvdec0 {
>>> +	status = "okay";
>>> +};
>>
>> Enable of rkvdec0 should probably be split out from the patch that adds
>> the rkvdec0 node to soc dtsi.
> 
> Ack
> 
>> Also why is rkvdec0 only enabled on rock-5b and orangepi-5?
> 
> I only could test on those two but I can enable it on all rk3588 devices.

Because the decoder is an integrated part of the SoC the default should
probably be that the IP is enabled, i.e. no status prop required for the
vdec and related mmu nodes in rk3588s.dtsi.

> 
>>> +
>>>
>>>  &saradc {
>>>  
>>>  	vref-supply = <&avcc_1v8_s0>;
>>>  	status = "okay";
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
>>> b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts index
>>> feea6b20a6bf..2828fb4c182a 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
>>> @@ -321,6 +321,10 @@ typec5v_pwren: typec5v-pwren {
>>>
>>>  	};
>>>  
>>>  };
>>>
>>> +&rkvdec0 {
>>> +	status = "okay";
>>> +};
>>> +
>>>
>>>  &saradc {
>>>  
>>>  	vref-supply = <&avcc_1v8_s0>;
>>>  	status = "okay";
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>>> b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi index
>>> 0fecbf46e127..09672636dcea 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>>> @@ -3034,6 +3034,9 @@ system_sram2: sram@ff001000 {
>>>
>>>  		ranges = <0x0 0x0 0xff001000 0xef000>;
>>>  		#address-cells = <1>;
>>>  		#size-cells = <1>;
>>
>> Blank line is missing.
>>
>>> +		rkvdec0_sram: rkvdec-sram@0 {
>>> +			reg = <0x0 0x78000>;
>>> +		};
>>>
>>>  	};
>>>  	
>>>  	pinctrl: pinctrl {
>>>
>>> @@ -3103,6 +3106,22 @@ gpio4: gpio@fec50000 {
>>>
>>>  			#interrupt-cells = <2>;
>>>  		
>>>  		};
>>>  	
>>>  	};
>>>
>>> +
>>> +	rkvdec0: video-decoder@fdc38100 {

To match prior generations the symbol should probably be called vdec0.

>>> +		compatible = "rockchip,rk3588-vdec2";
>>> +		reg = <0x0 0xfdc38100 0x0 0x500>;
>>> +		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
>>> +		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>, 
> <&cru
>>> CLK_RKVDEC0_CORE>, +			 <&cru 
> CLK_RKVDEC0_CA>, <&cru
>>> CLK_RKVDEC0_HEVC_CA>;
>>> +		clock-names = "axi", "ahb", "core",
>>> +			      "cabac", "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>;
>>> +		power-domains = <&power RK3588_PD_RKVDEC0>;
>>
>> iommus and resets should probably be added.
>>
>>> +		status = "disabled";
>>> +	};
>>
>> The iommu node for rkvdec0_mmu seem to be missing, is it not required to
>> be able to use memory >4GiB as decoding buffers?
> 
> I need to check if the current rockchip iommu driver will work for this 
> decoder. I remember that the iommu code for AV1 was a bit different, not sure 
> about this rkvdec.

The device tree should describe the HW not what drivers are capable of.

If there are substantial differences in iommu IP a new compatible should
probably be added for that iommu.

> 
>> I would also consider adding the rkvdec1 node(s), if I am understanding
>> correctly they can both be used in a cluster or completely independent.
> 
> They can be used independently, yes. I'll add rkvdec1 for rk3588 devices 
> (rk3588s only has  1 core)

I do not think that is true, the rk3588s variant should also include two
decoder and two encoder cores.

However, the rk3582/rk3583 variants (rk3588s with one or more bad cores)
may have 0-2 cores working for the decoder and/or encoder.

E.g on my rk3582 boards I have following different ip-state in otp:
- 1 bad cpu core (ip-state: 10 00 00)
- 1 bad decoder core (ip-state: 00 80 00)
- 1 bad encoder core (ip-state: 00 00 04)

The general idea is that bootloader will disable or delete the offending
nodes in the device tree to correctly describe the HW for the OS.

Regards,
Jonas

> 
> Regards,
> Detlev.
> 
>> Also on RK3582/RK3583 one (or both) of the decoder cores may be marked
>> as bad, yet the remaining one can still be used independently. The idea
>> will be that bootloader fixup the DT and disabled/delete-node the bad
>> core(s).
>>
>> Regards,
>> Jonas
>>
>>>  };
>>>  
>>>  #include "rk3588s-pinctrl.dtsi"
>
  
Dragan Simic June 16, 2024, 11:55 a.m. UTC | #5
Hello all,

On 2024-06-16 11:17, Jonas Karlman wrote:
> On 2024-06-15 21:55, Detlev Casanova wrote:
>> On Saturday, June 15, 2024 4:25:27 A.M. EDT Jonas Karlman wrote:
>>> On 2024-06-15 03:56, Detlev Casanova wrote:
>>>> Add the rkvdec2 Video Decoder to the RK3588s devicetree.
>>>> 
>>>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>>>> ---
>>>> 
>>>>  .../boot/dts/rockchip/rk3588-rock-5b.dts      |  4 ++++
>>>>  .../boot/dts/rockchip/rk3588s-orangepi-5.dts  |  4 ++++
>>>>  arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 19 
>>>> +++++++++++++++++++
>>>>  3 files changed, 27 insertions(+)
>>>> 
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>>> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
>>>> c551b676860c..965322c24a65 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>>> @@ -503,6 +503,10 @@ &pwm1 {
>>>> 
>>>>  	status = "okay";
>>>> 
>>>>  };
>>>> 
>>>> +&rkvdec0 {
>>>> +	status = "okay";
>>>> +};
>>> 
>>> Enable of rkvdec0 should probably be split out from the patch that 
>>> adds
>>> the rkvdec0 node to soc dtsi.
>> 
>> Ack
>> 
>>> Also why is rkvdec0 only enabled on rock-5b and orangepi-5?
>> 
>> I only could test on those two but I can enable it on all rk3588 
>> devices.
> 
> Because the decoder is an integrated part of the SoC the default should
> probably be that the IP is enabled, i.e. no status prop required for 
> the
> vdec and related mmu nodes in rk3588s.dtsi.

Agreed, the decoder is a SoC feature.  As such, it should be enabled on
the SoC level, instead of having it enabled on the board level.
  
Detlev Casanova June 17, 2024, 2:31 p.m. UTC | #6
On Sunday, June 16, 2024 3:28:25 A.M. EDT Heiko Stuebner wrote:
> Am Samstag, 15. Juni 2024, 21:55:54 CEST schrieb Detlev Casanova:
> > On Saturday, June 15, 2024 4:25:27 A.M. EDT Jonas Karlman wrote:
> > > Hi Detlev,
> > > 
> > > On 2024-06-15 03:56, Detlev Casanova wrote:
> > > > Add the rkvdec2 Video Decoder to the RK3588s devicetree.
> > > > 
> > > > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > > > ---
> > > > 
> > > >  .../boot/dts/rockchip/rk3588-rock-5b.dts      |  4 ++++
> > > >  .../boot/dts/rockchip/rk3588s-orangepi-5.dts  |  4 ++++
> > > >  arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 19
> > > >  +++++++++++++++++++
> > > >  3 files changed, 27 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
> > > > c551b676860c..965322c24a65 100644
> > > > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > > @@ -503,6 +503,10 @@ &pwm1 {
> > > > 
> > > >  	status = "okay";
> > > >  
> > > >  };
> > > > 
> > > > +&rkvdec0 {
> > > > +	status = "okay";
> > > > +};
> > > 
> > > Enable of rkvdec0 should probably be split out from the patch that adds
> > > the rkvdec0 node to soc dtsi.
> > 
> > Ack
> > 
> > > Also why is rkvdec0 only enabled on rock-5b and orangepi-5?
> > 
> > I only could test on those two but I can enable it on all rk3588 devices.
> > 
> > > > +
> > > > 
> > > >  &saradc {
> > > >  
> > > >  	vref-supply = <&avcc_1v8_s0>;
> > > >  	status = "okay";
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> > > > b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts index
> > > > feea6b20a6bf..2828fb4c182a 100644
> > > > --- a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> > > > +++ b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> > > > @@ -321,6 +321,10 @@ typec5v_pwren: typec5v-pwren {
> > > > 
> > > >  	};
> > > >  
> > > >  };
> > > > 
> > > > +&rkvdec0 {
> > > > +	status = "okay";
> > > > +};
> > > > +
> > > > 
> > > >  &saradc {
> > > >  
> > > >  	vref-supply = <&avcc_1v8_s0>;
> > > >  	status = "okay";
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > > > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi index
> > > > 0fecbf46e127..09672636dcea 100644
> > > > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > > > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > > > @@ -3034,6 +3034,9 @@ system_sram2: sram@ff001000 {
> > > > 
> > > >  		ranges = <0x0 0x0 0xff001000 0xef000>;
> > > >  		#address-cells = <1>;
> > > >  		#size-cells = <1>;
> > > 
> > > Blank line is missing.
> > > 
> > > > +		rkvdec0_sram: rkvdec-sram@0 {
> > > > +			reg = <0x0 0x78000>;
> > > > +		};
> > > > 
> > > >  	};
> > > >  	
> > > >  	pinctrl: pinctrl {
> > > > 
> > > > @@ -3103,6 +3106,22 @@ gpio4: gpio@fec50000 {
> > > > 
> > > >  			#interrupt-cells = <2>;
> > > >  		
> > > >  		};
> > > >  	
> > > >  	};
> > > > 
> > > > +
> > > > +	rkvdec0: video-decoder@fdc38100 {
> > > > +		compatible = "rockchip,rk3588-vdec2";
> > > > +		reg = <0x0 0xfdc38100 0x0 0x500>;
> > > > +		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
> > > > +		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>,
> > 
> > <&cru
> > 
> > > > CLK_RKVDEC0_CORE>, +			 <&cru
> > 
> > CLK_RKVDEC0_CA>, <&cru
> > 
> > > > CLK_RKVDEC0_HEVC_CA>;
> > > > +		clock-names = "axi", "ahb", "core",
> > > > +			      "cabac", "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>;
> > > > +		power-domains = <&power RK3588_PD_RKVDEC0>;
> > > 
> > > iommus and resets should probably be added.
> > > 
> > > > +		status = "disabled";
> > > > +	};
> > > 
> > > The iommu node for rkvdec0_mmu seem to be missing, is it not required to
> > > be able to use memory >4GiB as decoding buffers?
> > 
> > I need to check if the current rockchip iommu driver will work for this
> > decoder. I remember that the iommu code for AV1 was a bit different, not
> > sure about this rkvdec.
> > 
> > > I would also consider adding the rkvdec1 node(s), if I am understanding
> > > correctly they can both be used in a cluster or completely independent.
> > 
> > They can be used independently, yes. I'll add rkvdec1 for rk3588 devices
> > (rk3588s only has  1 core)
> 
> Please check in with Sebastian Reichel about clusters/independent
> controllers. He had a lot of fruitful discussions for the VEPU121/VPU121
> support he is working on.

Yes, basically, it makes the driver check for all nodes with the same 
compatible and list the instances in the same device instance.
If multicore is not supported, the other instances can simple be ignored.

See here: https://lore.kernel.org/linux-kernel/20240613135034.31684-4-sebastian.reichel@collabora.com/

> Baseline being, while we want the hw to be described correctly wrt the
> multiple instances, we don't generally want to expose them individually
> to userspace, because that would then require userspace to do all the
> scheduling.

Yes, so Sebastien's patch will allow the device tree to have the 2 cores but 
only one visible by the userspace (even when multicore decoding is not 
supported)
  
Detlev Casanova June 17, 2024, 2:36 p.m. UTC | #7
On Sunday, June 16, 2024 5:17:31 A.M. EDT Jonas Karlman wrote:
> Hi Detlev,
> 
> On 2024-06-15 21:55, Detlev Casanova wrote:
> > On Saturday, June 15, 2024 4:25:27 A.M. EDT Jonas Karlman wrote:
> >> Hi Detlev,
> >> 
> >> On 2024-06-15 03:56, Detlev Casanova wrote:
> >>> Add the rkvdec2 Video Decoder to the RK3588s devicetree.
> >>> 
> >>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> >>> ---
> >>> 
> >>>  .../boot/dts/rockchip/rk3588-rock-5b.dts      |  4 ++++
> >>>  .../boot/dts/rockchip/rk3588s-orangepi-5.dts  |  4 ++++
> >>>  arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 19 +++++++++++++++++++
> >>>  3 files changed, 27 insertions(+)
> >>> 
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >>> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
> >>> c551b676860c..965322c24a65 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >>> @@ -503,6 +503,10 @@ &pwm1 {
> >>> 
> >>>  	status = "okay";
> >>>  
> >>>  };
> >>> 
> >>> +&rkvdec0 {
> >>> +	status = "okay";
> >>> +};
> >> 
> >> Enable of rkvdec0 should probably be split out from the patch that adds
> >> the rkvdec0 node to soc dtsi.
> > 
> > Ack
> > 
> >> Also why is rkvdec0 only enabled on rock-5b and orangepi-5?
> > 
> > I only could test on those two but I can enable it on all rk3588 devices.
> 
> Because the decoder is an integrated part of the SoC the default should
> probably be that the IP is enabled, i.e. no status prop required for the
> vdec and related mmu nodes in rk3588s.dtsi.
> 
> >>> +
> >>> 
> >>>  &saradc {
> >>>  
> >>>  	vref-supply = <&avcc_1v8_s0>;
> >>>  	status = "okay";
> >>> 
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> >>> b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts index
> >>> feea6b20a6bf..2828fb4c182a 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> >>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
> >>> @@ -321,6 +321,10 @@ typec5v_pwren: typec5v-pwren {
> >>> 
> >>>  	};
> >>>  
> >>>  };
> >>> 
> >>> +&rkvdec0 {
> >>> +	status = "okay";
> >>> +};
> >>> +
> >>> 
> >>>  &saradc {
> >>>  
> >>>  	vref-supply = <&avcc_1v8_s0>;
> >>>  	status = "okay";
> >>> 
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >>> b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi index
> >>> 0fecbf46e127..09672636dcea 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >>> @@ -3034,6 +3034,9 @@ system_sram2: sram@ff001000 {
> >>> 
> >>>  		ranges = <0x0 0x0 0xff001000 0xef000>;
> >>>  		#address-cells = <1>;
> >>>  		#size-cells = <1>;
> >> 
> >> Blank line is missing.
> >> 
> >>> +		rkvdec0_sram: rkvdec-sram@0 {
> >>> +			reg = <0x0 0x78000>;
> >>> +		};
> >>> 
> >>>  	};
> >>>  	
> >>>  	pinctrl: pinctrl {
> >>> 
> >>> @@ -3103,6 +3106,22 @@ gpio4: gpio@fec50000 {
> >>> 
> >>>  			#interrupt-cells = <2>;
> >>>  		
> >>>  		};
> >>>  	
> >>>  	};
> >>> 
> >>> +
> >>> +	rkvdec0: video-decoder@fdc38100 {
> 
> To match prior generations the symbol should probably be called vdec0.
> 
> >>> +		compatible = "rockchip,rk3588-vdec2";
> >>> +		reg = <0x0 0xfdc38100 0x0 0x500>;
> >>> +		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
> >>> +		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>,
> > 
> > <&cru
> > 
> >>> CLK_RKVDEC0_CORE>, +			 <&cru
> > 
> > CLK_RKVDEC0_CA>, <&cru
> > 
> >>> CLK_RKVDEC0_HEVC_CA>;
> >>> +		clock-names = "axi", "ahb", "core",
> >>> +			      "cabac", "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>;
> >>> +		power-domains = <&power RK3588_PD_RKVDEC0>;
> >> 
> >> iommus and resets should probably be added.
> >> 
> >>> +		status = "disabled";
> >>> +	};
> >> 
> >> The iommu node for rkvdec0_mmu seem to be missing, is it not required to
> >> be able to use memory >4GiB as decoding buffers?
> > 
> > I need to check if the current rockchip iommu driver will work for this
> > decoder. I remember that the iommu code for AV1 was a bit different, not
> > sure about this rkvdec.
> 
> The device tree should describe the HW not what drivers are capable of.
> 
> If there are substantial differences in iommu IP a new compatible should
> probably be added for that iommu.
> 
> >> I would also consider adding the rkvdec1 node(s), if I am understanding
> >> correctly they can both be used in a cluster or completely independent.
> > 
> > They can be used independently, yes. I'll add rkvdec1 for rk3588 devices
> > (rk3588s only has  1 core)
> 
> I do not think that is true, the rk3588s variant should also include two
> decoder and two encoder cores.

You are right, I had HDMI ports in mind. There are 2 decoders available on 
rk3588s too.

> However, the rk3582/rk3583 variants (rk3588s with one or more bad cores)
> may have 0-2 cores working for the decoder and/or encoder.
> 
> E.g on my rk3582 boards I have following different ip-state in otp:
> - 1 bad cpu core (ip-state: 10 00 00)
> - 1 bad decoder core (ip-state: 00 80 00)
> - 1 bad encoder core (ip-state: 00 00 04)
> 
> The general idea is that bootloader will disable or delete the offending
> nodes in the device tree to correctly describe the HW for the OS.

I see, so I will add both cores, enabled, in rk3588s.dtsi and let the 
bootloader disable the ones that are bad.

Should I also add compatibles for rk3582/rk3583 then ?

> Regards,
> Jonas
> 
> > Regards,
> > Detlev.
> > 
> >> Also on RK3582/RK3583 one (or both) of the decoder cores may be marked
> >> as bad, yet the remaining one can still be used independently. The idea
> >> will be that bootloader fixup the DT and disabled/delete-node the bad
> >> core(s).
> >> 
> >> Regards,
> >> Jonas
> >> 
> >>>  };
> >>>  
> >>>  #include "rk3588s-pinctrl.dtsi"
  

Patch

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
index c551b676860c..965322c24a65 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
@@ -503,6 +503,10 @@  &pwm1 {
 	status = "okay";
 };
 
+&rkvdec0 {
+	status = "okay";
+};
+
 &saradc {
 	vref-supply = <&avcc_1v8_s0>;
 	status = "okay";
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
index feea6b20a6bf..2828fb4c182a 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-orangepi-5.dts
@@ -321,6 +321,10 @@  typec5v_pwren: typec5v-pwren {
 	};
 };
 
+&rkvdec0 {
+	status = "okay";
+};
+
 &saradc {
 	vref-supply = <&avcc_1v8_s0>;
 	status = "okay";
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
index 0fecbf46e127..09672636dcea 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -3034,6 +3034,9 @@  system_sram2: sram@ff001000 {
 		ranges = <0x0 0x0 0xff001000 0xef000>;
 		#address-cells = <1>;
 		#size-cells = <1>;
+		rkvdec0_sram: rkvdec-sram@0 {
+			reg = <0x0 0x78000>;
+		};
 	};
 
 	pinctrl: pinctrl {
@@ -3103,6 +3106,22 @@  gpio4: gpio@fec50000 {
 			#interrupt-cells = <2>;
 		};
 	};
+
+	rkvdec0: video-decoder@fdc38100 {
+		compatible = "rockchip,rk3588-vdec2";
+		reg = <0x0 0xfdc38100 0x0 0x500>;
+		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>, <&cru CLK_RKVDEC0_CORE>,
+			 <&cru CLK_RKVDEC0_CA>, <&cru CLK_RKVDEC0_HEVC_CA>;
+		clock-names = "axi", "ahb", "core",
+			      "cabac", "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>;
+		power-domains = <&power RK3588_PD_RKVDEC0>;
+		status = "disabled";
+	};
 };
 
 #include "rk3588s-pinctrl.dtsi"