On 28/08/2026 16:22, [email protected] wrote:
> 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.
Addressed separately.
>
>> +}
>
> [ ... ]
>
>> @@ -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.
It's pre-existing approach but I will address it in v2.
>
> [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.
No, this is just pixel clock, not bus clocks.
>
>> 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?
Ack
Best regards,
Krzysztof
