Re: [PATCH v14 10/11] media: uvcvideo: initilaize ROI control to default value

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Yunke,

Thank you for the patch.

In the commit message, s/initilaize/Initialize/

On Fri, Dec 01, 2023 at 04:19:01PM +0900, Yunke Cao wrote:
> Add an init function to uvc_control_info. Use the function to
> initialize ROI control to default value.
> 
> Also moves utility functions to the top of uvc_ctrl.c, above
> the uvc_ctrls definition. uvc_ctrl_init_roi() calls uvc_ctrl_data()
> and need to be declared before uvc_ctrls.

Please move functions in a separate patch that does not change anything
else. It's otherwise difficult to review the changes.

> Signed-off-by: Yunke Cao <yunkec@google.com>
> ---
> Changelog since v8:
> - No change.
> Changelog since v7:
> - Newly added patch. Split initializing from the previous patch.
> - Add an init operation to uvc_control_info and use it for ROI
>   initialization.
>   
>  drivers/media/usb/uvc/uvc_ctrl.c | 273 ++++++++++++++++++-------------
>  drivers/media/usb/uvc/uvcvideo.h |   3 +
>  2 files changed, 160 insertions(+), 116 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index d405b2a9d477..bda625c392c2 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -32,6 +32,158 @@
>  #define UVC_CTRL_DATA_DEF	5
>  #define UVC_CTRL_DATA_LAST	6
>  
> +/* ------------------------------------------------------------------------
> + * Utility functions
> + */
> +
> +static inline u8 *uvc_ctrl_data(struct uvc_control *ctrl, int id)
> +{
> +	return ctrl->uvc_data + id * ctrl->info.size;
> +}
> +
> +static inline int uvc_test_bit(const u8 *data, int bit)
> +{
> +	return (data[bit >> 3] >> (bit & 7)) & 1;
> +}
> +
> +static inline void uvc_clear_bit(u8 *data, int bit)
> +{
> +	data[bit >> 3] &= ~(1 << (bit & 7));
> +}
> +
> +/*
> + * Extract the bit string specified by mapping->offset and mapping->data_size
> + * from the little-endian data stored at 'data' and return the result as
> + * a signed 32bit integer. Sign extension will be performed if the mapping
> + * references a signed data type.
> + */
> +static s32 uvc_get_le_value(struct uvc_control_mapping *mapping,
> +			    u8 query, const u8 *data)
> +{
> +	int bits = mapping->data_size;
> +	int offset = mapping->offset;
> +	s32 value = 0;
> +	u8 mask;
> +
> +	data += offset / 8;
> +	offset &= 7;
> +	mask = ((1LL << bits) - 1) << offset;
> +
> +	while (1) {
> +		u8 byte = *data & mask;
> +
> +		value |= offset > 0 ? (byte >> offset) : (byte << (-offset));
> +		bits -= 8 - (offset > 0 ? offset : 0);
> +		if (bits <= 0)
> +			break;
> +
> +		offset -= 8;
> +		mask = (1 << bits) - 1;
> +		data++;
> +	}
> +
> +	/* Sign-extend the value if needed. */
> +	if (mapping->data_type == UVC_CTRL_DATA_TYPE_SIGNED)
> +		value |= -(value & (1 << (mapping->data_size - 1)));
> +
> +	return value;
> +}
> +
> +/*
> + * Set the bit string specified by mapping->offset and mapping->data_size
> + * in the little-endian data stored at 'data' to the value 'value'.
> + */
> +static void uvc_set_le_value(struct uvc_control_mapping *mapping,
> +			     s32 value, u8 *data)
> +{
> +	int bits = mapping->data_size;
> +	int offset = mapping->offset;
> +	u8 mask;
> +
> +	/*
> +	 * According to the v4l2 spec, writing any value to a button control
> +	 * should result in the action belonging to the button control being
> +	 * triggered. UVC devices however want to see a 1 written -> override
> +	 * value.
> +	 */
> +	if (mapping->v4l2_type == V4L2_CTRL_TYPE_BUTTON)
> +		value = -1;
> +
> +	data += offset / 8;
> +	offset &= 7;
> +
> +	for (; bits > 0; data++) {
> +		mask = ((1LL << bits) - 1) << offset;
> +		*data = (*data & ~mask) | ((value << offset) & mask);
> +		value >>= offset ? offset : 8;
> +		bits -= 8 - offset;
> +		offset = 0;
> +	}
> +}
> +
> +/*
> + * 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, const u8 *data_min,
> +			    const u8 *data_max, 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)
> +{
> +	return mapping->v4l2_type >= V4L2_CTRL_COMPOUND_TYPES;
> +}
> +
> +static int uvc_ctrl_init_roi(struct uvc_device *dev, struct uvc_control *ctrl)
> +{
> +	int ret;
> +
> +	ret = uvc_query_ctrl(dev, UVC_GET_DEF, ctrl->entity->id, dev->intfnum,
> +			     UVC_CT_REGION_OF_INTEREST_CONTROL,
> +			     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF),
> +			     ctrl->info.size);
> +	if (ret)
> +		goto out;
> +
> +	/*
> +	 * Most firmwares have wrong GET_CUR configuration. E.g. it's
> +	 * below GET_MIN, or have rectangle coordinates mixed up. This

Seriously :'-(

> +	 * causes problems sometimes, because we are unable to set
> +	 * auto-controls value without first setting ROI rectangle to
> +	 * valid configuration.
> +	 *
> +	 * We expect that default configuration is always correct and
> +	 * is within the GET_MIN / GET_MAX boundaries.
> +	 *
> +	 * Set current ROI configuration to GET_DEF, so that we will
> +	 * always have properly configured ROI.
> +	 */

