| Message ID | 20220628075705.2278044-1-yunkec@google.com |
|---|---|
| Headers | show |
| Series | media: Implement UVC v1.5 ROI | expand |
Hi Laurent, Do you have time to review this version? Ricardo has already reviewed it, I hope it is easier to review now. Thanks in advance! Yunke On Tue, Jun 28, 2022 at 4:57 PM Yunke Cao <yunkec@google.com> wrote: > > This patch set implements UVC v1.5 region of interest using V4L2 > control API. > > ROI control is consisted two uvc specific controls. > 1. A rectangle control with a newly added type V4L2_CTRL_TYPE_RECT. > 2. An auto control with type bitmask. > > V4L2_CTRL_WHICH_MIN/MAX_VAL is added to support the rectangle control. > > Tested on two different usb cameras using v4l2-compliance, v4l2-ctl > and calling ioctls. > > 1/7 add V4L2_CTRL_TYPE_RECT. > 2/7 and 3/7 support compound types in UVC. > 4/7 implement ROI in UVC. > 5/7 is a cherry-pick for Hans' implementation of > V4L2_CTRL_WHICH_MIN/MAX_VAL in v4l2-core. > 6/7 support MIN/MAX in UVC. > 7/7 document the changes. > > Changelog since v6: > -Add patch 2 and 3 to support compound types properly in UVC and > implement ROI on top of them. > -Reorder the patches. > > Changelog since v5: > -Add a __uvc_ctrl_get_p_rect_to_user instead of modifying > __uvc_ctrl_get. > -Support V4L2_CTRL_FLAG_NEXT_COMPOUND correctly. > -Fix formats. > > Changelog since v4: > -Cherry-pick the original patch > "v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL". > -Split patch "media: uvcvideo: implement UVC v1.5 ROI" into two patches. > The codes for supporting min/max in uvc are in patch 4/5 now. > -Minor fixes. Detailed changelog in patches > > Changelog since v3: > - Reordered/sliced the patches. > 1. Add rect type. > 2. Add min/max. > 3. Add the roi controls (including init to default). > 4. Document the roi controls. > - Define the roi controls as uvc-specific in uvcvideo.h. > - Modified documentation. > - Removed the vivid change. Given the controls are now uvc-specific. > I'm not sure how valuable it is to add it in vivid. Let me know > otherwise. > > Hans Verkuil (1): > v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL > > Yunke Cao (6): > media: v4l2_ctrl: Add V4L2_CTRL_TYPE_RECT > media: uvcvideo: add uvc_ctrl_get_fixed for getting default value > media: uvcvideo: Add support for compound controls > media: uvcvideo: implement UVC v1.5 ROI > media: uvcvideo: support V4L2_CTRL_WHICH_MIN/MAX_VAL > media: uvcvideo: document UVC v1.5 ROI > > .../userspace-api/media/drivers/uvcvideo.rst | 61 +++ > .../media/v4l/vidioc-g-ext-ctrls.rst | 11 +- > .../media/videodev2.h.rst.exceptions | 3 + > drivers/media/i2c/imx214.c | 5 +- > .../media/platform/qcom/venus/venc_ctrls.c | 4 + > drivers/media/usb/uvc/uvc_ctrl.c | 479 ++++++++++++++++-- > drivers/media/usb/uvc/uvc_v4l2.c | 20 +- > drivers/media/usb/uvc/uvcvideo.h | 14 + > drivers/media/v4l2-core/v4l2-ctrls-api.c | 51 +- > drivers/media/v4l2-core/v4l2-ctrls-core.c | 155 +++++- > drivers/media/v4l2-core/v4l2-ioctl.c | 4 +- > include/media/v4l2-ctrls.h | 34 +- > include/uapi/linux/usb/video.h | 1 + > include/uapi/linux/uvcvideo.h | 13 + > include/uapi/linux/v4l2-controls.h | 8 + > include/uapi/linux/videodev2.h | 4 + > 16 files changed, 788 insertions(+), 79 deletions(-) > > -- > 2.37.0.rc0.161.g10f37bed90-goog >
Hi Yunke, On Fri, Jul 15, 2022 at 08:25:20AM +0900, Yunke Cao wrote: > Hi Laurent, > > Do you have time to review this version? Ricardo has already reviewed > it, I hope it is easier to review now. I'll try find time, but I doubt it will be before a couple of weeks. > On Tue, Jun 28, 2022 at 4:57 PM Yunke Cao <yunkec@google.com> wrote: > > > > This patch set implements UVC v1.5 region of interest using V4L2 > > control API. > > > > ROI control is consisted two uvc specific controls. > > 1. A rectangle control with a newly added type V4L2_CTRL_TYPE_RECT. > > 2. An auto control with type bitmask. > > > > V4L2_CTRL_WHICH_MIN/MAX_VAL is added to support the rectangle control. > > > > Tested on two different usb cameras using v4l2-compliance, v4l2-ctl > > and calling ioctls. > > > > 1/7 add V4L2_CTRL_TYPE_RECT. > > 2/7 and 3/7 support compound types in UVC. > > 4/7 implement ROI in UVC. > > 5/7 is a cherry-pick for Hans' implementation of > > V4L2_CTRL_WHICH_MIN/MAX_VAL in v4l2-core. > > 6/7 support MIN/MAX in UVC. > > 7/7 document the changes. > > > > Changelog since v6: > > -Add patch 2 and 3 to support compound types properly in UVC and > > implement ROI on top of them. > > -Reorder the patches. > > > > Changelog since v5: > > -Add a __uvc_ctrl_get_p_rect_to_user instead of modifying > > __uvc_ctrl_get. > > -Support V4L2_CTRL_FLAG_NEXT_COMPOUND correctly. > > -Fix formats. > > > > Changelog since v4: > > -Cherry-pick the original patch > > "v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL". > > -Split patch "media: uvcvideo: implement UVC v1.5 ROI" into two patches. > > The codes for supporting min/max in uvc are in patch 4/5 now. > > -Minor fixes. Detailed changelog in patches > > > > Changelog since v3: > > - Reordered/sliced the patches. > > 1. Add rect type. > > 2. Add min/max. > > 3. Add the roi controls (including init to default). > > 4. Document the roi controls. > > - Define the roi controls as uvc-specific in uvcvideo.h. > > - Modified documentation. > > - Removed the vivid change. Given the controls are now uvc-specific. > > I'm not sure how valuable it is to add it in vivid. Let me know > > otherwise. > > > > Hans Verkuil (1): > > v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL > > > > Yunke Cao (6): > > media: v4l2_ctrl: Add V4L2_CTRL_TYPE_RECT > > media: uvcvideo: add uvc_ctrl_get_fixed for getting default value > > media: uvcvideo: Add support for compound controls > > media: uvcvideo: implement UVC v1.5 ROI > > media: uvcvideo: support V4L2_CTRL_WHICH_MIN/MAX_VAL > > media: uvcvideo: document UVC v1.5 ROI > > > > .../userspace-api/media/drivers/uvcvideo.rst | 61 +++ > > .../media/v4l/vidioc-g-ext-ctrls.rst | 11 +- > > .../media/videodev2.h.rst.exceptions | 3 + > > drivers/media/i2c/imx214.c | 5 +- > > .../media/platform/qcom/venus/venc_ctrls.c | 4 + > > drivers/media/usb/uvc/uvc_ctrl.c | 479 ++++++++++++++++-- > > drivers/media/usb/uvc/uvc_v4l2.c | 20 +- > > drivers/media/usb/uvc/uvcvideo.h | 14 + > > drivers/media/v4l2-core/v4l2-ctrls-api.c | 51 +- > > drivers/media/v4l2-core/v4l2-ctrls-core.c | 155 +++++- > > drivers/media/v4l2-core/v4l2-ioctl.c | 4 +- > > include/media/v4l2-ctrls.h | 34 +- > > include/uapi/linux/usb/video.h | 1 + > > include/uapi/linux/uvcvideo.h | 13 + > > include/uapi/linux/v4l2-controls.h | 8 + > > include/uapi/linux/videodev2.h | 4 + > > 16 files changed, 788 insertions(+), 79 deletions(-)
Hi Yunke, Thank you for the patch. On Tue, Jun 28, 2022 at 04:56:59PM +0900, Yunke Cao wrote: > Add p_rect to struct v4l2_ext_control with basic support in > v4l2-ctrls. This looks good to me, so Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I'm no expert on the V4L2 control framework though, a review from Hans would be useful. > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > Signed-off-by: Yunke Cao <yunkec@google.com> > --- > .../media/v4l/vidioc-g-ext-ctrls.rst | 4 ++++ > .../media/videodev2.h.rst.exceptions | 1 + > drivers/media/v4l2-core/v4l2-ctrls-core.c | 20 +++++++++++++++++++ > include/media/v4l2-ctrls.h | 2 ++ > include/uapi/linux/videodev2.h | 2 ++ > 5 files changed, 29 insertions(+) > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > index 29971a45a2d4..7473baa4e977 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > @@ -189,6 +189,10 @@ still cause this situation. > - ``p_area`` > - A pointer to a struct :c:type:`v4l2_area`. Valid if this control is > of type ``V4L2_CTRL_TYPE_AREA``. > + * - struct :c:type:`v4l2_rect` * > + - ``p_rect`` > + - A pointer to a struct :c:type:`v4l2_rect`. Valid if this control is > + of type ``V4L2_CTRL_TYPE_RECT``. > * - struct :c:type:`v4l2_ctrl_h264_sps` * > - ``p_h264_sps`` > - A pointer to a struct :c:type:`v4l2_ctrl_h264_sps`. Valid if this control is > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > index 9cbb7a0c354a..7b423475281d 100644 > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > @@ -147,6 +147,7 @@ replace symbol V4L2_CTRL_TYPE_HEVC_SPS :c:type:`v4l2_ctrl_type` > replace symbol V4L2_CTRL_TYPE_HEVC_PPS :c:type:`v4l2_ctrl_type` > replace symbol V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS :c:type:`v4l2_ctrl_type` > replace symbol V4L2_CTRL_TYPE_AREA :c:type:`v4l2_ctrl_type` > +replace symbol V4L2_CTRL_TYPE_RECT :c:type:`v4l2_ctrl_type` > replace symbol V4L2_CTRL_TYPE_FWHT_PARAMS :c:type:`v4l2_ctrl_type` > replace symbol V4L2_CTRL_TYPE_VP8_FRAME :c:type:`v4l2_ctrl_type` > replace symbol V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR :c:type:`v4l2_ctrl_type` > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c > index 949c1884d9c1..35d43ba650db 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c > @@ -84,6 +84,11 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx, > return ptr1.p_u16[idx] == ptr2.p_u16[idx]; > case V4L2_CTRL_TYPE_U32: > return ptr1.p_u32[idx] == ptr2.p_u32[idx]; > + case V4L2_CTRL_TYPE_RECT: > + return ptr1.p_rect->top == ptr2.p_rect->top && > + ptr1.p_rect->left == ptr2.p_rect->left && > + ptr1.p_rect->height == ptr2.p_rect->height && > + ptr1.p_rect->width == ptr2.p_rect->width; > default: > if (ctrl->is_int) > return ptr1.p_s32[idx] == ptr2.p_s32[idx]; > @@ -307,6 +312,11 @@ static void std_log(const struct v4l2_ctrl *ctrl) > case V4L2_CTRL_TYPE_VP9_FRAME: > pr_cont("VP9_FRAME"); > break; > + case V4L2_CTRL_TYPE_RECT: > + pr_cont("%ux%u@%dx%d", > + ptr.p_rect->width, ptr.p_rect->height, > + ptr.p_rect->left, ptr.p_rect->top); > + break; > default: > pr_cont("unknown type %d", ctrl->type); > break; > @@ -525,6 +535,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx, > struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering; > struct v4l2_ctrl_hevc_decode_params *p_hevc_decode_params; > struct v4l2_area *area; > + struct v4l2_rect *rect; > void *p = ptr.p + idx * ctrl->elem_size; > unsigned int i; > > @@ -888,6 +899,12 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx, > return -EINVAL; > break; > > + case V4L2_CTRL_TYPE_RECT: > + rect = p; > + if (!rect->width || !rect->height) > + return -EINVAL; > + break; > + > default: > return -EINVAL; > } > @@ -1455,6 +1472,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > case V4L2_CTRL_TYPE_AREA: > elem_size = sizeof(struct v4l2_area); > break; > + case V4L2_CTRL_TYPE_RECT: > + elem_size = sizeof(struct v4l2_rect); > + break; > default: > if (type < V4L2_CTRL_COMPOUND_TYPES) > elem_size = sizeof(s32); > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > index b3ce438f1329..919e104de50b 100644 > --- a/include/media/v4l2-ctrls.h > +++ b/include/media/v4l2-ctrls.h > @@ -58,6 +58,7 @@ struct video_device; > * @p_hdr10_cll: Pointer to an HDR10 Content Light Level structure. > * @p_hdr10_mastering: Pointer to an HDR10 Mastering Display structure. > * @p_area: Pointer to an area. > + * @p_rect: Pointer to a rectangle. > * @p: Pointer to a compound value. > * @p_const: Pointer to a constant compound value. > */ > @@ -87,6 +88,7 @@ union v4l2_ctrl_ptr { > struct v4l2_ctrl_hdr10_cll_info *p_hdr10_cll; > struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering; > struct v4l2_area *p_area; > + struct v4l2_rect *p_rect; > void *p; > const void *p_const; > }; > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 343b95107fce..2e36bb610ea6 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -1754,6 +1754,7 @@ struct v4l2_ext_control { > __u16 __user *p_u16; > __u32 __user *p_u32; > struct v4l2_area __user *p_area; > + struct v4l2_rect __user *p_rect; > struct v4l2_ctrl_h264_sps __user *p_h264_sps; > struct v4l2_ctrl_h264_pps *p_h264_pps; > struct v4l2_ctrl_h264_scaling_matrix __user *p_h264_scaling_matrix; > @@ -1813,6 +1814,7 @@ enum v4l2_ctrl_type { > V4L2_CTRL_TYPE_U16 = 0x0101, > V4L2_CTRL_TYPE_U32 = 0x0102, > V4L2_CTRL_TYPE_AREA = 0x0106, > + V4L2_CTRL_TYPE_RECT = 0x0107, > > V4L2_CTRL_TYPE_HDR10_CLL_INFO = 0x0110, > V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY = 0x0111,
Hi Yunke, You will need to rebase this since some of the v4l2-ctrl internals have changed. On 28/06/2022 09:56, Yunke Cao wrote: > Add p_rect to struct v4l2_ext_control with basic support in > v4l2-ctrls. > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > Signed-off-by: Yunke Cao <yunkec@google.com> > --- > .../media/v4l/vidioc-g-ext-ctrls.rst | 4 ++++ > .../media/videodev2.h.rst.exceptions | 1 + > drivers/media/v4l2-core/v4l2-ctrls-core.c | 20 +++++++++++++++++++ > include/media/v4l2-ctrls.h | 2 ++ > include/uapi/linux/videodev2.h | 2 ++ > 5 files changed, 29 insertions(+) > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > index 29971a45a2d4..7473baa4e977 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > @@ -189,6 +189,10 @@ still cause this situation. > - ``p_area`` > - A pointer to a struct :c:type:`v4l2_area`. Valid if this control is > of type ``V4L2_CTRL_TYPE_AREA``. > + * - struct :c:type:`v4l2_rect` * > + - ``p_rect`` > + - A pointer to a struct :c:type:`v4l2_rect`. Valid if this control is > + of type ``V4L2_CTRL_TYPE_RECT``. > * - struct :c:type:`v4l2_ctrl_h264_sps` * > - ``p_h264_sps`` > - A pointer to a struct :c:type:`v4l2_ctrl_h264_sps`. Valid if this control is You also need to update Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst. > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > index 9cbb7a0c354a..7b423475281d 100644 > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > @@ -147,6 +147,7 @@ replace symbol V4L2_CTRL_TYPE_HEVC_SPS :c:type:`v4l2_ctrl_type` > replace symbol V4L2_CTRL_TYPE_HEVC_PPS :c:type:`v4l2_ctrl_type` > replace symbol V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS :c:type:`v4l2_ctrl_type` > replace symbol V4L2_CTRL_TYPE_AREA :c:type:`v4l2_ctrl_type` > +replace symbol V4L2_CTRL_TYPE_RECT :c:type:`v4l2_ctrl_type` > replace symbol V4L2_CTRL_TYPE_FWHT_PARAMS :c:type:`v4l2_ctrl_type` > replace symbol V4L2_CTRL_TYPE_VP8_FRAME :c:type:`v4l2_ctrl_type` > replace symbol V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR :c:type:`v4l2_ctrl_type` > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c > index 949c1884d9c1..35d43ba650db 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c > @@ -84,6 +84,11 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx, > return ptr1.p_u16[idx] == ptr2.p_u16[idx]; > case V4L2_CTRL_TYPE_U32: > return ptr1.p_u32[idx] == ptr2.p_u32[idx]; > + case V4L2_CTRL_TYPE_RECT: > + return ptr1.p_rect->top == ptr2.p_rect->top && > + ptr1.p_rect->left == ptr2.p_rect->left && > + ptr1.p_rect->height == ptr2.p_rect->height && > + ptr1.p_rect->width == ptr2.p_rect->width; You don't need to do anything here, it will fallback to a memcmp, and that's fine for struct v4l2_rect. > default: > if (ctrl->is_int) > return ptr1.p_s32[idx] == ptr2.p_s32[idx]; > @@ -307,6 +312,11 @@ static void std_log(const struct v4l2_ctrl *ctrl) > case V4L2_CTRL_TYPE_VP9_FRAME: > pr_cont("VP9_FRAME"); > break; > + case V4L2_CTRL_TYPE_RECT: > + pr_cont("%ux%u@%dx%d", > + ptr.p_rect->width, ptr.p_rect->height, > + ptr.p_rect->left, ptr.p_rect->top); > + break; > default: > pr_cont("unknown type %d", ctrl->type); > break; > @@ -525,6 +535,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx, > struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering; > struct v4l2_ctrl_hevc_decode_params *p_hevc_decode_params; > struct v4l2_area *area; > + struct v4l2_rect *rect; > void *p = ptr.p + idx * ctrl->elem_size; > unsigned int i; > > @@ -888,6 +899,12 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx, > return -EINVAL; > break; > > + case V4L2_CTRL_TYPE_RECT: > + rect = p; > + if (!rect->width || !rect->height) > + return -EINVAL; > + break; > + > default: > return -EINVAL; > } > @@ -1455,6 +1472,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > case V4L2_CTRL_TYPE_AREA: > elem_size = sizeof(struct v4l2_area); > break; > + case V4L2_CTRL_TYPE_RECT: > + elem_size = sizeof(struct v4l2_rect); > + break; > default: > if (type < V4L2_CTRL_COMPOUND_TYPES) > elem_size = sizeof(s32); > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > index b3ce438f1329..919e104de50b 100644 > --- a/include/media/v4l2-ctrls.h > +++ b/include/media/v4l2-ctrls.h > @@ -58,6 +58,7 @@ struct video_device; > * @p_hdr10_cll: Pointer to an HDR10 Content Light Level structure. > * @p_hdr10_mastering: Pointer to an HDR10 Mastering Display structure. > * @p_area: Pointer to an area. > + * @p_rect: Pointer to a rectangle. > * @p: Pointer to a compound value. > * @p_const: Pointer to a constant compound value. > */ > @@ -87,6 +88,7 @@ union v4l2_ctrl_ptr { > struct v4l2_ctrl_hdr10_cll_info *p_hdr10_cll; > struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering; > struct v4l2_area *p_area; > + struct v4l2_rect *p_rect; > void *p; > const void *p_const; > }; > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 343b95107fce..2e36bb610ea6 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -1754,6 +1754,7 @@ struct v4l2_ext_control { > __u16 __user *p_u16; > __u32 __user *p_u32; > struct v4l2_area __user *p_area; > + struct v4l2_rect __user *p_rect; > struct v4l2_ctrl_h264_sps __user *p_h264_sps; > struct v4l2_ctrl_h264_pps *p_h264_pps; > struct v4l2_ctrl_h264_scaling_matrix __user *p_h264_scaling_matrix; > @@ -1813,6 +1814,7 @@ enum v4l2_ctrl_type { > V4L2_CTRL_TYPE_U16 = 0x0101, > V4L2_CTRL_TYPE_U32 = 0x0102, > V4L2_CTRL_TYPE_AREA = 0x0106, > + V4L2_CTRL_TYPE_RECT = 0x0107, > > V4L2_CTRL_TYPE_HDR10_CLL_INFO = 0x0110, > V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY = 0x0111, Regards, Hans
On 24/08/2022 10:50, Hans Verkuil wrote: > Hi Yunke, > > You will need to rebase this since some of the v4l2-ctrl internals > have changed. Rebase to git://linuxtv.org/media_stage.git, master branch, to be precise. Hans
Hi Yunke and Hans, Thank you for the patch. On Tue, Jun 28, 2022 at 04:57:03PM +0900, Yunke Cao wrote: > From: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > Add the capability of retrieving the min and max values of a > compound control. > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Signed-off-by: Yunke Cao <yunkec@google.com> > --- > git am from https://lore.kernel.org/all/20191119113457.57833-3-hverkuil-cisco@xs4all.nl/ > -Fixed some merge conflits. > -Fixed the build error in drivers/media/platform/qcom/venus. > > .../media/v4l/vidioc-g-ext-ctrls.rst | 7 +- > .../media/videodev2.h.rst.exceptions | 2 + > drivers/media/i2c/imx214.c | 5 +- > .../media/platform/qcom/venus/venc_ctrls.c | 4 + > drivers/media/v4l2-core/v4l2-ctrls-api.c | 51 +++++-- > drivers/media/v4l2-core/v4l2-ctrls-core.c | 135 ++++++++++++++++-- > drivers/media/v4l2-core/v4l2-ioctl.c | 4 +- > include/media/v4l2-ctrls.h | 32 ++++- > include/uapi/linux/videodev2.h | 2 + > 9 files changed, 214 insertions(+), 28 deletions(-) > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > index 7473baa4e977..65a5f878cc28 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > @@ -284,14 +284,17 @@ still cause this situation. > - Which value of the control to get/set/try. > * - :cspan:`2` ``V4L2_CTRL_WHICH_CUR_VAL`` will return the current value of > the control, ``V4L2_CTRL_WHICH_DEF_VAL`` will return the default > + value of the control, ``V4L2_CTRL_WHICH_MIN_VAL`` will return the minimum > + value of the control, ``V4L2_CTRL_WHICH_MAX_VAL`` will return the maximum The concept of minimum and maximum values for compound controls can be quite ill-defined. Could we document here that the definition of the minimum and maximum values are provided by the control documentation for compound controls ? > value of the control and ``V4L2_CTRL_WHICH_REQUEST_VAL`` indicates that > these controls have to be retrieved from a request or tried/set for > a request. In the latter case the ``request_fd`` field contains the > file descriptor of the request that should be used. If the device > does not support requests, then ``EACCES`` will be returned. > > - When using ``V4L2_CTRL_WHICH_DEF_VAL`` be aware that you can only > - get the default value of the control, you cannot set or try it. > + When using ``V4L2_CTRL_WHICH_DEF_VAL``, ``V4L2_CTRL_WHICH_MIN_VAL`` > + or ``V4L2_CTRL_WHICH_MAX_VAL`` be aware that you can only get the > + default/minimum/maximum value of the control, you cannot set or try it. > > For backwards compatibility you can also use a control class here > (see :ref:`ctrl-class`). In that case all controls have to > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > index 7b423475281d..e2dde31d76df 100644 > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > @@ -553,6 +553,8 @@ ignore define V4L2_CTRL_DRIVER_PRIV > ignore define V4L2_CTRL_MAX_DIMS > ignore define V4L2_CTRL_WHICH_CUR_VAL > ignore define V4L2_CTRL_WHICH_DEF_VAL > +ignore define V4L2_CTRL_WHICH_MIN_VAL > +ignore define V4L2_CTRL_WHICH_MAX_VAL > ignore define V4L2_CTRL_WHICH_REQUEST_VAL > ignore define V4L2_OUT_CAP_CUSTOM_TIMINGS > ignore define V4L2_CID_MAX_CTRLS > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c > index 83c1737abeec..aa0bfd18f7b7 100644 > --- a/drivers/media/i2c/imx214.c > +++ b/drivers/media/i2c/imx214.c > @@ -1037,7 +1037,10 @@ static int imx214_probe(struct i2c_client *client) > imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls, > NULL, > V4L2_CID_UNIT_CELL_SIZE, > - v4l2_ctrl_ptr_create((void *)&unit_size)); > + v4l2_ctrl_ptr_create((void *)&unit_size), > + v4l2_ctrl_ptr_create(NULL), > + v4l2_ctrl_ptr_create(NULL)); Would it make sense to set min = max = default for read-only controls ? You should also update the documentation of V4L2_CID_UNIT_CELL_SIZE (and other controls below) to indicate how their minimum and maximum are defined. > + > ret = imx214->ctrls.error; > if (ret) { > dev_err(&client->dev, "%s control init failed (%d)\n", > diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c > index ed44e5800759..4af8f9ca12a6 100644 > --- a/drivers/media/platform/qcom/venus/venc_ctrls.c > +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c > @@ -579,10 +579,14 @@ int venc_ctrl_init(struct venus_inst *inst) > > v4l2_ctrl_new_std_compound(&inst->ctrl_handler, &venc_ctrl_ops, > V4L2_CID_COLORIMETRY_HDR10_CLL_INFO, > + v4l2_ctrl_ptr_create(NULL), > + v4l2_ctrl_ptr_create(NULL), > v4l2_ctrl_ptr_create(NULL)); > > v4l2_ctrl_new_std_compound(&inst->ctrl_handler, &venc_ctrl_ops, > V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY, > + v4l2_ctrl_ptr_create(NULL), > + v4l2_ctrl_ptr_create(NULL), > v4l2_ctrl_ptr_create(NULL)); > > v4l2_ctrl_new_std_menu(&inst->ctrl_handler, &venc_ctrl_ops, > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c > index db9baa0bd05f..8a9c816b0dab 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c > @@ -97,6 +97,28 @@ static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl) > return ptr_to_user(c, ctrl, ctrl->p_new); > } > > +/* Helper function: copy the minimum control value back to the caller */ > +static int min_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl) > +{ > + int idx; > + > + for (idx = 0; idx < ctrl->elems; idx++) > + ctrl->type_ops->minimum(ctrl, idx, ctrl->p_new); > + > + return ptr_to_user(c, ctrl, ctrl->p_new); > +} > + > +/* Helper function: copy the maximum control value back to the caller */ > +static int max_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl) > +{ > + int idx; > + > + for (idx = 0; idx < ctrl->elems; idx++) > + ctrl->type_ops->maximum(ctrl, idx, ctrl->p_new); > + > + return ptr_to_user(c, ctrl, ctrl->p_new); > +} > + > /* Helper function: copy the caller-provider value to the given control value */ > static int user_to_ptr(struct v4l2_ext_control *c, > struct v4l2_ctrl *ctrl, > @@ -220,8 +242,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl, > cs->error_idx = i; > > if (cs->which && > - cs->which != V4L2_CTRL_WHICH_DEF_VAL && > - cs->which != V4L2_CTRL_WHICH_REQUEST_VAL && > + (cs->which < V4L2_CTRL_WHICH_DEF_VAL || > + cs->which > V4L2_CTRL_WHICH_MAX_VAL) && > V4L2_CTRL_ID2WHICH(id) != cs->which) { > dprintk(vdev, > "invalid which 0x%x or control id 0x%x\n", > @@ -335,8 +357,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl, > */ > static int class_check(struct v4l2_ctrl_handler *hdl, u32 which) > { > - if (which == 0 || which == V4L2_CTRL_WHICH_DEF_VAL || > - which == V4L2_CTRL_WHICH_REQUEST_VAL) > + if (which == 0 || (which >= V4L2_CTRL_WHICH_DEF_VAL && > + which <= V4L2_CTRL_WHICH_MAX_VAL)) > return 0; > return find_ref_lock(hdl, which | 1) ? 0 : -EINVAL; > } > @@ -356,10 +378,12 @@ int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl, > struct v4l2_ctrl_helper *helpers = helper; > int ret; > int i, j; > - bool is_default, is_request; > + bool is_default, is_request, is_min, is_max; > > is_default = (cs->which == V4L2_CTRL_WHICH_DEF_VAL); > is_request = (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL); > + is_min = (cs->which == V4L2_CTRL_WHICH_MIN_VAL); > + is_max = (cs->which == V4L2_CTRL_WHICH_MAX_VAL); > > cs->error_idx = cs->count; > cs->which = V4L2_CTRL_ID2WHICH(cs->which); > @@ -399,13 +423,14 @@ int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl, > > /* > * g_volatile_ctrl will update the new control values. > - * This makes no sense for V4L2_CTRL_WHICH_DEF_VAL and > + * This makes no sense for V4L2_CTRL_WHICH_DEF_VAL, > + * V4L2_CTRL_WHICH_MIN_VAL, V4L2_CTRL_WHICH_MAX_VAL and > * V4L2_CTRL_WHICH_REQUEST_VAL. In the case of requests > * it is v4l2_ctrl_request_complete() that copies the > * volatile controls at the time of request completion > * to the request, so you don't want to do that again. > */ > - if (!is_default && !is_request && > + if (!is_default && !is_request && !is_min && !is_max && > ((master->flags & V4L2_CTRL_FLAG_VOLATILE) || > (master->has_volatiles && !is_cur_manual(master)))) { > for (j = 0; j < master->ncontrols; j++) > @@ -432,6 +457,10 @@ int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl, > ret = def_to_user(cs->controls + idx, ref->ctrl); > else if (is_request && ref->valid_p_req) > ret = req_to_user(cs->controls + idx, ref); > + else if (is_min) > + ret = min_to_user(cs->controls + idx, ref->ctrl); > + else if (is_max) > + ret = max_to_user(cs->controls + idx, ref->ctrl); > else if (is_volatile) > ret = new_to_user(cs->controls + idx, ref->ctrl); > else > @@ -523,9 +552,11 @@ int try_set_ext_ctrls_common(struct v4l2_fh *fh, > > cs->error_idx = cs->count; > > - /* Default value cannot be changed */ > - if (cs->which == V4L2_CTRL_WHICH_DEF_VAL) { > - dprintk(vdev, "%s: cannot change default value\n", > + /* Default/minimum/maximum values cannot be changed */ > + if (cs->which == V4L2_CTRL_WHICH_DEF_VAL || > + cs->which == V4L2_CTRL_WHICH_MIN_VAL || > + cs->which == V4L2_CTRL_WHICH_MAX_VAL) { > + dprintk(vdev, "%s: cannot change default/min/max value\n", > video_device_node_name(vdev)); > return -EINVAL; > } > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c > index 35d43ba650db..8c5828c7c6d3 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c > @@ -186,6 +186,28 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx, > } > } > > +static void std_min_compound(const struct v4l2_ctrl *ctrl, u32 idx, > + union v4l2_ctrl_ptr ptr) > +{ > + void *p = ptr.p + idx * ctrl->elem_size; > + > + if (ctrl->p_min.p_const) > + memcpy(p, ctrl->p_min.p_const, ctrl->elem_size); > + else > + memset(p, 0, ctrl->elem_size); > +} > + > +static void std_max_compound(const struct v4l2_ctrl *ctrl, u32 idx, > + union v4l2_ctrl_ptr ptr) > +{ > + void *p = ptr.p + idx * ctrl->elem_size; > + > + if (ctrl->p_max.p_const) > + memcpy(p, ctrl->p_max.p_const, ctrl->elem_size); > + else > + memset(p, 0, ctrl->elem_size); > +} > + > static void std_init(const struct v4l2_ctrl *ctrl, u32 idx, > union v4l2_ctrl_ptr ptr) > { > @@ -224,6 +246,82 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx, > } > } > > +static void std_minimum(const struct v4l2_ctrl *ctrl, u32 idx, > + union v4l2_ctrl_ptr ptr) > +{ > + switch (ctrl->type) { > + case V4L2_CTRL_TYPE_STRING: > + idx *= ctrl->elem_size; > + memset(ptr.p_char + idx, ' ', ctrl->minimum); > + ptr.p_char[idx + ctrl->minimum] = '\0'; > + break; > + case V4L2_CTRL_TYPE_INTEGER64: > + ptr.p_s64[idx] = ctrl->minimum; > + break; > + case V4L2_CTRL_TYPE_INTEGER: > + case V4L2_CTRL_TYPE_INTEGER_MENU: > + case V4L2_CTRL_TYPE_MENU: > + case V4L2_CTRL_TYPE_BITMASK: > + case V4L2_CTRL_TYPE_BOOLEAN: > + ptr.p_s32[idx] = ctrl->minimum; > + break; > + case V4L2_CTRL_TYPE_BUTTON: > + case V4L2_CTRL_TYPE_CTRL_CLASS: > + ptr.p_s32[idx] = 0; > + break; > + case V4L2_CTRL_TYPE_U8: > + ptr.p_u8[idx] = ctrl->minimum; > + break; > + case V4L2_CTRL_TYPE_U16: > + ptr.p_u16[idx] = ctrl->minimum; > + break; > + case V4L2_CTRL_TYPE_U32: > + ptr.p_u32[idx] = ctrl->minimum; > + break; > + default: > + std_min_compound(ctrl, idx, ptr); > + break; > + } > +} > + > +static void std_maximum(const struct v4l2_ctrl *ctrl, u32 idx, > + union v4l2_ctrl_ptr ptr) > +{ > + switch (ctrl->type) { > + case V4L2_CTRL_TYPE_STRING: > + idx *= ctrl->elem_size; > + memset(ptr.p_char + idx, ' ', ctrl->maximum); > + ptr.p_char[idx + ctrl->maximum] = '\0'; > + break; > + case V4L2_CTRL_TYPE_INTEGER64: > + ptr.p_s64[idx] = ctrl->maximum; > + break; > + case V4L2_CTRL_TYPE_INTEGER: > + case V4L2_CTRL_TYPE_INTEGER_MENU: > + case V4L2_CTRL_TYPE_MENU: > + case V4L2_CTRL_TYPE_BITMASK: > + case V4L2_CTRL_TYPE_BOOLEAN: > + ptr.p_s32[idx] = ctrl->maximum; > + break; > + case V4L2_CTRL_TYPE_BUTTON: > + case V4L2_CTRL_TYPE_CTRL_CLASS: > + ptr.p_s32[idx] = 0; > + break; > + case V4L2_CTRL_TYPE_U8: > + ptr.p_u8[idx] = ctrl->maximum; > + break; > + case V4L2_CTRL_TYPE_U16: > + ptr.p_u16[idx] = ctrl->maximum; > + break; > + case V4L2_CTRL_TYPE_U32: > + ptr.p_u32[idx] = ctrl->maximum; > + break; > + default: > + std_max_compound(ctrl, idx, ptr); > + break; > + } > +} > + > static void std_log(const struct v4l2_ctrl *ctrl) > { > union v4l2_ctrl_ptr ptr = ctrl->p_cur; > @@ -986,6 +1084,8 @@ static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx, > static const struct v4l2_ctrl_type_ops std_type_ops = { > .equal = std_equal, > .init = std_init, > + .minimum = std_minimum, > + .maximum = std_maximum, > .log = std_log, > .validate = std_validate, > }; > @@ -1368,7 +1468,10 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > s64 min, s64 max, u64 step, s64 def, > const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size, > u32 flags, const char * const *qmenu, > - const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def, > + const s64 *qmenu_int, > + const union v4l2_ctrl_ptr p_def, > + const union v4l2_ctrl_ptr p_min, > + const union v4l2_ctrl_ptr p_max, > void *priv) > { > struct v4l2_ctrl *ctrl; > @@ -1515,7 +1618,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > sz_extra += 2 * tot_ctrl_size; > > if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p_const) > - sz_extra += elem_size; > + sz_extra += elem_size * 3; > > ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL); > if (ctrl == NULL) { > @@ -1565,6 +1668,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size; > memcpy(ctrl->p_def.p, p_def.p_const, elem_size); > } > + if (type >= V4L2_CTRL_COMPOUND_TYPES && > + p_min.p_const && p_max.p_const) { > + ctrl->p_min.p = ctrl->p_cur.p + 2 * tot_ctrl_size; > + memcpy(ctrl->p_min.p, p_min.p_const, elem_size); > + ctrl->p_max.p = ctrl->p_cur.p + 3 * tot_ctrl_size; > + memcpy(ctrl->p_max.p, p_max.p_const, elem_size); > + } > > for (idx = 0; idx < elems; idx++) { > ctrl->type_ops->init(ctrl, idx, ctrl->p_cur); > @@ -1617,7 +1727,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, > type, min, max, > is_menu ? cfg->menu_skip_mask : step, def, > cfg->dims, cfg->elem_size, > - flags, qmenu, qmenu_int, cfg->p_def, priv); > + flags, qmenu, qmenu_int, cfg->p_def, cfg->p_min, > + cfg->p_max, priv); > if (ctrl) > ctrl->is_private = cfg->is_private; > return ctrl; > @@ -1642,7 +1753,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, > } > return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > min, max, step, def, NULL, 0, > - flags, NULL, NULL, ptr_null, NULL); > + flags, NULL, NULL, ptr_null, ptr_null, > + ptr_null, NULL); > } > EXPORT_SYMBOL(v4l2_ctrl_new_std); > > @@ -1675,7 +1787,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, > } > return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > 0, max, mask, def, NULL, 0, > - flags, qmenu, qmenu_int, ptr_null, NULL); > + flags, qmenu, qmenu_int, ptr_null, ptr_null, > + ptr_null, NULL); > } > EXPORT_SYMBOL(v4l2_ctrl_new_std_menu); > > @@ -1707,7 +1820,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > } > return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > 0, max, mask, def, NULL, 0, > - flags, qmenu, NULL, ptr_null, NULL); > + flags, qmenu, NULL, ptr_null, ptr_null, > + ptr_null, NULL); > > } > EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); > @@ -1715,7 +1829,9 @@ EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); > /* Helper function for standard compound controls */ > struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, > const struct v4l2_ctrl_ops *ops, u32 id, > - const union v4l2_ctrl_ptr p_def) > + const union v4l2_ctrl_ptr p_def, > + const union v4l2_ctrl_ptr p_min, > + const union v4l2_ctrl_ptr p_max) > { > const char *name; > enum v4l2_ctrl_type type; > @@ -1729,7 +1845,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, > } > return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > min, max, step, def, NULL, 0, > - flags, NULL, NULL, p_def, NULL); > + flags, NULL, NULL, p_def, p_min, p_max, NULL); > } > EXPORT_SYMBOL(v4l2_ctrl_new_std_compound); > > @@ -1753,7 +1869,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, > } > return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, > 0, max, 0, def, NULL, 0, > - flags, NULL, qmenu_int, ptr_null, NULL); > + flags, NULL, qmenu_int, ptr_null, ptr_null, > + ptr_null, NULL); > } > EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 21470de62d72..0f713b9a95f9 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -893,7 +893,9 @@ static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl) > return false; > break; > case V4L2_CTRL_WHICH_DEF_VAL: > - /* Default value cannot be changed */ > + case V4L2_CTRL_WHICH_MIN_VAL: > + case V4L2_CTRL_WHICH_MAX_VAL: > + /* Default, minimum or maximum value cannot be changed */ > if (ioctl == VIDIOC_S_EXT_CTRLS || > ioctl == VIDIOC_TRY_EXT_CTRLS) { > c->error_idx = c->count; > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > index 919e104de50b..c8ab8c44d7c6 100644 > --- a/include/media/v4l2-ctrls.h > +++ b/include/media/v4l2-ctrls.h > @@ -131,6 +131,8 @@ struct v4l2_ctrl_ops { > * > * @equal: return true if both values are equal. > * @init: initialize the value. > + * @minimum: set the value to the minimum value of the control. > + * @maximum: set the value to the maximum value of the control. > * @log: log the value. > * @validate: validate the value. Return 0 on success and a negative value > * otherwise. > @@ -141,6 +143,10 @@ struct v4l2_ctrl_type_ops { > union v4l2_ctrl_ptr ptr2); > void (*init)(const struct v4l2_ctrl *ctrl, u32 idx, > union v4l2_ctrl_ptr ptr); > + void (*minimum)(const struct v4l2_ctrl *ctrl, u32 idx, > + union v4l2_ctrl_ptr ptr); > + void (*maximum)(const struct v4l2_ctrl *ctrl, u32 idx, > + union v4l2_ctrl_ptr ptr); > void (*log)(const struct v4l2_ctrl *ctrl); > int (*validate)(const struct v4l2_ctrl *ctrl, u32 idx, > union v4l2_ctrl_ptr ptr); > @@ -237,6 +243,12 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv); > * @p_def: The control's default value represented via a union which > * provides a standard way of accessing control types > * through a pointer (for compound controls only). > + * @p_min: The control's minimum value represented via a union which > + * provides a standard way of accessing control types > + * through a pointer (for compound controls only). > + * @p_max: The control's maximum value represented via a union which > + * provides a standard way of accessing control types > + * through a pointer (for compound controls only). > * @p_cur: The control's current value represented via a union which > * provides a standard way of accessing control types > * through a pointer. > @@ -292,6 +304,8 @@ struct v4l2_ctrl { > } cur; > > union v4l2_ctrl_ptr p_def; > + union v4l2_ctrl_ptr p_min; > + union v4l2_ctrl_ptr p_max; > union v4l2_ctrl_ptr p_new; > union v4l2_ctrl_ptr p_cur; > }; > @@ -398,6 +412,8 @@ struct v4l2_ctrl_handler { > * @step: The control's step value for non-menu controls. > * @def: The control's default value. > * @p_def: The control's default value for compound controls. > + * @p_min: The control's minimum value for compound controls. > + * @p_max: The control's maximum value for compound controls. > * @dims: The size of each dimension. > * @elem_size: The size in bytes of the control. > * @flags: The control's flags. > @@ -427,6 +443,8 @@ struct v4l2_ctrl_config { > u64 step; > s64 def; > union v4l2_ctrl_ptr p_def; > + union v4l2_ctrl_ptr p_min; > + union v4l2_ctrl_ptr p_max; > u32 dims[V4L2_CTRL_MAX_DIMS]; > u32 elem_size; > u32 flags; > @@ -696,17 +714,21 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > * @ops: The control ops. > * @id: The control ID. > * @p_def: The control's default value. > + * @p_min: The control's minimum value. > + * @p_max: The control's maximum value. > * > - * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks > - * to the @p_def field. Use v4l2_ctrl_ptr_create() to create @p_def from a > - * pointer. Use v4l2_ctrl_ptr_create(NULL) if the default value of the > - * compound control should be all zeroes. > + * Same as v4l2_ctrl_new_std(), but with support to compound controls, thanks > + * to the @p_def/min/max field. Use v4l2_ctrl_ptr_create() to create s/field/fields/ > + * @p_def/min/max from a pointer. Use v4l2_ctrl_ptr_create(NULL) if the > + * default/min/max value of the compound control should be all zeroes. Hmmm... I expect that, for some compound control types, the concept of minimum and maximum won't be defined. Wouldn't it better for a get control operation on V4L2_CTRL_WHICH_{MIN,MAX}_VAL to return an error in that case, instead of zeroed memory ? > * > */ > struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, > const struct v4l2_ctrl_ops *ops, > u32 id, > - const union v4l2_ctrl_ptr p_def); > + const union v4l2_ctrl_ptr p_def, > + const union v4l2_ctrl_ptr p_min, > + const union v4l2_ctrl_ptr p_max); > > /** > * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control. > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 2e36bb610ea6..134f6a65eacc 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -1796,6 +1796,8 @@ struct v4l2_ext_controls { > #define V4L2_CTRL_WHICH_CUR_VAL 0 > #define V4L2_CTRL_WHICH_DEF_VAL 0x0f000000 > #define V4L2_CTRL_WHICH_REQUEST_VAL 0x0f010000 > +#define V4L2_CTRL_WHICH_MIN_VAL 0x0f020000 > +#define V4L2_CTRL_WHICH_MAX_VAL 0x0f030000 > > enum v4l2_ctrl_type { > V4L2_CTRL_TYPE_INTEGER = 1,
Hi Yunke, Thank you for the patch. On Tue, Jun 28, 2022 at 04:57:01PM +0900, Yunke Cao wrote: > Supports getting/setting current value. > Supports getting default value. > Handles V4L2_CTRL_FLAG_NEXT_COMPOUND. > > Signed-off-by: Yunke Cao <yunkec@google.com> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 279 ++++++++++++++++++++++++++----- > drivers/media/usb/uvc/uvcvideo.h | 4 + > 2 files changed, 238 insertions(+), 45 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 772d9d28a520..508ee04afbcd 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -815,6 +815,34 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping, > } > } > > +/* Extract the byte array specified by mapping->offset and mapping->size > + * stored at 'data' to the output array 'data_out'. > + */ Please use the /* * multi * line */ style instead of /* multi * line */ The uvcvideo driver has recently switched to the former in commit 699b9a86a3f0 ("media: uvcvideo: Fix comment blocks style"). > +static int uvc_get_array(struct uvc_control_mapping *mapping, const u8 *data, > + u8 *data_out) > +{ > + // Only supports byte-aligned data. And no C++-style comments either please. > + if (WARN_ON(mapping->offset % 8 || mapping->size % 8)) > + return -EINVAL; > + > + memcpy(data_out, data + mapping->offset / 8, mapping->size / 8); > + return 0; > +} > + > +/* Copy the byte array 'data_in' to the destination specified by mapping->offset > + * and mapping->size stored at 'data'. > + */ > +static int uvc_set_array(struct uvc_control_mapping *mapping, const u8 *data_in, > + u8 *data) > +{ > + // Only supports byte-aligned data. > + if (WARN_ON(mapping->offset % 8 || mapping->size % 8)) > + return -EINVAL; > + > + memcpy(data + mapping->offset / 8, data_in, mapping->size / 8); > + return 0; > +} > + Could you add here a static bool uvc_ctrl_mapping_is_compound(const struct uvc_control_mapping *mapping) { return mapping->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES; } and use it below ? That would be clearer I think. > /* ------------------------------------------------------------------------ > * Terminal and unit management > */ > @@ -831,7 +859,7 @@ static int uvc_entity_match_guid(const struct uvc_entity *entity, > > static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, > struct uvc_control_mapping **mapping, struct uvc_control **control, > - int next) > + int next, int next_compound) > { > struct uvc_control *ctrl; > struct uvc_control_mapping *map; > @@ -846,14 +874,18 @@ static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, > continue; > > list_for_each_entry(map, &ctrl->info.mappings, list) { > - if ((map->id == v4l2_id) && !next) { > + if (map->id == v4l2_id && !next && !next_compound) { > *control = ctrl; > *mapping = map; > return; > } > > if ((*mapping == NULL || (*mapping)->id > map->id) && > - (map->id > v4l2_id) && next) { > + (map->id > v4l2_id) && > + ((map->v4l2_type < V4L2_CTRL_COMPOUND_TYPES && > + next) || > + (map->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES && > + next_compound))) { > *control = ctrl; > *mapping = map; > } This could become for instance if ((*mapping == NULL || (*mapping)->id > map->id) && (map->id > v4l2_id) && ((!uvc_ctrl_mapping_is_compound(map) && next) || (uvc_ctrl_mapping_is_compound(map) && next_compound))) { *control = ctrl; *mapping = map; } > @@ -867,6 +899,7 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain, > struct uvc_control *ctrl = NULL; > struct uvc_entity *entity; > int next = v4l2_id & V4L2_CTRL_FLAG_NEXT_CTRL; > + int next_compound = v4l2_id & V4L2_CTRL_FLAG_NEXT_COMPOUND; > > *mapping = NULL; > > @@ -875,12 +908,13 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain, > > /* Find the control. */ > list_for_each_entry(entity, &chain->entities, chain) { > - __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next); > - if (ctrl && !next) > + __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next, > + next_compound); > + if (ctrl && !next && !next_compound) > return ctrl; > } > > - if (ctrl == NULL && !next) > + if (!ctrl && !next && !next_compound) > uvc_dbg(chain->dev, CONTROL, "Control 0x%08x not found\n", > v4l2_id); > > @@ -943,6 +977,39 @@ static int uvc_ctrl_populate_cache(struct uvc_video_chain *chain, > return 0; > } > > +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain, > + struct uvc_control *ctrl) > +{ I would declare u8 data; here, and > + int ret = 0; > + > + if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > + return -EACCES; > + > + if (ctrl->loaded) > + return 0; > + set data = uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT); here, and > + if (ctrl->entity->get_cur) { > + ret = ctrl->entity->get_cur(chain->dev, > + ctrl->entity, > + ctrl->info.selector, > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > + ctrl->info.size); improve indentation here ret = ctrl->entity->get_cur(chain->dev, ctrl->entity, ctrl->info.selector, data, ctrl->info.size); > + } else { and also use data here > + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > + ctrl->entity->id, chain->dev->intfnum, > + ctrl->info.selector, > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > + ctrl->info.size); ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id, chain->dev->intfnum, ctrl->info.selector, data, ctrl->info.size); > + } > + > + if (ret < 0) > + return ret; > + > + ctrl->loaded = 1; > + > + return ret; > +} > + > static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, > const u8 *data) > { > @@ -963,35 +1030,19 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, > return value; > } > > -static int __uvc_ctrl_get(struct uvc_video_chain *chain, > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > - s32 *value) > +static int __uvc_ctrl_get_std(struct uvc_video_chain *chain, > + struct uvc_control *ctrl, > + struct uvc_control_mapping *mapping, > + s32 *value) > { > int ret; > > - if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > - return -EACCES; > - > - if (!ctrl->loaded) { > - if (ctrl->entity->get_cur) { > - ret = ctrl->entity->get_cur(chain->dev, > - ctrl->entity, > - ctrl->info.selector, > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > - ctrl->info.size); > - } else { > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > - ctrl->entity->id, > - chain->dev->intfnum, > - ctrl->info.selector, > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > - ctrl->info.size); > - } > - if (ret < 0) > - return ret; > + if (mapping->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES) > + return -EINVAL; > > - ctrl->loaded = 1; > - } > + ret = __uvc_ctrl_load_cur(chain, ctrl); > + if (ret < 0) > + return ret; > > *value = __uvc_ctrl_get_value(mapping, > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > @@ -999,6 +1050,57 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain, > return 0; > } > > +static int __uvc_ctrl_get_compound_to_user(struct uvc_control_mapping *mapping, I don't like this name much. One option would be to rename the __uvc_ctrl_get_compound() function below to __uvc_ctrl_get_compound_cur() and call this function __uvc_ctrl_get_compound(). Better options are likely possible. > + struct uvc_control *ctrl, > + int id, > + struct v4l2_ext_control *xctrl) > +{ > + int ret, size; size is never negative, make it an unsigned int. > + u8 *data; > + > + if (WARN_ON(!mapping->size % 8)) > + return -EINVAL; This duplicates a check in the get_array function. Let's move all those checks to the function that adds mappings. > + > + size = mapping->size / 8; > + if (xctrl->size < size) { > + xctrl->size = size; > + return -ENOSPC; > + } > + > + data = kmalloc(size, GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + ret = mapping->get_array(mapping, uvc_ctrl_data(ctrl, id), data); > + if (ret < 0) > + goto out; > + > + ret = copy_to_user(xctrl->ptr, data, size) ? -EFAULT : 0; > + > +out: > + kfree(data); > + return ret; > +} > + > +static int __uvc_ctrl_get_compound(struct uvc_video_chain *chain, > + struct uvc_control *ctrl, > + struct uvc_control_mapping *mapping, > + struct v4l2_ext_control *xctrl) > +{ > + int ret; > + > + if (mapping->v4l2_type < V4L2_CTRL_COMPOUND_TYPES) > + return -EINVAL; > + > + ret = __uvc_ctrl_load_cur(chain, ctrl); > + if (ret < 0) > + return ret; > + > + return __uvc_ctrl_get_compound_to_user(mapping, ctrl, > + UVC_CTRL_DATA_CURRENT, > + xctrl); > +} > + > static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id, > u32 found_id) > { > @@ -1102,10 +1204,15 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > if (mapping->master_id) > __uvc_find_control(ctrl->entity, mapping->master_id, > - &master_map, &master_ctrl, 0); > + &master_map, &master_ctrl, 0, 0); > if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { > - s32 val; > - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); > + int ret; > + s32 val = 0; Move ret after val. > + > + if (master_map->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES) > + return -EINVAL; > + > + ret = __uvc_ctrl_get_std(chain, master_ctrl, master_map, &val); > if (ret < 0) > return ret; > > @@ -1113,6 +1220,15 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > } > > + if (v4l2_ctrl->type >= V4L2_CTRL_COMPOUND_TYPES) { > + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_HAS_PAYLOAD; > + v4l2_ctrl->default_value = 0; > + v4l2_ctrl->minimum = 0; > + v4l2_ctrl->maximum = 0; > + v4l2_ctrl->step = 0; > + return 0; > + } > + > if (!ctrl->cached) { > int ret = uvc_ctrl_populate_cache(chain, ctrl); > if (ret < 0) > @@ -1346,11 +1462,12 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, > u32 changes = V4L2_EVENT_CTRL_CH_FLAGS; > s32 val = 0; > > - __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0); > + __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0, 0); > if (ctrl == NULL) > return; > > - if (__uvc_ctrl_get(chain, ctrl, mapping, &val) == 0) > + if (mapping->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES || > + __uvc_ctrl_get_std(chain, ctrl, mapping, &val) == 0) > changes |= V4L2_EVENT_CTRL_CH_VALUE; > > uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); > @@ -1517,7 +1634,8 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems) > u32 changes = V4L2_EVENT_CTRL_CH_FLAGS; > s32 val = 0; > > - if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0) > + if (mapping->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES || > + __uvc_ctrl_get_std(handle->chain, ctrl, mapping, &val) == 0) > changes |= V4L2_EVENT_CTRL_CH_VALUE; > > uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, val, > @@ -1647,7 +1765,7 @@ static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, > > for (i = 0; i < ctrls->count; i++) { > __uvc_find_control(entity, ctrls->controls[i].id, &mapping, > - &ctrl_found, 0); > + &ctrl_found, 0, 0); > if (uvc_control == ctrl_found) > return i; > } > @@ -1694,11 +1812,14 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, > if (ctrl == NULL) > return -EINVAL; > > - return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value); > + if (mapping->v4l2_type < V4L2_CTRL_COMPOUND_TYPES) > + return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value); > + else > + return __uvc_ctrl_get_compound(chain, ctrl, mapping, xctrl); > } > > -int uvc_ctrl_get_fixed(struct uvc_video_chain *chain, > - struct v4l2_ext_control *xctrl) > +int __uvc_ctrl_get_fixed_std(struct uvc_video_chain *chain, > + struct v4l2_ext_control *xctrl) > { > struct v4l2_queryctrl qc = { .id = xctrl->id }; > int ret = uvc_query_v4l2_ctrl(chain, &qc); > @@ -1710,6 +1831,56 @@ int uvc_ctrl_get_fixed(struct uvc_video_chain *chain, > return 0; > } > > +int uvc_ctrl_get_fixed(struct uvc_video_chain *chain, > + struct v4l2_ext_control *xctrl) > +{ > + struct uvc_control *ctrl; > + struct uvc_control_mapping *mapping; > + int ret; > + > + if (__uvc_query_v4l2_class(chain, xctrl->id, 0) >= 0) > + return -EACCES; > + This check comes a bit out of the blue, I think it belongs to another patch, with a commit message that clearly explain the rationale. > + ctrl = uvc_find_control(chain, xctrl->id, &mapping); > + if (!ctrl) > + return -EINVAL; And here, you're duplicating the uvc_find_control() call from uvc_query_v4l2_ctrl(), called in __uvc_ctrl_get_fixed_std(). Furthermore, you're not holding the ctrl_mutex lock here, which I think is wrong. I'd like to see a patch before this one that refactors what needs to be refactored first, and then the introduction of compound types support without any extra change like this. Splitting code out to __uvc_ctrl_load_cur() could also be moved to a preparatory patch to simplify this one, it's quite hard to review. > + > + if (mapping->v4l2_type < V4L2_CTRL_COMPOUND_TYPES) > + return __uvc_ctrl_get_fixed_std(chain, xctrl); > + > + if (!ctrl->cached) { > + ret = uvc_ctrl_populate_cache(chain, ctrl); > + if (ret < 0) > + return ret; > + } > + > + return __uvc_ctrl_get_compound_to_user(mapping, ctrl, UVC_CTRL_DATA_DEF, > + xctrl); > +} > + > +int __uvc_ctrl_set_compound(struct uvc_control_mapping *mapping, > + struct v4l2_ext_control *xctrl, > + struct uvc_control *ctrl) > +{ > + int ret; > + u8 *data; Please move ret after data. > + > + data = kmalloc(xctrl->size, GFP_KERNEL); A limit on the size is needed to avoid denial of service attacks. > + if (!data) > + return -ENOMEM; > + > + ret = copy_from_user(data, xctrl->ptr, xctrl->size); > + if (ret < 0) > + goto out; > + > + ret = mapping->set_array(mapping, data, > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); ret = mapping->set_array(mapping, data, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); The V4L2 control model is that the kernel can update the value of a control if the requested value is out of range or otherwise not acceptable. You should here copy the data back to the user. > + > +out: > + kfree(data); > + return ret; > +} > + > int uvc_ctrl_set(struct uvc_fh *handle, > struct v4l2_ext_control *xctrl) > { > @@ -1820,8 +1991,14 @@ int uvc_ctrl_set(struct uvc_fh *handle, > ctrl->info.size); > } > > - mapping->set(mapping, value, > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > + if (mapping->v4l2_type < V4L2_CTRL_COMPOUND_TYPES) { > + mapping->set(mapping, value, > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > + } else { > + ret = __uvc_ctrl_set_compound(mapping, xctrl, ctrl); > + if (ret < 0) > + return ret; > + } > > if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > ctrl->handle = handle; > @@ -2220,10 +2397,14 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > return -ENOMEM; > } > > - if (map->get == NULL) > + if (!map->get && map->v4l2_type < V4L2_CTRL_COMPOUND_TYPES) > map->get = uvc_get_le_value; > - if (map->set == NULL) > + if (!map->set && map->v4l2_type < V4L2_CTRL_COMPOUND_TYPES) > map->set = uvc_set_le_value; > + if (!map->get_array && map->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES) > + map->get_array = uvc_get_array; > + if (!map->set_array && map->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES) > + map->set_array = uvc_set_array; > > for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) { > if (V4L2_CTRL_ID2WHICH(uvc_control_classes[i]) == > @@ -2233,6 +2414,14 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > } > } > > + if (map->v4l2_type < V4L2_CTRL_COMPOUND_TYPES && > + WARN_ON(!map->get || !map->set)) > + return -EINVAL; > + > + if (map->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES && > + WARN_ON(!map->get_array || !map->set_array)) > + return -EINVAL; > + Can this happen, given that you set them above if they're null ? > list_add_tail(&map->list, &ctrl->info.mappings); > uvc_dbg(chain->dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n", > uvc_map_get_name(map), ctrl->info.entity, > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index ba028ba7c34e..2f9b75faae83 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -262,8 +262,12 @@ struct uvc_control_mapping { > > s32 (*get)(struct uvc_control_mapping *mapping, u8 query, > const u8 *data); > + int (*get_array)(struct uvc_control_mapping *mapping, const u8 *data, > + u8 *data_out); > void (*set)(struct uvc_control_mapping *mapping, s32 value, > u8 *data); > + int (*set_array)(struct uvc_control_mapping *mapping, const u8 *data_in, > + u8 *data); Wouldn't those operations be better names get_compound and set_compound ? > }; > > struct uvc_control {
Hi Yunke, Thank you for the patch. On Tue, Jun 28, 2022 at 04:57:05PM +0900, Yunke Cao wrote: > Added documentation of V4L2_CID_UVC_REGION_OF_INTEREST_RECT and > V4L2_CID_UVC_REGION_OF_INTEREST_AUTO. > > Signed-off-by: Yunke Cao <yunkec@google.com> > --- > .../userspace-api/media/drivers/uvcvideo.rst | 61 +++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/Documentation/userspace-api/media/drivers/uvcvideo.rst b/Documentation/userspace-api/media/drivers/uvcvideo.rst > index a290f9fadae9..ee4c182aa274 100644 > --- a/Documentation/userspace-api/media/drivers/uvcvideo.rst > +++ b/Documentation/userspace-api/media/drivers/uvcvideo.rst > @@ -181,6 +181,7 @@ Argument: struct uvc_xu_control_mapping > UVC_CTRL_DATA_TYPE_BOOLEAN Boolean > UVC_CTRL_DATA_TYPE_ENUM Enumeration > UVC_CTRL_DATA_TYPE_BITMASK Bitmask > + UVC_CTRL_DATA_TYPE_RECT Rectangular area > > > UVCIOC_CTRL_QUERY - Query a UVC XU control > @@ -255,3 +256,63 @@ Argument: struct uvc_xu_control_query > __u8 query Request code to send to the device > __u16 size Control data size (in bytes) > __u8 *data Control value > + You can add a second blank line here, as above the "Extension Unit (XU) support" title. > +Private V4L2 controls "Driver-specific V4L2 controls" would be better, those controls are not private. > +--------------------- > + > +A few UVC specific V4L2 control IDs are listed below. s/UVC specific/UVC-specific/ But I'd write The uvcvideo driver implements the following UVC-specific controls: > + > +``V4L2_CID_UVC_REGION_OF_INTEREST_RECT (struct)`` > + This control determines the region of interest (ROI). ROI is an s/is an/is a/ > + rectangular area represented by a struct :c:type:`v4l2_rect`. The > + rectangle is in global sensor coordinates and pixel units. It is > + independent of the field of view, not impacted by any cropping or > + scaling. > + > + Use ``V4L2_CTRL_WHICH_MIN_VAL`` and ``V4L2_CTRL_WHICH_MAX_VAL`` to query > + the range of rectangle sizes. For example, a device can have a minimum > + ROI rectangle of 1x1@0x0 and a maximum of 640x480@0x0. Minimum and maximum values for rectangles are ill-defined. Are the top and left coordinates always 0 ? If so that should be documented. > + > + Setting a ROI allows the camera to optimize the capture for the region. > + The value of ``V4L2_CID_REGION_OF_INTEREST_AUTO`` control determines > + the detailed behavior. > + > + > +``V4L2_CID_UVC_REGION_OF_INTEREST_AUTO (bitmask)`` > + This determines which, if any, on board features should track to the > + Region of Interest specified by the current value of > + ``V4L2_CID_UVD__REGION_OF_INTEREST_RECT``. > + > + Max value is a mask indicating all supported Auto > + Controls. No need to wrap this line. > + > +.. flat-table:: > + :header-rows: 0 > + :stub-columns: 0 > + > + * - ``V4L2_REGION_OF_INTEREST_AUTO_EXPOSURE`` s/V4L2/V4L2_UVC/ Same below. > + - Setting this to true enables automatic exposure time for the specified s/exposure time/exposure/ (as it can also include gain) > + region. It's not that it enables automatic exposure for the specified region, is that it causes the automatic exposure to track the region of interest instead of the whole image. Same below. > + * - ``V4L2_REGION_OF_INTEREST_AUTO_IRIS`` > + - Setting this to true enables automatic iris aperture for the specified > + region. > + * - ``V4L2_REGION_OF_INTEREST_AUTO_WHITE_BALANCE`` > + - Setting this to true enables automatic white balance adjustment for the > + specified region. > + * - ``V4L2_REGION_OF_INTEREST_AUTO_FOCUS`` > + - Setting this to true enables automatic focus adjustment for the > + specified region. > + * - ``V4L2_REGION_OF_INTEREST_AUTO_FACE_DETECT`` > + - Setting this to true enables automatic face detection for the > + specified region. > + * - ``V4L2_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK`` > + - Setting this to true enables automatic face detection and tracking. The > + current value of ``V4L2_CID_REGION_OF_INTEREST_RECT`` may be updated by > + the driver. > + * - ``V4L2_REGION_OF_INTEREST_AUTO_IMAGE_STABILIZATION`` > + - Setting this to true enables automatic image stabilization. The > + current value of ``V4L2_CID_REGION_OF_INTEREST_RECT`` may be updated by > + the driver. How so ? > + * - ``V4L2_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY`` > + - Setting this to true enables automatically capture the specified region > + with higher quality if possible. Could you please clarify this one ? I'm not sure to understand what it means.
Hi Hans, Hi Laurent, Thank you for the review! On Wed, Aug 24, 2022 at 5:50 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > Hi Yunke, > > You will need to rebase this since some of the v4l2-ctrl internals > have changed. > Thanks, I will rebase for the next version. > On 28/06/2022 09:56, Yunke Cao wrote: > > Add p_rect to struct v4l2_ext_control with basic support in > > v4l2-ctrls. > > > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > Signed-off-by: Yunke Cao <yunkec@google.com> > > --- > > .../media/v4l/vidioc-g-ext-ctrls.rst | 4 ++++ > > .../media/videodev2.h.rst.exceptions | 1 + > > drivers/media/v4l2-core/v4l2-ctrls-core.c | 20 +++++++++++++++++++ > > include/media/v4l2-ctrls.h | 2 ++ > > include/uapi/linux/videodev2.h | 2 ++ > > 5 files changed, 29 insertions(+) > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > > index 29971a45a2d4..7473baa4e977 100644 > > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > > @@ -189,6 +189,10 @@ still cause this situation. > > - ``p_area`` > > - A pointer to a struct :c:type:`v4l2_area`. Valid if this control is > > of type ``V4L2_CTRL_TYPE_AREA``. > > + * - struct :c:type:`v4l2_rect` * > > + - ``p_rect`` > > + - A pointer to a struct :c:type:`v4l2_rect`. Valid if this control is > > + of type ``V4L2_CTRL_TYPE_RECT``. > > * - struct :c:type:`v4l2_ctrl_h264_sps` * > > - ``p_h264_sps`` > > - A pointer to a struct :c:type:`v4l2_ctrl_h264_sps`. Valid if this control is > > You also need to update Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst. > Ah, I missed that. Will add it in v8. > > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > > index 9cbb7a0c354a..7b423475281d 100644 > > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions > > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > > @@ -147,6 +147,7 @@ replace symbol V4L2_CTRL_TYPE_HEVC_SPS :c:type:`v4l2_ctrl_type` > > replace symbol V4L2_CTRL_TYPE_HEVC_PPS :c:type:`v4l2_ctrl_type` > > replace symbol V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS :c:type:`v4l2_ctrl_type` > > replace symbol V4L2_CTRL_TYPE_AREA :c:type:`v4l2_ctrl_type` > > +replace symbol V4L2_CTRL_TYPE_RECT :c:type:`v4l2_ctrl_type` > > replace symbol V4L2_CTRL_TYPE_FWHT_PARAMS :c:type:`v4l2_ctrl_type` > > replace symbol V4L2_CTRL_TYPE_VP8_FRAME :c:type:`v4l2_ctrl_type` > > replace symbol V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR :c:type:`v4l2_ctrl_type` > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c > > index 949c1884d9c1..35d43ba650db 100644 > > --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c > > @@ -84,6 +84,11 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx, > > return ptr1.p_u16[idx] == ptr2.p_u16[idx]; > > case V4L2_CTRL_TYPE_U32: > > return ptr1.p_u32[idx] == ptr2.p_u32[idx]; > > + case V4L2_CTRL_TYPE_RECT: > > + return ptr1.p_rect->top == ptr2.p_rect->top && > > + ptr1.p_rect->left == ptr2.p_rect->left && > > + ptr1.p_rect->height == ptr2.p_rect->height && > > + ptr1.p_rect->width == ptr2.p_rect->width; > > You don't need to do anything here, it will fallback to a memcmp, and > that's fine for struct v4l2_rect. > Thanks! Will remove it in v8. Best, Yunke > > default: > > if (ctrl->is_int) > > return ptr1.p_s32[idx] == ptr2.p_s32[idx]; > > @@ -307,6 +312,11 @@ static void std_log(const struct v4l2_ctrl *ctrl) > > case V4L2_CTRL_TYPE_VP9_FRAME: > > pr_cont("VP9_FRAME"); > > break; > > + case V4L2_CTRL_TYPE_RECT: > > + pr_cont("%ux%u@%dx%d", > > + ptr.p_rect->width, ptr.p_rect->height, > > + ptr.p_rect->left, ptr.p_rect->top); > > + break; > > default: > > pr_cont("unknown type %d", ctrl->type); > > break; > > @@ -525,6 +535,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx, > > struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering; > > struct v4l2_ctrl_hevc_decode_params *p_hevc_decode_params; > > struct v4l2_area *area; > > + struct v4l2_rect *rect; > > void *p = ptr.p + idx * ctrl->elem_size; > > unsigned int i; > > > > @@ -888,6 +899,12 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx, > > return -EINVAL; > > break; > > > > + case V4L2_CTRL_TYPE_RECT: > > + rect = p; > > + if (!rect->width || !rect->height) > > + return -EINVAL; > > + break; > > + > > default: > > return -EINVAL; > > } > > @@ -1455,6 +1472,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, > > case V4L2_CTRL_TYPE_AREA: > > elem_size = sizeof(struct v4l2_area); > > break; > > + case V4L2_CTRL_TYPE_RECT: > > + elem_size = sizeof(struct v4l2_rect); > > + break; > > default: > > if (type < V4L2_CTRL_COMPOUND_TYPES) > > elem_size = sizeof(s32); > > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > > index b3ce438f1329..919e104de50b 100644 > > --- a/include/media/v4l2-ctrls.h > > +++ b/include/media/v4l2-ctrls.h > > @@ -58,6 +58,7 @@ struct video_device; > > * @p_hdr10_cll: Pointer to an HDR10 Content Light Level structure. > > * @p_hdr10_mastering: Pointer to an HDR10 Mastering Display structure. > > * @p_area: Pointer to an area. > > + * @p_rect: Pointer to a rectangle. > > * @p: Pointer to a compound value. > > * @p_const: Pointer to a constant compound value. > > */ > > @@ -87,6 +88,7 @@ union v4l2_ctrl_ptr { > > struct v4l2_ctrl_hdr10_cll_info *p_hdr10_cll; > > struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering; > > struct v4l2_area *p_area; > > + struct v4l2_rect *p_rect; > > void *p; > > const void *p_const; > > }; > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > > index 343b95107fce..2e36bb610ea6 100644 > > --- a/include/uapi/linux/videodev2.h > > +++ b/include/uapi/linux/videodev2.h > > @@ -1754,6 +1754,7 @@ struct v4l2_ext_control { > > __u16 __user *p_u16; > > __u32 __user *p_u32; > > struct v4l2_area __user *p_area; > > + struct v4l2_rect __user *p_rect; > > struct v4l2_ctrl_h264_sps __user *p_h264_sps; > > struct v4l2_ctrl_h264_pps *p_h264_pps; > > struct v4l2_ctrl_h264_scaling_matrix __user *p_h264_scaling_matrix; > > @@ -1813,6 +1814,7 @@ enum v4l2_ctrl_type { > > V4L2_CTRL_TYPE_U16 = 0x0101, > > V4L2_CTRL_TYPE_U32 = 0x0102, > > V4L2_CTRL_TYPE_AREA = 0x0106, > > + V4L2_CTRL_TYPE_RECT = 0x0107, > > > > V4L2_CTRL_TYPE_HDR10_CLL_INFO = 0x0110, > > V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY = 0x0111, > > Regards, > > Hans
Hi Laurent, Thanks for the review! On Thu, Aug 25, 2022 at 4:56 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Yunke, > > Thank you for the patch. > > On Tue, Jun 28, 2022 at 04:57:01PM +0900, Yunke Cao wrote: > > Supports getting/setting current value. > > Supports getting default value. > > Handles V4L2_CTRL_FLAG_NEXT_COMPOUND. > > > > Signed-off-by: Yunke Cao <yunkec@google.com> > > --- > > drivers/media/usb/uvc/uvc_ctrl.c | 279 ++++++++++++++++++++++++++----- > > drivers/media/usb/uvc/uvcvideo.h | 4 + > > 2 files changed, 238 insertions(+), 45 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > index 772d9d28a520..508ee04afbcd 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -815,6 +815,34 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping, > > } > > } > > > > +/* Extract the byte array specified by mapping->offset and mapping->size > > + * stored at 'data' to the output array 'data_out'. > > + */ > > Please use the > > /* > * multi > * line > */ > > style instead of > > /* multi > * line > */ > > The uvcvideo driver has recently switched to the former in commit > 699b9a86a3f0 ("media: uvcvideo: Fix comment blocks style"). > > > +static int uvc_get_array(struct uvc_control_mapping *mapping, const u8 *data, > > + u8 *data_out) > > +{ > > + // Only supports byte-aligned data. > > And no C++-style comments either please. > Will fix the comments style in the next version. > > + if (WARN_ON(mapping->offset % 8 || mapping->size % 8)) > > + return -EINVAL; > > + > > + memcpy(data_out, data + mapping->offset / 8, mapping->size / 8); > > + return 0; > > +} > > + > > +/* Copy the byte array 'data_in' to the destination specified by mapping->offset > > + * and mapping->size stored at 'data'. > > + */ > > +static int uvc_set_array(struct uvc_control_mapping *mapping, const u8 *data_in, > > + u8 *data) > > +{ > > + // Only supports byte-aligned data. > > + if (WARN_ON(mapping->offset % 8 || mapping->size % 8)) > > + return -EINVAL; > > + > > + memcpy(data + mapping->offset / 8, data_in, mapping->size / 8); > > + return 0; > > +} > > + > > Could you add here a > > static bool > uvc_ctrl_mapping_is_compound(const struct uvc_control_mapping *mapping) > { > return mapping->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES; > } > > and use it below ? That would be clearer I think. > > > /* ------------------------------------------------------------------------ > > * Terminal and unit management > > */ > > @@ -831,7 +859,7 @@ static int uvc_entity_match_guid(const struct uvc_entity *entity, > > > > static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, > > struct uvc_control_mapping **mapping, struct uvc_control **control, > > - int next) > > + int next, int next_compound) > > { > > struct uvc_control *ctrl; > > struct uvc_control_mapping *map; > > @@ -846,14 +874,18 @@ static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id, > > continue; > > > > list_for_each_entry(map, &ctrl->info.mappings, list) { > > - if ((map->id == v4l2_id) && !next) { > > + if (map->id == v4l2_id && !next && !next_compound) { > > *control = ctrl; > > *mapping = map; > > return; > > } > > > > if ((*mapping == NULL || (*mapping)->id > map->id) && > > - (map->id > v4l2_id) && next) { > > + (map->id > v4l2_id) && > > + ((map->v4l2_type < V4L2_CTRL_COMPOUND_TYPES && > > + next) || > > + (map->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES && > > + next_compound))) { > > *control = ctrl; > > *mapping = map; > > } > > This could become for instance > > if ((*mapping == NULL || (*mapping)->id > map->id) && > (map->id > v4l2_id) && > ((!uvc_ctrl_mapping_is_compound(map) && next) || > (uvc_ctrl_mapping_is_compound(map) && next_compound))) { > *control = ctrl; > *mapping = map; > } > Sure! That does look much better. > > @@ -867,6 +899,7 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain, > > struct uvc_control *ctrl = NULL; > > struct uvc_entity *entity; > > int next = v4l2_id & V4L2_CTRL_FLAG_NEXT_CTRL; > > + int next_compound = v4l2_id & V4L2_CTRL_FLAG_NEXT_COMPOUND; > > > > *mapping = NULL; > > > > @@ -875,12 +908,13 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain, > > > > /* Find the control. */ > > list_for_each_entry(entity, &chain->entities, chain) { > > - __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next); > > - if (ctrl && !next) > > + __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next, > > + next_compound); > > + if (ctrl && !next && !next_compound) > > return ctrl; > > } > > > > - if (ctrl == NULL && !next) > > + if (!ctrl && !next && !next_compound) > > uvc_dbg(chain->dev, CONTROL, "Control 0x%08x not found\n", > > v4l2_id); > > > > @@ -943,6 +977,39 @@ static int uvc_ctrl_populate_cache(struct uvc_video_chain *chain, > > return 0; > > } > > > > +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain, > > + struct uvc_control *ctrl) > > +{ > > I would declare > > u8 data; > > here, and > > > + int ret = 0; > > + > > + if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > > + return -EACCES; > > + > > + if (ctrl->loaded) > > + return 0; > > + > > set > > data = uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT); > > here, and > > > + if (ctrl->entity->get_cur) { > > + ret = ctrl->entity->get_cur(chain->dev, > > + ctrl->entity, > > + ctrl->info.selector, > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > + ctrl->info.size); > > improve indentation here > > ret = ctrl->entity->get_cur(chain->dev, ctrl->entity, > ctrl->info.selector, data, > ctrl->info.size); > > > > + } else { > > and also use data here > > > + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > + ctrl->entity->id, chain->dev->intfnum, > > + ctrl->info.selector, > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > + ctrl->info.size); > > ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > ctrl->entity->id, chain->dev->intfnum, > ctrl->info.selector, data, > ctrl->info.size); > > > + } > > + > > + if (ret < 0) > > + return ret; > > + > > + ctrl->loaded = 1; > > + > > + return ret; > > +} > > + > > static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, > > const u8 *data) > > { > > @@ -963,35 +1030,19 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, > > return value; > > } > > > > -static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > > - s32 *value) > > +static int __uvc_ctrl_get_std(struct uvc_video_chain *chain, > > + struct uvc_control *ctrl, > > + struct uvc_control_mapping *mapping, > > + s32 *value) > > { > > int ret; > > > > - if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > > - return -EACCES; > > - > > - if (!ctrl->loaded) { > > - if (ctrl->entity->get_cur) { > > - ret = ctrl->entity->get_cur(chain->dev, > > - ctrl->entity, > > - ctrl->info.selector, > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > - ctrl->info.size); > > - } else { > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > - ctrl->entity->id, > > - chain->dev->intfnum, > > - ctrl->info.selector, > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > - ctrl->info.size); > > - } > > - if (ret < 0) > > - return ret; > > + if (mapping->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES) > > + return -EINVAL; > > > > - ctrl->loaded = 1; > > - } > > + ret = __uvc_ctrl_load_cur(chain, ctrl); > > + if (ret < 0) > > + return ret; > > > > *value = __uvc_ctrl_get_value(mapping, > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > @@ -999,6 +1050,57 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > return 0; > > } > > > > +static int __uvc_ctrl_get_compound_to_user(struct uvc_control_mapping *mapping, > > I don't like this name much. One option would be to rename the > __uvc_ctrl_get_compound() function below to > __uvc_ctrl_get_compound_cur() and call this function > __uvc_ctrl_get_compound(). Better options are likely possible. > > > + struct uvc_control *ctrl, > > + int id, > > + struct v4l2_ext_control *xctrl) > > +{ > > + int ret, size; > > size is never negative, make it an unsigned int. > > > + u8 *data; > > + > > + if (WARN_ON(!mapping->size % 8)) > > + return -EINVAL; > > This duplicates a check in the get_array function. Let's move all those > checks to the function that adds mappings. > > > + > > + size = mapping->size / 8; > > + if (xctrl->size < size) { > > + xctrl->size = size; > > + return -ENOSPC; > > + } > > + > > + data = kmalloc(size, GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + ret = mapping->get_array(mapping, uvc_ctrl_data(ctrl, id), data); > > + if (ret < 0) > > + goto out; > > + > > + ret = copy_to_user(xctrl->ptr, data, size) ? -EFAULT : 0; > > + > > +out: > > + kfree(data); > > + return ret; > > +} > > + > > +static int __uvc_ctrl_get_compound(struct uvc_video_chain *chain, > > + struct uvc_control *ctrl, > > + struct uvc_control_mapping *mapping, > > + struct v4l2_ext_control *xctrl) > > +{ > > + int ret; > > + > > + if (mapping->v4l2_type < V4L2_CTRL_COMPOUND_TYPES) > > + return -EINVAL; > > + > > + ret = __uvc_ctrl_load_cur(chain, ctrl); > > + if (ret < 0) > > + return ret; > > + > > + return __uvc_ctrl_get_compound_to_user(mapping, ctrl, > > + UVC_CTRL_DATA_CURRENT, > > + xctrl); > > +} > > + > > static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id, > > u32 found_id) > > { > > @@ -1102,10 +1204,15 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > > > if (mapping->master_id) > > __uvc_find_control(ctrl->entity, mapping->master_id, > > - &master_map, &master_ctrl, 0); > > + &master_map, &master_ctrl, 0, 0); > > if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { > > - s32 val; > > - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); > > + int ret; > > + s32 val = 0; > > Move ret after val. > > > + > > + if (master_map->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES) > > + return -EINVAL; > > + > > + ret = __uvc_ctrl_get_std(chain, master_ctrl, master_map, &val); > > if (ret < 0) > > return ret; > > > > @@ -1113,6 +1220,15 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; > > } > > > > + if (v4l2_ctrl->type >= V4L2_CTRL_COMPOUND_TYPES) { > > + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_HAS_PAYLOAD; > > + v4l2_ctrl->default_value = 0; > > + v4l2_ctrl->minimum = 0; > > + v4l2_ctrl->maximum = 0; > > + v4l2_ctrl->step = 0; > > + return 0; > > + } > > + > > if (!ctrl->cached) { > > int ret = uvc_ctrl_populate_cache(chain, ctrl); > > if (ret < 0) > > @@ -1346,11 +1462,12 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, > > u32 changes = V4L2_EVENT_CTRL_CH_FLAGS; > > s32 val = 0; > > > > - __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0); > > + __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0, 0); > > if (ctrl == NULL) > > return; > > > > - if (__uvc_ctrl_get(chain, ctrl, mapping, &val) == 0) > > + if (mapping->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES || > > + __uvc_ctrl_get_std(chain, ctrl, mapping, &val) == 0) > > changes |= V4L2_EVENT_CTRL_CH_VALUE; > > > > uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); > > @@ -1517,7 +1634,8 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems) > > u32 changes = V4L2_EVENT_CTRL_CH_FLAGS; > > s32 val = 0; > > > > - if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0) > > + if (mapping->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES || > > + __uvc_ctrl_get_std(handle->chain, ctrl, mapping, &val) == 0) > > changes |= V4L2_EVENT_CTRL_CH_VALUE; > > > > uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, val, > > @@ -1647,7 +1765,7 @@ static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity, > > > > for (i = 0; i < ctrls->count; i++) { > > __uvc_find_control(entity, ctrls->controls[i].id, &mapping, > > - &ctrl_found, 0); > > + &ctrl_found, 0, 0); > > if (uvc_control == ctrl_found) > > return i; > > } > > @@ -1694,11 +1812,14 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, > > if (ctrl == NULL) > > return -EINVAL; > > > > - return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value); > > + if (mapping->v4l2_type < V4L2_CTRL_COMPOUND_TYPES) > > + return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value); > > + else > > + return __uvc_ctrl_get_compound(chain, ctrl, mapping, xctrl); > > } > > > > -int uvc_ctrl_get_fixed(struct uvc_video_chain *chain, > > - struct v4l2_ext_control *xctrl) > > +int __uvc_ctrl_get_fixed_std(struct uvc_video_chain *chain, > > + struct v4l2_ext_control *xctrl) > > { > > struct v4l2_queryctrl qc = { .id = xctrl->id }; > > int ret = uvc_query_v4l2_ctrl(chain, &qc); > > @@ -1710,6 +1831,56 @@ int uvc_ctrl_get_fixed(struct uvc_video_chain *chain, > > return 0; > > } > > > > +int uvc_ctrl_get_fixed(struct uvc_video_chain *chain, > > + struct v4l2_ext_control *xctrl) > > +{ > > + struct uvc_control *ctrl; > > + struct uvc_control_mapping *mapping; > > + int ret; > > + > > + if (__uvc_query_v4l2_class(chain, xctrl->id, 0) >= 0) > > + return -EACCES; > > + > > This check comes a bit out of the blue, I think it belongs to another > patch, with a commit message that clearly explain the rationale. > > > + ctrl = uvc_find_control(chain, xctrl->id, &mapping); > > + if (!ctrl) > > + return -EINVAL; > > And here, you're duplicating the uvc_find_control() call from > uvc_query_v4l2_ctrl(), called in __uvc_ctrl_get_fixed_std(). > Furthermore, you're not holding the ctrl_mutex lock here, which I think > is wrong. > Thanks for catching this! I'm thinking of fixing it by calling __uvc_query_v4l2_ctrl() in __uvc_ctrl_get_fixed_std(). And acquire the ctrl_mutex lock here in uvc_ctrl_get_fixed(). Does that sound right to you? > I'd like to see a patch before this one that refactors what needs to be > refactored first, and then the introduction of compound types support > without any extra change like this. Splitting code out to > __uvc_ctrl_load_cur() could also be moved to a preparatory patch to > simplify this one, it's quite hard to review. > I see. For __uvc_ctrl_load_cur(), I guess I can cherry-pick "[PATCH v4] media: uvcvideo: Use entity get_cur in uvc_ctrl_set" before this. I am trying to split the refactoring in 2/7. Should I introduce the "get/set_std()" functions before this as well? > > + > > + if (mapping->v4l2_type < V4L2_CTRL_COMPOUND_TYPES) > > + return __uvc_ctrl_get_fixed_std(chain, xctrl); > > + > > + if (!ctrl->cached) { > > + ret = uvc_ctrl_populate_cache(chain, ctrl); > > + if (ret < 0) > > + return ret; > > + } > > + > > + return __uvc_ctrl_get_compound_to_user(mapping, ctrl, UVC_CTRL_DATA_DEF, > > + xctrl); > > +} > > + > > +int __uvc_ctrl_set_compound(struct uvc_control_mapping *mapping, > > + struct v4l2_ext_control *xctrl, > > + struct uvc_control *ctrl) > > +{ > > + int ret; > > + u8 *data; > > Please move ret after data. > > > + > > + data = kmalloc(xctrl->size, GFP_KERNEL); > > A limit on the size is needed to avoid denial of service attacks. > > > + if (!data) > > + return -ENOMEM; > > + > > + ret = copy_from_user(data, xctrl->ptr, xctrl->size); > > + if (ret < 0) > > + goto out; > > + > > + ret = mapping->set_array(mapping, data, > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > ret = mapping->set_array(mapping, data, > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > The V4L2 control model is that the kernel can update the value of a > control if the requested value is out of range or otherwise not > acceptable. You should here copy the data back to the user. Ah okay. Will call "__uvc_ctrl_get_compound_to_user()" here. > > > + > > +out: > > + kfree(data); > > + return ret; > > +} > > + > > int uvc_ctrl_set(struct uvc_fh *handle, > > struct v4l2_ext_control *xctrl) > > { > > @@ -1820,8 +1991,14 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > ctrl->info.size); > > } > > > > - mapping->set(mapping, value, > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > + if (mapping->v4l2_type < V4L2_CTRL_COMPOUND_TYPES) { > > + mapping->set(mapping, value, > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > + } else { > > + ret = __uvc_ctrl_set_compound(mapping, xctrl, ctrl); > > + if (ret < 0) > > + return ret; > > + } > > > > if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > > ctrl->handle = handle; > > @@ -2220,10 +2397,14 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > > return -ENOMEM; > > } > > > > - if (map->get == NULL) > > + if (!map->get && map->v4l2_type < V4L2_CTRL_COMPOUND_TYPES) > > map->get = uvc_get_le_value; > > - if (map->set == NULL) > > + if (!map->set && map->v4l2_type < V4L2_CTRL_COMPOUND_TYPES) > > map->set = uvc_set_le_value; > > + if (!map->get_array && map->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES) > > + map->get_array = uvc_get_array; > > + if (!map->set_array && map->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES) > > + map->set_array = uvc_set_array; > > > > for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) { > > if (V4L2_CTRL_ID2WHICH(uvc_control_classes[i]) == > > @@ -2233,6 +2414,14 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain, > > } > > } > > > > + if (map->v4l2_type < V4L2_CTRL_COMPOUND_TYPES && > > + WARN_ON(!map->get || !map->set)) > > + return -EINVAL; > > + > > + if (map->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES && > > + WARN_ON(!map->get_array || !map->set_array)) > > + return -EINVAL; > > + > > Can this happen, given that you set them above if they're null ? > It cannot happen. Will remove this in v8. > > list_add_tail(&map->list, &ctrl->info.mappings); > > uvc_dbg(chain->dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n", > > uvc_map_get_name(map), ctrl->info.entity, > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > index ba028ba7c34e..2f9b75faae83 100644 > > --- a/drivers/media/usb/uvc/uvcvideo.h > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > @@ -262,8 +262,12 @@ struct uvc_control_mapping { > > > > s32 (*get)(struct uvc_control_mapping *mapping, u8 query, > > const u8 *data); > > + int (*get_array)(struct uvc_control_mapping *mapping, const u8 *data, > > + u8 *data_out); > > void (*set)(struct uvc_control_mapping *mapping, s32 value, > > u8 *data); > > + int (*set_array)(struct uvc_control_mapping *mapping, const u8 *data_in, > > + u8 *data); > > Wouldn't those operations be better names get_compound and set_compound ? > Agreed. Will rename to get_compound and set_compound. Best, Yunke > > }; > > > > struct uvc_control { > > -- > Regards, > > Laurent Pinchart
Hi Laurent, Thanks for the review! I've been working on v8 based on your feedback. I have some questions about your comments and replied in some of the patches. Would you mind taking another look? Best, Yunke On Fri, Jul 15, 2022 at 9:52 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Yunke, > > On Fri, Jul 15, 2022 at 08:25:20AM +0900, Yunke Cao wrote: > > Hi Laurent, > > > > Do you have time to review this version? Ricardo has already reviewed > > it, I hope it is easier to review now. > > I'll try find time, but I doubt it will be before a couple of weeks. > > > On Tue, Jun 28, 2022 at 4:57 PM Yunke Cao <yunkec@google.com> wrote: > > > > > > This patch set implements UVC v1.5 region of interest using V4L2 > > > control API. > > > > > > ROI control is consisted two uvc specific controls. > > > 1. A rectangle control with a newly added type V4L2_CTRL_TYPE_RECT. > > > 2. An auto control with type bitmask. > > > > > > V4L2_CTRL_WHICH_MIN/MAX_VAL is added to support the rectangle control. > > > > > > Tested on two different usb cameras using v4l2-compliance, v4l2-ctl > > > and calling ioctls. > > > > > > 1/7 add V4L2_CTRL_TYPE_RECT. > > > 2/7 and 3/7 support compound types in UVC. > > > 4/7 implement ROI in UVC. > > > 5/7 is a cherry-pick for Hans' implementation of > > > V4L2_CTRL_WHICH_MIN/MAX_VAL in v4l2-core. > > > 6/7 support MIN/MAX in UVC. > > > 7/7 document the changes. > > > > > > Changelog since v6: > > > -Add patch 2 and 3 to support compound types properly in UVC and > > > implement ROI on top of them. > > > -Reorder the patches. > > > > > > Changelog since v5: > > > -Add a __uvc_ctrl_get_p_rect_to_user instead of modifying > > > __uvc_ctrl_get. > > > -Support V4L2_CTRL_FLAG_NEXT_COMPOUND correctly. > > > -Fix formats. > > > > > > Changelog since v4: > > > -Cherry-pick the original patch > > > "v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL". > > > -Split patch "media: uvcvideo: implement UVC v1.5 ROI" into two patches. > > > The codes for supporting min/max in uvc are in patch 4/5 now. > > > -Minor fixes. Detailed changelog in patches > > > > > > Changelog since v3: > > > - Reordered/sliced the patches. > > > 1. Add rect type. > > > 2. Add min/max. > > > 3. Add the roi controls (including init to default). > > > 4. Document the roi controls. > > > - Define the roi controls as uvc-specific in uvcvideo.h. > > > - Modified documentation. > > > - Removed the vivid change. Given the controls are now uvc-specific. > > > I'm not sure how valuable it is to add it in vivid. Let me know > > > otherwise. > > > > > > Hans Verkuil (1): > > > v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL > > > > > > Yunke Cao (6): > > > media: v4l2_ctrl: Add V4L2_CTRL_TYPE_RECT > > > media: uvcvideo: add uvc_ctrl_get_fixed for getting default value > > > media: uvcvideo: Add support for compound controls > > > media: uvcvideo: implement UVC v1.5 ROI > > > media: uvcvideo: support V4L2_CTRL_WHICH_MIN/MAX_VAL > > > media: uvcvideo: document UVC v1.5 ROI > > > > > > .../userspace-api/media/drivers/uvcvideo.rst | 61 +++ > > > .../media/v4l/vidioc-g-ext-ctrls.rst | 11 +- > > > .../media/videodev2.h.rst.exceptions | 3 + > > > drivers/media/i2c/imx214.c | 5 +- > > > .../media/platform/qcom/venus/venc_ctrls.c | 4 + > > > drivers/media/usb/uvc/uvc_ctrl.c | 479 ++++++++++++++++-- > > > drivers/media/usb/uvc/uvc_v4l2.c | 20 +- > > > drivers/media/usb/uvc/uvcvideo.h | 14 + > > > drivers/media/v4l2-core/v4l2-ctrls-api.c | 51 +- > > > drivers/media/v4l2-core/v4l2-ctrls-core.c | 155 +++++- > > > drivers/media/v4l2-core/v4l2-ioctl.c | 4 +- > > > include/media/v4l2-ctrls.h | 34 +- > > > include/uapi/linux/usb/video.h | 1 + > > > include/uapi/linux/uvcvideo.h | 13 + > > > include/uapi/linux/v4l2-controls.h | 8 + > > > include/uapi/linux/videodev2.h | 4 + > > > 16 files changed, 788 insertions(+), 79 deletions(-) > > -- > Regards, > > Laurent Pinchart
Hi Yunke, Laurent, On 8/24/22 18:20, Laurent Pinchart wrote: > Hi Yunke and Hans, > > Thank you for the patch. > > On Tue, Jun 28, 2022 at 04:57:03PM +0900, Yunke Cao wrote: >> From: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> >> Add the capability of retrieving the min and max values of a >> compound control. >> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> Signed-off-by: Yunke Cao <yunkec@google.com> >> --- >> git am from https://lore.kernel.org/all/20191119113457.57833-3-hverkuil-cisco@xs4all.nl/ >> -Fixed some merge conflits. >> -Fixed the build error in drivers/media/platform/qcom/venus. >> >> .../media/v4l/vidioc-g-ext-ctrls.rst | 7 +- >> .../media/videodev2.h.rst.exceptions | 2 + >> drivers/media/i2c/imx214.c | 5 +- >> .../media/platform/qcom/venus/venc_ctrls.c | 4 + >> drivers/media/v4l2-core/v4l2-ctrls-api.c | 51 +++++-- >> drivers/media/v4l2-core/v4l2-ctrls-core.c | 135 ++++++++++++++++-- >> drivers/media/v4l2-core/v4l2-ioctl.c | 4 +- >> include/media/v4l2-ctrls.h | 32 ++++- >> include/uapi/linux/videodev2.h | 2 + >> 9 files changed, 214 insertions(+), 28 deletions(-) >> >> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst >> index 7473baa4e977..65a5f878cc28 100644 >> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst >> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst >> @@ -284,14 +284,17 @@ still cause this situation. >> - Which value of the control to get/set/try. >> * - :cspan:`2` ``V4L2_CTRL_WHICH_CUR_VAL`` will return the current value of >> the control, ``V4L2_CTRL_WHICH_DEF_VAL`` will return the default >> + value of the control, ``V4L2_CTRL_WHICH_MIN_VAL`` will return the minimum >> + value of the control, ``V4L2_CTRL_WHICH_MAX_VAL`` will return the maximum > > The concept of minimum and maximum values for compound controls can be > quite ill-defined. Could we document here that the definition of the > minimum and maximum values are provided by the control documentation for > compound controls ? Makes sense! I agree with the updated text in v8. > >> value of the control and ``V4L2_CTRL_WHICH_REQUEST_VAL`` indicates that >> these controls have to be retrieved from a request or tried/set for >> a request. In the latter case the ``request_fd`` field contains the >> file descriptor of the request that should be used. If the device >> does not support requests, then ``EACCES`` will be returned. >> >> - When using ``V4L2_CTRL_WHICH_DEF_VAL`` be aware that you can only >> - get the default value of the control, you cannot set or try it. >> + When using ``V4L2_CTRL_WHICH_DEF_VAL``, ``V4L2_CTRL_WHICH_MIN_VAL`` >> + or ``V4L2_CTRL_WHICH_MAX_VAL`` be aware that you can only get the >> + default/minimum/maximum value of the control, you cannot set or try it. >> >> For backwards compatibility you can also use a control class here >> (see :ref:`ctrl-class`). In that case all controls have to >> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions >> index 7b423475281d..e2dde31d76df 100644 >> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions >> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions >> @@ -553,6 +553,8 @@ ignore define V4L2_CTRL_DRIVER_PRIV >> ignore define V4L2_CTRL_MAX_DIMS >> ignore define V4L2_CTRL_WHICH_CUR_VAL >> ignore define V4L2_CTRL_WHICH_DEF_VAL >> +ignore define V4L2_CTRL_WHICH_MIN_VAL >> +ignore define V4L2_CTRL_WHICH_MAX_VAL >> ignore define V4L2_CTRL_WHICH_REQUEST_VAL >> ignore define V4L2_OUT_CAP_CUSTOM_TIMINGS >> ignore define V4L2_CID_MAX_CTRLS >> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c >> index 83c1737abeec..aa0bfd18f7b7 100644 >> --- a/drivers/media/i2c/imx214.c >> +++ b/drivers/media/i2c/imx214.c >> @@ -1037,7 +1037,10 @@ static int imx214_probe(struct i2c_client *client) >> imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls, >> NULL, >> V4L2_CID_UNIT_CELL_SIZE, >> - v4l2_ctrl_ptr_create((void *)&unit_size)); >> + v4l2_ctrl_ptr_create((void *)&unit_size), >> + v4l2_ctrl_ptr_create(NULL), >> + v4l2_ctrl_ptr_create(NULL)); > > Would it make sense to set min = max = default for read-only controls ? No, that doesn't make sense. Read-only controls are only read-only from the PoV of userspace. The kernel *does* set them, so you still want to have a range of values that the control can take. > You should also update the documentation of V4L2_CID_UNIT_CELL_SIZE (and > other controls below) to indicate how their minimum and maximum are > defined. > >> + >> ret = imx214->ctrls.error; >> if (ret) { >> dev_err(&client->dev, "%s control init failed (%d)\n", >> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c >> index ed44e5800759..4af8f9ca12a6 100644 >> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c >> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c >> @@ -579,10 +579,14 @@ int venc_ctrl_init(struct venus_inst *inst) >> >> v4l2_ctrl_new_std_compound(&inst->ctrl_handler, &venc_ctrl_ops, >> V4L2_CID_COLORIMETRY_HDR10_CLL_INFO, >> + v4l2_ctrl_ptr_create(NULL), >> + v4l2_ctrl_ptr_create(NULL), >> v4l2_ctrl_ptr_create(NULL)); >> >> v4l2_ctrl_new_std_compound(&inst->ctrl_handler, &venc_ctrl_ops, >> V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY, >> + v4l2_ctrl_ptr_create(NULL), >> + v4l2_ctrl_ptr_create(NULL), >> v4l2_ctrl_ptr_create(NULL)); >> >> v4l2_ctrl_new_std_menu(&inst->ctrl_handler, &venc_ctrl_ops, >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c >> index db9baa0bd05f..8a9c816b0dab 100644 >> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c >> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c >> @@ -97,6 +97,28 @@ static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl) >> return ptr_to_user(c, ctrl, ctrl->p_new); >> } >> >> +/* Helper function: copy the minimum control value back to the caller */ >> +static int min_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl) >> +{ >> + int idx; >> + >> + for (idx = 0; idx < ctrl->elems; idx++) >> + ctrl->type_ops->minimum(ctrl, idx, ctrl->p_new); >> + >> + return ptr_to_user(c, ctrl, ctrl->p_new); >> +} >> + >> +/* Helper function: copy the maximum control value back to the caller */ >> +static int max_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl) >> +{ >> + int idx; >> + >> + for (idx = 0; idx < ctrl->elems; idx++) >> + ctrl->type_ops->maximum(ctrl, idx, ctrl->p_new); >> + >> + return ptr_to_user(c, ctrl, ctrl->p_new); >> +} >> + >> /* Helper function: copy the caller-provider value to the given control value */ >> static int user_to_ptr(struct v4l2_ext_control *c, >> struct v4l2_ctrl *ctrl, >> @@ -220,8 +242,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl, >> cs->error_idx = i; >> >> if (cs->which && >> - cs->which != V4L2_CTRL_WHICH_DEF_VAL && >> - cs->which != V4L2_CTRL_WHICH_REQUEST_VAL && >> + (cs->which < V4L2_CTRL_WHICH_DEF_VAL || >> + cs->which > V4L2_CTRL_WHICH_MAX_VAL) && >> V4L2_CTRL_ID2WHICH(id) != cs->which) { >> dprintk(vdev, >> "invalid which 0x%x or control id 0x%x\n", >> @@ -335,8 +357,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl, >> */ >> static int class_check(struct v4l2_ctrl_handler *hdl, u32 which) >> { >> - if (which == 0 || which == V4L2_CTRL_WHICH_DEF_VAL || >> - which == V4L2_CTRL_WHICH_REQUEST_VAL) >> + if (which == 0 || (which >= V4L2_CTRL_WHICH_DEF_VAL && >> + which <= V4L2_CTRL_WHICH_MAX_VAL)) >> return 0; >> return find_ref_lock(hdl, which | 1) ? 0 : -EINVAL; >> } >> @@ -356,10 +378,12 @@ int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl, >> struct v4l2_ctrl_helper *helpers = helper; >> int ret; >> int i, j; >> - bool is_default, is_request; >> + bool is_default, is_request, is_min, is_max; >> >> is_default = (cs->which == V4L2_CTRL_WHICH_DEF_VAL); >> is_request = (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL); >> + is_min = (cs->which == V4L2_CTRL_WHICH_MIN_VAL); >> + is_max = (cs->which == V4L2_CTRL_WHICH_MAX_VAL); >> >> cs->error_idx = cs->count; >> cs->which = V4L2_CTRL_ID2WHICH(cs->which); >> @@ -399,13 +423,14 @@ int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl, >> >> /* >> * g_volatile_ctrl will update the new control values. >> - * This makes no sense for V4L2_CTRL_WHICH_DEF_VAL and >> + * This makes no sense for V4L2_CTRL_WHICH_DEF_VAL, >> + * V4L2_CTRL_WHICH_MIN_VAL, V4L2_CTRL_WHICH_MAX_VAL and >> * V4L2_CTRL_WHICH_REQUEST_VAL. In the case of requests >> * it is v4l2_ctrl_request_complete() that copies the >> * volatile controls at the time of request completion >> * to the request, so you don't want to do that again. >> */ >> - if (!is_default && !is_request && >> + if (!is_default && !is_request && !is_min && !is_max && >> ((master->flags & V4L2_CTRL_FLAG_VOLATILE) || >> (master->has_volatiles && !is_cur_manual(master)))) { >> for (j = 0; j < master->ncontrols; j++) >> @@ -432,6 +457,10 @@ int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl, >> ret = def_to_user(cs->controls + idx, ref->ctrl); >> else if (is_request && ref->valid_p_req) >> ret = req_to_user(cs->controls + idx, ref); >> + else if (is_min) >> + ret = min_to_user(cs->controls + idx, ref->ctrl); >> + else if (is_max) >> + ret = max_to_user(cs->controls + idx, ref->ctrl); >> else if (is_volatile) >> ret = new_to_user(cs->controls + idx, ref->ctrl); >> else >> @@ -523,9 +552,11 @@ int try_set_ext_ctrls_common(struct v4l2_fh *fh, >> >> cs->error_idx = cs->count; >> >> - /* Default value cannot be changed */ >> - if (cs->which == V4L2_CTRL_WHICH_DEF_VAL) { >> - dprintk(vdev, "%s: cannot change default value\n", >> + /* Default/minimum/maximum values cannot be changed */ >> + if (cs->which == V4L2_CTRL_WHICH_DEF_VAL || >> + cs->which == V4L2_CTRL_WHICH_MIN_VAL || >> + cs->which == V4L2_CTRL_WHICH_MAX_VAL) { >> + dprintk(vdev, "%s: cannot change default/min/max value\n", >> video_device_node_name(vdev)); >> return -EINVAL; >> } >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c >> index 35d43ba650db..8c5828c7c6d3 100644 >> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c >> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c >> @@ -186,6 +186,28 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx, >> } >> } >> >> +static void std_min_compound(const struct v4l2_ctrl *ctrl, u32 idx, >> + union v4l2_ctrl_ptr ptr) >> +{ >> + void *p = ptr.p + idx * ctrl->elem_size; >> + >> + if (ctrl->p_min.p_const) >> + memcpy(p, ctrl->p_min.p_const, ctrl->elem_size); >> + else >> + memset(p, 0, ctrl->elem_size); >> +} >> + >> +static void std_max_compound(const struct v4l2_ctrl *ctrl, u32 idx, >> + union v4l2_ctrl_ptr ptr) >> +{ >> + void *p = ptr.p + idx * ctrl->elem_size; >> + >> + if (ctrl->p_max.p_const) >> + memcpy(p, ctrl->p_max.p_const, ctrl->elem_size); >> + else >> + memset(p, 0, ctrl->elem_size); >> +} >> + >> static void std_init(const struct v4l2_ctrl *ctrl, u32 idx, >> union v4l2_ctrl_ptr ptr) >> { >> @@ -224,6 +246,82 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx, >> } >> } >> >> +static void std_minimum(const struct v4l2_ctrl *ctrl, u32 idx, >> + union v4l2_ctrl_ptr ptr) >> +{ >> + switch (ctrl->type) { >> + case V4L2_CTRL_TYPE_STRING: >> + idx *= ctrl->elem_size; >> + memset(ptr.p_char + idx, ' ', ctrl->minimum); >> + ptr.p_char[idx + ctrl->minimum] = '\0'; >> + break; >> + case V4L2_CTRL_TYPE_INTEGER64: >> + ptr.p_s64[idx] = ctrl->minimum; >> + break; >> + case V4L2_CTRL_TYPE_INTEGER: >> + case V4L2_CTRL_TYPE_INTEGER_MENU: >> + case V4L2_CTRL_TYPE_MENU: >> + case V4L2_CTRL_TYPE_BITMASK: >> + case V4L2_CTRL_TYPE_BOOLEAN: >> + ptr.p_s32[idx] = ctrl->minimum; >> + break; >> + case V4L2_CTRL_TYPE_BUTTON: >> + case V4L2_CTRL_TYPE_CTRL_CLASS: >> + ptr.p_s32[idx] = 0; >> + break; >> + case V4L2_CTRL_TYPE_U8: >> + ptr.p_u8[idx] = ctrl->minimum; >> + break; >> + case V4L2_CTRL_TYPE_U16: >> + ptr.p_u16[idx] = ctrl->minimum; >> + break; >> + case V4L2_CTRL_TYPE_U32: >> + ptr.p_u32[idx] = ctrl->minimum; >> + break; >> + default: >> + std_min_compound(ctrl, idx, ptr); >> + break; >> + } >> +} >> + >> +static void std_maximum(const struct v4l2_ctrl *ctrl, u32 idx, >> + union v4l2_ctrl_ptr ptr) >> +{ >> + switch (ctrl->type) { >> + case V4L2_CTRL_TYPE_STRING: >> + idx *= ctrl->elem_size; >> + memset(ptr.p_char + idx, ' ', ctrl->maximum); >> + ptr.p_char[idx + ctrl->maximum] = '\0'; >> + break; >> + case V4L2_CTRL_TYPE_INTEGER64: >> + ptr.p_s64[idx] = ctrl->maximum; >> + break; >> + case V4L2_CTRL_TYPE_INTEGER: >> + case V4L2_CTRL_TYPE_INTEGER_MENU: >> + case V4L2_CTRL_TYPE_MENU: >> + case V4L2_CTRL_TYPE_BITMASK: >> + case V4L2_CTRL_TYPE_BOOLEAN: >> + ptr.p_s32[idx] = ctrl->maximum; >> + break; >> + case V4L2_CTRL_TYPE_BUTTON: >> + case V4L2_CTRL_TYPE_CTRL_CLASS: >> + ptr.p_s32[idx] = 0; >> + break; >> + case V4L2_CTRL_TYPE_U8: >> + ptr.p_u8[idx] = ctrl->maximum; >> + break; >> + case V4L2_CTRL_TYPE_U16: >> + ptr.p_u16[idx] = ctrl->maximum; >> + break; >> + case V4L2_CTRL_TYPE_U32: >> + ptr.p_u32[idx] = ctrl->maximum; >> + break; >> + default: >> + std_max_compound(ctrl, idx, ptr); >> + break; >> + } >> +} >> + >> static void std_log(const struct v4l2_ctrl *ctrl) >> { >> union v4l2_ctrl_ptr ptr = ctrl->p_cur; >> @@ -986,6 +1084,8 @@ static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx, >> static const struct v4l2_ctrl_type_ops std_type_ops = { >> .equal = std_equal, >> .init = std_init, >> + .minimum = std_minimum, >> + .maximum = std_maximum, >> .log = std_log, >> .validate = std_validate, >> }; >> @@ -1368,7 +1468,10 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, >> s64 min, s64 max, u64 step, s64 def, >> const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size, >> u32 flags, const char * const *qmenu, >> - const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def, >> + const s64 *qmenu_int, >> + const union v4l2_ctrl_ptr p_def, >> + const union v4l2_ctrl_ptr p_min, >> + const union v4l2_ctrl_ptr p_max, >> void *priv) >> { >> struct v4l2_ctrl *ctrl; >> @@ -1515,7 +1618,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, >> sz_extra += 2 * tot_ctrl_size; >> >> if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p_const) >> - sz_extra += elem_size; >> + sz_extra += elem_size * 3; >> >> ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL); >> if (ctrl == NULL) { >> @@ -1565,6 +1668,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, >> ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size; >> memcpy(ctrl->p_def.p, p_def.p_const, elem_size); >> } >> + if (type >= V4L2_CTRL_COMPOUND_TYPES && >> + p_min.p_const && p_max.p_const) { >> + ctrl->p_min.p = ctrl->p_cur.p + 2 * tot_ctrl_size; >> + memcpy(ctrl->p_min.p, p_min.p_const, elem_size); >> + ctrl->p_max.p = ctrl->p_cur.p + 3 * tot_ctrl_size; >> + memcpy(ctrl->p_max.p, p_max.p_const, elem_size); >> + } >> >> for (idx = 0; idx < elems; idx++) { >> ctrl->type_ops->init(ctrl, idx, ctrl->p_cur); >> @@ -1617,7 +1727,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, >> type, min, max, >> is_menu ? cfg->menu_skip_mask : step, def, >> cfg->dims, cfg->elem_size, >> - flags, qmenu, qmenu_int, cfg->p_def, priv); >> + flags, qmenu, qmenu_int, cfg->p_def, cfg->p_min, >> + cfg->p_max, priv); >> if (ctrl) >> ctrl->is_private = cfg->is_private; >> return ctrl; >> @@ -1642,7 +1753,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl, >> } >> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >> min, max, step, def, NULL, 0, >> - flags, NULL, NULL, ptr_null, NULL); >> + flags, NULL, NULL, ptr_null, ptr_null, >> + ptr_null, NULL); >> } >> EXPORT_SYMBOL(v4l2_ctrl_new_std); >> >> @@ -1675,7 +1787,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl, >> } >> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >> 0, max, mask, def, NULL, 0, >> - flags, qmenu, qmenu_int, ptr_null, NULL); >> + flags, qmenu, qmenu_int, ptr_null, ptr_null, >> + ptr_null, NULL); >> } >> EXPORT_SYMBOL(v4l2_ctrl_new_std_menu); >> >> @@ -1707,7 +1820,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, >> } >> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >> 0, max, mask, def, NULL, 0, >> - flags, qmenu, NULL, ptr_null, NULL); >> + flags, qmenu, NULL, ptr_null, ptr_null, >> + ptr_null, NULL); >> >> } >> EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); >> @@ -1715,7 +1829,9 @@ EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items); >> /* Helper function for standard compound controls */ >> struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, >> const struct v4l2_ctrl_ops *ops, u32 id, >> - const union v4l2_ctrl_ptr p_def) >> + const union v4l2_ctrl_ptr p_def, >> + const union v4l2_ctrl_ptr p_min, >> + const union v4l2_ctrl_ptr p_max) >> { >> const char *name; >> enum v4l2_ctrl_type type; >> @@ -1729,7 +1845,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, >> } >> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >> min, max, step, def, NULL, 0, >> - flags, NULL, NULL, p_def, NULL); >> + flags, NULL, NULL, p_def, p_min, p_max, NULL); >> } >> EXPORT_SYMBOL(v4l2_ctrl_new_std_compound); >> >> @@ -1753,7 +1869,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, >> } >> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type, >> 0, max, 0, def, NULL, 0, >> - flags, NULL, qmenu_int, ptr_null, NULL); >> + flags, NULL, qmenu_int, ptr_null, ptr_null, >> + ptr_null, NULL); >> } >> EXPORT_SYMBOL(v4l2_ctrl_new_int_menu); >> >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >> index 21470de62d72..0f713b9a95f9 100644 >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >> @@ -893,7 +893,9 @@ static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl) >> return false; >> break; >> case V4L2_CTRL_WHICH_DEF_VAL: >> - /* Default value cannot be changed */ >> + case V4L2_CTRL_WHICH_MIN_VAL: >> + case V4L2_CTRL_WHICH_MAX_VAL: >> + /* Default, minimum or maximum value cannot be changed */ >> if (ioctl == VIDIOC_S_EXT_CTRLS || >> ioctl == VIDIOC_TRY_EXT_CTRLS) { >> c->error_idx = c->count; >> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h >> index 919e104de50b..c8ab8c44d7c6 100644 >> --- a/include/media/v4l2-ctrls.h >> +++ b/include/media/v4l2-ctrls.h >> @@ -131,6 +131,8 @@ struct v4l2_ctrl_ops { >> * >> * @equal: return true if both values are equal. >> * @init: initialize the value. >> + * @minimum: set the value to the minimum value of the control. >> + * @maximum: set the value to the maximum value of the control. >> * @log: log the value. >> * @validate: validate the value. Return 0 on success and a negative value >> * otherwise. >> @@ -141,6 +143,10 @@ struct v4l2_ctrl_type_ops { >> union v4l2_ctrl_ptr ptr2); >> void (*init)(const struct v4l2_ctrl *ctrl, u32 idx, >> union v4l2_ctrl_ptr ptr); >> + void (*minimum)(const struct v4l2_ctrl *ctrl, u32 idx, >> + union v4l2_ctrl_ptr ptr); >> + void (*maximum)(const struct v4l2_ctrl *ctrl, u32 idx, >> + union v4l2_ctrl_ptr ptr); >> void (*log)(const struct v4l2_ctrl *ctrl); >> int (*validate)(const struct v4l2_ctrl *ctrl, u32 idx, >> union v4l2_ctrl_ptr ptr); >> @@ -237,6 +243,12 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv); >> * @p_def: The control's default value represented via a union which >> * provides a standard way of accessing control types >> * through a pointer (for compound controls only). >> + * @p_min: The control's minimum value represented via a union which >> + * provides a standard way of accessing control types >> + * through a pointer (for compound controls only). >> + * @p_max: The control's maximum value represented via a union which >> + * provides a standard way of accessing control types >> + * through a pointer (for compound controls only). >> * @p_cur: The control's current value represented via a union which >> * provides a standard way of accessing control types >> * through a pointer. >> @@ -292,6 +304,8 @@ struct v4l2_ctrl { >> } cur; >> >> union v4l2_ctrl_ptr p_def; >> + union v4l2_ctrl_ptr p_min; >> + union v4l2_ctrl_ptr p_max; >> union v4l2_ctrl_ptr p_new; >> union v4l2_ctrl_ptr p_cur; >> }; >> @@ -398,6 +412,8 @@ struct v4l2_ctrl_handler { >> * @step: The control's step value for non-menu controls. >> * @def: The control's default value. >> * @p_def: The control's default value for compound controls. >> + * @p_min: The control's minimum value for compound controls. >> + * @p_max: The control's maximum value for compound controls. >> * @dims: The size of each dimension. >> * @elem_size: The size in bytes of the control. >> * @flags: The control's flags. >> @@ -427,6 +443,8 @@ struct v4l2_ctrl_config { >> u64 step; >> s64 def; >> union v4l2_ctrl_ptr p_def; >> + union v4l2_ctrl_ptr p_min; >> + union v4l2_ctrl_ptr p_max; >> u32 dims[V4L2_CTRL_MAX_DIMS]; >> u32 elem_size; >> u32 flags; >> @@ -696,17 +714,21 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, >> * @ops: The control ops. >> * @id: The control ID. >> * @p_def: The control's default value. >> + * @p_min: The control's minimum value. >> + * @p_max: The control's maximum value. >> * >> - * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks >> - * to the @p_def field. Use v4l2_ctrl_ptr_create() to create @p_def from a >> - * pointer. Use v4l2_ctrl_ptr_create(NULL) if the default value of the >> - * compound control should be all zeroes. >> + * Same as v4l2_ctrl_new_std(), but with support to compound controls, thanks >> + * to the @p_def/min/max field. Use v4l2_ctrl_ptr_create() to create > > s/field/fields/ You forgot to fix this in v8. > >> + * @p_def/min/max from a pointer. Use v4l2_ctrl_ptr_create(NULL) if the >> + * default/min/max value of the compound control should be all zeroes. > > Hmmm... I expect that, for some compound control types, the concept of > minimum and maximum won't be defined. Wouldn't it better for a get > control operation on V4L2_CTRL_WHICH_{MIN,MAX}_VAL to return an error in > that case, instead of zeroed memory ? I agree with that. The error code needs to be documented, though. And I don't see that in v8. If I understand it correctly, then you return -EINVAL in v8 if no min or max info is available. I'm not so keen on that since it isn't an invalid argument. Perhaps -ENODATA? Regards, Hans > >> * >> */ >> struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, >> const struct v4l2_ctrl_ops *ops, >> u32 id, >> - const union v4l2_ctrl_ptr p_def); >> + const union v4l2_ctrl_ptr p_def, >> + const union v4l2_ctrl_ptr p_min, >> + const union v4l2_ctrl_ptr p_max); >> >> /** >> * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control. >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >> index 2e36bb610ea6..134f6a65eacc 100644 >> --- a/include/uapi/linux/videodev2.h >> +++ b/include/uapi/linux/videodev2.h >> @@ -1796,6 +1796,8 @@ struct v4l2_ext_controls { >> #define V4L2_CTRL_WHICH_CUR_VAL 0 >> #define V4L2_CTRL_WHICH_DEF_VAL 0x0f000000 >> #define V4L2_CTRL_WHICH_REQUEST_VAL 0x0f010000 >> +#define V4L2_CTRL_WHICH_MIN_VAL 0x0f020000 >> +#define V4L2_CTRL_WHICH_MAX_VAL 0x0f030000 >> >> enum v4l2_ctrl_type { >> V4L2_CTRL_TYPE_INTEGER = 1, >