| Message ID | 20211117143347.314294-1-s.hauer@pengutronix.de |
|---|---|
| Headers | show |
| Series | drm/rockchip: RK356x VOP2 support | expand |
Am Mittwoch, 17. November 2021, 15:33:46 CET schrieb Sascha Hauer: > With upcoming VOP2 support VOP won't be the only choice anymore, so make > the VOP driver optional. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > arch/arm/configs/multi_v7_defconfig | 1 + > arch/arm64/configs/defconfig | 1 + > drivers/gpu/drm/rockchip/Kconfig | 7 +++++++ > drivers/gpu/drm/rockchip/Makefile | 3 ++- > drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 2 +- > 5 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig > index c951aeed2138c..fc123e8f3e2f9 100644 > --- a/arch/arm/configs/multi_v7_defconfig > +++ b/arch/arm/configs/multi_v7_defconfig > @@ -667,6 +667,7 @@ CONFIG_DRM_EXYNOS_DPI=y > CONFIG_DRM_EXYNOS_DSI=y > CONFIG_DRM_EXYNOS_HDMI=y > CONFIG_DRM_ROCKCHIP=m > +CONFIG_ROCKCHIP_VOP=y > CONFIG_ROCKCHIP_ANALOGIX_DP=y > CONFIG_ROCKCHIP_DW_HDMI=y > CONFIG_ROCKCHIP_DW_MIPI_DSI=y > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index f2e2b9bdd7024..a623386473dc9 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -682,6 +682,7 @@ CONFIG_DRM_EXYNOS_DSI=y > CONFIG_DRM_EXYNOS_HDMI=y > CONFIG_DRM_EXYNOS_MIC=y > CONFIG_DRM_ROCKCHIP=m > +CONFIG_ROCKCHIP_VOP=y > CONFIG_ROCKCHIP_ANALOGIX_DP=y > CONFIG_ROCKCHIP_CDN_DP=y > CONFIG_ROCKCHIP_DW_HDMI=y > diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig > index 9f1ecefc39332..a1c4158259099 100644 > --- a/drivers/gpu/drm/rockchip/Kconfig > +++ b/drivers/gpu/drm/rockchip/Kconfig > @@ -21,8 +21,15 @@ config DRM_ROCKCHIP > > if DRM_ROCKCHIP > > +config ROCKCHIP_VOP > + bool "Rockchip VOP driver" would this benefit from a default-y ? For builds reusing preexisting .configs. Heiko > + help > + This selects support for the VOP driver. You should enable it > + on all older SoCs up to RK3399. > + > config ROCKCHIP_ANALOGIX_DP > bool "Rockchip specific extensions for Analogix DP driver" > + depends on ROCKCHIP_VOP > help > This selects support for Rockchip SoC specific extensions > for the Analogix Core DP driver. If you want to enable DP > diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile > index 17a9e7eb2130d..cd6e7bb5ce9c5 100644 > --- a/drivers/gpu/drm/rockchip/Makefile > +++ b/drivers/gpu/drm/rockchip/Makefile > @@ -4,9 +4,10 @@ > # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher. > > rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \ > - rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o > + rockchip_drm_gem.o > rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o > > +rockchipdrm-$(CONFIG_ROCKCHIP_VOP) += rockchip_drm_vop.o rockchip_vop_reg.o > rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o > rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o > rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > index e4ebe60b3cc1a..64fa5fd62c01a 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > @@ -473,7 +473,7 @@ static int __init rockchip_drm_init(void) > int ret; > > num_rockchip_sub_drivers = 0; > - ADD_ROCKCHIP_SUB_DRIVER(vop_platform_driver, CONFIG_DRM_ROCKCHIP); > + ADD_ROCKCHIP_SUB_DRIVER(vop_platform_driver, CONFIG_ROCKCHIP_VOP); > ADD_ROCKCHIP_SUB_DRIVER(rockchip_lvds_driver, > CONFIG_ROCKCHIP_LVDS); > ADD_ROCKCHIP_SUB_DRIVER(rockchip_dp_driver, >
On Wed, Nov 17, 2021 at 03:40:26PM +0100, Heiko Stübner wrote: > Am Mittwoch, 17. November 2021, 15:33:46 CET schrieb Sascha Hauer: > > With upcoming VOP2 support VOP won't be the only choice anymore, so make > > the VOP driver optional. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > arch/arm/configs/multi_v7_defconfig | 1 + > > arch/arm64/configs/defconfig | 1 + > > drivers/gpu/drm/rockchip/Kconfig | 7 +++++++ > > drivers/gpu/drm/rockchip/Makefile | 3 ++- > > drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 2 +- > > 5 files changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig > > index c951aeed2138c..fc123e8f3e2f9 100644 > > --- a/arch/arm/configs/multi_v7_defconfig > > +++ b/arch/arm/configs/multi_v7_defconfig > > @@ -667,6 +667,7 @@ CONFIG_DRM_EXYNOS_DPI=y > > CONFIG_DRM_EXYNOS_DSI=y > > CONFIG_DRM_EXYNOS_HDMI=y > > CONFIG_DRM_ROCKCHIP=m > > +CONFIG_ROCKCHIP_VOP=y > > CONFIG_ROCKCHIP_ANALOGIX_DP=y > > CONFIG_ROCKCHIP_DW_HDMI=y > > CONFIG_ROCKCHIP_DW_MIPI_DSI=y > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > > index f2e2b9bdd7024..a623386473dc9 100644 > > --- a/arch/arm64/configs/defconfig > > +++ b/arch/arm64/configs/defconfig > > @@ -682,6 +682,7 @@ CONFIG_DRM_EXYNOS_DSI=y > > CONFIG_DRM_EXYNOS_HDMI=y > > CONFIG_DRM_EXYNOS_MIC=y > > CONFIG_DRM_ROCKCHIP=m > > +CONFIG_ROCKCHIP_VOP=y > > CONFIG_ROCKCHIP_ANALOGIX_DP=y > > CONFIG_ROCKCHIP_CDN_DP=y > > CONFIG_ROCKCHIP_DW_HDMI=y > > diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig > > index 9f1ecefc39332..a1c4158259099 100644 > > --- a/drivers/gpu/drm/rockchip/Kconfig > > +++ b/drivers/gpu/drm/rockchip/Kconfig > > @@ -21,8 +21,15 @@ config DRM_ROCKCHIP > > > > if DRM_ROCKCHIP > > > > +config ROCKCHIP_VOP > > + bool "Rockchip VOP driver" > > would this benefit from a default-y ? > For builds reusing preexisting .configs. I enabled CONFIG_ROCKCHIP_VOP for all configs in the tree that enable CONFIG_DRM_ROCKCHIP, so defconfig users shouldn't see any surprises. That won't help users of custom configs of course. I don't know what's preferred in such cases, I can change to default-y if you like. Sascha
On Wed, Nov 17, 2021 at 8:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote: > > This series adds initial graphics support for the Rockchip RK356[68] > SoCs. Graphics support is based around the VOP2 controller which > replaces the VOP controller found on earlier Rockchip SoCs. The driver > has been tested with HDMI support included in this series and MIPI-DSI > which is not included because it needs some more work. The driver is > taken from the downstream Rockchip kernel and heavily polished, most non > standard features have been removed for now. I tested the driver with > the libdrm modetest utility and also with weston with both pixman and > panfrost driver support. Michael Riesch reported the driver to work on > the RK3566 as well, but device tree support for this SoC is not yet > included in this series. Can you outline what exactly you want to disable? I don't think 'status' is the right way. I think between the parent device being disabled, an incomplete graph and user configuration choice that should be enough to disable parts. Rob
On Wed, Nov 17, 2021 at 8:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote: > > Add support for the HDMI port found on RK3568. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > arch/arm64/boot/dts/rockchip/rk356x.dtsi | 65 ++++++++++++++++++++++++ > 1 file changed, 65 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > index 6ebf7c14e096a..53be61a7ce595 100644 > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > @@ -472,18 +472,36 @@ vp0: port@0 { > #address-cells = <1>; > #size-cells = <0>; > reg = <0>; > + > + vp0_out_hdmi: endpoint@0 { > + reg = <0>; > + remote-endpoint = <&hdmi_in_vp0>; > + status = "disabled"; > + }; > }; > > vp1: port@1 { > #address-cells = <1>; > #size-cells = <0>; > reg = <1>; > + > + vp1_out_hdmi: endpoint@0 { > + reg = <0>; > + remote-endpoint = <&hdmi_in_vp1>; > + status = "disabled"; > + }; > }; > > vp2: port@2 { > #address-cells = <1>; > #size-cells = <0>; > reg = <2>; > + > + vp2_out_hdmi: endpoint@0 { > + reg = <0>; > + remote-endpoint = <&hdmi_in_vp2>; > + status = "disabled"; > + }; > }; > }; > }; > @@ -499,6 +517,53 @@ vop_mmu: iommu@fe043e00 { > status = "disabled"; > }; > > + hdmi: hdmi@fe0a0000 { > + compatible = "rockchip,rk3568-dw-hdmi"; > + reg = <0x0 0xfe0a0000 0x0 0x20000>; > + interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&cru PCLK_HDMI_HOST>, > + <&cru CLK_HDMI_SFR>, > + <&cru CLK_HDMI_CEC>, > + <&cru HCLK_VOP>; > + clock-names = "iahb", "isfr", "cec", "hclk"; > + power-domains = <&power RK3568_PD_VO>; > + reg-io-width = <4>; > + rockchip,grf = <&grf>; > + #sound-dai-cells = <0>; > + pinctrl-names = "default"; > + pinctrl-0 = <&hdmitx_scl &hdmitx_sda &hdmitxm0_cec>; > + status = "disabled"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + hdmi_in: port@0 { The schema says there is only 1 'port' node. Please run schema validation when making DT changes. However, an HDMI bridge should also define an output port to a connector node (or another bridge). So the fix is to allow 'port@0' and add an output port. > + reg = <0>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + hdmi_in_vp0: endpoint@0 { > + reg = <0>; > + remote-endpoint = <&vp0_out_hdmi>; > + status = "disabled"; > + }; > + > + hdmi_in_vp1: endpoint@1 { > + reg = <1>; > + remote-endpoint = <&vp1_out_hdmi>; > + status = "disabled"; > + }; > + > + hdmi_in_vp2: endpoint@2 { > + reg = <2>; > + remote-endpoint = <&vp2_out_hdmi>; > + status = "disabled"; > + }; > + }; > + }; > + }; > + > qos_gpu: qos@fe128000 { > compatible = "rockchip,rk3568-qos", "syscon"; > reg = <0x0 0xfe128000 0x0 0x20>; > -- > 2.30.2 >
Am Mittwoch, 17. November 2021, 15:50:54 CET schrieb Sascha Hauer: > On Wed, Nov 17, 2021 at 03:40:26PM +0100, Heiko Stübner wrote: > > Am Mittwoch, 17. November 2021, 15:33:46 CET schrieb Sascha Hauer: > > > With upcoming VOP2 support VOP won't be the only choice anymore, so make > > > the VOP driver optional. > > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > --- > > > arch/arm/configs/multi_v7_defconfig | 1 + > > > arch/arm64/configs/defconfig | 1 + > > > drivers/gpu/drm/rockchip/Kconfig | 7 +++++++ > > > drivers/gpu/drm/rockchip/Makefile | 3 ++- > > > drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 2 +- > > > 5 files changed, 12 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig > > > index c951aeed2138c..fc123e8f3e2f9 100644 > > > --- a/arch/arm/configs/multi_v7_defconfig > > > +++ b/arch/arm/configs/multi_v7_defconfig > > > @@ -667,6 +667,7 @@ CONFIG_DRM_EXYNOS_DPI=y > > > CONFIG_DRM_EXYNOS_DSI=y > > > CONFIG_DRM_EXYNOS_HDMI=y > > > CONFIG_DRM_ROCKCHIP=m > > > +CONFIG_ROCKCHIP_VOP=y > > > CONFIG_ROCKCHIP_ANALOGIX_DP=y > > > CONFIG_ROCKCHIP_DW_HDMI=y > > > CONFIG_ROCKCHIP_DW_MIPI_DSI=y > > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > > > index f2e2b9bdd7024..a623386473dc9 100644 > > > --- a/arch/arm64/configs/defconfig > > > +++ b/arch/arm64/configs/defconfig > > > @@ -682,6 +682,7 @@ CONFIG_DRM_EXYNOS_DSI=y > > > CONFIG_DRM_EXYNOS_HDMI=y > > > CONFIG_DRM_EXYNOS_MIC=y > > > CONFIG_DRM_ROCKCHIP=m > > > +CONFIG_ROCKCHIP_VOP=y > > > CONFIG_ROCKCHIP_ANALOGIX_DP=y > > > CONFIG_ROCKCHIP_CDN_DP=y > > > CONFIG_ROCKCHIP_DW_HDMI=y > > > diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig > > > index 9f1ecefc39332..a1c4158259099 100644 > > > --- a/drivers/gpu/drm/rockchip/Kconfig > > > +++ b/drivers/gpu/drm/rockchip/Kconfig > > > @@ -21,8 +21,15 @@ config DRM_ROCKCHIP > > > > > > if DRM_ROCKCHIP > > > > > > +config ROCKCHIP_VOP > > > + bool "Rockchip VOP driver" > > > > would this benefit from a default-y ? > > For builds reusing preexisting .configs. > > I enabled CONFIG_ROCKCHIP_VOP for all configs in the tree that enable > CONFIG_DRM_ROCKCHIP, so defconfig users shouldn't see any surprises. > That won't help users of custom configs of course. > > I don't know what's preferred in such cases, I can change to default-y > if you like. default-y would keep the behaviour identical for all existing configs I guess and right now vop(1) is still the most used variant and will stay that way for a while longer, so I guess my preference would be for going that route - also so that we don't drown in "my display stopped working" during 5.17 ;-) Heiko
On Wed, Nov 17, 2021 at 8:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote: > > This enabled the VOP2 display controller along with hdmi and the > required port routes which is enough to get a picture out of the > hdmi port of the board. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > .../boot/dts/rockchip/rk3568-evb1-v10.dts | 24 +++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > index 184e2aa2416af..156e001492173 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > @@ -106,6 +106,12 @@ &gmac1m1_rgmii_clk > status = "okay"; > }; > > +&hdmi { > + status = "okay"; > + avdd-0v9-supply = <&vdda0v9_image>; > + avdd-1v8-supply = <&vcca1v8_image>; > +}; > + > &i2c0 { > status = "okay"; > > @@ -390,3 +396,21 @@ &sdmmc0 { > &uart2 { > status = "okay"; > }; > + > +&vop { > + status = "okay"; > + assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>; > + assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>; > +}; > + > +&vop_mmu { > + status = "okay"; > +}; > + > +&hdmi_in_vp0 { > + status = "okay"; > +}; > + > +&vp0_out_hdmi { > + status = "okay"; > +}; You can accomplish the same thing already with: &vp0_out_hdmi { remote-endpoint = <&hdmi_in_vp0>; }; or: &vp0_out_hdmi { /delete-property/ remote-endpoint; };
Hi Sascha, On 11/17/21 3:33 PM, Sascha Hauer wrote: > This enabled the VOP2 display controller along with hdmi and the > required port routes which is enough to get a picture out of the > hdmi port of the board. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > .../boot/dts/rockchip/rk3568-evb1-v10.dts | 24 +++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > index 184e2aa2416af..156e001492173 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > @@ -106,6 +106,12 @@ &gmac1m1_rgmii_clk > status = "okay"; > }; > > +&hdmi { > + status = "okay"; > + avdd-0v9-supply = <&vdda0v9_image>; > + avdd-1v8-supply = <&vcca1v8_image>; > +}; > + > &i2c0 { > status = "okay"; > > @@ -390,3 +396,21 @@ &sdmmc0 { > &uart2 { > status = "okay"; > }; > + > +&vop { > + status = "okay"; > + assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>; > + assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>; > +}; > + > +&vop_mmu { > + status = "okay"; > +}; > + > +&hdmi_in_vp0 { > + status = "okay"; > +}; Minor nitpick: This should probably be sorted alphabetically. Best regards, Michael > + > +&vp0_out_hdmi { > + status = "okay"; > +}; >
On Wed, Nov 17, 2021 at 08:54:37AM -0600, Rob Herring wrote: > On Wed, Nov 17, 2021 at 8:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > > This series adds initial graphics support for the Rockchip RK356[68] > > SoCs. Graphics support is based around the VOP2 controller which > > replaces the VOP controller found on earlier Rockchip SoCs. The driver > > has been tested with HDMI support included in this series and MIPI-DSI > > which is not included because it needs some more work. The driver is > > taken from the downstream Rockchip kernel and heavily polished, most non > > standard features have been removed for now. I tested the driver with > > the libdrm modetest utility and also with weston with both pixman and > > panfrost driver support. Michael Riesch reported the driver to work on > > the RK3566 as well, but device tree support for this SoC is not yet > > included in this series. > > Can you outline what exactly you want to disable? I don't think > 'status' is the right way. I think between the parent device being > disabled, an incomplete graph and user configuration choice that > should be enough to disable parts. The VOP2 on the RK3568 has three CRTCS, or video ports (VP) in Rockchip nomenclature. Each of them can be connected to the different outputs, like HDMI, MIPI-DSI and so on. In the device tree the CRTCs are described as of-graph ports with links to the HDMI, MIPI-DSI nodes. An example limited to HDMI looks like this: vop: vop@fe040000 { compatible = "rockchip,rk3568-vop"; vop_out: ports { vp0: port@0 { vp0_out_hdmi: endpoint@0 { reg = <0>; remote-endpoint = <&hdmi_in_vp0>; status = "disabled"; }; ... MIPI, dP, ... }; vp1: port@1 { vp1_out_hdmi: endpoint@0 { reg = <0>; remote-endpoint = <&hdmi_in_vp1>; status = "disabled"; }; ... MIPI, dP, ... }; vp2: port@2 { ... }; }; }; hdmi: hdmi@fe0a0000 { compatible = "rockchip,rk3568-dw-hdmi"; ports { hdmi_in: port@0 { hdmi_in_vp0: endpoint@0 { reg = <0>; remote-endpoint = <&vp0_out_hdmi>; status = "disabled"; }; hdmi_in_vp1: endpoint@1 { reg = <1>; remote-endpoint = <&vp1_out_hdmi>; status = "disabled"; }; ... }; }; }; Theoretically every VP can be routed to every output, but depending on the board there are some constraints. For example for the three vps there are only two PLLs for the pixel clock, and the HDMI port is hardwired to one single PLL. To avoid different VPs setting conflicting rates on a PLL we can only allow a subset of the possible routes. Sascha
On Mittwoch, 17. November 2021 15:33:47 CET Sascha Hauer wrote: > The VOP2 unit is found on Rockchip SoCs beginning with rk3566/rk3568. > It replaces the VOP unit found in the older Rockchip SoCs. > > This driver has been derived from the downstream Rockchip Kernel and > heavily modified: > > - All nonstandard DRM properties have been removed > - dropped struct vop2_plane_state and pass around less data between > functions > - Dropped all DRM_FORMAT_* not known on upstream > - rework register access to get rid of excessively used macros > > The driver is tested with HDMI and MIPI-DSI display on a RK3568-EVB > board. Overlay support is tested with the modetest utility. AFBC support > is still present in the driver, but currently untested due to the lack > of suitable image sources. Also the driver has been tested with weston > using pixman and (yet to be upstreamed) panfrost driver support. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- Hi Sascha, thank you very much for your work on this! I gave it a try tonight, and unfortunately it appears to currently always attempt to use 1920x1080p60 as the mode regardless of the monitor. For example, on an old 720p monitor I had laying around: [ 225.732342] rockchip-drm display-subsystem: [drm] Update mode to 1920x1080p60, type: 11 for vp0, output 0x00000800 HDMI0 This results in a broken picture (all white with occasional glitches). Somebody else observed the same behaviour on a 1440p monitor. Thanks, Nicolas Frattaroli
On Wed, Nov 17, 2021 at 07:05:33PM +0100, Nicolas Frattaroli wrote: > On Mittwoch, 17. November 2021 15:33:47 CET Sascha Hauer wrote: > > The VOP2 unit is found on Rockchip SoCs beginning with rk3566/rk3568. > > It replaces the VOP unit found in the older Rockchip SoCs. > > > > This driver has been derived from the downstream Rockchip Kernel and > > heavily modified: > > > > - All nonstandard DRM properties have been removed > > - dropped struct vop2_plane_state and pass around less data between > > functions > > - Dropped all DRM_FORMAT_* not known on upstream > > - rework register access to get rid of excessively used macros > > > > The driver is tested with HDMI and MIPI-DSI display on a RK3568-EVB > > board. Overlay support is tested with the modetest utility. AFBC support > > is still present in the driver, but currently untested due to the lack > > of suitable image sources. Also the driver has been tested with weston > > using pixman and (yet to be upstreamed) panfrost driver support. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > Hi Sascha, > > thank you very much for your work on this! I gave it a try tonight, > and unfortunately it appears to currently always attempt to use > 1920x1080p60 as the mode regardless of the monitor. For example, > on an old 720p monitor I had laying around: > > [ 225.732342] rockchip-drm display-subsystem: [drm] Update mode to 1920x1080p60, type: 11 for vp0, output 0x00000800 HDMI0 > > This results in a broken picture (all white with occasional glitches). > Somebody else observed the same behaviour on a 1440p monitor. Unfortunately all my monitors I have here have exactly 1920x1080p60, so I didn't notice. Anyway, when I try another mode like 1280x1024 which should be supported as well then my monitor responds with "out of range", so something is indeed fishy here. I'll have a look into it. Sascha
Hi Sascha Hauer, On 2021/11/17 下午10:33, Sascha Hauer wrote: > This series adds initial graphics support for the Rockchip RK356[68] > SoCs. Graphics support is based around the VOP2 controller which > replaces the VOP controller found on earlier Rockchip SoCs. The driver > has been tested with HDMI support included in this series and MIPI-DSI > which is not included because it needs some more work. The driver is > taken from the downstream Rockchip kernel Yes, you do know this is from Rockchip kernel. Could you point me out where is the information about original author in your commit? > and heavily polished, most non > standard features have been removed for now. I don't agree with this, we do believe you have do some clean up to meet the requirement of upstream, but all the framework and feature implement are from Rockchip engineer, we have made a great effort to make everything work which block us to upstream this driver for now. NAK for this series. - Kever
Hi Kever, Am Donnerstag, 18. November 2021, 02:27:10 CET schrieb Kever Yang: > Hi Sascha Hauer, > > On 2021/11/17 下午10:33, Sascha Hauer wrote: > > This series adds initial graphics support for the Rockchip RK356[68] > > SoCs. Graphics support is based around the VOP2 controller which > > replaces the VOP controller found on earlier Rockchip SoCs. The driver > > has been tested with HDMI support included in this series and MIPI-DSI > > which is not included because it needs some more work. The driver is > > taken from the downstream Rockchip kernel > > Yes, you do know this is from Rockchip kernel. > > Could you point me out where is the information about original author > in your commit? The copyrights for added files seem to have stayed intact. For example the added rockchip_drm_vop2.c file in patch12 does contain the copyright as Copyright (c) 2020 Rockchip Electronics Co., Ltd. Author: Andy Yan <andy.yan@rock-chips.com> We can of course debate if the commit-author should also be set to Andy or another Rockchip engineer, with Sascha adding a "Co-developed-by" with his credentials. That's probably a nice compromise, I guess. > > and heavily polished, most non > > standard features have been removed for now. > > I don't agree with this, we do believe you have do some clean up to meet > the requirement > > of upstream, but all the framework and feature implement are from > Rockchip engineer, > > we have made a great effort to make everything work which block us to > upstream this driver for now. I don't fully understand what you mean here (language barrier probably), but dropping non-essential functionality in a first round is pretty common to at least get basic functionality working for everyone. With the special features getting added again in later patches over time. This happenened on the old vop as well. And of course, having a kernel that can "just" do normal graphics without the additional features is still preferable over having _NO_ graphics support at all ;-) > NAK for this series. As you might've seen from previous graphics related patches, there is a big number of people _and companies_ that seems to want/need to work with the rk3566/rk3568 with a kernel based on mainline. --> Most likely even in real products! While Rockchip did say that they want to upstream VOP2 support, there has been _NO_ movement or even information at all on this over at least the last year(!), so it's pretty understandable that developers will do this themself at some point, because they don't want to wait anymore for something that might never happen. So a simple "NAK" without additional information is not really helpful here. If you don't like Sascha's series, I really want to know _WHEN_ Rockchip plans on upstreaming at least basic graphis support themself. The kernel is often called a do-ocracy - the one who does the work, gets to decide. So if you really don't like Sascha's series at all, I do expect Rockchip to step up and provide a solution themself - and in a usable timeframe. Heiko
Hi, On Thu, 18 Nov 2021 at 09:26, Heiko Stübner <heiko@sntech.de> wrote: > Am Donnerstag, 18. November 2021, 02:27:10 CET schrieb Kever Yang: > > I don't agree with this, we do believe you have do some clean up to meet > > the requirement > > > > of upstream, but all the framework and feature implement are from > > Rockchip engineer, > > > > we have made a great effort to make everything work which block us to > > upstream this driver for now. > > I don't fully understand what you mean here (language barrier probably), > but dropping non-essential functionality in a first round is pretty common > to at least get basic functionality working for everyone. With the special > features getting added again in later patches over time. This happenened > on the old vop as well. > > And of course, having a kernel that can "just" do normal graphics without > the additional features is still preferable over having _NO_ graphics support > at all ;-) > > > NAK for this series. > > As you might've seen from previous graphics related patches, there > is a big number of people _and companies_ that seems to want/need > to work with the rk3566/rk3568 with a kernel based on mainline. > > --> Most likely even in real products! Yes, we've been trying to ship a real product based on RK356x. We started by using the vendor VOP2 driver, but it is broken beyond belief. The driver needs a fundamental ground-up rework, and all the additional features get in the way of doing this core rework to make it actually function correctly. So, NAK to the NAK. I would like to see the VOP2 support start simple, with more features being added one by one. > While Rockchip did say that they want to upstream VOP2 support, there > has been _NO_ movement or even information at all on this over at least > the last year(!), so it's pretty understandable that developers will do this > themself at some point, because they don't want to wait anymore for > something that might never happen. > > So a simple "NAK" without additional information is not really helpful here. > > If you don't like Sascha's series, I really want to know _WHEN_ Rockchip > plans on upstreaming at least basic graphis support themself. > > The kernel is often called a do-ocracy - the one who does the work, gets > to decide. So if you really don't like Sascha's series at all, I do expect > Rockchip to step up and provide a solution themself - and in a usable > timeframe. Exactly what Heiko said. If you would like to upstream the driver then that would be fantastic to see, but I'm afraid you do not get to prevent someone else from doing the work themselves. Cheers, Daniel
On Thu, Nov 18, 2021 at 10:26:29AM +0100, Heiko Stübner wrote: > Hi Kever, > > Am Donnerstag, 18. November 2021, 02:27:10 CET schrieb Kever Yang: > > Hi Sascha Hauer, > > > > On 2021/11/17 下午10:33, Sascha Hauer wrote: > > > This series adds initial graphics support for the Rockchip RK356[68] > > > SoCs. Graphics support is based around the VOP2 controller which > > > replaces the VOP controller found on earlier Rockchip SoCs. The driver > > > has been tested with HDMI support included in this series and MIPI-DSI > > > which is not included because it needs some more work. The driver is > > > taken from the downstream Rockchip kernel > > > > Yes, you do know this is from Rockchip kernel. > > > > Could you point me out where is the information about original author > > in your commit? > > The copyrights for added files seem to have stayed intact. > For example the added rockchip_drm_vop2.c file in patch12 > does contain the copyright as > > Copyright (c) 2020 Rockchip Electronics Co., Ltd. > Author: Andy Yan <andy.yan@rock-chips.com> > > > We can of course debate if the commit-author should also be set to > Andy or another Rockchip engineer, with Sascha adding a > "Co-developed-by" with his credentials. > > That's probably a nice compromise, I guess. The commit author was merely lost while copying the driver file into my git. Changing it back to Andy is fine with me, I didn't mean to steal anyones credits. Sascha
On 2021/11/18 下午5:53, Daniel Stone wrote: > Hi, > > On Thu, 18 Nov 2021 at 09:26, Heiko Stübner <heiko@sntech.de> wrote: >> Am Donnerstag, 18. November 2021, 02:27:10 CET schrieb Kever Yang: >>> I don't agree with this, we do believe you have do some clean up to meet >>> the requirement >>> >>> of upstream, but all the framework and feature implement are from >>> Rockchip engineer, >>> >>> we have made a great effort to make everything work which block us to >>> upstream this driver for now. >> I don't fully understand what you mean here (language barrier probably), >> but dropping non-essential functionality in a first round is pretty common >> to at least get basic functionality working for everyone. With the special >> features getting added again in later patches over time. This happenened >> on the old vop as well. >> >> And of course, having a kernel that can "just" do normal graphics without >> the additional features is still preferable over having _NO_ graphics support >> at all ;-) >> >>> NAK for this series. >> As you might've seen from previous graphics related patches, there >> is a big number of people _and companies_ that seems to want/need >> to work with the rk3566/rk3568 with a kernel based on mainline. >> >> --> Most likely even in real products! > Yes, we've been trying to ship a real product based on RK356x. We > started by using the vendor VOP2 driver, but it is broken beyond > belief. The driver needs a fundamental ground-up rework, and all the > additional features get in the way of doing this core rework to make > it actually function correctly. > > So, NAK to the NAK. I would like to see the VOP2 support start simple, > with more features being added one by one. > >> While Rockchip did say that they want to upstream VOP2 support, there >> has been _NO_ movement or even information at all on this over at least >> the last year(!), so it's pretty understandable that developers will do this >> themself at some point, because they don't want to wait anymore for >> something that might never happen. >> >> So a simple "NAK" without additional information is not really helpful here. >> >> If you don't like Sascha's series, I really want to know _WHEN_ Rockchip >> plans on upstreaming at least basic graphis support themself. >> >> The kernel is often called a do-ocracy - the one who does the work, gets >> to decide. So if you really don't like Sascha's series at all, I do expect >> Rockchip to step up and provide a solution themself - and in a usable >> timeframe. > Exactly what Heiko said. If you would like to upstream the driver then > that would be fantastic to see, but I'm afraid you do not get to > prevent someone else from doing the work themselves. First of all, we never stop any one to doing there work on upstream if the source code is write totally by themselves. Second, there are also many modules are upstream by developers based on Rockchip source code, please note that all of them have basic respect to our work, they do communicate with us first. But this committer do not take any respect to our engineers and their hard working: - He didn't contact with us; - There isn't any information about original author in the commit message; As I have known, if I use source code from another developer, I need to at least add a "Signed-off-by" with original author; - This commit and mail does not even have a 'CC' to original author. I NAK because I think this is not part of the open source spirit, and this kind of behavior should not be encouraged. Thanks, - Kever
Hello Kever, On 11/18/21 11:50 AM, Kever Yang wrote: > > On 2021/11/18 下午5:53, Daniel Stone wrote: >> Hi, >> >> On Thu, 18 Nov 2021 at 09:26, Heiko Stübner <heiko@sntech.de> wrote: >>> Am Donnerstag, 18. November 2021, 02:27:10 CET schrieb Kever Yang: >>>> I don't agree with this, we do believe you have do some clean up to >>>> meet >>>> the requirement >>>> >>>> of upstream, but all the framework and feature implement are from >>>> Rockchip engineer, >>>> >>>> we have made a great effort to make everything work which block us to >>>> upstream this driver for now. >>> I don't fully understand what you mean here (language barrier probably), >>> but dropping non-essential functionality in a first round is pretty >>> common >>> to at least get basic functionality working for everyone. With the >>> special >>> features getting added again in later patches over time. This happenened >>> on the old vop as well. >>> >>> And of course, having a kernel that can "just" do normal graphics >>> without >>> the additional features is still preferable over having _NO_ graphics >>> support >>> at all ;-) >>> >>>> NAK for this series. >>> As you might've seen from previous graphics related patches, there >>> is a big number of people _and companies_ that seems to want/need >>> to work with the rk3566/rk3568 with a kernel based on mainline. >>> >>> --> Most likely even in real products! >> Yes, we've been trying to ship a real product based on RK356x. We >> started by using the vendor VOP2 driver, but it is broken beyond >> belief. The driver needs a fundamental ground-up rework, and all the >> additional features get in the way of doing this core rework to make >> it actually function correctly. >> >> So, NAK to the NAK. I would like to see the VOP2 support start simple, >> with more features being added one by one. >> >>> While Rockchip did say that they want to upstream VOP2 support, there >>> has been _NO_ movement or even information at all on this over at least >>> the last year(!), so it's pretty understandable that developers will >>> do this >>> themself at some point, because they don't want to wait anymore for >>> something that might never happen. >>> >>> So a simple "NAK" without additional information is not really >>> helpful here. >>> >>> If you don't like Sascha's series, I really want to know _WHEN_ Rockchip >>> plans on upstreaming at least basic graphis support themself. >>> >>> The kernel is often called a do-ocracy - the one who does the work, gets >>> to decide. So if you really don't like Sascha's series at all, I do >>> expect >>> Rockchip to step up and provide a solution themself - and in a usable >>> timeframe. >> Exactly what Heiko said. If you would like to upstream the driver then >> that would be fantastic to see, but I'm afraid you do not get to >> prevent someone else from doing the work themselves. > > First of all, we never stop any one to doing there work on upstream if > the source code is write totally by themselves. > > Second, there are also many modules are upstream by developers based on > Rockchip source code, please note that > all of them have basic respect to our work, they do communicate with us > first. > > > But this committer do not take any respect to our engineers and their > hard working: > - He didn't contact with us; I approached Andy Yan and you off-list on October 20, 2021 in this regard, as Andy mentioned on linux-rockchip in July 2021 some plans to bring the driver mainline. Since there was no response, we asked Sascha to make this happen. > - There isn't any information about original author in the commit message; > As I have known, if I use source code from another developer, I > need to at least add a "Signed-off-by" with original author; As has been discussed before, this will be fixed in v2. Simple mistake, no harm intended. > - This commit and mail does not even have a 'CC' to original author. > > I NAK because I think this is not part of the open source spirit, and > this kind of behavior should not be encouraged. It is great to hear that you care about the open source spirit. IMHO communication is a big part thereof. If Rockchip would communicate better their plans to bring things mainline including a time schedule, it would be a lot easier for all of us. Best regards, Michael
Hi Kever, On Thu, 18 Nov 2021 at 10:50, Kever Yang <kever.yang@rock-chips.com> wrote: > On 2021/11/18 下午5:53, Daniel Stone wrote: > > Exactly what Heiko said. If you would like to upstream the driver then > > that would be fantastic to see, but I'm afraid you do not get to > > prevent someone else from doing the work themselves. > > First of all, we never stop any one to doing there work on upstream if > the source code is write totally by themselves. > > Second, there are also many modules are upstream by developers based on > Rockchip source code, please note that > all of them have basic respect to our work, they do communicate with us > first. > > But this committer do not take any respect to our engineers and their > hard working: > - He didn't contact with us; > - There isn't any information about original author in the commit message; > As I have known, if I use source code from another developer, I > need to at least add a "Signed-off-by" with original author; > - This commit and mail does not even have a 'CC' to original author. > > I NAK because I think this is not part of the open source spirit, and > this kind of behavior should not be encouraged. OK, I see where you're coming from, and I agree that the attribution should have been handled more carefully. On the other hand, please consider this from the other perspective. Sascha has been free to take the downstream Rockchip BSP code and attempt to upstream it to the Linux kernel, which you are unhappy about. But then the Rockchip driver was developed totally downstream, with no attempt to ever communicate with the upstream Linux or DRM/KMS developers. Rockchip advertises that it is shipped as a Linux kernel with a KMS driver. But we were never informed, or CCed, or anything. If you would like the community to more actively work with you - then please yourself work more actively with the community. The first commit of the VOP2 driver was in July 2020, and that was of the full driver so presumably it started quite some time before then. So that is a minimum of 17 months that you have had to engage with upstream ... Technically, the driver cannot be upstreamed as-is. It looks as if it were a pre-atomic driver, that was half-converted to atomic, and then has been half-converted to atomic helpers as well. Things like reference counting and global state are not handled correctly at all. You can see this if you try to run Weston on top of the VOP2 driver: the framerate is decimated because the event handling massively over-synchronises, and the event timestamps which arrive are incorrect. This would be fixed by correctly using the event helpers that we have had in the tree for years (which would also eliminate the unnecessary framebuffer reference handling). It also does not work with the GPU drivers in the tree because it lacks the one-liner to correctly handle dma_resv synchronisation, which makes it both too fast as it displays content which is not ready, and too slow because it can't do it at full frame rate. Similarly, on the RK3566 EVB, the DSI does not work unless HDMI is also active, but when HDMI is active at the same time as DSI, it just shows a blank screen. I believe the root cause of this is that the VOP2 driver does not use any of the atomic state correctly, and instead stores its own state in driver-global structures, using a lot of unnecessary mutexes to try to synchronise this state. Not only does this synchronisation not actually work, but it causes a severe performance degradation due to mutex contention. I believe the best path forward to an upstream VOP2 driver is a patch series consisting of: - start from a blank slate, using the atomic framework and helpers as they were intended to be, with basic support for the VOP2 and one or two connector types, doing linear XRGB only - any cleanups which would enable this to share more code with - add YUV support, including planar buffers - add AFBC support, with the AFBC enable/disable correctly synchronised through atomic state (this is necessary since the AFBC decoder is not directly on the planes per se but shared) - add more connector types - add writeback support - add other Rockchip-specific codepaths such as HDR10 Cheers, Daniel
Hi Daniel: On 11/18/21 8:07 PM, Daniel Stone wrote: > Hi Kever, > > On Thu, 18 Nov 2021 at 10:50, Kever Yang <kever.yang@rock-chips.com> wrote: >> On 2021/11/18 下午5:53, Daniel Stone wrote: >>> Exactly what Heiko said. If you would like to upstream the driver then >>> that would be fantastic to see, but I'm afraid you do not get to >>> prevent someone else from doing the work themselves. >> First of all, we never stop any one to doing there work on upstream if >> the source code is write totally by themselves. >> >> Second, there are also many modules are upstream by developers based on >> Rockchip source code, please note that >> all of them have basic respect to our work, they do communicate with us >> first. >> >> But this committer do not take any respect to our engineers and their >> hard working: >> - He didn't contact with us; >> - There isn't any information about original author in the commit message; >> As I have known, if I use source code from another developer, I >> need to at least add a "Signed-off-by" with original author; >> - This commit and mail does not even have a 'CC' to original author. >> >> I NAK because I think this is not part of the open source spirit, and >> this kind of behavior should not be encouraged. > OK, I see where you're coming from, and I agree that the attribution > should have been handled more carefully. > > On the other hand, please consider this from the other perspective. > Sascha has been free to take the downstream Rockchip BSP code and > attempt to upstream it to the Linux kernel, which you are unhappy > about. But then the Rockchip driver was developed totally downstream, > with no attempt to ever communicate with the upstream Linux or DRM/KMS > developers. Rockchip advertises that it is shipped as a Linux kernel > with a KMS driver. But we were never informed, or CCed, or anything. > > If you would like the community to more actively work with you - then > please yourself work more actively with the community. The first > commit of the VOP2 driver was in July 2020, and that was of the full > driver so presumably it started quite some time before then. So that > is a minimum of 17 months that you have had to engage with upstream > ... > > Technically, the driver cannot be upstreamed as-is. It looks as if it > were a pre-atomic driver, that was half-converted to atomic, and then > has been half-converted to atomic helpers as well. Things like > reference counting and global state are not handled correctly at all. > You can see this if you try to run Weston on top of the VOP2 driver: > the framerate is decimated because the event handling massively > over-synchronises, and the event timestamps which arrive are > incorrect. This would be fixed by correctly using the event helpers > that we have had in the tree for years (which would also eliminate the > unnecessary framebuffer reference handling). It also does not work > with the GPU drivers in the tree because it lacks the one-liner to > correctly handle dma_resv synchronisation, which makes it both too > fast as it displays content which is not ready, and too slow because > it can't do it at full frame rate. We have different team run Android , X11, Weston on rk356x, especially for android, we can run at 60 fps. Our vop2 driver is developed on Linux 4.19, am not sure which version of kernel you put our drivers on. > > Similarly, on the RK3566 EVB, the DSI does not work unless HDMI is > also active, but when HDMI is active at the same time as DSI, it just I am very sure rk3566 evb DSI can work without HDMI. But take care that the vop on rk3566 has a special limitation: there are three windows(Cluster1/Esmart1/Smart1) that have a mirror lock, that means they can't be programed framebuffer address independently , they can only share framebuffer address with Cluster0/Esmart0/Smart0. We use these feature on Android. I have comment these limitation in our driver. Compared to old vop, vop is strong but a bit complicated, we try very had to make it work on as much display framework as possible. We have upstream plane, but I am really in a rush this year. So sorry for the late of upstream, but we glad to work with community. So Sascha, please feel free to go on with your work. > shows a blank screen. I believe the root cause of this is that the > VOP2 driver does not use any of the atomic state correctly, and > instead stores its own state in driver-global structures, using a lot > of unnecessary mutexes to try to synchronise this state. Not only does > this synchronisation not actually work, but it causes a severe > performance degradation due to mutex contention. > > I believe the best path forward to an upstream VOP2 driver is a patch > series consisting of: > - start from a blank slate, using the atomic framework and helpers > as they were intended to be, with basic support for the VOP2 and one > or two connector types, doing linear XRGB only > - any cleanups which would enable this to share more code with > - add YUV support, including planar buffers > - add AFBC support, with the AFBC enable/disable correctly > synchronised through atomic state (this is necessary since the AFBC > decoder is not directly on the planes per se but shared) > - add more connector types > - add writeback support > - add other Rockchip-specific codepaths such as HDR10 > > Cheers, > Daniel > > >
Hi Andy, On Thu, 18 Nov 2021 at 13:14, Andy Yan <andy.yan@rock-chips.com> wrote: > On 11/18/21 8:07 PM, Daniel Stone wrote: > > Technically, the driver cannot be upstreamed as-is. It looks as if it > > were a pre-atomic driver, that was half-converted to atomic, and then > > has been half-converted to atomic helpers as well. Things like > > reference counting and global state are not handled correctly at all. > > You can see this if you try to run Weston on top of the VOP2 driver: > > the framerate is decimated because the event handling massively > > over-synchronises, and the event timestamps which arrive are > > incorrect. This would be fixed by correctly using the event helpers > > that we have had in the tree for years (which would also eliminate the > > unnecessary framebuffer reference handling). It also does not work > > with the GPU drivers in the tree because it lacks the one-liner to > > correctly handle dma_resv synchronisation, which makes it both too > > fast as it displays content which is not ready, and too slow because > > it can't do it at full frame rate. > > We have different team run Android , X11, Weston on rk356x, especially > for android, we can run at 60 fps. > > Our vop2 driver is developed on Linux 4.19, am not sure which version of > kernel you put our drivers on. We forward-ported it to a current mainline kernel and started to work on fixing some of the issues. When we did this, we went back to the BSP tree posted on GitHub to test using a pure-BSP environment, and observed the same breakage there. > > Similarly, on the RK3566 EVB, the DSI does not work unless HDMI is > > also active, but when HDMI is active at the same time as DSI, it just > > I am very sure rk3566 evb DSI can work without HDMI. I'd love to know how. :) Using the meta-rockchip layer as published on GitHub, we cannot get working DSI without HDMI, using upstream Weston. When the HDMI connector is disabled, DSI comes up blank. When the HDMI connector is enabled, DSI works fine but HDMI is blank. > But take care that the vop on rk3566 has a special limitation: there are > three windows(Cluster1/Esmart1/Smart1) that have a mirror lock, that means they > can't be programed framebuffer address independently , they can only > share framebuffer address with Cluster0/Esmart0/Smart0. We use these feature > on Android. > > I have comment these limitation in our driver. Yeah, we noticed this. > Compared to old vop, vop is strong but a bit complicated, we try very had to > make it work on as much display framework as possible. > > We have upstream plane, but I am really in a rush this year. So sorry > for the late of upstream, but we glad to work with community. > > So Sascha, please feel free to go on with your work. Great, thanks. If you agree with the plan I posted, then we can all go forward with that upstream, doing the development work with ourselves and Sascha. Hopefully Rockchip will be able to support this effort. Cheers, Daniel
Hi Sascha, Am 17.11.21 um 15:33 schrieb Sascha Hauer: > This series adds initial graphics support for the Rockchip RK356[68] > SoCs. Graphics support is based around the VOP2 controller which > replaces the VOP controller found on earlier Rockchip SoCs. The driver > has been tested with HDMI support included in this series and MIPI-DSI > which is not included because it needs some more work. The driver is > taken from the downstream Rockchip kernel and heavily polished, most non > standard features have been removed for now. I tested the driver with > the libdrm modetest utility and also with weston with both pixman and > panfrost driver support. Michael Riesch reported the driver to work on > the RK3566 as well, but device tree support for this SoC is not yet > included in this series. > > The HDMI changes are based on patches from Benjamin Gaignard, but > modified a bit as I found out that the HDMI port on the RK3568 only > needs one additional clock, not two. Also I added regulator support > which is needed to get the HDMI up on the rk3568-EVB board. > > All review and testing feedback welcome thanks for working on that - it's very (very,very) much appreciated. It took me some time to figure it out: It seems rk3568-iommu driver s broken - I did only get "white noise" when using it alongside vop (similar like it was reported here before). However: removing the iommu-property from vop makes it working for me with HDMI output on quartz64 as well. Could you check if you have the iommu driver in kernel enabled if it works for you, if the property is present in DT? (I used 5.16-rc1 + this series + [0]). Also vop mmu seems to have the power-domain missing in your series (same as downstream) - however adding that doesn't help much currently. As a sidenote: I verfied this with using Ezequiel's vpu addtion for RK356x: It did only work when removing the iommu there as well (getting tons of page faults otherwise) - so iommu driver really seems to broken, at least for RK3566. (Or I'm a missing a option in kernel config, which wasn't required for the older iommu version?) But as reported before: For HDMI this does currently only work for pixel clock rates, which are integer-divisable with hpll clock rate (which is the hardcoded parent of vop0's dclk) As discussed in Benjamin's initial submission of the addition of RK3568's hdmi controller [1] same as with RK3288's and RK3399's hdmi phy needs a reference clock (it's called vpll there) which needs to get switched before the vop switches the mode (since phy rate switching is done before) - it's HPLL in case of RK356x. For whatever reason it's called "ref" for RK356x only downstream [2] - so you should add another clock "vpll" (renaming it to "ref" for _ALL_ SoCs which have it would be a _GREAT_ idea) which is <&pmucru PLL_HPLL>. What brings us to the "real" clock problem and the reason, why non-integer divisable pixel clock rates are not possible ATM: This is a long standing issue for RK3288 and RK3399 as well (and one of the main reasons why 4k modes are not possible for those older SoCs currently): Upstream all PLL rates are controlled with those PLL rate tables in the clock driver and they have to be _exactly_ defined as they are used (HDMI sinks are very picky). You will not see any additional rates downstream for RK3568: they have a mechanism there to automatically calculate the PLL settings if the rate doesn't exist in these tables (IIRC this was submitted upstream also: but it was rejected/ignored by maintainers). As a quick hackarround (for testing): You could use this table [3] we are using in LibreElec for RK3399 to get 4k modes working and assign it to HPLL in RK3568's clock driver (I tested it and it works great). It might be possible to just add those rates (some also without frac dividers) to the common PLL table for RK3568. I'm sorry I didn't reply inline as I'm supposed to do: It's late and I wanted to offload my findings now :) (You probably should also remove the printks in V2) Best, Alex [0] https://patchwork.kernel.org/project/linux-rockchip/patch/20211117154429.2274443-1-michael.riesch@wolfvision.net/ [1] https://patchwork.kernel.org/comment/24295683/ [2] https://github.com/rockchip-linux/kernel/blob/develop-4.19/arch/arm64/boot/dts/rockchip/rk3568.dtsi#L1715-L1720 [3] https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/patches/linux/default/linux-1000-drm-rockchip.patch#L3155-L3182
Hi Alex, On Mon, Nov 22, 2021 at 12:18:47AM +0100, Alex Bee wrote: > Hi Sascha, > > Am 17.11.21 um 15:33 schrieb Sascha Hauer: > > This series adds initial graphics support for the Rockchip RK356[68] > > SoCs. Graphics support is based around the VOP2 controller which > > replaces the VOP controller found on earlier Rockchip SoCs. The driver > > has been tested with HDMI support included in this series and MIPI-DSI > > which is not included because it needs some more work. The driver is > > taken from the downstream Rockchip kernel and heavily polished, most non > > standard features have been removed for now. I tested the driver with > > the libdrm modetest utility and also with weston with both pixman and > > panfrost driver support. Michael Riesch reported the driver to work on > > the RK3566 as well, but device tree support for this SoC is not yet > > included in this series. > > > > The HDMI changes are based on patches from Benjamin Gaignard, but > > modified a bit as I found out that the HDMI port on the RK3568 only > > needs one additional clock, not two. Also I added regulator support > > which is needed to get the HDMI up on the rk3568-EVB board. > > > > All review and testing feedback welcome > > > thanks for working on that - it's very (very,very) much appreciated. > > It took me some time to figure it out: It seems rk3568-iommu driver s > broken - I did only get "white noise" when using it alongside vop > (similar like it was reported here before). However: removing the > iommu-property from vop makes it working for me with HDMI output on > quartz64 as well. Could you check if you have the iommu driver in kernel > enabled if it works for you, if the property is present in DT? (I used > 5.16-rc1 + this series + [0]). I have the iommu driver enabled and it works for me. I get this during boot: [0.263287] rockchip-vop2 fe040000.vop: Adding to iommu group 0 So I expect it is indeed used. > Also vop mmu seems to have the > power-domain missing in your series (same as downstream) - however > adding that doesn't help much currently. Probably the power domain gets enabled anyway when the VOP is activated, so adding it to the iommu won't help anything. Nevertheless it seems correct to add the property, I'll do so in the next round. > As a sidenote: I verfied this with using Ezequiel's vpu addtion for > RK356x: It did only work when removing the iommu there as well (getting > tons of page faults otherwise) - so iommu driver really seems to broken, > at least for RK3566. (Or I'm a missing a option in kernel config, which > wasn't required for the older iommu version?) I don't think so. I started from defconfig and disabled other architectures and unneeded drivers, but I did not enable anything specific to iommu. > > But as reported before: For HDMI this does currently only work for pixel > clock rates, which are integer-divisable with hpll clock rate (which is > the hardcoded parent of vop0's dclk) > As discussed in Benjamin's initial submission of the addition of > RK3568's hdmi controller [1] same as with RK3288's and RK3399's hdmi phy > needs a reference clock (it's called vpll there) which needs to get > switched before the vop switches the mode (since phy rate switching is > done before) - it's HPLL in case of RK356x. For whatever reason it's > called "ref" for RK356x only downstream [2] - so you should add another > clock "vpll" (renaming it to "ref" for _ALL_ SoCs which have it would be > a _GREAT_ idea) which is <&pmucru PLL_HPLL>. Yeah, a consumer clock should be named after the usage in the consumer, not after the provider name. I also stumbled over this and naming it "ref" makes much more sense. We'll likely have to keep supporting "vpll" as well for compatibility to old device trees. > What brings us to the "real" clock problem and the reason, why > non-integer divisable pixel clock rates are not possible ATM: This is a > long standing issue for RK3288 and RK3399 as well (and one of the main > reasons why 4k modes are not possible for those older SoCs currently): > Upstream all PLL rates are controlled with those PLL rate tables in the > clock driver and they have to be _exactly_ defined as they are used > (HDMI sinks are very picky). > You will not see any additional rates downstream for RK3568: they have a > mechanism there to automatically calculate the PLL settings if the rate > doesn't exist in these tables (IIRC this was submitted upstream also: > but it was rejected/ignored by maintainers). Looks like we have to try harder to get it upstream. Do you have a pointer to this patch? > As a quick hackarround (for > testing): You could use this table [3] we are using in LibreElec for > RK3399 to get 4k modes working and assign it to HPLL in RK3568's clock > driver (I tested it and it works great). It might be possible to just > add those rates (some also without frac dividers) to the common PLL > table for RK3568. Thanks for noting. This could also explain why currently only 1080p is working. Sascha
Am 22.11.21 um 09:10 schrieb Sascha Hauer: > Hi Alex, > > On Mon, Nov 22, 2021 at 12:18:47AM +0100, Alex Bee wrote: >> Hi Sascha, >> >> Am 17.11.21 um 15:33 schrieb Sascha Hauer: >>> This series adds initial graphics support for the Rockchip RK356[68] >>> SoCs. Graphics support is based around the VOP2 controller which >>> replaces the VOP controller found on earlier Rockchip SoCs. The driver >>> has been tested with HDMI support included in this series and MIPI-DSI >>> which is not included because it needs some more work. The driver is >>> taken from the downstream Rockchip kernel and heavily polished, most non >>> standard features have been removed for now. I tested the driver with >>> the libdrm modetest utility and also with weston with both pixman and >>> panfrost driver support. Michael Riesch reported the driver to work on >>> the RK3566 as well, but device tree support for this SoC is not yet >>> included in this series. >>> >>> The HDMI changes are based on patches from Benjamin Gaignard, but >>> modified a bit as I found out that the HDMI port on the RK3568 only >>> needs one additional clock, not two. Also I added regulator support >>> which is needed to get the HDMI up on the rk3568-EVB board. >>> >>> All review and testing feedback welcome >> >> thanks for working on that - it's very (very,very) much appreciated. >> >> It took me some time to figure it out: It seems rk3568-iommu driver s >> broken - I did only get "white noise" when using it alongside vop >> (similar like it was reported here before). However: removing the >> iommu-property from vop makes it working for me with HDMI output on >> quartz64 as well. Could you check if you have the iommu driver in kernel >> enabled if it works for you, if the property is present in DT? (I used >> 5.16-rc1 + this series + [0]). > I have the iommu driver enabled and it works for me. I get this during > boot: > > [0.263287] rockchip-vop2 fe040000.vop: Adding to iommu group 0 > > So I expect it is indeed used. > >> Also vop mmu seems to have the >> power-domain missing in your series (same as downstream) - however >> adding that doesn't help much currently. > Probably the power domain gets enabled anyway when the VOP is activated, > so adding it to the iommu won't help anything. Nevertheless it seems > correct to add the property, I'll do so in the next round. > >> As a sidenote: I verfied this with using Ezequiel's vpu addtion for >> RK356x: It did only work when removing the iommu there as well (getting >> tons of page faults otherwise) - so iommu driver really seems to broken, >> at least for RK3566. (Or I'm a missing a option in kernel config, which >> wasn't required for the older iommu version?) > I don't think so. I started from defconfig and disabled other > architectures and unneeded drivers, but I did not enable anything > specific to iommu. I've found out now that I can make it work with iommu, by limiting the available memory to something below 4G (I have a 8G board). So there is something wrong in the driver or somewhere in memory mapping, iommu api (since it works when using CMA), ... however: it does clearly not relate to your patch. >> >> But as reported before: For HDMI this does currently only work for pixel >> clock rates, which are integer-divisable with hpll clock rate (which is >> the hardcoded parent of vop0's dclk) >> As discussed in Benjamin's initial submission of the addition of >> RK3568's hdmi controller [1] same as with RK3288's and RK3399's hdmi phy >> needs a reference clock (it's called vpll there) which needs to get >> switched before the vop switches the mode (since phy rate switching is >> done before) - it's HPLL in case of RK356x. For whatever reason it's >> called "ref" for RK356x only downstream [2] - so you should add another >> clock "vpll" (renaming it to "ref" for _ALL_ SoCs which have it would be >> a _GREAT_ idea) which is <&pmucru PLL_HPLL>. > Yeah, a consumer clock should be named after the usage in the consumer, > not after the provider name. I also stumbled over this and naming it > "ref" makes much more sense. We'll likely have to keep supporting "vpll" > as well for compatibility to old device trees. > >> What brings us to the "real" clock problem and the reason, why >> non-integer divisable pixel clock rates are not possible ATM: This is a >> long standing issue for RK3288 and RK3399 as well (and one of the main >> reasons why 4k modes are not possible for those older SoCs currently): >> Upstream all PLL rates are controlled with those PLL rate tables in the >> clock driver and they have to be _exactly_ defined as they are used >> (HDMI sinks are very picky). >> You will not see any additional rates downstream for RK3568: they have a >> mechanism there to automatically calculate the PLL settings if the rate >> doesn't exist in these tables (IIRC this was submitted upstream also: >> but it was rejected/ignored by maintainers). > Looks like we have to try harder to get it upstream. Do you have a > pointer to this patch? Sure: https://patchwork.kernel.org/project/linux-clk/patch/20191204082527.19957-1-zhangqing@rock-chips.com/ I don't know if that is the was last submitted version. Best regards, Alex >> As a quick hackarround (for >> testing): You could use this table [3] we are using in LibreElec for >> RK3399 to get 4k modes working and assign it to HPLL in RK3568's clock >> driver (I tested it and it works great). It might be possible to just >> add those rates (some also without frac dividers) to the common PLL >> table for RK3568. > Thanks for noting. This could also explain why currently only 1080p is > working. > > Sascha >
On 2021-11-22 17:47, Alex Bee wrote: > Am 22.11.21 um 09:10 schrieb Sascha Hauer: >> Hi Alex, >> >> On Mon, Nov 22, 2021 at 12:18:47AM +0100, Alex Bee wrote: >>> Hi Sascha, >>> >>> Am 17.11.21 um 15:33 schrieb Sascha Hauer: >>>> This series adds initial graphics support for the Rockchip RK356[68] >>>> SoCs. Graphics support is based around the VOP2 controller which >>>> replaces the VOP controller found on earlier Rockchip SoCs. The driver >>>> has been tested with HDMI support included in this series and MIPI-DSI >>>> which is not included because it needs some more work. The driver is >>>> taken from the downstream Rockchip kernel and heavily polished, most non >>>> standard features have been removed for now. I tested the driver with >>>> the libdrm modetest utility and also with weston with both pixman and >>>> panfrost driver support. Michael Riesch reported the driver to work on >>>> the RK3566 as well, but device tree support for this SoC is not yet >>>> included in this series. >>>> >>>> The HDMI changes are based on patches from Benjamin Gaignard, but >>>> modified a bit as I found out that the HDMI port on the RK3568 only >>>> needs one additional clock, not two. Also I added regulator support >>>> which is needed to get the HDMI up on the rk3568-EVB board. >>>> >>>> All review and testing feedback welcome >>> >>> thanks for working on that - it's very (very,very) much appreciated. >>> >>> It took me some time to figure it out: It seems rk3568-iommu driver s >>> broken - I did only get "white noise" when using it alongside vop >>> (similar like it was reported here before). However: removing the >>> iommu-property from vop makes it working for me with HDMI output on >>> quartz64 as well. Could you check if you have the iommu driver in kernel >>> enabled if it works for you, if the property is present in DT? (I used >>> 5.16-rc1 + this series + [0]). >> I have the iommu driver enabled and it works for me. I get this during >> boot: >> >> [0.263287] rockchip-vop2 fe040000.vop: Adding to iommu group 0 >> >> So I expect it is indeed used. >> >>> Also vop mmu seems to have the >>> power-domain missing in your series (same as downstream) - however >>> adding that doesn't help much currently. >> Probably the power domain gets enabled anyway when the VOP is activated, >> so adding it to the iommu won't help anything. Nevertheless it seems >> correct to add the property, I'll do so in the next round. >> >>> As a sidenote: I verfied this with using Ezequiel's vpu addtion for >>> RK356x: It did only work when removing the iommu there as well (getting >>> tons of page faults otherwise) - so iommu driver really seems to broken, >>> at least for RK3566. (Or I'm a missing a option in kernel config, which >>> wasn't required for the older iommu version?) >> I don't think so. I started from defconfig and disabled other >> architectures and unneeded drivers, but I did not enable anything >> specific to iommu. > > I've found out now that I can make it work with iommu, by limiting the > available memory to something below 4G (I have a 8G board). So there is > something wrong in the driver or somewhere in memory mapping, iommu api > (since it works when using CMA), ... however: it does clearly not relate > to your patch. FWIW it doesn't surprise me that there might still be bugs lurking in the IOMMU driver's relatively recent changes for packing 40-bit physical addresses into 32-bit pagetable entries and registers - that sort of thing is always tricky to get right. You're correct that that's something that wants debugging in its own right, though. Robin.
Hi Sascha, On 11/17/21 3:33 PM, Sascha Hauer wrote: > The VOP2 is the display output controller on the RK3568. Add the node > for it to the dtsi file along with the required display-subsystem node > and the iommu node. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > arch/arm64/boot/dts/rockchip/rk356x.dtsi | 52 ++++++++++++++++++++++++ > 1 file changed, 52 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > index 46d9552f60284..6ebf7c14e096a 100644 > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > @@ -447,6 +447,58 @@ gmac1_mtl_tx_setup: tx-queues-config { > }; > }; > > + display_subsystem: display-subsystem { > + compatible = "rockchip,display-subsystem"; > + ports = <&vop_out>; > + }; Some DT sort rules: For nodes: Sort things without reg alphabetical first, then sort the rest by reg address. > + > + vop: vop@fe040000 { > + compatible = "rockchip,rk3568-vop"; From rockchip-vop2.yaml: +properties: + compatible: + enum: + - rockchip,rk3568-vop + - rockchip,rk3566-vop Maybe sort yaml compatibles in alphabetical order. rockchip,rk3566-vop is not used in the dtsi I think. Comment by Andy Yan: > > But take care that the vop on rk3566 has a special limitation: there are > three > > windows(Cluster1/Esmart1/Smart1) that have a mirror lock, that means they > > can't be programed framebuffer address independently , they can only > > share framebuffer address with Cluster0/Esmart0/Smart0. Question: Given Andy's comment could someone explain weather the vop and hdmi nodes should be in rk3566.dtsi and rk3568.dtsi with an extra rockchip,rk3566-dw-hdmi compatible? > + reg = <0x0 0xfe040000 0x0 0x3000>, <0x0 0xfe044000 0x0 0x1000>; > + reg-names = "regs", "gamma_lut"; > + rockchip,grf = <&grf>; Heiko's sort rules: compatible reg interrupts [alphabetical] status [if needed] > + interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&cru ACLK_VOP>, <&cru HCLK_VOP>, <&cru DCLK_VOP0>, <&cru DCLK_VOP1>, <&cru DCLK_VOP2>; > + clock-names = "aclk_vop", "hclk_vop", "dclk_vp0", "dclk_vp1", "dclk_vp2"; > + iommus = <&vop_mmu>; > + power-domains = <&power RK3568_PD_VO>; > + status = "disabled"; > + > + vop_out: ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + vp0: port@0 { > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <0>; My incomplete list: Inside nodes: If exists on top: compatible, reg and interrupts. In alphabetical order the required properties. Then in alphabetical order the other properties. And as last things that start with '#' in alphabetical order. Add status below all other properties for soc internal components with any board-specifics. > + }; > + > + vp1: port@1 { > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <1>; > + }; > + > + vp2: port@2 { > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <2>; > + }; > + }; > + }; > + > + vop_mmu: iommu@fe043e00 { > + compatible = "rockchip,rk3568-iommu"; > + reg = <0x0 0xfe043e00 0x0 0x100>, <0x0 0xfe043f00 0x0 0x100>; > + interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "vop_mmu"; ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dt.yaml: iommu@fe043e00: 'interrupt-names' does not match any of the regexes: 'pinctrl-[0-9]+' From schema: /Documentation/devicetree/bindings/iommu/rockchip,iommu.yaml > + clocks = <&cru ACLK_VOP>, <&cru HCLK_VOP>; > + clock-names = "aclk", "iface"; > + #iommu-cells = <0>; > + status = "disabled"; > + }; > + > qos_gpu: qos@fe128000 { > compatible = "rockchip,rk3568-qos", "syscon"; > reg = <0x0 0xfe128000 0x0 0x20>; >
On Thu, Nov 25, 2021 at 09:25:28PM +0100, Johan Jonker wrote: > Hi Sascha, > > > On 11/17/21 3:33 PM, Sascha Hauer wrote: > > The VOP2 is the display output controller on the RK3568. Add the node > > for it to the dtsi file along with the required display-subsystem node > > and the iommu node. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > arch/arm64/boot/dts/rockchip/rk356x.dtsi | 52 ++++++++++++++++++++++++ > > 1 file changed, 52 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > index 46d9552f60284..6ebf7c14e096a 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > @@ -447,6 +447,58 @@ gmac1_mtl_tx_setup: tx-queues-config { > > }; > > }; > > > > > + display_subsystem: display-subsystem { > > + compatible = "rockchip,display-subsystem"; > > + ports = <&vop_out>; > > + }; > > Some DT sort rules: > > For nodes: > Sort things without reg alphabetical first, > then sort the rest by reg address. > > > + > > + vop: vop@fe040000 { > > > + compatible = "rockchip,rk3568-vop"; > > From rockchip-vop2.yaml: > +properties: > + compatible: > + enum: > > + - rockchip,rk3568-vop > + - rockchip,rk3566-vop > > Maybe sort yaml compatibles in alphabetical order. > > rockchip,rk3566-vop is not used in the dtsi I think. > > Comment by Andy Yan: > > > > But take care that the vop on rk3566 has a special limitation: there are > > three > > > > windows(Cluster1/Esmart1/Smart1) that have a mirror lock, that means they > > > > can't be programed framebuffer address independently , they can only > > > > share framebuffer address with Cluster0/Esmart0/Smart0. > > Question: > Given Andy's comment could someone explain weather the vop and hdmi > nodes should be in rk3566.dtsi and rk3568.dtsi with an extra > rockchip,rk3566-dw-hdmi compatible? We could put the vop/hdmi nodes into rk356x.dtsi and just add the compatible properties to rk3566.dtsi and rk3568.dtsi. We'll need the exact SoC type, besides the mirror lock thingy there are a few other minor differences between the SoCs. Sascha
Am Freitag, 26. November 2021, 08:40:21 CET schrieb Sascha Hauer: > On Thu, Nov 25, 2021 at 09:25:28PM +0100, Johan Jonker wrote: > > Hi Sascha, > > > > > > On 11/17/21 3:33 PM, Sascha Hauer wrote: > > > The VOP2 is the display output controller on the RK3568. Add the node > > > for it to the dtsi file along with the required display-subsystem node > > > and the iommu node. > > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > --- > > > arch/arm64/boot/dts/rockchip/rk356x.dtsi | 52 ++++++++++++++++++++++++ > > > 1 file changed, 52 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > > index 46d9552f60284..6ebf7c14e096a 100644 > > > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > > @@ -447,6 +447,58 @@ gmac1_mtl_tx_setup: tx-queues-config { > > > }; > > > }; > > > > > > > > + display_subsystem: display-subsystem { > > > + compatible = "rockchip,display-subsystem"; > > > + ports = <&vop_out>; > > > + }; > > > > Some DT sort rules: > > > > For nodes: > > Sort things without reg alphabetical first, > > then sort the rest by reg address. > > > > > + > > > + vop: vop@fe040000 { > > > > > + compatible = "rockchip,rk3568-vop"; > > > > From rockchip-vop2.yaml: > > +properties: > > + compatible: > > + enum: > > > > + - rockchip,rk3568-vop > > + - rockchip,rk3566-vop > > > > Maybe sort yaml compatibles in alphabetical order. > > > > rockchip,rk3566-vop is not used in the dtsi I think. > > > > Comment by Andy Yan: > > > > > > But take care that the vop on rk3566 has a special limitation: there are > > > three > > > > > > windows(Cluster1/Esmart1/Smart1) that have a mirror lock, that means they > > > > > > can't be programed framebuffer address independently , they can only > > > > > > share framebuffer address with Cluster0/Esmart0/Smart0. > > > > Question: > > Given Andy's comment could someone explain weather the vop and hdmi > > nodes should be in rk3566.dtsi and rk3568.dtsi with an extra > > rockchip,rk3566-dw-hdmi compatible? > > We could put the vop/hdmi nodes into rk356x.dtsi and just add the > compatible properties to rk3566.dtsi and rk3568.dtsi. sounds about right. We have similar solutions in place in other socs. Heiko
On Wed, Nov 17, 2021 at 09:13:33AM -0600, Rob Herring wrote: > On Wed, Nov 17, 2021 at 8:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > > Add support for the HDMI port found on RK3568. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > arch/arm64/boot/dts/rockchip/rk356x.dtsi | 65 ++++++++++++++++++++++++ > > 1 file changed, 65 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > index 6ebf7c14e096a..53be61a7ce595 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > @@ -472,18 +472,36 @@ vp0: port@0 { > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <0>; > > + > > + vp0_out_hdmi: endpoint@0 { > > + reg = <0>; > > + remote-endpoint = <&hdmi_in_vp0>; > > + status = "disabled"; > > + }; > > }; > > > > vp1: port@1 { > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <1>; > > + > > + vp1_out_hdmi: endpoint@0 { > > + reg = <0>; > > + remote-endpoint = <&hdmi_in_vp1>; > > + status = "disabled"; > > + }; > > }; > > > > vp2: port@2 { > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <2>; > > + > > + vp2_out_hdmi: endpoint@0 { > > + reg = <0>; > > + remote-endpoint = <&hdmi_in_vp2>; > > + status = "disabled"; > > + }; > > }; > > }; > > }; > > @@ -499,6 +517,53 @@ vop_mmu: iommu@fe043e00 { > > status = "disabled"; > > }; > > > > + hdmi: hdmi@fe0a0000 { > > + compatible = "rockchip,rk3568-dw-hdmi"; > > + reg = <0x0 0xfe0a0000 0x0 0x20000>; > > + interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&cru PCLK_HDMI_HOST>, > > + <&cru CLK_HDMI_SFR>, > > + <&cru CLK_HDMI_CEC>, > > + <&cru HCLK_VOP>; > > + clock-names = "iahb", "isfr", "cec", "hclk"; > > + power-domains = <&power RK3568_PD_VO>; > > + reg-io-width = <4>; > > + rockchip,grf = <&grf>; > > + #sound-dai-cells = <0>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&hdmitx_scl &hdmitx_sda &hdmitxm0_cec>; > > + status = "disabled"; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + hdmi_in: port@0 { > > > The schema says there is only 1 'port' node. Please run schema > validation when making DT changes. > > However, an HDMI bridge should also define an output port to a > connector node (or another bridge). So the fix is to allow 'port@0' > and add an output port. The rockchip bindings traditionally don't have a connector explicitly specified in their device trees. I'll stick to that for the next round. If necessary I'll look later what it takes to add a connector node. Sascha
On Wed, Nov 17, 2021 at 09:19:38AM -0600, Rob Herring wrote: > On Wed, Nov 17, 2021 at 8:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > > This enabled the VOP2 display controller along with hdmi and the > > required port routes which is enough to get a picture out of the > > hdmi port of the board. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > .../boot/dts/rockchip/rk3568-evb1-v10.dts | 24 +++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > > index 184e2aa2416af..156e001492173 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > > +++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > > @@ -106,6 +106,12 @@ &gmac1m1_rgmii_clk > > status = "okay"; > > }; > > > > +&hdmi { > > + status = "okay"; > > + avdd-0v9-supply = <&vdda0v9_image>; > > + avdd-1v8-supply = <&vcca1v8_image>; > > +}; > > + > > &i2c0 { > > status = "okay"; > > > > @@ -390,3 +396,21 @@ &sdmmc0 { > > &uart2 { > > status = "okay"; > > }; > > + > > +&vop { > > + status = "okay"; > > + assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>; > > + assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>; > > +}; > > + > > +&vop_mmu { > > + status = "okay"; > > +}; > > + > > +&hdmi_in_vp0 { > > + status = "okay"; > > +}; > > + > > +&vp0_out_hdmi { > > + status = "okay"; > > +}; > > You can accomplish the same thing already with: > > &vp0_out_hdmi { > remote-endpoint = <&hdmi_in_vp0>; > }; My idea was to describe all possible connections in the dtsi file and let the board dts writer only en/disable the needed connections. When the connections are specified in the dts file then writing it is more difficult and error prone. > > or: > > &vp0_out_hdmi { > /delete-property/ remote-endpoint; > }; With this I have to change all connections that I don't need. With status = "okay" I have to change all connections that I actually do need, which will be much easier to read and write. I'll stick to the status = "okay" method for the next round, maybe I can still convince you ;) If it's the 'status' property you don't like being used when it's not a device that is enabled/disabled, then every other name would be fine with me as well. Sascha
Hi Sascha, Am Donnerstag, 2. Dezember 2021, 16:34:49 CET schrieb Sascha Hauer: > On Wed, Nov 17, 2021 at 09:19:38AM -0600, Rob Herring wrote: > > On Wed, Nov 17, 2021 at 8:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > > > > This enabled the VOP2 display controller along with hdmi and the > > > required port routes which is enough to get a picture out of the > > > hdmi port of the board. > > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > --- > > > .../boot/dts/rockchip/rk3568-evb1-v10.dts | 24 +++++++++++++++++++ > > > 1 file changed, 24 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > > > index 184e2aa2416af..156e001492173 100644 > > > --- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > > > +++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > > > @@ -106,6 +106,12 @@ &gmac1m1_rgmii_clk > > > status = "okay"; > > > }; > > > > > > +&hdmi { > > > + status = "okay"; > > > + avdd-0v9-supply = <&vdda0v9_image>; > > > + avdd-1v8-supply = <&vcca1v8_image>; > > > +}; > > > + > > > &i2c0 { > > > status = "okay"; > > > > > > @@ -390,3 +396,21 @@ &sdmmc0 { > > > &uart2 { > > > status = "okay"; > > > }; > > > + > > > +&vop { > > > + status = "okay"; > > > + assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>; > > > + assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>; > > > +}; > > > + > > > +&vop_mmu { > > > + status = "okay"; > > > +}; > > > + > > > +&hdmi_in_vp0 { > > > + status = "okay"; > > > +}; > > > + > > > +&vp0_out_hdmi { > > > + status = "okay"; > > > +}; > > > > You can accomplish the same thing already with: > > > > &vp0_out_hdmi { > > remote-endpoint = <&hdmi_in_vp0>; > > }; > > My idea was to describe all possible connections in the dtsi file and > let the board dts writer only en/disable the needed connections. When > the connections are specified in the dts file then writing it is more > difficult and error prone. > > > > > or: > > > > &vp0_out_hdmi { > > /delete-property/ remote-endpoint; > > }; > > With this I have to change all connections that I don't need. With > status = "okay" I have to change all connections that I actually do > need, which will be much easier to read and write. > > I'll stick to the status = "okay" method for the next round, maybe I can > still convince you ;) > > If it's the 'status' property you don't like being used when it's not a > device that is enabled/disabled, then every other name would be fine > with me as well. hmm, we do have code in the rockchip drm-driver to find out if the device at the end of a graph-connection is disabled or not [0] , So on previous Rockchip socs, there are already all connections established, and the driver weeds out the disabled ones. So I'm wondering what is missing to use that in a vop2 context? Heiko [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rockchip/rockchip_drm_drv.c#n274
Hi Heiko, On Thu, Dec 02, 2021 at 04:41:17PM +0100, Heiko Stübner wrote: > Hi Sascha, > > Am Donnerstag, 2. Dezember 2021, 16:34:49 CET schrieb Sascha Hauer: > > On Wed, Nov 17, 2021 at 09:19:38AM -0600, Rob Herring wrote: > > > On Wed, Nov 17, 2021 at 8:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > > > > > > This enabled the VOP2 display controller along with hdmi and the > > > > required port routes which is enough to get a picture out of the > > > > hdmi port of the board. > > > > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > > --- > > > > .../boot/dts/rockchip/rk3568-evb1-v10.dts | 24 +++++++++++++++++++ > > > > 1 file changed, 24 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > > > > index 184e2aa2416af..156e001492173 100644 > > > > --- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > > > > +++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > > > > @@ -106,6 +106,12 @@ &gmac1m1_rgmii_clk > > > > status = "okay"; > > > > }; > > > > > > > > +&hdmi { > > > > + status = "okay"; > > > > + avdd-0v9-supply = <&vdda0v9_image>; > > > > + avdd-1v8-supply = <&vcca1v8_image>; > > > > +}; > > > > + > > > > &i2c0 { > > > > status = "okay"; > > > > > > > > @@ -390,3 +396,21 @@ &sdmmc0 { > > > > &uart2 { > > > > status = "okay"; > > > > }; > > > > + > > > > +&vop { > > > > + status = "okay"; > > > > + assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>; > > > > + assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>; > > > > +}; > > > > + > > > > +&vop_mmu { > > > > + status = "okay"; > > > > +}; > > > > + > > > > +&hdmi_in_vp0 { > > > > + status = "okay"; > > > > +}; > > > > + > > > > +&vp0_out_hdmi { > > > > + status = "okay"; > > > > +}; > > > > > > You can accomplish the same thing already with: > > > > > > &vp0_out_hdmi { > > > remote-endpoint = <&hdmi_in_vp0>; > > > }; > > > > My idea was to describe all possible connections in the dtsi file and > > let the board dts writer only en/disable the needed connections. When > > the connections are specified in the dts file then writing it is more > > difficult and error prone. > > > > > > > > or: > > > > > > &vp0_out_hdmi { > > > /delete-property/ remote-endpoint; > > > }; > > > > With this I have to change all connections that I don't need. With > > status = "okay" I have to change all connections that I actually do > > need, which will be much easier to read and write. > > > > I'll stick to the status = "okay" method for the next round, maybe I can > > still convince you ;) > > > > If it's the 'status' property you don't like being used when it's not a > > device that is enabled/disabled, then every other name would be fine > > with me as well. > > hmm, we do have code in the rockchip drm-driver to find out > if the device at the end of a graph-connection is disabled or not [0] , > So on previous Rockchip socs, there are already all connections > established, and the driver weeds out the disabled ones. > > So I'm wondering what is missing to use that in a vop2 context? The vop2 has three video ports (crtcs) instead of only one. All three are described in the device tree and each of them has a of_graph connection to the different encoders, so something like: vp0 <-> hdmi vp0 <-> mipi vp1 <-> hdmi vp1 <-> mipi vp2 <-> hdmi vp2 <-> mipi Enabling just vp0 <-> hdmi means only the first video port is can do hdmi. Different constraints in the clock tree (hdmi reference clock is hardwired to hpll, not enough PLLs to put all video ports on independent ones) prevent us from just allowing all connections. Sascha