diff mbox series

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

Message ID 20241116-vop2-hdmi0-disp-modes-v1-3-2bca51db4898@collabora.com (mailing list archive)
State New, archived
Headers show
Series Improve Rockchip VOP2 display modes handling on RK3588 HDMI0 | expand

Commit Message

Cristian Ciocaltea Nov. 16, 2024, 6:22 p.m. UTC 
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.

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

Comments

Jonas Karlman Nov. 16, 2024, 7:12 p.m. UTC | #1 
Hi Cristian,

On 2024-11-16 19:22, 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.
> 
> 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 3e4c1cfd0bac6fa90f4cab85e27c2a69b86fc9aa..dfe1a50132d596f036430d7db3631398d0802972 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)
>  
> @@ -1103,6 +1107,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--;
> @@ -2192,6 +2199,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;
> +			}
> +		}
> +	}

Why do we need to do this dynamically here?

The device tree set PLL_HPLL as parent:

&vop {
	assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>;
	assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>;
	status = "okay";
};

Could this not just be changed to assign hdptxphy_hdmi0 as parent?

&vop {
	assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>;
	assigned-clock-parents = <&hdptxphy_hdmi0>, <&cru PLL_VPLL>;
	status = "okay";
};

or something similar?

For RK3328 the vop dclk parent is assigned to hdmiphy using DT.

Regards,
Jonas

> +
>  	clk_set_rate(vp->dclk, clock);
>  
>  	vop2_post_config(crtc);
> @@ -3355,6 +3383,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");
>
Cristian Ciocaltea Nov. 16, 2024, 11:22 p.m. UTC | #2 
Hi Jonas,

On 11/16/24 9:12 PM, Jonas Karlman wrote:
> Hi Cristian,
> 
> On 2024-11-16 19:22, 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.
>>
>> 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 3e4c1cfd0bac6fa90f4cab85e27c2a69b86fc9aa..dfe1a50132d596f036430d7db3631398d0802972 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c

[...]

>> +	/*
>> +	 * 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;
>> +			}
>> +		}
>> +	}
> 
> Why do we need to do this dynamically here?
> 
> The device tree set PLL_HPLL as parent:
> 
> &vop {
> 	assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>;
> 	assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>;
> 	status = "okay";
> };
> 
> Could this not just be changed to assign hdptxphy_hdmi0 as parent?
> 
> &vop {
> 	assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>;
> 	assigned-clock-parents = <&hdptxphy_hdmi0>, <&cru PLL_VPLL>;
> 	status = "okay";
> };
> 
> or something similar?
> 
> For RK3328 the vop dclk parent is assigned to hdmiphy using DT.

Yes, that would normally work.  The problem is that the PHY PLLs cannot
provide pixel clocks for resolutions above 4K@60Hz (hence limited to
HDMI 2.0), while VOP2 on RK3588 supports up to 8K@60Hz (making use of
HDMI 2.1).

On top of that, the 2 PLLs are shared between 3 out of the 4 video ports
of the display controller.  There is quite a bit of complexity in
downstream driver to handle all possible usecases - see [1] for a brief
description on how is that supposed 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
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 3e4c1cfd0bac6fa90f4cab85e27c2a69b86fc9aa..dfe1a50132d596f036430d7db3631398d0802972 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)
 
@@ -1103,6 +1107,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--;
@@ -2192,6 +2199,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);
@@ -3355,6 +3383,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");