[libcamera-devel] [PATCH v3 3/4] pipeline: raspberrypi: Only enabled embedded stream when available

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

s/enabled/enable/ in the subject ?

On Thu, Feb 18, 2021 at 05:01:25PM +0000, Naushir Patuck wrote:
> The pipeline handler would enable and use the Unicam embedded data stream
> even if the sensor did not support it. This was to allow a means to
> pass exposure and gain values for the frame to the IPA in a synchronised
> way.
> 
> The recent changes to get the pipeline handler to pass a ControlList
> with exposure and gain values means this is no longer required. Disable
> the use of the embedded data stream when a sensor does not support it.

Nice :-)

> This change also removes the mappedEmbeddedBuffers_ map as it is no
> longer used.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 112 +++++++++++-------
>  1 file changed, 70 insertions(+), 42 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index b43d86166c63..d969c77993eb 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -177,12 +177,6 @@ public:
>  	/* Stores the ids of the buffers mapped in the IPA. */
>  	std::unordered_set<unsigned int> ipaBuffers_;
>  
> -	/*
> -	 * Map of (internal) mmaped embedded data buffers, to avoid having to
> -	 * map/unmap on every frame.
> -	 */
> -	std::map<unsigned int, MappedFrameBuffer> mappedEmbeddedBuffers_;
> -
>  	/* DMAHEAP allocation helper. */
>  	RPi::DmaHeap dmaHeap_;
>  	FileDescriptor lsTable_;
> @@ -636,14 +630,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  
>  		if (isRaw(cfg.pixelFormat)) {
>  			cfg.setStream(&data->unicam_[Unicam::Image]);
> -			/*
> -			 * We must set both Unicam streams as external, even
> -			 * though the application may only request RAW frames.
> -			 * This is because we match timestamps on both streams
> -			 * to synchronise buffers.
> -			 */
>  			data->unicam_[Unicam::Image].setExternal(true);
> -			data->unicam_[Unicam::Embedded].setExternal(true);
>  			continue;
>  		}
>  
> @@ -715,17 +702,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		return ret;
>  	}
>  
> -	/* Unicam embedded data output format. */
> -	format = {};
> -	format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA);
> -	LOG(RPI, Debug) << "Setting embedded data format.";
> -	ret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format);
> -	if (ret) {
> -		LOG(RPI, Error) << "Failed to set format on Unicam embedded: "
> -				<< format.toString();
> -		return ret;
> -	}
> -
>  	/* Figure out the smallest selection the ISP will allow. */
>  	Rectangle testCrop(0, 0, 1, 1);
>  	data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);
> @@ -742,6 +718,41 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	if (ret)
>  		LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
>  
> +	/*
> +	 * The IPA will set data->sensorMetadata_ to true if embedded data is
> +	 * supported on this sensor. If so, open the Unicam embedded data
> +	 * node and configure the output format.
> +	 */
> +	if (data->sensorMetadata_) {
> +		format = {};
> +		format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA);
> +		LOG(RPI, Debug) << "Setting embedded data format.";
> +		data->unicam_[Unicam::Embedded].dev()->open();

The device is opened here, but never closed. Should that be fixed ? What
are the drawbacks in opening the device in match() as done today, even
if not used ?

