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@xxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx>
> }
>
> static const struct vb2_ops vsp1_video_queue_qops = {
--
Regards,
Laurent Pinchart
