| Message ID | eb4f7f29a94231c5fa404f7492dba8e7fd9fbb23.1686746422.git.soyer@irl.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: uvcvideo: uvc_ctrl_get_rel_speed: use 0 as default | expand |
[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 >
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 > > > >
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 > > > > > > > >
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
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 > > >
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 > > > > > > >
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 > > > > > > > > > > > > >
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
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
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 >
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
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
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 >
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; } }
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(-) base-commit: be9aac187433af6abba5fcc2e73d91d0794ba360