[libcamera-devel,v4,2/2] pipeline: raspberrypi: Tidy the camera enumeration and registration logic
diff mbox series

Message ID 20211126153538.2594702-2-naush@raspberrypi.com
State Accepted
Commit 22574ff19545d96e8f873081d51fde2682a12293
Headers show
Series
  • [libcamera-devel,v4,1/2] pipeline: raspberrypi: Split out device enumeration and camera registration
Related show

Commit Message

Naushir Patuck Nov. 26, 2021, 3:35 p.m. UTC 
When acquiring the media device, it is not necessary to match all entity names,
so remove it. Aditionally, we do not need to keep the MediaEntity pointers for
the Unicam and ISP devices stored within the PipelineHandlerRPi class. Instead
these can be stored locally in PipelineHandlerRPi::match().

PipelineHandlerRPi::registerCamera() now returns an int error code instead of a
boolean for pass/fail.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 89 +++++++++++--------
 1 file changed, 52 insertions(+), 37 deletions(-)

Comments

Laurent Pinchart Nov. 26, 2021, 4:09 p.m. UTC | #1 
Hi Naush,

Thank you for the patch.

On Fri, Nov 26, 2021 at 03:35:38PM +0000, Naushir Patuck wrote:
> When acquiring the media device, it is not necessary to match all entity names,
> so remove it. Aditionally, we do not need to keep the MediaEntity pointers for
> the Unicam and ISP devices stored within the PipelineHandlerRPi class. Instead
> these can be stored locally in PipelineHandlerRPi::match().
> 
> PipelineHandlerRPi::registerCamera() now returns an int error code instead of a
> boolean for pass/fail.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 89 +++++++++++--------
>  1 file changed, 52 insertions(+), 37 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 5a6dfa4e8b2a..e31fa3b23859 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -314,14 +314,11 @@ private:
>  		return static_cast<RPiCameraData *>(camera->_d());
>  	}
>  
> -	bool registerCamera();
> +	int registerCamera(MediaDevice *unicam, MediaDevice *isp);
>  	int queueAllBuffers(Camera *camera);
>  	int prepareBuffers(Camera *camera);
>  	void freeBuffers(Camera *camera);
>  	void mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask);
> -
> -	MediaDevice *unicam_;
> -	MediaDevice *isp_;
>  };
>  
>  RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)
> @@ -512,7 +509,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  }
>  
>  PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)
> -	: PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)
> +	: PipelineHandler(manager)
>  {
>  }
>  
> @@ -997,48 +994,62 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
>  bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>  {
>  	DeviceMatch unicam("unicam");
> -	DeviceMatch isp("bcm2835-isp");
> +	MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);
>  
> -	unicam.add("unicam-image");
> +	if (!unicamDevice) {
> +		LOG(RPI, Debug) << "Unable to acquire a Unicam instance";
> +		return false;
> +	}
>  
> -	isp.add("bcm2835-isp0-output0"); /* Input */
> -	isp.add("bcm2835-isp0-capture1"); /* Output 0 */
> -	isp.add("bcm2835-isp0-capture2"); /* Output 1 */
> -	isp.add("bcm2835-isp0-capture3"); /* Stats */
> +	DeviceMatch isp("bcm2835-isp");
> +	MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);
>  
> -	unicam_ = acquireMediaDevice(enumerator, unicam);
> -	if (!unicam_)
> +	if (!ispDevice) {
> +		LOG(RPI, Debug) << "Unable to acquire ISP instance";
>  		return false;
> +	}
>  
> -	isp_ = acquireMediaDevice(enumerator, isp);
> -	if (!isp_)
> +	int ret = registerCamera(unicamDevice, ispDevice);
> +	if (ret) {
> +		LOG(RPI, Error) << "Failed to register camera: " << ret;
>  		return false;
> +	}
>  
> -	return registerCamera();
> +	return true;
>  }
>  
> -bool PipelineHandlerRPi::registerCamera()
> +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp)
>  {
>  	std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
> +
>  	if (!data->dmaHeap_.isValid())
> -		return false;
> +		return -ENOMEM;
> +
> +	MediaEntity *unicamImage = unicam->getEntityByName("unicam-image");
> +	MediaEntity *ispOutput0 = isp->getEntityByName("bcm2835-isp0-output0");
> +	MediaEntity *ispCapture1 = isp->getEntityByName("bcm2835-isp0-capture1");
> +	MediaEntity *ispCapture2 = isp->getEntityByName("bcm2835-isp0-capture2");
> +	MediaEntity *ispCapture3 = isp->getEntityByName("bcm2835-isp0-capture3");
> +
> +	if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3)
> +		return -ENOENT;
>  
>  	/* Locate and open the unicam video streams. */
> -	data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image"));
> +	data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicamImage);
>  
>  	/* An embedded data node will not be present if the sensor does not support it. */
> -	MediaEntity *embeddedEntity = unicam_->getEntityByName("unicam-embedded");
> -	if (embeddedEntity) {
> -		data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", embeddedEntity);
> +	MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam-embedded");
> +	if (unicamEmbedded) {
> +		data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded);
>  		data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),
>  									   &RPiCameraData::unicamBufferDequeue);
>  	}
>  
>  	/* Tag the ISP input stream as an import stream. */
> -	data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
> -	data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
> -	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"));
> +	data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, true);
> +	data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1);
> +	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()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);
> @@ -1049,7 +1060,7 @@ bool PipelineHandlerRPi::registerCamera()
>  	data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);
>  
>  	/* Identify the sensor. */
> -	for (MediaEntity *entity : unicam_->entities()) {
> +	for (MediaEntity *entity : unicam->entities()) {
>  		if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {
>  			data->sensor_ = std::make_unique<CameraSensor>(entity);
>  			break;
> @@ -1057,23 +1068,23 @@ bool PipelineHandlerRPi::registerCamera()
>  	}
>  
>  	if (!data->sensor_)
> -		return false;
> +		return -EINVAL;
>  
>  	if (data->sensor_->init())
> -		return false;
> +		return -EINVAL;
>  
>  	data->sensorFormats_ = populateSensorFormats(data->sensor_);
>  
>  	ipa::RPi::SensorConfig sensorConfig;
>  	if (data->loadIPA(&sensorConfig)) {
>  		LOG(RPI, Error) << "Failed to load a suitable IPA library";
> -		return false;
> +		return -EINVAL;
>  	}
>  
> -	if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {
> +	if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {
>  		LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!";
>  		sensorConfig.sensorMetadata = false;
> -		if (embeddedEntity)
> +		if (unicamEmbedded)
>  			data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();
>  	}
>  
> @@ -1093,13 +1104,14 @@ bool PipelineHandlerRPi::registerCamera()
>  		data->streams_.push_back(&stream);
>  
>  	for (auto stream : data->streams_) {
> -		if (stream->dev()->open())
> -			return false;
> +		int ret = stream->dev()->open();
> +		if (ret)
> +			return ret;
>  	}
>  
>  	if (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {
>  		LOG(RPI, Error) << "Unicam driver does not use the MediaController, please update your kernel!";
> -		return false;
> +		return -EINVAL;
>  	}
>  
>  	/*
> @@ -1161,7 +1173,7 @@ bool PipelineHandlerRPi::registerCamera()
>  
>  	if (!bayerFormat.isValid()) {
>  		LOG(RPI, Error) << "No Bayer format found";
> -		return false;
> +		return -EINVAL;
>  	}
>  	data->nativeBayerOrder_ = bayerFormat.order;
>  
> @@ -1181,7 +1193,10 @@ bool PipelineHandlerRPi::registerCamera()
>  		Camera::create(std::move(data), id, streams);
>  	PipelineHandler::registerCamera(std::move(camera));
>  
> -	return true;
> +	LOG(RPI, Info) << "Registered camera " << id
> +		       << " to Unicam device " << unicam->deviceNode()
> +		       << " and ISP device " << isp->deviceNode();
> +	return 0;
>  }
>  
>  int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
Kieran Bingham Nov. 29, 2021, 2:40 p.m. UTC | #2 
Quoting Naushir Patuck (2021-11-26 15:35:38)
> When acquiring the media device, it is not necessary to match all entity names,
> so remove it. Aditionally, we do not need to keep the MediaEntity pointers for
> the Unicam and ISP devices stored within the PipelineHandlerRPi class. Instead
> these can be stored locally in PipelineHandlerRPi::match().
> 
> PipelineHandlerRPi::registerCamera() now returns an int error code instead of a
> boolean for pass/fail.

This looks good. I think it's clearer even for a single camera too, and
it will be nice to see when we get multiple camera's running through.

I know of the StereoPi use case already, and I guess there will be
others with the CM4 using this.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 89 +++++++++++--------
>  1 file changed, 52 insertions(+), 37 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 5a6dfa4e8b2a..e31fa3b23859 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -314,14 +314,11 @@ private:
>                 return static_cast<RPiCameraData *>(camera->_d());
>         }
>  
> -       bool registerCamera();
> +       int registerCamera(MediaDevice *unicam, MediaDevice *isp);
>         int queueAllBuffers(Camera *camera);
>         int prepareBuffers(Camera *camera);
>         void freeBuffers(Camera *camera);
>         void mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask);
> -
> -       MediaDevice *unicam_;
> -       MediaDevice *isp_;
>  };
>  
>  RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)
> @@ -512,7 +509,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  }
>  
>  PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)
> -       : PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)
> +       : PipelineHandler(manager)
>  {
>  }
>  
> @@ -997,48 +994,62 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
>  bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>  {
>         DeviceMatch unicam("unicam");
> -       DeviceMatch isp("bcm2835-isp");
> +       MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);
>  
> -       unicam.add("unicam-image");
> +       if (!unicamDevice) {
> +               LOG(RPI, Debug) << "Unable to acquire a Unicam instance";
> +               return false;
> +       }
>  
> -       isp.add("bcm2835-isp0-output0"); /* Input */
> -       isp.add("bcm2835-isp0-capture1"); /* Output 0 */
> -       isp.add("bcm2835-isp0-capture2"); /* Output 1 */
> -       isp.add("bcm2835-isp0-capture3"); /* Stats */
> +       DeviceMatch isp("bcm2835-isp");
> +       MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);
>  
> -       unicam_ = acquireMediaDevice(enumerator, unicam);
> -       if (!unicam_)
> +       if (!ispDevice) {
> +               LOG(RPI, Debug) << "Unable to acquire ISP instance";
>                 return false;
> +       }
>  
> -       isp_ = acquireMediaDevice(enumerator, isp);
> -       if (!isp_)
> +       int ret = registerCamera(unicamDevice, ispDevice);
> +       if (ret) {
> +               LOG(RPI, Error) << "Failed to register camera: " << ret;
>                 return false;
> +       }
>  
> -       return registerCamera();
> +       return true;
>  }
>  
> -bool PipelineHandlerRPi::registerCamera()
> +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp)
>  {
>         std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
> +
>         if (!data->dmaHeap_.isValid())
> -               return false;
> +               return -ENOMEM;
> +
> +       MediaEntity *unicamImage = unicam->getEntityByName("unicam-image");
> +       MediaEntity *ispOutput0 = isp->getEntityByName("bcm2835-isp0-output0");
> +       MediaEntity *ispCapture1 = isp->getEntityByName("bcm2835-isp0-capture1");
> +       MediaEntity *ispCapture2 = isp->getEntityByName("bcm2835-isp0-capture2");
> +       MediaEntity *ispCapture3 = isp->getEntityByName("bcm2835-isp0-capture3");
> +
> +       if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3)
> +               return -ENOENT;
>  
>         /* Locate and open the unicam video streams. */
> -       data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image"));
> +       data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicamImage);
>  
>         /* An embedded data node will not be present if the sensor does not support it. */
> -       MediaEntity *embeddedEntity = unicam_->getEntityByName("unicam-embedded");
> -       if (embeddedEntity) {
> -               data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", embeddedEntity);
> +       MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam-embedded");
> +       if (unicamEmbedded) {
> +               data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded);
>                 data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),
>                                                                            &RPiCameraData::unicamBufferDequeue);
>         }
>  
>         /* Tag the ISP input stream as an import stream. */
> -       data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
> -       data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
> -       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"));
> +       data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, true);
> +       data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1);
> +       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()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);
> @@ -1049,7 +1060,7 @@ bool PipelineHandlerRPi::registerCamera()
>         data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);
>  
>         /* Identify the sensor. */
> -       for (MediaEntity *entity : unicam_->entities()) {
> +       for (MediaEntity *entity : unicam->entities()) {
>                 if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {
>                         data->sensor_ = std::make_unique<CameraSensor>(entity);
>                         break;
> @@ -1057,23 +1068,23 @@ bool PipelineHandlerRPi::registerCamera()
>         }
>  
>         if (!data->sensor_)
> -               return false;
> +               return -EINVAL;
>  
>         if (data->sensor_->init())
> -               return false;
> +               return -EINVAL;
>  
>         data->sensorFormats_ = populateSensorFormats(data->sensor_);
>  
>         ipa::RPi::SensorConfig sensorConfig;
>         if (data->loadIPA(&sensorConfig)) {
>                 LOG(RPI, Error) << "Failed to load a suitable IPA library";
> -               return false;
> +               return -EINVAL;
>         }
>  
> -       if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {
> +       if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {
>                 LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!";
>                 sensorConfig.sensorMetadata = false;
> -               if (embeddedEntity)
> +               if (unicamEmbedded)
>                         data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();
>         }
>  
> @@ -1093,13 +1104,14 @@ bool PipelineHandlerRPi::registerCamera()
>                 data->streams_.push_back(&stream);
>  
>         for (auto stream : data->streams_) {
> -               if (stream->dev()->open())
> -                       return false;
> +               int ret = stream->dev()->open();
> +               if (ret)
> +                       return ret;
>         }
>  
>         if (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {
>                 LOG(RPI, Error) << "Unicam driver does not use the MediaController, please update your kernel!";
> -               return false;
> +               return -EINVAL;
>         }
>  
>         /*
> @@ -1161,7 +1173,7 @@ bool PipelineHandlerRPi::registerCamera()
>  
>         if (!bayerFormat.isValid()) {
>                 LOG(RPI, Error) << "No Bayer format found";
> -               return false;
> +               return -EINVAL;
>         }
>         data->nativeBayerOrder_ = bayerFormat.order;
>  
> @@ -1181,7 +1193,10 @@ bool PipelineHandlerRPi::registerCamera()
>                 Camera::create(std::move(data), id, streams);
>         PipelineHandler::registerCamera(std::move(camera));
>  
> -       return true;
> +       LOG(RPI, Info) << "Registered camera " << id
> +                      << " to Unicam device " << unicam->deviceNode()
> +                      << " and ISP device " << isp->deviceNode();
> +       return 0;
>  }
>  
>  int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> -- 
> 2.25.1
>
Dave Stevenson Nov. 29, 2021, 2:51 p.m. UTC | #3 
On Mon, 29 Nov 2021 at 14:41, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Naushir Patuck (2021-11-26 15:35:38)
> > When acquiring the media device, it is not necessary to match all entity names,
> > so remove it. Aditionally, we do not need to keep the MediaEntity pointers for
> > the Unicam and ISP devices stored within the PipelineHandlerRPi class. Instead
> > these can be stored locally in PipelineHandlerRPi::match().
> >
> > PipelineHandlerRPi::registerCamera() now returns an int error code instead of a
> > boolean for pass/fail.
>
> This looks good. I think it's clearer even for a single camera too, and
> it will be nice to see when we get multiple camera's running through.
>
> I know of the StereoPi use case already, and I guess there will be
> others with the CM4 using this.

This is two simultaneous but independent cameras.
Stereoscopic requires a bit more work to be able to combine the output
of two ISP passes into one buffer for side-by-side, or top-bottom.
I suspect there will be a load of discussion required for how the
libcamera frameworks wish to support stereoscopic, and how IPAs are
expected to handle stereo.

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 89 +++++++++++--------
> >  1 file changed, 52 insertions(+), 37 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 5a6dfa4e8b2a..e31fa3b23859 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -314,14 +314,11 @@ private:
> >                 return static_cast<RPiCameraData *>(camera->_d());
> >         }
> >
> > -       bool registerCamera();
> > +       int registerCamera(MediaDevice *unicam, MediaDevice *isp);
> >         int queueAllBuffers(Camera *camera);
> >         int prepareBuffers(Camera *camera);
> >         void freeBuffers(Camera *camera);
> >         void mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask);
> > -
> > -       MediaDevice *unicam_;
> > -       MediaDevice *isp_;
> >  };
> >
> >  RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)
> > @@ -512,7 +509,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> >  }
> >
> >  PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)
> > -       : PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)
> > +       : PipelineHandler(manager)
> >  {
> >  }
> >
> > @@ -997,48 +994,62 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> >  bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> >  {
> >         DeviceMatch unicam("unicam");
> > -       DeviceMatch isp("bcm2835-isp");
> > +       MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);
> >
> > -       unicam.add("unicam-image");
> > +       if (!unicamDevice) {
> > +               LOG(RPI, Debug) << "Unable to acquire a Unicam instance";
> > +               return false;
> > +       }
> >
> > -       isp.add("bcm2835-isp0-output0"); /* Input */
> > -       isp.add("bcm2835-isp0-capture1"); /* Output 0 */
> > -       isp.add("bcm2835-isp0-capture2"); /* Output 1 */
> > -       isp.add("bcm2835-isp0-capture3"); /* Stats */
> > +       DeviceMatch isp("bcm2835-isp");
> > +       MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);
> >
> > -       unicam_ = acquireMediaDevice(enumerator, unicam);
> > -       if (!unicam_)
> > +       if (!ispDevice) {
> > +               LOG(RPI, Debug) << "Unable to acquire ISP instance";
> >                 return false;
> > +       }
> >
> > -       isp_ = acquireMediaDevice(enumerator, isp);
> > -       if (!isp_)
> > +       int ret = registerCamera(unicamDevice, ispDevice);
> > +       if (ret) {
> > +               LOG(RPI, Error) << "Failed to register camera: " << ret;
> >                 return false;
> > +       }
> >
> > -       return registerCamera();
> > +       return true;
> >  }
> >
> > -bool PipelineHandlerRPi::registerCamera()
> > +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp)
> >  {
> >         std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
> > +
> >         if (!data->dmaHeap_.isValid())
> > -               return false;
> > +               return -ENOMEM;
> > +
> > +       MediaEntity *unicamImage = unicam->getEntityByName("unicam-image");
> > +       MediaEntity *ispOutput0 = isp->getEntityByName("bcm2835-isp0-output0");
> > +       MediaEntity *ispCapture1 = isp->getEntityByName("bcm2835-isp0-capture1");
> > +       MediaEntity *ispCapture2 = isp->getEntityByName("bcm2835-isp0-capture2");
> > +       MediaEntity *ispCapture3 = isp->getEntityByName("bcm2835-isp0-capture3");
> > +
> > +       if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3)
> > +               return -ENOENT;
> >
> >         /* Locate and open the unicam video streams. */
> > -       data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image"));
> > +       data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicamImage);
> >
> >         /* An embedded data node will not be present if the sensor does not support it. */
> > -       MediaEntity *embeddedEntity = unicam_->getEntityByName("unicam-embedded");
> > -       if (embeddedEntity) {
> > -               data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", embeddedEntity);
> > +       MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam-embedded");
> > +       if (unicamEmbedded) {
> > +               data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded);
> >                 data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),
> >                                                                            &RPiCameraData::unicamBufferDequeue);
> >         }
> >
> >         /* Tag the ISP input stream as an import stream. */
> > -       data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
> > -       data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
> > -       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"));
> > +       data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, true);
> > +       data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1);
> > +       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()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);
> > @@ -1049,7 +1060,7 @@ bool PipelineHandlerRPi::registerCamera()
> >         data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);
> >
> >         /* Identify the sensor. */
> > -       for (MediaEntity *entity : unicam_->entities()) {
> > +       for (MediaEntity *entity : unicam->entities()) {
> >                 if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {
> >                         data->sensor_ = std::make_unique<CameraSensor>(entity);
> >                         break;
> > @@ -1057,23 +1068,23 @@ bool PipelineHandlerRPi::registerCamera()
> >         }
> >
> >         if (!data->sensor_)
> > -               return false;
> > +               return -EINVAL;
> >
> >         if (data->sensor_->init())
> > -               return false;
> > +               return -EINVAL;
> >
> >         data->sensorFormats_ = populateSensorFormats(data->sensor_);
> >
> >         ipa::RPi::SensorConfig sensorConfig;
> >         if (data->loadIPA(&sensorConfig)) {
> >                 LOG(RPI, Error) << "Failed to load a suitable IPA library";
> > -               return false;
> > +               return -EINVAL;
> >         }
> >
> > -       if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {
> > +       if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {
> >                 LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!";
> >                 sensorConfig.sensorMetadata = false;
> > -               if (embeddedEntity)
> > +               if (unicamEmbedded)
> >                         data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();
> >         }
> >
> > @@ -1093,13 +1104,14 @@ bool PipelineHandlerRPi::registerCamera()
> >                 data->streams_.push_back(&stream);
> >
> >         for (auto stream : data->streams_) {
> > -               if (stream->dev()->open())
> > -                       return false;
> > +               int ret = stream->dev()->open();
> > +               if (ret)
> > +                       return ret;
> >         }
> >
> >         if (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {
> >                 LOG(RPI, Error) << "Unicam driver does not use the MediaController, please update your kernel!";
> > -               return false;
> > +               return -EINVAL;
> >         }
> >
> >         /*
> > @@ -1161,7 +1173,7 @@ bool PipelineHandlerRPi::registerCamera()
> >
> >         if (!bayerFormat.isValid()) {
> >                 LOG(RPI, Error) << "No Bayer format found";
> > -               return false;
> > +               return -EINVAL;
> >         }
> >         data->nativeBayerOrder_ = bayerFormat.order;
> >
> > @@ -1181,7 +1193,10 @@ bool PipelineHandlerRPi::registerCamera()
> >                 Camera::create(std::move(data), id, streams);
> >         PipelineHandler::registerCamera(std::move(camera));
> >
> > -       return true;
> > +       LOG(RPI, Info) << "Registered camera " << id
> > +                      << " to Unicam device " << unicam->deviceNode()
> > +                      << " and ISP device " << isp->deviceNode();
> > +       return 0;
> >  }
> >
> >  int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> > --
> > 2.25.1
> >
Kieran Bingham Nov. 29, 2021, 11:15 p.m. UTC | #4 
Quoting Dave Stevenson (2021-11-29 14:51:08)
> On Mon, 29 Nov 2021 at 14:41, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Quoting Naushir Patuck (2021-11-26 15:35:38)
> > > When acquiring the media device, it is not necessary to match all entity names,
> > > so remove it. Aditionally, we do not need to keep the MediaEntity pointers for
> > > the Unicam and ISP devices stored within the PipelineHandlerRPi class. Instead
> > > these can be stored locally in PipelineHandlerRPi::match().
> > >
> > > PipelineHandlerRPi::registerCamera() now returns an int error code instead of a
> > > boolean for pass/fail.
> >
> > This looks good. I think it's clearer even for a single camera too, and
> > it will be nice to see when we get multiple camera's running through.
> >
> > I know of the StereoPi use case already, and I guess there will be
> > others with the CM4 using this.
> 
> This is two simultaneous but independent cameras.
> Stereoscopic requires a bit more work to be able to combine the output
> of two ISP passes into one buffer for side-by-side, or top-bottom.
> I suspect there will be a load of discussion required for how the
> libcamera frameworks wish to support stereoscopic, and how IPAs are
> expected to handle stereo.

Ah, yes I can see keeping them in sync will take more work than just
capturing the same frames and having frame-sync between them. We'll have
to make sure the IPA's either communicate, or are consistent between the
two cameras.

Well, that's for later, this series is merged, so at least we can
capture from both cameras.

Thanks.

Kieran


> 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 89 +++++++++++--------
> > >  1 file changed, 52 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 5a6dfa4e8b2a..e31fa3b23859 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -314,14 +314,11 @@ private:
> > >                 return static_cast<RPiCameraData *>(camera->_d());
> > >         }
> > >
> > > -       bool registerCamera();
> > > +       int registerCamera(MediaDevice *unicam, MediaDevice *isp);
> > >         int queueAllBuffers(Camera *camera);
> > >         int prepareBuffers(Camera *camera);
> > >         void freeBuffers(Camera *camera);
> > >         void mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask);
> > > -
> > > -       MediaDevice *unicam_;
> > > -       MediaDevice *isp_;
> > >  };
> > >
> > >  RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)
> > > @@ -512,7 +509,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > >  }
> > >
> > >  PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)
> > > -       : PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)
> > > +       : PipelineHandler(manager)
> > >  {
> > >  }
> > >
> > > @@ -997,48 +994,62 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> > >  bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> > >  {
> > >         DeviceMatch unicam("unicam");
> > > -       DeviceMatch isp("bcm2835-isp");
> > > +       MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);
> > >
> > > -       unicam.add("unicam-image");
> > > +       if (!unicamDevice) {
> > > +               LOG(RPI, Debug) << "Unable to acquire a Unicam instance";
> > > +               return false;
> > > +       }
> > >
> > > -       isp.add("bcm2835-isp0-output0"); /* Input */
> > > -       isp.add("bcm2835-isp0-capture1"); /* Output 0 */
> > > -       isp.add("bcm2835-isp0-capture2"); /* Output 1 */
> > > -       isp.add("bcm2835-isp0-capture3"); /* Stats */
> > > +       DeviceMatch isp("bcm2835-isp");
> > > +       MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);
> > >
> > > -       unicam_ = acquireMediaDevice(enumerator, unicam);
> > > -       if (!unicam_)
> > > +       if (!ispDevice) {
> > > +               LOG(RPI, Debug) << "Unable to acquire ISP instance";
> > >                 return false;
> > > +       }
> > >
> > > -       isp_ = acquireMediaDevice(enumerator, isp);
> > > -       if (!isp_)
> > > +       int ret = registerCamera(unicamDevice, ispDevice);
> > > +       if (ret) {
> > > +               LOG(RPI, Error) << "Failed to register camera: " << ret;
> > >                 return false;
> > > +       }
> > >
> > > -       return registerCamera();
> > > +       return true;
> > >  }
> > >
> > > -bool PipelineHandlerRPi::registerCamera()
> > > +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp)
> > >  {
> > >         std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
> > > +
> > >         if (!data->dmaHeap_.isValid())
> > > -               return false;
> > > +               return -ENOMEM;
> > > +
> > > +       MediaEntity *unicamImage = unicam->getEntityByName("unicam-image");
> > > +       MediaEntity *ispOutput0 = isp->getEntityByName("bcm2835-isp0-output0");
> > > +       MediaEntity *ispCapture1 = isp->getEntityByName("bcm2835-isp0-capture1");
> > > +       MediaEntity *ispCapture2 = isp->getEntityByName("bcm2835-isp0-capture2");
> > > +       MediaEntity *ispCapture3 = isp->getEntityByName("bcm2835-isp0-capture3");
> > > +
> > > +       if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3)
> > > +               return -ENOENT;
> > >
> > >         /* Locate and open the unicam video streams. */
> > > -       data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image"));
> > > +       data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicamImage);
> > >
> > >         /* An embedded data node will not be present if the sensor does not support it. */
> > > -       MediaEntity *embeddedEntity = unicam_->getEntityByName("unicam-embedded");
> > > -       if (embeddedEntity) {
> > > -               data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", embeddedEntity);
> > > +       MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam-embedded");
> > > +       if (unicamEmbedded) {
> > > +               data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded);
> > >                 data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),
> > >                                                                            &RPiCameraData::unicamBufferDequeue);
> > >         }
> > >
> > >         /* Tag the ISP input stream as an import stream. */
> > > -       data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
> > > -       data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
> > > -       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"));
> > > +       data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, true);
> > > +       data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1);
> > > +       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()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);
> > > @@ -1049,7 +1060,7 @@ bool PipelineHandlerRPi::registerCamera()
> > >         data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);
> > >
> > >         /* Identify the sensor. */
> > > -       for (MediaEntity *entity : unicam_->entities()) {
> > > +       for (MediaEntity *entity : unicam->entities()) {
> > >                 if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {
> > >                         data->sensor_ = std::make_unique<CameraSensor>(entity);
> > >                         break;
> > > @@ -1057,23 +1068,23 @@ bool PipelineHandlerRPi::registerCamera()
> > >         }
> > >
> > >         if (!data->sensor_)
> > > -               return false;
> > > +               return -EINVAL;
> > >
> > >         if (data->sensor_->init())
> > > -               return false;
> > > +               return -EINVAL;
> > >
> > >         data->sensorFormats_ = populateSensorFormats(data->sensor_);
> > >
> > >         ipa::RPi::SensorConfig sensorConfig;
> > >         if (data->loadIPA(&sensorConfig)) {
> > >                 LOG(RPI, Error) << "Failed to load a suitable IPA library";
> > > -               return false;
> > > +               return -EINVAL;
> > >         }
> > >
> > > -       if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {
> > > +       if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {
> > >                 LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!";
> > >                 sensorConfig.sensorMetadata = false;
> > > -               if (embeddedEntity)
> > > +               if (unicamEmbedded)
> > >                         data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();
> > >         }
> > >
> > > @@ -1093,13 +1104,14 @@ bool PipelineHandlerRPi::registerCamera()
> > >                 data->streams_.push_back(&stream);
> > >
> > >         for (auto stream : data->streams_) {
> > > -               if (stream->dev()->open())
> > > -                       return false;
> > > +               int ret = stream->dev()->open();
> > > +               if (ret)
> > > +                       return ret;
> > >         }
> > >
> > >         if (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {
> > >                 LOG(RPI, Error) << "Unicam driver does not use the MediaController, please update your kernel!";
> > > -               return false;
> > > +               return -EINVAL;
> > >         }
> > >
> > >         /*
> > > @@ -1161,7 +1173,7 @@ bool PipelineHandlerRPi::registerCamera()
> > >
> > >         if (!bayerFormat.isValid()) {
> > >                 LOG(RPI, Error) << "No Bayer format found";
> > > -               return false;
> > > +               return -EINVAL;
> > >         }
> > >         data->nativeBayerOrder_ = bayerFormat.order;
> > >
> > > @@ -1181,7 +1193,10 @@ bool PipelineHandlerRPi::registerCamera()
> > >                 Camera::create(std::move(data), id, streams);
> > >         PipelineHandler::registerCamera(std::move(camera));
> > >
> > > -       return true;
> > > +       LOG(RPI, Info) << "Registered camera " << id
> > > +                      << " to Unicam device " << unicam->deviceNode()
> > > +                      << " and ISP device " << isp->deviceNode();
> > > +       return 0;
> > >  }
> > >
> > >  int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> > > --
> > > 2.25.1
> > >

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 5a6dfa4e8b2a..e31fa3b23859 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -314,14 +314,11 @@  private:
 		return static_cast<RPiCameraData *>(camera->_d());
 	}
 
-	bool registerCamera();
+	int registerCamera(MediaDevice *unicam, MediaDevice *isp);
 	int queueAllBuffers(Camera *camera);
 	int prepareBuffers(Camera *camera);
 	void freeBuffers(Camera *camera);
 	void mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask);
-
-	MediaDevice *unicam_;
-	MediaDevice *isp_;
 };
 
 RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)
@@ -512,7 +509,7 @@  CameraConfiguration::Status RPiCameraConfiguration::validate()
 }
 
 PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)
-	: PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)
+	: PipelineHandler(manager)
 {
 }
 
@@ -997,48 +994,62 @@  int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
 bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
 {
 	DeviceMatch unicam("unicam");
-	DeviceMatch isp("bcm2835-isp");
+	MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);
 
-	unicam.add("unicam-image");
+	if (!unicamDevice) {
+		LOG(RPI, Debug) << "Unable to acquire a Unicam instance";
+		return false;
+	}
 
-	isp.add("bcm2835-isp0-output0"); /* Input */
-	isp.add("bcm2835-isp0-capture1"); /* Output 0 */
-	isp.add("bcm2835-isp0-capture2"); /* Output 1 */
-	isp.add("bcm2835-isp0-capture3"); /* Stats */
+	DeviceMatch isp("bcm2835-isp");
+	MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);
 
-	unicam_ = acquireMediaDevice(enumerator, unicam);
-	if (!unicam_)
+	if (!ispDevice) {
+		LOG(RPI, Debug) << "Unable to acquire ISP instance";
 		return false;
+	}
 
-	isp_ = acquireMediaDevice(enumerator, isp);
-	if (!isp_)
+	int ret = registerCamera(unicamDevice, ispDevice);
+	if (ret) {
+		LOG(RPI, Error) << "Failed to register camera: " << ret;
 		return false;
+	}
 
-	return registerCamera();
+	return true;
 }
 
-bool PipelineHandlerRPi::registerCamera()
+int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp)
 {
 	std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
+
 	if (!data->dmaHeap_.isValid())
-		return false;
+		return -ENOMEM;
+
+	MediaEntity *unicamImage = unicam->getEntityByName("unicam-image");
+	MediaEntity *ispOutput0 = isp->getEntityByName("bcm2835-isp0-output0");
+	MediaEntity *ispCapture1 = isp->getEntityByName("bcm2835-isp0-capture1");
+	MediaEntity *ispCapture2 = isp->getEntityByName("bcm2835-isp0-capture2");
+	MediaEntity *ispCapture3 = isp->getEntityByName("bcm2835-isp0-capture3");
+
+	if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3)
+		return -ENOENT;
 
 	/* Locate and open the unicam video streams. */
-	data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image"));
+	data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicamImage);
 
 	/* An embedded data node will not be present if the sensor does not support it. */
