[libcamera-devel] [PATCH v4 16/32] ipa: rkisp1: Convert to use the FCQueue

[Kieran Bingham]

Quoting Laurent Pinchart (2022-09-08 02:41:44)
> From: Kieran Bingham <kieran.bingham at 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 at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Umang Jain <umang.jain at 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 at 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
>