[libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: Add method to retrieve Request from Buffer
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Apr 5 17:45:16 CEST 2019
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.
> > + 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.
> > +
> > + 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
More information about the libcamera-devel
mailing list