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

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

[Christian Gmeiner]

From: Christian Gmeiner <cgmeiner@igalia.com>

The controller copies NEXT_BUF to CUR_BUF at the end of each frame. When
a commit writes NEXT_BUF just after that point, the new buffer is only
scanned out one frame later, but the event armed in atomic_flush is
still sent at the next frame done interrupt. Userspace then renders into
a buffer that is still on screen, which shows up as half drawn frames
when a compositor renders directly into its scanout buffers.

Keep the event in the driver and send it from the interrupt handler once
CUR_BUF matches NEXT_BUF, and AS_BUF matches AS_NEXT_BUF for an enabled
overlay. On i.MX8MQ about 2% of the flips completed one frame early
before this change, for both planes.

Fixes: b9f5937680a8 ("drm: mxsfb: Enable vblank handling")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Gmeiner <cgmeiner@igalia.com>
---
 drivers/gpu/drm/mxsfb/mxsfb_drv.c |  2 +-
 drivers/gpu/drm/mxsfb/mxsfb_drv.h |  3 +++
 drivers/gpu/drm/mxsfb/mxsfb_kms.c | 47 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index 641691fc92c8..d21f12074a81 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -166,7 +166,7 @@ static irqreturn_t mxsfb_irq_handler(int irq, void *data)
 	reg = readl(mxsfb->base + LCDC_CTRL1);
 
 	if (reg & CTRL1_CUR_FRAME_DONE_IRQ) {
-		drm_crtc_handle_vblank(&mxsfb->crtc);
+		mxsfb_crtc_handle_vblank(mxsfb);
 		if (mxsfb->crc_active) {
 			crc = readl(mxsfb->base + LCDC_V4_CRC_STAT);
 			vbc = drm_crtc_accurate_vblank_count(&mxsfb->crtc);
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
index bdc47ce5be79..9264ca53999d 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
@@ -49,6 +49,8 @@ struct mxsfb_drm_private {
 	u32				bus_format;
 
 	bool				crc_active;
+
+	struct drm_pending_vblank_event	*event;
 };
 
 static inline struct mxsfb_drm_private *
@@ -61,5 +63,6 @@ void mxsfb_enable_axi_clk(struct mxsfb_drm_private *mxsfb);
 void mxsfb_disable_axi_clk(struct mxsfb_drm_private *mxsfb);
 
 int mxsfb_kms_init(struct mxsfb_drm_private *mxsfb);
+void mxsfb_crtc_handle_vblank(struct mxsfb_drm_private *mxsfb);
 
 #endif /* __MXSFB_DRV_H__ */
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index bbc2a2df9f16..c7e516758a1a 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);
+}
+
 static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
 				     struct drm_atomic_commit *state)
 {
@@ -425,6 +468,8 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc,
 		crtc->state->event = NULL;
 		drm_crtc_send_vblank_event(crtc, event);
 	}
+	if (mxsfb->event)
+		mxsfb_crtc_finish_page_flip(mxsfb);
 	spin_unlock_irq(&drm->event_lock);
 
 	drm_crtc_vblank_off(crtc);

---
base-commit: 19e67d977d3d4d0c54f03906ef578578190db9e1
change-id: 20261108-mxsfb-flip-latch-ef53c4526ba6

Best regards,
-- 
Christian Gmeiner <cgmeiner@igalia.com>

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

[sashiko-bot]

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