Re: [PATCH RFC v2 6/6] media: v4l2-ctrls: add controls for individual zoom lenses

[Michael Riesch <michael.riesch@wolfvision.net>]

Hi Hans,

On 6/6/23 12:29, Hans Verkuil wrote:
> On 25/04/2023 11:45, Michael Riesch wrote:
>> A zoom lens group may consist of several lenses, and in a calibration
>> context it may be necessary to position the lenses individually. Add a
>> tuple of V4L2_CID_LENS_CALIB_ZOOMx_{ABSOLUTE,CURRENT,STATUS} controls
>> for each individual lens, where x = {1...5}.
>>
>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>> ---
>>  .../userspace-api/media/v4l/ext-ctrls-camera.rst   | 30 ++++++++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c          | 25 ++++++++++++++++++
>>  include/uapi/linux/v4l2-controls.h                 | 18 +++++++++++++
>>  3 files changed, 73 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> index 8b54a0f3a617..21391f076971 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> @@ -332,6 +332,36 @@ enum v4l2_auto_focus_range -
>>      * - ``V4L2_LENS_CALIB_FAILED``
>>        - Lens calibration procedure has failed.
>>  
>> +``V4L2_CID_LENS_CALIB_ZOOM{1...5}_ABSOLUTE`` (integer)
>> +    Set the absolute position of the individual lens of the zoom lens group.
>> +    Most likely, this is done in a calibration context. The unit is
>> +    driver-specific.
>> +
>> +``V4L2_CID_LENS_CALIB_ZOOM{1...5}_CURRENT`` (integer)
>> +    The current absolute position of the individual lens of the zoom lens group.
>> +    Most likely, this is done in a calibration context. The unit is
>> +    driver-specific. This is a read-only control.
>> +
>> +``V4L2_CID_LENS_CALIB_ZOOM{1...5}_STATUS`` (bitmask)
>> +    The current status of the individual lens of the zoom lens group.
>> +    Most likely, this is done in a calibration context. The possible flags are
>> +    described in the table below. This is a read-only control.
> 
> Wouldn't it be better to have this as an array control? That way the number of
> lenses is set by the driver and can be easily read by userspace.

Are you referring to the _STATUS controls exclusively or is this a
suggestion for _CURRENT and _ABSOLUTE as well?

This would mean that the user space can read out e.g., the status of all
lenses in one batch, right? This would make sense to me.

I am not sure about setting all target positions at the same time ->
I'll have to think about that one a bit.

Best regards,
Michael

