Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The SCDC scrambling monitor work item fails to reschedule itself, terminating the periodic polling after a single successful check or transient error.
- [High] Missing NULL check for connector->ddc in drm_connector_hdmi_disable_scrambling() causes a crash when scrambling is disabled for a connector without a DDC bus.
- [High] TOCTOU race condition in drm_connector_hdmi_scdc_work leading to a potential Use-After-Free of the I2C adapter.
- [High] Dead code in drm_connector_hdmi_sync_scdc fails to synchronize with hardware programming due to an impossible condition.
--
--- Patch [10]: [PATCH v10 10/69] drm/display: hdmi: Add HDMI 2.0 scrambling management helpers ---
commit 3991d28f3f40213e274370d230b6145165dadbee
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
drm/display: hdmi: Add HDMI 2.0 scrambling management helpers
This commit adds helpers to manage the full lifecycle of HDMI 2.0 SCDC
scrambling on both source and sink sides. It introduces checking if a mode
needs scrambling, enabling/disabling with monitoring, and restoring the
SCDC state on reconnection.
> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index 5cb0b033b1713..cfecfef639ad0 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
[ ... ]
> @@ -426,3 +434,291 @@ drm_hdmi_acr_get_n_cts(unsigned long long tmds_char_rate,
> *out_cts = cts;
> }
> EXPORT_SYMBOL(drm_hdmi_acr_get_n_cts);
[ ... ]
> +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]
Does this prevent the SCDC worker from retrying on transient errors?
If drm_scdc_set_high_tmds_clock_ratio() fails, the function returns -EIO
early and skips the schedule_delayed_work() call, preventing any further
monitoring of the scrambling state.
[ ... ]
> +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);
> +
> + /*
> + * The connector, and hence this work item, may outlive the source
> + * driver, e.g. when userspace keeps a DRM file descriptor open past
> + * unbind. Never poke the DDC bus after the connector got unregistered,
> + * as the underlying I2C adapter may already be gone.
> + */
> + if (drm_connector_is_unregistered(connector))
> + return;
[Severity: High]
Could this lockless check lead to a Use-After-Free of the I2C adapter?
If the worker thread is preempted immediately after this check returns false,
the driver unbind process could complete and free the I2C adapter. When the
worker resumes, it calls drm_scdc_get_scrambling_status() which accesses the
now-freed connector->ddc.
> +
> + if (READ_ONCE(connector->hdmi.scrambler_enabled) &&
> + !drm_scdc_get_scrambling_status(connector))
> + drm_connector_hdmi_try_scrambling_setup(connector);
[Severity: High]
Will this work item fail to reschedule itself if the SCDC status is correct?
If drm_scdc_get_scrambling_status() returns true (meaning scrambling is
working), this function simply returns without rescheduling the worker. The
rescheduling only happens inside drm_connector_hdmi_try_scrambling_setup()
on a successful setup, so the periodic monitoring stops after the first
successful check.
> +}
[ ... ]
> +int drm_connector_hdmi_disable_scrambling(struct drm_connector *connector)
> +{
> + struct drm_connector_hdmi *hdmi = &connector->hdmi;
> + struct drm_device *dev = connector->dev;
> +
> + if (!READ_ONCE(hdmi->scrambler_enabled))
> + return 0;
> +
> + drm_dbg_kms(dev, "Disabling scrambling\n");
> +
> + WRITE_ONCE(hdmi->scrambler_enabled, false);
> +
> + /*
> + * 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);
> +
> + if (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);
[Severity: High]
Can this cause a NULL pointer dereference if connector->ddc is missing?
If a driver forces hdmi->scrambler_enabled to true at init to trigger a
disable at boot, and the connector has no DDC bus (connector->ddc is NULL),
drm_scdc_set_scrambling() will unconditionally dereference the NULL DDC
adapter. The enable path correctly checks for connector->ddc, but the disable
path does not.
> + }
> +
> + return hdmi->funcs->scrambler_disable(connector);
> +}
[ ... ]
> +int drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
> + struct drm_modeset_acquire_ctx *ctx)
> +{
> + struct drm_connector_hdmi *hdmi = &connector->hdmi;
> + struct drm_device *dev = connector->dev;
> + struct drm_connector_state *conn_state;
> + struct drm_crtc *crtc;
> + u8 config;
> + int ret;
> +
> + if (!connector || !plugged || !ctx)
> + return 0;
[ ... ]
> + crtc = conn_state->crtc;
> + if (!crtc)
> + return 0;
> +
> + ret = drm_modeset_lock(&crtc->mutex, ctx);
> + if (ret)
> + return ret;
> +
> + if (!crtc->state || !crtc->state->active)
> + return 0;
> +
> + if (!connector->ddc || !drm_scdc_sink_supports_scrambling(connector))
> + return 0;
> +
> + if (conn_state->commit &&
> + !try_wait_for_completion(&conn_state->commit->hw_done))
[Severity: High]
Is this wait condition dead code?
Earlier in the function, it verifies that crtc is not NULL (if (!crtc)
return 0;). According to the struct drm_connector_state documentation, the
commit field is only set when crtc is NULL. Since crtc is guaranteed to be
non-NULL here, conn_state->commit will always be NULL, making this check
impossible to hit. Consequently, the function proceeds to perform SCDC I2C
reads concurrently with active hardware programming.
> + return 0;
> +
> + ret = drm_scdc_readb(connector->ddc, SCDC_TMDS_CONFIG, &config);
@@ -4,12 +4,20 @@
#include <linux/module.h>
#include <drm/display/drm_hdmi_helper.h>
+#include <drm/display/drm_scdc_helper.h>
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
#include <drm/drm_edid.h>
#include <drm/drm_modes.h>
+#include <drm/drm_modeset_lock.h>
#include <drm/drm_print.h>
#include <drm/drm_property.h>
+#define DRM_HDMI_SCDC_POLL_DELAY_MS 1000
+
static inline bool is_eotf_supported(u8 output_eotf, u8 sink_eotf)
{
return sink_eotf & BIT(output_eotf);
@@ -426,3 +434,291 @@ drm_hdmi_acr_get_n_cts(unsigned long long tmds_char_rate,
*out_cts = cts;
}
EXPORT_SYMBOL(drm_hdmi_acr_get_n_cts);
+
+/**
+ * drm_hdmi_mode_needs_scrambling() - Check if an HDMI mode requires scrambling
+ * @mode: Display mode to check
+ * @bpc: Bits per color component
+ * @fmt: Output pixel format
+ *
+ * Computes the TMDS character rate for the given mode, bits per component and
+ * output format, and checks whether it exceeds the HDMI 1.3 maximum TMDS
+ * character rate of 340 MHz.
+ *
+ * Returns:
+ * true if the mode requires scrambling, false otherwise.
+ */
+bool drm_hdmi_mode_needs_scrambling(const struct drm_display_mode *mode,
+ unsigned int bpc,
+ enum drm_output_color_format fmt)
+{
+ unsigned long long tmds_char_rate;
+
+ tmds_char_rate = drm_hdmi_compute_mode_clock(mode, bpc, fmt);
+
+ return tmds_char_rate > HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
+}
+EXPORT_SYMBOL(drm_hdmi_mode_needs_scrambling);
+
+static bool drm_scdc_sink_supports_scrambling(struct drm_connector *connector)
+{
+ const struct drm_display_info *info = &connector->display_info;
+
+ return info->is_hdmi &&
+ info->hdmi.scdc.supported &&
+ info->hdmi.scdc.scrambling.supported;
+}
+
+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;
+
+ 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);
+
+ /*
+ * The connector, and hence this work item, may outlive the source
+ * driver, e.g. when userspace keeps a DRM file descriptor open past
+ * unbind. Never poke the DDC bus after the connector got unregistered,
+ * as the underlying I2C adapter may already be gone.
+ */
+ if (drm_connector_is_unregistered(connector))
+ return;
+
+ if (READ_ONCE(connector->hdmi.scrambler_enabled) &&
+ !drm_scdc_get_scrambling_status(connector))
+ drm_connector_hdmi_try_scrambling_setup(connector);
+}
+
+/**
+ * drm_connector_hdmi_enable_scrambling() - enable scrambling and monitor SCDC status
+ * @connector: connector
+ * @conn_state: connector state
+ *
+ * Enables scrambling and high TMDS clock ratio on both source and sink sides.
+ * Additionally, use a delayed work item to monitor the scrambling status on
+ * the sink side and retry the operation, as some displays refuse to set the
+ * scrambling bit right away.
+ *
+ * The work item is cancelled by drm_connector_hdmi_disable_scrambling(), hence
+ * source drivers must ensure the display pipeline is shut down on unbind, e.g.
+ * via drm_atomic_helper_shutdown(). Otherwise the work may outlive the driver's
+ * devres-managed resources, since the connector itself is only cleaned up when
+ * the last reference to the DRM device is dropped.
+ *
+ * Returns:
+ * Zero if scrambling is set successfully, an error code otherwise.
+ */
+int drm_connector_hdmi_enable_scrambling(struct drm_connector *connector,
+ const struct drm_connector_state *conn_state)
+{
+ struct drm_connector_hdmi *hdmi = &connector->hdmi;
+ struct drm_device *dev = connector->dev;
+ int ret;
+
+ if (!conn_state)
+ return -EINVAL;
+
+ if (!conn_state->hdmi.scrambler_needed)
+ return 0;
+
+ if (!drm_connector_hdmi_scrambler_supported(connector)) {
+ drm_dbg_kms(dev, "Source doesn't support scrambling.\n");
+ return -EINVAL;
+ }
+
+ if (!drm_scdc_sink_supports_scrambling(connector)) {
+ drm_dbg_kms(dev, "Sink doesn't support scrambling.\n");
+ return -EINVAL;
+ }
+
+ if (!connector->ddc)
+ return -EINVAL;
+
+ drm_dbg_kms(dev, "Enabling scrambling\n");
+
+ if (!hdmi->scdc_work_initialized) {
+ INIT_DELAYED_WORK(&hdmi->scdc_work,
+ drm_connector_hdmi_scdc_work);
+ hdmi->scdc_work_initialized = true;
+ }
+
+ WRITE_ONCE(hdmi->scrambler_enabled, true);
+
+ ret = drm_connector_hdmi_try_scrambling_setup(connector);
+ if (!ret)
+ ret = hdmi->funcs->scrambler_enable(connector);
+
+ if (ret) {
+ WRITE_ONCE(hdmi->scrambler_enabled, false);
+ cancel_delayed_work_sync(&hdmi->scdc_work);
+
+ drm_scdc_set_scrambling(connector, false);
+ drm_scdc_set_high_tmds_clock_ratio(connector, false);
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL(drm_connector_hdmi_enable_scrambling);
+
+/**
+ * drm_connector_hdmi_disable_scrambling() - disable scrambling and SCDC monitoring
+ * @connector: connector
+ *
+ * Disables scrambling and high TMDS clock ratio on both source and sink sides.
+ * Also cancels the SCDC status monitoring work item, if it is still pending.
+ *
+ * Returns:
+ * Zero if scrambling is reset successfully, an error code otherwise.
+ */
+int drm_connector_hdmi_disable_scrambling(struct drm_connector *connector)
+{
+ struct drm_connector_hdmi *hdmi = &connector->hdmi;
+ struct drm_device *dev = connector->dev;
+
+ if (!READ_ONCE(hdmi->scrambler_enabled))
+ return 0;
+
+ drm_dbg_kms(dev, "Disabling scrambling\n");
+
+ WRITE_ONCE(hdmi->scrambler_enabled, false);
+
+ /*
+ * 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);
+
+ if (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);
+}
+EXPORT_SYMBOL(drm_connector_hdmi_disable_scrambling);
+
+/**
+ * drm_connector_hdmi_sync_scdc() - resync the sink-side SCDC upon reconnect
+ * @connector: connector
+ * @plugged: connector plugged status event
+ * @ctx: initialized lock acquisition context
+ *
+ * When receiving hotplug disconnect/reconnect event, while the display is
+ * still active (CRTC enabled), the SCDC status on the sink side is reset
+ * and must be explicitly restored.
+ *
+ * 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)
+{
+ struct drm_connector_hdmi *hdmi = &connector->hdmi;
+ struct drm_device *dev = connector->dev;
+ struct drm_connector_state *conn_state;
+ struct drm_crtc *crtc;
+ u8 config;
+ int ret;
+
+ if (!connector || !plugged || !ctx)
+ return 0;
+
+ /*
+ * This is normally part of .detect_ctx() call path, which already holds
+ * connection_mutex through @ctx. However, re-acquiring it with the
+ * same context is a no-op and makes the helper safe under any caller.
+ */
+ ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex, ctx);
+ if (ret)
+ return ret;
+
+ /* TODO: Also handle HDMI 2.1 FRL link training */
+
+ if (!drm_connector_hdmi_scrambler_supported(connector) ||
+ !READ_ONCE(hdmi->scrambler_enabled))
+ return 0;
+
+ conn_state = connector->state;
+ if (!conn_state)
+ return 0;
+
+ crtc = conn_state->crtc;
+ if (!crtc)
+ return 0;
+
+ ret = drm_modeset_lock(&crtc->mutex, ctx);
+ if (ret)
+ return ret;
+
+ if (!crtc->state || !crtc->state->active)
+ return 0;
+
+ if (!connector->ddc || !drm_scdc_sink_supports_scrambling(connector))
+ return 0;
+
+ if (conn_state->commit &&
+ !try_wait_for_completion(&conn_state->commit->hw_done))
+ return 0;
+
+ ret = drm_scdc_readb(connector->ddc, SCDC_TMDS_CONFIG, &config);
+ if (ret) {
+ drm_dbg_kms(dev, "Failed to read TMDS config: %d\n", ret);
+ return ret;
+ }
+
+ if (config & SCDC_SCRAMBLING_ENABLE)
+ return 0;
+
+ /*
+ * HDMI 2.0 requires that one should not send scrambled data prior
+ * to configuring the sink scrambling, and that TMDS clock/data
+ * transmission should be suspended when changing the TMDS clock rate
+ * in the sink. So let's just do a full modeset here, even though some
+ * sinks would be perfectly happy if were to just reconfigure the SCDC
+ * settings on the fly.
+ *
+ * The disable/re-enable cycle triggered by the reset should call
+ * drm_connector_hdmi_enable_scrambling() during re-enable, thus
+ * properly configuring the sink before data transmission resumes.
+ */
+ drm_dbg_kms(dev, "Resetting CRTC to restore SCDC status\n");
+
+ ret = drm_atomic_helper_reset_crtc(crtc, ctx);
+ if (ret && ret != -EDEADLK)
+ drm_dbg_kms(dev, "Failed to reset CRTC: %d\n", ret);
+
+ return ret;
+}
+EXPORT_SYMBOL(drm_connector_hdmi_sync_scdc);
@@ -8,6 +8,7 @@
struct drm_connector;
struct drm_connector_state;
struct drm_display_mode;
+struct drm_modeset_acquire_ctx;
enum drm_output_color_format;
void
@@ -35,4 +36,18 @@ drm_hdmi_acr_get_n_cts(unsigned long long tmds_char_rate,
unsigned int *out_n,
unsigned int *out_cts);
+bool
+drm_hdmi_mode_needs_scrambling(const struct drm_display_mode *mode,
+ unsigned int bpc, enum drm_output_color_format fmt);
+
+int
+drm_connector_hdmi_enable_scrambling(struct drm_connector *connector,
+ const struct drm_connector_state *conn_state);
+int
+drm_connector_hdmi_disable_scrambling(struct drm_connector *connector);
+
+int
+drm_connector_hdmi_sync_scdc(struct drm_connector *connector, bool plugged,
+ struct drm_modeset_acquire_ctx *ctx);
+
#endif