Re: [PATCH v2 10/12] media: uvcvideo: Add get_* functions to uvc_entity

[Hans de Goede <hansg@kernel.org>]

Hi,

On 16-Jul-25 12:32, Ricardo Ribalda wrote:
> On Tue, 15 Jul 2025 at 21:35, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> 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