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

[Laurent Pinchart]

Hi Paul,

On Thu, Aug 18, 2022 at 10:39:23PM +0900, paul.elder at ideasonboard.com wrote:
> On Thu, Aug 18, 2022 at 10:16:12PM +0900, Paul Elder via libcamera-devel wrote:
> > 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?

To be precise, it indicates if the AWB hardware block is enabled and
applied colour gains to the pixels. The alternative is the AWB block
being disabled, in which case it would be bypassed from a pixel
processing point of view, and would also not produce AWB statistics.
This would happen when using monochrome sensors, as white balance
doesn't make sense there.

It's the Awb::prepare() function that configures the ISP to enable the
AWB, and it does so unconditionally if the AWB algorithm is
instantiated. The enabled field is set to true in Awb::configure(), but
it could equally be set to true in the Awb::init() function if we
wanted.

> > 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).

>From the point of view of the AWB hardware module, automatic white
balance or manual white balance is irrelevant. The hardware gets colour
gains from the IPA and applies them, regardless of whether those gains
are supplied by the application in manual mode or computed by the
algorithm in automatic mode.

> > 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.

Any recommendation about how I should document it here and in the commit
message ?

> > >   */
> > >  
> > >  /**
> > > 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 {

-- 
Regards,

Laurent Pinchart