[libcamera-devel] [PATCH] ipa: rkisp1: ipa_context: Add camera data to IPAContext

[Daniel Semkowicz]

Hi Laurent, Hi Kieran, Hi Jacopo,

Thank you for this patch! This will simplify the work with AF and lens control.

On Sun, Apr 30, 2023 at 7:03 PM Jacopo Mondi via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> On Wed, Apr 26, 2023 at 04:10:37AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > The IPARkISP1 class caches data that is immutable over the life time of
> > the camera in private class members at init() time. This covers the
> > current needs to the IPA module, but doesn't provide a mechanism to
> > access that data from IPA algorithms. This will be needed for instance
> > by the upcoming auto-focus algorithm.
> >
> > To prepare for such use cases, create a new IPACameraData structure that
> > stores immutable camera data, and instantiate it in the IPAContext
> > structure. The IPACameraData structure is currently empty, new members
> > will be added as needed.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Patch looks good, should we already collect it or wait for Daniel to
> take it into his AF series ?

If existence of the empty structures in the code base is not an issue, then
it would be good to have this merged before the AF series. This makes
life easier
when pushing new versions of patch series.

>
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> > ---
> >
> > Bikeshedding on the IPACameraData and IPAContext::data names is welcome
> > :-)

I would like to take advantage of this opportunity and propose a small change :)
I think "data" is too general. If it is intended for storing camera data, then
We can just name it "camera" or "cameraData", so the typical usage will clearly
state the data hierarchy:

    context.camera.lens.minFocusPosition

The naming is secondary matter, and the logic part of the patch looks good,
so I am fine with it even without the name change:

Reviewed-by: Daniel Semkowicz <dse at thaumatec.com>

> >
> > ---
> >  src/ipa/rkisp1/ipa_context.cpp | 13 +++++++++++++
> >  src/ipa/rkisp1/ipa_context.h   |  4 ++++
> >  src/ipa/rkisp1/rkisp1.cpp      |  2 +-
> >  3 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 9bbf368432fa..986b955ea29d 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -14,6 +14,15 @@
> >
> >  namespace libcamera::ipa::rkisp1 {
> >
> > +/**
> > + * \struct IPACameraData
> > + * \brief Camera data for the IPA module
> > + *
> > + * The camera data contains all information about the camera that remains
> > + * constant for the lifetime of the IPA context. It is typically set during the
> > + * init() operation of the IPA module.
> > + */
> > +
> >  /**
> >   * \struct IPASessionConfiguration
> >   * \brief Session configuration for the IPA module
> > @@ -337,6 +346,10 @@ namespace libcamera::ipa::rkisp1 {
> >   * \struct IPAContext
> >   * \brief Global IPA context data shared between all algorithms
> >   *
> > + * \var IPAContext::data
> > + * \brief The IPA camera data, initialized at init time and immutable for the
> > + * lifetime of the context
> > + *
> >   * \var IPAContext::configuration
> >   * \brief The IPA session configuration, immutable during the session
> >   *
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index b9b2065328d6..53b79110c932 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -20,6 +20,9 @@ namespace libcamera {
> >
> >  namespace ipa::rkisp1 {
> >
> > +struct IPACameraData {
> > +};
> > +
> >  struct IPASessionConfiguration {
> >       struct {
> >               struct rkisp1_cif_isp_window measureWindow;
> > @@ -143,6 +146,7 @@ struct IPAFrameContext : public FrameContext {
> >  };
> >
> >  struct IPAContext {
> > +     IPACameraData data;
> >       IPASessionConfiguration configuration;
> >       IPAActiveState activeState;
> >
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 6544c925ba25..2fe66d34b6d4 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -111,7 +111,7 @@ const ControlInfoMap::Map rkisp1Controls{
> >  } /* namespace */
> >
> >  IPARkISP1::IPARkISP1()
> > -     : context_({ {}, {}, { kMaxFrameContexts } })
> > +     : context_({ {}, {}, {}, { kMaxFrameContexts } })
> >  {
> >  }
> >
> >
> > base-commit: 683c6da83f078d09fc020d2b48a4abe471853b2b
> > --
> > Regards,
> >
> > Laurent Pinchart
> >