[PATCH 0/5] media: uvc: Probe PLF limits at start-up

[PATCH 0/5] media: uvc: Probe PLF limits at start-up

[Ricardo Ribalda]

The UVC standard descries the values for the PLF control for its
versions 1.1 and and 1.5, but it does not describe which values MUST be
implemented.

So far, we have been adding "device quirks" to provide proper limits for
the control, but this is failing to scale.

Add a function that during probe-time checks the capability of the
control.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Ricardo Ribalda (5):
      media: uvcvideo: Allow custom control mapping
      media: uvcvideo: Refactor Power Line Frequency limit selection
      media: uvcvideo: Probe the PLF characteristics
      media: uvcvideo: Cleanup version-specific mapping
      media: uvcvideo: Remove PLF device quirking

 drivers/media/usb/uvc/uvc_ctrl.c   | 173 ++++++++++++++++++++++++-------------
 drivers/media/usb/uvc/uvc_driver.c | 122 --------------------------
 drivers/media/usb/uvc/uvcvideo.h   |  61 ++++++-------
 3 files changed, 143 insertions(+), 213 deletions(-)
---
base-commit: b14257abe7057def6127f6fb2f14f9adc8acabdb
change-id: 20240313-billion-5b2a45fa86f4

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH 1/5] media: uvcvideo: Allow custom control mapping

[Ricardo Ribalda]

Some advanced controls might not be completely implemented by vendors.

If the controls are a enumeration, UVC does not gives a way to probe
what is implemented and what is not.

Lets create a new callback function where heuristics can be implemented
to detect what is implemented and what not.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 15 ++++++++--
 drivers/media/usb/uvc/uvcvideo.h | 59 +++++++++++++++++++++-------------------
 2 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index e59a463c27618..3e939b4fbaaaf 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2434,6 +2434,14 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 	return -ENOMEM;
 }
 
+static int __uvc_ctrl_add_custom_mapping(struct uvc_video_chain *chain,
+	struct uvc_control *ctrl, const struct uvc_control_mapping *mapping)
+{
+	if (mapping && mapping->add_mapping)
+		return mapping->add_mapping(chain, ctrl, mapping);
+	return __uvc_ctrl_add_mapping(chain, ctrl, mapping);
+}
+
 int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 	const struct uvc_control_mapping *mapping)
 {
@@ -2637,7 +2645,8 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
 
 			if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
 			    ctrl->info.selector == mapping->selector) {
-				__uvc_ctrl_add_mapping(chain, ctrl, mapping);
+				__uvc_ctrl_add_custom_mapping(chain, ctrl,
+							      mapping);
 				custom = true;
 			}
 		}
@@ -2652,7 +2661,7 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
 
 		if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
 		    ctrl->info.selector == mapping->selector)
-			__uvc_ctrl_add_mapping(chain, ctrl, mapping);
+			__uvc_ctrl_add_custom_mapping(chain, ctrl, mapping);
 	}
 
 	/* Finally process version-specific mappings. */
@@ -2664,7 +2673,7 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
 
 		if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
 		    ctrl->info.selector == mapping->selector)
-			__uvc_ctrl_add_mapping(chain, ctrl, mapping);
+			__uvc_ctrl_add_custom_mapping(chain, ctrl, mapping);
 	}
 }
 
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 6fb0a78b1b009..611350a82c37f 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -101,34 +101,6 @@ struct uvc_control_info {
 	u32 flags;
 };
 
-struct uvc_control_mapping {
-	struct list_head list;
-	struct list_head ev_subs;
-
-	u32 id;
-	char *name;
-	u8 entity[16];
-	u8 selector;
-
-	u8 size;
-	u8 offset;
-	enum v4l2_ctrl_type v4l2_type;
-	u32 data_type;
-
-	const u32 *menu_mapping;
-	const char (*menu_names)[UVC_MENU_NAME_LEN];
-	unsigned long menu_mask;
-
-	u32 master_id;
-	s32 master_manual;
-	u32 slave_ids[2];
-
-	s32 (*get)(struct uvc_control_mapping *mapping, u8 query,
-		   const u8 *data);
-	void (*set)(struct uvc_control_mapping *mapping, s32 value,
-		    u8 *data);
-};
-
 struct uvc_control {
 	struct uvc_entity *entity;
 	struct uvc_control_info info;
@@ -336,6 +308,37 @@ struct uvc_video_chain {
 	u8 ctrl_class_bitmap;			/* Bitmap of valid classes */
 };
 
+struct uvc_control_mapping {
+	struct list_head list;
+	struct list_head ev_subs;
+
+	u32 id;
+	char *name;
+	u8 entity[16];
+	u8 selector;
+
+	u8 size;
+	u8 offset;
+	enum v4l2_ctrl_type v4l2_type;
+	u32 data_type;
+
+	const u32 *menu_mapping;
+	const char (*menu_names)[UVC_MENU_NAME_LEN];
+	unsigned long menu_mask;
+
+	u32 master_id;
+	s32 master_manual;
+	u32 slave_ids[2];
+
+	int (*add_mapping)(struct uvc_video_chain *chain,
+			   struct uvc_control *ctrl,
+			   const struct uvc_control_mapping *mapping);
+	s32 (*get)(struct uvc_control_mapping *mapping, u8 query,
+		   const u8 *data);
+	void (*set)(struct uvc_control_mapping *mapping, s32 value,
+		    u8 *data);
+};
+
 struct uvc_stats_frame {
 	unsigned int size;		/* Number of bytes captured */
 	unsigned int first_data;	/* Index of the first non-empty packet */

-- 
2.44.0.291.gc1ea87d7ee-goog

Re: [PATCH 1/5] media: uvcvideo: Allow custom control mapping

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Mon, Mar 18, 2024 at 11:55:23PM +0000, Ricardo Ribalda wrote:
> Some advanced controls might not be completely implemented by vendors.
> 
> If the controls are a enumeration, UVC does not gives a way to probe
> what is implemented and what is not.
> 
> Lets create a new callback function where heuristics can be implemented

s/Lets/Let's/

> to detect what is implemented and what not.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 15 ++++++++--
>  drivers/media/usb/uvc/uvcvideo.h | 59 +++++++++++++++++++++-------------------
>  2 files changed, 43 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index e59a463c27618..3e939b4fbaaaf 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2434,6 +2434,14 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>  	return -ENOMEM;
>  }
>  
> +static int __uvc_ctrl_add_custom_mapping(struct uvc_video_chain *chain,
> +	struct uvc_control *ctrl, const struct uvc_control_mapping *mapping)

The name isn't great is in most cases it doesn't add a custom mapping.

> +{
> +	if (mapping && mapping->add_mapping)
> +		return mapping->add_mapping(chain, ctrl, mapping);
> +	return __uvc_ctrl_add_mapping(chain, ctrl, mapping);
> +}
> +
>  int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>  	const struct uvc_control_mapping *mapping)
>  {
> @@ -2637,7 +2645,8 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
>  
>  			if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
>  			    ctrl->info.selector == mapping->selector) {
> -				__uvc_ctrl_add_mapping(chain, ctrl, mapping);
> +				__uvc_ctrl_add_custom_mapping(chain, ctrl,
> +							      mapping);
>  				custom = true;
>  			}
>  		}
> @@ -2652,7 +2661,7 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
>  
>  		if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
>  		    ctrl->info.selector == mapping->selector)
> -			__uvc_ctrl_add_mapping(chain, ctrl, mapping);
> +			__uvc_ctrl_add_custom_mapping(chain, ctrl, mapping);
>  	}
>  
>  	/* Finally process version-specific mappings. */
> @@ -2664,7 +2673,7 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
>  
>  		if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
>  		    ctrl->info.selector == mapping->selector)
> -			__uvc_ctrl_add_mapping(chain, ctrl, mapping);
> +			__uvc_ctrl_add_custom_mapping(chain, ctrl, mapping);
>  	}
>  }
>  
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 6fb0a78b1b009..611350a82c37f 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -101,34 +101,6 @@ struct uvc_control_info {
>  	u32 flags;
>  };
>  
> -struct uvc_control_mapping {
> -	struct list_head list;
> -	struct list_head ev_subs;
> -
> -	u32 id;
> -	char *name;
> -	u8 entity[16];
> -	u8 selector;
> -
> -	u8 size;
> -	u8 offset;
> -	enum v4l2_ctrl_type v4l2_type;
> -	u32 data_type;
> -
> -	const u32 *menu_mapping;
> -	const char (*menu_names)[UVC_MENU_NAME_LEN];
> -	unsigned long menu_mask;
> -
> -	u32 master_id;
> -	s32 master_manual;
> -	u32 slave_ids[2];
> -
> -	s32 (*get)(struct uvc_control_mapping *mapping, u8 query,
> -		   const u8 *data);
> -	void (*set)(struct uvc_control_mapping *mapping, s32 value,
> -		    u8 *data);
> -};
> -

