Message-ID: <0718981e-a433-0119-ba34-7e28ebbf4a2b@wolfvision.net>
Date:   Tue, 13 Jun 2023 09:36:12 +0200
From:   Michael Riesch <michael.riesch@...fvision.net>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        libcamera-devel@...ts.libcamera.org,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Matthias Fend <Matthias.Fend@...fvision.net>,
        Dave Stevenson <dave.stevenson@...pberrypi.com>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC v2 3/6] media: v4l2-ctrls: add lens group status
 controls for zoom and focus

Hi Laurent,

Thanks for the review. I'll address your comments in the next iteration,
but please be aware that it may take some time due to other things
needing my attention.

On 6/7/23 09:03, Laurent Pinchart wrote:
> Hi Michael,
> 
> Thank you for the patch.
> 
> On Tue, Apr 25, 2023 at 11:45:13AM +0200, Michael Riesch wrote:
>> Add the controls V4L2_CID_{FOCUS,ZOOM}_{CURRENT,STATUS} that report the
>> current position and status, respectively, of the zoom lens group and
>> the focus lens group.
>>
>> Signed-off-by: Michael Riesch <michael.riesch@...fvision.net>
>> ---
>>  .../userspace-api/media/v4l/ext-ctrls-camera.rst   | 46 ++++++++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c          | 10 +++++
>>  include/uapi/linux/v4l2-controls.h                 |  9 +++++
>>  3 files changed, 65 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> index 42cf4c3cda0c..3ea4175f9619 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> @@ -150,6 +150,29 @@ enum v4l2_exposure_metering -
>>      write-only control. It should be implemented only if the device cannot
>>      handle absolute values.
>>  
>> +``V4L2_CID_FOCUS_CURRENT (integer)``
>> +    The current position of the focal point. The unit is undefined. Larger
>> +    values indicate that the focus is closer to the camera, smaller values
>> +    indicate that the focus is closer to infinity. This is a read-only control.
> 
> I think you should also update the definition of the
> V4L2_CID_FOCUS_ABSOLUTE control to indicate the reading the control back
> will return the focus *target*, not the current focus position. This
> control should also refer to V4L2_CID_FOCUS_ABSOLUTE.
> 
> I think we should also require the V4L2_CID_FOCUS_CURRENT and
> V4L2_CID_FOCUS_ABSOLUTE controls to have the same unit.
> 
> Is this control expected to generate events, when the lens reaches its
> target position, or during movement ?

Although I can't rule it out completely, it wouldn't expect that the
current position of a lens triggers events. I would say that this is a
continuous movement (well, in case of a stepper motor a quasi-continuous
one), so when exactly should the controller generate in interrupt/event?

I expect that the current position is polled instead, e.g., once per
frame in the scope of an autofocus algorithm.

When the lens reaches its target, an event could be generated, but this
would be related to the _STATUS control below.

> How should we deal with the drivers that implement
> V4L2_CID_FOCUS_ABSOLUTE, do any of them implement reading
> V4L2_CID_FOCUS_ABSOLUTE with the semantics of the V4L2_CID_FOCUS_CURRENT
> control, instead of returning the focus target ?

Not sure whether I understand your question correctly, but drivers that
implement V4L2_CID_FOCUS_ABSOLUTE are mostly VCMs where I believe it is
assumed that target always equals the current position due to their fast
response.

But then there is the UVC driver of course. Only had a quick look, but
it seems to me that Focus (Absolute) does not support GET_CUR. I would
guess that GET_CUR could return the current value in the sense of
V4L2_CID_FOCUS_CURRENT.

>> +
>> +``V4L2_CID_FOCUS_STATUS (bitmask)``
>> +    The status of the focus lens group. The possible flags are described in
>> +    the table below. This is a read-only control.
> 
> Is this control expected to generate events ?

Yes, one could think of events when the lens reached its target or a
collision prevented exactly that.

>> +
>> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
>> +
>> +.. flat-table::
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +
>> +    * - ``V4L2_LENS_STATUS_IDLE``
>> +      - Focus lens group is at rest.
>> +    * - ``V4L2_LENS_STATUS_BUSY``
>> +      - Focus lens group is moving.
> 
> V4L2_LENS_STATUS_MOVING would be a better name if it's defined as "is
> moving".
> 
>> +    * - ``V4L2_LENS_STATUS_FAILED``
>> +      - Focus lens group has failed to reach its target position. The driver
> 
> What are the expected reasons for this ?

Variable focal length optics are complex beasts. It could happen, for
instance, that the range of the focus lens depends on the position of
the zoom lens(es). A smart controller could handle such situations
properly and the failure will never occur, but there might be
not-so-smart controllers as well.

