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

[Niklas Söderlund]

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 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_;

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

-- 
Regards,
Niklas Söderlund