[libcamera-devel,v5,2/5] libcamera: camera_sensor: Generate a sensor ID

Message ID 20200729115055.3840110-3-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • libcamera: Generate unique and stable camera names
Related show

Commit Message

Niklas Söderlund July 29, 2020, 11:50 a.m. UTC
Generate a constant and unique string ID for the sensor. The ID is
generated from information from the firmware description of the camera
image sensor. The ID is unique and persistent across reboots of the
system.

For OF based systems the ID is the full path of the sensors in the
device tree description. For ACPI based systems the ID is the ACPI
firmware nodes path. Both ID sources are guaranteed to be unique and
persistent as long as the firmware of the system is not changed.

A special case is needed to deal with the VIMC pipeline that implements
a virtual pipeline that is not backed by any hardware device and is
therefore not described in the device firmware.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
* Changes since v4
- Fix spelling.

* Changes since v3
- Update commit message.
- Add description of how ID are generated to comment.
---
 include/libcamera/internal/camera_sensor.h |  4 +
 src/libcamera/camera_sensor.cpp            | 94 ++++++++++++++++++++++
 2 files changed, 98 insertions(+)

Comments

Laurent Pinchart Aug. 2, 2020, 6:26 p.m. UTC | #1
Hi Niklas,

Thank you for the patch.