You can leave the structure here, close to the other related structuers,
and add forward declarations of the uvc_video_chain and uvc_control
structures just before the existing forward declaration of uvc_device.

>  struct uvc_control {
>  	struct uvc_entity *entity;
>  	struct uvc_control_info info;
> @@ -336,6 +308,37 @@ struct uvc_video_chain {
>  	u8 ctrl_class_bitmap;			/* Bitmap of valid classes */
>  };
>  
> +struct uvc_control_mapping {
> +	struct list_head list;
> +	struct list_head ev_subs;
> +
> +	u32 id;
> +	char *name;
> +	u8 entity[16];
> +	u8 selector;
> +
> +	u8 size;
> +	u8 offset;
> +	enum v4l2_ctrl_type v4l2_type;
> +	u32 data_type;
> +
> +	const u32 *menu_mapping;
> +	const char (*menu_names)[UVC_MENU_NAME_LEN];
> +	unsigned long menu_mask;
> +
> +	u32 master_id;
> +	s32 master_manual;
> +	u32 slave_ids[2];
> +
> +	int (*add_mapping)(struct uvc_video_chain *chain,
> +			   struct uvc_control *ctrl,
> +			   const struct uvc_control_mapping *mapping);
> +	s32 (*get)(struct uvc_control_mapping *mapping, u8 query,
> +		   const u8 *data);
> +	void (*set)(struct uvc_control_mapping *mapping, s32 value,
> +		    u8 *data);
> +};
> +
>  struct uvc_stats_frame {
>  	unsigned int size;		/* Number of bytes captured */
>  	unsigned int first_data;	/* Index of the first non-empty packet */
> 

-- 
Regards,

Laurent Pinchart

[PATCH 2/5] media: uvcvideo: Refactor Power Line Frequency limit selection

[Ricardo Ribalda]

Move the PLF mapping logic to its own function. This patch does not
introduce any new functionality to the logic, it is just a preparation
patch.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 93 ++++++++++++++++++++++++----------------
 1 file changed, 55 insertions(+), 38 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 3e939b4fbaaaf..67522143c6c85 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -459,6 +459,56 @@ static void uvc_ctrl_set_rel_speed(struct uvc_control_mapping *mapping,
 	data[first+1] = min_t(int, abs(value), 0xff);
 }
 
+const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
+	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
+	.entity		= UVC_GUID_UVC_PROCESSING,
+	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
+	.size		= 2,
+	.offset		= 0,
+	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
+	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
+	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ,
+				  V4L2_CID_POWER_LINE_FREQUENCY_50HZ),
+};
+
+const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc11 = {
+	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
+	.entity		= UVC_GUID_UVC_PROCESSING,
+	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
+	.size		= 2,
+	.offset		= 0,
+	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
+	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
+	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ,
+				  V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
+};
+
+static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc15 = {
+	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
+	.entity		= UVC_GUID_UVC_PROCESSING,
+	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
+	.size		= 2,
+	.offset		= 0,
+	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
+	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
+	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_AUTO,
+				  V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
+};
+
+static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
+	struct uvc_control *ctrl, const struct uvc_control_mapping *mapping);
+
+static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain,
+	struct uvc_control *ctrl, const struct uvc_control_mapping *mapping)
+{
+	if (chain->dev->uvc_version < 0x150)
+		return __uvc_ctrl_add_mapping(chain, ctrl,
+					      &uvc_ctrl_power_line_mapping_uvc11);
+
+	return __uvc_ctrl_add_mapping(chain, ctrl,
+				      &uvc_ctrl_power_line_mapping_uvc15);
+}
+
 static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	{
 		.id		= V4L2_CID_BRIGHTNESS,
@@ -748,51 +798,18 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 		.v4l2_type	= V4L2_CTRL_TYPE_BOOLEAN,
 		.data_type	= UVC_CTRL_DATA_TYPE_BOOLEAN,
 	},
-};
-
-const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
-	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
-	.entity		= UVC_GUID_UVC_PROCESSING,
-	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
-	.size		= 2,
-	.offset		= 0,
-	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
-	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
-	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ,
-				  V4L2_CID_POWER_LINE_FREQUENCY_50HZ),
-};
-
-const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc11 = {
-	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
-	.entity		= UVC_GUID_UVC_PROCESSING,
-	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
-	.size		= 2,
-	.offset		= 0,
-	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
-	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
-	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ,
-				  V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
+	{
+		.entity		= UVC_GUID_UVC_PROCESSING,
+		.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
+		.add_mapping	= uvc_ctrl_add_plf_mapping,
+	},
 };
 
 static const struct uvc_control_mapping *uvc_ctrl_mappings_uvc11[] = {
-	&uvc_ctrl_power_line_mapping_uvc11,
 	NULL, /* Sentinel */
 };
 
-static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc15 = {
-	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
-	.entity		= UVC_GUID_UVC_PROCESSING,
-	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
-	.size		= 2,
-	.offset		= 0,
-	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
-	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
-	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_AUTO,
-				  V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
-};
-
 static const struct uvc_control_mapping *uvc_ctrl_mappings_uvc15[] = {
-	&uvc_ctrl_power_line_mapping_uvc15,
 	NULL, /* Sentinel */
 };
 

-- 
2.44.0.291.gc1ea87d7ee-goog

Re: [PATCH 2/5] media: uvcvideo: Refactor Power Line Frequency limit selection

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Mon, Mar 18, 2024 at 11:55:24PM +0000, Ricardo Ribalda wrote:
> Move the PLF mapping logic to its own function. This patch does not
> introduce any new functionality to the logic, it is just a preparation
> patch.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 93 ++++++++++++++++++++++++----------------
>  1 file changed, 55 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 3e939b4fbaaaf..67522143c6c85 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -459,6 +459,56 @@ static void uvc_ctrl_set_rel_speed(struct uvc_control_mapping *mapping,
>  	data[first+1] = min_t(int, abs(value), 0xff);
>  }
>  
> +const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
> +	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
> +	.entity		= UVC_GUID_UVC_PROCESSING,
> +	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> +	.size		= 2,
> +	.offset		= 0,
> +	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
> +	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
> +	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ,
> +				  V4L2_CID_POWER_LINE_FREQUENCY_50HZ),
> +};
> +
> +const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc11 = {
> +	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
> +	.entity		= UVC_GUID_UVC_PROCESSING,
> +	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> +	.size		= 2,
> +	.offset		= 0,
> +	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
> +	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
> +	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ,
> +				  V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
> +};
> +
> +static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc15 = {
> +	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
> +	.entity		= UVC_GUID_UVC_PROCESSING,
> +	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> +	.size		= 2,
> +	.offset		= 0,
> +	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
> +	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
> +	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_AUTO,
> +				  V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
> +};
> +
> +static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> +	struct uvc_control *ctrl, const struct uvc_control_mapping *mapping);

I wonder if we could avoid the forward declaration by turning the
.add_mapping() operation into a .filter_mapping() (name to be
bikshedded) that would return a replacement mapping instead of adding
it. The caller (the __uvc_ctrl_add_custom_mapping() function) would then
call __uvc_ctrl_add_mapping() unconditionally. You could actually call
the new operation directly in __uvc_ctrl_add_custom_mapping() without
having to add a new __uvc_ctrl_add_custom_mapping() function. What do
you think, would that be simpler and more redable ?

