mbox series

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

Message ID 20240601-b4-rk3588-bridge-upstream-v1-0-f6203753232b@collabora.com (mailing list archive)
Headers show
Series Add initial support for the Rockchip RK3588 HDMI TX Controller | expand

Message

Cristian Ciocaltea June 1, 2024, 1:12 p.m. UTC
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:

* Fixed Rate Link (FRL)
* 4K@120Hz and 8K@60Hz video modes
* Variable Refresh Rate (VRR) including Quick Media Switching (QMS)
* Fast Vactive (FVA)
* 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)

This is the last required component that needs to be supported in order
to enable the HDMI output functionality on the RK3588 based SBCs, such
as the RADXA Rock 5B. The other components are the Video Output
Processor (VOP2) and the Samsung IP based HDMI/eDP TX Combo PHY, for
which basic support has been already made available via [1] and [2],
respectively.

The patches are grouped as follows:
* PATCH 1..7: DW HDMI TX driver refactor to minimize code duplication in
  the new QP driver (no functional changes intended)

* PATCH 8..11: Rockchip DW HDMI glue driver cleanup/improvements (no
  functional changes intended)

* PATCH 12..13: The new DW HDMI QP TX driver reusing the previously
  exported functions and structs from existing DW HDMI TX driver

* PATCH 14: Rockchip DW HDMI glue driver update to support RK3588 and
  make use of DW HDMI QP TX

They provide just the basic HDMI support for now, i.e. RGB output up to
4K@60Hz, without audio, CEC or any of the HDMI 2.1 specific features.
Also note the vop2 driver is currently not able to properly handle all
display modes supported by the connected screens, e.g. it doesn't cope
with non-integer refresh rates.

A possible workaround consists of enabling the display controller to
make use of the clock provided by the HDMI PHY PLL. This is still work
in progress and will be submitted later, as well as the required DTS
updates.

To facilitate testing and experimentation, all HDMI output related
patches, including those part of this series, are available at [3].
So far I could only verify this on the RADXA Rock 3A and 5B boards.

Thanks,
Cristian

[1]: 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
[2]: 553be2830c5f ("phy: rockchip: Add Samsung HDMI/eDP Combo PHY driver")
[3]: https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/commits/rk3588-hdmi-bridge-v6.10-rc1

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
Cristian Ciocaltea (14):
      drm/bridge: dw-hdmi: Simplify clock handling
      drm/bridge: dw-hdmi: Add dw-hdmi-common.h header
      drm/bridge: dw-hdmi: Commonize dw_hdmi_i2c_adapter()
      drm/bridge: dw-hdmi: Factor out AVI infoframe setup
      drm/bridge: dw-hdmi: Factor out vmode setup
      drm/bridge: dw-hdmi: Factor out hdmi_data_info setup
      drm/bridge: dw-hdmi: Commonize dw_hdmi_connector_create()
      drm/rockchip: dw_hdmi: Use modern drm_device based logging
      drm/rockchip: dw_hdmi: Simplify clock handling
      drm/rockchip: dw_hdmi: Use devm_regulator_get_enable()
      drm/rockchip: dw_hdmi: Drop superfluous assignments of mpll_cfg, cur_ctr and phy_config
      dt-bindings: display: rockchip,dw-hdmi: Add compatible for RK3588
      drm/bridge: synopsys: Add DW HDMI QP TX controller driver
      drm/rockchip: dw_hdmi: Add basic RK3588 support

 .../display/rockchip/rockchip,dw-hdmi.yaml         | 127 +++-
 drivers/gpu/drm/bridge/synopsys/Makefile           |   2 +-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-common.h   | 179 +++++
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c       | 787 +++++++++++++++++++
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h       | 831 +++++++++++++++++++++
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          | 353 +++------
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        | 351 +++++++--
 include/drm/bridge/dw_hdmi.h                       |   8 +
 8 files changed, 2290 insertions(+), 348 deletions(-)
---
base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
change-id: 20240601-b4-rk3588-bridge-upstream-a27baff1b8fc

Comments

Dmitry Baryshkov June 1, 2024, 4:32 p.m. UTC | #1
On Sat, Jun 01, 2024 at 04:12:22PM +0300, 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:
> 
> * Fixed Rate Link (FRL)
> * 4K@120Hz and 8K@60Hz video modes
> * Variable Refresh Rate (VRR) including Quick Media Switching (QMS)
> * Fast Vactive (FVA)
> * 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)

It would be really nice if you can take a look at using the HDMI
connector framework (landed few days ago) with adaptations for the
drm_bridge / drm_bridge_connector ([1]). Your comments for the
drm_bridge patches would be defeinitely appreciated.

[1] https://lore.kernel.org/dri-devel/20240531-bridge-hdmi-connector-v4-0-5110f7943622@linaro.org/

