| Message ID | 20230406-feature-controls-lens-v2-2-faa8ad2bc404@wolfvision.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: v4l2-ctrls: add controls for complex lens controller devices | expand |
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 >
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/
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/
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/
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 --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
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(-)