Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Sleeping functions called from atomic context. - [High] Unbalanced reference counting on AXI clock enable failure leads to premature clock disable for active CRTCs. Pre-existing issues: - [High] Failure to enforce IOMMU requirement by allowing a NULL VM fallback in drm_gpuvm conversion. -- commit f1ffff35bea0debf56db480bb3da6443ad42c41a Author: Alexandre MINETTE <[email protected]> Subject: drm/msm/mdp4: keep the bus clock enabled while a CRTC is active This patch keeps the AXI/bus clock enabled for the lifetime of each active CRTC to avoid temporary MDP4 disable paths gating the bus clock while scanout is still running. > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c > b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c > index e8066f9fd5343..9e53c9d956ca8 100644 > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c > @@ -271,6 +271,7 @@ static void mdp4_crtc_atomic_disable(struct drm_crtc > *crtc, > > mdp_irq_unregister(&mdp4_kms->base, &mdp4_crtc->err); > mdp4_disable(mdp4_kms); > + mdp4_crtc_bus_put(mdp4_kms); [Severity: High] Does calling mdp4_crtc_bus_put() here introduce a sleeping lock into an atomic context? CRTC atomic disable callbacks execute in an atomic context where sleeping is forbidden, but mdp4_crtc_bus_put() acquires a mutex lock. [ ... ] > @@ -295,6 +296,7 @@ static void mdp4_crtc_atomic_enable(struct drm_crtc *crtc, > return; > > mdp4_enable(mdp4_kms); > + mdp4_crtc_bus_get(mdp4_kms); [Severity: High] Similarly, does this introduce sleeping calls into the atomic enable path? mdp4_crtc_bus_get() uses both mutex_lock() and clk_prepare_enable(), which may sleep. [ ... ] > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > index 809ca191e9de7..4c60b49fb7842 100644 > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c > @@ -118,6 +118,39 @@ static long mdp4_round_pixclk(struct msm_kms *kms, > unsigned long rate, [ ... ] > +void mdp4_crtc_bus_get(struct mdp4_kms *mdp4_kms) > +{ > + int ret = 0; > + > + mutex_lock(&mdp4_kms->clock_lock); > + > + if (!mdp4_kms->crtc_bus_count) > + ret = clk_prepare_enable(mdp4_kms->axi_clk); > + > + if (!ret) > + mdp4_kms->crtc_bus_count++; > + > + mutex_unlock(&mdp4_kms->clock_lock); > + > + WARN_ON(ret); > +} > + > +void mdp4_crtc_bus_put(struct mdp4_kms *mdp4_kms) > +{ > + mutex_lock(&mdp4_kms->clock_lock); > + if (!mdp4_kms->crtc_bus_count) > + goto unlock; > + > + mdp4_kms->crtc_bus_count--; [Severity: High] Could this result in unbalanced reference counting and premature clock disable? If clk_prepare_enable() fails in mdp4_crtc_bus_get(), crtc_bus_count is not incremented, but the error isn't propagated since the function returns void. If a subsequent CRTC successfully enables the clock, the counter becomes 1. Later, when the first (failed) CRTC is disabled, mdp4_crtc_bus_put() will unconditionally decrement crtc_bus_count to 0 and disable the clock. This steals the reference belonging to the second active CRTC, leaving it scanning out with its bus clock disabled. [ ... ] While reviewing this file, I noticed a pre-existing issue in mdp4_kms_init(): drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c:mdp4_kms_init() { ... vm = msm_kms_init_vm(mdp4_kms->dev, NULL); if (IS_ERR(vm)) { ret = PTR_ERR(vm); goto fail; } kms->vm = vm; ... } [Severity: High] This is a pre-existing issue, but does this bypass the IOMMU requirement if msm_kms_init_vm() returns NULL? The DRM subsystem requires IOMMU support for drm_gpuvm and does not support fallback modes. Since msm_kms_init_vm() might return NULL when IOMMU is unavailable, checking only IS_ERR(vm) allows driver initialization to proceed with a NULL VM instead of failing with an error. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260611-mainline-fix-mdp4-blue-screen-sending-v1-v1-0-22925600e...@alex-min.fr?part=1
