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

Message ID 20220816015414.7462-8-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Add DPF tuning support for RkISP1
Related show

Commit Message

Laurent Pinchart Aug. 16, 2022, 1:54 a.m. UTC
From: Florian Sylvestre <fsylvestre@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@baylibre.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@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(+)

Comments

Nicolas Dufresne via libcamera-devel Aug. 18, 2022, 1:16 p.m. UTC | #1
Hi Laurent,

On Tue, Aug 16, 2022 at 04:54:12AM +0300, Laurent Pinchart via libcamera-devel wrote:
> From: Florian Sylvestre <fsylvestre@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@baylibre.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Umang Jain <umang.jain@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


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 {
Nicolas Dufresne via libcamera-devel Aug. 18, 2022, 1:39 p.m. UTC | #2
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@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@baylibre.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Umang Jain <umang.jain@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 {
Laurent Pinchart Aug. 18, 2022, 5:49 p.m. UTC | #3
Hi Paul,

On Thu, Aug 18, 2022 at 10:39:23PM +0900, paul.elder@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@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@baylibre.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Reviewed-by: Umang Jain <umang.jain@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 {
Nicolas Dufresne via libcamera-devel Aug. 21, 2022, 10:16 a.m. UTC | #4
Hi Laurent,

Sorry for the delay.

On Thu, Aug 18, 2022 at 08:49:59PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> On Thu, Aug 18, 2022 at 10:39:23PM +0900, paul.elder@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@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@baylibre.com>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Reviewed-by: Umang Jain <umang.jain@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.

> 
> 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@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 {
Laurent Pinchart Aug. 21, 2022, 6:05 p.m. UTC | #5
Hi Paul,

On Sun, Aug 21, 2022 at 07:16:30PM +0900, paul.elder@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@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@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@baylibre.com>
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Reviewed-by: Umang Jain <umang.jain@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@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 {

Patch
diff mbox series

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