On Wed, Jul 29, 2020 at 01:50:52PM +0200, Niklas Söderlund wrote:
> Generate a constant and unique string ID for the sensor. The ID is
> generated from information from the firmware description of the camera
> image sensor. The ID is unique and persistent across reboots of the
> system.
> 
> For OF based systems the ID is the full path of the sensors in the
> device tree description. For ACPI based systems the ID is the ACPI
> firmware nodes path. Both ID sources are guaranteed to be unique and
> persistent as long as the firmware of the system is not changed.
> 
> A special case is needed to deal with the VIMC pipeline that implements
> a virtual pipeline that is not backed by any hardware device and is
> therefore not described in the device firmware.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> * Changes since v4
> - Fix spelling.
> 
> * Changes since v3
> - Update commit message.
> - Add description of how ID are generated to comment.
> ---
>  include/libcamera/internal/camera_sensor.h |  4 +
>  src/libcamera/camera_sensor.cpp            | 94 ++++++++++++++++++++++
>  2 files changed, 98 insertions(+)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 06c8292ca30129de..e0d2d9f63b47c2fe 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -47,6 +47,7 @@ public:
>  	int init();
>  
>  	const std::string &model() const { return model_; }
> +	const std::string &id() const { return id_; }
>  	const MediaEntity *entity() const { return entity_; }
>  	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
>  	const std::vector<Size> &sizes() const { return sizes_; }
> @@ -67,11 +68,14 @@ protected:
>  	std::string logPrefix() const override;
>  
>  private:
> +	int generateID();
> +
>  	const MediaEntity *entity_;
>  	std::unique_ptr<V4L2Subdevice> subdev_;
>  	unsigned int pad_;
>  
>  	std::string model_;
> +	std::string id_;
>  
>  	V4L2Subdevice::Formats formats_;
>  	Size resolution_;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 350f49accad99c7b..6dc616945dad5ef1 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -9,10 +9,12 @@
>  
>  #include <algorithm>
>  #include <float.h>
> +#include <fstream>
>  #include <iomanip>
>  #include <limits.h>
>  #include <math.h>
>  #include <regex>
> +#include <sys/stat.h>
>  
>  #include <libcamera/property_ids.h>
>  
> @@ -204,6 +206,11 @@ int CameraSensor::init()
>  	if (ret < 0)
>  		return ret;
>  
> +	/* Generate a unique ID for the sensor. */
> +	ret = generateID();
> +	if (ret)
> +		return ret;
> +
>  	/* Retrieve and store the camera sensor properties. */
>  	const ControlInfoMap &controls = subdev_->controls();
>  	int32_t propertyValue;
> @@ -283,6 +290,26 @@ int CameraSensor::init()
>   * \return The sensor model name
>   */
>  
> +/**
> + * \fn CameraSensor::id()
> + * \brief Retrieve the sensor ID
> + *
> + * The sensor ID is a free-formed string that uniquely identifies the sensor in
> + * the system. The ID is persistent between different instances of libcamera and
> + * between resets of the system.
> + *
> + * For OF based systems the ID is the full path of the sensors in the device
> + * tree description. For ACPI based systems the ID is the ACPI firmware nodes
> + * path. Both ID sources are guaranteed to be unique and persistent as long as
> + * the firmware of the system is not changed.
> + *
> + * A special case is needed to deal with the VIMC pipeline that implements a
> + * virtual pipeline that is not backed by any hardware device and is therefore
> + * not described in the device firmware.
> + *
> + * \return The sensor ID
> + */
> +
>  /**
>   * \fn CameraSensor::entity()
>   * \brief Retrieve the sensor media entity
> @@ -541,4 +568,71 @@ std::string CameraSensor::logPrefix() const
>  	return "'" + entity_->name() + "'";
>  }
>  
> +int CameraSensor::generateID()
> +{
> +	std::string path, devPath = subdev_->devicePath();

We usually split declarations on multiple lines.

> +	struct stat statbuf;
> +
> +	/* Try to generate ID from OF device tree path.  */
> +	path = devPath + "/of_node";
> +	if (stat(path.c_str(), &statbuf) == 0) {

You can use

	if (!File::exists(path)) {

> +		char ofPath[PATH_MAX];
> +
> +		if (!realpath(path.c_str(), ofPath)) {

As mentioned in the review of 1/5, should we use realpath with nullptr
as the second argument ? Or maybe just use readlink() with PATH_MAX ?

> +			LOG(CameraSensor, Error) << "Failed to read sensor OF based ID";

s/OF based/OF-based/

> +			return -EINVAL;
> +		}
> +
> +		id_ = ofPath;
> +		const std::string dropStr = "/sys/firmware/devicetree/";

static const ?

Same comments for the other options below.

> +		if (id_.find(dropStr) == 0)
> +			id_.erase(0, dropStr.length());
> +
> +		return 0;
> +	}
> +
> +	/* Try to generate ID from ACPI path. */
> +	path = devPath + "/firmware_node/path";
> +	if (stat(path.c_str(), &statbuf) == 0) {
> +		std::ifstream file(path.c_str());
> +
> +		if (!file.is_open()) {
> +			LOG(CameraSensor, Error) << "Failed to read sensor ACPI based ID";
> +			return -EINVAL;
> +		}
> +
> +		std::getline(file, id_);
> +		file.close();
> +
> +		return 0;
> +	}
> +
> +	/*
> +	 * VIMC is a virtual video pipeline not backed hardware and has no OF
> +	 * or ACPI firmware nodes. Handle this pipeline as a special case and
> +	 * generate IDs based on the sensor model.
> +	 */
> +	path = devPath + "/modalias";
> +	if (stat(path.c_str(), &statbuf) == 0) {
> +		std::ifstream file(path.c_str());
> +
> +		if (!file.is_open()) {
> +			LOG(CameraSensor, Error) << "Failed to read sensor ACPI based ID";
> +			return -EINVAL;
> +		}
> +
> +		std::string modalias;
> +		std::getline(file, modalias);
> +		file.close();
> +
> +		if (modalias == "platform:vimc") {
> +			id_ = "VIMC " + model();
> +			return 0;
> +		}

Should we make this case more generic with the following (pseudo-code) ?

	id_ = basename(readlink(devPath + "/device")) + "/" + model_;

> +	}
> +
> +	LOG(CameraSensor, Error) << "Unknown sensor device type, can not generate ID";
> +	return -EINVAL;
> +}
> +
>  } /* namespace libcamera */
Niklas Söderlund Aug. 2, 2020, 9:46 p.m. UTC | #2
Hi Laurent,

Thanks for your feedback.

