Re: [PATCH v5 05/10] drm/bridge: dw-hdmi-qp: Add HDMI 2.0 SCDC scrambling and high TMDS clock ratio support

Dmitry Baryshkov
2026-04-28T01:39:00Z

On Sun, Apr 26, 2026 at 03:20:17AM +0300, Cristian Ciocaltea wrote:
> Enable HDMI 2.0 display modes (e.g. 4K@60Hz) by adding SCDC management
> for the high TMDS clock ratio and scrambling, required when the TMDS
> character rate exceeds the 340 MHz HDMI 1.4b limit.
>
> A periodic work item monitors the sink's scrambling status to recover
> from sink-side resets. On hotplug detect, if SCDC scrambling state is
> out of sync with the driver, trigger a CRTC reset to re-establish the
> link.
>
> Reject modes requiring TMDS rates above 600 MHz, as those fall in the
> HDMI 2.1 FRL domain which is not supported. In no_hpd configurations,
> further restrict to 340 MHz since SCDC requires a connected sink.
>
> Tested-by: Diederik de Haas <diederik@cknow-tech.com>
> Tested-by: Maud Spierings <maud_spierings@hotmail.com>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
> drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 188 ++++++++++++++++++++++++---
> 1 file changed, 172 insertions(+), 16 deletions(-)

My main issue with this patch (sorry) is that this adds yet another copy
of SCDC-related helpers into the driver which already OP_HDMI and other
helpers.

> @@ -39,7 +42,9 @@
> #define DDC_SEGMENT_ADDR 0x30
>
> #define HDMI14_MAX_TMDSCLK 340000000
> +#define HDMI20_MAX_TMDSRATE 600000000
>
> +#define SCDC_MAX_SOURCE_VERSION 0x1
> #define SCRAMB_POLL_DELAY_MS 3000
>
> /*
> @@ -164,6 +169,11 @@ struct dw_hdmi_qp {
> } phy;
>
> unsigned long ref_clk_rate;
> +
> + struct drm_connector *curr_conn;
> + struct delayed_work scramb_work;
> + bool scramb_enabled;

s/scramb/scrambler/

Can we move those two to the drm_connector_hdmi

> +
> struct regmap *regm;
> int main_irq;
>
> @@ -749,28 +759,124 @@ static struct i2c_adapter *dw_hdmi_qp_i2c_adapter(struct dw_hdmi_qp *hdmi)
> return adap;
> }
>
> +static bool dw_hdmi_qp_supports_scrambling(struct drm_display_info *display)
> +{
> + if (!display->is_hdmi)
> + return false;
> +
> + return display->hdmi.scdc.supported &&
> + display->hdmi.scdc.scrambling.supported;
> +}
> +
> +static int dw_hdmi_qp_set_scramb(struct dw_hdmi_qp *hdmi)
> +{
> + bool done;
> +
> + dev_dbg(hdmi->dev, "set scrambling\n");
> +
> + done = drm_scdc_set_high_tmds_clock_ratio(hdmi->curr_conn, true);
> + if (!done)
> + return -EIO;
> +
> + done = drm_scdc_set_scrambling(hdmi->curr_conn, true);
> + if (!done) {
> + drm_scdc_set_high_tmds_clock_ratio(hdmi->curr_conn, false);
> + return -EIO;
> + }
> +
> + schedule_delayed_work(&hdmi->scramb_work,
> + msecs_to_jiffies(SCRAMB_POLL_DELAY_MS));
> + return 0;
> +}
> +
> +static void dw_hdmi_qp_scramb_work(struct work_struct *work)
> +{
> + struct dw_hdmi_qp *hdmi = container_of(to_delayed_work(work),
> + struct dw_hdmi_qp,
> + scramb_work);
> + if (READ_ONCE(hdmi->scramb_enabled) &&
> + !drm_scdc_get_scrambling_status(hdmi->curr_conn))
> + dw_hdmi_qp_set_scramb(hdmi);
> +}
> +
> +static void dw_hdmi_qp_enable_scramb(struct dw_hdmi_qp *hdmi)
> +{
> + int ret;
> + u8 ver;
> +
> + if (!dw_hdmi_qp_supports_scrambling(&hdmi->curr_conn->display_info))
> + return;
> +
> + ret = drm_scdc_readb(hdmi->bridge.ddc, SCDC_SINK_VERSION, &ver);
> + if (ret) {
> + dev_err(hdmi->dev, "Failed to read SCDC_SINK_VERSION: %d\n", ret);
> + return;
> + }
> +
> + ret = drm_scdc_writeb(hdmi->bridge.ddc, SCDC_SOURCE_VERSION,
> + min_t(u8, ver, SCDC_MAX_SOURCE_VERSION));
> + if (ret) {
> + dev_err(hdmi->dev, "Failed to write SCDC_SOURCE_VERSION: %d\n", ret);
> + return;
> + }
> +
> + WRITE_ONCE(hdmi->scramb_enabled, true);
> +
> + ret = dw_hdmi_qp_set_scramb(hdmi);
> + if (ret) {
> + hdmi->scramb_enabled = false;
> + return;
> + }
> +
> + dw_hdmi_qp_write(hdmi, 1, SCRAMB_CONFIG0);
> +
> + /* Wait at least 1 ms before resuming TMDS transmission */
> + usleep_range(1000, 5000);
> +}
> +
> +static void dw_hdmi_qp_disable_scramb(struct dw_hdmi_qp *hdmi)
> +{
> + if (!hdmi->scramb_enabled)
> + return;
> +
> + dev_dbg(hdmi->dev, "disable scrambling\n");
> +
> + WRITE_ONCE(hdmi->scramb_enabled, false);
> + cancel_delayed_work_sync(&hdmi->scramb_work);
> +
> + dw_hdmi_qp_write(hdmi, 0, SCRAMB_CONFIG0);
> +
> + if (hdmi->curr_conn->status == connector_status_connected) {
> + drm_scdc_set_scrambling(hdmi->curr_conn, false);
> + drm_scdc_set_high_tmds_clock_ratio(hdmi->curr_conn, false);
> + }
> +}

