| Message ID | b062c3ec615a69cbc1b154b1838df3cdc3e1282a.1718726777.git.soyer@irl.hu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: UVC minimum relative pan/tilt/zoom speed fix | expand |
Hi Gergo Thanks for your your patch. On Tue, 18 Jun 2024 at 18:33, Gergo Koteles <soyer@irl.hu> wrote: > > From: John Bauer <johnebgood@securitylive.com> > > The minimum UVC control value for the relative pan/tilt/zoom speeds > cannot be probed as the implementation condenses the pan and tilt > direction and speed into two 16 bit values. The minimum cannot be > set at probe time because it is probed first and the maximum is not > yet known. With this fix if a relative speed control is queried > or set the minimum is set and checked based on the additive inverse of > the maximum at that time. > > Signed-off-by: John Bauer <johnebgood@securitylive.com> > Signed-off-by: Gergo Koteles <soyer@irl.hu> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 38 +++++++++++++++++++++++++++----- > 1 file changed, 33 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 4b685f883e4d..93ed2462e90b 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -441,7 +441,6 @@ static s32 uvc_ctrl_get_rel_speed(struct uvc_control_mapping *mapping, > return (rel == 0) ? 0 : (rel > 0 ? data[first+1] > : -data[first+1]); > case UVC_GET_MIN: > - return -data[first+1]; > case UVC_GET_MAX: > case UVC_GET_RES: > case UVC_GET_DEF: > @@ -1233,6 +1232,17 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl, > return ~0; > } > > +static bool is_relative_ptz_ctrl(__u32 ctrl_id) > +{ > + switch (ctrl_id) { > + case V4L2_CID_ZOOM_CONTINUOUS: > + case V4L2_CID_PAN_SPEED: > + case V4L2_CID_TILT_SPEED: > + return true; > + } > + return false; > +} > + > static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > struct uvc_control *ctrl, > struct uvc_control_mapping *mapping, > @@ -1322,14 +1332,23 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > break; > } > > - if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) > - v4l2_ctrl->minimum = mapping->get(mapping, UVC_GET_MIN, > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN)); > - > if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX) > v4l2_ctrl->maximum = mapping->get(mapping, UVC_GET_MAX, > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX)); > > + if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) { > + /* > + * For the relative speed implementation the minimum > + * value cannot be probed so it becomes the additive > + * inverse of maximum. > + */ > + if (is_relative_ptz_ctrl(v4l2_ctrl->id)) > + v4l2_ctrl->minimum = -v4l2_ctrl->maximum; > + else > + v4l2_ctrl->minimum = mapping->get(mapping, UVC_GET_MIN, > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN)); > + } > + > if (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES) > v4l2_ctrl->step = mapping->get(mapping, UVC_GET_RES, > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES)); > @@ -1916,6 +1935,15 @@ int uvc_ctrl_set(struct uvc_fh *handle, > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN)); > max = mapping->get(mapping, UVC_GET_MAX, > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX)); > + > + /* > + * For the relative speed implementation the minimum > + * value cannot be probed so it becomes the additive > + * inverse of maximum. > + */ > + if (is_relative_ptz_ctrl(xctrl->id)) > + min = -max; > + nit: The following would probably be more correct but less clear: if (is_relative_ptz_ctrl(xctrl->id)) min = -max; else min = mapping->get(mapping, UVC_GET_MIN,...) So up to you what do you/Laurent what is better ;) > step = mapping->get(mapping, UVC_GET_RES, > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES)); > if (step == 0) > -- > 2.45.2 >
Hi Ricardo, Thanks for the review. On Wed, 2024-06-19 at 08:15 +0200, Ricardo Ribalda wrote: > > nit: The following would probably be more correct but less clear: > > if (is_relative_ptz_ctrl(xctrl->id)) > min = -max; > else > min = mapping->get(mapping, UVC_GET_MIN,...) > > So up to you what do you/Laurent what is better ;) > > I like this better. I'll send a v4. Best regards, Gergo Koteles
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 4b685f883e4d..93ed2462e90b 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -441,7 +441,6 @@ static s32 uvc_ctrl_get_rel_speed(struct uvc_control_mapping *mapping, return (rel == 0) ? 0 : (rel > 0 ? data[first+1] : -data[first+1]); case UVC_GET_MIN: - return -data[first+1]; case UVC_GET_MAX: case UVC_GET_RES: case UVC_GET_DEF: @@ -1233,6 +1232,17 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl, return ~0; } +static bool is_relative_ptz_ctrl(__u32 ctrl_id) +{ + switch (ctrl_id) { + case V4L2_CID_ZOOM_CONTINUOUS: + case V4L2_CID_PAN_SPEED: + case V4L2_CID_TILT_SPEED: + return true; + } + return false; +} + static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, struct uvc_control *ctrl, struct uvc_control_mapping *mapping, @@ -1322,14 +1332,23 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, break; } - if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) - v4l2_ctrl->minimum = mapping->get(mapping, UVC_GET_MIN, - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN)); - if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX) v4l2_ctrl->maximum = mapping->get(mapping, UVC_GET_MAX, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX)); + if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) { + /* + * For the relative speed implementation the minimum + * value cannot be probed so it becomes the additive + * inverse of maximum. + */ + if (is_relative_ptz_ctrl(v4l2_ctrl->id)) + v4l2_ctrl->minimum = -v4l2_ctrl->maximum; + else + v4l2_ctrl->minimum = mapping->get(mapping, UVC_GET_MIN, + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN)); + } + if (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES) v4l2_ctrl->step = mapping->get(mapping, UVC_GET_RES, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES)); @@ -1916,6 +1935,15 @@ int uvc_ctrl_set(struct uvc_fh *handle, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN)); max = mapping->get(mapping, UVC_GET_MAX, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX)); + + /* + * For the relative speed implementation the minimum + * value cannot be probed so it becomes the additive + * inverse of maximum. + */ + if (is_relative_ptz_ctrl(xctrl->id)) + min = -max; + step = mapping->get(mapping, UVC_GET_RES, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES)); if (step == 0)