[PATCH] uvcvideo: Apply flags from device to actual properties

[PATCH] uvcvideo: Apply flags from device to actual properties

[Edgar Thier]

Use flags the device exposes for UVC controls.

Signed-off-by: Edgar Thier <info@edgarthier.net>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index c2ee6e3..bc69e92 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1568,7 +1568,8 @@ int uvc_ctrl_set(struct uvc_video_chain *chain,
 				return ret;
 		}

-		ctrl->loaded = 1;
+		if (!(ctrl->info.flags && UVC_CTRL_FLAG_AUTO_UPDATE))
+			ctrl->loaded = 1;
 	}

 	/* Backup the current value in case we need to rollback later. */
@@ -1889,8 +1890,13 @@ int uvc_ctrl_restore_values(struct uvc_device *dev)
 static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
 	const struct uvc_control_info *info)
 {
+	u8 *data;
 	int ret = 0;

+	data = kmalloc(2, GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
 	ctrl->info = *info;
 	INIT_LIST_HEAD(&ctrl->info.mappings);

@@ -1904,6 +1910,23 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,

 	ctrl->initialized = 1;

+	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
+                         info->selector, data, 1);
+	if (ret < 0) {
+		uvc_trace(UVC_TRACE_CONTROL,
+			  "GET_INFO failed on control %pUl/%u (%d).\n",
+			  info->entity, info->selector, ret);
+	}
+	else {
+		ctrl->info.flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
+		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
+		    | (data[0] & UVC_CONTROL_CAP_GET ?
+		       UVC_CTRL_FLAG_GET_CUR : 0)
+		    | (data[0] & UVC_CONTROL_CAP_SET ?
+		       UVC_CTRL_FLAG_SET_CUR : 0)
+		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
+		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
+	}
 	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
 		"entity %u\n", ctrl->info.entity, ctrl->info.selector,
 		dev->udev->devpath, ctrl->entity->id);
@@ -1911,6 +1934,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
 done:
 	if (ret < 0)
 		kfree(ctrl->uvc_data);
+	kfree(data);
 	return ret;
 }

-- 
2.7.4

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Laurent Pinchart]

Hi Edgar,

Thank you for the patch.

On Tuesday 15 Aug 2017 12:59:47 Edgar Thier wrote:
> Use flags the device exposes for UVC controls.

In addition to explaining what the patch does, the commit message should 
explain why the change is needed. What is the problem you're trying to address 
here ?

> Signed-off-by: Edgar Thier <info@edgarthier.net>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> b/drivers/media/usb/uvc/uvc_ctrl.c index c2ee6e3..bc69e92 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1568,7 +1568,8 @@ int uvc_ctrl_set(struct uvc_video_chain *chain,
>  				return ret;
>  		}
> 
> -		ctrl->loaded = 1;
> +		if (!(ctrl->info.flags && UVC_CTRL_FLAG_AUTO_UPDATE))
> +			ctrl->loaded = 1;

I think this change is unrelated to the subject of this patch. It should be 
split to a separate patch.

This being said, why is this change needed ? The uvc_ctrl_set() function is 
called from uvc_ctrl_s_ctrl() and uvc_ioctl_s_try_ext_ctrls(), and followed by 
a uvc_ctrl_rollback() or uvc_ctrl_commit() call that will set the loaded flag 
back to 0 in uvc_ctrl_commit_entity().

>  	}
> 
>  	/* Backup the current value in case we need to rollback later. */
> @@ -1889,8 +1890,13 @@ int uvc_ctrl_restore_values(struct uvc_device *dev)
>  static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control
> *ctrl, const struct uvc_control_info *info)
>  {
> +	u8 *data;
>  	int ret = 0;
> 
> +	data = kmalloc(2, GFP_KERNEL);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
>  	ctrl->info = *info;
>  	INIT_LIST_HEAD(&ctrl->info.mappings);
> 
> @@ -1904,6 +1910,23 @@ static int uvc_ctrl_add_info(struct uvc_device *dev,
> struct uvc_control *ctrl,
> 
>  	ctrl->initialized = 1;
> 
> +	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev-
>intfnum,
> +                         info->selector, data, 1);
> +	if (ret < 0) {
> +		uvc_trace(UVC_TRACE_CONTROL,
> +			  "GET_INFO failed on control %pUl/%u (%d).\n",
> +			  info->entity, info->selector, ret);
> +	}
> +	else {
> +		ctrl->info.flags = UVC_CTRL_FLAG_GET_MIN | 
UVC_CTRL_FLAG_GET_MAX
> +		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> +		    | (data[0] & UVC_CONTROL_CAP_GET ?
> +		       UVC_CTRL_FLAG_GET_CUR : 0)
> +		    | (data[0] & UVC_CONTROL_CAP_SET ?
> +		       UVC_CTRL_FLAG_SET_CUR : 0)
> +		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> +		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +	}

This code seems copied from uvc_ctrl_fill_xu_info(). I'd rather move it to a 
function that can be called by both instead of duplicating it.

>  	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
>  		"entity %u\n", ctrl->info.entity, ctrl->info.selector,
>  		dev->udev->devpath, ctrl->entity->id);
> @@ -1911,6 +1934,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev,
> struct uvc_control *ctrl, done:
>  	if (ret < 0)
>  		kfree(ctrl->uvc_data);
> +	kfree(data);
>  	return ret;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Edgar Thier]

Use flags the device exposes for UVC controls.
This allows the device to define which property flags are set.

Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
the values of other properties (e.g. gain) can change in the camera.
Examining the flags ensures that the driver is aware of such properties.

Signed-off-by: Edgar Thier <info@edgarthier.net>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 58 +++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 18 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index c2ee6e3..6922c0cb 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1629,6 +1629,9 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
 	}
 }

+static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
+	const struct uvc_control_info *info);
+
 /*
  * Query control information (size and flags) for XU controls.
  */
@@ -1659,24 +1662,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,

 	info->size = le16_to_cpup((__le16 *)data);

-	/* Query the control information (GET_INFO) */
-	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
-			     info->selector, data, 1);
-	if (ret < 0) {
-		uvc_trace(UVC_TRACE_CONTROL,
-			  "GET_INFO failed on control %pUl/%u (%d).\n",
-			  info->entity, info->selector, ret);
-		goto done;
-	}
-
-	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
-		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
-		    | (data[0] & UVC_CONTROL_CAP_GET ?
-		       UVC_CTRL_FLAG_GET_CUR : 0)
-		    | (data[0] & UVC_CONTROL_CAP_SET ?
-		       UVC_CTRL_FLAG_SET_CUR : 0)
-		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
-		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
+	info->flags = uvc_ctrl_get_flags(dev, ctrl, info);

 	uvc_ctrl_fixup_xu_info(dev, ctrl, info);

@@ -1884,6 +1870,40 @@ int uvc_ctrl_restore_values(struct uvc_device *dev)
  */

 /*
+ * Retrieve flags for a given control
+ */
+static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
+	const struct uvc_control_info *info)
+{
+	u8 *data;
+	int ret = 0;
+	int flags = 0;
+
+	data = kmalloc(2, GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
+	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
+						 info->selector, data, 1);
+	if (ret < 0) {
+		uvc_trace(UVC_TRACE_CONTROL,
+				  "GET_INFO failed on control %pUl/%u (%d).\n",
+				  info->entity, info->selector, ret);
+	} else {
+		flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
+			| UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
+			| (data[0] & UVC_CONTROL_CAP_GET ?
+			   UVC_CTRL_FLAG_GET_CUR : 0)
+			| (data[0] & UVC_CONTROL_CAP_SET ?
+			   UVC_CTRL_FLAG_SET_CUR : 0)
+			| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
+			   UVC_CTRL_FLAG_AUTO_UPDATE : 0);
+	}
+	kfree(data);
+	return flags;
+}
+
+/*
  * Add control information to a given control.
  */
 static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
@@ -1902,6 +1922,8 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
 		goto done;
 	}

+	ctrl->info.flags = uvc_ctrl_get_flags(dev, ctrl, info);
+
 	ctrl->initialized = 1;

 	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
-- 
2.7.4

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Edgar Thier]

Hi all,

I was wondering if there are any problems with my latest patch or if it simply slipped through.
Feedback would be welcome.

Regards,

Edgar

On 08/18/2017 12:12 PM, Edgar Thier wrote:
> 
> Use flags the device exposes for UVC controls.
> This allows the device to define which property flags are set.
> 
> Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
> the values of other properties (e.g. gain) can change in the camera.
> Examining the flags ensures that the driver is aware of such properties.
> 
> Signed-off-by: Edgar Thier <info@edgarthier.net>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 58 +++++++++++++++++++++++++++-------------
>  1 file changed, 40 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index c2ee6e3..6922c0cb 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1629,6 +1629,9 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
>  	}
>  }
> 
> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
> +	const struct uvc_control_info *info);
> +
>  /*
>   * Query control information (size and flags) for XU controls.
>   */
> @@ -1659,24 +1662,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
> 
>  	info->size = le16_to_cpup((__le16 *)data);
> 
> -	/* Query the control information (GET_INFO) */
> -	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> -			     info->selector, data, 1);
> -	if (ret < 0) {
> -		uvc_trace(UVC_TRACE_CONTROL,
> -			  "GET_INFO failed on control %pUl/%u (%d).\n",
> -			  info->entity, info->selector, ret);
> -		goto done;
> -	}
> -
> -	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> -		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> -		    | (data[0] & UVC_CONTROL_CAP_GET ?
> -		       UVC_CTRL_FLAG_GET_CUR : 0)
> -		    | (data[0] & UVC_CONTROL_CAP_SET ?
> -		       UVC_CTRL_FLAG_SET_CUR : 0)
> -		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> -		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +	info->flags = uvc_ctrl_get_flags(dev, ctrl, info);
> 
>  	uvc_ctrl_fixup_xu_info(dev, ctrl, info);
> 
> @@ -1884,6 +1870,40 @@ int uvc_ctrl_restore_values(struct uvc_device *dev)
>   */
> 
>  /*
> + * Retrieve flags for a given control
> + */
> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
> +	const struct uvc_control_info *info)
> +{
> +	u8 *data;
> +	int ret = 0;
> +	int flags = 0;
> +
> +	data = kmalloc(2, GFP_KERNEL);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> +						 info->selector, data, 1);
> +	if (ret < 0) {
> +		uvc_trace(UVC_TRACE_CONTROL,
> +				  "GET_INFO failed on control %pUl/%u (%d).\n",
> +				  info->entity, info->selector, ret);
> +	} else {
> +		flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> +			| UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> +			| (data[0] & UVC_CONTROL_CAP_GET ?
> +			   UVC_CTRL_FLAG_GET_CUR : 0)
> +			| (data[0] & UVC_CONTROL_CAP_SET ?
> +			   UVC_CTRL_FLAG_SET_CUR : 0)
> +			| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> +			   UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +	}
> +	kfree(data);
> +	return flags;
> +}
> +
> +/*
>   * Add control information to a given control.
>   */
>  static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
> @@ -1902,6 +1922,8 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>  		goto done;
>  	}
> 
> +	ctrl->info.flags = uvc_ctrl_get_flags(dev, ctrl, info);
> +
>  	ctrl->initialized = 1;
> 
>  	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
> 

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Kieran Bingham]

Hi Edgar,

On 18/08/17 11:12, Edgar Thier wrote:
> 
> Use flags the device exposes for UVC controls.
> This allows the device to define which property flags are set.
> 
> Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
> the values of other properties (e.g. gain) can change in the camera.
> Examining the flags ensures that the driver is aware of such properties.
> 
> Signed-off-by: Edgar Thier <info@edgarthier.net>> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 58 +++++++++++++++++++++++++++-------------
>  1 file changed, 40 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index c2ee6e3..6922c0cb 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1629,6 +1629,9 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
>  	}
>  }
> 
> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
> +	const struct uvc_control_info *info);

Rather than forward declaring the function ... Could you put the function higher
in the module please?

> +
>  /*
>   * Query control information (size and flags) for XU controls.
>   */
> @@ -1659,24 +1662,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
> 
>  	info->size = le16_to_cpup((__le16 *)data);
> 
> -	/* Query the control information (GET_INFO) */
> -	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> -			     info->selector, data, 1);
> -	if (ret < 0) {
> -		uvc_trace(UVC_TRACE_CONTROL,
> -			  "GET_INFO failed on control %pUl/%u (%d).\n",
> -			  info->entity, info->selector, ret);
> -		goto done;
> -	}
> -
> -	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> -		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> -		    | (data[0] & UVC_CONTROL_CAP_GET ?
> -		       UVC_CTRL_FLAG_GET_CUR : 0)
> -		    | (data[0] & UVC_CONTROL_CAP_SET ?
> -		       UVC_CTRL_FLAG_SET_CUR : 0)
> -		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> -		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +	info->flags = uvc_ctrl_get_flags(dev, ctrl, info);
> 
>  	uvc_ctrl_fixup_xu_info(dev, ctrl, info);
> 
> @@ -1884,6 +1870,40 @@ int uvc_ctrl_restore_values(struct uvc_device *dev)
>   */
> 
>  /*
> + * Retrieve flags for a given control
> + */
> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
> +	const struct uvc_control_info *info)
> +{
> +	u8 *data;
> +	int ret = 0;
> +	int flags = 0;
> +
> +	data = kmalloc(2, GFP_KERNEL);

kmalloc seems a bit of an overhead for 2 bytes (only one of which is used).
Can this use local stack storage?

(Laurent, looks like you originally wrote the code that did that, was there a
reason for the kmalloc for 2 bytes?)

> +	if (data == NULL)
> +		return -ENOMEM;

This will set the callers 'flags' to -ENOMEM ? Is that desired?

Of course removing the kmalloc will fix that anyway ...


> +
> +	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> +						 info->selector, data, 1);
> +	if (ret < 0) {
> +		uvc_trace(UVC_TRACE_CONTROL,
> +				  "GET_INFO failed on control %pUl/%u (%d).\n",
> +				  info->entity, info->selector, ret);
> +	} else {
> +		flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> +			| UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> +			| (data[0] & UVC_CONTROL_CAP_GET ?
> +			   UVC_CTRL_FLAG_GET_CUR : 0)
> +			| (data[0] & UVC_CONTROL_CAP_SET ?
> +			   UVC_CTRL_FLAG_SET_CUR : 0)
> +			| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> +			   UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +	}
> +	kfree(data);
> +	return flags;
> +}
> +
> +/*
>   * Add control information to a given control.
>   */
>  static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
> @@ -1902,6 +1922,8 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>  		goto done;
>  	}
> 
> +	ctrl->info.flags = uvc_ctrl_get_flags(dev, ctrl, info);
> +
>  	ctrl->initialized = 1;
> 
>  	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
> 

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Kieran Bingham]

Hi Edgar,

On 05/10/17 10:43, Kieran Bingham wrote:
> Hi Edgar,
> 
> On 18/08/17 11:12, Edgar Thier wrote:
>>
>> Use flags the device exposes for UVC controls.
>> This allows the device to define which property flags are set.
>>
>> Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
>> the values of other properties (e.g. gain) can change in the camera.
>> Examining the flags ensures that the driver is aware of such properties.
>>
>> Signed-off-by: Edgar Thier <info@edgarthier.net>> ---
>>  drivers/media/usb/uvc/uvc_ctrl.c | 58 +++++++++++++++++++++++++++-------------
>>  1 file changed, 40 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
>> index c2ee6e3..6922c0cb 100644
>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
>> @@ -1629,6 +1629,9 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
>>  	}
>>  }
>>
>> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
>> +	const struct uvc_control_info *info);
> 
> Rather than forward declaring the function ... Could you put the function higher
> in the module please?
> 
>> +
>>  /*
>>   * Query control information (size and flags) for XU controls.
>>   */
>> @@ -1659,24 +1662,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
>>
>>  	info->size = le16_to_cpup((__le16 *)data);
>>
>> -	/* Query the control information (GET_INFO) */
>> -	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
>> -			     info->selector, data, 1);
>> -	if (ret < 0) {
>> -		uvc_trace(UVC_TRACE_CONTROL,
>> -			  "GET_INFO failed on control %pUl/%u (%d).\n",
>> -			  info->entity, info->selector, ret);
>> -		goto done;
>> -	}
>> -
>> -	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
>> -		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
>> -		    | (data[0] & UVC_CONTROL_CAP_GET ?
>> -		       UVC_CTRL_FLAG_GET_CUR : 0)
>> -		    | (data[0] & UVC_CONTROL_CAP_SET ?
>> -		       UVC_CTRL_FLAG_SET_CUR : 0)
>> -		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
>> -		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
>> +	info->flags = uvc_ctrl_get_flags(dev, ctrl, info);
>>
>>  	uvc_ctrl_fixup_xu_info(dev, ctrl, info);
>>
>> @@ -1884,6 +1870,40 @@ int uvc_ctrl_restore_values(struct uvc_device *dev)
>>   */
>>
>>  /*
>> + * Retrieve flags for a given control
>> + */
>> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
>> +	const struct uvc_control_info *info)
>> +{
>> +	u8 *data;
>> +	int ret = 0;
>> +	int flags = 0;
>> +
>> +	data = kmalloc(2, GFP_KERNEL);
> 
> kmalloc seems a bit of an overhead for 2 bytes (only one of which is used).
> Can this use local stack storage?
> 
> (Laurent, looks like you originally wrote the code that did that, was there a
> reason for the kmalloc for 2 bytes?)


Aha - OK, Just spoke with Laurent and - yes this is needed, as we can't DMA to
the stack :) - I hadn't realised the 'data' was being DMA'd ..

