[libcamera-devel,RFC,v2,1/3] ipa: ipu3: Separate out frame context queue as a distinct class

16103 diff mbox series

There are cases where we need more checks and balance to be carried out
by the frame context queue class. For that, separate it out as a
distinct class on which we can build upon.

For now, a minimialistic implementation is provided with .get(frame)
helper which returns a IPAFrameContext for the required frame.
Going ahead more such helpers can be provided to access/modify the
frame context queue.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/ipa/ipu3/ipa_context.cpp | 49 ++++++++++++++++++++++++++++++++++--
 src/ipa/ipu3/ipa_context.h   | 11 +++++++-
 src/ipa/ipu3/ipu3.cpp        |  6 ++---
 3 files changed, 60 insertions(+), 6 deletions(-)

Hi Umang,

On Fri, May 27, 2022 at 09:17:38PM +0200, Umang Jain via libcamera-devel wrote:
> There are cases where we need more checks and balance to be carried out
> by the frame context queue class. For that, separate it out as a
> distinct class on which we can build upon.
> 
> For now, a minimialistic implementation is provided with .get(frame)
> helper which returns a IPAFrameContext for the required frame.
> Going ahead more such helpers can be provided to access/modify the
> frame context queue.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/ipa/ipu3/ipa_context.cpp | 49 ++++++++++++++++++++++++++++++++++--
>  src/ipa/ipu3/ipa_context.h   | 11 +++++++-
>  src/ipa/ipu3/ipu3.cpp        |  6 ++---
>  3 files changed, 60 insertions(+), 6 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 13cdb835..e5010e3f 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -7,12 +7,18 @@
>  
>  #include "ipa_context.h"
>  
> +#include <libcamera/base/log.h>
> +
>  /**
>   * \file ipa_context.h
>   * \brief Context and state information shared between the algorithms
>   */
>  
> -namespace libcamera::ipa::ipu3 {
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPAIPU3)
> +
> +namespace ipa::ipu3 {
>  
>  /**
>   * \struct IPASessionConfiguration
> @@ -211,4 +217,43 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
>   * \brief Analogue gain multiplier
>   */
>  
> -} /* namespace libcamera::ipa::ipu3 */
> +/**
> + * \brief FCQueue constructor
> + */
> +FCQueue::FCQueue()
> +{
> +	clear();
> +}
> +
> +/**
> + * Retrieve the IPAFrameContext for the frame
> + * \param[in] frame Frame number for which the IPAFrameContext needs to
> + * retrieved
> + *
> + * \return Reference to the IPAFrameContext
> + */
> +IPAFrameContext &FCQueue::get(uint32_t frame)
> +{
> +	IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> +
> +	if (frame != frameContext.frame) {
> +		LOG(IPAIPU3, Warning)
> +			<< "Got wrong frame context for frame" << frame
> +			<< " or frame context doesn't exist yet";
> +	}
> +
> +	return frameContext;
> +}
> +
> +/**
> + * \brief Clear the FCQueue by resetting all the entries in the ring-buffer
> + */
> +void FCQueue::clear()
> +{
> +	IPAFrameContext initFrameContext;
> +	this->fill(initFrameContext);
> +}
> +
> +} /* namespace ipa::ipu3 */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 42e11141..61454b41 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -89,11 +89,20 @@ struct IPAFrameContext {
>  	ControlList frameControls;
>  };
>  
> +class FCQueue : public std::array<IPAFrameContext, kMaxFrameContexts>
> +{
> +public:
> +	FCQueue();
> +
> +	void clear();
> +	IPAFrameContext &get(uint32_t frame);
> +};
> +
>  struct IPAContext {
>  	IPASessionConfiguration configuration;
>  	IPAActiveState activeState;
>  
> -	std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
> +	FCQueue frameContexts;
>  };
>  
>  } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 2f6bb672..c48d2f62 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -456,8 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>  
>  	/* Clean IPAActiveState at each reconfiguration. */
>  	context_.activeState = {};
> -	IPAFrameContext initFrameContext;
> -	context_.frameContexts.fill(initFrameContext);
> +	context_.frameContexts.clear();
> +
>  
>  	if (!validateSensorControls()) {
>  		LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> @@ -569,7 +569,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>  	const ipu3_uapi_stats_3a *stats =
>  		reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>  
> -	IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];
> +	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>  
>  	if (frameContext.frame != frame)
>  		LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
> -- 
> 2.31.1
>

