Hi,
On Tue, Jun 02, 2026 at 01:44:25AM +0300, Cristian Ciocaltea wrote:
> Replace the vc4-local scrambling implementation with the newly
> introduced DRM common SCDC scrambling infrastructure:
>
> - Advertise source-side scrambling support by setting
> connector->hdmi.scrambling_supported based on the variant's
> max_pixel_clock before drmm_connector_hdmi_init().
>
> - Provide minimal .scrambler_{enable|disable} connector callbacks that
> only toggle the VC5 HDMI_SCRAMBLER_CTL register. Sink-side SCDC
> programming and periodic status monitoring are now delegated to
> drm_scdc_{start|stop}_scrambling().
>
> - Replace vc4_hdmi_enable_scrambling() with a conditional call to
> drm_scdc_start_scrambling() in post_crtc_enable, gated on
> conn_state->hdmi.scrambler_needed (computed by the HDMI state helper).
>
> - Replace vc4_hdmi_disable_scrambling() with drm_scdc_stop_scrambling()
> in post_crtc_disable.
>
> - Drop vc4_hdmi_reset_link() and vc4_hdmi_handle_hotplug(), switching
> the .detect_ctx() path to
> drm_atomic_helper_connector_hdmi_hotplug_ctx() which internally calls
> drm_scdc_sync_status() to trigger a CRTC reset on reconnection.
>
> - Drop the local scrambling_work delayed workqueue and scdc_enabled
> flag, now tracked by the common drm_connector_hdmi layer.
>
> - Drop vc4_hdmi_supports_scrambling() and
> vc4_hdmi_mode_needs_scrambling() helpers, inlining the remaining 4KP60
> warning with an explicit drm_hdmi_compute_mode_clock() check.
>
> - Seed connector->hdmi.scrambler_enabled = true in connector_init() to
> ensure drm_scdc_stop_scrambling() runs at boot and disables any stale
> scrambling state left by the bootloader.
>
> No functional change is expected for the supported modes.
>
> Signed-off-by: Cristian Ciocaltea <[email protected]>
I'd really like it to be broken down into several patches:
> ---
> drivers/gpu/drm/vc4/vc4_hdmi.c | 265
> ++++++-----------------------------------
> drivers/gpu/drm/vc4/vc4_hdmi.h | 8 --
> 2 files changed, 35 insertions(+), 238 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 046ac4f43ba8..02f6ca6ab52b 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -114,31 +114,6 @@
> #define HSM_MIN_CLOCK_FREQ 120000000
> #define CEC_CLOCK_FREQ 40000
>
> -static bool vc4_hdmi_supports_scrambling(struct vc4_hdmi *vc4_hdmi)
> -{
> - struct drm_display_info *display = &vc4_hdmi->connector.display_info;
> -
> - lockdep_assert_held(&vc4_hdmi->mutex);
> -
> - if (!display->is_hdmi)
> - return false;
> -
> - if (!display->hdmi.scdc.supported ||
> - !display->hdmi.scdc.scrambling.supported)
> - return false;
> -
> - return true;
> -}
> -
> -static bool vc4_hdmi_mode_needs_scrambling(const struct drm_display_mode
> *mode,
> - unsigned int bpc,
> - enum drm_output_color_format fmt)
> -{
> - unsigned long long clock = drm_hdmi_compute_mode_clock(mode, bpc, fmt);
> -
> - return clock > HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
> -}
> -
> static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
> {
> struct drm_debugfs_entry *entry = m->private;
> @@ -272,124 +247,6 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi
> *vc4_hdmi)
> static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {}
> #endif
>
> -static int vc4_hdmi_reset_link(struct drm_connector *connector,
> - struct drm_modeset_acquire_ctx *ctx)
> -{
> - struct drm_device *drm;
> - struct vc4_hdmi *vc4_hdmi;
> - struct drm_connector_state *conn_state;
> - struct drm_crtc_state *crtc_state;
> - struct drm_crtc *crtc;
> - bool scrambling_needed;
> - u8 config;
> - int ret;
> -
> - if (!connector)
> - return 0;
> -
> - drm = connector->dev;
> - ret = drm_modeset_lock(&drm->mode_config.connection_mutex, ctx);
> - if (ret)
> - return ret;
> -
> - conn_state = connector->state;
> - crtc = conn_state->crtc;
> - if (!crtc)
> - return 0;
> -
> - ret = drm_modeset_lock(&crtc->mutex, ctx);
> - if (ret)
> - return ret;
> -
> - crtc_state = crtc->state;
> - if (!crtc_state->active)
> - return 0;
> -
> - vc4_hdmi = connector_to_vc4_hdmi(connector);
> - mutex_lock(&vc4_hdmi->mutex);
> -
> - if (!vc4_hdmi_supports_scrambling(vc4_hdmi)) {
> - mutex_unlock(&vc4_hdmi->mutex);
> - return 0;
> - }
> -
> - scrambling_needed =
> vc4_hdmi_mode_needs_scrambling(&vc4_hdmi->saved_adjusted_mode,
> - vc4_hdmi->output_bpc,
> -
> vc4_hdmi->output_format);
> - if (!scrambling_needed) {
> - mutex_unlock(&vc4_hdmi->mutex);
> - return 0;
> - }
> -
> - if (conn_state->commit &&
> - !try_wait_for_completion(&conn_state->commit->hw_done)) {
> - mutex_unlock(&vc4_hdmi->mutex);
> - return 0;
> - }
> -
> - ret = drm_scdc_readb(connector->ddc, SCDC_TMDS_CONFIG, &config);
> - if (ret < 0) {
> - drm_err(drm, "Failed to read TMDS config: %d\n", ret);
> - mutex_unlock(&vc4_hdmi->mutex);
> - return 0;
> - }
> -
> - if (!!(config & SCDC_SCRAMBLING_ENABLE) == scrambling_needed) {
> - mutex_unlock(&vc4_hdmi->mutex);
> - return 0;
> - }
> -
> - mutex_unlock(&vc4_hdmi->mutex);
> -
> - /*
> - * HDMI 2.0 says 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.
> - */
> - return drm_atomic_helper_reset_crtc(crtc, ctx);
> -}
This one doesn't look functionally equivalent to me to
drm_scdc_reset_crtc: this part was, in part, making sure we would only
reset the scrambler if it was enabled in the first place.
drm_scdc_reset_crtc() doesn't and will always trigger a modeset on
hotplug. That's unnecessary and a significant functional different.
I'd argue that it's drm_scdc_reset_crtc() that needs to align to what
vc4 was doing, not the opposite.
> -static void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi,
> - struct drm_modeset_acquire_ctx *ctx,
> - enum drm_connector_status status)
> -{
> - struct drm_connector *connector = &vc4_hdmi->connector;
> - int ret;
> -
> - /*
> - * NOTE: This function should really be called with vc4_hdmi->mutex
> - * held, but doing so results in reentrancy issues since
> - * cec_s_phys_addr() might call .adap_enable, which leads to that
> - * funtion being called with our mutex held.
> - *
> - * A similar situation occurs with vc4_hdmi_reset_link() that
> - * will call into our KMS hooks if the scrambling was enabled.
> - *
> - * Concurrency isn't an issue at the moment since we don't share
> - * any state with any of the other frameworks so we can ignore
> - * the lock for now.
> - */
> -
> - drm_atomic_helper_connector_hdmi_hotplug(connector, status);
> -
> - if (status != connector_status_connected)
> - return;
> -
> - for (;;) {
> - ret = vc4_hdmi_reset_link(connector, ctx);
> - if (ret == -EDEADLK) {
> - drm_modeset_backoff(ctx);
> - continue;
> - }
> -
> - break;
> - }
> -}
> -
> static int vc4_hdmi_connector_detect_ctx(struct drm_connector *connector,
> struct drm_modeset_acquire_ctx *ctx,
> bool force)
> @@ -401,8 +258,8 @@ static int vc4_hdmi_connector_detect_ctx(struct
> drm_connector *connector,
> /*
> * NOTE: This function should really take vc4_hdmi->mutex, but
> * doing so results in reentrancy issues since
> - * vc4_hdmi_handle_hotplug() can call into other functions that
> - * would take the mutex while it's held here.
> + * drm_atomic_helper_connector_hdmi_hotplug_ctx() can call into other
> + * functions that would take the mutex while it's held here.
> *
> * Concurrency isn't an issue at the moment since we don't share
> * any state with any of the other frameworks so we can ignore
> @@ -425,10 +282,11 @@ static int vc4_hdmi_connector_detect_ctx(struct
> drm_connector *connector,
> status = connector_status_connected;
> }
>
> - vc4_hdmi_handle_hotplug(vc4_hdmi, ctx, status);
> + ret = drm_atomic_helper_connector_hdmi_hotplug_ctx(connector, status,
> ctx);
> +
> pm_runtime_put(&vc4_hdmi->pdev->dev);
>
> - return status;
> + return ret == -EDEADLK ? ret : status;
> }
>
> static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
> @@ -441,9 +299,12 @@ static int vc4_hdmi_connector_get_modes(struct
> drm_connector *connector)
> if (!vc4->hvs->vc5_hdmi_enable_hdmi_20) {
> struct drm_device *drm = connector->dev;
> const struct drm_display_mode *mode;
> + unsigned long long clock;
>
> list_for_each_entry(mode, &connector->probed_modes, head) {
> - if (vc4_hdmi_mode_needs_scrambling(mode, 8,
> DRM_OUTPUT_COLOR_FORMAT_RGB444)) {
> + clock = drm_hdmi_compute_mode_clock(mode, 8,
> +
> DRM_OUTPUT_COLOR_FORMAT_RGB444);
> + if (clock > HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ) {
This should be a patch of its own, but I think we should turn
vc4_hdmi_mode_needs_scrambling() into a helper, instead of checking the
clock rate in every driver that might need it. From a logical standpoint
it's equivalent, but not from a semantic one.
> drm_warn_once(drm, "The core clock cannot reach
> frequencies high enough to support 4k @ 60Hz.");
> drm_warn_once(drm, "Please change your
> config.txt file to add hdmi_enable_4kp60.");
> }
> @@ -540,6 +401,9 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
> if (vc4_hdmi->variant->supports_hdr)
> max_bpc = 12;
>
> + connector->hdmi.scrambler_supported =
> + vc4_hdmi->variant->max_pixel_clock >
> HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
> +
> ret = drmm_connector_hdmi_init(dev, connector,
> "Broadcom", "Videocore",
> &vc4_hdmi_connector_funcs,
> @@ -561,6 +425,14 @@ static int vc4_hdmi_connector_init(struct drm_device
> *dev,
>
> drm_connector_helper_add(connector, &vc4_hdmi_connector_helper_funcs);
>
> + /*
> + * Since we don't know the state of the controller and its
> + * display (if any), let's assume it's always enabled.
> + * drm_scdc_stop_scrambling() will thus run at boot, make
> + * sure it's disabled, and avoid any inconsistency.
> + */
> + connector->hdmi.scrambler_enabled = connector->hdmi.scrambler_supported;
> +
> /*
> * Some of the properties below require access to state, like bpc.
> * Allocate some default initial connector state with our reset helper.
> @@ -786,93 +658,30 @@ static int vc4_hdmi_write_spd_infoframe(struct
> drm_connector *connector,
> buffer, len);
> }
>
> -#define SCRAMBLING_POLLING_DELAY_MS 1000
> -
> -static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder)
> +static int vc4_hdmi_scrambler_enable(struct drm_connector *connector)
> {
> - struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
> - struct drm_connector *connector = &vc4_hdmi->connector;
> - struct drm_device *drm = connector->dev;
> - const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
> + struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
> unsigned long flags;
> - int idx;
> -
> - lockdep_assert_held(&vc4_hdmi->mutex);
> -
> - if (!vc4_hdmi_supports_scrambling(vc4_hdmi))
> - return;
> -
> - if (!vc4_hdmi_mode_needs_scrambling(mode,
> - vc4_hdmi->output_bpc,
> - vc4_hdmi->output_format))
> - return;
> -
> - if (!drm_dev_enter(drm, &idx))
> - return;
drm_dev_enter should be kept here
> - drm_scdc_set_high_tmds_clock_ratio(connector, true);
> - drm_scdc_set_scrambling(connector, true);
>
> spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
> HDMI_WRITE(HDMI_SCRAMBLER_CTL, HDMI_READ(HDMI_SCRAMBLER_CTL) |
> VC5_HDMI_SCRAMBLER_CTL_ENABLE);
> spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
>
> - drm_dev_exit(idx);
And exit here.
> -static void vc4_hdmi_disable_scrambling(struct drm_encoder *encoder)
> +static int vc4_hdmi_scrambler_disable(struct drm_connector *connector)
> {
> - struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
> - struct drm_connector *connector = &vc4_hdmi->connector;
> - struct drm_device *drm = connector->dev;
> + struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
> unsigned long flags;
> - int idx;
> -
> - lockdep_assert_held(&vc4_hdmi->mutex);
> -
> - if (!vc4_hdmi->scdc_enabled)
> - return;
> -
> - vc4_hdmi->scdc_enabled = false;
> -
> - if (delayed_work_pending(&vc4_hdmi->scrambling_work))
> - cancel_delayed_work_sync(&vc4_hdmi->scrambling_work);
> -
> - if (!drm_dev_enter(drm, &idx))
> - return;
Ditto
> spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
> HDMI_WRITE(HDMI_SCRAMBLER_CTL, HDMI_READ(HDMI_SCRAMBLER_CTL) &
> ~VC5_HDMI_SCRAMBLER_CTL_ENABLE);
> spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
>
> - drm_scdc_set_scrambling(connector, false);
> - drm_scdc_set_high_tmds_clock_ratio(connector, false);
> -
> - drm_dev_exit(idx);
> -}
> -
> -static void vc4_hdmi_scrambling_wq(struct work_struct *work)
> -{
> - struct vc4_hdmi *vc4_hdmi = container_of(to_delayed_work(work),
> - struct vc4_hdmi,
> - scrambling_work);
> - struct drm_connector *connector = &vc4_hdmi->connector;
> -
> - if (drm_scdc_get_scrambling_status(connector))
> - return;
> -
> - drm_scdc_set_high_tmds_clock_ratio(connector, true);
> - drm_scdc_set_scrambling(connector, true);
> -
> - queue_delayed_work(system_percpu_wq, &vc4_hdmi->scrambling_work,
> - msecs_to_jiffies(SCRAMBLING_POLLING_DELAY_MS));
> + return 0;
> }
>
> static void vc4_hdmi_encoder_post_crtc_disable(struct drm_encoder *encoder,
> @@ -917,7 +726,7 @@ static void vc4_hdmi_encoder_post_crtc_disable(struct
> drm_encoder *encoder,
> spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
> }
>
> - vc4_hdmi_disable_scrambling(encoder);
> + drm_scdc_stop_scrambling(&vc4_hdmi->connector);
I don't think the names here are right. It's not *only* related to scdc
but also to the HDMI controller. I'm fine with using a scdc prefix but
only for the things related to scdc. This is related (in part) to the
HDMI controller, so it should use a drm_connector_hdmi prefix.
> drm_dev_exit(idx);
>
> @@ -1625,6 +1434,7 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct
> drm_encoder *encoder,
> struct drm_display_info *display = &vc4_hdmi->connector.display_info;
> bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC;
> bool vsync_pos = mode->flags & DRM_MODE_FLAG_PVSYNC;
> + struct drm_connector_state *conn_state;
> unsigned long flags;
> int ret;
> int idx;
> @@ -1693,7 +1503,10 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct
> drm_encoder *encoder,
> }
>
> vc4_hdmi_recenter_fifo(vc4_hdmi);
> - vc4_hdmi_enable_scrambling(encoder);
> +
> + conn_state = drm_atomic_get_new_connector_state(state, connector);
> + if (conn_state && conn_state->hdmi.scrambler_needed)
> + drm_scdc_start_scrambling(connector);
And the nice thing with a drm_connector_hdmi_* prefix is that you don't
have to shoehorn it into SCDC support anymore, so you can take a state
as an argument, and do the check in the helper instead of doing it in
every driver and hoping they get it right.
Maxime
signature.asc
Description: PGP signature
