[libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow registration of multiple cameras
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Nov 23 01:09:29 CET 2021
Hi Naush,
Thank you for the patch.
On Mon, Nov 22, 2021 at 01:56:50PM +0000, Naushir Patuck wrote:
> Hi Laurent,
>
> Thank you for the feedback. I apologize for the possibly very obvious questions
> that follow below - but this enumeration business is a bit unclear to me :-)
I never accept apologies for such questions, because there's nothing to
apologize for :-)
> On Mon, 22 Nov 2021 at 13:01, Laurent Pinchart wrote:
> > On Mon, Nov 22, 2021 at 12:34:27PM +0000, Naushir Patuck wrote:
> > > Expand the pipeline handler camera registration to correctly handle multiple
> > > cameras attached to the platform. For example, Raspberry Pi Compute Module
> > > platforms have two camera connectors, and this change would allow the user to
> > > select either of the two cameras to run.
> > >
> > > There are associated kernel driver changes for both Unicam and the ISP needed
> > > to correctly advertise multiple media devices and nodes for multi-camera usage:
> > >
> > > https://github.com/raspberrypi/linux/pull/4140
> > > https://github.com/raspberrypi/linux/pull/4709
> > >
> > > However, this change is backward compatible with kernel builds that do not have
> > > these changes for standard single camera usage.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > > .../pipeline/raspberrypi/raspberrypi.cpp | 113 ++++++++++++------
> > > 1 file changed, 74 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 9aa7e9eef5e7..3f9e15514ed9 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -311,14 +311,11 @@ private:
> > > return static_cast<RPiCameraData *>(camera->_d());
> > > }
> > >
> > > - bool registerCameras();
> > > + int registerCameras(MediaDevice *unicam, MediaDevice *isp, const std::string &deviceId);
> >
> > Given that a pipeline handler instance doesn't register multiple cameras
> > anymore in v2, should this be called registerCamera() ?
>
> Yes!
>
> > > int queueAllBuffers(Camera *camera);
> > > int prepareBuffers(Camera *camera);
> > > void freeBuffers(Camera *camera);
> > > void mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask);
> > > -
> > > - MediaDevice *unicam_;
> > > - MediaDevice *isp_;
> > > };
> > >
> > > RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)
> > > @@ -509,7 +506,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > > }
> > >
> > > PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)
> > > - : PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)
> > > + : PipelineHandler(manager)
> > > {
> > > }
> > >
> > > @@ -993,49 +990,85 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> > >
> > > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> > > {
> > > - DeviceMatch unicam("unicam");
> > > - DeviceMatch isp("bcm2835-isp");
> > > + MediaDevice *unicamDevice, *ispDevice;
> > > + std::string deviceId;
> > >
> > > - unicam.add("unicam-image");
> > > + /*
> > > + * String of indexes to append to the entity names when searching for
> > > + * the Unican media devices. The first string is empty (un-indexed) to
> > > + * to maintain backward compatability with old versions of the Unicam
> > > + * kernel driver that did not advertise instance indexes.
> > > + */
> > > + for (const std::string &id : { "", "0", "1" }) {
> > > + DeviceMatch unicam("unicam");
> > > + unicam.add("unicam" + id + "-image");
> > > + unicamDevice = acquireMediaDevice(enumerator, unicam);
> > >
> > > - isp.add("bcm2835-isp0-output0"); /* Input */
> > > - isp.add("bcm2835-isp0-capture1"); /* Output 0 */
> > > - isp.add("bcm2835-isp0-capture2"); /* Output 1 */
> > > - isp.add("bcm2835-isp0-capture3"); /* Stats */
> > > + if (unicamDevice) {
> > > + deviceId = id == "1" ? "1" : "0";
> > > + break;
> > > + }
> > > + }
> >
> > Wouldn't it be better to extend DeviceMatch to accept regexps ?
> > https://en.cppreference.com/w/cpp/header/regex should make that easy.
>
> I'm not sure - see below.
>
> > > - unicam_ = acquireMediaDevice(enumerator, unicam);
> > > - if (!unicam_)
> > > + if (!unicamDevice) {
> > > + LOG(RPI, Debug) << "Unable to acquire a Unicam instance";
> > > return false;
> > > + }
> > >
> > > - isp_ = acquireMediaDevice(enumerator, isp);
> > > - if (!isp_)
> > > + DeviceMatch isp("bcm2835-isp");
> > > + isp.add("bcm2835-isp" + deviceId + "-output0"); /* Input */
> > > + isp.add("bcm2835-isp" + deviceId + "-capture1"); /* Output 0 */
> > > + isp.add("bcm2835-isp" + deviceId + "-capture2"); /* Output 1 */
> > > + isp.add("bcm2835-isp" + deviceId + "-capture3"); /* Stats */
> >
> > Unless you need to match the unicam and ISP instances, you could here
> > drop the isp.add() calls as matching on the device name should be
> > enough. Unless there are bcm2835-isp instances that don't have the
> > needed nodes ?
>
> I've set things up so that the 2 instances of unicam have the same device
> name ("unicam"). To distinguish the different instances, the pad entity names
> have an index in the string. Ditto for the ISP driver.
The media controller API doesn't report a device name for a
media_device, but a driver name, a model name, and bus information. The
driver name should be identical for all instances. The model name can
differentiate different models (for instance if the Unicam IP core
existed in different versions in different SoCs), but should be
identical for different instances of the same device. The bus
information is the field that tells instances apart.
> If I were to remove the isp.add() calls, then DeviceEnumerator would only ever find
> the first match to "bcm2825-isp" device and I would never be able to access the
> second instance, is that correct? This loops back to the previous question, and
> why I am not sure if regexps in DeviceMatch would make any difference.
Once a pipeline handler acquires a media device, it is removed from the
pool of available devices. The next call to match() will call
acquireMediaDevice() with the same driver name ("bcm2825-isp"), which
will then return the second ISP instance as the first one will have been
acquired already.
This mechanism allows pipeline handlers to not have to differentiate
between identical instances of the same device, when there's no hardware
constraint that requires otherwise (for instance if the Unicam and ISP
instances had to be paired exactly, you would be able to use the media
device acquisition mechanism without caring about the instance ID for
Unicam, but you would then have to acquire a specific ISP instance).
> > > + ispDevice = acquireMediaDevice(enumerator, isp);
> > > +
> > > + if (!ispDevice) {
> > > + LOG(RPI, Error) << "Unable to acquire ISP instance " << deviceId;
> > > return false;
> > > + }
> > > +
> > > + int ret = registerCameras(unicamDevice, ispDevice, deviceId);
> > > + if (ret) {
> > > + LOG(RPI, Error) << "Failed to register camera: " << ret;
> > > + return false;
> > > + }
> > >
> > > - return registerCameras();
> > > + return true;
> > > }
> > >
> > > -bool PipelineHandlerRPi::registerCameras()
> > > +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam, MediaDevice *isp,
> > > + const std::string &deviceId)
> > > {
> > > std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
> > > +
> > > if (!data->dmaHeap_.isValid())
> > > - return false;
> > > + return -ENOMEM;
> > > +
> > > + MediaEntity *unicamImage = unicam->getEntityByName("unicam" + deviceId + "-image");
> > > + MediaEntity *ispOutput0 = isp->getEntityByName("bcm2835-isp" + deviceId + "-output0");
> > > + MediaEntity *ispCapture1 = isp->getEntityByName("bcm2835-isp" + deviceId + "-capture1");
> > > + MediaEntity *ispCapture2 = isp->getEntityByName("bcm2835-isp" + deviceId + "-capture2");
> > > + MediaEntity *ispCapture3 = isp->getEntityByName("bcm2835-isp" + deviceId + "-capture3");
> >
> > I'm wondering if wildcards would be nice here too, but maybe a simpler
> > option would be to not include the device ID in the entity names in the
> > driver ? We have a bus info field that the kernel reports for the media
> > device, to differentiate between multiple instances of the same device.
> > I suppose we are missing guidelines on the kernel side regarding entity
> > naming. Sakari, any opinion on this ?
>
> Same problem, if all instances have the same driver name and entity names, I can
> never acquire the second instance, as the first instance will always return a match.
>
> Again, I am sure my interpretation of the API is not fully correct, so these issues may
> not actually exist :-) So please do correct me where I went wrong.
>
> For clarity, this is how the 2 instances of unicam advertises themselves:
>
> pi at cm4:~ $ media-ctl -d /dev/media0 -p
> Media controller API version 5.10.78
>
> Media device information
> ------------------------
> driver unicam
> model unicam
> serial
> bus info platform:fe800000.csi
> hw revision 0x0
> driver version 5.10.78
>
> Device topology
> - entity 1: imx219 0-0010 (2 pads, 2 links)
> type V4L2 subdev subtype Sensor flags 0
> device node name /dev/v4l-subdev0
> pad0: Source
> [fmt:SRGGB10_1X10/1640x1232 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range crop.bounds:(8,8)/3280x2464 crop:(8,8)/3280x2464]
> -> "unicam1-image":0 [ENABLED,IMMUTABLE]
> pad1: Source
> [fmt:unknown/16384x1 field:none crop.bounds:(8,8)/3280x2464 crop:(8,8)/3280x2464]
> -> "unicam1-embedded":0 [ENABLED,IMMUTABLE]
>
> - entity 4: unicam1-image (1 pad, 1 link)
> type Node subtype V4L flags 1
> device node name /dev/video0
> pad0: Sink
> <- "imx219 0-0010":0 [ENABLED,IMMUTABLE]
>
> - entity 10: unicam1-embedded (1 pad, 1 link)
> type Node subtype V4L flags 0
> device node name /dev/video1
> pad0: Sink
> <- "imx219 0-0010":1 [ENABLED,IMMUTABLE]
>
> pi at cm4:~ $ media-ctl -d /dev/media1 -p
> Media controller API version 5.10.78
>
> Media device information
> ------------------------
> driver unicam
> model unicam
> serial
> bus info platform:fe801000.csi
> hw revision 0x0
> driver version 5.10.78
>
> Device topology
> - entity 1: imx219 10-0010 (2 pads, 2 links)
> type V4L2 subdev subtype Sensor flags 0
> device node name /dev/v4l-subdev1
> pad0: Source
> [fmt:SRGGB10_1X10/1640x1232 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range crop.bounds:(8,8)/3280x2464 crop:(8,8)/3280x2464]
> -> "unicam0-image":0 [ENABLED,IMMUTABLE]
> pad1: Source
> [fmt:unknown/16384x1 field:none crop.bounds:(8,8)/3280x2464 crop:(8,8)/3280x2464]
> -> "unicam0-embedded":0 [ENABLED,IMMUTABLE]
>
> - entity 4: unicam0-image (1 pad, 1 link)
> type Node subtype V4L flags 1
> device node name /dev/video2
> pad0: Sink
> <- "imx219 10-0010":0 [ENABLED,IMMUTABLE]
>
> - entity 10: unicam0-embedded (1 pad, 1 link)
> type Node subtype V4L flags 0
> device node name /dev/video3
> pad0: Sink
> <- "imx219 10-0010":1 [ENABLED,IMMUTABLE]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list