>> +	if (data == NULL)
>> +		return -ENOMEM;
> 
> This will set the callers 'flags' to -ENOMEM ? Is that desired?
> 
> Of course removing the kmalloc will fix that anyway ...

Perhaps we have to return an empty flags here, which is what we will return if
the uvc_query_ctrl() fails anyway.


>> +
>> +	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
>> +						 info->selector, data, 1);
>> +	if (ret < 0) {
>> +		uvc_trace(UVC_TRACE_CONTROL,
>> +				  "GET_INFO failed on control %pUl/%u (%d).\n",
>> +				  info->entity, info->selector, ret);
>> +	} else {
>> +		flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
>> +			| UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
>> +			| (data[0] & UVC_CONTROL_CAP_GET ?
>> +			   UVC_CTRL_FLAG_GET_CUR : 0)
>> +			| (data[0] & UVC_CONTROL_CAP_SET ?
>> +			   UVC_CTRL_FLAG_SET_CUR : 0)
>> +			| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
>> +			   UVC_CTRL_FLAG_AUTO_UPDATE : 0);
>> +	}
>> +	kfree(data);
>> +	return flags;
>> +}
>> +
>> +/*
>>   * Add control information to a given control.
>>   */
>>  static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>> @@ -1902,6 +1922,8 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>>  		goto done;
>>  	}
>>
>> +	ctrl->info.flags = uvc_ctrl_get_flags(dev, ctrl, info);
>> +
>>  	ctrl->initialized = 1;
>>
>>  	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
>>

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Edgar Thier]

Hi Kieran,

> Rather than forward declaring the function ... Could you put the function higher
> in the module please?

Will do. Patch will come as a reply shortly.


>>> +	if (data == NULL)
>>> +		return -ENOMEM;
>>
>> This will set the callers 'flags' to -ENOMEM ? Is that desired?
>>
>> Of course removing the kmalloc will fix that anyway ...
> Perhaps we have to return an empty flags here, which is what we will return if
> the uvc_query_ctrl() fails anyway.

I wanted to keep the changes to a minimum. So I didn't touch the return values.
Are there any checks done for -ENOMEM et al? I didn't see any.

-Edgar

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Edgar Thier]

Use flags the device exposes for UVC controls.
This allows the device to define which property flags are set.

Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
the values of other properties (e.g. gain) can change in the camera.
Examining the flags ensures that the driver is aware of such properties.

Signed-off-by: Edgar Thier <info@edgarthier.net>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 56 +++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 18 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 20397ab..5091086 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1630,6 +1630,41 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
 }

 /*
+ * Retrieve flags for a given control
+ */
+static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
+	const struct uvc_control_info *info)
+{
+	u8 *data;
+	int ret = 0;
+	int flags = 0;
+
+	data = kmalloc(2, GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
+	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
+						 info->selector, data, 1);
+	if (ret < 0) {
+		uvc_trace(UVC_TRACE_CONTROL,
+				  "GET_INFO failed on control %pUl/%u (%d).\n",
+				  info->entity, info->selector, ret);
+	} else {
+		flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
+			| UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
+			| (data[0] & UVC_CONTROL_CAP_GET ?
+			   UVC_CTRL_FLAG_GET_CUR : 0)
+			| (data[0] & UVC_CONTROL_CAP_SET ?
+			   UVC_CTRL_FLAG_SET_CUR : 0)
+			| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
+			   UVC_CTRL_FLAG_AUTO_UPDATE : 0);
+	}
+	kfree(data);
+	return flags;
+}
+
+
+/*
  * Query control information (size and flags) for XU controls.
  */
 static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
@@ -1659,24 +1694,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,

 	info->size = le16_to_cpup((__le16 *)data);

-	/* Query the control information (GET_INFO) */
-	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
-			     info->selector, data, 1);
-	if (ret < 0) {
-		uvc_trace(UVC_TRACE_CONTROL,
-			  "GET_INFO failed on control %pUl/%u (%d).\n",
-			  info->entity, info->selector, ret);
-		goto done;
-	}
-
-	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
-		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
-		    | (data[0] & UVC_CONTROL_CAP_GET ?
-		       UVC_CTRL_FLAG_GET_CUR : 0)
-		    | (data[0] & UVC_CONTROL_CAP_SET ?
-		       UVC_CTRL_FLAG_SET_CUR : 0)
-		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
-		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
+	info->flags = uvc_ctrl_get_flags(dev, ctrl, info);

 	uvc_ctrl_fixup_xu_info(dev, ctrl, info);

@@ -1902,6 +1920,8 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
 		goto done;
 	}

+	ctrl->info.flags = uvc_ctrl_get_flags(dev, ctrl, info);
+
 	ctrl->initialized = 1;

 	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
-- 
2.7.4

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Kieran Bingham]

Hi Edgar,

Thank you for the patch respin.

I'm still a bit concerned about that -ENOMEM though:

On 06/10/17 11:34, Edgar Thier wrote:
> 
> Use flags the device exposes for UVC controls.
> This allows the device to define which property flags are set.
> 
> Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
> the values of other properties (e.g. gain) can change in the camera.
> Examining the flags ensures that the driver is aware of such properties.
> 
> Signed-off-by: Edgar Thier <info@edgarthier.net>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 56 +++++++++++++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 20397ab..5091086 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1630,6 +1630,41 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
>  }
> 
>  /*
> + * Retrieve flags for a given control
> + */
> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
> +	const struct uvc_control_info *info)
> +{
> +	u8 *data;
> +	int ret = 0;
> +	int flags = 0;
> +
> +	data = kmalloc(2, GFP_KERNEL);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> +						 info->selector, data, 1);
> +	if (ret < 0) {
> +		uvc_trace(UVC_TRACE_CONTROL,
> +				  "GET_INFO failed on control %pUl/%u (%d).\n",
> +				  info->entity, info->selector, ret);
> +	} else {
> +		flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> +			| UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> +			| (data[0] & UVC_CONTROL_CAP_GET ?
> +			   UVC_CTRL_FLAG_GET_CUR : 0)
> +			| (data[0] & UVC_CONTROL_CAP_SET ?
> +			   UVC_CTRL_FLAG_SET_CUR : 0)
> +			| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> +			   UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +	}
> +	kfree(data);
> +	return flags;
> +}
> +
> +

(Minor nit) There's a double blank line there ...

> +/*
>   * Query control information (size and flags) for XU controls.
>   */
>  static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
> @@ -1659,24 +1694,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
> 
>  	info->size = le16_to_cpup((__le16 *)data);
> 
> -	/* Query the control information (GET_INFO) */
> -	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> -			     info->selector, data, 1);
> -	if (ret < 0) {
> -		uvc_trace(UVC_TRACE_CONTROL,
> -			  "GET_INFO failed on control %pUl/%u (%d).\n",
> -			  info->entity, info->selector, ret);
> -		goto done;
> -	}
> -
> -	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> -		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> -		    | (data[0] & UVC_CONTROL_CAP_GET ?
> -		       UVC_CTRL_FLAG_GET_CUR : 0)
> -		    | (data[0] & UVC_CONTROL_CAP_SET ?
> -		       UVC_CTRL_FLAG_SET_CUR : 0)
> -		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> -		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);

In this original code : info->flags is set based on the flags.
If the higher kmalloc failed, we wouldn't get here - and an error would have
already been returned to the caller.

> +	info->flags = uvc_ctrl_get_flags(dev, ctrl, info);

But now ... info->flags could be set to an error return code if
uvc_ctrl_get_flags fails.

That error code is not checked anywhere and could be mis-interpreted as 'real
(invalid) flags' in other code....

And at no point - have we presented an error message to state that these flags
are now set ... but completely invalid.


>  	uvc_ctrl_fixup_xu_info(dev, ctrl, info);
> 
> @@ -1902,6 +1920,8 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>  		goto done;
>  	}
> 
> +	ctrl->info.flags = uvc_ctrl_get_flags(dev, ctrl, info);
> +

And again here of course - now ctrl->info.flags could be set to -ENOMEM, or
rather -12 ... 0x(FFFFFFFF)FFFFFFF4

>  	ctrl->initialized = 1;
> 
>  	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "

Regards

Kieran

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Edgar Thier]

Use flags the device exposes for UVC controls.
This allows the device to define which property flags are set.

Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
the values of other properties (e.g. gain) can change in the camera.
Examining the flags ensures that the driver is aware of such properties.

