[PATCH v2 0/5] Improve Rockchip VOP2 display modes handling on RK3588 HDMI0

[PATCH v2 0/5] Improve Rockchip VOP2 display modes handling on RK3588 HDMI0

[Cristian Ciocaltea]

VOP2 support for RK3588 SoC is currently not capable to handle the full
range of display modes advertised by the connected screens, e.g. it
doesn't cope well with non-integer refresh rates like 59.94, 29.97,
23.98, etc.

There are two HDMI PHYs available on RK3588, each providing a PLL that
can be used by three out of the four VOP2 video ports as an alternative
and more accurate pixel clock source. This is able to correctly handle
all display modes up to 4K@60Hz.

As for the moment HDMI1 output is not supported upstream, the patch
series targets HDMI0 only.

Additionally, note that testing any HDMI 2.0 specific modes, e.g.
4K@60Hz, requires high TMDS clock ratio and scrambling support [1]. The
patch is usable but not yet ready to be submitted - I will handle this
soon.

Thanks,
Cristian

[1] https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/commits/rk3588-hdmi-bridge-next-20241115

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
Changes in v2:
- Collected Acked-by tag from Rob and Tested-by from Naoki
- Rebased series onto v6.13-rc1
- Link to v1: https://lore.kernel.org/r/20241116-vop2-hdmi0-disp-modes-v1-0-2bca51db4898@collabora.com

---
Cristian Ciocaltea (5):
      dt-bindings: display: vop2: Add optional PLL clock properties
      drm/rockchip: vop2: Drop unnecessary if_pixclk_rate computation
      drm/rockchip: vop2: Improve display modes handling on RK3588 HDMI0
      arm64: dts: rockchip: Enable HDMI0 PHY clk provider on RK3588
      arm64: dts: rockchip: Add HDMI0 PHY PLL clock source to VOP2 on RK3588

 .../bindings/display/rockchip/rockchip-vop2.yaml   |  4 +++
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi      |  7 +++--
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c       | 36 +++++++++++++++++++++-
 3 files changed, 44 insertions(+), 3 deletions(-)
---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20241116-vop2-hdmi0-disp-modes-b39e3619768f

[PATCH v2 1/5] dt-bindings: display: vop2: Add optional PLL clock properties

[Cristian Ciocaltea]

On RK3588, HDMI PHY PLL can be used as an alternative and more accurate
pixel clock source for VOP2 video ports 0, 1 and 2.

Document the optional PLL clock properties corresponding to the two HDMI
PHYs available on the SoC.

Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
index 2531726af306bd388c00c3c0a1785b2c7367e2bd..46d956e63338e196361483a668fbf5597ebce24f 100644
--- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
+++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
@@ -53,6 +53,8 @@ properties:
       - description: Pixel clock for video port 2.
       - description: Pixel clock for video port 3.
       - description: Peripheral(vop grf/dsi) clock.
+      - description: Alternative pixel clock provided by HDMI0 PHY PLL.
+      - description: Alternative pixel clock provided by HDMI1 PHY PLL.
 
   clock-names:
     minItems: 5
@@ -64,6 +66,8 @@ properties:
       - const: dclk_vp2
       - const: dclk_vp3
       - const: pclk_vop
+      - const: pll_hdmiphy0
+      - const: pll_hdmiphy1
 
   rockchip,grf:
     $ref: /schemas/types.yaml#/definitions/phandle

-- 
2.47.0

[PATCH v2 2/5] drm/rockchip: vop2: Drop unnecessary if_pixclk_rate computation

[Cristian Ciocaltea]

The if_pixclk_rate variable is not being used outside of the if-block in
rk3588_calc_cru_cfg(), hence move the superfluous assignment from the
first branch to the inner comment-block.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 9873172e3fd331f4457fb70c5416dad7af9e3536..8b2f53ffefdbf1cc8737b3a86e630a03a7fd9348 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1695,8 +1695,8 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
 			K = 2;
 		}
 
-		if_pixclk_rate = (dclk_core_rate << 1) / K;
 		/*
+		 * if_pixclk_rate = (dclk_core_rate << 1) / K;
 		 * if_dclk_rate = dclk_core_rate / K;
 		 * *if_pixclk_div = dclk_rate / if_pixclk_rate;
 		 * *if_dclk_div = dclk_rate / if_dclk_rate;

-- 
2.47.0

[PATCH v2 3/5] drm/rockchip: vop2: Improve display modes handling on RK3588 HDMI0

[Cristian Ciocaltea]

The RK3588 specific implementation is currently quite limited in terms
of handling the full range of display modes supported by the connected
screens, e.g. 2560x1440@75Hz, 2048x1152@60Hz, 1024x768@60Hz are just a
few of them.

Additionally, it doesn't cope well with non-integer refresh rates like
59.94, 29.97, 23.98, etc.

Make use of HDMI0 PHY PLL as a more accurate DCLK source to handle
all display modes up to 4K@60Hz.

Tested-by: FUKAUMI Naoki <naoki@radxa.com>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 34 ++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 8b2f53ffefdbf1cc8737b3a86e630a03a7fd9348..393fe6aa170aaee9663c4a6d98c1cd6a5ef79392 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -158,6 +158,7 @@ struct vop2_video_port {
 	struct drm_crtc crtc;
 	struct vop2 *vop2;
 	struct clk *dclk;
+	struct clk *dclk_src;
 	unsigned int id;
 	const struct vop2_video_port_data *data;
 
@@ -212,6 +213,7 @@ struct vop2 {
 	struct clk *hclk;
 	struct clk *aclk;
 	struct clk *pclk;
+	struct clk *pll_hdmiphy0;
 
 	/* optional internal rgb encoder */
 	struct rockchip_rgb *rgb;
@@ -220,6 +222,8 @@ struct vop2 {
 	struct vop2_win win[];
 };
 
+#define VOP2_MAX_DCLK_RATE		600000 /* kHz */
+
 #define vop2_output_if_is_hdmi(x)	((x) == ROCKCHIP_VOP2_EP_HDMI0 || \
 					 (x) == ROCKCHIP_VOP2_EP_HDMI1)
 
@@ -1033,6 +1037,9 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
 
 	vop2_crtc_disable_irq(vp, VP_INT_DSP_HOLD_VALID);
 
+	if (vp->dclk_src)
+		clk_set_parent(vp->dclk, vp->dclk_src);
+
 	clk_disable_unprepare(vp->dclk);
 
 	vop2->enable_count--;
@@ -2049,6 +2056,27 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	vop2_vp_write(vp, RK3568_VP_MIPI_CTRL, 0);
 
+	/*
+	 * Switch to HDMI PHY PLL as DCLK source for display modes up
+	 * to 4K@60Hz, if available, otherwise keep using the system CRU.
+	 */
+	if (vop2->pll_hdmiphy0 && mode->crtc_clock <= VOP2_MAX_DCLK_RATE) {
+		drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
+			struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
+
+			if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI0) {
+				if (!vp->dclk_src)
+					vp->dclk_src = clk_get_parent(vp->dclk);
+
+				ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy0);
+				if (ret < 0)
+					drm_warn(vop2->drm,
+						 "Could not switch to HDMI0 PHY PLL: %d\n", ret);
+				break;
+			}
+		}
+	}
+
 	clk_set_rate(vp->dclk, clock);
 
 	vop2_post_config(crtc);
@@ -3167,6 +3195,12 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
 		return PTR_ERR(vop2->pclk);
 	}
 
