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

[Cristian Ciocaltea <cristian.ciocaltea@collabora.com>]

Hi Dmitry,

Thanks for taking the time to review the series!

On 4/28/26 4:38 AM, Dmitry Baryshkov wrote:
> 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

Sure, will do.

>> +
>>  	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.

I'll factor these out.

>> +
>>  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.

Ack.

>> +
>> +	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.

For some reason, without it, the connector seems to lose its state after reset:

[ 2142.982967] dwhdmiqp-rockchip fde80000.hdmi: resetting crtc
[ 2142.994132] rockchip-drm display-subsystem: [drm] HDMI Sink doesn't support RGB, something's wrong.
[ 2143.140827] dwhdmiqp-rockchip fde80000.hdmi: dw_hdmi_qp_bridge_atomic_enable mode=DVI

Will recheck if that's still the case after the HPD handling rework.

> 
>> +	/*
>> +	 * 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.

Ok, I'll try that.

Regards,
Cristian

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip