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 <[email protected]> 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/[email protected]?part=1
