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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Apr 15, 2019 at 11:25:37AM +0200, Jacopo Mondi wrote:
> On Sun, Apr 14, 2019 at 10:05:16PM +0200, Niklas Söderlund wrote:
> > 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()) {

bufferMap is not a map. You can name this

	for (auto it : 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.
> 
> No, it's not correct, but it opens up  an opportunity for further
> discussions.
> 
> The comment at the beginning of the if() switch I think it's clear,
> and bufferCount is a counter that it is used to pick buffers from the
> internal pool. It's not nice and will have to be changed.
> 
> The fact is that queuing and dequeuing frames from 'output' is mandatory to
> have ImgU operations triggered and you can't get any frame out from
> viewfinder if output has no buffer queued.
> 
> Now, there are three possible configurations:
> 1) output is not part of the configured streams
> 2) output is configured and in all capture requests
> 3) output is configured but not part of all the capture requests
> 
> 1) is easy: we use the internal buffer pool in this case
>    - reserve enough buffers in the output (as many as the ones
>      reserved for the viewfinder)
>    - export them to an internal pool
>    - for every request queue a new buffer from the internal pool to
>      the 'output' capture device
>    - when a buffer completes from viewfinder the request is completed, the
>      buffer from output had completed already and will be discarded as
>      it it not connected to any request
>    - if applications queue a number of request > than the buffer pool
>      size all at once (before enough request completes) we could get
>      into a buffer underrun
> 
> 2) It's easy as well, as we act the same but buffers are mapped to the
>    Stream's pool. All request will include buffers from the 'output'
>    node. In the same way, if applications queue 100 requests before
>    completing any, than what happens I guess depends on the IO mode
>    implemented by the pipeline handler: the request queuing could
>    block on QBUF or if working in non-blocking mode it would return an
>    error. I think this should be standardized and we need to specify
>    how pipeline handlers should behave in such a case (or is it already?)
> 
> 3) It's not easy, as 'output' is configured, but not all requests
>    include it in the list of streams to capture from. In that case
>    (output is not included) we need to queue buffers to it to get
>    anything out from viewfinder and stat. And that's where my ugly
>    bufferCount gets into play: it picks buffers from the internal pool
>    and use them for to trigger ImgU processing.
> 
>    Now we have two problems:
>    a) memory mapping and exporting
>       As 'output' is configured, its buffers have been exported to the
>       stream pool, not to the internal one. Sincerely, I'm not sure
>       why it works at the moment, as the internal pool's buffers
>       should be invalid. And I'm not sure how to handle this neither,
>       one possible solutions could be:
>       a.1) Export always to the internal pool without allocating, as
>            allocation happens when exporting to the Stream's pool,
>            but this mean we have exported the same buffers to two
>            different pools and we alternate them depending on the
>            content of the request... not sure it is desirable.
>    b) buffer underrun
>       This is of less concern to me, as it reduce to the same as 1)
>       as if we have viewfinder complete, it means that output
>       completed as well, so it's safe to assume that if we got a frame
>       from viewfinder, the same has completed on output too.
> 
>    The bufferCount mechanism is an hack, and should be replaced with a
>    dynamic queue of available buffers probably, but that does not
>    solve how to handle allocation and queueing in case 3)
> 
>    How do you think it should be handled?

I think you should allocate extra buffers for the output video node, and
use them as scratch buffers in case a request doesn't contain any output
buffer. Possibly easier said than done though, I haven't checked. A
single scratch buffer will unfortunately not work as it would prevent
queuing multiple requests, so you have to basically double the number of
buffers.

This should really be fixed on the driver side.

> >> +	}
> >>
> >>  	PipelineHandler::queueRequest(camera, request);
> >>

-- 
Regards,

Laurent Pinchart