> 
> This is the last required component that needs to be supported in order
> to enable the HDMI output functionality on the RK3588 based SBCs, such
> as the RADXA Rock 5B. The other components are the Video Output
> Processor (VOP2) and the Samsung IP based HDMI/eDP TX Combo PHY, for
> which basic support has been already made available via [1] and [2],
> respectively.
> 
> The patches are grouped as follows:
> * PATCH 1..7: DW HDMI TX driver refactor to minimize code duplication in
>   the new QP driver (no functional changes intended)
> 
> * PATCH 8..11: Rockchip DW HDMI glue driver cleanup/improvements (no
>   functional changes intended)
> 
> * PATCH 12..13: The new DW HDMI QP TX driver reusing the previously
>   exported functions and structs from existing DW HDMI TX driver
> 
> * PATCH 14: Rockchip DW HDMI glue driver update to support RK3588 and
>   make use of DW HDMI QP TX
> 
> They provide just the basic HDMI support for now, i.e. RGB output up to
> 4K@60Hz, without audio, CEC or any of the HDMI 2.1 specific features.
> Also note the vop2 driver is currently not able to properly handle all
> display modes supported by the connected screens, e.g. it doesn't cope
> with non-integer refresh rates.
> 
> A possible workaround consists of enabling the display controller to
> make use of the clock provided by the HDMI PHY PLL. This is still work
> in progress and will be submitted later, as well as the required DTS
> updates.
> 
> To facilitate testing and experimentation, all HDMI output related
> patches, including those part of this series, are available at [3].
> So far I could only verify this on the RADXA Rock 3A and 5B boards.
> 
> Thanks,
> Cristian
> 
> [1]: 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
> [2]: 553be2830c5f ("phy: rockchip: Add Samsung HDMI/eDP Combo PHY driver")
> [3]: https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/commits/rk3588-hdmi-bridge-v6.10-rc1
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
> Cristian Ciocaltea (14):
>       drm/bridge: dw-hdmi: Simplify clock handling
>       drm/bridge: dw-hdmi: Add dw-hdmi-common.h header
>       drm/bridge: dw-hdmi: Commonize dw_hdmi_i2c_adapter()
>       drm/bridge: dw-hdmi: Factor out AVI infoframe setup
>       drm/bridge: dw-hdmi: Factor out vmode setup
>       drm/bridge: dw-hdmi: Factor out hdmi_data_info setup
>       drm/bridge: dw-hdmi: Commonize dw_hdmi_connector_create()
>       drm/rockchip: dw_hdmi: Use modern drm_device based logging
>       drm/rockchip: dw_hdmi: Simplify clock handling
>       drm/rockchip: dw_hdmi: Use devm_regulator_get_enable()
>       drm/rockchip: dw_hdmi: Drop superfluous assignments of mpll_cfg, cur_ctr and phy_config
>       dt-bindings: display: rockchip,dw-hdmi: Add compatible for RK3588
>       drm/bridge: synopsys: Add DW HDMI QP TX controller driver
>       drm/rockchip: dw_hdmi: Add basic RK3588 support
> 
>  .../display/rockchip/rockchip,dw-hdmi.yaml         | 127 +++-
>  drivers/gpu/drm/bridge/synopsys/Makefile           |   2 +-
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi-common.h   | 179 +++++
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c       | 787 +++++++++++++++++++
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h       | 831 +++++++++++++++++++++
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          | 353 +++------
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        | 351 +++++++--
>  include/drm/bridge/dw_hdmi.h                       |   8 +
>  8 files changed, 2290 insertions(+), 348 deletions(-)
> ---
> base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
> change-id: 20240601-b4-rk3588-bridge-upstream-a27baff1b8fc
>
Piotr Oniszczuk June 2, 2024, 7:46 a.m. UTC | #2
Cristian,

I was awaiting over year for this work!

I’m devel. 2 distros where single mainline kernel serves 2835/2711/2712/h6/h313/h616/h618/rk3328/rk3399/rk3566/rk3568/rk3588/s905/s912/sm1/g12.

Before this work rk3588 was excluded because rk3588 hdmi was regressing hdmi on other socs.
With this code all other socs seems work ok now. Perfect.

As one of my project is multimedia appliance - good news is that now i can nicely play hdtv on rk3588 using mainline common 6.9.3 kernel and….started to hear from my users a lot of Qs like: „ah so nice! rk3588 now works nicely….but where is hdmi audio and cec?”

It will be fantastic to add (e.g. by backport Detlev https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/tree/rk3588-hdmi-audio?ref_type=heads <https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/tree/rk3588-hdmi-audio?ref_type=heads> ) audio code to get basic support hdmi audio?
   
thx again for fantastic work!


