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

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(-)
  

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] SCDC sync fails to handle scrambler teardown on disconnect, leaving the source scrambler active when swapping to a non-scrambling sink.
--

--- Patch [22]: [PATCH v10 22/69] drm/display: hdmi-state-helper: Sync SCDC state on hotplug ---
commit 7cc2ce705b4793d8696717d86562fbd6c0bde231
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

drm/display: hdmi-state-helper: Sync SCDC state on hotplug

This commit hooks drm_connector_hdmi_sync_scdc() into the connect and
disconnect paths in drm_atomic_helper_connector_hdmi_update() to synchronize
the SCDC status on hotplug events.

> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 4a93c279c9a76..3377ea9361209 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);

[Severity: High]
Does this call successfully tear down the scrambler on disconnect?

Looking at drm_connector_hdmi_sync_scdc() in drm_hdmi_helper.c, it seems
to return immediately if the plugged parameter is false:

int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
                                 struct drm_modeset_acquire_ctx *ctx)
{
        ...
        if (!connector || !plugged || !ctx)
                return 0;

This would make the disconnect call a no-op, failing to disable the source
scrambler.

Additionally, if a non-scrambling sink is subsequently plugged in, the
connect path will call drm_connector_hdmi_sync_scdc() with plugged set to
true, which also bails out early:

        if (!connector->ddc || !drm_scdc_sink_supports_scrambling(connector))
                return 0;

Could this leave the source scrambler incorrectly enabled if a user swaps from
an HDMI 2.0 monitor to an older HDMI 1.4 monitor, resulting in broken display
output due to mismatched hardware state?

> +		if (ret != -EDEADLK) {
> +			drm_connector_hdmi_audio_plugged_notify(connector, false);
> +			drm_edid_connector_update(connector, NULL);
> +			drm_connector_cec_phys_addr_invalidate(connector);
> +		}
> +		return ret;
>  	}

[ ... ]
  

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?
Ie, we should return ret all the time anyway? And if we do that, we
should return ret for drm_edid_connector_update() too.

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

Maxime
  

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