[libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: Add method to retrieve Request from Buffer
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Apr 8 15:06:58 CEST 2019
Hi Jacopo,
On Mon, Apr 08, 2019 at 10:05:58AM +0200, Jacopo Mondi wrote:
> On Fri, Apr 05, 2019 at 06:45:16PM +0300, Laurent Pinchart wrote:
> > 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.
Probably quite a bit, but I think we should implement it in the IPU3
pipeline handler first, and then refactor the code when a second
pipeline handler will need this (of course if you already have a clear
view of how it will look like, you can also have a go :-)).
> >>> + 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.
>
> >>> +
> >>> + 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