[libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Allow registration of multiple cameras
Naushir Patuck
naush at raspberrypi.com
Fri Nov 19 17:45:27 CET 2021
Hi Laurent,
Thank you for your review for this series.
On Fri, 19 Nov 2021 at 13:03, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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 ?
>
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.
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.
>
> > >> 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 :-)
>
> > >> + 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.
Regards,
Naush
>
> > >> + } 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/20211119/7d4b4932/attachment-0001.htm>
More information about the libcamera-devel
mailing list