| Message ID | 20211122123427.808484-3-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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() ? > 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. > > - 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 ? > + 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 ? > + > + if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3) > + return -ENOENT; > > /* Locate and open the unicam video streams. */ > - data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image")); > + data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicamImage); > > /* An embedded data node will not be present if the sensor does not support it. */ > - MediaEntity *embeddedEntity = unicam_->getEntityByName("unicam-embedded"); > - if (embeddedEntity) { > - data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", embeddedEntity); > + MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam" + deviceId + "-embedded"); > + if (unicamEmbedded) { > + data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded); > data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(), > &RPiCameraData::unicamBufferDequeue); > } > > /* Tag the ISP input stream as an import stream. */ > - data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true); > - data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1")); > - data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2")); > - data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3")); > + data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, true); > + data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1); > + data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2); > + data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3); > > /* Wire up all the buffer connections. */ > data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted); > @@ -1046,7 +1079,7 @@ bool PipelineHandlerRPi::registerCameras() > data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue); > > /* Identify the sensor. */ > - for (MediaEntity *entity : unicam_->entities()) { > + for (MediaEntity *entity : unicam->entities()) { > if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) { > data->sensor_ = std::make_unique<CameraSensor>(entity); > break; > @@ -1054,23 +1087,23 @@ bool PipelineHandlerRPi::registerCameras() > } > > if (!data->sensor_) > - return false; > + return -EINVAL; > > if (data->sensor_->init()) > - return false; > + return -EINVAL; > > data->sensorFormats_ = populateSensorFormats(data->sensor_); > > ipa::RPi::SensorConfig sensorConfig; > if (data->loadIPA(&sensorConfig)) { > LOG(RPI, Error) << "Failed to load a suitable IPA library"; > - return false; > + return -EINVAL; > } > > - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) { > + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) { > LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!"; > sensorConfig.sensorMetadata = false; > - if (embeddedEntity) > + if (unicamEmbedded) > data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect(); > } > > @@ -1091,12 +1124,12 @@ bool PipelineHandlerRPi::registerCameras() > > for (auto stream : data->streams_) { > if (stream->dev()->open()) > - return false; > + continue; > } > > if (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) { > LOG(RPI, Error) << "Unicam driver does not use the MediaController, please update your kernel!"; > - return false; > + return -EINVAL; > } > > /* > @@ -1158,7 +1191,7 @@ bool PipelineHandlerRPi::registerCameras() > > if (!bayerFormat.isValid()) { > LOG(RPI, Error) << "No Bayer format found"; > - return false; > + return -EINVAL; > } > data->nativeBayerOrder_ = bayerFormat.order; > > @@ -1173,12 +1206,14 @@ bool PipelineHandlerRPi::registerCameras() > streams.insert(&data->isp_[Isp::Output1]); > > /* Create and register the camera. */ > - const std::string &id = data->sensor_->id(); > + const std::string &cameraId = data->sensor_->id(); > std::shared_ptr<Camera> camera = > - Camera::create(std::move(data), id, streams); > + Camera::create(std::move(data), cameraId, streams); > registerCamera(std::move(camera)); > > - return true; > + LOG(RPI, Info) << "Registered camera " << cameraId > + << " to instance \"" << deviceId << "\""; > + return 0; > } > > int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
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@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@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@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@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]
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@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@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@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]
Hi Laurent, On Tue, 23 Nov 2021 at 00:09, Laurent Pinchart < laurent.pinchart@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@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@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@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 >
Hi Laurent, Naushir, On Mon, Nov 22, 2021 at 03:00:40PM +0200, Laurent Pinchart wrote: > > -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 ? I think it'd be best to be able to match multiple fields, not just entity name. But I guess a regex goes a long way already. There are some (unwritten?) rules for naming devices and they mostly work. What's deviceId here? Would you need that as these are on separate MC devices? (I suppose it's up to the driver thought.)
Hi Sakari, On Tue, Nov 23, 2021 at 06:28:56PM +0200, Sakari Ailus wrote: > Hi Laurent, Naushir, > > On Mon, Nov 22, 2021 at 03:00:40PM +0200, Laurent Pinchart wrote: > > > -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 ? > > I think it'd be best to be able to match multiple fields, not just entity > name. But I guess a regex goes a long way already. > > There are some (unwritten?) rules for naming devices and they mostly work. This is the part I'd like your opinion on. I can read written rules myself, but have harder access to the ones that are only in your brain :-) From an MC point of view, my understanding is that the driver and model names should not be instance-specific (the model can vary between different SoCs that integrate different versions/models of the same IP, but shouldn't differ between multiple instances of the same IP in a given SoC, at least if those instances are identical). For entity names, we have a (probably unwritten) requirement for all entities in a given media device to have different names. What's your opinion about entity names when we have different media devices that correspond to multiple instances of the same IP core ? For instance, with two completely independent CSI-2 receivers, exposed as two separate media graphs, would you name the CSI-2 RX subdevs differently or identically ? If named differently, how would they differ ? > What's deviceId here? Would you need that as these are on separate MC > devices? (I suppose it's up to the driver thought.) In this specific case it's an index (equal to 0 or 1) that identifies a particular instance of the ISP (there's a single ISP at the hardware level, exposed as two virtual instances by the firmware).
Hi Laurent, On Tue, Nov 23, 2021 at 06:57:54PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Tue, Nov 23, 2021 at 06:28:56PM +0200, Sakari Ailus wrote: > > Hi Laurent, Naushir, > > > > On Mon, Nov 22, 2021 at 03:00:40PM +0200, Laurent Pinchart wrote: > > > > -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 ? > > > > I think it'd be best to be able to match multiple fields, not just entity > > name. But I guess a regex goes a long way already. > > > > There are some (unwritten?) rules for naming devices and they mostly work. > > This is the part I'd like your opinion on. I can read written rules > myself, but have harder access to the ones that are only in your brain > :-) > > From an MC point of view, my understanding is that the driver and model > names should not be instance-specific (the model can vary between > different SoCs that integrate different versions/models of the same IP, > but shouldn't differ between multiple instances of the same IP in a > given SoC, at least if those instances are identical). For entity names, Agreed. > we have a (probably unwritten) requirement for all entities in a given > media device to have different names. What's your opinion about entity > names when we have different media devices that correspond to multiple > instances of the same IP core ? For instance, with two completely > independent CSI-2 receivers, exposed as two separate media graphs, would > you name the CSI-2 RX subdevs differently or identically ? If named > differently, how would they differ ? I don't think there's any benefit from having globally unique media entity names, they simply need to be unique within a media device. Camera sensor entities have been named as: sensor name [subdev name if more than one ]i2c-address I guess that works fine as-is if you're targetting for a given system but that is seldom the case anymore. Still it should be possible to recognise different sensors from this if you know what you're looking for. Regular expressions should help here. > > What's deviceId here? Would you need that as these are on separate MC > > devices? (I suppose it's up to the driver thought.) > > In this specific case it's an index (equal to 0 or 1) that identifies a > particular instance of the ISP (there's a single ISP at the hardware > level, exposed as two virtual instances by the firmware). I suppose the two still are present in the same media device?
Hi Sakari and Laurent, On Wed, 24 Nov 2021 at 06:53, Sakari Ailus <sakari.ailus@iki.fi> wrote: > Hi Laurent, > > On Tue, Nov 23, 2021 at 06:57:54PM +0200, Laurent Pinchart wrote: > > Hi Sakari, > > > > On Tue, Nov 23, 2021 at 06:28:56PM +0200, Sakari Ailus wrote: > > > Hi Laurent, Naushir, > > > > > > On Mon, Nov 22, 2021 at 03:00:40PM +0200, Laurent Pinchart wrote: > > > > > -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 ? > > > > > > I think it'd be best to be able to match multiple fields, not just > entity > > > name. But I guess a regex goes a long way already. > > > > > > There are some (unwritten?) rules for naming devices and they mostly > work. > > > > This is the part I'd like your opinion on. I can read written rules > > myself, but have harder access to the ones that are only in your brain > > :-) > > > > From an MC point of view, my understanding is that the driver and model > > names should not be instance-specific (the model can vary between > > different SoCs that integrate different versions/models of the same IP, > > but shouldn't differ between multiple instances of the same IP in a > > given SoC, at least if those instances are identical). For entity names, > > Agreed. > > > we have a (probably unwritten) requirement for all entities in a given > > media device to have different names. What's your opinion about entity > > names when we have different media devices that correspond to multiple > > instances of the same IP core ? For instance, with two completely > > independent CSI-2 receivers, exposed as two separate media graphs, would > > you name the CSI-2 RX subdevs differently or identically ? If named > > differently, how would they differ ? > > I don't think there's any benefit from having globally unique media entity > names, they simply need to be unique within a media device. > > Camera sensor entities have been named as: > > sensor name [subdev name if more than one ]i2c-address > > I guess that works fine as-is if you're targetting for a given system but > that is seldom the case anymore. Still it should be possible to recognise > different sensors from this if you know what you're looking for. Regular > expressions should help here. > > > > What's deviceId here? Would you need that as these are on separate MC > > > devices? (I suppose it's up to the driver thought.) > > > > In this specific case it's an index (equal to 0 or 1) that identifies a > > particular instance of the ISP (there's a single ISP at the hardware > > level, exposed as two virtual instances by the firmware). > > I suppose the two still are present in the same media device? > Thanks for all the feedback! We have no problem duplicating driver and model names as well as entity names across multiple instances of the hardware (i.e. unindexed). In many ways this makes my life easier during enumeration in libcamera. The confusion (and subsequent discussion) started from my assumption that the names needed to be different in order to enumerate the different instances of the devices through libcamera. Laurent clarified that this is not the case, and I've got it working now. So if there are no objections, we will keep driver/entity names the same across all instances of the unicam and bcm2835_isp driver. Regards, Naush > > -- > Kind regards, > > Sakari Ailus >
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); 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; + } + } - 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 */ + 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"); + + if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3) + return -ENOENT; /* Locate and open the unicam video streams. */ - data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image")); + data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicamImage); /* An embedded data node will not be present if the sensor does not support it. */ - MediaEntity *embeddedEntity = unicam_->getEntityByName("unicam-embedded"); - if (embeddedEntity) { - data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", embeddedEntity); + MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam" + deviceId + "-embedded"); + if (unicamEmbedded) { + data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded); data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue); } /* Tag the ISP input stream as an import stream. */ - data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true); - data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1")); - data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2")); - data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3")); + data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, true); + data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1); + data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2); + data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3); /* Wire up all the buffer connections. */ data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted); @@ -1046,7 +1079,7 @@ bool PipelineHandlerRPi::registerCameras() data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue); /* Identify the sensor. */ - for (MediaEntity *entity : unicam_->entities()) { + for (MediaEntity *entity : unicam->entities()) { if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) { data->sensor_ = std::make_unique<CameraSensor>(entity); break; @@ -1054,23 +1087,23 @@ bool PipelineHandlerRPi::registerCameras() } if (!data->sensor_) - return false; + return -EINVAL; if (data->sensor_->init()) - return false; + return -EINVAL; data->sensorFormats_ = populateSensorFormats(data->sensor_); ipa::RPi::SensorConfig sensorConfig; if (data->loadIPA(&sensorConfig)) { LOG(RPI, Error) << "Failed to load a suitable IPA library"; - return false; + return -EINVAL; } - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) { + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) { LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!"; sensorConfig.sensorMetadata = false; - if (embeddedEntity) + if (unicamEmbedded) data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect(); } @@ -1091,12 +1124,12 @@ bool PipelineHandlerRPi::registerCameras() for (auto stream : data->streams_) { if (stream->dev()->open()) - return false; + continue; } if (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) { LOG(RPI, Error) << "Unicam driver does not use the MediaController, please update your kernel!"; - return false; + return -EINVAL; } /* @@ -1158,7 +1191,7 @@ bool PipelineHandlerRPi::registerCameras() if (!bayerFormat.isValid()) { LOG(RPI, Error) << "No Bayer format found"; - return false; + return -EINVAL; } data->nativeBayerOrder_ = bayerFormat.order; @@ -1173,12 +1206,14 @@ bool PipelineHandlerRPi::registerCameras() streams.insert(&data->isp_[Isp::Output1]); /* Create and register the camera. */ - const std::string &id = data->sensor_->id(); + const std::string &cameraId = data->sensor_->id(); std::shared_ptr<Camera> camera = - Camera::create(std::move(data), id, streams); + Camera::create(std::move(data), cameraId, streams); registerCamera(std::move(camera)); - return true; + LOG(RPI, Info) << "Registered camera " << cameraId + << " to instance \"" << deviceId << "\""; + return 0; } int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
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@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 113 ++++++++++++------ 1 file changed, 74 insertions(+), 39 deletions(-)