+	vop2->pll_hdmiphy0 = devm_clk_get_optional(vop2->dev, "pll_hdmiphy0");
+	if (IS_ERR(vop2->pll_hdmiphy0)) {
+		drm_err(vop2->drm, "failed to get pll_hdmiphy0\n");
+		return PTR_ERR(vop2->pll_hdmiphy0);
+	}
+
 	vop2->irq = platform_get_irq(pdev, 0);
 	if (vop2->irq < 0) {
 		drm_err(vop2->drm, "cannot find irq for vop2\n");

-- 
2.47.0

[PATCH v2 4/5] arm64: dts: rockchip: Enable HDMI0 PHY clk provider on RK3588

[Cristian Ciocaltea]

Since commit c4b09c562086 ("phy: phy-rockchip-samsung-hdptx: Add clock
provider support"), the HDMI PHY PLL can be used as an alternative and
more accurate pixel clock source for VOP2 to improve display modes
handling on RK3588 SoC.

Add the missing #clock-cells property to allow using the clock provider
functionality of HDMI0 PHY.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
index a337f3fb8377e4a3a200d4d3a3773a237de2bd6e..22462e86f48027ab7c5e270f2fa04df7afcc1d24 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
@@ -2811,6 +2811,7 @@ hdptxphy_hdmi0: phy@fed60000 {
 		reg = <0x0 0xfed60000 0x0 0x2000>;
 		clocks = <&cru CLK_USB2PHY_HDPTXRXPHY_REF>, <&cru PCLK_HDPTX0>;
 		clock-names = "ref", "apb";
+		#clock-cells = <0>;
 		#phy-cells = <0>;
 		resets = <&cru SRST_HDPTX0>, <&cru SRST_P_HDPTX0>,
 			 <&cru SRST_HDPTX0_INIT>, <&cru SRST_HDPTX0_CMN>,

-- 
2.47.0

[PATCH v2 5/5] arm64: dts: rockchip: Add HDMI0 PHY PLL clock source to VOP2 on RK3588

[Cristian Ciocaltea]

VOP2 on RK3588 is able to use the HDMI PHY PLL as an alternative and
more accurate pixel clock source to improve handling of display modes up
to 4K@60Hz on video ports 0, 1 and 2.

For now only HDMI0 output is supported, hence add the related PLL clock.

Tested-by: FUKAUMI Naoki <naoki@radxa.com>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
index 22462e86f48027ab7c5e270f2fa04df7afcc1d24..d07be2a81f28b4cbfe314992c662d8cfb3d3d344 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
@@ -1262,14 +1262,16 @@ vop: vop@fdd90000 {
 			 <&cru DCLK_VOP1>,
 			 <&cru DCLK_VOP2>,
 			 <&cru DCLK_VOP3>,
-			 <&cru PCLK_VOP_ROOT>;
+			 <&cru PCLK_VOP_ROOT>,
+			 <&hdptxphy_hdmi0>;
 		clock-names = "aclk",
 			      "hclk",
 			      "dclk_vp0",
 			      "dclk_vp1",
 			      "dclk_vp2",
 			      "dclk_vp3",
-			      "pclk_vop";
+			      "pclk_vop",
+			      "pll_hdmiphy0";
 		iommus = <&vop_mmu>;
 		power-domains = <&power RK3588_PD_VOP>;
 		rockchip,grf = <&sys_grf>;

-- 
2.47.0

Re: [PATCH v2 3/5] drm/rockchip: vop2: Improve display modes handling on RK3588 HDMI0

[Maxime Ripard]

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

On Wed, Dec 11, 2024 at 12:15:07PM +0200, Cristian Ciocaltea wrote:
> The RK3588 specific implementation is currently quite limited in terms
> of handling the full range of display modes supported by the connected
> screens, e.g. 2560x1440@75Hz, 2048x1152@60Hz, 1024x768@60Hz are just a
> few of them.
> 
> Additionally, it doesn't cope well with non-integer refresh rates like
> 59.94, 29.97, 23.98, etc.
> 
> Make use of HDMI0 PHY PLL as a more accurate DCLK source to handle
> all display modes up to 4K@60Hz.
> 
> Tested-by: FUKAUMI Naoki <naoki@radxa.com>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 34 ++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 8b2f53ffefdbf1cc8737b3a86e630a03a7fd9348..393fe6aa170aaee9663c4a6d98c1cd6a5ef79392 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -158,6 +158,7 @@ struct vop2_video_port {
>  	struct drm_crtc crtc;
>  	struct vop2 *vop2;
>  	struct clk *dclk;
> +	struct clk *dclk_src;
>  	unsigned int id;
>  	const struct vop2_video_port_data *data;
>  
> @@ -212,6 +213,7 @@ struct vop2 {
>  	struct clk *hclk;
>  	struct clk *aclk;
>  	struct clk *pclk;
> +	struct clk *pll_hdmiphy0;
>  
>  	/* optional internal rgb encoder */
>  	struct rockchip_rgb *rgb;
> @@ -220,6 +222,8 @@ struct vop2 {
>  	struct vop2_win win[];
>  };
>  
> +#define VOP2_MAX_DCLK_RATE		600000 /* kHz */
> +
>  #define vop2_output_if_is_hdmi(x)	((x) == ROCKCHIP_VOP2_EP_HDMI0 || \
>  					 (x) == ROCKCHIP_VOP2_EP_HDMI1)
>  
> @@ -1033,6 +1037,9 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
>  
>  	vop2_crtc_disable_irq(vp, VP_INT_DSP_HOLD_VALID);
>  
> +	if (vp->dclk_src)
> +		clk_set_parent(vp->dclk, vp->dclk_src);
> +
>  	clk_disable_unprepare(vp->dclk);
>  
>  	vop2->enable_count--;
> @@ -2049,6 +2056,27 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>  
>  	vop2_vp_write(vp, RK3568_VP_MIPI_CTRL, 0);
>  
> +	/*
> +	 * Switch to HDMI PHY PLL as DCLK source for display modes up
> +	 * to 4K@60Hz, if available, otherwise keep using the system CRU.
> +	 */
> +	if (vop2->pll_hdmiphy0 && mode->crtc_clock <= VOP2_MAX_DCLK_RATE) {
> +		drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
> +			struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
> +
> +			if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI0) {
> +				if (!vp->dclk_src)
> +					vp->dclk_src = clk_get_parent(vp->dclk);
> +
> +				ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy0);
> +				if (ret < 0)
> +					drm_warn(vop2->drm,
> +						 "Could not switch to HDMI0 PHY PLL: %d\n", ret);
> +				break;
> +			}
> +		}
> +	}
> +

It seems pretty fragile to do it at atomic_enable time, even more so
since you don't lock the parent either.

Any reason not to do it in the DRM or clock driver probe, and make sure
you never change the parent somehow?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2 3/5] drm/rockchip: vop2: Improve display modes handling on RK3588 HDMI0

[Heiko Stübner]

Am Mittwoch, 11. Dezember 2024, 18:07:57 CET schrieb Maxime Ripard:
> On Wed, Dec 11, 2024 at 12:15:07PM +0200, Cristian Ciocaltea wrote:
> > The RK3588 specific implementation is currently quite limited in terms
> > of handling the full range of display modes supported by the connected
> > screens, e.g. 2560x1440@75Hz, 2048x1152@60Hz, 1024x768@60Hz are just a
> > few of them.
> > 
> > Additionally, it doesn't cope well with non-integer refresh rates like
> > 59.94, 29.97, 23.98, etc.
> > 
> > Make use of HDMI0 PHY PLL as a more accurate DCLK source to handle
> > all display modes up to 4K@60Hz.
> > 
> > Tested-by: FUKAUMI Naoki <naoki@radxa.com>
> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> > ---
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 34 ++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > index 8b2f53ffefdbf1cc8737b3a86e630a03a7fd9348..393fe6aa170aaee9663c4a6d98c1cd6a5ef79392 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > @@ -158,6 +158,7 @@ struct vop2_video_port {
> >  	struct drm_crtc crtc;
> >  	struct vop2 *vop2;
> >  	struct clk *dclk;
> > +	struct clk *dclk_src;
> >  	unsigned int id;
> >  	const struct vop2_video_port_data *data;
> >  
> > @@ -212,6 +213,7 @@ struct vop2 {
> >  	struct clk *hclk;
> >  	struct clk *aclk;
> >  	struct clk *pclk;
> > +	struct clk *pll_hdmiphy0;
> >  
> >  	/* optional internal rgb encoder */
> >  	struct rockchip_rgb *rgb;
> > @@ -220,6 +222,8 @@ struct vop2 {
> >  	struct vop2_win win[];
> >  };
> >  
> > +#define VOP2_MAX_DCLK_RATE		600000 /* kHz */
> > +
> >  #define vop2_output_if_is_hdmi(x)	((x) == ROCKCHIP_VOP2_EP_HDMI0 || \
> >  					 (x) == ROCKCHIP_VOP2_EP_HDMI1)
> >  
> > @@ -1033,6 +1037,9 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
> >  
> >  	vop2_crtc_disable_irq(vp, VP_INT_DSP_HOLD_VALID);
> >  
> > +	if (vp->dclk_src)
> > +		clk_set_parent(vp->dclk, vp->dclk_src);
> > +
> >  	clk_disable_unprepare(vp->dclk);
> >  
> >  	vop2->enable_count--;
> > @@ -2049,6 +2056,27 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
> >  
> >  	vop2_vp_write(vp, RK3568_VP_MIPI_CTRL, 0);
> >  
> > +	/*
> > +	 * Switch to HDMI PHY PLL as DCLK source for display modes up
> > +	 * to 4K@60Hz, if available, otherwise keep using the system CRU.
> > +	 */
> > +	if (vop2->pll_hdmiphy0 && mode->crtc_clock <= VOP2_MAX_DCLK_RATE) {
> > +		drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
> > +			struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
> > +
> > +			if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI0) {
> > +				if (!vp->dclk_src)
> > +					vp->dclk_src = clk_get_parent(vp->dclk);
> > +
> > +				ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy0);
> > +				if (ret < 0)
> > +					drm_warn(vop2->drm,
> > +						 "Could not switch to HDMI0 PHY PLL: %d\n", ret);
> > +				break;
> > +			}
> > +		}
> > +	}
> > +
> 
> It seems pretty fragile to do it at atomic_enable time, even more so
> since you don't lock the parent either.
> 
> Any reason not to do it in the DRM or clock driver probe, and make sure
> you never change the parent somehow?

On rk3588 we have 3 dclk_s and 2 hdmi controllers. Each video-port can
use the clock generated from either the hdmi0phy or hdmi1phy, depending
on which hdmi-controller it uses.

So you actually need to know which vpX will output to which hdmiY to then
reparent that dclk to the hdmiphy output.

Re: [PATCH v2 3/5] drm/rockchip: vop2: Improve display modes handling on RK3588 HDMI0

[Maxime Ripard]

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