On 2020-08-02 21:26:55 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Wed, Jul 29, 2020 at 01:50:52PM +0200, Niklas Söderlund wrote:
> > Generate a constant and unique string ID for the sensor. The ID is
> > generated from information from the firmware description of the camera
> > image sensor. The ID is unique and persistent across reboots of the
> > system.
> > 
> > For OF based systems the ID is the full path of the sensors in the
> > device tree description. For ACPI based systems the ID is the ACPI
> > firmware nodes path. Both ID sources are guaranteed to be unique and
> > persistent as long as the firmware of the system is not changed.
> > 
> > A special case is needed to deal with the VIMC pipeline that implements
> > a virtual pipeline that is not backed by any hardware device and is
> > therefore not described in the device firmware.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> > * Changes since v4
> > - Fix spelling.
> > 
> > * Changes since v3
> > - Update commit message.
> > - Add description of how ID are generated to comment.
> > ---
> >  include/libcamera/internal/camera_sensor.h |  4 +
> >  src/libcamera/camera_sensor.cpp            | 94 ++++++++++++++++++++++
> >  2 files changed, 98 insertions(+)
> > 
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index 06c8292ca30129de..e0d2d9f63b47c2fe 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -47,6 +47,7 @@ public:
> >  	int init();
> >  
> >  	const std::string &model() const { return model_; }
> > +	const std::string &id() const { return id_; }
> >  	const MediaEntity *entity() const { return entity_; }
> >  	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> >  	const std::vector<Size> &sizes() const { return sizes_; }
> > @@ -67,11 +68,14 @@ protected:
> >  	std::string logPrefix() const override;
> >  
> >  private:
> > +	int generateID();
> > +
> >  	const MediaEntity *entity_;
> >  	std::unique_ptr<V4L2Subdevice> subdev_;
> >  	unsigned int pad_;
> >  
> >  	std::string model_;
> > +	std::string id_;
> >  
> >  	V4L2Subdevice::Formats formats_;
> >  	Size resolution_;
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 350f49accad99c7b..6dc616945dad5ef1 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -9,10 +9,12 @@
> >  
> >  #include <algorithm>
> >  #include <float.h>
> > +#include <fstream>
> >  #include <iomanip>
> >  #include <limits.h>
> >  #include <math.h>
> >  #include <regex>
> > +#include <sys/stat.h>
> >  
> >  #include <libcamera/property_ids.h>
> >  
> > @@ -204,6 +206,11 @@ int CameraSensor::init()
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	/* Generate a unique ID for the sensor. */
> > +	ret = generateID();
> > +	if (ret)
> > +		return ret;
> > +
> >  	/* Retrieve and store the camera sensor properties. */
> >  	const ControlInfoMap &controls = subdev_->controls();
> >  	int32_t propertyValue;
> > @@ -283,6 +290,26 @@ int CameraSensor::init()
> >   * \return The sensor model name
> >   */
> >  
> > +/**
> > + * \fn CameraSensor::id()
> > + * \brief Retrieve the sensor ID
> > + *
> > + * The sensor ID is a free-formed string that uniquely identifies the sensor in
> > + * the system. The ID is persistent between different instances of libcamera and
> > + * between resets of the system.
> > + *
> > + * For OF based systems the ID is the full path of the sensors in the device
> > + * tree description. For ACPI based systems the ID is the ACPI firmware nodes
> > + * path. Both ID sources are guaranteed to be unique and persistent as long as
> > + * the firmware of the system is not changed.
> > + *
> > + * A special case is needed to deal with the VIMC pipeline that implements a
> > + * virtual pipeline that is not backed by any hardware device and is therefore
> > + * not described in the device firmware.
> > + *
> > + * \return The sensor ID
> > + */
> > +
> >  /**
> >   * \fn CameraSensor::entity()
> >   * \brief Retrieve the sensor media entity
> > @@ -541,4 +568,71 @@ std::string CameraSensor::logPrefix() const
> >  	return "'" + entity_->name() + "'";
> >  }
> >  
> > +int CameraSensor::generateID()
> > +{
> > +	std::string path, devPath = subdev_->devicePath();
> 
> We usually split declarations on multiple lines.
> 
> > +	struct stat statbuf;
> > +
> > +	/* Try to generate ID from OF device tree path.  */
> > +	path = devPath + "/of_node";
> > +	if (stat(path.c_str(), &statbuf) == 0) {
> 
> You can use
> 
> 	if (!File::exists(path)) {
> 
> > +		char ofPath[PATH_MAX];
> > +
> > +		if (!realpath(path.c_str(), ofPath)) {
> 
> As mentioned in the review of 1/5, should we use realpath with nullptr
> as the second argument ? Or maybe just use readlink() with PATH_MAX ?
> 
> > +			LOG(CameraSensor, Error) << "Failed to read sensor OF based ID";
> 
> s/OF based/OF-based/
> 
> > +			return -EINVAL;
> > +		}
> > +
> > +		id_ = ofPath;
> > +		const std::string dropStr = "/sys/firmware/devicetree/";
> 
> static const ?
> 
> Same comments for the other options below.
> 
> > +		if (id_.find(dropStr) == 0)
> > +			id_.erase(0, dropStr.length());
> > +
> > +		return 0;
> > +	}
> > +
> > +	/* Try to generate ID from ACPI path. */
> > +	path = devPath + "/firmware_node/path";
> > +	if (stat(path.c_str(), &statbuf) == 0) {
> > +		std::ifstream file(path.c_str());
> > +
> > +		if (!file.is_open()) {
> > +			LOG(CameraSensor, Error) << "Failed to read sensor ACPI based ID";
> > +			return -EINVAL;
> > +		}
> > +
> > +		std::getline(file, id_);
> > +		file.close();
> > +
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * VIMC is a virtual video pipeline not backed hardware and has no OF
> > +	 * or ACPI firmware nodes. Handle this pipeline as a special case and
> > +	 * generate IDs based on the sensor model.
> > +	 */
> > +	path = devPath + "/modalias";
> > +	if (stat(path.c_str(), &statbuf) == 0) {
> > +		std::ifstream file(path.c_str());
> > +
> > +		if (!file.is_open()) {
> > +			LOG(CameraSensor, Error) << "Failed to read sensor ACPI based ID";
> > +			return -EINVAL;
> > +		}
> > +
> > +		std::string modalias;
> > +		std::getline(file, modalias);
> > +		file.close();
> > +
> > +		if (modalias == "platform:vimc") {
> > +			id_ = "VIMC " + model();
> > +			return 0;
> > +		}
> 
> Should we make this case more generic with the following (pseudo-code) ?
> 
> 	id_ = basename(readlink(devPath + "/device")) + "/" + model_;

I don't think so, at least not for now. The only valid use-case to reach 
this code is for the VIMC pipeline handler (and later perhaps VIVID). I 
would prefers keeping it strict for now and loosen it as we add 
pipelines we can test and make sure the id is stable.

> 
> > +	}
> > +
> > +	LOG(CameraSensor, Error) << "Unknown sensor device type, can not generate ID";
> > +	return -EINVAL;
> > +}
> > +
> >  } /* namespace libcamera */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart Aug. 2, 2020, 9:50 p.m. UTC | #3
Hi Niklas,

