[v10,02/11] media: uvcvideo: add uvc_ctrl_get_boundary for getting default value

Introduce uvc_ctrl_get_boundary(), making it easier to extend to
support compound controls and V4L2_CTRL_WHICH_MIN/MAX_VAL in the
following patches.

For now, it simply returns EINVAL for compound controls and calls
__query_v4l2_ctrl() for non-compound controls.

Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: Yunke Cao <yunkec@google.com>
---
Changelog since v9:
- Make __uvc_ctrl_get_boundary_std() static.
Changelog since v8:
- No Change.
Changelog since v7:
- Rename uvc_ctrl_get_fixed() to uvc_ctrl_get_boundary().
- Move refactor (introduce  __uvc_ctrl_get_boundary_std()) in this patch
  instead of in the patch where we implement compound control.
- Fix locking. uvc_ctrl_get_boundary() now acquires ctrl_mutex.
  __uvc_ctrl_get_boundary_std() calls __uvc_query_v4l2_ctrl() while
  holding the mutex.
- Define a uvc_ctrl_mapping_is_compound() for readability.

 drivers/media/usb/uvc/uvc_ctrl.c | 49 ++++++++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvc_v4l2.c |  6 +---
 drivers/media/usb/uvc/uvcvideo.h |  2 ++
 3 files changed, 52 insertions(+), 5 deletions(-)

Hi Yunke

On 09/11/2022 06:06, Yunke Cao wrote:
> Introduce uvc_ctrl_get_boundary(), making it easier to extend to
> support compound controls and V4L2_CTRL_WHICH_MIN/MAX_VAL in the
> following patches.
>
> For now, it simply returns EINVAL for compound controls and calls
> __query_v4l2_ctrl() for non-compound controls.
>
> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> Signed-off-by: Yunke Cao <yunkec@google.com>
> ---
> Changelog since v9:
> - Make __uvc_ctrl_get_boundary_std() static.
> Changelog since v8:
> - No Change.
> Changelog since v7:
> - Rename uvc_ctrl_get_fixed() to uvc_ctrl_get_boundary().
> - Move refactor (introduce  __uvc_ctrl_get_boundary_std()) in this patch
>    instead of in the patch where we implement compound control.
> - Fix locking. uvc_ctrl_get_boundary() now acquires ctrl_mutex.
>    __uvc_ctrl_get_boundary_std() calls __uvc_query_v4l2_ctrl() while
>    holding the mutex.
> - Define a uvc_ctrl_mapping_is_compound() for readability.
>
>   drivers/media/usb/uvc/uvc_ctrl.c | 49 ++++++++++++++++++++++++++++++++
>   drivers/media/usb/uvc/uvc_v4l2.c |  6 +---
>   drivers/media/usb/uvc/uvcvideo.h |  2 ++
>   3 files changed, 52 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index c95a2229f4fa..dfb9d1daece6 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -837,6 +837,12 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping,
>   	}
>   }
>   
> +static bool
> +uvc_ctrl_mapping_is_compound(const struct uvc_control_mapping *mapping)
> +{
> +	return mapping->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES;
> +}
> +
>   /* ------------------------------------------------------------------------
>    * Terminal and unit management
>    */
> @@ -1743,6 +1749,49 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
>   	return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
>   }
>   
> +static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain,
> +				       struct uvc_control *ctrl,
> +				       struct uvc_control_mapping *mapping,
> +				       u32 v4l2_which,
> +				       struct v4l2_ext_control *xctrl)


Here you define a parameter for v4l2_which, but further down...

> +{
> +	struct v4l2_queryctrl qc = { .id = xctrl->id };
> +
> +	int ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, &qc);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	xctrl->value = qc.default_value;
> +	return 0;
> +}
> +
> +int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
> +			  struct v4l2_ext_control *xctrl)
> +{
> +	struct uvc_control *ctrl;
> +	struct uvc_control_mapping *mapping;
> +	int ret;
> +
> +	if (mutex_lock_interruptible(&chain->ctrl_mutex))
> +		return -ERESTARTSYS;
> +
> +	ctrl = uvc_find_control(chain, xctrl->id, &mapping);
> +	if (!ctrl) {
> +		ret = -EINVAL;
> +		goto done;
> +	}
> +
> +	if (uvc_ctrl_mapping_is_compound(mapping))
> +		ret = -EINVAL;
> +	else
> +		ret = __uvc_ctrl_get_boundary_std(chain, ctrl, mapping, xctrl);


...here, you're not passing it, which causes a compilation error. 
Otherwise I think the patch looks ok.

> +
> +done:
> +	mutex_unlock(&chain->ctrl_mutex);
> +	return ret;
> +}
> +
>   int uvc_ctrl_set(struct uvc_fh *handle,
>   	struct v4l2_ext_control *xctrl)
>   {
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index f4d4c33b6dfb..e807e348aa41 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1046,15 +1046,11 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
>   
>   	if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
>   		for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> -			struct v4l2_queryctrl qc = { .id = ctrl->id };
> -
> -			ret = uvc_query_v4l2_ctrl(chain, &qc);
> +			ret = uvc_ctrl_get_boundary(chain, ctrl);
>   			if (ret < 0) {
>   				ctrls->error_idx = i;
>   				return ret;
>   			}
> -
> -			ctrl->value = qc.default_value;
>   		}
>   
>   		return 0;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index df93db259312..b2ee3d59a4c8 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -759,6 +759,8 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
>   }
>   
>   int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
> +int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
> +			  struct v4l2_ext_control *xctrl);
>   int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl);
>   int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
>   			   bool read);