Signed-off-by: Edgar Thier <info@edgarthier.net>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 65 ++++++++++++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 15 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 20397ab..7fbfeef 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1630,12 +1630,47 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
 }

 /*
+ * Retrieve flags for a given control
+ */
+static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
+	const struct uvc_control_info *info)
+{
+	u8 *data;
+	int ret = 0;
+	int flags = 0;
+
+	data = kmalloc(2, GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
+	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
+						 info->selector, data, 1);
+	if (ret < 0) {
+		uvc_trace(UVC_TRACE_CONTROL,
+				  "GET_INFO failed on control %pUl/%u (%d).\n",
+				  info->entity, info->selector, ret);
+	} else {
+		flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
+			| UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
+			| (data[0] & UVC_CONTROL_CAP_GET ?
+			   UVC_CTRL_FLAG_GET_CUR : 0)
+			| (data[0] & UVC_CONTROL_CAP_SET ?
+			   UVC_CTRL_FLAG_SET_CUR : 0)
+			| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
+			   UVC_CTRL_FLAG_AUTO_UPDATE : 0);
+	}
+	kfree(data);
+	return flags;
+}
+
+/*
  * Query control information (size and flags) for XU controls.
  */
 static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
 	const struct uvc_control *ctrl, struct uvc_control_info *info)
 {
 	u8 *data;
+	int flags;
 	int ret;

 	data = kmalloc(2, GFP_KERNEL);
@@ -1659,24 +1694,14 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,

 	info->size = le16_to_cpup((__le16 *)data);

-	/* Query the control information (GET_INFO) */
-	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
-			     info->selector, data, 1);
-	if (ret < 0) {
+	flags = uvc_ctrl_get_flags(dev, ctrl, info);
+
+	if (flags < 0) {
 		uvc_trace(UVC_TRACE_CONTROL,
-			  "GET_INFO failed on control %pUl/%u (%d).\n",
-			  info->entity, info->selector, ret);
+			  "Failed to retrieve flags (%d).\n", ret);
 		goto done;
 	}
-
-	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
-		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
-		    | (data[0] & UVC_CONTROL_CAP_GET ?
-		       UVC_CTRL_FLAG_GET_CUR : 0)
-		    | (data[0] & UVC_CONTROL_CAP_SET ?
-		       UVC_CTRL_FLAG_SET_CUR : 0)
-		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
-		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
+	info->flags = flags;

 	uvc_ctrl_fixup_xu_info(dev, ctrl, info);

@@ -1890,6 +1915,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
 	const struct uvc_control_info *info)
 {
 	int ret = 0;
+	int flags = 0;

 	ctrl->info = *info;
 	INIT_LIST_HEAD(&ctrl->info.mappings);
@@ -1902,6 +1928,15 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
 		goto done;
 	}

+	flags = uvc_ctrl_get_flags(dev, ctrl, info);
+
+	if (flags < 0) {
+		uvc_trace(UVC_TRACE_CONTROL,
+			  "Failed to retrieve flags (%d).\n", ret);
+	}
+
+	ctrl->info.flags = flags;
+
 	ctrl->initialized = 1;

 	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
-- 
2.7.4

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Kieran Bingham]

Hi Edgar,

Sorry for not replying to you yesterday on IRC, but by the time I got to reply
to the message you weren't online..

On 11/10/17 12:56, Edgar Thier wrote:
> 
> Use flags the device exposes for UVC controls.
> This allows the device to define which property flags are set.
> 
> Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
> the values of other properties (e.g. gain) can change in the camera.
> Examining the flags ensures that the driver is aware of such properties.
> 
> Signed-off-by: Edgar Thier <info@edgarthier.net>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 65 ++++++++++++++++++++++++++++++----------
>  1 file changed, 50 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 20397ab..7fbfeef 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1630,12 +1630,47 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
>  }
> 
>  /*
> + * Retrieve flags for a given control
> + */
> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
> +	const struct uvc_control_info *info)
> +{
> +	u8 *data;
> +	int ret = 0;
> +	int flags = 0;
> +
> +	data = kmalloc(2, GFP_KERNEL);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> +						 info->selector, data, 1);
> +	if (ret < 0) {
> +		uvc_trace(UVC_TRACE_CONTROL,
> +				  "GET_INFO failed on control %pUl/%u (%d).\n",
> +				  info->entity, info->selector, ret);
> +	} else {
> +		flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> +			| UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> +			| (data[0] & UVC_CONTROL_CAP_GET ?
> +			   UVC_CTRL_FLAG_GET_CUR : 0)
> +			| (data[0] & UVC_CONTROL_CAP_SET ?
> +			   UVC_CTRL_FLAG_SET_CUR : 0)
> +			| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> +			   UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +	}
> +	kfree(data);
> +	return flags;
> +}
> +
> +/*
>   * Query control information (size and flags) for XU controls.
>   */
>  static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
>  	const struct uvc_control *ctrl, struct uvc_control_info *info)
>  {
>  	u8 *data;
> +	int flags;
>  	int ret;
> 
>  	data = kmalloc(2, GFP_KERNEL);
> @@ -1659,24 +1694,14 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
> 
>  	info->size = le16_to_cpup((__le16 *)data);
> 
> -	/* Query the control information (GET_INFO) */
> -	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> -			     info->selector, data, 1);
> -	if (ret < 0) {
> +	flags = uvc_ctrl_get_flags(dev, ctrl, info);
> +
> +	if (flags < 0) {
>  		uvc_trace(UVC_TRACE_CONTROL,
> -			  "GET_INFO failed on control %pUl/%u (%d).\n",
> -			  info->entity, info->selector, ret);
> +			  "Failed to retrieve flags (%d).\n", ret);

I think we need to set 'ret' to be an error value here ?
ret = flags? or ret = -ENOMEM?

Otherwise in 'done:' ret will be the return value of the previous
uvc_query_ctrl() call.

>  		goto done;
>  	}> -
> -	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> -		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> -		    | (data[0] & UVC_CONTROL_CAP_GET ?
> -		       UVC_CTRL_FLAG_GET_CUR : 0)
> -		    | (data[0] & UVC_CONTROL_CAP_SET ?
> -		       UVC_CTRL_FLAG_SET_CUR : 0)
> -		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> -		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +	info->flags = flags;
> 
>  	uvc_ctrl_fixup_xu_info(dev, ctrl, info);
> 
> @@ -1890,6 +1915,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>  	const struct uvc_control_info *info)
>  {
>  	int ret = 0;
> +	int flags = 0;
> 
>  	ctrl->info = *info;
>  	INIT_LIST_HEAD(&ctrl->info.mappings);
> @@ -1902,6 +1928,15 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>  		goto done;
>  	}
> 
> +	flags = uvc_ctrl_get_flags(dev, ctrl, info);
> +

I think the if could hug the call, and remove this blank line.

> +	if (flags < 0) {
> +		uvc_trace(UVC_TRACE_CONTROL,
> +			  "Failed to retrieve flags (%d).\n", ret);
> +	}

An if statement shouldn't have braces if it has only a single statement.

> +
> +	ctrl->info.flags = flags;

This is still setting an error value into ctrl->info.flags in the event of a fault.

Perhaps it should be initialised to something and set only if flags >= 0 ?

> +
>  	ctrl->initialized = 1;
> 
>  	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "

Regards

Kieran

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Edgar Thier]

Use flags the device exposes for UVC controls.
This allows the device to define which property flags are set.

Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
the values of other properties (e.g. gain) can change in the camera.
Examining the flags ensures that the driver is aware of such properties.

Signed-off-by: Edgar Thier <info@edgarthier.net>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 64 ++++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 15 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 20397aba..8f902a41 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1629,6 +1629,40 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
 	}
 }

+/*
+ * Retrieve flags for a given control
+ */
+static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
+	const struct uvc_control_info *info)
+{
+	u8 *data;
+	int ret = 0;
+	int flags = 0;
+
+	data = kmalloc(2, GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
+	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
+						 info->selector, data, 1);
+	if (ret < 0) {
+		uvc_trace(UVC_TRACE_CONTROL,
+				  "GET_INFO failed on control %pUl/%u (%d).\n",
+				  info->entity, info->selector, ret);
+	} else {
+		flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
+			| UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
+			| (data[0] & UVC_CONTROL_CAP_GET ?
+			   UVC_CTRL_FLAG_GET_CUR : 0)
+			| (data[0] & UVC_CONTROL_CAP_SET ?
+			   UVC_CTRL_FLAG_SET_CUR : 0)
+			| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
+			   UVC_CTRL_FLAG_AUTO_UPDATE : 0);
+	}
+	kfree(data);
+	return flags;
+}
+
 /*
  * Query control information (size and flags) for XU controls.
  */
