[libcamera-devel,v4,24/31] libcamera: ipu3: Queue request for multiple streams

770 diff mbox series

Add support for queue request on the main and secondary outputs of the
ImgU.

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

Hi Jacopo,

Thank you for the patch.

On Wed, Mar 20, 2019 at 05:30:48PM +0100, Jacopo Mondi wrote:
> Add support for queue request on the main and secondary outputs of the
> ImgU.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 46 ++++++++++++++++------------
>  1 file changed, 27 insertions(+), 19 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index ff1e5329c83d..b2df9a4ac922 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -689,38 +689,46 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>  
>  int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
>  {
> +	std::set<Stream *> streams = request->streams();

Maybe

	const std::set<Stream *> &streams = request->streams();

?

>  	IPU3CameraData *data = cameraData(camera);
>  	V4L2Device *viewfinder = data->imgu->viewfinder;
>  	V4L2Device *output = data->imgu->output;
>  	V4L2Device *stat = data->imgu->stat;
> -	Stream *stream = &data->streams_[0];
> +	ImgUDevice *imgu = data->imgu;
>  	Buffer *tmpBuffer;
>  
> -	/*
> -	 * Queue buffer on VF and stat.
> -	 * FIXME: this is an hack!
> -	 */
> -	tmpBuffer = &data->imgu->vfPool.buffers()[tmpBufferCount];
> -	viewfinder->queueBuffer(tmpBuffer);
> +	for (Stream *stream : streams) {
> +		Buffer *buffer = request->findBuffer(stream);

This clearly calls for exposing the stream to buffers map instead of
implementing a streams() method :-)

> +		if (!buffer) {
> +			LOG(IPU3, Error) << "Attempt to queue invalid request";
> +			return -ENOENT;
> +		}
> +
> +		V4L2Device *videoDevice = isOutput(data, stream)
> +					? output : viewfinder;
> +
> +		int ret = videoDevice->queueBuffer(buffer);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (isOutput(data, stream) && !isViewfinderActive(data)) {
> +			tmpBuffer = &imgu->vfPool.buffers()[tmpBufferCount];
> +			viewfinder->queueBuffer(tmpBuffer);
> +		}
>  
> +		if (isViewfinder(data, stream) && !isOutputActive(data)) {
> +			tmpBuffer = &imgu->outputPool.buffers()[tmpBufferCount];
> +			output->queueBuffer(tmpBuffer);
> +		}

I think the code would be clearer if you moved it out of the loop. How
about making a copy of the set of active streams, removing streams
present in the request from the copy, and then loop over the remaining
streams in the copy to queue the dummy buffers ?

> +	}
> +
> +	/* Queue buffer on stat unconditionally. */
>  	tmpBuffer = &data->imgu->statPool.buffers()[tmpBufferCount];
>  	stat->queueBuffer(tmpBuffer);
>  
>  	tmpBufferCount++;
>  	tmpBufferCount %= IPU3_IMGU_BUFFER_COUNT;

This won't work as expected. If you have, let's say, 4 buffers for each
stream, an application could queue 8 requests consecutively (4 requests
with the output stream only and 4 requests with the viewfinder stream
only). With 4 stats buffers, you'll have a problem.

> -	/* 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;
> -	}
> -
> -	int ret = output->queueBuffer(buffer);
> -	if (ret < 0)
> -		return ret;
> -
>  	PipelineHandler::queueRequest(camera, request);
>  
>  	return 0;