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

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

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.

> Then, if you want to add additional checks and constraints (like the
> SCDC_SINK_VERSION) that's fine, but it should come on top, and with its
> own explanation.

I'll recheck if there's anything that I might have missed from the original
implementation, while moving the additional changes to separate patch(es).

Thanks,
Cristian