diff mbox series

[v10,05/11] media: uvcvideo: Add support for compound controls

Message ID 20221109060621.704531-6-yunkec@google.com
State Superseded
Headers show
Series media: Implement UVC v1.5 ROI | expand

Commit Message

Yunke Cao Nov. 9, 2022, 6:06 a.m. UTC 
Supports getting/setting current value.
Supports getting default value.
Handles V4L2_CTRL_FLAG_NEXT_COMPOUND.

Signed-off-by: Yunke Cao <yunkec@google.com>
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changelog since v9:
- Make __uvc_ctrl_set_compound() static.
Changelog since v8:
- No change.
Changelog since v7:
- Fixed comments styles, indentation and a few other style issues.
- Renamed uvc_g/set_array() to uvc_g/set_compound().
- Moved size check to __uvc_ctrl_add_mapping().
- After setting a new value, copy it back to user.
- In __uvc_ctrl_set_compound(), check size before allocating.

 drivers/media/usb/uvc/uvc_ctrl.c | 184 ++++++++++++++++++++++++++++---
 drivers/media/usb/uvc/uvcvideo.h |   4 +
 2 files changed, 170 insertions(+), 18 deletions(-)

Comments

Daniel Scally Dec. 15, 2022, 1:40 p.m. UTC | #1 
Hi Yunke

On 09/11/2022 06:06, Yunke Cao wrote:
> Supports getting/setting current value.
> Supports getting default value.
> Handles V4L2_CTRL_FLAG_NEXT_COMPOUND.
>
> Signed-off-by: Yunke Cao <yunkec@google.com>
> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> Changelog since v9:
> - Make __uvc_ctrl_set_compound() static.
> Changelog since v8:
> - No change.
> Changelog since v7:
> - Fixed comments styles, indentation and a few other style issues.
> - Renamed uvc_g/set_array() to uvc_g/set_compound().
> - Moved size check to __uvc_ctrl_add_mapping().
> - After setting a new value, copy it back to user.
> - In __uvc_ctrl_set_compound(), check size before allocating.
>
>   drivers/media/usb/uvc/uvc_ctrl.c | 184 ++++++++++++++++++++++++++++---
>   drivers/media/usb/uvc/uvcvideo.h |   4 +
>   2 files changed, 170 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 5c4aa4b82218..7d86aa695b34 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -837,6 +837,28 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping,
>   	}
>   }
>   
> +/*
> + * Extract the byte array specified by mapping->offset and mapping->data_size
> + * stored at 'data' to the output array 'data_out'.
> + */
> +static int uvc_get_compound(struct uvc_control_mapping *mapping, const u8 *data,
> +			    u8 *data_out)
> +{
> +	memcpy(data_out, data + mapping->offset / 8, mapping->data_size / 8);
> +	return 0;
> +}
> +
> +/*
> + * Copy the byte array 'data_in' to the destination specified by mapping->offset
> + * and mapping->data_size stored at 'data'.
> + */
> +static int uvc_set_compound(struct uvc_control_mapping *mapping,
> +			    const u8 *data_in, u8 *data)
> +{
> +	memcpy(data + mapping->offset / 8, data_in, mapping->data_size / 8);
> +	return 0;
> +}
> +
>   static bool
>   uvc_ctrl_mapping_is_compound(const struct uvc_control_mapping *mapping)
>   {
> @@ -859,7 +881,7 @@ static int uvc_entity_match_guid(const struct uvc_entity *entity,
>   
>   static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id,
>   	struct uvc_control_mapping **mapping, struct uvc_control **control,
> -	int next)
> +	int next, int next_compound)
>   {
>   	struct uvc_control *ctrl;
>   	struct uvc_control_mapping *map;
> @@ -874,14 +896,17 @@ static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id,
>   			continue;
>   
>   		list_for_each_entry(map, &ctrl->info.mappings, list) {
> -			if ((map->id == v4l2_id) && !next) {
> +			if (map->id == v4l2_id && !next && !next_compound) {
>   				*control = ctrl;
>   				*mapping = map;
>   				return;
>   			}
>   
>   			if ((*mapping == NULL || (*mapping)->id > map->id) &&
> -			    (map->id > v4l2_id) && next) {
> +			    (map->id > v4l2_id) &&
> +			    ((!uvc_ctrl_mapping_is_compound(map) && next) ||
> +			     (uvc_ctrl_mapping_is_compound(map) &&
> +			      next_compound))) {


I think I'd do (uvc_ctrl_mapping_is_compound(map) ? next_compound : next))

>   				*control = ctrl;
>   				*mapping = map;
>   			}
> @@ -895,6 +920,7 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain,
>   	struct uvc_control *ctrl = NULL;
>   	struct uvc_entity *entity;
>   	int next = v4l2_id & V4L2_CTRL_FLAG_NEXT_CTRL;
> +	int next_compound = v4l2_id & V4L2_CTRL_FLAG_NEXT_COMPOUND;
>   
>   	*mapping = NULL;
>   
> @@ -903,12 +929,13 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain,
>   
>   	/* Find the control. */
>   	list_for_each_entry(entity, &chain->entities, chain) {
> -		__uvc_find_control(entity, v4l2_id, mapping, &ctrl, next);
> -		if (ctrl && !next)
> +		__uvc_find_control(entity, v4l2_id, mapping, &ctrl, next,
> +				   next_compound);
> +		if (ctrl && !next && !next_compound)
>   			return ctrl;
>   	}
>   
> -	if (ctrl == NULL && !next)
> +	if (!ctrl && !next && !next_compound)
>   		uvc_dbg(chain->dev, CONTROL, "Control 0x%08x not found\n",
>   			v4l2_id);
>   


Actually, is next_compound even needed? Can we just set next = v4l2_id & 
(V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND)?

> @@ -1048,10 +1075,59 @@ static int __uvc_ctrl_get_std(struct uvc_video_chain *chain,
>   	return 0;
>   }
>   
> +static int __uvc_ctrl_get_compound(struct uvc_control_mapping *mapping,
> +				   struct uvc_control *ctrl,
> +				   int id,
> +				   struct v4l2_ext_control *xctrl)
> +{
> +	u8 size;
> +	u8 *data;
> +	int ret;
> +
> +	size = mapping->v4l2_size / 8;
> +	if (xctrl->size < size) {
> +		xctrl->size = size;
> +		return -ENOSPC;
> +	}
> +
> +	data = kmalloc(size, GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	ret = mapping->get_compound(mapping, uvc_ctrl_data(ctrl, id), data);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = copy_to_user(xctrl->ptr, data, size) ? -EFAULT : 0;
> +
> +out:
> +	kfree(data);
> +	return ret;
> +}
> +
> +static int __uvc_ctrl_get_compound_cur(struct uvc_video_chain *chain,
> +				       struct uvc_control *ctrl,
> +				       struct uvc_control_mapping *mapping,
> +				       struct v4l2_ext_control *xctrl)
> +{
> +	int ret;
> +
> +	if (!uvc_ctrl_mapping_is_compound(mapping))
> +		return -EINVAL;


This is already guarded against in uvc_ctrl_get()

> +
> +	ret = __uvc_ctrl_load_cur(chain, ctrl);
> +	if (ret < 0)
> +		return ret;
> +
> +	return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT,
> +				       xctrl);
> +}
> +
>   static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
>   				  u32 found_id)
>   {
>   	bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL;
> +	bool find_next_compound = req_id & V4L2_CTRL_FLAG_NEXT_COMPOUND;
>   	unsigned int i;
>   
>   	req_id &= V4L2_CTRL_ID_MASK;
> @@ -1059,7 +1135,7 @@ static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
>   	for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) {
>   		if (!(chain->ctrl_class_bitmap & BIT(i)))
>   			continue;
> -		if (!find_next) {
> +		if (!find_next && !find_next_compound) {


Same as above; can we make do with find_next = req_id 
&(V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND)?

>   			if (uvc_control_classes[i] == req_id)
>   				return i;
>   			continue;
> @@ -1151,7 +1227,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>   
>   	if (mapping->master_id)
>   		__uvc_find_control(ctrl->entity, mapping->master_id,
> -				   &master_map, &master_ctrl, 0);
> +				   &master_map, &master_ctrl, 0, 0);
>   	if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
>   		s32 val = 0;
>   		int ret;
> @@ -1167,6 +1243,15 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>   				v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
>   	}
>   
> +	if (v4l2_ctrl->type >= V4L2_CTRL_COMPOUND_TYPES) {
> +		v4l2_ctrl->flags |= V4L2_CTRL_FLAG_HAS_PAYLOAD;
> +		v4l2_ctrl->default_value = 0;
> +		v4l2_ctrl->minimum = 0;
> +		v4l2_ctrl->maximum = 0;
> +		v4l2_ctrl->step = 0;
> +		return 0;
> +	}
> +
>   	if (!ctrl->cached) {
>   		int ret = uvc_ctrl_populate_cache(chain, ctrl);
>   		if (ret < 0)
> @@ -1400,7 +1485,7 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
>   	u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
>   	s32 val = 0;
>   
> -	__uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0);
> +	__uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0, 0);
>   	if (ctrl == NULL)
>   		return;
>   
> @@ -1706,7 +1791,7 @@ static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity,
>   
>   	for (i = 0; i < ctrls->count; i++) {
>   		__uvc_find_control(entity, ctrls->controls[i].id, &mapping,
> -				   &ctrl_found, 0);
> +				   &ctrl_found, 0, 0);
>   		if (uvc_control == ctrl_found)
>   			return i;
>   	}
> @@ -1754,7 +1839,7 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
>   		return -EINVAL;
>   
>   	if (uvc_ctrl_mapping_is_compound(mapping))
> -		return -EINVAL;
> +		return __uvc_ctrl_get_compound_cur(chain, ctrl, mapping, xctrl);
>   	else
>   		return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value);
>   }
> @@ -1776,6 +1861,25 @@ static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain,
>   	return 0;
>   }
>   
> +static int __uvc_ctrl_get_boundary_compound(struct uvc_video_chain *chain,
> +					    struct uvc_control *ctrl,
> +					    struct uvc_control_mapping *mapping,
> +					    struct v4l2_ext_control *xctrl)
> +{
> +	int ret;
> +
> +	if (!uvc_ctrl_mapping_is_compound(mapping))
> +		return -EINVAL;


This is already guarded against in uvc_ctrl_get_boundary()

> +
> +	if (!ctrl->cached) {
> +		ret = uvc_ctrl_populate_cache(chain, ctrl);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_DEF, xctrl);
> +}
> +
>   int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
>   			  struct v4l2_ext_control *xctrl)
>   {
> @@ -1793,7 +1897,8 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
>   	}
>   
>   	if (uvc_ctrl_mapping_is_compound(mapping))
> -		ret = -EINVAL;
> +		ret = __uvc_ctrl_get_boundary_compound(chain, ctrl, mapping,
> +						       xctrl);
>   	else
>   		ret = __uvc_ctrl_get_boundary_std(chain, ctrl, mapping, xctrl);
>   
> @@ -1802,6 +1907,34 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
>   	return ret;
>   }
>   
> +static int __uvc_ctrl_set_compound(struct uvc_control_mapping *mapping,
> +				   struct v4l2_ext_control *xctrl,
> +				   struct uvc_control *ctrl)
> +{
> +	u8 *data;
> +	int ret;
> +
> +	if (xctrl->size != mapping->v4l2_size / 8)
> +		return -EINVAL;
> +
> +	data = kmalloc(xctrl->size, GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	ret = copy_from_user(data, xctrl->ptr, xctrl->size);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = mapping->set_compound(mapping, data,
> +			uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> +
> +	__uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT, xctrl);
> +
> +out:
> +	kfree(data);
> +	return ret;
> +}
> +
>   int uvc_ctrl_set(struct uvc_fh *handle,
>   	struct v4l2_ext_control *xctrl)
>   {
> @@ -1903,12 +2036,14 @@ int uvc_ctrl_set(struct uvc_fh *handle,
>   		       ctrl->info.size);
>   	}
>   
> -	if (!uvc_ctrl_mapping_is_compound(mapping))
> +	if (!uvc_ctrl_mapping_is_compound(mapping)) {
>   		mapping->set(mapping, value,
>   			     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> -	else
> -		return -EINVAL;
> -
> +	} else {
> +		ret = __uvc_ctrl_set_compound(mapping, xctrl, ctrl);
> +		if (ret < 0)
> +			return ret;
> +	}
>   
>   	if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
>   		ctrl->handle = handle;
> @@ -2308,10 +2443,23 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>   		return -ENOMEM;
>   	}
>   
> -	if (map->get == NULL)
> +	if (uvc_ctrl_mapping_is_compound(map)) {
> +		if (map->data_size != map->v4l2_size)
> +			return -EINVAL;
> +
> +		/* Only supports byte-aligned data. */
> +		if (WARN_ON(map->offset % 8 || map->data_size % 8))
> +			return -EINVAL;
> +	}
> +
> +	if (!map->get && !uvc_ctrl_mapping_is_compound(map))
>   		map->get = uvc_get_le_value;
> -	if (map->set == NULL)
> +	if (!map->set && !uvc_ctrl_mapping_is_compound(map))
>   		map->set = uvc_set_le_value;
> +	if (!map->get_compound && uvc_ctrl_mapping_is_compound(map))
> +		map->get_compound = uvc_get_compound;
> +	if (!map->set_compound && uvc_ctrl_mapping_is_compound(map))
> +		map->set_compound = uvc_set_compound;
>   
>   	for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) {
>   		if (V4L2_CTRL_ID2WHICH(uvc_control_classes[i]) ==
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 8f7938205a63..1e1bccd3b2e5 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -129,8 +129,12 @@ struct uvc_control_mapping {
>   
>   	s32 (*get)(struct uvc_control_mapping *mapping, u8 query,
>   		   const u8 *data);
> +	int (*get_compound)(struct uvc_control_mapping *mapping, const u8 *data,
> +			    u8 *data_out);
>   	void (*set)(struct uvc_control_mapping *mapping, s32 value,
>   		    u8 *data);
> +	int (*set_compound)(struct uvc_control_mapping *mapping, const u8 *data_in,
> +			    u8 *data);
>   };
>   
>   struct uvc_control {
Yunke Cao Dec. 16, 2022, 1:23 a.m. UTC | #2 
Hi Dan,

Thank you for the review!

On Thu, Dec 15, 2022 at 10:40 PM Dan Scally <dan.scally@ideasonboard.com> wrote:
>
> Hi Yunke
>
> On 09/11/2022 06:06, Yunke Cao wrote:
> > Supports getting/setting current value.
> > Supports getting default value.
> > Handles V4L2_CTRL_FLAG_NEXT_COMPOUND.
> >
> > Signed-off-by: Yunke Cao <yunkec@google.com>
> > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > Changelog since v9:
> > - Make __uvc_ctrl_set_compound() static.
> > Changelog since v8:
> > - No change.
> > Changelog since v7:
> > - Fixed comments styles, indentation and a few other style issues.
> > - Renamed uvc_g/set_array() to uvc_g/set_compound().
> > - Moved size check to __uvc_ctrl_add_mapping().
> > - After setting a new value, copy it back to user.
> > - In __uvc_ctrl_set_compound(), check size before allocating.
> >
> >   drivers/media/usb/uvc/uvc_ctrl.c | 184 ++++++++++++++++++++++++++++---
> >   drivers/media/usb/uvc/uvcvideo.h |   4 +
> >   2 files changed, 170 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 5c4aa4b82218..7d86aa695b34 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -837,6 +837,28 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping,
> >       }
> >   }
> >
> > +/*
> > + * Extract the byte array specified by mapping->offset and mapping->data_size
> > + * stored at 'data' to the output array 'data_out'.
> > + */
> > +static int uvc_get_compound(struct uvc_control_mapping *mapping, const u8 *data,
> > +                         u8 *data_out)
> > +{
> > +     memcpy(data_out, data + mapping->offset / 8, mapping->data_size / 8);
> > +     return 0;
> > +}
> > +
> > +/*
> > + * Copy the byte array 'data_in' to the destination specified by mapping->offset
> > + * and mapping->data_size stored at 'data'.
> > + */
> > +static int uvc_set_compound(struct uvc_control_mapping *mapping,
> > +                         const u8 *data_in, u8 *data)
> > +{
> > +     memcpy(data + mapping->offset / 8, data_in, mapping->data_size / 8);
> > +     return 0;
> > +}
> > +
> >   static bool
> >   uvc_ctrl_mapping_is_compound(const struct uvc_control_mapping *mapping)
> >   {
> > @@ -859,7 +881,7 @@ static int uvc_entity_match_guid(const struct uvc_entity *entity,
> >
> >   static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id,
> >       struct uvc_control_mapping **mapping, struct uvc_control **control,
> > -     int next)
> > +     int next, int next_compound)
> >   {
> >       struct uvc_control *ctrl;
> >       struct uvc_control_mapping *map;
> > @@ -874,14 +896,17 @@ static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id,
> >                       continue;
> >
> >               list_for_each_entry(map, &ctrl->info.mappings, list) {
> > -                     if ((map->id == v4l2_id) && !next) {
> > +                     if (map->id == v4l2_id && !next && !next_compound) {
> >                               *control = ctrl;
> >                               *mapping = map;
> >                               return;
> >                       }
> >
> >                       if ((*mapping == NULL || (*mapping)->id > map->id) &&
> > -                         (map->id > v4l2_id) && next) {
> > +                         (map->id > v4l2_id) &&
> > +                         ((!uvc_ctrl_mapping_is_compound(map) && next) ||
> > +                          (uvc_ctrl_mapping_is_compound(map) &&
> > +                           next_compound))) {
>
>
> I think I'd do (uvc_ctrl_mapping_is_compound(map) ? next_compound : next))
>

Sounds good.

> >                               *control = ctrl;
> >                               *mapping = map;
> >                       }
> > @@ -895,6 +920,7 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain,
> >       struct uvc_control *ctrl = NULL;
> >       struct uvc_entity *entity;
> >       int next = v4l2_id & V4L2_CTRL_FLAG_NEXT_CTRL;
> > +     int next_compound = v4l2_id & V4L2_CTRL_FLAG_NEXT_COMPOUND;
> >
> >       *mapping = NULL;
> >
> > @@ -903,12 +929,13 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain,
> >
> >       /* Find the control. */
> >       list_for_each_entry(entity, &chain->entities, chain) {
> > -             __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next);
> > -             if (ctrl && !next)
> > +             __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next,
> > +                                next_compound);
> > +             if (ctrl && !next && !next_compound)
> >                       return ctrl;
> >       }
> >
> > -     if (ctrl == NULL && !next)
> > +     if (!ctrl && !next && !next_compound)
> >               uvc_dbg(chain->dev, CONTROL, "Control 0x%08x not found\n",
> >                       v4l2_id);
> >
>
>
> Actually, is next_compound even needed? Can we just set next = v4l2_id &
> (V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND)?
>

I think it is needed. next and next_compound are passed to uvc_find_control().
In __uvc_find_control(), we iterate all controls if next && next_compound
only non-compound controls if next && !next_compound,
only compound controls if !next && next_compound.
This matches the behavior documented in
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-queryctrl.html#description.

Simply setting next = v4l2_id & (V4L2_CTRL_FLAG_NEXT_CTRL |
V4L2_CTRL_FLAG_NEXT_COMPOUND)
changes the behavior to always iterate all controls, which is wrong.
V4L2-compliance fails, too.


> > @@ -1048,10 +1075,59 @@ static int __uvc_ctrl_get_std(struct uvc_video_chain *chain,
> >       return 0;
> >   }
> >
> > +static int __uvc_ctrl_get_compound(struct uvc_control_mapping *mapping,
> > +                                struct uvc_control *ctrl,
> > +                                int id,
> > +                                struct v4l2_ext_control *xctrl)
> > +{
> > +     u8 size;
> > +     u8 *data;
> > +     int ret;
> > +
> > +     size = mapping->v4l2_size / 8;
> > +     if (xctrl->size < size) {
> > +             xctrl->size = size;
> > +             return -ENOSPC;
> > +     }
> > +
> > +     data = kmalloc(size, GFP_KERNEL);
> > +     if (!data)
> > +             return -ENOMEM;
> > +
> > +     ret = mapping->get_compound(mapping, uvc_ctrl_data(ctrl, id), data);
> > +     if (ret < 0)
> > +             goto out;
> > +
> > +     ret = copy_to_user(xctrl->ptr, data, size) ? -EFAULT : 0;
> > +
> > +out:
> > +     kfree(data);
> > +     return ret;
> > +}
> > +
> > +static int __uvc_ctrl_get_compound_cur(struct uvc_video_chain *chain,
> > +                                    struct uvc_control *ctrl,
> > +                                    struct uvc_control_mapping *mapping,
> > +                                    struct v4l2_ext_control *xctrl)
> > +{
> > +     int ret;
> > +
> > +     if (!uvc_ctrl_mapping_is_compound(mapping))
> > +             return -EINVAL;
>
>
> This is already guarded against in uvc_ctrl_get()
>

Thanks, I will remove it.

> > +
> > +     ret = __uvc_ctrl_load_cur(chain, ctrl);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT,
> > +                                    xctrl);
> > +}
> > +
> >   static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
> >                                 u32 found_id)
> >   {
> >       bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL;
> > +     bool find_next_compound = req_id & V4L2_CTRL_FLAG_NEXT_COMPOUND;
> >       unsigned int i;
> >
> >       req_id &= V4L2_CTRL_ID_MASK;
> > @@ -1059,7 +1135,7 @@ static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
> >       for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) {
> >               if (!(chain->ctrl_class_bitmap & BIT(i)))
> >                       continue;
> > -             if (!find_next) {
> > +             if (!find_next && !find_next_compound) {
>
>
> Same as above; can we make do with find_next = req_id
> &(V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND)?
>

Yes, we can do it here. Will update in the next version.

> >                       if (uvc_control_classes[i] == req_id)
> >                               return i;
> >                       continue;
> > @@ -1151,7 +1227,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> >
> >       if (mapping->master_id)
> >               __uvc_find_control(ctrl->entity, mapping->master_id,
> > -                                &master_map, &master_ctrl, 0);
> > +                                &master_map, &master_ctrl, 0, 0);
> >       if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
> >               s32 val = 0;
> >               int ret;
> > @@ -1167,6 +1243,15 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> >                               v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> >       }
> >
> > +     if (v4l2_ctrl->type >= V4L2_CTRL_COMPOUND_TYPES) {
> > +             v4l2_ctrl->flags |= V4L2_CTRL_FLAG_HAS_PAYLOAD;
> > +             v4l2_ctrl->default_value = 0;
> > +             v4l2_ctrl->minimum = 0;
> > +             v4l2_ctrl->maximum = 0;
> > +             v4l2_ctrl->step = 0;
> > +             return 0;
> > +     }
> > +
> >       if (!ctrl->cached) {
> >               int ret = uvc_ctrl_populate_cache(chain, ctrl);
> >               if (ret < 0)
> > @@ -1400,7 +1485,7 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
> >       u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
> >       s32 val = 0;
> >
> > -     __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0);
> > +     __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0, 0);
> >       if (ctrl == NULL)
> >               return;
> >
> > @@ -1706,7 +1791,7 @@ static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity,
> >
> >       for (i = 0; i < ctrls->count; i++) {
> >               __uvc_find_control(entity, ctrls->controls[i].id, &mapping,
> > -                                &ctrl_found, 0);
> > +                                &ctrl_found, 0, 0);
> >               if (uvc_control == ctrl_found)
> >                       return i;
> >       }
> > @@ -1754,7 +1839,7 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
> >               return -EINVAL;
> >
> >       if (uvc_ctrl_mapping_is_compound(mapping))
> > -             return -EINVAL;
> > +             return __uvc_ctrl_get_compound_cur(chain, ctrl, mapping, xctrl);
> >       else
> >               return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value);
> >   }
> > @@ -1776,6 +1861,25 @@ static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain,
> >       return 0;
> >   }
> >
> > +static int __uvc_ctrl_get_boundary_compound(struct uvc_video_chain *chain,
> > +                                         struct uvc_control *ctrl,
> > +                                         struct uvc_control_mapping *mapping,
> > +                                         struct v4l2_ext_control *xctrl)
> > +{
> > +     int ret;
> > +
> > +     if (!uvc_ctrl_mapping_is_compound(mapping))
> > +             return -EINVAL;
>
>
> This is already guarded against in uvc_ctrl_get_boundary()

Thanks. Will remove this.

>
> > +
> > +     if (!ctrl->cached) {
> > +             ret = uvc_ctrl_populate_cache(chain, ctrl);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> > +     return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_DEF, xctrl);
> > +}
> > +
> >   int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
> >                         struct v4l2_ext_control *xctrl)
> >   {
> > @@ -1793,7 +1897,8 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
> >       }
> >
> >       if (uvc_ctrl_mapping_is_compound(mapping))
> > -             ret = -EINVAL;
> > +             ret = __uvc_ctrl_get_boundary_compound(chain, ctrl, mapping,
> > +                                                    xctrl);
> >       else
> >               ret = __uvc_ctrl_get_boundary_std(chain, ctrl, mapping, xctrl);
> >
> > @@ -1802,6 +1907,34 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
> >       return ret;
> >   }
> >
> > +static int __uvc_ctrl_set_compound(struct uvc_control_mapping *mapping,
> > +                                struct v4l2_ext_control *xctrl,
> > +                                struct uvc_control *ctrl)
> > +{
> > +     u8 *data;
> > +     int ret;
> > +
> > +     if (xctrl->size != mapping->v4l2_size / 8)
> > +             return -EINVAL;
> > +
> > +     data = kmalloc(xctrl->size, GFP_KERNEL);
> > +     if (!data)
> > +             return -ENOMEM;
> > +
> > +     ret = copy_from_user(data, xctrl->ptr, xctrl->size);
> > +     if (ret < 0)
> > +             goto out;
> > +
> > +     ret = mapping->set_compound(mapping, data,
> > +                     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> > +
> > +     __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT, xctrl);
> > +
> > +out:
> > +     kfree(data);
> > +     return ret;
> > +}
> > +
> >   int uvc_ctrl_set(struct uvc_fh *handle,
> >       struct v4l2_ext_control *xctrl)
> >   {
> > @@ -1903,12 +2036,14 @@ int uvc_ctrl_set(struct uvc_fh *handle,
> >                      ctrl->info.size);
> >       }
> >
> > -     if (!uvc_ctrl_mapping_is_compound(mapping))
> > +     if (!uvc_ctrl_mapping_is_compound(mapping)) {
> >               mapping->set(mapping, value,
> >                            uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> > -     else
> > -             return -EINVAL;
> > -
> > +     } else {
> > +             ret = __uvc_ctrl_set_compound(mapping, xctrl, ctrl);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> >
> >       if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> >               ctrl->handle = handle;
> > @@ -2308,10 +2443,23 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> >               return -ENOMEM;
> >       }
> >
> > -     if (map->get == NULL)
> > +     if (uvc_ctrl_mapping_is_compound(map)) {
> > +             if (map->data_size != map->v4l2_size)
> > +                     return -EINVAL;
> > +
> > +             /* Only supports byte-aligned data. */
> > +             if (WARN_ON(map->offset % 8 || map->data_size % 8))
> > +                     return -EINVAL;
> > +     }
> > +
> > +     if (!map->get && !uvc_ctrl_mapping_is_compound(map))
> >               map->get = uvc_get_le_value;
> > -     if (map->set == NULL)
> > +     if (!map->set && !uvc_ctrl_mapping_is_compound(map))
> >               map->set = uvc_set_le_value;
> > +     if (!map->get_compound && uvc_ctrl_mapping_is_compound(map))
> > +             map->get_compound = uvc_get_compound;
> > +     if (!map->set_compound && uvc_ctrl_mapping_is_compound(map))
> > +             map->set_compound = uvc_set_compound;
> >
> >       for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) {
> >               if (V4L2_CTRL_ID2WHICH(uvc_control_classes[i]) ==
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 8f7938205a63..1e1bccd3b2e5 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -129,8 +129,12 @@ struct uvc_control_mapping {
> >
> >       s32 (*get)(struct uvc_control_mapping *mapping, u8 query,
> >                  const u8 *data);
> > +     int (*get_compound)(struct uvc_control_mapping *mapping, const u8 *data,
> > +                         u8 *data_out);
> >       void (*set)(struct uvc_control_mapping *mapping, s32 value,
> >                   u8 *data);
> > +     int (*set_compound)(struct uvc_control_mapping *mapping, const u8 *data_in,
> > +                         u8 *data);
> >   };
> >
> >   struct uvc_control {

Best,
Yunke
Daniel Scally Dec. 16, 2022, 11:14 a.m. UTC | #3 
Hi Yunke

On 16/12/2022 01:23, Yunke Cao wrote:
> Hi Dan,
>
> Thank you for the review!
>
> On Thu, Dec 15, 2022 at 10:40 PM Dan Scally <dan.scally@ideasonboard.com> wrote:
>> Hi Yunke
>>
>> On 09/11/2022 06:06, Yunke Cao wrote:
>>> Supports getting/setting current value.
>>> Supports getting default value.
>>> Handles V4L2_CTRL_FLAG_NEXT_COMPOUND.
>>>
>>> Signed-off-by: Yunke Cao <yunkec@google.com>
>>> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>> Changelog since v9:
>>> - Make __uvc_ctrl_set_compound() static.
>>> Changelog since v8:
>>> - No change.
>>> Changelog since v7:
>>> - Fixed comments styles, indentation and a few other style issues.
>>> - Renamed uvc_g/set_array() to uvc_g/set_compound().
>>> - Moved size check to __uvc_ctrl_add_mapping().
>>> - After setting a new value, copy it back to user.
>>> - In __uvc_ctrl_set_compound(), check size before allocating.
>>>
>>>    drivers/media/usb/uvc/uvc_ctrl.c | 184 ++++++++++++++++++++++++++++---
>>>    drivers/media/usb/uvc/uvcvideo.h |   4 +
>>>    2 files changed, 170 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
>>> index 5c4aa4b82218..7d86aa695b34 100644
>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
>>> @@ -837,6 +837,28 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping,
>>>        }
>>>    }
>>>
>>> +/*
>>> + * Extract the byte array specified by mapping->offset and mapping->data_size
>>> + * stored at 'data' to the output array 'data_out'.
>>> + */
>>> +static int uvc_get_compound(struct uvc_control_mapping *mapping, const u8 *data,
>>> +                         u8 *data_out)
>>> +{
>>> +     memcpy(data_out, data + mapping->offset / 8, mapping->data_size / 8);
>>> +     return 0;
>>> +}
>>> +
>>> +/*
>>> + * Copy the byte array 'data_in' to the destination specified by mapping->offset
>>> + * and mapping->data_size stored at 'data'.
>>> + */
>>> +static int uvc_set_compound(struct uvc_control_mapping *mapping,
>>> +                         const u8 *data_in, u8 *data)
>>> +{
>>> +     memcpy(data + mapping->offset / 8, data_in, mapping->data_size / 8);
>>> +     return 0;
>>> +}
>>> +
>>>    static bool
>>>    uvc_ctrl_mapping_is_compound(const struct uvc_control_mapping *mapping)
>>>    {
>>> @@ -859,7 +881,7 @@ static int uvc_entity_match_guid(const struct uvc_entity *entity,
>>>
>>>    static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id,
>>>        struct uvc_control_mapping **mapping, struct uvc_control **control,
>>> -     int next)
>>> +     int next, int next_compound)
>>>    {
>>>        struct uvc_control *ctrl;
>>>        struct uvc_control_mapping *map;
>>> @@ -874,14 +896,17 @@ static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id,
>>>                        continue;
>>>
>>>                list_for_each_entry(map, &ctrl->info.mappings, list) {
>>> -                     if ((map->id == v4l2_id) && !next) {
>>> +                     if (map->id == v4l2_id && !next && !next_compound) {
>>>                                *control = ctrl;
>>>                                *mapping = map;
>>>                                return;
>>>                        }
>>>
>>>                        if ((*mapping == NULL || (*mapping)->id > map->id) &&
>>> -                         (map->id > v4l2_id) && next) {
>>> +                         (map->id > v4l2_id) &&
>>> +                         ((!uvc_ctrl_mapping_is_compound(map) && next) ||
>>> +                          (uvc_ctrl_mapping_is_compound(map) &&
>>> +                           next_compound))) {
>>
>> I think I'd do (uvc_ctrl_mapping_is_compound(map) ? next_compound : next))
>>
> Sounds good.
>
>>>                                *control = ctrl;
>>>                                *mapping = map;
>>>                        }
>>> @@ -895,6 +920,7 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain,
>>>        struct uvc_control *ctrl = NULL;
>>>        struct uvc_entity *entity;
>>>        int next = v4l2_id & V4L2_CTRL_FLAG_NEXT_CTRL;
>>> +     int next_compound = v4l2_id & V4L2_CTRL_FLAG_NEXT_COMPOUND;
>>>
>>>        *mapping = NULL;
>>>
>>> @@ -903,12 +929,13 @@ static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain,
>>>
>>>        /* Find the control. */
>>>        list_for_each_entry(entity, &chain->entities, chain) {
>>> -             __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next);
>>> -             if (ctrl && !next)
>>> +             __uvc_find_control(entity, v4l2_id, mapping, &ctrl, next,
>>> +                                next_compound);
>>> +             if (ctrl && !next && !next_compound)
>>>                        return ctrl;
>>>        }
>>>
>>> -     if (ctrl == NULL && !next)
>>> +     if (!ctrl && !next && !next_compound)
>>>                uvc_dbg(chain->dev, CONTROL, "Control 0x%08x not found\n",
>>>                        v4l2_id);
>>>
>>
>> Actually, is next_compound even needed? Can we just set next = v4l2_id &
>> (V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND)?
>>
> I think it is needed. next and next_compound are passed to uvc_find_control().
> In __uvc_find_control(), we iterate all controls if next && next_compound
> only non-compound controls if next && !next_compound,
> only compound controls if !next && next_compound.
> This matches the behavior documented in
> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-queryctrl.html#description.
>
> Simply setting next = v4l2_id & (V4L2_CTRL_FLAG_NEXT_CTRL |
> V4L2_CTRL_FLAG_NEXT_COMPOUND)
> changes the behavior to always iterate all controls, which is wrong.
> V4L2-compliance fails, too.


Ah - yes I see. Thanks for the explanation

>
>>> @@ -1048,10 +1075,59 @@ static int __uvc_ctrl_get_std(struct uvc_video_chain *chain,
>>>        return 0;
>>>    }
>>>
>>> +static int __uvc_ctrl_get_compound(struct uvc_control_mapping *mapping,
>>> +                                struct uvc_control *ctrl,
>>> +                                int id,
>>> +                                struct v4l2_ext_control *xctrl)
>>> +{
>>> +     u8 size;
>>> +     u8 *data;
>>> +     int ret;
>>> +
>>> +     size = mapping->v4l2_size / 8;
>>> +     if (xctrl->size < size) {
>>> +             xctrl->size = size;
>>> +             return -ENOSPC;
>>> +     }
>>> +
>>> +     data = kmalloc(size, GFP_KERNEL);
>>> +     if (!data)
>>> +             return -ENOMEM;
>>> +
>>> +     ret = mapping->get_compound(mapping, uvc_ctrl_data(ctrl, id), data);
>>> +     if (ret < 0)
>>> +             goto out;
>>> +
>>> +     ret = copy_to_user(xctrl->ptr, data, size) ? -EFAULT : 0;
>>> +
>>> +out:
>>> +     kfree(data);
>>> +     return ret;
>>> +}
>>> +
>>> +static int __uvc_ctrl_get_compound_cur(struct uvc_video_chain *chain,
>>> +                                    struct uvc_control *ctrl,
>>> +                                    struct uvc_control_mapping *mapping,
>>> +                                    struct v4l2_ext_control *xctrl)
>>> +{
>>> +     int ret;
>>> +
>>> +     if (!uvc_ctrl_mapping_is_compound(mapping))
>>> +             return -EINVAL;
>>
>> This is already guarded against in uvc_ctrl_get()
>>
> Thanks, I will remove it.
>
>>> +
>>> +     ret = __uvc_ctrl_load_cur(chain, ctrl);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT,
>>> +                                    xctrl);
>>> +}
>>> +
>>>    static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
>>>                                  u32 found_id)
>>>    {
>>>        bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL;
>>> +     bool find_next_compound = req_id & V4L2_CTRL_FLAG_NEXT_COMPOUND;
>>>        unsigned int i;
>>>
>>>        req_id &= V4L2_CTRL_ID_MASK;
>>> @@ -1059,7 +1135,7 @@ static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
>>>        for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) {
>>>                if (!(chain->ctrl_class_bitmap & BIT(i)))
>>>                        continue;
>>> -             if (!find_next) {
>>> +             if (!find_next && !find_next_compound) {
>>
>> Same as above; can we make do with find_next = req_id
>> &(V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND)?
>>
> Yes, we can do it here. Will update in the next version.
>
>>>                        if (uvc_control_classes[i] == req_id)
>>>                                return i;
>>>                        continue;
>>> @@ -1151,7 +1227,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>>>
>>>        if (mapping->master_id)
>>>                __uvc_find_control(ctrl->entity, mapping->master_id,
>>> -                                &master_map, &master_ctrl, 0);
>>> +                                &master_map, &master_ctrl, 0, 0);
>>>        if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
>>>                s32 val = 0;
>>>                int ret;
>>> @@ -1167,6 +1243,15 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>>>                                v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
>>>        }
>>>
>>> +     if (v4l2_ctrl->type >= V4L2_CTRL_COMPOUND_TYPES) {
>>> +             v4l2_ctrl->flags |= V4L2_CTRL_FLAG_HAS_PAYLOAD;
>>> +             v4l2_ctrl->default_value = 0;
>>> +             v4l2_ctrl->minimum = 0;
>>> +             v4l2_ctrl->maximum = 0;
>>> +             v4l2_ctrl->step = 0;
>>> +             return 0;
>>> +     }
>>> +
>>>        if (!ctrl->cached) {
>>>                int ret = uvc_ctrl_populate_cache(chain, ctrl);
>>>                if (ret < 0)
>>> @@ -1400,7 +1485,7 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
>>>        u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
>>>        s32 val = 0;
>>>
>>> -     __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0);
>>> +     __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0, 0);
>>>        if (ctrl == NULL)
>>>                return;
>>>
>>> @@ -1706,7 +1791,7 @@ static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity,
>>>
>>>        for (i = 0; i < ctrls->count; i++) {
>>>                __uvc_find_control(entity, ctrls->controls[i].id, &mapping,
>>> -                                &ctrl_found, 0);
>>> +                                &ctrl_found, 0, 0);
>>>                if (uvc_control == ctrl_found)
>>>                        return i;
>>>        }
>>> @@ -1754,7 +1839,7 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
>>>                return -EINVAL;
>>>
>>>        if (uvc_ctrl_mapping_is_compound(mapping))
>>> -             return -EINVAL;
>>> +             return __uvc_ctrl_get_compound_cur(chain, ctrl, mapping, xctrl);
>>>        else
>>>                return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value);
>>>    }
>>> @@ -1776,6 +1861,25 @@ static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain,
>>>        return 0;
>>>    }
>>>
>>> +static int __uvc_ctrl_get_boundary_compound(struct uvc_video_chain *chain,
>>> +                                         struct uvc_control *ctrl,
>>> +                                         struct uvc_control_mapping *mapping,
>>> +                                         struct v4l2_ext_control *xctrl)
>>> +{
>>> +     int ret;
>>> +
>>> +     if (!uvc_ctrl_mapping_is_compound(mapping))
>>> +             return -EINVAL;
>>
>> This is already guarded against in uvc_ctrl_get_boundary()
> Thanks. Will remove this.
>
>>> +
>>> +     if (!ctrl->cached) {
>>> +             ret = uvc_ctrl_populate_cache(chain, ctrl);
>>> +             if (ret < 0)
>>> +                     return ret;
>>> +     }
>>> +
>>> +     return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_DEF, xctrl);
>>> +}
>>> +
>>>    int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
>>>                          struct v4l2_ext_control *xctrl)
>>>    {
>>> @@ -1793,7 +1897,8 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
>>>        }
>>>
>>>        if (uvc_ctrl_mapping_is_compound(mapping))
>>> -             ret = -EINVAL;
>>> +             ret = __uvc_ctrl_get_boundary_compound(chain, ctrl, mapping,
>>> +                                                    xctrl);
>>>        else
>>>                ret = __uvc_ctrl_get_boundary_std(chain, ctrl, mapping, xctrl);
>>>
>>> @@ -1802,6 +1907,34 @@ int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
>>>        return ret;
>>>    }
>>>
>>> +static int __uvc_ctrl_set_compound(struct uvc_control_mapping *mapping,
>>> +                                struct v4l2_ext_control *xctrl,
>>> +                                struct uvc_control *ctrl)
>>> +{
>>> +     u8 *data;
>>> +     int ret;
>>> +
>>> +     if (xctrl->size != mapping->v4l2_size / 8)
>>> +             return -EINVAL;
>>> +
>>> +     data = kmalloc(xctrl->size, GFP_KERNEL);
>>> +     if (!data)
>>> +             return -ENOMEM;
>>> +
>>> +     ret = copy_from_user(data, xctrl->ptr, xctrl->size);
>>> +     if (ret < 0)
>>> +             goto out;
>>> +
>>> +     ret = mapping->set_compound(mapping, data,
>>> +                     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
>>> +
>>> +     __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT, xctrl);
>>> +
>>> +out:
>>> +     kfree(data);
>>> +     return ret;
>>> +}
>>> +
>>>    int uvc_ctrl_set(struct uvc_fh *handle,
>>>        struct v4l2_ext_control *xctrl)
>>>    {
>>> @@ -1903,12 +2036,14 @@ int uvc_ctrl_set(struct uvc_fh *handle,
>>>                       ctrl->info.size);
>>>        }
>>>
>>> -     if (!uvc_ctrl_mapping_is_compound(mapping))
>>> +     if (!uvc_ctrl_mapping_is_compound(mapping)) {
>>>                mapping->set(mapping, value,
>>>                             uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
>>> -     else
>>> -             return -EINVAL;
>>> -
>>> +     } else {
>>> +             ret = __uvc_ctrl_set_compound(mapping, xctrl, ctrl);
>>> +             if (ret < 0)
>>> +                     return ret;
>>> +     }
>>>
>>>        if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
>>>                ctrl->handle = handle;
>>> @@ -2308,10 +2443,23 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>>>                return -ENOMEM;
>>>        }
>>>
>>> -     if (map->get == NULL)
>>> +     if (uvc_ctrl_mapping_is_compound(map)) {
>>> +             if (map->data_size != map->v4l2_size)
>>> +                     return -EINVAL;
>>> +
>>> +             /* Only supports byte-aligned data. */
>>> +             if (WARN_ON(map->offset % 8 || map->data_size % 8))
>>> +                     return -EINVAL;
>>> +     }
>>> +
>>> +     if (!map->get && !uvc_ctrl_mapping_is_compound(map))
>>>                map->get = uvc_get_le_value;
>>> -     if (map->set == NULL)
>>> +     if (!map->set && !uvc_ctrl_mapping_is_compound(map))
>>>                map->set = uvc_set_le_value;
>>> +     if (!map->get_compound && uvc_ctrl_mapping_is_compound(map))
>>> +             map->get_compound = uvc_get_compound;
>>> +     if (!map->set_compound && uvc_ctrl_mapping_is_compound(map))
>>> +             map->set_compound = uvc_set_compound;
>>>
>>>        for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) {
>>>                if (V4L2_CTRL_ID2WHICH(uvc_control_classes[i]) ==
>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>> index 8f7938205a63..1e1bccd3b2e5 100644
>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>> @@ -129,8 +129,12 @@ struct uvc_control_mapping {
>>>
>>>        s32 (*get)(struct uvc_control_mapping *mapping, u8 query,
>>>                   const u8 *data);
>>> +     int (*get_compound)(struct uvc_control_mapping *mapping, const u8 *data,
>>> +                         u8 *data_out);
>>>        void (*set)(struct uvc_control_mapping *mapping, s32 value,
>>>                    u8 *data);
>>> +     int (*set_compound)(struct uvc_control_mapping *mapping, const u8 *data_in,
>>> +                         u8 *data);
>>>    };
>>>
>>>    struct uvc_control {
> Best,
> Yunke
diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 5c4aa4b82218..7d86aa695b34 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -837,6 +837,28 @@  static void uvc_set_le_value(struct uvc_control_mapping *mapping,
 	}
 }
 
+/*
+ * Extract the byte array specified by mapping->offset and mapping->data_size
+ * stored at 'data' to the output array 'data_out'.
+ */
+static int uvc_get_compound(struct uvc_control_mapping *mapping, const u8 *data,
+			    u8 *data_out)
+{
+	memcpy(data_out, data + mapping->offset / 8, mapping->data_size / 8);
+	return 0;
+}
+
+/*
+ * Copy the byte array 'data_in' to the destination specified by mapping->offset
+ * and mapping->data_size stored at 'data'.
+ */
+static int uvc_set_compound(struct uvc_control_mapping *mapping,
+			    const u8 *data_in, u8 *data)
+{
+	memcpy(data + mapping->offset / 8, data_in, mapping->data_size / 8);
+	return 0;
+}
+
 static bool
 uvc_ctrl_mapping_is_compound(const struct uvc_control_mapping *mapping)
 {
@@ -859,7 +881,7 @@  static int uvc_entity_match_guid(const struct uvc_entity *entity,
 
 static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id,
 	struct uvc_control_mapping **mapping, struct uvc_control **control,
-	int next)
+	int next, int next_compound)
 {
 	struct uvc_control *ctrl;
 	struct uvc_control_mapping *map;
@@ -874,14 +896,17 @@  static void __uvc_find_control(struct uvc_entity *entity, u32 v4l2_id,
 			continue;
 
 		list_for_each_entry(map, &ctrl->info.mappings, list) {
-			if ((map->id == v4l2_id) && !next) {
+			if (map->id == v4l2_id && !next && !next_compound) {
 				*control = ctrl;
 				*mapping = map;
 				return;
 			}
 
 			if ((*mapping == NULL || (*mapping)->id > map->id) &&
-			    (map->id > v4l2_id) && next) {
+			    (map->id > v4l2_id) &&
+			    ((!uvc_ctrl_mapping_is_compound(map) && next) ||
+			     (uvc_ctrl_mapping_is_compound(map) &&
+			      next_compound))) {
 				*control = ctrl;
 				*mapping = map;
 			}
@@ -895,6 +920,7 @@  static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain,
 	struct uvc_control *ctrl = NULL;
 	struct uvc_entity *entity;
 	int next = v4l2_id & V4L2_CTRL_FLAG_NEXT_CTRL;
+	int next_compound = v4l2_id & V4L2_CTRL_FLAG_NEXT_COMPOUND;
 
 	*mapping = NULL;
 
@@ -903,12 +929,13 @@  static struct uvc_control *uvc_find_control(struct uvc_video_chain *chain,
 
 	/* Find the control. */
 	list_for_each_entry(entity, &chain->entities, chain) {
-		__uvc_find_control(entity, v4l2_id, mapping, &ctrl, next);
-		if (ctrl && !next)
+		__uvc_find_control(entity, v4l2_id, mapping, &ctrl, next,
+				   next_compound);
+		if (ctrl && !next && !next_compound)
 			return ctrl;
 	}
 
-	if (ctrl == NULL && !next)
+	if (!ctrl && !next && !next_compound)
 		uvc_dbg(chain->dev, CONTROL, "Control 0x%08x not found\n",
 			v4l2_id);
 
@@ -1048,10 +1075,59 @@  static int __uvc_ctrl_get_std(struct uvc_video_chain *chain,
 	return 0;
 }
 
+static int __uvc_ctrl_get_compound(struct uvc_control_mapping *mapping,
+				   struct uvc_control *ctrl,
+				   int id,
+				   struct v4l2_ext_control *xctrl)
+{
+	u8 size;
+	u8 *data;
+	int ret;
+
+	size = mapping->v4l2_size / 8;
+	if (xctrl->size < size) {
+		xctrl->size = size;
+		return -ENOSPC;
+	}
+
+	data = kmalloc(size, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	ret = mapping->get_compound(mapping, uvc_ctrl_data(ctrl, id), data);
+	if (ret < 0)
+		goto out;
+
+	ret = copy_to_user(xctrl->ptr, data, size) ? -EFAULT : 0;
+
+out:
+	kfree(data);
+	return ret;
+}
+
+static int __uvc_ctrl_get_compound_cur(struct uvc_video_chain *chain,
+				       struct uvc_control *ctrl,
+				       struct uvc_control_mapping *mapping,
+				       struct v4l2_ext_control *xctrl)
+{
+	int ret;
+
+	if (!uvc_ctrl_mapping_is_compound(mapping))
+		return -EINVAL;
+
+	ret = __uvc_ctrl_load_cur(chain, ctrl);
+	if (ret < 0)
+		return ret;
+
+	return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT,
+				       xctrl);
+}
+
 static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
 				  u32 found_id)
 {
 	bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL;
+	bool find_next_compound = req_id & V4L2_CTRL_FLAG_NEXT_COMPOUND;
 	unsigned int i;
 
 	req_id &= V4L2_CTRL_ID_MASK;
@@ -1059,7 +1135,7 @@  static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
 	for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) {
 		if (!(chain->ctrl_class_bitmap & BIT(i)))
 			continue;
-		if (!find_next) {
+		if (!find_next && !find_next_compound) {
 			if (uvc_control_classes[i] == req_id)
 				return i;
 			continue;
@@ -1151,7 +1227,7 @@  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
 
 	if (mapping->master_id)
 		__uvc_find_control(ctrl->entity, mapping->master_id,
-				   &master_map, &master_ctrl, 0);
+				   &master_map, &master_ctrl, 0, 0);
 	if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
 		s32 val = 0;
 		int ret;
@@ -1167,6 +1243,15 @@  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
 				v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
 	}
 
+	if (v4l2_ctrl->type >= V4L2_CTRL_COMPOUND_TYPES) {
+		v4l2_ctrl->flags |= V4L2_CTRL_FLAG_HAS_PAYLOAD;
+		v4l2_ctrl->default_value = 0;
+		v4l2_ctrl->minimum = 0;
+		v4l2_ctrl->maximum = 0;
+		v4l2_ctrl->step = 0;
+		return 0;
+	}
+
 	if (!ctrl->cached) {
 		int ret = uvc_ctrl_populate_cache(chain, ctrl);
 		if (ret < 0)
@@ -1400,7 +1485,7 @@  static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
 	u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
 	s32 val = 0;
 
-	__uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0);
+	__uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0, 0);
 	if (ctrl == NULL)
 		return;
 
@@ -1706,7 +1791,7 @@  static int uvc_ctrl_find_ctrl_idx(struct uvc_entity *entity,
 
 	for (i = 0; i < ctrls->count; i++) {
 		__uvc_find_control(entity, ctrls->controls[i].id, &mapping,
-				   &ctrl_found, 0);
+				   &ctrl_found, 0, 0);
 		if (uvc_control == ctrl_found)
 			return i;
 	}
@@ -1754,7 +1839,7 @@  int uvc_ctrl_get(struct uvc_video_chain *chain,
 		return -EINVAL;
 
 	if (uvc_ctrl_mapping_is_compound(mapping))
-		return -EINVAL;
+		return __uvc_ctrl_get_compound_cur(chain, ctrl, mapping, xctrl);
 	else
 		return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value);
 }
@@ -1776,6 +1861,25 @@  static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain,
 	return 0;
 }
 
+static int __uvc_ctrl_get_boundary_compound(struct uvc_video_chain *chain,
+					    struct uvc_control *ctrl,
+					    struct uvc_control_mapping *mapping,
+					    struct v4l2_ext_control *xctrl)
+{
+	int ret;
+
+	if (!uvc_ctrl_mapping_is_compound(mapping))
+		return -EINVAL;
+
+	if (!ctrl->cached) {
+		ret = uvc_ctrl_populate_cache(chain, ctrl);
+		if (ret < 0)
+			return ret;
+	}
+
+	return __uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_DEF, xctrl);
+}
+
 int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
 			  struct v4l2_ext_control *xctrl)
 {
@@ -1793,7 +1897,8 @@  int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
 	}
 
 	if (uvc_ctrl_mapping_is_compound(mapping))
-		ret = -EINVAL;
+		ret = __uvc_ctrl_get_boundary_compound(chain, ctrl, mapping,
+						       xctrl);
 	else
 		ret = __uvc_ctrl_get_boundary_std(chain, ctrl, mapping, xctrl);
 
@@ -1802,6 +1907,34 @@  int uvc_ctrl_get_boundary(struct uvc_video_chain *chain,
 	return ret;
 }
 
+static int __uvc_ctrl_set_compound(struct uvc_control_mapping *mapping,
+				   struct v4l2_ext_control *xctrl,
+				   struct uvc_control *ctrl)
+{
+	u8 *data;
+	int ret;
+
+	if (xctrl->size != mapping->v4l2_size / 8)
+		return -EINVAL;
+
+	data = kmalloc(xctrl->size, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	ret = copy_from_user(data, xctrl->ptr, xctrl->size);
+	if (ret < 0)
+		goto out;
+
+	ret = mapping->set_compound(mapping, data,
+			uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
+
+	__uvc_ctrl_get_compound(mapping, ctrl, UVC_CTRL_DATA_CURRENT, xctrl);
+
+out:
+	kfree(data);
+	return ret;
+}
+
 int uvc_ctrl_set(struct uvc_fh *handle,
 	struct v4l2_ext_control *xctrl)
 {
@@ -1903,12 +2036,14 @@  int uvc_ctrl_set(struct uvc_fh *handle,
 		       ctrl->info.size);
 	}
 
-	if (!uvc_ctrl_mapping_is_compound(mapping))
+	if (!uvc_ctrl_mapping_is_compound(mapping)) {
 		mapping->set(mapping, value,
 			     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
-	else
-		return -EINVAL;
-
+	} else {
+		ret = __uvc_ctrl_set_compound(mapping, xctrl, ctrl);
+		if (ret < 0)
+			return ret;
+	}
 
 	if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
 		ctrl->handle = handle;
@@ -2308,10 +2443,23 @@  static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 		return -ENOMEM;
 	}
 
-	if (map->get == NULL)
+	if (uvc_ctrl_mapping_is_compound(map)) {
+		if (map->data_size != map->v4l2_size)
+			return -EINVAL;
+
+		/* Only supports byte-aligned data. */
+		if (WARN_ON(map->offset % 8 || map->data_size % 8))
+			return -EINVAL;
+	}
+
+	if (!map->get && !uvc_ctrl_mapping_is_compound(map))
 		map->get = uvc_get_le_value;
-	if (map->set == NULL)
+	if (!map->set && !uvc_ctrl_mapping_is_compound(map))
 		map->set = uvc_set_le_value;
+	if (!map->get_compound && uvc_ctrl_mapping_is_compound(map))
+		map->get_compound = uvc_get_compound;
+	if (!map->set_compound && uvc_ctrl_mapping_is_compound(map))
+		map->set_compound = uvc_set_compound;
 
 	for (i = 0; i < ARRAY_SIZE(uvc_control_classes); i++) {
 		if (V4L2_CTRL_ID2WHICH(uvc_control_classes[i]) ==
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 8f7938205a63..1e1bccd3b2e5 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -129,8 +129,12 @@  struct uvc_control_mapping {
 
 	s32 (*get)(struct uvc_control_mapping *mapping, u8 query,
 		   const u8 *data);
+	int (*get_compound)(struct uvc_control_mapping *mapping, const u8 *data,
+			    u8 *data_out);
 	void (*set)(struct uvc_control_mapping *mapping, s32 value,
 		    u8 *data);
+	int (*set_compound)(struct uvc_control_mapping *mapping, const u8 *data_in,
+			    u8 *data);
 };
 
 struct uvc_control {