Re: [PATCH v7 25/30] drm/vc4: hdmi: Convert to common HDMI 2.0 SCDC scrambling helpers

[Cristian Ciocaltea <cristian.ciocaltea@collabora.com>]

Hi Maxime,

On 6/12/26 3:04 PM, Maxime Ripard wrote:
> 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 <cristian.ciocaltea@collabora.com>
> 
> 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.

drm_scdc_reset_crtc() alone was not meant to be an equivalent of
vc4_hdmi_reset_link(), as it only checks the sink side and it serves as an
internal helper used exclusively by drm_scdc_sync_status().  

As a matter of fact, the latter is the one responsible for verifying if the
scrambler was enabled on the controller side before attempting to invoke the
reset logic, hence we should get the same behavior.  But we don't invoke it
directly either, it's part of the drm_atomic_helper_connector_hdmi_hotplug_ctx()
call path.

> I'd argue that it's drm_scdc_reset_crtc() that needs to align to what
> vc4 was doing, not the opposite.

The only difference consists in dropping the crtc state check:

    ret = drm_modeset_lock(&crtc->mutex, ctx);
    if (ret)
            return ret;

    crtc_state = crtc->state;
    if (!crtc_state->active)
            return 0;

The rationale was that when CRTC is inactive, drm_atomic_helper_reset_crtc()
should result in a no-op commit anyway.

And the one for the in-flight commit:

    if (conn_state->commit &&
        !try_wait_for_completion(&conn_state->commit->hw_done)) {
            mutex_unlock(&vc4_hdmi->mutex);
            return 0;
    }

Both checks are also missing in drm_bridge_helper_reset_crtc(), taken as an
initial reference. Should we still keep any/both and sync the bridge helper
accordingly?

>> -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.

Ack.

> 
>>  				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

Sorry, somehow I missed to realize when I prepared the patches that I
accidentally dropped these during my initial driver rework.

> 
>> -	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.

Ack. I guess we should also move these helpers out of drm_scdc_helper.c, but
unsure where.  FWIW I'm currently working on adding HDMI 2.1 FRL support, and
implemented the link training in a dedicated drm_hdmi_frl_helper.c.  

Should we create drm_hdmi_scrambler_helper.c?  Or maybe have a common one to
hold both - any suggestion for the naming?

> 
>>  	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.

I was about to consider a similar approach before deciding to let drivers manage
the logic, i.e. to prevent loosing flexibility later when dealing with HDMI 2.1.
That was mostly in the context of supporting drivers to define if/when a display
mode that fits in TMDS should be sent over FRL. 

Thinking again, that's not really a valid concern right now, e.g. will use TMDS
by default for all supported modes, and switch to FRL only when absolutely
required.  Later we might consider extending the infrastructure to support
dynamic control if required.

Thanks,
Cristian