| Message ID | 20200728003058.2871461-1-niklas.soderlund@ragnatech.se |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Tue, Jul 28, 2020 at 02:30:52AM +0200, Niklas Söderlund wrote: > Hello, > > This series aims to make and enforce unique camera names that are static > between system resets while keeping them user-friendly and adding more > information describing where the cameras are located in the system. > This v2 is a complete rewrite of v1 (libcamera: camera: Add camera ID) > of this series that centered around bus information instead of this v2 > that focus more on user friendly names. > > The weakness in this series is that not a lot of platforms describe the > rather new location and rotation properties in their devicetree/ACPI. > This leads to issues that can be observed below where both cameras on > the ipu3 and rkisp1 platforms are reported to be located on the front > while in reality one is located on the front and the other on the back. > This is not a shortcoming of this series however and will solve itself > once the platforms gets update firmware or when CameraSensor learns to > read this information from configuration files (or by some other means). > > Patch 3/6 also needs more work to query the device enumerator for the > sysfs path instead of building a path that assumes the standard naming > schema. This is a implementation detail and I think it's more important > to get speedy feedback on the over all approach of the series. > > Before this series camera names on different systems looked like this (I > do not have access to a simple pipeline device): > > - ipu3 > ov13858 8-0010 > ov5670 10-0036 > - raspberrypi > imx219 > - rkisp1 > ov5695 7-0036 > ov2685 7-003c > - uvcvideo > Venus USB2.0 Camera: Venus USB2 > Logitech Webcam C930e > - vimc > VIMC Sensor B > > With this series applied camera names on the same systems: > > - ipu3 > ov13858 location front (PipelineHandlerIPU3) > ov5670 location front (PipelineHandlerIPU3) > - raspberrypi > imx219 location front (PipelineHandlerRPi) > - rkisp1 > ov5695 location front (PipelineHandlerRkISP1) > ov2685 location front (PipelineHandlerRkISP1) > - uvcvideo > Venus USB2.0 Camera: Venus USB2 on bus 3:9 (PipelineHandlerUVC) > Logitech Webcam C930e on bus 3:4 serial 9F8F445E (PipelineHandlerUVC) > - vimc > Sensor B location front (PipelineHandlerVimc) I'm sorry if I sound negative, but I have comments on the general naming scheme this series builds on. I'm sorry you went down to a full implementation, discussing the names provided in that cover letter would have been enough to get feedbacks imho. That said, a few comments on the naming scheme 1) location: it is reported through camera properties, I don't see much value having it in the name, maybe in the "friendly" one only, but if I'm not mistaken this series makes the "friendly" name unique. I expect most applications (most, a lot..) to select cameras based on their location, and they will get it through the property, not by parsing the name. 1.a) In a system like a RPi, where cameras are connected through flex cables, cameras will be marked as 'external'. If you can connect two sensors named "amazingsensor" you would get two identical camera names 'amazingsensor location external' 2) pipeline handler name: I can see UVC co-existing with other pipeline handlers, but the UVC camera names won't collide with built-in camera names, so I don't see much value in reporting the pipeline handler that has created the camera I'm afraid. If we need to convey this information, this should be a camera property imho. We've ruled out -a lot- of information we could use to generate camera names: video device numbers are not stable, sysfs paths are not because of that, the bus identifiers might be re-assigned between reboots if behing a PCI bus... there's not much in the system we can rely on to construct a stable and unique name if we consider all systems to be equal. I however still think it should be useful to define a set of use cases we support and for each of them identify if there's some information we can use. In example, on a generic DT-based embedded system it would be enough to rely on 1) the media entity name 2) alphabetical order Combine the two and you have an ordering 1 ov13858-2-0047 2 ov5670-2-0036 Works for system with the same sensor, as they could not live on the same bus 1 ov5670-2-0036 2 ov5670-4-0036 For non-PCI system, the i2c bus number is stable, hence, the camera on the first bus comes first. For PCI, is there a PCI id or something -stable- we can rely on ? UVC you have built the vid/pid identifier here, right ? that seems stable to me but is it identical between two instances of the same device ? I think we really need to sort out this question before going into coding. That's why I have pointed you to udev, and systemd predictable names, because even if in a different context they do what we should aim to do: define a naming scheme, and I see there reported information we might found useful about "PCI slot number" and "USB port number chain". Knowing how they build those part could help us solve this issue, but it needs to be researched first. Thanks j > > Niklas Söderlund (6): > libcamera: camera: Append pipeline name to camera name > libcamera: camera: Generate camera name from a CameraSensor > libcamera: v4l2_device: Add method to lookup device path > libcamera: media_device: Expose media device serial number > libcamera: pipeline: uvcvideo: Generate unique camera names > libcamera: camera_manager: Enforce unique camera names > > include/libcamera/camera.h | 5 +++ > include/libcamera/internal/media_device.h | 2 + > include/libcamera/internal/v4l2_device.h | 1 + > src/libcamera/camera.cpp | 42 ++++++++++++++++++- > src/libcamera/camera_manager.cpp | 6 +-- > src/libcamera/media_device.cpp | 7 ++++ > src/libcamera/pipeline/ipu3/ipu3.cpp | 12 +++--- > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > src/libcamera/pipeline/simple/simple.cpp | 2 +- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 41 +++++++++++++++++- > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > src/libcamera/v4l2_device.cpp | 24 +++++++++++ > test/camera/buffer_import.cpp | 2 +- > test/camera/capture.cpp | 2 +- > test/camera/configuration_default.cpp | 2 +- > test/camera/configuration_set.cpp | 2 +- > test/camera/statemachine.cpp | 2 +- > test/controls/control_info_map.cpp | 2 +- > test/controls/control_list.cpp | 2 +- > test/serialization/serialization_test.h | 2 +- > 21 files changed, 142 insertions(+), 25 deletions(-) > > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, This series aims to make and enforce unique camera names that are static between system resets while keeping them user-friendly and adding more information describing where the cameras are located in the system. This v2 is a complete rewrite of v1 (libcamera: camera: Add camera ID) of this series that centered around bus information instead of this v2 that focus more on user friendly names. The weakness in this series is that not a lot of platforms describe the rather new location and rotation properties in their devicetree/ACPI. This leads to issues that can be observed below where both cameras on the ipu3 and rkisp1 platforms are reported to be located on the front while in reality one is located on the front and the other on the back. This is not a shortcoming of this series however and will solve itself once the platforms gets update firmware or when CameraSensor learns to read this information from configuration files (or by some other means). Patch 3/6 also needs more work to query the device enumerator for the sysfs path instead of building a path that assumes the standard naming schema. This is a implementation detail and I think it's more important to get speedy feedback on the over all approach of the series. Before this series camera names on different systems looked like this (I do not have access to a simple pipeline device): - ipu3 ov13858 8-0010 ov5670 10-0036 - raspberrypi imx219 - rkisp1 ov5695 7-0036 ov2685 7-003c - uvcvideo Venus USB2.0 Camera: Venus USB2 Logitech Webcam C930e - vimc VIMC Sensor B With this series applied camera names on the same systems: - ipu3 ov13858 location front (PipelineHandlerIPU3) ov5670 location front (PipelineHandlerIPU3) - raspberrypi imx219 location front (PipelineHandlerRPi) - rkisp1 ov5695 location front (PipelineHandlerRkISP1) ov2685 location front (PipelineHandlerRkISP1) - uvcvideo Venus USB2.0 Camera: Venus USB2 on bus 3:9 (PipelineHandlerUVC) Logitech Webcam C930e on bus 3:4 serial 9F8F445E (PipelineHandlerUVC) - vimc Sensor B location front (PipelineHandlerVimc) Niklas Söderlund (6): libcamera: camera: Append pipeline name to camera name libcamera: camera: Generate camera name from a CameraSensor libcamera: v4l2_device: Add method to lookup device path libcamera: media_device: Expose media device serial number libcamera: pipeline: uvcvideo: Generate unique camera names libcamera: camera_manager: Enforce unique camera names include/libcamera/camera.h | 5 +++ include/libcamera/internal/media_device.h | 2 + include/libcamera/internal/v4l2_device.h | 1 + src/libcamera/camera.cpp | 42 ++++++++++++++++++- src/libcamera/camera_manager.cpp | 6 +-- src/libcamera/media_device.cpp | 7 ++++ src/libcamera/pipeline/ipu3/ipu3.cpp | 12 +++--- .../pipeline/raspberrypi/raspberrypi.cpp | 3 +- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- src/libcamera/pipeline/simple/simple.cpp | 2 +- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 41 +++++++++++++++++- src/libcamera/pipeline/vimc/vimc.cpp | 4 +- src/libcamera/v4l2_device.cpp | 24 +++++++++++ test/camera/buffer_import.cpp | 2 +- test/camera/capture.cpp | 2 +- test/camera/configuration_default.cpp | 2 +- test/camera/configuration_set.cpp | 2 +- test/camera/statemachine.cpp | 2 +- test/controls/control_info_map.cpp | 2 +- test/controls/control_list.cpp | 2 +- test/serialization/serialization_test.h | 2 +- 21 files changed, 142 insertions(+), 25 deletions(-)