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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jul 06, 2020 at 01:37:56PM +0200, Jacopo Mondi wrote:
> On Mon, Jul 06, 2020 at 02:08:57PM +0300, Laurent Pinchart wrote:
> > 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() ?

Yes, that's the idea.

> >> With this fixed
> >> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> Please reain my tag here!
> 
> >>>  		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