diff mbox series

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

Message ID 20210208051749.1785246-4-sergey.senozhatsky@gmail.com (mailing list archive)
State New, archived
Headers show
Series Add UVC 1.5 Region Of Interest control to uvcvideo | expand

Commit Message

Sergey Senozhatsky Feb. 8, 2021, 5:17 a.m. UTC 
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(-)

Comments

Ricardo Ribalda Delgado March 16, 2021, 6:46 p.m. UTC | #1 
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
>
Sergey Senozhatsky March 17, 2021, 1:59 a.m. UTC | #2 
On (21/03/16 19:46), Ricardo Ribalda Delgado wrote:
> > -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; ?

Perhaps.

> > +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?

Between V4L2_CID_REGION_OF_INTEREST_AUTO and this path?
Hmm. They write to different offsets and don't seem to be overlapping.

> > +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);
[...]
> > +       if ((u16)sel->r.width > stream->cur_frame->wWidth)
> > +               ok = false;
> > +       if ((u16)sel->r.height > stream->cur_frame->wHeight)
> > +               ok = false;

> Maybe you should not release this mutex until query_ctrl is done?

Maybe... These two tests are somewhat made up. Not sure if we need them.
On one hand it's reasonable to keep ROI within the frames. On the other
hand - such relation is not spelled out in the spec. If the stream change
its frame dimensions ROI is not getting invalidated or re-calculated by
the firmware. UVC spec says that ROI should be bounded only by the
CT_DIGITAL_WINDOW_CONTROL (GET_MIN / GET_MAX), ut we don't implement
WINDOW_CONTROL.

So maybe I can remove stream->cuf_frame boundaries check instead.

> > +       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

GET_CUR?

> 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.

What is the adjusted rectangle here? Does this mean that firmware can
successfully apply SET_CUR and return 0, but in reality it was not happy
with the rectangle dimensions so it modified it behind the scenes?

I can add GET_CUR I guess, but do we want to double the traffic, given
that ROI SET_CUR can be executed relatively often?
Ricardo Ribalda Delgado March 17, 2021, 7:58 a.m. UTC | #3 
Hi

On Wed, Mar 17, 2021 at 2:59 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> On (21/03/16 19:46), Ricardo Ribalda Delgado wrote:
> > > -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; ?
>
> Perhaps.
>
> > > +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?
>
> Between V4L2_CID_REGION_OF_INTEREST_AUTO and this path?
> Hmm. They write to different offsets and don't seem to be overlapping.
>
> > > +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);
> [...]
> > > +       if ((u16)sel->r.width > stream->cur_frame->wWidth)
> > > +               ok = false;
> > > +       if ((u16)sel->r.height > stream->cur_frame->wHeight)
> > > +               ok = false;
>
> > Maybe you should not release this mutex until query_ctrl is done?
>
> Maybe... These two tests are somewhat made up. Not sure if we need them.
> On one hand it's reasonable to keep ROI within the frames. On the other
> hand - such relation is not spelled out in the spec. If the stream change
> its frame dimensions ROI is not getting invalidated or re-calculated by
> the firmware. UVC spec says that ROI should be bounded only by the
> CT_DIGITAL_WINDOW_CONTROL (GET_MIN / GET_MAX), ut we don't implement
> WINDOW_CONTROL.

I would remove this check completely then, and rely on set_cur,
get_cur. Leave only the < 0x10000 check

>
> So maybe I can remove stream->cuf_frame boundaries check instead.
>
> > > +       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
>
> GET_CUR?
yep

>
> > 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.
>
> What is the adjusted rectangle here? Does this mean that firmware can
> successfully apply SET_CUR and return 0, but in reality it was not happy
> with the rectangle dimensions so it modified it behind the scenes?

I can imagine that some hw might have spooky requirements for the roi
rectangle (multiple of 4, not crossing the bayer filter, odd/even
line...) so they might be able to go the closest valid config.


>
> I can add GET_CUR I guess, but do we want to double the traffic, given
> that ROI SET_CUR can be executed relatively often?