You can reflow the text to 80 columns.

> +	ret = uvc_query_ctrl(dev, UVC_SET_CUR, 1, dev->intfnum,

Hardcoding the entity ID to 1 doesn't seem right.

> +			     UVC_CT_REGION_OF_INTEREST_CONTROL,
> +			     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF),
> +			     ctrl->info.size);
> +out:
> +	if (ret)
> +		dev_err(&dev->udev->dev, "Failed to fixup ROI (%d).\n", ret);
> +	return ret;
> +}
> +
>  /* ------------------------------------------------------------------------
>   * Controls
>   */
> @@ -375,6 +527,7 @@ static const struct uvc_control_info uvc_ctrls[] = {
>  				| UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
>  				| UVC_CTRL_FLAG_GET_DEF
>  				| UVC_CTRL_FLAG_AUTO_UPDATE,
> +		.init		= uvc_ctrl_init_roi,
>  	},
>  };
>  
> @@ -901,122 +1054,6 @@ static const struct uvc_control_mapping *uvc_ctrl_mappings_uvc15[] = {
>  	NULL, /* Sentinel */
>  };
>  
> -/* ------------------------------------------------------------------------
> - * Utility functions
> - */
> -
> -static inline u8 *uvc_ctrl_data(struct uvc_control *ctrl, int id)
> -{
> -	return ctrl->uvc_data + id * ctrl->info.size;
> -}
> -
> -static inline int uvc_test_bit(const u8 *data, int bit)
> -{
> -	return (data[bit >> 3] >> (bit & 7)) & 1;
> -}
> -
> -static inline void uvc_clear_bit(u8 *data, int bit)
> -{
> -	data[bit >> 3] &= ~(1 << (bit & 7));
> -}
> -
> -/*
> - * Extract the bit string specified by mapping->offset and mapping->data_size
> - * from the little-endian data stored at 'data' and return the result as
> - * a signed 32bit integer. Sign extension will be performed if the mapping
> - * references a signed data type.
> - */
> -static s32 uvc_get_le_value(struct uvc_control_mapping *mapping,
> -	u8 query, const u8 *data)
> -{
> -	int bits = mapping->data_size;
> -	int offset = mapping->offset;
> -	s32 value = 0;
> -	u8 mask;
> -
> -	data += offset / 8;
> -	offset &= 7;
> -	mask = ((1LL << bits) - 1) << offset;
> -
> -	while (1) {
> -		u8 byte = *data & mask;
> -		value |= offset > 0 ? (byte >> offset) : (byte << (-offset));
> -		bits -= 8 - (offset > 0 ? offset : 0);
> -		if (bits <= 0)
> -			break;
> -
> -		offset -= 8;
> -		mask = (1 << bits) - 1;
> -		data++;
> -	}
> -
> -	/* Sign-extend the value if needed. */
> -	if (mapping->data_type == UVC_CTRL_DATA_TYPE_SIGNED)
> -		value |= -(value & (1 << (mapping->data_size - 1)));
> -
> -	return value;
> -}
> -
> -/*
> - * Set the bit string specified by mapping->offset and mapping->data_size
> - * in the little-endian data stored at 'data' to the value 'value'.
> - */
> -static void uvc_set_le_value(struct uvc_control_mapping *mapping,
> -	s32 value, u8 *data)
> -{
> -	int bits = mapping->data_size;
> -	int offset = mapping->offset;
> -	u8 mask;
> -
> -	/*
> -	 * According to the v4l2 spec, writing any value to a button control
> -	 * should result in the action belonging to the button control being
> -	 * triggered. UVC devices however want to see a 1 written -> override
> -	 * value.
> -	 */
> -	if (mapping->v4l2_type == V4L2_CTRL_TYPE_BUTTON)
> -		value = -1;
> -
> -	data += offset / 8;
> -	offset &= 7;
> -
> -	for (; bits > 0; data++) {
> -		mask = ((1LL << bits) - 1) << offset;
> -		*data = (*data & ~mask) | ((value << offset) & mask);
> -		value >>= offset ? offset : 8;
> -		bits -= 8 - offset;
> -		offset = 0;
> -	}
> -}
> -
> -/*
> - * 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, const u8 *data_min,
> -			    const u8 *data_max, u8 *data)
> -{
> -	memcpy(data + mapping->offset / 8, data_in, mapping->data_size / 8);
> -	return 0;
> -}

Looks like there was a missing blank line in a previous patch in the
series.

> -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
>   */
> @@ -2984,6 +3021,10 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
>  			 * GET_INFO on standard controls.
>  			 */
>  			uvc_ctrl_get_flags(chain->dev, ctrl, &ctrl->info);
> +
> +			if (info->init)
> +				info->init(chain->dev, ctrl);
> +
>  			break;
>  		 }
>  	}
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 18da5e0b8cb2..335b565da6de 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -85,6 +85,7 @@
>  struct gpio_desc;
>  struct sg_table;
>  struct uvc_device;
> +struct uvc_control;

Alphabetical order please.

>  
>  /*
>   * TODO: Put the most frequently accessed fields at the beginning of
> @@ -99,6 +100,8 @@ struct uvc_control_info {
>  
>  	u16 size;
>  	u32 flags;
> +
> +	int (*init)(struct uvc_device *dev, struct uvc_control *ctrl);
>  };
>  
>  struct uvc_control_mapping {

-- 
Regards,

Laurent Pinchart