Re: [PATCH v2 10/12] media: uvcvideo: Add get_* functions to uvc_entity
From: Hans de Goede <hansg@kernel.org>
Date: 2025-09-08 11:31:47
Also in:
linux-acpi, linux-gpio, linux-media, linux-usb, lkml
Hi, On 16-Jul-25 12:32, Ricardo Ribalda wrote:
On Tue, 15 Jul 2025 at 21:35, Laurent Pinchart [off-list ref] wrote:
...
quoted hunk ↗ jump to hunk
quoted
As for the minimum and maximum, they are currently set to 0 if the corresponding operations are not supported. I wonder if we should set them to the current value instead for read-only controls (as in controls whose flags report support for GET_CUR only)..I am not sure that I like that approach IMO the code looks worse... but if you prefer that, we can go that waydiff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index ec472e111248..47224437018b 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c@@ -35,6 +35,8 @@ /* ------------------------------------------------------------------------ * Controls */ +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain, + struct uvc_control *ctrl); static const struct uvc_control_info uvc_ctrls[] = { {@@ -1272,6 +1274,13 @@ static int uvc_ctrl_populate_cache(structuvc_video_chain *chain, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF)); if (ret < 0) return ret; + } else if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR)) { + ret = __uvc_ctrl_load_cur(chain, ctrl); + if (!ret) { + memcpy(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF), + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), + ctrl->info.size); + } } if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) {
Interesting change. Note you also need to check for
(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) being true,
__uvc_ctrl_load_cur() will return a 0 filled buffer
and success if that is not set.
I wonder why not do something like this instead though:
if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) &&
(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) &&
__uvc_ctrl_load_cur(chain, ctrl) == 0) {
/* Read-only control, set def / min / max to cur */
memcpy(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF),
uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
ctrl->info.size);
memcpy(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN),
uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
ctrl->info.size);
memcpy(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX),
uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
ctrl->info.size);
}
IOW why bother to make the GET_DEF, etc. calls at all for a
read-only control (even if they are supported) ?
Generally speaking making less calls into the hw seems better?
Although maybe replace the:
if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) &&
(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) &&
part of the check with a flag in ctrl->info indicating to do
this and do this for specific controls like the new
rotation and orientation controls ?
...
quoted hunk ↗ jump to hunk
@@ -1541,11 +1573,8 @@ static int __uvc_queryctrl_boundaries(structuvc_video_chain *chain, return ret; } - if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) v4l2_ctrl->default_value = uvc_mapping_get_s32(mapping, UVC_GET_DEF, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF)); - else - v4l2_ctrl->default_value = 0; switch (mapping->v4l2_type) { case V4L2_CTRL_TYPE_MENU:@@ -1576,23 +1605,14 @@ static int __uvc_queryctrl_boundaries(structuvc_video_chain *chain, break; } - if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) - v4l2_ctrl->minimum = uvc_mapping_get_s32(mapping, UVC_GET_MIN, - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN)); - else - v4l2_ctrl->minimum = 0; + v4l2_ctrl->minimum = uvc_mapping_get_s32(mapping, UVC_GET_MIN, + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN)); - if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX) - v4l2_ctrl->maximum = uvc_mapping_get_s32(mapping, UVC_GET_MAX, - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX)); - else - v4l2_ctrl->maximum = 0; + v4l2_ctrl->maximum = uvc_mapping_get_s32(mapping, UVC_GET_MAX, + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX)); - if (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES) - v4l2_ctrl->step = uvc_mapping_get_s32(mapping, UVC_GET_RES, - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES)); - else - v4l2_ctrl->step = 0; + v4l2_ctrl->step = uvc_mapping_get_s32(mapping, UVC_GET_RES, + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES)); return 0; }
I agree with Laurent that thee changes are nice, but please split them into a separate patch. Regards, Hans