[RFC,v1,03/20] libcamera: camera_sensor: Generate sensor IDs from a media entity
diff mbox series

Message ID 20260918080734.1228227-4-naush@raspberrypi.com
State New
Headers show
Series
  • libcamera: New enumeration API
Related show

Commit Message

Naushir Patuck Sept. 18, 2026, 7:59 a.m. UTC 
Add a CameraSensorFactoryBase::generateId() helper that computes the
sensor ID from a MediaEntity using sysfs without opening the device.
Update CameraSensorLegacy and CameraSensorRaw to generate the sensor ID
with this new helper.

This change is in preparation for generating camera IDs at device
enumeration time.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/internal/camera_sensor.h    |  1 +
 src/libcamera/sensor/camera_sensor.cpp        | 27 +++++++++++++++++++
 src/libcamera/sensor/camera_sensor_legacy.cpp |  6 ++---
 src/libcamera/sensor/camera_sensor_raw.cpp    |  3 +--
 4 files changed, 32 insertions(+), 5 deletions(-)

Comments

Jacopo Mondi Oct. 16, 2026, 10:53 a.m. UTC | #1 
Hi Naush

On Fri, Sep 18, 2026 at 08:59:41AM +0100, Naushir Patuck wrote:
> Add a CameraSensorFactoryBase::generateId() helper that computes the
> sensor ID from a MediaEntity using sysfs without opening the device.
> Update CameraSensorLegacy and CameraSensorRaw to generate the sensor ID
> with this new helper.
>
> This change is in preparation for generating camera IDs at device
> enumeration time.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/internal/camera_sensor.h    |  1 +
>  src/libcamera/sensor/camera_sensor.cpp        | 27 +++++++++++++++++++
>  src/libcamera/sensor/camera_sensor_legacy.cpp |  6 ++---
>  src/libcamera/sensor/camera_sensor_raw.cpp    |  3 +--
>  4 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 2a2f1f2c936d..17080ba32d37 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -92,6 +92,7 @@ public:
>  	virtual ~CameraSensorFactoryBase() = default;

Is the CameraSensorFactoryBase class the right place for this ?
True, the alternative would be to create a derived class of CameraSensor
that could be instantiated and only implement the static function
there.

Not sure it's worth it, and CameraSensorFactoryBase() already have
the
        static std::unique_ptr<CameraSensor> create(MediaEntity *entity);

static helper which takes an *entity, so the new helper is not that
out of place maybe ?

>
>  	static std::unique_ptr<CameraSensor> create(MediaEntity *entity);
> +	static std::string generateId(const MediaEntity *entity);
>
>  	const std::string &name() const { return name_; }
>  	int priority() const { return priority_; }
> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp
> index 580d3626216c..05654354ea1b 100644
> --- a/src/libcamera/sensor/camera_sensor.cpp
> +++ b/src/libcamera/sensor/camera_sensor.cpp
> @@ -13,6 +13,7 @@
>  #include <libcamera/base/log.h>
>
>  #include "libcamera/internal/media_object.h"
> +#include "libcamera/internal/sysfs.h"
>
>  /**
>   * \file camera_sensor.h
> @@ -474,6 +475,32 @@ std::unique_ptr<CameraSensor> CameraSensorFactoryBase::create(MediaEntity *entit
>  	return nullptr;
>  }
>
> +/**
> + * \brief Generate a unique ID for a camera sensor
> + * \param[in] entity The media entity that corresponds to the camera sensor
> + *
> + * The ID is derived from the firmware description (device tree or ACPI) of the
> + * device associated with the sensor's media \a entity. It is unique and stable
> + * as long as the system firmware is not modified. Sensors that the firmware
> + * does not describe have no ID.
> + *
> + * The ID is computed from sysfs only, using the device numbers stored in the
> + * media graph, without opening or otherwise accessing the sensor device.
> + *
> + * \sa sysfs::firmwareNodePath()
> + *
> + * \return The sensor ID on success or an empty string on failure
> + */
> +std::string CameraSensorFactoryBase::generateId(const MediaEntity *entity)
> +{
> +	const std::string devPath = sysfs::devicePath(entity->deviceMajor(),
> +						      entity->deviceMinor());
> +	if (devPath.empty())
> +		return {};
> +
> +	return sysfs::firmwareNodePath(devPath);
> +}
> +

The only remaining user of V4L2Device::devicePath() is the uvcvideo
pipeline handler

	std::ifstream file(video_->devicePath() + "/../removable");

