[libcamera-devel] [PATCH 13/13] pipeline: vc4: Connect/disconnect IPA and dequeue signals on start/stop

[Laurent Pinchart]

Hi Naush,

On Wed, Apr 26, 2023 at 02:10:57PM +0100, Naushir Patuck via libcamera-devel wrote:
> We currently rely on a state check to see if any of the IPA and buffer
> dequeue signal functions need to run. Replace this check by explicitly
> disconnecting the appropriate signals on camera stop. Re-connect the
> signals on camera start.

I'm a bit concerned about this. I've debugged an issue no later than
today where a race condition caused events to be processed after a stop
call. I briefly considered disabling the event notifier at stop time
(that's equivalent to disconnecting the signal here), but I then
realized that a quick stop/start cycle would reenable the notifier
before the event loop would get a chance to process and drop the event.
Disconnecting signals to ignore events makes state handling implicit,
and that in turn makes it more difficult to prove correctness of the
code. The resulting race conditions are also likely harder to debug as
they will occur less often.

> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/libcamera/pipeline/rpi/vc4/vc4.cpp | 54 +++++++++++---------------
>  1 file changed, 23 insertions(+), 31 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index bd7bfb3a7663..4b3f5a7fc9fe 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -315,11 +315,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer
>  
>  	/* An embedded data node will not be present if the sensor does not support it. */
>  	MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam-embedded");
> -	if (unicamEmbedded) {
> +	if (unicamEmbedded)
>  		data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded);
> -		data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data,
> -									   &Vc4CameraData::unicamBufferDequeue);
> -	}
>  
>  	/* Tag the ISP input stream as an import stream. */
>  	data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, Flag::ImportOnly);
> @@ -327,18 +324,9 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer
>  	data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2);
>  	data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3);
>  
> -	/* Wire up all the buffer connections. */
> -	data->unicam_[Unicam::Image].dev()->bufferReady.connect(data, &Vc4CameraData::unicamBufferDequeue);
> -	data->isp_[Isp::Input].dev()->bufferReady.connect(data, &Vc4CameraData::ispInputDequeue);
> -	data->isp_[Isp::Output0].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue);
> -	data->isp_[Isp::Output1].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue);
> -	data->isp_[Isp::Stats].dev()->bufferReady.connect(data, &Vc4CameraData::ispOutputDequeue);
> -
>  	if (data->sensorMetadata_ ^ !!data->unicam_[Unicam::Embedded].dev()) {
>  		LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!";
>  		data->sensorMetadata_ = false;
> -		if (data->unicam_[Unicam::Embedded].dev())
> -			data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();
>  	}
>  
>  	/*
> @@ -367,9 +355,7 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr<RPi::CameraData> &camer
>  		return -EINVAL;
>  	}
>  
> -	/* Write up all the IPA connections. */
> -	data->ipa_->processStatsComplete.connect(data, &Vc4CameraData::processStatsComplete);
> -	data->ipa_->prepareIspComplete.connect(data, &Vc4CameraData::prepareIspComplete);
> +	/* Wire up the default IPA connections. The others get connected on start() */
>  	data->ipa_->setIspControls.connect(data, &Vc4CameraData::setIspControls);
>  	data->ipa_->setCameraTimeout.connect(data, &Vc4CameraData::setCameraTimeout);
>  
> @@ -691,10 +677,31 @@ int Vc4CameraData::platformConfigureIpa(ipa::RPi::ConfigParams &params)
>  
>  void Vc4CameraData::platformStart()
>  {
> +	unicam_[Unicam::Image].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue);
> +	isp_[Isp::Input].dev()->bufferReady.connect(this, &Vc4CameraData::ispInputDequeue);
> +	isp_[Isp::Output0].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue);
> +	isp_[Isp::Output1].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue);
> +	isp_[Isp::Stats].dev()->bufferReady.connect(this, &Vc4CameraData::ispOutputDequeue);
> +	ipa_->processStatsComplete.connect(this, &Vc4CameraData::processStatsComplete);
> +	ipa_->prepareIspComplete.connect(this, &Vc4CameraData::prepareIspComplete);
> +
> +	if (sensorMetadata_)
> +		unicam_[Unicam::Embedded].dev()->bufferReady.connect(this, &Vc4CameraData::unicamBufferDequeue);
>  }
>  
>  void Vc4CameraData::platformStop()
>  {
> +	unicam_[Unicam::Image].dev()->bufferReady.disconnect();
> +	isp_[Isp::Input].dev()->bufferReady.disconnect();
> +	isp_[Isp::Output0].dev()->bufferReady.disconnect();
> +	isp_[Isp::Output1].dev()->bufferReady.disconnect();
> +	isp_[Isp::Stats].dev()->bufferReady.disconnect();
> +	ipa_->processStatsComplete.disconnect();
> +	ipa_->prepareIspComplete.disconnect();
> +
> +	if (sensorMetadata_)
> +		unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();
> +
>  	bayerQueue_ = {};
>  	embeddedQueue_ = {};
>  }
> @@ -704,9 +711,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer)
>  	RPi::Stream *stream = nullptr;
>  	unsigned int index;
>  
> -	if (!isRunning())
> -		return;
> -
>  	for (RPi::Stream &s : unicam_) {
>  		index = s.getBufferId(buffer);
>  		if (index) {
> @@ -743,9 +747,6 @@ void Vc4CameraData::unicamBufferDequeue(FrameBuffer *buffer)
>  
>  void Vc4CameraData::ispInputDequeue(FrameBuffer *buffer)
>  {
> -	if (!isRunning())
> -		return;
> -
>  	LOG(RPI, Debug) << "Stream ISP Input buffer complete"
>  			<< ", buffer id " << unicam_[Unicam::Image].getBufferId(buffer)
>  			<< ", timestamp: " << buffer->metadata().timestamp;
> @@ -760,9 +761,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer)
>  	RPi::Stream *stream = nullptr;
>  	unsigned int index;
>  
> -	if (!isRunning())
> -		return;
> -
>  	for (RPi::Stream &s : isp_) {
>  		index = s.getBufferId(buffer);
>  		if (index) {
> @@ -803,9 +801,6 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer)
>  
>  void Vc4CameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)
>  {
> -	if (!isRunning())
> -		return;
> -
>  	FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID);
>  
>  	handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> @@ -846,9 +841,6 @@ void Vc4CameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)
>  	unsigned int bayer = buffers.bayer & RPi::MaskID;
>  	FrameBuffer *buffer;
>  
> -	if (!isRunning())
> -		return;
> -
>  	buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);
>  	LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bayer & RPi::MaskID)
>  			<< ", timestamp: " << buffer->metadata().timestamp;

-- 
Regards,

Laurent Pinchart