[libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow registration of multiple cameras
Naushir Patuck
naush at raspberrypi.com
Sat Nov 20 08:28:19 CET 2021
Hi Laurent,
On Fri, 19 Nov 2021 at 20:53, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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.
>
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?
>
> > 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.
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/20211120/af499ca5/attachment-0001.htm>
More information about the libcamera-devel
mailing list