On Wed, Dec 11, 2024 at 06:23:03PM +0100, Heiko Stübner wrote:
> Am Mittwoch, 11. Dezember 2024, 18:07:57 CET schrieb Maxime Ripard:
> > On Wed, Dec 11, 2024 at 12:15:07PM +0200, Cristian Ciocaltea wrote:
> > > The RK3588 specific implementation is currently quite limited in terms
> > > of handling the full range of display modes supported by the connected
> > > screens, e.g. 2560x1440@75Hz, 2048x1152@60Hz, 1024x768@60Hz are just a
> > > few of them.
> > > 
> > > Additionally, it doesn't cope well with non-integer refresh rates like
> > > 59.94, 29.97, 23.98, etc.
> > > 
> > > Make use of HDMI0 PHY PLL as a more accurate DCLK source to handle
> > > all display modes up to 4K@60Hz.
> > > 
> > > Tested-by: FUKAUMI Naoki <naoki@radxa.com>
> > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> > > ---
> > >  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 34 ++++++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > index 8b2f53ffefdbf1cc8737b3a86e630a03a7fd9348..393fe6aa170aaee9663c4a6d98c1cd6a5ef79392 100644
> > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > @@ -158,6 +158,7 @@ struct vop2_video_port {
> > >  	struct drm_crtc crtc;
> > >  	struct vop2 *vop2;
> > >  	struct clk *dclk;
> > > +	struct clk *dclk_src;
> > >  	unsigned int id;
> > >  	const struct vop2_video_port_data *data;
> > >  
> > > @@ -212,6 +213,7 @@ struct vop2 {
> > >  	struct clk *hclk;
> > >  	struct clk *aclk;
> > >  	struct clk *pclk;
> > > +	struct clk *pll_hdmiphy0;
> > >  
> > >  	/* optional internal rgb encoder */
> > >  	struct rockchip_rgb *rgb;
> > > @@ -220,6 +222,8 @@ struct vop2 {
> > >  	struct vop2_win win[];
> > >  };
> > >  
> > > +#define VOP2_MAX_DCLK_RATE		600000 /* kHz */
> > > +
> > >  #define vop2_output_if_is_hdmi(x)	((x) == ROCKCHIP_VOP2_EP_HDMI0 || \
> > >  					 (x) == ROCKCHIP_VOP2_EP_HDMI1)
> > >  
> > > @@ -1033,6 +1037,9 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
> > >  
> > >  	vop2_crtc_disable_irq(vp, VP_INT_DSP_HOLD_VALID);
> > >  
> > > +	if (vp->dclk_src)
> > > +		clk_set_parent(vp->dclk, vp->dclk_src);
> > > +
> > >  	clk_disable_unprepare(vp->dclk);
> > >  
> > >  	vop2->enable_count--;
> > > @@ -2049,6 +2056,27 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
> > >  
> > >  	vop2_vp_write(vp, RK3568_VP_MIPI_CTRL, 0);
> > >  
> > > +	/*
> > > +	 * Switch to HDMI PHY PLL as DCLK source for display modes up
> > > +	 * to 4K@60Hz, if available, otherwise keep using the system CRU.
> > > +	 */
> > > +	if (vop2->pll_hdmiphy0 && mode->crtc_clock <= VOP2_MAX_DCLK_RATE) {
> > > +		drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
> > > +			struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
> > > +
> > > +			if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI0) {
> > > +				if (!vp->dclk_src)
> > > +					vp->dclk_src = clk_get_parent(vp->dclk);
> > > +
> > > +				ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy0);
> > > +				if (ret < 0)
> > > +					drm_warn(vop2->drm,
> > > +						 "Could not switch to HDMI0 PHY PLL: %d\n", ret);
> > > +				break;
> > > +			}
> > > +		}
> > > +	}
> > > +
> > 
> > It seems pretty fragile to do it at atomic_enable time, even more so
> > since you don't lock the parent either.
> > 
> > Any reason not to do it in the DRM or clock driver probe, and make sure
> > you never change the parent somehow?
> 
> On rk3588 we have 3 dclk_s and 2 hdmi controllers. Each video-port can
> use the clock generated from either the hdmi0phy or hdmi1phy, depending
> on which hdmi-controller it uses.
> 
> So you actually need to know which vpX will output to which hdmiY to then
> reparent that dclk to the hdmiphy output.

The Rockchip nomenclature isn't super obvious to me, sorry. Is there a
datasheet for this somewhere? Also, does this vpX -> HDMI-Y mapping need
to be dynamic?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2 3/5] drm/rockchip: vop2: Improve display modes handling on RK3588 HDMI0

[Heiko Stübner]

Am Mittwoch, 11. Dezember 2024, 18:47:44 CET schrieb Maxime Ripard:
> On Wed, Dec 11, 2024 at 06:23:03PM +0100, Heiko Stübner wrote:
> > Am Mittwoch, 11. Dezember 2024, 18:07:57 CET schrieb Maxime Ripard:
> > > On Wed, Dec 11, 2024 at 12:15:07PM +0200, Cristian Ciocaltea wrote:
> > > > The RK3588 specific implementation is currently quite limited in terms
> > > > of handling the full range of display modes supported by the connected
> > > > screens, e.g. 2560x1440@75Hz, 2048x1152@60Hz, 1024x768@60Hz are just a
> > > > few of them.
> > > > 
> > > > Additionally, it doesn't cope well with non-integer refresh rates like
> > > > 59.94, 29.97, 23.98, etc.
> > > > 
> > > > Make use of HDMI0 PHY PLL as a more accurate DCLK source to handle
> > > > all display modes up to 4K@60Hz.
> > > > 
> > > > Tested-by: FUKAUMI Naoki <naoki@radxa.com>
> > > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> > > > ---
> > > >  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 34 ++++++++++++++++++++++++++++
> > > >  1 file changed, 34 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > > index 8b2f53ffefdbf1cc8737b3a86e630a03a7fd9348..393fe6aa170aaee9663c4a6d98c1cd6a5ef79392 100644
> > > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > > @@ -158,6 +158,7 @@ struct vop2_video_port {
> > > >  	struct drm_crtc crtc;
> > > >  	struct vop2 *vop2;
> > > >  	struct clk *dclk;
> > > > +	struct clk *dclk_src;
> > > >  	unsigned int id;
> > > >  	const struct vop2_video_port_data *data;
> > > >  
> > > > @@ -212,6 +213,7 @@ struct vop2 {
> > > >  	struct clk *hclk;
> > > >  	struct clk *aclk;
> > > >  	struct clk *pclk;
> > > > +	struct clk *pll_hdmiphy0;
> > > >  
> > > >  	/* optional internal rgb encoder */
> > > >  	struct rockchip_rgb *rgb;
> > > > @@ -220,6 +222,8 @@ struct vop2 {
> > > >  	struct vop2_win win[];
> > > >  };
> > > >  
> > > > +#define VOP2_MAX_DCLK_RATE		600000 /* kHz */
> > > > +
> > > >  #define vop2_output_if_is_hdmi(x)	((x) == ROCKCHIP_VOP2_EP_HDMI0 || \
> > > >  					 (x) == ROCKCHIP_VOP2_EP_HDMI1)
> > > >  
> > > > @@ -1033,6 +1037,9 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
> > > >  
> > > >  	vop2_crtc_disable_irq(vp, VP_INT_DSP_HOLD_VALID);
> > > >  
> > > > +	if (vp->dclk_src)
> > > > +		clk_set_parent(vp->dclk, vp->dclk_src);
> > > > +
> > > >  	clk_disable_unprepare(vp->dclk);
> > > >  
> > > >  	vop2->enable_count--;
> > > > @@ -2049,6 +2056,27 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
> > > >  
> > > >  	vop2_vp_write(vp, RK3568_VP_MIPI_CTRL, 0);
> > > >  
> > > > +	/*
> > > > +	 * Switch to HDMI PHY PLL as DCLK source for display modes up
> > > > +	 * to 4K@60Hz, if available, otherwise keep using the system CRU.
> > > > +	 */
> > > > +	if (vop2->pll_hdmiphy0 && mode->crtc_clock <= VOP2_MAX_DCLK_RATE) {
> > > > +		drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
> > > > +			struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
> > > > +
> > > > +			if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI0) {
> > > > +				if (!vp->dclk_src)
> > > > +					vp->dclk_src = clk_get_parent(vp->dclk);
> > > > +
> > > > +				ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy0);
> > > > +				if (ret < 0)
> > > > +					drm_warn(vop2->drm,
> > > > +						 "Could not switch to HDMI0 PHY PLL: %d\n", ret);
> > > > +				break;
> > > > +			}
> > > > +		}
> > > > +	}
> > > > +
> > > 
> > > It seems pretty fragile to do it at atomic_enable time, even more so
> > > since you don't lock the parent either.
> > > 
> > > Any reason not to do it in the DRM or clock driver probe, and make sure
> > > you never change the parent somehow?
> > 
> > On rk3588 we have 3 dclk_s and 2 hdmi controllers. Each video-port can
> > use the clock generated from either the hdmi0phy or hdmi1phy, depending
> > on which hdmi-controller it uses.
> > 
> > So you actually need to know which vpX will output to which hdmiY to then
> > reparent that dclk to the hdmiphy output.
> 
> The Rockchip nomenclature isn't super obvious to me, sorry. Is there a
> datasheet for this somewhere? Also, does this vpX -> HDMI-Y mapping need
> to be dynamic?

VPs are CRTCs in drm-language and each of them can drive a differing
number of output encoders. Those video-ports also have differing output
characteristics in terms of supported resolution and other properties.

The rk3588 TRM has leaked in a number of places, and if you find a
TRM-part2, there is a section labeled "Display Output Interface Description"
that has a nice graphic for that.

