Hi
On Wed, Mar 17, 2021 at 2:59 AM Sergey Senozhatsky
<sergey.senozhatsky.work@xxxxxxxxx> 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
