[libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow registration of multiple cameras
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Nov 19 14:03:11 CET 2021
Hi Naush,
Thank you for the patch.
On Fri, Nov 19, 2021 at 11:28:39AM +0000, Naushir Patuck wrote:
> On Fri, 19 Nov 2021 at 11:27, Naushir Patuck wrote:
> > On Fri, 19 Nov 2021 at 11:10, Naushir Patuck wrote:
> >
> >> Expand the pipeline handler camera registration to correctly handle multiple
> >> cameras attached to the platform. For example, Raspberry Pi Compute Module
> >> platforms have two camera connectors, and this change would allow the user to
> >> select either of the two cameras to run.
That's interesting :-)
> >> There are associated kernel driver changes for both Unicam and the ISP needed
> >> to correctly advertise multiple media devices and nodes for multi-camera usage:
> >>
> >> https://github.com/raspberrypi/linux/pull/4140
> >> https://github.com/raspberrypi/linux/pull/4709
There's still a single ISP instance. How does it work at the
hardware/firmware level ? The kernel patch seems simple, how are ISP
operations queued by users scheduled and arbitrated ? Can the two
cameras be used concurrently, or are they mutually exclusive ?
> >> However, this change is backward compatible with kernel builds that do not have
> >> these changes for standard single camera usage.
> >>
> >> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> >> ---
> >> .../pipeline/raspberrypi/raspberrypi.cpp | 112 ++++++++++++------
> >> 1 file changed, 77 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> index 9aa7e9eef5e7..8ed2ebcaafe7 100644
> >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> @@ -311,7 +311,8 @@ private:
> >> return static_cast<RPiCameraData *>(camera->_d());
> >> }
> >>
> >> - bool registerCameras();
> >> + int registerCameras(MediaDevice *unicam, MediaDevice *isp,
> >> + const std::string &unicamIdStr, const std::string &ispIdStr);
> >> int queueAllBuffers(Camera *camera);
> >> int prepareBuffers(Camera *camera);
> >> void freeBuffers(Camera *camera);
> >> @@ -993,49 +994,87 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> >>
> >> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> >> {
> >> - DeviceMatch unicam("unicam");
> >> - DeviceMatch isp("bcm2835-isp");
> >> + unsigned int numCameras = 0;
> >>
> >> - unicam.add("unicam-image");
> >> + /*
> >> + * String of indexes to append to the entity names when searching for
> >> + * the Unican media devices. The first string is empty (un-indexed) to
> >> + * to maintain backward compatability with old versions of the Unicam
> >> + * kernel driver that did not advertise instance indexes.
> >> + */
> >> + for (const std::string &unicamIdStr : { "", "0", "1" }) {
> >> + MediaDevice *unicamDevice;
> >>
> >> - 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 unicam("unicam");
> >> + unicam.add("unicam" + unicamIdStr + "-image");
> >> + unicamDevice = acquireMediaDevice(enumerator, unicam);
> >>
> >> - unicam_ = acquireMediaDevice(enumerator, unicam);
> >> - if (!unicam_)
> >> - return false;
> >> + if (!unicamDevice)
> >> + continue;
> >>
> >> - isp_ = acquireMediaDevice(enumerator, isp);
> >> - if (!isp_)
> >> - return false;
> >> + for (const std::string &ispIdStr : { "0", "1" }) {
I'm not sure to understand why you have nested loops, shouldn't you
acquire the unicam and ISP devices separately ?
> >> + MediaDevice *ispDevice;
> >> + int ret;
> >> +
> >> + DeviceMatch isp("bcm2835-isp");
> >> + isp.add("bcm2835-isp" + ispIdStr + "-output0"); /* Input */
> >> + isp.add("bcm2835-isp" + ispIdStr + "-capture1"); /* Output 0 */
> >> + isp.add("bcm2835-isp" + ispIdStr + "-capture2"); /* Output 0 */
> >> + isp.add("bcm2835-isp" + ispIdStr + "-capture3"); /* Stats */
> >> + ispDevice = acquireMediaDevice(enumerator, isp);
> >> +
> >> + if (!ispDevice)
> >> + continue;
> >> +
> >> + ret = registerCameras(unicamDevice, ispDevice, unicamIdStr, ispIdStr);
> >> + if (ret) {
> >> + LOG(RPI, Error) << "Failed to register camera: " << ret;
> >> + ispDevice->release();
> >> + unicamDevice->release();
> >
> > I am not too sure if the above two lines are
> > enough. PipelineHandler::acquireMediaDevice()
> > does a MediaDevice::acquire() but also adds the media device internally to
> > a private member:
> >
> > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_;
> >
> > Perhaps I ought to implement a new public
> > function PipelineHandler::acquireMediaDevice(MediaDevice *)
>
> Sorry, that should read PipelineHandler::releaseMediaDevice(MediaDevice *)
> :-(
>
> > that removes the entry in mediaDevices_ as well as does a
> > MediaDevice::release(). Thoughts?
The media device acquired by the pipeline handler are meant to be
released automatically. Camera registration failure isn't supposed to
happen, in case one camera fails to register and the other doesn't, is
there any issue keeping the media devices for the failed camera acquired
?
> >> + } else
> >> + numCameras++;
> >>
> >> - return registerCameras();
> >> + break;
> >> + }
> >> + }
> >> +
> >> + return !!numCameras;
> >> }
> >>
> >> -bool PipelineHandlerRPi::registerCameras()
> >> +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam, MediaDevice *isp,
> >> + const std::string &unicamIdStr,
> >> + const std::string &ispIdStr)
> >> {
> >> std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
> >> +
> >> if (!data->dmaHeap_.isValid())
> >> - return false;
> >> + return -ENOMEM;
> >> +
> >> + MediaEntity *unicamImage = unicam->getEntityByName("unicam" + unicamIdStr + "-image");
> >> + MediaEntity *ispOutput0 = isp->getEntityByName("bcm2835-isp" + ispIdStr + "-output0");
> >> + MediaEntity *ispCapture1 = isp->getEntityByName("bcm2835-isp" + ispIdStr + "-capture1");
> >> + MediaEntity *ispCapture2 = isp->getEntityByName("bcm2835-isp" + ispIdStr + "-capture2");
> >> + MediaEntity *ispCapture3 = isp->getEntityByName("bcm2835-isp" + ispIdStr + "-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" + unicamIdStr + "-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);
> >> @@ -1046,7 +1085,7 @@ bool PipelineHandlerRPi::registerCameras()
> >> 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;
> >> @@ -1054,23 +1093,23 @@ bool PipelineHandlerRPi::registerCameras()
> >> }
> >>
> >> 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();
> >> }
> >>
> >> @@ -1091,12 +1130,12 @@ bool PipelineHandlerRPi::registerCameras()
> >>
> >> for (auto stream : data->streams_) {
> >> if (stream->dev()->open())
> >> - return false;
> >> + continue;
> >> }
> >>
> >> 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;
> >> }
> >>
> >> /*
> >> @@ -1158,7 +1197,7 @@ bool PipelineHandlerRPi::registerCameras()
> >>
> >> if (!bayerFormat.isValid()) {
> >> LOG(RPI, Error) << "No Bayer format found";
> >> - return false;
> >> + return -EINVAL;
> >> }
> >> data->nativeBayerOrder_ = bayerFormat.order;
> >>
> >> @@ -1178,7 +1217,10 @@ bool PipelineHandlerRPi::registerCameras()
> >> Camera::create(std::move(data), id, streams);
> >> registerCamera(std::move(camera));
> >>
> >> - return true;
> >> + LOG(RPI, Info) << "Registered camera " << id
> >> + << " to instances \"unicam" << unicamIdStr << "\""
> >> + << " and \"isp" << ispIdStr << "\"";
> >> + return 0;
> >> }
> >>
> >> int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list