> 
>> +
>> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
>> +
>> +.. flat-table::
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +
>> +    * - ``V4L2_LENS_STATUS_IDLE``
>> +      - Zoom lens is at rest.
>> +    * - ``V4L2_LENS_STATUS_BUSY``
>> +      - Zoom lens is moving.
>> +    * - ``V4L2_LENS_STATUS_FAILED``
>> +      - Zoom lens has failed to reach its target position. The driver will
>> +	not transition from this state until another action is performed by an
>> +	application.
>> +
>>  ``V4L2_CID_IRIS_ABSOLUTE (integer)``
>>      This control sets the camera's aperture to the specified value. The
>>      unit is undefined. Larger values open the iris wider, smaller values
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> index faddfecba6d9..8a78cffcd3e8 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> @@ -1052,6 +1052,21 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_ZOOM_SPEED:		return "Zoom, Speed";
>>  	case V4L2_CID_LENS_CALIB_CONTROL:	return "Lens Calibration, Control";
>>  	case V4L2_CID_LENS_CALIB_STATUS:	return "Lens Calibration, Status";
>> +	case V4L2_CID_LENS_CALIB_ZOOM1_ABSOLUTE:	return "Zoom1, Absolute";
>> +	case V4L2_CID_LENS_CALIB_ZOOM2_ABSOLUTE:	return "Zoom2, Absolute";
>> +	case V4L2_CID_LENS_CALIB_ZOOM3_ABSOLUTE:	return "Zoom3, Absolute";
>> +	case V4L2_CID_LENS_CALIB_ZOOM4_ABSOLUTE:	return "Zoom4, Absolute";
>> +	case V4L2_CID_LENS_CALIB_ZOOM5_ABSOLUTE:	return "Zoom5, Absolute";
>> +	case V4L2_CID_LENS_CALIB_ZOOM1_CURRENT:	return "Zoom1, Current";
>> +	case V4L2_CID_LENS_CALIB_ZOOM2_CURRENT:	return "Zoom1, Current";
>> +	case V4L2_CID_LENS_CALIB_ZOOM3_CURRENT:	return "Zoom1, Current";
>> +	case V4L2_CID_LENS_CALIB_ZOOM4_CURRENT:	return "Zoom1, Current";
>> +	case V4L2_CID_LENS_CALIB_ZOOM5_CURRENT:	return "Zoom1, Current";
> 
> You forget to update the number, it's all Zoom1 here.
> 
>> +	case V4L2_CID_LENS_CALIB_ZOOM1_STATUS:	return "Zoom1, Status";
>> +	case V4L2_CID_LENS_CALIB_ZOOM2_STATUS:	return "Zoom2, Status";
>> +	case V4L2_CID_LENS_CALIB_ZOOM3_STATUS:	return "Zoom3, Status";
>> +	case V4L2_CID_LENS_CALIB_ZOOM4_STATUS:	return "Zoom4, Status";
>> +	case V4L2_CID_LENS_CALIB_ZOOM5_STATUS:	return "Zoom5, Status";
>>  
>>  	/* FM Radio Modulator controls */
>>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>> @@ -1607,6 +1622,16 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>  	case V4L2_CID_ZOOM_CURRENT:
>>  	case V4L2_CID_ZOOM_STATUS:
>>  	case V4L2_CID_LENS_CALIB_STATUS:
>> +	case V4L2_CID_LENS_CALIB_ZOOM1_CURRENT:
>> +	case V4L2_CID_LENS_CALIB_ZOOM2_CURRENT:
>> +	case V4L2_CID_LENS_CALIB_ZOOM3_CURRENT:
>> +	case V4L2_CID_LENS_CALIB_ZOOM4_CURRENT:
>> +	case V4L2_CID_LENS_CALIB_ZOOM5_CURRENT:
>> +	case V4L2_CID_LENS_CALIB_ZOOM1_STATUS:
>> +	case V4L2_CID_LENS_CALIB_ZOOM2_STATUS:
>> +	case V4L2_CID_LENS_CALIB_ZOOM3_STATUS:
>> +	case V4L2_CID_LENS_CALIB_ZOOM4_STATUS:
>> +	case V4L2_CID_LENS_CALIB_ZOOM5_STATUS:
>>  		*flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_VOLATILE;
> 
> Same issue as for patch 3 w.r.t. VOLATILE.
> 
>>  		break;
>>  	case V4L2_CID_FLASH_STROBE_STATUS:
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 24c0eb5f4d29..7c49c0ba23d4 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -1016,6 +1016,24 @@ enum v4l2_auto_focus_range {
>>  
>>  #define V4L2_CID_LENS_CALIB_STATUS		(V4L2_CID_CAMERA_CLASS_BASE+44)
>>  
>> +#define V4L2_CID_LENS_CALIB_ZOOM1_ABSOLUTE	(V4L2_CID_CAMERA_CLASS_BASE+45)
>> +#define V4L2_CID_LENS_CALIB_ZOOM2_ABSOLUTE	(V4L2_CID_CAMERA_CLASS_BASE+46)
>> +#define V4L2_CID_LENS_CALIB_ZOOM3_ABSOLUTE	(V4L2_CID_CAMERA_CLASS_BASE+47)
>> +#define V4L2_CID_LENS_CALIB_ZOOM4_ABSOLUTE	(V4L2_CID_CAMERA_CLASS_BASE+48)
>> +#define V4L2_CID_LENS_CALIB_ZOOM5_ABSOLUTE	(V4L2_CID_CAMERA_CLASS_BASE+49)
>> +
>> +#define V4L2_CID_LENS_CALIB_ZOOM1_CURRENT	(V4L2_CID_CAMERA_CLASS_BASE+50)
>> +#define V4L2_CID_LENS_CALIB_ZOOM2_CURRENT	(V4L2_CID_CAMERA_CLASS_BASE+51)
>> +#define V4L2_CID_LENS_CALIB_ZOOM3_CURRENT	(V4L2_CID_CAMERA_CLASS_BASE+52)
>> +#define V4L2_CID_LENS_CALIB_ZOOM4_CURRENT	(V4L2_CID_CAMERA_CLASS_BASE+53)
>> +#define V4L2_CID_LENS_CALIB_ZOOM5_CURRENT	(V4L2_CID_CAMERA_CLASS_BASE+54)
>> +
>> +#define V4L2_CID_LENS_CALIB_ZOOM1_STATUS	(V4L2_CID_CAMERA_CLASS_BASE+55)
>> +#define V4L2_CID_LENS_CALIB_ZOOM2_STATUS	(V4L2_CID_CAMERA_CLASS_BASE+56)
>> +#define V4L2_CID_LENS_CALIB_ZOOM3_STATUS	(V4L2_CID_CAMERA_CLASS_BASE+57)
>> +#define V4L2_CID_LENS_CALIB_ZOOM4_STATUS	(V4L2_CID_CAMERA_CLASS_BASE+58)
>> +#define V4L2_CID_LENS_CALIB_ZOOM5_STATUS	(V4L2_CID_CAMERA_CLASS_BASE+59)
>> +
>>  /* FM Modulator class control IDs */
>>  
>>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
>>
> 
> Disclaimer: I do not have enough domain knowledge to comment on if this is the
> right solution or not. I can only comment on the control framework specifics.
> 
> Regards,
> 
> 	Hans