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

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

On 6/25/26 10:44 AM, Maxime Ripard wrote:
> On Sat, Jun 13, 2026 at 03:41:20AM +0300, Cristian Ciocaltea wrote:
>> 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(-)
>>>>

[...]

>>>> -
>>>> -	/*
>>>> -	 * 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.
> 
> Oh, right, sorry.
> 
>>> 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.
> 
> A commit is expensive, so I'd skip it if we can.
> 
>> 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;
>>     }
> 
> And yeah, we'll need this one too.
> 
>> 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?
> 
> Yes, but I'd expect the bridge helpers to converge / reuse your helpers
> eventually anyway?

Ack.

[...]

>>>> -	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?
> 
> display/drm_hdmi_helper.c sounds like a great place for both?

Indeed, let's move all the stuff there.

>>>>  	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 for working on FRL as well :)
> 
> I agree, let's focus on getting HDMI 2.0 right, and then we'll try to
> make 2.1 the easiest to work with for drivers.

Thanks for clarifying the remaining details — I should now have everything I
need to prepare a new revision. :)

Regards,
Cristian