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

Cristian Ciocaltea
2026-04-28T13:51:49Z

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