diff mbox series

[RFC,v2,2/6] media: v4l2-ctrls: clarify documentation of V4L2_CID_FOCUS_RELATIVE

Message ID 20230406-feature-controls-lens-v2-2-faa8ad2bc404@wolfvision.net
State New
Headers
Series media: v4l2-ctrls: add controls for complex lens controller devices |

Commit Message

Michael Riesch April 25, 2023, 9:45 a.m. UTC 
  The control V4L2_CID_FOCUS_RELATIVE only makes sense if the device cannot
handle absolute focal point positioning with V4L2_CID_FOCUS_ABSOLUTE.
Clarify this in the documentation.

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
 Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart June 6, 2023, 10:36 a.m. UTC | #1 
Hi Michael,

Thank you for the patch.

On Tue, Apr 25, 2023 at 11:45:12AM +0200, Michael Riesch wrote:
> The control V4L2_CID_FOCUS_RELATIVE only makes sense if the device cannot
> handle absolute focal point positioning with V4L2_CID_FOCUS_ABSOLUTE.
> Clarify this in the documentation.
> 
> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index df29150dce7b..42cf4c3cda0c 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -147,7 +147,9 @@ enum v4l2_exposure_metering -
>      This control moves the focal point of the camera by the specified
>      amount. The unit is undefined. Positive values move the focus closer
>      to the camera, negative values towards infinity. This is a
> -    write-only control.
> +    write-only control. It should be implemented only if the device cannot
> +    handle absolute values.
> +

Extra blank line.

I don't think this is right. The control was added for the UVC driver,
and there are devices that implement both absolute and relative focus.

>  
>  ``V4L2_CID_FOCUS_AUTO (boolean)``
>      Enables continuous automatic focus adjustments. The effect of manual
>
Michael Riesch June 6, 2023, 1:15 p.m. UTC | #2 
Hi Laurent,

On 6/6/23 12:36, Laurent Pinchart wrote:
> Hi Michael,
> 
> Thank you for the patch.
> 
> On Tue, Apr 25, 2023 at 11:45:12AM +0200, Michael Riesch wrote:
>> The control V4L2_CID_FOCUS_RELATIVE only makes sense if the device cannot
>> handle absolute focal point positioning with V4L2_CID_FOCUS_ABSOLUTE.
>> Clarify this in the documentation.
>>
>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>> ---
>>  Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> index df29150dce7b..42cf4c3cda0c 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> @@ -147,7 +147,9 @@ enum v4l2_exposure_metering -
>>      This control moves the focal point of the camera by the specified
>>      amount. The unit is undefined. Positive values move the focus closer
>>      to the camera, negative values towards infinity. This is a
>> -    write-only control.
>> +    write-only control. It should be implemented only if the device cannot
>> +    handle absolute values.
>> +
> 
> Extra blank line.

Will fix that.

> I don't think this is right. The control was added for the UVC driver,
> and there are devices that implement both absolute and relative focus.

I am by no means an expert here and just following Sakari's suggestion
here (see [0]). I can drop the patch, leave it as-is, or modify it.
Whatever makes sense to you guys. But maybe I should leave this to
someone more knowledgeable in this area and drop the patch from my
series. The changes above are completely orthogonal to my work.

Best regards,
Michael

> 
>>  
>>  ``V4L2_CID_FOCUS_AUTO (boolean)``
>>      Enables continuous automatic focus adjustments. The effect of manual
>>
> 

[0] https://lore.kernel.org/all/ZDbChgZJHVaaX3%2Fx@kekkonen.localdomain/
Laurent Pinchart June 7, 2023, 6:42 a.m. UTC | #3 
Hi Michael,

On Tue, Jun 06, 2023 at 03:15:33PM +0200, Michael Riesch wrote:
> On 6/6/23 12:36, Laurent Pinchart wrote:
> > On Tue, Apr 25, 2023 at 11:45:12AM +0200, Michael Riesch wrote:
> >> The control V4L2_CID_FOCUS_RELATIVE only makes sense if the device cannot
> >> handle absolute focal point positioning with V4L2_CID_FOCUS_ABSOLUTE.
> >> Clarify this in the documentation.
> >>
> >> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> >> ---
> >>  Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >> index df29150dce7b..42cf4c3cda0c 100644
> >> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >> @@ -147,7 +147,9 @@ enum v4l2_exposure_metering -
> >>      This control moves the focal point of the camera by the specified
> >>      amount. The unit is undefined. Positive values move the focus closer
> >>      to the camera, negative values towards infinity. This is a
> >> -    write-only control.
> >> +    write-only control. It should be implemented only if the device cannot
> >> +    handle absolute values.
> >> +
> > 
> > Extra blank line.
> 
> Will fix that.
> 
> > I don't think this is right. The control was added for the UVC driver,
> > and there are devices that implement both absolute and relative focus.
> 
> I am by no means an expert here and just following Sakari's suggestion
> here (see [0]). I can drop the patch, leave it as-is, or modify it.
> Whatever makes sense to you guys. But maybe I should leave this to
> someone more knowledgeable in this area and drop the patch from my
> series. The changes above are completely orthogonal to my work.

