Re: [PATCH v4 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 2 potential
issues to consider:
- [Medium] drm/vc4: hdmi: stale scrambler_enabled in
  vc4_hdmi_connector_init() resets CRTC
- [Medium] drm/vc4: hdmi: boot disable in
  vc4_hdmi_encoder_post_crtc_powerdown() skips SCDC
--

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

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

This commit converts the vc4 HDMI driver from its local scrambling and SCDC
handling to the common DRM HDMI 2.0 scrambling helpers.

> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index c3636ed60d56a..d7ef47dc4b02e 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c

[ ... ]

> @@ -544,6 +419,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]
As reported by Maíra Canal on v3 [1], can connector->hdmi.scrambler_enabled
remain stuck at true and cause a spurious CRTC reset on hotplug or connector
polling when the CRTC is inactive at boot?

During bind, vc4_crtc_disable_at_boot() returns early without calling
vc4_crtc_disable() -> vc4_hdmi_encoder_post_crtc_powerdown() ->
drm_connector_hdmi_disable_scrambling() if the PixelValve is not enabled:

drivers/gpu/drm/vc4/vc4_crtc.c:vc4_crtc_disable_at_boot() {
	if (!(CRTC_READ(PV_CONTROL) & PV_CONTROL_EN))
		return 0;

	if (!(CRTC_READ(PV_V_CONTROL) & PV_VCONTROL_VIDEN))
		return 0;

When the CRTC is later enabled in a mode that does not need scrambling,
drm_connector_hdmi_enable_scrambling() returns early on
!conn_state->hdmi.scrambler_needed without clearing scrambler_enabled.
Whereas vc4_hdmi_reset_link() previously checked
drm_hdmi_mode_needs_scrambling(), drm_atomic_helper_connector_hdmi_hotplug()
-> drm_connector_hdmi_sync_scdc() checks hdmi->scrambler_enabled, sees it mismatch
the sink's SCDC_SCRAMBLING_ENABLE bit, and triggers an unnecessary full
modeset via drm_atomic_helper_reset_crtc().

[ ... ]

> @@ -931,7 +766,7 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder,
>  	if (vc4_hdmi->variant->phy_disable)
>  		vc4_hdmi->variant->phy_disable(vc4_hdmi);
>  
> -	vc4_hdmi_disable_scrambling(encoder);
> +	drm_connector_hdmi_disable_scrambling(&vc4_hdmi->connector);

[Severity: Medium]
As reported by Maíra Canal on v3 [2], does replacing
vc4_hdmi_disable_scrambling() with drm_connector_hdmi_disable_scrambling()
skip clearing sink-side SCDC scrambling during vc4_crtc_disable_at_boot()?

vc4_drm_bind() runs vc4_crtc_disable_at_boot() before drm_dev_register() and
before connector detection or EDID probing has populated connector->status
and connector->display_info.  Unlike vc4_hdmi_disable_scrambling(), which
unconditionally cleared SCDC over DDC, drm_connector_hdmi_disable_scrambling()
guards the SCDC writes:

drivers/gpu/drm/display/drm_hdmi_helper.c:drm_connector_hdmi_disable_scrambling() {
	...
	if (connector->ddc &&
	    connector->status == connector_status_connected &&
	    drm_scdc_sink_supports_scrambling(connector)) {
		drm_scdc_set_scrambling(connector, false);
		drm_scdc_set_high_tmds_clock_ratio(connector, false);
	}

If firmware left an HDMI 2.0 pipeline active in a scrambled mode at boot and
the subsequent modeset selects an unscrambled mode, the controller disables
scrambling while the sink remains configured for scrambled 1:40 TMDS, leaving
the display output broken.

[1] https://lore.kernel.org/r/45ea1610-360f-4f6b-9ea6-30b28fbe8514@igalia.com
[2] https://lore.kernel.org/r/45ea1610-360f-4f6b-9ea6-30b28fbe8514@igalia.com

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