[libcamera-devel] [PATCH v3 1/5] ipa: ipu3: af: Move constants to implementation
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Mar 29 12:28:51 CEST 2022
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 at ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham at 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 {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list