Message-ID: <d17341c2-3f97-41d6-8125-bff838149798@collabora.com>
Date: Wed, 5 Jun 2024 14:20:04 +0300
From: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
To: Andy Yan <andyshrk@....com>, Maxime Ripard <mripard@...nel.org>
Cc: neil.armstrong@...aro.org, Heiko Stuebner <heiko@...ech.de>,
 Andrzej Hajda <andrzej.hajda@...el.com>, Robert Foss <rfoss@...nel.org>,
 Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
 Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
 Daniel Vetter <daniel@...ll.ch>, Sandy Huang <hjc@...k-chips.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Mark Yao <markyao0591@...il.com>,
 Andy Yan <andy.yan@...k-chips.com>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, devicetree@...r.kernel.org,
 kernel@...labora.com, Alexandre ARNOUD <aarnoud@...com>,
 Luis de Arquer <ldearquer@...il.com>, Algea Cao <algea.cao@...k-chips.com>
Subject: Re: [PATCH 00/14] Add initial support for the Rockchip RK3588 HDMI TX
 Controller

On 6/5/24 12:49 PM, Andy Yan wrote:
> 
> Hi,
> At 2024-06-05 17:39:48, "Maxime Ripard" <mripard@...nel.org> wrote:
>> On Wed, Jun 05, 2024 at 11:28:41AM GMT, neil.armstrong@...aro.org wrote:
>>> Hi,
>>>
>>> On 05/06/2024 11:25, Andy Yan wrote:
>>>>
>>>> Hi,
>>>>
>>>> At 2024-06-05 04:33:57, "Cristian Ciocaltea" <cristian.ciocaltea@...labora.com> wrote:
>>>>> On 6/3/24 4:08 PM, neil.armstrong@...aro.org wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 03/06/2024 15:03, Heiko Stuebner wrote:
>>>>>>> Am Montag, 3. Juni 2024, 14:14:17 CEST schrieb Andy Yan:
>>>>>>>> Hi Neil:
>>>>>>>>
>>>>>>>> On 6/3/24 16:55, Neil Armstrong wrote:
>>>>>>>>> Hi Christian,
>>>>>>>>>
>>>>>>>>> On 01/06/2024 15:12, Cristian Ciocaltea wrote:
>>>>>>>>>> The RK3588 SoC family integrates a Quad-Pixel (QP) variant of the
>>>>>>>>>> Synopsys DesignWare HDMI TX controller used in the previous SoCs.
>>>>>>>>>>
>>>>>>>>>> It is HDMI 2.1 compliant and supports the following features, among
>>>>>>>>>> others:
>>>>>>>>>>
>>>>>>>>> .
>>>>>>>>>
>>>>>>>>> ..
>>>>>>>>>
>>>>>>>>>> * SCDC I2C DDC access
>>>>>>>>>> * TMDS Scrambler enabling 2160p@...z with RGB/YCbCr4:4:4
>>>>>>>>>> * YCbCr4:2:0 enabling 2160p@...z at lower HDMI link speeds
>>>>>>>>>> * Multi-stream audio
>>>>>>>>>> * Enhanced Audio Return Channel (EARC)
>>>>>>>>> -> Those features were already supported by the HDMI 2.0a compliant
>>>>>>>>> HW, just
>>>>>>>>> list the _new_ features for HDMI 2.1
>>>>>>>>>
>>>>>>>>> I did a quick review of your patchset and I don't understand why you
>>>>>>>>> need
>>>>>>>>> to add a separate dw-hdmi-qp.c since you only need simple variants
>>>>>>>>> of the I2C
>>>>>>>>> bus, infoframe and bridge setup.
>>>>>>>>>
>>>>>>>>> Can you elaborate further ? isn't this Quad-Pixel (QP) TX controller
>>>>>>>>> version
>>>>>>>>> detectable at runtime ?
>>>>>>>>>
>>>>>>>>> I would prefer to keep a single dw-hdmi driver if possible.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> The QP HDMI controller is a completely different variant with totally
>>>>>>>> different
>>>>>>>> registers layout, see PATCH 13/14.
>>>>>>>> I think make it a separate driver will be easier for development and
>>>>>>>> maintenance.
>>>>>>>
>>>>>>> I'm with Andy here. Trying to navigate a driver for two IP blocks really
>>>>>>> sounds taxing especially when both are so different.
>>>>>
>>>>> Thank you all for the valuable feedback!
>>>>>
>>>>>> I agree, I just wanted more details than "variant of the
>>>>>> Synopsys DesignWare HDMI TX controller", if the register mapping is 100%
>>>>>> different, and does not match at all with the old IP, then it's indeed time
>>>>>> to make a brand new driver, but instead of doing a mix up, it's time to
>>>>>> extract
>>>>>> the dw-hdmi code that could be common helpers into a dw-hdmi-common module
>>>>>> and use them.
>>>>>
>>>>> Sounds good, will handle this in v2.
>>>>>
>>>>>> As I see, no "driver" code can be shared, only DRM plumbings, so perhaps
>>>>>> those
>>>>>> plumbing code should go into the DRM core ?
>>>>>>
>>>>>> In any case, please add more details on the cover letter, including the
>>>>>> detailed
>>>>>> HW differrence and the design you chose so support this new IP.
>>>>>
>>>>> Andy, could you please help with a summary of the HW changes?
>>>>> The information I could provide is rather limited, since I don't have
>>>>> access to any DW IP datasheets and I'm also not familiar enough with the
>>>>> old variant.
>>>>>
>>>>   Accurately, we should refer to it as an entirely new IP，it has nothing in common with
>>>> the current mainline dw-hdmi。 The only  commonality is that they both come from
>>>> Synopsys DesignWare：
>>>> （1）It has a 100% different register mapping
>>>> （2）It supports FRL and DSC
>>>> （3）different configuration flow in many places。
>>>>
>>>> So I have the same feeling with Heiko and Maxime：
>>>> The DW_HDMI_QP should have a  separate driver and with it's  own CONFIG  such as DRM_DW_HDMI_QP  in Kconfig.
>>>> and the rockchip part should also be split from dw_hdmi-rockchip.c.
>>>> I am sorry we mixed them in dw_hdmi-rockchip.c when we develop the bsp driver，but we really regretted this decision
>>>> when  we repeatedly broke compatibility with dw-hdmi on other socs。
>>>
>>> Yes please, and as I say, if there's code common with the old dw-hdmi, please add a common
>>> module if this code can't be moved in core bridge helpers.
>>
>> And chances are that the common code is actually there to deal with HDMI
>> spec itself and not really the hardware, which is solved by moving both
>> drivers to the HDMI helpers that just got merged.

I will make use of the new HDMI helpers and see if there is anything
else remaining in terms of common code.

> Yes, +1.
> I don't think we need to share some common code with dw-hdmi here.

Ok, I will completely separate the new driver's code, including the
Rockchip glue layer.

Thanks,
Cristian

Powered by blists - more mailing lists