[libcamera-devel] [PATCH 1/3] ipu3: Remove the usage of SharedItemPool
Umang Jain
umang.jain at ideasonboard.com
Tue Oct 5 09:03:22 CEST 2021
Hi Han-Lin,
Thank you for the patch.
> From: hanlinchen<hanlinchen at google.com>
>
> The SharedItemPool was migrated from Chrome OS to maintain RGBS and AF
> grids in IPAIPU3Stats. The orginal reason is to reserve the validness
> unitl the grids are processed in a different thread. However, it leads
Ah, I think we were not aware of any thread processing in statistics at
that point. Yes, maintaining a buffer pool would have made more sense in
that case.
> to a subtle bug which recycles the buffer before sending into AIQ
> libraries. Since the IPU3 IPA (Unlike Chrome OS) process IPAIPU3Stats
> in a single thread, It would be simpler to maintain the grids as the
> members of IPAIPU3Stats, and remove the usage of SharedItemPool.
Agreed.
>
> Signed-off-by: Han-Lin Chen<hanlinchen at google.com>
> ---
> stats/ipa_ipu3_stats.cpp | 145 ++++++-------------------------------
> stats/ipa_ipu3_stats.h | 41 +++++++----
> stats/meson.build | 1 -
> stats/shared_item_pool.cpp | 129 ---------------------------------
> stats/shared_item_pool.h | 114 -----------------------------
> 5 files changed, 47 insertions(+), 383 deletions(-)
> delete mode 100644 stats/shared_item_pool.cpp
> delete mode 100644 stats/shared_item_pool.h
>
> diff --git a/stats/ipa_ipu3_stats.cpp b/stats/ipa_ipu3_stats.cpp
> index c14bd7e..6697c9b 100644
> --- a/stats/ipa_ipu3_stats.cpp
> +++ b/stats/ipa_ipu3_stats.cpp
> @@ -15,30 +15,22 @@ namespacelibcamera::ipa::ipu3 {
>
> LOG_DEFINE_CATEGORY(IPAIPU3Stats)
>
> -IPAIPU3Stats::IPAIPU3Stats()
> -{
> - aiqStatsInputParams_ = {};
> -
> - /* \todo: Is this fine here or we need separate helper? */
> - rgbsGridBuffPool_ =std::make_shared<SharedItemPool<ia_aiq_rgbs_grid>>("RgbsGridBuffPool");
> - afFilterBuffPool_ =std::make_shared<SharedItemPool<ia_aiq_af_grid>>("AfFilterBuffPool");
> -#define PUBLIC_STATS_POOL_SIZE 9 /* comes from CrOS */
> - int ret = allocateStatBufferPools(PUBLIC_STATS_POOL_SIZE);
> - if (ret < 0)
> - LOG(IPAIPU3Stats, Error) << "Failed to allocate stats grid buffers";
> -}
> -
> -IPAIPU3Stats::~IPAIPU3Stats()
> -{
> - freeStatBufferPools();
> - rgbsGridBuffPool_.reset();
> - afFilterBuffPool_.reset();
> -}
> -
> ia_aiq_statistics_input_params *
> IPAIPU3Stats::getInputStatsParams(int frame,aiq::AiqResults *results,
> const ipu3_uapi_stats_3a *stats)
> {
> + IPU3AllStats::ipu3_stats_all_stats outStats = {};
> + IPU3AllStats::ipu3_stats_get_3a(&outStats, stats);
> +
> + rgbsGrid_.blocks_ptr = rgbsGridBlock_;
> +
> + afGrid_.filter_response_1 = filterResponse1_;
> + afGrid_.filter_response_2 = filterResponse2_;
> +
> + IPU3AllStats::intel_skycam_statistics_convert(
> + outStats.ia_css_4a_statistics, &rgbsGrid_, &afGrid_);
> +
> + aiqStatsInputParams_ = {};
> aiqStatsInputParams_.frame_id = frame;
> aiqStatsInputParams_.frame_ae_parameters = results->ae();
> aiqStatsInputParams_.frame_af_parameters = results->af();
> @@ -47,117 +39,24 @@IPAIPU3Stats::getInputStatsParams(int frame,aiq::AiqResults *results,
> aiqStatsInputParams_.frame_sa_parameters = results->sa();
> aiqStatsInputParams_.camera_orientation = ia_aiq_camera_orientation_unknown;
>
> - IPU3AllStats::ipu3_stats_all_stats outStats;
> - memset(&outStats, 0, sizeof(IPU3AllStats::ipu3_stats_all_stats));
> - IPU3AllStats::ipu3_stats_get_3a(&outStats, stats);
> -
> - std::shared_ptr<ia_aiq_rgbs_grid> rgbsGrid = nullptr;
> - std::shared_ptr<ia_aiq_af_grid> afGrid = nullptr;
> - int ret = afFilterBuffPool_->acquireItem(afGrid);
> - ret |= rgbsGridBuffPool_->acquireItem(rgbsGrid);
> - if (ret != 0 || afGrid.get() == nullptr || rgbsGrid.get() == nullptr) {
> - LOG(IPAIPU3Stats, Error) << "Failed to acquire 3A buffers from pools";
> - return nullptr;
> - }
> -
> - IPU3AllStats::intel_skycam_statistics_convert(outStats.ia_css_4a_statistics,
> - rgbsGrid.get(), afGrid.get());
> -
> - const ia_aiq_rgbs_grid *rgbsGridPtr = rgbsGrid.get();
> - const ia_aiq_af_grid *afGridPtr = afGrid.get();
> -
> + rgbsGridPtr_ = &rgbsGrid_;
> aiqStatsInputParams_.num_rgbs_grids = 1;
> - aiqStatsInputParams_.rgbs_grids = &rgbsGridPtr;
> + aiqStatsInputParams_.rgbs_grids = &rgbsGridPtr_;
> +
> + afGridPtr_ = &afGrid_;
> aiqStatsInputParams_.num_af_grids = 1;
> - aiqStatsInputParams_.af_grids = &afGridPtr;
> + aiqStatsInputParams_.af_grids = &afGridPtr_;
>
> aiqStatsInputParams_.hdr_rgbs_grid = nullptr;
> aiqStatsInputParams_.depth_grids = nullptr;
>
> - return &aiqStatsInputParams_;
> -}
> + aiqStatsInputParams_.num_external_histograms = 0;
> + aiqStatsInputParams_.external_histograms = nullptr;
We were not setting this earlier, so +1 on adding them
>
> -intIPAIPU3Stats::allocateStatBufferPools(int numBufs)
> -{
> - int ret = afFilterBuffPool_->init(numBufs);
> - ret |= rgbsGridBuffPool_->init(numBufs);
> - if (ret != 0) {
> - LOG(IPAIPU3Stats, Error) << "Failed to initialize 3A statistics pools";
> - freeStatBufferPools();
> - return -ENOMEM;
> - }
> -#define IPU3_MAX_STATISTICS_WIDTH 80
> -#define IPU3_MAX_STATISTICS_HEIGHT 60
> - int maxGridSize = IPU3_MAX_STATISTICS_WIDTH * IPU3_MAX_STATISTICS_HEIGHT;
> - std::shared_ptr<ia_aiq_rgbs_grid> rgbsGrid = nullptr;
> - std::shared_ptr<ia_aiq_af_grid> afGrid = nullptr;
> -
> - for (int allocated = 0; allocated < numBufs; allocated++) {
> - ret = afFilterBuffPool_->acquireItem(afGrid);
> - ret |= rgbsGridBuffPool_->acquireItem(rgbsGrid);
> -
> - if (ret != 0 || afGrid.get() == nullptr ||
> - rgbsGrid.get() == nullptr) {
> - LOG(IPAIPU3Stats, Error) << "Failed to acquire memory from pools";
> - freeStatBufferPools();
> - return -ENOMEM;
> - }
> -
> - rgbsGrid->blocks_ptr = new rgbs_grid_block[maxGridSize];
> - rgbsGrid->grid_height = 0;
> - rgbsGrid->grid_width = 0;
> -
> - afGrid->filter_response_1 = new int[maxGridSize];
> - afGrid->filter_response_2 = new int[maxGridSize];
> - afGrid->block_height = 0;
> - afGrid->block_width = 0;
> - afGrid->grid_height = 0;
> - afGrid->grid_width = 0;
> - }
> -
> - return 0;
> -}
> + /* \todo: Fill the face state after the integration of face detection. */
> + aiqStatsInputParams_.faces = nullptr;
this too, I like to see more and more fields being set,
>
> -voidIPAIPU3Stats::freeStatBufferPools()
> -{
> - if (!afFilterBuffPool_->isFull() || !rgbsGridBuffPool_->isFull()) {
> - /* We will leak if we errored out in allocateStatBufferPools*/
> - if (!afFilterBuffPool_->isFull())
> - LOG(IPAIPU3Stats, Warning) << "AfFilterBuffPool is leaking";
> - if (!rgbsGridBuffPool_->isFull())
> - LOG(IPAIPU3Stats, Warning) << "RgbsGridBuffPool is leaking";
> - }
> -
> - int ret;
> - size_t availableItems = afFilterBuffPool_->availableItems();
> - std::shared_ptr<ia_aiq_af_grid> afGrid = nullptr;
> - for (size_t i = 0; i < availableItems; i++) {
> - ret = afFilterBuffPool_->acquireItem(afGrid);
> - if (ret == 0 && afGrid.get() != nullptr) {
> - delete[] afGrid->filter_response_1;
> - afGrid->filter_response_1 = nullptr;
> - delete[] afGrid->filter_response_2;
> - afGrid->filter_response_2 = nullptr;
> - } else {
> - LOG(IPAIPU3Stats, Warning)
> - << "Could not acquire AF filter response "
> - << i << "for deletion - leak?";
> - }
> - }
> -
> - availableItems = rgbsGridBuffPool_->availableItems();
> - std::shared_ptr<ia_aiq_rgbs_grid> rgbsGrid = nullptr;
> - for (size_t i = 0; i < availableItems; i++) {
> - ret = rgbsGridBuffPool_->acquireItem(rgbsGrid);
> - if (ret == 0 && rgbsGrid.get() != nullptr) {
> - delete[] rgbsGrid->blocks_ptr;
> - rgbsGrid->blocks_ptr = nullptr;
> - } else {
> - LOG(IPAIPU3Stats, Warning)
> - << "Could not acquire RGBS grid " << i
> - << "for deletion - leak?";
> - }
> - }
> + return &aiqStatsInputParams_;
> }
>
> } /* namespacelibcamera::ipa::ipu3 */
> diff --git a/stats/ipa_ipu3_stats.h b/stats/ipa_ipu3_stats.h
> index 4320024..5f59558 100644
> --- a/stats/ipa_ipu3_stats.h
> +++ b/stats/ipa_ipu3_stats.h
> @@ -5,38 +5,47 @@
> * IPAIPU3Stats.cpp: Generate statistics in IA AIQ consumable format.
> */
>
> -#include "aiq/aiq_results.h"
> -
> #ifndef IPA_IPU3_STATS_H
> #define IPA_IPU3_STATS_H
>
> -#include <ia_imaging/ia_aiq_types.h>
> #include <linux/intel-ipu3.h>
> -
> -#include "shared_item_pool.h"
> +#include "aiq/aiq_results.h"
>
> namespacelibcamera::ipa::ipu3 {
>
> +constexpr uint32_t IPU3MaxStatisticsWidth = 80;
> +constexpr uint32_t IPU3MaxStatisticsHeight = 60;
> +constexpr uint32_t IPU3MaxStatisticsGridSize =
> + IPU3MaxStatisticsWidth * IPU3MaxStatisticsHeight;
> +
Are these max width/height same as ia_aiq_init() call when initializing
the library? I think so, probably it makes sense to centralize them
somewhere.
The patch makes sense to me so,
Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
Thank you for your work Han-Lin.
> struct AiqResults;
>
> class IPAIPU3Stats
> {
> public:
> - IPAIPU3Stats();
> - ~IPAIPU3Stats();
> + IPAIPU3Stats() = default;
> + ~IPAIPU3Stats() = default;
>
> - ia_aiq_statistics_input_params *
> - getInputStatsParams(int frame,
> - aiq::AiqResults *results,
> - const ipu3_uapi_stats_3a *stats);
> + ia_aiq_statistics_input_params* getInputStatsParams(
> + int frame,
> + aiq::AiqResults *results,
> + const ipu3_uapi_stats_3a *stats);
>
> private:
> - void freeStatBufferPools();
> - int allocateStatBufferPools(int numBufs);
> + ia_aiq_statistics_input_params aiqStatsInputParams_ = {};
> +
> + /*!< ia_aiq_statistics_input_params pointer contents */
> + const ia_aiq_rgbs_grid* rgbsGridPtr_ = nullptr;
> + const ia_aiq_af_grid* afGridPtr_ = nullptr;
> + ia_aiq_rgbs_grid rgbsGrid_ = {};
> + ia_aiq_af_grid afGrid_ = {};
> +
> + /*!< ia_aiq_rgbs_grid pointer contents */
> + rgbs_grid_block rgbsGridBlock_[IPU3MaxStatisticsGridSize] = {};
>
> - ia_aiq_statistics_input_params aiqStatsInputParams_;
> - std::shared_ptr<SharedItemPool<ia_aiq_af_grid>> afFilterBuffPool_;
> - std::shared_ptr<SharedItemPool<ia_aiq_rgbs_grid>> rgbsGridBuffPool_;
> + /*!< ia_aiq_af_grid pointer contents */
> + int filterResponse1_[IPU3MaxStatisticsGridSize] = {};
> + int filterResponse2_[IPU3MaxStatisticsGridSize] = {};
> };
>
> } /* namespacelibcamera::ipa::ipu3 */
> diff --git a/stats/meson.build b/stats/meson.build
> index 74ce657..e28a6b9 100644
> --- a/stats/meson.build
> +++ b/stats/meson.build
> @@ -3,6 +3,5 @@
> ipu3_ipa_files += files([
> 'ipa_ipu3_stats.cpp',
> 'ipu3_all_stats.cpp',
> - 'shared_item_pool.cpp',
> ])
>
> diff --git a/stats/shared_item_pool.cpp b/stats/shared_item_pool.cpp
> deleted file mode 100644
> index 326a400..0000000
> --- a/stats/shared_item_pool.cpp
> +++ /dev/null
> @@ -1,129 +0,0 @@
> -/* SPDX-License-Identifier: Apache-2.0 */
> -/*
> - * Copyright (C) 2014-2018 Intel Corporation
> - *
> - * This implementation is highly derived from ChromeOS:
> - * platform2/camera/hal/intel/ipu3/common/SharedItemPool.cpp
> - */
> -
> -#include <ia_imaging/ia_aiq_types.h>
> -
> -#include <libcamera/base/log.h>
> -
> -#include "shared_item_pool.h"
> -
> -namespace libcamera {
> -
> -LOG_DEFINE_CATEGORY(SharedItemPool)
> -
> -template<class ItemType>
> -SharedItemPool<ItemType>::SharedItemPool(const char *name)
> - : allocated_(nullptr), capacity_(0), deleter_(this), poolName_(name),
> - resetter_(nullptr)
> -{
> -}
> -
> -template<class ItemType>
> -SharedItemPool<ItemType>::~SharedItemPool()
> -{
> - deInit();
> -}
> -
> -template<class ItemType>
> -int SharedItemPool<ItemType>::init(int32_t capacity, void (*resetter)(ItemType *))
> -{
> - if (capacity_ != 0) {
> - LOG(SharedItemPool, Error) << "Pool initialized already";
> - return -ENOSYS;
> - }
> - std::lock_guard<std::mutex> l(mutex_);
> - resetter_ = resetter;
> - capacity_ = capacity;
> - available_.reserve(capacity);
> - allocated_ = new ItemType[capacity];
> -
> - for (int32_t i = 0; i < capacity; i++)
> - available_.push_back(&allocated_[i]);
> -
> - LOG(SharedItemPool, Debug) << "Shared pool " << poolName_ << "init with " << capacity << " items";
> -
> - return 0;
> -}
> -
> -template<class ItemType>
> -bool SharedItemPool<ItemType>::isFull()
> -{
> - std::lock_guard<std::mutex> l(mutex_);
> - bool ret = (available_.size() == capacity_);
> - return ret;
> -}
> -
> -template<class ItemType>
> -int SharedItemPool<ItemType>::deInit()
> -{
> - std::lock_guard<std::mutex> l(mutex_);
> - if (capacity_ == 0) {
> - LOG(SharedItemPool, Debug) << "Shared pool " << poolName_
> - << " isn't initialized or already de-initialized";
> - return 0;
> - }
> - if (available_.size() != capacity_) {
> - LOG(SharedItemPool, Error) << "Not all items are returned "
> - << "when destroying pool " << poolName_
> - << "(" << available_.size() << "/" << capacity_ << ")";
> - }
> -
> - delete[] allocated_;
> - allocated_ = nullptr;
> - available_.clear();
> - capacity_ = 0;
> - LOG(SharedItemPool, Debug) << "Shared pool " << poolName_
> - << " deinit done";
> -
> - return 0;
> -}
> -
> -template<class ItemType>
> -int SharedItemPool<ItemType>::acquireItem(std::shared_ptr<ItemType> &item)
> -{
> - item.reset();
> - std::lock_guard<std::mutex> l(mutex_);
> - if (available_.empty()) {
> - LOG(SharedItemPool, Error) << "Shared pool " << poolName_
> - << "is empty";
> - return -ENOSYS;
> - }
> -
> - std::shared_ptr<ItemType> sh(available_[0], deleter_);
> - available_.erase(available_.begin());
> - item = sh;
> - LOG(SharedItemPool, Debug) << "Shared pool " << poolName_
> - << "acquire items " << sh.get();
> - return 0;
> -}
> -
> -template<class ItemType>
> -size_t SharedItemPool<ItemType>::availableItems()
> -{
> - std::lock_guard<std::mutex> l(mutex_);
> - size_t ret = available_.size();
> - return ret;
> -}
> -
> -template<class ItemType>
> -int SharedItemPool<ItemType>::_releaseItem(ItemType *item)
> -{
> - std::lock_guard<std::mutex> l(mutex_);
> - if (resetter_)
> - resetter_(item);
> -
> - LOG(SharedItemPool, Debug) << "Shared pool " << poolName_
> - << "returning item " << item;
> -
> - available_.push_back(item);
> - return 0;
> -}
> -
> -template class SharedItemPool<ia_aiq_rgbs_grid>;
> -template class SharedItemPool<ia_aiq_af_grid>;
> -} /* namespace libcamera */
> diff --git a/stats/shared_item_pool.h b/stats/shared_item_pool.h
> deleted file mode 100644
> index 89dc9b3..0000000
> --- a/stats/shared_item_pool.h
> +++ /dev/null
> @@ -1,114 +0,0 @@
> -/* SPDX-License-Identifier: Apache-2.0 */
> -/*
> - * Copyright (C) 2014-2018 Intel Corporation
> - *
> - * This implementation is highly derived from ChromeOS:
> - * platform2/camera/hal/intel/ipu3/common/SharedItemPool.h
> - */
> -
> -#ifndef SHARED_ITEM_POOL_H
> -#define SHARED_ITEM_POOL_H
> -
> -#include <memory>
> -#include <mutex>
> -#include <pthread.h>
> -#include <vector>
> -
> -/**
> - * \class SharedItemPool
> - *
> - * Pool of ref counted items. This class creates a pool of items and manages
> - * the acquisition of them. When all references to this item have disappeared
> - * The item is returned to the pool.
> - *
> - * This class is thread safe, i.e. it can be called from multiple threads.
> - * When the element is recycled to the pool it can be reset via a client
> - * provided method.
> - *
> - */
> -
> -namespace libcamera {
> -
> -template<class ItemType>
> -class SharedItemPool
> -{
> -public:
> - SharedItemPool(const char *name = "Unnamed");
> - ~SharedItemPool();
> -
> - /**
> - * Initializes the capacity of the pool. It allocates the objects.
> - * optionally it will take function to reset the item before recycling
> - * it to the pool.
> - * This method is thread safe.
> - *
> - * \param capacity[IN]: Number of items the pool will hold
> - * \param resetter[IN]: Function to reset the item before recycling to the
> - * pool.
> - * \return -ENOSYS if trying to initialize twice
> - * \return 0 If everything went ok.
> - */
> - int init(int32_t capacity, void (*resetter)(ItemType *) = nullptr);
> -
> - bool isFull();
> -
> - /**
> - * Free the resources of the pool
> - *
> - * \return 0 on success
> - */
> - int deInit();
> -
> - /**
> - * Acquire an item from the pool.
> - * This method is thread safe. Access to the internal acquire/release
> - * methods are protected.
> - * BUT the thread-safety for the utilization of the item after it has been
> - * acquired is the user's responsibility.
> - * Be careful not to provide the same item to multiple threads that write
> - * into it.
> - *
> - * \param item[OUT] shared pointer to an item.
> - * \return 0 on success
> - */
> - int acquireItem(std::shared_ptr<ItemType> &item);
> -
> - /**
> - * Returns the number of currently available items
> - * It there would be issues acquiring the lock the method returns 0
> - * available items.
> - *
> - * \return item count
> - */
> - size_t availableItems();
> -
> -private:
> - int _releaseItem(ItemType *item);
> -
> - class ItemDeleter
> - {
> - public:
> - ItemDeleter(SharedItemPool *pool)
> - : mPool(pool) {}
> - void operator()(ItemType *item) const
> - {
> - mPool->_releaseItem(item);
> - }
> -
> - private:
> - SharedItemPool *mPool;
> - };
> -
> - std::vector<ItemType *> available_; /* SharedItemPool doesn't have ownership */
> - ItemType *allocated_;
> - size_t capacity_;
> - ItemDeleter deleter_;
> - std::mutex mutex_; /* protects available_, allocated_, capacity_ */
> - const char *poolName_;
> - void (*resetter_)(ItemType *);
> -};
> -
> -} /* namespace libcamera */
> -
> -#endif /* SHARED_ITEM_POOL_H */
> -
> -- 2.33.0.800.g4c38ced690-goog
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211005/bd59eb59/attachment-0001.htm>
More information about the libcamera-devel
mailing list