Linux Media Controller development
 help / color / mirror / Atom feed
* [PATCH] media: renesas: vsp1: Fix race condition when stopping display pipeline
@ 2026-05-11 22:38 Laurent Pinchart
  2026-05-12  7:42 ` Jacopo Mondi
  2026-05-12 15:35 ` Niklas Söderlund
  0 siblings, 2 replies; 5+ messages in thread
From: Laurent Pinchart @ 2026-05-11 22:38 UTC (permalink / raw)
  To: linux-media
  Cc: linux-renesas-soc, Kieran Bingham, Jacopo Mondi,
	Niklas Söderlund, stable

When stopping a display pipeline, the vsp1_du_setup_lif() function first
stops the hardware by calling vsp1_pipeline_stop(), and then resets
drm_pipe->du_complete to NULL. Stopping the hardware ensures no new
interrupt is generated, but does not wait for completion of any running
interrupt handler. This creates a race with vsp1_du_pipeline_frame_end()
which tests drm_pipe->du_complete before calling the function pointer.
If the drm_pipe->du_complete pointer is reset to NULL between those two
operations, a NULL pointer derefence will occur.

Fix this by setting pipe->state to STOPPING before stopping the
hardware, and avoid calling the frame end handler if the state is not
RUNNING. Condition the latter to the display pipeline, as the other
pipeline use a different stop procedure that waits for the frame end
handler to set the state to STOPPED.

The state check needs to be protected by the pipe->irqlock. The lock is
used by the video and vspx completion handlers already, so move it one
level up to vsp1_pipeline_frame_end().

Fixes: d7ade201ae7f ("v4l: vsp1: Extend VSP1 module API to allow DRM callbacks")
Cc: stable@vger.kernel.org
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---

I have noticed this race condition while debugging a separate issue and
adding printk() statements in the display pipeline frame end. I have
tested the fix with the DU test suite and VSP test suite, exercising
both the display and video pipelines. I'm fairly confident that the VSPX
pipeline won't be negatively affected, but it would be good to
double-check that. Jacopo, Niklas, would you be able to give test it ?

---
 drivers/media/platform/renesas/vsp1/vsp1_pipe.c  | 12 ++++++++++--
 drivers/media/platform/renesas/vsp1/vsp1_video.c |  5 -----
 drivers/media/platform/renesas/vsp1/vsp1_vspx.c  | 13 +++++--------
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c
index 5d769cc42fe1..aaec1aa15091 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_pipe.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_pipe.c
@@ -509,6 +509,10 @@ int vsp1_pipeline_stop(struct vsp1_pipeline *pipe)
 		 * When using display lists in continuous frame mode the only
 		 * way to stop the pipeline is to reset the hardware.
 		 */
+		scoped_guard(spinlock_irqsave, &pipe->irqlock) {
+			pipe->state = VSP1_PIPELINE_STOPPING;
+		}
+
 		ret = vsp1_reset_wpf(vsp1, pipe->output->entity.index);
 		if (ret == 0) {
 			spin_lock_irqsave(&pipe->irqlock, flags);
@@ -583,8 +587,12 @@ void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe)
 	 * Regardless of frame completion we still need to notify the pipe
 	 * frame_end to account for vblank events.
 	 */
-	if (pipe->frame_end)
-		pipe->frame_end(pipe, flags);
+	scoped_guard(spinlock_irqsave, &pipe->irqlock) {
+		if (pipe->state == VSP1_PIPELINE_RUNNING || !pipe->lif) {
+			if (pipe->frame_end)
+				pipe->frame_end(pipe, flags);
+		}
+	}
 
 	pipe->sequence++;
 }
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c
index fe1dac11d4ae..a8db94bdb670 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_video.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c
@@ -325,14 +325,11 @@ static void vsp1_video_pipeline_frame_end(struct vsp1_pipeline *pipe,
 {
 	struct vsp1_device *vsp1 = pipe->output->entity.vsp1;
 	enum vsp1_pipeline_state state;
-	unsigned long flags;
 	unsigned int i;
 
 	/* M2M Pipelines should never call here with an incomplete frame. */
 	WARN_ON_ONCE(!(completion & VSP1_DL_FRAME_END_COMPLETED));
 
-	spin_lock_irqsave(&pipe->irqlock, flags);
-
 	/* Complete buffers on all video nodes. */
 	for (i = 0; i < vsp1->info->rpf_count; ++i) {
 		if (!pipe->inputs[i])
@@ -354,8 +351,6 @@ static void vsp1_video_pipeline_frame_end(struct vsp1_pipeline *pipe,
 		wake_up(&pipe->wq);
 	else if (vsp1_pipeline_ready(pipe))
 		vsp1_video_pipeline_run(pipe);
-
-	spin_unlock_irqrestore(&pipe->irqlock, flags);
 }
 
 static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe,
diff --git a/drivers/media/platform/renesas/vsp1/vsp1_vspx.c b/drivers/media/platform/renesas/vsp1/vsp1_vspx.c
index 1673479be0ff..26c477708858 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_vspx.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_vspx.c
@@ -176,14 +176,11 @@ static void vsp1_vspx_pipeline_frame_end(struct vsp1_pipeline *pipe,
 {
 	struct vsp1_vspx_pipeline *vspx_pipe = to_vsp1_vspx_pipeline(pipe);
 
-	scoped_guard(spinlock_irqsave, &pipe->irqlock) {
-		/*
-		 * Operating the vsp1_pipe in singleshot mode requires to
-		 * manually set the pipeline state to stopped when a transfer
-		 * is completed.
-		 */
-		pipe->state = VSP1_PIPELINE_STOPPED;
-	}
+	/*
+	 * Operating the vsp1_pipe in singleshot mode requires to manually set
+	 * the pipeline state to stopped when a transfer is completed.
+	 */
+	pipe->state = VSP1_PIPELINE_STOPPED;
 
 	if (vspx_pipe->vspx_frame_end)
 		vspx_pipe->vspx_frame_end(vspx_pipe->frame_end_data);

base-commit: bc1ba628e37c93cf2abeb2c79716f49087f8a024
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-05-12 16:43 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-11 22:38 [PATCH] media: renesas: vsp1: Fix race condition when stopping display pipeline Laurent Pinchart
2026-05-12  7:42 ` Jacopo Mondi
2026-05-12  8:28   ` Laurent Pinchart
2026-05-12 15:35 ` Niklas Söderlund
2026-05-12 16:43   ` Niklas Söderlund

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox