[PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Gergo Koteles]

The Logitech BCC950 incorrectly reports 1 (the max value)
for the default values of V4L2_CID_PAN_SPEED,
V4L2_CID_TILT_SPEED.

This patch sets them to 0, which is the stop value.

Previous discussion
Link: https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/

Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 5e9d3da862dd..e131958c0930 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -444,9 +444,10 @@ static s32 uvc_ctrl_get_rel_speed(struct uvc_control_mapping *mapping,
 		return -data[first+1];
 	case UVC_GET_MAX:
 	case UVC_GET_RES:
+		return data[first+1];
 	case UVC_GET_DEF:
 	default:
-		return data[first+1];
+		return 0;
 	}
 }
 

base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
-- 
2.40.1

Re: [PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Ricardo Ribalda]

[Now in plain text mode]

Hi Gergo

Doesn't your patch affect pan and tilt for all the cameras, not only the BCC950?

Also it seems that 1 means that device does not support programmable
speed. Is that correct?

```
The bPanSpeed field is used to specify the speed of the movement for
the Pan direction. A low
number indicates a slow speed and a high number indicates a higher
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 for the Pan control, it will return the value 1
in this field for all these
requests.
```

When you program that value do you see any difference on the device?
What is max, min and res?

Thanks!

Regards!


On Wed, 14 Jun 2023 at 15:13, Gergo Koteles <soyer@irl.hu> wrote:
>
> The Logitech BCC950 incorrectly reports 1 (the max value)
> for the default values of V4L2_CID_PAN_SPEED,
> V4L2_CID_TILT_SPEED.
>
> This patch sets them to 0, which is the stop value.
>
> Previous discussion
> Link: https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/
>
> Signed-off-by: Gergo Koteles <soyer@irl.hu>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 5e9d3da862dd..e131958c0930 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -444,9 +444,10 @@ static s32 uvc_ctrl_get_rel_speed(struct uvc_control_mapping *mapping,
>                 return -data[first+1];
>         case UVC_GET_MAX:
>         case UVC_GET_RES:
> +               return data[first+1];
>         case UVC_GET_DEF:
>         default:
> -               return data[first+1];
> +               return 0;
>         }
>  }
>
>
> base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
> --
> 2.40.1
>


-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[soyer]

Hi Ricardo

On Wed, 2023-06-14 at 16:19 +0200, Ricardo Ribalda wrote:
> [Now in plain text mode]
> 
> Hi Gergo
> 
> Doesn't your patch affect pan and tilt for all the cameras, not only
> the BCC950?
> 
Yes, it affects all cameras that support CT_PANTILT_RELATIVE_CONTROL.

> Also it seems that 1 means that device does not support programmable
> speed. Is that correct?
> 
> ```
> The bPanSpeed field is used to specify the speed of the movement for
> the Pan direction. A low
> number indicates a slow speed and a high number indicates a higher
> 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 for the Pan control, it will return the value 1
> in this field for all these
> requests.
> ```
> 
I started from the V4L2 control description.

V4L2_CID_PAN_SPEED (integer)
This control turns the camera horizontally at the specific speed. The
unit is undefined. A positive value moves the camera to the right
(clockwise when viewed from above), a negative value to the left. A
value of zero stops the motion if one is in progress and has no effect
otherwise.

And this is why I thought that 1 is not a good default value, because
it moves the camera.
The other V4L2 controls have a default value that I can safely set the
controls to.

Are you using it to determine if the camera supports speed control?

> When you program that value do you see any difference on the device?
> What is max, min and res?
> 

No, it works the same way.
Only the default value changes (from 1 to 0)

 pan_speed 0x009a0920 (int)    : min=-1 max=1 step=1 default=0 value=0
tilt_speed 0x009a0921 (int)    : min=-1 max=1 step=1 default=0 value=0



> 
> Thanks!
> 
> Regards!
> 
> 

Thanks,
Gergo

> On Wed, 14 Jun 2023 at 15:13, Gergo Koteles <soyer@irl.hu> wrote:
> > 
> > The Logitech BCC950 incorrectly reports 1 (the max value)
> > for the default values of V4L2_CID_PAN_SPEED,
> > V4L2_CID_TILT_SPEED.
> > 
> > This patch sets them to 0, which is the stop value.
> > 
> > Previous discussion
> > Link:
> > https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/
> > 
> > Signed-off-by: Gergo Koteles <soyer@irl.hu>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 5e9d3da862dd..e131958c0930 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -444,9 +444,10 @@ static s32 uvc_ctrl_get_rel_speed(struct
> > uvc_control_mapping *mapping,
> >                 return -data[first+1];
> >         case UVC_GET_MAX:
> >         case UVC_GET_RES:
> > +               return data[first+1];
> >         case UVC_GET_DEF:
> >         default:
> > -               return data[first+1];
> > +               return 0;
> >         }
> >  }
> > 
> > 
> > base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
> > --
> > 2.40.1
> > 
> 
> 

Re: [PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Ricardo Ribalda]

Hi Soyer


On Wed, 14 Jun 2023 at 16:59, soyer <soyer@irl.hu> wrote:
>
> Hi Ricardo
>
> On Wed, 2023-06-14 at 16:19 +0200, Ricardo Ribalda wrote:
> > [Now in plain text mode]
> >
> > Hi Gergo
> >
> > Doesn't your patch affect pan and tilt for all the cameras, not only
> > the BCC950?
> >
> Yes, it affects all cameras that support CT_PANTILT_RELATIVE_CONTROL.
>
> > Also it seems that 1 means that device does not support programmable
> > speed. Is that correct?
> >
> > ```
> > The bPanSpeed field is used to specify the speed of the movement for
> > the Pan direction. A low
> > number indicates a slow speed and a high number indicates a higher
> > 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 for the Pan control, it will return the value 1
> > in this field for all these
> > requests.
> > ```

It seems to me that the module is compliant to the standard. It
returns 1 as GET_DEF because it does not support speed control.

Maybe you should ignore the speed control instead of changing its default value?


> >
> I started from the V4L2 control description.
>
> V4L2_CID_PAN_SPEED (integer)
> This control turns the camera horizontally at the specific speed. The
> unit is undefined. A positive value moves the camera to the right
> (clockwise when viewed from above), a negative value to the left. A
> value of zero stops the motion if one is in progress and has no effect
> otherwise.
>
> And this is why I thought that 1 is not a good default value, because
> it moves the camera.
> The other V4L2 controls have a default value that I can safely set the
> controls to.
>
> Are you using it to determine if the camera supports speed control?
>
> > When you program that value do you see any difference on the device?
> > What is max, min and res?
> >
>
> No, it works the same way.
> Only the default value changes (from 1 to 0)
>
>  pan_speed 0x009a0920 (int)    : min=-1 max=1 step=1 default=0 value=0
> tilt_speed 0x009a0921 (int)    : min=-1 max=1 step=1 default=0 value=0
>
>
>
> >
> > Thanks!
> >
> > Regards!
> >
> >
>
> Thanks,
> Gergo
>
> > On Wed, 14 Jun 2023 at 15:13, Gergo Koteles <soyer@irl.hu> wrote:
> > >
> > > The Logitech BCC950 incorrectly reports 1 (the max value)
> > > for the default values of V4L2_CID_PAN_SPEED,
> > > V4L2_CID_TILT_SPEED.
> > >
> > > This patch sets them to 0, which is the stop value.
> > >
> > > Previous discussion
> > > Link:
> > > https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/
> > >
> > > Signed-off-by: Gergo Koteles <soyer@irl.hu>
> > > ---
> > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > > b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 5e9d3da862dd..e131958c0930 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -444,9 +444,10 @@ static s32 uvc_ctrl_get_rel_speed(struct
> > > uvc_control_mapping *mapping,
> > >                 return -data[first+1];
> > >         case UVC_GET_MAX:
> > >         case UVC_GET_RES:
> > > +               return data[first+1];
> > >         case UVC_GET_DEF:
> > >         default:
> > > -               return data[first+1];
> > > +               return 0;
> > >         }
> > >  }
> > >
> > >
> > > base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
> > > --
> > > 2.40.1
> > >
> >
> >
>


-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Ricardo Ribalda]

On Wed, 14 Jun 2023 at 17:07, Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Hi Soyer
>
>
> On Wed, 14 Jun 2023 at 16:59, soyer <soyer@irl.hu> wrote:
> >
> > Hi Ricardo
> >
> > On Wed, 2023-06-14 at 16:19 +0200, Ricardo Ribalda wrote:
> > > [Now in plain text mode]
> > >
> > > Hi Gergo
> > >
> > > Doesn't your patch affect pan and tilt for all the cameras, not only
> > > the BCC950?
> > >
> > Yes, it affects all cameras that support CT_PANTILT_RELATIVE_CONTROL.
> >
> > > Also it seems that 1 means that device does not support programmable
> > > speed. Is that correct?
> > >
> > > ```
> > > The bPanSpeed field is used to specify the speed of the movement for
> > > the Pan direction. A low
> > > number indicates a slow speed and a high number indicates a higher
> > > 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 for the Pan control, it will return the value 1
> > > in this field for all these
> > > requests.
> > > ```
>
> It seems to me that the module is compliant to the standard. It
> returns 1 as GET_DEF because it does not support speed control.
>
> Maybe you should ignore the speed control instead of changing its default value?

( this is the standard I am refering to: 4.2.2.1.15 PanTilt (Relative) Control

 https://www.usb.org/document-library/video-class-v15-document-set )
>
>
> > >
> > I started from the V4L2 control description.
> >
> > V4L2_CID_PAN_SPEED (integer)
> > This control turns the camera horizontally at the specific speed. The
> > unit is undefined. A positive value moves the camera to the right
> > (clockwise when viewed from above), a negative value to the left. A
> > value of zero stops the motion if one is in progress and has no effect
> > otherwise.
> >
> > And this is why I thought that 1 is not a good default value, because
> > it moves the camera.
> > The other V4L2 controls have a default value that I can safely set the
> > controls to.
> >
> > Are you using it to determine if the camera supports speed control?
> >
> > > When you program that value do you see any difference on the device?
> > > What is max, min and res?
> > >
> >
> > No, it works the same way.
> > Only the default value changes (from 1 to 0)
> >
> >  pan_speed 0x009a0920 (int)    : min=-1 max=1 step=1 default=0 value=0
> > tilt_speed 0x009a0921 (int)    : min=-1 max=1 step=1 default=0 value=0
> >
> >
> >
> > >
> > > Thanks!
> > >
> > > Regards!
> > >
> > >
> >
> > Thanks,
> > Gergo
> >
> > > On Wed, 14 Jun 2023 at 15:13, Gergo Koteles <soyer@irl.hu> wrote:
> > > >
> > > > The Logitech BCC950 incorrectly reports 1 (the max value)
> > > > for the default values of V4L2_CID_PAN_SPEED,
> > > > V4L2_CID_TILT_SPEED.
> > > >
> > > > This patch sets them to 0, which is the stop value.
> > > >
> > > > Previous discussion
> > > > Link:
> > > > https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/
> > > >
> > > > Signed-off-by: Gergo Koteles <soyer@irl.hu>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > index 5e9d3da862dd..e131958c0930 100644
> > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > @@ -444,9 +444,10 @@ static s32 uvc_ctrl_get_rel_speed(struct
> > > > uvc_control_mapping *mapping,
> > > >                 return -data[first+1];
> > > >         case UVC_GET_MAX:
> > > >         case UVC_GET_RES:
> > > > +               return data[first+1];
> > > >         case UVC_GET_DEF:
> > > >         default:
> > > > -               return data[first+1];
> > > > +               return 0;
> > > >         }
> > > >  }
> > > >
> > > >
> > > > base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
> > > > --
> > > > 2.40.1
> > > >
> > >
> > >
> >
>
>
> --
> Ricardo Ribalda



-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Gergo Koteles]

Hi Ricardo,
On Wed, 2023-06-14 at 17:08 +0200, Ricardo Ribalda wrote:
> On Wed, 14 Jun 2023 at 17:07, Ricardo Ribalda <ribalda@chromium.org>
> wrote:
> > 
> > Hi Soyer
> > 
> > 
> > On Wed, 14 Jun 2023 at 16:59, soyer <soyer@irl.hu> wrote:
> > > 
> > > Hi Ricardo
> > > 
> > > On Wed, 2023-06-14 at 16:19 +0200, Ricardo Ribalda wrote:
> > > > [Now in plain text mode]
> > > > 
> > > > Hi Gergo
> > > > 
> > > > Doesn't your patch affect pan and tilt for all the cameras, not
> > > > only
> > > > the BCC950?
> > > > 
> > > Yes, it affects all cameras that support
> > > CT_PANTILT_RELATIVE_CONTROL.
> > > 
> > > > Also it seems that 1 means that device does not support
> > > > programmable
> > > > speed. Is that correct?
> > > > 
> > > > ```
> > > > The bPanSpeed field is used to specify the speed of the
> > > > movement for
> > > > the Pan direction. A low
> > > > number indicates a slow speed and a high number indicates a
> > > > higher
> > > > 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 for the Pan control, it will return the
> > > > value 1
> > > > in this field for all these
> > > > requests.
> > > > ```
> > 
> > It seems to me that the module is compliant to the standard. It
> > returns 1 as GET_DEF because it does not support speed control.
> > 
> > Maybe you should ignore the speed control instead of changing its
> > default value?
> 
> ( this is the standard I am refering to: 4.2.2.1.15 PanTilt
> (Relative) Control
> 
>  https://www.usb.org/document-library/video-class-v15-document-set )
> > 
> > 

It's a different API. V4L2 control values are not the same as the UVC
standard control values.

Eg the V4L2_CID_PAN_SPEED control value calculated from 
CT_PANTILT_RELATIVE_CONTROL's bPanRelative and bPanSpeed value.

It only bothers me that I have to handle these two controls
differently.

> > > > 
> > > I started from the V4L2 control description.
> > > 
> > > V4L2_CID_PAN_SPEED (integer)
> > > This control turns the camera horizontally at the specific speed.
> > > The
> > > unit is undefined. A positive value moves the camera to the right
> > > (clockwise when viewed from above), a negative value to the left.
> > > A
> > > value of zero stops the motion if one is in progress and has no
> > > effect
> > > otherwise.
> > > 
> > > And this is why I thought that 1 is not a good default value,
> > > because
> > > it moves the camera.
> > > The other V4L2 controls have a default value that I can safely
> > > set the
> > > controls to.
> > > 
> > > Are you using it to determine if the camera supports speed
> > > control?
> > > 
> > > > When you program that value do you see any difference on the
> > > > device?
> > > > What is max, min and res?
> > > > 
> > > 
> > > No, it works the same way.
> > > Only the default value changes (from 1 to 0)
> > > 
> > >  pan_speed 0x009a0920 (int)    : min=-1 max=1 step=1 default=0
> > > value=0
> > > tilt_speed 0x009a0921 (int)    : min=-1 max=1 step=1 default=0
> > > value=0
> > > 
> > > 
> > > 
> > > > 
> > > > Thanks!
> > > > 
> > > > Regards!
> > > > 
> > > > 
> > > 
> > > Thanks,
> > > Gergo
> > > 
> > > > On Wed, 14 Jun 2023 at 15:13, Gergo Koteles <soyer@irl.hu>
> > > > wrote:
> > > > > 
> > > > > The Logitech BCC950 incorrectly reports 1 (the max value)
> > > > > for the default values of V4L2_CID_PAN_SPEED,
> > > > > V4L2_CID_TILT_SPEED.
> > > > > 
> > > > > This patch sets them to 0, which is the stop value.
> > > > > 
> > > > > Previous discussion
> > > > > Link:
> > > > > https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/
> > > > > 
> > > > > Signed-off-by: Gergo Koteles <soyer@irl.hu>
> > > > > ---
> > > > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > index 5e9d3da862dd..e131958c0930 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > @@ -444,9 +444,10 @@ static s32 uvc_ctrl_get_rel_speed(struct
> > > > > uvc_control_mapping *mapping,
> > > > >                 return -data[first+1];
> > > > >         case UVC_GET_MAX:
> > > > >         case UVC_GET_RES:
> > > > > +               return data[first+1];
> > > > >         case UVC_GET_DEF:
> > > > >         default:
> > > > > -               return data[first+1];
> > > > > +               return 0;
> > > > >         }
> > > > >  }
> > > > > 
> > > > > 
> > > > > base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
> > > > > --
> > > > > 2.40.1
> > > > > 
> > > > 
> > > > 
> > > 
> > 
> > 
> > --
> > Ricardo Ribalda
> 
> 
> 

Re: [PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Ricardo Ribalda]

On Wed, 14 Jun 2023 at 17:22, Gergo Koteles <soyer@irl.hu> wrote:
>
> Hi Ricardo,
> On Wed, 2023-06-14 at 17:08 +0200, Ricardo Ribalda wrote:
> > On Wed, 14 Jun 2023 at 17:07, Ricardo Ribalda <ribalda@chromium.org>
> > wrote:
> > >
> > > Hi Soyer
> > >
> > >
> > > On Wed, 14 Jun 2023 at 16:59, soyer <soyer@irl.hu> wrote:
> > > >
> > > > Hi Ricardo
> > > >
> > > > On Wed, 2023-06-14 at 16:19 +0200, Ricardo Ribalda wrote:
> > > > > [Now in plain text mode]
> > > > >
> > > > > Hi Gergo
> > > > >
> > > > > Doesn't your patch affect pan and tilt for all the cameras, not
> > > > > only
> > > > > the BCC950?
> > > > >
> > > > Yes, it affects all cameras that support
> > > > CT_PANTILT_RELATIVE_CONTROL.
> > > >
> > > > > Also it seems that 1 means that device does not support
> > > > > programmable
> > > > > speed. Is that correct?
> > > > >
> > > > > ```
> > > > > The bPanSpeed field is used to specify the speed of the
> > > > > movement for
> > > > > the Pan direction. A low
> > > > > number indicates a slow speed and a high number indicates a
> > > > > higher
> > > > > 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 for the Pan control, it will return the
> > > > > value 1
> > > > > in this field for all these
> > > > > requests.
> > > > > ```
> > >
> > > It seems to me that the module is compliant to the standard. It
> > > returns 1 as GET_DEF because it does not support speed control.
> > >
> > > Maybe you should ignore the speed control instead of changing its
> > > default value?
> >
> > ( this is the standard I am refering to: 4.2.2.1.15 PanTilt
> > (Relative) Control
> >
> >  https://www.usb.org/document-library/video-class-v15-document-set )
> > >
> > >
>
> It's a different API. V4L2 control values are not the same as the UVC
> standard control values.

What I am saying, is that if
CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF is 1 you should not
create the mapping to
V4L2_CID_PAN_SPEED

>
> Eg the V4L2_CID_PAN_SPEED control value calculated from
> CT_PANTILT_RELATIVE_CONTROL's bPanRelative and bPanSpeed value.
>
> It only bothers me that I have to handle these two controls
> differently.
>
> > > > >
> > > > I started from the V4L2 control description.
> > > >
> > > > V4L2_CID_PAN_SPEED (integer)
> > > > This control turns the camera horizontally at the specific speed.
> > > > The
> > > > unit is undefined. A positive value moves the camera to the right
> > > > (clockwise when viewed from above), a negative value to the left.
> > > > A
> > > > value of zero stops the motion if one is in progress and has no
> > > > effect
> > > > otherwise.
> > > >
> > > > And this is why I thought that 1 is not a good default value,
> > > > because
> > > > it moves the camera.
> > > > The other V4L2 controls have a default value that I can safely
> > > > set the
> > > > controls to.
> > > >
> > > > Are you using it to determine if the camera supports speed
> > > > control?
> > > >
> > > > > When you program that value do you see any difference on the
> > > > > device?
> > > > > What is max, min and res?
> > > > >
> > > >
> > > > No, it works the same way.
> > > > Only the default value changes (from 1 to 0)
> > > >
> > > >  pan_speed 0x009a0920 (int)    : min=-1 max=1 step=1 default=0
> > > > value=0
> > > > tilt_speed 0x009a0921 (int)    : min=-1 max=1 step=1 default=0
> > > > value=0
> > > >
> > > >
> > > >
> > > > >
> > > > > Thanks!
> > > > >
> > > > > Regards!
> > > > >
> > > > >
> > > >
> > > > Thanks,
> > > > Gergo
> > > >
> > > > > On Wed, 14 Jun 2023 at 15:13, Gergo Koteles <soyer@irl.hu>
> > > > > wrote:
> > > > > >
> > > > > > The Logitech BCC950 incorrectly reports 1 (the max value)
> > > > > > for the default values of V4L2_CID_PAN_SPEED,
> > > > > > V4L2_CID_TILT_SPEED.
> > > > > >
> > > > > > This patch sets them to 0, which is the stop value.
> > > > > >
> > > > > > Previous discussion
> > > > > > Link:
> > > > > > https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/
> > > > > >
> > > > > > Signed-off-by: Gergo Koteles <soyer@irl.hu>
> > > > > > ---
> > > > > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
> > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > index 5e9d3da862dd..e131958c0930 100644
> > > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > @@ -444,9 +444,10 @@ static s32 uvc_ctrl_get_rel_speed(struct
> > > > > > uvc_control_mapping *mapping,
> > > > > >                 return -data[first+1];
> > > > > >         case UVC_GET_MAX:
> > > > > >         case UVC_GET_RES:
> > > > > > +               return data[first+1];
> > > > > >         case UVC_GET_DEF:
> > > > > >         default:
> > > > > > -               return data[first+1];
> > > > > > +               return 0;
> > > > > >         }
> > > > > >  }
> > > > > >
> > > > > >
> > > > > > base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
> > > > > > --
> > > > > > 2.40.1
> > > > > >
> > > > >
> > > > >
> > > >
> > >
> > >
> > > --
> > > Ricardo Ribalda
> >
> >
> >
>


-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Gergo Koteles]

Hi Ricardo,
On Wed, 2023-06-14 at 17:25 +0200, Ricardo Ribalda wrote:
> On Wed, 14 Jun 2023 at 17:22, Gergo Koteles <soyer@irl.hu> wrote:
> > 
> > Hi Ricardo,
> > On Wed, 2023-06-14 at 17:08 +0200, Ricardo Ribalda wrote:
> > > On Wed, 14 Jun 2023 at 17:07, Ricardo Ribalda
> > > <ribalda@chromium.org>
> > > wrote:
> > > > 
> > > > Hi Soyer
> > > > 
> > > > 
> > > > On Wed, 14 Jun 2023 at 16:59, soyer <soyer@irl.hu> wrote:
> > > > > 
> > > > > Hi Ricardo
> > > > > 
> > > > > On Wed, 2023-06-14 at 16:19 +0200, Ricardo Ribalda wrote:
> > > > > > [Now in plain text mode]
> > > > > > 
> > > > > > Hi Gergo
> > > > > > 
> > > > > > Doesn't your patch affect pan and tilt for all the cameras,
> > > > > > not
> > > > > > only
> > > > > > the BCC950?
> > > > > > 
> > > > > Yes, it affects all cameras that support
> > > > > CT_PANTILT_RELATIVE_CONTROL.
> > > > > 
> > > > > > Also it seems that 1 means that device does not support
> > > > > > programmable
> > > > > > speed. Is that correct?
> > > > > > 
> > > > > > ```
> > > > > > The bPanSpeed field is used to specify the speed of the
> > > > > > movement for
> > > > > > the Pan direction. A low
> > > > > > number indicates a slow speed and a high number indicates a
> > > > > > higher
> > > > > > 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 for the Pan control, it will return
> > > > > > the
> > > > > > value 1
> > > > > > in this field for all these
> > > > > > requests.
> > > > > > ```
> > > > 
> > > > It seems to me that the module is compliant to the standard. It
> > > > returns 1 as GET_DEF because it does not support speed control.
> > > > 
> > > > Maybe you should ignore the speed control instead of changing
> > > > its
> > > > default value?
> > > 
> > > ( this is the standard I am refering to: 4.2.2.1.15 PanTilt
> > > (Relative) Control
> > > 
> > >  
> > > https://www.usb.org/document-library/video-class-v15-document-set 
> > > )
> > > > 
> > > > 
> > 
> > It's a different API. V4L2 control values are not the same as the
> > UVC
> > standard control values.
> 
> What I am saying, is that if
> CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF is 1 you should not
> create the mapping to
> V4L2_CID_PAN_SPEED
> 
If I set V4L2_CID_PAN_SPEED to 1 (max), the BCC950 starts to move at
maximum speed. This is what it should do according to description of
the V4L2_CID_PAN_SPEED.

My understanding is that, if the camera supports
CT_PANTILT_RELATIVE_CONTROL there should be a V4L2_CID_PAN_SPEED.
CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF == 1 only says that only
one speed is available not a range.



> > 
> > Eg the V4L2_CID_PAN_SPEED control value calculated from
> > CT_PANTILT_RELATIVE_CONTROL's bPanRelative and bPanSpeed value.
> > 
> > It only bothers me that I have to handle these two controls
> > differently.
> > 
> > > > > > 
> > > > > I started from the V4L2 control description.
> > > > > 
> > > > > V4L2_CID_PAN_SPEED (integer)
> > > > > This control turns the camera horizontally at the specific
> > > > > speed.
> > > > > The
> > > > > unit is undefined. A positive value moves the camera to the
> > > > > right
> > > > > (clockwise when viewed from above), a negative value to the
> > > > > left.
> > > > > A
> > > > > value of zero stops the motion if one is in progress and has
> > > > > no
> > > > > effect
> > > > > otherwise.
> > > > > 
> > > > > And this is why I thought that 1 is not a good default value,
> > > > > because
> > > > > it moves the camera.
> > > > > The other V4L2 controls have a default value that I can
> > > > > safely
> > > > > set the
> > > > > controls to.
> > > > > 
> > > > > Are you using it to determine if the camera supports speed
> > > > > control?
> > > > > 
> > > > > > When you program that value do you see any difference on
> > > > > > the
> > > > > > device?
> > > > > > What is max, min and res?
> > > > > > 
> > > > > 
> > > > > No, it works the same way.
> > > > > Only the default value changes (from 1 to 0)
> > > > > 
> > > > >  pan_speed 0x009a0920 (int)    : min=-1 max=1 step=1
> > > > > default=0
> > > > > value=0
> > > > > tilt_speed 0x009a0921 (int)    : min=-1 max=1 step=1
> > > > > default=0
> > > > > value=0
> > > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > Thanks!
> > > > > > 
> > > > > > Regards!
> > > > > > 
> > > > > > 
> > > > > 
> > > > > Thanks,
> > > > > Gergo
> > > > > 
> > > > > > On Wed, 14 Jun 2023 at 15:13, Gergo Koteles <soyer@irl.hu>
> > > > > > wrote:
> > > > > > > 
> > > > > > > The Logitech BCC950 incorrectly reports 1 (the max value)
> > > > > > > for the default values of V4L2_CID_PAN_SPEED,
> > > > > > > V4L2_CID_TILT_SPEED.
> > > > > > > 
> > > > > > > This patch sets them to 0, which is the stop value.
> > > > > > > 
> > > > > > > Previous discussion
> > > > > > > Link:
> > > > > > > https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/
> > > > > > > 
> > > > > > > Signed-off-by: Gergo Koteles <soyer@irl.hu>
> > > > > > > ---
> > > > > > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
> > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > index 5e9d3da862dd..e131958c0930 100644
> > > > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > @@ -444,9 +444,10 @@ static s32
> > > > > > > uvc_ctrl_get_rel_speed(struct
> > > > > > > uvc_control_mapping *mapping,
> > > > > > >                 return -data[first+1];
> > > > > > >         case UVC_GET_MAX:
> > > > > > >         case UVC_GET_RES:
> > > > > > > +               return data[first+1];
> > > > > > >         case UVC_GET_DEF:
> > > > > > >         default:
> > > > > > > -               return data[first+1];
> > > > > > > +               return 0;
> > > > > > >         }
> > > > > > >  }
> > > > > > > 
> > > > > > > 
> > > > > > > base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
> > > > > > > --
> > > > > > > 2.40.1
> > > > > > > 
> > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > > 
> > > > --
> > > > Ricardo Ribalda
> > > 
> > > 
> > > 
> > 
> 
> 

Re: [PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Ricardo Ribalda]

Hi Gergo



On Wed, 14 Jun 2023 at 17:46, Gergo Koteles <soyer@irl.hu> wrote:
>
> Hi Ricardo,
> On Wed, 2023-06-14 at 17:25 +0200, Ricardo Ribalda wrote:
> > On Wed, 14 Jun 2023 at 17:22, Gergo Koteles <soyer@irl.hu> wrote:
> > >
> > > Hi Ricardo,
> > > On Wed, 2023-06-14 at 17:08 +0200, Ricardo Ribalda wrote:
> > > > On Wed, 14 Jun 2023 at 17:07, Ricardo Ribalda
> > > > <ribalda@chromium.org>
> > > > wrote:
> > > > >
> > > > > Hi Soyer
> > > > >
> > > > >
> > > > > On Wed, 14 Jun 2023 at 16:59, soyer <soyer@irl.hu> wrote:
> > > > > >
> > > > > > Hi Ricardo
> > > > > >
> > > > > > On Wed, 2023-06-14 at 16:19 +0200, Ricardo Ribalda wrote:
> > > > > > > [Now in plain text mode]
> > > > > > >
> > > > > > > Hi Gergo
> > > > > > >
> > > > > > > Doesn't your patch affect pan and tilt for all the cameras,
> > > > > > > not
> > > > > > > only
> > > > > > > the BCC950?
> > > > > > >
> > > > > > Yes, it affects all cameras that support
> > > > > > CT_PANTILT_RELATIVE_CONTROL.
> > > > > >
> > > > > > > Also it seems that 1 means that device does not support
> > > > > > > programmable
> > > > > > > speed. Is that correct?
> > > > > > >
> > > > > > > ```
> > > > > > > The bPanSpeed field is used to specify the speed of the
> > > > > > > movement for
> > > > > > > the Pan direction. A low
> > > > > > > number indicates a slow speed and a high number indicates a
> > > > > > > higher
> > > > > > > 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 for the Pan control, it will return
> > > > > > > the
> > > > > > > value 1
> > > > > > > in this field for all these
> > > > > > > requests.
> > > > > > > ```
> > > > >
> > > > > It seems to me that the module is compliant to the standard. It
> > > > > returns 1 as GET_DEF because it does not support speed control.
> > > > >
> > > > > Maybe you should ignore the speed control instead of changing
> > > > > its
> > > > > default value?
> > > >
> > > > ( this is the standard I am refering to: 4.2.2.1.15 PanTilt
> > > > (Relative) Control
> > > >
> > > >
> > > > https://www.usb.org/document-library/video-class-v15-document-set
> > > > )
> > > > >
> > > > >
> > >
> > > It's a different API. V4L2 control values are not the same as the
> > > UVC
> > > standard control values.
> >
> > What I am saying, is that if
> > CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF is 1 you should not
> > create the mapping to
> > V4L2_CID_PAN_SPEED
> >
> If I set V4L2_CID_PAN_SPEED to 1 (max), the BCC950 starts to move at
> maximum speed. This is what it should do according to description of
> the V4L2_CID_PAN_SPEED.
>
> My understanding is that, if the camera supports
> CT_PANTILT_RELATIVE_CONTROL there should be a V4L2_CID_PAN_SPEED.
> CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF == 1 only says that only
> one speed is available not a range.

I think I got confused by the names.

Can you check if this works?

ribalda@alco:~/work/linux$ git diff
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 5e9d3da862dd..abab2f4efc94 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -437,6 +437,7 @@ static s32 uvc_ctrl_get_rel_speed(struct
uvc_control_mapping *mapping,
        s8 rel = (s8)data[first];

        switch (query) {
+       case UVC_GET_DEF:
        case UVC_GET_CUR:
                return (rel == 0) ? 0 : (rel > 0 ? data[first+1]
                                                 : -data[first+1]);
@@ -444,7 +445,6 @@ static s32 uvc_ctrl_get_rel_speed(struct
uvc_control_mapping *mapping,
                return -data[first+1];
        case UVC_GET_MAX:
        case UVC_GET_RES:
-       case UVC_GET_DEF:
        default:
                return data[first+1];
        }
ribalda@alco:~/work/linux$


>
>
>
> > >
> > > Eg the V4L2_CID_PAN_SPEED control value calculated from
> > > CT_PANTILT_RELATIVE_CONTROL's bPanRelative and bPanSpeed value.
> > >
> > > It only bothers me that I have to handle these two controls
> > > differently.
> > >
> > > > > > >
> > > > > > I started from the V4L2 control description.
> > > > > >
> > > > > > V4L2_CID_PAN_SPEED (integer)
> > > > > > This control turns the camera horizontally at the specific
> > > > > > speed.
> > > > > > The
> > > > > > unit is undefined. A positive value moves the camera to the
> > > > > > right
> > > > > > (clockwise when viewed from above), a negative value to the
> > > > > > left.
> > > > > > A
> > > > > > value of zero stops the motion if one is in progress and has
> > > > > > no
> > > > > > effect
> > > > > > otherwise.
> > > > > >
> > > > > > And this is why I thought that 1 is not a good default value,
> > > > > > because
> > > > > > it moves the camera.
> > > > > > The other V4L2 controls have a default value that I can
> > > > > > safely
> > > > > > set the
> > > > > > controls to.
> > > > > >
> > > > > > Are you using it to determine if the camera supports speed
> > > > > > control?
> > > > > >
> > > > > > > When you program that value do you see any difference on
> > > > > > > the
> > > > > > > device?
> > > > > > > What is max, min and res?
> > > > > > >
> > > > > >
> > > > > > No, it works the same way.
> > > > > > Only the default value changes (from 1 to 0)
> > > > > >
> > > > > >  pan_speed 0x009a0920 (int)    : min=-1 max=1 step=1
> > > > > > default=0
> > > > > > value=0
> > > > > > tilt_speed 0x009a0921 (int)    : min=-1 max=1 step=1
> > > > > > default=0
> > > > > > value=0
> > > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > Thanks!
> > > > > > >
> > > > > > > Regards!
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Gergo
> > > > > >
> > > > > > > On Wed, 14 Jun 2023 at 15:13, Gergo Koteles <soyer@irl.hu>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > The Logitech BCC950 incorrectly reports 1 (the max value)
> > > > > > > > for the default values of V4L2_CID_PAN_SPEED,
> > > > > > > > V4L2_CID_TILT_SPEED.
> > > > > > > >
> > > > > > > > This patch sets them to 0, which is the stop value.
> > > > > > > >
> > > > > > > > Previous discussion
> > > > > > > > Link:
> > > > > > > > https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/
> > > > > > > >
> > > > > > > > Signed-off-by: Gergo Koteles <soyer@irl.hu>
> > > > > > > > ---
> > > > > > > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
> > > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > index 5e9d3da862dd..e131958c0930 100644
> > > > > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > @@ -444,9 +444,10 @@ static s32
> > > > > > > > uvc_ctrl_get_rel_speed(struct
> > > > > > > > uvc_control_mapping *mapping,
> > > > > > > >                 return -data[first+1];
> > > > > > > >         case UVC_GET_MAX:
> > > > > > > >         case UVC_GET_RES:
> > > > > > > > +               return data[first+1];
> > > > > > > >         case UVC_GET_DEF:
> > > > > > > >         default:
> > > > > > > > -               return data[first+1];
> > > > > > > > +               return 0;
> > > > > > > >         }
> > > > > > > >  }
> > > > > > > >
> > > > > > > >
> > > > > > > > base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
> > > > > > > > --
> > > > > > > > 2.40.1
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Ricardo Ribalda
> > > >
> > > >
> > > >
> > >
> >
> >
>


--
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Gergo Koteles]

