<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for the feedback. I apologize for the possibly very obvious questions</div><div>that follow below - but this enumeration business is a bit unclear to me :-)</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 22 Nov 2021 at 13:01, 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>
(CC'ing Sakari)<br>
<br>
Thank you for the patch.<br>
<br>
On Mon, Nov 22, 2021 at 12:34:27PM +0000, Naushir Patuck wrote:<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>
> 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 | 113 ++++++++++++------<br>
> 1 file changed, 74 insertions(+), 39 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 9aa7e9eef5e7..3f9e15514ed9 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -311,14 +311,11 @@ private:<br>
> return static_cast<RPiCameraData *>(camera->_d());<br>
> }<br>
> <br>
> - bool registerCameras();<br>
> + int registerCameras(MediaDevice *unicam, MediaDevice *isp, const std::string &deviceId);<br>
<br>
Given that a pipeline handler instance doesn't register multiple cameras<br>
anymore in v2, should this be called registerCamera() ?<br></blockquote><div><br></div><div>Yes!</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>
> int queueAllBuffers(Camera *camera);<br>
> int prepareBuffers(Camera *camera);<br>
> void freeBuffers(Camera *camera);<br>
> void mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask);<br>
> -<br>
> - MediaDevice *unicam_;<br>
> - MediaDevice *isp_;<br>
> };<br>
> <br>
> RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)<br>
> @@ -509,7 +506,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()<br>
> }<br>
> <br>
> PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)<br>
> - : PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)<br>
> + : PipelineHandler(manager)<br>
> {<br>
> }<br>
> <br>
> @@ -993,49 +990,85 @@ 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>
> + MediaDevice *unicamDevice, *ispDevice;<br>
> + std::string deviceId;<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 &id : { "", "0", "1" }) {<br>
> + DeviceMatch unicam("unicam");<br>
> + unicam.add("unicam" + id + "-image");<br>
> + unicamDevice = acquireMediaDevice(enumerator, unicam);<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>
> + if (unicamDevice) {<br>
> + deviceId = id == "1" ? "1" : "0";<br>
> + break;<br>
> + }<br>
> + }<br>
<br>
Wouldn't it be better to extend DeviceMatch to accept regexps ?<br>
<a href="https://en.cppreference.com/w/cpp/header/regex" rel="noreferrer" target="_blank">https://en.cppreference.com/w/cpp/header/regex</a> should make that easy.<br></blockquote><div><br></div><div>I'm not sure - see below.</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>
> <br>
> - unicam_ = acquireMediaDevice(enumerator, unicam);<br>
> - if (!unicam_)<br>
> + if (!unicamDevice) {<br>
> + LOG(RPI, Debug) << "Unable to acquire a Unicam instance";<br>
> return false;<br>
> + }<br>
> <br>
> - isp_ = acquireMediaDevice(enumerator, isp);<br>
> - if (!isp_)<br>
> + DeviceMatch isp("bcm2835-isp");<br>
> + isp.add("bcm2835-isp" + deviceId + "-output0"); /* Input */<br>
> + isp.add("bcm2835-isp" + deviceId + "-capture1"); /* Output 0 */<br>
> + isp.add("bcm2835-isp" + deviceId + "-capture2"); /* Output 1 */<br>
> + isp.add("bcm2835-isp" + deviceId + "-capture3"); /* Stats */<br>
<br>
Unless you need to match the unicam and ISP instances, you could here<br>
drop the isp.add() calls as matching on the device name should be<br>
enough. Unless there are bcm2835-isp instances that don't have the<br>
needed nodes ?<br></blockquote><div><br></div><div>I've set things up so that the 2 instances of unicam have the same device</div><div>name ("unicam"). To distinguish the different instances, the pad entity names</div><div>have an index in the string. Ditto for the ISP driver.</div><div><br></div><div>If I were to remove the isp.add() calls, then DeviceEnumerator would only ever find</div><div>the first match to "bcm2825-isp" device and I would never be able to access the</div><div>second instance, is that correct? This loops back to the previous question, and</div><div>why I am not sure if regexps in DeviceMatch would make any difference.</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>
> + ispDevice = acquireMediaDevice(enumerator, isp);<br>
> +<br>
> + if (!ispDevice) {<br>
> + LOG(RPI, Error) << "Unable to acquire ISP instance " << deviceId;<br>
> return false;<br>
> + }<br>
> +<br>
> + int ret = registerCameras(unicamDevice, ispDevice, deviceId);<br>
> + if (ret) {<br>
> + LOG(RPI, Error) << "Failed to register camera: " << ret;<br>
> + return false;<br>
> + }<br>
> <br>
> - return registerCameras();<br>
> + return true;<br>
> }<br>
> <br>
> -bool PipelineHandlerRPi::registerCameras()<br>
> +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam, MediaDevice *isp,<br>
> + const std::string &deviceId)<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" + deviceId + "-image");<br>
> + MediaEntity *ispOutput0 = isp->getEntityByName("bcm2835-isp" + deviceId + "-output0");<br>
> + MediaEntity *ispCapture1 = isp->getEntityByName("bcm2835-isp" + deviceId + "-capture1");<br>
> + MediaEntity *ispCapture2 = isp->getEntityByName("bcm2835-isp" + deviceId + "-capture2");<br>
> + MediaEntity *ispCapture3 = isp->getEntityByName("bcm2835-isp" + deviceId + "-capture3");<br>
<br>
I'm wondering if wildcards would be nice here too, but maybe a simpler<br>
option would be to not include the device ID in the entity names in the<br>
driver ? We have a bus info field that the kernel reports for the media<br>
device, to differentiate between multiple instances of the same device.<br>
I suppose we are missing guidelines on the kernel side regarding entity<br>
naming. Sakari, any opinion on this ?<br></blockquote><div><br></div><div>Same problem, if all instances have the same driver name and entity names, I can</div><div>never acquire the second instance, as the first instance will always return a match.</div><div><br></div><div>Again, I am sure my interpretation of the API is not fully correct, so these issues may</div><div>not actually exist :-) So please do correct me where I went wrong.</div><div><br></div><div>For clarity, this is how the 2 instances of unicam advertises themselves:<br></div><div><br></div><div>pi@cm4:~ $ media-ctl -d /dev/media0 -p<br>Media controller API version 5.10.78<br><br>Media device information<br>------------------------<br>driver unicam<br>model unicam<br>serial <br>bus info platform:fe800000.csi<br>hw revision 0x0<br>driver version 5.10.78<br><br>Device topology<br>- entity 1: imx219 0-0010 (2 pads, 2 links)<br> type V4L2 subdev subtype Sensor flags 0<br> device node name /dev/v4l-subdev0<br> pad0: Source<br> [fmt:SRGGB10_1X10/1640x1232 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range<br> crop.bounds:(8,8)/3280x2464<br> crop:(8,8)/3280x2464]<br> -> "unicam1-image":0 [ENABLED,IMMUTABLE]<br> pad1: Source<br> [fmt:unknown/16384x1 field:none<br> crop.bounds:(8,8)/3280x2464<br> crop:(8,8)/3280x2464]<br> -> "unicam1-embedded":0 [ENABLED,IMMUTABLE]<br><br>- entity 4: unicam1-image (1 pad, 1 link)<br> type Node subtype V4L flags 1<br> device node name /dev/video0<br> pad0: Sink<br> <- "imx219 0-0010":0 [ENABLED,IMMUTABLE]<br><br>- entity 10: unicam1-embedded (1 pad, 1 link)<br> type Node subtype V4L flags 0<br> device node name /dev/video1<br> pad0: Sink<br> <- "imx219 0-0010":1 [ENABLED,IMMUTABLE]<br></div><div><br></div><div>pi@cm4:~ $ media-ctl -d /dev/media1 -p<br>Media controller API version 5.10.78<br><br>Media device information<br>------------------------<br>driver unicam<br>model unicam<br>serial <br>bus info platform:fe801000.csi<br>hw revision 0x0<br>driver version 5.10.78<br><br>Device topology<br>- entity 1: imx219 10-0010 (2 pads, 2 links)<br> type V4L2 subdev subtype Sensor flags 0<br> device node name /dev/v4l-subdev1<br> pad0: Source<br> [fmt:SRGGB10_1X10/1640x1232 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range<br> crop.bounds:(8,8)/3280x2464<br> crop:(8,8)/3280x2464]<br> -> "unicam0-image":0 [ENABLED,IMMUTABLE]<br> pad1: Source<br> [fmt:unknown/16384x1 field:none<br> crop.bounds:(8,8)/3280x2464<br> crop:(8,8)/3280x2464]<br> -> "unicam0-embedded":0 [ENABLED,IMMUTABLE]<br><br>- entity 4: unicam0-image (1 pad, 1 link)<br> type Node subtype V4L flags 1<br> device node name /dev/video2<br> pad0: Sink<br> <- "imx219 10-0010":0 [ENABLED,IMMUTABLE]<br><br>- entity 10: unicam0-embedded (1 pad, 1 link)<br> type Node subtype V4L flags 0<br> device node name /dev/video3<br> pad0: Sink<br> <- "imx219 10-0010":1 [ENABLED,IMMUTABLE]<br></div></div></div>