Re:Re: [PATCH 00/14] Add initial support for the Rockchip RK3588 HDMI TX Controller

[Andy Yan]

Hi,
At 2024-06-05 17:39:48, "Maxime Ripard" <mripard@xxxxxxxxxx> wrote:
>On Wed, Jun 05, 2024 at 11:28:41AM GMT, neil.armstrong@xxxxxxxxxx wrote:
>> Hi,
>> 
>> On 05/06/2024 11:25, Andy Yan wrote:
>> > 
>> > Hi,
>> > 
>> > At 2024-06-05 04:33:57, "Cristian Ciocaltea" <cristian.ciocaltea@xxxxxxxxxxxxx> wrote:
>> > > On 6/3/24 4:08 PM, neil.armstrong@xxxxxxxxxx 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@60Hz with RGB/YCbCr4:4:4
>> > > > > > > > * YCbCr4:2:0 enabling 2160p@60Hz 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.
>

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

>Maxime