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

[Cristian Ciocaltea <cristian.ciocaltea@collabora.com>]

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
>>


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip