| Message ID | 20240601-b4-rk3588-bridge-upstream-v1-0-f6203753232b@collabora.com |
|---|---|
| Headers | show |
| Series | Add initial support for the Rockchip RK3588 HDMI TX Controller | expand |
Hi Cristian, a few drive-by comments below. Sam > + > +static const struct drm_connector_funcs dw_hdmi_qp_connector_funcs = { > + .fill_modes = drm_helper_probe_single_connector_modes, > + .detect = dw_hdmi_connector_detect, > + .destroy = drm_connector_cleanup, > + .force = dw_hdmi_qp_connector_force, > + .reset = drm_atomic_helper_connector_reset, > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > +}; > + > +static int dw_hdmi_qp_bridge_attach(struct drm_bridge *bridge, > + enum drm_bridge_attach_flags flags) > +{ > + struct dw_hdmi *hdmi = bridge->driver_private; > + > + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) > + return drm_bridge_attach(bridge->encoder, hdmi->next_bridge, > + bridge, flags); > + > + return dw_hdmi_connector_create(hdmi, &dw_hdmi_qp_connector_funcs); > +} Are there any users left that requires the display driver to create the connector? In other words - could this driver fail if DRM_BRIDGE_ATTACH_NO_CONNECTOR is not passed and drop dw_hdmi_connector_create()? I did not try to verify this - just a naive question. > + > +static enum drm_mode_status > +dw_hdmi_qp_bridge_mode_valid(struct drm_bridge *bridge, > + const struct drm_display_info *info, > + const struct drm_display_mode *mode) > +{ > + struct dw_hdmi *hdmi = bridge->driver_private; > + const struct dw_hdmi_plat_data *pdata = hdmi->plat_data; > + enum drm_mode_status mode_status = MODE_OK; > + > + if (pdata->mode_valid) > + mode_status = pdata->mode_valid(hdmi, pdata->priv_data, info, > + mode); > + > + return mode_status; > +} > + > +static void dw_hdmi_qp_bridge_atomic_disable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_state) > +{ > + struct dw_hdmi *hdmi = bridge->driver_private; > + > + mutex_lock(&hdmi->mutex); > + hdmi->disabled = true; > + hdmi->curr_conn = NULL; > + dw_hdmi_qp_update_power(hdmi); > + dw_handle_plugged_change(hdmi, false); > + mutex_unlock(&hdmi->mutex); > +} > + > +static void dw_hdmi_qp_bridge_atomic_enable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_state) > +{ > + struct dw_hdmi *hdmi = bridge->driver_private; > + struct drm_atomic_state *state = old_state->base.state; > + struct drm_connector *connector; > + > + connector = drm_atomic_get_new_connector_for_encoder(state, > + bridge->encoder); > + > + mutex_lock(&hdmi->mutex); > + hdmi->disabled = false; > + hdmi->curr_conn = connector; > + dw_hdmi_qp_update_power(hdmi); > + dw_handle_plugged_change(hdmi, true); > + mutex_unlock(&hdmi->mutex); > +} > + > +static const struct drm_bridge_funcs dw_hdmi_qp_bridge_funcs = { > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > + .atomic_reset = drm_atomic_helper_bridge_reset, > + .attach = dw_hdmi_qp_bridge_attach, > + .detach = dw_hdmi_bridge_detach, > + .atomic_check = dw_hdmi_bridge_atomic_check, > + .atomic_enable = dw_hdmi_qp_bridge_atomic_enable, > + .atomic_disable = dw_hdmi_qp_bridge_atomic_disable, > + .mode_set = dw_hdmi_bridge_mode_set, The use of mode_set is deprecated - see drm_bridge.h
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 >
(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
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 >
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 >> >
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
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 > >
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
Hi Sam, On 6/1/24 5:32 PM, Sam Ravnborg wrote: > Hi Cristian, > > a few drive-by comments below. > > Sam > > >> + >> +static const struct drm_connector_funcs dw_hdmi_qp_connector_funcs = { >> + .fill_modes = drm_helper_probe_single_connector_modes, >> + .detect = dw_hdmi_connector_detect, >> + .destroy = drm_connector_cleanup, >> + .force = dw_hdmi_qp_connector_force, >> + .reset = drm_atomic_helper_connector_reset, >> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >> +}; >> + >> +static int dw_hdmi_qp_bridge_attach(struct drm_bridge *bridge, >> + enum drm_bridge_attach_flags flags) >> +{ >> + struct dw_hdmi *hdmi = bridge->driver_private; >> + >> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) >> + return drm_bridge_attach(bridge->encoder, hdmi->next_bridge, >> + bridge, flags); >> + >> + return dw_hdmi_connector_create(hdmi, &dw_hdmi_qp_connector_funcs); >> +} > > Are there any users left that requires the display driver to create the > connector? > In other words - could this driver fail if DRM_BRIDGE_ATTACH_NO_CONNECTOR > is not passed and drop dw_hdmi_connector_create()? > > I did not try to verify this - just a naive question. I've just tested this and it doesn't work - dw_hdmi_connector_create() is still needed. Regards, Cristian
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
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.
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
Hi Cristian. On Tue, Jun 04, 2024 at 10:32:04PM +0300, Cristian Ciocaltea wrote: > Hi Sam, > > On 6/1/24 5:32 PM, Sam Ravnborg wrote: > > Hi Cristian, > > > > a few drive-by comments below. > > > > Sam > > > > > >> + > >> +static const struct drm_connector_funcs dw_hdmi_qp_connector_funcs = { > >> + .fill_modes = drm_helper_probe_single_connector_modes, > >> + .detect = dw_hdmi_connector_detect, > >> + .destroy = drm_connector_cleanup, > >> + .force = dw_hdmi_qp_connector_force, > >> + .reset = drm_atomic_helper_connector_reset, > >> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > >> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > >> +}; > >> + > >> +static int dw_hdmi_qp_bridge_attach(struct drm_bridge *bridge, > >> + enum drm_bridge_attach_flags flags) > >> +{ > >> + struct dw_hdmi *hdmi = bridge->driver_private; > >> + > >> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) > >> + return drm_bridge_attach(bridge->encoder, hdmi->next_bridge, > >> + bridge, flags); > >> + > >> + return dw_hdmi_connector_create(hdmi, &dw_hdmi_qp_connector_funcs); > >> +} > > > > Are there any users left that requires the display driver to create the > > connector? > > In other words - could this driver fail if DRM_BRIDGE_ATTACH_NO_CONNECTOR > > is not passed and drop dw_hdmi_connector_create()? > > > > I did not try to verify this - just a naive question. > > I've just tested this and it doesn't work - dw_hdmi_connector_create() > is still needed. Hmm, seems the display driver or some other bridge driver fails to support "DRM_BRIDGE_ATTACH_NO_CONNECTOR". what other drivers are involved? Note that my comments here should be seen as potential future improvements, and do not block the patch from being used. Sam
On 6/4/24 11:41 PM, Sam Ravnborg wrote: > Hi Cristian. > > On Tue, Jun 04, 2024 at 10:32:04PM +0300, Cristian Ciocaltea wrote: >> Hi Sam, >> >> On 6/1/24 5:32 PM, Sam Ravnborg wrote: >>> Hi Cristian, >>> >>> a few drive-by comments below. >>> >>> Sam >>> >>> >>>> + >>>> +static const struct drm_connector_funcs dw_hdmi_qp_connector_funcs = { >>>> + .fill_modes = drm_helper_probe_single_connector_modes, >>>> + .detect = dw_hdmi_connector_detect, >>>> + .destroy = drm_connector_cleanup, >>>> + .force = dw_hdmi_qp_connector_force, >>>> + .reset = drm_atomic_helper_connector_reset, >>>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >>>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >>>> +}; >>>> + >>>> +static int dw_hdmi_qp_bridge_attach(struct drm_bridge *bridge, >>>> + enum drm_bridge_attach_flags flags) >>>> +{ >>>> + struct dw_hdmi *hdmi = bridge->driver_private; >>>> + >>>> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) >>>> + return drm_bridge_attach(bridge->encoder, hdmi->next_bridge, >>>> + bridge, flags); >>>> + >>>> + return dw_hdmi_connector_create(hdmi, &dw_hdmi_qp_connector_funcs); >>>> +} >>> >>> Are there any users left that requires the display driver to create the >>> connector? >>> In other words - could this driver fail if DRM_BRIDGE_ATTACH_NO_CONNECTOR >>> is not passed and drop dw_hdmi_connector_create()? >>> >>> I did not try to verify this - just a naive question. >> >> I've just tested this and it doesn't work - dw_hdmi_connector_create() >> is still needed. > > Hmm, seems the display driver or some other bridge driver fails to > support "DRM_BRIDGE_ATTACH_NO_CONNECTOR". > what other drivers are involved? Could it be related to the glue driver (updated in the next patch) which is also responsible for setting up the encoder? > Note that my comments here should be seen as potential future > improvements, and do not block the patch from being used. Thanks for the heads up! Will try to get back to this soon and investigate. Cristian
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.
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
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
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
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
On 6/5/24 12:34 AM, Cristian Ciocaltea wrote: > On 6/4/24 11:41 PM, Sam Ravnborg wrote: >> Hi Cristian. >> >> On Tue, Jun 04, 2024 at 10:32:04PM +0300, Cristian Ciocaltea wrote: >>> Hi Sam, >>> >>> On 6/1/24 5:32 PM, Sam Ravnborg wrote: >>>> Hi Cristian, >>>> >>>> a few drive-by comments below. >>>> >>>> Sam >>>> >>>> >>>>> + >>>>> +static const struct drm_connector_funcs dw_hdmi_qp_connector_funcs = { >>>>> + .fill_modes = drm_helper_probe_single_connector_modes, >>>>> + .detect = dw_hdmi_connector_detect, >>>>> + .destroy = drm_connector_cleanup, >>>>> + .force = dw_hdmi_qp_connector_force, >>>>> + .reset = drm_atomic_helper_connector_reset, >>>>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >>>>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >>>>> +}; >>>>> + >>>>> +static int dw_hdmi_qp_bridge_attach(struct drm_bridge *bridge, >>>>> + enum drm_bridge_attach_flags flags) >>>>> +{ >>>>> + struct dw_hdmi *hdmi = bridge->driver_private; >>>>> + >>>>> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) >>>>> + return drm_bridge_attach(bridge->encoder, hdmi->next_bridge, >>>>> + bridge, flags); >>>>> + >>>>> + return dw_hdmi_connector_create(hdmi, &dw_hdmi_qp_connector_funcs); >>>>> +} >>>> >>>> Are there any users left that requires the display driver to create the >>>> connector? >>>> In other words - could this driver fail if DRM_BRIDGE_ATTACH_NO_CONNECTOR >>>> is not passed and drop dw_hdmi_connector_create()? >>>> >>>> I did not try to verify this - just a naive question. >>> >>> I've just tested this and it doesn't work - dw_hdmi_connector_create() >>> is still needed. >> >> Hmm, seems the display driver or some other bridge driver fails to >> support "DRM_BRIDGE_ATTACH_NO_CONNECTOR". >> what other drivers are involved? > > Could it be related to the glue driver (updated in the next patch) which > is also responsible for setting up the encoder? > >> Note that my comments here should be seen as potential future >> improvements, and do not block the patch from being used. > > Thanks for the heads up! Will try to get back to this soon and investigate. IIUC, modern bridges should not create the connector but rely on display drivers to take care of, which in this case is the VOP2 driver. However, it also handles some of the older SoCs relying on the non-QP variant of DW HDMI IP. Hence the existing dw-hdmi driver would be also impacted in order to come up with a proper solution. A quick check shows there are several users of this IP: $ git grep -E '= dw_hdmi_(bind|probe)\(' drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c: hdmi->dw_hdmi = dw_hdmi_probe(pdev, plat_data); drivers/gpu/drm/bridge/synopsys/dw-hdmi.c: hdmi = dw_hdmi_probe(pdev, plat_data); drivers/gpu/drm/imx/ipuv3/dw_hdmi-imx.c: hdmi->hdmi = dw_hdmi_probe(pdev, match->data); drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c: hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data); drivers/gpu/drm/meson/meson_dw_hdmi.c: meson_dw_hdmi->hdmi = dw_hdmi_probe(pdev, &meson_dw_hdmi->dw_plat_data); drivers/gpu/drm/renesas/rcar-du/rcar_dw_hdmi.c: hdmi = dw_hdmi_probe(pdev, &rcar_dw_hdmi_plat_data); drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c: hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data); drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c: hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data); I didn't check which display drivers would be involved, I'd guess there are quite a few of them as well. So it seems this ends up being a pretty complex task. Cristian
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
On 05/06/2024 12:11, Cristian Ciocaltea wrote: > On 6/5/24 12:34 AM, Cristian Ciocaltea wrote: >> On 6/4/24 11:41 PM, Sam Ravnborg wrote: >>> Hi Cristian. >>> >>> On Tue, Jun 04, 2024 at 10:32:04PM +0300, Cristian Ciocaltea wrote: >>>> Hi Sam, >>>> >>>> On 6/1/24 5:32 PM, Sam Ravnborg wrote: >>>>> Hi Cristian, >>>>> >>>>> a few drive-by comments below. >>>>> >>>>> Sam >>>>> >>>>> >>>>>> + >>>>>> +static const struct drm_connector_funcs dw_hdmi_qp_connector_funcs = { >>>>>> + .fill_modes = drm_helper_probe_single_connector_modes, >>>>>> + .detect = dw_hdmi_connector_detect, >>>>>> + .destroy = drm_connector_cleanup, >>>>>> + .force = dw_hdmi_qp_connector_force, >>>>>> + .reset = drm_atomic_helper_connector_reset, >>>>>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >>>>>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >>>>>> +}; >>>>>> + >>>>>> +static int dw_hdmi_qp_bridge_attach(struct drm_bridge *bridge, >>>>>> + enum drm_bridge_attach_flags flags) >>>>>> +{ >>>>>> + struct dw_hdmi *hdmi = bridge->driver_private; >>>>>> + >>>>>> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) >>>>>> + return drm_bridge_attach(bridge->encoder, hdmi->next_bridge, >>>>>> + bridge, flags); >>>>>> + >>>>>> + return dw_hdmi_connector_create(hdmi, &dw_hdmi_qp_connector_funcs); >>>>>> +} >>>>> >>>>> Are there any users left that requires the display driver to create the >>>>> connector? >>>>> In other words - could this driver fail if DRM_BRIDGE_ATTACH_NO_CONNECTOR >>>>> is not passed and drop dw_hdmi_connector_create()? >>>>> >>>>> I did not try to verify this - just a naive question. >>>> >>>> I've just tested this and it doesn't work - dw_hdmi_connector_create() >>>> is still needed. >>> >>> Hmm, seems the display driver or some other bridge driver fails to >>> support "DRM_BRIDGE_ATTACH_NO_CONNECTOR". >>> what other drivers are involved? >> >> Could it be related to the glue driver (updated in the next patch) which >> is also responsible for setting up the encoder? >> >>> Note that my comments here should be seen as potential future >>> improvements, and do not block the patch from being used. >> >> Thanks for the heads up! Will try to get back to this soon and investigate. > > IIUC, modern bridges should not create the connector but rely on display > drivers to take care of, which in this case is the VOP2 driver. However, > it also handles some of the older SoCs relying on the non-QP variant of > DW HDMI IP. Hence the existing dw-hdmi driver would be also impacted in > order to come up with a proper solution. > > A quick check shows there are several users of this IP: > > $ git grep -E '= dw_hdmi_(bind|probe)\(' > drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c: hdmi->dw_hdmi = dw_hdmi_probe(pdev, plat_data); > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c: hdmi = dw_hdmi_probe(pdev, plat_data); > drivers/gpu/drm/imx/ipuv3/dw_hdmi-imx.c: hdmi->hdmi = dw_hdmi_probe(pdev, match->data); > drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c: hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data); > drivers/gpu/drm/meson/meson_dw_hdmi.c: meson_dw_hdmi->hdmi = dw_hdmi_probe(pdev, &meson_dw_hdmi->dw_plat_data); > drivers/gpu/drm/renesas/rcar-du/rcar_dw_hdmi.c: hdmi = dw_hdmi_probe(pdev, &rcar_dw_hdmi_plat_data); > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c: hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data); > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c: hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data); > > I didn't check which display drivers would be involved, I'd guess there > are quite a few of them as well. So it seems this ends up being a pretty > complex task. If this would be a brand new driver, then it should only support DRM_BRIDGE_ATTACH_NO_CONNECTOR, so you should not create a connector from the driver. The fact dw-hdmi accepts an attach without the flag is for legacy purpose since some DRM drivers haven't switched to DRM_BRIDGE_ATTACH_NO_CONNECTOR yes, but it's a requirement for new bridges so at some point you'll need to make sure the rockchip glue and drm driver supports DRM_BRIDGE_ATTACH_NO_CONNECTOR. This will greatly simplify the driver! Neil > > Cristian
On 6/5/24 2:48 PM, Neil Armstrong wrote: > On 05/06/2024 12:11, Cristian Ciocaltea wrote: >> On 6/5/24 12:34 AM, Cristian Ciocaltea wrote: >>> On 6/4/24 11:41 PM, Sam Ravnborg wrote: >>>> Hi Cristian. >>>> >>>> On Tue, Jun 04, 2024 at 10:32:04PM +0300, Cristian Ciocaltea wrote: >>>>> Hi Sam, >>>>> >>>>> On 6/1/24 5:32 PM, Sam Ravnborg wrote: >>>>>> Hi Cristian, >>>>>> >>>>>> a few drive-by comments below. >>>>>> >>>>>> Sam >>>>>> >>>>>> >>>>>>> + >>>>>>> +static const struct drm_connector_funcs >>>>>>> dw_hdmi_qp_connector_funcs = { >>>>>>> + .fill_modes = drm_helper_probe_single_connector_modes, >>>>>>> + .detect = dw_hdmi_connector_detect, >>>>>>> + .destroy = drm_connector_cleanup, >>>>>>> + .force = dw_hdmi_qp_connector_force, >>>>>>> + .reset = drm_atomic_helper_connector_reset, >>>>>>> + .atomic_duplicate_state = >>>>>>> drm_atomic_helper_connector_duplicate_state, >>>>>>> + .atomic_destroy_state = >>>>>>> drm_atomic_helper_connector_destroy_state, >>>>>>> +}; >>>>>>> + >>>>>>> +static int dw_hdmi_qp_bridge_attach(struct drm_bridge *bridge, >>>>>>> + enum drm_bridge_attach_flags flags) >>>>>>> +{ >>>>>>> + struct dw_hdmi *hdmi = bridge->driver_private; >>>>>>> + >>>>>>> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) >>>>>>> + return drm_bridge_attach(bridge->encoder, >>>>>>> hdmi->next_bridge, >>>>>>> + bridge, flags); >>>>>>> + >>>>>>> + return dw_hdmi_connector_create(hdmi, >>>>>>> &dw_hdmi_qp_connector_funcs); >>>>>>> +} >>>>>> >>>>>> Are there any users left that requires the display driver to >>>>>> create the >>>>>> connector? >>>>>> In other words - could this driver fail if >>>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR >>>>>> is not passed and drop dw_hdmi_connector_create()? >>>>>> >>>>>> I did not try to verify this - just a naive question. >>>>> >>>>> I've just tested this and it doesn't work - dw_hdmi_connector_create() >>>>> is still needed. >>>> >>>> Hmm, seems the display driver or some other bridge driver fails to >>>> support "DRM_BRIDGE_ATTACH_NO_CONNECTOR". >>>> what other drivers are involved? >>> >>> Could it be related to the glue driver (updated in the next patch) which >>> is also responsible for setting up the encoder? >>> >>>> Note that my comments here should be seen as potential future >>>> improvements, and do not block the patch from being used. >>> >>> Thanks for the heads up! Will try to get back to this soon and >>> investigate. >> IIUC, modern bridges should not create the connector but rely on >> display >> drivers to take care of, which in this case is the VOP2 driver. However, >> it also handles some of the older SoCs relying on the non-QP variant of >> DW HDMI IP. Hence the existing dw-hdmi driver would be also impacted in >> order to come up with a proper solution. >> >> A quick check shows there are several users of this IP: >> >> $ git grep -E '= dw_hdmi_(bind|probe)\(' >> drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c: hdmi->dw_hdmi = >> dw_hdmi_probe(pdev, plat_data); >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c: hdmi = >> dw_hdmi_probe(pdev, plat_data); >> drivers/gpu/drm/imx/ipuv3/dw_hdmi-imx.c: hdmi->hdmi = >> dw_hdmi_probe(pdev, match->data); >> drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c: hdmi = >> dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data); >> drivers/gpu/drm/meson/meson_dw_hdmi.c: meson_dw_hdmi->hdmi = >> dw_hdmi_probe(pdev, &meson_dw_hdmi->dw_plat_data); >> drivers/gpu/drm/renesas/rcar-du/rcar_dw_hdmi.c: hdmi = >> dw_hdmi_probe(pdev, &rcar_dw_hdmi_plat_data); >> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c: hdmi->hdmi = >> dw_hdmi_bind(pdev, encoder, plat_data); >> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c: hdmi->hdmi = >> dw_hdmi_bind(pdev, encoder, plat_data); >> >> I didn't check which display drivers would be involved, I'd guess there >> are quite a few of them as well. So it seems this ends up being a pretty >> complex task. > > If this would be a brand new driver, then it should only support > DRM_BRIDGE_ATTACH_NO_CONNECTOR, > so you should not create a connector from the driver. > > The fact dw-hdmi accepts an attach without the flag is for legacy purpose > since some DRM drivers haven't switched to > DRM_BRIDGE_ATTACH_NO_CONNECTOR yes, > but it's a requirement for new bridges so at some point you'll need to make > sure the rockchip glue and drm driver supports > DRM_BRIDGE_ATTACH_NO_CONNECTOR. > > This will greatly simplify the driver! Got it, thanks for the clarifications! Cristian
Am Samstag, 1. Juni 2024, 15:12:35 CEST schrieb Cristian Ciocaltea: > The Synopsys DesignWare HDMI 2.1 Quad-Pixel (QP) TX controller 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), aka > Cinema VRR > * Fast Vactive (FVA), aka Quick Frame Transport (QFT) > * 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) > > Add driver to enable basic support, i.e. RGB output up to 4K@60Hz, > without audio, CEC or any HDMI 2.1 specific features. > > Co-developed-by: Algea Cao <algea.cao@rock-chips.com> > Signed-off-by: Algea Cao <algea.cao@rock-chips.com> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> > --- > drivers/gpu/drm/bridge/synopsys/Makefile | 2 +- > drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 787 +++++++++++++++++++++++++ > drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h | 831 +++++++++++++++++++++++++++ > include/drm/bridge/dw_hdmi.h | 8 + > 4 files changed, 1627 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/Makefile b/drivers/gpu/drm/bridge/synopsys/Makefile > index ce715562e9e5..8354e4879f70 100644 > --- a/drivers/gpu/drm/bridge/synopsys/Makefile > +++ b/drivers/gpu/drm/bridge/synopsys/Makefile > +static int dw_hdmi_qp_i2c_read(struct dw_hdmi *hdmi, > + unsigned char *buf, unsigned int length) > +{ > + struct dw_hdmi_i2c *i2c = hdmi->i2c; > + int stat; > + > + if (!i2c->is_regaddr) { > + dev_dbg(hdmi->dev, "set read register address to 0\n"); > + i2c->slave_reg = 0x00; > + i2c->is_regaddr = true; > + } > + > + while (length--) { > + reinit_completion(&i2c->cmp); > + > + dw_hdmi_qp_mod(hdmi, i2c->slave_reg++ << 12, I2CM_ADDR, > + I2CM_INTERFACE_CONTROL0); > + > + dw_hdmi_qp_mod(hdmi, I2CM_FM_READ, I2CM_WR_MASK, > + I2CM_INTERFACE_CONTROL0); Somehow the segment handling is present in the rest of the i2c code here, but not the actual handling for reads. The vendor-kernel does: - dw_hdmi_qp_mod(hdmi, I2CM_FM_READ, I2CM_WR_MASK, - I2CM_INTERFACE_CONTROL0); + if (i2c->is_segment) + dw_hdmi_qp_mod(hdmi, I2CM_EXT_READ, I2CM_WR_MASK, + I2CM_INTERFACE_CONTROL0); + else + dw_hdmi_qp_mod(hdmi, I2CM_FM_READ, I2CM_WR_MASK, + I2CM_INTERFACE_CONTROL0); Without this change, connecting to a DVI display does not work, and reading the EDID ends in the "i2c read error" below. Adding the segment handling as above makes the DVI connection work (as it does in the vendor-kernel). So it would be nice if you could maybe incorporate this in the next version? Thanks Heiko > + > + stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10); > + if (!stat) { > + dev_err(hdmi->dev, "i2c read timed out\n"); > + dw_hdmi_qp_write(hdmi, 0x01, I2CM_CONTROL0); > + return -EAGAIN; > + } > + > + /* Check for error condition on the bus */ > + if (i2c->stat & I2CM_NACK_RCVD_IRQ) { > + dev_err(hdmi->dev, "i2c read error\n"); > + dw_hdmi_qp_write(hdmi, 0x01, I2CM_CONTROL0); > + return -EIO; > + } > + > + *buf++ = dw_hdmi_qp_read(hdmi, I2CM_INTERFACE_RDDATA_0_3) & 0xff; > + dw_hdmi_qp_mod(hdmi, 0, I2CM_WR_MASK, I2CM_INTERFACE_CONTROL0); > + } > + > + i2c->is_segment = false; > + > + return 0; > +}
On 6/5/24 16:48, Heiko Stübner wrote: > Without this change, connecting to a DVI display does not work, and > reading the EDID ends in the "i2c read error" below. I had a lot of problems initially with the vendor driver on my DVI display, and am aware that several changes were required. However, I tested Cristian patch and worked fine. All modes were apparently detected from the display and they all worked. But maybe I was just lucky and it was using a somehow cached table, I can't say. This is an AOC DVI display from 2011 with a passive adapter. Luis
Am Mittwoch, 5. Juni 2024, 21:58:23 CEST schrieb Luis de Arquer: > On 6/5/24 16:48, Heiko Stübner wrote: > > Without this change, connecting to a DVI display does not work, and > > reading the EDID ends in the "i2c read error" below. > > I had a lot of problems initially with the vendor driver on my DVI > display, and am aware that several changes were required. > > However, I tested Cristian patch and worked fine. All modes were > apparently detected from the display and they all worked. But maybe I > was just lucky and it was using a somehow cached table, I can't say. > > This is an AOC DVI display from 2011 with a passive adapter. For me it is an LG IPS235. On its native hdmi input I always get regular 1080p output. But on its DVI input I always ran into the i2c read error before adding that change. I guess I should determine which reads actually fail.
On 6/5/24 5:48 PM, Heiko Stübner wrote: > Am Samstag, 1. Juni 2024, 15:12:35 CEST schrieb Cristian Ciocaltea: >> The Synopsys DesignWare HDMI 2.1 Quad-Pixel (QP) TX controller 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), aka >> Cinema VRR >> * Fast Vactive (FVA), aka Quick Frame Transport (QFT) >> * 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) >> >> Add driver to enable basic support, i.e. RGB output up to 4K@60Hz, >> without audio, CEC or any HDMI 2.1 specific features. >> >> Co-developed-by: Algea Cao <algea.cao@rock-chips.com> >> Signed-off-by: Algea Cao <algea.cao@rock-chips.com> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >> --- >> drivers/gpu/drm/bridge/synopsys/Makefile | 2 +- >> drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 787 +++++++++++++++++++++++++ >> drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h | 831 +++++++++++++++++++++++++++ >> include/drm/bridge/dw_hdmi.h | 8 + >> 4 files changed, 1627 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/Makefile b/drivers/gpu/drm/bridge/synopsys/Makefile >> index ce715562e9e5..8354e4879f70 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/Makefile >> +++ b/drivers/gpu/drm/bridge/synopsys/Makefile > >> +static int dw_hdmi_qp_i2c_read(struct dw_hdmi *hdmi, >> + unsigned char *buf, unsigned int length) >> +{ >> + struct dw_hdmi_i2c *i2c = hdmi->i2c; >> + int stat; >> + >> + if (!i2c->is_regaddr) { >> + dev_dbg(hdmi->dev, "set read register address to 0\n"); >> + i2c->slave_reg = 0x00; >> + i2c->is_regaddr = true; >> + } >> + >> + while (length--) { >> + reinit_completion(&i2c->cmp); >> + >> + dw_hdmi_qp_mod(hdmi, i2c->slave_reg++ << 12, I2CM_ADDR, >> + I2CM_INTERFACE_CONTROL0); >> + >> + dw_hdmi_qp_mod(hdmi, I2CM_FM_READ, I2CM_WR_MASK, >> + I2CM_INTERFACE_CONTROL0); > > Somehow the segment handling is present in the rest of the i2c code here, but > not the actual handling for reads. > > The vendor-kernel does: > > - dw_hdmi_qp_mod(hdmi, I2CM_FM_READ, I2CM_WR_MASK, > - I2CM_INTERFACE_CONTROL0); > + if (i2c->is_segment) > + dw_hdmi_qp_mod(hdmi, I2CM_EXT_READ, I2CM_WR_MASK, > + I2CM_INTERFACE_CONTROL0); > + else > + dw_hdmi_qp_mod(hdmi, I2CM_FM_READ, I2CM_WR_MASK, > + I2CM_INTERFACE_CONTROL0); Hmm, for some reason this is not present in the stable-5.10-rock5 branch I've been using as an implementation reference: https://github.com/radxa/kernel/blob/stable-5.10-rock5/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c#L760 Is there an updated fork? > Without this change, connecting to a DVI display does not work, and > reading the EDID ends in the "i2c read error" below. > > Adding the segment handling as above makes the DVI connection > work (as it does in the vendor-kernel). > > So it would be nice if you could maybe incorporate this in the next version? Sure, thanks for pointing this out! Cristian
Am Donnerstag, 6. Juni 2024, 11:53:23 CEST schrieb Cristian Ciocaltea: > On 6/5/24 5:48 PM, Heiko Stübner wrote: > > Am Samstag, 1. Juni 2024, 15:12:35 CEST schrieb Cristian Ciocaltea: > >> The Synopsys DesignWare HDMI 2.1 Quad-Pixel (QP) TX controller 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), aka > >> Cinema VRR > >> * Fast Vactive (FVA), aka Quick Frame Transport (QFT) > >> * 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) > >> > >> Add driver to enable basic support, i.e. RGB output up to 4K@60Hz, > >> without audio, CEC or any HDMI 2.1 specific features. > >> > >> Co-developed-by: Algea Cao <algea.cao@rock-chips.com> > >> Signed-off-by: Algea Cao <algea.cao@rock-chips.com> > >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> > >> --- > >> drivers/gpu/drm/bridge/synopsys/Makefile | 2 +- > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 787 +++++++++++++++++++++++++ > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h | 831 +++++++++++++++++++++++++++ > >> include/drm/bridge/dw_hdmi.h | 8 + > >> 4 files changed, 1627 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/synopsys/Makefile b/drivers/gpu/drm/bridge/synopsys/Makefile > >> index ce715562e9e5..8354e4879f70 100644 > >> --- a/drivers/gpu/drm/bridge/synopsys/Makefile > >> +++ b/drivers/gpu/drm/bridge/synopsys/Makefile > > > >> +static int dw_hdmi_qp_i2c_read(struct dw_hdmi *hdmi, > >> + unsigned char *buf, unsigned int length) > >> +{ > >> + struct dw_hdmi_i2c *i2c = hdmi->i2c; > >> + int stat; > >> + > >> + if (!i2c->is_regaddr) { > >> + dev_dbg(hdmi->dev, "set read register address to 0\n"); > >> + i2c->slave_reg = 0x00; > >> + i2c->is_regaddr = true; > >> + } > >> + > >> + while (length--) { > >> + reinit_completion(&i2c->cmp); > >> + > >> + dw_hdmi_qp_mod(hdmi, i2c->slave_reg++ << 12, I2CM_ADDR, > >> + I2CM_INTERFACE_CONTROL0); > >> + > >> + dw_hdmi_qp_mod(hdmi, I2CM_FM_READ, I2CM_WR_MASK, > >> + I2CM_INTERFACE_CONTROL0); > > > > Somehow the segment handling is present in the rest of the i2c code here, but > > not the actual handling for reads. > > > > The vendor-kernel does: > > > > - dw_hdmi_qp_mod(hdmi, I2CM_FM_READ, I2CM_WR_MASK, > > - I2CM_INTERFACE_CONTROL0); > > + if (i2c->is_segment) > > + dw_hdmi_qp_mod(hdmi, I2CM_EXT_READ, I2CM_WR_MASK, > > + I2CM_INTERFACE_CONTROL0); > > + else > > + dw_hdmi_qp_mod(hdmi, I2CM_FM_READ, I2CM_WR_MASK, > > + I2CM_INTERFACE_CONTROL0); > > Hmm, for some reason this is not present in the stable-5.10-rock5 branch > I've been using as an implementation reference: > > https://github.com/radxa/kernel/blob/stable-5.10-rock5/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c#L760 > > Is there an updated fork? I think the radxa code-base is quite old in terms of sdk-version it's based on. Grabbing a 6.1 branch from Radxa shows it in: https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr1/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c#L995 > > Without this change, connecting to a DVI display does not work, and > > reading the EDID ends in the "i2c read error" below. > > > > Adding the segment handling as above makes the DVI connection > > work (as it does in the vendor-kernel). > > > > So it would be nice if you could maybe incorporate this in the next version? > > Sure, thanks for pointing this out! > > Cristian >
On 6/6/24 1:16 PM, Heiko Stübner wrote: > Am Donnerstag, 6. Juni 2024, 11:53:23 CEST schrieb Cristian Ciocaltea: >> On 6/5/24 5:48 PM, Heiko Stübner wrote: >>> Am Samstag, 1. Juni 2024, 15:12:35 CEST schrieb Cristian Ciocaltea: >>>> The Synopsys DesignWare HDMI 2.1 Quad-Pixel (QP) TX controller 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), aka >>>> Cinema VRR >>>> * Fast Vactive (FVA), aka Quick Frame Transport (QFT) >>>> * 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) >>>> >>>> Add driver to enable basic support, i.e. RGB output up to 4K@60Hz, >>>> without audio, CEC or any HDMI 2.1 specific features. >>>> >>>> Co-developed-by: Algea Cao <algea.cao@rock-chips.com> >>>> Signed-off-by: Algea Cao <algea.cao@rock-chips.com> >>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >>>> --- >>>> drivers/gpu/drm/bridge/synopsys/Makefile | 2 +- >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 787 +++++++++++++++++++++++++ >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h | 831 +++++++++++++++++++++++++++ >>>> include/drm/bridge/dw_hdmi.h | 8 + >>>> 4 files changed, 1627 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/Makefile b/drivers/gpu/drm/bridge/synopsys/Makefile >>>> index ce715562e9e5..8354e4879f70 100644 >>>> --- a/drivers/gpu/drm/bridge/synopsys/Makefile >>>> +++ b/drivers/gpu/drm/bridge/synopsys/Makefile >>> >>>> +static int dw_hdmi_qp_i2c_read(struct dw_hdmi *hdmi, >>>> + unsigned char *buf, unsigned int length) >>>> +{ >>>> + struct dw_hdmi_i2c *i2c = hdmi->i2c; >>>> + int stat; >>>> + >>>> + if (!i2c->is_regaddr) { >>>> + dev_dbg(hdmi->dev, "set read register address to 0\n"); >>>> + i2c->slave_reg = 0x00; >>>> + i2c->is_regaddr = true; >>>> + } >>>> + >>>> + while (length--) { >>>> + reinit_completion(&i2c->cmp); >>>> + >>>> + dw_hdmi_qp_mod(hdmi, i2c->slave_reg++ << 12, I2CM_ADDR, >>>> + I2CM_INTERFACE_CONTROL0); >>>> + >>>> + dw_hdmi_qp_mod(hdmi, I2CM_FM_READ, I2CM_WR_MASK, >>>> + I2CM_INTERFACE_CONTROL0); >>> >>> Somehow the segment handling is present in the rest of the i2c code here, but >>> not the actual handling for reads. >>> >>> The vendor-kernel does: >>> >>> - dw_hdmi_qp_mod(hdmi, I2CM_FM_READ, I2CM_WR_MASK, >>> - I2CM_INTERFACE_CONTROL0); >>> + if (i2c->is_segment) >>> + dw_hdmi_qp_mod(hdmi, I2CM_EXT_READ, I2CM_WR_MASK, >>> + I2CM_INTERFACE_CONTROL0); >>> + else >>> + dw_hdmi_qp_mod(hdmi, I2CM_FM_READ, I2CM_WR_MASK, >>> + I2CM_INTERFACE_CONTROL0); >> >> Hmm, for some reason this is not present in the stable-5.10-rock5 branch >> I've been using as an implementation reference: >> >> https://github.com/radxa/kernel/blob/stable-5.10-rock5/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c#L760 >> >> Is there an updated fork? > > I think the radxa code-base is quite old in terms of sdk-version it's based on. > Grabbing a 6.1 branch from Radxa shows it in: > https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr1/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c#L995 Indeed, I should have switched to using this one as it seems to include quite a few potentially interesting updates (will check in detail for v2). For now, we miss cdc0984c90dc ("drm/bridge: synopsys: dw-hdmi-qp: Support read ext block edid"), which provides an additional change: @@ -1086,7 +1090,7 @@ static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap, i2c->is_segment = true; hdmi_modb(hdmi, DDC_SEGMENT_ADDR, I2CM_SEG_ADDR, I2CM_INTERFACE_CONTROL1); - hdmi_modb(hdmi, *msgs[i].buf, I2CM_SEG_PTR, + hdmi_modb(hdmi, *msgs[i].buf << 7, I2CM_SEG_PTR, I2CM_INTERFACE_CONTROL1); } else { if (msgs[i].flags & I2C_M_RD) Will try to grab some more displays to improve testing on my end. Thanks, Cristian
Hi Neil， At 2024-06-05 19:48:09, "Neil Armstrong" <neil.armstrong@linaro.org> wrote: >On 05/06/2024 12:11, Cristian Ciocaltea wrote: >> On 6/5/24 12:34 AM, Cristian Ciocaltea wrote: >>> On 6/4/24 11:41 PM, Sam Ravnborg wrote: >>>> Hi Cristian. >>>> >>>> On Tue, Jun 04, 2024 at 10:32:04PM +0300, Cristian Ciocaltea wrote: >>>>> Hi Sam, >>>>> >>>>> On 6/1/24 5:32 PM, Sam Ravnborg wrote: >>>>>> Hi Cristian, >>>>>> >>>>>> a few drive-by comments below. >>>>>> >>>>>> Sam >>>>>> >>>>>> >>>>>>> + >>>>>>> +static const struct drm_connector_funcs dw_hdmi_qp_connector_funcs = { >>>>>>> + .fill_modes = drm_helper_probe_single_connector_modes, >>>>>>> + .detect = dw_hdmi_connector_detect, >>>>>>> + .destroy = drm_connector_cleanup, >>>>>>> + .force = dw_hdmi_qp_connector_force, >>>>>>> + .reset = drm_atomic_helper_connector_reset, >>>>>>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >>>>>>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >>>>>>> +}; >>>>>>> + >>>>>>> +static int dw_hdmi_qp_bridge_attach(struct drm_bridge *bridge, >>>>>>> + enum drm_bridge_attach_flags flags) >>>>>>> +{ >>>>>>> + struct dw_hdmi *hdmi = bridge->driver_private; >>>>>>> + >>>>>>> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) >>>>>>> + return drm_bridge_attach(bridge->encoder, hdmi->next_bridge, >>>>>>> + bridge, flags); >>>>>>> + >>>>>>> + return dw_hdmi_connector_create(hdmi, &dw_hdmi_qp_connector_funcs); >>>>>>> +} >>>>>> >>>>>> Are there any users left that requires the display driver to create the >>>>>> connector? >>>>>> In other words - could this driver fail if DRM_BRIDGE_ATTACH_NO_CONNECTOR >>>>>> is not passed and drop dw_hdmi_connector_create()? >>>>>> >>>>>> I did not try to verify this - just a naive question. >>>>> >>>>> I've just tested this and it doesn't work - dw_hdmi_connector_create() >>>>> is still needed. >>>> >>>> Hmm, seems the display driver or some other bridge driver fails to >>>> support "DRM_BRIDGE_ATTACH_NO_CONNECTOR". >>>> what other drivers are involved? >>> >>> Could it be related to the glue driver (updated in the next patch) which >>> is also responsible for setting up the encoder? >>> >>>> Note that my comments here should be seen as potential future >>>> improvements, and do not block the patch from being used. >>> >>> Thanks for the heads up! Will try to get back to this soon and investigate. >> >> IIUC, modern bridges should not create the connector but rely on display >> drivers to take care of, which in this case is the VOP2 driver. However, >> it also handles some of the older SoCs relying on the non-QP variant of >> DW HDMI IP. Hence the existing dw-hdmi driver would be also impacted in >> order to come up with a proper solution. >> >> A quick check shows there are several users of this IP: >> >> $ git grep -E '= dw_hdmi_(bind|probe)\(' >> drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c: hdmi->dw_hdmi = dw_hdmi_probe(pdev, plat_data); >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c: hdmi = dw_hdmi_probe(pdev, plat_data); >> drivers/gpu/drm/imx/ipuv3/dw_hdmi-imx.c: hdmi->hdmi = dw_hdmi_probe(pdev, match->data); >> drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c: hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data); >> drivers/gpu/drm/meson/meson_dw_hdmi.c: meson_dw_hdmi->hdmi = dw_hdmi_probe(pdev, &meson_dw_hdmi->dw_plat_data); >> drivers/gpu/drm/renesas/rcar-du/rcar_dw_hdmi.c: hdmi = dw_hdmi_probe(pdev, &rcar_dw_hdmi_plat_data); >> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c: hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data); >> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c: hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data); >> >> I didn't check which display drivers would be involved, I'd guess there >> are quite a few of them as well. So it seems this ends up being a pretty >> complex task. > >If this would be a brand new driver, then it should only support DRM_BRIDGE_ATTACH_NO_CONNECTOR, >so you should not create a connector from the driver. > >The fact dw-hdmi accepts an attach without the flag is for legacy purpose >since some DRM drivers haven't switched to DRM_BRIDGE_ATTACH_NO_CONNECTOR yes, >but it's a requirement for new bridges so at some point you'll need to make >sure the rockchip glue and drm driver supports DRM_BRIDGE_ATTACH_NO_CONNECTOR. > >This will greatly simplify the driver! Based on the previous discussion， the DW HDMI QP drivers will be implemented like this： Core bridge library： drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c Rockchip platform specific glue： drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c As a new bridge driver should only support DRM_BRIDGE_ATTACH_NO_CONNECTOR, Is it acceptable if we implement the connector at the rockchip glue dw_hdmi_qp-rockchip.c ？ Our current combination is a bit complex： The display controller driver is drivers/gpu/drm/rockchip/rockchip_drm_vop2.c ，which shared by rk3568， rk3588 and some upcoming soc like rk3528/rk3562. For rk3588， we have totally new HDMI、DP、DSI2 IP， they need brand new bridge driver that should only support DRM_BRIDGE_ATTACH_NO_CONNECTOR， and there is also an eDP on rk3588 use analogix_dp_core.c that create connector by analogix_dp bridge。 For rk3568， the HDMI/eDP/DSI IP are based on old IP， the corresponding drivers are dw-hdmi，analogix_dp and dw-mipi-dsi, they both create drm connector by it's bridge driver. And rk3528/rk3562 are like this too。 So if we can create drm_connector at glue side (such as dw_hdmi_qp-rockchip.c), let the interface driver decide if it should create drm_connector or not will make the vop2 driver simpler。 > >Neil > >> >> Cristian
Hi, On Fri, Jun 14, 2024 at 02:56:00PM GMT, Andy Yan wrote: > At 2024-06-05 19:48:09, "Neil Armstrong" <neil.armstrong@linaro.org> wrote: > >On 05/06/2024 12:11, Cristian Ciocaltea wrote: > >> On 6/5/24 12:34 AM, Cristian Ciocaltea wrote: > >>> On 6/4/24 11:41 PM, Sam Ravnborg wrote: > >>>> Hi Cristian. > >>>> > >>>> On Tue, Jun 04, 2024 at 10:32:04PM +0300, Cristian Ciocaltea wrote: > >>>>> Hi Sam, > >>>>> > >>>>> On 6/1/24 5:32 PM, Sam Ravnborg wrote: > >>>>>> Hi Cristian, > >>>>>> > >>>>>> a few drive-by comments below. > >>>>>> > >>>>>> Sam > >>>>>> > >>>>>> > >>>>>>> + > >>>>>>> +static const struct drm_connector_funcs dw_hdmi_qp_connector_funcs = { > >>>>>>> + .fill_modes = drm_helper_probe_single_connector_modes, > >>>>>>> + .detect = dw_hdmi_connector_detect, > >>>>>>> + .destroy = drm_connector_cleanup, > >>>>>>> + .force = dw_hdmi_qp_connector_force, > >>>>>>> + .reset = drm_atomic_helper_connector_reset, > >>>>>>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > >>>>>>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > >>>>>>> +}; > >>>>>>> + > >>>>>>> +static int dw_hdmi_qp_bridge_attach(struct drm_bridge *bridge, > >>>>>>> + enum drm_bridge_attach_flags flags) > >>>>>>> +{ > >>>>>>> + struct dw_hdmi *hdmi = bridge->driver_private; > >>>>>>> + > >>>>>>> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) > >>>>>>> + return drm_bridge_attach(bridge->encoder, hdmi->next_bridge, > >>>>>>> + bridge, flags); > >>>>>>> + > >>>>>>> + return dw_hdmi_connector_create(hdmi, &dw_hdmi_qp_connector_funcs); > >>>>>>> +} > >>>>>> > >>>>>> Are there any users left that requires the display driver to create the > >>>>>> connector? > >>>>>> In other words - could this driver fail if DRM_BRIDGE_ATTACH_NO_CONNECTOR > >>>>>> is not passed and drop dw_hdmi_connector_create()? > >>>>>> > >>>>>> I did not try to verify this - just a naive question. > >>>>> > >>>>> I've just tested this and it doesn't work - dw_hdmi_connector_create() > >>>>> is still needed. > >>>> > >>>> Hmm, seems the display driver or some other bridge driver fails to > >>>> support "DRM_BRIDGE_ATTACH_NO_CONNECTOR". > >>>> what other drivers are involved? > >>> > >>> Could it be related to the glue driver (updated in the next patch) which > >>> is also responsible for setting up the encoder? > >>> > >>>> Note that my comments here should be seen as potential future > >>>> improvements, and do not block the patch from being used. > >>> > >>> Thanks for the heads up! Will try to get back to this soon and investigate. > >> > >> IIUC, modern bridges should not create the connector but rely on display > >> drivers to take care of, which in this case is the VOP2 driver. However, > >> it also handles some of the older SoCs relying on the non-QP variant of > >> DW HDMI IP. Hence the existing dw-hdmi driver would be also impacted in > >> order to come up with a proper solution. > >> > >> A quick check shows there are several users of this IP: > >> > >> $ git grep -E '= dw_hdmi_(bind|probe)\(' > >> drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c: hdmi->dw_hdmi = dw_hdmi_probe(pdev, plat_data); > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c: hdmi = dw_hdmi_probe(pdev, plat_data); > >> drivers/gpu/drm/imx/ipuv3/dw_hdmi-imx.c: hdmi->hdmi = dw_hdmi_probe(pdev, match->data); > >> drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c: hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data); > >> drivers/gpu/drm/meson/meson_dw_hdmi.c: meson_dw_hdmi->hdmi = dw_hdmi_probe(pdev, &meson_dw_hdmi->dw_plat_data); > >> drivers/gpu/drm/renesas/rcar-du/rcar_dw_hdmi.c: hdmi = dw_hdmi_probe(pdev, &rcar_dw_hdmi_plat_data); > >> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c: hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data); > >> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c: hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data); > >> > >> I didn't check which display drivers would be involved, I'd guess there > >> are quite a few of them as well. So it seems this ends up being a pretty > >> complex task. > > > >If this would be a brand new driver, then it should only support DRM_BRIDGE_ATTACH_NO_CONNECTOR, > >so you should not create a connector from the driver. > > > >The fact dw-hdmi accepts an attach without the flag is for legacy purpose > >since some DRM drivers haven't switched to DRM_BRIDGE_ATTACH_NO_CONNECTOR yes, > >but it's a requirement for new bridges so at some point you'll need to make > >sure the rockchip glue and drm driver supports DRM_BRIDGE_ATTACH_NO_CONNECTOR. > > > >This will greatly simplify the driver! > > Based on the previous discussion， the DW HDMI QP drivers will be implemented like this： > > Core bridge library： > drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c > Rockchip platform specific glue： > drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c Note that the bridge HDMI helpers Dmitry was mentioning have now been merged, so I'd expect the core bridge library to be fairly minimal. And the new driver should obviously use it as much as it can. Maxime
On 14/06/2024 08:56, Andy Yan wrote: > > > > > > > > Hi Neil， > > At 2024-06-05 19:48:09, "Neil Armstrong" <neil.armstrong@linaro.org> wrote: >> On 05/06/2024 12:11, Cristian Ciocaltea wrote: >>> On 6/5/24 12:34 AM, Cristian Ciocaltea wrote: >>>> On 6/4/24 11:41 PM, Sam Ravnborg wrote: >>>>> Hi Cristian. >>>>> >>>>> On Tue, Jun 04, 2024 at 10:32:04PM +0300, Cristian Ciocaltea wrote: >>>>>> Hi Sam, >>>>>> >>>>>> On 6/1/24 5:32 PM, Sam Ravnborg wrote: >>>>>>> Hi Cristian, >>>>>>> >>>>>>> a few drive-by comments below. >>>>>>> >>>>>>> Sam >>>>>>> >>>>>>> >>>>>>>> + >>>>>>>> +static const struct drm_connector_funcs dw_hdmi_qp_connector_funcs = { >>>>>>>> + .fill_modes = drm_helper_probe_single_connector_modes, >>>>>>>> + .detect = dw_hdmi_connector_detect, >>>>>>>> + .destroy = drm_connector_cleanup, >>>>>>>> + .force = dw_hdmi_qp_connector_force, >>>>>>>> + .reset = drm_atomic_helper_connector_reset, >>>>>>>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >>>>>>>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >>>>>>>> +}; >>>>>>>> + >>>>>>>> +static int dw_hdmi_qp_bridge_attach(struct drm_bridge *bridge, >>>>>>>> + enum drm_bridge_attach_flags flags) >>>>>>>> +{ >>>>>>>> + struct dw_hdmi *hdmi = bridge->driver_private; >>>>>>>> + >>>>>>>> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) >>>>>>>> + return drm_bridge_attach(bridge->encoder, hdmi->next_bridge, >>>>>>>> + bridge, flags); >>>>>>>> + >>>>>>>> + return dw_hdmi_connector_create(hdmi, &dw_hdmi_qp_connector_funcs); >>>>>>>> +} >>>>>>> >>>>>>> Are there any users left that requires the display driver to create the >>>>>>> connector? >>>>>>> In other words - could this driver fail if DRM_BRIDGE_ATTACH_NO_CONNECTOR >>>>>>> is not passed and drop dw_hdmi_connector_create()? >>>>>>> >>>>>>> I did not try to verify this - just a naive question. >>>>>> >>>>>> I've just tested this and it doesn't work - dw_hdmi_connector_create() >>>>>> is still needed. >>>>> >>>>> Hmm, seems the display driver or some other bridge driver fails to >>>>> support "DRM_BRIDGE_ATTACH_NO_CONNECTOR". >>>>> what other drivers are involved? >>>> >>>> Could it be related to the glue driver (updated in the next patch) which >>>> is also responsible for setting up the encoder? >>>> >>>>> Note that my comments here should be seen as potential future >>>>> improvements, and do not block the patch from being used. >>>> >>>> Thanks for the heads up! Will try to get back to this soon and investigate. >>> >>> IIUC, modern bridges should not create the connector but rely on display >>> drivers to take care of, which in this case is the VOP2 driver. However, >>> it also handles some of the older SoCs relying on the non-QP variant of >>> DW HDMI IP. Hence the existing dw-hdmi driver would be also impacted in >>> order to come up with a proper solution. >>> >>> A quick check shows there are several users of this IP: >>> >>> $ git grep -E '= dw_hdmi_(bind|probe)\(' >>> drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c: hdmi->dw_hdmi = dw_hdmi_probe(pdev, plat_data); >>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c: hdmi = dw_hdmi_probe(pdev, plat_data); >>> drivers/gpu/drm/imx/ipuv3/dw_hdmi-imx.c: hdmi->hdmi = dw_hdmi_probe(pdev, match->data); >>> drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c: hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data); >>> drivers/gpu/drm/meson/meson_dw_hdmi.c: meson_dw_hdmi->hdmi = dw_hdmi_probe(pdev, &meson_dw_hdmi->dw_plat_data); >>> drivers/gpu/drm/renesas/rcar-du/rcar_dw_hdmi.c: hdmi = dw_hdmi_probe(pdev, &rcar_dw_hdmi_plat_data); >>> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c: hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data); >>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c: hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data); >>> >>> I didn't check which display drivers would be involved, I'd guess there >>> are quite a few of them as well. So it seems this ends up being a pretty >>> complex task. >> >> If this would be a brand new driver, then it should only support DRM_BRIDGE_ATTACH_NO_CONNECTOR, >> so you should not create a connector from the driver. >> >> The fact dw-hdmi accepts an attach without the flag is for legacy purpose >> since some DRM drivers haven't switched to DRM_BRIDGE_ATTACH_NO_CONNECTOR yes, >> but it's a requirement for new bridges so at some point you'll need to make >> sure the rockchip glue and drm driver supports DRM_BRIDGE_ATTACH_NO_CONNECTOR. >> >> This will greatly simplify the driver! > > Based on the previous discussion， the DW HDMI QP drivers will be implemented like this： > > Core bridge library： > drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c > Rockchip platform specific glue： > drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c > > As a new bridge driver should only support DRM_BRIDGE_ATTACH_NO_CONNECTOR, > Is it acceptable if we implement the connector at the rockchip glue dw_hdmi_qp-rockchip.c ？ > > Our current combination is a bit complex： > The display controller driver is drivers/gpu/drm/rockchip/rockchip_drm_vop2.c ，which shared > by rk3568， rk3588 and some upcoming soc like rk3528/rk3562. > > For rk3588， we have totally new HDMI、DP、DSI2 IP， they need brand new bridge driver that > should only support DRM_BRIDGE_ATTACH_NO_CONNECTOR， and there is also an eDP on rk3588 > use analogix_dp_core.c that create connector by analogix_dp bridge。 > > For rk3568， the HDMI/eDP/DSI IP are based on old IP， the corresponding drivers are dw-hdmi，analogix_dp > and dw-mipi-dsi, they both create drm connector by it's bridge driver. And rk3528/rk3562 are like this too。 > > So if we can create drm_connector at glue side (such as dw_hdmi_qp-rockchip.c), let the interface driver decide > if it should create drm_connector or not will make the vop2 driver simpler。 I think you should start migration to drm_bridge_connector instead of hacking dw_hdmi_qp-rockchip.c into fitting into DRM_BRIDGE_ATTACH_NO_CONNECTOR. You'll add technical dept, and the migration will be even harder afterwards. But in any case, bridge/synopsys/dw-hdmi-qp.c and rockchip/dw_hdmi_qp-rockchip.c should be send in two separate patchsets, so how rockchip DRM_BRIDGE_ATTACH_NO_CONNECTOR is a different story. Neil > > > > > >> >> Neil >> >>> >>> Cristian
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
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