[libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow registration of multiple cameras
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Nov 19 21:52:55 CET 2021
Hi Naush,
On Fri, Nov 19, 2021 at 04:45:27PM +0000, Naushir Patuck wrote:
> On Fri, 19 Nov 2021 at 13:03, Laurent Pinchart wrote:
> > 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 ?
>
> The isp driver change will allow concurrent usage of the ISP hardware.
> What's missing from your view is the shim in the firmware that handles
> the arbitration between the many users ;-) This makes adding multi-user
> to the kernel driver mostly a duplication of the single-user instance.
Are there advantages in doing so in the firmware instead of in the Linux
kernel driver, or even in userspace in libcamera ? I suppose that if
it's there already it's the simplest way forward, I'm not requesting a
change here, but I'm wondering what the design rationale is.
How should libcamera cope with limitations in terms of ISP bandwidth
with multiple instances ?
> As an aside, I have run into a few issues with actual concurrent usage
> in the pipeline handlers. I've been talking with Jacopo offline about
> this, but we can keep that discussion as a separate thread to this work.
https://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=mtk/multi-cam
may help already. We'll also need an API for pipeline handlers to report
mutual exclusion constraints between camera, and a way to expose that to
applications.
> > > >> 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 ?
>
> Each unicam instance must be matched with an isp instance, else
> the camera cannot be registered.
>
> I used nested loops for convenience, it's not strictly necessary.
> With the nested loops, I don't need to keep arrays of MediaDevice
> pointers sticking around :-)
If the two ISP instances are identical, it sounds like you could support
this in a much simpler way, by acquiring one unicam instance and one ISP
instance only in the pipeline handler. match() will be called in a loop
until it returns false, so you'll have two independent pipeline handler
instances, handling one camera each. Handling multiple cameras in a
single pipeline handler instance is only required when cameras share
resources.
> > > >> + 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
> > ?
>
> I suppose there is no harm in keeping the device acquired when the camera cannot
> be registered, particularly as it cannot be used again. In which case I can simplify the
> error path logic.
>
> > > >> + } 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