<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 23 Nov 2021 at 00:09, 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>
Thank you for the patch.<br>
<br>
On Mon, Nov 22, 2021 at 01:56:50PM +0000, Naushir Patuck wrote:<br>
> Hi Laurent,<br>
> <br>
> Thank you for the feedback. I apologize for the possibly very obvious questions<br>
> that follow below - but this enumeration business is a bit unclear to me :-)<br>
<br>
I never accept apologies for such questions, because there's nothing to<br>
apologize for :-)<br>
<br>
> On Mon, 22 Nov 2021 at 13:01, Laurent Pinchart wrote:<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>
> <br>
> Yes!<br>
> <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>
> <br>
> I'm not sure - see below.<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>
> <br>
> I've set things up so that the 2 instances of unicam have the same device<br>
> name ("unicam"). To distinguish the different instances, the pad entity names<br>
> have an index in the string. Ditto for the ISP driver.<br>
<br>
The media controller API doesn't report a device name for a<br>
media_device, but a driver name, a model name, and bus information. The<br>
driver name should be identical for all instances. The model name can<br>
differentiate different models (for instance if the Unicam IP core<br>
existed in different versions in different SoCs), but should be<br>
identical for different instances of the same device. The bus<br>
information is the field that tells instances apart.<br>
<br>
> If I were to remove the isp.add() calls, then DeviceEnumerator would only ever find<br>
> the first match to "bcm2825-isp" device and I would never be able to access the<br>
> second instance, is that correct? This loops back to the previous question, and<br>
> why I am not sure if regexps in DeviceMatch would make any difference.<br>
<br>
Once a pipeline handler acquires a media device, it is removed from the<br>
pool of available devices. The next call to match() will call<br>
acquireMediaDevice() with the same driver name ("bcm2825-isp"), which<br>
will then return the second ISP instance as the first one will have been<br>
acquired already.<br>
<br>
This mechanism allows pipeline handlers to not have to differentiate<br>
between identical instances of the same device, when there's no hardware<br>
constraint that requires otherwise (for instance if the Unicam and ISP<br>
instances had to be paired exactly, you would be able to use the media<br>
device acquisition mechanism without caring about the instance ID for<br>
Unicam, but you would then have to acquire a specific ISP instance).<br></blockquote><div><br></div><div>Thank you! That certainly clarifies things for me. It also makes it possible</div><div>to simplify my enumeration code significantly if I keep the device driver name</div><div>and entity names the same for all instances. Will post an update with those</div><div>changes soon.</div><div><br></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>
> > > + 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>
> <br>
> Same problem, if all instances have the same driver name and entity names, I can<br>
> never acquire the second instance, as the first instance will always return a match.<br>
> <br>
> Again, I am sure my interpretation of the API is not fully correct, so these issues may<br>
> not actually exist :-) So please do correct me where I went wrong.<br>
> <br>
> For clarity, this is how the 2 instances of unicam advertises themselves:<br>
> <br>
> 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 crop.bounds:(8,8)/3280x2464 crop:(8,8)/3280x2464]<br>
> -> "unicam1-image":0 [ENABLED,IMMUTABLE]<br>
> pad1: Source<br>
> [fmt:unknown/16384x1 field:none crop.bounds:(8,8)/3280x2464 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>
> <br>
> 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 crop.bounds:(8,8)/3280x2464 crop:(8,8)/3280x2464]<br>
> -> "unicam0-image":0 [ENABLED,IMMUTABLE]<br>
> pad1: Source<br>
> [fmt:unknown/16384x1 field:none crop.bounds:(8,8)/3280x2464 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>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>