[libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow registration of multiple cameras
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Nov 22 00:25:21 CET 2021
Hi Naush,
On Sat, Nov 20, 2021 at 07:28:19AM +0000, Naushir Patuck wrote:
> On Fri, 19 Nov 2021 at 20:53, Laurent Pinchart wrote:
> > 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.
>
> The firmware already has support for concurrent usage as we need
> it to share the ISP HW with the codec driver where ISP is used to generate
> a format that our codec block understands.
>
> Another advantage is that it would be easier to context switch between frames
> on a stripe level rather than at frame level. This could be duplicated in the
> kernel driver but would require much more effort.
>
> > How should libcamera cope with limitations in terms of ISP bandwidth
> > with multiple instances ?
>
> That's a good question, and I am not sure of the answer. Right now, our
> pipeline handler will simply drop frames when it cannot keep up in the HW.
> It would be easy enough to calculate expected throughput in the pipeline handler
> instance, but how do we know if/when other users outside of libcamera
> are also using the ISP?
If multiplexing is handled in the firmware, we'd need an API to
negotiate bandwidth allocation with the firmware I suppose.
> > > 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.
>
> Ah, this is what I was missing! I did not realise that match() would be called
> in a loop until it fails. So I can adjust this code to only register one camera
> per call and things should be simpler! I'll make that change for the next
> version of this series.
You may need to extend DeviceMatch to allow regexps, depending on how
entities are named in the kernel.
> > > > > >> + 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