[libcamera-devel] [PATCH 09/11] ipa: ipu3: agc: Document AGC mean-based algorithm

[Laurent Pinchart]

On Fri, Oct 22, 2021 at 11:23:56AM +0100, Kieran Bingham wrote:
> Hi,
> 
> Seems I'm replying to quite an old mail, so I'm not sure if this is
> already done/handled/relevant, but in case it is...
> 
> Quoting Laurent Pinchart (2021-09-14 17:18:47)
> > Hi Kieran,
> > 
> > On Tue, Sep 14, 2021 at 12:20:14PM +0100, Kieran Bingham wrote:
> > > On 14/09/2021 06:21, Laurent Pinchart wrote:
> > > >>  {
> > > >> +  /*
> > > >> +   * Get the applied grid from the statistics buffer. When the kernel
> > > >> +   * receives a grid from the parameters buffer, it will check and align
> > > >> +   * all the values. For instance, it will automatically fill the x_end
> > > >> +   * value based on x_start, grid width and log2 width.
> > > >> +   * \todo Use the grid calculated in configure as there is a bug in IPU3
> > > >> +   * causing the width (maybe height) to be bit-shifted.
> > > > 
> > > > Don't we do so already ?
> > > 
> > > Is this relating to the line directly below which references the grid
> > > from the stats->stats_4a_config.awb_config.grid?
> > > 
> > > Can we simply/directly reference our grid from our IPAContext?
> > > 
> > > And if so - it might be better to do that immediately, rather than add
> > > the \todo.
> > > 
> > > And if we do that we should mention that it is referenced from our grid,
> > > rather than the one provided in the statistics due to a kernel bug..
> > 
> > I meant that we use grid.width, with the following comment:
> > 
> >  * We observed a bit shift which makes the value 160 to
> >  * be 32 in the stats grid. Use the one from configure.
> > 
> > Isn't this what the \todo is about ? Or did I misundertand either the
> > code of the \todo ?
> 
> My statement was that the todo being added sounds so simple - it would
> be better to add a patch to *do* the thing, rather than add a todo.
> 
> If you believe the action of the todo is already done, then that's even
> better.
> 
> Either way, I don't think this should be added as a todo - and it should
> be ... done ;-), and perhaps it already is. If it isn't then can we do
> it please? It sounds like a very short patch if it isn't done already.

I understand it as being done already, but I could be wrong.

-- 
Regards,

Laurent Pinchart