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@xxxxxxxxx>
>>>>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@xxxxxxxxxxxxx>
>>>>>>>> ---
>>>>>>>> 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