@@ -1636,6 +1670,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
 	const struct uvc_control *ctrl, struct uvc_control_info *info)
 {
 	u8 *data;
+	int flags;
 	int ret;

 	data = kmalloc(2, GFP_KERNEL);
@@ -1659,24 +1694,15 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,

 	info->size = le16_to_cpup((__le16 *)data);

-	/* Query the control information (GET_INFO) */
-	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
-			     info->selector, data, 1);
-	if (ret < 0) {
+	flags = uvc_ctrl_get_flags(dev, ctrl, info);
+
+	if (flags < 0) {
 		uvc_trace(UVC_TRACE_CONTROL,
-			  "GET_INFO failed on control %pUl/%u (%d).\n",
-			  info->entity, info->selector, ret);
+			  "Failed to retrieve flags (%d).\n", ret);
+ 		ret = flags;
 		goto done;
 	}
-
-	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
-		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
-		    | (data[0] & UVC_CONTROL_CAP_GET ?
-		       UVC_CTRL_FLAG_GET_CUR : 0)
-		    | (data[0] & UVC_CONTROL_CAP_SET ?
-		       UVC_CTRL_FLAG_SET_CUR : 0)
-		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
-		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
+	info->flags = flags;

 	uvc_ctrl_fixup_xu_info(dev, ctrl, info);

@@ -1890,6 +1916,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
 	const struct uvc_control_info *info)
 {
 	int ret = 0;
+	int flags = 0;

 	ctrl->info = *info;
 	INIT_LIST_HEAD(&ctrl->info.mappings);
@@ -1902,6 +1929,13 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
 		goto done;
 	}

+	flags = uvc_ctrl_get_flags(dev, ctrl, info);
+	if (flags < 0)
+		uvc_trace(UVC_TRACE_CONTROL,
+			  "Failed to retrieve flags (%d).\n", ret);
+	else
+		ctrl->info.flags = flags;
+
 	ctrl->initialized = 1;

 	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
-- 
2.14.2

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Edgar Thier]

Hi,

I was wondering if there are any problems with my latest patch or if it simply slipped through.

Regards,

Edgar

On 10/12/2017 09:54 AM, Edgar Thier wrote:
> 
> Use flags the device exposes for UVC controls.
> This allows the device to define which property flags are set.
> 
> Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
> the values of other properties (e.g. gain) can change in the camera.
> Examining the flags ensures that the driver is aware of such properties.
> 
> Signed-off-by: Edgar Thier <info@edgarthier.net>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 64 ++++++++++++++++++++++++++++++----------
>  1 file changed, 49 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 20397aba..8f902a41 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1629,6 +1629,40 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
>  	}
>  }
> 
> +/*
> + * Retrieve flags for a given control
> + */
> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
> +	const struct uvc_control_info *info)
> +{
> +	u8 *data;
> +	int ret = 0;
> +	int flags = 0;
> +
> +	data = kmalloc(2, GFP_KERNEL);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> +						 info->selector, data, 1);
> +	if (ret < 0) {
> +		uvc_trace(UVC_TRACE_CONTROL,
> +				  "GET_INFO failed on control %pUl/%u (%d).\n",
> +				  info->entity, info->selector, ret);
> +	} else {
> +		flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> +			| UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> +			| (data[0] & UVC_CONTROL_CAP_GET ?
> +			   UVC_CTRL_FLAG_GET_CUR : 0)
> +			| (data[0] & UVC_CONTROL_CAP_SET ?
> +			   UVC_CTRL_FLAG_SET_CUR : 0)
> +			| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> +			   UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +	}
> +	kfree(data);
> +	return flags;
> +}
> +
>  /*
>   * Query control information (size and flags) for XU controls.
>   */
> @@ -1636,6 +1670,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
>  	const struct uvc_control *ctrl, struct uvc_control_info *info)
>  {
>  	u8 *data;
> +	int flags;
>  	int ret;
> 
>  	data = kmalloc(2, GFP_KERNEL);
> @@ -1659,24 +1694,15 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
> 
>  	info->size = le16_to_cpup((__le16 *)data);
> 
> -	/* Query the control information (GET_INFO) */
> -	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> -			     info->selector, data, 1);
> -	if (ret < 0) {
> +	flags = uvc_ctrl_get_flags(dev, ctrl, info);
> +
> +	if (flags < 0) {
>  		uvc_trace(UVC_TRACE_CONTROL,
> -			  "GET_INFO failed on control %pUl/%u (%d).\n",
> -			  info->entity, info->selector, ret);
> +			  "Failed to retrieve flags (%d).\n", ret);
> + 		ret = flags;
>  		goto done;
>  	}
> -
> -	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> -		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> -		    | (data[0] & UVC_CONTROL_CAP_GET ?
> -		       UVC_CTRL_FLAG_GET_CUR : 0)
> -		    | (data[0] & UVC_CONTROL_CAP_SET ?
> -		       UVC_CTRL_FLAG_SET_CUR : 0)
> -		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> -		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +	info->flags = flags;
> 
>  	uvc_ctrl_fixup_xu_info(dev, ctrl, info);
> 
> @@ -1890,6 +1916,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>  	const struct uvc_control_info *info)
>  {
>  	int ret = 0;
> +	int flags = 0;
> 
>  	ctrl->info = *info;
>  	INIT_LIST_HEAD(&ctrl->info.mappings);
> @@ -1902,6 +1929,13 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>  		goto done;
>  	}
> 
> +	flags = uvc_ctrl_get_flags(dev, ctrl, info);
> +	if (flags < 0)
> +		uvc_trace(UVC_TRACE_CONTROL,
> +			  "Failed to retrieve flags (%d).\n", ret);
> +	else
> +		ctrl->info.flags = flags;
> +
>  	ctrl->initialized = 1;
> 
>  	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
> 

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Kieran Bingham]

Hi Edgar,

On 15/11/17 08:11, Edgar Thier wrote:
> Hi,
> 
> I was wondering if there are any problems with my latest patch or if it simply slipped through.

Slipped though in high mail volumes :)

I think it's easier to see updated patches if they are posted as a new thread,
with an increased version number. [PATCH v2], [PATCH v3] etc...

Not a problem now - but might help your updated patches get seen next time.

Looks like my concerns were addressed, so that's a Reviewed-by: tag from me.

--
Kieran

> 
> Regards,
> 
> Edgar
> 
> On 10/12/2017 09:54 AM, Edgar Thier wrote:
>>
>> Use flags the device exposes for UVC controls.
>> This allows the device to define which property flags are set.
>>
>> Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
>> the values of other properties (e.g. gain) can change in the camera.
>> Examining the flags ensures that the driver is aware of such properties.
>>
>> Signed-off-by: Edgar Thier <info@edgarthier.net>
>> ---
>>  drivers/media/usb/uvc/uvc_ctrl.c | 64 ++++++++++++++++++++++++++++++----------
>>  1 file changed, 49 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
>> index 20397aba..8f902a41 100644
>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
>> @@ -1629,6 +1629,40 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
>>  	}
>>  }
>>
>> +/*
>> + * Retrieve flags for a given control
>> + */
>> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
>> +	const struct uvc_control_info *info)
>> +{
>> +	u8 *data;
>> +	int ret = 0;
>> +	int flags = 0;
>> +
>> +	data = kmalloc(2, GFP_KERNEL);
>> +	if (data == NULL)
>> +		return -ENOMEM;
>> +
>> +	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
>> +						 info->selector, data, 1);
>> +	if (ret < 0) {
>> +		uvc_trace(UVC_TRACE_CONTROL,
>> +				  "GET_INFO failed on control %pUl/%u (%d).\n",
>> +				  info->entity, info->selector, ret);
>> +	} else {
>> +		flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
>> +			| UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
>> +			| (data[0] & UVC_CONTROL_CAP_GET ?
>> +			   UVC_CTRL_FLAG_GET_CUR : 0)
>> +			| (data[0] & UVC_CONTROL_CAP_SET ?
>> +			   UVC_CTRL_FLAG_SET_CUR : 0)
>> +			| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
>> +			   UVC_CTRL_FLAG_AUTO_UPDATE : 0);
>> +	}
>> +	kfree(data);
>> +	return flags;
>> +}
>> +
>>  /*
>>   * Query control information (size and flags) for XU controls.
>>   */
>> @@ -1636,6 +1670,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
>>  	const struct uvc_control *ctrl, struct uvc_control_info *info)
>>  {
>>  	u8 *data;
>> +	int flags;
>>  	int ret;
>>
>>  	data = kmalloc(2, GFP_KERNEL);
>> @@ -1659,24 +1694,15 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
>>
>>  	info->size = le16_to_cpup((__le16 *)data);
>>
>> -	/* Query the control information (GET_INFO) */
>> -	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
>> -			     info->selector, data, 1);
>> -	if (ret < 0) {
>> +	flags = uvc_ctrl_get_flags(dev, ctrl, info);
>> +
>> +	if (flags < 0) {
>>  		uvc_trace(UVC_TRACE_CONTROL,
>> -			  "GET_INFO failed on control %pUl/%u (%d).\n",
>> -			  info->entity, info->selector, ret);
>> +			  "Failed to retrieve flags (%d).\n", ret);
>> + 		ret = flags;
>>  		goto done;
>>  	}
>> -
>> -	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
>> -		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
>> -		    | (data[0] & UVC_CONTROL_CAP_GET ?
>> -		       UVC_CTRL_FLAG_GET_CUR : 0)
>> -		    | (data[0] & UVC_CONTROL_CAP_SET ?
>> -		       UVC_CTRL_FLAG_SET_CUR : 0)
>> -		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
>> -		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
>> +	info->flags = flags;
>>
>>  	uvc_ctrl_fixup_xu_info(dev, ctrl, info);
>>
>> @@ -1890,6 +1916,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>>  	const struct uvc_control_info *info)
>>  {
>>  	int ret = 0;
>> +	int flags = 0;
>>
>>  	ctrl->info = *info;
>>  	INIT_LIST_HEAD(&ctrl->info.mappings);
>> @@ -1902,6 +1929,13 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>>  		goto done;
>>  	}
>>
>> +	flags = uvc_ctrl_get_flags(dev, ctrl, info);
>> +	if (flags < 0)
>> +		uvc_trace(UVC_TRACE_CONTROL,
>> +			  "Failed to retrieve flags (%d).\n", ret);
>> +	else
>> +		ctrl->info.flags = flags;
>> +
>>  	ctrl->initialized = 1;
>>
>>  	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
>>

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Kieran Bingham]

