[libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: Add method to retrieve Request from Buffer
Jacopo Mondi
jacopo at jmondi.org
Mon Apr 8 10:05:58 CEST 2019
Hi Laurent, Niklas,
On Fri, Apr 05, 2019 at 06:45:16PM +0300, Laurent Pinchart wrote:
> Hello Jacopo,
>
> Thank you for the patch.
>
> On Fri, Apr 05, 2019 at 01:48:35PM +0200, Niklas Söderlund wrote:
> > On 2019-04-03 17:07:34 +0200, Jacopo Mondi wrote:
> > > Add a method to CameraData base class to retrieve a pointer to the
> > > Request that contains a given buffer. Intended users are buffer
> > > completion slots that needs to associate a Request to a just completed
> > > Buffer.
> > >
> > > In preparation to support multiple requests from different streams,
> > > update all the pipeline handler implementations to use this method
> > > instead of using the last queued request.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > include/libcamera/request.h | 2 ++
> > > src/libcamera/include/pipeline_handler.h | 3 +++
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++-
> > > src/libcamera/pipeline/uvcvideo.cpp | 3 ++-
> > > src/libcamera/pipeline/vimc.cpp | 3 ++-
> > > src/libcamera/pipeline_handler.cpp | 29 ++++++++++++++++++++++++
> > > src/libcamera/request.cpp | 7 ++++++
> > > 7 files changed, 47 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > > index 5ac4d20d1d9f..8f5892fd3111 100644
> > > --- a/include/libcamera/request.h
> > > +++ b/include/libcamera/request.h
> > > @@ -38,6 +38,8 @@ public:
> > >
> > > const std::list<Stream *> streams() const;
> > >
> > > + const std::unordered_set<Buffer *> &pending() const { return pending_; }
> > > +
> > > Status status() const { return status_; }
> > >
> > > private:
> > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > > index 920b57609470..6cdadcbdc3ea 100644
> > > --- a/src/libcamera/include/pipeline_handler.h
> > > +++ b/src/libcamera/include/pipeline_handler.h
> > > @@ -39,6 +39,9 @@ public:
> > > PipelineHandler *pipe_;
> > > std::list<Request *> queuedRequests_;
> > >
> > > +protected:
> > > + Request *requestFromBuffer(Buffer *buffer);
> > > +
> > > private:
> > > CameraData(const CameraData &) = delete;
> > > CameraData &operator=(const CameraData &) = delete;
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 8c67cf985d1e..17e3e8677e28 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -801,7 +801,8 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)
> > > */
> > > void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
> > > {
> > > - Request *request = queuedRequests_.front();
> > > + Request *request = requestFromBuffer(buffer);
> > > + ASSERT(request);
> > >
> > > pipe_->completeBuffer(camera_, request, buffer);
> > > pipe_->completeRequest(camera_, request);
> > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > > index 128f0c49dba3..d571b8b4ea83 100644
> > > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > > @@ -226,7 +226,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> > >
> > > void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer)
> > > {
> > > - Request *request = queuedRequests_.front();
> > > + Request *request = requestFromBuffer(buffer);
> >
> > This make me feel uneasy :-(
> >
> > We talked in the past about how to handle request completion. The idea
> > from the beginning was conceived with the reequest API in mind, which
> > IIRC guarantees that requests are completed in the same order they are
> > queued. While V4L2 have no such guarantees for for buffers. This was one
> > of the issues I hit with my single stream old UVC camera where the first
> > buffer queue would contains errors and the uvcvideo driver would not
> > return it to user-space and move on to the second buffer queued and
> > libcamera fail as buffers are dequeued out of order (from it's point of
> > view).
> >
> > I understand what you try to do in this patch and I agree it might be
> > needed. But I fear we first need to discuss how we should handle V4L2
> > buffers being dequeued out of order. As this change would hide this
> > behavior while it's intent is indeed something different.
>
> I share Niklas' concern here. Buffers may complete out of order at the
> V4L2 level, but we must ensure that requests complete in order. Niklas,
> have you thought about how you would like to solve this ?
>
> Thinking out loud, I'm thinking about the following.
>
> - When a buffer is dequeued, find the corresponding request.
> - If the buffer doesn't belong to any request this is an error, but
> ASSERT is a bit too harsh, we should notify the application instead to
> allow a clean shutdown.
> - Otherwise, mark the buffer as complete in the request (remove it from
> the pending list).
> - Then, in the pipeline handler, if no more buffer is pending, and if
> all other data that need to be stored in the request are available,
> mark the request as complete. We don't have any additional data beyond
> buffers yet, but the point here is that this decision should be under
> control of the pipeline handler to make this possible.
> - If the request just marked as complete is not the first in the queue,
> leave and there.
> - Otherwise, notify the application of request completion for all
> complete requests starting at the beginning of the queue.
>
I wonder how much of this could be moved to the PipelineHandler base class
and make it transparent for pipeline handler implementations.
> > > + ASSERT(request);
> > >
> > > pipe_->completeBuffer(camera_, request, buffer);
> > > pipe_->completeRequest(camera_, request);
> > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > > index 6735940799d8..e83416effad8 100644
> > > --- a/src/libcamera/pipeline/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc.cpp
> > > @@ -223,7 +223,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> > >
> > > void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer)
> > > {
> > > - Request *request = queuedRequests_.front();
> > > + Request *request = requestFromBuffer(buffer);
> > > + ASSERT(request);
> > >
> > > pipe_->completeBuffer(camera_, request, buffer);
> > > pipe_->completeRequest(camera_, request);
> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > > index 9a8a4fde57e6..830ff354ed3e 100644
> > > --- a/src/libcamera/pipeline_handler.cpp
> > > +++ b/src/libcamera/pipeline_handler.cpp
> > > @@ -86,6 +86,35 @@ LOG_DEFINE_CATEGORY(Pipeline)
> > > * PipelineHandler::completeRequest()
> > > */
> > >
> > > +/**
> > > + * \brief Retrieve the pending request that contains \a buffer
> > > + * \param[in] buffer The buffer contained in the returned request
> > > + *
> > > + * Return the request that contains \a buffer, or nullptr if no such request
> > > + * is found. The intended callers of this method are buffer completion slots
> > > + * implemented in CameraData sub-classes which needs to associated a request
> > > + * to the just completed buffer. It is up to the caller of this function to
> > > + * deal with the case the buffer does not belong to any previously queued
> > > + * request or the request has already completed, possibly because of a
> > > + * duplicated buffer completion notification. This is generally considered
> > > + * a fatal error, and callers are expected to assert the validity of the
> > > + * returned request.
> > > + *
> > > + * \return A pointer to the pending Request that contains the Buffer \a buffer,
> > > + * or nullptr if no such request is found
> > > + */
> > > +Request *CameraData::requestFromBuffer(Buffer *buffer)
> > > +{
> > > + for (Request *req : queuedRequests_) {
> > > + for (Buffer *b : req->pending()) {
> > > + if (b == buffer)
> > > + return req;
> > > + }
> > > + }
>
> From an implementation point of view we may want to add a Request
> pointer to the Buffer class to make this more efficient.
>
Indeed, as Niklas pointed out, there are enough loops on the Stream
and Buffers containers around.
Thanks
j
> > > +
> > > + return nullptr;
> > > +}
> > > +
> > > /**
> > > * \class PipelineHandler
> > > * \brief Create and manage cameras based on a set of media devices
> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > > index 3a7841fb2bb3..c555752b2c0b 100644
> > > --- a/src/libcamera/request.cpp
> > > +++ b/src/libcamera/request.cpp
> > > @@ -108,6 +108,13 @@ const std::list<Stream *> Request::streams() const
> > > return streams;
> > > }
> > >
> > > +/**
> > > + * \fn Request::pending()
> > > + * \brief Retrieve the list of not-yet-completed buffers
> > > + *
> > > + * \return The set of pending buffers
> > > + */
> > > +
> > > /**
> > > * \fn Request::status()
> > > * \brief Retrieve the request completion status
>
> --
> Regards,
>
> Laurent Pinchart
-------------- 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/20190408/acfce9f2/attachment.sig>
More information about the libcamera-devel
mailing list