[v2,2/5] media: v4l: ctrls: Add a control for temperature

  Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
celsius as a volatile and read-only control, and its documentation.
Useful to monitor thermals from v4l controls for sensors that support
this.

Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
 Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 3 +++
 drivers/media/v4l2-core/v4l2-ctrls-defs.c                  | 4 ++++
 include/uapi/linux/v4l2-controls.h                         | 2 ++
 3 files changed, 9 insertions(+)
  

Hi Benjamin,

Thank you for the patch.

On Fri, Apr 15, 2022 at 01:18:42PM +0200, Benjamin Mugnier wrote:
> Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
> celsius as a volatile and read-only control, and its documentation.
> Useful to monitor thermals from v4l controls for sensors that support
> this.
> 
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 3 +++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c                  | 4 ++++
>  include/uapi/linux/v4l2-controls.h                         | 2 ++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index 4c5061aa9cd4..26fa21f5c45a 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -661,3 +661,6 @@ enum v4l2_scene_mode -
>  .. [#f1]
>     This control may be changed to a menu control in the future, if more
>     options are required.
> +
> +``V4L2_CID_TEMPERATURE (integer)``
> +    The temperature of the sensor in celsius. This is a read-only control.

I've seen sensors where the temperature sensor has a 1/10th degree
precision. Should we standardize on that ? Anything more precise is
likely overkill.

There are also sensors with multiple temperature sensors. If there are
too many of them I suppose the temperature would be reported in embedded
data, but perhaps not always. How can we prepare for this ?

There are also a few details that I think should be documented. Is the
temperature always read on-demand when reading the control, or updated
periodically ? I would assume most drivers would implement the former,
which means no control notification events will be generated. This
should be documented. Furthermore, do drivers need to support reading
the temperature when the sensor isn't streaming ? If not, when should a
control read ioctl return, the last value, or an error ?

> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 54ca4e6b820b..45ad3edd59e0 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1042,6 +1042,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_UNIT_CELL_SIZE:		return "Unit Cell Size";
>  	case V4L2_CID_CAMERA_ORIENTATION:	return "Camera Orientation";
>  	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
> +	case V4L2_CID_TEMPERATURE:		return "Temperature in °C";
>  
>  	/* FM Radio Modulator controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1597,6 +1598,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_RF_TUNER_PLL_LOCK:
>  		*flags |= V4L2_CTRL_FLAG_VOLATILE;
>  		break;
> +	case V4L2_CID_TEMPERATURE:
> +		*flags |= V4L2_CTRL_FLAG_READ_ONLY |
> +			  V4L2_CTRL_FLAG_VOLATILE;
>  	}
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_fill);
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index bb40129446d4..705b4043c2de 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1008,6 +1008,8 @@ enum v4l2_auto_focus_range {
>  
>  #define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
>  
> +#define V4L2_CID_TEMPERATURE			(V4L2_CID_CAMERA_CLASS_BASE+36)
> +
>  /* FM Modulator class control IDs */
>  
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
  

On 15/04/2022 13:18, Benjamin Mugnier wrote:
> Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
> celsius as a volatile and read-only control, and its documentation.

celsius -> degrees Celsius

(see https://en.wikipedia.org/wiki/Celsius)

> Useful to monitor thermals from v4l controls for sensors that support
> this.
> 
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 3 +++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c                  | 4 ++++
>  include/uapi/linux/v4l2-controls.h                         | 2 ++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index 4c5061aa9cd4..26fa21f5c45a 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -661,3 +661,6 @@ enum v4l2_scene_mode -
>  .. [#f1]
>     This control may be changed to a menu control in the future, if more
>     options are required.
> +
> +``V4L2_CID_TEMPERATURE (integer)``
> +    The temperature of the sensor in celsius. This is a read-only control.

Ditto

> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 54ca4e6b820b..45ad3edd59e0 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1042,6 +1042,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_UNIT_CELL_SIZE:		return "Unit Cell Size";
>  	case V4L2_CID_CAMERA_ORIENTATION:	return "Camera Orientation";
>  	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
> +	case V4L2_CID_TEMPERATURE:		return "Temperature in °C";

I am not sure how well this ° symbol will work. The V4L2 spec says that this is an
ASCII string, so that doesn't allow for this symbol.

I would just call it "Temperature".

>  
>  	/* FM Radio Modulator controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1597,6 +1598,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_RF_TUNER_PLL_LOCK:
>  		*flags |= V4L2_CTRL_FLAG_VOLATILE;
>  		break;
> +	case V4L2_CID_TEMPERATURE:
> +		*flags |= V4L2_CTRL_FLAG_READ_ONLY |
> +			  V4L2_CTRL_FLAG_VOLATILE;

Add a break!

>  	}
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_fill);
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index bb40129446d4..705b4043c2de 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1008,6 +1008,8 @@ enum v4l2_auto_focus_range {
>  
>  #define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
>  
> +#define V4L2_CID_TEMPERATURE			(V4L2_CID_CAMERA_CLASS_BASE+36)

Does it make sense to add this to CAMERA_CLASS? Can't this be a generic temperature
control? (i.e. in USER_BASE) Any device can have a temperature sensor.

I also think that making this an array control would make sense as well in case there
are multiple temperature sensors.

Brainstorming some more: does this even belong here? Isn't this more a hwmon thing?
E.g. compare this to drivers/nvme/host/hwmon.c.

A hwmon implementation seems to be a more natural mechanism.

Regards,

	Hans

> +
>  /* FM Modulator class control IDs */
>  
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
  

Hi,

adding linux-hwmon in CC for a wider feedback.

Le vendredi 15 avril 2022 à 13:18 +0200, Benjamin Mugnier a écrit :
> Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
> celsius as a volatile and read-only control, and its documentation.
> Useful to monitor thermals from v4l controls for sensors that support
> this.

Any justification to expose a temperature sensor outside of the dedicated kernel
API hwmon ? I know if may makes it harder to use, as the sensor isn't bound to
the camera driver, and also the sensor may not work if the camera is not
streaming. But in the end, the API in hwmon does not look that complex, and is
perhaps more precise ?

All in all, I think we need a strong justification to implement a custom
thermometer interface, something described here and documented with care to
prevent abuse. I would also see opinion from folks outside of the linux-media,
hence why I have CCed hwmon mailing list.

regards,
Nicolas

> 
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 3 +++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c                  | 4 ++++
>  include/uapi/linux/v4l2-controls.h                         | 2 ++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index 4c5061aa9cd4..26fa21f5c45a 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -661,3 +661,6 @@ enum v4l2_scene_mode -
>  .. [#f1]
>     This control may be changed to a menu control in the future, if more
>     options are required.
> +
> +``V4L2_CID_TEMPERATURE (integer)``
> +    The temperature of the sensor in celsius. This is a read-only control.
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 54ca4e6b820b..45ad3edd59e0 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1042,6 +1042,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_UNIT_CELL_SIZE:		return "Unit Cell Size";
>  	case V4L2_CID_CAMERA_ORIENTATION:	return "Camera Orientation";
>  	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
> +	case V4L2_CID_TEMPERATURE:		return "Temperature in °C";
>  
>  	/* FM Radio Modulator controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1597,6 +1598,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_RF_TUNER_PLL_LOCK:
>  		*flags |= V4L2_CTRL_FLAG_VOLATILE;
>  		break;
> +	case V4L2_CID_TEMPERATURE:
> +		*flags |= V4L2_CTRL_FLAG_READ_ONLY |
> +			  V4L2_CTRL_FLAG_VOLATILE;
>  	}
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_fill);
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index bb40129446d4..705b4043c2de 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1008,6 +1008,8 @@ enum v4l2_auto_focus_range {
>  
>  #define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
>  
> +#define V4L2_CID_TEMPERATURE			(V4L2_CID_CAMERA_CLASS_BASE+36)
> +
>  /* FM Modulator class control IDs */
>  
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
  

On 4/19/22 11:24, Nicolas Dufresne wrote:
> Hi,
> 
> adding linux-hwmon in CC for a wider feedback.
> 
> Le vendredi 15 avril 2022 à 13:18 +0200, Benjamin Mugnier a écrit :
>> Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
>> celsius as a volatile and read-only control, and its documentation.
>> Useful to monitor thermals from v4l controls for sensors that support
>> this.
> 
> Any justification to expose a temperature sensor outside of the dedicated kernel
> API hwmon ? I know if may makes it harder to use, as the sensor isn't bound to
> the camera driver, and also the sensor may not work if the camera is not
> streaming. But in the end, the API in hwmon does not look that complex, and is
> perhaps more precise ?
> 
> All in all, I think we need a strong justification to implement a custom
> thermometer interface, something described here and documented with care to
> prevent abuse. I would also see opinion from folks outside of the linux-media,
> hence why I have CCed hwmon mailing list.
> 


All I can say is that this seems to be odd and a bit outside the scope of
v4l2. I would have expected the vgxy61 driver to register a hwmon device
instead.

Guenter
  

On Tue, Apr 19, 2022 at 12:28:06PM -0700, Guenter Roeck wrote:
> On 4/19/22 11:24, Nicolas Dufresne wrote:
> > Hi,
> > 
> > adding linux-hwmon in CC for a wider feedback.
> > 
> > Le vendredi 15 avril 2022 à 13:18 +0200, Benjamin Mugnier a écrit :
> >> Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
> >> celsius as a volatile and read-only control, and its documentation.
> >> Useful to monitor thermals from v4l controls for sensors that support
> >> this.
> > 
> > Any justification to expose a temperature sensor outside of the dedicated kernel
> > API hwmon ? I know if may makes it harder to use, as the sensor isn't bound to
> > the camera driver, and also the sensor may not work if the camera is not
> > streaming. But in the end, the API in hwmon does not look that complex, and is
> > perhaps more precise ?
> > 
> > All in all, I think we need a strong justification to implement a custom
> > thermometer interface, something described here and documented with care to
> > prevent abuse. I would also see opinion from folks outside of the linux-media,
> > hence why I have CCed hwmon mailing list.
> 
> All I can say is that this seems to be odd and a bit outside the scope of
> v4l2. I would have expected the vgxy61 driver to register a hwmon device
> instead.

I don't have a definitive opinion yet, but as Nicolas raised the issue
by pushing towards hwmon, I'll offer counter-arguments for the sake of
it :-)

The temperature sensor in the imaging sensor is used to measure the die
temperature, in order to tune the noise and spectral response model of
the imaging sensor. It's thus not a generic-purpose temperature sensor.
If the feature were to be exposed through hwmon, userspace would need to
associate an hwmon device to the imaging sensor V4L2 subdev (we have a
way to do so through the MC API, it doesn't support hwmon at this point,
but I suppose it could be added). There are also various constraints
that tie the temperature reading to the imaging side, it could be that
the temperature would only be readable while capturing frames. That's
probably possible to handle too but returning an error from the
temperature read.

Code-wise, both the driver and userspace would be more complex, for very
little practical gain (I don't dispute a theorical gain).
  

On 4/19/22 14:01, Laurent Pinchart wrote:
> On Tue, Apr 19, 2022 at 12:28:06PM -0700, Guenter Roeck wrote:
>> On 4/19/22 11:24, Nicolas Dufresne wrote:
>>> Hi,
>>>
>>> adding linux-hwmon in CC for a wider feedback.
>>>
>>> Le vendredi 15 avril 2022 à 13:18 +0200, Benjamin Mugnier a écrit :
>>>> Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
>>>> celsius as a volatile and read-only control, and its documentation.
>>>> Useful to monitor thermals from v4l controls for sensors that support
>>>> this.
>>>
>>> Any justification to expose a temperature sensor outside of the dedicated kernel
>>> API hwmon ? I know if may makes it harder to use, as the sensor isn't bound to
>>> the camera driver, and also the sensor may not work if the camera is not
>>> streaming. But in the end, the API in hwmon does not look that complex, and is
>>> perhaps more precise ?
>>>
>>> All in all, I think we need a strong justification to implement a custom
>>> thermometer interface, something described here and documented with care to
>>> prevent abuse. I would also see opinion from folks outside of the linux-media,
>>> hence why I have CCed hwmon mailing list.
>>
>> All I can say is that this seems to be odd and a bit outside the scope of
>> v4l2. I would have expected the vgxy61 driver to register a hwmon device
>> instead.
> 
> I don't have a definitive opinion yet, but as Nicolas raised the issue
> by pushing towards hwmon, I'll offer counter-arguments for the sake of
> it :-)
> 
> The temperature sensor in the imaging sensor is used to measure the die
> temperature, in order to tune the noise and spectral response model of
> the imaging sensor. It's thus not a generic-purpose temperature sensor.
> If the feature were to be exposed through hwmon, userspace would need to
> associate an hwmon device to the imaging sensor V4L2 subdev (we have a
> way to do so through the MC API, it doesn't support hwmon at this point,
> but I suppose it could be added). There are also various constraints
> that tie the temperature reading to the imaging side, it could be that
> the temperature would only be readable while capturing frames. That's
> probably possible to handle too but returning an error from the
> temperature read.
> 
> Code-wise, both the driver and userspace would be more complex, for very
> little practical gain (I don't dispute a theorical gain).
> 

All I can say is - not my subsystem, not my call to make. If you say this
is special and is better handled in V4L2, I'll take you by your word.

Guenter
  

Hi Hans,

Thank you for your review.

On 19/04/2022 09:03, Hans Verkuil wrote:
> On 15/04/2022 13:18, Benjamin Mugnier wrote:
>> Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
>> celsius as a volatile and read-only control, and its documentation.
> 
> celsius -> degrees Celsius
> 
> (see https://en.wikipedia.org/wiki/Celsius)
> 

Yes, thank you.

>> Useful to monitor thermals from v4l controls for sensors that support
>> this.
>>
>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>> ---
>>  Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 3 +++
>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c                  | 4 ++++
>>  include/uapi/linux/v4l2-controls.h                         | 2 ++
>>  3 files changed, 9 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> index 4c5061aa9cd4..26fa21f5c45a 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> @@ -661,3 +661,6 @@ enum v4l2_scene_mode -
>>  .. [#f1]
>>     This control may be changed to a menu control in the future, if more
>>     options are required.
>> +
>> +``V4L2_CID_TEMPERATURE (integer)``
>> +    The temperature of the sensor in celsius. This is a read-only control.
> 
> Ditto
> 
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> index 54ca4e6b820b..45ad3edd59e0 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> @@ -1042,6 +1042,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_UNIT_CELL_SIZE:		return "Unit Cell Size";
>>  	case V4L2_CID_CAMERA_ORIENTATION:	return "Camera Orientation";
>>  	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
>> +	case V4L2_CID_TEMPERATURE:		return "Temperature in °C";
> 
> I am not sure how well this ° symbol will work. The V4L2 spec says that this is an
> ASCII string, so that doesn't allow for this symbol.
> 
> I would just call it "Temperature".
> 

From what I see in v4l2-ctl it translate it to "temperature_in_c".
If we agree this is implicitly in in degrees celsius then let's remove it.

>>  
>>  	/* FM Radio Modulator controls */
>>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>> @@ -1597,6 +1598,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>  	case V4L2_CID_RF_TUNER_PLL_LOCK:
>>  		*flags |= V4L2_CTRL_FLAG_VOLATILE;
>>  		break;
>> +	case V4L2_CID_TEMPERATURE:
>> +		*flags |= V4L2_CTRL_FLAG_READ_ONLY |
>> +			  V4L2_CTRL_FLAG_VOLATILE;
> 
> Add a break!
> 

Whoops!

>>  	}
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_fill);
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index bb40129446d4..705b4043c2de 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -1008,6 +1008,8 @@ enum v4l2_auto_focus_range {
>>  
>>  #define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
>>  
>> +#define V4L2_CID_TEMPERATURE			(V4L2_CID_CAMERA_CLASS_BASE+36)
> 
> Does it make sense to add this to CAMERA_CLASS? Can't this be a generic temperature
> control? (i.e. in USER_BASE) Any device can have a temperature sensor.
> 

I see no issue in moving it. Your call.

> I also think that making this an array control would make sense as well in case there
> are multiple temperature sensors.
> 
> Brainstorming some more: does this even belong here? Isn't this more a hwmon thing?
> E.g. compare this to drivers/nvme/host/hwmon.c.
> 
> A hwmon implementation seems to be a more natural mechanism.
> 
> Regards,
> 
> 	Hans
> 
>> +
>>  /* FM Modulator class control IDs */
>>  
>>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
>
  

Hi Laurent,

Thank you for your review.

On 15/04/2022 16:37, Laurent Pinchart wrote:
> Hi Benjamin,
> 
> Thank you for the patch.
> 
> On Fri, Apr 15, 2022 at 01:18:42PM +0200, Benjamin Mugnier wrote:
>> Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
>> celsius as a volatile and read-only control, and its documentation.
>> Useful to monitor thermals from v4l controls for sensors that support
>> this.
>>
>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>> ---
>>  Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 3 +++
>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c                  | 4 ++++
>>  include/uapi/linux/v4l2-controls.h                         | 2 ++
>>  3 files changed, 9 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> index 4c5061aa9cd4..26fa21f5c45a 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> @@ -661,3 +661,6 @@ enum v4l2_scene_mode -
>>  .. [#f1]
>>     This control may be changed to a menu control in the future, if more
>>     options are required.
>> +
>> +``V4L2_CID_TEMPERATURE (integer)``
>> +    The temperature of the sensor in celsius. This is a read-only control.
> 
> I've seen sensors where the temperature sensor has a 1/10th degree
> precision. Should we standardize on that ? Anything more precise is
> likely overkill.
> 

This sensor also has a 1/10th degree precision, here I only display the integer part. Ok to standardize on that.

> There are also sensors with multiple temperature sensors. If there are
> too many of them I suppose the temperature would be reported in embedded
> data, but perhaps not always. How can we prepare for this ?
> 
> There are also a few details that I think should be documented. Is the
> temperature always read on-demand when reading the control, or updated
> periodically ? I would assume most drivers would implement the former,
> which means no control notification events will be generated. This
> should be documented. Furthermore, do drivers need to support reading
> the temperature when the sensor isn't streaming ? If not, when should a
> control read ioctl return, the last value, or an error ?
> 

From what I see read on demand seems sufficient.
This sensor supports reading the temperature read even if not streaming. I don't know for other sensors.
See my comments on driver implementation on 5/5 for more.

>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> index 54ca4e6b820b..45ad3edd59e0 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> @@ -1042,6 +1042,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_UNIT_CELL_SIZE:		return "Unit Cell Size";
>>  	case V4L2_CID_CAMERA_ORIENTATION:	return "Camera Orientation";
>>  	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
>> +	case V4L2_CID_TEMPERATURE:		return "Temperature in °C";
>>  
>>  	/* FM Radio Modulator controls */
>>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>> @@ -1597,6 +1598,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>  	case V4L2_CID_RF_TUNER_PLL_LOCK:
>>  		*flags |= V4L2_CTRL_FLAG_VOLATILE;
>>  		break;
>> +	case V4L2_CID_TEMPERATURE:
>> +		*flags |= V4L2_CTRL_FLAG_READ_ONLY |
>> +			  V4L2_CTRL_FLAG_VOLATILE;
>>  	}
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_fill);
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index bb40129446d4..705b4043c2de 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -1008,6 +1008,8 @@ enum v4l2_auto_focus_range {
>>  
>>  #define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
>>  
>> +#define V4L2_CID_TEMPERATURE			(V4L2_CID_CAMERA_CLASS_BASE+36)
>> +
>>  /* FM Modulator class control IDs */
>>  
>>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
>
  

Hi everyone,

Just some little comments for my use case. Hope that helps.

On 20/04/2022 00:04, Guenter Roeck wrote:
> On 4/19/22 14:01, Laurent Pinchart wrote:
>> On Tue, Apr 19, 2022 at 12:28:06PM -0700, Guenter Roeck wrote:
>>> On 4/19/22 11:24, Nicolas Dufresne wrote:
>>>> Hi,
>>>>
>>>> adding linux-hwmon in CC for a wider feedback.
>>>>
>>>> Le vendredi 15 avril 2022 à 13:18 +0200, Benjamin Mugnier a écrit :
>>>>> Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
>>>>> celsius as a volatile and read-only control, and its documentation.
>>>>> Useful to monitor thermals from v4l controls for sensors that support
>>>>> this.
>>>>
>>>> Any justification to expose a temperature sensor outside of the dedicated kernel
>>>> API hwmon ? I know if may makes it harder to use, as the sensor isn't bound to
>>>> the camera driver, and also the sensor may not work if the camera is not
>>>> streaming. But in the end, the API in hwmon does not look that complex, and is
>>>> perhaps more precise ?

This sensor is able to read the temperature even if not streaming.

>>>>
>>>> All in all, I think we need a strong justification to implement a custom
>>>> thermometer interface, something described here and documented with care to
>>>> prevent abuse. I would also see opinion from folks outside of the linux-media,
>>>> hence why I have CCed hwmon mailing list.
>>>
>>> All I can say is that this seems to be odd and a bit outside the scope of
>>> v4l2. I would have expected the vgxy61 driver to register a hwmon device
>>> instead.
>>
>> I don't have a definitive opinion yet, but as Nicolas raised the issue
>> by pushing towards hwmon, I'll offer counter-arguments for the sake of
>> it :-)
>>
>> The temperature sensor in the imaging sensor is used to measure the die
>> temperature, in order to tune the noise and spectral response model of
>> the imaging sensor. It's thus not a generic-purpose temperature sensor.
>> If the feature were to be exposed through hwmon, userspace would need to
>> associate an hwmon device to the imaging sensor V4L2 subdev (we have a
>> way to do so through the MC API, it doesn't support hwmon at this point,
>> but I suppose it could be added). There are also various constraints
>> that tie the temperature reading to the imaging side, it could be that
>> the temperature would only be readable while capturing frames. That's
>> probably possible to handle too but returning an error from the
>> temperature read.
>>
>> Code-wise, both the driver and userspace would be more complex, for very
>> little practical gain (I don't dispute a theorical gain).
>>
> 
> All I can say is - not my subsystem, not my call to make. If you say this
> is special and is better handled in V4L2, I'll take you by your word.
> 
> Guenter
> 

I'll happily implement whatever conclusion we make here.

I could also drop this control for the first iteration of the driver, and come back later once a consensus is reached.

Benjamin
  

Hi Benjamin,

On Wed, Apr 20, 2022 at 03:01:18PM +0200, Benjamin Mugnier wrote:
> On 20/04/2022 00:04, Guenter Roeck wrote:
> > On 4/19/22 14:01, Laurent Pinchart wrote:
> >> On Tue, Apr 19, 2022 at 12:28:06PM -0700, Guenter Roeck wrote:
> >>> On 4/19/22 11:24, Nicolas Dufresne wrote:
> >>>> Hi,
> >>>>
> >>>> adding linux-hwmon in CC for a wider feedback.
> >>>>
> >>>> Le vendredi 15 avril 2022 à 13:18 +0200, Benjamin Mugnier a écrit :
> >>>>> Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
> >>>>> celsius as a volatile and read-only control, and its documentation.
> >>>>> Useful to monitor thermals from v4l controls for sensors that support
> >>>>> this.
> >>>>
> >>>> Any justification to expose a temperature sensor outside of the dedicated kernel
> >>>> API hwmon ? I know if may makes it harder to use, as the sensor isn't bound to
> >>>> the camera driver, and also the sensor may not work if the camera is not
> >>>> streaming. But in the end, the API in hwmon does not look that complex, and is
> >>>> perhaps more precise ?
> 
> This sensor is able to read the temperature even if not streaming.
> 
> >>>>
> >>>> All in all, I think we need a strong justification to implement a custom
> >>>> thermometer interface, something described here and documented with care to
> >>>> prevent abuse. I would also see opinion from folks outside of the linux-media,
> >>>> hence why I have CCed hwmon mailing list.
> >>>
> >>> All I can say is that this seems to be odd and a bit outside the scope of
> >>> v4l2. I would have expected the vgxy61 driver to register a hwmon device
> >>> instead.
> >>
> >> I don't have a definitive opinion yet, but as Nicolas raised the issue
> >> by pushing towards hwmon, I'll offer counter-arguments for the sake of
> >> it :-)
> >>
> >> The temperature sensor in the imaging sensor is used to measure the die
> >> temperature, in order to tune the noise and spectral response model of
> >> the imaging sensor. It's thus not a generic-purpose temperature sensor.
> >> If the feature were to be exposed through hwmon, userspace would need to
> >> associate an hwmon device to the imaging sensor V4L2 subdev (we have a
> >> way to do so through the MC API, it doesn't support hwmon at this point,
> >> but I suppose it could be added). There are also various constraints
> >> that tie the temperature reading to the imaging side, it could be that
> >> the temperature would only be readable while capturing frames. That's
> >> probably possible to handle too but returning an error from the
> >> temperature read.
> >>
> >> Code-wise, both the driver and userspace would be more complex, for very
> >> little practical gain (I don't dispute a theorical gain).
> >>
> > 
> > All I can say is - not my subsystem, not my call to make. If you say this
> > is special and is better handled in V4L2, I'll take you by your word.
> > 
> > Guenter
> > 
> 
> I'll happily implement whatever conclusion we make here.
> 
> I could also drop this control for the first iteration of the driver,
> and come back later once a consensus is reached.

That would work too. By the way, what are your use cases for the
temperature sensor ? Have you added the control for the sake of
completeness, or do you have use cases ?
  

On 4/20/22 06:21, Laurent Pinchart wrote:
> Hi Benjamin,
> 
> On Wed, Apr 20, 2022 at 03:01:18PM +0200, Benjamin Mugnier wrote:
>> On 20/04/2022 00:04, Guenter Roeck wrote:
>>> On 4/19/22 14:01, Laurent Pinchart wrote:
>>>> On Tue, Apr 19, 2022 at 12:28:06PM -0700, Guenter Roeck wrote:
>>>>> On 4/19/22 11:24, Nicolas Dufresne wrote:
>>>>>> Hi,
>>>>>>
>>>>>> adding linux-hwmon in CC for a wider feedback.
>>>>>>
>>>>>> Le vendredi 15 avril 2022 à 13:18 +0200, Benjamin Mugnier a écrit :
>>>>>>> Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
>>>>>>> celsius as a volatile and read-only control, and its documentation.
>>>>>>> Useful to monitor thermals from v4l controls for sensors that support
>>>>>>> this.
>>>>>>
>>>>>> Any justification to expose a temperature sensor outside of the dedicated kernel
>>>>>> API hwmon ? I know if may makes it harder to use, as the sensor isn't bound to
>>>>>> the camera driver, and also the sensor may not work if the camera is not
>>>>>> streaming. But in the end, the API in hwmon does not look that complex, and is
>>>>>> perhaps more precise ?
>>
>> This sensor is able to read the temperature even if not streaming.
>>
>>>>>>
>>>>>> All in all, I think we need a strong justification to implement a custom
>>>>>> thermometer interface, something described here and documented with care to
>>>>>> prevent abuse. I would also see opinion from folks outside of the linux-media,
>>>>>> hence why I have CCed hwmon mailing list.
>>>>>
>>>>> All I can say is that this seems to be odd and a bit outside the scope of
>>>>> v4l2. I would have expected the vgxy61 driver to register a hwmon device
>>>>> instead.
>>>>
>>>> I don't have a definitive opinion yet, but as Nicolas raised the issue
>>>> by pushing towards hwmon, I'll offer counter-arguments for the sake of
>>>> it :-)
>>>>
>>>> The temperature sensor in the imaging sensor is used to measure the die
>>>> temperature, in order to tune the noise and spectral response model of
>>>> the imaging sensor. It's thus not a generic-purpose temperature sensor.
>>>> If the feature were to be exposed through hwmon, userspace would need to
>>>> associate an hwmon device to the imaging sensor V4L2 subdev (we have a
>>>> way to do so through the MC API, it doesn't support hwmon at this point,
>>>> but I suppose it could be added). There are also various constraints
>>>> that tie the temperature reading to the imaging side, it could be that
>>>> the temperature would only be readable while capturing frames. That's
>>>> probably possible to handle too but returning an error from the
>>>> temperature read.
>>>>
>>>> Code-wise, both the driver and userspace would be more complex, for very
>>>> little practical gain (I don't dispute a theorical gain).
>>>>
>>>
>>> All I can say is - not my subsystem, not my call to make. If you say this
>>> is special and is better handled in V4L2, I'll take you by your word.
>>>
>>> Guenter
>>>
>>
>> I'll happily implement whatever conclusion we make here.
>>
>> I could also drop this control for the first iteration of the driver,
>> and come back later once a consensus is reached.
> 
> That would work too. By the way, what are your use cases for the
> temperature sensor ? Have you added the control for the sake of
> completeness, or do you have use cases ?
> 

You provided a use case above. Are you saying you made it up ?
Still fine with me, your call, just wondering.

Guenter
  

On Wed, Apr 20, 2022 at 06:58:32AM -0700, Guenter Roeck wrote:
> On 4/20/22 06:21, Laurent Pinchart wrote:
> > On Wed, Apr 20, 2022 at 03:01:18PM +0200, Benjamin Mugnier wrote:
> >> On 20/04/2022 00:04, Guenter Roeck wrote:
> >>> On 4/19/22 14:01, Laurent Pinchart wrote:
> >>>> On Tue, Apr 19, 2022 at 12:28:06PM -0700, Guenter Roeck wrote:
> >>>>> On 4/19/22 11:24, Nicolas Dufresne wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> adding linux-hwmon in CC for a wider feedback.
> >>>>>>
> >>>>>> Le vendredi 15 avril 2022 à 13:18 +0200, Benjamin Mugnier a écrit :
> >>>>>>> Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
> >>>>>>> celsius as a volatile and read-only control, and its documentation.
> >>>>>>> Useful to monitor thermals from v4l controls for sensors that support
> >>>>>>> this.
> >>>>>>
> >>>>>> Any justification to expose a temperature sensor outside of the dedicated kernel
> >>>>>> API hwmon ? I know if may makes it harder to use, as the sensor isn't bound to
> >>>>>> the camera driver, and also the sensor may not work if the camera is not
> >>>>>> streaming. But in the end, the API in hwmon does not look that complex, and is
> >>>>>> perhaps more precise ?
> >>
> >> This sensor is able to read the temperature even if not streaming.
> >>
> >>>>>>
> >>>>>> All in all, I think we need a strong justification to implement a custom
> >>>>>> thermometer interface, something described here and documented with care to
> >>>>>> prevent abuse. I would also see opinion from folks outside of the linux-media,
> >>>>>> hence why I have CCed hwmon mailing list.
> >>>>>
> >>>>> All I can say is that this seems to be odd and a bit outside the scope of
> >>>>> v4l2. I would have expected the vgxy61 driver to register a hwmon device
> >>>>> instead.
> >>>>
> >>>> I don't have a definitive opinion yet, but as Nicolas raised the issue
> >>>> by pushing towards hwmon, I'll offer counter-arguments for the sake of
> >>>> it :-)
> >>>>
> >>>> The temperature sensor in the imaging sensor is used to measure the die
> >>>> temperature, in order to tune the noise and spectral response model of
> >>>> the imaging sensor. It's thus not a generic-purpose temperature sensor.
> >>>> If the feature were to be exposed through hwmon, userspace would need to
> >>>> associate an hwmon device to the imaging sensor V4L2 subdev (we have a
> >>>> way to do so through the MC API, it doesn't support hwmon at this point,
> >>>> but I suppose it could be added). There are also various constraints
> >>>> that tie the temperature reading to the imaging side, it could be that
> >>>> the temperature would only be readable while capturing frames. That's
> >>>> probably possible to handle too but returning an error from the
> >>>> temperature read.
> >>>>
> >>>> Code-wise, both the driver and userspace would be more complex, for very
> >>>> little practical gain (I don't dispute a theorical gain).
> >>>>
> >>>
> >>> All I can say is - not my subsystem, not my call to make. If you say this
> >>> is special and is better handled in V4L2, I'll take you by your word.
> >>>
> >>> Guenter
> >>>
> >>
> >> I'll happily implement whatever conclusion we make here.
> >>
> >> I could also drop this control for the first iteration of the driver,
> >> and come back later once a consensus is reached.
> > 
> > That would work too. By the way, what are your use cases for the
> > temperature sensor ? Have you added the control for the sake of
> > completeness, or do you have use cases ?
> 
> You provided a use case above. Are you saying you made it up ?
> Still fine with me, your call, just wondering.

It's the two most common use cases for imaging sensor temperature
measurements that I know of. My question to Benjamin is if he has the
same and/or other use cases.
  

On 20/04/2022 16:23, Laurent Pinchart wrote:
> On Wed, Apr 20, 2022 at 06:58:32AM -0700, Guenter Roeck wrote:
>> On 4/20/22 06:21, Laurent Pinchart wrote:
>>> On Wed, Apr 20, 2022 at 03:01:18PM +0200, Benjamin Mugnier wrote:
>>>> On 20/04/2022 00:04, Guenter Roeck wrote:
>>>>> On 4/19/22 14:01, Laurent Pinchart wrote:
>>>>>> On Tue, Apr 19, 2022 at 12:28:06PM -0700, Guenter Roeck wrote:
>>>>>>> On 4/19/22 11:24, Nicolas Dufresne wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> adding linux-hwmon in CC for a wider feedback.
>>>>>>>>
>>>>>>>> Le vendredi 15 avril 2022 à 13:18 +0200, Benjamin Mugnier a écrit :
>>>>>>>>> Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
>>>>>>>>> celsius as a volatile and read-only control, and its documentation.
>>>>>>>>> Useful to monitor thermals from v4l controls for sensors that support
>>>>>>>>> this.
>>>>>>>>
>>>>>>>> Any justification to expose a temperature sensor outside of the dedicated kernel
>>>>>>>> API hwmon ? I know if may makes it harder to use, as the sensor isn't bound to
>>>>>>>> the camera driver, and also the sensor may not work if the camera is not
>>>>>>>> streaming. But in the end, the API in hwmon does not look that complex, and is
>>>>>>>> perhaps more precise ?
>>>>
>>>> This sensor is able to read the temperature even if not streaming.
>>>>
>>>>>>>>
>>>>>>>> All in all, I think we need a strong justification to implement a custom
>>>>>>>> thermometer interface, something described here and documented with care to
>>>>>>>> prevent abuse. I would also see opinion from folks outside of the linux-media,
>>>>>>>> hence why I have CCed hwmon mailing list.
>>>>>>>
>>>>>>> All I can say is that this seems to be odd and a bit outside the scope of
>>>>>>> v4l2. I would have expected the vgxy61 driver to register a hwmon device
>>>>>>> instead.
>>>>>>
>>>>>> I don't have a definitive opinion yet, but as Nicolas raised the issue
>>>>>> by pushing towards hwmon, I'll offer counter-arguments for the sake of
>>>>>> it :-)
>>>>>>
>>>>>> The temperature sensor in the imaging sensor is used to measure the die
>>>>>> temperature, in order to tune the noise and spectral response model of
>>>>>> the imaging sensor. It's thus not a generic-purpose temperature sensor.
>>>>>> If the feature were to be exposed through hwmon, userspace would need to
>>>>>> associate an hwmon device to the imaging sensor V4L2 subdev (we have a
>>>>>> way to do so through the MC API, it doesn't support hwmon at this point,
>>>>>> but I suppose it could be added). There are also various constraints
>>>>>> that tie the temperature reading to the imaging side, it could be that
>>>>>> the temperature would only be readable while capturing frames. That's
>>>>>> probably possible to handle too but returning an error from the
>>>>>> temperature read.
>>>>>>
>>>>>> Code-wise, both the driver and userspace would be more complex, for very
>>>>>> little practical gain (I don't dispute a theorical gain).
>>>>>>
>>>>>
>>>>> All I can say is - not my subsystem, not my call to make. If you say this
>>>>> is special and is better handled in V4L2, I'll take you by your word.
>>>>>
>>>>> Guenter
>>>>>
>>>>
>>>> I'll happily implement whatever conclusion we make here.
>>>>
>>>> I could also drop this control for the first iteration of the driver,
>>>> and come back later once a consensus is reached.
>>>
>>> That would work too. By the way, what are your use cases for the
>>> temperature sensor ? Have you added the control for the sake of
>>> completeness, or do you have use cases ?
>>
>> You provided a use case above. Are you saying you made it up ?
>> Still fine with me, your call, just wondering.
> 
> It's the two most common use cases for imaging sensor temperature
> measurements that I know of. My question to Benjamin is if he has the
> same and/or other use cases.
> 

Just like you said in a previous mail. This temperature sensor can be used to implement a retroactive loop from the host according to its value, such as noise correction for instance.
We don't have anything in the Linux user space that implements this yet, this was in anticipation.
So dropping it is fine, I will come back to it if need be ;)
  

On 4/20/22 08:19, Benjamin Mugnier wrote:
[ ... ]
>>
>> It's the two most common use cases for imaging sensor temperature
>> measurements that I know of. My question to Benjamin is if he has the
>> same and/or other use cases.
>>
> 
> Just like you said in a previous mail. This temperature sensor can be used to implement a retroactive loop from the host according to its value, such as noise correction for instance.
> We don't have anything in the Linux user space that implements this yet, this was in anticipation.
> So dropping it is fine, I will come back to it if need be ;)

When you implement this in userspace, you might want to consider situations
where the temperature is _not_ reported via media controls (which might
at least in theory happen if the temperature sensor is not part of the
v4l device), or for existing drivers with hwmon support
(drivers/media/i2c/video-i2c.c comes into mind).

Guenter
  

On Wed, Apr 20, 2022 at 09:19:53AM -0700, Guenter Roeck wrote:
> On 4/20/22 08:19, Benjamin Mugnier wrote:
> [ ... ]
> >>
> >> It's the two most common use cases for imaging sensor temperature
> >> measurements that I know of. My question to Benjamin is if he has the
> >> same and/or other use cases.
> >>
> > 
> > Just like you said in a previous mail. This temperature sensor can
> > be used to implement a retroactive loop from the host according to
> > its value, such as noise correction for instance.
> > We don't have anything in the Linux user space that implements this
> > yet, this was in anticipation.
> > So dropping it is fine, I will come back to it if need be ;)
> 
> When you implement this in userspace, you might want to consider situations
> where the temperature is _not_ reported via media controls (which might
> at least in theory happen if the temperature sensor is not part of the
> v4l device), or for existing drivers with hwmon support
> (drivers/media/i2c/video-i2c.c comes into mind).

That's a good point. I wouldn't expect external temperature sensors to
be very useful for this use case though, as what we really need is the
temperature of the camera sensor die.