<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 19 Nov 2021 at 20:53, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On Fri, Nov 19, 2021 at 04:45:27PM +0000, Naushir Patuck wrote:<br>
> On Fri, 19 Nov 2021 at 13:03, Laurent Pinchart wrote:<br>
> > On Fri, Nov 19, 2021 at 11:28:39AM +0000, Naushir Patuck wrote:<br>
> > > On Fri, 19 Nov 2021 at 11:27, Naushir Patuck wrote:<br>
> > > > On Fri, 19 Nov 2021 at 11:10, Naushir Patuck wrote:<br>
> > > ><br>
> > > >> Expand the pipeline handler camera registration to correctly handle multiple<br>
> > > >> cameras attached to the platform. For example, Raspberry Pi Compute Module<br>
> > > >> platforms have two camera connectors, and this change would allow the user to<br>
> > > >> select either of the two cameras to run.<br>
> ><br>
> > That's interesting :-)<br>
> ><br>
> > > >> There are associated kernel driver changes for both Unicam and the ISP needed<br>
> > > >> to correctly advertise multiple media devices and nodes for multi-camera usage:<br>
> > > >><br>
> > > >> <a href="https://github.com/raspberrypi/linux/pull/4140" rel="noreferrer" target="_blank">https://github.com/raspberrypi/linux/pull/4140</a><br>
> > > >> <a href="https://github.com/raspberrypi/linux/pull/4709" rel="noreferrer" target="_blank">https://github.com/raspberrypi/linux/pull/4709</a><br>
> ><br>
> > There's still a single ISP instance. How does it work at the<br>
> > hardware/firmware level ? The kernel patch seems simple, how are ISP<br>
> > operations queued by users scheduled and arbitrated ? Can the two<br>
> > cameras be used concurrently, or are they mutually exclusive ?<br>
> <br>
> The isp driver change will allow concurrent usage of the ISP hardware.<br>
> What's missing from your view is the shim in the firmware that handles<br>
> the arbitration between the many users ;-) This makes adding multi-user<br>
> to the kernel driver mostly a duplication of the single-user instance.<br>
<br>
Are there advantages in doing so in the firmware instead of in the Linux<br>
kernel driver, or even in userspace in libcamera ? I suppose that if<br>
it's there already it's the simplest way forward, I'm not requesting a<br>
change here, but I'm wondering what the design rationale is.<br></blockquote><div><br></div><div>The firmware already has support for concurrent usage as we need</div><div>it to share the ISP HW with the codec driver where ISP is used to generate</div><div>a format that our codec block understands.</div><div><br></div><div>Another advantage is that it would be easier to context switch between frames</div><div>on a stripe level rather than at frame level. This could be duplicated in the</div><div>kernel driver but would require much more effort.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
How should libcamera cope with limitations in terms of ISP bandwidth<br>
with multiple instances ?<br></blockquote><div><br></div><div>That's a good question, and I am not sure of the answer. Right now, our</div><div>pipeline handler will simply drop frames when it cannot keep up in the HW.</div><div>It would be easy enough to calculate expected throughput in the pipeline handler</div><div>instance, but how do we know if/when other users outside of libcamera</div><div>are also using the ISP? </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> As an aside, I have run into a few issues with actual concurrent usage<br>
> in the pipeline handlers. I've been talking with Jacopo offline about<br>
> this, but we can keep that discussion as a separate thread to this work.<br>
<br>
<a href="https://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=mtk/multi-cam" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/pinchartl/libcamera.git/log/?h=mtk/multi-cam</a><br>
may help already. We'll also need an API for pipeline handlers to report<br>
mutual exclusion constraints between camera, and a way to expose that to<br>
applications.<br>
<br>
> > > >> However, this change is backward compatible with kernel builds that do not have<br>
> > > >> these changes for standard single camera usage.<br>
> > > >><br>
> > > >> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > >> ---<br>
> > > >> .../pipeline/raspberrypi/raspberrypi.cpp | 112 ++++++++++++------<br>
> > > >> 1 file changed, 77 insertions(+), 35 deletions(-)<br>
> > > >><br>
> > > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > >> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > >> index 9aa7e9eef5e7..8ed2ebcaafe7 100644<br>
> > > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > >> @@ -311,7 +311,8 @@ private:<br>
> > > >> return static_cast<RPiCameraData *>(camera->_d());<br>
> > > >> }<br>
> > > >><br>
> > > >> - bool registerCameras();<br>
> > > >> + int registerCameras(MediaDevice *unicam, MediaDevice *isp,<br>
> > > >> + const std::string &unicamIdStr, const std::string &ispIdStr);<br>
> > > >> int queueAllBuffers(Camera *camera);<br>
> > > >> int prepareBuffers(Camera *camera);<br>
> > > >> void freeBuffers(Camera *camera);<br>
> > > >> @@ -993,49 +994,87 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)<br>
> > > >><br>
> > > >> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)<br>
> > > >> {<br>
> > > >> - DeviceMatch unicam("unicam");<br>
> > > >> - DeviceMatch isp("bcm2835-isp");<br>
> > > >> + unsigned int numCameras = 0;<br>
> > > >><br>
> > > >> - unicam.add("unicam-image");<br>
> > > >> + /*<br>
> > > >> + * String of indexes to append to the entity names when searching for<br>
> > > >> + * the Unican media devices. The first string is empty (un-indexed) to<br>
> > > >> + * to maintain backward compatability with old versions of the Unicam<br>
> > > >> + * kernel driver that did not advertise instance indexes.<br>
> > > >> + */<br>
> > > >> + for (const std::string &unicamIdStr : { "", "0", "1" }) {<br>
> > > >> + MediaDevice *unicamDevice;<br>
> > > >><br>
> > > >> - isp.add("bcm2835-isp0-output0"); /* Input */<br>
> > > >> - isp.add("bcm2835-isp0-capture1"); /* Output 0 */<br>
> > > >> - isp.add("bcm2835-isp0-capture2"); /* Output 1 */<br>
> > > >> - isp.add("bcm2835-isp0-capture3"); /* Stats */<br>
> > > >> + DeviceMatch unicam("unicam");<br>
> > > >> + unicam.add("unicam" + unicamIdStr + "-image");<br>
> > > >> + unicamDevice = acquireMediaDevice(enumerator, unicam);<br>
> > > >><br>
> > > >> - unicam_ = acquireMediaDevice(enumerator, unicam);<br>
> > > >> - if (!unicam_)<br>
> > > >> - return false;<br>
> > > >> + if (!unicamDevice)<br>
> > > >> + continue;<br>
> > > >><br>
> > > >> - isp_ = acquireMediaDevice(enumerator, isp);<br>
> > > >> - if (!isp_)<br>
> > > >> - return false;<br>
> > > >> + for (const std::string &ispIdStr : { "0", "1" }) {<br>
> ><br>
> > I'm not sure to understand why you have nested loops, shouldn't you<br>
> > acquire the unicam and ISP devices separately ?<br>
> <br>
> Each unicam instance must be matched with an isp instance, else<br>
> the camera cannot be registered.<br>
> <br>
> I used nested loops for convenience, it's not strictly necessary.<br>
> With the nested loops, I don't need to keep arrays of MediaDevice<br>
> pointers sticking around :-)<br>
<br>
If the two ISP instances are identical, it sounds like you could support<br>
this in a much simpler way, by acquiring one unicam instance and one ISP<br>
instance only in the pipeline handler. match() will be called in a loop<br>
until it returns false, so you'll have two independent pipeline handler<br>
instances, handling one camera each. Handling multiple cameras in a<br>
single pipeline handler instance is only required when cameras share<br>
resources.<br></blockquote><div><br></div><div>Ah, this is what I was missing! I did not realise that match() would be called</div><div>in a loop until it fails. So I can adjust this code to only register one camera</div><div>per call and things should be simpler! I'll make that change for the next</div><div>version of this series.</div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> > > >> + MediaDevice *ispDevice;<br>
> > > >> + int ret;<br>
> > > >> +<br>
> > > >> + DeviceMatch isp("bcm2835-isp");<br>
> > > >> + isp.add("bcm2835-isp" + ispIdStr + "-output0"); /* Input */<br>
> > > >> + isp.add("bcm2835-isp" + ispIdStr + "-capture1"); /* Output 0 */<br>
> > > >> + isp.add("bcm2835-isp" + ispIdStr + "-capture2"); /* Output 0 */<br>
> > > >> + isp.add("bcm2835-isp" + ispIdStr + "-capture3"); /* Stats */<br>
> > > >> + ispDevice = acquireMediaDevice(enumerator, isp);<br>
> > > >> +<br>
> > > >> + if (!ispDevice)<br>
> > > >> + continue;<br>
> > > >> +<br>
> > > >> + ret = registerCameras(unicamDevice, ispDevice, unicamIdStr, ispIdStr);<br>
> > > >> + if (ret) {<br>
> > > >> + LOG(RPI, Error) << "Failed to register camera: " << ret;<br>
> > > >> + ispDevice->release();<br>
> > > >> + unicamDevice->release();<br>
> > > ><br>
> > > > I am not too sure if the above two lines are<br>
> > > > enough. PipelineHandler::acquireMediaDevice()<br>
> > > > does a MediaDevice::acquire() but also adds the media device internally to<br>
> > > > a private member:<br>
> > > ><br>
> > > > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_;<br>
> > > ><br>
> > > > Perhaps I ought to implement a new public<br>
> > > > function PipelineHandler::acquireMediaDevice(MediaDevice *)<br>
> > ><br>
> > > Sorry, that should read PipelineHandler::releaseMediaDevice(MediaDevice *)<br>
> > > :-(<br>
> > ><br>
> > > > that removes the entry in mediaDevices_ as well as does a<br>
> > > > MediaDevice::release(). Thoughts?<br>
> ><br>
> > The media device acquired by the pipeline handler are meant to be<br>
> > released automatically. Camera registration failure isn't supposed to<br>
> > happen, in case one camera fails to register and the other doesn't, is<br>
> > there any issue keeping the media devices for the failed camera acquired<br>
> > ?<br>
> <br>
> I suppose there is no harm in keeping the device acquired when the camera cannot<br>
> be registered, particularly as it cannot be used again. In which case I can simplify the<br>
> error path logic.<br>
> <br>
> > > >> + } else<br>
> > > >> + numCameras++;<br>
> > > >><br>
> > > >> - return registerCameras();<br>
> > > >> + break;<br>
> > > >> + }<br>
> > > >> + }<br>
> > > >> +<br>
> > > >> + return !!numCameras;<br>
> > > >> }<br>
> > > >><br>
> > > >> -bool PipelineHandlerRPi::registerCameras()<br>
> > > >> +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam, MediaDevice *isp,<br>
> > > >> + const std::string &unicamIdStr,<br>
> > > >> + const std::string &ispIdStr)<br>
> > > >> {<br>
> > > >> std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);<br>
> > > >> +<br>
> > > >> if (!data->dmaHeap_.isValid())<br>
> > > >> - return false;<br>
> > > >> + return -ENOMEM;<br>
> > > >> +<br>
> > > >> + MediaEntity *unicamImage = unicam->getEntityByName("unicam" + unicamIdStr + "-image");<br>
> > > >> + MediaEntity *ispOutput0 = isp->getEntityByName("bcm2835-isp" + ispIdStr + "-output0");<br>
> > > >> + MediaEntity *ispCapture1 = isp->getEntityByName("bcm2835-isp" + ispIdStr + "-capture1");<br>
> > > >> + MediaEntity *ispCapture2 = isp->getEntityByName("bcm2835-isp" + ispIdStr + "-capture2");<br>
> > > >> + MediaEntity *ispCapture3 = isp->getEntityByName("bcm2835-isp" + ispIdStr + "-capture3");<br>
> > > >> +<br>
> > > >> + if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3)<br>
> > > >> + return -ENOENT;<br>
> > > >><br>
> > > >> /* Locate and open the unicam video streams. */<br>
> > > >> - data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image"));<br>
> > > >> + data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicamImage);<br>
> > > >><br>
> > > >> /* An embedded data node will not be present if the sensor does not support it. */<br>
> > > >> - MediaEntity *embeddedEntity = unicam_->getEntityByName("unicam-embedded");<br>
> > > >> - if (embeddedEntity) {<br>
> > > >> - data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", embeddedEntity);<br>
> > > >> + MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam" + unicamIdStr + "-embedded");<br>
> > > >> + if (unicamEmbedded) {<br>
> > > >> + data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded);<br>
> > > >><br>
> > > >> data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),<br>
> > > >> &RPiCameraData::unicamBufferDequeue);<br>
> > > >> }<br>
> > > >><br>
> > > >> /* Tag the ISP input stream as an import stream. */<br>
> > > >> - data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);<br>
> > > >> - data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));<br>
> > > >> - data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"));<br>
> > > >> - data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"));<br>
> > > >> + data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, true);<br>
> > > >> + data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1);<br>
> > > >> + data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2);<br>
> > > >> + data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3);<br>
> > > >><br>
> > > >> /* Wire up all the buffer connections. */<br>
> > > >> data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);<br>
> > > >> @@ -1046,7 +1085,7 @@ bool PipelineHandlerRPi::registerCameras()<br>
> > > >> data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);<br>
> > > >><br>
> > > >> /* Identify the sensor. */<br>
> > > >> - for (MediaEntity *entity : unicam_->entities()) {<br>
> > > >> + for (MediaEntity *entity : unicam->entities()) {<br>
> > > >> if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {<br>
> > > >> data->sensor_ = std::make_unique<CameraSensor>(entity);<br>
> > > >> break;<br>
> > > >> @@ -1054,23 +1093,23 @@ bool PipelineHandlerRPi::registerCameras()<br>
> > > >> }<br>
> > > >><br>
> > > >> if (!data->sensor_)<br>
> > > >> - return false;<br>
> > > >> + return -EINVAL;<br>
> > > >><br>
> > > >> if (data->sensor_->init())<br>
> > > >> - return false;<br>
> > > >> + return -EINVAL;<br>
> > > >><br>
> > > >> data->sensorFormats_ = populateSensorFormats(data->sensor_);<br>
> > > >><br>
> > > >> ipa::RPi::SensorConfig sensorConfig;<br>
> > > >> if (data->loadIPA(&sensorConfig)) {<br>
> > > >> LOG(RPI, Error) << "Failed to load a suitable IPA library";<br>
> > > >> - return false;<br>
> > > >> + return -EINVAL;<br>
> > > >> }<br>
> > > >><br>
> > > >> - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {<br>
> > > >> + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {<br>
> > > >> LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!";<br>
> > > >> sensorConfig.sensorMetadata = false;<br>
> > > >> - if (embeddedEntity)<br>
> > > >> + if (unicamEmbedded)<br>
> > > >> data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();<br>
> > > >> }<br>
> > > >><br>
> > > >> @@ -1091,12 +1130,12 @@ bool PipelineHandlerRPi::registerCameras()<br>
> > > >><br>
> > > >> for (auto stream : data->streams_) {<br>
> > > >> if (stream->dev()->open())<br>
> > > >> - return false;<br>
> > > >> + continue;<br>
> > > >> }<br>
> > > >><br>
> > > >> if (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {<br>
> > > >> LOG(RPI, Error) << "Unicam driver does not use the MediaController, please update your kernel!";<br>
> > > >> - return false;<br>
> > > >> + return -EINVAL;<br>
> > > >> }<br>
> > > >><br>
> > > >> /*<br>
> > > >> @@ -1158,7 +1197,7 @@ bool PipelineHandlerRPi::registerCameras()<br>
> > > >><br>
> > > >> if (!bayerFormat.isValid()) {<br>
> > > >> LOG(RPI, Error) << "No Bayer format found";<br>
> > > >> - return false;<br>
> > > >> + return -EINVAL;<br>
> > > >> }<br>
> > > >> data->nativeBayerOrder_ = bayerFormat.order;<br>
> > > >><br>
> > > >> @@ -1178,7 +1217,10 @@ bool PipelineHandlerRPi::registerCameras()<br>
> > > >> Camera::create(std::move(data), id, streams);<br>
> > > >> registerCamera(std::move(camera));<br>
> > > >><br>
> > > >> - return true;<br>
> > > >> + LOG(RPI, Info) << "Registered camera " << id<br>
> > > >> + << " to instances \"unicam" << unicamIdStr << "\""<br>
> > > >> + << " and \"isp" << ispIdStr << "\"";<br>
> > > >> + return 0;<br>
> > > >> }<br>
> > > >><br>
> > > >> int PipelineHandlerRPi::queueAllBuffers(Camera *camera)<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>