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

[Laurent Pinchart]

Hi Paul,

On Sun, Aug 21, 2022 at 07:16:30PM +0900, paul.elder at ideasonboard.com wrote:
> On Thu, Aug 18, 2022 at 08:49:59PM +0300, Laurent Pinchart wrote:
> > 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.
> 
> Aah, I see. So this variable is only for the AWB hardware block, and is
> unrelated (mostly) to whether or not the AWB algorithm is enabled or
> not. (In which case for my other awb patch (oh, that's already been
> merged) it does need a separate awb enabled variable in the context and
> not just the one in configuration).
> 
> So from what I understand we could still have awb.enabled set to false
> even if configuration succeeds, because we're using a monochrome sensor
> for example.

Possibly. It depends on how monochrome sensors will be handled, I'd
imagine that their tuning file would not include an AWB algorithm, as it
makes no sense for them, but we could also let the algorithm load but
disable itself.

> > 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 ?
> 
> So then I think documentation here and in the commit message can mirror
> what you have for LSC in the next patch, since that's practically the
> same deal.
> 
> With that fix,
> 
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> > > > >   */
> > > > >  
> > > > >  /**
> > > > > 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