Re: [PATCH v11 10/74] drm/display: hdmi: Add HDMI 2.0 scrambling management helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] HDMI scrambling lifecycle helpers introduce sleeping functions into atomic display paths.
- [Medium] Logic error in SCDC scrambling status monitor: the delayed work item stops scheduling itself, violating the intended periodic monitoring.
--

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

drm/display: hdmi: Add HDMI 2.0 scrambling management helpers

This commit introduces helpers to manage the full lifecycle of HDMI 2.0
SCDC scrambling on both source and sink sides. It includes configuration
of scrambling, high TMDS clock ratios, and a delayed work item to
monitor and restore the sink's scrambling status.

> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index 5cb0b033b1713..bf1c7963b984b 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
[ ... ]
> +static int drm_connector_hdmi_try_scrambling_setup(struct drm_connector *connector)
> +{
> +	bool done;
> +
> +	done = drm_scdc_set_high_tmds_clock_ratio(connector, true);
> +	if (!done)
> +		return -EIO;

[Severity: High]
Since drm_connector_hdmi_enable_scrambling() might be called from atomic
commit paths (like CRTC atomic enable), is it safe to call
drm_scdc_set_high_tmds_clock_ratio() here?

Looking at drm_scdc_set_high_tmds_clock_ratio() in drm_scdc_helper.c, it
uses a sleeping wait:

    usleep_range(1000, 2000);

Could this lead to sleeping in atomic context warnings or deadlocks if
reached during an atomic display update?

[Severity: Medium]
If drm_scdc_set_high_tmds_clock_ratio() or drm_scdc_set_scrambling() fail due
to a transient DDC error, we return -EIO early here and bypass the
schedule_delayed_work() call below.

Does this inadvertently stop the periodic monitoring from retrying the setup
later?

> +
> +	done = drm_scdc_set_scrambling(connector, true);
> +	if (!done)
> +		return -EIO;
> +
> +	if (READ_ONCE(connector->hdmi.scrambler_enabled))
> +		schedule_delayed_work(&connector->hdmi.scdc_work,
> +				      msecs_to_jiffies(DRM_HDMI_SCDC_POLL_DELAY_MS));
> +
> +	return 0;
> +}
> +
> +static void drm_connector_hdmi_scdc_work(struct work_struct *work)
> +{
[ ... ]
> +	if (READ_ONCE(connector->hdmi.scrambler_enabled) &&
> +	    !drm_scdc_get_scrambling_status(connector))
> +		drm_connector_hdmi_try_scrambling_setup(connector);
> +}

[Severity: Medium]
If the sink scrambling status is healthy (drm_scdc_get_scrambling_status()
returns true), this function exits without rescheduling itself.

Does this mean the periodic work item stops monitoring the SCDC status
entirely after the first successful check? The commit message mentions a
periodic work item that monitors the status and retries when lost, but it
appears the loop breaks as soon as the sink is in a good state.

[ ... ]
> +int drm_connector_hdmi_disable_scrambling(struct drm_connector *connector)
> +{
[ ... ]
> +	/*
> +	 * A driver may force scrambler_enabled at init to trigger a disable
> +	 * at boot (e.g. vc4), bypassing the enable path that sets up the work
> +	 * item. Only cancel it once it has actually been initialized.
> +	 */
> +	if (hdmi->scdc_work_initialized)
> +		cancel_delayed_work_sync(&hdmi->scdc_work);

[Severity: High]
If drm_connector_hdmi_disable_scrambling() is invoked from an atomic
context (e.g., during CRTC or encoder atomic disable callbacks), could
calling cancel_delayed_work_sync() cause a sleep in atomic context?

Since cancel_delayed_work_sync() can block waiting for the work to finish,
this might cause system instability if called while holding spinlocks or
from a non-preemptible path.

> +
> +	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);
> +	}
> +
> +	return hdmi->funcs->scrambler_disable(connector);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-dw-hdmi-qp-scramb-v11-0-bc12954a0688@collabora.com?part=10