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

[Jacopo Mondi]

Hi Laurent,

On Mon, Jul 06, 2020 at 02:08:57PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Jul 06, 2020 at 11:30:11AM +0200, Jacopo Mondi wrote:
> > On Sat, Jul 04, 2020 at 03:52:27AM +0300, 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
> >
> > s/start/started ?
>
> Indeed.
>
> > > 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>
> > > ---
> > >  .../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);
> >
> > Do we want to stop the whole camera or just the IPA here ?
> > I think you meant data->ipa_->stop(), right ?
>
> The idea is that the PipelineHandlerRPi::stop() function should clean up
> everything that start() may have done, and can be called in start() when
> an error occurs. To make this easier I need IPAProxy::stop() to be safe
> to call when the IPA is already stopped. I'll send a patch to do so.
>

Does this mean all the error paths in the start() function should call
stop() ?

> > With this fixed
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >

Please reain my tag here!

Thanks
  j

> > >  		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