| Message ID | 20241114-uvc-roi-v15-18-64cfeb56b6f8@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Implement UVC v1.5 ROI | expand |
Hi Gergo Sorry, I forgot to reply to your comment in v14. On Thu, 14 Nov 2024 at 20:53, Gergo Koteles <soyer@irl.hu> wrote: > > Hi Ricardo, > > On Thu, 2024-11-14 at 19:10 +0000, Ricardo Ribalda wrote: > > > > + }, > > + { > > + .id = V4L2_CID_UVC_REGION_OF_INTEREST_AUTO, > > + .entity = UVC_GUID_UVC_CAMERA, > > + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL, > > + .size = 16, > > + .offset = 64, > > + .v4l2_type = V4L2_CTRL_TYPE_BITMASK, > > + .data_type = UVC_CTRL_DATA_TYPE_BITMASK, > > + .name = "Region Of Interest Auto Controls", > > + }, > > }; > > > > Wouldn't be better to use 8 V4L2_CTRL_TYPE_BOOLEAN controls for this? If I create 8 Booleans, they will always be shown in the device. And the user will not have a way to know which values are available and which are not. We will also fail the v4l2-compliance test, because there will be up to 7 boolean controls that will not be able to be set to 1, eventhough they are writable. Thanks for the prompt review. > > Thanks, > Gergo > -- Ricardo Ribalda
Hi On Thu, 14 Nov 2024 at 21:16, Gergo Koteles <soyer@irl.hu> wrote: > > Hi Ricardo, > > On Thu, 2024-11-14 at 21:03 +0100, Ricardo Ribalda wrote: > > Hi Gergo > > > > Sorry, I forgot to reply to your comment in v14. > > > > On Thu, 14 Nov 2024 at 20:53, Gergo Koteles <soyer@irl.hu> wrote: > > > > > > Hi Ricardo, > > > > > > On Thu, 2024-11-14 at 19:10 +0000, Ricardo Ribalda wrote: > > > > > > > > + }, > > > > + { > > > > + .id = V4L2_CID_UVC_REGION_OF_INTEREST_AUTO, > > > > + .entity = UVC_GUID_UVC_CAMERA, > > > > + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL, > > > > + .size = 16, > > > > + .offset = 64, > > > > + .v4l2_type = V4L2_CTRL_TYPE_BITMASK, > > > > + .data_type = UVC_CTRL_DATA_TYPE_BITMASK, > > > > + .name = "Region Of Interest Auto Controls", > > > > + }, > > > > }; > > > > > > > > > > Wouldn't be better to use 8 V4L2_CTRL_TYPE_BOOLEAN controls for this? > > > > If I create 8 Booleans, they will always be shown in the device. And > > the user will not have a way to know which values are available and > > which are not. > > > > We will also fail the v4l2-compliance test, because there will be up > > to 7 boolean controls that will not be able to be set to 1, eventhough > > they are writable. > > > > And can't it be that only those returned by GET_MAX be added? > > ``` > The bmAutoControls bitmask determines which, if any, on board features > should track to the region of interest. To detect if a device supports > a particular Auto Control, use GET_MAX which returns a mask indicating > all supported Auto Controls. > ``` > > Sorry for the misunderstanding, I just don't quite understand. I guess we could, but we would have to make a big change in the way the controls are probed today. uvc does not use the control framework. What will be the benefit of using 8 controls? - Applications still have to know what those controls do, they should not rely on the control name. - Changing from lets say AUTO_EXPOSURE to AUTO_FOCUS, will require to send at least 2 controls via v4l2_s_ext_control... I think it is more practical and less prone to errrors to send just one control If the number of autos were unknown, then having multiple controls could be a good idea, but they are part of the uvc standard and unlikely to be changed. Thanks! > > Thanks, > > Gergo
Hi Ricardo, On Thu, 2024-11-14 at 21:28 +0100, Ricardo Ribalda wrote: > Hi > > On Thu, 14 Nov 2024 at 21:16, Gergo Koteles <soyer@irl.hu> wrote: > > > > Hi Ricardo, > > > > On Thu, 2024-11-14 at 21:03 +0100, Ricardo Ribalda wrote: > > > Hi Gergo > > > > > > Sorry, I forgot to reply to your comment in v14. > > > > > > On Thu, 14 Nov 2024 at 20:53, Gergo Koteles <soyer@irl.hu> wrote: > > > > > > > > Hi Ricardo, > > > > > > > > On Thu, 2024-11-14 at 19:10 +0000, Ricardo Ribalda wrote: > > > > > > > > > > + }, > > > > > + { > > > > > + .id = V4L2_CID_UVC_REGION_OF_INTEREST_AUTO, > > > > > + .entity = UVC_GUID_UVC_CAMERA, > > > > > + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL, > > > > > + .size = 16, > > > > > + .offset = 64, > > > > > + .v4l2_type = V4L2_CTRL_TYPE_BITMASK, > > > > > + .data_type = UVC_CTRL_DATA_TYPE_BITMASK, > > > > > + .name = "Region Of Interest Auto Controls", > > > > > + }, > > > > > }; > > > > > > > > > > > > > Wouldn't be better to use 8 V4L2_CTRL_TYPE_BOOLEAN controls for this? > > > > > > If I create 8 Booleans, they will always be shown in the device. And > > > the user will not have a way to know which values are available and > > > which are not. > > > > > > We will also fail the v4l2-compliance test, because there will be up > > > to 7 boolean controls that will not be able to be set to 1, eventhough > > > they are writable. > > > > > > > And can't it be that only those returned by GET_MAX be added? > > > > ``` > > The bmAutoControls bitmask determines which, if any, on board features > > should track to the region of interest. To detect if a device supports > > a particular Auto Control, use GET_MAX which returns a mask indicating > > all supported Auto Controls. > > ``` > > > > Sorry for the misunderstanding, I just don't quite understand. > > I guess we could, but we would have to make a big change in the way > the controls are probed today. uvc does not use the control framework. > > What will be the benefit of using 8 controls? > - Applications still have to know what those controls do, they should > not rely on the control name. Applications like v4l2-ctl are not aware of every controls, work by control type, and let the user decide what to do, based on the name. To avoid having to know each bitmask type control, they need to be able to query which bit means what and what to display to the user. Could VIDIOC_QUERYMENU be supplemented with this? > - Changing from lets say AUTO_EXPOSURE to AUTO_FOCUS, will require to > send at least 2 controls via v4l2_s_ext_control... I think it is more > practical and less prone to errrors to send just one control > Yes, that could be a good reason. Thanks, Gergo
On Fri, 15 Nov 2024 at 01:04, Gergo Koteles <soyer@irl.hu> wrote: > > Hi Ricardo, > > On Thu, 2024-11-14 at 21:28 +0100, Ricardo Ribalda wrote: > > Hi > > > > On Thu, 14 Nov 2024 at 21:16, Gergo Koteles <soyer@irl.hu> wrote: > > > > > > Hi Ricardo, > > > > > > On Thu, 2024-11-14 at 21:03 +0100, Ricardo Ribalda wrote: > > > > Hi Gergo > > > > > > > > Sorry, I forgot to reply to your comment in v14. > > > > > > > > On Thu, 14 Nov 2024 at 20:53, Gergo Koteles <soyer@irl.hu> wrote: > > > > > > > > > > Hi Ricardo, > > > > > > > > > > On Thu, 2024-11-14 at 19:10 +0000, Ricardo Ribalda wrote: > > > > > > > > > > > > + }, > > > > > > + { > > > > > > + .id = V4L2_CID_UVC_REGION_OF_INTEREST_AUTO, > > > > > > + .entity = UVC_GUID_UVC_CAMERA, > > > > > > + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL, > > > > > > + .size = 16, > > > > > > + .offset = 64, > > > > > > + .v4l2_type = V4L2_CTRL_TYPE_BITMASK, > > > > > > + .data_type = UVC_CTRL_DATA_TYPE_BITMASK, > > > > > > + .name = "Region Of Interest Auto Controls", > > > > > > + }, > > > > > > }; > > > > > > > > > > > > > > > > Wouldn't be better to use 8 V4L2_CTRL_TYPE_BOOLEAN controls for this? > > > > > > > > If I create 8 Booleans, they will always be shown in the device. And > > > > the user will not have a way to know which values are available and > > > > which are not. > > > > > > > > We will also fail the v4l2-compliance test, because there will be up > > > > to 7 boolean controls that will not be able to be set to 1, eventhough > > > > they are writable. > > > > > > > > > > And can't it be that only those returned by GET_MAX be added? > > > > > > ``` > > > The bmAutoControls bitmask determines which, if any, on board features > > > should track to the region of interest. To detect if a device supports > > > a particular Auto Control, use GET_MAX which returns a mask indicating > > > all supported Auto Controls. > > > ``` > > > > > > Sorry for the misunderstanding, I just don't quite understand. > > > > I guess we could, but we would have to make a big change in the way > > the controls are probed today. uvc does not use the control framework. > > > > What will be the benefit of using 8 controls? > > - Applications still have to know what those controls do, they should > > not rely on the control name. > > Applications like v4l2-ctl are not aware of every controls, work by > control type, and let the user decide what to do, based on the name. > > To avoid having to know each bitmask type control, they need to be able > to query which bit means what and what to display to the user. > > Could VIDIOC_QUERYMENU be supplemented with this? I believe that violates compliance. VIDIOC_QUERYMENU should only be used on menus. https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-controls.cpp?h=v4l-utils-1.28.1#n143 > > > > - Changing from lets say AUTO_EXPOSURE to AUTO_FOCUS, will require to > > send at least 2 controls via v4l2_s_ext_control... I think it is more > > practical and less prone to errrors to send just one control > > > > Yes, that could be a good reason. > > Thanks, > Gergo > >
Hi Ricardo, On 14-Nov-24 9:03 PM, Ricardo Ribalda wrote: > Hi Gergo > > Sorry, I forgot to reply to your comment in v14. > > On Thu, 14 Nov 2024 at 20:53, Gergo Koteles <soyer@irl.hu> wrote: >> >> Hi Ricardo, >> >> On Thu, 2024-11-14 at 19:10 +0000, Ricardo Ribalda wrote: >>> >>> + }, >>> + { >>> + .id = V4L2_CID_UVC_REGION_OF_INTEREST_AUTO, >>> + .entity = UVC_GUID_UVC_CAMERA, >>> + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL, >>> + .size = 16, >>> + .offset = 64, >>> + .v4l2_type = V4L2_CTRL_TYPE_BITMASK, >>> + .data_type = UVC_CTRL_DATA_TYPE_BITMASK, >>> + .name = "Region Of Interest Auto Controls", >>> + }, >>> }; >>> >> >> Wouldn't be better to use 8 V4L2_CTRL_TYPE_BOOLEAN controls for this? > > If I create 8 Booleans, they will always be shown in the device. And > the user will not have a way to know which values are available and > which are not. > > We will also fail the v4l2-compliance test, because there will be up > to 7 boolean controls that will not be able to be set to 1, eventhough > they are writable. So why can't these other controls be set to 1? Because only one of the options in the bitmask can be selected at a time ? If only 1 bit in the UVC_CTRL_DATA_TYPE_BITMASK for this can be one at the time, then this should be mapped to a V4L2_CTRL_TYPE_MENU just like how that is done for V4L2_CID_EXPOSURE_AUTO already. Actually looking at existing comments about UVC_CTRL_DATA_TYPE_BITMASK in the driver there is this comment on top of uvc_mapping_get_menu_value() * For controls of type UVC_CTRL_DATA_TYPE_BITMASK, the UVC control value is * expressed as a bitmask and is thus guaranteed to have a single bit set. Assuming this "guaranteed to have a single bit set" comment is valid for the V4L2_CID_UVC_REGION_OF_INTEREST_AUTO part of UVC_CT_REGION_OF_INTEREST_CONTROL too then I think we should simply map this to a menu similar to how this is done for V4L2_CID_EXPOSURE_AUTO. Note V4L2_CID_EXPOSURE_AUTO is the only existing user of UVC_CTRL_DATA_TYPE_BITMASK at the moment. Mapping this to a menu should nicely address Gergo's concerns here. Regards, Hans
Hi On Mon, Nov 18, 2024 at 4:59 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Ricardo, > > On 14-Nov-24 9:03 PM, Ricardo Ribalda wrote: > > Hi Gergo > > > > Sorry, I forgot to reply to your comment in v14. > > > > On Thu, 14 Nov 2024 at 20:53, Gergo Koteles <soyer@irl.hu> wrote: > >> > >> Hi Ricardo, > >> > >> On Thu, 2024-11-14 at 19:10 +0000, Ricardo Ribalda wrote: > >>> > >>> + }, > >>> + { > >>> + .id = V4L2_CID_UVC_REGION_OF_INTEREST_AUTO, > >>> + .entity = UVC_GUID_UVC_CAMERA, > >>> + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL, > >>> + .size = 16, > >>> + .offset = 64, > >>> + .v4l2_type = V4L2_CTRL_TYPE_BITMASK, > >>> + .data_type = UVC_CTRL_DATA_TYPE_BITMASK, > >>> + .name = "Region Of Interest Auto Controls", > >>> + }, > >>> }; > >>> > >> > >> Wouldn't be better to use 8 V4L2_CTRL_TYPE_BOOLEAN controls for this? > > > > If I create 8 Booleans, they will always be shown in the device. And > > the user will not have a way to know which values are available and > > which are not. > > > > We will also fail the v4l2-compliance test, because there will be up > > to 7 boolean controls that will not be able to be set to 1, eventhough > > they are writable. > > So why can't these other controls be set to 1? Because only one > of the options in the bitmask can be selected at a time ? > > If only 1 bit in the UVC_CTRL_DATA_TYPE_BITMASK for this can be one > at the time, then this should be mapped to a V4L2_CTRL_TYPE_MENU > just like how that is done for V4L2_CID_EXPOSURE_AUTO already. > > Actually looking at existing comments about UVC_CTRL_DATA_TYPE_BITMASK > in the driver there is this comment on top of uvc_mapping_get_menu_value() > > * For controls of type UVC_CTRL_DATA_TYPE_BITMASK, the UVC control value is > * expressed as a bitmask and is thus guaranteed to have a single bit set. > > Assuming this "guaranteed to have a single bit set" comment is valid for > the V4L2_CID_UVC_REGION_OF_INTEREST_AUTO part of UVC_CT_REGION_OF_INTEREST_CONTROL > too then I think we should simply map this to a menu similar to how > this is done for V4L2_CID_EXPOSURE_AUTO. > > Note V4L2_CID_EXPOSURE_AUTO is the only existing user of UVC_CTRL_DATA_TYPE_BITMASK > at the moment. > > Mapping this to a menu should nicely address Gergo's concerns here. The UVC standard is not very clear re bmAutoControls. It says: """ The bmAutoControls bitmask determines which, if any, on board features should track to the region of interest. To detect if a device supports a particular Auto Control, use GET_MAX which returns a mask indicating all supported Auto Controls. GET_CUR returns the current Region of Interest (RoI) being employed by the device. This RoI should be the same as specified in most recent SET_CUR except in the case where the ‘Auto Detect and Track’ and/or ‘Image Stabilization’ bit have been set. """ Which makes me believe that you can set another Auto value + one of these ones. So I do not think that we can assume "guaranteed to have a single bit set". The behaviour will vary module to module. So I'd rather take a conservative approach here and let the hardware clamp the value and not us. > > Regards, > > Hans >
Hi Ricardo, On 18-Nov-24 5:16 PM, Ricardo Ribalda Delgado wrote: > Hi > > On Mon, Nov 18, 2024 at 4:59 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi Ricardo, >> >> On 14-Nov-24 9:03 PM, Ricardo Ribalda wrote: >>> Hi Gergo >>> >>> Sorry, I forgot to reply to your comment in v14. >>> >>> On Thu, 14 Nov 2024 at 20:53, Gergo Koteles <soyer@irl.hu> wrote: >>>> >>>> Hi Ricardo, >>>> >>>> On Thu, 2024-11-14 at 19:10 +0000, Ricardo Ribalda wrote: >>>>> >>>>> + }, >>>>> + { >>>>> + .id = V4L2_CID_UVC_REGION_OF_INTEREST_AUTO, >>>>> + .entity = UVC_GUID_UVC_CAMERA, >>>>> + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL, >>>>> + .size = 16, >>>>> + .offset = 64, >>>>> + .v4l2_type = V4L2_CTRL_TYPE_BITMASK, >>>>> + .data_type = UVC_CTRL_DATA_TYPE_BITMASK, >>>>> + .name = "Region Of Interest Auto Controls", >>>>> + }, >>>>> }; >>>>> >>>> >>>> Wouldn't be better to use 8 V4L2_CTRL_TYPE_BOOLEAN controls for this? >>> >>> If I create 8 Booleans, they will always be shown in the device. And >>> the user will not have a way to know which values are available and >>> which are not. >>> >>> We will also fail the v4l2-compliance test, because there will be up >>> to 7 boolean controls that will not be able to be set to 1, eventhough >>> they are writable. >> >> So why can't these other controls be set to 1? Because only one >> of the options in the bitmask can be selected at a time ? >> >> If only 1 bit in the UVC_CTRL_DATA_TYPE_BITMASK for this can be one >> at the time, then this should be mapped to a V4L2_CTRL_TYPE_MENU >> just like how that is done for V4L2_CID_EXPOSURE_AUTO already. >> >> Actually looking at existing comments about UVC_CTRL_DATA_TYPE_BITMASK >> in the driver there is this comment on top of uvc_mapping_get_menu_value() >> >> * For controls of type UVC_CTRL_DATA_TYPE_BITMASK, the UVC control value is >> * expressed as a bitmask and is thus guaranteed to have a single bit set. >> >> Assuming this "guaranteed to have a single bit set" comment is valid for >> the V4L2_CID_UVC_REGION_OF_INTEREST_AUTO part of UVC_CT_REGION_OF_INTEREST_CONTROL >> too then I think we should simply map this to a menu similar to how >> this is done for V4L2_CID_EXPOSURE_AUTO. >> >> Note V4L2_CID_EXPOSURE_AUTO is the only existing user of UVC_CTRL_DATA_TYPE_BITMASK >> at the moment. >> >> Mapping this to a menu should nicely address Gergo's concerns here. > > The UVC standard is not very clear re bmAutoControls. It says: > """ > The bmAutoControls bitmask determines which, if any, on board features > should track to the region of interest. To detect if a device supports > a particular Auto Control, use GET_MAX which returns a mask indicating > all supported Auto Controls. > GET_CUR returns the current Region of Interest (RoI) being employed by > the device. This RoI should be the same as specified in most recent > SET_CUR except in the case where the ‘Auto Detect and Track’ and/or > ‘Image Stabilization’ bit have been set. > """ > > Which makes me believe that you can set another Auto value + one of > these ones. So I do not think that we can assume "guaranteed to have a > single bit set". I see I already was afraid it would be something like this but it would have been nice if we could have turned this into a menu control. > The behaviour will vary module to module. So I'd rather take a > conservative approach here and let the hardware clamp the value and > not us. Agreed. Regards, Hans
On Mon, Dec 2, 2024 at 6:26 PM Ricardo Ribalda <ribalda@chromium.org> wrote: > > On Mon, 2 Dec 2024 at 09:02, Yunke Cao <yunkec@google.com> wrote: > > > > Hi Ricardo, > > > > Thanks for the new version!! > > > > On Fri, Nov 15, 2024 at 4:11 AM Ricardo Ribalda <ribalda@chromium.org> wrote: > > > > > > From: Yunke Cao <yunkec@google.com> > > > > > > Implement support for ROI as described in UVC 1.5: > > > 4.2.2.1.20 Digital Region of Interest (ROI) Control > > > > > > ROI control is implemented using V4L2 control API as > > > two UVC-specific controls: > > > V4L2_CID_UVC_REGION_OF_INTEREST_RECT and > > > V4L2_CID_UVC_REGION_OF_INTEREST_AUTO. > > > > > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > > Signed-off-by: Yunke Cao <yunkec@google.com> > > > --- > > > drivers/media/usb/uvc/uvc_ctrl.c | 81 ++++++++++++++++++++++++++++++++++++++ > > > drivers/media/usb/uvc/uvcvideo.h | 7 ++++ > > > include/uapi/linux/usb/video.h | 1 + > > > include/uapi/linux/uvcvideo.h | 13 ++++++ > > > include/uapi/linux/v4l2-controls.h | 9 +++++ > > > 5 files changed, 111 insertions(+) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > > index f262e05ad3a8..5b619ef40dd3 100644 > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > > @@ -358,6 +358,24 @@ static const struct uvc_control_info uvc_ctrls[] = { > > > .flags = UVC_CTRL_FLAG_GET_CUR > > > | UVC_CTRL_FLAG_AUTO_UPDATE, > > > }, > > > + /* > > > + * UVC_CTRL_FLAG_AUTO_UPDATE is needed because the RoI may get updated > > > + * by sensors. > > > + * "This RoI should be the same as specified in most recent SET_CUR > > > + * except in the case where the ‘Auto Detect and Track’ and/or > > > + * ‘Image Stabilization’ bit have been set." > > > + * 4.2.2.1.20 Digital Region of Interest (ROI) Control > > > + */ > > > + { > > > + .entity = UVC_GUID_UVC_CAMERA, > > > + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL, > > > + .index = 21, > > > + .size = 10, > > > + .flags = UVC_CTRL_FLAG_SET_CUR | UVC_CTRL_FLAG_GET_CUR > > > + | UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX > > > + | UVC_CTRL_FLAG_GET_DEF > > > + | UVC_CTRL_FLAG_AUTO_UPDATE, > > > + }, > > > }; > > > > > > static const u32 uvc_control_classes[] = { > > > @@ -603,6 +621,44 @@ static const struct uvc_control_mapping *uvc_ctrl_filter_plf_mapping( > > > return out_mapping; > > > } > > > > > > +static int uvc_get_rect(struct uvc_control_mapping *mapping, u8 query, > > > + const void *uvc_in, size_t v4l2_size, void *v4l2_out) > > > +{ > > > + const struct uvc_rect *uvc_rect = uvc_in; > > > + struct v4l2_rect *v4l2_rect = v4l2_out; > > > + > > > + if (WARN_ON(v4l2_size != sizeof(struct v4l2_rect))) > > > + return -EINVAL; > > > + > > > + if (uvc_rect->left > uvc_rect->right || > > > + uvc_rect->top > uvc_rect->bottom) > > > + return -EIO; > > > + > > > + v4l2_rect->top = uvc_rect->top; > > > + v4l2_rect->left = uvc_rect->left; > > > + v4l2_rect->height = uvc_rect->bottom - uvc_rect->top + 1; > > > + v4l2_rect->width = uvc_rect->right - uvc_rect->left + 1; > > > + > > > + return 0; > > > +} > > > + > > > +static int uvc_set_rect(struct uvc_control_mapping *mapping, size_t v4l2_size, > > > + const void *v4l2_in, void *uvc_out) > > > +{ > > > + struct uvc_rect *uvc_rect = uvc_out; > > > + const struct v4l2_rect *v4l2_rect = v4l2_in; > > > + > > > + if (WARN_ON(v4l2_size != sizeof(struct v4l2_rect))) > > > + return -EINVAL; > > > + > > > + uvc_rect->top = max(0xffff, v4l2_rect->top); > > > + uvc_rect->left = max(0xffff, v4l2_rect->left); > > > + uvc_rect->bottom = max(0xffff, v4l2_rect->height + v4l2_rect->top - 1); > > > + uvc_rect->right = max(0xffff, v4l2_rect->width + v4l2_rect->left - 1); > > > > Should these be min()? > > Ups :). > > Fixed in the next version. > > Thanks! I've tested the patchset with this fix on Chrome OS and verified ROI works as expected. Reviewed-by: Yunke Cao <yunkec@google.com> Tested-by: Yunke Cao <yunkec@google.com> > > > > > > > > > + > > > + return 0; > > > +} > > > + > > > static const struct uvc_control_mapping uvc_ctrl_mappings[] = { > > > { > > > .id = V4L2_CID_BRIGHTNESS, > > > @@ -897,6 +953,28 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = { > > > .selector = UVC_PU_POWER_LINE_FREQUENCY_CONTROL, > > > .filter_mapping = uvc_ctrl_filter_plf_mapping, > > > }, > > > + { > > > + .id = V4L2_CID_UVC_REGION_OF_INTEREST_RECT, > > > + .entity = UVC_GUID_UVC_CAMERA, > > > + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL, > > > + .size = sizeof(struct uvc_rect) * 8, > > > + .offset = 0, > > > + .v4l2_type = V4L2_CTRL_TYPE_RECT, > > > + .data_type = UVC_CTRL_DATA_TYPE_RECT, > > > + .get = uvc_get_rect, > > > + .set = uvc_set_rect, > > > + .name = "Region Of Interest Rectangle", > > > + }, > > > + { > > > + .id = V4L2_CID_UVC_REGION_OF_INTEREST_AUTO, > > > + .entity = UVC_GUID_UVC_CAMERA, > > > + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL, > > > + .size = 16, > > > + .offset = 64, > > > + .v4l2_type = V4L2_CTRL_TYPE_BITMASK, > > > + .data_type = UVC_CTRL_DATA_TYPE_BITMASK, > > > + .name = "Region Of Interest Auto Controls", > > > + }, > > > }; > > > > > > /* ------------------------------------------------------------------------ > > > @@ -1465,6 +1543,9 @@ static int __uvc_queryctrl_boundaries(struct uvc_video_chain *chain, > > > > > > static size_t uvc_mapping_v4l2_size(struct uvc_control_mapping *mapping) > > > { > > > + if (mapping->v4l2_type == V4L2_CTRL_TYPE_RECT) > > > + return sizeof(struct v4l2_rect); > > > + > > > if (uvc_ctrl_mapping_is_compound(mapping)) > > > return DIV_ROUND_UP(mapping->size, 8); > > > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > > index 8aca1a2fe587..d910a5e5b514 100644 > > > --- a/drivers/media/usb/uvc/uvcvideo.h > > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > > @@ -294,6 +294,13 @@ struct uvc_streaming_header { > > > u8 bTriggerUsage; > > > }; > > > > > > +struct uvc_rect { > > > + u16 top; > > > + u16 left; > > > + u16 bottom; > > > + u16 right; > > > +} __packed; > > > + > > > enum uvc_buffer_state { > > > UVC_BUF_STATE_IDLE = 0, > > > UVC_BUF_STATE_QUEUED = 1, > > > diff --git a/include/uapi/linux/usb/video.h b/include/uapi/linux/usb/video.h > > > index 2ff0e8a3a683..2afb4420e6c4 100644 > > > --- a/include/uapi/linux/usb/video.h > > > +++ b/include/uapi/linux/usb/video.h > > > @@ -104,6 +104,7 @@ > > > #define UVC_CT_ROLL_ABSOLUTE_CONTROL 0x0f > > > #define UVC_CT_ROLL_RELATIVE_CONTROL 0x10 > > > #define UVC_CT_PRIVACY_CONTROL 0x11 > > > +#define UVC_CT_REGION_OF_INTEREST_CONTROL 0x14 > > > > > > /* A.9.5. Processing Unit Control Selectors */ > > > #define UVC_PU_CONTROL_UNDEFINED 0x00 > > > diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h > > > index f86185456dc5..cbe15bca9569 100644 > > > --- a/include/uapi/linux/uvcvideo.h > > > +++ b/include/uapi/linux/uvcvideo.h > > > @@ -16,6 +16,7 @@ > > > #define UVC_CTRL_DATA_TYPE_BOOLEAN 3 > > > #define UVC_CTRL_DATA_TYPE_ENUM 4 > > > #define UVC_CTRL_DATA_TYPE_BITMASK 5 > > > +#define UVC_CTRL_DATA_TYPE_RECT 6 > > > > > > /* Control flags */ > > > #define UVC_CTRL_FLAG_SET_CUR (1 << 0) > > > @@ -38,6 +39,18 @@ > > > > > > #define UVC_MENU_NAME_LEN 32 > > > > > > +/* V4L2 driver-specific controls */ > > > +#define V4L2_CID_UVC_REGION_OF_INTEREST_RECT (V4L2_CID_USER_UVC_BASE + 1) > > > +#define V4L2_CID_UVC_REGION_OF_INTEREST_AUTO (V4L2_CID_USER_UVC_BASE + 2) > > > +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_EXPOSURE (1 << 0) > > > +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_IRIS (1 << 1) > > > +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_WHITE_BALANCE (1 << 2) > > > +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_FOCUS (1 << 3) > > > +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_FACE_DETECT (1 << 4) > > > +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK (1 << 5) > > > +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_IMAGE_STABILIZATION (1 << 6) > > > +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY (1 << 7) > > > + > > > struct uvc_menu_info { > > > __u32 value; > > > __u8 name[UVC_MENU_NAME_LEN]; > > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > > > index 974fd254e573..6c91d6fa4708 100644 > > > --- a/include/uapi/linux/v4l2-controls.h > > > +++ b/include/uapi/linux/v4l2-controls.h > > > @@ -215,6 +215,13 @@ enum v4l2_colorfx { > > > */ > > > #define V4L2_CID_USER_THP7312_BASE (V4L2_CID_USER_BASE + 0x11c0) > > > > > > +/* > > > + * The base for the uvc driver controls. > > > + * See linux/uvcvideo.h for the list of controls. > > > + * We reserve 64 controls for this driver. > > > + */ > > > +#define V4L2_CID_USER_UVC_BASE (V4L2_CID_USER_BASE + 0x11e0) > > > + > > > /* MPEG-class control IDs */ > > > /* The MPEG controls are applicable to all codec controls > > > * and the 'MPEG' part of the define is historical */ > > > @@ -1089,6 +1096,8 @@ enum v4l2_auto_focus_range { > > > > > > #define V4L2_CID_HDR_SENSOR_MODE (V4L2_CID_CAMERA_CLASS_BASE+36) > > > > > > +/* CAMERA-class private control IDs */ > > > + > > > > Do we still need this comment? > > > > Best, > > Yunke > > > > > /* FM Modulator class control IDs */ > > > > > > #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900) > > > > > > -- > > > 2.47.0.338.g60cca15819-goog > > > > > > > -- > Ricardo Ribalda
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index f262e05ad3a8..5b619ef40dd3 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -358,6 +358,24 @@ static const struct uvc_control_info uvc_ctrls[] = { .flags = UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_AUTO_UPDATE, }, + /* + * UVC_CTRL_FLAG_AUTO_UPDATE is needed because the RoI may get updated + * by sensors. + * "This RoI should be the same as specified in most recent SET_CUR + * except in the case where the ‘Auto Detect and Track’ and/or + * ‘Image Stabilization’ bit have been set." + * 4.2.2.1.20 Digital Region of Interest (ROI) Control + */ + { + .entity = UVC_GUID_UVC_CAMERA, + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL, + .index = 21, + .size = 10, + .flags = UVC_CTRL_FLAG_SET_CUR | UVC_CTRL_FLAG_GET_CUR + | UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX + | UVC_CTRL_FLAG_GET_DEF + | UVC_CTRL_FLAG_AUTO_UPDATE, + }, }; static const u32 uvc_control_classes[] = { @@ -603,6 +621,44 @@ static const struct uvc_control_mapping *uvc_ctrl_filter_plf_mapping( return out_mapping; } +static int uvc_get_rect(struct uvc_control_mapping *mapping, u8 query, + const void *uvc_in, size_t v4l2_size, void *v4l2_out) +{ + const struct uvc_rect *uvc_rect = uvc_in; + struct v4l2_rect *v4l2_rect = v4l2_out; + + if (WARN_ON(v4l2_size != sizeof(struct v4l2_rect))) + return -EINVAL; + + if (uvc_rect->left > uvc_rect->right || + uvc_rect->top > uvc_rect->bottom) + return -EIO; + + v4l2_rect->top = uvc_rect->top; + v4l2_rect->left = uvc_rect->left; + v4l2_rect->height = uvc_rect->bottom - uvc_rect->top + 1; + v4l2_rect->width = uvc_rect->right - uvc_rect->left + 1; + + return 0; +} + +static int uvc_set_rect(struct uvc_control_mapping *mapping, size_t v4l2_size, + const void *v4l2_in, void *uvc_out) +{ + struct uvc_rect *uvc_rect = uvc_out; + const struct v4l2_rect *v4l2_rect = v4l2_in; + + if (WARN_ON(v4l2_size != sizeof(struct v4l2_rect))) + return -EINVAL; + + uvc_rect->top = max(0xffff, v4l2_rect->top); + uvc_rect->left = max(0xffff, v4l2_rect->left); + uvc_rect->bottom = max(0xffff, v4l2_rect->height + v4l2_rect->top - 1); + uvc_rect->right = max(0xffff, v4l2_rect->width + v4l2_rect->left - 1); + + return 0; +} + static const struct uvc_control_mapping uvc_ctrl_mappings[] = { { .id = V4L2_CID_BRIGHTNESS, @@ -897,6 +953,28 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = { .selector = UVC_PU_POWER_LINE_FREQUENCY_CONTROL, .filter_mapping = uvc_ctrl_filter_plf_mapping, }, + { + .id = V4L2_CID_UVC_REGION_OF_INTEREST_RECT, + .entity = UVC_GUID_UVC_CAMERA, + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL, + .size = sizeof(struct uvc_rect) * 8, + .offset = 0, + .v4l2_type = V4L2_CTRL_TYPE_RECT, + .data_type = UVC_CTRL_DATA_TYPE_RECT, + .get = uvc_get_rect, + .set = uvc_set_rect, + .name = "Region Of Interest Rectangle", + }, + { + .id = V4L2_CID_UVC_REGION_OF_INTEREST_AUTO, + .entity = UVC_GUID_UVC_CAMERA, + .selector = UVC_CT_REGION_OF_INTEREST_CONTROL, + .size = 16, + .offset = 64, + .v4l2_type = V4L2_CTRL_TYPE_BITMASK, + .data_type = UVC_CTRL_DATA_TYPE_BITMASK, + .name = "Region Of Interest Auto Controls", + }, }; /* ------------------------------------------------------------------------ @@ -1465,6 +1543,9 @@ static int __uvc_queryctrl_boundaries(struct uvc_video_chain *chain, static size_t uvc_mapping_v4l2_size(struct uvc_control_mapping *mapping) { + if (mapping->v4l2_type == V4L2_CTRL_TYPE_RECT) + return sizeof(struct v4l2_rect); + if (uvc_ctrl_mapping_is_compound(mapping)) return DIV_ROUND_UP(mapping->size, 8); diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 8aca1a2fe587..d910a5e5b514 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -294,6 +294,13 @@ struct uvc_streaming_header { u8 bTriggerUsage; }; +struct uvc_rect { + u16 top; + u16 left; + u16 bottom; + u16 right; +} __packed; + enum uvc_buffer_state { UVC_BUF_STATE_IDLE = 0, UVC_BUF_STATE_QUEUED = 1, diff --git a/include/uapi/linux/usb/video.h b/include/uapi/linux/usb/video.h index 2ff0e8a3a683..2afb4420e6c4 100644 --- a/include/uapi/linux/usb/video.h +++ b/include/uapi/linux/usb/video.h @@ -104,6 +104,7 @@ #define UVC_CT_ROLL_ABSOLUTE_CONTROL 0x0f #define UVC_CT_ROLL_RELATIVE_CONTROL 0x10 #define UVC_CT_PRIVACY_CONTROL 0x11 +#define UVC_CT_REGION_OF_INTEREST_CONTROL 0x14 /* A.9.5. Processing Unit Control Selectors */ #define UVC_PU_CONTROL_UNDEFINED 0x00 diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h index f86185456dc5..cbe15bca9569 100644 --- a/include/uapi/linux/uvcvideo.h +++ b/include/uapi/linux/uvcvideo.h @@ -16,6 +16,7 @@ #define UVC_CTRL_DATA_TYPE_BOOLEAN 3 #define UVC_CTRL_DATA_TYPE_ENUM 4 #define UVC_CTRL_DATA_TYPE_BITMASK 5 +#define UVC_CTRL_DATA_TYPE_RECT 6 /* Control flags */ #define UVC_CTRL_FLAG_SET_CUR (1 << 0) @@ -38,6 +39,18 @@ #define UVC_MENU_NAME_LEN 32 +/* V4L2 driver-specific controls */ +#define V4L2_CID_UVC_REGION_OF_INTEREST_RECT (V4L2_CID_USER_UVC_BASE + 1) +#define V4L2_CID_UVC_REGION_OF_INTEREST_AUTO (V4L2_CID_USER_UVC_BASE + 2) +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_EXPOSURE (1 << 0) +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_IRIS (1 << 1) +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_WHITE_BALANCE (1 << 2) +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_FOCUS (1 << 3) +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_FACE_DETECT (1 << 4) +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK (1 << 5) +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_IMAGE_STABILIZATION (1 << 6) +#define V4L2_UVC_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY (1 << 7) + struct uvc_menu_info { __u32 value; __u8 name[UVC_MENU_NAME_LEN]; diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h index 974fd254e573..6c91d6fa4708 100644 --- a/include/uapi/linux/v4l2-controls.h +++ b/include/uapi/linux/v4l2-controls.h @@ -215,6 +215,13 @@ enum v4l2_colorfx { */ #define V4L2_CID_USER_THP7312_BASE (V4L2_CID_USER_BASE + 0x11c0) +/* + * The base for the uvc driver controls. + * See linux/uvcvideo.h for the list of controls. + * We reserve 64 controls for this driver. + */ +#define V4L2_CID_USER_UVC_BASE (V4L2_CID_USER_BASE + 0x11e0) + /* MPEG-class control IDs */ /* The MPEG controls are applicable to all codec controls * and the 'MPEG' part of the define is historical */ @@ -1089,6 +1096,8 @@ enum v4l2_auto_focus_range { #define V4L2_CID_HDR_SENSOR_MODE (V4L2_CID_CAMERA_CLASS_BASE+36) +/* CAMERA-class private control IDs */ + /* FM Modulator class control IDs */ #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)