[libcamera-devel] [PATCH v3 1/5] ipa: ipu3: af: Move constants to implementation
Kate Hsuan
hpa at redhat.com
Tue Mar 29 13:23:45 CEST 2022
Hi Laurent and Kieran,
On Tue, Mar 29, 2022 at 6:28 PM Laurent Pinchart
<laurent.pinchart at 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 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.
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
>
--
BR,
Kate
More information about the libcamera-devel
mailing list