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

[Laurent Pinchart]

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 at ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Kieran Bingham <kieran.bingham at 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 */

-- 
Regards,

Laurent Pinchart