Re: [PATCH v10 22/69] drm/display: hdmi-state-helper: Sync SCDC state on hotplug

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

On 8/20/26 11:53 AM, Maxime Ripard wrote:
> On Fri, Jul 31, 2026 at 07:19:29PM +0300, Cristian Ciocaltea wrote:
>> drm_atomic_helper_connector_hdmi_hotplug() does not currently
>> synchronize SCDC status on hotplug events, leaving the scrambler state
>> potentially inconsistent after (re)connect.
>>
>> Hook drm_connector_hdmi_sync_scdc() into both the connect and disconnect
>> paths, replacing the existing TODOs around missing scrambler handling.
>>
>> Tested-by: Maud Spierings <maud_spierings@hotmail.com>
>> Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, Rock 5B
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 23 ++++++++++++++---------
>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> index 4a93c279c9a7..3377ea936120 100644
>> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> @@ -1205,13 +1205,16 @@ drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector,
>>  					enum drm_connector_status status)
>>  {
>>  	const struct drm_edid *drm_edid;
>> +	int ret = 0;
>>  
>>  	if (status == connector_status_disconnected) {
>> -		// TODO: also handle scramber, HDMI sink disconnected.
>> -		drm_connector_hdmi_audio_plugged_notify(connector, false);
>> -		drm_edid_connector_update(connector, NULL);
>> -		drm_connector_cec_phys_addr_invalidate(connector);
>> -		return 0;
>> +		ret = drm_connector_hdmi_sync_scdc(connector, false, ctx);
>> +		if (ret != -EDEADLK) {
>> +			drm_connector_hdmi_audio_plugged_notify(connector, false);
>> +			drm_edid_connector_update(connector, NULL);
>> +			drm_connector_cec_phys_addr_invalidate(connector);
>> +		}
> 
> If there's a deadlock, shouldn't we restart the whole sequence there?

In that case we do already propagate -EDEADLK and let the callers
(drm_helper_probe_detect_ctx(), drm_helper_probe_single_connector_modes())
to ensure the sequence is restarted.

> Ie, we should return ret all the time anyway? And if we do that, we
> should return ret for drm_edid_connector_update() too.

Per .detect_ctx() contract, implementations shall return a drm_connector_status
value or -EDEADLK only.  On the other hand, .force_ctx() accepts any error code,
but the probe helpers just log it.  Hence returning anything else wouldn't
really have an impact on the functionality.

Returning errors from drm_edid_connector_update() would potentially override
non-deadlock ones from sync_scdc().  Since both helpers already log their own
failures, I think it isn't worth the trouble.

> Either way, a comment on why we're doing it this way would be nice.

Indeed.  Would the following be too verbose?

/*
 * The SCDC resync may reset the CRTC, which might involve aquiring
 * modeset locks.  If that fails, -EDEADLK is reported and the callers
 * passing a non-NULL @ctx drop the locks and restart the sequence
 * - see drm_helper_probe_detect_ctx() and
 * drm_helper_probe_single_connector_modes().
 *
 * The resync runs first, and the audio and CEC helpers only once the
 * link state has settled: the CRTC reset is a blocking commit, so on
 * success the pipeline is already up again, while on -EDEADLK nothing
 * has been resynced yet and the pending retry redoes everything.  This
 * keeps userspace from acting upon a link that is about to be reset.
 *
 * -EDEADLK is the only status gating the helpers below, as it is the
 * sole one guaranteeing a new run.  The other failures are merely
 * reported: .force_ctx() accepts any error code and the probe helpers
 * just log it, while .detect_ctx() has to swallow it, being only
 * allowed to return a drm_connector_status value or -EDEADLK.
 * Propagating the status of drm_edid_connector_update() on top would
 * therefore only make it compete with an earlier resync failure over a
 * value that triggers no recovery, the more so as both helpers already
 * log their own errors.
 */

Thanks,
Cristian