Re: [PATCH v3 1/7] v4l: vsp1: Release buffers in start_streaming error path

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

Hi Kieran,

Thank you for the patch.

On Friday 04 Aug 2017 16:57:05 Kieran Bingham wrote:
> Presently any received buffers are only released back to vb2 if
> vsp1_video_stop_streaming() is called. If vsp1_video_start_streaming()
> encounters an error, we will be warned by the vb2 handlers that buffers
> have not been returned.
> 
> Move the buffer cleanup code to it's own function to prevent duplication

s/it's/its/

> and call from both vsp1_video_stop_streaming() and the error path in
> vsp1_video_start_streaming()

s/$/./

> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_video.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index 5af3486afe07..a24033429cd7
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -822,6 +822,19 @@ static int vsp1_video_setup_pipeline(struct
> vsp1_pipeline *pipe) return 0;
>  }
> 
> +static void vsp1_video_cleanup_pipeline(struct vsp1_video *video)

Should this function take a pipe pointer instead of a video pointer for 
symmetry with vsp1_video_setup_pipeline() ?

> +{
> +	struct vsp1_vb2_buffer *buffer;
> +	unsigned long flags;
> +
> +	/* Remove all buffers from the IRQ queue. */
> +	spin_lock_irqsave(&video->irqlock, flags);
> +	list_for_each_entry(buffer, &video->irqqueue, queue)
> +		vb2_buffer_done(&buffer->buf.vb2_buf, VB2_BUF_STATE_ERROR);
> +	INIT_LIST_HEAD(&video->irqqueue);
> +	spin_unlock_irqrestore(&video->irqlock, flags);
> +}
> +
>  static int vsp1_video_start_streaming(struct vb2_queue *vq, unsigned int
> count) {
>  	struct vsp1_video *video = vb2_get_drv_priv(vq);
> @@ -835,6 +848,7 @@ static int vsp1_video_start_streaming(struct vb2_queue
> *vq, unsigned int count) ret = vsp1_video_setup_pipeline(pipe);
>  		if (ret < 0) {
>  			mutex_unlock(&pipe->lock);
> +			vsp1_video_cleanup_pipeline(video);
>  			return ret;
>  		}
> 
> @@ -866,7 +880,6 @@ static void vsp1_video_stop_streaming(struct vb2_queue
> *vq) {
>  	struct vsp1_video *video = vb2_get_drv_priv(vq);
>  	struct vsp1_pipeline *pipe = video->rwpf->pipe;
> -	struct vsp1_vb2_buffer *buffer;
>  	unsigned long flags;
>  	int ret;
> 
> @@ -893,12 +906,7 @@ static void vsp1_video_stop_streaming(struct vb2_queue
> *vq) media_pipeline_stop(&video->video.entity);
>  	vsp1_video_pipeline_put(pipe);
> 
> -	/* Remove all buffers from the IRQ queue. */
> -	spin_lock_irqsave(&video->irqlock, flags);
> -	list_for_each_entry(buffer, &video->irqqueue, queue)
> -		vb2_buffer_done(&buffer->buf.vb2_buf, VB2_BUF_STATE_ERROR);
> -	INIT_LIST_HEAD(&video->irqqueue);
> -	spin_unlock_irqrestore(&video->irqlock, flags);
> +	vsp1_video_cleanup_pipeline(video);

The vsp1_video_cleanup_pipeline() call should go before 
vsp1_video_pipeline_put(), as you've noticed in patch 7/7.

With all that fixed,

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

>  }
> 
>  static const struct vb2_ops vsp1_video_queue_qops = {

-- 
Regards,

Laurent Pinchart