> +		ret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format);
> +		if (ret) {
> +			LOG(RPI, Error) << "Failed to set format on Unicam embedded: "
> +					<< format.toString();
> +			return ret;
> +		}
> +
> +		/*
> +		 * If a RAW/Bayer stream has been requested by the application,
> +		 * we must set both Unicam streams as external, even though the
> +		 * application may only request RAW frames. This is because we
> +		 * match timestamps on both streams to synchronise buffers.
> +		 */
> +		if (rawStream)
> +			data->unicam_[Unicam::Embedded].setExternal(true);
> +	} else {
> +		/*
> +		 * No embedded data present, so we do not want to iterate over
> +		 * the embedded data stream when starting and stopping.
> +		 */
> +		data->streams_.erase(std::remove(data->streams_.begin(), data->streams_.end(),
> +						 &data->unicam_[Unicam::Embedded]),
> +				     data->streams_.end());

Hmmmm... Given that only one sensor, and thus one camera, is supported
by this pipeline handler, this should work, but conceptually it's not
very nice. Isn't this a decision that should be made at match() time,
not configure() time ? It would be right to do this here if the code was
modelled to support multiple sensors, but in that case we would need to
close() the embedded data video node in appropriate locations, and also
add the embedded stream back to data->streams_ when metadata is present.

One option to move this to match() time would be for the IPA to return
if the sensor supports metadata from init() instead of configure(). That
would require passing the sensor name to the IPA in init() too.

It's the mix n' match that bothers me I think. I don't mind if we decide
to model the pipeline handler and IPA with the assumption that there can
only be a single camera, with a hardcoded pipeline known from the very
beginning, or in a more dynamic way that could allow runtime switching
between sensors, but it would be nice if the architecture of the
pipeline handler and IPA was consistent relatively to the model we pick.
Does this make sense ?

> +	}
> +
>  	/*
>  	 * Update the ScalerCropMaximum to the correct value for this camera mode.
>  	 * For us, it's the same as the "analogue crop".
> @@ -949,10 +960,16 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>  	for (auto &stream : data->isp_)
>  		data->streams_.push_back(&stream);
>  
> -	/* Open all Unicam and ISP streams. */
> +	/*
> +	 * Open all Unicam and ISP streams. The exception is the embedded data
> +	 * stream, which only gets opened if the IPA reports that the sensor
> +	 * supports embedded data. This happens in RPiCameraData::configureIPA().
> +	 */
>  	for (auto const stream : data->streams_) {
> -		if (stream->dev()->open())
> -			return false;
> +		if (stream != &data->unicam_[Unicam::Embedded]) {
> +			if (stream->dev()->open())
> +				return false;
> +		}
>  	}
>  
>  	/* Wire up all the buffer connections. */
> @@ -1109,19 +1126,13 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  			return ret;
>  	}
>  
> -	if (!data->sensorMetadata_) {
> -		for (auto const &it : data->unicam_[Unicam::Embedded].getBuffers()) {
> -			MappedFrameBuffer fb(it.second, PROT_READ | PROT_WRITE);
> -			data->mappedEmbeddedBuffers_.emplace(it.first, std::move(fb));
> -		}
> -	}
> -
>  	/*
>  	 * Pass the stats and embedded data buffers to the IPA. No other
>  	 * buffers need to be passed.
>  	 */
>  	mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), ipa::RPi::MaskStats);
> -	mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), ipa::RPi::MaskEmbeddedData);
> +	if (data->sensorMetadata_)
> +		mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), ipa::RPi::MaskEmbeddedData);

Maybe a bit of line wrap ? :-)

>  
>  	return 0;
>  }
> @@ -1154,7 +1165,6 @@ void PipelineHandlerRPi::freeBuffers(Camera *camera)
>  	std::vector<unsigned int> ipaBuffers(data->ipaBuffers_.begin(), data->ipaBuffers_.end());
>  	data->ipa_->unmapBuffers(ipaBuffers);
>  	data->ipaBuffers_.clear();
> -	data->mappedEmbeddedBuffers_.clear();
>  
>  	for (auto const stream : data->streams_)
>  		stream->releaseBuffers();
> @@ -1652,7 +1662,7 @@ void RPiCameraData::tryRunPipeline()
>  
>  	/* If any of our request or buffer queues are empty, we cannot proceed. */
>  	if (state_ != State::Idle || requestQueue_.empty() ||
> -	    bayerQueue_.empty() || embeddedQueue_.empty())
> +	    bayerQueue_.empty() || (embeddedQueue_.empty() && sensorMetadata_))
>  		return;
>  
>  	if (!findMatchingBuffers(bayerFrame, embeddedBuffer))
> @@ -1675,17 +1685,24 @@ void RPiCameraData::tryRunPipeline()
>  	state_ = State::Busy;
>  
>  	unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);
> -	unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
>  
>  	LOG(RPI, Debug) << "Signalling signalIspPrepare:"
> -			<< " Bayer buffer id: " << bayerId
> -			<< " Embedded buffer id: " << embeddedId;
> +			<< " Bayer buffer id: " << bayerId;
>  
>  	ipa::RPi::ISPConfig ispPrepare;
> -	ispPrepare.embeddedBufferId = ipa::RPi::MaskEmbeddedData | embeddedId;
>  	ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId;
> -	ispPrepare.embeddedBufferPresent = true;
>  	ispPrepare.controls = std::move(bayerFrame.controls);
> +
> +	if (embeddedBuffer) {
> +		unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
> +
> +		ispPrepare.embeddedBufferId = ipa::RPi::MaskEmbeddedData | embeddedId;
> +		ispPrepare.embeddedBufferPresent = true;
> +
> +		LOG(RPI, Debug) << "Signalling signalIspPrepare:"
> +				<< " Bayer buffer id: " << embeddedId;
> +	}
> +
>  	ipa_->signalIspPrepare(ispPrepare);
>  }
>  
> @@ -1727,6 +1744,17 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em
>  
>  			LOG(RPI, Debug) << "Could not find matching embedded buffer";
>  
> +			if (!sensorMetadata_) {
> +				/*
> +				 * If there is no sensor metadata, simply return the
> +				 * first bayer frame in the queue.
> +				 */
> +				LOG(RPI, Debug) << "Returning bayer frame without a match";
> +				bayerQueue_.pop();
> +				embeddedBuffer = nullptr;
> +				return true;
> +			}
> +
>  			if (!embeddedQueue_.empty()) {
>  				/*
>  				 * Not found a matching embedded buffer for the bayer buffer in

-- 
Regards,

Laurent Pinchart