Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control

[Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>]

Hi Sergey

Thanks for the patch
On Mon, Feb 8, 2021 at 6:23 AM Sergey Senozhatsky
<sergey.senozhatsky@gmail.com> wrote:
>
> From: Sergey Senozhatsky <senozhatsky@chromium.org>
>
> This patch implements parts of UVC 1.5 Region of Interest (ROI)
> control, using the uvcvideo selection API.
>
> There are several things to mention here.
>
> First, UVC 1.5 defines CT_DIGITAL_WINDOW_CONTROL; and ROI rectangle
> coordinates "must be within the current Digital Window as specified
> by the CT_WINDOW control."  (4.2.2.1.20 Digital Region of Interest
> (ROI) Control.) This is not entirely clear if we need to implement
> CT_DIGITAL_WINDOW_CONTROL. ROI is naturally limited by: ROI GET_MIN
> and GET_MAX rectangles. Besides, the H/W that I'm playing with
> implements ROI, but doesn't implement CT_DIGITAL_WINDOW_CONTROL,
> so WINDOW_CONTROL is probably optional.
>
> Second, the patch doesn't implement all of the ROI requests.
> Namely, SEL_TGT_BOUNDS for ROI implements GET_MAX (that is maximal
> ROI rectangle area). GET_MIN is not implemented (as of now) since
> it's not very clear if user space would need such information.
>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 143 ++++++++++++++++++++++++++++++-
>  1 file changed, 140 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 252136cc885c..71b4577196e5 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1139,14 +1139,60 @@ static int uvc_ioctl_querymenu(struct file *file, void *fh,
>         return uvc_query_v4l2_menu(chain, qm);
>  }
>
> -static int uvc_ioctl_g_selection(struct file *file, void *fh,
> -                                struct v4l2_selection *sel)
> +/* UVC 1.5 ROI rectangle is half the size of v4l2_rect */
> +struct uvc_roi_rect {
> +       __u16                   top;
> +       __u16                   left;
> +       __u16                   bottom;
> +       __u16                   right;
> +};

Perhaps __packed; ?

