[libcamera-devel] [PATCH 09/11] libcamera: ipu3: Add helper for parameter and statistic buffers
Jacopo Mondi
jacopo at jmondi.org
Wed Nov 18 17:52:24 CET 2020
Hi Niklas,
On Thu, Nov 05, 2020 at 01:15:44AM +0100, Niklas Söderlund wrote:
> Add a helper class to aid in associating a parameter and statistic
> buffer with each request queued to the pipeline. The helper helps with
> tracking the state of the extra buffers and in completing the request
> once all extra processing is done.
>
> This change only adds the helper more work is needed to integrate it
> with the pipeline and an IPA.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/pipeline/ipu3/frames.cpp | 164 ++++++++++++++++++++++++
> src/libcamera/pipeline/ipu3/frames.h | 68 ++++++++++
> src/libcamera/pipeline/ipu3/meson.build | 1 +
> 3 files changed, 233 insertions(+)
> create mode 100644 src/libcamera/pipeline/ipu3/frames.cpp
> create mode 100644 src/libcamera/pipeline/ipu3/frames.h
>
> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp
> new file mode 100644
> index 0000000000000000..824844e345e13679
> --- /dev/null
> +++ b/src/libcamera/pipeline/ipu3/frames.cpp
> @@ -0,0 +1,164 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * frames.cpp - Intel IPU3 Frames helper
> + */
> +
> +#include "frames.h"
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/request.h>
> +
> +#include <libcamera/ipa/ipa_interface.h>
> +
> +#include "libcamera/internal/pipeline_handler.h"
> +#include "libcamera/internal/v4l2_videodevice.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPU3)
> +
> +IPU3Frames::IPU3Frames(PipelineHandler *pipe, IPAProxy *ipa)
> + : pipe_(pipe), ipa_(ipa), nextId_(0)
> +{
> +}
> +
> +void IPU3Frames::mapBuffers(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuffers,
> + const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers)
> +{
> + unsigned int ipaBufferId = 1;
> +
> + for (const std::unique_ptr<FrameBuffer> &buffer : paramBuffers) {
> + buffer->setCookie(ipaBufferId++);
> + ipaBuffers_.push_back({ .id = buffer->cookie(),
> + .planes = buffer->planes() });
> + availableParamBuffers_.push(buffer.get());
> + }
> +
> + for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers) {
> + buffer->setCookie(ipaBufferId++);
> + ipaBuffers_.push_back({ .id = buffer->cookie(),
> + .planes = buffer->planes() });
> + availableStatBuffers_.push(buffer.get());
> + }
> +
> + ipa_->mapBuffers(ipaBuffers_);
> +
> + nextId_ = 0;
> + frameInfo_.clear();
> +}
> +
> +void IPU3Frames::unmapBuffers()
> +{
> + availableParamBuffers_ = {};
> + availableStatBuffers_ = {};
> +
> + std::vector<unsigned int> ids;
> + for (IPABuffer &ipabuf : ipaBuffers_)
> + ids.push_back(ipabuf.id);
> +
> + ipa_->unmapBuffers(ids);
> + ipaBuffers_.clear();
> +}
I would do these two operations in the pipeline. Having the helper
calling the IPA is not nice imho and this really does too many
things, set cookies in buffers, map buffers in the IPA, fill the
available buffers in the helper, reset the id counter...
I would make this an:
IPU3Frames::initialize() or reset()
that initializes the available buffers and reset counters and leave
the IPA buffer mapping and cookies initialization to the pipeline handler.
> +
> +IPU3Frames::Info *IPU3Frames::create(Request *request)
> +{
> + unsigned int id = nextId_++;
> +
> + if (availableParamBuffers_.empty()) {
> + LOG(IPU3, Error) << "Parameters buffer underrun";
> + return nullptr;
> + }
> + FrameBuffer *paramBuffer = availableParamBuffers_.front();
> +
> + if (availableStatBuffers_.empty()) {
> + LOG(IPU3, Error) << "Statisitc buffer underrun";
> + return nullptr;
> + }
> + FrameBuffer *statBuffer = availableStatBuffers_.front();
> +
> + availableParamBuffers_.pop();
> + availableStatBuffers_.pop();
> +
> + std::unique_ptr<Info> info = std::make_unique<Info>();
> +
> + info->id = id;
> + info->request = request;
> + info->rawBuffer = nullptr;
> + info->paramBuffer = paramBuffer;
> + info->statBuffer = statBuffer;
> + info->paramFilled = false;
> + info->paramDequeued = false;
> + info->metadataProcessed = false;
> +
> + frameInfo_[id] = std::move(info);
> +
> + return frameInfo_[id].get();
> +}
> +
> +bool IPU3Frames::tryComplete(IPU3Frames::Info *info)
> +{
> + Request *request = info->request;
> +
> + if (request->hasPendingBuffers())
> + return false;
> +
> + if (!info->metadataProcessed)
> + return false;
> +
> + if (!info->paramDequeued)
> + return false;
So, paramDequeued is set when rawOnly, and paramFilled when the IPA is
done, but never really checked for here. Shouldn't this be a single
flag maybe 'paramDone' set in case the ipa is done or the request
contains a raw frame only ?
> +
> + /* Return params and stat buffer for reuse. */
> + availableParamBuffers_.push(info->paramBuffer);
> + availableStatBuffers_.push(info->statBuffer);
> +
> + /* Delete the extended frame information. */
> + frameInfo_.erase(info->id);
> +
> + pipe_->completeRequest(request);
pipe_ is only used here, and calling back into the pipeline handler is
an implicit behaviour I would avoid.
What about the pipeline handler doing:
if (frameInfo_->tryComplete(id))
completeRequest(frameInfo_->find(id)->request)
?
> +
> + return true;
> +}
> +
> +IPU3Frames::Info *IPU3Frames::find(unsigned int id)
> +{
> + const auto &itInfo = frameInfo_.find(id);
> +
> + if (itInfo != frameInfo_.end())
> + return itInfo->second.get();
> +
> + return nullptr;
> +}
> +
> +IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)
> +{
> + for (auto const &itInfo : frameInfo_) {
> + Info *info = itInfo.second.get();
> +
> + for (auto const itBuffers : info->request->buffers())
> + if (itBuffers.second == buffer)
> + return info;
> +
> + if (info->rawBuffer == buffer || info->paramBuffer == buffer ||
> + info->statBuffer == buffer)
> + return info;
> + }
> +
> + return nullptr;
> +}
> +
> +IPU3Frames::Info *IPU3Frames::find(Request *request)
> +{
> + for (auto const &itInfo : frameInfo_) {
> + Info *info = itInfo.second.get();
> +
> + if (info->request == request)
> + return info;
> + }
> +
> + return nullptr;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h
> new file mode 100644
> index 0000000000000000..5c072f8ddbc9660f
> --- /dev/null
> +++ b/src/libcamera/pipeline/ipu3/frames.h
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * frames.h - Intel IPU3 Frames helper
> + */
> +#ifndef __LIBCAMERA_PIPELINE_IPU3_FRAMES_H__
> +#define __LIBCAMERA_PIPELINE_IPU3_FRAMES_H__
> +
> +#include <map>
> +#include <memory>
> +#include <queue>
> +#include <vector>
> +
> +namespace libcamera {
> +
> +class FrameBuffer;
> +class IPAProxy;
> +class PipelineHandler;
> +class Request;
> +class V4L2VideoDevice;
> +struct IPABuffer;
> +
> +class IPU3Frames
> +{
> +public:
> + struct Info {
> + unsigned int id;
> + Request *request;
> +
> + FrameBuffer *rawBuffer;
> + FrameBuffer *paramBuffer;
> + FrameBuffer *statBuffer;
> +
> + bool paramFilled;
> + bool paramDequeued;
> + bool metadataProcessed;
> + };
> +
> + IPU3Frames(PipelineHandler *pipe, IPAProxy *ipa);
> +
> + void mapBuffers(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuffers,
> + const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers);
> + void unmapBuffers();
> +
> + Info *create(Request *request);
> + bool tryComplete(IPU3Frames::Info *info);
> +
> + Info *find(unsigned int id);
> + Info *find(FrameBuffer *buffer);
> + Info *find(Request *request);
> +
> +private:
> + PipelineHandler *pipe_;
> + IPAProxy *ipa_;
> +
> + std::queue<FrameBuffer *> availableParamBuffers_;
> + std::queue<FrameBuffer *> availableStatBuffers_;
> +
> + std::vector<IPABuffer> ipaBuffers_;
> +
> + unsigned int nextId_;
> + std::map<unsigned int, std::unique_ptr<Info>> frameInfo_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_PIPELINE_IPU3_FRAMES_H__ */
> diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build
> index d60e07ae6ccac2bc..a1b0b31ac5bcf864 100644
> --- a/src/libcamera/pipeline/ipu3/meson.build
> +++ b/src/libcamera/pipeline/ipu3/meson.build
> @@ -2,6 +2,7 @@
>
> libcamera_sources += files([
> 'cio2.cpp',
> + 'frames.cpp',
> 'imgu.cpp',
> 'ipu3.cpp',
> ])
> --
> 2.29.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list