[libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow registration of multiple cameras
Naushir Patuck
naush at raspberrypi.com
Tue Nov 23 10:43:01 CET 2021
Hi Laurent,
On Tue, 23 Nov 2021 at 00:09, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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).
>
Thank you! That certainly clarifies things for me. It also makes it
possible
to simplify my enumeration code significantly if I keep the device driver
name
and entity names the same for all instances. Will post an update with those
changes soon.
Naush
>
> > > > + 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211123/723c7df2/attachment-0001.htm>
More information about the libcamera-devel
mailing list