> +
> +static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain,
> +	struct uvc_control *ctrl, const struct uvc_control_mapping *mapping)
> +{
> +	if (chain->dev->uvc_version < 0x150)
> +		return __uvc_ctrl_add_mapping(chain, ctrl,
> +					      &uvc_ctrl_power_line_mapping_uvc11);
> +
> +	return __uvc_ctrl_add_mapping(chain, ctrl,
> +				      &uvc_ctrl_power_line_mapping_uvc15);
> +}
> +
>  static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	{
>  		.id		= V4L2_CID_BRIGHTNESS,
> @@ -748,51 +798,18 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  		.v4l2_type	= V4L2_CTRL_TYPE_BOOLEAN,
>  		.data_type	= UVC_CTRL_DATA_TYPE_BOOLEAN,
>  	},
> -};
> -
> -const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
> -	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
> -	.entity		= UVC_GUID_UVC_PROCESSING,
> -	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> -	.size		= 2,
> -	.offset		= 0,
> -	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
> -	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
> -	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ,
> -				  V4L2_CID_POWER_LINE_FREQUENCY_50HZ),
> -};
> -
> -const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc11 = {
> -	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
> -	.entity		= UVC_GUID_UVC_PROCESSING,
> -	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> -	.size		= 2,
> -	.offset		= 0,
> -	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
> -	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
> -	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ,
> -				  V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
> +	{
> +		.entity		= UVC_GUID_UVC_PROCESSING,
> +		.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> +		.add_mapping	= uvc_ctrl_add_plf_mapping,
> +	},
>  };
>  
>  static const struct uvc_control_mapping *uvc_ctrl_mappings_uvc11[] = {
> -	&uvc_ctrl_power_line_mapping_uvc11,
>  	NULL, /* Sentinel */
>  };
>  
> -static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc15 = {
> -	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
> -	.entity		= UVC_GUID_UVC_PROCESSING,
> -	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> -	.size		= 2,
> -	.offset		= 0,
> -	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
> -	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
> -	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_AUTO,
> -				  V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
> -};
> -
>  static const struct uvc_control_mapping *uvc_ctrl_mappings_uvc15[] = {
> -	&uvc_ctrl_power_line_mapping_uvc15,
>  	NULL, /* Sentinel */
>  };
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] media: uvcvideo: Refactor Power Line Frequency limit selection

[Ricardo Ribalda]

Hi Laurent

On Mon, 10 Jun 2024 at 17:21, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> > +static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> > +     struct uvc_control *ctrl, const struct uvc_control_mapping *mapping);
>
> I wonder if we could avoid the forward declaration by turning the
> .add_mapping() operation into a .filter_mapping() (name to be
> bikshedded) that would return a replacement mapping instead of adding
> it. The caller (the __uvc_ctrl_add_custom_mapping() function) would then
> call __uvc_ctrl_add_mapping() unconditionally. You could actually call
> the new operation directly in __uvc_ctrl_add_custom_mapping() without
> having to add a new __uvc_ctrl_add_custom_mapping() function. What do
> you think, would that be simpler and more redable ?

Let me add it as a forward patch, let me know what you think.

Regards!

[PATCH 3/5] media: uvcvideo: Probe the PLF characteristics

[Ricardo Ribalda]

The UVC 1.5 standard defines 4 values for the PLF control: Off, 50Hz,
60Hz and Auto. But it does not clearly define if all the values must be
implemented or not.

Instead of just using the UVC version to determine what the PLF control
can do, probe it.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 54 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 50 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 67522143c6c85..9a0b81aca30d1 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -501,12 +501,58 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain,
 	struct uvc_control *ctrl, const struct uvc_control_mapping *mapping)
 {
+	const struct uvc_control_mapping *out_mapping =
+					&uvc_ctrl_power_line_mapping_uvc11;
+	u8 init_val;
+	u8 *buf;
+	int ret;
+
+	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	/* Save the default PLF value, so we can restore it. */
+	ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id,
+			     chain->dev->intfnum, ctrl->info.selector,
+			     buf, sizeof(*buf));
+	/* If we cannot read the control skip it. */
+	if (ret) {
+		kfree(buf);
+		return ret;
+	}
+	init_val = *buf;
+
+	/* If PLF value cannot be set to off, it is limited. */
+	*buf = V4L2_CID_POWER_LINE_FREQUENCY_DISABLED;
+	ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
+			     chain->dev->intfnum, ctrl->info.selector,
+			     buf, sizeof(*buf));
+	if (ret) {
+		out_mapping = &uvc_ctrl_power_line_mapping_limited;
+		goto end;
+	}
+
+	/* UVC 1.1 does not define auto, we can exit. */
 	if (chain->dev->uvc_version < 0x150)
-		return __uvc_ctrl_add_mapping(chain, ctrl,
-					      &uvc_ctrl_power_line_mapping_uvc11);
+		goto end;
+
+	/* Check if the device supports auto. */
+	*buf = V4L2_CID_POWER_LINE_FREQUENCY_AUTO;
+	ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
+			     chain->dev->intfnum, ctrl->info.selector,
+			     buf, sizeof(*buf));
+	if (!ret)
+		out_mapping = &uvc_ctrl_power_line_mapping_uvc15;
+
+end:
+	/* Restore initial value and add mapping. */
+	*buf = init_val;
+	uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
+		       chain->dev->intfnum, ctrl->info.selector,
+		       buf, sizeof(*buf));
 
-	return __uvc_ctrl_add_mapping(chain, ctrl,
-				      &uvc_ctrl_power_line_mapping_uvc15);
+	kfree(buf);
+	return __uvc_ctrl_add_mapping(chain, ctrl, out_mapping);
 }
 
 static const struct uvc_control_mapping uvc_ctrl_mappings[] = {

-- 
2.44.0.291.gc1ea87d7ee-goog

Re: [PATCH 3/5] media: uvcvideo: Probe the PLF characteristics

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Mon, Mar 18, 2024 at 11:55:25PM +0000, Ricardo Ribalda wrote:
> The UVC 1.5 standard defines 4 values for the PLF control: Off, 50Hz,
> 60Hz and Auto. But it does not clearly define if all the values must be
> implemented or not.
> 
> Instead of just using the UVC version to determine what the PLF control
> can do, probe it.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 54 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 67522143c6c85..9a0b81aca30d1 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -501,12 +501,58 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>  static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain,
>  	struct uvc_control *ctrl, const struct uvc_control_mapping *mapping)
>  {
> +	const struct uvc_control_mapping *out_mapping =
> +					&uvc_ctrl_power_line_mapping_uvc11;
> +	u8 init_val;
> +	u8 *buf;

	u8 *buf __free(kfree) = NULL;

will simplify the exit paths.

> +	int ret;
> +
> +	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	/* Save the default PLF value, so we can restore it. */
> +	ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id,
> +			     chain->dev->intfnum, ctrl->info.selector,
> +			     buf, sizeof(*buf));

That's the current value, not the default. Is that intended ?

> +	/* If we cannot read the control skip it. */
> +	if (ret) {
> +		kfree(buf);
> +		return ret;
> +	}
> +	init_val = *buf;
> +
> +	/* If PLF value cannot be set to off, it is limited. */
> +	*buf = V4L2_CID_POWER_LINE_FREQUENCY_DISABLED;
> +	ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> +			     chain->dev->intfnum, ctrl->info.selector,
> +			     buf, sizeof(*buf));
> +	if (ret) {
> +		out_mapping = &uvc_ctrl_power_line_mapping_limited;
> +		goto end;

If setting the value failed you don't need to restore it, do you ?

> +	}
> +
> +	/* UVC 1.1 does not define auto, we can exit. */
>  	if (chain->dev->uvc_version < 0x150)
> -		return __uvc_ctrl_add_mapping(chain, ctrl,
> -					      &uvc_ctrl_power_line_mapping_uvc11);
> +		goto end;
> +
> +	/* Check if the device supports auto. */
> +	*buf = V4L2_CID_POWER_LINE_FREQUENCY_AUTO;
> +	ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> +			     chain->dev->intfnum, ctrl->info.selector,
> +			     buf, sizeof(*buf));