Hi Edgar,

Thanks for addressing my concerns in this updated patch.

On 12/10/17 08:54, Edgar Thier wrote:
> 
> Use flags the device exposes for UVC controls.
> This allows the device to define which property flags are set.
> 
> Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
> the values of other properties (e.g. gain) can change in the camera.
> Examining the flags ensures that the driver is aware of such properties.
> 
> Signed-off-by: Edgar Thier <info@edgarthier.net>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 64 ++++++++++++++++++++++++++++++----------
>  1 file changed, 49 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 20397aba..8f902a41 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1629,6 +1629,40 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
>  	}
>  }
> 
> +/*
> + * Retrieve flags for a given control
> + */
> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
> +	const struct uvc_control_info *info)
> +{
> +	u8 *data;
> +	int ret = 0;
> +	int flags = 0;
> +
> +	data = kmalloc(2, GFP_KERNEL);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> +						 info->selector, data, 1);
> +	if (ret < 0) {
> +		uvc_trace(UVC_TRACE_CONTROL,
> +				  "GET_INFO failed on control %pUl/%u (%d).\n",
> +				  info->entity, info->selector, ret);
> +	} else {
> +		flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> +			| UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> +			| (data[0] & UVC_CONTROL_CAP_GET ?
> +			   UVC_CTRL_FLAG_GET_CUR : 0)
> +			| (data[0] & UVC_CONTROL_CAP_SET ?
> +			   UVC_CTRL_FLAG_SET_CUR : 0)
> +			| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> +			   UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +	}
> +	kfree(data);
> +	return flags;
> +}
> +
>  /*
>   * Query control information (size and flags) for XU controls.
>   */
> @@ -1636,6 +1670,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
>  	const struct uvc_control *ctrl, struct uvc_control_info *info)
>  {
>  	u8 *data;
> +	int flags;
>  	int ret;
> 
>  	data = kmalloc(2, GFP_KERNEL);
> @@ -1659,24 +1694,15 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
> 
>  	info->size = le16_to_cpup((__le16 *)data);
> 
> -	/* Query the control information (GET_INFO) */
> -	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> -			     info->selector, data, 1);
> -	if (ret < 0) {
> +	flags = uvc_ctrl_get_flags(dev, ctrl, info);
> +
> +	if (flags < 0) {
>  		uvc_trace(UVC_TRACE_CONTROL,
> -			  "GET_INFO failed on control %pUl/%u (%d).\n",
> -			  info->entity, info->selector, ret);
> +			  "Failed to retrieve flags (%d).\n", ret);
> + 		ret = flags;
>  		goto done;
>  	}
> -
> -	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> -		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> -		    | (data[0] & UVC_CONTROL_CAP_GET ?
> -		       UVC_CTRL_FLAG_GET_CUR : 0)
> -		    | (data[0] & UVC_CONTROL_CAP_SET ?
> -		       UVC_CTRL_FLAG_SET_CUR : 0)
> -		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> -		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +	info->flags = flags;
> 
>  	uvc_ctrl_fixup_xu_info(dev, ctrl, info);
> 
> @@ -1890,6 +1916,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>  	const struct uvc_control_info *info)
>  {
>  	int ret = 0;
> +	int flags = 0;
> 
>  	ctrl->info = *info;
>  	INIT_LIST_HEAD(&ctrl->info.mappings);
> @@ -1902,6 +1929,13 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>  		goto done;
>  	}
> 
> +	flags = uvc_ctrl_get_flags(dev, ctrl, info);
> +	if (flags < 0)
> +		uvc_trace(UVC_TRACE_CONTROL,
> +			  "Failed to retrieve flags (%d).\n", ret);
> +	else
> +		ctrl->info.flags = flags;
> +
>  	ctrl->initialized = 1;
> 
>  	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
> 

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Edgar Thier]

Hi Kieran,

> I think it's easier to see updated patches if they are posted as a new thread,
> with an increased version number. [PATCH v2], [PATCH v3] etc...
>
> Not a problem now - but might help your updated patches get seen next time.

I will keep that in mind for next time. :)

> Looks like my concerns were addressed, so that's a Reviewed-by: tag from me.

Thanks!

Regards,

Edgar

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Edgar Thier]

Hi,

Another month, another mail. Are there still issues keeping this from being merged?

Regards,

Edgar

On 11/15/2017 12:54 PM, Kieran Bingham wrote:
> Hi Edgar,
> 
> Thanks for addressing my concerns in this updated patch.
> 
> On 12/10/17 08:54, Edgar Thier wrote:
>>
>> Use flags the device exposes for UVC controls.
>> This allows the device to define which property flags are set.
>>
>> Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
>> the values of other properties (e.g. gain) can change in the camera.
>> Examining the flags ensures that the driver is aware of such properties.
>>
>> Signed-off-by: Edgar Thier <info@edgarthier.net>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
>> ---
>>  drivers/media/usb/uvc/uvc_ctrl.c | 64 ++++++++++++++++++++++++++++++----------
>>  1 file changed, 49 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
>> index 20397aba..8f902a41 100644
>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
>> @@ -1629,6 +1629,40 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
>>  	}
>>  }
>>
>> +/*
>> + * Retrieve flags for a given control
>> + */
>> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
>> +	const struct uvc_control_info *info)
>> +{
>> +	u8 *data;
>> +	int ret = 0;
>> +	int flags = 0;
>> +
>> +	data = kmalloc(2, GFP_KERNEL);
>> +	if (data == NULL)
>> +		return -ENOMEM;
>> +
>> +	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
>> +						 info->selector, data, 1);
>> +	if (ret < 0) {
>> +		uvc_trace(UVC_TRACE_CONTROL,
>> +				  "GET_INFO failed on control %pUl/%u (%d).\n",
>> +				  info->entity, info->selector, ret);
>> +	} else {
>> +		flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
>> +			| UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
>> +			| (data[0] & UVC_CONTROL_CAP_GET ?
>> +			   UVC_CTRL_FLAG_GET_CUR : 0)
>> +			| (data[0] & UVC_CONTROL_CAP_SET ?
>> +			   UVC_CTRL_FLAG_SET_CUR : 0)
>> +			| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
>> +			   UVC_CTRL_FLAG_AUTO_UPDATE : 0);
>> +	}
>> +	kfree(data);
>> +	return flags;
>> +}
>> +
>>  /*
>>   * Query control information (size and flags) for XU controls.
>>   */
>> @@ -1636,6 +1670,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
>>  	const struct uvc_control *ctrl, struct uvc_control_info *info)
>>  {
>>  	u8 *data;
>> +	int flags;
>>  	int ret;
>>
>>  	data = kmalloc(2, GFP_KERNEL);
>> @@ -1659,24 +1694,15 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
>>
>>  	info->size = le16_to_cpup((__le16 *)data);
>>
>> -	/* Query the control information (GET_INFO) */
>> -	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
>> -			     info->selector, data, 1);
>> -	if (ret < 0) {
>> +	flags = uvc_ctrl_get_flags(dev, ctrl, info);
>> +
>> +	if (flags < 0) {
>>  		uvc_trace(UVC_TRACE_CONTROL,
>> -			  "GET_INFO failed on control %pUl/%u (%d).\n",
>> -			  info->entity, info->selector, ret);
>> +			  "Failed to retrieve flags (%d).\n", ret);
>> + 		ret = flags;
>>  		goto done;
>>  	}
>> -
>> -	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
>> -		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
>> -		    | (data[0] & UVC_CONTROL_CAP_GET ?
>> -		       UVC_CTRL_FLAG_GET_CUR : 0)
>> -		    | (data[0] & UVC_CONTROL_CAP_SET ?
>> -		       UVC_CTRL_FLAG_SET_CUR : 0)
>> -		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
>> -		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
>> +	info->flags = flags;
>>
>>  	uvc_ctrl_fixup_xu_info(dev, ctrl, info);
>>
>> @@ -1890,6 +1916,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>>  	const struct uvc_control_info *info)
>>  {
>>  	int ret = 0;
>> +	int flags = 0;
>>
>>  	ctrl->info = *info;
>>  	INIT_LIST_HEAD(&ctrl->info.mappings);
>> @@ -1902,6 +1929,13 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>>  		goto done;
>>  	}
>>
>> +	flags = uvc_ctrl_get_flags(dev, ctrl, info);
>> +	if (flags < 0)
>> +		uvc_trace(UVC_TRACE_CONTROL,
>> +			  "Failed to retrieve flags (%d).\n", ret);
>> +	else
>> +		ctrl->info.flags = flags;
>> +
>>  	ctrl->initialized = 1;
>>
>>  	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
>>

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Laurent Pinchart]

