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

Commit Message

Kieran Bingham May 18, 2018, 8:41 p.m. UTC

From: Kieran Bingham <[e-mail]>

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
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: [e-mail] # v4.13+

Signed-off-by: Kieran Bingham <[e-mail]>
---
 drivers/media/platform/vsp1/vsp1_video.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

Comments

Laurent Pinchart May 18, 2018, 8:53 p.m. UTC | #1

Hi Kieran,

Thank you for the patch.

On Friday, 18 May 2018 23:41:54 EEST Kieran Bingham wrote:
> From: Kieran Bingham <[e-mail]>
> 
> 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: [e-mail] # v4.13+

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

> 
> Signed-off-by: Kieran Bingham <[e-mail]>

Reviewed-by: Laurent Pinchart <[e-mail]>

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);
>  }

Kieran Bingham May 18, 2018, 8:55 p.m. UTC | #2

On 18/05/18 21:53, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Friday, 18 May 2018 23:41:54 EEST Kieran Bingham wrote:
>> From: Kieran Bingham <[e-mail]>
>>
>> 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: [e-mail] # v4.13+
> 
> Commit 372b2b0399fc was introduced in v4.14, should this be v4.14+ ?

Yes, thank you - that's me mis-interpreting my own scripts to get the version
for fixes.


>>
>> Signed-off-by: Kieran Bingham <[e-mail]>
> 
> Reviewed-by: Laurent Pinchart <[e-mail]>
> 
> No need to resubmit for this, I'll fix the commit message when applying.

Great.

--
Kieran

> 
>> ---
>>  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);
>>  }
>

Patch

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);
 }