Hi Maxime, On 12/18/24 3:36 AM, Andy Yan wrote: > > Hi, > > 在 2024-12-18 00:59:57，"Cristian Ciocaltea" <[email protected]> > 写道： >> 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 <[email protected]> >>>>>>>>>> Signed-off-by: Cristian Ciocaltea <[email protected]> >>>>>>>>>> --- >>>>>>>>>> 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 >>