Or in short:
- CRTC(VP)0 supports 8K resolution and can drive DP0+1, HDMI0+1, eDP0+1
  [if I'm reading things correctly, 8K together with CRTC1 somehow)
- CRTC(VP)1 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP0+1
- CRTC(VP)2 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP01, DSI0+1
- CRTC(VP)3 supports 2K resolution and can drive DSI0+1 and some BT1120,BT656

so for the 3 higher resolution CRTCs there are essentially 6 or 8 output options
depending on the board design

Re: [PATCH v2 3/5] drm/rockchip: vop2: Improve display modes handling on RK3588 HDMI0

[Maxime Ripard]

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

On Wed, Dec 11, 2024 at 07:01:15PM +0100, Heiko Stübner wrote:
> Am Mittwoch, 11. Dezember 2024, 18:47:44 CET schrieb Maxime Ripard:
> > On Wed, Dec 11, 2024 at 06:23:03PM +0100, Heiko Stübner wrote:
> > > Am Mittwoch, 11. Dezember 2024, 18:07:57 CET schrieb Maxime Ripard:
> > > > On Wed, Dec 11, 2024 at 12:15:07PM +0200, Cristian Ciocaltea wrote:
> > > > > The RK3588 specific implementation is currently quite limited in terms
> > > > > of handling the full range of display modes supported by the connected
> > > > > screens, e.g. 2560x1440@75Hz, 2048x1152@60Hz, 1024x768@60Hz are just a
> > > > > few of them.
> > > > > 
> > > > > Additionally, it doesn't cope well with non-integer refresh rates like
> > > > > 59.94, 29.97, 23.98, etc.
> > > > > 
> > > > > Make use of HDMI0 PHY PLL as a more accurate DCLK source to handle
> > > > > all display modes up to 4K@60Hz.
> > > > > 
> > > > > Tested-by: FUKAUMI Naoki <naoki@radxa.com>
> > > > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> > > > > ---
> > > > >  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 34 ++++++++++++++++++++++++++++
> > > > >  1 file changed, 34 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > > > index 8b2f53ffefdbf1cc8737b3a86e630a03a7fd9348..393fe6aa170aaee9663c4a6d98c1cd6a5ef79392 100644
> > > > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > > > @@ -158,6 +158,7 @@ struct vop2_video_port {
> > > > >  	struct drm_crtc crtc;
> > > > >  	struct vop2 *vop2;
> > > > >  	struct clk *dclk;
> > > > > +	struct clk *dclk_src;
> > > > >  	unsigned int id;
> > > > >  	const struct vop2_video_port_data *data;
> > > > >  
> > > > > @@ -212,6 +213,7 @@ struct vop2 {
> > > > >  	struct clk *hclk;
> > > > >  	struct clk *aclk;
> > > > >  	struct clk *pclk;
> > > > > +	struct clk *pll_hdmiphy0;
> > > > >  
> > > > >  	/* optional internal rgb encoder */
> > > > >  	struct rockchip_rgb *rgb;
> > > > > @@ -220,6 +222,8 @@ struct vop2 {
> > > > >  	struct vop2_win win[];
> > > > >  };
> > > > >  
> > > > > +#define VOP2_MAX_DCLK_RATE		600000 /* kHz */
> > > > > +
> > > > >  #define vop2_output_if_is_hdmi(x)	((x) == ROCKCHIP_VOP2_EP_HDMI0 || \
> > > > >  					 (x) == ROCKCHIP_VOP2_EP_HDMI1)
> > > > >  
> > > > > @@ -1033,6 +1037,9 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
> > > > >  
> > > > >  	vop2_crtc_disable_irq(vp, VP_INT_DSP_HOLD_VALID);
> > > > >  
> > > > > +	if (vp->dclk_src)
> > > > > +		clk_set_parent(vp->dclk, vp->dclk_src);
> > > > > +
> > > > >  	clk_disable_unprepare(vp->dclk);
> > > > >  
> > > > >  	vop2->enable_count--;
> > > > > @@ -2049,6 +2056,27 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
> > > > >  
> > > > >  	vop2_vp_write(vp, RK3568_VP_MIPI_CTRL, 0);
> > > > >  
> > > > > +	/*
> > > > > +	 * Switch to HDMI PHY PLL as DCLK source for display modes up
> > > > > +	 * to 4K@60Hz, if available, otherwise keep using the system CRU.
> > > > > +	 */
> > > > > +	if (vop2->pll_hdmiphy0 && mode->crtc_clock <= VOP2_MAX_DCLK_RATE) {
> > > > > +		drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
> > > > > +			struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
> > > > > +
> > > > > +			if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI0) {
> > > > > +				if (!vp->dclk_src)
> > > > > +					vp->dclk_src = clk_get_parent(vp->dclk);
> > > > > +
> > > > > +				ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy0);
> > > > > +				if (ret < 0)
> > > > > +					drm_warn(vop2->drm,
> > > > > +						 "Could not switch to HDMI0 PHY PLL: %d\n", ret);
> > > > > +				break;
> > > > > +			}
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > 
> > > > It seems pretty fragile to do it at atomic_enable time, even more so
> > > > since you don't lock the parent either.
> > > > 
> > > > Any reason not to do it in the DRM or clock driver probe, and make sure
> > > > you never change the parent somehow?
> > > 
> > > On rk3588 we have 3 dclk_s and 2 hdmi controllers. Each video-port can
> > > use the clock generated from either the hdmi0phy or hdmi1phy, depending
> > > on which hdmi-controller it uses.
> > > 
> > > So you actually need to know which vpX will output to which hdmiY to then
> > > reparent that dclk to the hdmiphy output.
> > 
> > The Rockchip nomenclature isn't super obvious to me, sorry. Is there a
> > datasheet for this somewhere? Also, does this vpX -> HDMI-Y mapping need
> > to be dynamic?
> 
> VPs are CRTCs in drm-language and each of them can drive a differing
> number of output encoders. Those video-ports also have differing output
> characteristics in terms of supported resolution and other properties.
> 
> The rk3588 TRM has leaked in a number of places, and if you find a
> TRM-part2, there is a section labeled "Display Output Interface Description"
> that has a nice graphic for that.
> 
> Or in short:
> - CRTC(VP)0 supports 8K resolution and can drive DP0+1, HDMI0+1, eDP0+1
>   [if I'm reading things correctly, 8K together with CRTC1 somehow)
> - CRTC(VP)1 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP0+1
> - CRTC(VP)2 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP01, DSI0+1
> - CRTC(VP)3 supports 2K resolution and can drive DSI0+1 and some BT1120,BT656
> 
> so for the 3 higher resolution CRTCs there are essentially 6 or 8 output options
> depending on the board design

That's much clearer, thanks. I'm not entirely sure how that links to the
need for the PLL to change its parent depending on the ouput. Do you
need to always have all the outputs on the same PLL?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2 3/5] drm/rockchip: vop2: Improve display modes handling on RK3588 HDMI0

[Cristian Ciocaltea]

On 12/17/24 5:00 PM, Maxime Ripard wrote:
> On Wed, Dec 11, 2024 at 07:01:15PM +0100, Heiko Stübner wrote:
>> Am Mittwoch, 11. Dezember 2024, 18:47:44 CET schrieb Maxime Ripard:
>>> On Wed, Dec 11, 2024 at 06:23:03PM +0100, Heiko Stübner wrote:
>>>> Am Mittwoch, 11. Dezember 2024, 18:07:57 CET schrieb Maxime Ripard:
>>>>> On Wed, Dec 11, 2024 at 12:15:07PM +0200, Cristian Ciocaltea wrote:
>>>>>> The RK3588 specific implementation is currently quite limited in terms
>>>>>> of handling the full range of display modes supported by the connected
>>>>>> screens, e.g. 2560x1440@75Hz, 2048x1152@60Hz, 1024x768@60Hz are just a
>>>>>> few of them.
>>>>>>
>>>>>> Additionally, it doesn't cope well with non-integer refresh rates like
>>>>>> 59.94, 29.97, 23.98, etc.
>>>>>>
>>>>>> Make use of HDMI0 PHY PLL as a more accurate DCLK source to handle
>>>>>> all display modes up to 4K@60Hz.
>>>>>>
>>>>>> Tested-by: FUKAUMI Naoki <naoki@radxa.com>
>>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 34 ++++++++++++++++++++++++++++
>>>>>>  1 file changed, 34 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>>> index 8b2f53ffefdbf1cc8737b3a86e630a03a7fd9348..393fe6aa170aaee9663c4a6d98c1cd6a5ef79392 100644
>>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>>> @@ -158,6 +158,7 @@ struct vop2_video_port {
>>>>>>  	struct drm_crtc crtc;
>>>>>>  	struct vop2 *vop2;
>>>>>>  	struct clk *dclk;
>>>>>> +	struct clk *dclk_src;
>>>>>>  	unsigned int id;
>>>>>>  	const struct vop2_video_port_data *data;
>>>>>>  
>>>>>> @@ -212,6 +213,7 @@ struct vop2 {
>>>>>>  	struct clk *hclk;
>>>>>>  	struct clk *aclk;
>>>>>>  	struct clk *pclk;
>>>>>> +	struct clk *pll_hdmiphy0;
>>>>>>  
>>>>>>  	/* optional internal rgb encoder */
>>>>>>  	struct rockchip_rgb *rgb;
>>>>>> @@ -220,6 +222,8 @@ struct vop2 {
>>>>>>  	struct vop2_win win[];
>>>>>>  };
>>>>>>  
>>>>>> +#define VOP2_MAX_DCLK_RATE		600000 /* kHz */
>>>>>> +
>>>>>>  #define vop2_output_if_is_hdmi(x)	((x) == ROCKCHIP_VOP2_EP_HDMI0 || \
>>>>>>  					 (x) == ROCKCHIP_VOP2_EP_HDMI1)
>>>>>>  
>>>>>> @@ -1033,6 +1037,9 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
>>>>>>  
>>>>>>  	vop2_crtc_disable_irq(vp, VP_INT_DSP_HOLD_VALID);
>>>>>>  
>>>>>> +	if (vp->dclk_src)
>>>>>> +		clk_set_parent(vp->dclk, vp->dclk_src);
>>>>>> +
>>>>>>  	clk_disable_unprepare(vp->dclk);
>>>>>>  
>>>>>>  	vop2->enable_count--;
>>>>>> @@ -2049,6 +2056,27 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>>>>>>  
>>>>>>  	vop2_vp_write(vp, RK3568_VP_MIPI_CTRL, 0);
>>>>>>  
>>>>>> +	/*
>>>>>> +	 * Switch to HDMI PHY PLL as DCLK source for display modes up
>>>>>> +	 * to 4K@60Hz, if available, otherwise keep using the system CRU.
>>>>>> +	 */
>>>>>> +	if (vop2->pll_hdmiphy0 && mode->crtc_clock <= VOP2_MAX_DCLK_RATE) {
>>>>>> +		drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
>>>>>> +			struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
>>>>>> +
>>>>>> +			if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI0) {
>>>>>> +				if (!vp->dclk_src)
>>>>>> +					vp->dclk_src = clk_get_parent(vp->dclk);
>>>>>> +
>>>>>> +				ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy0);
>>>>>> +				if (ret < 0)
>>>>>> +					drm_warn(vop2->drm,
>>>>>> +						 "Could not switch to HDMI0 PHY PLL: %d\n", ret);
>>>>>> +				break;
>>>>>> +			}
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>
>>>>> It seems pretty fragile to do it at atomic_enable time, even more so
>>>>> since you don't lock the parent either.
>>>>>
>>>>> Any reason not to do it in the DRM or clock driver probe, and make sure
>>>>> you never change the parent somehow?
>>>>
>>>> On rk3588 we have 3 dclk_s and 2 hdmi controllers. Each video-port can
>>>> use the clock generated from either the hdmi0phy or hdmi1phy, depending
>>>> on which hdmi-controller it uses.
>>>>
>>>> So you actually need to know which vpX will output to which hdmiY to then
>>>> reparent that dclk to the hdmiphy output.
>>>
>>> The Rockchip nomenclature isn't super obvious to me, sorry. Is there a
>>> datasheet for this somewhere? Also, does this vpX -> HDMI-Y mapping need
>>> to be dynamic?
>>
>> VPs are CRTCs in drm-language and each of them can drive a differing
>> number of output encoders. Those video-ports also have differing output
>> characteristics in terms of supported resolution and other properties.
>>
>> The rk3588 TRM has leaked in a number of places, and if you find a
>> TRM-part2, there is a section labeled "Display Output Interface Description"
>> that has a nice graphic for that.
>>
>> Or in short:
>> - CRTC(VP)0 supports 8K resolution and can drive DP0+1, HDMI0+1, eDP0+1
>>   [if I'm reading things correctly, 8K together with CRTC1 somehow)
>> - CRTC(VP)1 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP0+1
>> - CRTC(VP)2 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP01, DSI0+1
>> - CRTC(VP)3 supports 2K resolution and can drive DSI0+1 and some BT1120,BT656
>>
>> so for the 3 higher resolution CRTCs there are essentially 6 or 8 output options
>> depending on the board design
> 
> That's much clearer, thanks. I'm not entirely sure how that links to the
> need for the PLL to change its parent depending on the ouput. Do you
> need to always have all the outputs on the same PLL?

One of the problems is that the PHY PLLs cannot be used as clock sources
for resolutions above 4K@60Hz, while VOP2 on RK3588 supports up to 8K@60Hz,
which is supposed to be handled by the system CRU.

Moreover, the 2 PLLs are shared between 3 out of the 4 video ports already
mentioned by Heiko.  There is quite a bit of complexity in downstream
driver to handle all possible usecases - see [1] for a brief description on
how was that designed to work.

Regards,
Cristian

[1] https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr4.1/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c#L4742

Re: [PATCH v2 3/5] drm/rockchip: vop2: Improve display modes handling on RK3588 HDMI0

[Maxime Ripard]

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

On Tue, Dec 17, 2024 at 06:36:41PM +0200, Cristian Ciocaltea wrote:
> On 12/17/24 5:00 PM, Maxime Ripard wrote:
> > On Wed, Dec 11, 2024 at 07:01:15PM +0100, Heiko Stübner wrote:
> >> Am Mittwoch, 11. Dezember 2024, 18:47:44 CET schrieb Maxime Ripard:
> >>> On Wed, Dec 11, 2024 at 06:23:03PM +0100, Heiko Stübner wrote:
> >>>> Am Mittwoch, 11. Dezember 2024, 18:07:57 CET schrieb Maxime Ripard:
> >>>>> On Wed, Dec 11, 2024 at 12:15:07PM +0200, Cristian Ciocaltea wrote:
> >>>>>> The RK3588 specific implementation is currently quite limited in terms
> >>>>>> of handling the full range of display modes supported by the connected
> >>>>>> screens, e.g. 2560x1440@75Hz, 2048x1152@60Hz, 1024x768@60Hz are just a
> >>>>>> few of them.
> >>>>>>
> >>>>>> Additionally, it doesn't cope well with non-integer refresh rates like
> >>>>>> 59.94, 29.97, 23.98, etc.
> >>>>>>
> >>>>>> Make use of HDMI0 PHY PLL as a more accurate DCLK source to handle
> >>>>>> all display modes up to 4K@60Hz.
> >>>>>>
> >>>>>> Tested-by: FUKAUMI Naoki <naoki@radxa.com>
> >>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> >>>>>> ---
> >>>>>>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 34 ++++++++++++++++++++++++++++
> >>>>>>  1 file changed, 34 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> >>>>>> index 8b2f53ffefdbf1cc8737b3a86e630a03a7fd9348..393fe6aa170aaee9663c4a6d98c1cd6a5ef79392 100644
> >>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> >>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> >>>>>> @@ -158,6 +158,7 @@ struct vop2_video_port {
> >>>>>>  	struct drm_crtc crtc;
> >>>>>>  	struct vop2 *vop2;
> >>>>>>  	struct clk *dclk;
> >>>>>> +	struct clk *dclk_src;
> >>>>>>  	unsigned int id;
> >>>>>>  	const struct vop2_video_port_data *data;
> >>>>>>  
> >>>>>> @@ -212,6 +213,7 @@ struct vop2 {
> >>>>>>  	struct clk *hclk;
> >>>>>>  	struct clk *aclk;
> >>>>>>  	struct clk *pclk;
> >>>>>> +	struct clk *pll_hdmiphy0;
> >>>>>>  
> >>>>>>  	/* optional internal rgb encoder */
> >>>>>>  	struct rockchip_rgb *rgb;
> >>>>>> @@ -220,6 +222,8 @@ struct vop2 {
> >>>>>>  	struct vop2_win win[];
> >>>>>>  };
> >>>>>>  
> >>>>>> +#define VOP2_MAX_DCLK_RATE		600000 /* kHz */
> >>>>>> +
> >>>>>>  #define vop2_output_if_is_hdmi(x)	((x) == ROCKCHIP_VOP2_EP_HDMI0 || \
> >>>>>>  					 (x) == ROCKCHIP_VOP2_EP_HDMI1)
> >>>>>>  
> >>>>>> @@ -1033,6 +1037,9 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
> >>>>>>  
> >>>>>>  	vop2_crtc_disable_irq(vp, VP_INT_DSP_HOLD_VALID);
> >>>>>>  
> >>>>>> +	if (vp->dclk_src)
> >>>>>> +		clk_set_parent(vp->dclk, vp->dclk_src);
> >>>>>> +
> >>>>>>  	clk_disable_unprepare(vp->dclk);
> >>>>>>  
> >>>>>>  	vop2->enable_count--;
> >>>>>> @@ -2049,6 +2056,27 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
> >>>>>>  
> >>>>>>  	vop2_vp_write(vp, RK3568_VP_MIPI_CTRL, 0);
> >>>>>>  
> >>>>>> +	/*
> >>>>>> +	 * Switch to HDMI PHY PLL as DCLK source for display modes up
> >>>>>> +	 * to 4K@60Hz, if available, otherwise keep using the system CRU.
> >>>>>> +	 */
> >>>>>> +	if (vop2->pll_hdmiphy0 && mode->crtc_clock <= VOP2_MAX_DCLK_RATE) {
> >>>>>> +		drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
> >>>>>> +			struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
> >>>>>> +
> >>>>>> +			if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI0) {
> >>>>>> +				if (!vp->dclk_src)
> >>>>>> +					vp->dclk_src = clk_get_parent(vp->dclk);
> >>>>>> +
> >>>>>> +				ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy0);
> >>>>>> +				if (ret < 0)
> >>>>>> +					drm_warn(vop2->drm,
> >>>>>> +						 "Could not switch to HDMI0 PHY PLL: %d\n", ret);
> >>>>>> +				break;
> >>>>>> +			}
> >>>>>> +		}
> >>>>>> +	}
> >>>>>> +
> >>>>>
> >>>>> It seems pretty fragile to do it at atomic_enable time, even more so
> >>>>> since you don't lock the parent either.
> >>>>>
> >>>>> Any reason not to do it in the DRM or clock driver probe, and make sure
> >>>>> you never change the parent somehow?
> >>>>
> >>>> On rk3588 we have 3 dclk_s and 2 hdmi controllers. Each video-port can
> >>>> use the clock generated from either the hdmi0phy or hdmi1phy, depending
> >>>> on which hdmi-controller it uses.
> >>>>
> >>>> So you actually need to know which vpX will output to which hdmiY to then
> >>>> reparent that dclk to the hdmiphy output.
> >>>
> >>> The Rockchip nomenclature isn't super obvious to me, sorry. Is there a
> >>> datasheet for this somewhere? Also, does this vpX -> HDMI-Y mapping need
> >>> to be dynamic?
> >>
> >> VPs are CRTCs in drm-language and each of them can drive a differing
> >> number of output encoders. Those video-ports also have differing output
> >> characteristics in terms of supported resolution and other properties.
> >>
> >> The rk3588 TRM has leaked in a number of places, and if you find a
> >> TRM-part2, there is a section labeled "Display Output Interface Description"
> >> that has a nice graphic for that.
> >>
> >> Or in short:
> >> - CRTC(VP)0 supports 8K resolution and can drive DP0+1, HDMI0+1, eDP0+1
> >>   [if I'm reading things correctly, 8K together with CRTC1 somehow)
> >> - CRTC(VP)1 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP0+1
> >> - CRTC(VP)2 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP01, DSI0+1
> >> - CRTC(VP)3 supports 2K resolution and can drive DSI0+1 and some BT1120,BT656
> >>
> >> so for the 3 higher resolution CRTCs there are essentially 6 or 8 output options
> >> depending on the board design
> > 
> > That's much clearer, thanks. I'm not entirely sure how that links to the
> > need for the PLL to change its parent depending on the ouput. Do you
> > need to always have all the outputs on the same PLL?
> 
> One of the problems is that the PHY PLLs cannot be used as clock sources
> for resolutions above 4K@60Hz, while VOP2 on RK3588 supports up to 8K@60Hz,
> which is supposed to be handled by the system CRU.

But can that system CRU drive resolutions lower than 4k@60? If so, why
do we bother with PHYs?

> Moreover, the 2 PLLs are shared between 3 out of the 4 video ports already
> mentioned by Heiko.  There is quite a bit of complexity in downstream
> driver to handle all possible usecases - see [1] for a brief description on
> how was that designed to work.

That's a generic allocation issue. Multiple drivers (vc4 for example)
has this issue for other components. It can be fairly easily dealt with
at atomic_check time.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2 3/5] drm/rockchip: vop2: Improve display modes handling on RK3588 HDMI0

[Cristian Ciocaltea]

On 12/17/24 6:53 PM, Maxime Ripard wrote:
> On Tue, Dec 17, 2024 at 06:36:41PM +0200, Cristian Ciocaltea wrote:
>> On 12/17/24 5:00 PM, Maxime Ripard wrote:
>>> On Wed, Dec 11, 2024 at 07:01:15PM +0100, Heiko Stübner wrote:
>>>> Am Mittwoch, 11. Dezember 2024, 18:47:44 CET schrieb Maxime Ripard:
>>>>> On Wed, Dec 11, 2024 at 06:23:03PM +0100, Heiko Stübner wrote:
>>>>>> Am Mittwoch, 11. Dezember 2024, 18:07:57 CET schrieb Maxime Ripard:
>>>>>>> On Wed, Dec 11, 2024 at 12:15:07PM +0200, Cristian Ciocaltea wrote:
>>>>>>>> The RK3588 specific implementation is currently quite limited in terms
>>>>>>>> of handling the full range of display modes supported by the connected
>>>>>>>> screens, e.g. 2560x1440@75Hz, 2048x1152@60Hz, 1024x768@60Hz are just a
>>>>>>>> few of them.
>>>>>>>>
>>>>>>>> Additionally, it doesn't cope well with non-integer refresh rates like
>>>>>>>> 59.94, 29.97, 23.98, etc.
>>>>>>>>
>>>>>>>> Make use of HDMI0 PHY PLL as a more accurate DCLK source to handle
>>>>>>>> all display modes up to 4K@60Hz.
>>>>>>>>
>>>>>>>> Tested-by: FUKAUMI Naoki <naoki@radxa.com>
>>>>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>>>>>> ---
>>>>>>>>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 34 ++++++++++++++++++++++++++++
>>>>>>>>  1 file changed, 34 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>>>>> index 8b2f53ffefdbf1cc8737b3a86e630a03a7fd9348..393fe6aa170aaee9663c4a6d98c1cd6a5ef79392 100644
>>>>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>>>>> @@ -158,6 +158,7 @@ struct vop2_video_port {
>>>>>>>>  	struct drm_crtc crtc;
>>>>>>>>  	struct vop2 *vop2;
>>>>>>>>  	struct clk *dclk;
>>>>>>>> +	struct clk *dclk_src;
>>>>>>>>  	unsigned int id;
>>>>>>>>  	const struct vop2_video_port_data *data;
>>>>>>>>  
>>>>>>>> @@ -212,6 +213,7 @@ struct vop2 {
>>>>>>>>  	struct clk *hclk;
>>>>>>>>  	struct clk *aclk;
>>>>>>>>  	struct clk *pclk;
>>>>>>>> +	struct clk *pll_hdmiphy0;
>>>>>>>>  
>>>>>>>>  	/* optional internal rgb encoder */
>>>>>>>>  	struct rockchip_rgb *rgb;
>>>>>>>> @@ -220,6 +222,8 @@ struct vop2 {
>>>>>>>>  	struct vop2_win win[];
>>>>>>>>  };
>>>>>>>>  
>>>>>>>> +#define VOP2_MAX_DCLK_RATE		600000 /* kHz */
>>>>>>>> +
>>>>>>>>  #define vop2_output_if_is_hdmi(x)	((x) == ROCKCHIP_VOP2_EP_HDMI0 || \
>>>>>>>>  					 (x) == ROCKCHIP_VOP2_EP_HDMI1)
>>>>>>>>  
>>>>>>>> @@ -1033,6 +1037,9 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
>>>>>>>>  
>>>>>>>>  	vop2_crtc_disable_irq(vp, VP_INT_DSP_HOLD_VALID);
>>>>>>>>  
>>>>>>>> +	if (vp->dclk_src)
>>>>>>>> +		clk_set_parent(vp->dclk, vp->dclk_src);
>>>>>>>> +
>>>>>>>>  	clk_disable_unprepare(vp->dclk);
>>>>>>>>  
>>>>>>>>  	vop2->enable_count--;
>>>>>>>> @@ -2049,6 +2056,27 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>>>>>>>>  
>>>>>>>>  	vop2_vp_write(vp, RK3568_VP_MIPI_CTRL, 0);
>>>>>>>>  
>>>>>>>> +	/*
>>>>>>>> +	 * Switch to HDMI PHY PLL as DCLK source for display modes up
>>>>>>>> +	 * to 4K@60Hz, if available, otherwise keep using the system CRU.
>>>>>>>> +	 */
>>>>>>>> +	if (vop2->pll_hdmiphy0 && mode->crtc_clock <= VOP2_MAX_DCLK_RATE) {
>>>>>>>> +		drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
>>>>>>>> +			struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
>>>>>>>> +
>>>>>>>> +			if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI0) {
>>>>>>>> +				if (!vp->dclk_src)
>>>>>>>> +					vp->dclk_src = clk_get_parent(vp->dclk);
>>>>>>>> +
>>>>>>>> +				ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy0);
>>>>>>>> +				if (ret < 0)
>>>>>>>> +					drm_warn(vop2->drm,
>>>>>>>> +						 "Could not switch to HDMI0 PHY PLL: %d\n", ret);
>>>>>>>> +				break;
>>>>>>>> +			}
>>>>>>>> +		}
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>
>>>>>>> It seems pretty fragile to do it at atomic_enable time, even more so
>>>>>>> since you don't lock the parent either.
>>>>>>>
>>>>>>> Any reason not to do it in the DRM or clock driver probe, and make sure
>>>>>>> you never change the parent somehow?
>>>>>>
>>>>>> On rk3588 we have 3 dclk_s and 2 hdmi controllers. Each video-port can
>>>>>> use the clock generated from either the hdmi0phy or hdmi1phy, depending
>>>>>> on which hdmi-controller it uses.
>>>>>>
>>>>>> So you actually need to know which vpX will output to which hdmiY to then
>>>>>> reparent that dclk to the hdmiphy output.
>>>>>
>>>>> The Rockchip nomenclature isn't super obvious to me, sorry. Is there a
>>>>> datasheet for this somewhere? Also, does this vpX -> HDMI-Y mapping need
>>>>> to be dynamic?
>>>>
>>>> VPs are CRTCs in drm-language and each of them can drive a differing
>>>> number of output encoders. Those video-ports also have differing output
>>>> characteristics in terms of supported resolution and other properties.
>>>>
>>>> The rk3588 TRM has leaked in a number of places, and if you find a
>>>> TRM-part2, there is a section labeled "Display Output Interface Description"
>>>> that has a nice graphic for that.
>>>>
>>>> Or in short:
>>>> - CRTC(VP)0 supports 8K resolution and can drive DP0+1, HDMI0+1, eDP0+1
>>>>   [if I'm reading things correctly, 8K together with CRTC1 somehow)
>>>> - CRTC(VP)1 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP0+1
>>>> - CRTC(VP)2 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP01, DSI0+1
>>>> - CRTC(VP)3 supports 2K resolution and can drive DSI0+1 and some BT1120,BT656
>>>>
>>>> so for the 3 higher resolution CRTCs there are essentially 6 or 8 output options
>>>> depending on the board design
>>>
>>> That's much clearer, thanks. I'm not entirely sure how that links to the
>>> need for the PLL to change its parent depending on the ouput. Do you
>>> need to always have all the outputs on the same PLL?
>>
>> One of the problems is that the PHY PLLs cannot be used as clock sources
>> for resolutions above 4K@60Hz, while VOP2 on RK3588 supports up to 8K@60Hz,
>> which is supposed to be handled by the system CRU.
> 
> But can that system CRU drive resolutions lower than 4k@60? If so, why
> do we bother with PHYs?

It can, but it's not accurate enough to support all modes, e.g. those
having fractional refresh rates, among others.  That's actually the
reason we'd want to make use of the PHY PLLs.

>> Moreover, the 2 PLLs are shared between 3 out of the 4 video ports already
>> mentioned by Heiko.  There is quite a bit of complexity in downstream
>> driver to handle all possible usecases - see [1] for a brief description on
>> how was that designed to work.
> 
> That's a generic allocation issue. Multiple drivers (vc4 for example)
> has this issue for other components. It can be fairly easily dealt with
> at atomic_check time.
> 
> Maxime

Re:Re: [PATCH v2 3/5] drm/rockchip: vop2: Improve display modes handling on RK3588 HDMI0

[Andy Yan]

Hi,

在 2024-12-18 00:59:57，"Cristian Ciocaltea" <cristian.ciocaltea@collabora.com> 写道：
>On 12/17/24 6:53 PM, Maxime Ripard wrote:
>> On Tue, Dec 17, 2024 at 06:36:41PM +0200, Cristian Ciocaltea wrote:
>>> On 12/17/24 5:00 PM, Maxime Ripard wrote:
>>>> On Wed, Dec 11, 2024 at 07:01:15PM +0100, Heiko Stübner wrote:
>>>>> Am Mittwoch, 11. Dezember 2024, 18:47:44 CET schrieb Maxime Ripard:
>>>>>> On Wed, Dec 11, 2024 at 06:23:03PM +0100, Heiko Stübner wrote:
>>>>>>> Am Mittwoch, 11. Dezember 2024, 18:07:57 CET schrieb Maxime Ripard:
>>>>>>>> On Wed, Dec 11, 2024 at 12:15:07PM +0200, Cristian Ciocaltea wrote:
>>>>>>>>> The RK3588 specific implementation is currently quite limited in terms
>>>>>>>>> of handling the full range of display modes supported by the connected
>>>>>>>>> screens, e.g. 2560x1440@75Hz, 2048x1152@60Hz, 1024x768@60Hz are just a
>>>>>>>>> few of them.
>>>>>>>>>
>>>>>>>>> Additionally, it doesn't cope well with non-integer refresh rates like
>>>>>>>>> 59.94, 29.97, 23.98, etc.
>>>>>>>>>
>>>>>>>>> Make use of HDMI0 PHY PLL as a more accurate DCLK source to handle
>>>>>>>>> all display modes up to 4K@60Hz.
>>>>>>>>>
>>>>>>>>> Tested-by: FUKAUMI Naoki <naoki@radxa.com>
>>>>>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>>>>>>> ---
>>>>>>>>>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 34 ++++++++++++++++++++++++++++
>>>>>>>>>  1 file changed, 34 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>>>>>> index 8b2f53ffefdbf1cc8737b3a86e630a03a7fd9348..393fe6aa170aaee9663c4a6d98c1cd6a5ef79392 100644
>>>>>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>>>>>> @@ -158,6 +158,7 @@ struct vop2_video_port {
>>>>>>>>>  	struct drm_crtc crtc;
>>>>>>>>>  	struct vop2 *vop2;
>>>>>>>>>  	struct clk *dclk;
>>>>>>>>> +	struct clk *dclk_src;
>>>>>>>>>  	unsigned int id;
>>>>>>>>>  	const struct vop2_video_port_data *data;
>>>>>>>>>  
>>>>>>>>> @@ -212,6 +213,7 @@ struct vop2 {
>>>>>>>>>  	struct clk *hclk;
>>>>>>>>>  	struct clk *aclk;
>>>>>>>>>  	struct clk *pclk;
>>>>>>>>> +	struct clk *pll_hdmiphy0;
>>>>>>>>>  
>>>>>>>>>  	/* optional internal rgb encoder */
>>>>>>>>>  	struct rockchip_rgb *rgb;
>>>>>>>>> @@ -220,6 +222,8 @@ struct vop2 {
>>>>>>>>>  	struct vop2_win win[];
>>>>>>>>>  };
>>>>>>>>>  
>>>>>>>>> +#define VOP2_MAX_DCLK_RATE		600000 /* kHz */
>>>>>>>>> +
>>>>>>>>>  #define vop2_output_if_is_hdmi(x)	((x) == ROCKCHIP_VOP2_EP_HDMI0 || \
>>>>>>>>>  					 (x) == ROCKCHIP_VOP2_EP_HDMI1)
>>>>>>>>>  
>>>>>>>>> @@ -1033,6 +1037,9 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
>>>>>>>>>  
>>>>>>>>>  	vop2_crtc_disable_irq(vp, VP_INT_DSP_HOLD_VALID);
>>>>>>>>>  
>>>>>>>>> +	if (vp->dclk_src)
>>>>>>>>> +		clk_set_parent(vp->dclk, vp->dclk_src);
>>>>>>>>> +
>>>>>>>>>  	clk_disable_unprepare(vp->dclk);
>>>>>>>>>  
>>>>>>>>>  	vop2->enable_count--;
>>>>>>>>> @@ -2049,6 +2056,27 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>>>>>>>>>  
>>>>>>>>>  	vop2_vp_write(vp, RK3568_VP_MIPI_CTRL, 0);
>>>>>>>>>  
>>>>>>>>> +	/*
>>>>>>>>> +	 * Switch to HDMI PHY PLL as DCLK source for display modes up
>>>>>>>>> +	 * to 4K@60Hz, if available, otherwise keep using the system CRU.
>>>>>>>>> +	 */
>>>>>>>>> +	if (vop2->pll_hdmiphy0 && mode->crtc_clock <= VOP2_MAX_DCLK_RATE) {
>>>>>>>>> +		drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
>>>>>>>>> +			struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
>>>>>>>>> +
>>>>>>>>> +			if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI0) {
>>>>>>>>> +				if (!vp->dclk_src)
>>>>>>>>> +					vp->dclk_src = clk_get_parent(vp->dclk);
>>>>>>>>> +
>>>>>>>>> +				ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy0);
>>>>>>>>> +				if (ret < 0)
>>>>>>>>> +					drm_warn(vop2->drm,
>>>>>>>>> +						 "Could not switch to HDMI0 PHY PLL: %d\n", ret);
>>>>>>>>> +				break;
>>>>>>>>> +			}
>>>>>>>>> +		}
>>>>>>>>> +	}
>>>>>>>>> +
>>>>>>>>
>>>>>>>> It seems pretty fragile to do it at atomic_enable time, even more so
>>>>>>>> since you don't lock the parent either.
>>>>>>>>
>>>>>>>> Any reason not to do it in the DRM or clock driver probe, and make sure
>>>>>>>> you never change the parent somehow?
>>>>>>>
>>>>>>> On rk3588 we have 3 dclk_s and 2 hdmi controllers. Each video-port can
>>>>>>> use the clock generated from either the hdmi0phy or hdmi1phy, depending
>>>>>>> on which hdmi-controller it uses.
>>>>>>>
>>>>>>> So you actually need to know which vpX will output to which hdmiY to then
>>>>>>> reparent that dclk to the hdmiphy output.
>>>>>>
>>>>>> The Rockchip nomenclature isn't super obvious to me, sorry. Is there a
>>>>>> datasheet for this somewhere? Also, does this vpX -> HDMI-Y mapping need
>>>>>> to be dynamic?
>>>>>
>>>>> VPs are CRTCs in drm-language and each of them can drive a differing
>>>>> number of output encoders. Those video-ports also have differing output
>>>>> characteristics in terms of supported resolution and other properties.
>>>>>
>>>>> The rk3588 TRM has leaked in a number of places, and if you find a
>>>>> TRM-part2, there is a section labeled "Display Output Interface Description"
>>>>> that has a nice graphic for that.
>>>>>
>>>>> Or in short:
>>>>> - CRTC(VP)0 supports 8K resolution and can drive DP0+1, HDMI0+1, eDP0+1
>>>>>   [if I'm reading things correctly, 8K together with CRTC1 somehow)
>>>>> - CRTC(VP)1 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP0+1
>>>>> - CRTC(VP)2 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP01, DSI0+1
>>>>> - CRTC(VP)3 supports 2K resolution and can drive DSI0+1 and some BT1120,BT656
>>>>>
>>>>> so for the 3 higher resolution CRTCs there are essentially 6 or 8 output options
>>>>> depending on the board design
>>>>
>>>> That's much clearer, thanks. I'm not entirely sure how that links to the
>>>> need for the PLL to change its parent depending on the ouput. Do you
>>>> need to always have all the outputs on the same PLL?
>>>
>>> One of the problems is that the PHY PLLs cannot be used as clock sources
>>> for resolutions above 4K@60Hz, while VOP2 on RK3588 supports up to 8K@60Hz,
>>> which is supposed to be handled by the system CRU.
>> 
>> But can that system CRU drive resolutions lower than 4k@60? If so, why
>> do we bother with PHYs?
>
>It can, but it's not accurate enough to support all modes, e.g. those
>having fractional refresh rates, among others.  That's actually the
>reason we'd want to make use of the PHY PLLs.

Not only that. For resolutions lower than 4k@60, if we use system cur as dclk parent,
it can't  sync with HDMI TMDS clock , this could lead to stability/compatibility i issues,
which may not be easy to encounter, but we have indeed experienced them  few timems
in a large amount of productization practice.

>
>>> Moreover, the 2 PLLs are shared between 3 out of the 4 video ports already
>>> mentioned by Heiko.  There is quite a bit of complexity in downstream
>>> driver to handle all possible usecases - see [1] for a brief description on
>>> how was that designed to work.
>> 
>> That's a generic allocation issue. Multiple drivers (vc4 for example)
>> has this issue for other components. It can be fairly easily dealt with
>> at atomic_check time.
>> 
>> Maxime
>

Re: [PATCH v2 3/5] drm/rockchip: vop2: Improve display modes handling on RK3588 HDMI0

[Cristian Ciocaltea]

Hi Maxime,

On 12/18/24 3:36 AM, Andy Yan wrote:
> 
> Hi,
> 
> 在 2024-12-18 00:59:57，"Cristian Ciocaltea" <cristian.ciocaltea@collabora.com> 写道：
>> On 12/17/24 6:53 PM, Maxime Ripard wrote:
>>> On Tue, Dec 17, 2024 at 06:36:41PM +0200, Cristian Ciocaltea wrote:
>>>> On 12/17/24 5:00 PM, Maxime Ripard wrote:
>>>>> On Wed, Dec 11, 2024 at 07:01:15PM +0100, Heiko Stübner wrote:
>>>>>> Am Mittwoch, 11. Dezember 2024, 18:47:44 CET schrieb Maxime Ripard:
>>>>>>> On Wed, Dec 11, 2024 at 06:23:03PM +0100, Heiko Stübner wrote:
>>>>>>>> Am Mittwoch, 11. Dezember 2024, 18:07:57 CET schrieb Maxime Ripard:
>>>>>>>>> On Wed, Dec 11, 2024 at 12:15:07PM +0200, Cristian Ciocaltea wrote:
>>>>>>>>>> The RK3588 specific implementation is currently quite limited in terms
>>>>>>>>>> of handling the full range of display modes supported by the connected
>>>>>>>>>> screens, e.g. 2560x1440@75Hz, 2048x1152@60Hz, 1024x768@60Hz are just a
>>>>>>>>>> few of them.
>>>>>>>>>>
>>>>>>>>>> Additionally, it doesn't cope well with non-integer refresh rates like
>>>>>>>>>> 59.94, 29.97, 23.98, etc.
>>>>>>>>>>
>>>>>>>>>> Make use of HDMI0 PHY PLL as a more accurate DCLK source to handle
>>>>>>>>>> all display modes up to 4K@60Hz.
>>>>>>>>>>
>>>>>>>>>> Tested-by: FUKAUMI Naoki <naoki@radxa.com>
>>>>>>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>>>>>>>> ---
>>>>>>>>>>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 34 ++++++++++++++++++++++++++++
>>>>>>>>>>  1 file changed, 34 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>>>>>>> index 8b2f53ffefdbf1cc8737b3a86e630a03a7fd9348..393fe6aa170aaee9663c4a6d98c1cd6a5ef79392 100644
>>>>>>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>>>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>>>>>>> @@ -158,6 +158,7 @@ struct vop2_video_port {
>>>>>>>>>>  	struct drm_crtc crtc;
>>>>>>>>>>  	struct vop2 *vop2;
>>>>>>>>>>  	struct clk *dclk;
>>>>>>>>>> +	struct clk *dclk_src;
>>>>>>>>>>  	unsigned int id;
>>>>>>>>>>  	const struct vop2_video_port_data *data;
>>>>>>>>>>  
>>>>>>>>>> @@ -212,6 +213,7 @@ struct vop2 {
>>>>>>>>>>  	struct clk *hclk;
>>>>>>>>>>  	struct clk *aclk;
>>>>>>>>>>  	struct clk *pclk;
>>>>>>>>>> +	struct clk *pll_hdmiphy0;
>>>>>>>>>>  
>>>>>>>>>>  	/* optional internal rgb encoder */
>>>>>>>>>>  	struct rockchip_rgb *rgb;
>>>>>>>>>> @@ -220,6 +222,8 @@ struct vop2 {
>>>>>>>>>>  	struct vop2_win win[];
>>>>>>>>>>  };
>>>>>>>>>>  
>>>>>>>>>> +#define VOP2_MAX_DCLK_RATE		600000 /* kHz */
>>>>>>>>>> +
>>>>>>>>>>  #define vop2_output_if_is_hdmi(x)	((x) == ROCKCHIP_VOP2_EP_HDMI0 || \
>>>>>>>>>>  					 (x) == ROCKCHIP_VOP2_EP_HDMI1)
>>>>>>>>>>  
>>>>>>>>>> @@ -1033,6 +1037,9 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
>>>>>>>>>>  
>>>>>>>>>>  	vop2_crtc_disable_irq(vp, VP_INT_DSP_HOLD_VALID);
>>>>>>>>>>  
>>>>>>>>>> +	if (vp->dclk_src)
>>>>>>>>>> +		clk_set_parent(vp->dclk, vp->dclk_src);
>>>>>>>>>> +
>>>>>>>>>>  	clk_disable_unprepare(vp->dclk);
>>>>>>>>>>  
>>>>>>>>>>  	vop2->enable_count--;
>>>>>>>>>> @@ -2049,6 +2056,27 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>>>>>>>>>>  
>>>>>>>>>>  	vop2_vp_write(vp, RK3568_VP_MIPI_CTRL, 0);
>>>>>>>>>>  
>>>>>>>>>> +	/*
>>>>>>>>>> +	 * Switch to HDMI PHY PLL as DCLK source for display modes up
>>>>>>>>>> +	 * to 4K@60Hz, if available, otherwise keep using the system CRU.
>>>>>>>>>> +	 */
>>>>>>>>>> +	if (vop2->pll_hdmiphy0 && mode->crtc_clock <= VOP2_MAX_DCLK_RATE) {
>>>>>>>>>> +		drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
>>>>>>>>>> +			struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
>>>>>>>>>> +
>>>>>>>>>> +			if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI0) {
>>>>>>>>>> +				if (!vp->dclk_src)
>>>>>>>>>> +					vp->dclk_src = clk_get_parent(vp->dclk);
>>>>>>>>>> +
>>>>>>>>>> +				ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy0);
>>>>>>>>>> +				if (ret < 0)
>>>>>>>>>> +					drm_warn(vop2->drm,
>>>>>>>>>> +						 "Could not switch to HDMI0 PHY PLL: %d\n", ret);
>>>>>>>>>> +				break;
>>>>>>>>>> +			}
>>>>>>>>>> +		}
>>>>>>>>>> +	}
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> It seems pretty fragile to do it at atomic_enable time, even more so
>>>>>>>>> since you don't lock the parent either.
>>>>>>>>>
>>>>>>>>> Any reason not to do it in the DRM or clock driver probe, and make sure
>>>>>>>>> you never change the parent somehow?
>>>>>>>>
>>>>>>>> On rk3588 we have 3 dclk_s and 2 hdmi controllers. Each video-port can
>>>>>>>> use the clock generated from either the hdmi0phy or hdmi1phy, depending
>>>>>>>> on which hdmi-controller it uses.
>>>>>>>>
>>>>>>>> So you actually need to know which vpX will output to which hdmiY to then
>>>>>>>> reparent that dclk to the hdmiphy output.
>>>>>>>
>>>>>>> The Rockchip nomenclature isn't super obvious to me, sorry. Is there a
>>>>>>> datasheet for this somewhere? Also, does this vpX -> HDMI-Y mapping need
>>>>>>> to be dynamic?
>>>>>>
>>>>>> VPs are CRTCs in drm-language and each of them can drive a differing
>>>>>> number of output encoders. Those video-ports also have differing output
>>>>>> characteristics in terms of supported resolution and other properties.
>>>>>>
>>>>>> The rk3588 TRM has leaked in a number of places, and if you find a
>>>>>> TRM-part2, there is a section labeled "Display Output Interface Description"
>>>>>> that has a nice graphic for that.
>>>>>>
>>>>>> Or in short:
>>>>>> - CRTC(VP)0 supports 8K resolution and can drive DP0+1, HDMI0+1, eDP0+1
>>>>>>   [if I'm reading things correctly, 8K together with CRTC1 somehow)
>>>>>> - CRTC(VP)1 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP0+1
>>>>>> - CRTC(VP)2 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP01, DSI0+1
>>>>>> - CRTC(VP)3 supports 2K resolution and can drive DSI0+1 and some BT1120,BT656
>>>>>>
>>>>>> so for the 3 higher resolution CRTCs there are essentially 6 or 8 output options
>>>>>> depending on the board design
>>>>>
>>>>> That's much clearer, thanks. I'm not entirely sure how that links to the
>>>>> need for the PLL to change its parent depending on the ouput. Do you
>>>>> need to always have all the outputs on the same PLL?
>>>>
>>>> One of the problems is that the PHY PLLs cannot be used as clock sources
>>>> for resolutions above 4K@60Hz, while VOP2 on RK3588 supports up to 8K@60Hz,
>>>> which is supposed to be handled by the system CRU.
>>>
>>> But can that system CRU drive resolutions lower than 4k@60? If so, why
>>> do we bother with PHYs?
>>
>> It can, but it's not accurate enough to support all modes, e.g. those
>> having fractional refresh rates, among others.  That's actually the
>> reason we'd want to make use of the PHY PLLs.
> 
> Not only that. For resolutions lower than 4k@60, if we use system cur as dclk parent,
> it can't  sync with HDMI TMDS clock , this could lead to stability/compatibility i issues,
> which may not be easy to encounter, but we have indeed experienced them  few timems
> in a large amount of productization practice.

Could you please indicate which would be the required changes to make
this acceptable upstream and unblock the series?

Thanks,
Cristian

>>>> Moreover, the 2 PLLs are shared between 3 out of the 4 video ports already
>>>> mentioned by Heiko.  There is quite a bit of complexity in downstream
>>>> driver to handle all possible usecases - see [1] for a brief description on
>>>> how was that designed to work.
>>>
>>> That's a generic allocation issue. Multiple drivers (vc4 for example)
>>> has this issue for other components. It can be fairly easily dealt with
>>> at atomic_check time.
>>>
>>> Maxime
>>