Now for the real annoying question. I've encountered quite a few devices
that would crash when the driver tried to get/set lots of controls at
probe time. This is why the control cache is populated the first time a
control is queried, and not when the device is probed. I'm always
worried when adding more control accesses at probe time that some
devices will behave improperly.

Given the number of UVC users I tend to be on the conservative side, but
obviously, if we were to strictly avoid new access patterns to the
device, the driver wouldn't be able to evolve. I do like patches 4/5 and
5/5, so I'm tempted to take the risk and revert this series later if
needed. That would likely make some other users unhappy if they rely on
the heuristic.

Another issue is how to get vendors to implement the power line
frequency control correctly. With the series applied, vendors won't
notice they're doing something wrong. Of course, they probably don't
check the behaviour of their devices with Linux in the first place, so
I'm not sure what we could do.

> +	if (!ret)
> +		out_mapping = &uvc_ctrl_power_line_mapping_uvc15;
> +
> +end:
> +	/* Restore initial value and add mapping. */
> +	*buf = init_val;
> +	uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> +		       chain->dev->intfnum, ctrl->info.selector,
> +		       buf, sizeof(*buf));
>  
> -	return __uvc_ctrl_add_mapping(chain, ctrl,
> -				      &uvc_ctrl_power_line_mapping_uvc15);
> +	kfree(buf);
> +	return __uvc_ctrl_add_mapping(chain, ctrl, out_mapping);
>  }
>  
>  static const struct uvc_control_mapping uvc_ctrl_mappings[] = {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] media: uvcvideo: Probe the PLF characteristics

[Ricardo Ribalda]

Hi Laurent

On Mon, 10 Jun 2024 at 16:14, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Mon, Mar 18, 2024 at 11:55:25PM +0000, Ricardo Ribalda wrote:
> > The UVC 1.5 standard defines 4 values for the PLF control: Off, 50Hz,
> > 60Hz and Auto. But it does not clearly define if all the values must be
> > implemented or not.
> >
> > Instead of just using the UVC version to determine what the PLF control
> > can do, probe it.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 54 +++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 50 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 67522143c6c85..9a0b81aca30d1 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -501,12 +501,58 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> >  static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain,
> >       struct uvc_control *ctrl, const struct uvc_control_mapping *mapping)
> >  {
> > +     const struct uvc_control_mapping *out_mapping =
> > +                                     &uvc_ctrl_power_line_mapping_uvc11;
> > +     u8 init_val;
> > +     u8 *buf;
>
>         u8 *buf __free(kfree) = NULL;
>
> will simplify the exit paths.
>
> > +     int ret;
> > +
> > +     buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> > +     if (!buf)
> > +             return -ENOMEM;
> > +
> > +     /* Save the default PLF value, so we can restore it. */
> > +     ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id,
> > +                          chain->dev->intfnum, ctrl->info.selector,
> > +                          buf, sizeof(*buf));
>
> That's the current value, not the default. Is that intended ?

Yes, the driver does not init the other controls to the default value.
So I'd rather be consistent.

>
> > +     /* If we cannot read the control skip it. */
> > +     if (ret) {
> > +             kfree(buf);
> > +             return ret;
> > +     }
> > +     init_val = *buf;
> > +
> > +     /* If PLF value cannot be set to off, it is limited. */
> > +     *buf = V4L2_CID_POWER_LINE_FREQUENCY_DISABLED;
> > +     ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> > +                          chain->dev->intfnum, ctrl->info.selector,
> > +                          buf, sizeof(*buf));
> > +     if (ret) {
> > +             out_mapping = &uvc_ctrl_power_line_mapping_limited;
> > +             goto end;
>
> If setting the value failed you don't need to restore it, do you ?
>
> > +     }
> > +
> > +     /* UVC 1.1 does not define auto, we can exit. */
> >       if (chain->dev->uvc_version < 0x150)
> > -             return __uvc_ctrl_add_mapping(chain, ctrl,
> > -                                           &uvc_ctrl_power_line_mapping_uvc11);
> > +             goto end;
> > +
> > +     /* Check if the device supports auto. */
> > +     *buf = V4L2_CID_POWER_LINE_FREQUENCY_AUTO;
> > +     ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> > +                          chain->dev->intfnum, ctrl->info.selector,
> > +                          buf, sizeof(*buf));
>
> Now for the real annoying question. I've encountered quite a few devices
> that would crash when the driver tried to get/set lots of controls at
> probe time. This is why the control cache is populated the first time a
> control is queried, and not when the device is probed. I'm always
> worried when adding more control accesses at probe time that some
> devices will behave improperly.

If we encounter a device like that we could quirk it.

>
> Given the number of UVC users I tend to be on the conservative side, but
> obviously, if we were to strictly avoid new access patterns to the
> device, the driver wouldn't be able to evolve. I do like patches 4/5 and
> 5/5, so I'm tempted to take the risk and revert this series later if
> needed. That would likely make some other users unhappy if they rely on
> the heuristic.
>
> Another issue is how to get vendors to implement the power line
> frequency control correctly. With the series applied, vendors won't
> notice they're doing something wrong. Of course, they probably don't
> check the behaviour of their devices with Linux in the first place, so
> I'm not sure what we could do.

We can add the check to v4l2-compliance....

Although I would love to see a uvc-compliance tool. If the tool can be
easily run in windows/linux without a driver I guess ISP vendors will
run it to validate their code.
Right now there is no way to validate a usb camera beyond: it runs in
windows and in cheese.

>
> > +     if (!ret)
> > +             out_mapping = &uvc_ctrl_power_line_mapping_uvc15;
> > +
> > +end:
> > +     /* Restore initial value and add mapping. */
> > +     *buf = init_val;
> > +     uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> > +                    chain->dev->intfnum, ctrl->info.selector,
> > +                    buf, sizeof(*buf));
> >
> > -     return __uvc_ctrl_add_mapping(chain, ctrl,
> > -                                   &uvc_ctrl_power_line_mapping_uvc15);
> > +     kfree(buf);
> > +     return __uvc_ctrl_add_mapping(chain, ctrl, out_mapping);
> >  }
> >
> >  static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH 3/5] media: uvcvideo: Probe the PLF characteristics

[Laurent Pinchart]

Hi Ricardo,

On Mon, Jun 10, 2024 at 11:51:55PM +0200, Ricardo Ribalda wrote:
> On Mon, 10 Jun 2024 at 16:14, Laurent Pinchart wrote:
> > On Mon, Mar 18, 2024 at 11:55:25PM +0000, Ricardo Ribalda wrote:
> > > The UVC 1.5 standard defines 4 values for the PLF control: Off, 50Hz,
> > > 60Hz and Auto. But it does not clearly define if all the values must be
> > > implemented or not.
> > >
> > > Instead of just using the UVC version to determine what the PLF control
> > > can do, probe it.
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_ctrl.c | 54 +++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 50 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 67522143c6c85..9a0b81aca30d1 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -501,12 +501,58 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> > >  static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain,
> > >       struct uvc_control *ctrl, const struct uvc_control_mapping *mapping)
> > >  {
> > > +     const struct uvc_control_mapping *out_mapping =
> > > +                                     &uvc_ctrl_power_line_mapping_uvc11;
> > > +     u8 init_val;
> > > +     u8 *buf;
> >
> >         u8 *buf __free(kfree) = NULL;
> >
> > will simplify the exit paths.
> >
> > > +     int ret;
> > > +
> > > +     buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> > > +     if (!buf)
> > > +             return -ENOMEM;
> > > +
> > > +     /* Save the default PLF value, so we can restore it. */
> > > +     ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id,
> > > +                          chain->dev->intfnum, ctrl->info.selector,
> > > +                          buf, sizeof(*buf));
> >
> > That's the current value, not the default. Is that intended ?
> 
> Yes, the driver does not init the other controls to the default value.
> So I'd rather be consistent.

I'm fine with that, let's update the comment to "Save the current PLF
value".

> > > +     /* If we cannot read the control skip it. */
> > > +     if (ret) {
> > > +             kfree(buf);
> > > +             return ret;
> > > +     }
> > > +     init_val = *buf;
> > > +
> > > +     /* If PLF value cannot be set to off, it is limited. */
> > > +     *buf = V4L2_CID_POWER_LINE_FREQUENCY_DISABLED;
> > > +     ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> > > +                          chain->dev->intfnum, ctrl->info.selector,
> > > +                          buf, sizeof(*buf));
> > > +     if (ret) {
> > > +             out_mapping = &uvc_ctrl_power_line_mapping_limited;
> > > +             goto end;
> >
> > If setting the value failed you don't need to restore it, do you ?
> >
> > > +     }
> > > +
> > > +     /* UVC 1.1 does not define auto, we can exit. */
> > >       if (chain->dev->uvc_version < 0x150)
> > > -             return __uvc_ctrl_add_mapping(chain, ctrl,
> > > -                                           &uvc_ctrl_power_line_mapping_uvc11);
> > > +             goto end;
> > > +
> > > +     /* Check if the device supports auto. */
> > > +     *buf = V4L2_CID_POWER_LINE_FREQUENCY_AUTO;
> > > +     ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> > > +                          chain->dev->intfnum, ctrl->info.selector,
> > > +                          buf, sizeof(*buf));
> >
> > Now for the real annoying question. I've encountered quite a few devices
> > that would crash when the driver tried to get/set lots of controls at
> > probe time. This is why the control cache is populated the first time a
> > control is queried, and not when the device is probed. I'm always
> > worried when adding more control accesses at probe time that some
> > devices will behave improperly.
> 
> If we encounter a device like that we could quirk it.

Now we could place bets on what is less likely to scale, quirking
devices that have a bad PLF implementation, or quirking devices whose
firmware will crash when queried too much at probe time :-)

> > Given the number of UVC users I tend to be on the conservative side, but
> > obviously, if we were to strictly avoid new access patterns to the
> > device, the driver wouldn't be able to evolve. I do like patches 4/5 and
> > 5/5, so I'm tempted to take the risk and revert this series later if
> > needed. That would likely make some other users unhappy if they rely on
> > the heuristic.
> >
> > Another issue is how to get vendors to implement the power line
> > frequency control correctly. With the series applied, vendors won't
> > notice they're doing something wrong. Of course, they probably don't
> > check the behaviour of their devices with Linux in the first place, so
> > I'm not sure what we could do.
> 
> We can add the check to v4l2-compliance....
> 
> Although I would love to see a uvc-compliance tool. If the tool can be
> easily run in windows/linux without a driver I guess ISP vendors will
> run it to validate their code.

*without a driver* is doable with libusb but would be *lots* of work,
basically duplicating the whole uvcvideo driver in userspace. That's not
a project I would start, but it would be interesting.

> Right now there is no way to validate a usb camera beyond: it runs in
> windows and in cheese.

Isn't the USB-IF supposed to have a compliance test suite ? Given all
the broken devices we hear about, it must either not be very good, or
not be used by vendors.

I think a compliance tool based on top of the uvcvideo kernel driver
would already be a good step forward.

> > > +     if (!ret)
> > > +             out_mapping = &uvc_ctrl_power_line_mapping_uvc15;
> > > +
> > > +end:
> > > +     /* Restore initial value and add mapping. */
> > > +     *buf = init_val;
> > > +     uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> > > +                    chain->dev->intfnum, ctrl->info.selector,
> > > +                    buf, sizeof(*buf));
> > >
> > > -     return __uvc_ctrl_add_mapping(chain, ctrl,
> > > -                                   &uvc_ctrl_power_line_mapping_uvc15);
> > > +     kfree(buf);
> > > +     return __uvc_ctrl_add_mapping(chain, ctrl, out_mapping);
> > >  }
> > >
> > >  static const struct uvc_control_mapping uvc_ctrl_mappings[] = {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] media: uvcvideo: Probe the PLF characteristics

[Ricardo Ribalda]

Hi Laurent

On Tue, 11 Jun 2024 at 01:48, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Mon, Jun 10, 2024 at 11:51:55PM +0200, Ricardo Ribalda wrote:
> > On Mon, 10 Jun 2024 at 16:14, Laurent Pinchart wrote:
> > > On Mon, Mar 18, 2024 at 11:55:25PM +0000, Ricardo Ribalda wrote:
> > > > The UVC 1.5 standard defines 4 values for the PLF control: Off, 50Hz,
> > > > 60Hz and Auto. But it does not clearly define if all the values must be
> > > > implemented or not.
> > > >
> > > > Instead of just using the UVC version to determine what the PLF control
> > > > can do, probe it.
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_ctrl.c | 54 +++++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 50 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > index 67522143c6c85..9a0b81aca30d1 100644
> > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > @@ -501,12 +501,58 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> > > >  static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain,
> > > >       struct uvc_control *ctrl, const struct uvc_control_mapping *mapping)
> > > >  {
> > > > +     const struct uvc_control_mapping *out_mapping =
> > > > +                                     &uvc_ctrl_power_line_mapping_uvc11;
> > > > +     u8 init_val;
> > > > +     u8 *buf;
> > >
> > >         u8 *buf __free(kfree) = NULL;
> > >
> > > will simplify the exit paths.
> > >
> > > > +     int ret;
> > > > +
> > > > +     buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> > > > +     if (!buf)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     /* Save the default PLF value, so we can restore it. */
> > > > +     ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, ctrl->entity->id,
> > > > +                          chain->dev->intfnum, ctrl->info.selector,
> > > > +                          buf, sizeof(*buf));
> > >
> > > That's the current value, not the default. Is that intended ?
> >
> > Yes, the driver does not init the other controls to the default value.
> > So I'd rather be consistent.
>
> I'm fine with that, let's update the comment to "Save the current PLF
> value".

done in my tree, if there is a v3 I will fix it, otherwise you are
free to modify it if you want ;)


>
> > > > +     /* If we cannot read the control skip it. */
> > > > +     if (ret) {
> > > > +             kfree(buf);
> > > > +             return ret;
> > > > +     }
> > > > +     init_val = *buf;
> > > > +
> > > > +     /* If PLF value cannot be set to off, it is limited. */
> > > > +     *buf = V4L2_CID_POWER_LINE_FREQUENCY_DISABLED;
> > > > +     ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> > > > +                          chain->dev->intfnum, ctrl->info.selector,
> > > > +                          buf, sizeof(*buf));
> > > > +     if (ret) {
> > > > +             out_mapping = &uvc_ctrl_power_line_mapping_limited;
> > > > +             goto end;
> > >
> > > If setting the value failed you don't need to restore it, do you ?
> > >
> > > > +     }
> > > > +
> > > > +     /* UVC 1.1 does not define auto, we can exit. */
> > > >       if (chain->dev->uvc_version < 0x150)
> > > > -             return __uvc_ctrl_add_mapping(chain, ctrl,
> > > > -                                           &uvc_ctrl_power_line_mapping_uvc11);
> > > > +             goto end;
> > > > +
> > > > +     /* Check if the device supports auto. */
> > > > +     *buf = V4L2_CID_POWER_LINE_FREQUENCY_AUTO;
> > > > +     ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> > > > +                          chain->dev->intfnum, ctrl->info.selector,
> > > > +                          buf, sizeof(*buf));
> > >
> > > Now for the real annoying question. I've encountered quite a few devices
> > > that would crash when the driver tried to get/set lots of controls at
> > > probe time. This is why the control cache is populated the first time a
> > > control is queried, and not when the device is probed. I'm always
> > > worried when adding more control accesses at probe time that some
> > > devices will behave improperly.
> >
> > If we encounter a device like that we could quirk it.
>
> Now we could place bets on what is less likely to scale, quirking
> devices that have a bad PLF implementation, or quirking devices whose
> firmware will crash when queried too much at probe time :-)

