uvcvideo: Apply flags from device to actual properties
Commit Message
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(-)
Comments
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 "
>
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 "
>
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 "
>>
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
@@ -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 "