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

[Detlev Casanova <detlev.casanova@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 5378 bytes --]

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"


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]