-	MediaEntity *embeddedEntity = unicam_->getEntityByName("unicam-embedded");
-	if (embeddedEntity) {
-		data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", embeddedEntity);
+	MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam-embedded");
+	if (unicamEmbedded) {
+		data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded);
 		data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),
 									   &RPiCameraData::unicamBufferDequeue);
 	}
 
 	/* Tag the ISP input stream as an import stream. */
-	data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
-	data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
-	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"));
+	data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, true);
+	data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1);
+	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()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);
@@ -1049,7 +1060,7 @@  bool PipelineHandlerRPi::registerCamera()
 	data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);
 
 	/* Identify the sensor. */
-	for (MediaEntity *entity : unicam_->entities()) {
+	for (MediaEntity *entity : unicam->entities()) {
 		if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {
 			data->sensor_ = std::make_unique<CameraSensor>(entity);
 			break;
@@ -1057,23 +1068,23 @@  bool PipelineHandlerRPi::registerCamera()
 	}
 
 	if (!data->sensor_)
-		return false;
+		return -EINVAL;
 
 	if (data->sensor_->init())
-		return false;
+		return -EINVAL;
 
 	data->sensorFormats_ = populateSensorFormats(data->sensor_);
 
 	ipa::RPi::SensorConfig sensorConfig;
 	if (data->loadIPA(&sensorConfig)) {
 		LOG(RPI, Error) << "Failed to load a suitable IPA library";
-		return false;
+		return -EINVAL;
 	}
 
-	if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {
+	if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {
 		LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!";
 		sensorConfig.sensorMetadata = false;
-		if (embeddedEntity)
+		if (unicamEmbedded)
 			data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();
 	}
 
@@ -1093,13 +1104,14 @@  bool PipelineHandlerRPi::registerCamera()
 		data->streams_.push_back(&stream);
 
 	for (auto stream : data->streams_) {
-		if (stream->dev()->open())
-			return false;
+		int ret = stream->dev()->open();
+		if (ret)
+			return ret;
 	}
 
 	if (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {
 		LOG(RPI, Error) << "Unicam driver does not use the MediaController, please update your kernel!";
-		return false;
+		return -EINVAL;
 	}
 
 	/*
@@ -1161,7 +1173,7 @@  bool PipelineHandlerRPi::registerCamera()
 
 	if (!bayerFormat.isValid()) {
 		LOG(RPI, Error) << "No Bayer format found";
-		return false;
+		return -EINVAL;
 	}
 	data->nativeBayerOrder_ = bayerFormat.order;
 
@@ -1181,7 +1193,10 @@  bool PipelineHandlerRPi::registerCamera()
 		Camera::create(std::move(data), id, streams);
 	PipelineHandler::registerCamera(std::move(camera));
 
-	return true;
+	LOG(RPI, Info) << "Registered camera " << id
+		       << " to Unicam device " << unicam->deviceNode()
+		       << " and ISP device " << isp->deviceNode();
+	return 0;
 }
 
 int PipelineHandlerRPi::queueAllBuffers(Camera *camera)