[libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: Add method to retrieve Request from Buffer
Niklas Söderlund
niklas.soderlund at ragnatech.se
Fri Apr 5 13:48:35 CEST 2019
Hi Jacopo,
Thanks for your work.
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.
> + 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;
> + }
> + }
> +
> + 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
> --
> 2.21.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list