| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8dff095.1555.193d768ad5e.Coremail.andyshrk@163.com>
Date: Wed, 18 Dec 2024 09:36:21 +0800 (CST)
From: "Andy Yan" <andyshrk@....com>
To: "Cristian Ciocaltea" <cristian.ciocaltea@...labora.com>
Cc: "Maxime Ripard" <mripard@...nel.org>,
Heiko Stübner <heiko@...ech.de>,
"Sandy Huang" <hjc@...k-chips.com>,
"Andy Yan" <andy.yan@...k-chips.com>,
"Maarten Lankhorst" <maarten.lankhorst@...ux.intel.com>,
"Thomas Zimmermann" <tzimmermann@...e.de>,
"David Airlie" <airlied@...il.com>,
"Simona Vetter" <simona@...ll.ch>, "Rob Herring" <robh@...nel.org>,
"Krzysztof Kozlowski" <krzk+dt@...nel.org>,
"Conor Dooley" <conor+dt@...nel.org>, kernel@...labora.com,
dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
"FUKAUMI Naoki" <naoki@...xa.com>,
"Algea Cao" <algea.cao@...k-chips.com>
Subject: Re:Re: [PATCH v2 3/5] drm/rockchip: vop2: Improve display modes
handling on RK3588 HDMI0
Hi,
在 2024-12-18 00:59:57，"Cristian Ciocaltea" <cristian.ciocaltea@...labora.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@...z, 2048x1152@...z, 1024x768@...z 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@...z.
>>>>>>>>>
>>>>>>>>> Tested-by: FUKAUMI Naoki <naoki@...xa.com>
>>>>>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.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@...z, 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@...z, while VOP2 on RK3588 supports up to 8K@...z,
>>> 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
>
Powered by blists - more mailing lists