[libcamera-devel] [PATCH v2 3/7] pipeline: raspberrypi: Conditionally open the embedded data node

[Laurent Pinchart]

Hi Naush,

On Tue, Mar 23, 2021 at 12:27:35PM +0000, Naushir Patuck wrote:
> Conditionally open the embedded data node in pipeline_handler::match()
> based on whether the ipa::init() result reports if the sensor supports
> embedded data or not.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Tested-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 57 +++++++------------
>  1 file changed, 21 insertions(+), 36 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index ce1994186d66..4a3f7cbe3065 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -138,7 +138,7 @@ class RPiCameraData : public CameraData
>  {
>  public:
>  	RPiCameraData(PipelineHandler *pipe)
> -		: CameraData(pipe), embeddedNodeOpened_(false), state_(State::Stopped),
> +		: CameraData(pipe), state_(State::Stopped),
>  		  supportsFlips_(false), flipsAlterBayerOrder_(false),
>  		  updateScalerCrop_(true), dropFrameCount_(0), ispOutputCount_(0)
>  	{
> @@ -183,7 +183,6 @@ public:
>  
>  	std::unique_ptr<DelayedControls> delayedCtrls_;
>  	bool sensorMetadata_;
> -	bool embeddedNodeOpened_;
>  
>  	/*
>  	 * All the functions in this class are called from a single calling
> @@ -749,19 +748,13 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		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.
> +	 * Configure the Unicam embedded data output format only if the sensor
> +	 * supports it.
>  	 */
>  	if (data->sensorMetadata_) {
>  		format = {};
>  		format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA);
>  
> -		if (!data->embeddedNodeOpened_) {
> -			data->unicam_[Unicam::Embedded].dev()->open();
> -			data->embeddedNodeOpened_ = true;
> -		}
> -
>  		LOG(RPI, Debug) << "Setting embedded data format.";
>  		ret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format);
>  		if (ret) {
> @@ -778,14 +771,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		 */
>  		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());
>  	}
>  
>  	/*
> @@ -989,24 +974,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>  	data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"));
>  	data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"));
>  
> -	/* This is just for convenience so that we can easily iterate over all streams. */
> -	for (auto &stream : data->unicam_)
> -		data->streams_.push_back(&stream);
> -	for (auto &stream : data->isp_)
> -		data->streams_.push_back(&stream);
> -
> -	/*
> -	 * 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 != &data->unicam_[Unicam::Embedded]) {
> -			if (stream->dev()->open())
> -				return false;
> -		}
> -	}
> -
>  	/* Wire up all the buffer connections. */
>  	data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);
>  	data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue);
> @@ -1036,6 +1003,24 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>  		return false;
>  	}
>  
> +	/*
> +	 * Open all Unicam and ISP streams. The exception is the embedded data
> +	 * stream, which only gets opened below if the IPA reports that the sensor
> +	 * supports embedded data.
> +	 *
> +	 * The below grouping is just for convenience so that we can easily
> +	 * iterate over all streams in one go.
> +	 */
> +	data->streams_.push_back(&data->unicam_[Unicam::Image]);
> +	if (sensorConfig.sensorMetadata)
> +		data->streams_.push_back(&data->unicam_[Unicam::Embedded]);
> +
> +	for (auto &stream : data->isp_) {
> +		data->streams_.push_back(&stream);
> +		if (stream.dev()->open())
> +			return false;
> +	}

Unless I'm mistaken, you're not opening the unicam devices anymore.
That's why I had too loops in my proposal:

	for (auto &stream : data->isp_)
		data->streams_.push_back(&stream);

	for (auto stream : data->streams_) {
		if (stream->dev()->open())
			return false;
	}

If that's indeed the case, could you make sure to test the next version
? :-)

> +
>  	/*
>  	 * Setup our delayed control writer with the sensor default
>  	 * gain and exposure delays. Mark VBLANK for priority write.

-- 
Regards,

Laurent Pinchart