Hi Edgar,

Thank you for the patch.

On Thursday, 12 October 2017 10:54:17 EET Edgar Thier wrote:
> Use flags the device exposes for UVC controls.
> This allows the device to define which property flags are set.
> 
> Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
> the values of other properties (e.g. gain) can change in the camera.
> Examining the flags ensures that the driver is aware of such properties.
> 
> Signed-off-by: Edgar Thier <info@edgarthier.net>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 64 +++++++++++++++++++++++++++----------
>  1 file changed, 49 insertions(+), 15 deletions(-)

Running scripts/checkpatch.pl on the patch produces the following warning and 
errors.

WARNING: line over 80 characters
#83: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1635:
+static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct 
uvc_control *ctrl,

ERROR: code indent should use tabs where possible
#140: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1702:
+ ^I^Iret = flags;$

WARNING: please, no space before tabs
#140: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1702:
+ ^I^Iret = flags;$

WARNING: please, no spaces at the start of a line
#140: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1702:
+ ^I^Iret = flags;$

The last three should be fixed. The first one can sometimes be considered a 
matter of taste, but in this case it's easy to fix so we should address it 
too.

> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> b/drivers/media/usb/uvc/uvc_ctrl.c index 20397aba..8f902a41 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1629,6 +1629,40 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device
> *dev, }
>  }
> 
> +/*
> + * Retrieve flags for a given control
> + */
> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct
> uvc_control *ctrl,
> +	const struct uvc_control_info *info)
> +{
> +	u8 *data;
> +	int ret = 0;

No need to initialize ret to 0.

> +	int flags = 0;
> +
> +	data = kmalloc(2, GFP_KERNEL);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> +						 info->selector, data, 1);
> +	if (ret < 0) {
> +		uvc_trace(UVC_TRACE_CONTROL,
> +				  "GET_INFO failed on control %pUl/%u (%d).\n",
> +				  info->entity, info->selector, ret);
> +	} else {
> +		flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> +			| UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> +			| (data[0] & UVC_CONTROL_CAP_GET ?
> +			   UVC_CTRL_FLAG_GET_CUR : 0)
> +			| (data[0] & UVC_CONTROL_CAP_SET ?
> +			   UVC_CTRL_FLAG_SET_CUR : 0)
> +			| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> +			   UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +	}
> +	kfree(data);
> +	return flags;

So in case of failure you will return 0, and the caller will ignore the 
failure. This changes the behaviour of uvc_ctrl_fill_xu_info() which I don't 
think is a good idea. For uvc_ctrl_add_info(), on the other hand, ignoring 
errors is probably good, as otherwise we could introduce regressions with 
devices that don't implement GET_INFO properly on standard controls (the 
driver currently doesn't query information for standard controls so doesn't 
catch those devices).

> +}
> +
>  /*
>   * Query control information (size and flags) for XU controls.
>   */
> @@ -1636,6 +1670,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device
> *dev, const struct uvc_control *ctrl, struct uvc_control_info *info)
>  {
>  	u8 *data;
> +	int flags;
>  	int ret;
> 
>  	data = kmalloc(2, GFP_KERNEL);
> @@ -1659,24 +1694,15 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device
> *dev,
> 
>  	info->size = le16_to_cpup((__le16 *)data);
> 
> -	/* Query the control information (GET_INFO) */
> -	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> -			     info->selector, data, 1);
> -	if (ret < 0) {
> +	flags = uvc_ctrl_get_flags(dev, ctrl, info);
> +

No need for a blank line here.

> +	if (flags < 0) {
>  		uvc_trace(UVC_TRACE_CONTROL,
> -			  "GET_INFO failed on control %pUl/%u (%d).\n",
> -			  info->entity, info->selector, ret);
> +			  "Failed to retrieve flags (%d).\n", ret);
> + 		ret = flags;
>  		goto done;
>  	}
> -
> -	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> -		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> -		    | (data[0] & UVC_CONTROL_CAP_GET ?
> -		       UVC_CTRL_FLAG_GET_CUR : 0)
> -		    | (data[0] & UVC_CONTROL_CAP_SET ?
> -		       UVC_CTRL_FLAG_SET_CUR : 0)
> -		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> -		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +	info->flags = flags;
> 
>  	uvc_ctrl_fixup_xu_info(dev, ctrl, info);
> 
> @@ -1890,6 +1916,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev,
> struct uvc_control *ctrl, const struct uvc_control_info *info)
>  {
>  	int ret = 0;
> +	int flags = 0;

There's no need to initialize the flags variable to 0.

> 
>  	ctrl->info = *info;
>  	INIT_LIST_HEAD(&ctrl->info.mappings);
> @@ -1902,6 +1929,13 @@ static int uvc_ctrl_add_info(struct uvc_device *dev,
> struct uvc_control *ctrl, goto done;
>  	}
> 
> +	flags = uvc_ctrl_get_flags(dev, ctrl, info);
> +	if (flags < 0)
> +		uvc_trace(UVC_TRACE_CONTROL,
> +			  "Failed to retrieve flags (%d).\n", ret);
> +	else
> +		ctrl->info.flags = flags;
> +
>  	ctrl->initialized = 1;
> 
>  	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "

All these are small issues. Let me try to address them, I'll send you an 
updated patch shortly.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Laurent Pinchart]

Hi Edgar,

A few more comments.

On Tuesday, 2 January 2018 22:07:24 EET Laurent Pinchart wrote:
> On Thursday, 12 October 2017 10:54:17 EET Edgar Thier wrote:
> > Use flags the device exposes for UVC controls.
> > This allows the device to define which property flags are set.
> > 
> > Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
> > the values of other properties (e.g. gain) can change in the camera.
> > Examining the flags ensures that the driver is aware of such properties.
> > 
> > Signed-off-by: Edgar Thier <info@edgarthier.net>
> > ---
> > 
> >  drivers/media/usb/uvc/uvc_ctrl.c | 64 +++++++++++++++++++++++++----------
> >  1 file changed, 49 insertions(+), 15 deletions(-)
> 
> Running scripts/checkpatch.pl on the patch produces the following warning
> and errors.
> 
> WARNING: line over 80 characters
> #83: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1635:
> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct
> uvc_control *ctrl,
> 
> ERROR: code indent should use tabs where possible
> #140: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1702:
> + ^I^Iret = flags;$
> 
> WARNING: please, no space before tabs
> #140: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1702:
> + ^I^Iret = flags;$
> 
> WARNING: please, no spaces at the start of a line
> #140: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1702:
> + ^I^Iret = flags;$
> 
> The last three should be fixed. The first one can sometimes be considered a
> matter of taste, but in this case it's easy to fix so we should address it
> too.
> 
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > b/drivers/media/usb/uvc/uvc_ctrl.c index 20397aba..8f902a41 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1629,6 +1629,40 @@ static void uvc_ctrl_fixup_xu_info(struct
> > uvc_device
> > *dev, }
> > 
> >  }
> > 
> > +/*
> > + * Retrieve flags for a given control
> > + */
> > +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct
> > uvc_control *ctrl,
> > +	const struct uvc_control_info *info)
> > +{
> > +	u8 *data;
> > +	int ret = 0;
> 
> No need to initialize ret to 0.
> 
> > +	int flags = 0;
> > +
> > +	data = kmalloc(2, GFP_KERNEL);

Isn't 1 byte enough ?

> > +	if (data == NULL)
> > +		return -ENOMEM;
> > +
> > +	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> > +						 info->selector, data, 1);

Indentation is incorrect here.

