Re: [PATCH v9 08/61] drm/display: hdmi: Add HDMI 2.0 scrambling management helpers

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] SCDC scrambling monitor work item stops polling permanently after first success or I2C failure.
- [High] Incorrect pending commit check in drm_connector_hdmi_sync_scdc() bypasses synchronization and allows concurrent hardware programming.
- [High] Sleeping functions called from atomic context in SCDC scrambling lifecycle helpers.
- [Medium] The drm_connector_hdmi_sync_scdc() function is dead code and breaks the automatic SCDC sync feature.
--

--- Patch [8]: [PATCH v9 08/61] drm/display: hdmi: Add HDMI 2.0 scrambling management helpers ---
commit ef23d36fe9814f6ecd453af5cfc123e83ec840f5
Author: Cristian Ciocaltea <[email protected]>

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

Add helpers to manage the full lifecycle of HDMI 2.0 SCDC scrambling on
both source and sink sides.

> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index 5cb0b033b1713..b9be0a2e833ad 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);

[Severity: High]
Does this code sleep in atomic context? drm_scdc_set_high_tmds_clock_ratio()
internally calls usleep_range(1000, 2000). If this is called from atomic
modesetting callbacks (like encoder enable), it will violate the atomic
context constraint.

> +	if (!done)
> +		return -EIO;

[Severity: High]
If an I2C error occurs here, the function returns early without rescheduling
the work. Will this cause the polling to stop permanently upon a transient
failure?

> +
> +	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)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_hdmi *hdmi;
> +
> +	hdmi = container_of(to_delayed_work(work), struct drm_connector_hdmi,
> +			    scdc_work);
> +	connector = container_of(hdmi, struct drm_connector, hdmi);
> +
> +	if (READ_ONCE(connector->hdmi.scrambler_enabled) &&
> +	    !drm_scdc_get_scrambling_status(connector))
> +		drm_connector_hdmi_try_scrambling_setup(connector);

[Severity: High]
If the SCDC status is correct (!drm_scdc_get_scrambling_status() is false),
the delayed work item is not rescheduled. Does this mean the periodic monitor
will only run once and then stop tracking the SCDC status permanently?

> +}
[ ... ]
> +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]
Could this cause a sleep in atomic context? cancel_delayed_work_sync() is a
blocking function. If this disable path is executed during an atomic commit,
it will violate atomic context constraints.

[ ... ]
> + * The typical solution for this is to trigger an empty modeset in
> + * drm_connector_helper_funcs.detect_ctx(), which is what this helper does
> + * by triggering a CRTC reset on reconnection.
> + *
> + * When making use of the HDMI connector framework, this is automatically
> + * triggered via drm_atomic_helper_connector_hdmi_hotplug().
> + *
> + * Returns:
> + * Zero on success, an error code otherwise, including -EDEADLK.
> + */
> +int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
> +				 struct drm_modeset_acquire_ctx *ctx)
> +{

[Severity: Medium]
Is this function actually hooked up? It appears to be dead code that is never
called from drm_atomic_helper_connector_hdmi_hotplug() as claimed in the
commit message and comments, which would mean the automatic SCDC sync feature
is not functional.

> +	struct drm_connector_hdmi *hdmi = &connector->hdmi;
[ ... ]
> +	conn_state = connector->state;
> +	if (!conn_state)
> +		return 0;
> +
> +	crtc = connector->state->crtc;
> +	if (!crtc)
> +		return 0;
[ ... ]
> +	if (conn_state->commit &&
> +	    !try_wait_for_completion(&conn_state->commit->hw_done))
> +		return 0;

[Severity: High]
Does this check the wrong commit pointer? According to the DRM core
documentation, conn_state->commit is only set when crtc is NULL. However,
earlier in this function, it explicitly returns if crtc is NULL.

This means conn_state->commit is guaranteed to be NULL here, which bypasses
synchronization and could allow concurrent hardware programming while a
non-blocking commit is active. Should this be checking crtc->state->commit
instead?

> +
> +	ret = drm_scdc_readb(connector->ddc, SCDC_TMDS_CONFIG, &config);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8