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

[Niklas Söderlund]

Hi Kieran,

Thanks for your comments.

On 2020-07-29 11:23:32 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 29/07/2020 10:21, 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>
> > ---
> > * 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..a12d196ad898bb92 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.
> 
> If my comment about generic catching of virtual hardware is worthy of
> this series, this can be "A special case is needed to deal with virtual
> pipelines..." or such.
> 
> But that could also be an 'on top' patch anyway.
> 
> > + *
> > + * \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 have no OF
> 
> s/have/has/
> 
> > +	 * or ACPI firmware nodes. Handle this pipeline as a special case and
> > +	 * generate IDs based on the sensor model.
> > +	 */
> 
> 
> How about instead of making this specific to VIMC, make it the generic
> 'last chance/best-effort' fallthrough ?

No, the idea is that we really should only allow known good devices. So 
I'm afraid a special case for the out-of-tree vivid pipeline is need. I 
think it would be rather simple tho, see below.

> 
> > +	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();
> 
> This could then just be:
> 	id_ = modalias + model()
> 
> + anything else desired to keep things more likely to be unique...

I picked this string for VIMC as it is the string used today and we can 
guarantee it's unique. I would not object to chancing this to your 
suggestion if that is the group suggestion.

I would however not like to have a generic fall-thru here. But adding 
support for more virtual hardware should be simple right?


    else if (modalias == "platfrom:vivid")
        id_ = "VIVID " ....;

> 
> 
> > +			return 0;
> > +		}
> > +	}
> 
> I'm being a little selfish of course, but it's because the 'out of tree'
> vivid pipeline handler would not get handled here, and I had
> hoped/intended that it could remain out of tree as an instructional
> series to support new developers.

Selfish is good, we all scratch out itches right :-) I feel your pain of 
maintaining an out-of-tree pipeline handler, I'm sorry this change 
effects that.

> 
> This would 'break' vivid otherwise.
> 
> Of course this could potentially also be on top if you don't want to
> support it in this series.
> 
> Let me know in that case, as I think I'll then have to do it next time I
> rebase the vivid series.

I don't intend to make this a generic fall-true unless I'm seriously 
pushed in that direction from the group :-)

> 
> therefore, with/without making this generic (and the small grammatical
> error fixed above)
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Thanks!

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

-- 
Regards,
Niklas Söderlund