Hi Kieran,
Thank you for the patch.
On Friday, 18 May 2018 23:41:54 EEST Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+rene...@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: sta...@vger.kernel.org # v4.13+
Commit 372b2b0399fc was introduced in v4.14, should this be v4.14+ ?
>
> Signed-off-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinch...@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