V4L2_CID_FOCUS_RELATIVE is an annoying control. It was introduced for
UVC, and to my surprise, it turns out it has never been implemented in
the uvcvideo driver. The 3 devices I know of that implement the UVC
relative focus control also implement the UVC absolute focus control.

I'm tempted to deprecate this control completely. Sakari, any opinion ?

> >>  
> >>  ``V4L2_CID_FOCUS_AUTO (boolean)``
> >>      Enables continuous automatic focus adjustments. The effect of manual
> >>
> > 
> 
> [0] https://lore.kernel.org/all/ZDbChgZJHVaaX3%2Fx@kekkonen.localdomain/
Laurent Pinchart June 7, 2023, 6:55 a.m. UTC | #4 
On Wed, Jun 07, 2023 at 09:42:07AM +0300, Laurent Pinchart wrote:
> On Tue, Jun 06, 2023 at 03:15:33PM +0200, Michael Riesch wrote:
> > On 6/6/23 12:36, Laurent Pinchart wrote:
> > > On Tue, Apr 25, 2023 at 11:45:12AM +0200, Michael Riesch wrote:
> > >> The control V4L2_CID_FOCUS_RELATIVE only makes sense if the device cannot
> > >> handle absolute focal point positioning with V4L2_CID_FOCUS_ABSOLUTE.
> > >> Clarify this in the documentation.
> > >>
> > >> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> > >> ---
> > >>  Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 +++-
> > >>  1 file changed, 3 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >> index df29150dce7b..42cf4c3cda0c 100644
> > >> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >> @@ -147,7 +147,9 @@ enum v4l2_exposure_metering -
> > >>      This control moves the focal point of the camera by the specified
> > >>      amount. The unit is undefined. Positive values move the focus closer
> > >>      to the camera, negative values towards infinity. This is a
> > >> -    write-only control.
> > >> +    write-only control. It should be implemented only if the device cannot
> > >> +    handle absolute values.
> > >> +
> > > 
> > > Extra blank line.
> > 
> > Will fix that.
> > 
> > > I don't think this is right. The control was added for the UVC driver,
> > > and there are devices that implement both absolute and relative focus.
> > 
> > I am by no means an expert here and just following Sakari's suggestion
> > here (see [0]). I can drop the patch, leave it as-is, or modify it.
> > Whatever makes sense to you guys. But maybe I should leave this to
> > someone more knowledgeable in this area and drop the patch from my
> > series. The changes above are completely orthogonal to my work.
> 
> V4L2_CID_FOCUS_RELATIVE is an annoying control. It was introduced for
> UVC, and to my surprise, it turns out it has never been implemented in
> the uvcvideo driver. The 3 devices I know of that implement the UVC
> relative focus control also implement the UVC absolute focus control.
> 
> I'm tempted to deprecate this control completely. Sakari, any opinion ?

This is how the UVC relative focus control is defined.

  4.2.2.1.7 Focus (Relative) Control

  The Focus (Relative) Control is used to move the focus lens group to
  specify the distance to the optimally focused target.

  The bFocusRelative field indicates whether the focus lens group is
  stopped or is moving for near or for infinity direction. A value of 1
  indicates that the focus lens group is moved for near direction. A
  value of 0 indicates that the focus lens group is stopped. And a value
  of 0xFF indicates that the lens group is moved for infinity direction.
  The GET_MIN, GET_MAX, GET_RES and GET_DEF requests will return zero
  for this field.

  The bSpeed field indicates the speed of the lens group movement. A low
  number indicates a slow speed and a high number indicates a high
  speed. The GET_MIN, GET_MAX and GET_RES requests are used to retrieve
  the range and resolution for this field. The GET_DEF request is used
  to retrieve the default value for this field. If the control does not
  support speed control, it will return the value 1 in this field for
  all these requests.

  If both Relative and Absolute Controls are supported, a SET_CUR to the
  Relative Control with a value other than 0x00 shall result in a
  Control Change interrupt for the Absolute Control at the end of the
  movement (see section 2.4.2.2, “Status Interrupt Endpoint”). The end
  of movement can be due to physical device limits, or due to an
  explicit request by the host to stop the movement. If the end of
  movement is due to physical device limits (such as a limit in range of
  motion), a Control Change interrupt shall be generated for this
  Relative Control. If there is no limit in range of motion, a Control
  Change interrupt is not required.

It seems there's no way we can just map this to V4L2_CID_FOCUS_RELATIVE,
making the V4L2 relative focus control quite useless.

> > >>  
> > >>  ``V4L2_CID_FOCUS_AUTO (boolean)``
> > >>      Enables continuous automatic focus adjustments. The effect of manual
> > >>
> > 
> > [0] https://lore.kernel.org/all/ZDbChgZJHVaaX3%2Fx@kekkonen.localdomain/
Sakari Ailus June 8, 2023, 1:41 p.m. UTC | #5 
Hi Laurent,

On Wed, Jun 07, 2023 at 09:55:20AM +0300, Laurent Pinchart wrote:
> On Wed, Jun 07, 2023 at 09:42:07AM +0300, Laurent Pinchart wrote:
> > On Tue, Jun 06, 2023 at 03:15:33PM +0200, Michael Riesch wrote:
> > > On 6/6/23 12:36, Laurent Pinchart wrote:
> > > > On Tue, Apr 25, 2023 at 11:45:12AM +0200, Michael Riesch wrote:
> > > >> The control V4L2_CID_FOCUS_RELATIVE only makes sense if the device cannot
> > > >> handle absolute focal point positioning with V4L2_CID_FOCUS_ABSOLUTE.
> > > >> Clarify this in the documentation.
> > > >>
> > > >> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> > > >> ---
> > > >>  Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 +++-
> > > >>  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > > >> index df29150dce7b..42cf4c3cda0c 100644
> > > >> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > > >> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > > >> @@ -147,7 +147,9 @@ enum v4l2_exposure_metering -
> > > >>      This control moves the focal point of the camera by the specified
> > > >>      amount. The unit is undefined. Positive values move the focus closer
> > > >>      to the camera, negative values towards infinity. This is a
> > > >> -    write-only control.
> > > >> +    write-only control. It should be implemented only if the device cannot
> > > >> +    handle absolute values.
> > > >> +
> > > > 
> > > > Extra blank line.
> > > 
> > > Will fix that.
> > > 
> > > > I don't think this is right. The control was added for the UVC driver,
> > > > and there are devices that implement both absolute and relative focus.
> > > 
> > > I am by no means an expert here and just following Sakari's suggestion
> > > here (see [0]). I can drop the patch, leave it as-is, or modify it.
> > > Whatever makes sense to you guys. But maybe I should leave this to
> > > someone more knowledgeable in this area and drop the patch from my
> > > series. The changes above are completely orthogonal to my work.
> > 
> > V4L2_CID_FOCUS_RELATIVE is an annoying control. It was introduced for
> > UVC, and to my surprise, it turns out it has never been implemented in
> > the uvcvideo driver. The 3 devices I know of that implement the UVC
> > relative focus control also implement the UVC absolute focus control.
> > 
> > I'm tempted to deprecate this control completely. Sakari, any opinion ?
> 
> This is how the UVC relative focus control is defined.
> 
>   4.2.2.1.7 Focus (Relative) Control
> 
>   The Focus (Relative) Control is used to move the focus lens group to
>   specify the distance to the optimally focused target.
> 
>   The bFocusRelative field indicates whether the focus lens group is
>   stopped or is moving for near or for infinity direction. A value of 1
>   indicates that the focus lens group is moved for near direction. A
>   value of 0 indicates that the focus lens group is stopped. And a value
>   of 0xFF indicates that the lens group is moved for infinity direction.
>   The GET_MIN, GET_MAX, GET_RES and GET_DEF requests will return zero
>   for this field.
> 
>   The bSpeed field indicates the speed of the lens group movement. A low
>   number indicates a slow speed and a high number indicates a high
>   speed. The GET_MIN, GET_MAX and GET_RES requests are used to retrieve
>   the range and resolution for this field. The GET_DEF request is used
>   to retrieve the default value for this field. If the control does not
>   support speed control, it will return the value 1 in this field for
>   all these requests.
> 
>   If both Relative and Absolute Controls are supported, a SET_CUR to the
>   Relative Control with a value other than 0x00 shall result in a
>   Control Change interrupt for the Absolute Control at the end of the
>   movement (see section 2.4.2.2, “Status Interrupt Endpoint”). The end
>   of movement can be due to physical device limits, or due to an
>   explicit request by the host to stop the movement. If the end of
>   movement is due to physical device limits (such as a limit in range of
>   motion), a Control Change interrupt shall be generated for this
>   Relative Control. If there is no limit in range of motion, a Control
>   Change interrupt is not required.
> 
> It seems there's no way we can just map this to V4L2_CID_FOCUS_RELATIVE,
> making the V4L2 relative focus control quite useless.

I'd deprecate this control as it doesn't have any use.

The control documentation + other references in V4L2 control framework
could be even removed but the control ID definition needs to stay as user
space may refer to it.
diff mbox series

Patch

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
index df29150dce7b..42cf4c3cda0c 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
@@ -147,7 +147,9 @@  enum v4l2_exposure_metering -
     This control moves the focal point of the camera by the specified
     amount. The unit is undefined. Positive values move the focus closer
     to the camera, negative values towards infinity. This is a
-    write-only control.
+    write-only control. It should be implemented only if the device cannot
+    handle absolute values.
+
 
 ``V4L2_CID_FOCUS_AUTO (boolean)``
     Enables continuous automatic focus adjustments. The effect of manual