:)

>
> > > Given the number of UVC users I tend to be on the conservative side, but
> > > obviously, if we were to strictly avoid new access patterns to the
> > > device, the driver wouldn't be able to evolve. I do like patches 4/5 and
> > > 5/5, so I'm tempted to take the risk and revert this series later if
> > > needed. That would likely make some other users unhappy if they rely on
> > > the heuristic.
> > >
> > > Another issue is how to get vendors to implement the power line
> > > frequency control correctly. With the series applied, vendors won't
> > > notice they're doing something wrong. Of course, they probably don't
> > > check the behaviour of their devices with Linux in the first place, so
> > > I'm not sure what we could do.
> >
> > We can add the check to v4l2-compliance....
> >
> > Although I would love to see a uvc-compliance tool. If the tool can be
> > easily run in windows/linux without a driver I guess ISP vendors will
> > run it to validate their code.
>
> *without a driver* is doable with libusb but would be *lots* of work,
> basically duplicating the whole uvcvideo driver in userspace. That's not
> a project I would start, but it would be interesting.
>
> > Right now there is no way to validate a usb camera beyond: it runs in
> > windows and in cheese.
>
> Isn't the USB-IF supposed to have a compliance test suite ? Given all
> the broken devices we hear about, it must either not be very good, or
> not be used by vendors.

At least there is no public compliance test suite, and if third
parties cannot validate the results the test is not very useful.

>
> I think a compliance tool based on top of the uvcvideo kernel driver
> would already be a good step forward.
>
> > > > +     if (!ret)
> > > > +             out_mapping = &uvc_ctrl_power_line_mapping_uvc15;
> > > > +
> > > > +end:
> > > > +     /* Restore initial value and add mapping. */
> > > > +     *buf = init_val;
> > > > +     uvc_query_ctrl(chain->dev, UVC_SET_CUR, ctrl->entity->id,
> > > > +                    chain->dev->intfnum, ctrl->info.selector,
> > > > +                    buf, sizeof(*buf));
> > > >
> > > > -     return __uvc_ctrl_add_mapping(chain, ctrl,
> > > > -                                   &uvc_ctrl_power_line_mapping_uvc15);
> > > > +     kfree(buf);
> > > > +     return __uvc_ctrl_add_mapping(chain, ctrl, out_mapping);
> > > >  }
> > > >
> > > >  static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>
> --
> Regards,
>
> Laurent Pinchart

Regards!

-- 
Ricardo Ribalda

[PATCH 4/5] media: uvcvideo: Cleanup version-specific mapping

[Ricardo Ribalda]

We do not have more version specific mappings. Let's remove this code
for now. It can be easily reverted later if needed.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 9a0b81aca30d1..41578ded1174e 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -851,14 +851,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 };
 
-static const struct uvc_control_mapping *uvc_ctrl_mappings_uvc11[] = {
-	NULL, /* Sentinel */
-};
-
-static const struct uvc_control_mapping *uvc_ctrl_mappings_uvc15[] = {
-	NULL, /* Sentinel */
-};
-
 /* ------------------------------------------------------------------------
  * Utility functions
  */
@@ -2660,7 +2652,6 @@ static void uvc_ctrl_prune_entity(struct uvc_device *dev,
 static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
 			       struct uvc_control *ctrl)
 {
-	const struct uvc_control_mapping **mappings;
 	unsigned int i;
 
 	/*
@@ -2726,18 +2717,6 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
 		    ctrl->info.selector == mapping->selector)
 			__uvc_ctrl_add_custom_mapping(chain, ctrl, mapping);
 	}
-
-	/* Finally process version-specific mappings. */
-	mappings = chain->dev->uvc_version < 0x0150
-		 ? uvc_ctrl_mappings_uvc11 : uvc_ctrl_mappings_uvc15;
-
-	for (i = 0; mappings[i]; ++i) {
-		const struct uvc_control_mapping *mapping = mappings[i];
-
-		if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
-		    ctrl->info.selector == mapping->selector)
-			__uvc_ctrl_add_custom_mapping(chain, ctrl, mapping);
-	}
 }
 
 /*

-- 
2.44.0.291.gc1ea87d7ee-goog

Re: [PATCH 4/5] media: uvcvideo: Cleanup version-specific mapping

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Mon, Mar 18, 2024 at 11:55:26PM +0000, Ricardo Ribalda wrote:
> We do not have more version specific mappings. Let's remove this code
> for now. It can be easily reverted later if needed.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 21 ---------------------
>  1 file changed, 21 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 9a0b81aca30d1..41578ded1174e 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -851,14 +851,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  };
>  
> -static const struct uvc_control_mapping *uvc_ctrl_mappings_uvc11[] = {
> -	NULL, /* Sentinel */
> -};
> -
> -static const struct uvc_control_mapping *uvc_ctrl_mappings_uvc15[] = {
> -	NULL, /* Sentinel */
> -};
> -
>  /* ------------------------------------------------------------------------
>   * Utility functions
>   */
> @@ -2660,7 +2652,6 @@ static void uvc_ctrl_prune_entity(struct uvc_device *dev,
>  static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
>  			       struct uvc_control *ctrl)
>  {
> -	const struct uvc_control_mapping **mappings;
>  	unsigned int i;
>  
>  	/*
> @@ -2726,18 +2717,6 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
>  		    ctrl->info.selector == mapping->selector)
>  			__uvc_ctrl_add_custom_mapping(chain, ctrl, mapping);
>  	}
> -
> -	/* Finally process version-specific mappings. */
> -	mappings = chain->dev->uvc_version < 0x0150
> -		 ? uvc_ctrl_mappings_uvc11 : uvc_ctrl_mappings_uvc15;
> -
> -	for (i = 0; mappings[i]; ++i) {
> -		const struct uvc_control_mapping *mapping = mappings[i];
> -
> -		if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
> -		    ctrl->info.selector == mapping->selector)
> -			__uvc_ctrl_add_custom_mapping(chain, ctrl, mapping);
> -	}
>  }
>  
>  /*

-- 
Regards,

Laurent Pinchart

[PATCH 5/5] media: uvcvideo: Remove PLF device quirking

[Ricardo Ribalda]

We can use heuristics to figure out the proper range of the control
instead of quirking every single device.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c   |   4 +-
 drivers/media/usb/uvc/uvc_driver.c | 122 -------------------------------------
 drivers/media/usb/uvc/uvcvideo.h   |   2 -
 3 files changed, 2 insertions(+), 126 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 41578ded1174e..11ba1e8ee25b8 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -459,7 +459,7 @@ static void uvc_ctrl_set_rel_speed(struct uvc_control_mapping *mapping,
 	data[first+1] = min_t(int, abs(value), 0xff);
 }
 
-const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
+static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
 	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
 	.entity		= UVC_GUID_UVC_PROCESSING,
 	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
@@ -471,7 +471,7 @@ const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
 				  V4L2_CID_POWER_LINE_FREQUENCY_50HZ),
 };
 
-const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc11 = {
+static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc11 = {
 	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
 	.entity		= UVC_GUID_UVC_PROCESSING,
 	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index bbd90123a4e76..5f689fee60a9e 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2383,20 +2383,6 @@ MODULE_PARM_DESC(timeout, "Streaming control requests timeout");
  * Driver initialization and cleanup
  */
 
-static const struct uvc_device_info uvc_ctrl_power_line_limited = {
-	.mappings = (const struct uvc_control_mapping *[]) {
-		&uvc_ctrl_power_line_mapping_limited,
-		NULL, /* Sentinel */
-	},
-};
-
-static const struct uvc_device_info uvc_ctrl_power_line_uvc11 = {
-	.mappings = (const struct uvc_control_mapping *[]) {
-		&uvc_ctrl_power_line_mapping_uvc11,
-		NULL, /* Sentinel */
-	},
-};
-
 static const struct uvc_device_info uvc_quirk_probe_minmax = {
 	.quirks = UVC_QUIRK_PROBE_MINMAX,
 };