Should we replace it with

	std::ifstream file(sysfs::devicePath(video_->deviceNode()) + "/../removable");

And remove V4L2Device::devicePath() ?

>  /**
>   * \fn CameraSensorFactoryBase::name()
>   * \brief Retrieve the camera sensor factory name
> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> index 83e2c2593215..99661f6fd02e 100644
> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
> @@ -302,10 +302,8 @@ int CameraSensorLegacy::init()
>
>  int CameraSensorLegacy::generateId()
>  {
> -	const std::string devPath = subdev_->devicePath();
> -
>  	/* Try to get ID from firmware description. */
> -	id_ = sysfs::firmwareNodePath(devPath);
> +	id_ = CameraSensorFactoryBase::generateId(entity_);
>  	if (!id_.empty())
>  		return 0;
>
> @@ -315,6 +313,8 @@ int CameraSensorLegacy::generateId()
>  	 * Verify it's a platform device and construct ID from the device path
>  	 * and model of sensor.
>  	 */
> +	const std::string devPath = sysfs::devicePath(entity_->deviceMajor(),
> +						      entity_->deviceMinor());
>  	if (devPath.find("/sys/devices/platform/", 0) == 0) {
>  		id_ = devPath.substr(strlen("/sys/devices/")) + " " + model();
>  		return 0;
> diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
> index 6344a34fc4c5..423b14af2890 100644
> --- a/src/libcamera/sensor/camera_sensor_raw.cpp
> +++ b/src/libcamera/sensor/camera_sensor_raw.cpp
> @@ -37,7 +37,6 @@
>  #include "libcamera/internal/camera_sensor_properties.h"
>  #include "libcamera/internal/formats.h"
>  #include "libcamera/internal/media_device.h"
> -#include "libcamera/internal/sysfs.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
>
>  namespace libcamera {
> @@ -571,7 +570,7 @@ int CameraSensorRaw::initProperties()
>  	properties_.set(properties::Model, utils::toAscii(model_));
>
>  	/* Generate a unique ID for the sensor. */
> -	id_ = sysfs::firmwareNodePath(subdev_->devicePath());
> +	id_ = CameraSensorFactoryBase::generateId(entity_);
>  	if (id_.empty()) {
>  		LOG(CameraSensor, Error) << "Can't generate sensor ID";
>  		return -EINVAL;
> --
> 2.53.0
>
Naushir Patuck Oct. 16, 2026, 11:27 a.m. UTC | #2 
Hi Jacopo,

On Fri, 16 Oct 2026 at 11:54, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Naush
>
> On Fri, Sep 18, 2026 at 08:59:41AM +0100, Naushir Patuck wrote:
> > Add a CameraSensorFactoryBase::generateId() helper that computes the
> > sensor ID from a MediaEntity using sysfs without opening the device.
> > Update CameraSensorLegacy and CameraSensorRaw to generate the sensor ID
> > with this new helper.
> >
> > This change is in preparation for generating camera IDs at device
> > enumeration time.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  include/libcamera/internal/camera_sensor.h    |  1 +
> >  src/libcamera/sensor/camera_sensor.cpp        | 27 +++++++++++++++++++
> >  src/libcamera/sensor/camera_sensor_legacy.cpp |  6 ++---
> >  src/libcamera/sensor/camera_sensor_raw.cpp    |  3 +--
> >  4 files changed, 32 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index 2a2f1f2c936d..17080ba32d37 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -92,6 +92,7 @@ public:
> >       virtual ~CameraSensorFactoryBase() = default;
>
> Is the CameraSensorFactoryBase class the right place for this ?
> True, the alternative would be to create a derived class of CameraSensor
> that could be instantiated and only implement the static function
> there.
>
> Not sure it's worth it, and CameraSensorFactoryBase() already have
> the
>         static std::unique_ptr<CameraSensor> create(MediaEntity *entity);
>
> static helper which takes an *entity, so the new helper is not that
> out of place maybe ?

I must admit that I had the same thoughts as well, and I put it in
CameraSensorFactoryBase() somewhat arbitrarily.  If folks would prefer
somewhere else, I'm happy to consider alternatives.

Regards,
Naush


>
> >
> >       static std::unique_ptr<CameraSensor> create(MediaEntity *entity);
> > +     static std::string generateId(const MediaEntity *entity);
> >
> >       const std::string &name() const { return name_; }
> >       int priority() const { return priority_; }
> > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp
> > index 580d3626216c..05654354ea1b 100644
> > --- a/src/libcamera/sensor/camera_sensor.cpp
> > +++ b/src/libcamera/sensor/camera_sensor.cpp
> > @@ -13,6 +13,7 @@
> >  #include <libcamera/base/log.h>
> >
> >  #include "libcamera/internal/media_object.h"
> > +#include "libcamera/internal/sysfs.h"
> >
> >  /**
> >   * \file camera_sensor.h
> > @@ -474,6 +475,32 @@ std::unique_ptr<CameraSensor> CameraSensorFactoryBase::create(MediaEntity *entit
> >       return nullptr;
> >  }
> >
> > +/**
> > + * \brief Generate a unique ID for a camera sensor
> > + * \param[in] entity The media entity that corresponds to the camera sensor
> > + *
> > + * The ID is derived from the firmware description (device tree or ACPI) of the
> > + * device associated with the sensor's media \a entity. It is unique and stable
> > + * as long as the system firmware is not modified. Sensors that the firmware
> > + * does not describe have no ID.
> > + *
> > + * The ID is computed from sysfs only, using the device numbers stored in the
> > + * media graph, without opening or otherwise accessing the sensor device.
> > + *
> > + * \sa sysfs::firmwareNodePath()
> > + *
> > + * \return The sensor ID on success or an empty string on failure
> > + */
> > +std::string CameraSensorFactoryBase::generateId(const MediaEntity *entity)
> > +{
> > +     const std::string devPath = sysfs::devicePath(entity->deviceMajor(),
> > +                                                   entity->deviceMinor());
> > +     if (devPath.empty())
> > +             return {};
> > +
> > +     return sysfs::firmwareNodePath(devPath);
> > +}
> > +
>
> The only remaining user of V4L2Device::devicePath() is the uvcvideo
> pipeline handler
>
>         std::ifstream file(video_->devicePath() + "/../removable");
>
> Should we replace it with
>
>         std::ifstream file(sysfs::devicePath(video_->deviceNode()) + "/../removable");
>
> And remove V4L2Device::devicePath() ?
>
> >  /**
> >   * \fn CameraSensorFactoryBase::name()
> >   * \brief Retrieve the camera sensor factory name
> > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> > index 83e2c2593215..99661f6fd02e 100644
> > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
> > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
> > @@ -302,10 +302,8 @@ int CameraSensorLegacy::init()
> >
> >  int CameraSensorLegacy::generateId()
> >  {
> > -     const std::string devPath = subdev_->devicePath();
> > -
> >       /* Try to get ID from firmware description. */
> > -     id_ = sysfs::firmwareNodePath(devPath);
> > +     id_ = CameraSensorFactoryBase::generateId(entity_);
> >       if (!id_.empty())
> >               return 0;
> >
> > @@ -315,6 +313,8 @@ int CameraSensorLegacy::generateId()
> >        * Verify it's a platform device and construct ID from the device path
> >        * and model of sensor.
> >        */
> > +     const std::string devPath = sysfs::devicePath(entity_->deviceMajor(),
> > +                                                   entity_->deviceMinor());
> >       if (devPath.find("/sys/devices/platform/", 0) == 0) {
> >               id_ = devPath.substr(strlen("/sys/devices/")) + " " + model();
> >               return 0;
> > diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
> > index 6344a34fc4c5..423b14af2890 100644
> > --- a/src/libcamera/sensor/camera_sensor_raw.cpp
> > +++ b/src/libcamera/sensor/camera_sensor_raw.cpp
> > @@ -37,7 +37,6 @@
> >  #include "libcamera/internal/camera_sensor_properties.h"
> >  #include "libcamera/internal/formats.h"
> >  #include "libcamera/internal/media_device.h"
> > -#include "libcamera/internal/sysfs.h"
> >  #include "libcamera/internal/v4l2_subdevice.h"
> >
> >  namespace libcamera {
> > @@ -571,7 +570,7 @@ int CameraSensorRaw::initProperties()
> >       properties_.set(properties::Model, utils::toAscii(model_));
> >
> >       /* Generate a unique ID for the sensor. */
> > -     id_ = sysfs::firmwareNodePath(subdev_->devicePath());
> > +     id_ = CameraSensorFactoryBase::generateId(entity_);
> >       if (id_.empty()) {
> >               LOG(CameraSensor, Error) << "Can't generate sensor ID";
> >               return -EINVAL;
> > --
> > 2.53.0
> >

Patch
diff mbox series 

diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
index 2a2f1f2c936d..17080ba32d37 100644
--- a/include/libcamera/internal/camera_sensor.h
+++ b/include/libcamera/internal/camera_sensor.h
@@ -92,6 +92,7 @@  public:
 	virtual ~CameraSensorFactoryBase() = default;
 
 	static std::unique_ptr<CameraSensor> create(MediaEntity *entity);
+	static std::string generateId(const MediaEntity *entity);
 
 	const std::string &name() const { return name_; }
 	int priority() const { return priority_; }
diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp
index 580d3626216c..05654354ea1b 100644
--- a/src/libcamera/sensor/camera_sensor.cpp
+++ b/src/libcamera/sensor/camera_sensor.cpp
@@ -13,6 +13,7 @@ 
 #include <libcamera/base/log.h>
 
 #include "libcamera/internal/media_object.h"
+#include "libcamera/internal/sysfs.h"
 
 /**
  * \file camera_sensor.h
@@ -474,6 +475,32 @@  std::unique_ptr<CameraSensor> CameraSensorFactoryBase::create(MediaEntity *entit
 	return nullptr;
 }
 
+/**
+ * \brief Generate a unique ID for a camera sensor
+ * \param[in] entity The media entity that corresponds to the camera sensor
+ *
+ * The ID is derived from the firmware description (device tree or ACPI) of the
+ * device associated with the sensor's media \a entity. It is unique and stable
+ * as long as the system firmware is not modified. Sensors that the firmware
+ * does not describe have no ID.
+ *
+ * The ID is computed from sysfs only, using the device numbers stored in the
+ * media graph, without opening or otherwise accessing the sensor device.
+ *
+ * \sa sysfs::firmwareNodePath()
+ *
+ * \return The sensor ID on success or an empty string on failure
+ */
+std::string CameraSensorFactoryBase::generateId(const MediaEntity *entity)
+{
+	const std::string devPath = sysfs::devicePath(entity->deviceMajor(),
+						      entity->deviceMinor());
+	if (devPath.empty())
+		return {};
+
+	return sysfs::firmwareNodePath(devPath);
+}
+
 /**
  * \fn CameraSensorFactoryBase::name()
  * \brief Retrieve the camera sensor factory name
diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
index 83e2c2593215..99661f6fd02e 100644
--- a/src/libcamera/sensor/camera_sensor_legacy.cpp
+++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
@@ -302,10 +302,8 @@  int CameraSensorLegacy::init()
 
 int CameraSensorLegacy::generateId()
 {
-	const std::string devPath = subdev_->devicePath();
-
 	/* Try to get ID from firmware description. */
-	id_ = sysfs::firmwareNodePath(devPath);
+	id_ = CameraSensorFactoryBase::generateId(entity_);
 	if (!id_.empty())
 		return 0;
 
@@ -315,6 +313,8 @@  int CameraSensorLegacy::generateId()
 	 * Verify it's a platform device and construct ID from the device path
 	 * and model of sensor.
 	 */
+	const std::string devPath = sysfs::devicePath(entity_->deviceMajor(),
+						      entity_->deviceMinor());
 	if (devPath.find("/sys/devices/platform/", 0) == 0) {
 		id_ = devPath.substr(strlen("/sys/devices/")) + " " + model();
 		return 0;
diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
index 6344a34fc4c5..423b14af2890 100644
--- a/src/libcamera/sensor/camera_sensor_raw.cpp
+++ b/src/libcamera/sensor/camera_sensor_raw.cpp
@@ -37,7 +37,6 @@ 
 #include "libcamera/internal/camera_sensor_properties.h"
 #include "libcamera/internal/formats.h"
 #include "libcamera/internal/media_device.h"
-#include "libcamera/internal/sysfs.h"
 #include "libcamera/internal/v4l2_subdevice.h"
 
 namespace libcamera {
@@ -571,7 +570,7 @@  int CameraSensorRaw::initProperties()
 	properties_.set(properties::Model, utils::toAscii(model_));
 
 	/* Generate a unique ID for the sensor. */
-	id_ = sysfs::firmwareNodePath(subdev_->devicePath());
+	id_ = CameraSensorFactoryBase::generateId(entity_);
 	if (id_.empty()) {
 		LOG(CameraSensor, Error) << "Can't generate sensor ID";
 		return -EINVAL;