[libcamera-devel] [PATCH v1 5/9] libcamera: pipeline: raspberrypi: Move configureIPA() to RPiCameraData

[Naushir Patuck]

Hi Laurent,

Thank you for the patch.

On Mon, 29 Jun 2020 at 15:33, Niklas Söderlund
<niklas.soderlund at ragnatech.se> wrote:
>
> Hi Laurent,
>
> Thanks for your work.
>
> On 2020-06-29 02:19:30 +0300, Laurent Pinchart wrote:
> > The PipelineHandlerRPi::configureIPA() function accesses plenty of
> > member data from the RPiCameraData class and no member from the
> > PipelineHandlerRPi class. Move it to RPiCameraData where it logically
> > belongs.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

Reviewed-by: Naushir Patuck <naush at raspberrypi.com>

>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 117 +++++++++---------
> >  1 file changed, 58 insertions(+), 59 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 3b5cdf1e1849..0f9237a7f346 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -311,6 +311,8 @@ public:
> >       void frameStarted(uint32_t sequence);
> >
> >       int loadIPA();
> > +     int configureIPA();
> > +
> >       void queueFrameAction(unsigned int frame, const IPAOperationData &action);
> >
> >       /* bufferComplete signal handlers. */
> > @@ -396,8 +398,6 @@ private:
> >               return static_cast<RPiCameraData *>(PipelineHandler::cameraData(camera));
> >       }
> >
> > -     int configureIPA(Camera *camera);
> > -
> >       int queueAllBuffers(Camera *camera);
> >       int prepareBuffers(Camera *camera);
> >       void freeBuffers(Camera *camera);
> > @@ -752,7 +752,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >       crop.y = (sensorFormat.size.height - crop.height) >> 1;
> >       data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> >
> > -     ret = configureIPA(camera);
> > +     ret = data->configureIPA();
> >       if (ret)
> >               LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
> >
> > @@ -968,62 +968,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> >       return true;
> >  }
> >
> > -int PipelineHandlerRPi::configureIPA(Camera *camera)
> > -{
> > -     std::map<unsigned int, IPAStream> streamConfig;
> > -     std::map<unsigned int, const ControlInfoMap &> entityControls;
> > -     RPiCameraData *data = cameraData(camera);
> > -
> > -     /* Get the device format to pass to the IPA. */
> > -     V4L2DeviceFormat sensorFormat;
> > -     data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);
> > -     /* Inform IPA of stream configuration and sensor controls. */
> > -     unsigned int i = 0;
> > -     for (auto const &stream : data->isp_) {
> > -             if (stream.isExternal()) {
> > -                     streamConfig[i] = {
> > -                             .pixelFormat = stream.configuration().pixelFormat,
> > -                             .size = stream.configuration().size
> > -                     };
> > -             }
> > -     }
> > -     entityControls.emplace(0, data->unicam_[Unicam::Image].dev()->controls());
> > -     entityControls.emplace(1, data->isp_[Isp::Input].dev()->controls());
> > -
> > -     /* Allocate the lens shading table via vcsm and pass to the IPA. */
> > -     if (!data->lsTable_) {
> > -             data->lsTable_ = data->vcsm_.alloc("ls_grid", MAX_LS_GRID_SIZE);
> > -             uintptr_t ptr = reinterpret_cast<uintptr_t>(data->lsTable_);
> > -
> > -             if (!data->lsTable_)
> > -                     return -ENOMEM;
> > -
> > -             /*
> > -              * 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),
> > -                         data->vcsm_.getVCHandle(data->lsTable_) };
> > -             data->ipa_->processEvent(op);
> > -     }
> > -
> > -     CameraSensorInfo sensorInfo = {};
> > -     int ret = data->sensor_->sensorInfo(&sensorInfo);
> > -     if (ret) {
> > -             LOG(RPI, Error) << "Failed to retrieve camera sensor info";
> > -             return ret;
> > -     }
> > -
> > -     /* Ready the IPA - it must know about the sensor resolution. */
> > -     IPAOperationData ipaConfig;
> > -     data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > -                           ipaConfig, nullptr);
> > -
> > -     return 0;
> > -}
> > -
> >  int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> >  {
> >       RPiCameraData *data = cameraData(camera);
> > @@ -1161,6 +1105,61 @@ int RPiCameraData::loadIPA()
> >       return ipa_->start();
> >  }
> >
> > +int RPiCameraData::configureIPA()
> > +{
> > +     std::map<unsigned int, IPAStream> streamConfig;
> > +     std::map<unsigned int, const ControlInfoMap &> entityControls;
> > +
> > +     /* Get the device format to pass to the IPA. */
> > +     V4L2DeviceFormat sensorFormat;
> > +     unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);
> > +     /* Inform IPA of stream configuration and sensor controls. */
> > +     unsigned int i = 0;
> > +     for (auto const &stream : isp_) {
> > +             if (stream.isExternal()) {
> > +                     streamConfig[i] = {
> > +                             .pixelFormat = stream.configuration().pixelFormat,
> > +                             .size = stream.configuration().size
> > +                     };
> > +             }
> > +     }
> > +     entityControls.emplace(0, unicam_[Unicam::Image].dev()->controls());
> > +     entityControls.emplace(1, isp_[Isp::Input].dev()->controls());
> > +
> > +     /* Allocate the lens shading table via vcsm and pass to the IPA. */
> > +     if (!lsTable_) {
> > +             lsTable_ = vcsm_.alloc("ls_grid", MAX_LS_GRID_SIZE);
> > +             uintptr_t ptr = reinterpret_cast<uintptr_t>(lsTable_);
> > +
> > +             if (!lsTable_)
> > +                     return -ENOMEM;
> > +
> > +             /*
> > +              * 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);
> > +     }
> > +
> > +     CameraSensorInfo sensorInfo = {};
> > +     int ret = sensor_->sensorInfo(&sensorInfo);
> > +     if (ret) {
> > +             LOG(RPI, Error) << "Failed to retrieve camera sensor info";
> > +             return ret;
> > +     }
> > +
> > +     /* Ready the IPA - it must know about the sensor resolution. */
> > +     IPAOperationData ipaConfig;
> > +     ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> > +                     nullptr);
> > +
> > +     return 0;
> > +}
> > +
> >  void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData &action)
> >  {
> >       /*
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel