[linux-uvc-devel] [PATCH] uvcvideo: Acer Integrated Camera 5986:055a (add UVC_QUIRK_FIX_BROKEN_CHAIN)

[Henrik Ingo]

Acer Integrated Camera 5986:055a (as found on many Lenovo E... laptops)
fails uvc_scan_chain(), but it was found that if the error is ignored
the camera works just fine. The problem is that the output terminal
baSourceID refers to an entity 6, while the entities list only contains
entities 1 to 4. This patch adds a new quirk to simply link the observed
entities as a chain 1->2->4->3.

Signed-off-by: Henrik Ingo <***@avoinelama.fi>
---
drivers/media/usb/uvc/uvc_driver.c | 30 ++++++++++++++++++++++++++++++
drivers/media/usb/uvc/uvcvideo.h | 1 +
2 files changed, 31 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index fae81ef0e0d2..9d6a3a020f25 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1841,6 +1841,19 @@ static int uvc_register_chains(struct uvc_device *dev)
return 0;
}

+static int uvc_quirk_fix_broken_chain(struct uvc_device *dev)
+{
+ struct uvc_entity *entity;
+ __u8 broken_id = 3;
+
+ entity = uvc_entity_by_id(dev, broken_id);
+ if (entity == NULL)
+ return -EINVAL;
+ *(entity->baSourceID) = 4;
+ return 0;
+}
+
+
/* ------------------------------------------------------------------------
* USB probe, disconnect, suspend and resume
*/
@@ -1927,6 +1940,14 @@ static int uvc_probe(struct usb_interface *intf,
if (uvc_ctrl_init_device(dev) < 0)
goto error;

+ if (dev->quirks & UVC_QUIRK_FIX_BROKEN_CHAIN) {
+ if (uvc_quirk_fix_broken_chain(dev) < 0)
+ uvc_trace(UVC_TRACE_PROBE,
+ "Warning: tried to fix a broken uvc chain "
+ "(UVC_QUIRK_FIX_BROKEN_CHAIN), but failed. "
+ "Will continue probing the device, "
+ "but it may fail later.\n");
+ }
/* Scan the device for video chains. */
if (uvc_scan_device(dev) < 0)
goto error;
@@ -2545,6 +2566,15 @@ static struct usb_device_id uvc_ids[] = {
.bInterfaceSubClass = 1,
.bInterfaceProtocol = 0,
.driver_info = UVC_QUIRK_FORCE_Y8 },
+ /* Acer Integrated Camera */
+ { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
+ | USB_DEVICE_ID_MATCH_INT_INFO,
+ .idVendor = 0x5986,
+ .idProduct = 0x055a,
+ .bInterfaceClass = USB_CLASS_VIDEO,
+ .bInterfaceSubClass = 1,
+ .bInterfaceProtocol = 0,
+ .driver_info = UVC_QUIRK_FIX_BROKEN_CHAIN },
/* Generic USB Video Class */
{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, 0) },
{}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 816dd1a0fd81..fa2c397e6326 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -152,6 +152,7 @@
#define UVC_QUIRK_RESTRICT_FRAME_RATE 0x00000200
#define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400
#define UVC_QUIRK_FORCE_Y8 0x00000800
+#define UVC_QUIRK_FIX_BROKEN_CHAIN 0x00001000

/* Format flags */
#define UVC_FMT_FLAG_COMPRESSED 0x00000001
--
2.5.0


------------------------------------------------------------------------------

[Laurent Pinchart]

Hi Henrik,

Thank you for the patch.

On Thursday 10 December 2015 14:50:17 Henrik Ingo wrote:
> Acer Integrated Camera 5986:055a (as found on many Lenovo E... laptops)
> fails uvc_scan_chain(), but it was found that if the error is ignored
> the camera works just fine. The problem is that the output terminal
> baSourceID refers to an entity 6, while the entities list only contains
> entities 1 to 4. This patch adds a new quirk to simply link the observed
> entities as a chain 1->2->4->3.

While I agree that we need to fix the problem, a quirk doesn't seem to be the
right solution. See https://www.mail-archive.com/linux-***@vger.kernel.org/msg87108.html for my thoughts on the topic.

> Signed-off-by: Henrik Ingo <***@avoinelama.fi>
> ---
> drivers/media/usb/uvc/uvc_driver.c | 30 ++++++++++++++++++++++++++++++
> drivers/media/usb/uvc/uvcvideo.h | 1 +
> 2 files changed, 31 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index fae81ef0e0d2..9d6a3a020f25
> 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1841,6 +1841,19 @@ static int uvc_register_chains(struct uvc_device
> *dev) return 0;
> }
>
> +static int uvc_quirk_fix_broken_chain(struct uvc_device *dev)
> +{
> + struct uvc_entity *entity;
> + __u8 broken_id = 3;
> +
> + entity = uvc_entity_by_id(dev, broken_id);
> + if (entity == NULL)
> + return -EINVAL;
> + *(entity->baSourceID) = 4;
> + return 0;
> +}
> +
> +
> /* ------------------------------------------------------------------------
> * USB probe, disconnect, suspend and resume
> */
> @@ -1927,6 +1940,14 @@ static int uvc_probe(struct usb_interface *intf,
> if (uvc_ctrl_init_device(dev) < 0)
> goto error;
>
> + if (dev->quirks & UVC_QUIRK_FIX_BROKEN_CHAIN) {
> + if (uvc_quirk_fix_broken_chain(dev) < 0)
> + uvc_trace(UVC_TRACE_PROBE,
> + "Warning: tried to fix a broken uvc chain "
> + "(UVC_QUIRK_FIX_BROKEN_CHAIN), but failed. "
> + "Will continue probing the device, "
> + "but it may fail later.\n");
> + }
> /* Scan the device for video chains. */
> if (uvc_scan_device(dev) < 0)
> goto error;
> @@ -2545,6 +2566,15 @@ static struct usb_device_id uvc_ids[] = {
> .bInterfaceSubClass = 1,
> .bInterfaceProtocol = 0,
> .driver_info = UVC_QUIRK_FORCE_Y8 },
> + /* Acer Integrated Camera */
> + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> + | USB_DEVICE_ID_MATCH_INT_INFO,
> + .idVendor = 0x5986,
> + .idProduct = 0x055a,
> + .bInterfaceClass = USB_CLASS_VIDEO,
> + .bInterfaceSubClass = 1,
> + .bInterfaceProtocol = 0,
> + .driver_info = UVC_QUIRK_FIX_BROKEN_CHAIN },
> /* Generic USB Video Class */
> { USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, 0) },
> {}
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 816dd1a0fd81..fa2c397e6326 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -152,6 +152,7 @@
> #define UVC_QUIRK_RESTRICT_FRAME_RATE 0x00000200
> #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400
> #define UVC_QUIRK_FORCE_Y8 0x00000800
> +#define UVC_QUIRK_FIX_BROKEN_CHAIN 0x00001000
>
> /* Format flags */
> #define UVC_FMT_FLAG_COMPRESSED 0x00000001

--
Regards,

Laurent Pinchart


------------------------------------------------------------------------------

[Henrik Ingo]

Some devices have invalid baSourceID references, causing uvc_scan_chain()
to fail, but if we just take the entities we can find and put them
together in the most sensible chain we can think of, turns out they do
work anyway. Note: This heuristic assumes there is a single chain.

At the time of writing, devices known to have such a broken chain are
- Acer Integrated Camera (5986:055a)
- Realtek rtl157a7 (0bda:57a7)

---

Based on feedback from Laurent, this is the second attempt at fixing
the Acer Integrated Camera but also any other cameras with the
same problem, now or in the future, in a generic way. I will send
a separate email on some design decisions taken.

Signed-off-by: Henrik Ingo <***@avoinelama.fi>
---
drivers/media/usb/uvc/uvc_driver.c | 88 +++++++++++++++++++++++++++++++++++++-
1 file changed, 86 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index fae81ef0e0d2..c30db277174f 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1575,6 +1575,82 @@ static const char *uvc_print_chain(struct uvc_video_chain *chain)
}

