[libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow registration of multiple cameras
Naushir Patuck
naush at raspberrypi.com
Mon Nov 22 14:56:50 CET 2021
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 :-)
On Mon, 22 Nov 2021 at 13:01, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> (CC'ing Sakari)
>
> Thank you for the patch.
>
> 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.
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.
>
> > + 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]
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211122/a206f3b5/attachment-0001.htm>
More information about the libcamera-devel
mailing list