> > +	if (ret < 0) {
> > +		uvc_trace(UVC_TRACE_CONTROL,
> > +				  "GET_INFO failed on control %pUl/%u (%d).\n",
> > +				  info->entity, info->selector, ret);

And here too.

> > +	} else {
> > +		flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> > +			| UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> > +			| (data[0] & UVC_CONTROL_CAP_GET ?
> > +			   UVC_CTRL_FLAG_GET_CUR : 0)
> > +			| (data[0] & UVC_CONTROL_CAP_SET ?
> > +			   UVC_CTRL_FLAG_SET_CUR : 0)
> > +			| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> > +			   UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> > +	}
> > +	kfree(data);
> > +	return flags;
> 
> So in case of failure you will return 0, and the caller will ignore the
> failure. This changes the behaviour of uvc_ctrl_fill_xu_info() which I don't
> think is a good idea. For uvc_ctrl_add_info(), on the other hand, ignoring
> errors is probably good, as otherwise we could introduce regressions with
> devices that don't implement GET_INFO properly on standard controls (the
> driver currently doesn't query information for standard controls so doesn't
> catch those devices).
> 
> > +}
> > +
> >  /*
> >   * Query control information (size and flags) for XU controls.
> >   */
> > @@ -1636,6 +1670,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device
> > *dev, const struct uvc_control *ctrl, struct uvc_control_info *info)
> >  {
> >  	u8 *data;
> > +	int flags;
> >  	int ret;
> >  	
> >  	data = kmalloc(2, GFP_KERNEL);
> > @@ -1659,24 +1694,15 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device
> > *dev,
> > 
> >  	info->size = le16_to_cpup((__le16 *)data);
> > 
> > -	/* Query the control information (GET_INFO) */
> > -	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
> > -			     info->selector, data, 1);
> > -	if (ret < 0) {
> > +	flags = uvc_ctrl_get_flags(dev, ctrl, info);
> > +
> 
> No need for a blank line here.
> 
> > +	if (flags < 0) {
> >  		uvc_trace(UVC_TRACE_CONTROL,
> > -			  "GET_INFO failed on control %pUl/%u (%d).\n",
> > -			  info->entity, info->selector, ret);
> > +			  "Failed to retrieve flags (%d).\n", ret);
> > + 		ret = flags;
> >  		goto done;
> >  	}
> > -
> > -	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
> > -		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
> > -		    | (data[0] & UVC_CONTROL_CAP_GET ?
> > -		       UVC_CTRL_FLAG_GET_CUR : 0)
> > -		    | (data[0] & UVC_CONTROL_CAP_SET ?
> > -		       UVC_CTRL_FLAG_SET_CUR : 0)
> > -		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> > -		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> > +	info->flags = flags;
> > 
> >  	uvc_ctrl_fixup_xu_info(dev, ctrl, info);
> > 
> > @@ -1890,6 +1916,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev,
> > struct uvc_control *ctrl, const struct uvc_control_info *info)
> >  {
> >  	int ret = 0;
> > +	int flags = 0;
> 
> There's no need to initialize the flags variable to 0.
> 
> >  	ctrl->info = *info;
> >  	INIT_LIST_HEAD(&ctrl->info.mappings);
> > 
> > @@ -1902,6 +1929,13 @@ static int uvc_ctrl_add_info(struct uvc_device
> > *dev, struct uvc_control *ctrl,
> >  		goto done;
> >  	}
> > 
> > +	flags = uvc_ctrl_get_flags(dev, ctrl, info);
> > +	if (flags < 0)
> > +		uvc_trace(UVC_TRACE_CONTROL,
> > +			  "Failed to retrieve flags (%d).\n", ret);
> > +	else
> > +		ctrl->info.flags = flags;
> > +
> >  	ctrl->initialized = 1;
> >  	
> >  	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
> 
> All these are small issues. Let me try to address them, I'll send you an
> updated patch shortly.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Edgar Thier]

Hi Emmanuel,

>>> +	int flags = 0;
>>> +
>>> +	data = kmalloc(2, GFP_KERNEL);
> 
> Isn't 1 byte enough ?
> 

To quote from Kieran further up this thread:
>> kmalloc seems a bit of an overhead for 2 bytes (only one of which is used).
>> Can this use local stack storage?
>>
>> (Laurent, looks like you originally wrote the code that did that, was there a
>> reason for the kmalloc for 2 bytes?)
> Aha - OK, Just spoke with Laurent and - yes this is needed, as we can't DMA to
> the stack  - I hadn't realised the 'data' was being DMA'd ..

>>
>> All these are small issues. Let me try to address them, I'll send you an
>> updated patch shortly.

I'll be waiting.

Regards,

Edgar

Re: [PATCH] uvcvideo: Apply flags from device to actual properties

[Laurent Pinchart]

Hi Edgar,

On Wednesday, 3 January 2018 08:07:44 EET Edgar Thier wrote:
> Hi Emmanuel,

If we pick names randomly I'll call you David :-)

> >>> +	int flags = 0;
> >>> +
> >>> +	data = kmalloc(2, GFP_KERNEL);
> > 
> > Isn't 1 byte enough ?
> 
> To quote from Kieran further up this thread:
> 
> >> kmalloc seems a bit of an overhead for 2 bytes (only one of which is
> >> used). Can this use local stack storage?
> >> 
> >> (Laurent, looks like you originally wrote the code that did that, was
> >> there a reason for the kmalloc for 2 bytes?)
> > 
> > Aha - OK, Just spoke with Laurent and - yes this is needed, as we can't
> > DMA to the stack  - I hadn't realised the 'data' was being DMA'd ..

I don't dispute the fact that we need to kmalloc the memory, but I think we 
only need to kmalloc one byte, not two. The existing 2 bytes allocation comes 
from the size of the GET_LEN reply. Now that you only issue a GET_INFO here, 
one byte is enough.

> >> All these are small issues. Let me try to address them, I'll send you an
> >> updated patch shortly.
> 
> I'll be waiting.

-- 
Regards,

Laurent Pinchart

[PATCH] uvcvideo: Apply flags from device to actual properties

[Laurent Pinchart]

From: Edgar Thier <info@edgarthier.net>

Use flags the device exposes for UVC controls.
This allows the device to define which property flags are set.

Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
the values of other properties (e.g. gain) can change in the camera.
Examining the flags ensures that the driver is aware of such properties.

Signed-off-by: Edgar Thier <info@edgarthier.net>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 52 ++++++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 20397aba6849..586f0e94061b 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1590,6 +1590,36 @@ int uvc_ctrl_set(struct uvc_video_chain *chain,
  * Dynamic controls
  */
 
+/*
+ * Retrieve flags for a given control
+ */
+static int uvc_ctrl_get_flags(struct uvc_device *dev,
+			      const struct uvc_control *ctrl,
+			      struct uvc_control_info *info)
+{
+	u8 *data;
+	int ret;
+
+	data = kmalloc(1, GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
+	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
+			     info->selector, data, 1);
+	if (!ret)
+		info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
+			    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
+			    | (data[0] & UVC_CONTROL_CAP_GET ?
+			       UVC_CTRL_FLAG_GET_CUR : 0)
+			    | (data[0] & UVC_CONTROL_CAP_SET ?
+			       UVC_CTRL_FLAG_SET_CUR : 0)
+			    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
+			       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
+
+	kfree(data);
+	return ret;
+}
+
 static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
 	const struct uvc_control *ctrl, struct uvc_control_info *info)
 {
@@ -1659,25 +1689,14 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
 
 	info->size = le16_to_cpup((__le16 *)data);
 
-	/* Query the control information (GET_INFO) */
-	ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
-			     info->selector, data, 1);
+	ret = uvc_ctrl_get_flags(dev, ctrl, info);
 	if (ret < 0) {
 		uvc_trace(UVC_TRACE_CONTROL,
-			  "GET_INFO failed on control %pUl/%u (%d).\n",
+			  "Failed to get flags for control %pUl/%u (%d).\n",
 			  info->entity, info->selector, ret);
 		goto done;
 	}
 
-	info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
-		    | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
-		    | (data[0] & UVC_CONTROL_CAP_GET ?
-		       UVC_CTRL_FLAG_GET_CUR : 0)
-		    | (data[0] & UVC_CONTROL_CAP_SET ?
-		       UVC_CTRL_FLAG_SET_CUR : 0)
-		    | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
-		       UVC_CTRL_FLAG_AUTO_UPDATE : 0);
-
 	uvc_ctrl_fixup_xu_info(dev, ctrl, info);
 
 	uvc_trace(UVC_TRACE_CONTROL, "XU control %pUl/%u queried: len %u, "
@@ -1902,6 +1921,13 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
 		goto done;
 	}
 
+	/*
+	 * Retrieve control flags from the device. Ignore errors and work with
+	 * default flag values from the uvc_ctrl array when the device doesn't
+	 * properly implement GET_INFO on standard controls.
+	 */
+	uvc_ctrl_get_flags(dev, ctrl, &ctrl->info);
+
 	ctrl->initialized = 1;
 
 	uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
-- 
Regards,

Laurent Pinchart