Re: [PATCH] media: renesas: vsp1: Fix race condition when stopping display pipeline

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Tue, May 12, 2026 at 09:42:13AM +0200, Jacopo Mondi wrote:
> On Tue, May 12, 2026 at 01:38:32AM +0300, Laurent Pinchart wrote:
> > 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.
> 
> Uh that was a tiny window!

It got larger when I added printk statements in the CRC read function,
that's how I noticed. I'm still surprised nobody ever reported the
crash.

> > 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 ?
> 
> I had several runs with this patch and noticed no issues
> I'm also running with LOCKDEP enabled and got no complaints

Thank you.

> Reviewed-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> 
> > ---
> >  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;
> > -	}
> 
> This might be worth a
> 
> 	lockdep_assert_held(&pipe->irqlock);
> 
> before accessing pipe->state

We should then add it to vsp1_du_pipeline_frame_end() and
vsp1_video_pipeline_frame_end() as well. I think that can go to a
separate patch.

> > +	/*
> > +	 * 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;
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> 
> >
> >  	if (vspx_pipe->vspx_frame_end)
> >  		vspx_pipe->vspx_frame_end(vspx_pipe->frame_end_data);
> >
> > base-commit: bc1ba628e37c93cf2abeb2c79716f49087f8a024

-- 
Regards,

Laurent Pinchart