[libcamera-devel] [PATCH v4 06/12] libcamera: ipu3: Queue requests for multiple streams

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2019-04-09 21:25:42 +0200, Jacopo Mondi wrote:
> Add support for queueing requests for multiple streams in the IPU3
> pipeline handler class. The output video node should be queued with
> buffers even if not part of the requested streams.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 46 +++++++++++++++++++++-------
>  1 file changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index f5d768f9f87f..bb8d4ce644ca 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -146,6 +146,7 @@ public:
>  	std::string name_;
>  	ImgUDevice::ImgUOutput *device_;
>  	const StreamConfiguration *cfg_;
> +	unsigned int bufferCount;

I do not understand the full usage of bufferCount, I think see bellow.

>  };
>  
>  class PipelineHandlerIPU3 : public PipelineHandler
> @@ -473,6 +474,9 @@ int PipelineHandlerIPU3::start(Camera *camera)
>  	ImgUDevice *imgu = data->imgu_;
>  	int ret;
>  
> +	if (!data->outStream_.active_)
> +		data->outStream_.bufferCount = 0;
> +
>  	/*
>  	 * Start the ImgU video devices, buffers will be queued to the
>  	 * ImgU output and viewfinder when requests will be queued.
> @@ -513,20 +517,40 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>  int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	V4L2Device *output = data->imgu_->output_.dev;
> -	IPU3Stream *stream = &data->outStream_;
> +	IPU3Stream *output = &data->outStream_;
> +	bool outputRequested = false;
>  
> -	/* Queue a buffer to the ImgU output for capture. */
> -	Buffer *buffer = request->findBuffer(stream);
> -	if (!buffer) {
> -		LOG(IPU3, Error)
> -			<< "Attempt to queue request with invalid stream";
> -		return -ENOENT;
> +	for (auto const &bufferMap : request->bufferMap()) {
> +		IPU3Stream *stream = static_cast<IPU3Stream *>(bufferMap.first);
> +		Buffer *buffer = bufferMap.second;
> +
> +		int ret = stream->device_->dev->queueBuffer(buffer);
> +		if (ret)
> +			continue;
> +
> +		if (stream == output)
> +			outputRequested = true;
>  	}
>  
> -	int ret = output->queueBuffer(buffer);
> -	if (ret < 0)
> -		return ret;
> +	/*
> +	 * The output node should be queued with buffers even if not part
> +	 * of the request, otherwise the ImgU stalls.
> +	 *
> +	 * Use buffers from the stream's pool or the internal pool depending
> +	 * if the output stream has been configured or not.
> +	 */
> +	if (!outputRequested) {
> +		BufferPool *pool = output->active_ ? &output->bufferPool()
> +						   : output->device_->pool;
> +		Buffer *buffer = &pool->buffers()[output->bufferCount];
> +		unsigned int numBuffers = pool->count();
> +
> +		int ret = output->device_->dev->queueBuffer(buffer);
> +		if (ret)
> +			return ret;
> +
> +		output->bufferCount = (output->bufferCount + 1) % numBuffers;

This looks worthy of a comment to me, I can't really tell what's going 
on. You cyclically pick buffers from the pool's buffers. I'm the first 
one to admit I'm no expert in the Buffer part of libcamera so I have to 
ask, is there any grantee the buffer picked is free? I'm thinking about 
under-run situations. Maybe it's just my inexperience that is showing 
and this is correct.

> +	}
>  
>  	PipelineHandler::queueRequest(camera, request);
>  
> -- 
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund