Messages in this thread |  | | Date | Tue, 28 Apr 2026 04:38:54 +0300 | | From | Dmitry Baryshkov <> | | Subject | Re: [PATCH v5 05/10] drm/bridge: dw-hdmi-qp: Add HDMI 2.0 SCDC scrambling and high TMDS clock ratio support |
| |
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
|  |