Hi,
On 16-Jul-25 12:32, Ricardo Ribalda wrote:
> On Tue, 15 Jul 2025 at 21:35, Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
...
>> 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 way
>
>
> diff --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(struct
> uvc_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 ?
...
> @@ -1541,11 +1573,8 @@ static int __uvc_queryctrl_boundaries(struct
> uvc_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(struct
> uvc_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
