On 6/5/24 12:49 PM, Andy Yan wrote: > > 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. 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
