Re: [PATCH] drm/vc4: hdmi: write SCDC TMDS_Bit_Clock_Ratio before the TMDS clock starts

Re: [PATCH] drm/vc4: hdmi: write SCDC TMDS_Bit_Clock_Ratio before the TMDS clock starts

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/vc4: hdmi: SCDC bits updated while TMDS clock runs on BCM2712
- [Medium] drm/vc4: hdmi: split SCDC and scrambler enable across crtc callbacks
- [Medium] drm/vc4: hdmi: race with scrambling_work when disabling scrambling in powerdown
--

commit 5be38e4d64847031b3ada407365354ff41c9b9d0
Author: Lucas Tanure <lucas.tanure@neat.no>

drm/vc4: hdmi: write SCDC TMDS_Bit_Clock_Ratio before the TMDS clock starts

This commit moves the SCDC high TMDS clock ratio and scrambling configuration
into vc4_hdmi_encoder_pre_crtc_configure() before PHY initialization, and
moves vc4_hdmi_disable_scrambling() into
vc4_hdmi_encoder_post_crtc_powerdown() after phy_disable().

Link: https://github.com/raspberrypi/linux/issues/7693

> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 3a90dbb61f4ea..11b9811bb97cf 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -812,9 +812,6 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder)
>  	if (!drm_dev_enter(drm, &idx))
>  		return;
>  
> -	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);
> @@ -919,8 +916,6 @@ 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_dev_exit(idx);
>  
>  out:
> @@ -948,6 +943,8 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder,
>  	if (vc4_hdmi->variant->phy_disable)
>  		vc4_hdmi->variant->phy_disable(vc4_hdmi);
>  
> +	vc4_hdmi_disable_scrambling(encoder);
> +

[Severity: High]
Does moving vc4_hdmi_disable_scrambling() after variant->phy_disable()
actually stop the TMDS clock before clearing the SCDC bits on BCM2712?

While bcm2711_hdmi0_variant sets .phy_disable to vc5_hdmi_phy_disable() (which
calls vc5_hdmi_reset_phy()), bcm2712_hdmi0_variant and bcm2712_hdmi1_variant
set .phy_disable to vc6_hdmi_phy_disable(), which is an empty stub:

drivers/gpu/drm/vc4/vc4_hdmi_phy.c:
void vc6_hdmi_phy_disable(struct vc4_hdmi *vc4_hdmi)
{
}

Because clk_disable_unprepare() and pm_runtime_put() have not executed yet in
vc4_hdmi_encoder_post_crtc_powerdown(), vc4_hdmi_disable_scrambling() still
clears SCDC_SCRAMBLING_ENABLE and SCDC_TMDS_BIT_CLOCK_RATIO_BY_40 while the
BCM2712 PHY PLL and TX clock lane (VC6_HDMI_TX_PHY_HDMI_POWERUP_CTL_TX_CK_PWRUP)
are powered up and outputting the TMDS clock.

Furthermore, during a mode switch (for example from 1080p60 to 3840x2160@60)
or a hotplug link reset via vc4_hdmi_connector_detect_ctx() ->
vc4_hdmi_handle_hotplug() -> vc4_hdmi_reset_link() ->
drm_atomic_helper_reset_crtc(), runtime PM may not synchronously suspend and
reset the hardware between drm_atomic_helper_commit_modeset_disables() and
drm_atomic_helper_commit_modeset_enables() (for instance when pm_runtime_put()
is asynchronous, when CEC holds a runtime PM reference via
vc4_hdmi_cec_enable(), or when vc4_hdmi_connector_detect_ctx() holds a runtime
PM reference across vc4_hdmi_handle_hotplug()). Since vc6_hdmi_phy_disable()
did not call vc6_hdmi_reset_phy(), won't the previous mode's TMDS clock still
be running when vc4_hdmi_encoder_pre_crtc_configure() writes the SCDC
registers and waits 1-2 ms before vc6_hdmi_phy_init() finally calls
vc6_hdmi_reset_phy()?

[Severity: Medium]
Can deferring vc4_hdmi_disable_scrambling() to
vc4_hdmi_encoder_post_crtc_powerdown() race with vc4_hdmi->scrambling_work and
leave SCDC scrambling enabled after teardown?

During vc4_crtc_atomic_disable() -> vc4_crtc_disable(),
vc4_hdmi_encoder_post_crtc_disable() stops HDMI output and drops
vc4_hdmi->mutex, followed by vc4_crtc_pixelvalve_reset(),
vc4_hvs_stop_channel(), and variant->phy_disable(). Throughout this teardown
sequence, vc4_hdmi->scrambling_work remains armed while the link and PHY are
shut down, causing the sink to drop SCDC_SCRAMBLING_STATUS to 0.

