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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

On 04/08/17 17:13, Laurent Pinchart wrote:
> On Friday 04 Aug 2017 17:08:34 Kieran Bingham wrote:
>> On 04/08/17 17:03, Laurent Pinchart wrote:
>>> 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/
>>
>> Ah yes - I'm always terrible with my its'y bits.
>>
>>>> and call from both vsp1_video_stop_streaming() and the error path in
>>>> vsp1_video_start_streaming()
>>>
>>> s/$/./
>>
>> :D
>>
>>>> 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() ?
>>
>> I passed this way because the cleanup needed a *video.
>>
>> Is it possible to get from a *pipe to a *video?
> 
> Yes, pipe->output->video

Ah yes of course.


>>>> +{
>>>> +	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);
>>>> +}
> 
> [snip]
> 
>>>> @@ -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.
>>
>> I chose to do the move in 3/7 so that this patch did not change the existing
>> functionality.
>>
>> There is no (explicit) need for the cleanup to happen before the
>> pipeline_put() until the cleanup function references the pipe...
> 
> Except if you pass a *pipe to the cleanup function, as 
> vsp1_video_pipeline_put() can free the pipe.

Indeed!

> 
>>> With all that fixed,

Patch update imminent.

Thanks.

>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>>
>>>>  }
>>>>  
>>>>  static const struct vb2_ops vsp1_video_queue_qops = {
> 



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux