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