--
Ricardo Ribalda
Sergey Senozhatsky March 18, 2021, 4:47 a.m. UTC | #4 
On (21/03/17 08:58), Ricardo Ribalda Delgado wrote:
[..]
> >
> > GET_CUR?
> yep
> 
> >
> > > 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.
> >
> > What is the adjusted rectangle here? Does this mean that firmware can
> > successfully apply SET_CUR and return 0, but in reality it was not happy
> > with the rectangle dimensions so it modified it behind the scenes?
> 
> I can imagine that some hw might have spooky requirements for the roi
> rectangle (multiple of 4, not crossing the bayer filter, odd/even
> line...) so they might be able to go the closest valid config.

Hmm. Honestly, I'm very unsure about it. ROI::SET_CUR can be a very
hot path, depending on what user-space considers to be of interest
and how frequently that object of interest changes its position/shape/etc.
Doing GET_CUR after every SET_CUR doubles the number of firmware calls
we issue, that's for sure; is it worth it - that's something that I'm
not sure of.

May I please ask for more opinions on this?

	-ss
Ricardo Ribalda March 18, 2021, 9:19 p.m. UTC | #5 
Hi Sergey

On Thu, Mar 18, 2021 at 5:47 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> On (21/03/17 08:58), Ricardo Ribalda Delgado wrote:
> [..]
> > >
> > > GET_CUR?
> > yep
> >
> > >
> > > > 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.
> > >
> > > What is the adjusted rectangle here? Does this mean that firmware can
> > > successfully apply SET_CUR and return 0, but in reality it was not happy
> > > with the rectangle dimensions so it modified it behind the scenes?
> >
> > I can imagine that some hw might have spooky requirements for the roi
> > rectangle (multiple of 4, not crossing the bayer filter, odd/even
> > line...) so they might be able to go the closest valid config.
>
> Hmm. Honestly, I'm very unsure about it. ROI::SET_CUR can be a very
> hot path, depending on what user-space considers to be of interest
> and how frequently that object of interest changes its position/shape/etc.
> Doing GET_CUR after every SET_CUR doubles the number of firmware calls
> we issue, that's for sure; is it worth it - that's something that I'm
> not sure of.
>
> May I please ask for more opinions on this?

Could you try setting the roi in a loop in your device and verify that
it accepts all the values with no modification. If so we can implement
the set/get as a quirk for other devices.

>
>         -ss
Ricardo Ribalda March 18, 2021, 9:20 p.m. UTC | #6 
On Thu, Mar 18, 2021 at 10:19 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Hi Sergey
>
> On Thu, Mar 18, 2021 at 5:47 AM Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
> >
> > On (21/03/17 08:58), Ricardo Ribalda Delgado wrote:
> > [..]
> > > >
> > > > GET_CUR?
> > > yep
> > >
> > > >
> > > > > 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.
> > > >
> > > > What is the adjusted rectangle here? Does this mean that firmware can
> > > > successfully apply SET_CUR and return 0, but in reality it was not happy
> > > > with the rectangle dimensions so it modified it behind the scenes?
> > >
> > > I can imagine that some hw might have spooky requirements for the roi
> > > rectangle (multiple of 4, not crossing the bayer filter, odd/even
> > > line...) so they might be able to go the closest valid config.
> >
> > Hmm. Honestly, I'm very unsure about it. ROI::SET_CUR can be a very
> > hot path, depending on what user-space considers to be of interest
> > and how frequently that object of interest changes its position/shape/etc.
> > Doing GET_CUR after every SET_CUR doubles the number of firmware calls
> > we issue, that's for sure; is it worth it - that's something that I'm
> > not sure of.
> >
> > May I please ask for more opinions on this?
>
> Could you try setting the roi in a loop in your device and verify that
> it accepts all the values with no modification. If so we can implement
> the set/get as a quirk for other devices.

as a loop I mean testing all the values not the same value again-and-again
;)