Hi Ricardo,

On Wed, 2023-06-14 at 18:29 +0200, Ricardo Ribalda wrote:
> Hi Gergo
> 
> 
> 
> On Wed, 14 Jun 2023 at 17:46, Gergo Koteles <soyer@irl.hu> wrote:
> > 
> > Hi Ricardo,
> > On Wed, 2023-06-14 at 17:25 +0200, Ricardo Ribalda wrote:
> > > On Wed, 14 Jun 2023 at 17:22, Gergo Koteles <soyer@irl.hu> wrote:
> > > > 
> > > > Hi Ricardo,
> > > > On Wed, 2023-06-14 at 17:08 +0200, Ricardo Ribalda wrote:
> > > > > On Wed, 14 Jun 2023 at 17:07, Ricardo Ribalda
> > > > > <ribalda@chromium.org>
> > > > > wrote:
> > > > > > 
> > > > > > Hi Soyer
> > > > > > 
> > > > > > 
> > > > > > On Wed, 14 Jun 2023 at 16:59, soyer <soyer@irl.hu> wrote:
> > > > > > > 
> > > > > > > Hi Ricardo
> > > > > > > 
> > > > > > > On Wed, 2023-06-14 at 16:19 +0200, Ricardo Ribalda wrote:
> > > > > > > > [Now in plain text mode]
> > > > > > > > 
> > > > > > > > Hi Gergo
> > > > > > > > 
> > > > > > > > Doesn't your patch affect pan and tilt for all the
> > > > > > > > cameras,
> > > > > > > > not
> > > > > > > > only
> > > > > > > > the BCC950?
> > > > > > > > 
> > > > > > > Yes, it affects all cameras that support
> > > > > > > CT_PANTILT_RELATIVE_CONTROL.
> > > > > > > 
> > > > > > > > Also it seems that 1 means that device does not support
> > > > > > > > programmable
> > > > > > > > speed. Is that correct?
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > The bPanSpeed field is used to specify the speed of the
> > > > > > > > movement for
> > > > > > > > the Pan direction. A low
> > > > > > > > number indicates a slow speed and a high number
> > > > > > > > indicates a
> > > > > > > > higher
> > > > > > > > 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 for the Pan control, it will
> > > > > > > > return
> > > > > > > > the
> > > > > > > > value 1
> > > > > > > > in this field for all these
> > > > > > > > requests.
> > > > > > > > ```
> > > > > > 
> > > > > > It seems to me that the module is compliant to the
> > > > > > standard. It
> > > > > > returns 1 as GET_DEF because it does not support speed
> > > > > > control.
> > > > > > 
> > > > > > Maybe you should ignore the speed control instead of
> > > > > > changing
> > > > > > its
> > > > > > default value?
> > > > > 
> > > > > ( this is the standard I am refering to: 4.2.2.1.15 PanTilt
> > > > > (Relative) Control
> > > > > 
> > > > > 
> > > > > https://www.usb.org/document-library/video-class-v15-document-set
> > > > > )
> > > > > > 
> > > > > > 
> > > > 
> > > > It's a different API. V4L2 control values are not the same as
> > > > the
> > > > UVC
> > > > standard control values.
> > > 
> > > What I am saying, is that if
> > > CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF is 1 you should not
> > > create the mapping to
> > > V4L2_CID_PAN_SPEED
> > > 
> > If I set V4L2_CID_PAN_SPEED to 1 (max), the BCC950 starts to move
> > at
> > maximum speed. This is what it should do according to description
> > of
> > the V4L2_CID_PAN_SPEED.
> > 
> > My understanding is that, if the camera supports
> > CT_PANTILT_RELATIVE_CONTROL there should be a V4L2_CID_PAN_SPEED.
> > CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF == 1 only says that
> > only
> > one speed is available not a range.
> 
> I think I got confused by the names.
> 
> Can you check if this works?
> 
> ribalda@alco:~/work/linux$ git diff
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> b/drivers/media/usb/uvc/uvc_ctrl.c
> index 5e9d3da862dd..abab2f4efc94 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -437,6 +437,7 @@ static s32 uvc_ctrl_get_rel_speed(struct
> uvc_control_mapping *mapping,
>         s8 rel = (s8)data[first];
> 
>         switch (query) {
> +       case UVC_GET_DEF:
>         case UVC_GET_CUR:
>                 return (rel == 0) ? 0 : (rel > 0 ? data[first+1]
>                                                  : -data[first+1]);
> @@ -444,7 +445,6 @@ static s32 uvc_ctrl_get_rel_speed(struct
> uvc_control_mapping *mapping,
>                 return -data[first+1];
>         case UVC_GET_MAX:
>         case UVC_GET_RES:
> -       case UVC_GET_DEF:
>         default:
>                 return data[first+1];
>         }
> ribalda@alco:~/work/linux$
> 
> 

It works this way also.
Should I send a new version?


> > 
> > 
> > 
> > > > 
> > > > Eg the V4L2_CID_PAN_SPEED control value calculated from
> > > > CT_PANTILT_RELATIVE_CONTROL's bPanRelative and bPanSpeed value.
> > > > 
> > > > It only bothers me that I have to handle these two controls
> > > > differently.
> > > > 
> > > > > > > > 
> > > > > > > I started from the V4L2 control description.
> > > > > > > 
> > > > > > > V4L2_CID_PAN_SPEED (integer)
> > > > > > > This control turns the camera horizontally at the
> > > > > > > specific
> > > > > > > speed.
> > > > > > > The
> > > > > > > unit is undefined. A positive value moves the camera to
> > > > > > > the
> > > > > > > right
> > > > > > > (clockwise when viewed from above), a negative value to
> > > > > > > the
> > > > > > > left.
> > > > > > > A
> > > > > > > value of zero stops the motion if one is in progress and
> > > > > > > has
> > > > > > > no
> > > > > > > effect
> > > > > > > otherwise.
> > > > > > > 
> > > > > > > And this is why I thought that 1 is not a good default
> > > > > > > value,
> > > > > > > because
> > > > > > > it moves the camera.
> > > > > > > The other V4L2 controls have a default value that I can
> > > > > > > safely
> > > > > > > set the
> > > > > > > controls to.
> > > > > > > 
> > > > > > > Are you using it to determine if the camera supports
> > > > > > > speed
> > > > > > > control?
> > > > > > > 
> > > > > > > > When you program that value do you see any difference
> > > > > > > > on
> > > > > > > > the
> > > > > > > > device?
> > > > > > > > What is max, min and res?
> > > > > > > > 
> > > > > > > 
> > > > > > > No, it works the same way.
> > > > > > > Only the default value changes (from 1 to 0)
> > > > > > > 
> > > > > > >  pan_speed 0x009a0920 (int)    : min=-1 max=1 step=1
> > > > > > > default=0
> > > > > > > value=0
> > > > > > > tilt_speed 0x009a0921 (int)    : min=-1 max=1 step=1
> > > > > > > default=0
> > > > > > > value=0
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > Thanks!
> > > > > > > > 
> > > > > > > > Regards!
> > > > > > > > 
> > > > > > > > 
> > > > > > > 
> > > > > > > Thanks,
> > > > > > > Gergo
> > > > > > > 
> > > > > > > > On Wed, 14 Jun 2023 at 15:13, Gergo Koteles
> > > > > > > > <soyer@irl.hu>
> > > > > > > > wrote:
> > > > > > > > > 
> > > > > > > > > The Logitech BCC950 incorrectly reports 1 (the max
> > > > > > > > > value)
> > > > > > > > > for the default values of V4L2_CID_PAN_SPEED,
> > > > > > > > > V4L2_CID_TILT_SPEED.
> > > > > > > > > 
> > > > > > > > > This patch sets them to 0, which is the stop value.
> > > > > > > > > 
> > > > > > > > > Previous discussion
> > > > > > > > > Link:
> > > > > > > > > https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Gergo Koteles <soyer@irl.hu>
> > > > > > > > > ---
> > > > > > > > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
> > > > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > index 5e9d3da862dd..e131958c0930 100644
> > > > > > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > @@ -444,9 +444,10 @@ static s32
> > > > > > > > > uvc_ctrl_get_rel_speed(struct
> > > > > > > > > uvc_control_mapping *mapping,
> > > > > > > > >                 return -data[first+1];
> > > > > > > > >         case UVC_GET_MAX:
> > > > > > > > >         case UVC_GET_RES:
> > > > > > > > > +               return data[first+1];
> > > > > > > > >         case UVC_GET_DEF:
> > > > > > > > >         default:
> > > > > > > > > -               return data[first+1];
> > > > > > > > > +               return 0;
> > > > > > > > >         }
> > > > > > > > >  }
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
> > > > > > > > > --
> > > > > > > > > 2.40.1
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > > 
> > > > > > --
> > > > > > Ricardo Ribalda
> > > > > 
> > > > > 
> > > > > 
> > > > 
> > > 
> > > 
> > 
> 
> 
> --
> Ricardo Ribalda

Re: [PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Ricardo Ribalda]

On Wed, 14 Jun 2023 at 18:56, Gergo Koteles <soyer@irl.hu> wrote:
>
> Hi Ricardo,
>
> On Wed, 2023-06-14 at 18:29 +0200, Ricardo Ribalda wrote:
> > Hi Gergo
> >
> >
> >
> > On Wed, 14 Jun 2023 at 17:46, Gergo Koteles <soyer@irl.hu> wrote:
> > >
> > > Hi Ricardo,
> > > On Wed, 2023-06-14 at 17:25 +0200, Ricardo Ribalda wrote:
> > > > On Wed, 14 Jun 2023 at 17:22, Gergo Koteles <soyer@irl.hu> wrote:
> > > > >
> > > > > Hi Ricardo,
> > > > > On Wed, 2023-06-14 at 17:08 +0200, Ricardo Ribalda wrote:
> > > > > > On Wed, 14 Jun 2023 at 17:07, Ricardo Ribalda
> > > > > > <ribalda@chromium.org>
> > > > > > wrote:
> > > > > > >
> > > > > > > Hi Soyer
> > > > > > >
> > > > > > >
> > > > > > > On Wed, 14 Jun 2023 at 16:59, soyer <soyer@irl.hu> wrote:
> > > > > > > >
> > > > > > > > Hi Ricardo
> > > > > > > >
> > > > > > > > On Wed, 2023-06-14 at 16:19 +0200, Ricardo Ribalda wrote:
> > > > > > > > > [Now in plain text mode]
> > > > > > > > >
> > > > > > > > > Hi Gergo
> > > > > > > > >
> > > > > > > > > Doesn't your patch affect pan and tilt for all the
> > > > > > > > > cameras,
> > > > > > > > > not
> > > > > > > > > only
> > > > > > > > > the BCC950?
> > > > > > > > >
> > > > > > > > Yes, it affects all cameras that support
> > > > > > > > CT_PANTILT_RELATIVE_CONTROL.
> > > > > > > >
> > > > > > > > > Also it seems that 1 means that device does not support
> > > > > > > > > programmable
> > > > > > > > > speed. Is that correct?
> > > > > > > > >
> > > > > > > > > ```
> > > > > > > > > The bPanSpeed field is used to specify the speed of the
> > > > > > > > > movement for
> > > > > > > > > the Pan direction. A low
> > > > > > > > > number indicates a slow speed and a high number
> > > > > > > > > indicates a
> > > > > > > > > higher
> > > > > > > > > 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 for the Pan control, it will
> > > > > > > > > return
> > > > > > > > > the
> > > > > > > > > value 1
> > > > > > > > > in this field for all these
> > > > > > > > > requests.
> > > > > > > > > ```
> > > > > > >
> > > > > > > It seems to me that the module is compliant to the
> > > > > > > standard. It
> > > > > > > returns 1 as GET_DEF because it does not support speed
> > > > > > > control.
> > > > > > >
> > > > > > > Maybe you should ignore the speed control instead of
> > > > > > > changing
> > > > > > > its
> > > > > > > default value?
> > > > > >
> > > > > > ( this is the standard I am refering to: 4.2.2.1.15 PanTilt
> > > > > > (Relative) Control
> > > > > >
> > > > > >
> > > > > > https://www.usb.org/document-library/video-class-v15-document-set
> > > > > > )
> > > > > > >
> > > > > > >
> > > > >
> > > > > It's a different API. V4L2 control values are not the same as
> > > > > the
> > > > > UVC
> > > > > standard control values.
> > > >
> > > > What I am saying, is that if
> > > > CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF is 1 you should not
> > > > create the mapping to
> > > > V4L2_CID_PAN_SPEED
> > > >
> > > If I set V4L2_CID_PAN_SPEED to 1 (max), the BCC950 starts to move
> > > at
> > > maximum speed. This is what it should do according to description
> > > of
> > > the V4L2_CID_PAN_SPEED.
> > >
> > > My understanding is that, if the camera supports
> > > CT_PANTILT_RELATIVE_CONTROL there should be a V4L2_CID_PAN_SPEED.
> > > CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF == 1 only says that
> > > only
> > > one speed is available not a range.
> >
> > I think I got confused by the names.
> >
> > Can you check if this works?
> >
> > ribalda@alco:~/work/linux$ git diff
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 5e9d3da862dd..abab2f4efc94 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -437,6 +437,7 @@ static s32 uvc_ctrl_get_rel_speed(struct
> > uvc_control_mapping *mapping,
> >         s8 rel = (s8)data[first];
> >
> >         switch (query) {
> > +       case UVC_GET_DEF:
> >         case UVC_GET_CUR:
> >                 return (rel == 0) ? 0 : (rel > 0 ? data[first+1]
> >                                                  : -data[first+1]);
> > @@ -444,7 +445,6 @@ static s32 uvc_ctrl_get_rel_speed(struct
> > uvc_control_mapping *mapping,
> >                 return -data[first+1];
> >         case UVC_GET_MAX:
> >         case UVC_GET_RES:
> > -       case UVC_GET_DEF:
> >         default:
> >                 return data[first+1];
> >         }
> > ribalda@alco:~/work/linux$
> >
> >
>
> It works this way also.
> Should I send a new version?

I believe this is slightly better... but it is up to Laurent ;)

Thanks!

>
>
> > >
> > >
> > >
> > > > >
> > > > > Eg the V4L2_CID_PAN_SPEED control value calculated from
> > > > > CT_PANTILT_RELATIVE_CONTROL's bPanRelative and bPanSpeed value.
> > > > >
> > > > > It only bothers me that I have to handle these two controls
> > > > > differently.
> > > > >
> > > > > > > > >
> > > > > > > > I started from the V4L2 control description.
> > > > > > > >
> > > > > > > > V4L2_CID_PAN_SPEED (integer)
> > > > > > > > This control turns the camera horizontally at the
> > > > > > > > specific
> > > > > > > > speed.
> > > > > > > > The
> > > > > > > > unit is undefined. A positive value moves the camera to
> > > > > > > > the
> > > > > > > > right
> > > > > > > > (clockwise when viewed from above), a negative value to
> > > > > > > > the
> > > > > > > > left.
> > > > > > > > A
> > > > > > > > value of zero stops the motion if one is in progress and
> > > > > > > > has
> > > > > > > > no
> > > > > > > > effect
> > > > > > > > otherwise.
> > > > > > > >
> > > > > > > > And this is why I thought that 1 is not a good default
> > > > > > > > value,
> > > > > > > > because
> > > > > > > > it moves the camera.
> > > > > > > > The other V4L2 controls have a default value that I can
> > > > > > > > safely
> > > > > > > > set the
> > > > > > > > controls to.
> > > > > > > >
> > > > > > > > Are you using it to determine if the camera supports
> > > > > > > > speed
> > > > > > > > control?
> > > > > > > >
> > > > > > > > > When you program that value do you see any difference
> > > > > > > > > on
> > > > > > > > > the
> > > > > > > > > device?
> > > > > > > > > What is max, min and res?
> > > > > > > > >
> > > > > > > >
> > > > > > > > No, it works the same way.
> > > > > > > > Only the default value changes (from 1 to 0)
> > > > > > > >
> > > > > > > >  pan_speed 0x009a0920 (int)    : min=-1 max=1 step=1
> > > > > > > > default=0
> > > > > > > > value=0
> > > > > > > > tilt_speed 0x009a0921 (int)    : min=-1 max=1 step=1
> > > > > > > > default=0
> > > > > > > > value=0
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks!
> > > > > > > > >
> > > > > > > > > Regards!
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Gergo
> > > > > > > >
> > > > > > > > > On Wed, 14 Jun 2023 at 15:13, Gergo Koteles
> > > > > > > > > <soyer@irl.hu>
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > The Logitech BCC950 incorrectly reports 1 (the max
> > > > > > > > > > value)
> > > > > > > > > > for the default values of V4L2_CID_PAN_SPEED,
> > > > > > > > > > V4L2_CID_TILT_SPEED.
> > > > > > > > > >
> > > > > > > > > > This patch sets them to 0, which is the stop value.
> > > > > > > > > >
> > > > > > > > > > Previous discussion
> > > > > > > > > > Link:
> > > > > > > > > > https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Gergo Koteles <soyer@irl.hu>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
> > > > > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > index 5e9d3da862dd..e131958c0930 100644
> > > > > > > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > @@ -444,9 +444,10 @@ static s32
> > > > > > > > > > uvc_ctrl_get_rel_speed(struct
> > > > > > > > > > uvc_control_mapping *mapping,
> > > > > > > > > >                 return -data[first+1];
> > > > > > > > > >         case UVC_GET_MAX:
> > > > > > > > > >         case UVC_GET_RES:
> > > > > > > > > > +               return data[first+1];
> > > > > > > > > >         case UVC_GET_DEF:
> > > > > > > > > >         default:
> > > > > > > > > > -               return data[first+1];
> > > > > > > > > > +               return 0;
> > > > > > > > > >         }
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
> > > > > > > > > > --
> > > > > > > > > > 2.40.1
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Ricardo Ribalda
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > >
> >
> >
> > --
> > Ricardo Ribalda
>


-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Laurent Pinchart]

Hi Ricardo,

On Wed, Jun 14, 2023 at 10:15:43PM +0200, Ricardo Ribalda wrote:
> On Wed, 14 Jun 2023 at 18:56, Gergo Koteles <soyer@irl.hu> wrote:
> > On Wed, 2023-06-14 at 18:29 +0200, Ricardo Ribalda wrote:
> > > On Wed, 14 Jun 2023 at 17:46, Gergo Koteles <soyer@irl.hu> wrote:
> > > > On Wed, 2023-06-14 at 17:25 +0200, Ricardo Ribalda wrote:
> > > > > On Wed, 14 Jun 2023 at 17:22, Gergo Koteles <soyer@irl.hu> wrote:
> > > > > > On Wed, 2023-06-14 at 17:08 +0200, Ricardo Ribalda wrote:
> > > > > > > On Wed, 14 Jun 2023 at 17:07, Ricardo Ribalda <ribalda@chromium.org> wrote:
> > > > > > > > On Wed, 14 Jun 2023 at 16:59, soyer <soyer@irl.hu> wrote:
> > > > > > > > > On Wed, 2023-06-14 at 16:19 +0200, Ricardo Ribalda wrote:
> > > > > > > > > > [Now in plain text mode]
> > > > > > > > > >
> > > > > > > > > > Hi Gergo
> > > > > > > > > >
> > > > > > > > > > Doesn't your patch affect pan and tilt for all the
> > > > > > > > > > cameras,
> > > > > > > > > > not
> > > > > > > > > > only
> > > > > > > > > > the BCC950?

/me wonders which e-mail client is to be blamed for this

> > > > > > > > > >
> > > > > > > > > Yes, it affects all cameras that support
> > > > > > > > > CT_PANTILT_RELATIVE_CONTROL.
> > > > > > > > >
> > > > > > > > > > Also it seems that 1 means that device does not support
> > > > > > > > > > programmable
> > > > > > > > > > speed. Is that correct?
> > > > > > > > > >
> > > > > > > > > > ```
> > > > > > > > > > The bPanSpeed field is used to specify the speed of the
> > > > > > > > > > movement for
> > > > > > > > > > the Pan direction. A low
> > > > > > > > > > number indicates a slow speed and a high number
> > > > > > > > > > indicates a
> > > > > > > > > > higher
> > > > > > > > > > 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 for the Pan control, it will
> > > > > > > > > > return
> > > > > > > > > > the
> > > > > > > > > > value 1
> > > > > > > > > > in this field for all these
> > > > > > > > > > requests.
> > > > > > > > > > ```
> > > > > > > >
> > > > > > > > It seems to me that the module is compliant to the
> > > > > > > > standard. It
> > > > > > > > returns 1 as GET_DEF because it does not support speed
> > > > > > > > control.
> > > > > > > >
> > > > > > > > Maybe you should ignore the speed control instead of
> > > > > > > > changing
> > > > > > > > its
> > > > > > > > default value?
> > > > > > >
> > > > > > > ( this is the standard I am refering to: 4.2.2.1.15 PanTilt
> > > > > > > (Relative) Control
> > > > > > >
> > > > > > >
> > > > > > > https://www.usb.org/document-library/video-class-v15-document-set
> > > > > > > )
> > > > > > > >
> > > > > > > >
> > > > > >
> > > > > > It's a different API. V4L2 control values are not the same as
> > > > > > the
> > > > > > UVC
> > > > > > standard control values.
> > > > >
> > > > > What I am saying, is that if
> > > > > CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF is 1 you should not
> > > > > create the mapping to
> > > > > V4L2_CID_PAN_SPEED
> > > > >
> > > > If I set V4L2_CID_PAN_SPEED to 1 (max), the BCC950 starts to move
> > > > at
> > > > maximum speed. This is what it should do according to description
> > > > of
> > > > the V4L2_CID_PAN_SPEED.
> > > >
> > > > My understanding is that, if the camera supports
> > > > CT_PANTILT_RELATIVE_CONTROL there should be a V4L2_CID_PAN_SPEED.
> > > > CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF == 1 only says that
> > > > only
> > > > one speed is available not a range.
> > >
> > > I think I got confused by the names.
> > >
> > > Can you check if this works?
> > >
> > > ribalda@alco:~/work/linux$ git diff
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > > b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 5e9d3da862dd..abab2f4efc94 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -437,6 +437,7 @@ static s32 uvc_ctrl_get_rel_speed(struct
> > > uvc_control_mapping *mapping,
> > >         s8 rel = (s8)data[first];
> > >
> > >         switch (query) {
> > > +       case UVC_GET_DEF:
> > >         case UVC_GET_CUR:
> > >                 return (rel == 0) ? 0 : (rel > 0 ? data[first+1]
> > >                                                  : -data[first+1]);
> > > @@ -444,7 +445,6 @@ static s32 uvc_ctrl_get_rel_speed(struct
> > > uvc_control_mapping *mapping,
> > >                 return -data[first+1];
> > >         case UVC_GET_MAX:
> > >         case UVC_GET_RES:
> > > -       case UVC_GET_DEF:
> > >         default:
> > >                 return data[first+1];
> > >         }
> > > ribalda@alco:~/work/linux$
> > >
> > >
> >
> > It works this way also.
> > Should I send a new version?
> 
> I believe this is slightly better... but it is up to Laurent ;)

I'm not sure to see how it's better. UVC_GET_DEF is required by the UVC
specification to return 0 for the bPanRelative and bTiltRelative fields.
The above will thus return 0 if the device complies with the spec, or a
different value if it doesn't. Isn't it better to hardcode 0 ? Unless
I'm missing something, I think Gergo's original patch is better. The
commit message, however, needs improvements to explain the issue. The
Logitech BCC950 mention should be dropped as the problem isn't specific
to a particular camera.

> > > > > > Eg the V4L2_CID_PAN_SPEED control value calculated from
> > > > > > CT_PANTILT_RELATIVE_CONTROL's bPanRelative and bPanSpeed value.
> > > > > >
> > > > > > It only bothers me that I have to handle these two controls
> > > > > > differently.
> > > > > >
> > > > > > > > > >
> > > > > > > > > I started from the V4L2 control description.
> > > > > > > > >
> > > > > > > > > V4L2_CID_PAN_SPEED (integer)
> > > > > > > > > This control turns the camera horizontally at the
> > > > > > > > > specific
> > > > > > > > > speed.
> > > > > > > > > The
> > > > > > > > > unit is undefined. A positive value moves the camera to
> > > > > > > > > the
> > > > > > > > > right
> > > > > > > > > (clockwise when viewed from above), a negative value to
> > > > > > > > > the
> > > > > > > > > left.
> > > > > > > > > A
> > > > > > > > > value of zero stops the motion if one is in progress and
> > > > > > > > > has
> > > > > > > > > no
> > > > > > > > > effect
> > > > > > > > > otherwise.
> > > > > > > > >
> > > > > > > > > And this is why I thought that 1 is not a good default
> > > > > > > > > value,
> > > > > > > > > because
> > > > > > > > > it moves the camera.
> > > > > > > > > The other V4L2 controls have a default value that I can
> > > > > > > > > safely
> > > > > > > > > set the
> > > > > > > > > controls to.
> > > > > > > > >
> > > > > > > > > Are you using it to determine if the camera supports
> > > > > > > > > speed
> > > > > > > > > control?
> > > > > > > > >
> > > > > > > > > > When you program that value do you see any difference
> > > > > > > > > > on
> > > > > > > > > > the
> > > > > > > > > > device?
> > > > > > > > > > What is max, min and res?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > No, it works the same way.
> > > > > > > > > Only the default value changes (from 1 to 0)
> > > > > > > > >
> > > > > > > > >  pan_speed 0x009a0920 (int)    : min=-1 max=1 step=1
> > > > > > > > > default=0
> > > > > > > > > value=0
> > > > > > > > > tilt_speed 0x009a0921 (int)    : min=-1 max=1 step=1
> > > > > > > > > default=0
> > > > > > > > > value=0
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks!
> > > > > > > > > >
> > > > > > > > > > Regards!
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > Gergo
> > > > > > > > >
> > > > > > > > > > On Wed, 14 Jun 2023 at 15:13, Gergo Koteles
> > > > > > > > > > <soyer@irl.hu>
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > The Logitech BCC950 incorrectly reports 1 (the max
> > > > > > > > > > > value)
> > > > > > > > > > > for the default values of V4L2_CID_PAN_SPEED,
> > > > > > > > > > > V4L2_CID_TILT_SPEED.
> > > > > > > > > > >
> > > > > > > > > > > This patch sets them to 0, which is the stop value.
> > > > > > > > > > >
> > > > > > > > > > > Previous discussion
> > > > > > > > > > > Link:
> > > > > > > > > > > https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Gergo Koteles <soyer@irl.hu>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
> > > > > > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > > b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > > index 5e9d3da862dd..e131958c0930 100644
> > > > > > > > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > > @@ -444,9 +444,10 @@ static s32
> > > > > > > > > > > uvc_ctrl_get_rel_speed(struct
> > > > > > > > > > > uvc_control_mapping *mapping,
> > > > > > > > > > >                 return -data[first+1];
> > > > > > > > > > >         case UVC_GET_MAX:
> > > > > > > > > > >         case UVC_GET_RES:
> > > > > > > > > > > +               return data[first+1];
> > > > > > > > > > >         case UVC_GET_DEF:
> > > > > > > > > > >         default:
> > > > > > > > > > > -               return data[first+1];
> > > > > > > > > > > +               return 0;
> > > > > > > > > > >         }
> > > > > > > > > > >  }
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Ricardo Ribalda]

Hi

On Wed, 14 Jun 2023 at 22:53, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Wed, Jun 14, 2023 at 10:15:43PM +0200, Ricardo Ribalda wrote:
> > On Wed, 14 Jun 2023 at 18:56, Gergo Koteles <soyer@irl.hu> wrote:
> > > On Wed, 2023-06-14 at 18:29 +0200, Ricardo Ribalda wrote:
> > > > On Wed, 14 Jun 2023 at 17:46, Gergo Koteles <soyer@irl.hu> wrote:
> > > > > On Wed, 2023-06-14 at 17:25 +0200, Ricardo Ribalda wrote:
> > > > > > On Wed, 14 Jun 2023 at 17:22, Gergo Koteles <soyer@irl.hu> wrote:
> > > > > > > On Wed, 2023-06-14 at 17:08 +0200, Ricardo Ribalda wrote:
> > > > > > > > On Wed, 14 Jun 2023 at 17:07, Ricardo Ribalda <ribalda@chromium.org> wrote:
> > > > > > > > > On Wed, 14 Jun 2023 at 16:59, soyer <soyer@irl.hu> wrote:
> > > > > > > > > > On Wed, 2023-06-14 at 16:19 +0200, Ricardo Ribalda wrote:
> > > > > > > > > > > [Now in plain text mode]
> > > > > > > > > > >
> > > > > > > > > > > Hi Gergo
> > > > > > > > > > >
> > > > > > > > > > > Doesn't your patch affect pan and tilt for all the
> > > > > > > > > > > cameras,
> > > > > > > > > > > not
> > > > > > > > > > > only
> > > > > > > > > > > the BCC950?
>
> /me wonders which e-mail client is to be blamed for this
>
> > > > > > > > > > >
> > > > > > > > > > Yes, it affects all cameras that support
> > > > > > > > > > CT_PANTILT_RELATIVE_CONTROL.
> > > > > > > > > >
> > > > > > > > > > > Also it seems that 1 means that device does not support
> > > > > > > > > > > programmable
> > > > > > > > > > > speed. Is that correct?
> > > > > > > > > > >
> > > > > > > > > > > ```
> > > > > > > > > > > The bPanSpeed field is used to specify the speed of the
> > > > > > > > > > > movement for
> > > > > > > > > > > the Pan direction. A low
> > > > > > > > > > > number indicates a slow speed and a high number
> > > > > > > > > > > indicates a
> > > > > > > > > > > higher
> > > > > > > > > > > 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 for the Pan control, it will
> > > > > > > > > > > return
> > > > > > > > > > > the
> > > > > > > > > > > value 1
> > > > > > > > > > > in this field for all these
> > > > > > > > > > > requests.
> > > > > > > > > > > ```
> > > > > > > > >
> > > > > > > > > It seems to me that the module is compliant to the
> > > > > > > > > standard. It
> > > > > > > > > returns 1 as GET_DEF because it does not support speed
> > > > > > > > > control.
> > > > > > > > >
> > > > > > > > > Maybe you should ignore the speed control instead of
> > > > > > > > > changing
> > > > > > > > > its
> > > > > > > > > default value?
> > > > > > > >
> > > > > > > > ( this is the standard I am refering to: 4.2.2.1.15 PanTilt
> > > > > > > > (Relative) Control
> > > > > > > >
> > > > > > > >
> > > > > > > > https://www.usb.org/document-library/video-class-v15-document-set
> > > > > > > > )
> > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > > > It's a different API. V4L2 control values are not the same as
> > > > > > > the
> > > > > > > UVC
> > > > > > > standard control values.
> > > > > >
> > > > > > What I am saying, is that if
> > > > > > CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF is 1 you should not
> > > > > > create the mapping to
> > > > > > V4L2_CID_PAN_SPEED
> > > > > >
> > > > > If I set V4L2_CID_PAN_SPEED to 1 (max), the BCC950 starts to move
> > > > > at
> > > > > maximum speed. This is what it should do according to description
> > > > > of
> > > > > the V4L2_CID_PAN_SPEED.
> > > > >
> > > > > My understanding is that, if the camera supports
> > > > > CT_PANTILT_RELATIVE_CONTROL there should be a V4L2_CID_PAN_SPEED.
> > > > > CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF == 1 only says that
> > > > > only
> > > > > one speed is available not a range.
> > > >
> > > > I think I got confused by the names.
> > > >
> > > > Can you check if this works?
> > > >
> > > > ribalda@alco:~/work/linux$ git diff
> > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > index 5e9d3da862dd..abab2f4efc94 100644
> > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > @@ -437,6 +437,7 @@ static s32 uvc_ctrl_get_rel_speed(struct
> > > > uvc_control_mapping *mapping,
> > > >         s8 rel = (s8)data[first];
> > > >
> > > >         switch (query) {
> > > > +       case UVC_GET_DEF:
> > > >         case UVC_GET_CUR:
> > > >                 return (rel == 0) ? 0 : (rel > 0 ? data[first+1]
> > > >                                                  : -data[first+1]);
> > > > @@ -444,7 +445,6 @@ static s32 uvc_ctrl_get_rel_speed(struct
> > > > uvc_control_mapping *mapping,
> > > >                 return -data[first+1];
> > > >         case UVC_GET_MAX:
> > > >         case UVC_GET_RES:
> > > > -       case UVC_GET_DEF:
> > > >         default:
> > > >                 return data[first+1];
> > > >         }
> > > > ribalda@alco:~/work/linux$
> > > >
> > > >
> > >
> > > It works this way also.
> > > Should I send a new version?
> >
> > I believe this is slightly better... but it is up to Laurent ;)
>
> I'm not sure to see how it's better. UVC_GET_DEF is required by the UVC
> specification to return 0 for the bPanRelative and bTiltRelative fields.
> The above will thus return 0 if the device complies with the spec, or a
> different value if it doesn't. Isn't it better to hardcode 0 ? Unless
> I'm missing something, I think Gergo's original patch is better. The
> commit message, however, needs improvements to explain the issue. The
> Logitech BCC950 mention should be dropped as the problem isn't specific
> to a particular camera.

I guess I want to catch firmware bugs :)

You are right! with the proper commit message the first patch is ok.


Thanks!


>
> > > > > > > Eg the V4L2_CID_PAN_SPEED control value calculated from
> > > > > > > CT_PANTILT_RELATIVE_CONTROL's bPanRelative and bPanSpeed value.
> > > > > > >
> > > > > > > It only bothers me that I have to handle these two controls
> > > > > > > differently.
> > > > > > >
> > > > > > > > > > >
> > > > > > > > > > I started from the V4L2 control description.
> > > > > > > > > >
> > > > > > > > > > V4L2_CID_PAN_SPEED (integer)
> > > > > > > > > > This control turns the camera horizontally at the
> > > > > > > > > > specific
> > > > > > > > > > speed.
> > > > > > > > > > The
> > > > > > > > > > unit is undefined. A positive value moves the camera to
> > > > > > > > > > the
> > > > > > > > > > right
> > > > > > > > > > (clockwise when viewed from above), a negative value to
> > > > > > > > > > the
> > > > > > > > > > left.
> > > > > > > > > > A
> > > > > > > > > > value of zero stops the motion if one is in progress and
> > > > > > > > > > has
> > > > > > > > > > no
> > > > > > > > > > effect
> > > > > > > > > > otherwise.
> > > > > > > > > >
> > > > > > > > > > And this is why I thought that 1 is not a good default
> > > > > > > > > > value,
> > > > > > > > > > because
> > > > > > > > > > it moves the camera.
> > > > > > > > > > The other V4L2 controls have a default value that I can
> > > > > > > > > > safely
> > > > > > > > > > set the
> > > > > > > > > > controls to.
> > > > > > > > > >
> > > > > > > > > > Are you using it to determine if the camera supports
> > > > > > > > > > speed
> > > > > > > > > > control?
> > > > > > > > > >
> > > > > > > > > > > When you program that value do you see any difference
> > > > > > > > > > > on
> > > > > > > > > > > the
> > > > > > > > > > > device?
> > > > > > > > > > > What is max, min and res?
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > No, it works the same way.
> > > > > > > > > > Only the default value changes (from 1 to 0)
> > > > > > > > > >
> > > > > > > > > >  pan_speed 0x009a0920 (int)    : min=-1 max=1 step=1
> > > > > > > > > > default=0
> > > > > > > > > > value=0
> > > > > > > > > > tilt_speed 0x009a0921 (int)    : min=-1 max=1 step=1
> > > > > > > > > > default=0
> > > > > > > > > > value=0
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Thanks!
> > > > > > > > > > >
> > > > > > > > > > > Regards!
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > > Gergo
> > > > > > > > > >
> > > > > > > > > > > On Wed, 14 Jun 2023 at 15:13, Gergo Koteles
> > > > > > > > > > > <soyer@irl.hu>
> > > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > The Logitech BCC950 incorrectly reports 1 (the max
> > > > > > > > > > > > value)
> > > > > > > > > > > > for the default values of V4L2_CID_PAN_SPEED,
> > > > > > > > > > > > V4L2_CID_TILT_SPEED.
> > > > > > > > > > > >
> > > > > > > > > > > > This patch sets them to 0, which is the stop value.
> > > > > > > > > > > >
> > > > > > > > > > > > Previous discussion
> > > > > > > > > > > > Link:
> > > > > > > > > > > > https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Gergo Koteles <soyer@irl.hu>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
> > > > > > > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > > > b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > > > index 5e9d3da862dd..e131958c0930 100644
> > > > > > > > > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > > > @@ -444,9 +444,10 @@ static s32
> > > > > > > > > > > > uvc_ctrl_get_rel_speed(struct
> > > > > > > > > > > > uvc_control_mapping *mapping,
> > > > > > > > > > > >                 return -data[first+1];
> > > > > > > > > > > >         case UVC_GET_MAX:
> > > > > > > > > > > >         case UVC_GET_RES:
> > > > > > > > > > > > +               return data[first+1];
> > > > > > > > > > > >         case UVC_GET_DEF:
> > > > > > > > > > > >         default:
> > > > > > > > > > > > -               return data[first+1];
> > > > > > > > > > > > +               return 0;
> > > > > > > > > > > >         }
> > > > > > > > > > > >  }
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Gergo Koteles]

Hi Laurent,
On Wed, 2023-06-14 at 23:53 +0300, Laurent Pinchart wrote:
> Hi Ricardo,
> 
> On Wed, Jun 14, 2023 at 10:15:43PM +0200, Ricardo Ribalda wrote:
> > On Wed, 14 Jun 2023 at 18:56, Gergo Koteles <soyer@irl.hu> wrote:
> > > On Wed, 2023-06-14 at 18:29 +0200, Ricardo Ribalda wrote:
> > > > On Wed, 14 Jun 2023 at 17:46, Gergo Koteles <soyer@irl.hu> wrote:
> > > > > On Wed, 2023-06-14 at 17:25 +0200, Ricardo Ribalda wrote:
> > > > > > On Wed, 14 Jun 2023 at 17:22, Gergo Koteles <soyer@irl.hu> wrote:
> > > > > > > On Wed, 2023-06-14 at 17:08 +0200, Ricardo Ribalda wrote:
> > > > > > > > On Wed, 14 Jun 2023 at 17:07, Ricardo Ribalda <ribalda@chromium.org> wrote:
> > > > > > > > > On Wed, 14 Jun 2023 at 16:59, soyer <soyer@irl.hu> wrote:
> > > > > > > > > > On Wed, 2023-06-14 at 16:19 +0200, Ricardo Ribalda wrote:
> > > > > > > > > > > [Now in plain text mode]
> > > > > > > > > > > 
> > > > > > > > > > > Hi Gergo
> > > > > > > > > > > 
> > > > > > > > > > > Doesn't your patch affect pan and tilt for all the
> > > > > > > > > > > cameras,
> > > > > > > > > > > not
> > > > > > > > > > > only
> > > > > > > > > > > the BCC950?
> 
> /me wonders which e-mail client is to be blamed for this
> 

It looks like it's mine. I've just switched to Evolution.
It has very strange default settings.
Thank you for the notice.

> > > > > > > > > > > 
> > > > > > > > > > Yes, it affects all cameras that support
> > > > > > > > > > CT_PANTILT_RELATIVE_CONTROL.
> > > > > > > > > > 
> > > > > > > > > > > Also it seems that 1 means that device does not support
> > > > > > > > > > > programmable
> > > > > > > > > > > speed. Is that correct?
> > > > > > > > > > > 
> > > > > > > > > > > ```
> > > > > > > > > > > The bPanSpeed field is used to specify the speed of the
> > > > > > > > > > > movement for
> > > > > > > > > > > the Pan direction. A low
> > > > > > > > > > > number indicates a slow speed and a high number
> > > > > > > > > > > indicates a
> > > > > > > > > > > higher
> > > > > > > > > > > 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 for the Pan control, it will
> > > > > > > > > > > return
> > > > > > > > > > > the
> > > > > > > > > > > value 1
> > > > > > > > > > > in this field for all these
> > > > > > > > > > > requests.
> > > > > > > > > > > ```
> > > > > > > > > 
> > > > > > > > > It seems to me that the module is compliant to the
> > > > > > > > > standard. It
> > > > > > > > > returns 1 as GET_DEF because it does not support speed
> > > > > > > > > control.
> > > > > > > > > 
> > > > > > > > > Maybe you should ignore the speed control instead of
> > > > > > > > > changing
> > > > > > > > > its
> > > > > > > > > default value?
> > > > > > > > 
> > > > > > > > ( this is the standard I am refering to: 4.2.2.1.15 PanTilt
> > > > > > > > (Relative) Control
> > > > > > > > 
> > > > > > > > 
> > > > > > > > https://www.usb.org/document-library/video-class-v15-document-set
> > > > > > > > )
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > 
> > > > > > > It's a different API. V4L2 control values are not the same as
> > > > > > > the
> > > > > > > UVC
> > > > > > > standard control values.
> > > > > > 
> > > > > > What I am saying, is that if
> > > > > > CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF is 1 you should not
> > > > > > create the mapping to
> > > > > > V4L2_CID_PAN_SPEED
> > > > > > 
> > > > > If I set V4L2_CID_PAN_SPEED to 1 (max), the BCC950 starts to move
> > > > > at
> > > > > maximum speed. This is what it should do according to description
> > > > > of
> > > > > the V4L2_CID_PAN_SPEED.
> > > > > 
> > > > > My understanding is that, if the camera supports
> > > > > CT_PANTILT_RELATIVE_CONTROL there should be a V4L2_CID_PAN_SPEED.
> > > > > CT_PANTILT_RELATIVE_CONTROL.bPanSpeed.GET_DEF == 1 only says that
> > > > > only
> > > > > one speed is available not a range.
> > > > 
> > > > I think I got confused by the names.
> > > > 
> > > > Can you check if this works?
> > > > 
> > > > ribalda@alco:~/work/linux$ git diff
> > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > index 5e9d3da862dd..abab2f4efc94 100644
> > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > @@ -437,6 +437,7 @@ static s32 uvc_ctrl_get_rel_speed(struct
> > > > uvc_control_mapping *mapping,
> > > >         s8 rel = (s8)data[first];
> > > > 
> > > >         switch (query) {
> > > > +       case UVC_GET_DEF:
> > > >         case UVC_GET_CUR:
> > > >                 return (rel == 0) ? 0 : (rel > 0 ? data[first+1]
> > > >                                                  : -data[first+1]);
> > > > @@ -444,7 +445,6 @@ static s32 uvc_ctrl_get_rel_speed(struct
> > > > uvc_control_mapping *mapping,
> > > >                 return -data[first+1];
> > > >         case UVC_GET_MAX:
> > > >         case UVC_GET_RES:
> > > > -       case UVC_GET_DEF:
> > > >         default:
> > > >                 return data[first+1];
> > > >         }
> > > > ribalda@alco:~/work/linux$
> > > > 
> > > > 
> > > 
> > > It works this way also.
> > > Should I send a new version?
> > 
> > I believe this is slightly better... but it is up to Laurent ;)
> 
> I'm not sure to see how it's better. UVC_GET_DEF is required by the UVC
> specification to return 0 for the bPanRelative and bTiltRelative fields.
> The above will thus return 0 if the device complies with the spec, or a
> different value if it doesn't. Isn't it better to hardcode 0 ? Unless
> I'm missing something, I think Gergo's original patch is better. The
> commit message, however, needs improvements to explain the issue. The
> Logitech BCC950 mention should be dropped as the problem isn't specific
> to a particular camera.
> 
I'll create a V2 with a better commit message.

Thanks,
G

> > > > > > > Eg the V4L2_CID_PAN_SPEED control value calculated from
> > > > > > > CT_PANTILT_RELATIVE_CONTROL's bPanRelative and bPanSpeed value.
> > > > > > > 
> > > > > > > It only bothers me that I have to handle these two controls
> > > > > > > differently.
> > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > I started from the V4L2 control description.
> > > > > > > > > > 
> > > > > > > > > > V4L2_CID_PAN_SPEED (integer)
> > > > > > > > > > This control turns the camera horizontally at the
> > > > > > > > > > specific
> > > > > > > > > > speed.
> > > > > > > > > > The
> > > > > > > > > > unit is undefined. A positive value moves the camera to
> > > > > > > > > > the
> > > > > > > > > > right
> > > > > > > > > > (clockwise when viewed from above), a negative value to
> > > > > > > > > > the
> > > > > > > > > > left.
> > > > > > > > > > A
> > > > > > > > > > value of zero stops the motion if one is in progress and
> > > > > > > > > > has
> > > > > > > > > > no
> > > > > > > > > > effect
> > > > > > > > > > otherwise.
> > > > > > > > > > 
> > > > > > > > > > And this is why I thought that 1 is not a good default
> > > > > > > > > > value,
> > > > > > > > > > because
> > > > > > > > > > it moves the camera.
> > > > > > > > > > The other V4L2 controls have a default value that I can
> > > > > > > > > > safely
> > > > > > > > > > set the
> > > > > > > > > > controls to.
> > > > > > > > > > 
> > > > > > > > > > Are you using it to determine if the camera supports
> > > > > > > > > > speed
> > > > > > > > > > control?
> > > > > > > > > > 
> > > > > > > > > > > When you program that value do you see any difference
> > > > > > > > > > > on
> > > > > > > > > > > the
> > > > > > > > > > > device?
> > > > > > > > > > > What is max, min and res?
> > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > No, it works the same way.
> > > > > > > > > > Only the default value changes (from 1 to 0)
> > > > > > > > > > 
> > > > > > > > > >  pan_speed 0x009a0920 (int)    : min=-1 max=1 step=1
> > > > > > > > > > default=0
> > > > > > > > > > value=0
> > > > > > > > > > tilt_speed 0x009a0921 (int)    : min=-1 max=1 step=1
> > > > > > > > > > default=0
> > > > > > > > > > value=0
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > Thanks!
> > > > > > > > > > > 
> > > > > > > > > > > Regards!
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Thanks,
> > > > > > > > > > Gergo
> > > > > > > > > > 
> > > > > > > > > > > On Wed, 14 Jun 2023 at 15:13, Gergo Koteles
> > > > > > > > > > > <soyer@irl.hu>
> > > > > > > > > > > wrote:
> > > > > > > > > > > > 
> > > > > > > > > > > > The Logitech BCC950 incorrectly reports 1 (the max
> > > > > > > > > > > > value)
> > > > > > > > > > > > for the default values of V4L2_CID_PAN_SPEED,
> > > > > > > > > > > > V4L2_CID_TILT_SPEED.
> > > > > > > > > > > > 
> > > > > > > > > > > > This patch sets them to 0, which is the stop value.
> > > > > > > > > > > > 
> > > > > > > > > > > > Previous discussion
> > > > > > > > > > > > Link:
> > > > > > > > > > > > https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Gergo Koteles <soyer@irl.hu>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
> > > > > > > > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > > > > > 
> > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > > > b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > > > index 5e9d3da862dd..e131958c0930 100644
> > > > > > > > > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > > > > > > > @@ -444,9 +444,10 @@ static s32
> > > > > > > > > > > > uvc_ctrl_get_rel_speed(struct
> > > > > > > > > > > > uvc_control_mapping *mapping,
> > > > > > > > > > > >                 return -data[first+1];
> > > > > > > > > > > >         case UVC_GET_MAX:
> > > > > > > > > > > >         case UVC_GET_RES:
> > > > > > > > > > > > +               return data[first+1];
> > > > > > > > > > > >         case UVC_GET_DEF:
> > > > > > > > > > > >         default:
> > > > > > > > > > > > -               return data[first+1];
> > > > > > > > > > > > +               return 0;
> > > > > > > > > > > >         }
> > > > > > > > > > > >  }
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
> 

[PATCH v2] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Gergo Koteles]

Devices with pan/tilt controls but without pan/tilt speed controls
return 1 for the default value of V4L2_CID_PAN_SPEED or
V4L2_CID_TILT_SPEED. For these controls, the value of 1 means a
move and that's not a good default.

Currently, for these controls the UVC_GET_DEF query returns
bPanSpeed or bTiltSpeed of CT_PANTILT_RELATIVE_CONTROL.

According to the UVC 1.5 specification, the default value of bPanSpeed
or bTiltSpeed should be 1 if the pan/tilt control doesn't support
speed control.

"If the control does not support speed control for the Tilt control,
it will return the value 1 in this field for all these requests."

This patch modifies the uvc_ctrl_get_rel_speed to return hardcoded 0
for UVC_GET_DEF query, because that's the stop or don't move value
for these V4L2 controls.

Previous discussion
Link: https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/

Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 5e9d3da862dd..e131958c0930 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -444,9 +444,10 @@ static s32 uvc_ctrl_get_rel_speed(struct uvc_control_mapping *mapping,
 		return -data[first+1];
 	case UVC_GET_MAX:
 	case UVC_GET_RES:
+		return data[first+1];
 	case UVC_GET_DEF:
 	default:
-		return data[first+1];
+		return 0;
 	}
 }
 

base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
-- 
2.40.1

[PATCH v2 RESEND] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Gergo Koteles]

Devices with pan/tilt controls but without pan/tilt speed controls
return 1 for the default value of V4L2_CID_PAN_SPEED or
V4L2_CID_TILT_SPEED. For these controls, the value of 1 means a
move and that's not a good default.

Currently, for these controls the UVC_GET_DEF query returns
bPanSpeed or bTiltSpeed of CT_PANTILT_RELATIVE_CONTROL.

According to the UVC 1.5 specification, the default value of bPanSpeed
or bTiltSpeed should be 1 if the pan/tilt control doesn't support
speed control.

"If the control does not support speed control for the Tilt control,
it will return the value 1 in this field for all these requests."

This patch modifies the uvc_ctrl_get_rel_speed to return hardcoded 0
for UVC_GET_DEF query, because that's the stop or don't move value
for these V4L2 controls.

Previous discussion
Link: https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/

Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 5e9d3da862dd..e131958c0930 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -444,9 +444,10 @@ static s32 uvc_ctrl_get_rel_speed(struct uvc_control_mapping *mapping,
 		return -data[first+1];
 	case UVC_GET_MAX:
 	case UVC_GET_RES:
+		return data[first+1];
 	case UVC_GET_DEF:
 	default:
-		return data[first+1];
+		return 0;
 	}
 }
 

base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
-- 
2.42.0

Re: [PATCH v2 RESEND] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Ricardo Ribalda]

Hi Gergo

I missed sending the reviewed-by sorry :)

Btw, do you still have access to the device? Could you tell me what
you get from  and UVC_GET_MAX, UVC_GET_MIN for
UVC_CT_PANTILT_RELATIVE_CONTROL and UVC_CT_ZOOM_RELATIVE_CONTROL ?

Thanks!

On Mon, 20 Nov 2023 at 19:53, Gergo Koteles <soyer@irl.hu> wrote:
>
> Devices with pan/tilt controls but without pan/tilt speed controls
> return 1 for the default value of V4L2_CID_PAN_SPEED or
> V4L2_CID_TILT_SPEED. For these controls, the value of 1 means a
> move and that's not a good default.
>
> Currently, for these controls the UVC_GET_DEF query returns
> bPanSpeed or bTiltSpeed of CT_PANTILT_RELATIVE_CONTROL.
>
> According to the UVC 1.5 specification, the default value of bPanSpeed
> or bTiltSpeed should be 1 if the pan/tilt control doesn't support
> speed control.
>
> "If the control does not support speed control for the Tilt control,
> it will return the value 1 in this field for all these requests."
>
> This patch modifies the uvc_ctrl_get_rel_speed to return hardcoded 0
> for UVC_GET_DEF query, because that's the stop or don't move value
> for these V4L2 controls.
>
> Previous discussion
> Link: https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/
>
> Signed-off-by: Gergo Koteles <soyer@irl.hu>
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 5e9d3da862dd..e131958c0930 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -444,9 +444,10 @@ static s32 uvc_ctrl_get_rel_speed(struct uvc_control_mapping *mapping,
>                 return -data[first+1];
>         case UVC_GET_MAX:
>         case UVC_GET_RES:
> +               return data[first+1];
>         case UVC_GET_DEF:
>         default:
> -               return data[first+1];
> +               return 0;
>         }
>  }
>
>
> base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
> --
> 2.42.0
>
>


-- 
Ricardo Ribalda

Re: [PATCH v2 RESEND] media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default

[Gergo Koteles]

Hi Ricardo,

On Tue, 2024-03-19 at 10:43 +0100, Ricardo Ribalda wrote:
> Hi Gergo
> 
> I missed sending the reviewed-by sorry :)
> 
> Btw, do you still have access to the device? Could you tell me what
> you get from  and UVC_GET_MAX, UVC_GET_MIN for
> UVC_CT_PANTILT_RELATIVE_CONTROL and UVC_CT_ZOOM_RELATIVE_CONTROL ?
> 

PTZ Pro and BCC950 only have UVC_CT_ZOOM_ABSOLUTE_CONTROL, not
UVC_CT_ZOOM_RELATIVE_CONTROL.

v4l2-ctl -l:

zoom_absolute(int): min=100 max=1000 step=1 default=100 value=100
pan_speed(int): min=-1 max=1 step=1 default=1 value=0
tilt_speed(int): min=-1 max=1 step=1 default=1 value=0

printing data[first], data[first+1] in uvc_ctrl_get_rel_speed:

GET_DEF: 0 1
GET_MIN: 0 1
GET_MAX: 0 1
GET_RES: 0 1
GET_CUR: 0 1


I found the output of Obsbot Tiny v4l2-ctl -l at
https://www.labohyt.net/blog/gadget/post-7643:

pan_absolute(int): min=-468000 max=468000 step=3600 default=0 value=0
tilt_absolute(int): min=-324000 max=324000 step=7200 default=0 value=0
focus_absolute(int): min=0 max=100 step=1 default=0 value=0
flags=inactive
focus_automatic_continuous(bool): default=1 value=1
zoom_absolute(int): min=0 max=100 step=1 default=0 value=0
zoom_continuous(int): min=0 max=100 step=1 default=100 value=0
pan_speed(int): min=-1 max=160 step=1 default=20 value=0
tilt_speed(int): min=-1 max=120 step=1 default=20 value=0

This is where the default value of pan_speed/tilt_speed/zoom_continuous
can be useful, even if they don't work exactly like the defaults for
other controls.

So this patch isn't that good after all.

Regards,
Gergo


> Thanks!
> 
> On Mon, 20 Nov 2023 at 19:53, Gergo Koteles <soyer@irl.hu> wrote:
> > 
> > Devices with pan/tilt controls but without pan/tilt speed controls
> > return 1 for the default value of V4L2_CID_PAN_SPEED or
> > V4L2_CID_TILT_SPEED. For these controls, the value of 1 means a
> > move and that's not a good default.
> > 
> > Currently, for these controls the UVC_GET_DEF query returns
> > bPanSpeed or bTiltSpeed of CT_PANTILT_RELATIVE_CONTROL.
> > 
> > According to the UVC 1.5 specification, the default value of bPanSpeed
> > or bTiltSpeed should be 1 if the pan/tilt control doesn't support
> > speed control.
> > 
> > "If the control does not support speed control for the Tilt control,
> > it will return the value 1 in this field for all these requests."
> > 
> > This patch modifies the uvc_ctrl_get_rel_speed to return hardcoded 0
> > for UVC_GET_DEF query, because that's the stop or don't move value
> > for these V4L2 controls.
> > 
> > Previous discussion
> > Link: https://lore.kernel.org/all/CAP_ceTy6XVmvTTAmvCp1YU2wxHwXqnarm69Yaz8K4FmpJqYxAg@mail.gmail.com/
> > 
> > Signed-off-by: Gergo Koteles <soyer@irl.hu>
> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 5e9d3da862dd..e131958c0930 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -444,9 +444,10 @@ static s32 uvc_ctrl_get_rel_speed(struct uvc_control_mapping *mapping,
> >                 return -data[first+1];
> >         case UVC_GET_MAX:
> >         case UVC_GET_RES:
> > +               return data[first+1];
> >         case UVC_GET_DEF:
> >         default:
> > -               return data[first+1];
> > +               return 0;
> >         }
> >  }
> > 
> > 
> > base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360
> > --
> > 2.42.0
> > 
> > 
> 
>