On Sat, Oct 03, 2026 at 01:34:29AM +0000, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) > to consider: > - [High] drm/msm: premature unpin of framebuffers causing IOMMU fault > -- > > commit 86394032733ab2293f0e387866c3cbd603bc3285 > Author: Dmitry Baryshkov <[email protected]> > Subject: drm/msm: release scanout framebuffers only after a vblank > > This commit defers scanout framebuffer release until a vblank by handing them > to a drm_vblank_work per CRTC. On CRTC disable, pending works are flushed and > framebuffers are unpinned directly from msm_crtc_vblank_off(). > > > diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c > > index 7e1df472592d3..cb497d419ab83 100644 > > --- a/drivers/gpu/drm/msm/msm_kms.c > > +++ b/drivers/gpu/drm/msm/msm_kms.c > [ ... ] > > +/* > > + * The crtc stops fetching here, and with it the vblanks the pending works > > are > > + * waiting for, so release the framebuffers directly. > > + */ > > +static void msm_kms_fb_unpin_release_all(struct msm_kms_fb_unpin *pending) > > +{ > [ ... ] > > + drm_vblank_work_cancel_sync(&unpin->base); > > + msm_kms_fb_unpin_release(unpin); > > + } > > +} > > + > > +void msm_crtc_vblank_off(struct drm_crtc *crtc) > > +{ > > + struct msm_drm_private *priv = crtc->dev->dev_private; > > + struct msm_kms *kms = priv->kms; > > + unsigned int idx = drm_crtc_index(crtc); > > + > > + if (kms && idx < ARRAY_SIZE(kms->fb_unpin)) > > + msm_kms_fb_unpin_release_all(&kms->fb_unpin[idx]); > > + > > + drm_crtc_vblank_off(crtc); > > +} > [ ... ] > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > index 77a2d1f6700d0..35daf46fc4257 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > @@ -1213,7 +1213,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, > > } > > > > /* Disable/save vblank irq handling */ > > - drm_crtc_vblank_off(crtc); > > + msm_crtc_vblank_off(crtc); > > [Severity: High] > Can this lead to an IOMMU context fault if the display hardware is still > fetching the frame? > > When a userspace application issues an async plane update followed closely by > a CRTC disable, the ongoing hardware fetch might hit an IOMMU fault because > the memory is unmapped immediately during the disable sequence. > > Because msm_crtc_vblank_off() now synchronously unpins and unmaps all pending > framebuffers via msm_kms_fb_unpin_release_all(), executing this before the > hardware is done fetching creates a race window. > > Looking at dpu_crtc_disable(), we wait for the hardware to finish its current > frame fetch much later in the function: > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:dpu_crtc_disable() { > ... > msm_crtc_vblank_off(crtc); > ... > if (_dpu_crtc_wait_for_frame_done(crtc)) > DPU_ERROR(...); > ... > } > > Does this sequence leave a race window where the display is left reading > unmapped memory?
I think it is not a problem: the atomic helpers disable the encoders before the CRTCs. dpu_encoder_virt_atomic_disable() waits for the pending kickoffs to complete and dpu_encoder_phys_vid_disable() turns off the timing engine and waits for the vsync which latches the to the hardware. For the MDP5 driver mdp5_vid_encoder_disable() does the same. After that the interface no longer fetches when dpu_crtc_disable() calls msm_crtc_vblank_off(). Any async update is flushed by ->wait_flush() at the start of msm_atomic_commit_tail(), before the disables. -- With best wishes Dmitry