Hi Umang,

Thanks for the patch !

On 27/05/2022 21:17, Umang Jain via libcamera-devel wrote:
> There are cases where we need more checks and balance to be carried out
> by the frame context queue class. For that, separate it out as a
> distinct class on which we can build upon.
> 
> For now, a minimialistic implementation is provided with .get(frame)
> helper which returns a IPAFrameContext for the required frame.
> Going ahead more such helpers can be provided to access/modify the
> frame context queue.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   src/ipa/ipu3/ipa_context.cpp | 49 ++++++++++++++++++++++++++++++++++--
>   src/ipa/ipu3/ipa_context.h   | 11 +++++++-
>   src/ipa/ipu3/ipu3.cpp        |  6 ++---
>   3 files changed, 60 insertions(+), 6 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 13cdb835..e5010e3f 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -7,12 +7,18 @@
>   
>   #include "ipa_context.h"
>   
> +#include <libcamera/base/log.h>
> +
>   /**
>    * \file ipa_context.h
>    * \brief Context and state information shared between the algorithms
>    */
>   
> -namespace libcamera::ipa::ipu3 {
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPAIPU3)
> +
> +namespace ipa::ipu3 {
>   
>   /**
>    * \struct IPASessionConfiguration
> @@ -211,4 +217,43 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
>    * \brief Analogue gain multiplier
>    */
>   

I think you missed the class documentation, and Doxygen warns it:
'''
/home/jm/libcamera/src/ipa/ipu3/ipa_context.h:92: warning: Compound 
libcamera::ipa::ipu3::FCQueue is not documented.
'''

It is in 2/3 but should be here I think.

With this:
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

> -} /* namespace libcamera::ipa::ipu3 */
> +/**
> + * \brief FCQueue constructor
> + */
> +FCQueue::FCQueue()
> +{
> +	clear();
> +}
> +
> +/**
> + * Retrieve the IPAFrameContext for the frame
> + * \param[in] frame Frame number for which the IPAFrameContext needs to
> + * retrieved
> + *
> + * \return Reference to the IPAFrameContext
> + */
> +IPAFrameContext &FCQueue::get(uint32_t frame)
> +{
> +	IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> +
> +	if (frame != frameContext.frame) {
> +		LOG(IPAIPU3, Warning)
> +			<< "Got wrong frame context for frame" << frame
> +			<< " or frame context doesn't exist yet";
> +	}
> +
> +	return frameContext;
> +}
> +
> +/**
> + * \brief Clear the FCQueue by resetting all the entries in the ring-buffer
> + */
> +void FCQueue::clear()
> +{
> +	IPAFrameContext initFrameContext;
> +	this->fill(initFrameContext);
> +}
> +
> +} /* namespace ipa::ipu3 */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 42e11141..61454b41 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -89,11 +89,20 @@ struct IPAFrameContext {
>   	ControlList frameControls;
>   };
>   
> +class FCQueue : public std::array<IPAFrameContext, kMaxFrameContexts>
> +{
> +public:
> +	FCQueue();
> +
> +	void clear();
> +	IPAFrameContext &get(uint32_t frame);
> +};
> +
>   struct IPAContext {
>   	IPASessionConfiguration configuration;
>   	IPAActiveState activeState;
>   
> -	std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
> +	FCQueue frameContexts;
>   };
>   
>   } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 2f6bb672..c48d2f62 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -456,8 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>   
>   	/* Clean IPAActiveState at each reconfiguration. */
>   	context_.activeState = {};
> -	IPAFrameContext initFrameContext;
> -	context_.frameContexts.fill(initFrameContext);
> +	context_.frameContexts.clear();
> +
>   
>   	if (!validateSensorControls()) {
>   		LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> @@ -569,7 +569,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>   	const ipu3_uapi_stats_3a *stats =
>   		reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>   
> -	IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];
> +	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>   
>   	if (frameContext.frame != frame)
>   		LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";