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

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

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