@@ -2427,33 +2413,6 @@ static const struct uvc_device_info uvc_quirk_force_y8 = {
  * though they are compliant.
  */
 static const struct usb_device_id uvc_ids[] = {
-	/* Quanta USB2.0 HD UVC Webcam */
-	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
-				| USB_DEVICE_ID_MATCH_INT_INFO,
-	  .idVendor		= 0x0408,
-	  .idProduct		= 0x3090,
-	  .bInterfaceClass	= USB_CLASS_VIDEO,
-	  .bInterfaceSubClass	= 1,
-	  .bInterfaceProtocol	= 0,
-	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
-	/* Quanta USB2.0 HD UVC Webcam */
-	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
-				| USB_DEVICE_ID_MATCH_INT_INFO,
-	  .idVendor		= 0x0408,
-	  .idProduct		= 0x4030,
-	  .bInterfaceClass	= USB_CLASS_VIDEO,
-	  .bInterfaceSubClass	= 1,
-	  .bInterfaceProtocol	= 0,
-	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
-	/* Quanta USB2.0 HD UVC Webcam */
-	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
-				| USB_DEVICE_ID_MATCH_INT_INFO,
-	  .idVendor		= 0x0408,
-	  .idProduct		= 0x4034,
-	  .bInterfaceClass	= USB_CLASS_VIDEO,
-	  .bInterfaceSubClass	= 1,
-	  .bInterfaceProtocol	= UVC_PC_PROTOCOL_15,
-	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
 	/* LogiLink Wireless Webcam */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 				| USB_DEVICE_ID_MATCH_INT_INFO,
@@ -2583,42 +2542,6 @@ static const struct usb_device_id uvc_ids[] = {
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0,
 	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_RESTRICT_FRAME_RATE) },
-	/* Chicony EasyCamera */
-	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
-				| USB_DEVICE_ID_MATCH_INT_INFO,
-	  .idVendor		= 0x04f2,
-	  .idProduct		= 0xb5eb,
-	  .bInterfaceClass	= USB_CLASS_VIDEO,
-	  .bInterfaceSubClass	= 1,
-	  .bInterfaceProtocol	= 0,
-	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
-	/* Chicony Electronics Co., Ltd Integrated Camera */
-	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
-				| USB_DEVICE_ID_MATCH_INT_INFO,
-	  .idVendor		= 0x04f2,
-	  .idProduct		= 0xb67c,
-	  .bInterfaceClass	= USB_CLASS_VIDEO,
-	  .bInterfaceSubClass	= 1,
-	  .bInterfaceProtocol	= UVC_PC_PROTOCOL_15,
-	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_uvc11 },
-	/* Chicony EasyCamera */
-	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
-				| USB_DEVICE_ID_MATCH_INT_INFO,
-	  .idVendor		= 0x04f2,
-	  .idProduct		= 0xb6ba,
-	  .bInterfaceClass	= USB_CLASS_VIDEO,
-	  .bInterfaceSubClass	= 1,
-	  .bInterfaceProtocol	= 0,
-	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
-	/* Chicony EasyCamera */
-	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
-				| USB_DEVICE_ID_MATCH_INT_INFO,
-	  .idVendor		= 0x04f2,
-	  .idProduct		= 0xb746,
-	  .bInterfaceClass	= USB_CLASS_VIDEO,
-	  .bInterfaceSubClass	= 1,
-	  .bInterfaceProtocol	= 0,
-	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
 	/* Alcor Micro AU3820 (Future Boy PC USB Webcam) */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 				| USB_DEVICE_ID_MATCH_INT_INFO,
@@ -3003,51 +2926,6 @@ static const struct usb_device_id uvc_ids[] = {
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0,
 	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_FORCE_BPP) },
-	/* SunplusIT Inc HD Camera */
-	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
-				| USB_DEVICE_ID_MATCH_INT_INFO,
-	  .idVendor		= 0x2b7e,
-	  .idProduct		= 0xb752,
-	  .bInterfaceClass	= USB_CLASS_VIDEO,
-	  .bInterfaceSubClass	= 1,
-	  .bInterfaceProtocol	= UVC_PC_PROTOCOL_15,
-	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_uvc11 },
-	/* Lenovo Integrated Camera */
-	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
-				| USB_DEVICE_ID_MATCH_INT_INFO,
-	  .idVendor		= 0x30c9,
-	  .idProduct		= 0x0093,
-	  .bInterfaceClass	= USB_CLASS_VIDEO,
-	  .bInterfaceSubClass	= 1,
-	  .bInterfaceProtocol	= UVC_PC_PROTOCOL_15,
-	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_uvc11 },
-	/* Sonix Technology USB 2.0 Camera */
-	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
-				| USB_DEVICE_ID_MATCH_INT_INFO,
-	  .idVendor		= 0x3277,
-	  .idProduct		= 0x0072,
-	  .bInterfaceClass	= USB_CLASS_VIDEO,
-	  .bInterfaceSubClass	= 1,
-	  .bInterfaceProtocol	= 0,
-	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
-	/* Acer EasyCamera */
-	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
-				| USB_DEVICE_ID_MATCH_INT_INFO,
-	  .idVendor		= 0x5986,
-	  .idProduct		= 0x1172,
-	  .bInterfaceClass	= USB_CLASS_VIDEO,
-	  .bInterfaceSubClass	= 1,
-	  .bInterfaceProtocol	= 0,
-	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
-	/* Acer EasyCamera */
-	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
-				| USB_DEVICE_ID_MATCH_INT_INFO,
-	  .idVendor		= 0x5986,
-	  .idProduct		= 0x1180,
-	  .bInterfaceClass	= USB_CLASS_VIDEO,
-	  .bInterfaceSubClass	= 1,
-	  .bInterfaceProtocol	= 0,
-	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
 	/* Intel D410/ASR depth camera */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 				| USB_DEVICE_ID_MATCH_INT_INFO,
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 611350a82c37f..896bb7362fa27 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -751,8 +751,6 @@ int uvc_status_start(struct uvc_device *dev, gfp_t flags);
 void uvc_status_stop(struct uvc_device *dev);
 
 /* Controls */
-extern const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited;
-extern const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc11;
 extern const struct v4l2_subscribed_event_ops uvc_ctrl_sub_ev_ops;
 
 int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,

-- 
2.44.0.291.gc1ea87d7ee-goog

