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

[Sergey Senozhatsky]

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?