[libcamera-devel,v7,2/5] libcamera: add model() for retriving model name in V4L2Subdevice

14990 diff mbox series

CameraSensor retrives model name from media entity. Move the heuristics
method into V4L2Subdevice, so CameraLens can reuse the function.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/internal/v4l2_subdevice.h |  4 ++
 src/libcamera/camera_sensor.cpp             | 32 +---------------
 src/libcamera/v4l2_subdevice.cpp            | 42 +++++++++++++++++++++
 3 files changed, 48 insertions(+), 30 deletions(-)

in $SUBJECT
 s/retriving/retrieving/

Quoting Han-Lin Chen (2021-12-02 14:03:14)
> CameraSensor retrives model name from media entity. Move the heuristics

s/retrives/retrieves/


Can be fixed while applying though.

--
Kieran


> method into V4L2Subdevice, so CameraLens can reuse the function.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_subdevice.h |  4 ++
>  src/libcamera/camera_sensor.cpp             | 32 +---------------
>  src/libcamera/v4l2_subdevice.cpp            | 42 +++++++++++++++++++++
>  3 files changed, 48 insertions(+), 30 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index 97b89fb9..763c4dec 100644
> --- a/include/libcamera/internal/v4l2_subdevice.h
> +++ b/include/libcamera/internal/v4l2_subdevice.h
> @@ -61,6 +61,8 @@ public:
>         int setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>                       Whence whence = ActiveFormat);
>  
> +       std::string model();
> +
>         static std::unique_ptr<V4L2Subdevice>
>         fromEntityName(const MediaDevice *media, const std::string &entity);
>  
> @@ -75,6 +77,8 @@ private:
>                                             unsigned int code);
>  
>         const MediaEntity *entity_;
> +
> +       std::string model_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 9fdb8c09..6151b32e 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -13,7 +13,6 @@
>  #include <iomanip>
>  #include <limits.h>
>  #include <math.h>
> -#include <regex>
>  #include <string.h>
>  
>  #include <libcamera/property_ids.h>
> @@ -347,34 +346,7 @@ void CameraSensor::initTestPatternModes(
>  
>  int CameraSensor::initProperties()
>  {
> -       /*
> -        * Extract the camera sensor model name from the media entity name.
> -        *
> -        * There is no standardized naming scheme for sensor entities in the
> -        * Linux kernel at the moment.
> -        *
> -        * - The most common rule, used by I2C sensors, associates the model
> -        *   name with the I2C bus number and address (e.g. 'imx219 0-0010').
> -        *
> -        * - When the sensor exposes multiple subdevs, the model name is
> -        *   usually followed by a function name, as in the smiapp driver (e.g.
> -        *   'jt8ew9 pixel_array 0-0010').
> -        *
> -        * - The vimc driver names its sensors 'Sensor A' and 'Sensor B'.
> -        *
> -        * Other schemes probably exist. As a best effort heuristic, use the
> -        * part of the entity name before the first space if the name contains
> -        * an I2C address, and use the full entity name otherwise.
> -        */
> -       std::string entityName = entity_->name();
> -       std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> -       std::smatch match;
> -
> -       if (std::regex_search(entityName, match, i2cRegex))
> -               model_ = entityName.substr(0, entityName.find(' '));
> -       else
> -               model_ = entityName;
> -
> +       model_ = subdev_->model();
>         properties_.set(properties::Model, utils::toAscii(model_));
>  
>         /* Generate a unique ID for the sensor. */
> @@ -832,7 +804,7 @@ int CameraSensor::generateId()
>         /*
>          * Virtual sensors not described in firmware
>          *
> -        * Verify it's a platform device and construct ID from the deive path
> +        * Verify it's a platform device and construct ID from the device path
>          * and model of sensor.
>          */
>         if (devPath.find("/sys/devices/platform/", 0) == 0) {
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 023e2328..17ec9fdf 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -9,6 +9,7 @@
>  
>  #include <fcntl.h>
>  #include <iomanip>
> +#include <regex>
>  #include <sstream>
>  #include <string.h>
>  #include <sys/ioctl.h>
> @@ -442,6 +443,47 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>         return 0;
>  }
>  
> +/**
> + * \brief Retrieve the model name
> + * \return The model name of the device
> + */
> +std::string V4L2Subdevice::model()
> +{
> +       if (!model_.empty())
> +               return model_;
> +
> +       /*
> +        * Extract model name from the media entity name.
> +        *
> +        * There is no standardized naming scheme for sensor entities in the
> +        * Linux kernel at the moment.
> +        *
> +        * - The most common rule, used by I2C sensors, associates the model
> +        *   name with the I2C bus number and address (e.g. 'imx219 0-0010').
> +        *
> +        * - When the sensor exposes multiple subdevs, the model name is
> +        *   usually followed by a function name, as in the smiapp driver (e.g.
> +        *   'jt8ew9 pixel_array 0-0010').
> +        *
> +        * - The vimc driver names its sensors 'Sensor A' and 'Sensor B'.
> +        *
> +        * Other schemes probably exist. As a best effort heuristic, use the
> +        * part of the entity name before the first space if the name contains
> +        * an I2C address, and use the full entity name otherwise.
> +        */
> +       std::string entityName = entity_->name();
> +       std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> +       std::smatch match;
> +
> +       std::string model;
> +       if (std::regex_search(entityName, match, i2cRegex))
> +               model_ = entityName.substr(0, entityName.find(' '));
> +       else
> +               model_ = entityName;
> +
> +       return model_;
> +}
> +
>  /**
>   * \brief Create a new video subdevice instance from \a entity in media device
>   * \a media
> -- 
> 2.34.1.400.ga245620fadb-goog
>

Hi Han-Lin,

Thank you for the patch.

On 12/2/21 7:33 PM, Han-Lin Chen wrote:
> CameraSensor retrives model name from media entity. Move the heuristics
> method into V4L2Subdevice, so CameraLens can reuse the function.
>
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>   include/libcamera/internal/v4l2_subdevice.h |  4 ++
>   src/libcamera/camera_sensor.cpp             | 32 +---------------
>   src/libcamera/v4l2_subdevice.cpp            | 42 +++++++++++++++++++++
>   3 files changed, 48 insertions(+), 30 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index 97b89fb9..763c4dec 100644
> --- a/include/libcamera/internal/v4l2_subdevice.h
> +++ b/include/libcamera/internal/v4l2_subdevice.h
> @@ -61,6 +61,8 @@ public:
>   	int setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>   		      Whence whence = ActiveFormat);
>   
> +	std::string model();


Should we const it?

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> +
>   	static std::unique_ptr<V4L2Subdevice>
>   	fromEntityName(const MediaDevice *media, const std::string &entity);
>   
> @@ -75,6 +77,8 @@ private:
>   					    unsigned int code);
>   
>   	const MediaEntity *entity_;
> +
> +	std::string model_;
>   };
>   
>   } /* namespace libcamera */
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 9fdb8c09..6151b32e 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -13,7 +13,6 @@
>   #include <iomanip>
>   #include <limits.h>
>   #include <math.h>
> -#include <regex>
>   #include <string.h>
>   
>   #include <libcamera/property_ids.h>
> @@ -347,34 +346,7 @@ void CameraSensor::initTestPatternModes(
>   
>   int CameraSensor::initProperties()
>   {
> -	/*
> -	 * Extract the camera sensor model name from the media entity name.
> -	 *
> -	 * There is no standardized naming scheme for sensor entities in the
> -	 * Linux kernel at the moment.
> -	 *
> -	 * - The most common rule, used by I2C sensors, associates the model
> -	 *   name with the I2C bus number and address (e.g. 'imx219 0-0010').
> -	 *
> -	 * - When the sensor exposes multiple subdevs, the model name is
> -	 *   usually followed by a function name, as in the smiapp driver (e.g.
> -	 *   'jt8ew9 pixel_array 0-0010').
> -	 *
> -	 * - The vimc driver names its sensors 'Sensor A' and 'Sensor B'.
> -	 *
> -	 * Other schemes probably exist. As a best effort heuristic, use the
> -	 * part of the entity name before the first space if the name contains
> -	 * an I2C address, and use the full entity name otherwise.
> -	 */
> -	std::string entityName = entity_->name();
> -	std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> -	std::smatch match;
> -
> -	if (std::regex_search(entityName, match, i2cRegex))
> -		model_ = entityName.substr(0, entityName.find(' '));
> -	else
> -		model_ = entityName;
> -
> +	model_ = subdev_->model();
>   	properties_.set(properties::Model, utils::toAscii(model_));
>   
>   	/* Generate a unique ID for the sensor. */
> @@ -832,7 +804,7 @@ int CameraSensor::generateId()
>   	/*
>   	 * Virtual sensors not described in firmware
>   	 *
> -	 * Verify it's a platform device and construct ID from the deive path
> +	 * Verify it's a platform device and construct ID from the device path
>   	 * and model of sensor.
>   	 */
>   	if (devPath.find("/sys/devices/platform/", 0) == 0) {
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 023e2328..17ec9fdf 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -9,6 +9,7 @@
>   
>   #include <fcntl.h>
>   #include <iomanip>
> +#include <regex>
>   #include <sstream>
>   #include <string.h>
>   #include <sys/ioctl.h>
> @@ -442,6 +443,47 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>   	return 0;
>   }
>   
> +/**
> + * \brief Retrieve the model name
> + * \return The model name of the device
> + */
> +std::string V4L2Subdevice::model()
> +{
> +	if (!model_.empty())
> +		return model_;
> +
> +	/*
> +	 * Extract model name from the media entity name.
> +	 *
> +	 * There is no standardized naming scheme for sensor entities in the
> +	 * Linux kernel at the moment.
> +	 *
> +	 * - The most common rule, used by I2C sensors, associates the model
> +	 *   name with the I2C bus number and address (e.g. 'imx219 0-0010').
> +	 *
> +	 * - When the sensor exposes multiple subdevs, the model name is
> +	 *   usually followed by a function name, as in the smiapp driver (e.g.
> +	 *   'jt8ew9 pixel_array 0-0010').
> +	 *
> +	 * - The vimc driver names its sensors 'Sensor A' and 'Sensor B'.
> +	 *
> +	 * Other schemes probably exist. As a best effort heuristic, use the
> +	 * part of the entity name before the first space if the name contains
> +	 * an I2C address, and use the full entity name otherwise.
> +	 */
> +	std::string entityName = entity_->name();
> +	std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> +	std::smatch match;
> +
> +	std::string model;
> +	if (std::regex_search(entityName, match, i2cRegex))
> +		model_ = entityName.substr(0, entityName.find(' '));
> +	else
> +		model_ = entityName;
> +
> +	return model_;
> +}
> +
>   /**
>    * \brief Create a new video subdevice instance from \a entity in media device
>    * \a media

Hi Han-lin,

Thank you for the patch.

On Thu, Dec 02, 2021 at 10:03:14PM +0800, Han-Lin Chen wrote:
> CameraSensor retrives model name from media entity. Move the heuristics
> method into V4L2Subdevice, so CameraLens can reuse the function.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_subdevice.h |  4 ++
>  src/libcamera/camera_sensor.cpp             | 32 +---------------
>  src/libcamera/v4l2_subdevice.cpp            | 42 +++++++++++++++++++++
>  3 files changed, 48 insertions(+), 30 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index 97b89fb9..763c4dec 100644
> --- a/include/libcamera/internal/v4l2_subdevice.h
> +++ b/include/libcamera/internal/v4l2_subdevice.h
> @@ -61,6 +61,8 @@ public:
>  	int setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>  		      Whence whence = ActiveFormat);
>  
> +	std::string model();

Let's make it

	const std::string &model();

to avoid copies.

> +
>  	static std::unique_ptr<V4L2Subdevice>
>  	fromEntityName(const MediaDevice *media, const std::string &entity);
>  
> @@ -75,6 +77,8 @@ private:
>  					    unsigned int code);
>  
>  	const MediaEntity *entity_;
> +
> +	std::string model_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 9fdb8c09..6151b32e 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -13,7 +13,6 @@
>  #include <iomanip>
>  #include <limits.h>
>  #include <math.h>
> -#include <regex>
>  #include <string.h>
>  
>  #include <libcamera/property_ids.h>
> @@ -347,34 +346,7 @@ void CameraSensor::initTestPatternModes(
>  
>  int CameraSensor::initProperties()
>  {
> -	/*
> -	 * Extract the camera sensor model name from the media entity name.
> -	 *
> -	 * There is no standardized naming scheme for sensor entities in the
> -	 * Linux kernel at the moment.
> -	 *
> -	 * - The most common rule, used by I2C sensors, associates the model
> -	 *   name with the I2C bus number and address (e.g. 'imx219 0-0010').
> -	 *
> -	 * - When the sensor exposes multiple subdevs, the model name is
> -	 *   usually followed by a function name, as in the smiapp driver (e.g.
> -	 *   'jt8ew9 pixel_array 0-0010').
> -	 *
> -	 * - The vimc driver names its sensors 'Sensor A' and 'Sensor B'.
> -	 *
> -	 * Other schemes probably exist. As a best effort heuristic, use the
> -	 * part of the entity name before the first space if the name contains
> -	 * an I2C address, and use the full entity name otherwise.
> -	 */
> -	std::string entityName = entity_->name();
> -	std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> -	std::smatch match;
> -
> -	if (std::regex_search(entityName, match, i2cRegex))
> -		model_ = entityName.substr(0, entityName.find(' '));
> -	else
> -		model_ = entityName;
> -
> +	model_ = subdev_->model();
>  	properties_.set(properties::Model, utils::toAscii(model_));
>  
>  	/* Generate a unique ID for the sensor. */
> @@ -832,7 +804,7 @@ int CameraSensor::generateId()
>  	/*
>  	 * Virtual sensors not described in firmware
>  	 *
> -	 * Verify it's a platform device and construct ID from the deive path
> +	 * Verify it's a platform device and construct ID from the device path
>  	 * and model of sensor.
>  	 */
>  	if (devPath.find("/sys/devices/platform/", 0) == 0) {
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 023e2328..17ec9fdf 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -9,6 +9,7 @@
>  
>  #include <fcntl.h>
>  #include <iomanip>
> +#include <regex>
>  #include <sstream>
>  #include <string.h>
>  #include <sys/ioctl.h>
> @@ -442,6 +443,47 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>  	return 0;
>  }
>  
> +/**
> + * \brief Retrieve the model name
> + * \return The model name of the device

As there's no concept of model name in the V4L2 subdev API, I think this
would benefit for some more information.

/**
 * \brief Retrieve the model name of the device
 *
 * The model name allows identification of the specific device model. This can
 * be used to infer device characteristics, for instance to determine the
 * analogue gain model of a camera sensor based on the sensor model name.
 *
 * Neither the V4L2 API nor the Media Controller API expose an explicit model
 * name. This function implements a heuristics to extract the model name from
 * the subdevice's entity name. This should produce accurate results for
 * I2C-based devices. If the heuristics can't match a known naming pattern,
 * the function returns the full entity name.
 *
 * \return The model name of the device
 */

> + */
> +std::string V4L2Subdevice::model()
> +{
> +	if (!model_.empty())
> +		return model_;
> +
> +	/*
> +	 * Extract model name from the media entity name.
> +	 *
> +	 * There is no standardized naming scheme for sensor entities in the
> +	 * Linux kernel at the moment.

It's not just sensors anymore, s/sensor/sensor or other/

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	 *
> +	 * - The most common rule, used by I2C sensors, associates the model
> +	 *   name with the I2C bus number and address (e.g. 'imx219 0-0010').
> +	 *
> +	 * - When the sensor exposes multiple subdevs, the model name is
> +	 *   usually followed by a function name, as in the smiapp driver (e.g.
> +	 *   'jt8ew9 pixel_array 0-0010').
> +	 *
> +	 * - The vimc driver names its sensors 'Sensor A' and 'Sensor B'.
> +	 *
> +	 * Other schemes probably exist. As a best effort heuristic, use the
> +	 * part of the entity name before the first space if the name contains
> +	 * an I2C address, and use the full entity name otherwise.
> +	 */
> +	std::string entityName = entity_->name();
> +	std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> +	std::smatch match;
> +
> +	std::string model;
> +	if (std::regex_search(entityName, match, i2cRegex))
> +		model_ = entityName.substr(0, entityName.find(' '));
> +	else
> +		model_ = entityName;
> +
> +	return model_;
> +}
> +
>  /**
>   * \brief Create a new video subdevice instance from \a entity in media device
>   * \a media