Re: [PATCH v8 11/39] drm/display: hdmi-state-helper: Set HDMI scrambling requirement

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing validation of SCDC scrambling capabilities in HDMI atomic check, leading to atomic commit failure.
- [Low] Kernel doc for `scrambler_needed` incorrectly claims it considers capabilities.
--

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

drm/display: hdmi-state-helper: Set HDMI scrambling requirement
    
This commit sets the drm_connector_hdmi_state.scrambler_needed flag when
the computed TMDS character rate exceeds the HDMI 1.3 maximum limit.
By centralizing this clock-based requirement in the HDMI state helper, it
removes the need for drivers to open-code the threshold check.

> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 69ccfbf123fe3..34e48e04d9712 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -592,6 +592,7 @@ hdmi_compute_clock(const struct drm_connector *connector,
>  		return -EINVAL;
>  
>  	conn_state->hdmi.tmds_char_rate = clock;
> +	conn_state->hdmi.scrambler_needed = (clock > HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ);

[Severity: High]
Does this code fail to validate SCDC scrambling capabilities during atomic
check?

Setting scrambler_needed purely based on the clock rate, without validating if
scrambling is actually supported by the sink and source, allows invalid atomic
states to proceed.

If an HDMI sink provides a buggy EDID reporting >= 340 MHz but without SCDC
scrambling support (or the source lacks scrambling support), a mode requiring
>340 MHz TMDS rate could be requested.  hdmi_clock_valid() passes because
info->max_tmds_clock doesn't reject it.

Because hdmi_compute_clock() unconditionally sets scrambler_needed = true
without checking if scrambling is actually supported by both source and sink,
the atomic_check succeeds.

Then, atomic_commit calls drm_connector_hdmi_enable_scrambling(), which checks
capabilities, finds them lacking, and returns -EINVAL.

Returning an error during the atomic commit phase violates the DRM atomic KMS
contract.  This leads to a state where the hardware might be partially
programmed and the driver aborts the commit, which can cause black screens,
driver state desync, or kernel warnings.

[Severity: Low]
Does this change make the kernel documentation for drm_connector_hdmi_state
inaccurate?

In include/drm/drm_connector.h, the kernel doc for scrambler_needed states
it is computed from the capabilities:

include/drm/drm_connector.h:drm_connector_hdmi_state
	/**
	 * @scrambler_needed: Whether HDMI 2.0 SCDC scrambling is required
	 * for the negotiated mode/bpc/format.
	 *
	 * Computed by drm_atomic_helper_connector_hdmi_check() from
	 * @tmds_char_rate and the source/sink scrambling capabilities.

But the implementation now only checks the clock against
HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ, creating a documentation mismatch.

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-dw-hdmi-qp-scramb-v8-0-d79890d00b6a@collabora.com?part=11