Re: [PATCH v11 01/10] media: v4l: vsp1: Release buffers for each video node

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

Hi Kieran,

Thank you for the patch.

On Friday, 18 May 2018 23:41:54 EEST Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Commit 372b2b0399fc ("media: v4l: vsp1: Release buffers in
> start_streaming error path") introduced a helper to clean up buffers on
> error paths, but inadvertently changed the code such that only the
> output WPF buffers were cleaned, rather than the video node being
> operated on.
> 
> Since then vsp1_video_cleanup_pipeline() has grown to perform both video
> node cleanup, as well as pipeline cleanup. Split the implementation into
> two distinct functions that perform the required work, so that each
> video node can release it's buffers correctly on streamoff. The pipe

s/it's/its/

> cleanup that was performed in the vsp1_video_stop_streaming() (releasing
> the pipe->dl) is moved to the function for clarity.
> 
> Fixes: 372b2b0399fc ("media: v4l: vsp1: Release buffers in start_streaming
> error path")
> Cc: stable@vger.kernel.org # v4.13+

Commit 372b2b0399fc was introduced in v4.14, should this be v4.14+ ?

> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

No need to resubmit for this, I'll fix the commit message when applying.

> ---
>  drivers/media/platform/vsp1/vsp1_video.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index c8c12223a267..ba89dd176a13
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -842,9 +842,8 @@ static int vsp1_video_setup_pipeline(struct
> vsp1_pipeline *pipe) return 0;
>  }
> 
> -static void vsp1_video_cleanup_pipeline(struct vsp1_pipeline *pipe)
> +static void vsp1_video_release_buffers(struct vsp1_video *video)
>  {
> -	struct vsp1_video *video = pipe->output->video;
>  	struct vsp1_vb2_buffer *buffer;
>  	unsigned long flags;
> 
> @@ -854,12 +853,18 @@ static void vsp1_video_cleanup_pipeline(struct
> vsp1_pipeline *pipe) vb2_buffer_done(&buffer->buf.vb2_buf,
> VB2_BUF_STATE_ERROR);
>  	INIT_LIST_HEAD(&video->irqqueue);
>  	spin_unlock_irqrestore(&video->irqlock, flags);
> +}
> +
> +static void vsp1_video_cleanup_pipeline(struct vsp1_pipeline *pipe)
> +{
> +	lockdep_assert_held(&pipe->lock);
> 
>  	/* Release our partition table allocation */
> -	mutex_lock(&pipe->lock);
>  	kfree(pipe->part_table);
>  	pipe->part_table = NULL;
> -	mutex_unlock(&pipe->lock);
> +
> +	vsp1_dl_list_put(pipe->dl);
> +	pipe->dl = NULL;
>  }
> 
>  static int vsp1_video_start_streaming(struct vb2_queue *vq, unsigned int
> count) @@ -874,8 +879,9 @@ static int vsp1_video_start_streaming(struct
> vb2_queue *vq, unsigned int count) if (pipe->stream_count ==
> pipe->num_inputs) {
>  		ret = vsp1_video_setup_pipeline(pipe);
>  		if (ret < 0) {
> -			mutex_unlock(&pipe->lock);
> +			vsp1_video_release_buffers(video);
>  			vsp1_video_cleanup_pipeline(pipe);
> +			mutex_unlock(&pipe->lock);
>  			return ret;
>  		}
> 
> @@ -925,13 +931,12 @@ static void vsp1_video_stop_streaming(struct vb2_queue
> *vq) if (ret == -ETIMEDOUT)
>  			dev_err(video->vsp1->dev, "pipeline stop timeout\n");
> 
> -		vsp1_dl_list_put(pipe->dl);
> -		pipe->dl = NULL;
> +		vsp1_video_cleanup_pipeline(pipe);
>  	}
>  	mutex_unlock(&pipe->lock);
> 
>  	media_pipeline_stop(&video->video.entity);
> -	vsp1_video_cleanup_pipeline(pipe);
> +	vsp1_video_release_buffers(video);
>  	vsp1_video_pipeline_put(pipe);
>  }

-- 
Regards,

Laurent Pinchart