[libcamera-devel] [PATCH v4 02/10] ipa: rkisp1: Add lens limits to the session config

[Daniel Semkowicz]

Hi Jacopo,

On Tue, Mar 21, 2023 at 9:05 AM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi Daniel
>
> On Tue, Mar 21, 2023 at 07:42:53AM +0100, Daniel Semkowicz wrote:
> > Hi Jacopo,
> >
> > Thank you for spending your time on the review :)
> >
> > On Mon, Mar 20, 2023 at 8:25 PM Jacopo Mondi
> > <jacopo.mondi at ideasonboard.com> wrote:
> > >
> > > Hi Daniel
> > >
> > > On Tue, Mar 14, 2023 at 03:48:26PM +0100, Daniel Semkowicz wrote:
> > > > Add information about focus lens position limits to the IPA session
> > > > configuration. These information can then be used by IPA algorithms
> > > > to know which focus positions are valid.
> > > >
> > > > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > > > ---
> > > >  src/ipa/rkisp1/ipa_context.cpp | 12 ++++++++++++
> > > >  src/ipa/rkisp1/ipa_context.h   |  5 +++++
> > > >  src/ipa/rkisp1/rkisp1.cpp      | 13 ++++++++++++-
> > > >  3 files changed, 29 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > > > index 9bbf3684..401c098f 100644
> > > > --- a/src/ipa/rkisp1/ipa_context.cpp
> > > > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > > > @@ -89,6 +89,18 @@ namespace libcamera::ipa::rkisp1 {
> > > >   * \brief Sensor output resolution
> > > >   */
> > > >
> > > > +/**
> > > > + * \var IPASessionConfiguration::lens
> > > > + * \brief Lens-specific information
> > > > + *
> > > > + * \var IPASessionConfiguration::lens.minFocusPosition
> > > > + * \brief Minimum position supported by the camera focus lens
> > > > + *
> > > > + * \var IPASessionConfiguration::lens.maxFocusPosition
> > > > + * \brief Maximum position supported by the camera focus lens
> > >
> > > This doesn't report units, but the ::sensor fields don't either
> >
> > I am just exposing raw values from the v4l subnode, so it is not even
> > possible to know if any unit is assigned to the values.
> > And until we have a standardized focus values in libcamera, it will be
> > difficult to do it differently.
> >
> > >
> > > > + *
> > >
> > > Additional blank line

Regarding the additional blank line, it is intentional. For some reason
doxygen does not include the last field "maxFocusPosition" in the
documetnation without this blank line at the end of the block with
multiples fields.

This behaves the same for other already existing documentation blocks.
Please see for example: IPASessionConfiguration::grid.stride from the
ipu3/ipa_context.cpp. Other fields above like "bdsOutputSize" are
correctly visible in the doxygen html docs, however stride brief
description is not included.

Best regards
Daniel


> > >
> > > > + */
> > > > +
> > > >  /**
> > > >   * \var IPASessionConfiguration::raw
> > > >   * \brief Indicates if the camera is configured to capture raw frames
> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > index b9b20653..bfb6e1b7 100644
> > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > @@ -45,6 +45,11 @@ struct IPASessionConfiguration {
> > > >               Size size;
> > > >       } sensor;
> > > >
> > > > +     struct {
> > > > +             int32_t minFocusPosition;
> > > > +             int32_t maxFocusPosition;
> > > > +     } lens;
> > > > +
> > > >       struct {
> > > >               rkisp1_cif_isp_version revision;
> > > >       } hw;
> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > index 248cf5e0..cd1fbae3 100644
> > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > @@ -267,9 +267,20 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> > > >                       return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> > > >               });
> > > >
> > > > -     if (!ipaConfig.lensControls.empty())
> > > > +     if (!ipaConfig.lensControls.empty()) {
> > > >               lensControls_ = ipaConfig.lensControls;
> > > >
> > > > +             const ControlInfo focusAbsolute =
> > > > +                     lensControls_->find(V4L2_CID_FOCUS_ABSOLUTE)->second;
> > >
> > > Can we assume V4L2_CID_FOCUS_ABSOLUTE is there ? I would fail, as it
> > > seems to me neither the CameraSensor nor the pipeline make sure the
> > > control is there..
> >
> > There is a CameraLens::validateLensDriver() function to assure there
> > is a V4L2_CID_FOCUS_ABSOLUTE control, and it's called on
> > CameraLens::init(). If CameraLens::init() called in the
> > CameraSensor::discoverAncillaryDevices() returns error, the focusLens_
> > pointer is reset. This means the focusLens_ pointer in CameraLens is
> > set only if lens have a V4L2_CID_FOCUS_ABSOLUTE control. I base on this
> > behaviour to expose lens controls from pipeline to the IPA only if the
> > pointer was set. It is a bit hidden under several layers, but I tried
> > to avoid checking the same thing in different places.
> >
>
> Good point, you're right, please ignore the comment then ;)
>
> > >
> > > > +
> > > > +             LOG(IPARkISP1, Debug)
> > > > +                     << "Focus lens: " << focusAbsolute.toString();
> > > > +
> > > > +             auto &lensConfig = context_.configuration.lens;
> > > > +             lensConfig.minFocusPosition = focusAbsolute.min().get<int32_t>();
> > > > +             lensConfig.maxFocusPosition = focusAbsolute.max().get<int32_t>();
> > > > +     }
> > > > +
> > > >       for (auto const &a : algorithms()) {
> > > >               Algorithm *algo = static_cast<Algorithm *>(a.get());
> > > >
> > > > --
> > > > 2.39.2
> > > >
> >
> > Best regards
> > Daniel