If scrambling_work fires during this window and vc4_hdmi_scrambling_wq()
starts executing on system_percpu_wq before vc4_hdmi_disable_scrambling() is
called, delayed_work_pending(&vc4_hdmi->scrambling_work) will return false in
vc4_hdmi_disable_scrambling():

drivers/gpu/drm/vc4/vc4_hdmi.c:vc4_hdmi_disable_scrambling() {
	...
	if (delayed_work_pending(&vc4_hdmi->scrambling_work))
		cancel_delayed_work_sync(&vc4_hdmi->scrambling_work);
	...
}

Because vc4_hdmi_scrambling_wq() neither acquires vc4_hdmi->mutex nor checks
vc4_hdmi->scdc_enabled:

drivers/gpu/drm/vc4/vc4_hdmi.c:vc4_hdmi_scrambling_wq() {
	...
	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));
}

won't the in-flight worker observe drm_scdc_get_scrambling_status(connector)
returning false, write SCDC_TMDS_BIT_CLOCK_RATIO_BY_40 and
SCDC_SCRAMBLING_ENABLE back to true on the sink, and re-queue scrambling_work
every second after vc4_hdmi_disable_scrambling() has finished?

>  	clk_disable_unprepare(vc4_hdmi->pixel_bvb_clock);
>  	clk_disable_unprepare(vc4_hdmi->pixel_clock);
>  
> @@ -1555,6 +1552,13 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
>  		goto err_disable_pixel_clock;
>  	}
>  
> +	if (vc4_hdmi_supports_scrambling(vc4_hdmi) &&
> +	    vc4_hdmi_mode_needs_scrambling(mode, vc4_hdmi->output_bpc, vc4_hdmi->output_format)) {
> +		drm_scdc_set_high_tmds_clock_ratio(connector, true);
> +		drm_scdc_set_scrambling(connector, true);
> +		usleep_range(1000, 2000);
> +	}
> +

[Severity: Medium]
Is it intentional to enable SCDC scrambling in the sink here in
vc4_hdmi_encoder_pre_crtc_configure(), while leaving the source scrambler
enable (VC5_HDMI_SCRAMBLER_CTL_ENABLE) and vc4_hdmi->scdc_enabled = true at
the end of vc4_hdmi_encoder_post_crtc_enable()?

During vc4_atomic_commit_tail() -> drm_atomic_helper_commit_modeset_enables()
-> vc4_crtc_atomic_enable() for a mode above 340 MHz, after
vc4_hdmi_encoder_pre_crtc_configure() sets SCDC_SCRAMBLING_ENABLE in the sink,
the driver runs phy_init(), set_timings(), enables the PixelValve, and then in
vc4_hdmi_encoder_post_crtc_enable() enables video output, waits for
VC4_HDMI_SCHEDULER_CONTROL_HDMI_ACTIVE, enables packet RAM, transmits
InfoFrames via drm_atomic_helper_connector_hdmi_update_infoframes(), and calls
vc4_hdmi_recenter_fifo() before finally calling vc4_hdmi_enable_scrambling():

drivers/gpu/drm/vc4/vc4_hdmi.c:vc4_hdmi_encoder_post_crtc_enable() {
	...
	if (display->is_hdmi) {
		...
		HDMI_WRITE(HDMI_RAM_PACKET_CONFIG,
			   VC4_HDMI_RAM_PACKET_ENABLE);

		spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
		vc4_hdmi->packet_ram_enabled = true;

		drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
	}

	vc4_hdmi_recenter_fifo(vc4_hdmi);
	vc4_hdmi_enable_scrambling(encoder);
}

During that window, the sink's descrambler is enabled while the source is
transmitting unscrambled TMDS control periods, video data, and initial
InfoFrames.

In addition, because vc4_hdmi->mutex is dropped between
vc4_hdmi_encoder_pre_crtc_configure() and vc4_hdmi_encoder_post_crtc_enable(),
if vc4_hdmi_enable_scrambling() returns early (for example if a concurrent
vc4_hdmi_connector_detect_ctx() -> vc4_hdmi_handle_hotplug() ->
drm_atomic_helper_connector_hdmi_hotplug() updates connector->display_info so
vc4_hdmi_supports_scrambling() returns false, or if drm_dev_enter() fails),
vc4_hdmi->scdc_enabled will remain false while the sink's SCDC bits are left
enabled, causing vc4_hdmi_disable_scrambling() to skip clearing them on
disable.

Also, drm_scdc_set_high_tmds_clock_ratio() in
drivers/gpu/drm/display/drm_scdc_helper.c already calls usleep_range(1000,
2000) before returning, so is the additional usleep_range(1000, 2000) here
needed?

>  	if (vc4_hdmi->variant->phy_init)
>  		vc4_hdmi->variant->phy_init(vc4_hdmi, conn_state);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020134941.631907-1-lucas.tanure@neat.no?part=1