| Message ID | 20210208051749.1785246-1-sergey.senozhatsky@gmail.com |
|---|---|
| Headers | show |
| Series | Add UVC 1.5 Region Of Interest control to uvcvideo | expand |
On (21/02/08 14:17), Sergey Senozhatsky wrote: > Hello, > > RFC Hi Laurent, Gentle ping. -ss
Hi Sergey Thanks for the patch! On Mon, Feb 8, 2021 at 6:21 AM Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote: > > From: Sergey Senozhatsky <senozhatsky@chromium.org> > > Document new v4l2-selection target which will be used for the > Region of Interest v4l2 control. > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > .../media/v4l/selection-api-configuration.rst | 23 +++++++++++++++++++ > .../media/v4l/v4l2-selection-targets.rst | 21 +++++++++++++++++ > include/uapi/linux/v4l2-common.h | 8 +++++++ > 3 files changed, 52 insertions(+) > > diff --git a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst > index fee49bf1a1c0..9f69d71803f6 100644 > --- a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst > +++ b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst > @@ -135,3 +135,26 @@ and the height of rectangles obtained using ``V4L2_SEL_TGT_CROP`` and > ``V4L2_SEL_TGT_COMPOSE`` targets. If these are not equal then the > scaling is applied. The application can compute the scaling ratios using > these values. > + > +Configuration of Region of Interest (ROI) > +========================================= > + > +The range of coordinates of the top left corner, width and height of > +areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` target. > +It is recommended for the driver developers to put the top/left corner > +at position ``(0,0)``. The rectangle's coordinates are in global sensor > +coordinates. The units are in pixels and independent of the field of view. > +They are not impacted by any cropping or scaling that is currently being > +used. Can we also mention binning here? > + > +The top left corner, width and height of the Region of Interest area > +currently being employed by the device is given by the > +``V4L2_SEL_TGT_ROI_CURRENT`` target. It uses the same coordinate system > +as ``V4L2_SEL_TGT_ROI_BOUNDS``. Why do we need current? Cant we just read back V4L2_SEL_TGT_ROI ? > + > +In order to change active ROI top left, width and height coordinates > +use ``V4L2_SEL_TGT_ROI`` target. > + > +Each capture device has a default ROI rectangle, given by the > +``V4L2_SEL_TGT_ROI_DEFAULT`` target. Drivers shall set the ROI rectangle > +to the default when the driver is first loaded, but not later. > diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > index e877ebbdb32e..cb3809418fa6 100644 > --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst > @@ -69,3 +69,24 @@ of the two interfaces they are used. > modified by hardware. > - Yes > - No > + * - ``V4L2_SEL_TGT_ROI_CURRENT`` > + - 0x0200 > + - Current Region of Interest rectangle. > + - Yes > + - No > + * - ``V4L2_SEL_TGT_ROI_DEFAULT`` > + - 0x0201 > + - Suggested Region of Interest rectangle. > + - Yes > + - No > + * - ``V4L2_SEL_TGT_ROI_BOUNDS`` > + - 0x0202 > + - Bounds of the Region of Interest rectangle. All valid ROI rectangles fit > + inside the ROI bounds rectangle. > + - Yes > + - No > + * - ``V4L2_SEL_TGT_ROI`` > + - 0x0203 > + - Sets the new Region of Interest rectangle. > + - Yes > + - No As mentioned before I think we should not have TGT_ROI_CURRENT and TGT_ROI > diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h > index 7d21c1634b4d..d0c108fba638 100644 > --- a/include/uapi/linux/v4l2-common.h > +++ b/include/uapi/linux/v4l2-common.h > @@ -78,6 +78,14 @@ > #define V4L2_SEL_TGT_COMPOSE_BOUNDS 0x0102 > /* Current composing area plus all padding pixels */ > #define V4L2_SEL_TGT_COMPOSE_PADDED 0x0103 > +/* Current Region of Interest area */ > +#define V4L2_SEL_TGT_ROI_CURRENT 0x0200 > +/* Default Region of Interest area */ > +#define V4L2_SEL_TGT_ROI_DEFAULT 0x0201 > +/* Region of Interest bounds */ > +#define V4L2_SEL_TGT_ROI_BOUNDS 0x0202 > +/* Set Region of Interest area */ > +#define V4L2_SEL_TGT_ROI 0x0203 Nit: Maybe it could be a good idea to split doc and code. This way the backports/fixes are easier. > > /* Selection flags */ > #define V4L2_SEL_FLAG_GE (1 << 0) > -- > 2.30.0 > -- Ricardo Ribalda
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
On (21/03/16 19:19), Ricardo Ribalda Delgado wrote: > > +Configuration of Region of Interest (ROI) > > +========================================= > > + > > +The range of coordinates of the top left corner, width and height of > > +areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` target. > > +It is recommended for the driver developers to put the top/left corner > > +at position ``(0,0)``. The rectangle's coordinates are in global sensor > > +coordinates. The units are in pixels and independent of the field of view. > > +They are not impacted by any cropping or scaling that is currently being > > +used. > > Can we also mention binning here? What's binning? Is it in the UVC spec? > > +The top left corner, width and height of the Region of Interest area > > +currently being employed by the device is given by the > > +``V4L2_SEL_TGT_ROI_CURRENT`` target. It uses the same coordinate system > > +as ``V4L2_SEL_TGT_ROI_BOUNDS``. > > Why do we need current? Cant we just read back V4L2_SEL_TGT_ROI ? We don't. Will remove it. > > + * - ``V4L2_SEL_TGT_ROI_CURRENT`` > > + - 0x0200 > > + - Current Region of Interest rectangle. > > + - Yes > > + - No > > + * - ``V4L2_SEL_TGT_ROI_DEFAULT`` > > + - 0x0201 > > + - Suggested Region of Interest rectangle. > > + - Yes > > + - No > > + * - ``V4L2_SEL_TGT_ROI_BOUNDS`` > > + - 0x0202 > > + - Bounds of the Region of Interest rectangle. All valid ROI rectangles fit > > + inside the ROI bounds rectangle. > > + - Yes > > + - No > > + * - ``V4L2_SEL_TGT_ROI`` > > + - 0x0203 > > + - Sets the new Region of Interest rectangle. > > + - Yes > > + - No > As mentioned before I think we should not have TGT_ROI_CURRENT and TGT_ROI Agreed. > > diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h > > index 7d21c1634b4d..d0c108fba638 100644 > > --- a/include/uapi/linux/v4l2-common.h > > +++ b/include/uapi/linux/v4l2-common.h > > @@ -78,6 +78,14 @@ > > #define V4L2_SEL_TGT_COMPOSE_BOUNDS 0x0102 > > /* Current composing area plus all padding pixels */ > > #define V4L2_SEL_TGT_COMPOSE_PADDED 0x0103 > > +/* Current Region of Interest area */ > > +#define V4L2_SEL_TGT_ROI_CURRENT 0x0200 > > +/* Default Region of Interest area */ > > +#define V4L2_SEL_TGT_ROI_DEFAULT 0x0201 > > +/* Region of Interest bounds */ > > +#define V4L2_SEL_TGT_ROI_BOUNDS 0x0202 > > +/* Set Region of Interest area */ > > +#define V4L2_SEL_TGT_ROI 0x0203 > > Nit: Maybe it could be a good idea to split doc and code. This way the > backports/fixes are easier. I'm quite sure this is the first time I'm being asked to split code and documentation :) I'm usually asked to do the opposite - merge code and documentation.
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?
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
Hi On Wed, Mar 17, 2021 at 2:31 AM Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: > > On (21/03/16 19:19), Ricardo Ribalda Delgado wrote: > > > +Configuration of Region of Interest (ROI) > > > +========================================= > > > + > > > +The range of coordinates of the top left corner, width and height of > > > +areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` target. > > > +It is recommended for the driver developers to put the top/left corner > > > +at position ``(0,0)``. The rectangle's coordinates are in global sensor > > > +coordinates. The units are in pixels and independent of the field of view. > > > +They are not impacted by any cropping or scaling that is currently being > > > +used. > > > > Can we also mention binning here? > > What's binning? Is it in the UVC spec? Binning is when you reduce an image by adding up surrounding pixels. So you have a 100x100 image that you convert to a 50x50 but showing the same area of interest. > > > > +The top left corner, width and height of the Region of Interest area > > > +currently being employed by the device is given by the > > > +``V4L2_SEL_TGT_ROI_CURRENT`` target. It uses the same coordinate system > > > +as ``V4L2_SEL_TGT_ROI_BOUNDS``. > > > > Why do we need current? Cant we just read back V4L2_SEL_TGT_ROI ? > > We don't. Will remove it. > > > > + * - ``V4L2_SEL_TGT_ROI_CURRENT`` > > > + - 0x0200 > > > + - Current Region of Interest rectangle. > > > + - Yes > > > + - No > > > + * - ``V4L2_SEL_TGT_ROI_DEFAULT`` > > > + - 0x0201 > > > + - Suggested Region of Interest rectangle. > > > + - Yes > > > + - No > > > + * - ``V4L2_SEL_TGT_ROI_BOUNDS`` > > > + - 0x0202 > > > + - Bounds of the Region of Interest rectangle. All valid ROI rectangles fit > > > + inside the ROI bounds rectangle. > > > + - Yes > > > + - No > > > + * - ``V4L2_SEL_TGT_ROI`` > > > + - 0x0203 > > > + - Sets the new Region of Interest rectangle. > > > + - Yes > > > + - No > > As mentioned before I think we should not have TGT_ROI_CURRENT and TGT_ROI > > Agreed. > > > > diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h > > > index 7d21c1634b4d..d0c108fba638 100644 > > > --- a/include/uapi/linux/v4l2-common.h > > > +++ b/include/uapi/linux/v4l2-common.h > > > @@ -78,6 +78,14 @@ > > > #define V4L2_SEL_TGT_COMPOSE_BOUNDS 0x0102 > > > /* Current composing area plus all padding pixels */ > > > #define V4L2_SEL_TGT_COMPOSE_PADDED 0x0103 > > > +/* Current Region of Interest area */ > > > +#define V4L2_SEL_TGT_ROI_CURRENT 0x0200 > > > +/* Default Region of Interest area */ > > > +#define V4L2_SEL_TGT_ROI_DEFAULT 0x0201 > > > +/* Region of Interest bounds */ > > > +#define V4L2_SEL_TGT_ROI_BOUNDS 0x0202 > > > +/* Set Region of Interest area */ > > > +#define V4L2_SEL_TGT_ROI 0x0203 > > > > Nit: Maybe it could be a good idea to split doc and code. This way the > > backports/fixes are easier. > > I'm quite sure this is the first time I'm being asked to split code > and documentation :) I'm usually asked to do the opposite - merge code > and documentation. I got answered in both directions. I prefer to split it because the doc can go to different audience than the code, and then it makes my life easier when backporting. But if you or Laurent prefer otherwise I am of course happy with any option ;) -- Ricardo Ribalda
On (21/03/17 09:04), Ricardo Ribalda Delgado wrote: > On Wed, Mar 17, 2021 at 2:31 AM Sergey Senozhatsky > <sergey.senozhatsky.work@gmail.com> wrote: > > > > On (21/03/16 19:19), Ricardo Ribalda Delgado wrote: > > > > +Configuration of Region of Interest (ROI) > > > > +========================================= > > > > + > > > > +The range of coordinates of the top left corner, width and height of > > > > +areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` target. > > > > +It is recommended for the driver developers to put the top/left corner > > > > +at position ``(0,0)``. The rectangle's coordinates are in global sensor > > > > +coordinates. The units are in pixels and independent of the field of view. > > > > +They are not impacted by any cropping or scaling that is currently being > > > > +used. > > > > > > Can we also mention binning here? > > > > What's binning? Is it in the UVC spec? > > Binning is when you reduce an image by adding up surrounding pixels. > > So you have a 100x100 image that you convert to a 50x50 but showing > the same area of interest. I see. Hmm, not sure if I can comment on this. It's not in the spec, so it might be up to the firmware, maybe. What do you think? > > > > diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h > > > > index 7d21c1634b4d..d0c108fba638 100644 > > > > --- a/include/uapi/linux/v4l2-common.h > > > > +++ b/include/uapi/linux/v4l2-common.h > > > > @@ -78,6 +78,14 @@ > > > > #define V4L2_SEL_TGT_COMPOSE_BOUNDS 0x0102 > > > > /* Current composing area plus all padding pixels */ > > > > #define V4L2_SEL_TGT_COMPOSE_PADDED 0x0103 > > > > +/* Current Region of Interest area */ > > > > +#define V4L2_SEL_TGT_ROI_CURRENT 0x0200 > > > > +/* Default Region of Interest area */ > > > > +#define V4L2_SEL_TGT_ROI_DEFAULT 0x0201 > > > > +/* Region of Interest bounds */ > > > > +#define V4L2_SEL_TGT_ROI_BOUNDS 0x0202 > > > > +/* Set Region of Interest area */ > > > > +#define V4L2_SEL_TGT_ROI 0x0203 > > > > > > Nit: Maybe it could be a good idea to split doc and code. This way the > > > backports/fixes are easier. > > > > I'm quite sure this is the first time I'm being asked to split code > > and documentation :) I'm usually asked to do the opposite - merge code > > and documentation. > > I got answered in both directions. I prefer to split it because the > doc can go to different audience than the code, and then it makes my > life easier when backporting. > > But if you or Laurent prefer otherwise I am of course happy with any option ;) Either way works for me. Laurent, any preferences? -ss
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
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
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 -- Ricardo Ribalda
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"); ---
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"); > --- -- Ricardo Ribalda