Message ID | 20220908014200.28728-17-laurent.pinchart@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Quoting Laurent Pinchart (2022-09-08 02:41:44) > From: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Establish a queue of FrameContexts using the new FCQueue and use it to > supply the FrameContext to the algorithms. > > The algorithms on the RKISP1 do not use this yet themselves, but are > able to do so after the introduction of this patch. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- This has been converted to use the updated API for the FCQ, but perhaps that's implicit. Otherwise, I thnik this is still mostly my patch, but still it still does what I think needs to be done ;-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > src/ipa/rkisp1/ipa_context.cpp | 6 +++--- > src/ipa/rkisp1/ipa_context.h | 2 ++ > src/ipa/rkisp1/rkisp1.cpp | 34 ++++++++++++++++++++++++---------- > 3 files changed, 29 insertions(+), 13 deletions(-) > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > index d18f4996aad2..e9846742ee4f 100644 > --- a/src/ipa/rkisp1/ipa_context.cpp > +++ b/src/ipa/rkisp1/ipa_context.cpp > @@ -205,8 +205,7 @@ namespace libcamera::ipa::rkisp1 { > * \struct IPAFrameContext > * \brief Per-frame context for algorithms > * > - * This structure is currently unused and will be replaced by a real per-frame > - * context. > + * \todo Populate the frame context for all algorithms > */ > > /** > @@ -219,7 +218,8 @@ namespace libcamera::ipa::rkisp1 { > * \var IPAContext::activeState > * \brief The IPA active state, storing the latest state for all algorithms > * > - * \todo Introduce per-frame contexts > + * \var IPAContext::frameContexts > + * \brief Ring buffer of per-frame contexts > */ > > } /* namespace libcamera::ipa::rkisp1 */ > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index f6b3e6eb951c..f6aaefffed52 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -97,6 +97,8 @@ struct IPAFrameContext : public FrameContext { > struct IPAContext { > IPASessionConfiguration configuration; > IPAActiveState activeState; > + > + FCQueue<IPAFrameContext> frameContexts; > }; > > } /* namespace ipa::rkisp1 */ > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 24d5b9647838..c5ed0bb21f67 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -40,13 +40,18 @@ using namespace std::literals::chrono_literals; > > namespace ipa::rkisp1 { > > +/* Maximum number of frame contexts to be held */ > +static constexpr uint32_t kMaxFrameContexts = 16; > + > class IPARkISP1 : public IPARkISP1Interface, public Module > { > public: > + IPARkISP1(); > + > int init(const IPASettings &settings, unsigned int hwRevision, > ControlInfoMap *ipaControls) override; > int start() override; > - void stop() override {} > + void stop() override; > > int configure(const IPACameraSensorInfo &info, > const std::map<uint32_t, IPAStream> &streamConfig, > @@ -100,6 +105,11 @@ const ControlInfoMap::Map rkisp1Controls{ > > } /* namespace */ > > +IPARkISP1::IPARkISP1() > + : context_({ {}, {}, { kMaxFrameContexts } }) > +{ > +} > + > std::string IPARkISP1::logPrefix() const > { > return "rkisp1"; > @@ -186,6 +196,11 @@ int IPARkISP1::start() > return 0; > } > > +void IPARkISP1::stop() > +{ > + context_.frameContexts.clear(); > +} > + > /** > * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo > * if the connected sensor does not provide enough information to properly > @@ -223,8 +238,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, > << "Exposure: " << minExposure << "-" << maxExposure > << " Gain: " << minGain << "-" << maxGain; > > - /* Clean context at configuration */ > - context_ = {}; > + /* Clear the IPA context before the streaming session. */ > + context_.configuration = {}; > + context_.activeState = {}; > + context_.frameContexts.clear(); > > /* Set the hardware revision for the algorithms. */ > context_.configuration.hw.revision = hwRevision_; > @@ -287,8 +304,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids) > > void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls) > { > - /* \todo Obtain the frame context to pass to process from the FCQueue */ > - IPAFrameContext frameContext; > + IPAFrameContext &frameContext = context_.frameContexts.init(frame); > > for (auto const &algo : algorithms()) > algo->queueRequest(context_, frame, frameContext, controls); > @@ -296,8 +312,7 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls) > > void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) > { > - /* \todo Obtain the frame context to pass to process from the FCQueue */ > - IPAFrameContext frameContext; > + IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > rkisp1_params_cfg *params = > reinterpret_cast<rkisp1_params_cfg *>( > @@ -316,6 +331,8 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) > void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId, > const ControlList &sensorControls) > { > + IPAFrameContext &frameContext = context_.frameContexts.get(frame); > + > const rkisp1_stat_buffer *stats = > reinterpret_cast<rkisp1_stat_buffer *>( > mappedBuffers_.at(bufferId).planes()[0].data()); > @@ -327,9 +344,6 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId > > unsigned int aeState = 0; > > - /* \todo Obtain the frame context to pass to process from the FCQueue */ > - IPAFrameContext frameContext; > - > for (auto const &algo : algorithms()) > algo->process(context_, frame, frameContext, stats); > > -- > Regards, > > Laurent Pinchart >
Hi Laurent On Thu, Sep 08, 2022 at 04:41:44AM +0300, Laurent Pinchart via libcamera-devel wrote: > From: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Establish a queue of FrameContexts using the new FCQueue and use it to > supply the FrameContext to the algorithms. > > The algorithms on the RKISP1 do not use this yet themselves, but are > able to do so after the introduction of this patch. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > src/ipa/rkisp1/ipa_context.cpp | 6 +++--- > src/ipa/rkisp1/ipa_context.h | 2 ++ > src/ipa/rkisp1/rkisp1.cpp | 34 ++++++++++++++++++++++++---------- > 3 files changed, 29 insertions(+), 13 deletions(-) > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > index d18f4996aad2..e9846742ee4f 100644 > --- a/src/ipa/rkisp1/ipa_context.cpp > +++ b/src/ipa/rkisp1/ipa_context.cpp > @@ -205,8 +205,7 @@ namespace libcamera::ipa::rkisp1 { > * \struct IPAFrameContext > * \brief Per-frame context for algorithms > * > - * This structure is currently unused and will be replaced by a real per-frame > - * context. > + * \todo Populate the frame context for all algorithms > */ > > /** > @@ -219,7 +218,8 @@ namespace libcamera::ipa::rkisp1 { > * \var IPAContext::activeState > * \brief The IPA active state, storing the latest state for all algorithms > * > - * \todo Introduce per-frame contexts > + * \var IPAContext::frameContexts > + * \brief Ring buffer of per-frame contexts > */ > > } /* namespace libcamera::ipa::rkisp1 */ > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index f6b3e6eb951c..f6aaefffed52 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -97,6 +97,8 @@ struct IPAFrameContext : public FrameContext { > struct IPAContext { > IPASessionConfiguration configuration; > IPAActiveState activeState; > + > + FCQueue<IPAFrameContext> frameContexts; > }; > > } /* namespace ipa::rkisp1 */ > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 24d5b9647838..c5ed0bb21f67 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -40,13 +40,18 @@ using namespace std::literals::chrono_literals; > > namespace ipa::rkisp1 { > > +/* Maximum number of frame contexts to be held */ > +static constexpr uint32_t kMaxFrameContexts = 16; > + > class IPARkISP1 : public IPARkISP1Interface, public Module > { > public: > + IPARkISP1(); > + > int init(const IPASettings &settings, unsigned int hwRevision, > ControlInfoMap *ipaControls) override; > int start() override; > - void stop() override {} > + void stop() override; > > int configure(const IPACameraSensorInfo &info, > const std::map<uint32_t, IPAStream> &streamConfig, > @@ -100,6 +105,11 @@ const ControlInfoMap::Map rkisp1Controls{ > > } /* namespace */ > > +IPARkISP1::IPARkISP1() > + : context_({ {}, {}, { kMaxFrameContexts } }) > +{ > +} > + > std::string IPARkISP1::logPrefix() const > { > return "rkisp1"; > @@ -186,6 +196,11 @@ int IPARkISP1::start() > return 0; > } > > +void IPARkISP1::stop() > +{ > + context_.frameContexts.clear(); > +} > + > /** > * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo > * if the connected sensor does not provide enough information to properly > @@ -223,8 +238,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, > << "Exposure: " << minExposure << "-" << maxExposure > << " Gain: " << minGain << "-" << maxGain; > > - /* Clean context at configuration */ > - context_ = {}; > + /* Clear the IPA context before the streaming session. */ > + context_.configuration = {}; > + context_.activeState = {}; > + context_.frameContexts.clear(); > > /* Set the hardware revision for the algorithms. */ > context_.configuration.hw.revision = hwRevision_; > @@ -287,8 +304,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids) > > void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls) > { > - /* \todo Obtain the frame context to pass to process from the FCQueue */ > - IPAFrameContext frameContext; > + IPAFrameContext &frameContext = context_.frameContexts.init(frame); > > for (auto const &algo : algorithms()) > algo->queueRequest(context_, frame, frameContext, controls); > @@ -296,8 +312,7 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls) > > void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) > { > - /* \todo Obtain the frame context to pass to process from the FCQueue */ > - IPAFrameContext frameContext; > + IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > rkisp1_params_cfg *params = > reinterpret_cast<rkisp1_params_cfg *>( > @@ -316,6 +331,8 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) > void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId, > const ControlList &sensorControls) > { > + IPAFrameContext &frameContext = context_.frameContexts.get(frame); > + > const rkisp1_stat_buffer *stats = > reinterpret_cast<rkisp1_stat_buffer *>( > mappedBuffers_.at(bufferId).planes()[0].data()); > @@ -327,9 +344,6 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId > > unsigned int aeState = 0; > > - /* \todo Obtain the frame context to pass to process from the FCQueue */ > - IPAFrameContext frameContext; > - > for (auto const &algo : algorithms()) > algo->process(context_, frame, frameContext, stats); > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp index d18f4996aad2..e9846742ee4f 100644 --- a/src/ipa/rkisp1/ipa_context.cpp +++ b/src/ipa/rkisp1/ipa_context.cpp @@ -205,8 +205,7 @@ namespace libcamera::ipa::rkisp1 { * \struct IPAFrameContext * \brief Per-frame context for algorithms * - * This structure is currently unused and will be replaced by a real per-frame - * context. + * \todo Populate the frame context for all algorithms */ /** @@ -219,7 +218,8 @@ namespace libcamera::ipa::rkisp1 { * \var IPAContext::activeState * \brief The IPA active state, storing the latest state for all algorithms * - * \todo Introduce per-frame contexts + * \var IPAContext::frameContexts + * \brief Ring buffer of per-frame contexts */ } /* namespace libcamera::ipa::rkisp1 */ diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h index f6b3e6eb951c..f6aaefffed52 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -97,6 +97,8 @@ struct IPAFrameContext : public FrameContext { struct IPAContext { IPASessionConfiguration configuration; IPAActiveState activeState; + + FCQueue<IPAFrameContext> frameContexts; }; } /* namespace ipa::rkisp1 */ diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 24d5b9647838..c5ed0bb21f67 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -40,13 +40,18 @@ using namespace std::literals::chrono_literals; namespace ipa::rkisp1 { +/* Maximum number of frame contexts to be held */ +static constexpr uint32_t kMaxFrameContexts = 16; + class IPARkISP1 : public IPARkISP1Interface, public Module { public: + IPARkISP1(); + int init(const IPASettings &settings, unsigned int hwRevision, ControlInfoMap *ipaControls) override; int start() override; - void stop() override {} + void stop() override; int configure(const IPACameraSensorInfo &info, const std::map<uint32_t, IPAStream> &streamConfig, @@ -100,6 +105,11 @@ const ControlInfoMap::Map rkisp1Controls{ } /* namespace */ +IPARkISP1::IPARkISP1() + : context_({ {}, {}, { kMaxFrameContexts } }) +{ +} + std::string IPARkISP1::logPrefix() const { return "rkisp1"; @@ -186,6 +196,11 @@ int IPARkISP1::start() return 0; } +void IPARkISP1::stop() +{ + context_.frameContexts.clear(); +} + /** * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo * if the connected sensor does not provide enough information to properly @@ -223,8 +238,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, << "Exposure: " << minExposure << "-" << maxExposure << " Gain: " << minGain << "-" << maxGain; - /* Clean context at configuration */ - context_ = {}; + /* Clear the IPA context before the streaming session. */ + context_.configuration = {}; + context_.activeState = {}; + context_.frameContexts.clear(); /* Set the hardware revision for the algorithms. */ context_.configuration.hw.revision = hwRevision_; @@ -287,8 +304,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids) void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls) { - /* \todo Obtain the frame context to pass to process from the FCQueue */ - IPAFrameContext frameContext; + IPAFrameContext &frameContext = context_.frameContexts.init(frame); for (auto const &algo : algorithms()) algo->queueRequest(context_, frame, frameContext, controls); @@ -296,8 +312,7 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls) void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) { - /* \todo Obtain the frame context to pass to process from the FCQueue */ - IPAFrameContext frameContext; + IPAFrameContext &frameContext = context_.frameContexts.get(frame); rkisp1_params_cfg *params = reinterpret_cast<rkisp1_params_cfg *>( @@ -316,6 +331,8 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId, const ControlList &sensorControls) { + IPAFrameContext &frameContext = context_.frameContexts.get(frame); + const rkisp1_stat_buffer *stats = reinterpret_cast<rkisp1_stat_buffer *>( mappedBuffers_.at(bufferId).planes()[0].data()); @@ -327,9 +344,6 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId unsigned int aeState = 0; - /* \todo Obtain the frame context to pass to process from the FCQueue */ - IPAFrameContext frameContext; - for (auto const &algo : algorithms()) algo->process(context_, frame, frameContext, stats);