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

[Laurent Pinchart]

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.

> With this fixed
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> >  		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