[libcamera-devel,2/3] libcamera: ipu3: Do not re-queue failed buffers

1700 diff mbox series

When a video device is stopped all the buffers there queued are
released and their state is set to failure.

Currently, on buffer completion, failed buffers are blindly re-queued to
the ImgU input or CIO2 output devices, preventing them to be re-started
succesfully in future capture sessions.

Fix that by inspecting the buffers status and skip re-queueing if
they're reported as failing. For the ImgU output buffers this is not
required, as failed buffes should be anyhow delivered to applications in
order to report their failure.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++++++
 1 file changed, 6 insertions(+)

Hi Jacopo,

On 15/07/2019 06:59, Jacopo Mondi wrote:
> When a video device is stopped all the buffers there queued are
> released and their state is set to failure.
> 
> Currently, on buffer completion, failed buffers are blindly re-queued to
> the ImgU input or CIO2 output devices, preventing them to be re-started
> succesfully in future capture sessions.

s/succesfully/successfully/

> 
> Fix that by inspecting the buffers status and skip re-queueing if
> they're reported as failing. For the ImgU output buffers this is not
> required, as failed buffes should be anyhow delivered to applications in

s/buffes/buffers/

> order to report their failure.
> 

Will this patch lead to us 'leaking' buffers if they are failed for some
reason (other than stopping the pipeline) - leading to a complete
pipeline stall?

--
Kieran


> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 5204487684c2..11bf3af66ae6 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -916,6 +916,9 @@ int PipelineHandlerIPU3::registerCameras()
>   */
>  void IPU3CameraData::imguInputBufferReady(Buffer *buffer)
>  {
> +	if (buffer->status() != Buffer::BufferSuccess)
> +		return;
> +
>  	cio2_.output_->queueBuffer(buffer);
>  }
>  
> @@ -946,6 +949,9 @@ void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
>   */
>  void IPU3CameraData::cio2BufferReady(Buffer *buffer)
>  {
> +	if (buffer->status() != Buffer::BufferSuccess)
> +		return;
> +
>  	imgu_->input_->queueBuffer(buffer);
>  }
>  
>

Hi Jacopo,

Thank you for the patch.

On Mon, Jul 15, 2019 at 07:59:34AM +0200, Jacopo Mondi wrote:
> When a video device is stopped all the buffers there queued are
> released and their state is set to failure.

Isn't the state set to BufferCancelled, not BufferError ?

> Currently, on buffer completion, failed buffers are blindly re-queued to
> the ImgU input or CIO2 output devices, preventing them to be re-started
> succesfully in future capture sessions.
> 
> Fix that by inspecting the buffers status and skip re-queueing if
> they're reported as failing. For the ImgU output buffers this is not
> required, as failed buffes should be anyhow delivered to applications in

s/buffes/buffers/

> order to report their failure.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 5204487684c2..11bf3af66ae6 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -916,6 +916,9 @@ int PipelineHandlerIPU3::registerCameras()
>   */
>  void IPU3CameraData::imguInputBufferReady(Buffer *buffer)
>  {
> +	if (buffer->status() != Buffer::BufferSuccess)
> +		return;
> +
>  	cio2_.output_->queueBuffer(buffer);
>  }
>  
> @@ -946,6 +949,9 @@ void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
>   */
>  void IPU3CameraData::cio2BufferReady(Buffer *buffer)
>  {
> +	if (buffer->status() != Buffer::BufferSuccess)
> +		return;
> +

We may need to differentiate between Buffer::BufferCancelled and
Buffer::BufferError. The former clearly needs to return immediately,
while the latter should probably retry one way or another, at least a
finite number of times. Should we already test ==
Buffer::BufferCancelled, and ignore BufferError for now ?

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

>  	imgu_->input_->queueBuffer(buffer);
>  }