[libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow registration of multiple cameras
Naushir Patuck
naush at raspberrypi.com
Mon Nov 22 09:42:45 CET 2021
Hi Laurent,
On Sun, 21 Nov 2021 at 23:25, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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.
>
What do you think would be an acceptable failure route if the bandwidth
allocation exceeds the HW limitations? Should the use-case fail straight
away,
or do we throw a warning and drop frames gracefully if/when it happens.
We currently do the latter.
>
> > > > 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.
>
I think this is not needed for our implementation, but will put a new
version
together with my intended changes and reassess.
Regards,
Naush
>
> > > > > > >> + 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211122/b55d8355/attachment-0001.htm>
More information about the libcamera-devel
mailing list