> Wiadomość napisana przez Cristian Ciocaltea <cristian.ciocaltea@collabora.com> w dniu 01.06.2024, o godz. 15:12:
> 
> 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:
> 
> * Fixed Rate Link (FRL)
> * 4K@120Hz and 8K@60Hz video modes
> * Variable Refresh Rate (VRR) including Quick Media Switching (QMS)
> * Fast Vactive (FVA)
> * 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)
> 
> This is the last required component that needs to be supported in order
> to enable the HDMI output functionality on the RK3588 based SBCs, such
> as the RADXA Rock 5B. The other components are the Video Output
> Processor (VOP2) and the Samsung IP based HDMI/eDP TX Combo PHY, for
> which basic support has been already made available via [1] and [2],
> respectively.
> 
> The patches are grouped as follows:
> * PATCH 1..7: DW HDMI TX driver refactor to minimize code duplication in
>  the new QP driver (no functional changes intended)
> 
> * PATCH 8..11: Rockchip DW HDMI glue driver cleanup/improvements (no
>  functional changes intended)
> 
> * PATCH 12..13: The new DW HDMI QP TX driver reusing the previously
>  exported functions and structs from existing DW HDMI TX driver
> 
> * PATCH 14: Rockchip DW HDMI glue driver update to support RK3588 and
>  make use of DW HDMI QP TX
> 
> They provide just the basic HDMI support for now, i.e. RGB output up to
> 4K@60Hz, without audio, CEC or any of the HDMI 2.1 specific features.
> Also note the vop2 driver is currently not able to properly handle all
> display modes supported by the connected screens, e.g. it doesn't cope
> with non-integer refresh rates.
> 
> A possible workaround consists of enabling the display controller to
> make use of the clock provided by the HDMI PHY PLL. This is still work
> in progress and will be submitted later, as well as the required DTS
> updates.
> 
> To facilitate testing and experimentation, all HDMI output related
> patches, including those part of this series, are available at [3].
> So far I could only verify this on the RADXA Rock 3A and 5B boards.
> 
> Thanks,
> Cristian
> 
> [1]: 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
> [2]: 553be2830c5f ("phy: rockchip: Add Samsung HDMI/eDP Combo PHY driver")
> [3]: https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/commits/rk3588-hdmi-bridge-v6.10-rc1
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
> Cristian Ciocaltea (14):
>      drm/bridge: dw-hdmi: Simplify clock handling
>      drm/bridge: dw-hdmi: Add dw-hdmi-common.h header
>      drm/bridge: dw-hdmi: Commonize dw_hdmi_i2c_adapter()
>      drm/bridge: dw-hdmi: Factor out AVI infoframe setup
>      drm/bridge: dw-hdmi: Factor out vmode setup
>      drm/bridge: dw-hdmi: Factor out hdmi_data_info setup
>      drm/bridge: dw-hdmi: Commonize dw_hdmi_connector_create()
>      drm/rockchip: dw_hdmi: Use modern drm_device based logging
>      drm/rockchip: dw_hdmi: Simplify clock handling
>      drm/rockchip: dw_hdmi: Use devm_regulator_get_enable()
>      drm/rockchip: dw_hdmi: Drop superfluous assignments of mpll_cfg, cur_ctr and phy_config
>      dt-bindings: display: rockchip,dw-hdmi: Add compatible for RK3588
>      drm/bridge: synopsys: Add DW HDMI QP TX controller driver
>      drm/rockchip: dw_hdmi: Add basic RK3588 support
> 
> .../display/rockchip/rockchip,dw-hdmi.yaml         | 127 +++-
> drivers/gpu/drm/bridge/synopsys/Makefile           |   2 +-
> drivers/gpu/drm/bridge/synopsys/dw-hdmi-common.h   | 179 +++++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c       | 787 +++++++++++++++++++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h       | 831 +++++++++++++++++++++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          | 353 +++------
> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        | 351 +++++++--
> include/drm/bridge/dw_hdmi.h                       |   8 +
> 8 files changed, 2290 insertions(+), 348 deletions(-)
> ---
> base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
> change-id: 20240601-b4-rk3588-bridge-upstream-a27baff1b8fc
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Piotr Oniszczuk June 2, 2024, 7:59 a.m. UTC | #3
(resent as plain text instead of html)
 
Cristian,

I was awaiting over year for this work!

I’m devel. 2 distros where single mainline kernel serves 2835/2711/2712/h6/h313/h616/h618/rk3328/rk3399/rk3566/rk3568/rk3588/s905/s912/sm1/g12.

Before this work rk3588 was excluded because rk3588 hdmi was regressing hdmi on other socs.
With this code all other socs seems work ok now. Perfect.

As one of my project is multimedia appliance - good news is that now i can nicely play hdtv on rk3588 using mainline common 6.9.3 kernel and….started to hear from my users a lot of Qs like: „ah so nice! rk3588 now works nicely….but where is hdmi audio and cec?”

It will be fantastic to add (e.g. by backport Detlev https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/tree/rk3588-hdmi-audio?ref_type=heads ) audio code to get basic support hdmi audio?
   
thx again for fantastic work!

> Wiadomość napisana przez Cristian Ciocaltea <cristian.ciocaltea@collabora.com> w dniu 01.06.2024, o godz. 15:12:
> 
> 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:
> 
> * Fixed Rate Link (FRL)
> * 4K@120Hz and 8K@60Hz video modes
> * Variable Refresh Rate (VRR) including Quick Media Switching (QMS)
> * Fast Vactive (FVA)
> * 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)
> 
> This is the last required component that needs to be supported in order
> to enable the HDMI output functionality on the RK3588 based SBCs, such
> as the RADXA Rock 5B. The other components are the Video Output
> Processor (VOP2) and the Samsung IP based HDMI/eDP TX Combo PHY, for
> which basic support has been already made available via [1] and [2],
> respectively.
> 
> The patches are grouped as follows:
> * PATCH 1..7: DW HDMI TX driver refactor to minimize code duplication in
>  the new QP driver (no functional changes intended)
> 
> * PATCH 8..11: Rockchip DW HDMI glue driver cleanup/improvements (no
>  functional changes intended)
> 
> * PATCH 12..13: The new DW HDMI QP TX driver reusing the previously
>  exported functions and structs from existing DW HDMI TX driver
> 
> * PATCH 14: Rockchip DW HDMI glue driver update to support RK3588 and
>  make use of DW HDMI QP TX
> 
> They provide just the basic HDMI support for now, i.e. RGB output up to
> 4K@60Hz, without audio, CEC or any of the HDMI 2.1 specific features.
> Also note the vop2 driver is currently not able to properly handle all
> display modes supported by the connected screens, e.g. it doesn't cope
> with non-integer refresh rates.
> 
> A possible workaround consists of enabling the display controller to
> make use of the clock provided by the HDMI PHY PLL. This is still work
> in progress and will be submitted later, as well as the required DTS
> updates.
> 
> To facilitate testing and experimentation, all HDMI output related
> patches, including those part of this series, are available at [3].
> So far I could only verify this on the RADXA Rock 3A and 5B boards.
> 
> Thanks,
> Cristian
> 
> [1]: 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
> [2]: 553be2830c5f ("phy: rockchip: Add Samsung HDMI/eDP Combo PHY driver")
> [3]: https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/commits/rk3588-hdmi-bridge-v6.10-rc1
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
> Cristian Ciocaltea (14):
>      drm/bridge: dw-hdmi: Simplify clock handling
>      drm/bridge: dw-hdmi: Add dw-hdmi-common.h header
>      drm/bridge: dw-hdmi: Commonize dw_hdmi_i2c_adapter()
>      drm/bridge: dw-hdmi: Factor out AVI infoframe setup
>      drm/bridge: dw-hdmi: Factor out vmode setup
>      drm/bridge: dw-hdmi: Factor out hdmi_data_info setup
>      drm/bridge: dw-hdmi: Commonize dw_hdmi_connector_create()
>      drm/rockchip: dw_hdmi: Use modern drm_device based logging
>      drm/rockchip: dw_hdmi: Simplify clock handling
>      drm/rockchip: dw_hdmi: Use devm_regulator_get_enable()
>      drm/rockchip: dw_hdmi: Drop superfluous assignments of mpll_cfg, cur_ctr and phy_config
>      dt-bindings: display: rockchip,dw-hdmi: Add compatible for RK3588
>      drm/bridge: synopsys: Add DW HDMI QP TX controller driver
>      drm/rockchip: dw_hdmi: Add basic RK3588 support
> 
> .../display/rockchip/rockchip,dw-hdmi.yaml         | 127 +++-
> drivers/gpu/drm/bridge/synopsys/Makefile           |   2 +-
> drivers/gpu/drm/bridge/synopsys/dw-hdmi-common.h   | 179 +++++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c       | 787 +++++++++++++++++++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h       | 831 +++++++++++++++++++++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          | 353 +++------
> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        | 351 +++++++--
> include/drm/bridge/dw_hdmi.h                       |   8 +
> 8 files changed, 2290 insertions(+), 348 deletions(-)
> ---
> base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
> change-id: 20240601-b4-rk3588-bridge-upstream-a27baff1b8fc
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Neil Armstrong June 3, 2024, 8:55 a.m. UTC | #4
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.

Thanks,
Neil