/*
+ * Fallback heuristic for devices that don't define valid chain, but seem to work anyway.
+ *
+ * Some devices have invalid baSourceID references, causing uvc_scan_chain()
+ * to fail, but if we just take the entities we can find and put them
+ * together in the most sensible chain we can think of, turns out they do
+ * work anyway. Note: This heuristic assumes there is a single chain.
+ *
+ * At the time of writing, devices known to have such a broken chain are
+ * - Acer Integrated Camera (5986:055a)
+ * - Realtek rtl157a7 (0bda:57a7)
+ */
+static int uvc_scan_fallback(struct uvc_device *dev)
+{
+ struct uvc_video_chain *chain;
+ struct uvc_entity *entity;
+
+ chain = kzalloc(sizeof(*chain), GFP_KERNEL);
+ if (chain == NULL)
+ return -ENOMEM;
+ INIT_LIST_HEAD(&chain->entities);
+ mutex_init(&chain->ctrl_mutex);
+ chain->dev = dev;
+ v4l2_prio_init(&chain->prio);
+
+ /* We take whatever entities that are there, and just
+ * put them in a list in the order:
+ * OT <- any XU and PU <- IT
+ */
+ list_for_each_entry(entity, &dev->entities, list) {
+ entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
+ if (UVC_ENTITY_IS_OTERM(entity)) {
+ if (uvc_scan_chain_entity(chain, entity) < 0) {
+ uvc_trace(UVC_TRACE_DESCR, "Fallback "
+ "scan for output terminal %d failed.\n",
+ entity->id);
+ return -EINVAL;
+ }
+ }
+ }
+ /* At this point we accept pretty much anything, but there
+ * absolutely must be at least one output terminal. */
+ if (list_empty(&chain->list)) {
+ kfree(chain);
+ return -EINVAL;
+ }
+ list_for_each_entry(entity, &dev->entities, list) {
+ entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
+ if (UVC_ENTITY_IS_UNIT(entity)) {
+ if (uvc_scan_chain_entity(chain, entity) < 0) {
+ uvc_trace(UVC_TRACE_DESCR, "Fallback "
+ "scan for entity %d failed.\n",
+ entity->id);
+ return -EINVAL;
+ }
+ }
+ }
+ list_for_each_entry(entity, &dev->entities, list) {
+ entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
+ if (UVC_ENTITY_IS_ITERM(entity)) {
+ if (uvc_scan_chain_entity(chain, entity) < 0) {
+ uvc_trace(UVC_TRACE_DESCR, "Fallback "
+ "scan for input terminal %d failed.\n",
+ entity->id);
+ return -EINVAL;
+ }
+ }
+ }
+
+ list_add_tail(&chain->list, &dev->chains);
+ uvc_trace(UVC_TRACE_PROBE, "Video chain found by fallback heuristic: "
+ "%s.\n", uvc_print_chain(chain));
+ return 0;
+}
+
+
+/*
* Scan the device for video chains and register video devices.
*
* Chains are scanned starting at their output terminals and walked backwards.
@@ -1619,8 +1695,16 @@ static int uvc_scan_device(struct uvc_device *dev)
}

if (list_empty(&dev->chains)) {
- uvc_printk(KERN_INFO, "No valid video chain found.\n");
- return -1;
+ uvc_trace(UVC_TRACE_PROBE, "No valid video chain found. "
+ "Start fallback heuristic.\n");
+ if (uvc_scan_fallback(dev) != 0) {
+ uvc_printk(KERN_INFO, "No valid video chain found.\n");
+ return -1;
+ }
+ uvc_printk(KERN_INFO, "Had trouble probing a valid video "
+ "chain, but fallback heuristic succeeded. "
+ "Usually it means this device will work "
+ "just fine despite errors that may follow.\n");
}

return 0;
--
2.5.0


------------------------------------------------------------------------------

[Henrik Ingo]

So, this is the 2nd attempt, based on earlier feedback from Laurent.
It is a generic approach: when uvc_scan_chain() fails, we just try to
put everything into a single chain anyway, and hope it works. No
quirks or product id's needed.

Compared to our earlier email exchange, some design decisions differ:

- New: An output terminal is required. If one is found, almost any
other breakage is ignored.

- All entities found are put into a single chain = we assume there is
only one chain.

- The list is built as:
All output terminals
All PU and XU units
All input terminals
The deviation from earlier discussion is twofold...

- Within each category, I said units would be in ID order. What I
meant to say was actually *arbitrary order*. It turns out the simplest
implementation is to just add them to the uvc_video_chain->entities in
the order they are found in uvc_device->entities. That's what the
patch does.

- PU and XU units are mixed together, and appear in the order they are
in uvc_device->entities

- All baSourceID are now completely ignored. It seems it doesn't
really matter what order we build the uvc_video_chain.entities list
anyway. I first thought I should somehow follow the path of baSourceID
references as much as possible, but now I don't see a reason why.
Also, even if I should have, it is not easy to work with a broken
chain, when I can't anticipate in advance all the possible ways such
references could be broken. So it seemed easiest to just ignore them
altogether. Note that immediately after this uvc_register_chains()
will still look at baSourceID and seems to do something with that
information, like setup references to the source unit(s). It prints a
KERN_INFO when it fails to do so for entity #3, but this is not fatal.
Hence, it seems everything works well pretty much regardless of what
order I added the entities in.

A corollary of the above point is that uvc_scan_chain_entity will
print the following line when tracing:
[ 326.040654] OT 3 <- PU 2 <- XU 4 <- IT 1

This is actually wrong, as the correct order (according to baSourceID)
for this device would be:
OT 3 <- [break] <- XU 4 <- PU 2 <- IT 1

Other than this trace print, with my superficial knowledge, I didn't
spot any other code where this approach would be a problem. In
particular, uvc_register_chains() seems to use the baSourceID's
correctly anyway.


Questions:

I have obviously tested this with the Acer camera. Is anybody in a
position to test it also with the similarly broken Realtek?

The heuristic is pretty general, and it's possible that we will now
handle things too optimistically. What if, due to this code, we
"succeed" in probing some device, but it doesn't actually work after
all? I don't know if the risk for this is very high, but wanted to
call out this question. What could happen in such a case?



The dmesg output is attached. First there's a normal boot, then a
rmmod/modprobe with trace=0xffff.

henrik


On Thu, Jan 7, 2016 at 11:01 PM, Henrik Ingo <***@avoinelama.fi> wrote:
> Some devices have invalid baSourceID references, causing uvc_scan_chain()
> to fail, but if we just take the entities we can find and put them
> together in the most sensible chain we can think of, turns out they do
> work anyway. Note: This heuristic assumes there is a single chain.
>
> At the time of writing, devices known to have such a broken chain are
> - Acer Integrated Camera (5986:055a)
> - Realtek rtl157a7 (0bda:57a7)
>
> ---
>
> Based on feedback from Laurent, this is the second attempt at fixing
> the Acer Integrated Camera but also any other cameras with the
> same problem, now or in the future, in a generic way. I will send
> a separate email on some design decisions taken.
>
> Signed-off-by: Henrik Ingo <***@avoinelama.fi>
> ---
> drivers/media/usb/uvc/uvc_driver.c | 88 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 86 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index fae81ef0e0d2..c30db277174f 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1575,6 +1575,82 @@ static const char *uvc_print_chain(struct uvc_video_chain *chain)
> }
>
> /*
> + * Fallback heuristic for devices that don't define valid chain, but seem to work anyway.
> + *
> + * Some devices have invalid baSourceID references, causing uvc_scan_chain()
> + * to fail, but if we just take the entities we can find and put them
> + * together in the most sensible chain we can think of, turns out they do
> + * work anyway. Note: This heuristic assumes there is a single chain.
> + *
> + * At the time of writing, devices known to have such a broken chain are
> + * - Acer Integrated Camera (5986:055a)
> + * - Realtek rtl157a7 (0bda:57a7)
> + */
> +static int uvc_scan_fallback(struct uvc_device *dev)
> +{
> + struct uvc_video_chain *chain;
> + struct uvc_entity *entity;
> +
> + chain = kzalloc(sizeof(*chain), GFP_KERNEL);
> + if (chain == NULL)
> + return -ENOMEM;
> + INIT_LIST_HEAD(&chain->entities);
> + mutex_init(&chain->ctrl_mutex);
> + chain->dev = dev;
> + v4l2_prio_init(&chain->prio);
> +
> + /* We take whatever entities that are there, and just
> + * put them in a list in the order:
> + * OT <- any XU and PU <- IT
> + */
> + list_for_each_entry(entity, &dev->entities, list) {
> + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
> + if (UVC_ENTITY_IS_OTERM(entity)) {
> + if (uvc_scan_chain_entity(chain, entity) < 0) {
> + uvc_trace(UVC_TRACE_DESCR, "Fallback "
> + "scan for output terminal %d failed.\n",
> + entity->id);
> + return -EINVAL;
> + }
> + }
> + }
> + /* At this point we accept pretty much anything, but there
> + * absolutely must be at least one output terminal. */
> + if (list_empty(&chain->list)) {
> + kfree(chain);
> + return -EINVAL;
> + }
> + list_for_each_entry(entity, &dev->entities, list) {
> + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
> + if (UVC_ENTITY_IS_UNIT(entity)) {
> + if (uvc_scan_chain_entity(chain, entity) < 0) {
> + uvc_trace(UVC_TRACE_DESCR, "Fallback "
> + "scan for entity %d failed.\n",
> + entity->id);
> + return -EINVAL;
> + }
> + }
> + }
> + list_for_each_entry(entity, &dev->entities, list) {
> + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
> + if (UVC_ENTITY_IS_ITERM(entity)) {
> + if (uvc_scan_chain_entity(chain, entity) < 0) {
> + uvc_trace(UVC_TRACE_DESCR, "Fallback "
> + "scan for input terminal %d failed.\n",
> + entity->id);
> + return -EINVAL;
> + }
> + }
> + }
> +
> + list_add_tail(&chain->list, &dev->chains);
> + uvc_trace(UVC_TRACE_PROBE, "Video chain found by fallback heuristic: "
> + "%s.\n", uvc_print_chain(chain));
> + return 0;
> +}
> +
> +
> +/*
> * Scan the device for video chains and register video devices.
> *
> * Chains are scanned starting at their output terminals and walked backwards.
> @@ -1619,8 +1695,16 @@ static int uvc_scan_device(struct uvc_device *dev)
> }
>
> if (list_empty(&dev->chains)) {
> - uvc_printk(KERN_INFO, "No valid video chain found.\n");
> - return -1;
> + uvc_trace(UVC_TRACE_PROBE, "No valid video chain found. "
> + "Start fallback heuristic.\n");
> + if (uvc_scan_fallback(dev) != 0) {
> + uvc_printk(KERN_INFO, "No valid video chain found.\n");
> + return -1;
> + }
> + uvc_printk(KERN_INFO, "Had trouble probing a valid video "
> + "chain, but fallback heuristic succeeded. "
> + "Usually it means this device will work "
> + "just fine despite errors that may follow.\n");
> }
>
> return 0;
> --
> 2.5.0
>



--
***@avoinelama.fi
[phone] skype: henrik.ingo irc: hingo
www.openlife.cc

My LinkedIn profile: http://fi.linkedin.com/pub/henrik-ingo/3/232/8a7

[Henrik Ingo]

Some devices have invalid baSourceID references, causing uvc_scan_chain()
to fail, but if we just take the entities we can find and put them
together in the most sensible chain we can think of, turns out they do
work anyway. Note: This heuristic assumes there is a single chain.

At the time of writing, devices known to have such a broken chain are
- Acer Integrated Camera (5986:055a)
- Realtek rtl157a7 (0bda:57a7)

---

Based on feedback from Laurent, this is the second attempt at fixing
the Acer Integrated Camera but also any other cameras with the
same problem, now or in the future, in a generic way. I will send
a separate email on some design decisions taken.

v2: added missing kfree(chain)

Signed-off-by: Henrik Ingo <***@avoinelama.fi>
---
drivers/media/usb/uvc/uvc_driver.c | 91 +++++++++++++++++++++++++++++++++++++-
1 file changed, 89 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index fae81ef0e0d2..4b8aae7cb1da 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1575,6 +1575,85 @@ static const char *uvc_print_chain(struct uvc_video_chain *chain)
}

/*
+ * Fallback heuristic for devices that don't define valid chain, but seem to work anyway.
+ *
+ * Some devices have invalid baSourceID references, causing uvc_scan_chain()
+ * to fail, but if we just take the entities we can find and put them
+ * together in the most sensible chain we can think of, turns out they do
+ * work anyway. Note: This heuristic assumes there is a single chain.
+ *
+ * At the time of writing, devices known to have such a broken chain are
+ * - Acer Integrated Camera (5986:055a)
+ * - Realtek rtl157a7 (0bda:57a7)
+ */
+static int uvc_scan_fallback(struct uvc_device *dev)
+{
+ struct uvc_video_chain *chain;
+ struct uvc_entity *entity;
+
+ chain = kzalloc(sizeof(*chain), GFP_KERNEL);
+ if (chain == NULL)
+ return -ENOMEM;
+ INIT_LIST_HEAD(&chain->entities);
+ mutex_init(&chain->ctrl_mutex);
+ chain->dev = dev;
+ v4l2_prio_init(&chain->prio);
+
+ /* We take whatever entities that are there, and just
+ * put them in a list in the order:
+ * OT <- any XU and PU <- IT
+ */
+ list_for_each_entry(entity, &dev->entities, list) {
+ entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
+ if (UVC_ENTITY_IS_OTERM(entity)) {
+ if (uvc_scan_chain_entity(chain, entity) < 0) {
+ uvc_trace(UVC_TRACE_DESCR, "Fallback "
+ "scan for output terminal %d failed.\n",
+ entity->id);
+ kfree(chain);
+ return -EINVAL;
+ }
+ }
+ }
+ /* At this point we accept pretty much anything, but there
+ * absolutely must be at least one output terminal. */
+ if (list_empty(&chain->list)) {
+ kfree(chain);
+ return -EINVAL;
+ }
+ list_for_each_entry(entity, &dev->entities, list) {
+ entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
+ if (UVC_ENTITY_IS_UNIT(entity)) {
+ if (uvc_scan_chain_entity(chain, entity) < 0) {
+ uvc_trace(UVC_TRACE_DESCR, "Fallback "
+ "scan for entity %d failed.\n",
+ entity->id);
+ kfree(chain);
+ return -EINVAL;
+ }
+ }
+ }
+ list_for_each_entry(entity, &dev->entities, list) {
+ entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
+ if (UVC_ENTITY_IS_ITERM(entity)) {
+ if (uvc_scan_chain_entity(chain, entity) < 0) {
+ uvc_trace(UVC_TRACE_DESCR, "Fallback "
+ "scan for input terminal %d failed.\n",
+ entity->id);
+ kfree(chain);
+ return -EINVAL;
+ }
+ }
+ }
+
+ list_add_tail(&chain->list, &dev->chains);
+ uvc_trace(UVC_TRACE_PROBE, "Video chain found by fallback heuristic: "
+ "%s.\n", uvc_print_chain(chain));
+ return 0;
+}
+
+
+/*
* Scan the device for video chains and register video devices.
*
* Chains are scanned starting at their output terminals and walked backwards.
@@ -1619,8 +1698,16 @@ static int uvc_scan_device(struct uvc_device *dev)
}

if (list_empty(&dev->chains)) {
- uvc_printk(KERN_INFO, "No valid video chain found.\n");
- return -1;
+ uvc_trace(UVC_TRACE_PROBE, "No valid video chain found. "
+ "Start fallback heuristic.\n");
+ if (uvc_scan_fallback(dev) != 0) {
+ uvc_printk(KERN_INFO, "No valid video chain found.\n");
+ return -1;
+ }
+ uvc_printk(KERN_INFO, "Had trouble probing a valid video "
+ "chain, but fallback heuristic succeeded. "
+ "Usually it means this device will work "
+ "just fine despite errors that may follow.\n");
}

return 0;
--
2.5.0

[Henrik Ingo]

It seems we've passed the 2-month anniversary of this patch without a
single response.

Is there any fallback plan for situations like this (like a more
active mailing list where this could be submitted)? Or should I just
keep waiting, for next Christmas?

henrik

On Fri, Jan 8, 2016 at 10:51 PM, Henrik Ingo <***@avoinelama.fi> wrote:
> Some devices have invalid baSourceID references, causing uvc_scan_chain()
> to fail, but if we just take the entities we can find and put them
> together in the most sensible chain we can think of, turns out they do
> work anyway. Note: This heuristic assumes there is a single chain.
>
> At the time of writing, devices known to have such a broken chain are
> - Acer Integrated Camera (5986:055a)
> - Realtek rtl157a7 (0bda:57a7)
>
> ---
>
> Based on feedback from Laurent, this is the second attempt at fixing
> the Acer Integrated Camera but also any other cameras with the
> same problem, now or in the future, in a generic way. I will send
> a separate email on some design decisions taken.
>
> v2: added missing kfree(chain)
>
> Signed-off-by: Henrik Ingo <***@avoinelama.fi>
> ---
> drivers/media/usb/uvc/uvc_driver.c | 91 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 89 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index fae81ef0e0d2..4b8aae7cb1da 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1575,6 +1575,85 @@ static const char *uvc_print_chain(struct uvc_video_chain *chain)
> }
>
> /*
> + * Fallback heuristic for devices that don't define valid chain, but seem to work anyway.
> + *
> + * Some devices have invalid baSourceID references, causing uvc_scan_chain()
> + * to fail, but if we just take the entities we can find and put them
> + * together in the most sensible chain we can think of, turns out they do
> + * work anyway. Note: This heuristic assumes there is a single chain.
> + *
> + * At the time of writing, devices known to have such a broken chain are
> + * - Acer Integrated Camera (5986:055a)
> + * - Realtek rtl157a7 (0bda:57a7)
> + */
> +static int uvc_scan_fallback(struct uvc_device *dev)
> +{
> + struct uvc_video_chain *chain;
> + struct uvc_entity *entity;
> +
> + chain = kzalloc(sizeof(*chain), GFP_KERNEL);
> + if (chain == NULL)
> + return -ENOMEM;
> + INIT_LIST_HEAD(&chain->entities);
> + mutex_init(&chain->ctrl_mutex);
> + chain->dev = dev;
> + v4l2_prio_init(&chain->prio);
> +
> + /* We take whatever entities that are there, and just
> + * put them in a list in the order:
> + * OT <- any XU and PU <- IT
> + */
> + list_for_each_entry(entity, &dev->entities, list) {
> + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
> + if (UVC_ENTITY_IS_OTERM(entity)) {
> + if (uvc_scan_chain_entity(chain, entity) < 0) {
> + uvc_trace(UVC_TRACE_DESCR, "Fallback "
> + "scan for output terminal %d failed.\n",
> + entity->id);
> + kfree(chain);
> + return -EINVAL;
> + }
> + }
> + }
> + /* At this point we accept pretty much anything, but there
> + * absolutely must be at least one output terminal. */
> + if (list_empty(&chain->list)) {
> + kfree(chain);
> + return -EINVAL;
> + }
> + list_for_each_entry(entity, &dev->entities, list) {
> + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
> + if (UVC_ENTITY_IS_UNIT(entity)) {
> + if (uvc_scan_chain_entity(chain, entity) < 0) {
> + uvc_trace(UVC_TRACE_DESCR, "Fallback "
> + "scan for entity %d failed.\n",
> + entity->id);
> + kfree(chain);
> + return -EINVAL;
> + }
> + }
> + }
> + list_for_each_entry(entity, &dev->entities, list) {
> + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
> + if (UVC_ENTITY_IS_ITERM(entity)) {
> + if (uvc_scan_chain_entity(chain, entity) < 0) {
> + uvc_trace(UVC_TRACE_DESCR, "Fallback "
> + "scan for input terminal %d failed.\n",
> + entity->id);
> + kfree(chain);
> + return -EINVAL;
> + }
> + }
> + }
> +
> + list_add_tail(&chain->list, &dev->chains);
> + uvc_trace(UVC_TRACE_PROBE, "Video chain found by fallback heuristic: "
> + "%s.\n", uvc_print_chain(chain));
> + return 0;
> +}
> +
> +
> +/*
> * Scan the device for video chains and register video devices.
> *
> * Chains are scanned starting at their output terminals and walked backwards.
> @@ -1619,8 +1698,16 @@ static int uvc_scan_device(struct uvc_device *dev)
> }
>
> if (list_empty(&dev->chains)) {
> - uvc_printk(KERN_INFO, "No valid video chain found.\n");
> - return -1;
> + uvc_trace(UVC_TRACE_PROBE, "No valid video chain found. "
> + "Start fallback heuristic.\n");
> + if (uvc_scan_fallback(dev) != 0) {
> + uvc_printk(KERN_INFO, "No valid video chain found.\n");
> + return -1;
> + }
> + uvc_printk(KERN_INFO, "Had trouble probing a valid video "
> + "chain, but fallback heuristic succeeded. "
> + "Usually it means this device will work "
> + "just fine despite errors that may follow.\n");
> }
>
> return 0;
> --
> 2.5.0
>



--
***@avoinelama.fi
[phone] skype: henrik.ingo irc: hingo
www.openlife.cc

My LinkedIn profile: http://fi.linkedin.com/pub/henrik-ingo/3/232/8a7

[Anouk Wipprecht]

Hi list -
Camera Logitech c920 is not in the list,can this one be added?

> Date: Wed, 9 Mar 2016 11:55:40 +0200
> From: ***@avoinelama.fi
> To: linux-uvc-***@lists.sourceforge.net; ***@ideasonboard.com; ***@gmail.com; ***@gmail.com
> Subject: Re: [linux-uvc-devel] [PATCH v2] uvcvideo: uvc_scan_fallback() for webcams with broken chain descriptors
>
> It seems we've passed the 2-month anniversary of this patch without a
> single response.
>
> Is there any fallback plan for situations like this (like a more
> active mailing list where this could be submitted)? Or should I just
> keep waiting, for next Christmas?
>
> henrik
>
> On Fri, Jan 8, 2016 at 10:51 PM, Henrik Ingo <***@avoinelama.fi> wrote:
> > Some devices have invalid baSourceID references, causing uvc_scan_chain()
> > to fail, but if we just take the entities we can find and put them
> > together in the most sensible chain we can think of, turns out they do
> > work anyway. Note: This heuristic assumes there is a single chain.
> >
> > At the time of writing, devices known to have such a broken chain are
> > - Acer Integrated Camera (5986:055a)
> > - Realtek rtl157a7 (0bda:57a7)
> >
> > ---
> >
> > Based on feedback from Laurent, this is the second attempt at fixing
> > the Acer Integrated Camera but also any other cameras with the
> > same problem, now or in the future, in a generic way. I will send
> > a separate email on some design decisions taken.
> >
> > v2: added missing kfree(chain)
> >
> > Signed-off-by: Henrik Ingo <***@avoinelama.fi>
> > ---
> > drivers/media/usb/uvc/uvc_driver.c | 91 +++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 89 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index fae81ef0e0d2..4b8aae7cb1da 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1575,6 +1575,85 @@ static const char *uvc_print_chain(struct uvc_video_chain *chain)
> > }
> >
> > /*
> > + * Fallback heuristic for devices that don't define valid chain, but seem to work anyway.
> > + *
> > + * Some devices have invalid baSourceID references, causing uvc_scan_chain()
> > + * to fail, but if we just take the entities we can find and put them
> > + * together in the most sensible chain we can think of, turns out they do
> > + * work anyway. Note: This heuristic assumes there is a single chain.
> > + *
> > + * At the time of writing, devices known to have such a broken chain are
> > + * - Acer Integrated Camera (5986:055a)
> > + * - Realtek rtl157a7 (0bda:57a7)
> > + */
> > +static int uvc_scan_fallback(struct uvc_device *dev)
> > +{
> > + struct uvc_video_chain *chain;
> > + struct uvc_entity *entity;
> > +
> > + chain = kzalloc(sizeof(*chain), GFP_KERNEL);
> > + if (chain == NULL)
> > + return -ENOMEM;
> > + INIT_LIST_HEAD(&chain->entities);
> > + mutex_init(&chain->ctrl_mutex);
> > + chain->dev = dev;
> > + v4l2_prio_init(&chain->prio);
> > +
> > + /* We take whatever entities that are there, and just
> > + * put them in a list in the order:
> > + * OT <- any XU and PU <- IT
> > + */
> > + list_for_each_entry(entity, &dev->entities, list) {
> > + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
> > + if (UVC_ENTITY_IS_OTERM(entity)) {
> > + if (uvc_scan_chain_entity(chain, entity) < 0) {
> > + uvc_trace(UVC_TRACE_DESCR, "Fallback "
> > + "scan for output terminal %d failed.\n",
> > + entity->id);
> > + kfree(chain);
> > + return -EINVAL;
> > + }
> > + }
> > + }
> > + /* At this point we accept pretty much anything, but there
> > + * absolutely must be at least one output terminal. */
> > + if (list_empty(&chain->list)) {
> > + kfree(chain);
> > + return -EINVAL;
> > + }
> > + list_for_each_entry(entity, &dev->entities, list) {
> > + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
> > + if (UVC_ENTITY_IS_UNIT(entity)) {
> > + if (uvc_scan_chain_entity(chain, entity) < 0) {
> > + uvc_trace(UVC_TRACE_DESCR, "Fallback "
> > + "scan for entity %d failed.\n",
> > + entity->id);
> > + kfree(chain);
> > + return -EINVAL;
> > + }
> > + }
> > + }
> > + list_for_each_entry(entity, &dev->entities, list) {
> > + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
> > + if (UVC_ENTITY_IS_ITERM(entity)) {
> > + if (uvc_scan_chain_entity(chain, entity) < 0) {
> > + uvc_trace(UVC_TRACE_DESCR, "Fallback "
> > + "scan for input terminal %d failed.\n",
> > + entity->id);
> > + kfree(chain);
> > + return -EINVAL;
> > + }
> > + }
> > + }
> > +
> > + list_add_tail(&chain->list, &dev->chains);
> > + uvc_trace(UVC_TRACE_PROBE, "Video chain found by fallback heuristic: "
> > + "%s.\n", uvc_print_chain(chain));
> > + return 0;
> > +}
> > +
> > +
> > +/*
> > * Scan the device for video chains and register video devices.
> > *
> > * Chains are scanned starting at their output terminals and walked backwards.
> > @@ -1619,8 +1698,16 @@ static int uvc_scan_device(struct uvc_device *dev)
> > }
> >
> > if (list_empty(&dev->chains)) {
> > - uvc_printk(KERN_INFO, "No valid video chain found.\n");
> > - return -1;
> > + uvc_trace(UVC_TRACE_PROBE, "No valid video chain found. "
> > + "Start fallback heuristic.\n");
> > + if (uvc_scan_fallback(dev) != 0) {
> > + uvc_printk(KERN_INFO, "No valid video chain found.\n");
> > + return -1;
> > + }
> > + uvc_printk(KERN_INFO, "Had trouble probing a valid video "
> > + "chain, but fallback heuristic succeeded. "
> > + "Usually it means this device will work "
> > + "just fine despite errors that may follow.\n");
> > }
> >
> > return 0;
> > --
> > 2.5.0
> >
>
>
>
> --
> ***@avoinelama.fi
> [phone] skype: henrik.ingo irc: hingo
> www.openlife.cc
>
> My LinkedIn profile: http://fi.linkedin.com/pub/henrik-ingo/3/232/8a7
>
> ------------------------------------------------------------------------------
> Transform Data into Opportunity.
> Accelerate data analysis in your applications with
> Intel Data Analytics Acceleration Library.
> Click to learn more.
> http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140
> _______________________________________________
> Linux-uvc-devel mailing list
> Linux-uvc-***@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-uvc-devel

[Henrik Ingo]

Hi Anouk

Is this a reply specifically to my patch? If yes, could you be so kind
to do the following to verify:

On a kernel without this patch:
sudo rmmod uvcvideo
sudo modprobe uvcvideo trace=0xffff
dmesg > dmesg-broken.log

On a kernel with this patch (or just compiling the module
independently and loading it):
sudo rmmod uvcvideo
sudo modprobe uvcvideo trace=0xffff
dmesg > dmesg-scan-fallback-patch.log

Then post both of the *.log files to this list (maybe as a separate
thread, I'll see it).

henrik


On Wed, Mar 9, 2016 at 12:19 PM, Anouk Wipprecht
<***@hotmail.com> wrote:
> Hi list -
>
> Camera Logitech c920 is not in the list,
> can this one be added?
>
>> Date: Wed, 9 Mar 2016 11:55:40 +0200
>> From: ***@avoinelama.fi
>> To: linux-uvc-***@lists.sourceforge.net;
>> ***@ideasonboard.com; ***@gmail.com; ***@gmail.com
>> Subject: Re: [linux-uvc-devel] [PATCH v2] uvcvideo: uvc_scan_fallback()
>> for webcams with broken chain descriptors
>
>>
>> It seems we've passed the 2-month anniversary of this patch without a
>> single response.
>>
>> Is there any fallback plan for situations like this (like a more
>> active mailing list where this could be submitted)? Or should I just
>> keep waiting, for next Christmas?
>>
>> henrik
>>
>> On Fri, Jan 8, 2016 at 10:51 PM, Henrik Ingo <***@avoinelama.fi>
>> wrote:
>> > Some devices have invalid baSourceID references, causing
>> > uvc_scan_chain()
>> > to fail, but if we just take the entities we can find and put them
>> > together in the most sensible chain we can think of, turns out they do
>> > work anyway. Note: This heuristic assumes there is a single chain.
>> >
>> > At the time of writing, devices known to have such a broken chain are
>> > - Acer Integrated Camera (5986:055a)
>> > - Realtek rtl157a7 (0bda:57a7)
>> >
>> > ---
>> >
>> > Based on feedback from Laurent, this is the second attempt at fixing
>> > the Acer Integrated Camera but also any other cameras with the
>> > same problem, now or in the future, in a generic way. I will send
>> > a separate email on some design decisions taken.
>> >
>> > v2: added missing kfree(chain)
>> >
>> > Signed-off-by: Henrik Ingo <***@avoinelama.fi>
>> > ---
>> > drivers/media/usb/uvc/uvc_driver.c | 91
>> > +++++++++++++++++++++++++++++++++++++-
>> > 1 file changed, 89 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
>> > b/drivers/media/usb/uvc/uvc_driver.c
>> > index fae81ef0e0d2..4b8aae7cb1da 100644
>> > --- a/drivers/media/usb/uvc/uvc_driver.c
>> > +++ b/drivers/media/usb/uvc/uvc_driver.c
>> > @@ -1575,6 +1575,85 @@ static const char *uvc_print_chain(struct
>> > uvc_video_chain *chain)
>> > }
>> >
>> > /*
>> > + * Fallback heuristic for devices that don't define valid chain, but
>> > seem to work anyway.
>> > + *
>> > + * Some devices have invalid baSourceID references, causing
>> > uvc_scan_chain()
>> > + * to fail, but if we just take the entities we can find and put them
>> > + * together in the most sensible chain we can think of, turns out they
>> > do
>> > + * work anyway. Note: This heuristic assumes there is a single chain.
>> > + *
>> > + * At the time of writing, devices known to have such a broken chain
>> > are
>> > + * - Acer Integrated Camera (5986:055a)
>> > + * - Realtek rtl157a7 (0bda:57a7)
>> > + */
>> > +static int uvc_scan_fallback(struct uvc_device *dev)
>> > +{
>> > + struct uvc_video_chain *chain;
>> > + struct uvc_entity *entity;
>> > +
>> > + chain = kzalloc(sizeof(*chain), GFP_KERNEL);
>> > + if (chain == NULL)
>> > + return -ENOMEM;
>> > + INIT_LIST_HEAD(&chain->entities);
>> > + mutex_init(&chain->ctrl_mutex);
>> > + chain->dev = dev;
>> > + v4l2_prio_init(&chain->prio);
>> > +
>> > + /* We take whatever entities that are there, and just
>> > + * put them in a list in the order:
>> > + * OT <- any XU and PU <- IT
>> > + */
>> > + list_for_each_entry(entity, &dev->entities, list) {
>> > + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
>> > + if (UVC_ENTITY_IS_OTERM(entity)) {
>> > + if (uvc_scan_chain_entity(chain, entity) < 0) {
>> > + uvc_trace(UVC_TRACE_DESCR, "Fallback "
>> > + "scan for output terminal %d failed.\n",
>> > + entity->id);
>> > + kfree(chain);
>> > + return -EINVAL;
>> > + }
>> > + }
>> > + }
>> > + /* At this point we accept pretty much anything, but there
>> > + * absolutely must be at least one output terminal. */
>> > + if (list_empty(&chain->list)) {
>> > + kfree(chain);
>> > + return -EINVAL;
>> > + }
>> > + list_for_each_entry(entity, &dev->entities, list) {
>> > + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
>> > + if (UVC_ENTITY_IS_UNIT(entity)) {
>> > + if (uvc_scan_chain_entity(chain, entity) < 0) {
>> > + uvc_trace(UVC_TRACE_DESCR, "Fallback "
>> > + "scan for entity %d failed.\n",
>> > + entity->id);
>> > + kfree(chain);
>> > + return -EINVAL;
>> > + }
>> > + }
>> > + }
>> > + list_for_each_entry(entity, &dev->entities, list) {
>> > + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
>> > + if (UVC_ENTITY_IS_ITERM(entity)) {
>> > + if (uvc_scan_chain_entity(chain, entity) < 0) {
>> > + uvc_trace(UVC_TRACE_DESCR, "Fallback "
>> > + "scan for input terminal %d failed.\n",
>> > + entity->id);
>> > + kfree(chain);
>> > + return -EINVAL;
>> > + }
>> > + }
>> > + }
>> > +
>> > + list_add_tail(&chain->list, &dev->chains);
>> > + uvc_trace(UVC_TRACE_PROBE, "Video chain found by fallback heuristic: "
>> > + "%s.\n", uvc_print_chain(chain));
>> > + return 0;
>> > +}
>> > +
>> > +
>> > +/*
>> > * Scan the device for video chains and register video devices.
>> > *
>> > * Chains are scanned starting at their output terminals and walked
>> > backwards.
>> > @@ -1619,8 +1698,16 @@ static int uvc_scan_device(struct uvc_device
>> > *dev)
>> > }
>> >
>> > if (list_empty(&dev->chains)) {
>> > - uvc_printk(KERN_INFO, "No valid video chain found.\n");
>> > - return -1;
>> > + uvc_trace(UVC_TRACE_PROBE, "No valid video chain found. "
>> > + "Start fallback heuristic.\n");
>> > + if (uvc_scan_fallback(dev) != 0) {
>> > + uvc_printk(KERN_INFO, "No valid video chain found.\n");
>> > + return -1;
>> > + }
>> > + uvc_printk(KERN_INFO, "Had trouble probing a valid video "
>> > + "chain, but fallback heuristic succeeded. "
>> > + "Usually it means this device will work "
>> > + "just fine despite errors that may follow.\n");
>> > }
>> >
>> > return 0;
>> > --
>> > 2.5.0
>> >
>>
>>
>>
>> --
>> ***@avoinelama.fi
>> [phone] skype: henrik.ingo irc: hingo
>> www.openlife.cc
>>
>> My LinkedIn profile: http://fi.linkedin.com/pub/henrik-ingo/3/232/8a7
>>
>>
>> ------------------------------------------------------------------------------
>> Transform Data into Opportunity.
>> Accelerate data analysis in your applications with
>> Intel Data Analytics Acceleration Library.
>> Click to learn more.
>> http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140
>> _______________________________________________
>> Linux-uvc-devel mailing list
>> Linux-uvc-***@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-uvc-devel
>
> ------------------------------------------------------------------------------
> Transform Data into Opportunity.
> Accelerate data analysis in your applications with
> Intel Data Analytics Acceleration Library.
> Click to learn more.
> http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140
> _______________________________________________
> Linux-uvc-devel mailing list
> Linux-uvc-***@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-uvc-devel
>



--
***@avoinelama.fi
[phone] skype: henrik.ingo irc: hingo
www.openlife.cc

My LinkedIn profile: http://fi.linkedin.com/pub/henrik-ingo/3/232/8a7

[Anouk Wipprecht]

Henrik!

Ah no sorry it's a reply to the list!(sorry I should have made another topic)
http://www.ideasonboard.org/uvc/
It said sent an reply to the list if your UVC is not on it,I am testing Edison + stream over a Logitec C920, onlythe Logitec C910 is stated.
-Kind regards,Anouk Wipprechtcurator | designer | engineer_________________Anouk Wipprecht www.anoukwipprecht.nl

20161 March - 12 March Linz (AT, ARS Electronica FUTURELAB)12 March - 26 March Amsterdam (NL, AUDI China project)26 March - 1 April Paris (FR, AUDI exhibition) 1 April - 22 April Amsterdam (NL, AUDI China project)22 April - 5 May Toronto (CA, Terroir symposium + Digifest)15 May - 19 May Orlando (USA, Rapid 2016 3D printing event)19 May - 5 June San Francisco (USA, CODAME)5 June - 10 June Minneapolis (USA, Eyeo festival)10 June - 10 October San Francisco (USA, ART+TECH festival Oct 5-8 Fort Mason)
> Date: Wed, 9 Mar 2016 12:35:34 +0200
> Subject: Re: [linux-uvc-devel] [PATCH v2] uvcvideo: uvc_scan_fallback() for webcams with broken chain descriptors
> From: ***@avoinelama.fi
> To: ***@hotmail.com
> CC: linux-uvc-***@lists.sourceforge.net
>
> Hi Anouk
>
> Is this a reply specifically to my patch? If yes, could you be so kind
> to do the following to verify:
>
> On a kernel without this patch:
> sudo rmmod uvcvideo
> sudo modprobe uvcvideo trace=0xffff
> dmesg > dmesg-broken.log
>
> On a kernel with this patch (or just compiling the module
> independently and loading it):
> sudo rmmod uvcvideo
> sudo modprobe uvcvideo trace=0xffff
> dmesg > dmesg-scan-fallback-patch.log
>
> Then post both of the *.log files to this list (maybe as a separate
> thread, I'll see it).
>
> henrik
>
>
> On Wed, Mar 9, 2016 at 12:19 PM, Anouk Wipprecht
> <***@hotmail.com> wrote:
> > Hi list -
> >
> > Camera Logitech c920 is not in the list,
> > can this one be added?
> >
> >> Date: Wed, 9 Mar 2016 11:55:40 +0200
> >> From: ***@avoinelama.fi
> >> To: linux-uvc-***@lists.sourceforge.net;
> >> ***@ideasonboard.com; ***@gmail.com; ***@gmail.com
> >> Subject: Re: [linux-uvc-devel] [PATCH v2] uvcvideo: uvc_scan_fallback()
> >> for webcams with broken chain descriptors
> >
> >>
> >> It seems we've passed the 2-month anniversary of this patch without a
> >> single response.
> >>
> >> Is there any fallback plan for situations like this (like a more
> >> active mailing list where this could be submitted)? Or should I just
> >> keep waiting, for next Christmas?
> >>
> >> henrik
> >>
> >> On Fri, Jan 8, 2016 at 10:51 PM, Henrik Ingo <***@avoinelama.fi>
> >> wrote:
> >> > Some devices have invalid baSourceID references, causing
> >> > uvc_scan_chain()
> >> > to fail, but if we just take the entities we can find and put them
> >> > together in the most sensible chain we can think of, turns out they do
> >> > work anyway. Note: This heuristic assumes there is a single chain.
> >> >
> >> > At the time of writing, devices known to have such a broken chain are
> >> > - Acer Integrated Camera (5986:055a)
> >> > - Realtek rtl157a7 (0bda:57a7)
> >> >
> >> > ---
> >> >
> >> > Based on feedback from Laurent, this is the second attempt at fixing
> >> > the Acer Integrated Camera but also any other cameras with the
> >> > same problem, now or in the future, in a generic way. I will send
> >> > a separate email on some design decisions taken.
> >> >
> >> > v2: added missing kfree(chain)
> >> >
> >> > Signed-off-by: Henrik Ingo <***@avoinelama.fi>
> >> > ---
> >> > drivers/media/usb/uvc/uvc_driver.c | 91
> >> > +++++++++++++++++++++++++++++++++++++-
> >> > 1 file changed, 89 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> >> > b/drivers/media/usb/uvc/uvc_driver.c
> >> > index fae81ef0e0d2..4b8aae7cb1da 100644
> >> > --- a/drivers/media/usb/uvc/uvc_driver.c
> >> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> >> > @@ -1575,6 +1575,85 @@ static const char *uvc_print_chain(struct
> >> > uvc_video_chain *chain)
> >> > }
> >> >
> >> > /*
> >> > + * Fallback heuristic for devices that don't define valid chain, but
> >> > seem to work anyway.
> >> > + *
> >> > + * Some devices have invalid baSourceID references, causing
> >> > uvc_scan_chain()
> >> > + * to fail, but if we just take the entities we can find and put them
> >> > + * together in the most sensible chain we can think of, turns out they
> >> > do
> >> > + * work anyway. Note: This heuristic assumes there is a single chain.
> >> > + *
> >> > + * At the time of writing, devices known to have such a broken chain
> >> > are
> >> > + * - Acer Integrated Camera (5986:055a)
> >> > + * - Realtek rtl157a7 (0bda:57a7)
> >> > + */
> >> > +static int uvc_scan_fallback(struct uvc_device *dev)
> >> > +{
> >> > + struct uvc_video_chain *chain;
> >> > + struct uvc_entity *entity;
> >> > +
> >> > + chain = kzalloc(sizeof(*chain), GFP_KERNEL);
> >> > + if (chain == NULL)
> >> > + return -ENOMEM;
> >> > + INIT_LIST_HEAD(&chain->entities);
> >> > + mutex_init(&chain->ctrl_mutex);
> >> > + chain->dev = dev;
> >> > + v4l2_prio_init(&chain->prio);
> >> > +
> >> > + /* We take whatever entities that are there, and just
> >> > + * put them in a list in the order:
> >> > + * OT <- any XU and PU <- IT
> >> > + */
> >> > + list_for_each_entry(entity, &dev->entities, list) {
> >> > + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
> >> > + if (UVC_ENTITY_IS_OTERM(entity)) {
> >> > + if (uvc_scan_chain_entity(chain, entity) < 0) {
> >> > + uvc_trace(UVC_TRACE_DESCR, "Fallback "
> >> > + "scan for output terminal %d failed.\n",
> >> > + entity->id);
> >> > + kfree(chain);
> >> > + return -EINVAL;
> >> > + }
> >> > + }
> >> > + }
> >> > + /* At this point we accept pretty much anything, but there
> >> > + * absolutely must be at least one output terminal. */
> >> > + if (list_empty(&chain->list)) {
> >> > + kfree(chain);
> >> > + return -EINVAL;
> >> > + }
> >> > + list_for_each_entry(entity, &dev->entities, list) {
> >> > + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
> >> > + if (UVC_ENTITY_IS_UNIT(entity)) {
> >> > + if (uvc_scan_chain_entity(chain, entity) < 0) {
> >> > + uvc_trace(UVC_TRACE_DESCR, "Fallback "
> >> > + "scan for entity %d failed.\n",
> >> > + entity->id);
> >> > + kfree(chain);
> >> > + return -EINVAL;
> >> > + }
> >> > + }
> >> > + }
> >> > + list_for_each_entry(entity, &dev->entities, list) {
> >> > + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
> >> > + if (UVC_ENTITY_IS_ITERM(entity)) {
> >> > + if (uvc_scan_chain_entity(chain, entity) < 0) {
> >> > + uvc_trace(UVC_TRACE_DESCR, "Fallback "
> >> > + "scan for input terminal %d failed.\n",
> >> > + entity->id);
> >> > + kfree(chain);
> >> > + return -EINVAL;
> >> > + }
> >> > + }
> >> > + }
> >> > +
> >> > + list_add_tail(&chain->list, &dev->chains);
> >> > + uvc_trace(UVC_TRACE_PROBE, "Video chain found by fallback heuristic: "
> >> > + "%s.\n", uvc_print_chain(chain));
> >> > + return 0;
> >> > +}
> >> > +
> >> > +
> >> > +/*
> >> > * Scan the device for video chains and register video devices.
> >> > *
> >> > * Chains are scanned starting at their output terminals and walked
> >> > backwards.
> >> > @@ -1619,8 +1698,16 @@ static int uvc_scan_device(struct uvc_device
> >> > *dev)
> >> > }
> >> >
> >> > if (list_empty(&dev->chains)) {
> >> > - uvc_printk(KERN_INFO, "No valid video chain found.\n");
> >> > - return -1;
> >> > + uvc_trace(UVC_TRACE_PROBE, "No valid video chain found. "
> >> > + "Start fallback heuristic.\n");
> >> > + if (uvc_scan_fallback(dev) != 0) {
> >> > + uvc_printk(KERN_INFO, "No valid video chain found.\n");
> >> > + return -1;
> >> > + }
> >> > + uvc_printk(KERN_INFO, "Had trouble probing a valid video "
> >> > + "chain, but fallback heuristic succeeded. "
> >> > + "Usually it means this device will work "
> >> > + "just fine despite errors that may follow.\n");
> >> > }
> >> >
> >> > return 0;
> >> > --
> >> > 2.5.0
> >> >
> >>
> >>
> >>
> >> --
> >> ***@avoinelama.fi
> >> [phone] skype: henrik.ingo irc: hingo
> >> www.openlife.cc
> >>
> >> My LinkedIn profile: http://fi.linkedin.com/pub/henrik-ingo/3/232/8a7
> >>
> >>
> >> ------------------------------------------------------------------------------
> >> Transform Data into Opportunity.
> >> Accelerate data analysis in your applications with
> >> Intel Data Analytics Acceleration Library.
> >> Click to learn more.
> >> http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140
> >> _______________________________________________
> >> Linux-uvc-devel mailing list
> >> Linux-uvc-***@lists.sourceforge.net
> >> https://lists.sourceforge.net/lists/listinfo/linux-uvc-devel
> >
> > ------------------------------------------------------------------------------
> > Transform Data into Opportunity.
> > Accelerate data analysis in your applications with
> > Intel Data Analytics Acceleration Library.
> > Click to learn more.
> > http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140
> > _______________________________________________
> > Linux-uvc-devel mailing list
> > Linux-uvc-***@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-uvc-devel
> >
>
>
>
> --
> ***@avoinelama.fi
> [phone] skype: henrik.ingo irc: hingo
> www.openlife.cc
>
> My LinkedIn profile: http://fi.linkedin.com/pub/henrik-ingo/3/232/8a7

[Samir Ibradžić]

Henrik,

The patch landed in Ubuntu 16.04 beta, that fact makes it bit difficult
for me to test it. I can test it against Ubuntu 4.2 kernel, if it makes
any sense.

R,
S

On 2016年03月09日 19:35, Henrik Ingo wrote:
> Hi Anouk
>
> Is this a reply specifically to my patch? If yes, could you be so kind
> to do the following to verify:
>
> On a kernel without this patch:
> sudo rmmod uvcvideo
> sudo modprobe uvcvideo trace=0xffff
> dmesg > dmesg-broken.log
>
> On a kernel with this patch (or just compiling the module
> independently and loading it):
> sudo rmmod uvcvideo
> sudo modprobe uvcvideo trace=0xffff
> dmesg > dmesg-scan-fallback-patch.log
>
> Then post both of the *.log files to this list (maybe as a separate
> thread, I'll see it).
>
> henrik
>
>
> On Wed, Mar 9, 2016 at 12:19 PM, Anouk Wipprecht
> <***@hotmail.com> wrote:
>> Hi list -
>>
>> Camera Logitech c920 is not in the list,
>> can this one be added?
>>
>>> Date: Wed, 9 Mar 2016 11:55:40 +0200
>>> From: ***@avoinelama.fi
>>> To: linux-uvc-***@lists.sourceforge.net;
>>> ***@ideasonboard.com; ***@gmail.com; ***@gmail.com
>>> Subject: Re: [linux-uvc-devel] [PATCH v2] uvcvideo: uvc_scan_fallback()
>>> for webcams with broken chain descriptors
>>
>>>
>>> It seems we've passed the 2-month anniversary of this patch without a
>>> single response.
>>>
>>> Is there any fallback plan for situations like this (like a more
>>> active mailing list where this could be submitted)? Or should I just
>>> keep waiting, for next Christmas?
>>>
>>> henrik
>>>
>>> On Fri, Jan 8, 2016 at 10:51 PM, Henrik Ingo <***@avoinelama.fi>
>>> wrote:
>>>> Some devices have invalid baSourceID references, causing
>>>> uvc_scan_chain()
>>>> to fail, but if we just take the entities we can find and put them
>>>> together in the most sensible chain we can think of, turns out they do
>>>> work anyway. Note: This heuristic assumes there is a single chain.
>>>>
>>>> At the time of writing, devices known to have such a broken chain are
>>>> - Acer Integrated Camera (5986:055a)
>>>> - Realtek rtl157a7 (0bda:57a7)
>>>>
>>>> ---
>>>>
>>>> Based on feedback from Laurent, this is the second attempt at fixing
>>>> the Acer Integrated Camera but also any other cameras with the
>>>> same problem, now or in the future, in a generic way. I will send
>>>> a separate email on some design decisions taken.
>>>>
>>>> v2: added missing kfree(chain)
>>>>
>>>> Signed-off-by: Henrik Ingo <***@avoinelama.fi>
>>>> ---
>>>> drivers/media/usb/uvc/uvc_driver.c | 91
>>>> +++++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 89 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c
>>>> b/drivers/media/usb/uvc/uvc_driver.c
>>>> index fae81ef0e0d2..4b8aae7cb1da 100644
>>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>>> @@ -1575,6 +1575,85 @@ static const char *uvc_print_chain(struct
>>>> uvc_video_chain *chain)
>>>> }
>>>>
>>>> /*
>>>> + * Fallback heuristic for devices that don't define valid chain, but
>>>> seem to work anyway.
>>>> + *
>>>> + * Some devices have invalid baSourceID references, causing
>>>> uvc_scan_chain()
>>>> + * to fail, but if we just take the entities we can find and put them
>>>> + * together in the most sensible chain we can think of, turns out they
>>>> do
>>>> + * work anyway. Note: This heuristic assumes there is a single chain.
>>>> + *
>>>> + * At the time of writing, devices known to have such a broken chain
>>>> are
>>>> + * - Acer Integrated Camera (5986:055a)
>>>> + * - Realtek rtl157a7 (0bda:57a7)
>>>> + */
>>>> +static int uvc_scan_fallback(struct uvc_device *dev)
>>>> +{
>>>> + struct uvc_video_chain *chain;
>>>> + struct uvc_entity *entity;
>>>> +
>>>> + chain = kzalloc(sizeof(*chain), GFP_KERNEL);
>>>> + if (chain == NULL)
>>>> + return -ENOMEM;
>>>> + INIT_LIST_HEAD(&chain->entities);
>>>> + mutex_init(&chain->ctrl_mutex);
>>>> + chain->dev = dev;
>>>> + v4l2_prio_init(&chain->prio);
>>>> +
>>>> + /* We take whatever entities that are there, and just
>>>> + * put them in a list in the order:
>>>> + * OT <- any XU and PU <- IT
>>>> + */
>>>> + list_for_each_entry(entity, &dev->entities, list) {
>>>> + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
>>>> + if (UVC_ENTITY_IS_OTERM(entity)) {
>>>> + if (uvc_scan_chain_entity(chain, entity) < 0) {
>>>> + uvc_trace(UVC_TRACE_DESCR, "Fallback "
>>>> + "scan for output terminal %d failed.\n",
>>>> + entity->id);
>>>> + kfree(chain);
>>>> + return -EINVAL;
>>>> + }
>>>> + }
>>>> + }
>>>> + /* At this point we accept pretty much anything, but there
>>>> + * absolutely must be at least one output terminal. */
>>>> + if (list_empty(&chain->list)) {
>>>> + kfree(chain);
>>>> + return -EINVAL;
>>>> + }
>>>> + list_for_each_entry(entity, &dev->entities, list) {
>>>> + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
>>>> + if (UVC_ENTITY_IS_UNIT(entity)) {
>>>> + if (uvc_scan_chain_entity(chain, entity) < 0) {
>>>> + uvc_trace(UVC_TRACE_DESCR, "Fallback "
>>>> + "scan for entity %d failed.\n",
>>>> + entity->id);
>>>> + kfree(chain);
>>>> + return -EINVAL;
>>>> + }
>>>> + }
>>>> + }
>>>> + list_for_each_entry(entity, &dev->entities, list) {
>>>> + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
>>>> + if (UVC_ENTITY_IS_ITERM(entity)) {
>>>> + if (uvc_scan_chain_entity(chain, entity) < 0) {
>>>> + uvc_trace(UVC_TRACE_DESCR, "Fallback "
>>>> + "scan for input terminal %d failed.\n",
>>>> + entity->id);
>>>> + kfree(chain);
>>>> + return -EINVAL;
>>>> + }
>>>> + }
>>>> + }
>>>> +
>>>> + list_add_tail(&chain->list, &dev->chains);
>>>> + uvc_trace(UVC_TRACE_PROBE, "Video chain found by fallback heuristic: "
>>>> + "%s.\n", uvc_print_chain(chain));
>>>> + return 0;
>>>> +}
>>>> +
>>>> +
>>>> +/*
>>>> * Scan the device for video chains and register video devices.
>>>> *
>>>> * Chains are scanned starting at their output terminals and walked
>>>> backwards.
>>>> @@ -1619,8 +1698,16 @@ static int uvc_scan_device(struct uvc_device
>>>> *dev)
>>>> }
>>>>
>>>> if (list_empty(&dev->chains)) {
>>>> - uvc_printk(KERN_INFO, "No valid video chain found.\n");
>>>> - return -1;
>>>> + uvc_trace(UVC_TRACE_PROBE, "No valid video chain found. "
>>>> + "Start fallback heuristic.\n");
>>>> + if (uvc_scan_fallback(dev) != 0) {
>>>> + uvc_printk(KERN_INFO, "No valid video chain found.\n");
>>>> + return -1;
>>>> + }
>>>> + uvc_printk(KERN_INFO, "Had trouble probing a valid video "
>>>> + "chain, but fallback heuristic succeeded. "
>>>> + "Usually it means this device will work "
>>>> + "just fine despite errors that may follow.\n");
>>>> }
>>>>
>>>> return 0;
>>>> --
>>>> 2.5.0
>>>>
>>>
>>>
>>>
>>> --
>>> ***@avoinelama.fi
>>> [phone] skype: henrik.ingo irc: hingo
>>> www.openlife.cc
>>>
>>> My LinkedIn profile: http://fi.linkedin.com/pub/henrik-ingo/3/232/8a7
>>>
>>>
>>> ------------------------------------------------------------------------------
>>> Transform Data into Opportunity.
>>> Accelerate data analysis in your applications with
>>> Intel Data Analytics Acceleration Library.
>>> Click to learn more.
>>> http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140
>>> _______________________________________________
>>> Linux-uvc-devel mailing list
>>> Linux-uvc-***@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-uvc-devel
>>
>> ------------------------------------------------------------------------------
>> Transform Data into Opportunity.
>> Accelerate data analysis in your applications with
>> Intel Data Analytics Acceleration Library.
>> Click to learn more.
>> http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140
>> _______________________________________________
>> Linux-uvc-devel mailing list
>> Linux-uvc-***@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-uvc-devel
>>
>
>
>

[Henrik Ingo]

Ah, thanks for the info. I wasn't aware of that. I look forward to
upgrading my Ubuntu then :-)

This news makes it less urgent for myself to get it into upstream, but
I will of course stay active on this mailing list for as long as
necessary.

henrik

On Wed, Mar 9, 2016 at 3:48 PM, Samir Ibradžić <***@gmail.com> wrote:
> Henrik,
>
> The patch landed in Ubuntu 16.04 beta, that fact makes it bit difficult
> for me to test it. I can test it against Ubuntu 4.2 kernel, if it makes
> any sense.
>
> R,
> S
>
> On 2016年03月09日 19:35, Henrik Ingo wrote:
>> Hi Anouk
>>
>> Is this a reply specifically to my patch? If yes, could you be so kind
>> to do the following to verify:
>>
>> On a kernel without this patch:
>> sudo rmmod uvcvideo
>> sudo modprobe uvcvideo trace=0xffff
>> dmesg > dmesg-broken.log
>>
>> On a kernel with this patch (or just compiling the module
>> independently and loading it):
>> sudo rmmod uvcvideo
>> sudo modprobe uvcvideo trace=0xffff
>> dmesg > dmesg-scan-fallback-patch.log
>>
>> Then post both of the *.log files to this list (maybe as a separate
>> thread, I'll see it).
>>
>> henrik
>>
>>
>> On Wed, Mar 9, 2016 at 12:19 PM, Anouk Wipprecht
>> <***@hotmail.com> wrote:
>>> Hi list -
>>>
>>> Camera Logitech c920 is not in the list,
>>> can this one be added?
>>>
>>>> Date: Wed, 9 Mar 2016 11:55:40 +0200
>>>> From: ***@avoinelama.fi
>>>> To: linux-uvc-***@lists.sourceforge.net;
>>>> ***@ideasonboard.com; ***@gmail.com; ***@gmail.com
>>>> Subject: Re: [linux-uvc-devel] [PATCH v2] uvcvideo: uvc_scan_fallback()
>>>> for webcams with broken chain descriptors
>>>
>>>>
>>>> It seems we've passed the 2-month anniversary of this patch without a
>>>> single response.
>>>>
>>>> Is there any fallback plan for situations like this (like a more
>>>> active mailing list where this could be submitted)? Or should I just
>>>> keep waiting, for next Christmas?
>>>>
>>>> henrik
>>>>
>>>> On Fri, Jan 8, 2016 at 10:51 PM, Henrik Ingo <***@avoinelama.fi>
>>>> wrote:
>>>>> Some devices have invalid baSourceID references, causing
>>>>> uvc_scan_chain()
>>>>> to fail, but if we just take the entities we can find and put them
>>>>> together in the most sensible chain we can think of, turns out they do
>>>>> work anyway. Note: This heuristic assumes there is a single chain.
>>>>>
>>>>> At the time of writing, devices known to have such a broken chain are
>>>>> - Acer Integrated Camera (5986:055a)
>>>>> - Realtek rtl157a7 (0bda:57a7)
>>>>>
>>>>> ---
>>>>>
>>>>> Based on feedback from Laurent, this is the second attempt at fixing
>>>>> the Acer Integrated Camera but also any other cameras with the
>>>>> same problem, now or in the future, in a generic way. I will send
>>>>> a separate email on some design decisions taken.
>>>>>
>>>>> v2: added missing kfree(chain)
>>>>>
>>>>> Signed-off-by: Henrik Ingo <***@avoinelama.fi>
>>>>> ---
>>>>> drivers/media/usb/uvc/uvc_driver.c | 91
>>>>> +++++++++++++++++++++++++++++++++++++-
>>>>> 1 file changed, 89 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c
>>>>> b/drivers/media/usb/uvc/uvc_driver.c
>>>>> index fae81ef0e0d2..4b8aae7cb1da 100644
>>>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>>>> @@ -1575,6 +1575,85 @@ static const char *uvc_print_chain(struct
>>>>> uvc_video_chain *chain)
>>>>> }
>>>>>
>>>>> /*
>>>>> + * Fallback heuristic for devices that don't define valid chain, but
>>>>> seem to work anyway.
>>>>> + *
>>>>> + * Some devices have invalid baSourceID references, causing
>>>>> uvc_scan_chain()
>>>>> + * to fail, but if we just take the entities we can find and put them
>>>>> + * together in the most sensible chain we can think of, turns out they
>>>>> do
>>>>> + * work anyway. Note: This heuristic assumes there is a single chain.
>>>>> + *
>>>>> + * At the time of writing, devices known to have such a broken chain
>>>>> are
>>>>> + * - Acer Integrated Camera (5986:055a)
>>>>> + * - Realtek rtl157a7 (0bda:57a7)
>>>>> + */
>>>>> +static int uvc_scan_fallback(struct uvc_device *dev)
>>>>> +{
>>>>> + struct uvc_video_chain *chain;
>>>>> + struct uvc_entity *entity;
>>>>> +
>>>>> + chain = kzalloc(sizeof(*chain), GFP_KERNEL);
>>>>> + if (chain == NULL)
>>>>> + return -ENOMEM;
>>>>> + INIT_LIST_HEAD(&chain->entities);
>>>>> + mutex_init(&chain->ctrl_mutex);
>>>>> + chain->dev = dev;
>>>>> + v4l2_prio_init(&chain->prio);
>>>>> +
>>>>> + /* We take whatever entities that are there, and just
>>>>> + * put them in a list in the order:
>>>>> + * OT <- any XU and PU <- IT
>>>>> + */
>>>>> + list_for_each_entry(entity, &dev->entities, list) {
>>>>> + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
>>>>> + if (UVC_ENTITY_IS_OTERM(entity)) {
>>>>> + if (uvc_scan_chain_entity(chain, entity) < 0) {
>>>>> + uvc_trace(UVC_TRACE_DESCR, "Fallback "
>>>>> + "scan for output terminal %d failed.\n",
>>>>> + entity->id);
>>>>> + kfree(chain);
>>>>> + return -EINVAL;
>>>>> + }
>>>>> + }
>>>>> + }
>>>>> + /* At this point we accept pretty much anything, but there
>>>>> + * absolutely must be at least one output terminal. */
>>>>> + if (list_empty(&chain->list)) {
>>>>> + kfree(chain);
>>>>> + return -EINVAL;
>>>>> + }
>>>>> + list_for_each_entry(entity, &dev->entities, list) {
>>>>> + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
>>>>> + if (UVC_ENTITY_IS_UNIT(entity)) {
>>>>> + if (uvc_scan_chain_entity(chain, entity) < 0) {
>>>>> + uvc_trace(UVC_TRACE_DESCR, "Fallback "
>>>>> + "scan for entity %d failed.\n",
>>>>> + entity->id);
>>>>> + kfree(chain);
>>>>> + return -EINVAL;
>>>>> + }
>>>>> + }
>>>>> + }
>>>>> + list_for_each_entry(entity, &dev->entities, list) {
>>>>> + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
>>>>> + if (UVC_ENTITY_IS_ITERM(entity)) {
>>>>> + if (uvc_scan_chain_entity(chain, entity) < 0) {
>>>>> + uvc_trace(UVC_TRACE_DESCR, "Fallback "
>>>>> + "scan for input terminal %d failed.\n",
>>>>> + entity->id);
>>>>> + kfree(chain);
>>>>> + return -EINVAL;
>>>>> + }
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + list_add_tail(&chain->list, &dev->chains);
>>>>> + uvc_trace(UVC_TRACE_PROBE, "Video chain found by fallback heuristic: "
>>>>> + "%s.\n", uvc_print_chain(chain));
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +
>>>>> +/*
>>>>> * Scan the device for video chains and register video devices.
>>>>> *
>>>>> * Chains are scanned starting at their output terminals and walked
>>>>> backwards.
>>>>> @@ -1619,8 +1698,16 @@ static int uvc_scan_device(struct uvc_device
>>>>> *dev)
>>>>> }
>>>>>
>>>>> if (list_empty(&dev->chains)) {
>>>>> - uvc_printk(KERN_INFO, "No valid video chain found.\n");
>>>>> - return -1;
>>>>> + uvc_trace(UVC_TRACE_PROBE, "No valid video chain found. "
>>>>> + "Start fallback heuristic.\n");
>>>>> + if (uvc_scan_fallback(dev) != 0) {
>>>>> + uvc_printk(KERN_INFO, "No valid video chain found.\n");
>>>>> + return -1;
>>>>> + }
>>>>> + uvc_printk(KERN_INFO, "Had trouble probing a valid video "
>>>>> + "chain, but fallback heuristic succeeded. "
>>>>> + "Usually it means this device will work "
>>>>> + "just fine despite errors that may follow.\n");
>>>>> }
>>>>>
>>>>> return 0;
>>>>> --
>>>>> 2.5.0
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> ***@avoinelama.fi
>>>> [phone] skype: henrik.ingo irc: hingo
>>>> www.openlife.cc
>>>>
>>>> My LinkedIn profile: http://fi.linkedin.com/pub/henrik-ingo/3/232/8a7
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>> Transform Data into Opportunity.
>>>> Accelerate data analysis in your applications with
>>>> Intel Data Analytics Acceleration Library.
>>>> Click to learn more.
>>>> http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140
>>>> _______________________________________________
>>>> Linux-uvc-devel mailing list
>>>> Linux-uvc-***@lists.sourceforge.net
>>>> https://lists.sourceforge.net/lists/listinfo/linux-uvc-devel
>>>
>>> ------------------------------------------------------------------------------
>>> Transform Data into Opportunity.
>>> Accelerate data analysis in your applications with
>>> Intel Data Analytics Acceleration Library.
>>> Click to learn more.
>>> http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140
>>> _______________________________________________
>>> Linux-uvc-devel mailing list
>>> Linux-uvc-***@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-uvc-devel
>>>
>>
>>
>>
>
> ------------------------------------------------------------------------------
> Transform Data into Opportunity.
> Accelerate data analysis in your applications with
> Intel Data Analytics Acceleration Library.
> Click to learn more.
> http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140
> _______________________________________________
> Linux-uvc-devel mailing list
> Linux-uvc-***@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-uvc-devel



--
***@avoinelama.fi
[phone] skype: henrik.ingo irc: hingo
www.openlife.cc

My LinkedIn profile: http://fi.linkedin.com/pub/henrik-ingo/3/232/8a7

[Laurent Pinchart]

Hi Henrik,

On Wednesday 09 March 2016 11:55:40 Henrik Ingo wrote:
> It seems we've passed the 2-month anniversary of this patch without a
> single response.
>
> Is there any fallback plan for situations like this (like a more
> active mailing list where this could be submitted)? Or should I just
> keep waiting, for next Christmas?

I wish I'd receive free time for Christmas, but I don't think that will be the
case, so I'll review the patch now instead.

> On Fri, Jan 8, 2016 at 10:51 PM, Henrik Ingo wrote:
> > Some devices have invalid baSourceID references, causing uvc_scan_chain()
> > to fail, but if we just take the entities we can find and put them
> > together in the most sensible chain we can think of, turns out they do
> > work anyway. Note: This heuristic assumes there is a single chain.
> >
> > At the time of writing, devices known to have such a broken chain are
> >
> > - Acer Integrated Camera (5986:055a)
> > - Realtek rtl157a7 (0bda:57a7)
> >
> > ---
> >
> > Based on feedback from Laurent, this is the second attempt at fixing
> > the Acer Integrated Camera but also any other cameras with the
> > same problem, now or in the future, in a generic way. I will send
> > a separate email on some design decisions taken.
> >
> > v2: added missing kfree(chain)
> >
> > Signed-off-by: Henrik Ingo <***@avoinelama.fi>
> > ---
> >
> > drivers/media/usb/uvc/uvc_driver.c | 91 ++++++++++++++++++++++++++++++++-
> > 1 file changed, 89 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > b/drivers/media/usb/uvc/uvc_driver.c index fae81ef0e0d2..4b8aae7cb1da
> > 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1575,6 +1575,85 @@ static const char *uvc_print_chain(struct
> > uvc_video_chain *chain)>
> > }
> >
> > /*
> > + * Fallback heuristic for devices that don't define valid chain, but seem
> > to work anyway.

Please keep lines within the 80 columns limit. checkpatch.pl should warn you
about that.

> > + *
> > + * Some devices have invalid baSourceID references, causing
> > uvc_scan_chain()
> > + * to fail, but if we just take the entities we can find and put them
> > + * together in the most sensible chain we can think of, turns out they do
> > + * work anyway. Note: This heuristic assumes there is a single chain.
> > + *
> > + * At the time of writing, devices known to have such a broken chain are
> > + * - Acer Integrated Camera (5986:055a)
> > + * - Realtek rtl157a7 (0bda:57a7)
> > + */

There's one additional issue, the entities in the chain will still have their
baSourceID set to an invalid value. This will cause trouble in the
uvc_mc_create_links() function. You should patch the baSourceID when building
the chain to fix this. Please see below for one possible implementation.

> > +static int uvc_scan_fallback(struct uvc_device *dev)
> > +{
> > + struct uvc_video_chain *chain;
> > + struct uvc_entity *entity;
> > +
> > + chain = kzalloc(sizeof(*chain), GFP_KERNEL);
> > + if (chain == NULL)
> > + return -ENOMEM;
> > + INIT_LIST_HEAD(&chain->entities);
> > + mutex_init(&chain->ctrl_mutex);
> > + chain->dev = dev;
> > + v4l2_prio_init(&chain->prio);

To avoid code duplication I would extract the above code into a separate
function. You can call it uvc_alloc_chain and write it as

static struct uvc_video_chain *uvc_alloc_chain(struct uvc_device *dev)
{
struct uvc_video_chain *chain;

chain = kzalloc(sizeof(*chain), GFP_KERNEL);
if (chain == NULL)
return NULL;

INIT_LIST_HEAD(&chain->entities);
mutex_init(&chain->ctrl_mutex);
chain->dev = dev;
v4l2_prio_init(&chain->prio);

return chain;
}

> > + /* We take whatever entities that are there, and just
> > + * put them in a list in the order:
> > + * OT <- any XU and PU <- IT
> > + */
> > + list_for_each_entry(entity, &dev->entities, list) {
> > + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;

The flag only needs to be set for output terminals, and it is already set by
uvc_scan_device() anyway, so you can skip this here and below.

> > + if (UVC_ENTITY_IS_OTERM(entity)) {
> > + if (uvc_scan_chain_entity(chain, entity) < 0) {
> > + uvc_trace(UVC_TRACE_DESCR, "Fallback "
> > + "scan for output terminal %d
> > failed.\n",
> > + entity->id);

uvc_scan_chain_entity() will print a message in case of failure, there's no
need to add another one.

> > + kfree(chain);
> > + return -EINVAL;

Instead of scattering kfree() calls over the function it would be better to
centralize error handling code. You can add

error:
kfree(chain);
return -EINVAL;

at the end of the function and just goto error; when appropriate.

> > + }
> > + }
> > + }

Please add blank lines between code blocks (here and below), that improves
code readability by making the related lines of code stand out.

> > + /* At this point we accept pretty much anything, but there
> > + * absolutely must be at least one output terminal. */

Please terminate the comments with */ on an empty line to match the coding
style of the driver. Additionally, while you should keep lines below the 80
characters limit, there's nothing wrong with reaching it :-)

/* At this point we accept pretty much anything, but there absolutely
* must be at least one output terminal.
*/

> > + if (list_empty(&chain->list)) {
> > + kfree(chain);
> > + return -EINVAL;
> > + }
> > + list_for_each_entry(entity, &dev->entities, list) {
> > + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
> > + if (UVC_ENTITY_IS_UNIT(entity)) {

I would restrict this to PUs and XUs for now, other units would be more
complex to handle. For the XUs I would further restrict support to XUs with
exactly one source.

> > + if (uvc_scan_chain_entity(chain, entity) < 0) {
> > + uvc_trace(UVC_TRACE_DESCR, "Fallback "
> > + "scan for entity %d failed.\n",
> > + entity->id);
> > + kfree(chain);
> > + return -EINVAL;
> > + }
> > + }
> > + }
> > + list_for_each_entry(entity, &dev->entities, list) {
> > + entity->flags |= UVC_ENTITY_FLAG_DEFAULT;
> > + if (UVC_ENTITY_IS_ITERM(entity)) {
> > + if (uvc_scan_chain_entity(chain, entity) < 0) {
> > + uvc_trace(UVC_TRACE_DESCR, "Fallback "
> > + "scan for input terminal %d
> > failed.\n", + entity->id);
> > + kfree(chain);
> > + return -EINVAL;
> > + }
> > + }
> > + }
> > +
> > + list_add_tail(&chain->list, &dev->chains);
> > + uvc_trace(UVC_TRACE_PROBE, "Video chain found by fallback
> > heuristic: "
> > + "%s.\n",
> > uvc_print_chain(chain));

This is one of the cases where the 80 characters per line limit can be
relaxed: splitting strings across multiple lines is frowned upon.

> > + return 0;
> > +}


As we only support devices with exactly one input and one output terminal, I'd
write it this way (completely untested code).

static int uvc_scan_fallback(struct uvc_device *dev)
{
struct uvc_video_chain *chain;
struct uvc_entity *iterm = NULL;
struct uvc_entity *oterm = NULL;
struct uvc_entity *entity;
struct uvc_entity *prev;

/* Start by locating the input and output terminals. We only support
* devices with exactly one of each for now.
*/
list_for_each_entry(entity, &dev->entities, list) {
if (UVC_ENTITY_IS_ITERM(entity)) {
if (iterm)
return -EINVAL;
iterm = entity;
}

if (UVC_ENTITY_IS_OTERM(entity)) {
if (oterm)
return -EINVAL;
oterm = entity;
}
}

if (iterm == NULL || oterm == NULL)
return -EINVAL;

/* Allocate the chain and fill it. */
chain = uvc_alloc_chain(dev);
if (chain == NULL)
return -ENOMEM;

if (uvc_scan_chain_entity(chain, oterm) < 0)
goto error;

prev = oterm;

list_for_each_entry(entity, &dev->entities, list) {
if (entity->type != UVC_VC_PROCESSING_UNIT &&
entity->type != UVC_VC_EXTENSION_UNIT)
continue;

if (entity->num_pads != 2)
continue;

if (uvc_scan_chain_entity(chain, entity) < 0)
goto error;

prev->baSourceID[0] = entity->id;
prev = entity;
}

if (uvc_scan_chain_entity(chain, iterm) < 0)
goto error;

prev->baSourceID[0] = iterm->id;

list_add_tail(&chain->list, &dev->chains);

uvc_trace(UVC_TRACE_PROBE,
"Found a video chain by fallback heuristic (%s).\n",
uvc_print_chain(chain));

return 0;

error:
kfree(chain);
return -EINVAL;
}

> > +
> > +

A single blank line between functions is enough.

> > +/*
> > * Scan the device for video chains and register video devices.
> > *
> > * Chains are scanned starting at their output terminals and walked
> > backwards.
> > @@ -1619,8 +1698,16 @@ static int uvc_scan_device(struct uvc_device *dev)
> > }
> >
> > if (list_empty(&dev->chains)) {
> > - uvc_printk(KERN_INFO, "No valid video chain found.\n");
> > - return -1;
> > + uvc_trace(UVC_TRACE_PROBE, "No valid video chain found. "
> > + "Start fallback heuristic.\n");
> > + if (uvc_scan_fallback(dev) != 0) {
> > + uvc_printk(KERN_INFO, "No valid video chain
> > found.\n");
> > + return -1;
> > + }
> > + uvc_printk(KERN_INFO, "Had trouble probing a valid video "
> > + "chain, but fallback heuristic succeeded.
> > "
> > + "Usually it means this device will work "
> > + "just fine despite errors that may
> > follow.\n");

That's a very long message, maybe a bit too verbose. Given that the
uvc_scan_fallback function already prints a message on success, I think we can
simplify this block of code to just the following:

if (list_empty(&dev->chains))
uvc_scan_fallback(dev);

if (list_empty(&dev->chains)) {
uvc_printk(KERN_INFO, "No valid video chain found.\n");
return -1;
}

> > }
> >
> > return 0;

--
Regards,

Laurent Pinchart

[Henrik Ingo]

Hi Laurent

Thanks for a proper review and even help in simplifying the core of
this code. Several things you point out are things I was wondering
about, but chose the more conservative approach for my first
submission. (Like use of goto or verbose status messages.)

I only have one question before I start:

On Thu, Mar 17, 2016 at 3:51 PM, Laurent Pinchart
<***@ideasonboard.com> wrote:
> I would restrict this to PUs and XUs for now, other units would be more
> complex to handle. For the XUs I would further restrict support to XUs with
> exactly one source.
>
...
> As we only support devices with exactly one input and one output terminal, I'd
> write it this way (completely untested code).
>
> static int uvc_scan_fallback(struct uvc_device *dev)
> {
> struct uvc_video_chain *chain;
> struct uvc_entity *iterm = NULL;
> struct uvc_entity *oterm = NULL;
> struct uvc_entity *entity;
> struct uvc_entity *prev;
>
> /* Start by locating the input and output terminals. We only support
> * devices with exactly one of each for now.
> */
> list_for_each_entry(entity, &dev->entities, list) {
> if (UVC_ENTITY_IS_ITERM(entity)) {
> if (iterm)
> return -EINVAL;
> iterm = entity;
> }
>
> if (UVC_ENTITY_IS_OTERM(entity)) {
> if (oterm)
> return -EINVAL;
> oterm = entity;
> }
> }
>
> if (iterm == NULL || oterm == NULL)
> return -EINVAL;
>
> /* Allocate the chain and fill it. */
> chain = uvc_alloc_chain(dev);
> if (chain == NULL)
> return -ENOMEM;
>
> if (uvc_scan_chain_entity(chain, oterm) < 0)
> goto error;
>
> prev = oterm;
>
> list_for_each_entry(entity, &dev->entities, list) {
> if (entity->type != UVC_VC_PROCESSING_UNIT &&
> entity->type != UVC_VC_EXTENSION_UNIT)
> continue;


To be precise, and just to confirm I understand this correctly: The
above code would actually "support" also devices with more than one
input and output terminal, but will ignore all but the first one in
such case. Such a device then may or may not work or be useful once
loaded, depending on how important the ignored units were. Is this
correct?

henrik


--
***@avoinelama.fi
[phone] skype: henrik.ingo irc: hingo
www.openlife.cc

My LinkedIn profile: http://fi.linkedin.com/pub/henrik-ingo/3/232/8a7

[Laurent Pinchart]

Hi Henrik,

Believe me or not, I'm really ashamed for not taking more time to respond to
your e-mails. Please rest assure that your efforts are appreciated, and please
feel free to ping me as firmly as needed.

On Friday 18 Mar 2016 09:23:56 Henrik Ingo wrote:
> Hi Laurent
>
> Thanks for a proper review and even help in simplifying the core of
> this code. Several things you point out are things I was wondering
> about, but chose the more conservative approach for my first
> submission. (Like use of goto or verbose status messages.)
>
> I only have one question before I start:
>
> On Thu, Mar 17, 2016 at 3:51 PM, Laurent Pinchart wrote:
> > I would restrict this to PUs and XUs for now, other units would be more
> > complex to handle. For the XUs I would further restrict support to XUs
> > with exactly one source.
>
> ...
>
> > As we only support devices with exactly one input and one output terminal,
> > I'd write it this way (completely untested code).
> >
> > static int uvc_scan_fallback(struct uvc_device *dev)
> > {
> >
> > struct uvc_video_chain *chain;
> > struct uvc_entity *iterm = NULL;
> > struct uvc_entity *oterm = NULL;
> > struct uvc_entity *entity;
> > struct uvc_entity *prev;
> >
> > /* Start by locating the input and output terminals. We only
> > support
> > * devices with exactly one of each for now.
> > */
> > list_for_each_entry(entity, &dev->entities, list) {
> > if (UVC_ENTITY_IS_ITERM(entity)) {
> > if (iterm)
> > return -EINVAL;
> > iterm = entity;
> > }
> >
> > if (UVC_ENTITY_IS_OTERM(entity)) {
> > if (oterm)
> > return -EINVAL;
> > oterm = entity;
> > }
> > }
> >
> > if (iterm == NULL || oterm == NULL)
> > return -EINVAL;
> >
> > /* Allocate the chain and fill it. */
> > chain = uvc_alloc_chain(dev);
> > if (chain == NULL)
> > return -ENOMEM;
> >
> > if (uvc_scan_chain_entity(chain, oterm) < 0)
> > goto error;
> >
> > prev = oterm;
> >
> > list_for_each_entry(entity, &dev->entities, list) {
> > if (entity->type != UVC_VC_PROCESSING_UNIT &&
> > entity->type != UVC_VC_EXTENSION_UNIT)
> > continue;
>
> To be precise, and just to confirm I understand this correctly: The
> above code would actually "support" also devices with more than one
> input and output terminal, but will ignore all but the first one in
> such case. Such a device then may or may not work or be useful once
> loaded, depending on how important the ignored units were. Is this
> correct?

The uvc_scan_fallback() function will return -EINVAL if it finds multiple
input or output terminals, so devices with more than one input terminal or
output terminal will not be supported (assuming their USB descriptors are
invalid of course, compliant devices with multiple input and output terminals
will still be supported).

--
Regards,

Laurent Pinchart

[Henrik Ingo]

On Wed, Apr 27, 2016 at 5:09 PM, Laurent Pinchart
<***@ideasonboard.com> wrote:
> Believe me or not, I'm really ashamed for not taking more time to respond to
> your e-mails. Please rest assure that your efforts are appreciated, and please
> feel free to ping me as firmly as needed.

Thanks :-) Actually, I wasn't held back by waiting for your reply,
this time it was myself who was slow to context switch back to work on
this patch. Finally I've been able to do it over this weekend. The
patch was sent to this mailing list a minute ago.

The new patch is 100% according to your suggestions, with one twist:
For iterating over the XUs and PUs, I used list_for_each_prev()
instead of list_for_each_entry(). The reason is that iterating
backwards happens to produce the "most correct" chain for the
particular Acer camera I have on my laptop (and that many other people
seem to own as well). By this I mean that the baSourceIDs that do
point to another valid unit, remain unchanged and we only patch the
invalid reference from pointing to 6 to pointing to 4.

This was perhaps vanity on my part, since in general we don't do any
effort to preserve baSourceIDs once we're in the fallback heuristic.
So strictly speaking the order of XU and PU units should be considered
random, which just "accidentally on purpose" happens to be the correct
order for this device. But I figured it wouldn't hurt to do it for the
one device we're actually working with here, since I could do it
easily, without harming generality of the patch. I hope you don't mind
the additional line of code, since I now had to call list_entry()
separately.

The dmesg output is attached. The "most correct" order of units we
ended up producing is: OT 3 <- XU 4 <- PU 2 <- IT 1

henrik
--
***@avoinelama.fi
[phone] skype: henrik.ingo irc: hingo
www.openlife.cc

My LinkedIn profile: http://fi.linkedin.com/pub/henrik-ingo/3/232/8a7

[Henrik Ingo]

Some devices have invalid baSourceID references, causing uvc_scan_chain()
to fail, but if we just take the entities we can find and put them
together in the most sensible chain we can think of, turns out they do
work anyway. Note: This heuristic assumes there is a single chain.

At the time of writing, devices known to have such a broken chain are
- Acer Integrated Camera (5986:055a)
- Realtek rtl157a7 (0bda:57a7)

---
Implements fixes from review on March 17th.

Signed-off-by: Henrik Ingo <***@avoinelama.fi>
Suggested-by: Arno Wilhelm <***@gmail.com>
---
drivers/media/usb/uvc/uvc_driver.c | 112 +++++++++++++++++++++++++++++++++++--
1 file changed, 106 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index fae81ef0e0d2..2a01689605e5 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1574,6 +1574,108 @@ static const char *uvc_print_chain(struct uvc_video_chain *chain)
return buffer;
}

+static struct uvc_video_chain *uvc_alloc_chain(struct uvc_device *dev)
+{
+ struct uvc_video_chain *chain;
+
+ chain = kzalloc(sizeof(*chain), GFP_KERNEL);
+ if (chain == NULL)
+ return NULL;
+
+ INIT_LIST_HEAD(&chain->entities);
+ mutex_init(&chain->ctrl_mutex);
+ chain->dev = dev;
+ v4l2_prio_init(&chain->prio);
+
+ return chain;
+}
+
+/*
+ * Fallback heuristic for devices that don't define valid chain, but seem to
+ * work anyway.
+ *
+ * Some devices have invalid baSourceID references, causing uvc_scan_chain()
+ * to fail, but if we just take the entities we can find and put them
+ * together in the most sensible chain we can think of, turns out they do
+ * work anyway. Note: This heuristic assumes there is a single chain.
+ *
+ * At the time of writing, devices known to have such a broken chain are
+ * - Acer Integrated Camera (5986:055a)
+ * - Realtek rtl157a7 (0bda:57a7)
+ */
+static int uvc_scan_fallback(struct uvc_device *dev)
+{
+ struct uvc_video_chain *chain;
+ struct uvc_entity *iterm = NULL;
+ struct uvc_entity *oterm = NULL;
+ struct uvc_entity *entity;
+ struct uvc_entity *prev;
+ struct list_head *cursor;
+
+ /* Start by locating the input and output terminals. We only support
+ * devices with exactly one of each for now.
+ */
+ list_for_each_entry(entity, &dev->entities, list) {
+ if (UVC_ENTITY_IS_ITERM(entity)) {
+ if (iterm)
+ return -EINVAL;
+ iterm = entity;
+ }
+
+ if (UVC_ENTITY_IS_OTERM(entity)) {
+ if (oterm)
+ return -EINVAL;
+ oterm = entity;
+ }
+ }
+
+ if (iterm == NULL || oterm == NULL)
+ return -EINVAL;
+
+ /* Allocate the chain and fill it. */
+ chain = uvc_alloc_chain(dev);
+ if (chain == NULL)
+ return -ENOMEM;
+
+ if (uvc_scan_chain_entity(chain, oterm) < 0)
+ goto error;
+
+ prev = oterm;
+
+ list_for_each_prev(cursor, &dev->entities) {
+ entity = list_entry(cursor, struct uvc_entity, list);
+ if (entity->type != UVC_VC_PROCESSING_UNIT &&
+ entity->type != UVC_VC_EXTENSION_UNIT)
+ continue;
+
+ if (entity->num_pads != 2)
+ continue;
+
+ if (uvc_scan_chain_entity(chain, entity) < 0)
+ goto error;
+
+ prev->baSourceID[0] = entity->id;
+ prev = entity;
+ }
+
+ if (uvc_scan_chain_entity(chain, iterm) < 0)
+ goto error;
+
+ prev->baSourceID[0] = iterm->id;
+
+ list_add_tail(&chain->list, &dev->chains);
+
+ uvc_trace(UVC_TRACE_PROBE,
+ "Found a video chain by fallback heuristic (%s).\n",
+ uvc_print_chain(chain));
+
+ return 0;
+
+error:
+ kfree(chain);
+ return -EINVAL;
+}
+
/*
* Scan the device for video chains and register video devices.
*
@@ -1596,15 +1698,10 @@ static int uvc_scan_device(struct uvc_device *dev)
if (term->chain.next || term->chain.prev)
continue;

- chain = kzalloc(sizeof(*chain), GFP_KERNEL);
+ chain = uvc_alloc_chain(dev);
if (chain == NULL)
return -ENOMEM;

- INIT_LIST_HEAD(&chain->entities);
- mutex_init(&chain->ctrl_mutex);
- chain->dev = dev;
- v4l2_prio_init(&chain->prio);
-
term->flags |= UVC_ENTITY_FLAG_DEFAULT;

if (uvc_scan_chain(chain, term) < 0) {
@@ -1618,6 +1715,9 @@ static int uvc_scan_device(struct uvc_device *dev)
list_add_tail(&chain->list, &dev->chains);
}

+ if (list_empty(&dev->chains))
+ uvc_scan_fallback(dev);
+
if (list_empty(&dev->chains)) {
uvc_printk(KERN_INFO, "No valid video chain found.\n");
return -1;
--
2.5.0

[Laurent Pinchart]

Hello Henrik,

On Thursday 29 Sep 2016 16:36:25 Henrik Ingo wrote:
> Hi Laurent
>
> Did you intentionally leave the list out? If no, feel free to add it back.

No, I didn't, I think I replied to your private ping. Sorry about that, fixed
now.

> On Thu, Sep 29, 2016 at 1:41 PM, Laurent Pinchart wrote:
> > > +/*
> > > + * Fallback heuristic for devices that don't define valid chain, but
> > > seem to
> > > + * work anyway.
> > > + *
> > > + * Some devices have invalid baSourceID references, causing
> > > uvc_scan_chain()
> > > + * to fail, but if we just take the entities we can find and put them
> > > + * together in the most sensible chain we can think of, turns out they
> > > do
> > > + * work anyway. Note: This heuristic assumes there is a single chain.
> > > + *
> > > + * At the time of writing, devices known to have such a broken chain
> > > are
> > > + * - Acer Integrated Camera (5986:055a)
> > > + * - Realtek rtl157a7 (0bda:57a7)
> > > + */
> > > +static int uvc_scan_fallback(struct uvc_device *dev)
> > > +{
> > > + struct uvc_video_chain *chain;
> > > + struct uvc_entity *iterm = NULL;
> > > + struct uvc_entity *oterm = NULL;
> > > + struct uvc_entity *entity;
> > > + struct uvc_entity *prev;
> > > + struct list_head *cursor;
> > > +
> > > + /* Start by locating the input and output terminals. We only
> > > support + * devices with exactly one of each for now.
> > > + */
> > > + list_for_each_entry(entity, &dev->entities, list) {
> > > + if (UVC_ENTITY_IS_ITERM(entity)) {
> > > + if (iterm)
> > > + return -EINVAL;
> > > + iterm = entity;
> > > + }
> > > +
> > > + if (UVC_ENTITY_IS_OTERM(entity)) {
> > > + if (oterm)
> > > + return -EINVAL;
> > > + oterm = entity;
> > > + }
> > > + }
> > > +
> > > + if (iterm == NULL || oterm == NULL)
> > > + return -EINVAL;
> > > +
> > > + /* Allocate the chain and fill it. */
> > > + chain = uvc_alloc_chain(dev);
> > > + if (chain == NULL)
> > > + return -ENOMEM;
> > > +
> > > + if (uvc_scan_chain_entity(chain, oterm) < 0)
> > > + goto error;
> > > +
> > > + prev = oterm;
> > > +
> > > + list_for_each_prev(cursor, &dev->entities) {
> > > + entity = list_entry(cursor, struct uvc_entity, list);
> >
> > You can use list_for_each_entry_reverse() here to combine those two lines,
> > but I'm wondering why you need to traverse the list in the reverse
> > direction. Can't list_for_each_entry() do ?
>
> Oh, good. I tried to look, but didn't find
> list_for_each_entry_reverse() in the docs I traversed.
>
> I think I explained this in the separate email, but basically:
>
> The goal of this heuristic was to build a chain with
>
> single input -> all other entities in arbitrary order -> single output
>
> When debugging this on my Acer integrated camera, it does discover the
> following broken chain:
>
> OT 3 <- [missing 6 and presumably 5] <- XU 4 <- PU 2 <- IT 1
>
> While formally we don't care which order the middle entities end up
> being added in, I did notice that if I iterate over the list in
> reverse order, the entities on this particular camera happen to end up
> as OT 3 <- XU 4 <- PU 2 <- IT 1, whereas scanning in the forward
> direction 4 and 2 would switch places.
>
> The goal of course is not to optimize for any particular camera, but
> since either direction should be equal I picked the one that happens
> to end up minimizing the re-ordering for the camera I happened to
> have. I was thinking it's possible the same chip, or same error in
> other chips, will also have this property, in which case this is a
> good choice. If that turns out not to be the case, that's also not a
> problem. In that case it just reduces to an "easter egg" from the
> patch author I guess!
>
> I don't personally care if you want to use list_for_each_entry()
> instead, it works both ways.
>
> > If that works there's no need to resubmit the patch, I can change it when
> > applying.
>
> Thanks, please go ahead with whichever direction you prefer.

I'll use list_for_each_entry_reverse() with the following comment.

/*
* Add all Processing and Extension Units with two pads. The order
* doesn't matter much, use reverse list traversal to connect units in
* UVC descriptor order as we build the chain from output to input. This
* leads to units appearing in the order meant by the manufacturer for
* the cameras known to require this heuristic.
*/

> > > + if (entity->type != UVC_VC_PROCESSING_UNIT &&
> > > + entity->type != UVC_VC_EXTENSION_UNIT)
> >
> > Wrong indentation.
>
> I assume you will fix this as well.

Sure.

--
Regards,

Laurent Pinchart


------------------------------------------------------------------------------