[libcamera-devel] [PATCH v4 06/12] libcamera: ipu3: Queue requests for multiple streams
Jacopo Mondi
jacopo at jmondi.org
Mon Apr 15 11:25:37 CEST 2019
HI Niklas,
On Sun, Apr 14, 2019 at 10:05:16PM +0200, Niklas Söderlund wrote:
> 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.
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?
>
> > + }
> >
> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190415/bdd10558/attachment.sig>
More information about the libcamera-devel
mailing list