Re: [PATCH] drm/mxsfb: Complete page flips only once the new buffer is latched

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/mxsfb: premature plane cleanup in
  mxsfb_crtc_handle_vblank()
--

commit c09da195fd06536925306459ef31e61d236a956a
Author: Christian Gmeiner <cgmeiner@igalia.com>

drm/mxsfb: Complete page flips only once the new buffer is latched

This commit holds the pending vblank event in mxsfb_drm_private and
sends it from the vblank interrupt handler only once CUR_BUF matches
NEXT_BUF (and AS_BUF matches AS_NEXT_BUF when the overlay is enabled).

> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> index bbc2a2df9f161..c7e516758a1ae 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> @@ -328,6 +328,7 @@ static int mxsfb_crtc_atomic_check(struct drm_crtc *crtc,
>  static void mxsfb_crtc_atomic_flush(struct drm_crtc *crtc,
>  				    struct drm_atomic_commit *state)
>  {
> +	struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(crtc->dev);
>  	struct drm_pending_vblank_event *event;
>  
>  	event = crtc->state->event;
> @@ -338,12 +339,54 @@ static void mxsfb_crtc_atomic_flush(struct drm_crtc *crtc,
>  
>  	spin_lock_irq(&crtc->dev->event_lock);
>  	if (drm_crtc_vblank_get(crtc) == 0)
> -		drm_crtc_arm_vblank_event(crtc, event);
> +		mxsfb->event = event;
>  	else
>  		drm_crtc_send_vblank_event(crtc, event);
>  	spin_unlock_irq(&crtc->dev->event_lock);
>  }
>  
> +static bool mxsfb_crtc_buffers_latched(struct mxsfb_drm_private *mxsfb)
> +{
> +	if (readl(mxsfb->base + mxsfb->devdata->cur_buf) !=
> +	    readl(mxsfb->base + mxsfb->devdata->next_buf))
> +		return false;
> +
> +	if (mxsfb->devdata->has_overlay &&
> +	    (readl(mxsfb->base + LCDC_AS_CTRL) & AS_CTRL_AS_ENABLE) &&
> +	    readl(mxsfb->base + LCDC_AS_BUF) != readl(mxsfb->base + LCDC_AS_NEXT_BUF))
> +		return false;
> +
> +	return true;
> +}
> +
> +static void mxsfb_crtc_finish_page_flip(struct mxsfb_drm_private *mxsfb)
> +{
> +	struct drm_crtc *crtc = &mxsfb->crtc;
> +
> +	drm_crtc_send_vblank_event(crtc, mxsfb->event);
> +	drm_crtc_vblank_put(crtc);
> +	mxsfb->event = NULL;
> +}
> +
> +void mxsfb_crtc_handle_vblank(struct mxsfb_drm_private *mxsfb)
> +{
> +	struct drm_crtc *crtc = &mxsfb->crtc;
> +
> +	drm_crtc_handle_vblank(crtc);
> +
> +	/*
> +	 * The controller copies NEXT_BUF to CUR_BUF at the end of a frame. If
> +	 * the commit wrote NEXT_BUF after that point, the new buffer is only
> +	 * scanned out from the next frame on. Complete the flip only once the
> +	 * hardware has latched it, otherwise userspace gets back a buffer that
> +	 * is still being scanned out.
> +	 */
> +	spin_lock(&crtc->dev->event_lock);
> +	if (mxsfb->event && mxsfb_crtc_buffers_latched(mxsfb))
> +		mxsfb_crtc_finish_page_flip(mxsfb);
> +	spin_unlock(&crtc->dev->event_lock);
> +}

[Severity: High]
Can calling drm_crtc_handle_vblank() before the new buffer is latched
cause drm_atomic_helper_commit_tail_rpm() to clean up the old framebuffer
while it is still being scanned out?

mxsfb_mode_config_helpers uses drm_atomic_helper_commit_tail_rpm(), which
waits via drm_atomic_helper_wait_for_vblanks() rather than
drm_atomic_helper_wait_for_flip_done():

drivers/gpu/drm/drm_atomic_helper.c:drm_atomic_helper_commit_tail_rpm() {
	...
	drm_atomic_helper_commit_hw_done(state);

	drm_atomic_helper_wait_for_vblanks(dev, state);

	drm_atomic_helper_cleanup_planes(dev, state);
}

When a commit writes NEXT_BUF just after the end-of-frame copy, the next
frame-done interrupt sees mxsfb_crtc_buffers_latched() == false and defers
mxsfb_crtc_finish_page_flip() (leaving commit->flip_done unsignaled).
However, drm_crtc_handle_vblank() still increments the vblank count and
wakes drm_atomic_helper_wait_for_vblanks() on that unlatched interrupt.

commit_tail() then immediately runs drm_atomic_helper_cleanup_planes() and
drops the old framebuffer reference while the LCDIF DMA engine is still
scanning it out from CUR_BUF for another frame.  For a blocking commit, it
also returns to userspace before commit->flip_done is signaled, causing an
immediate follow-up non-blocking flip to fail with -EBUSY in
stall_checks().

A custom atomic_commit_tail that calls
drm_atomic_helper_wait_for_flip_done() instead of
drm_atomic_helper_wait_for_vblanks() would keep plane cleanup and commit
completion synchronized with mxsfb_crtc_finish_page_flip().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-mxsfb-flip-latch-v1-1-ad816b23b646@igalia.com?part=1