Re: [PATCH 5/5] media: uvcvideo: Remove PLF device quirking

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Mon, Mar 18, 2024 at 11:55:27PM +0000, Ricardo Ribalda wrote:
> We can use heuristics to figure out the proper range of the control
> instead of quirking every single device.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c   |   4 +-
>  drivers/media/usb/uvc/uvc_driver.c | 122 -------------------------------------
>  drivers/media/usb/uvc/uvcvideo.h   |   2 -
>  3 files changed, 2 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 41578ded1174e..11ba1e8ee25b8 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -459,7 +459,7 @@ static void uvc_ctrl_set_rel_speed(struct uvc_control_mapping *mapping,
>  	data[first+1] = min_t(int, abs(value), 0xff);
>  }
>  
> -const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
> +static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
>  	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
>  	.entity		= UVC_GUID_UVC_PROCESSING,
>  	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> @@ -471,7 +471,7 @@ const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
>  				  V4L2_CID_POWER_LINE_FREQUENCY_50HZ),
>  };
>  
> -const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc11 = {
> +static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc11 = {
>  	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
>  	.entity		= UVC_GUID_UVC_PROCESSING,
>  	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index bbd90123a4e76..5f689fee60a9e 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2383,20 +2383,6 @@ MODULE_PARM_DESC(timeout, "Streaming control requests timeout");
>   * Driver initialization and cleanup
>   */
>  
> -static const struct uvc_device_info uvc_ctrl_power_line_limited = {
> -	.mappings = (const struct uvc_control_mapping *[]) {
> -		&uvc_ctrl_power_line_mapping_limited,
> -		NULL, /* Sentinel */
> -	},

I think you can drop the mappings field from the uvc_device_info
structure, as well as the corresponding code in uvc_ctrl_init_ctrl().

> -};
> -
> -static const struct uvc_device_info uvc_ctrl_power_line_uvc11 = {
> -	.mappings = (const struct uvc_control_mapping *[]) {
> -		&uvc_ctrl_power_line_mapping_uvc11,
> -		NULL, /* Sentinel */
> -	},
> -};
> -
>  static const struct uvc_device_info uvc_quirk_probe_minmax = {
>  	.quirks = UVC_QUIRK_PROBE_MINMAX,
>  };
> @@ -2427,33 +2413,6 @@ static const struct uvc_device_info uvc_quirk_force_y8 = {
>   * though they are compliant.
>   */
>  static const struct usb_device_id uvc_ids[] = {
> -	/* Quanta USB2.0 HD UVC Webcam */
> -	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> -				| USB_DEVICE_ID_MATCH_INT_INFO,
> -	  .idVendor		= 0x0408,
> -	  .idProduct		= 0x3090,
> -	  .bInterfaceClass	= USB_CLASS_VIDEO,
> -	  .bInterfaceSubClass	= 1,
> -	  .bInterfaceProtocol	= 0,
> -	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
> -	/* Quanta USB2.0 HD UVC Webcam */
> -	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> -				| USB_DEVICE_ID_MATCH_INT_INFO,
> -	  .idVendor		= 0x0408,
> -	  .idProduct		= 0x4030,
> -	  .bInterfaceClass	= USB_CLASS_VIDEO,
> -	  .bInterfaceSubClass	= 1,
> -	  .bInterfaceProtocol	= 0,
> -	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
> -	/* Quanta USB2.0 HD UVC Webcam */
> -	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> -				| USB_DEVICE_ID_MATCH_INT_INFO,
> -	  .idVendor		= 0x0408,
> -	  .idProduct		= 0x4034,
> -	  .bInterfaceClass	= USB_CLASS_VIDEO,
> -	  .bInterfaceSubClass	= 1,
> -	  .bInterfaceProtocol	= UVC_PC_PROTOCOL_15,
> -	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
>  	/* LogiLink Wireless Webcam */
>  	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
>  				| USB_DEVICE_ID_MATCH_INT_INFO,
> @@ -2583,42 +2542,6 @@ static const struct usb_device_id uvc_ids[] = {
>  	  .bInterfaceSubClass	= 1,
>  	  .bInterfaceProtocol	= 0,
>  	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_RESTRICT_FRAME_RATE) },
> -	/* Chicony EasyCamera */
> -	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> -				| USB_DEVICE_ID_MATCH_INT_INFO,
> -	  .idVendor		= 0x04f2,
> -	  .idProduct		= 0xb5eb,
> -	  .bInterfaceClass	= USB_CLASS_VIDEO,
> -	  .bInterfaceSubClass	= 1,
> -	  .bInterfaceProtocol	= 0,
> -	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
> -	/* Chicony Electronics Co., Ltd Integrated Camera */
> -	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> -				| USB_DEVICE_ID_MATCH_INT_INFO,
> -	  .idVendor		= 0x04f2,
> -	  .idProduct		= 0xb67c,
> -	  .bInterfaceClass	= USB_CLASS_VIDEO,
> -	  .bInterfaceSubClass	= 1,
> -	  .bInterfaceProtocol	= UVC_PC_PROTOCOL_15,
> -	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_uvc11 },
> -	/* Chicony EasyCamera */
> -	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> -				| USB_DEVICE_ID_MATCH_INT_INFO,
> -	  .idVendor		= 0x04f2,
> -	  .idProduct		= 0xb6ba,
> -	  .bInterfaceClass	= USB_CLASS_VIDEO,
> -	  .bInterfaceSubClass	= 1,
> -	  .bInterfaceProtocol	= 0,
> -	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
> -	/* Chicony EasyCamera */
> -	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> -				| USB_DEVICE_ID_MATCH_INT_INFO,
> -	  .idVendor		= 0x04f2,
> -	  .idProduct		= 0xb746,
> -	  .bInterfaceClass	= USB_CLASS_VIDEO,
> -	  .bInterfaceSubClass	= 1,
> -	  .bInterfaceProtocol	= 0,
> -	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
>  	/* Alcor Micro AU3820 (Future Boy PC USB Webcam) */
>  	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
>  				| USB_DEVICE_ID_MATCH_INT_INFO,
> @@ -3003,51 +2926,6 @@ static const struct usb_device_id uvc_ids[] = {
>  	  .bInterfaceSubClass	= 1,
>  	  .bInterfaceProtocol	= 0,
>  	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_FORCE_BPP) },
> -	/* SunplusIT Inc HD Camera */
> -	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> -				| USB_DEVICE_ID_MATCH_INT_INFO,
> -	  .idVendor		= 0x2b7e,
> -	  .idProduct		= 0xb752,
> -	  .bInterfaceClass	= USB_CLASS_VIDEO,
> -	  .bInterfaceSubClass	= 1,
> -	  .bInterfaceProtocol	= UVC_PC_PROTOCOL_15,
> -	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_uvc11 },
> -	/* Lenovo Integrated Camera */
> -	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> -				| USB_DEVICE_ID_MATCH_INT_INFO,
> -	  .idVendor		= 0x30c9,
> -	  .idProduct		= 0x0093,
> -	  .bInterfaceClass	= USB_CLASS_VIDEO,
> -	  .bInterfaceSubClass	= 1,
> -	  .bInterfaceProtocol	= UVC_PC_PROTOCOL_15,
> -	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_uvc11 },
> -	/* Sonix Technology USB 2.0 Camera */
> -	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> -				| USB_DEVICE_ID_MATCH_INT_INFO,
> -	  .idVendor		= 0x3277,
> -	  .idProduct		= 0x0072,
> -	  .bInterfaceClass	= USB_CLASS_VIDEO,
> -	  .bInterfaceSubClass	= 1,
> -	  .bInterfaceProtocol	= 0,
> -	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
> -	/* Acer EasyCamera */
> -	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> -				| USB_DEVICE_ID_MATCH_INT_INFO,
> -	  .idVendor		= 0x5986,
> -	  .idProduct		= 0x1172,
> -	  .bInterfaceClass	= USB_CLASS_VIDEO,
> -	  .bInterfaceSubClass	= 1,
> -	  .bInterfaceProtocol	= 0,
> -	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
> -	/* Acer EasyCamera */
> -	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> -				| USB_DEVICE_ID_MATCH_INT_INFO,
> -	  .idVendor		= 0x5986,
> -	  .idProduct		= 0x1180,
> -	  .bInterfaceClass	= USB_CLASS_VIDEO,
> -	  .bInterfaceSubClass	= 1,
> -	  .bInterfaceProtocol	= 0,
> -	  .driver_info		= (kernel_ulong_t)&uvc_ctrl_power_line_limited },
>  	/* Intel D410/ASR depth camera */
>  	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
>  				| USB_DEVICE_ID_MATCH_INT_INFO,
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 611350a82c37f..896bb7362fa27 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -751,8 +751,6 @@ int uvc_status_start(struct uvc_device *dev, gfp_t flags);
>  void uvc_status_stop(struct uvc_device *dev);
>  
>  /* Controls */
> -extern const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited;
> -extern const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc11;
>  extern const struct v4l2_subscribed_event_ops uvc_ctrl_sub_ev_ops;
>  
>  int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/5] media: uvc: Probe PLF limits at start-up

[Sergey Senozhatsky]

On (24/03/18 23:55), Ricardo Ribalda wrote:
> 
> The UVC standard descries the values for the PLF control for its
> versions 1.1 and and 1.5, but it does not describe which values MUST be
> implemented.
> 
> So far, we have been adding "device quirks" to provide proper limits for
> the control, but this is failing to scale.
> 
> Add a function that during probe-time checks the capability of the
> control.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Looks good to me

FWIW
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>