> 
> This is the last required component that needs to be supported in order
> to enable the HDMI output functionality on the RK3588 based SBCs, such
> as the RADXA Rock 5B. The other components are the Video Output
> Processor (VOP2) and the Samsung IP based HDMI/eDP TX Combo PHY, for
> which basic support has been already made available via [1] and [2],
> respectively.
> 
> The patches are grouped as follows:
> * PATCH 1..7: DW HDMI TX driver refactor to minimize code duplication in
>    the new QP driver (no functional changes intended)
> 
> * PATCH 8..11: Rockchip DW HDMI glue driver cleanup/improvements (no
>    functional changes intended)
> 
> * PATCH 12..13: The new DW HDMI QP TX driver reusing the previously
>    exported functions and structs from existing DW HDMI TX driver
> 
> * PATCH 14: Rockchip DW HDMI glue driver update to support RK3588 and
>    make use of DW HDMI QP TX
> 
> They provide just the basic HDMI support for now, i.e. RGB output up to
> 4K@60Hz, without audio, CEC or any of the HDMI 2.1 specific features.
> Also note the vop2 driver is currently not able to properly handle all
> display modes supported by the connected screens, e.g. it doesn't cope
> with non-integer refresh rates.
> 
> A possible workaround consists of enabling the display controller to
> make use of the clock provided by the HDMI PHY PLL. This is still work
> in progress and will be submitted later, as well as the required DTS
> updates.
> 
> To facilitate testing and experimentation, all HDMI output related
> patches, including those part of this series, are available at [3].
> So far I could only verify this on the RADXA Rock 3A and 5B boards.
> 
> Thanks,
> Cristian
> 
> [1]: 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
> [2]: 553be2830c5f ("phy: rockchip: Add Samsung HDMI/eDP Combo PHY driver")
> [3]: https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/commits/rk3588-hdmi-bridge-v6.10-rc1
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
> Cristian Ciocaltea (14):
>        drm/bridge: dw-hdmi: Simplify clock handling
>        drm/bridge: dw-hdmi: Add dw-hdmi-common.h header
>        drm/bridge: dw-hdmi: Commonize dw_hdmi_i2c_adapter()
>        drm/bridge: dw-hdmi: Factor out AVI infoframe setup
>        drm/bridge: dw-hdmi: Factor out vmode setup
>        drm/bridge: dw-hdmi: Factor out hdmi_data_info setup
>        drm/bridge: dw-hdmi: Commonize dw_hdmi_connector_create()
>        drm/rockchip: dw_hdmi: Use modern drm_device based logging
>        drm/rockchip: dw_hdmi: Simplify clock handling
>        drm/rockchip: dw_hdmi: Use devm_regulator_get_enable()
>        drm/rockchip: dw_hdmi: Drop superfluous assignments of mpll_cfg, cur_ctr and phy_config
>        dt-bindings: display: rockchip,dw-hdmi: Add compatible for RK3588
>        drm/bridge: synopsys: Add DW HDMI QP TX controller driver
>        drm/rockchip: dw_hdmi: Add basic RK3588 support
> 
>   .../display/rockchip/rockchip,dw-hdmi.yaml         | 127 +++-
>   drivers/gpu/drm/bridge/synopsys/Makefile           |   2 +-
>   drivers/gpu/drm/bridge/synopsys/dw-hdmi-common.h   | 179 +++++
>   drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c       | 787 +++++++++++++++++++
>   drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h       | 831 +++++++++++++++++++++
>   drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          | 353 +++------
>   drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        | 351 +++++++--
>   include/drm/bridge/dw_hdmi.h                       |   8 +
>   8 files changed, 2290 insertions(+), 348 deletions(-)
> ---
> base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
> change-id: 20240601-b4-rk3588-bridge-upstream-a27baff1b8fc
>
Andy Yan June 3, 2024, 12:14 p.m. UTC | #5
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.
> 
> Thanks,
> Neil
> 
>>
>> This is the last required component that needs to be supported in order
>> to enable the HDMI output functionality on the RK3588 based SBCs, such
>> as the RADXA Rock 5B. The other components are the Video Output
>> Processor (VOP2) and the Samsung IP based HDMI/eDP TX Combo PHY, for
>> which basic support has been already made available via [1] and [2],
>> respectively.
>>
>> The patches are grouped as follows:
>> * PATCH 1..7: DW HDMI TX driver refactor to minimize code duplication in
>>    the new QP driver (no functional changes intended)
>>
>> * PATCH 8..11: Rockchip DW HDMI glue driver cleanup/improvements (no
>>    functional changes intended)
>>
>> * PATCH 12..13: The new DW HDMI QP TX driver reusing the previously
>>    exported functions and structs from existing DW HDMI TX driver
>>
>> * PATCH 14: Rockchip DW HDMI glue driver update to support RK3588 and
>>    make use of DW HDMI QP TX
>>
>> They provide just the basic HDMI support for now, i.e. RGB output up to
>> 4K@60Hz, without audio, CEC or any of the HDMI 2.1 specific features.
>> Also note the vop2 driver is currently not able to properly handle all
>> display modes supported by the connected screens, e.g. it doesn't cope
>> with non-integer refresh rates.
>>
>> A possible workaround consists of enabling the display controller to
>> make use of the clock provided by the HDMI PHY PLL. This is still work
>> in progress and will be submitted later, as well as the required DTS
>> updates.
>>
>> To facilitate testing and experimentation, all HDMI output related
>> patches, including those part of this series, are available at [3].
>> So far I could only verify this on the RADXA Rock 3A and 5B boards.
>>
>> Thanks,
>> Cristian
>>
>> [1]: 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
>> [2]: 553be2830c5f ("phy: rockchip: Add Samsung HDMI/eDP Combo PHY driver")
>> [3]: https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/commits/rk3588-hdmi-bridge-v6.10-rc1
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>> Cristian Ciocaltea (14):
>>        drm/bridge: dw-hdmi: Simplify clock handling
>>        drm/bridge: dw-hdmi: Add dw-hdmi-common.h header
>>        drm/bridge: dw-hdmi: Commonize dw_hdmi_i2c_adapter()
>>        drm/bridge: dw-hdmi: Factor out AVI infoframe setup
>>        drm/bridge: dw-hdmi: Factor out vmode setup
>>        drm/bridge: dw-hdmi: Factor out hdmi_data_info setup
>>        drm/bridge: dw-hdmi: Commonize dw_hdmi_connector_create()
>>        drm/rockchip: dw_hdmi: Use modern drm_device based logging
>>        drm/rockchip: dw_hdmi: Simplify clock handling
>>        drm/rockchip: dw_hdmi: Use devm_regulator_get_enable()
>>        drm/rockchip: dw_hdmi: Drop superfluous assignments of mpll_cfg, cur_ctr and phy_config
>>        dt-bindings: display: rockchip,dw-hdmi: Add compatible for RK3588
>>        drm/bridge: synopsys: Add DW HDMI QP TX controller driver
>>        drm/rockchip: dw_hdmi: Add basic RK3588 support
>>
>>   .../display/rockchip/rockchip,dw-hdmi.yaml         | 127 +++-
>>   drivers/gpu/drm/bridge/synopsys/Makefile           |   2 +-
>>   drivers/gpu/drm/bridge/synopsys/dw-hdmi-common.h   | 179 +++++
>>   drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c       | 787 +++++++++++++++++++
>>   drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h       | 831 +++++++++++++++++++++
>>   drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          | 353 +++------
>>   drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        | 351 +++++++--
>>   include/drm/bridge/dw_hdmi.h                       |   8 +
>>   8 files changed, 2290 insertions(+), 348 deletions(-)
>> ---
>> base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
>> change-id: 20240601-b4-rk3588-bridge-upstream-a27baff1b8fc
>>
>
Heiko Stuebner June 3, 2024, 1:03 p.m. UTC | #6
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.

