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 <cristian.ciocaltea@collabora.com>
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);
@@ -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,276 @@ 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);
+
+ 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.
+ *
+ * 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 = connector->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