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

[Maxime Ripard]

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.

Maxime

Attachment: signature.asc
Description: PGP signature