Synopsis also created a new dsi controller for the DSI2 standard, with
a vastly different registers layout.

I guess at some point there is time to say this really is a new IP ;-) .


Though while on that thought, I don't fully understand why both a compiled
under the dw_hdmi kconfig symbol. People going for a minimal kernel might
want one or the other, but not both for their specific board.


Heiko
Neil Armstrong June 3, 2024, 1:08 p.m. UTC | #7
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.

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.

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.

Neil

> 
> Synopsis also created a new dsi controller for the DSI2 standard, with
> a vastly different registers layout.
> 
> I guess at some point there is time to say this really is a new IP ;-) .
> 
> 
> Though while on that thought, I don't fully understand why both a compiled
> under the dw_hdmi kconfig symbol. People going for a minimal kernel might
> want one or the other, but not both for their specific board.
> 
> 
> Heiko
> 
>
Maxime Ripard June 3, 2024, 4:22 p.m. UTC | #8
On Mon, Jun 03, 2024 at 03:03:12PM GMT, 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.

If it's a completely new controller, I agree that it needs a new driver,
but then why do we need to share code between the two?

Maxime
Cristian Ciocaltea June 4, 2024, 7:44 p.m. UTC | #9
On 6/1/24 7:32 PM, Dmitry Baryshkov wrote:
> On Sat, Jun 01, 2024 at 04:12:22PM +0300, 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:
>>
>> * Fixed Rate Link (FRL)
>> * 4K@120Hz and 8K@60Hz video modes
>> * Variable Refresh Rate (VRR) including Quick Media Switching (QMS)
>> * Fast Vactive (FVA)
>> * 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)
> 
> It would be really nice if you can take a look at using the HDMI
> connector framework (landed few days ago) with adaptations for the
> drm_bridge / drm_bridge_connector ([1]). Your comments for the
> drm_bridge patches would be defeinitely appreciated.
> 
> [1] https://lore.kernel.org/dri-devel/20240531-bridge-hdmi-connector-v4-0-5110f7943622@linaro.org/

I will definitely check and try to use it, but I'd rather wait a bit
until this gets stabilized and focus instead on the mandatory changes
required to upstream this driver. That's mostly because my limited
availability and expertise on the matter, while trying to unblock other
work depending on this.

Thanks,
Cristian
Cristian Ciocaltea June 4, 2024, 7:59 p.m. UTC | #10
On 6/2/24 10:59 AM, Piotr Oniszczuk wrote:
> (resent as plain text instead of html)
>  
> Cristian,
> 
> I was awaiting over year for this work!
> 
> I’m devel. 2 distros where single mainline kernel serves 2835/2711/2712/h6/h313/h616/h618/rk3328/rk3399/rk3566/rk3568/rk3588/s905/s912/sm1/g12.
> 
> Before this work rk3588 was excluded because rk3588 hdmi was regressing hdmi on other socs.
> With this code all other socs seems work ok now. Perfect.

Many thanks for giving this a try on a broad range of SoCs, especially
considering my limited testing capabilities!

> As one of my project is multimedia appliance - good news is that now i can nicely play hdtv on rk3588 using mainline common 6.9.3 kernel and….started to hear from my users a lot of Qs like: „ah so nice! rk3588 now works nicely….but where is hdmi audio and cec?”
> 
> It will be fantastic to add (e.g. by backport Detlev https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/tree/rk3588-hdmi-audio?ref_type=heads ) audio code to get basic support hdmi audio?

The main focus is now on upstreaming the basic support. This should
further facilitate adding the missing features, so we will slowly get
there, eventually.
Cristian Ciocaltea June 4, 2024, 8:33 p.m. UTC | #11
On 6/3/24 4:08 PM, neil.armstrong@linaro.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@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.

> Neil
> 
>>
>> Synopsis also created a new dsi controller for the DSI2 standard, with
>> a vastly different registers layout.
>>
>> I guess at some point there is time to say this really is a new IP ;-) .
>>
>>
>> Though while on that thought, I don't fully understand why both a
>> compiled
>> under the dw_hdmi kconfig symbol. People going for a minimal kernel might
>> want one or the other, but not both for their specific board.

Indeed, it makes sense to have a dedicated Kconfig option. This is
mostly a leftover from downstream implementation, will fix in v2.