> +
> +static int uvc_ioctl_g_roi_target(struct file *file, void *fh,
> +                                 struct v4l2_selection *sel)
>  {
>         struct uvc_fh *handle = fh;
>         struct uvc_streaming *stream = handle->stream;
> +       struct uvc_roi_rect *roi;
> +       u8 query;
> +       int ret;
>
> -       if (sel->type != stream->type)
> +       switch (sel->target) {
> +       case V4L2_SEL_TGT_ROI_DEFAULT:
> +               query = UVC_GET_DEF;
> +               break;
> +       case V4L2_SEL_TGT_ROI_CURRENT:
> +               query = UVC_GET_CUR;
> +               break;
> +       case V4L2_SEL_TGT_ROI_BOUNDS:
> +               query = UVC_GET_MAX;
> +               break;
> +       default:
>                 return -EINVAL;
> +       }
> +
> +       roi = kzalloc(sizeof(struct uvc_roi_rect), GFP_KERNEL);
> +       if (!roi)
> +               return -ENOMEM;
> +
> +       ret = uvc_query_ctrl(stream->dev, query, 1, stream->dev->intfnum,
> +                            UVC_CT_REGION_OF_INTEREST_CONTROL, roi,
> +                            sizeof(struct uvc_roi_rect));

It is a pity that we have to alloc memory for this  :(.

@Laurent, do you know a better way?

> +       if (!ret) {
> +               sel->r.left     = roi->left;
> +               sel->r.top      = roi->top;
> +               sel->r.width    = roi->right;
> +               sel->r.height   = roi->bottom;
> +       }
> +
> +       kfree(roi);
> +       return ret;
> +}
> +
> +static int uvc_ioctl_g_sel_target(struct file *file, void *fh,
> +                                 struct v4l2_selection *sel)
> +{
> +       struct uvc_fh *handle = fh;
> +       struct uvc_streaming *stream = handle->stream;
>
>         switch (sel->target) {
>         case V4L2_SEL_TGT_CROP_DEFAULT:
> @@ -1173,6 +1219,96 @@ static int uvc_ioctl_g_selection(struct file *file, void *fh,
>         return 0;
>  }
>
> +static int uvc_ioctl_g_selection(struct file *file, void *fh,
> +                                struct v4l2_selection *sel)
> +{
> +       struct uvc_fh *handle = fh;
> +       struct uvc_streaming *stream = handle->stream;
> +
> +       if (sel->type != stream->type)
> +               return -EINVAL;
> +
> +       switch (sel->target) {
> +       case V4L2_SEL_TGT_CROP_DEFAULT:
> +       case V4L2_SEL_TGT_CROP_BOUNDS:
> +       case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> +       case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> +               return uvc_ioctl_g_sel_target(file, fh, sel);
> +       case V4L2_SEL_TGT_ROI_CURRENT:
> +       case V4L2_SEL_TGT_ROI_DEFAULT:
> +       case V4L2_SEL_TGT_ROI_BOUNDS:
> +               return uvc_ioctl_g_roi_target(file, fh, sel);
> +       }
> +
> +       return -EINVAL;
> +}

Are you sure that there is no lock needed between the control and the
selection API?

> +
> +static bool validate_roi_bounds(struct uvc_streaming *stream,
> +                               struct v4l2_selection *sel)
> +{
> +       bool ok = true;
> +
> +       if (sel->r.left > USHRT_MAX || sel->r.top > USHRT_MAX ||
> +           sel->r.width > USHRT_MAX || sel->r.height > USHRT_MAX)
> +               return false;
> +
> +       /* perhaps also can test against ROI GET_MAX */
> +
> +       mutex_lock(&stream->mutex);
Maybe you should not release this mutex until query_ctrl is done?

> +       if ((u16)sel->r.width > stream->cur_frame->wWidth)
> +               ok = false;
> +       if ((u16)sel->r.height > stream->cur_frame->wHeight)
> +               ok = false;
> +       mutex_unlock(&stream->mutex);
> +
> +       return ok;
> +}
> +
> +static int uvc_ioctl_s_roi(struct file *file, void *fh,
> +                          struct v4l2_selection *sel)
> +{
> +       struct uvc_fh *handle = fh;
> +       struct uvc_streaming *stream = handle->stream;
> +       struct uvc_roi_rect *roi;
> +       int ret;
> +
> +       if (!validate_roi_bounds(stream, sel))
> +               return -E2BIG;
> +
> +       roi = kzalloc(sizeof(struct uvc_roi_rect), GFP_KERNEL);
> +       if (!roi)
> +               return -ENOMEM;
> +
> +       roi->left       = sel->r.left;
> +       roi->top        = sel->r.top;
> +       roi->right      = sel->r.width;
> +       roi->bottom     = sel->r.height;
> +
> +       ret = uvc_query_ctrl(stream->dev, UVC_SET_CUR, 1, stream->dev->intfnum,
> +                            UVC_CT_REGION_OF_INTEREST_CONTROL, roi,
> +                            sizeof(struct uvc_roi_rect));

I think you need to read back from the device the actual value

https://www.kernel.org/doc/html/v4.13/media/uapi/v4l/vidioc-g-selection.html?highlight=vidioc_s_selection
On success the struct v4l2_rect r field contains the adjusted
rectangle. When the parameters are unsuitable the application may
modify the cropping (composing) or image parameters and repeat the
cycle until satisfactory parameters have been negotiated. If
constraints flags have to be violated at then ERANGE is returned. The
error indicates that there exist no rectangle that satisfies the
constraints.


> +
> +       kfree(roi);
> +       return ret;
> +}
> +
> +static int uvc_ioctl_s_selection(struct file *file, void *fh,
> +                                struct v4l2_selection *sel)
> +{
> +       struct uvc_fh *handle = fh;
> +       struct uvc_streaming *stream = handle->stream;
> +
> +       if (sel->type != stream->type)
> +               return -EINVAL;
> +
> +       switch (sel->target) {
> +       case V4L2_SEL_TGT_ROI:
> +               return uvc_ioctl_s_roi(file, fh, sel);
> +       }
> +
> +       return -EINVAL;
> +}
> +
>  static int uvc_ioctl_g_parm(struct file *file, void *fh,
>                             struct v4l2_streamparm *parm)
>  {
> @@ -1533,6 +1669,7 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>         .vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,
>         .vidioc_querymenu = uvc_ioctl_querymenu,
>         .vidioc_g_selection = uvc_ioctl_g_selection,
> +       .vidioc_s_selection = uvc_ioctl_s_selection,
>         .vidioc_g_parm = uvc_ioctl_g_parm,
>         .vidioc_s_parm = uvc_ioctl_s_parm,
>         .vidioc_enum_framesizes = uvc_ioctl_enum_framesizes,
> --
> 2.30.0
>


-- 
Ricardo Ribalda