[libcamera-devel] [PATCH v1 6/9] ipa: raspberrypi: Pass lens shading table through configure() function

[Laurent Pinchart]

Hi Naush,

On Tue, Jun 30, 2020 at 11:38:21AM +0100, Naushir Patuck wrote:
> On Mon, 29 Jun 2020 at 00:19, Laurent Pinchart wrote:
> >
> > The IPAInterface now accepts custom configuration data. Use it to pass
> > the lens shading table instead of using a custom IPA event. This will
> > allow starting the IPA when starting the camera, instead of pre-starting
> > it early in order to process the lens shading table allocation event.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.h                |  5 ++++-
> >  src/ipa/raspberrypi/raspberrypi.cpp                | 12 ++++++------
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------
> >  3 files changed, 14 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index a18ce9a884b6..c335d0259549 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -10,6 +10,10 @@
> >  #include <libcamera/control_ids.h>
> >  #include <libcamera/controls.h>
> >
> > +enum RPiConfigParameters {
> > +       RPI_IPA_CONFIG_LSTABLE = (1 << 0),
> > +};
> 
> Minor thing, but could you rename this to RPI_IPA_CONFIG_LS_TABLE to
> be consistent with other labels?

Sure, will do.

> Otherwise all looks good.
> 
> Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> 
> > +
> >  enum RPiOperations {
> >         RPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,
> >         RPI_IPA_ACTION_V4L2_SET_ISP,
> > @@ -21,7 +25,6 @@ enum RPiOperations {
> >         RPI_IPA_EVENT_SIGNAL_STAT_READY,
> >         RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
> >         RPI_IPA_EVENT_QUEUE_REQUEST,
> > -       RPI_IPA_EVENT_LS_TABLE_ALLOCATION,
> >  };
> >
> >  enum RPiIpaMask {
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 860be22ddb5d..c9f7dea375a5 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -277,6 +277,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >                 applyAGC(&agcStatus);
> >
> >         lastMode_ = mode_;
> > +
> > +       /* Store the lens shading table pointer and handle if available. */
> > +       if (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {
> > +               lsTable_ = reinterpret_cast<void *>(ipaConfig.data[0]);
> > +               lsTableHandle_ = ipaConfig.data[1];
> > +       }
> >  }
> >
> >  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> > @@ -358,12 +364,6 @@ void IPARPi::processEvent(const IPAOperationData &event)
> >                 break;
> >         }
> >
> > -       case RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {
> > -               lsTable_ = reinterpret_cast<void *>(event.data[0]);
> > -               lsTableHandle_ = event.data[1];
> > -               break;
> > -       }
> > -
> >         default:
> >                 LOG(IPARPI, Error) << "Unknown event " << event.operation;
> >                 break;
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 0f9237a7f346..903796790f44 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1109,6 +1109,7 @@ int RPiCameraData::configureIPA()
> >  {
> >         std::map<unsigned int, IPAStream> streamConfig;
> >         std::map<unsigned int, const ControlInfoMap &> entityControls;
> > +       IPAOperationData ipaConfig = {};
> >
> >         /* Get the device format to pass to the IPA. */
> >         V4L2DeviceFormat sensorFormat;
> > @@ -1138,11 +1139,9 @@ int RPiCameraData::configureIPA()
> >                  * The vcsm allocation will always be in the memory region
> >                  * < 32-bits to allow Videocore to access the memory.
> >                  */
> > -               IPAOperationData op;
> > -               op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;
> > -               op.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> > -                           vcsm_.getVCHandle(lsTable_) };
> > -               ipa_->processEvent(op);
> > +               ipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;
> > +               ipaConfig.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> > +                                  vcsm_.getVCHandle(lsTable_) };
> >         }
> >
> >         CameraSensorInfo sensorInfo = {};
> > @@ -1153,7 +1152,6 @@ int RPiCameraData::configureIPA()
> >         }
> >
> >         /* Ready the IPA - it must know about the sensor resolution. */
> > -       IPAOperationData ipaConfig;
> >         ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> >                         nullptr);
> >

-- 
Regards,

Laurent Pinchart