On Sun, Aug 02, 2020 at 11:46:37PM +0200, Niklas Söderlund wrote:
> On 2020-08-02 21:26:55 +0300, Laurent Pinchart wrote:
> > On Wed, Jul 29, 2020 at 01:50:52PM +0200, Niklas Söderlund wrote:
> > > Generate a constant and unique string ID for the sensor. The ID is
> > > generated from information from the firmware description of the camera
> > > image sensor. The ID is unique and persistent across reboots of the
> > > system.
> > > 
> > > For OF based systems the ID is the full path of the sensors in the
> > > device tree description. For ACPI based systems the ID is the ACPI
> > > firmware nodes path. Both ID sources are guaranteed to be unique and
> > > persistent as long as the firmware of the system is not changed.
> > > 
> > > A special case is needed to deal with the VIMC pipeline that implements
> > > a virtual pipeline that is not backed by any hardware device and is
> > > therefore not described in the device firmware.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > > * Changes since v4
> > > - Fix spelling.
> > > 
> > > * Changes since v3
> > > - Update commit message.
> > > - Add description of how ID are generated to comment.
> > > ---
> > >  include/libcamera/internal/camera_sensor.h |  4 +
> > >  src/libcamera/camera_sensor.cpp            | 94 ++++++++++++++++++++++
> > >  2 files changed, 98 insertions(+)
> > > 
> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > index 06c8292ca30129de..e0d2d9f63b47c2fe 100644
> > > --- a/include/libcamera/internal/camera_sensor.h
> > > +++ b/include/libcamera/internal/camera_sensor.h
> > > @@ -47,6 +47,7 @@ public:
> > >  	int init();
> > >  
> > >  	const std::string &model() const { return model_; }
> > > +	const std::string &id() const { return id_; }
> > >  	const MediaEntity *entity() const { return entity_; }
> > >  	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > >  	const std::vector<Size> &sizes() const { return sizes_; }
> > > @@ -67,11 +68,14 @@ protected:
> > >  	std::string logPrefix() const override;
> > >  
> > >  private:
> > > +	int generateID();
> > > +
> > >  	const MediaEntity *entity_;
> > >  	std::unique_ptr<V4L2Subdevice> subdev_;
> > >  	unsigned int pad_;
> > >  
> > >  	std::string model_;
> > > +	std::string id_;
> > >  
> > >  	V4L2Subdevice::Formats formats_;
> > >  	Size resolution_;
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > index 350f49accad99c7b..6dc616945dad5ef1 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -9,10 +9,12 @@
> > >  
> > >  #include <algorithm>
> > >  #include <float.h>
> > > +#include <fstream>
> > >  #include <iomanip>
> > >  #include <limits.h>
> > >  #include <math.h>
> > >  #include <regex>
> > > +#include <sys/stat.h>
> > >  
> > >  #include <libcamera/property_ids.h>
> > >  
> > > @@ -204,6 +206,11 @@ int CameraSensor::init()
> > >  	if (ret < 0)
> > >  		return ret;
> > >  
> > > +	/* Generate a unique ID for the sensor. */
> > > +	ret = generateID();
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	/* Retrieve and store the camera sensor properties. */
> > >  	const ControlInfoMap &controls = subdev_->controls();
> > >  	int32_t propertyValue;
> > > @@ -283,6 +290,26 @@ int CameraSensor::init()
> > >   * \return The sensor model name
> > >   */
> > >  
> > > +/**
> > > + * \fn CameraSensor::id()
> > > + * \brief Retrieve the sensor ID
> > > + *
> > > + * The sensor ID is a free-formed string that uniquely identifies the sensor in
> > > + * the system. The ID is persistent between different instances of libcamera and
> > > + * between resets of the system.
> > > + *
> > > + * For OF based systems the ID is the full path of the sensors in the device
> > > + * tree description. For ACPI based systems the ID is the ACPI firmware nodes
> > > + * path. Both ID sources are guaranteed to be unique and persistent as long as
> > > + * the firmware of the system is not changed.
> > > + *
> > > + * A special case is needed to deal with the VIMC pipeline that implements a
> > > + * virtual pipeline that is not backed by any hardware device and is therefore
> > > + * not described in the device firmware.
> > > + *
> > > + * \return The sensor ID
> > > + */
> > > +
> > >  /**
> > >   * \fn CameraSensor::entity()
> > >   * \brief Retrieve the sensor media entity
> > > @@ -541,4 +568,71 @@ std::string CameraSensor::logPrefix() const
> > >  	return "'" + entity_->name() + "'";
> > >  }
> > >  
> > > +int CameraSensor::generateID()
> > > +{
> > > +	std::string path, devPath = subdev_->devicePath();
> > 
> > We usually split declarations on multiple lines.
> > 
> > > +	struct stat statbuf;
> > > +
> > > +	/* Try to generate ID from OF device tree path.  */
> > > +	path = devPath + "/of_node";
> > > +	if (stat(path.c_str(), &statbuf) == 0) {
> > 
> > You can use
> > 
> > 	if (!File::exists(path)) {
> > 
> > > +		char ofPath[PATH_MAX];
> > > +
> > > +		if (!realpath(path.c_str(), ofPath)) {
> > 
> > As mentioned in the review of 1/5, should we use realpath with nullptr
> > as the second argument ? Or maybe just use readlink() with PATH_MAX ?
> > 
> > > +			LOG(CameraSensor, Error) << "Failed to read sensor OF based ID";
> > 
> > s/OF based/OF-based/
> > 
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		id_ = ofPath;
> > > +		const std::string dropStr = "/sys/firmware/devicetree/";
> > 
> > static const ?
> > 
> > Same comments for the other options below.
> > 
> > > +		if (id_.find(dropStr) == 0)
> > > +			id_.erase(0, dropStr.length());
> > > +
> > > +		return 0;
> > > +	}
> > > +
> > > +	/* Try to generate ID from ACPI path. */
> > > +	path = devPath + "/firmware_node/path";
> > > +	if (stat(path.c_str(), &statbuf) == 0) {
> > > +		std::ifstream file(path.c_str());
> > > +
> > > +		if (!file.is_open()) {
> > > +			LOG(CameraSensor, Error) << "Failed to read sensor ACPI based ID";
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		std::getline(file, id_);
> > > +		file.close();
> > > +
> > > +		return 0;
> > > +	}
> > > +
> > > +	/*
> > > +	 * VIMC is a virtual video pipeline not backed hardware and has no OF
> > > +	 * or ACPI firmware nodes. Handle this pipeline as a special case and
> > > +	 * generate IDs based on the sensor model.
> > > +	 */
> > > +	path = devPath + "/modalias";
> > > +	if (stat(path.c_str(), &statbuf) == 0) {
> > > +		std::ifstream file(path.c_str());
> > > +
> > > +		if (!file.is_open()) {
> > > +			LOG(CameraSensor, Error) << "Failed to read sensor ACPI based ID";

This shouldn't read "ACPI".

> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		std::string modalias;
> > > +		std::getline(file, modalias);
> > > +		file.close();
> > > +
> > > +		if (modalias == "platform:vimc") {
> > > +			id_ = "VIMC " + model();
> > > +			return 0;
> > > +		}
> > 
> > Should we make this case more generic with the following (pseudo-code) ?
> > 
> > 	id_ = basename(readlink(devPath + "/device")) + "/" + model_;
> 
> I don't think so, at least not for now. The only valid use-case to reach 
> this code is for the VIMC pipeline handler (and later perhaps VIVID). I 
> would prefers keeping it strict for now and loosen it as we add 
> pipelines we can test and make sure the id is stable.

We could keep the driver check, and already use the vimc.0 device name
instead of "VIMC" to prepare for the future.

And given that you will need to factor out the ACPI and OF parts as
they're useful for USB too, I wonder if the VIMC special case should be
kept in CameraSensor, or shouldn't move to the VIMC pipeline handler
instead. I'd prefer avoiding pipeline-handler-specific code in
CameraSensor if possible.

> > > +	}
> > > +
> > > +	LOG(CameraSensor, Error) << "Unknown sensor device type, can not generate ID";
> > > +	return -EINVAL;
> > > +}
> > > +
> > >  } /* namespace libcamera */

Patch

diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
index 06c8292ca30129de..e0d2d9f63b47c2fe 100644
--- a/include/libcamera/internal/camera_sensor.h
+++ b/include/libcamera/internal/camera_sensor.h
@@ -47,6 +47,7 @@  public:
 	int init();
 
 	const std::string &model() const { return model_; }
+	const std::string &id() const { return id_; }
 	const MediaEntity *entity() const { return entity_; }
 	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
 	const std::vector<Size> &sizes() const { return sizes_; }
@@ -67,11 +68,14 @@  protected:
 	std::string logPrefix() const override;
 
 private:
+	int generateID();
+
 	const MediaEntity *entity_;
 	std::unique_ptr<V4L2Subdevice> subdev_;
 	unsigned int pad_;
 
 	std::string model_;
+	std::string id_;
 
 	V4L2Subdevice::Formats formats_;
 	Size resolution_;
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
index 350f49accad99c7b..6dc616945dad5ef1 100644
--- a/src/libcamera/camera_sensor.cpp
+++ b/src/libcamera/camera_sensor.cpp
@@ -9,10 +9,12 @@ 
 
 #include <algorithm>
 #include <float.h>
+#include <fstream>
 #include <iomanip>
 #include <limits.h>
 #include <math.h>
 #include <regex>
+#include <sys/stat.h>
 
 #include <libcamera/property_ids.h>
 
@@ -204,6 +206,11 @@  int CameraSensor::init()
 	if (ret < 0)
 		return ret;
 
+	/* Generate a unique ID for the sensor. */
+	ret = generateID();
+	if (ret)
+		return ret;
+
 	/* Retrieve and store the camera sensor properties. */
 	const ControlInfoMap &controls = subdev_->controls();
 	int32_t propertyValue;
@@ -283,6 +290,26 @@  int CameraSensor::init()
  * \return The sensor model name
  */
 
+/**
+ * \fn CameraSensor::id()
+ * \brief Retrieve the sensor ID
+ *
+ * The sensor ID is a free-formed string that uniquely identifies the sensor in
+ * the system. The ID is persistent between different instances of libcamera and
+ * between resets of the system.
+ *
+ * For OF based systems the ID is the full path of the sensors in the device
+ * tree description. For ACPI based systems the ID is the ACPI firmware nodes
+ * path. Both ID sources are guaranteed to be unique and persistent as long as
+ * the firmware of the system is not changed.
+ *
+ * A special case is needed to deal with the VIMC pipeline that implements a
+ * virtual pipeline that is not backed by any hardware device and is therefore
+ * not described in the device firmware.
+ *
+ * \return The sensor ID
+ */
+
 /**
  * \fn CameraSensor::entity()
  * \brief Retrieve the sensor media entity
@@ -541,4 +568,71 @@  std::string CameraSensor::logPrefix() const
 	return "'" + entity_->name() + "'";
 }
 
+int CameraSensor::generateID()
+{
+	std::string path, devPath = subdev_->devicePath();
+	struct stat statbuf;
+
+	/* Try to generate ID from OF device tree path.  */
+	path = devPath + "/of_node";
+	if (stat(path.c_str(), &statbuf) == 0) {
+		char ofPath[PATH_MAX];
+
+		if (!realpath(path.c_str(), ofPath)) {
+			LOG(CameraSensor, Error) << "Failed to read sensor OF based ID";
+			return -EINVAL;
+		}
+
+		id_ = ofPath;
+		const std::string dropStr = "/sys/firmware/devicetree/";
+		if (id_.find(dropStr) == 0)
+			id_.erase(0, dropStr.length());
+
+		return 0;
+	}
+
+	/* Try to generate ID from ACPI path. */
+	path = devPath + "/firmware_node/path";
+	if (stat(path.c_str(), &statbuf) == 0) {
+		std::ifstream file(path.c_str());
+
+		if (!file.is_open()) {
+			LOG(CameraSensor, Error) << "Failed to read sensor ACPI based ID";
+			return -EINVAL;
+		}
+
+		std::getline(file, id_);
+		file.close();
+
+		return 0;
+	}
+
+	/*
+	 * VIMC is a virtual video pipeline not backed hardware and has no OF
+	 * or ACPI firmware nodes. Handle this pipeline as a special case and
+	 * generate IDs based on the sensor model.
+	 */
+	path = devPath + "/modalias";
+	if (stat(path.c_str(), &statbuf) == 0) {
+		std::ifstream file(path.c_str());
+
+		if (!file.is_open()) {
+			LOG(CameraSensor, Error) << "Failed to read sensor ACPI based ID";
+			return -EINVAL;
+		}
+
+		std::string modalias;
+		std::getline(file, modalias);
+		file.close();
+
+		if (modalias == "platform:vimc") {
+			id_ = "VIMC " + model();
+			return 0;
+		}
+	}
+
+	LOG(CameraSensor, Error) << "Unknown sensor device type, can not generate ID";
+	return -EINVAL;
+}
+
 } /* namespace libcamera */