[libcamera-devel,v3,1/5] ipa: ipu3: af: Move constants to implementation
diff mbox series

Message ID 20220325092555.1799897-2-kieran.bingham@ideasonboard.com
State Accepted
Headers show
Series
  • ipa: ipu3: af: Small improvements
Related show

Commit Message

Kieran Bingham March 25, 2022, 9:25 a.m. UTC 
A selection of constants are imported from ChromiumOS.

Move these out of the header, and simplify their documentation.  Further
more, add a direct reference to the location they were obtained from.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/af.cpp | 73 ++++++++++------------------------
 src/ipa/ipu3/algorithms/af.h   | 11 -----
 2 files changed, 22 insertions(+), 62 deletions(-)

Comments

Jean-Michel Hautbois March 29, 2022, 5:55 a.m. UTC | #1 
Hi Kieran,

Thanks for the patch !

On 25/03/2022 10:25, Kieran Bingham wrote:
> A selection of constants are imported from ChromiumOS.
> 
> Move these out of the header, and simplify their documentation.  Further
> more, add a direct reference to the location they were obtained from.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

> ---
>   src/ipa/ipu3/algorithms/af.cpp | 73 ++++++++++------------------------
>   src/ipa/ipu3/algorithms/af.h   | 11 -----
>   2 files changed, 22 insertions(+), 62 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> index 0170a3728892..634d0f2e3176 100644
> --- a/src/ipa/ipu3/algorithms/af.cpp
> +++ b/src/ipa/ipu3/algorithms/af.cpp
> @@ -29,59 +29,30 @@
>    * \file af.h
>    */
>   
> -/**
> - * \var kAfMinGridWidth
> - * \brief the minimum width of AF grid.
> - * The minimum grid horizontal dimensions.
> - */
> -
> -/**
> - * \var kAfMinGridHeight
> - * \brief the minimum height of AF grid.
> - * The minimum grid vertical dimensions.
> - */
> -
> -/**
> - * \var kAfMaxGridWidth
> - * \brief the maximum width of AF grid.
> - * The maximum grid horizontal dimensions.
> - */
> -
> -/**
> - * \var kAfMaxGridHeight
> - * \brief The maximum height of AF grid.
> - * The maximum grid vertical dimensions.
> - */
> -
> -/**
> - * \var kAfMinGridBlockWidth
> - * \brief The minimum block size of the width.
> - * The minimum value of Log2 of the width of the grid cell.
> - */
> -
> -/**
> - * \var kAfMinGridBlockHeight
> - * \brief The minimum block size of the height.
> - * The minimum value of Log2 of the height of the grid cell.
> - */
> -
> -/**
> - * \def kAfMaxGridBlockWidth
> - * \brief The maximum block size of the width.
> - * The maximum value of Log2 of the width of the grid cell.
> - */
> -
> -/**
> - * \var kAfMaxGridBlockHeight
> - * \brief The maximum block size of the height.
> - * The maximum value of Log2 of the height of the grid cell.
> +/*
> + * Static variables from ChromiumOS Intel Camera HAL and ia_imaging library:
> + * - https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/master/hal/intel/psl/ipu3/statsConverter/ipu3-stats.h
> + * - https://chromium.googlesource.com/chromiumos/platform/camera/+/refs/heads/main/hal/intel/ipu3/include/ia_imaging/af_public.h
>    */
>   
> -/**
> - * \var kAfDefaultHeightPerSlice
> - * \brief The default number of blocks in vertical axis per slice.
> - * The number of blocks in vertical axis per slice.
> - */
> +/** The minimum horizontal grid dimension. */
> +static constexpr uint8_t kAfMinGridWidth = 16;
> +/** The minimum vertical grid dimension. */
> +static constexpr uint8_t kAfMinGridHeight = 16;
> +/** The maximum horizontal grid dimension. */
> +static constexpr uint8_t kAfMaxGridWidth = 32;
> +/** The maximum vertical grid dimension. */
> +static constexpr uint8_t kAfMaxGridHeight = 24;
> +/** The minimum value of Log2 of the width of the grid cell. */
> +static constexpr uint16_t kAfMinGridBlockWidth = 4;
> +/** The minimum value of Log2 of the height of the grid cell. */
> +static constexpr uint16_t kAfMinGridBlockHeight = 3;
> +/** The maximum value of Log2 of the width of the grid cell. */
> +static constexpr uint16_t kAfMaxGridBlockWidth = 6;
> +/** The maximum value of Log2 of the height of the grid cell. */
> +static constexpr uint16_t kAfMaxGridBlockHeight = 6;
> +/** The number of blocks in vertical axis per slice. */
> +static constexpr uint16_t kAfDefaultHeightPerSlice = 2;
>   
>   namespace libcamera {
>   
> diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
> index 13c7e0e877fd..906f2843dd49 100644
> --- a/src/ipa/ipu3/algorithms/af.h
> +++ b/src/ipa/ipu3/algorithms/af.h
> @@ -15,17 +15,6 @@
>   
>   #include "algorithm.h"
>   
> -/* Static variables from repo of chromium */
> -static constexpr uint8_t kAfMinGridWidth = 16;
> -static constexpr uint8_t kAfMinGridHeight = 16;
> -static constexpr uint8_t kAfMaxGridWidth = 32;
> -static constexpr uint8_t kAfMaxGridHeight = 24;
> -static constexpr uint16_t kAfMinGridBlockWidth = 4;
> -static constexpr uint16_t kAfMinGridBlockHeight = 3;
> -static constexpr uint16_t kAfMaxGridBlockWidth = 6;
> -static constexpr uint16_t kAfMaxGridBlockHeight = 6;
> -static constexpr uint16_t kAfDefaultHeightPerSlice = 2;
> -
>   namespace libcamera {
>   
>   namespace ipa::ipu3::algorithms {
Kieran Bingham March 29, 2022, 10:17 a.m. UTC | #2 
Hi Kieran

Quoting Kieran Bingham (2022-03-25 09:25:51)
> A selection of constants are imported from ChromiumOS.
> 
> Move these out of the header, and simplify their documentation.  Further
> more, add a direct reference to the location they were obtained from.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/af.cpp | 73 ++++++++++------------------------
>  src/ipa/ipu3/algorithms/af.h   | 11 -----
>  2 files changed, 22 insertions(+), 62 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> index 0170a3728892..634d0f2e3176 100644
> --- a/src/ipa/ipu3/algorithms/af.cpp
> +++ b/src/ipa/ipu3/algorithms/af.cpp
> @@ -29,59 +29,30 @@
>   * \file af.h
>   */
>  
> -/**
> - * \var kAfMinGridWidth
> - * \brief the minimum width of AF grid.
> - * The minimum grid horizontal dimensions.
> - */
> -
> -/**
> - * \var kAfMinGridHeight
> - * \brief the minimum height of AF grid.
> - * The minimum grid vertical dimensions.
> - */
> -
> -/**
> - * \var kAfMaxGridWidth
> - * \brief the maximum width of AF grid.
> - * The maximum grid horizontal dimensions.
> - */
> -
> -/**
> - * \var kAfMaxGridHeight
> - * \brief The maximum height of AF grid.
> - * The maximum grid vertical dimensions.
> - */
> -
> -/**
> - * \var kAfMinGridBlockWidth
> - * \brief The minimum block size of the width.
> - * The minimum value of Log2 of the width of the grid cell.
> - */
> -
> -/**
> - * \var kAfMinGridBlockHeight
> - * \brief The minimum block size of the height.
> - * The minimum value of Log2 of the height of the grid cell.
> - */
> -
> -/**
> - * \def kAfMaxGridBlockWidth
> - * \brief The maximum block size of the width.
> - * The maximum value of Log2 of the width of the grid cell.
> - */
> -
> -/**
> - * \var kAfMaxGridBlockHeight
> - * \brief The maximum block size of the height.
> - * The maximum value of Log2 of the height of the grid cell.
> +/*
> + * Static variables from ChromiumOS Intel Camera HAL and ia_imaging library:
> + * - https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/master/hal/intel/psl/ipu3/statsConverter/ipu3-stats.h
> + * - https://chromium.googlesource.com/chromiumos/platform/camera/+/refs/heads/main/hal/intel/ipu3/include/ia_imaging/af_public.h
>   */
>  
> -/**
> - * \var kAfDefaultHeightPerSlice
> - * \brief The default number of blocks in vertical axis per slice.
> - * The number of blocks in vertical axis per slice.
> - */
> +/** The minimum horizontal grid dimension. */
> +static constexpr uint8_t kAfMinGridWidth = 16;
> +/** The minimum vertical grid dimension. */
> +static constexpr uint8_t kAfMinGridHeight = 16;
> +/** The maximum horizontal grid dimension. */
> +static constexpr uint8_t kAfMaxGridWidth = 32;
> +/** The maximum vertical grid dimension. */
> +static constexpr uint8_t kAfMaxGridHeight = 24;
> +/** The minimum value of Log2 of the width of the grid cell. */
> +static constexpr uint16_t kAfMinGridBlockWidth = 4;
> +/** The minimum value of Log2 of the height of the grid cell. */
> +static constexpr uint16_t kAfMinGridBlockHeight = 3;
> +/** The maximum value of Log2 of the width of the grid cell. */
> +static constexpr uint16_t kAfMaxGridBlockWidth = 6;
> +/** The maximum value of Log2 of the height of the grid cell. */
> +static constexpr uint16_t kAfMaxGridBlockHeight = 6;

It turns out some of these are unused and that breaks Clang-12
compilation by default:

clang++-12 -Isrc/ipa/ipu3/ipa_ipu3.so.p -Isrc/ipa/ipu3 -I../../../src/libcamera/src/ipa/ipu3 -Iinclude -I../../../src/libcamera/include -Isrc/ipa -I../../../src/libcamera/src/ipa -Iinclude/libcamera/ipa -Iinclude/libcamera -fcolor-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -Wextra-semi -Wthread-safety -Wshadow -include config.h -Wno-c99-designator -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o -MF src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o.d -o src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o -c ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp
../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:43:26: error: unused variable 'kAfMaxGridWidth' [-Werror,-Wunused-const-variable]
static constexpr uint8_t kAfMaxGridWidth = 32;
                         ^
../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:45:26: error: unused variable 'kAfMaxGridHeight' [-Werror,-Wunused-const-variable]
static constexpr uint8_t kAfMaxGridHeight = 24;
                         ^
../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:51:27: error: unused variable 'kAfMaxGridBlockWidth' [-Werror,-Wunused-const-variable]
static constexpr uint16_t kAfMaxGridBlockWidth = 6;
                          ^
../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:53:27: error: unused variable 'kAfMaxGridBlockHeight' [-Werror,-Wunused-const-variable]
static constexpr uint16_t kAfMaxGridBlockHeight = 6;
                          ^
4 errors generated.


Kate/JM - 

Do you think we should keep these either commented out or in some form
of documentation?

Or should we add an assert that makes use of them so that they can be
kept?

Or should we just drop the unused constants altogether?

--
Kieran


> +/** The number of blocks in vertical axis per slice. */
> +static constexpr uint16_t kAfDefaultHeightPerSlice = 2;
>  
>  namespace libcamera {
>  
> diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
> index 13c7e0e877fd..906f2843dd49 100644
> --- a/src/ipa/ipu3/algorithms/af.h
> +++ b/src/ipa/ipu3/algorithms/af.h
> @@ -15,17 +15,6 @@
>  
>  #include "algorithm.h"
>  
> -/* Static variables from repo of chromium */
> -static constexpr uint8_t kAfMinGridWidth = 16;
> -static constexpr uint8_t kAfMinGridHeight = 16;
> -static constexpr uint8_t kAfMaxGridWidth = 32;
> -static constexpr uint8_t kAfMaxGridHeight = 24;
> -static constexpr uint16_t kAfMinGridBlockWidth = 4;
> -static constexpr uint16_t kAfMinGridBlockHeight = 3;
> -static constexpr uint16_t kAfMaxGridBlockWidth = 6;
> -static constexpr uint16_t kAfMaxGridBlockHeight = 6;
> -static constexpr uint16_t kAfDefaultHeightPerSlice = 2;
> -
>  namespace libcamera {
>  
>  namespace ipa::ipu3::algorithms {
> -- 
> 2.32.0
>
Laurent Pinchart March 29, 2022, 10:28 a.m. UTC | #3 
On Tue, Mar 29, 2022 at 11:17:58AM +0100, Kieran Bingham wrote:
> Hi Kieran
> 
> Quoting Kieran Bingham (2022-03-25 09:25:51)
> > A selection of constants are imported from ChromiumOS.
> > 
> > Move these out of the header, and simplify their documentation.  Further
> > more, add a direct reference to the location they were obtained from.
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/af.cpp | 73 ++++++++++------------------------
> >  src/ipa/ipu3/algorithms/af.h   | 11 -----
> >  2 files changed, 22 insertions(+), 62 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> > index 0170a3728892..634d0f2e3176 100644
> > --- a/src/ipa/ipu3/algorithms/af.cpp
> > +++ b/src/ipa/ipu3/algorithms/af.cpp
> > @@ -29,59 +29,30 @@
> >   * \file af.h
> >   */
> >  
> > -/**
> > - * \var kAfMinGridWidth
> > - * \brief the minimum width of AF grid.
> > - * The minimum grid horizontal dimensions.
> > - */
> > -
> > -/**
> > - * \var kAfMinGridHeight
> > - * \brief the minimum height of AF grid.
> > - * The minimum grid vertical dimensions.
> > - */
> > -
> > -/**
> > - * \var kAfMaxGridWidth
> > - * \brief the maximum width of AF grid.
> > - * The maximum grid horizontal dimensions.
> > - */
> > -
> > -/**
> > - * \var kAfMaxGridHeight
> > - * \brief The maximum height of AF grid.
> > - * The maximum grid vertical dimensions.
> > - */
> > -
> > -/**
> > - * \var kAfMinGridBlockWidth
> > - * \brief The minimum block size of the width.
> > - * The minimum value of Log2 of the width of the grid cell.
> > - */
> > -
> > -/**
> > - * \var kAfMinGridBlockHeight
> > - * \brief The minimum block size of the height.
> > - * The minimum value of Log2 of the height of the grid cell.
> > - */
> > -
> > -/**
> > - * \def kAfMaxGridBlockWidth
> > - * \brief The maximum block size of the width.
> > - * The maximum value of Log2 of the width of the grid cell.
> > - */
> > -
> > -/**
> > - * \var kAfMaxGridBlockHeight
> > - * \brief The maximum block size of the height.
> > - * The maximum value of Log2 of the height of the grid cell.
> > +/*
> > + * Static variables from ChromiumOS Intel Camera HAL and ia_imaging library:
> > + * - https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/master/hal/intel/psl/ipu3/statsConverter/ipu3-stats.h
> > + * - https://chromium.googlesource.com/chromiumos/platform/camera/+/refs/heads/main/hal/intel/ipu3/include/ia_imaging/af_public.h
> >   */
> >  
> > -/**
> > - * \var kAfDefaultHeightPerSlice
> > - * \brief The default number of blocks in vertical axis per slice.
> > - * The number of blocks in vertical axis per slice.
> > - */
> > +/** The minimum horizontal grid dimension. */
> > +static constexpr uint8_t kAfMinGridWidth = 16;
> > +/** The minimum vertical grid dimension. */
> > +static constexpr uint8_t kAfMinGridHeight = 16;
> > +/** The maximum horizontal grid dimension. */
> > +static constexpr uint8_t kAfMaxGridWidth = 32;
> > +/** The maximum vertical grid dimension. */
> > +static constexpr uint8_t kAfMaxGridHeight = 24;
> > +/** The minimum value of Log2 of the width of the grid cell. */
> > +static constexpr uint16_t kAfMinGridBlockWidth = 4;
> > +/** The minimum value of Log2 of the height of the grid cell. */
> > +static constexpr uint16_t kAfMinGridBlockHeight = 3;
> > +/** The maximum value of Log2 of the width of the grid cell. */
> > +static constexpr uint16_t kAfMaxGridBlockWidth = 6;
> > +/** The maximum value of Log2 of the height of the grid cell. */
> > +static constexpr uint16_t kAfMaxGridBlockHeight = 6;
> 
> It turns out some of these are unused and that breaks Clang-12
> compilation by default:
> 
> clang++-12 -Isrc/ipa/ipu3/ipa_ipu3.so.p -Isrc/ipa/ipu3 -I../../../src/libcamera/src/ipa/ipu3 -Iinclude -I../../../src/libcamera/include -Isrc/ipa -I../../../src/libcamera/src/ipa -Iinclude/libcamera/ipa -Iinclude/libcamera -fcolor-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -Wextra-semi -Wthread-safety -Wshadow -include config.h -Wno-c99-designator -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o -MF src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o.d -o src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o -c ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp
> ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:43:26: error: unused variable 'kAfMaxGridWidth' [-Werror,-Wunused-const-variable]
> static constexpr uint8_t kAfMaxGridWidth = 32;
>                          ^
> ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:45:26: error: unused variable 'kAfMaxGridHeight' [-Werror,-Wunused-const-variable]
> static constexpr uint8_t kAfMaxGridHeight = 24;
>                          ^
> ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:51:27: error: unused variable 'kAfMaxGridBlockWidth' [-Werror,-Wunused-const-variable]
> static constexpr uint16_t kAfMaxGridBlockWidth = 6;
>                           ^
> ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:53:27: error: unused variable 'kAfMaxGridBlockHeight' [-Werror,-Wunused-const-variable]
> static constexpr uint16_t kAfMaxGridBlockHeight = 6;
>                           ^
> 4 errors generated.
> 
> 
> Kate/JM - 
> 
> Do you think we should keep these either commented out or in some form
> of documentation?
> 
> Or should we add an assert that makes use of them so that they can be
> kept?
> 
> Or should we just drop the unused constants altogether?

f you want to keep them, I'd use them in Af::configure() to clamp the
grid and block size values. It's quite pointless as the values are set
to the minimum, so clamping to [min, max] will not do anything useful,
but it can prepare for dynamic calculation of the grid parameters.

> > +/** The number of blocks in vertical axis per slice. */
> > +static constexpr uint16_t kAfDefaultHeightPerSlice = 2;
> >  
> >  namespace libcamera {
> >  
> > diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
> > index 13c7e0e877fd..906f2843dd49 100644
> > --- a/src/ipa/ipu3/algorithms/af.h
> > +++ b/src/ipa/ipu3/algorithms/af.h
> > @@ -15,17 +15,6 @@
> >  
> >  #include "algorithm.h"
> >  
> > -/* Static variables from repo of chromium */
> > -static constexpr uint8_t kAfMinGridWidth = 16;
> > -static constexpr uint8_t kAfMinGridHeight = 16;
> > -static constexpr uint8_t kAfMaxGridWidth = 32;
> > -static constexpr uint8_t kAfMaxGridHeight = 24;
> > -static constexpr uint16_t kAfMinGridBlockWidth = 4;
> > -static constexpr uint16_t kAfMinGridBlockHeight = 3;
> > -static constexpr uint16_t kAfMaxGridBlockWidth = 6;
> > -static constexpr uint16_t kAfMaxGridBlockHeight = 6;
> > -static constexpr uint16_t kAfDefaultHeightPerSlice = 2;
> > -
> >  namespace libcamera {
> >  
> >  namespace ipa::ipu3::algorithms {
Kate Hsuan March 29, 2022, 11:23 a.m. UTC | #4 
Hi Laurent and Kieran,

On Tue, Mar 29, 2022 at 6:28 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Tue, Mar 29, 2022 at 11:17:58AM +0100, Kieran Bingham wrote:
> > Hi Kieran
> >
> > Quoting Kieran Bingham (2022-03-25 09:25:51)
> > > A selection of constants are imported from ChromiumOS.
> > >
> > > Move these out of the header, and simplify their documentation.  Further
> > > more, add a direct reference to the location they were obtained from.
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  src/ipa/ipu3/algorithms/af.cpp | 73 ++++++++++------------------------
> > >  src/ipa/ipu3/algorithms/af.h   | 11 -----
> > >  2 files changed, 22 insertions(+), 62 deletions(-)
> > >
> > > diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> > > index 0170a3728892..634d0f2e3176 100644
> > > --- a/src/ipa/ipu3/algorithms/af.cpp
> > > +++ b/src/ipa/ipu3/algorithms/af.cpp
> > > @@ -29,59 +29,30 @@
> > >   * \file af.h
> > >   */
> > >
> > > -/**
> > > - * \var kAfMinGridWidth
> > > - * \brief the minimum width of AF grid.
> > > - * The minimum grid horizontal dimensions.
> > > - */
> > > -
> > > -/**
> > > - * \var kAfMinGridHeight
> > > - * \brief the minimum height of AF grid.
> > > - * The minimum grid vertical dimensions.
> > > - */
> > > -
> > > -/**
> > > - * \var kAfMaxGridWidth
> > > - * \brief the maximum width of AF grid.
> > > - * The maximum grid horizontal dimensions.
> > > - */
> > > -
> > > -/**
> > > - * \var kAfMaxGridHeight
> > > - * \brief The maximum height of AF grid.
> > > - * The maximum grid vertical dimensions.
> > > - */
> > > -
> > > -/**
> > > - * \var kAfMinGridBlockWidth
> > > - * \brief The minimum block size of the width.
> > > - * The minimum value of Log2 of the width of the grid cell.
> > > - */
> > > -
> > > -/**
> > > - * \var kAfMinGridBlockHeight
> > > - * \brief The minimum block size of the height.
> > > - * The minimum value of Log2 of the height of the grid cell.
> > > - */
> > > -
> > > -/**
> > > - * \def kAfMaxGridBlockWidth
> > > - * \brief The maximum block size of the width.
> > > - * The maximum value of Log2 of the width of the grid cell.
> > > - */
> > > -
> > > -/**
> > > - * \var kAfMaxGridBlockHeight
> > > - * \brief The maximum block size of the height.
> > > - * The maximum value of Log2 of the height of the grid cell.
> > > +/*
> > > + * Static variables from ChromiumOS Intel Camera HAL and ia_imaging library:
> > > + * - https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/master/hal/intel/psl/ipu3/statsConverter/ipu3-stats.h
> > > + * - https://chromium.googlesource.com/chromiumos/platform/camera/+/refs/heads/main/hal/intel/ipu3/include/ia_imaging/af_public.h
> > >   */
> > >
> > > -/**
> > > - * \var kAfDefaultHeightPerSlice
> > > - * \brief The default number of blocks in vertical axis per slice.
> > > - * The number of blocks in vertical axis per slice.
> > > - */
> > > +/** The minimum horizontal grid dimension. */
> > > +static constexpr uint8_t kAfMinGridWidth = 16;
> > > +/** The minimum vertical grid dimension. */
> > > +static constexpr uint8_t kAfMinGridHeight = 16;
> > > +/** The maximum horizontal grid dimension. */
> > > +static constexpr uint8_t kAfMaxGridWidth = 32;
> > > +/** The maximum vertical grid dimension. */
> > > +static constexpr uint8_t kAfMaxGridHeight = 24;
> > > +/** The minimum value of Log2 of the width of the grid cell. */
> > > +static constexpr uint16_t kAfMinGridBlockWidth = 4;
> > > +/** The minimum value of Log2 of the height of the grid cell. */
> > > +static constexpr uint16_t kAfMinGridBlockHeight = 3;
> > > +/** The maximum value of Log2 of the width of the grid cell. */
> > > +static constexpr uint16_t kAfMaxGridBlockWidth = 6;
> > > +/** The maximum value of Log2 of the height of the grid cell. */
> > > +static constexpr uint16_t kAfMaxGridBlockHeight = 6;
> >
> > It turns out some of these are unused and that breaks Clang-12
> > compilation by default:
> >
> > clang++-12 -Isrc/ipa/ipu3/ipa_ipu3.so.p -Isrc/ipa/ipu3 -I../../../src/libcamera/src/ipa/ipu3 -Iinclude -I../../../src/libcamera/include -Isrc/ipa -I../../../src/libcamera/src/ipa -Iinclude/libcamera/ipa -Iinclude/libcamera -fcolor-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -Wextra-semi -Wthread-safety -Wshadow -include config.h -Wno-c99-designator -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o -MF src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o.d -o src/ipa/ipu3/ipa_ipu3.so.p/algorithms_af.cpp.o -c ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp
> > ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:43:26: error: unused variable 'kAfMaxGridWidth' [-Werror,-Wunused-const-variable]
> > static constexpr uint8_t kAfMaxGridWidth = 32;
> >                          ^
> > ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:45:26: error: unused variable 'kAfMaxGridHeight' [-Werror,-Wunused-const-variable]
> > static constexpr uint8_t kAfMaxGridHeight = 24;
> >                          ^
> > ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:51:27: error: unused variable 'kAfMaxGridBlockWidth' [-Werror,-Wunused-const-variable]
> > static constexpr uint16_t kAfMaxGridBlockWidth = 6;
> >                           ^
> > ../../../src/libcamera/src/ipa/ipu3/algorithms/af.cpp:53:27: error: unused variable 'kAfMaxGridBlockHeight' [-Werror,-Wunused-const-variable]
> > static constexpr uint16_t kAfMaxGridBlockHeight = 6;
> >                           ^
> > 4 errors generated.
> >
> >
> > Kate/JM -
> >
> > Do you think we should keep these either commented out or in some form
> > of documentation?
> >
> > Or should we add an assert that makes use of them so that they can be
> > kept?
> >
> > Or should we just drop the unused constants altogether?
>
> f you want to keep them, I'd use them in Af::configure() to clamp the
> grid and block size values. It's quite pointless as the values are set
> to the minimum, so clamping to [min, max] will not do anything useful,
> but it can prepare for dynamic calculation of the grid parameters.

I would prefer to keep them for the dynamic grid configuration since
some of the camera can dynamically configure the size and coordinate
of AF grid.
Agree with Laurent, clamp can be used to perform additional checks to
avoid some incorrect configurations.

Thank you.

>
> > > +/** The number of blocks in vertical axis per slice. */
> > > +static constexpr uint16_t kAfDefaultHeightPerSlice = 2;
> > >
> > >  namespace libcamera {
> > >
> > > diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
> > > index 13c7e0e877fd..906f2843dd49 100644
> > > --- a/src/ipa/ipu3/algorithms/af.h
> > > +++ b/src/ipa/ipu3/algorithms/af.h
> > > @@ -15,17 +15,6 @@
> > >
> > >  #include "algorithm.h"
> > >
> > > -/* Static variables from repo of chromium */
> > > -static constexpr uint8_t kAfMinGridWidth = 16;
> > > -static constexpr uint8_t kAfMinGridHeight = 16;
> > > -static constexpr uint8_t kAfMaxGridWidth = 32;
> > > -static constexpr uint8_t kAfMaxGridHeight = 24;
> > > -static constexpr uint16_t kAfMinGridBlockWidth = 4;
> > > -static constexpr uint16_t kAfMinGridBlockHeight = 3;
> > > -static constexpr uint16_t kAfMaxGridBlockWidth = 6;
> > > -static constexpr uint16_t kAfMaxGridBlockHeight = 6;
> > > -static constexpr uint16_t kAfDefaultHeightPerSlice = 2;
> > > -
> > >  namespace libcamera {
> > >
> > >  namespace ipa::ipu3::algorithms {
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
index 0170a3728892..634d0f2e3176 100644
--- a/src/ipa/ipu3/algorithms/af.cpp
+++ b/src/ipa/ipu3/algorithms/af.cpp
@@ -29,59 +29,30 @@ 
  * \file af.h
  */
 
-/**
- * \var kAfMinGridWidth
- * \brief the minimum width of AF grid.
- * The minimum grid horizontal dimensions.
- */
-
-/**
- * \var kAfMinGridHeight
- * \brief the minimum height of AF grid.
- * The minimum grid vertical dimensions.
- */
-
-/**
- * \var kAfMaxGridWidth
- * \brief the maximum width of AF grid.
- * The maximum grid horizontal dimensions.
- */
-
-/**
- * \var kAfMaxGridHeight
- * \brief The maximum height of AF grid.
- * The maximum grid vertical dimensions.
- */
-
-/**
- * \var kAfMinGridBlockWidth
- * \brief The minimum block size of the width.
- * The minimum value of Log2 of the width of the grid cell.
- */
-
-/**
- * \var kAfMinGridBlockHeight
- * \brief The minimum block size of the height.
- * The minimum value of Log2 of the height of the grid cell.
- */
-
-/**
- * \def kAfMaxGridBlockWidth
- * \brief The maximum block size of the width.
- * The maximum value of Log2 of the width of the grid cell.
- */
-
-/**
- * \var kAfMaxGridBlockHeight
- * \brief The maximum block size of the height.
- * The maximum value of Log2 of the height of the grid cell.
+/*
+ * Static variables from ChromiumOS Intel Camera HAL and ia_imaging library:
+ * - https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/master/hal/intel/psl/ipu3/statsConverter/ipu3-stats.h
+ * - https://chromium.googlesource.com/chromiumos/platform/camera/+/refs/heads/main/hal/intel/ipu3/include/ia_imaging/af_public.h
  */
 
-/**
- * \var kAfDefaultHeightPerSlice
- * \brief The default number of blocks in vertical axis per slice.
- * The number of blocks in vertical axis per slice.
- */
+/** The minimum horizontal grid dimension. */
+static constexpr uint8_t kAfMinGridWidth = 16;
+/** The minimum vertical grid dimension. */
+static constexpr uint8_t kAfMinGridHeight = 16;
+/** The maximum horizontal grid dimension. */
+static constexpr uint8_t kAfMaxGridWidth = 32;
+/** The maximum vertical grid dimension. */
+static constexpr uint8_t kAfMaxGridHeight = 24;
+/** The minimum value of Log2 of the width of the grid cell. */
+static constexpr uint16_t kAfMinGridBlockWidth = 4;
+/** The minimum value of Log2 of the height of the grid cell. */
+static constexpr uint16_t kAfMinGridBlockHeight = 3;
+/** The maximum value of Log2 of the width of the grid cell. */
+static constexpr uint16_t kAfMaxGridBlockWidth = 6;
+/** The maximum value of Log2 of the height of the grid cell. */
+static constexpr uint16_t kAfMaxGridBlockHeight = 6;
+/** The number of blocks in vertical axis per slice. */
+static constexpr uint16_t kAfDefaultHeightPerSlice = 2;
 
 namespace libcamera {
 
diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
index 13c7e0e877fd..906f2843dd49 100644
--- a/src/ipa/ipu3/algorithms/af.h
+++ b/src/ipa/ipu3/algorithms/af.h
@@ -15,17 +15,6 @@ 
 
 #include "algorithm.h"
 
-/* Static variables from repo of chromium */
-static constexpr uint8_t kAfMinGridWidth = 16;
-static constexpr uint8_t kAfMinGridHeight = 16;
-static constexpr uint8_t kAfMaxGridWidth = 32;
-static constexpr uint8_t kAfMaxGridHeight = 24;
-static constexpr uint16_t kAfMinGridBlockWidth = 4;
-static constexpr uint16_t kAfMinGridBlockHeight = 3;
-static constexpr uint16_t kAfMaxGridBlockWidth = 6;
-static constexpr uint16_t kAfMaxGridBlockHeight = 6;
-static constexpr uint16_t kAfDefaultHeightPerSlice = 2;
-
 namespace libcamera {
 
 namespace ipa::ipu3::algorithms {