All of these feel like generic helpers.

> +
> static void dw_hdmi_qp_bridge_atomic_enable(struct drm_bridge *bridge,
> struct drm_atomic_state *state)
> {
> struct dw_hdmi_qp *hdmi = bridge->driver_private;
> struct drm_connector_state *conn_state;
> - struct drm_connector *connector;
> unsigned int op_mode;
>
> - connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
> - if (WARN_ON(!connector))
> + hdmi->curr_conn = drm_atomic_get_new_connector_for_encoder(state,
> + bridge->encoder);
> + if (WARN_ON(!hdmi->curr_conn))
> return;
>
> - conn_state = drm_atomic_get_new_connector_state(state, connector);
> + conn_state = drm_atomic_get_new_connector_state(state, hdmi->curr_conn);
> if (WARN_ON(!conn_state))
> return;
>
> - if (connector->display_info.is_hdmi) {
> + if (hdmi->curr_conn->display_info.is_hdmi) {
> dev_dbg(hdmi->dev, "%s mode=HDMI %s rate=%llu bpc=%u\n", __func__,
> drm_hdmi_connector_get_output_format_name(conn_state->hdmi.output_format),
> conn_state->hdmi.tmds_char_rate, conn_state->hdmi.output_bpc);
> op_mode = 0;
> hdmi->tmds_char_rate = conn_state->hdmi.tmds_char_rate;
> +
> + if (conn_state->hdmi.tmds_char_rate > HDMI14_MAX_TMDSCLK)
> + dw_hdmi_qp_enable_scramb(hdmi);
> } else {
> dev_dbg(hdmi->dev, "%s mode=DVI\n", __func__);
> op_mode = OPMODE_DVI;
> @@ -781,7 +887,7 @@ static void dw_hdmi_qp_bridge_atomic_enable(struct drm_bridge *bridge,
> dw_hdmi_qp_mod(hdmi, HDCP2_BYPASS, HDCP2_BYPASS, HDCP2LOGIC_CONFIG0);
> dw_hdmi_qp_mod(hdmi, op_mode, OPMODE_DVI, LINK_CONFIG0);
>
> - drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
> + drm_atomic_helper_connector_hdmi_update_infoframes(hdmi->curr_conn, state);
> }
>
> static void dw_hdmi_qp_bridge_atomic_disable(struct drm_bridge *bridge,
> @@ -791,14 +897,49 @@ static void dw_hdmi_qp_bridge_atomic_disable(struct drm_bridge *bridge,
>
> hdmi->tmds_char_rate = 0;
>
> + dw_hdmi_qp_disable_scramb(hdmi);
> +
> + hdmi->curr_conn = NULL;
> hdmi->phy.ops->disable(hdmi, hdmi->phy.data);
> }
>
> -static enum drm_connector_status
> -dw_hdmi_qp_bridge_detect(struct drm_bridge *bridge, struct drm_connector *connector)
> +static int dw_hdmi_qp_reset_crtc(struct dw_hdmi_qp *hdmi,
> + struct drm_connector *connector,
> + struct drm_modeset_acquire_ctx *ctx)
> +{
> + u8 config;
> + int ret;
> +
> + ret = drm_scdc_readb(hdmi->bridge.ddc, SCDC_TMDS_CONFIG, &config);
> + if (ret < 0) {
> + dev_err(hdmi->dev, "Failed to read TMDS config: %d\n", ret);
> + return ret;
> + }
> +
> + if (!!(config & SCDC_SCRAMBLING_ENABLE) == hdmi->scramb_enabled)
> + return 0;

Also please check the high TMDS clock ration bit.

> +
> + drm_atomic_helper_connector_hdmi_hotplug(connector,
> + connector_status_connected);

I don't see a forced hotplug event in the existing drivers. Why is it
necessary? This function is being called from the detect() path.

> + /*
> + * Conform to HDMI 2.0 spec by ensuring scrambled data is not sent
> + * before configuring the sink scrambling, as well as suspending any
> + * TMDS transmission while changing the TMDS clock rate in the sink.
> + */
> +
> + dev_dbg(hdmi->dev, "resetting crtc\n");
> +
> + return drm_bridge_helper_reset_crtc(&hdmi->bridge, ctx);
> +}
> +
> +static int dw_hdmi_qp_bridge_detect_ctx(struct drm_bridge *bridge,
> + struct drm_connector *connector,
> + struct drm_modeset_acquire_ctx *ctx)
> {
> struct dw_hdmi_qp *hdmi = bridge->driver_private;
> + enum drm_connector_status status;
> const struct drm_edid *drm_edid;
> + int ret;
>
> if (hdmi->no_hpd) {
> drm_edid = drm_edid_read_ddc(connector, bridge->ddc);
> @@ -808,7 +949,20 @@ dw_hdmi_qp_bridge_detect(struct drm_bridge *bridge, struct drm_connector *connec
> return connector_status_disconnected;
> }
>
> - return hdmi->phy.ops->read_hpd(hdmi, hdmi->phy.data);
> + status = hdmi->phy.ops->read_hpd(hdmi, hdmi->phy.data);
> +
> + dev_dbg(hdmi->dev, "%s status=%d scramb=%d\n", __func__,
> + status, hdmi->scramb_enabled);
> +
> + if (status == connector_status_connected && hdmi->scramb_enabled) {
> + ret = dw_hdmi_qp_reset_crtc(hdmi, connector, ctx);
> + if (ret == -EDEADLK)
> + return ret;
> + if (ret < 0)
> + status = connector_status_unknown;

Ideally this should go to the drm_atomic_helper_connector_hdmi_update().
And once it goes, I don't think we'd need the detect_ctx() callback for
bridges.

> + }
> +
> + return status;
> }
>
> static const struct drm_edid *
> @@ -832,12 +986,12 @@ dw_hdmi_qp_bridge_tmds_char_rate_valid(const struct drm_bridge *bridge,
> {
> struct dw_hdmi_qp *hdmi = bridge->driver_private;
>
> - /*
> - * TODO: when hdmi->no_hpd is 1 we must not support modes that
> - * require scrambling, including every mode with a clock above
> - * HDMI14_MAX_TMDSCLK.
> - */
> - if (rate > HDMI14_MAX_TMDSCLK) {
> + if (hdmi->no_hpd && rate > HDMI14_MAX_TMDSCLK) {
> + dev_dbg(hdmi->dev, "Unsupported TMDS char rate in no_hpd mode: %lld\n", rate);
> + return MODE_CLOCK_HIGH;
> + }
> +
> + if (rate > HDMI20_MAX_TMDSRATE) {
> dev_dbg(hdmi->dev, "Unsupported TMDS char rate: %lld\n", rate);
> return MODE_CLOCK_HIGH;
> }
> @@ -1197,7 +1351,7 @@ static const struct drm_bridge_funcs dw_hdmi_qp_bridge_funcs = {
> .atomic_reset = drm_atomic_helper_bridge_reset,
> .atomic_enable = dw_hdmi_qp_bridge_atomic_enable,
> .atomic_disable = dw_hdmi_qp_bridge_atomic_disable,
> - .detect = dw_hdmi_qp_bridge_detect,
> + .detect_ctx = dw_hdmi_qp_bridge_detect_ctx,
> .edid_read = dw_hdmi_qp_bridge_edid_read,
> .hdmi_tmds_char_rate_valid = dw_hdmi_qp_bridge_tmds_char_rate_valid,
> .hdmi_clear_avi_infoframe = dw_hdmi_qp_bridge_clear_avi_infoframe,
> @@ -1287,6 +1441,8 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
> if (IS_ERR(hdmi))
> return ERR_CAST(hdmi);
>
> + INIT_DELAYED_WORK(&hdmi->scramb_work, dw_hdmi_qp_scramb_work);
> +
> hdmi->dev = dev;
>
> regs = devm_platform_ioremap_resource(pdev, 0);
>
> --
> 2.53.0
>

--
With best wishes
Dmitry