Re: [PATCH v7 04/30] drm/display: scdc_helper: Add HDMI 2.0 scrambling management helpers

[Maxime Ripard]

On Sat, Jun 13, 2026 at 04:30:02AM +0300, Cristian Ciocaltea wrote:
> On 6/12/26 4:43 PM, Maxime Ripard wrote:
> > On Tue, Jun 02, 2026 at 01:44:04AM +0300, Cristian Ciocaltea wrote:
> >> +/**
> >> + * drm_scdc_start_scrambling - activate scrambling and monitor SCDC status
> >> + * @connector: connector
> >> + *
> >> + * Enables scrambling and high TMDS clock ratio on both source and sink sides.
> >> + * Additionally, use a delayed work item to monitor the scrambling status on
> >> + * the sink side and retry the operation, as some displays refuse to set the
> >> + * scrambling bit right away.
> >> + *
> >> + * Returns:
> >> + * Zero if scrambling is set successfully, an error code otherwise.
> >> + */
> >> +int drm_scdc_start_scrambling(struct drm_connector *connector)
> >> +{
> >> +	struct drm_display_info *info = &connector->display_info;
> >> +	struct drm_connector_hdmi *hdmi = &connector->hdmi;
> >> +	int ret;
> >> +	u8 ver;
> >> +
> >> +	if (!hdmi->scrambler_supported) {
> >> +		drm_scdc_dbg(connector, "Scrambler not supported, bailing.\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	if (!info->is_hdmi ||
> >> +	    !info->hdmi.scdc.supported ||
> >> +	    !info->hdmi.scdc.scrambling.supported) {
> >> +		drm_scdc_dbg(connector, "Sink doesn't support scrambling.\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	drm_scdc_dbg(connector, "Enabling scrambling\n");
> >> +
> >> +	ret = drm_scdc_readb(connector->ddc, SCDC_SINK_VERSION, &ver);
> >> +	if (ret) {
> >> +		drm_scdc_dbg(connector, "Failed to read SCDC_SINK_VERSION: %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	ret = drm_scdc_writeb(connector->ddc, SCDC_SOURCE_VERSION,
> >> +			      min_t(u8, ver, SCDC_MAX_SOURCE_VERSION));
> >> +	if (ret) {
> >> +		drm_scdc_dbg(connector, "Failed to write SCDC_SOURCE_VERSION: %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	hdmi->scdc_cb = drm_scdc_monitor_scrambler;
> >> +	WRITE_ONCE(hdmi->scrambler_enabled, true);
> >> +
> >> +	ret = drm_scdc_try_scrambling_setup(connector);
> >> +	if (!ret)
> >> +		ret = hdmi->funcs->scrambler_enable(connector);
> >> +
> >> +	if (ret) {
> >> +		WRITE_ONCE(hdmi->scrambler_enabled, false);
> >> +		cancel_delayed_work_sync(&hdmi->scdc_work);
> >> +		hdmi->scdc_cb = NULL;
> >> +
> >> +		drm_scdc_set_scrambling(connector, false);
> >> +		drm_scdc_set_high_tmds_clock_ratio(connector, false);
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> >> +EXPORT_SYMBOL(drm_scdc_start_scrambling);
> > 
> > This is also pretty significantly different than what vc4 implemented. I
> > don't necessarily mind, but claiming that it's functionally equivalent
> > isn't true. I think it would be a much better strategy to turn the vc4
> > code into helpers as is because it's been merged 4 years ago and we know
> > it's solid.
> 
> As explained in patch 25, I think there's been a slight misunderstanding of how
> the CRTC reset logic was actually implemented.

You're absolutely right, let me review it again. sorry.

Maxime

Attachment: signature.asc
Description: PGP signature