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

[Jacopo Mondi]

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.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

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