Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] The patch handles power-up failures by returning early from `atomic_pre_enable`, but leaves unconditional MMIO accesses unguarded in `atomic_post_disable`, leading to a fatal imprecise abort. Pre-existing issues: - [High] `msm_hdmi_set_timings` accesses MMIO registers before the HDMI power domain is enabled. -- commit 8b49f93cb61baf0b2ba06628a0200e4134a1eb45 Author: Krzysztof Kozlowski <[email protected]> drm/msm/hdmi: Handle msm_hdmi_power_on() errors during .atomic_pre_enable() This patch properly handles power up failures during .atomic_pre_enable() by aborting early to avoid accessing registers when blocks did not bring up, preventing imprecise aborts when the entire power domain is off. > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > index 157f19bd90b4e..db85400e574e4 100644 > --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c [ ... ] > @@ -287,13 +294,18 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct > drm_bridge *bridge, > > msm_hdmi_set_timings(hdmi, &crtc_state->adjusted_mode); [Severity: High] This is a pre-existing issue, but does this function access MMIO registers before the HDMI power domain is enabled? msm_hdmi_set_timings() writes to registers like REG_HDMI_TOTAL: hdmi_write(hdmi, REG_HDMI_TOTAL, HDMI_TOTAL_H_TOTAL(mode->htotal - 1) | ... Since this happens before msm_hdmi_power_on() enables the power domain and clocks, could this trigger an imprecise abort if HPD is not currently keeping the power domain active (for example, when userspace forces a display connection)? > > - mutex_lock(&hdmi->state_mutex); > - if (!hdmi->power_on) { > - msm_hdmi_phy_resource_enable(phy); > - msm_hdmi_power_on(bridge); > - hdmi->power_on = true; > + scoped_guard(mutex, &hdmi->state_mutex) { > + if (!hdmi->power_on) { > + if (msm_hdmi_phy_resource_enable(phy)) > + return; > + > + if (msm_hdmi_power_on(bridge)) { > + msm_hdmi_phy_resource_disable(phy); > + return; > + } [Severity: High] Does this new early return path cause a crash later in the modeset sequence? If msm_hdmi_power_on() fails, we return early leaving hdmi->power_on = false. When the DRM core later calls msm_hdmi_bridge_atomic_post_disable(), it will execute: msm_hdmi_set_mode(hdmi, hdmi->hpd_enabled); msm_hdmi_phy_powerdown(phy); Both of these functions access MMIO registers unconditionally. Since the power domain was never brought up, could these unconditional MMIO accesses trigger an imprecise abort? > + hdmi->power_on = true; > + } > } > - mutex_unlock(&hdmi->state_mutex); > > if (connector->display_info.is_hdmi) > msm_hdmi_audio_update(hdmi); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260911-drm-msm-hdmi-fixes-before-eliza-v4-0-87bdb9f19...@oss.qualcomm.com?part=2
