<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your review for this series.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 19 Nov 2021 at 13:03, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">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>
Thank you for the patch.<br>
<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></blockquote><div><br></div><div>The isp driver change will allow concurrent usage of the ISP hardware.</div><div>What's missing from your view is the shim in the firmware that handles</div><div>the arbitration between the many users ;-) This makes adding multi-user</div><div>to the kernel driver mostly a duplication of the single-user instance.</div><div><br></div><div>As an aside, I have run into a few issues with actual concurrent usage</div><div>in the pipeline handlers. I've been talking with Jacopo offline about</div><div>this, but we can keep that discussion as a separate thread to this work.</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>
> >> 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></blockquote><div><br></div><div><div>Each unicam instance must be matched with an isp instance, else</div><div>the camera cannot be registered.</div></div><div><br></div><div>I used nested loops for convenience, it's not strictly necessary.</div><div>With the nested loops, I don't need to keep arrays of MediaDevice</div><div>pointers sticking around :-)</div><div> <br></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></blockquote><div><br></div><div>I suppose there is no harm in keeping the device acquired when the camera cannot</div><div>be registered, particularly as it cannot be used again. In which case I can simplify the</div><div>error path logic.</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>
> >> + } 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>