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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxx> wrote:
> > > On Wed, 2023-06-14 at 18:29 +0200, Ricardo Ribalda wrote:
> > > > On Wed, 14 Jun 2023 at 17:46, Gergo Koteles <soyer@xxxxxx> wrote:
> > > > > On Wed, 2023-06-14 at 17:25 +0200, Ricardo Ribalda wrote:
> > > > > > On Wed, 14 Jun 2023 at 17:22, Gergo Koteles <soyer@xxxxxx> wrote:
> > > > > > > On Wed, 2023-06-14 at 17:08 +0200, Ricardo Ribalda wrote:
> > > > > > > > On Wed, 14 Jun 2023 at 17:07, Ricardo Ribalda <ribalda@xxxxxxxxxxxx> wrote:
> > > > > > > > > On Wed, 14 Jun 2023 at 16:59, soyer <soyer@xxxxxx> 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@xxxxxx>
> > > > > > > > > > > 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@xxxxxxxxxxxxxx/
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Gergo Koteles <soyer@xxxxxx>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  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
> 




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux