Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] The HDMI pixel clock is disabled before the 20ms vblank delay, breaking the intended shutdown sequence and risking hardware hangs. - [Medium] Unconditional clock disable and PHY deinitialization can cause unbalanced clock teardown or CCF warnings. Pre-existing issues: - [High] Hardware registers are accessed after dropping the runtime PM reference. - [High] Missing return value checks for hardware initialization functions, such as `pm_runtime_resume_and_get()` and `msm_hdmi_phy_resource_enable()`. -- commit 821f230494b55344c7aaf6bb73ba3e416ff8cb25 Author: Krzysztof Kozlowski <[email protected]> Subject: drm/msm/hdmi: Split PHY init from power up This commit reworks and splits the msm_hdmi_power_on() sequence into smaller steps. It separates out runtime PM enablement, introduces a new msm_hdmi_phy_init() step, and splits clock preparation and reparenting into msm_hdmi_clk_prepare() which runs after PHY initialization. > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > index 9d027f96ae243..ead6ba3b8192a 100644 > --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > @@ -15,12 +15,18 @@ > > static void msm_hdmi_power_on(struct drm_bridge *bridge) > { > - struct drm_device *dev = bridge->dev; > struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge); > struct hdmi *hdmi = hdmi_bridge->hdmi; > - int ret; > > pm_runtime_resume_and_get(&hdmi->pdev->dev); [Severity: High] This is a pre-existing issue, but is it safe to ignore the return value of pm_runtime_resume_and_get() here? If it fails, the power domain is not enabled and the PM usage count is not incremented. Proceeding to access hardware registers will cause an SError or panic, and the subsequent pm_runtime_put() in the disable path will underflow the usage count. > +} [ ... ] > @@ -341,6 +359,8 @@ static void msm_hdmi_bridge_atomic_post_disable(struct > drm_bridge *bridge, > msm_hdmi_set_mode(hdmi, hdmi->hpd_enabled); > > msm_hdmi_phy_powerdown(phy); > + msm_hdmi_clk_unprepare(bridge); > + msm_hdmi_phy_deinit(phy); > > if (hdmi->power_on) { [Severity: Medium] Can this unconditional clock disable and PHY deinitialization cause unbalanced clock teardown or Common Clock Framework warnings? If atomic_post_disable() is called to sanitize the hardware during initial modeset (where hdmi->power_on is false), msm_hdmi_clk_unprepare() will be called without a matching prior enable. This bypasses the state tracking guard and triggers a WARN in the CCF. [Severity: High] Does this code disable the HDMI pixel clock before the 20ms vblank delay inside power_off()? Because msm_hdmi_clk_unprepare() is called before power_off(), the pipeline is frozen mid-frame before the delay executes. Powering down a frozen display controller mid-frame frequently causes system-wide bus hangs (AXI bus stalls) on ARM SoCs. > power_off(bridge); [Severity: High] This is a pre-existing issue, but can the code following power_off() access hardware registers after the runtime PM reference has been dropped? The power_off() function drops the runtime PM reference via pm_runtime_put(). Immediately after, msm_hdmi_bridge_atomic_post_disable() calls msm_hdmi_audio_update() if the display info is HDMI. This accesses MMIO registers via hdmi_read() and hdmi_write(). It risks an asynchronous SError (kernel panic) if the power domain is turned off in the background before the accesses complete. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