In our case the status of the individual lenses is very important for
the initial calibration. Here, we need to be informed when a
collision/failure happened. Once the calibration is complete, this
should not occur anymore.

Best regards,
Michael

>> +	will not transition from this state until another action is performed
>> +	by an application.
> 
> You're talking about transitions here, I think you should document the
> state machine for the other states too. I expect the control to
> transition from IDLE to MOVING when the V4L2_CID_FOCUS_ABSOLUTE control
> is set, and transition from MOVING to IDLE or FAILED at the end of the
> motion. What happens if the user sets V4L2_CID_FOCUS_ABSOLUTE while the
> status is MOVING also needs to be documented.
> 
> It sounds we need helper functions to implement this state machine and
> generate events, leaving it to drivers would open the door to different
> behaviours for different devices.
> 
> All these comments apply to zoom too.
> 
>>  
>>  ``V4L2_CID_FOCUS_AUTO (boolean)``
>>      Enables continuous automatic focus adjustments. The effect of manual
>> @@ -241,6 +264,29 @@ enum v4l2_auto_focus_range -
>>      movement. A negative value moves the zoom lens group towards the
>>      wide-angle direction. The zoom speed unit is driver-specific.
>>  
>> +``V4L2_CID_ZOOM_CURRENT (integer)``
>> +    The current objective lens focal length. The unit is undefined and
>> +    its value should be a positive integer. This is a read-only control.
>> +
>> +``V4L2_CID_ZOOM_STATUS (bitmask)``
>> +    The status of the zoom lens group. The possible flags are described in
>> +    the table below. This is a read-only control.
>> +
>> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
>> +
>> +.. flat-table::
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +
>> +    * - ``V4L2_LENS_STATUS_IDLE``
>> +      - Zoom lens group is at rest.
>> +    * - ``V4L2_LENS_STATUS_BUSY``
>> +      - Zoom lens group is moving.
>> +    * - ``V4L2_LENS_STATUS_FAILED``
>> +      - Zoom lens group 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 564fedee2c88..794ef3ab0c02 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> @@ -1044,6 +1044,10 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_CAMERA_ORIENTATION:	return "Camera Orientation";
>>  	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
>>  	case V4L2_CID_HDR_SENSOR_MODE:		return "HDR Sensor Mode";
>> +	case V4L2_CID_FOCUS_CURRENT:		return "Focus, Current";
>> +	case V4L2_CID_FOCUS_STATUS:		return "Focus, Status";
>> +	case V4L2_CID_ZOOM_CURRENT:		return "Zoom, Current";
>> +	case V4L2_CID_ZOOM_STATUS:		return "Zoom, Status";
>>  
>>  	/* FM Radio Modulator controls */
>>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>> @@ -1593,6 +1597,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>  		*flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
>>  			  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
>>  		break;
>> +	case V4L2_CID_FOCUS_CURRENT:
>> +	case V4L2_CID_FOCUS_STATUS:
>> +	case V4L2_CID_ZOOM_CURRENT:
>> +	case V4L2_CID_ZOOM_STATUS:
>> +		*flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_VOLATILE;
>> +		break;
>>  	case V4L2_CID_FLASH_STROBE_STATUS:
>>  	case V4L2_CID_AUTO_FOCUS_STATUS:
>>  	case V4L2_CID_FLASH_READY:
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 5e80daa4ffe0..793ee8c65e87 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -993,6 +993,15 @@ enum v4l2_auto_focus_range {
>>  
>>  #define V4L2_CID_HDR_SENSOR_MODE		(V4L2_CID_CAMERA_CLASS_BASE+36)
>>  
>> +#define V4L2_LENS_STATUS_IDLE			(0 << 0)
>> +#define V4L2_LENS_STATUS_BUSY			(1 << 0)
>> +#define V4L2_LENS_STATUS_FAILED			(1 << 2)
>> +
>> +#define V4L2_CID_FOCUS_CURRENT			(V4L2_CID_CAMERA_CLASS_BASE+37)
>> +#define V4L2_CID_FOCUS_STATUS			(V4L2_CID_CAMERA_CLASS_BASE+38)
>> +#define V4L2_CID_ZOOM_CURRENT			(V4L2_CID_CAMERA_CLASS_BASE+39)
>> +#define V4L2_CID_ZOOM_STATUS			(V4L2_CID_CAMERA_CLASS_BASE+40)
>> +
>>  /* FM Modulator class control IDs */
>>  
>>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
>>
> 

Powered by blists - more mailing lists