Re: [PATCH v3 6/9] drm/vc4: hdmi: Convert to common HDMI 2.0 scrambling infrastructure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/vc4: spurious CRTC reset on hotplug due to stale scrambler_enabled flag
--

commit b54514a951277541ceb5327bc2186f46057b3bd2
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

drm/vc4: hdmi: Convert to common HDMI 2.0 scrambling infrastructure

This commit replaces the local VC4 scrambling logic with the common HDMI 2.0
scrambling support in the DRM core. It delegates sink-side SCDC programming
and monitoring to the core, drops local state tracking for scrambling, and
switches to the core hotplug helpers.

> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index a03e50e8afcb3..2451ff85e7597 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
[ ... ]
> @@ -535,6 +410,14 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
>  
>  	drm_connector_helper_add(connector, &vc4_hdmi_connector_helper_funcs);
>  
> +	/*
> +	 * Since we don't know the state of the controller and its
> +	 * display (if any), let's assume it's always enabled.
> +	 * drm_connector_hdmi_disable_scrambling() will thus run at boot,
> +	 * make sure it's disabled, and avoid any inconsistency.
> +	 */
> +	connector->hdmi.scrambler_enabled = drm_connector_hdmi_scrambler_supported(connector);
> +

[Severity: Medium]
Will this lead to spurious CRTC resets on hotplug events or EDID polls?

If the display starts disconnected, its initial atomic state has crtc set to
NULL. When a display is plugged in and a mode not requiring scrambling is set
(e.g. 1080p), the encoder disable hook is skipped by 
drm_atomic_helper_commit_modeset_disables() because old_conn_state->crtc is
NULL. 

Then drm_connector_hdmi_enable_scrambling() in the core returns early without
clearing the scrambler_enabled flag:

drivers/gpu/drm/display/drm_hdmi_helper.c:drm_connector_hdmi_enable_scrambling() {
	...
	if (!conn_state->hdmi.scrambler_needed)
		return 0;
	...
}

The flag remains true.

Subsequently, when drm_kms_helper_poll_worker() runs or an HPD event occurs,
drm_connector_hdmi_sync_scdc() sees the stale true flag:

drivers/gpu/drm/display/drm_hdmi_helper.c:drm_connector_hdmi_sync_scdc() {
	...
	if (!drm_connector_hdmi_scrambler_supported(connector) ||
	    !READ_ONCE(hdmi->scrambler_enabled))
		return 0;
	...
}

This reads the SCDC register (where scrambling is actually disabled) and
triggers a spurious drm_atomic_helper_reset_crtc(), causing the display to
flicker.

The previous local logic checked if scrambling was needed before reading SCDC,
but the new framework relies on this flag. How can we ensure the flag is
cleared when scrambling isn't needed on the first modeset?

>  	/*
>  	 * Some of the properties below require access to state, like bpc.
>  	 * Allocate some default initial connector state with our reset helper.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-vc4-common-scramb-v3-0-c68d58d959a2@collabora.com?part=6