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

[Naushir Patuck]

Hi Laurent,

Thank you for the patch.

On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> 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,

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
>