>
> >
> >         -ss
>
>
>
> --
> Ricardo Ribalda
Sergey Senozhatsky March 19, 2021, 5:35 a.m. UTC | #7 
On (21/03/18 22:19), Ricardo Ribalda wrote:
> >
> > May I please ask for more opinions on this?
> 
> Could you try setting the roi in a loop in your device and verify that
> it accepts all the values with no modification. If so we can implement
> the set/get as a quirk for other devices.

Tested on two (very) different devices.

Firmware D:

   CLAP all passed, we are cool

Firmware H:

   CLAP all passed, we are cool


Code sample

---
       in_selection.target     = V4L2_SEL_TGT_ROI;
       in_selection.flags      = V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE;

       for (int i = 0; i < 1001; i++) {
               in_selection.r.left     = 0 + i;
               in_selection.r.top      = 0 + i;
               in_selection.r.width    = 42 + i;
               in_selection.r.height   = 42 + i;

               ret = doioctl(fd, VIDIOC_S_SELECTION, &in_selection);
               if (ret) {
                       fprintf(stderr, "BOOM %d\n", ret);
                       exit(1);
               }

               ret = doioctl(fd, VIDIOC_G_SELECTION, &in_selection);
               if (ret) {
                       fprintf(stderr, "BANG %d\n", ret);
                       exit(2);
               }

               if (in_selection.r.left != i ||
                   in_selection.r.top != i ||
                   in_selection.r.width != i + 42 ||
                   in_selection.r.height != i + 42) {

                       fprintf(stderr, "SNAP %d %d %d %d != %d %d %d %d\n",
                               i, i, i + 42, i + 42,
                               in_selection.r.left,
                               in_selection.r.top,
                               in_selection.r.width,
                               in_selection.r.height);
                       exit(3);
               }
       }

       fprintf(stderr, "CLAP all passed, we are cool\n");
---
Ricardo Ribalda March 19, 2021, 4:40 p.m. UTC | #8 
Hi Sergey

On Fri, Mar 19, 2021 at 6:35 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (21/03/18 22:19), Ricardo Ribalda wrote:
> > >
> > > May I please ask for more opinions on this?
> >
> > Could you try setting the roi in a loop in your device and verify that
> > it accepts all the values with no modification. If so we can implement
> > the set/get as a quirk for other devices.
>
> Tested on two (very) different devices.

Ahoy, Matey ;)

That is great news. Thanks for testing this.


>
> Firmware D:
>
>    CLAP all passed, we are cool
>
> Firmware H:
>
>    CLAP all passed, we are cool
>
>
> Code sample
>
> ---
>        in_selection.target     = V4L2_SEL_TGT_ROI;
>        in_selection.flags      = V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE;
>
>        for (int i = 0; i < 1001; i++) {
>                in_selection.r.left     = 0 + i;
>                in_selection.r.top      = 0 + i;
>                in_selection.r.width    = 42 + i;
>                in_selection.r.height   = 42 + i;
>
>                ret = doioctl(fd, VIDIOC_S_SELECTION, &in_selection);
>                if (ret) {
>                        fprintf(stderr, "BOOM %d\n", ret);
>                        exit(1);
>                }
>
>                ret = doioctl(fd, VIDIOC_G_SELECTION, &in_selection);
>                if (ret) {
>                        fprintf(stderr, "BANG %d\n", ret);
>                        exit(2);
>                }
>
>                if (in_selection.r.left != i ||
>                    in_selection.r.top != i ||
>                    in_selection.r.width != i + 42 ||
>                    in_selection.r.height != i + 42) {
>
>                        fprintf(stderr, "SNAP %d %d %d %d != %d %d %d %d\n",
>                                i, i, i + 42, i + 42,
>                                in_selection.r.left,
>                                in_selection.r.top,
>                                in_selection.r.width,
>                                in_selection.r.height);
>                        exit(3);
>                }
>        }
>
>        fprintf(stderr, "CLAP all passed, we are cool\n");
> ---
diff mbox series

Patch

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;
+};
+
+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));
+	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;
+}
+
+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);
+	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));
+
+	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,