Thanks again,
Cristian
Dmitry Baryshkov June 4, 2024, 11:49 p.m. UTC | #12
On Tue, Jun 04, 2024 at 10:44:04PM +0300, Cristian Ciocaltea wrote:
> On 6/1/24 7:32 PM, Dmitry Baryshkov wrote:
> > On Sat, Jun 01, 2024 at 04:12:22PM +0300, 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:
> >>
> >> * Fixed Rate Link (FRL)
> >> * 4K@120Hz and 8K@60Hz video modes
> >> * Variable Refresh Rate (VRR) including Quick Media Switching (QMS)
> >> * Fast Vactive (FVA)
> >> * 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)
> > 
> > It would be really nice if you can take a look at using the HDMI
> > connector framework (landed few days ago) with adaptations for the
> > drm_bridge / drm_bridge_connector ([1]). Your comments for the
> > drm_bridge patches would be defeinitely appreciated.
> > 
> > [1] https://lore.kernel.org/dri-devel/20240531-bridge-hdmi-connector-v4-0-5110f7943622@linaro.org/
> 
> I will definitely check and try to use it, but I'd rather wait a bit
> until this gets stabilized and focus instead on the mandatory changes
> required to upstream this driver. That's mostly because my limited
> availability and expertise on the matter, while trying to unblock other
> work depending on this.

Ack.
Andy Yan June 5, 2024, 9:25 a.m. UTC | #13
Hi,

At 2024-06-05 04:33:57, "Cristian Ciocaltea" <cristian.ciocaltea@collabora.com> wrote:
>On 6/3/24 4:08 PM, neil.armstrong@linaro.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@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。 



>> Neil
>> 
>>>
>>> Synopsis also created a new dsi controller for the DSI2 standard, with
>>> a vastly different registers layout.
>>>
>>> I guess at some point there is time to say this really is a new IP ;-) .
>>>
>>>
>>> Though while on that thought, I don't fully understand why both a
>>> compiled
>>> under the dw_hdmi kconfig symbol. People going for a minimal kernel might
>>> want one or the other, but not both for their specific board.
>
>Indeed, it makes sense to have a dedicated Kconfig option. This is
>mostly a leftover from downstream implementation, will fix in v2.
>
>Thanks again,
>Cristian
>
>_______________________________________________
>linux-arm-kernel mailing list
>linux-arm-kernel@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Neil Armstrong June 5, 2024, 9:28 a.m. UTC | #14
Hi,

On 05/06/2024 11:25, Andy Yan wrote:
> 
> Hi,
> 
> At 2024-06-05 04:33:57, "Cristian Ciocaltea" <cristian.ciocaltea@collabora.com> wrote:
>> On 6/3/24 4:08 PM, neil.armstrong@linaro.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@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.

Neil

> 
> 
> 
>>> Neil
>>>
>>>>
>>>> Synopsis also created a new dsi controller for the DSI2 standard, with
>>>> a vastly different registers layout.
>>>>
>>>> I guess at some point there is time to say this really is a new IP ;-) .
>>>>
>>>>
>>>> Though while on that thought, I don't fully understand why both a
>>>> compiled
>>>> under the dw_hdmi kconfig symbol. People going for a minimal kernel might
>>>> want one or the other, but not both for their specific board.
>>
>> Indeed, it makes sense to have a dedicated Kconfig option. This is
>> mostly a leftover from downstream implementation, will fix in v2.
>>
>> Thanks again,
>> Cristian
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Maxime Ripard June 5, 2024, 9:39 a.m. UTC | #15
On Wed, Jun 05, 2024 at 11:28:41AM GMT, neil.armstrong@linaro.org wrote:
> Hi,
> 
> On 05/06/2024 11:25, Andy Yan wrote:
> > 
> > Hi,
> > 
> > At 2024-06-05 04:33:57, "Cristian Ciocaltea" <cristian.ciocaltea@collabora.com> wrote:
> > > On 6/3/24 4:08 PM, neil.armstrong@linaro.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@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
Andy Yan June 5, 2024, 9:49 a.m. UTC | #16
Hi,
At 2024-06-05 17:39:48, "Maxime Ripard" <mripard@kernel.org> wrote:
>On Wed, Jun 05, 2024 at 11:28:41AM GMT, neil.armstrong@linaro.org wrote:
>> Hi,
>> 
>> On 05/06/2024 11:25, Andy Yan wrote:
>> > 
>> > Hi,
>> > 
>> > At 2024-06-05 04:33:57, "Cristian Ciocaltea" <cristian.ciocaltea@collabora.com> wrote:
>> > > On 6/3/24 4:08 PM, neil.armstrong@linaro.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@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
Cristian Ciocaltea June 5, 2024, 11:20 a.m. UTC | #17
On 6/5/24 12:49 PM, Andy Yan wrote:
> 
> Hi,
> At 2024-06-05 17:39:48, "Maxime Ripard" <mripard@kernel.org> wrote:
>> On Wed, Jun 05, 2024 at 11:28:41AM GMT, neil.armstrong@linaro.org wrote:
>>> Hi,
>>>
>>> On 05/06/2024 11:25, Andy Yan wrote:
>>>>
>>>> Hi,
>>>>
>>>> At 2024-06-05 04:33:57, "Cristian Ciocaltea" <cristian.ciocaltea@collabora.com> wrote:
>>>>> On 6/3/24 4:08 PM, neil.armstrong@linaro.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@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
Piotr Oniszczuk July 14, 2024, 7:03 p.m. UTC | #18
Cristian,

I'm hacking with adding cec support for rk3588 hdmi on 6.10-rc7 mainline. 

Cec kernel module is my backport from bsp. 
Module loads. Cec line (observed on osciloscope) has pulses when i'm issuing i.e. cec-ctl -d /dev/cec0 --phys-addr=1.0.0.0 —playback 

My issue is that timings are 2,9 times longer that should be (start bit is 10,7mS instead of 3.6; zero is 4.4 instead 1.5 while one is 1,7 instead of 0.6).

This suggests me issue is cec clock isn't? 

Looking on bsp code https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr1/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp-cec.c#L186 there is nothing with clock. 

So probably issue is in 3588 clk code, or……
Maybe you have some hints how to move forward with this issue?


> Wiadomość napisana przez Cristian Ciocaltea <cristian.ciocaltea@collabora.com> w dniu 01.06.2024, o godz. 15:12:
> 
> 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:
> 
> * Fixed Rate Link (FRL)
> * 4K@120Hz and 8K@60Hz video modes
> * Variable Refresh Rate (VRR) including Quick Media Switching (QMS)
> * Fast Vactive (FVA)
> * 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)
> 
> This is the last required component that needs to be supported in order
> to enable the HDMI output functionality on the RK3588 based SBCs, such
> as the RADXA Rock 5B. The other components are the Video Output
> Processor (VOP2) and the Samsung IP based HDMI/eDP TX Combo PHY, for
> which basic support has been already made available via [1] and [2],
> respectively.
> 
> The patches are grouped as follows:
> * PATCH 1..7: DW HDMI TX driver refactor to minimize code duplication in
>  the new QP driver (no functional changes intended)
> 
> * PATCH 8..11: Rockchip DW HDMI glue driver cleanup/improvements (no
>  functional changes intended)
> 
> * PATCH 12..13: The new DW HDMI QP TX driver reusing the previously
>  exported functions and structs from existing DW HDMI TX driver
> 
> * PATCH 14: Rockchip DW HDMI glue driver update to support RK3588 and
>  make use of DW HDMI QP TX
> 
> They provide just the basic HDMI support for now, i.e. RGB output up to
> 4K@60Hz, without audio, CEC or any of the HDMI 2.1 specific features.
> Also note the vop2 driver is currently not able to properly handle all
> display modes supported by the connected screens, e.g. it doesn't cope
> with non-integer refresh rates.
> 
> A possible workaround consists of enabling the display controller to
> make use of the clock provided by the HDMI PHY PLL. This is still work
> in progress and will be submitted later, as well as the required DTS
> updates.
> 
> To facilitate testing and experimentation, all HDMI output related
> patches, including those part of this series, are available at [3].
> So far I could only verify this on the RADXA Rock 3A and 5B boards.
> 
> Thanks,
> Cristian
> 
> [1]: 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
> [2]: 553be2830c5f ("phy: rockchip: Add Samsung HDMI/eDP Combo PHY driver")
> [3]: https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/commits/rk3588-hdmi-bridge-v6.10-rc1
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
> Cristian Ciocaltea (14):
>      drm/bridge: dw-hdmi: Simplify clock handling
>      drm/bridge: dw-hdmi: Add dw-hdmi-common.h header
>      drm/bridge: dw-hdmi: Commonize dw_hdmi_i2c_adapter()
>      drm/bridge: dw-hdmi: Factor out AVI infoframe setup
>      drm/bridge: dw-hdmi: Factor out vmode setup
>      drm/bridge: dw-hdmi: Factor out hdmi_data_info setup
>      drm/bridge: dw-hdmi: Commonize dw_hdmi_connector_create()
>      drm/rockchip: dw_hdmi: Use modern drm_device based logging
>      drm/rockchip: dw_hdmi: Simplify clock handling
>      drm/rockchip: dw_hdmi: Use devm_regulator_get_enable()
>      drm/rockchip: dw_hdmi: Drop superfluous assignments of mpll_cfg, cur_ctr and phy_config
>      dt-bindings: display: rockchip,dw-hdmi: Add compatible for RK3588
>      drm/bridge: synopsys: Add DW HDMI QP TX controller driver
>      drm/rockchip: dw_hdmi: Add basic RK3588 support
> 
> .../display/rockchip/rockchip,dw-hdmi.yaml         | 127 +++-
> drivers/gpu/drm/bridge/synopsys/Makefile           |   2 +-
> drivers/gpu/drm/bridge/synopsys/dw-hdmi-common.h   | 179 +++++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c       | 787 +++++++++++++++++++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h       | 831 +++++++++++++++++++++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          | 353 +++------
> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        | 351 +++++++--
> include/drm/bridge/dw_hdmi.h                       |   8 +
> 8 files changed, 2290 insertions(+), 348 deletions(-)
> ---
> base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
> change-id: 20240601-b4-rk3588-bridge-upstream-a27baff1b8fc
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip