Message ID | 20210204162943.268517-11-niklas.soderlund@ragnatech.se |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
On Thu, Feb 04, 2021 at 05:29:42PM +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@ragnatech.se> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > --- > * Changes since v1 > - Move mapping of buffers to IPA to the pipeline handler and a separate > patch. > - Move all pipeline interactions out of helper. > > * Changes since v2 > - Add todo to remove dynamic memory allocation. > - Remove unused find(Request *request). > --- > src/libcamera/pipeline/ipu3/frames.cpp | 129 ++++++++++++++++++++++++ > src/libcamera/pipeline/ipu3/frames.h | 61 +++++++++++ > src/libcamera/pipeline/ipu3/meson.build | 1 + > 3 files changed, 191 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..b7f7f0a6575714d9 > --- /dev/null > +++ b/src/libcamera/pipeline/ipu3/frames.cpp > @@ -0,0 +1,129 @@ > +/* 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/internal/pipeline_handler.h" > +#include "libcamera/internal/v4l2_videodevice.h" > + > +namespace libcamera { > + > +LOG_DECLARE_CATEGORY(IPU3) > + > +IPU3Frames::IPU3Frames() > + : nextId_(0) > +{ > +} > + > +void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuffers, > + const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers) > +{ > + for (const std::unique_ptr<FrameBuffer> &buffer : paramBuffers) > + availableParamBuffers_.push(buffer.get()); > + > + for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers) > + availableStatBuffers_.push(buffer.get()); > + > + nextId_ = 0; > + frameInfo_.clear(); > +} > + > +void IPU3Frames::clear() > +{ > + availableParamBuffers_ = {}; > + availableStatBuffers_ = {}; > +} > + > +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(); > + > + /* \todo Remove the dynamic allocation of Info */ > + 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->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; > + > + /* Return params and stat buffer for reuse. */ > + availableParamBuffers_.push(info->paramBuffer); > + availableStatBuffers_.push(info->statBuffer); > + > + /* Delete the extended frame information. */ > + frameInfo_.erase(info->id); > + > + 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; > +} > + > +} /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h > new file mode 100644 > index 0000000000000000..93379d20722d65fb > --- /dev/null > +++ b/src/libcamera/pipeline/ipu3/frames.h > @@ -0,0 +1,61 @@ > +/* 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 paramDequeued; > + bool metadataProcessed; > + }; > + > + IPU3Frames(); > + > + void init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuffers, > + const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers); > + void clear(); > + > + Info *create(Request *request); > + bool tryComplete(IPU3Frames::Info *info); You can write this bool tryComplete(Info *info); Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> One idea for future improvements, especially if we want to share this helper between pipeline handlers, would be to rework the API in a way that would avoid the possibility of use-after-free if the pipeline handler tries to access the info pointer after a call to tryComplete() that returns true. > + > + Info *find(unsigned int id); > + Info *find(FrameBuffer *buffer); > + > +private: > + std::queue<FrameBuffer *> availableParamBuffers_; > + std::queue<FrameBuffer *> availableStatBuffers_; > + > + 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', > ])
Hi Laurent, Thanks for your feedback. On 2021-02-04 19:22:43 +0200, Laurent Pinchart wrote: > On Thu, Feb 04, 2021 at 05:29:42PM +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@ragnatech.se> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > * Changes since v1 > > - Move mapping of buffers to IPA to the pipeline handler and a separate > > patch. > > - Move all pipeline interactions out of helper. > > > > * Changes since v2 > > - Add todo to remove dynamic memory allocation. > > - Remove unused find(Request *request). > > --- > > src/libcamera/pipeline/ipu3/frames.cpp | 129 ++++++++++++++++++++++++ > > src/libcamera/pipeline/ipu3/frames.h | 61 +++++++++++ > > src/libcamera/pipeline/ipu3/meson.build | 1 + > > 3 files changed, 191 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..b7f7f0a6575714d9 > > --- /dev/null > > +++ b/src/libcamera/pipeline/ipu3/frames.cpp > > @@ -0,0 +1,129 @@ > > +/* 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/internal/pipeline_handler.h" > > +#include "libcamera/internal/v4l2_videodevice.h" > > + > > +namespace libcamera { > > + > > +LOG_DECLARE_CATEGORY(IPU3) > > + > > +IPU3Frames::IPU3Frames() > > + : nextId_(0) > > +{ > > +} > > + > > +void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuffers, > > + const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers) > > +{ > > + for (const std::unique_ptr<FrameBuffer> &buffer : paramBuffers) > > + availableParamBuffers_.push(buffer.get()); > > + > > + for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers) > > + availableStatBuffers_.push(buffer.get()); > > + > > + nextId_ = 0; > > + frameInfo_.clear(); > > +} > > + > > +void IPU3Frames::clear() > > +{ > > + availableParamBuffers_ = {}; > > + availableStatBuffers_ = {}; > > +} > > + > > +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(); > > + > > + /* \todo Remove the dynamic allocation of Info */ > > + 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->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; > > + > > + /* Return params and stat buffer for reuse. */ > > + availableParamBuffers_.push(info->paramBuffer); > > + availableStatBuffers_.push(info->statBuffer); > > + > > + /* Delete the extended frame information. */ > > + frameInfo_.erase(info->id); > > + > > + 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; > > +} > > + > > +} /* namespace libcamera */ > > diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h > > new file mode 100644 > > index 0000000000000000..93379d20722d65fb > > --- /dev/null > > +++ b/src/libcamera/pipeline/ipu3/frames.h > > @@ -0,0 +1,61 @@ > > +/* 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 paramDequeued; > > + bool metadataProcessed; > > + }; > > + > > + IPU3Frames(); > > + > > + void init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuffers, > > + const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers); > > + void clear(); > > + > > + Info *create(Request *request); > > + bool tryComplete(IPU3Frames::Info *info); > > You can write this > > bool tryComplete(Info *info); > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks. > > One idea for future improvements, especially if we want to share this > helper between pipeline handlers, would be to rework the API in a way > that would avoid the possibility of use-after-free if the pipeline > handler tries to access the info pointer after a call to tryComplete() > that returns true. Yes, that is a good idea. > > > + > > + Info *find(unsigned int id); > > + Info *find(FrameBuffer *buffer); > > + > > +private: > > + std::queue<FrameBuffer *> availableParamBuffers_; > > + std::queue<FrameBuffer *> availableStatBuffers_; > > + > > + 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', > > ]) > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp new file mode 100644 index 0000000000000000..b7f7f0a6575714d9 --- /dev/null +++ b/src/libcamera/pipeline/ipu3/frames.cpp @@ -0,0 +1,129 @@ +/* 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/internal/pipeline_handler.h" +#include "libcamera/internal/v4l2_videodevice.h" + +namespace libcamera { + +LOG_DECLARE_CATEGORY(IPU3) + +IPU3Frames::IPU3Frames() + : nextId_(0) +{ +} + +void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuffers, + const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers) +{ + for (const std::unique_ptr<FrameBuffer> &buffer : paramBuffers) + availableParamBuffers_.push(buffer.get()); + + for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers) + availableStatBuffers_.push(buffer.get()); + + nextId_ = 0; + frameInfo_.clear(); +} + +void IPU3Frames::clear() +{ + availableParamBuffers_ = {}; + availableStatBuffers_ = {}; +} + +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(); + + /* \todo Remove the dynamic allocation of Info */ + 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->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; + + /* Return params and stat buffer for reuse. */ + availableParamBuffers_.push(info->paramBuffer); + availableStatBuffers_.push(info->statBuffer); + + /* Delete the extended frame information. */ + frameInfo_.erase(info->id); + + 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; +} + +} /* namespace libcamera */ diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h new file mode 100644 index 0000000000000000..93379d20722d65fb --- /dev/null +++ b/src/libcamera/pipeline/ipu3/frames.h @@ -0,0 +1,61 @@ +/* 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 paramDequeued; + bool metadataProcessed; + }; + + IPU3Frames(); + + void init(const std::vector<std::unique_ptr<FrameBuffer>> ¶mBuffers, + const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers); + void clear(); + + Info *create(Request *request); + bool tryComplete(IPU3Frames::Info *info); + + Info *find(unsigned int id); + Info *find(FrameBuffer *buffer); + +private: + std::queue<FrameBuffer *> availableParamBuffers_; + std::queue<FrameBuffer *> availableStatBuffers_; + + 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', ])