[libcamera-devel] [PATCH v4 7/9] ipa: rkisp1: Add enable field for AWB algorithm in IPA context

[paul.elder at ideasonboard.com]

On Thu, Aug 18, 2022 at 10:16:12PM +0900, Paul Elder via libcamera-devel wrote:
> Hi Laurent,
> 
> On Tue, Aug 16, 2022 at 04:54:12AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > From: Florian Sylvestre <fsylvestre at baylibre.com>
> > 
> > Add an enable variable in the awb struct in IPASessionConfiguration which
> > indicates if the awb algorithm has been configured. This will allow other
> > algorithms to retrieve this information.
> > 
> > Signed-off-by: Florian Sylvestre <fsylvestre at baylibre.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/awb.cpp | 2 ++
> >  src/ipa/rkisp1/ipa_context.cpp    | 3 +++
> >  src/ipa/rkisp1/ipa_context.h      | 1 +
> >  3 files changed, 6 insertions(+)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index 9f00364d12b1..d1328f011081 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -48,6 +48,8 @@ int Awb::configure(IPAContext &context,
> >  	context.configuration.awb.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;
> >  	context.configuration.awb.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
> >  
> > +	context.configuration.awb.enabled = true;
> > +
> >  	return 0;
> >  }
> >  
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index ef8bb8e931c8..23a63f8c6e25 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -87,6 +87,9 @@ namespace libcamera::ipa::rkisp1 {
> >   *
> >   * \var IPASessionConfiguration::awb.measureWindow
> >   * \brief AWB measure window
> > + *
> > + * \var IPASessionConfiguration::awb.enabled
> > + * \brief Indicates if the AWB hardware is enabled to apply colour gains
> 
> Ah, so this is the enable variable that you were talking about :)
> 
> From what I understand, this variable holds whether or not the awb
> algorithm has been configured? So once Awb::configure() completes
> successfully then this is set to (and stays) true?
> 
> If that's the case, I think the description could use an upgrade, as
> the variable doesn't signify that *only* the hardware is enabled for awb
> (actually what I parse from the the current docstring is that it's
> enabled to apply /manual/ color gains).
> 
> Or maybe the docstirng is correct and I'm not parsing it correctly,
> which also means that it could use an upgrade :p

Okay, after having read the last patch that adds DPF, I see how this
variable is meant to be used, and I think that 1) how it's used, 2) how
it's meant to be used according to this changelog, and 3) how it's meant
to be used according to this docstring, are all different.


Paul

> >   */
> >  
> >  /**
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 2bdb6a81d7c9..7f7b3e4d88fa 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -29,6 +29,7 @@ struct IPASessionConfiguration {
> >  
> >  	struct {
> >  		struct rkisp1_cif_isp_window measureWindow;
> > +		bool enabled;
> >  	} awb;
> >  
> >  	struct {