[libcamera-devel] [PATCH v2 12/12] libcamera: pipeline: raspberrypi: Start IPA when starting camera

[Laurent Pinchart]

Hi Naush,

On Wed, Jul 08, 2020 at 12:12:58PM +0100, Naushir Patuck wrote:
> On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart wrote:
> >
> > The IPA is meant to be started when starting the camera, and stopped
> > when stopping it. It was so far start early in order to handle the
> > IPAInterface::processEvent() call related to lens shading table
> > allocation before IPAInterface::configure() to pass the table to the
> > IPA. Now that the lens shading table is passed through configure(),
> > starting the IPA early isn't needed anymore.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> 
> One question, what is the overhead of ipa_->start() and ipa_->stop().
> If we have to do this on a mode switch preparing for a capture (where
> we call pipeline_handler::stop() and pipeline_handler::start()), it
> might increase capture latency.  Perhaps this does not matter, or we
> can address if it is indeed a problem.  Otherwise,

There's a small overhead, but I think it's offset by
IPAInterface::configure() becoming a direct call instead of a
cross-thread call. It may be more of a problem for closed IPA modules
isolated in a separate process though, it's a good point. I could
reconsider that, but I'd rather first align all implementations on the
current model, and then rework start/stop (possibly with the addition of
prepare/finish to handle the expensive operations that are not related
to starting and stopping the camera itself) on top.

> Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> 
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 31 ++++++++++---------
> >  1 file changed, 17 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 9babf9f4a19c..c877820a6e1e 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -303,10 +303,6 @@ public:
> >                         vcsm_.free(lsTable_);
> >                         lsTable_ = nullptr;
> >                 }
> > -
> > -               /* Stop the IPA proxy thread. */
> > -               if (ipa_)
> > -                       ipa_->stop();
> >         }
> >
> >         void frameStarted(uint32_t sequence);
> > @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera)
> >         ret = queueAllBuffers(camera);
> >         if (ret) {
> >                 LOG(RPI, Error) << "Failed to queue buffers";
> > +               stop(camera);
> > +               return ret;
> > +       }
> > +
> > +       /* Start the IPA. */
> > +       ret = data->ipa_->start();
> > +       if (ret) {
> > +               LOG(RPI, Error)
> > +                       << "Failed to start IPA for " << camera->name();
> > +               stop(camera);
> >                 return ret;
> >         }
> >
> > @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera)
> >         V4L2DeviceFormat sensorFormat;
> >         data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);
> >         ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);
> > -       if (ret)
> > +       if (ret) {
> > +               stop(camera);
> >                 return ret;
> > +       }
> >
> >         /* Enable SOF event generation. */
> >         data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);
> > @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera)
> >         data->bayerQueue_ = std::queue<FrameBuffer *>{};
> >         data->embeddedQueue_ = std::queue<FrameBuffer *>{};
> >
> > +       /* Stop the IPA. */
> > +       data->ipa_->stop();
> > +
> >         freeBuffers(camera);
> >  }
> >
> > @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA()
> >                 .configurationFile = ipa_->configurationFile(sensor_->model() + ".json")
> >         };
> >
> > -       ipa_->init(settings);
> > -
> > -       /*
> > -        * Startup the IPA thread now. Without this call, none of the IPA API
> > -        * functions will run.
> > -        *
> > -        * It only gets stopped in the class destructor.
> > -        */
> > -       return ipa_->start();
> > +       return ipa_->init(settings);
> >  }
> >
> >  int RPiCameraData::configureIPA()

-- 
Regards,

Laurent Pinchart