<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 19 Nov 2021 at 11:27, Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@raspberrypi.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"><div dir="ltr"><div dir="ltr">Hi,<div><br></div></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 19 Nov 2021 at 11:10, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.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">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>
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>
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 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>
+ 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></blockquote><div><br></div><div>I am not too sure if the above two lines are enough. PipelineHandler::acquireMediaDevice()</div><div>does a MediaDevice::acquire() but also adds the media device internally to a private member:</div><div><br></div><div>std::vector<std::shared_ptr<MediaDevice>> mediaDevices_;<br></div><div><br></div><div>Perhaps I ought to implement a new public function PipelineHandler::acquireMediaDevice(MediaDevice *)</div></div></div></blockquote><div><br></div><div>Sorry, that should read PipelineHandler::releaseMediaDevice(MediaDevice *) :-(</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"><div dir="ltr"><div class="gmail_quote"><div>that removes the entry in mediaDevices_ as well as does a MediaDevice::release(). Thoughts?</div><div><br></div><div>Naush</div><div><br></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">
+ } 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>
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>
2.25.1<br>
<br>
</blockquote></div></div>
</blockquote></div></div>