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

[Jacopo Mondi]

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
> >
> > > + */
> > > +
> > >  /**
> > >   * \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