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

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

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