* [PATCH RESEND v2 1/8] media: uvc: Extend documentation of uvc_video_clock_decode()
2022-12-02 17:02 [PATCH RESEND v2 0/8] uvcvideo: Fixes for hw timestamping Ricardo Ribalda
@ 2022-12-02 17:02 ` Ricardo Ribalda
2022-12-30 13:36 ` Laurent Pinchart
2022-12-02 17:02 ` [PATCH RESEND v2 2/8] media: uvc: Allow quirking by entity guid Ricardo Ribalda
` (6 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Ricardo Ribalda @ 2022-12-02 17:02 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Laurent Pinchart
Cc: hn.chen, linux-media, Ricardo Ribalda, linux-kernel
Make a explicit reference to uvc 1.5, explaining how the algorithm
supports the different behaviour of uvc 1.1 and 1.5.
Tested-by: HungNien Chen <hn.chen@sunplusit.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_video.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 170a008f4006..ab56e65ca324 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -517,6 +517,9 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
/*
* To limit the amount of data, drop SCRs with an SOF identical to the
* previous one.
+ * This filtering is also needed for supporting UVC 1.5. Where all the
+ * data packages of the same frame contains the same sof. In that case
+ * only the first one will match the host_sof.
*/
dev_sof = get_unaligned_le16(&data[header_size - 2]);
if (dev_sof == stream->clock.last_sof)
--
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 1/8] media: uvc: Extend documentation of uvc_video_clock_decode()
2022-12-02 17:02 ` [PATCH RESEND v2 1/8] media: uvc: Extend documentation of uvc_video_clock_decode() Ricardo Ribalda
@ 2022-12-30 13:36 ` Laurent Pinchart
0 siblings, 0 replies; 25+ messages in thread
From: Laurent Pinchart @ 2022-12-30 13:36 UTC (permalink / raw)
To: Ricardo Ribalda; +Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel
Hi Ricardo,
Thank you for the patch.
s/uvc/uvcvideo/ in the subject line.
On Fri, Dec 02, 2022 at 06:02:41PM +0100, Ricardo Ribalda wrote:
> Make a explicit reference to uvc 1.5, explaining how the algorithm
s/a explicit/an explicit/
s/uvc/UVC/
> supports the different behaviour of uvc 1.1 and 1.5.
Ditto.
> Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/usb/uvc/uvc_video.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 170a008f4006..ab56e65ca324 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -517,6 +517,9 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> /*
> * To limit the amount of data, drop SCRs with an SOF identical to the
> * previous one.
> + * This filtering is also needed for supporting UVC 1.5. Where all the
> + * data packages of the same frame contains the same sof. In that case
> + * only the first one will match the host_sof.
* To limit the amount of data, drop SCRs with an SOF identical to the
* previous one. This filtering is also needed to support UVC 1.5, where
* all the data packets of the same frame contains the same SOF. In that
* case only the first one will match the host_sof.
Conditionally-Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
I can fix this when applying if there's no other need to submit a v3.
> */
> dev_sof = get_unaligned_le16(&data[header_size - 2]);
> if (dev_sof == stream->clock.last_sof)
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH RESEND v2 2/8] media: uvc: Allow quirking by entity guid
2022-12-02 17:02 [PATCH RESEND v2 0/8] uvcvideo: Fixes for hw timestamping Ricardo Ribalda
2022-12-02 17:02 ` [PATCH RESEND v2 1/8] media: uvc: Extend documentation of uvc_video_clock_decode() Ricardo Ribalda
@ 2022-12-02 17:02 ` Ricardo Ribalda
2022-12-30 13:40 ` Laurent Pinchart
2022-12-02 17:02 ` [PATCH RESEND v2 3/8] media: uvc: Create UVC_QUIRK_IGNORE_EMPTY_TS quirk Ricardo Ribalda
` (5 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Ricardo Ribalda @ 2022-12-02 17:02 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Laurent Pinchart
Cc: hn.chen, linux-media, Ricardo Ribalda, linux-kernel
When an IP is shared by multiple devices its erratas will be shared by
all of them. Instead of creating a long list of device quirks, or
waiting for the users to report errors in their hardware lets add a
routine to add quirks based on the entity guid.
Tested-by: HungNien Chen <hn.chen@sunplusit.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_driver.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 9c05776f11d1..c63ecfd4617d 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1493,6 +1493,28 @@ static int uvc_parse_control(struct uvc_device *dev)
return 0;
}
+static const struct uvc_entity_quirk {
+ u8 guid[16];
+ u32 quirks;
+} uvc_entity_quirk[] = {
+};
+
+static void uvc_entity_quirks(struct uvc_device *dev)
+{
+ struct uvc_entity *entity;
+ int i;
+
+ list_for_each_entry(entity, &dev->entities, list) {
+ for (i = 0; i < ARRAY_SIZE(uvc_entity_quirk); i++) {
+ if (memcmp(entity->guid, uvc_entity_quirk[i].guid,
+ sizeof(entity->guid)) == 0) {
+ dev->quirks |= uvc_entity_quirk[i].quirks;
+ break;
+ }
+ }
+ }
+}
+
/* -----------------------------------------------------------------------------
* Privacy GPIO
*/
@@ -2452,6 +2474,9 @@ static int uvc_probe(struct usb_interface *intf,
goto error;
}
+ /* Apply entity based quirks */
+ uvc_entity_quirks(dev);
+
dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
dev->uvc_version >> 8, dev->uvc_version & 0xff,
udev->product ? udev->product : "<unnamed>",
--
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 2/8] media: uvc: Allow quirking by entity guid
2022-12-02 17:02 ` [PATCH RESEND v2 2/8] media: uvc: Allow quirking by entity guid Ricardo Ribalda
@ 2022-12-30 13:40 ` Laurent Pinchart
2023-01-03 15:40 ` Ricardo Ribalda
0 siblings, 1 reply; 25+ messages in thread
From: Laurent Pinchart @ 2022-12-30 13:40 UTC (permalink / raw)
To: Ricardo Ribalda; +Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel
Hi Ricardo,
Thank you for the patch.
On Fri, Dec 02, 2022 at 06:02:42PM +0100, Ricardo Ribalda wrote:
> When an IP is shared by multiple devices its erratas will be shared by
> all of them. Instead of creating a long list of device quirks, or
> waiting for the users to report errors in their hardware lets add a
> routine to add quirks based on the entity guid.
I'm not thrilled by this. An entity is not an "IP". Quirks are needed to
handle issues with particular firmware versions on particular devices.
The same entity GUID can be used by different devices running different
firmware versions, some that would require a quirk and some that
wouldn't.
> Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/usb/uvc/uvc_driver.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 9c05776f11d1..c63ecfd4617d 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1493,6 +1493,28 @@ static int uvc_parse_control(struct uvc_device *dev)
> return 0;
> }
>
> +static const struct uvc_entity_quirk {
> + u8 guid[16];
> + u32 quirks;
> +} uvc_entity_quirk[] = {
> +};
> +
> +static void uvc_entity_quirks(struct uvc_device *dev)
> +{
> + struct uvc_entity *entity;
> + int i;
unsigned int
> +
> + list_for_each_entry(entity, &dev->entities, list) {
> + for (i = 0; i < ARRAY_SIZE(uvc_entity_quirk); i++) {
> + if (memcmp(entity->guid, uvc_entity_quirk[i].guid,
> + sizeof(entity->guid)) == 0) {
> + dev->quirks |= uvc_entity_quirk[i].quirks;
> + break;
> + }
> + }
> + }
> +}
> +
> /* -----------------------------------------------------------------------------
> * Privacy GPIO
> */
> @@ -2452,6 +2474,9 @@ static int uvc_probe(struct usb_interface *intf,
> goto error;
> }
>
> + /* Apply entity based quirks */
> + uvc_entity_quirks(dev);
> +
> dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
> dev->uvc_version >> 8, dev->uvc_version & 0xff,
> udev->product ? udev->product : "<unnamed>",
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 2/8] media: uvc: Allow quirking by entity guid
2022-12-30 13:40 ` Laurent Pinchart
@ 2023-01-03 15:40 ` Ricardo Ribalda
2023-01-03 21:16 ` Laurent Pinchart
0 siblings, 1 reply; 25+ messages in thread
From: Ricardo Ribalda @ 2023-01-03 15:40 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel, Tomasz Figa
Hi Laurent
On Fri, 30 Dec 2022 at 14:40, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
>
> On Fri, Dec 02, 2022 at 06:02:42PM +0100, Ricardo Ribalda wrote:
> > When an IP is shared by multiple devices its erratas will be shared by
> > all of them. Instead of creating a long list of device quirks, or
> > waiting for the users to report errors in their hardware lets add a
> > routine to add quirks based on the entity guid.
>
> I'm not thrilled by this. An entity is not an "IP". Quirks are needed to
> handle issues with particular firmware versions on particular devices.
> The same entity GUID can be used by different devices running different
> firmware versions, some that would require a quirk and some that
> wouldn't.
Unfortunately there are ISPs that do not support firmware upgrading
that have an error on their firmware (or in this particular case a
different interpretation of the standard). Those ISPs are mounted in
different boards with a VID:PID that is chosen by the module
manufacturer.
In those cases we cannot get a list of the devices that are broken, we
could only get a sublist that we will keep updating indefinitely as
users keep reporting bugs (if they do so).
We are lucky enough that SunplusIT has been very active and provided
us a way to detect what hardware requires quirking. In those
situations where the vendor is on board and there is no upgrade
mechanism I think that this is a good compromise.
>
> > Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > drivers/media/usb/uvc/uvc_driver.c | 25 +++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 9c05776f11d1..c63ecfd4617d 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1493,6 +1493,28 @@ static int uvc_parse_control(struct uvc_device *dev)
> > return 0;
> > }
> >
> > +static const struct uvc_entity_quirk {
> > + u8 guid[16];
> > + u32 quirks;
> > +} uvc_entity_quirk[] = {
> > +};
> > +
> > +static void uvc_entity_quirks(struct uvc_device *dev)
> > +{
> > + struct uvc_entity *entity;
> > + int i;
>
> unsigned int
>
> > +
> > + list_for_each_entry(entity, &dev->entities, list) {
> > + for (i = 0; i < ARRAY_SIZE(uvc_entity_quirk); i++) {
> > + if (memcmp(entity->guid, uvc_entity_quirk[i].guid,
> > + sizeof(entity->guid)) == 0) {
> > + dev->quirks |= uvc_entity_quirk[i].quirks;
> > + break;
> > + }
> > + }
> > + }
> > +}
> > +
> > /* -----------------------------------------------------------------------------
> > * Privacy GPIO
> > */
> > @@ -2452,6 +2474,9 @@ static int uvc_probe(struct usb_interface *intf,
> > goto error;
> > }
> >
> > + /* Apply entity based quirks */
> > + uvc_entity_quirks(dev);
> > +
> > dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
> > dev->uvc_version >> 8, dev->uvc_version & 0xff,
> > udev->product ? udev->product : "<unnamed>",
> >
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 2/8] media: uvc: Allow quirking by entity guid
2023-01-03 15:40 ` Ricardo Ribalda
@ 2023-01-03 21:16 ` Laurent Pinchart
0 siblings, 0 replies; 25+ messages in thread
From: Laurent Pinchart @ 2023-01-03 21:16 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel, Tomasz Figa
Hi Ricardo,
On Tue, Jan 03, 2023 at 04:40:48PM +0100, Ricardo Ribalda wrote:
> On Fri, 30 Dec 2022 at 14:40, Laurent Pinchart wrote:
> > On Fri, Dec 02, 2022 at 06:02:42PM +0100, Ricardo Ribalda wrote:
> > > When an IP is shared by multiple devices its erratas will be shared by
> > > all of them. Instead of creating a long list of device quirks, or
> > > waiting for the users to report errors in their hardware lets add a
> > > routine to add quirks based on the entity guid.
> >
> > I'm not thrilled by this. An entity is not an "IP". Quirks are needed to
> > handle issues with particular firmware versions on particular devices.
> > The same entity GUID can be used by different devices running different
> > firmware versions, some that would require a quirk and some that
> > wouldn't.
>
> Unfortunately there are ISPs that do not support firmware upgrading
> that have an error on their firmware (or in this particular case a
> different interpretation of the standard).
I recall we've discussed that. I'll stick to the non politically correct
interpretation of the issue, and call it a bug :-)
> Those ISPs are mounted in
> different boards with a VID:PID that is chosen by the module
> manufacturer.
> In those cases we cannot get a list of the devices that are broken, we
> could only get a sublist that we will keep updating indefinitely as
> users keep reporting bugs (if they do so).
>
> We are lucky enough that SunplusIT has been very active and provided
> us a way to detect what hardware requires quirking.
Is there a guarantee that none of the newer firmware versions that do
not exhibit this bug will *not* use the same XU GUID ?
> In those
> situations where the vendor is on board and there is no upgrade
> mechanism I think that this is a good compromise.
What I'm interested in is how to prevent this kind of issues in the
future. HungNien, would you be interested in engaging with us in order
to test future ISP firmwares against Linux ? I do appreciate that you
have supported Ricardo with handling this issue. We could work on a
publicly available UVC compliance test suite. You can reply about this
privately if you would rather not discuss this topic on public mailing
lists.
> > > Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > > drivers/media/usb/uvc/uvc_driver.c | 25 +++++++++++++++++++++++++
> > > 1 file changed, 25 insertions(+)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index 9c05776f11d1..c63ecfd4617d 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -1493,6 +1493,28 @@ static int uvc_parse_control(struct uvc_device *dev)
> > > return 0;
> > > }
> > >
> > > +static const struct uvc_entity_quirk {
> > > + u8 guid[16];
> > > + u32 quirks;
> > > +} uvc_entity_quirk[] = {
> > > +};
> > > +
> > > +static void uvc_entity_quirks(struct uvc_device *dev)
> > > +{
> > > + struct uvc_entity *entity;
> > > + int i;
> >
> > unsigned int
> >
> > > +
> > > + list_for_each_entry(entity, &dev->entities, list) {
> > > + for (i = 0; i < ARRAY_SIZE(uvc_entity_quirk); i++) {
> > > + if (memcmp(entity->guid, uvc_entity_quirk[i].guid,
> > > + sizeof(entity->guid)) == 0) {
> > > + dev->quirks |= uvc_entity_quirk[i].quirks;
> > > + break;
> > > + }
> > > + }
> > > + }
> > > +}
> > > +
> > > /* -----------------------------------------------------------------------------
> > > * Privacy GPIO
> > > */
> > > @@ -2452,6 +2474,9 @@ static int uvc_probe(struct usb_interface *intf,
> > > goto error;
> > > }
> > >
> > > + /* Apply entity based quirks */
> > > + uvc_entity_quirks(dev);
> > > +
> > > dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
> > > dev->uvc_version >> 8, dev->uvc_version & 0xff,
> > > udev->product ? udev->product : "<unnamed>",
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH RESEND v2 3/8] media: uvc: Create UVC_QUIRK_IGNORE_EMPTY_TS quirk
2022-12-02 17:02 [PATCH RESEND v2 0/8] uvcvideo: Fixes for hw timestamping Ricardo Ribalda
2022-12-02 17:02 ` [PATCH RESEND v2 1/8] media: uvc: Extend documentation of uvc_video_clock_decode() Ricardo Ribalda
2022-12-02 17:02 ` [PATCH RESEND v2 2/8] media: uvc: Allow quirking by entity guid Ricardo Ribalda
@ 2022-12-02 17:02 ` Ricardo Ribalda
2022-12-30 13:45 ` Laurent Pinchart
2022-12-02 17:02 ` [PATCH RESEND v2 4/8] media: uvcvideo: Quirk for invalid dev_sof in Logi C922 Ricardo Ribalda
` (4 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Ricardo Ribalda @ 2022-12-02 17:02 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Laurent Pinchart
Cc: hn.chen, linux-media, Ricardo Ribalda, linux-kernel
Some Sunplus cameras took a borderline interpretation of the UVC 1.5
standard, and fill the PTS and SCR fields with invalid data if the
package does not contain data.
"STC must be captured when the first video data of a video frame is put
on the USB bus."
Eg:
buffer: 0xa7755c00 len 000012 header:0x8c stc 00000000 sof 0000 pts 00000000
buffer: 0xa7755c00 len 000012 header:0x8c stc 00000000 sof 0000 pts 00000000
buffer: 0xa7755c00 len 000668 header:0x8c stc 73779dba sof 070c pts 7376d37a
This borderline/buggy interpretation has been implemented in a variety
of devices, from directly Sunplus and from other OEMs that rebrand
Sunplus products.
Luckily we can identify the affected modules by looking at the guid of
one of the extension units:
VideoControl Interface Descriptor:
guidExtensionCode {82066163-7050-ab49-b8cc-b3855e8d221d}
This patch adds a new quirk to take care of this.
Complete lsusb of one of the affected cameras:
Bus 001 Device 003: ID 1bcf:2a01 Sunplus Innovation Technology Inc.
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.01
bDeviceClass 239 Miscellaneous Device
bDeviceSubClass 2 ?
bDeviceProtocol 1 Interface Association
bMaxPacketSize0 64
idVendor 0x1bcf Sunplus Innovation Technology Inc.
idProduct 0x2a01
bcdDevice 0.02
iManufacturer 1 SunplusIT Inc
iProduct 2 HanChen Wise Camera
iSerial 3 01.00.00
bNumConfigurations 1
Tested-by: HungNien Chen <hn.chen@sunplusit.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
drivers/media/usb/uvc/uvc_video.c | 8 ++++++++
drivers/media/usb/uvc/uvcvideo.h | 1 +
3 files changed, 20 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index c63ecfd4617d..80ef0f0e04b0 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1497,6 +1497,17 @@ static const struct uvc_entity_quirk {
u8 guid[16];
u32 quirks;
} uvc_entity_quirk[] = {
+ /*
+ * Some SunPlus uvc 1.5 device firmware expects that packages with
+ * no frame data are ignored by the host.
+ * Therefore it does not clear the PTS/SCR bits in the header, and
+ * breaks the timestamp decode algorithm.
+ */
+ {
+ .guid = {0x82, 0x06, 0x61, 0x63, 0x70, 0x50, 0xab, 0x49,
+ 0xb8, 0xcc, 0xb3, 0x85, 0x5e, 0x8d, 0x22, 0x1d},
+ .quirks = UVC_QUIRK_IGNORE_EMPTY_TS,
+ },
};
static void uvc_entity_quirks(struct uvc_device *dev)
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index ab56e65ca324..a5b184e71ad7 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -500,6 +500,14 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
if (len < header_size)
return;
+ /*
+ * Ignore the hardware timestamp on frames with no data on
+ * miss-behaving devices.
+ */
+ if (stream->dev->quirks & UVC_QUIRK_IGNORE_EMPTY_TS &&
+ len == header_size)
+ return;
+
/*
* Extract the timestamps:
*
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 24c911aeebce..f395b67fe95a 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -212,6 +212,7 @@
#define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400
#define UVC_QUIRK_FORCE_Y8 0x00000800
#define UVC_QUIRK_FORCE_BPP 0x00001000
+#define UVC_QUIRK_IGNORE_EMPTY_TS 0x00002000
/* Format flags */
#define UVC_FMT_FLAG_COMPRESSED 0x00000001
--
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 3/8] media: uvc: Create UVC_QUIRK_IGNORE_EMPTY_TS quirk
2022-12-02 17:02 ` [PATCH RESEND v2 3/8] media: uvc: Create UVC_QUIRK_IGNORE_EMPTY_TS quirk Ricardo Ribalda
@ 2022-12-30 13:45 ` Laurent Pinchart
2023-01-03 16:00 ` Ricardo Ribalda
2023-01-07 1:16 ` Laurent Pinchart
0 siblings, 2 replies; 25+ messages in thread
From: Laurent Pinchart @ 2022-12-30 13:45 UTC (permalink / raw)
To: Ricardo Ribalda; +Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel
Hi Ricardo,
Thank you for the patch.
On Fri, Dec 02, 2022 at 06:02:43PM +0100, Ricardo Ribalda wrote:
> Some Sunplus cameras took a borderline interpretation of the UVC 1.5
> standard, and fill the PTS and SCR fields with invalid data if the
> package does not contain data.
>
> "STC must be captured when the first video data of a video frame is put
> on the USB bus."
>
> Eg:
>
> buffer: 0xa7755c00 len 000012 header:0x8c stc 00000000 sof 0000 pts 00000000
> buffer: 0xa7755c00 len 000012 header:0x8c stc 00000000 sof 0000 pts 00000000
> buffer: 0xa7755c00 len 000668 header:0x8c stc 73779dba sof 070c pts 7376d37a
>
> This borderline/buggy interpretation has been implemented in a variety
> of devices, from directly Sunplus and from other OEMs that rebrand
> Sunplus products.
>
> Luckily we can identify the affected modules by looking at the guid of
> one of the extension units:
>
> VideoControl Interface Descriptor:
> guidExtensionCode {82066163-7050-ab49-b8cc-b3855e8d221d}
>
> This patch adds a new quirk to take care of this.
>
> Complete lsusb of one of the affected cameras:
That's not complete (but that's fine from a commit message point of
view, the full descriptors would be too long). Can you share the full
descriptors for all the devices you know are affected by this ?
> Bus 001 Device 003: ID 1bcf:2a01 Sunplus Innovation Technology Inc.
> Device Descriptor:
> bLength 18
> bDescriptorType 1
> bcdUSB 2.01
> bDeviceClass 239 Miscellaneous Device
> bDeviceSubClass 2 ?
> bDeviceProtocol 1 Interface Association
> bMaxPacketSize0 64
> idVendor 0x1bcf Sunplus Innovation Technology Inc.
> idProduct 0x2a01
> bcdDevice 0.02
> iManufacturer 1 SunplusIT Inc
> iProduct 2 HanChen Wise Camera
> iSerial 3 01.00.00
> bNumConfigurations 1
>
> Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
> drivers/media/usb/uvc/uvc_video.c | 8 ++++++++
> drivers/media/usb/uvc/uvcvideo.h | 1 +
> 3 files changed, 20 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index c63ecfd4617d..80ef0f0e04b0 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1497,6 +1497,17 @@ static const struct uvc_entity_quirk {
> u8 guid[16];
> u32 quirks;
> } uvc_entity_quirk[] = {
> + /*
> + * Some SunPlus uvc 1.5 device firmware expects that packages with
s/uvc/UVC/
> + * no frame data are ignored by the host.
If it's two paragraphs, you need a blank line in-between. If it's a
single paragraph, you must not add a line break.
> + * Therefore it does not clear the PTS/SCR bits in the header, and
> + * breaks the timestamp decode algorithm.
> + */
> + {
> + .guid = {0x82, 0x06, 0x61, 0x63, 0x70, 0x50, 0xab, 0x49,
> + 0xb8, 0xcc, 0xb3, 0x85, 0x5e, 0x8d, 0x22, 0x1d},
> + .quirks = UVC_QUIRK_IGNORE_EMPTY_TS,
> + },
> };
>
> static void uvc_entity_quirks(struct uvc_device *dev)
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index ab56e65ca324..a5b184e71ad7 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -500,6 +500,14 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> if (len < header_size)
> return;
>
> + /*
> + * Ignore the hardware timestamp on frames with no data on
> + * miss-behaving devices.
> + */
Could you please expand this comment to explain the issue ? Having to
use git blame and read the commit message to understand why the check is
here isn't very convenient.
> + if (stream->dev->quirks & UVC_QUIRK_IGNORE_EMPTY_TS &&
> + len == header_size)
> + return;
> +
> /*
> * Extract the timestamps:
> *
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 24c911aeebce..f395b67fe95a 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -212,6 +212,7 @@
> #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400
> #define UVC_QUIRK_FORCE_Y8 0x00000800
> #define UVC_QUIRK_FORCE_BPP 0x00001000
> +#define UVC_QUIRK_IGNORE_EMPTY_TS 0x00002000
>
> /* Format flags */
> #define UVC_FMT_FLAG_COMPRESSED 0x00000001
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 3/8] media: uvc: Create UVC_QUIRK_IGNORE_EMPTY_TS quirk
2022-12-30 13:45 ` Laurent Pinchart
@ 2023-01-03 16:00 ` Ricardo Ribalda
2023-01-07 1:16 ` Laurent Pinchart
1 sibling, 0 replies; 25+ messages in thread
From: Ricardo Ribalda @ 2023-01-03 16:00 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel
Hi Laurent
This is from the one that I have access to. Maybe hn.chen (on copy)
can give you a bigger list.
Thanks!
Bus 001 Device 003: ID 1bcf:2a01 Sunplus Innovation Technology Inc.
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.01
bDeviceClass 239 Miscellaneous Device
bDeviceSubClass 2 ?
bDeviceProtocol 1 Interface Association
bMaxPacketSize0 64
idVendor 0x1bcf Sunplus Innovation Technology Inc.
idProduct 0x2a01
bcdDevice 0.02
iManufacturer 1 SunplusIT Inc
iProduct 2 HanChen Wise Camera
iSerial 3 01.00.00
bNumConfigurations 1
Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 935
bNumInterfaces 4
bConfigurationValue 1
iConfiguration 0
bmAttributes 0x80
(Bus Powered)
MaxPower 500mA
Interface Association:
bLength 8
bDescriptorType 11
bFirstInterface 0
bInterfaceCount 2
bFunctionClass 14 Video
bFunctionSubClass 3 Video Interface Collection
bFunctionProtocol 0
iFunction 4 HanChen Wise Camera
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 1 Video Control
bInterfaceProtocol 1
iInterface 4 HanChen Wise Camera
VideoControl Interface Descriptor:
bLength 13
bDescriptorType 36
bDescriptorSubtype 1 (HEADER)
bcdUVC 1.50
wTotalLength 111
dwClockFrequency 48.000000MHz
bInCollection 1
baInterfaceNr( 0) 1
VideoControl Interface Descriptor:
bLength 18
bDescriptorType 36
bDescriptorSubtype 2 (INPUT_TERMINAL)
bTerminalID 1
wTerminalType 0x0201 Camera Sensor
bAssocTerminal 0
iTerminal 0
wObjectiveFocalLengthMin 0
wObjectiveFocalLengthMax 0
wOcularFocalLength 0
bControlSize 3
bmControls 0x0030000e
Auto-Exposure Mode
Auto-Exposure Priority
Exposure Time (Absolute)
VideoControl Interface Descriptor:
bLength 13
bDescriptorType 36
bDescriptorSubtype 5 (PROCESSING_UNIT)
bUnitID 2
bSourceID 1
wMaxMultiplier 16384
bControlSize 3
bmControls 0x0000157f
Brightness
Contrast
Hue
Saturation
Sharpness
Gamma
White Balance Temperature
Backlight Compensation
Power Line Frequency
White Balance Temperature, Auto
iProcessing 0
bmVideoStandards 0x 0
VideoControl Interface Descriptor:
bLength 29
bDescriptorType 36
bDescriptorSubtype 6 (EXTENSION_UNIT)
bUnitID 3
guidExtensionCode {0fb885c3-68c2-4547-90f7-8f47579d95fc}
bNumControl 5
bNrPins 1
baSourceID( 0) 2
bControlSize 4
bmControls( 0) 0x1f
bmControls( 1) 0x00
bmControls( 2) 0x00
bmControls( 3) 0x00
iExtension 0
VideoControl Interface Descriptor:
bLength 29
bDescriptorType 36
bDescriptorSubtype 6 (EXTENSION_UNIT)
bUnitID 4
guidExtensionCode {63610682-5070-49ab-b8cc-b3855e8d221d}
bNumControl 20
bNrPins 1
baSourceID( 0) 3
bControlSize 4
bmControls( 0) 0xff
bmControls( 1) 0xff
bmControls( 2) 0x71
bmControls( 3) 0x00
iExtension 0
VideoControl Interface Descriptor:
bLength 9
bDescriptorType 36
bDescriptorSubtype 3 (OUTPUT_TERMINAL)
bTerminalID 5
wTerminalType 0x0101 USB Streaming
bAssocTerminal 0
bSourceID 4
iTerminal 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x87 EP 7 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0010 1x 16 bytes
bInterval 8
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 0
bNumEndpoints 0
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 1
iInterface 0
VideoStreaming Interface Descriptor:
bLength 15
bDescriptorType 36
bDescriptorSubtype 1 (INPUT_HEADER)
bNumFormats 2
wTotalLength 335
bEndPointAddress 129
bmInfo 0
bTerminalLink 5
bStillCaptureMethod 1
bTriggerSupport 0
bTriggerUsage 0
bControlSize 1
bmaControls( 0) 11
bmaControls( 1) 11
VideoStreaming Interface Descriptor:
bLength 11
bDescriptorType 36
bDescriptorSubtype 6 (FORMAT_MJPEG)
bFormatIndex 1
bNumFrameDescriptors 7
bFlags 1
Fixed-size samples: Yes
bDefaultFrameIndex 1
bAspectRatioX 0
bAspectRatioY 0
bmInterlaceFlags 0x00
Interlaced stream or variable: No
Fields per frame: 1 fields
Field 1 first: No
Field pattern: Field 1 only
bCopyProtect 0
VideoStreaming Interface Descriptor:
bLength 30
bDescriptorType 36
bDescriptorSubtype 7 (FRAME_MJPEG)
bFrameIndex 1
bmCapabilities 0x01
Still image supported
wWidth 1920
wHeight 1080
dwMinBitRate 995328000
dwMaxBitRate 995328000
dwMaxVideoFrameBufferSize 4147200
dwDefaultFrameInterval 333333
bFrameIntervalType 1
dwFrameInterval( 0) 333333
VideoStreaming Interface Descriptor:
bLength 30
bDescriptorType 36
bDescriptorSubtype 7 (FRAME_MJPEG)
bFrameIndex 2
bmCapabilities 0x01
Still image supported
wWidth 1280
wHeight 720
dwMinBitRate 442368000
dwMaxBitRate 442368000
dwMaxVideoFrameBufferSize 1843200
dwDefaultFrameInterval 333333
bFrameIntervalType 1
dwFrameInterval( 0) 333333
VideoStreaming Interface Descriptor:
bLength 30
bDescriptorType 36
bDescriptorSubtype 7 (FRAME_MJPEG)
bFrameIndex 3
bmCapabilities 0x01
Still image supported
wWidth 640
wHeight 480
dwMinBitRate 147456000
dwMaxBitRate 147456000
dwMaxVideoFrameBufferSize 614400
dwDefaultFrameInterval 333333
bFrameIntervalType 1
dwFrameInterval( 0) 333333
VideoStreaming Interface Descriptor:
bLength 30
bDescriptorType 36
bDescriptorSubtype 7 (FRAME_MJPEG)
bFrameIndex 4
bmCapabilities 0x01
Still image supported
wWidth 640
wHeight 360
dwMinBitRate 110592000
dwMaxBitRate 110592000
dwMaxVideoFrameBufferSize 460800
dwDefaultFrameInterval 333333
bFrameIntervalType 1
dwFrameInterval( 0) 333333
VideoStreaming Interface Descriptor:
bLength 30
bDescriptorType 36
bDescriptorSubtype 7 (FRAME_MJPEG)
bFrameIndex 5
bmCapabilities 0x01
Still image supported
wWidth 352
wHeight 288
dwMinBitRate 48660480
dwMaxBitRate 48660480
dwMaxVideoFrameBufferSize 202752
dwDefaultFrameInterval 333333
bFrameIntervalType 1
dwFrameInterval( 0) 333333
VideoStreaming Interface Descriptor:
bLength 30
bDescriptorType 36
bDescriptorSubtype 7 (FRAME_MJPEG)
bFrameIndex 6
bmCapabilities 0x01
Still image supported
wWidth 320
wHeight 240
dwMinBitRate 36864000
dwMaxBitRate 36864000
dwMaxVideoFrameBufferSize 153600
dwDefaultFrameInterval 333333
bFrameIntervalType 1
dwFrameInterval( 0) 333333
VideoStreaming Interface Descriptor:
bLength 30
bDescriptorType 36
bDescriptorSubtype 7 (FRAME_MJPEG)
bFrameIndex 7
bmCapabilities 0x01
Still image supported
wWidth 176
wHeight 144
dwMinBitRate 12165120
dwMaxBitRate 12165120
dwMaxVideoFrameBufferSize 50688
dwDefaultFrameInterval 333333
bFrameIntervalType 1
dwFrameInterval( 0) 333333
VideoStreaming Interface Descriptor:
bLength 6
bDescriptorType 36
bDescriptorSubtype 13 (COLORFORMAT)
bColorPrimaries 1 (BT.709,sRGB)
bTransferCharacteristics 1 (BT.709)
bMatrixCoefficients 4 (SMPTE 170M (BT.601))
VideoStreaming Interface Descriptor:
bLength 27
bDescriptorType 36
bDescriptorSubtype 4 (FORMAT_UNCOMPRESSED)
bFormatIndex 2
bNumFrameDescriptors 2
guidFormat
{32595559-0000-0010-8000-00aa00389b71}
bBitsPerPixel 16
bDefaultFrameIndex 1
bAspectRatioX 0
bAspectRatioY 0
bmInterlaceFlags 0x00
Interlaced stream or variable: No
Fields per frame: 2 fields
Field 1 first: No
Field pattern: Field 1 only
bCopyProtect 0
VideoStreaming Interface Descriptor:
bLength 30
bDescriptorType 36
bDescriptorSubtype 5 (FRAME_UNCOMPRESSED)
bFrameIndex 1
bmCapabilities 0x01
Still image supported
wWidth 640
wHeight 480
dwMinBitRate 147456000
dwMaxBitRate 147456000
dwMaxVideoFrameBufferSize 614400
dwDefaultFrameInterval 333333
bFrameIntervalType 1
dwFrameInterval( 0) 333333
VideoStreaming Interface Descriptor:
bLength 30
bDescriptorType 36
bDescriptorSubtype 5 (FRAME_UNCOMPRESSED)
bFrameIndex 2
bmCapabilities 0x01
Still image supported
wWidth 640
wHeight 360
dwMinBitRate 110592000
dwMaxBitRate 110592000
dwMaxVideoFrameBufferSize 460800
dwDefaultFrameInterval 333333
bFrameIntervalType 1
dwFrameInterval( 0) 333333
VideoStreaming Interface Descriptor:
bLength 6
bDescriptorType 36
bDescriptorSubtype 13 (COLORFORMAT)
bColorPrimaries 1 (BT.709,sRGB)
bTransferCharacteristics 1 (BT.709)
bMatrixCoefficients 4 (SMPTE 170M (BT.601))
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 1
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 1
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x00c0 1x 192 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 2
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 1
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x0180 1x 384 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 3
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 1
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 4
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 1
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x0280 1x 640 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 5
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 1
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x0320 1x 800 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 6
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 1
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x03b0 1x 944 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 7
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 1
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x0a80 2x 640 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 8
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 1
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x0b20 2x 800 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 9
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 1
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x0be0 2x 992 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 10
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 1
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x13c0 3x 960 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 11
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 1
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x13fc 3x 1020 bytes
bInterval 1
Interface Association:
bLength 8
bDescriptorType 11
bFirstInterface 2
bInterfaceCount 2
bFunctionClass 14 Video
bFunctionSubClass 3 Video Interface Collection
bFunctionProtocol 0
iFunction 11 HanChen IR Camera
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 2
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 1 Video Control
bInterfaceProtocol 1
iInterface 11 HanChen IR Camera
VideoControl Interface Descriptor:
bLength 13
bDescriptorType 36
bDescriptorSubtype 1 (HEADER)
bcdUVC 1.50
wTotalLength 111
dwClockFrequency 48.000000MHz
bInCollection 1
baInterfaceNr( 0) 3
VideoControl Interface Descriptor:
bLength 18
bDescriptorType 36
bDescriptorSubtype 2 (INPUT_TERMINAL)
bTerminalID 1
wTerminalType 0x0201 Camera Sensor
bAssocTerminal 0
iTerminal 0
wObjectiveFocalLengthMin 0
wObjectiveFocalLengthMax 0
wOcularFocalLength 0
bControlSize 3
bmControls 0x00300000
VideoControl Interface Descriptor:
bLength 13
bDescriptorType 36
bDescriptorSubtype 5 (PROCESSING_UNIT)
bUnitID 2
bSourceID 1
wMaxMultiplier 16384
bControlSize 3
bmControls 0x00000000
iProcessing 0
bmVideoStandards 0x 0
VideoControl Interface Descriptor:
bLength 29
bDescriptorType 36
bDescriptorSubtype 6 (EXTENSION_UNIT)
bUnitID 7
guidExtensionCode {0f3f95dc-2632-4c4e-92c9-a04782f43bc8}
bNumControl 4
bNrPins 1
baSourceID( 0) 2
bControlSize 4
bmControls( 0) 0xe0
bmControls( 1) 0x01
bmControls( 2) 0x00
bmControls( 3) 0x00
iExtension 0
VideoControl Interface Descriptor:
bLength 29
bDescriptorType 36
bDescriptorSubtype 6 (EXTENSION_UNIT)
bUnitID 4
guidExtensionCode {63610682-5070-49ab-b8cc-b3855e8d221d}
bNumControl 27
bNrPins 1
baSourceID( 0) 7
bControlSize 4
bmControls( 0) 0xff
bmControls( 1) 0xff
bmControls( 2) 0x77
bmControls( 3) 0x1f
iExtension 0
VideoControl Interface Descriptor:
bLength 9
bDescriptorType 36
bDescriptorSubtype 3 (OUTPUT_TERMINAL)
bTerminalID 5
wTerminalType 0x0101 USB Streaming
bAssocTerminal 0
bSourceID 4
iTerminal 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0010 1x 16 bytes
bInterval 8
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 3
bAlternateSetting 0
bNumEndpoints 0
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 1
iInterface 0
VideoStreaming Interface Descriptor:
bLength 14
bDescriptorType 36
bDescriptorSubtype 1 (INPUT_HEADER)
bNumFormats 1
wTotalLength 101
bEndPointAddress 130
bmInfo 0
bTerminalLink 5
bStillCaptureMethod 1
bTriggerSupport 0
bTriggerUsage 0
bControlSize 1
bmaControls( 0) 27
VideoStreaming Interface Descriptor:
bLength 27
bDescriptorType 36
bDescriptorSubtype 4 (FORMAT_UNCOMPRESSED)
bFormatIndex 1
bNumFrameDescriptors 1
guidFormat
{00000032-0002-0010-8000-00aa00389b71}
bBitsPerPixel 8
bDefaultFrameIndex 1
bAspectRatioX 0
bAspectRatioY 0
bmInterlaceFlags 0x00
Interlaced stream or variable: No
Fields per frame: 2 fields
Field 1 first: No
Field pattern: Field 1 only
bCopyProtect 0
VideoStreaming Interface Descriptor:
bLength 54
bDescriptorType 36
bDescriptorSubtype 5 (FRAME_UNCOMPRESSED)
bFrameIndex 1
bmCapabilities 0x01
Still image supported
wWidth 624
wHeight 352
dwMinBitRate 52715520
dwMaxBitRate 52715520
dwMaxVideoFrameBufferSize 439296
dwDefaultFrameInterval 333333
bFrameIntervalType 7
dwFrameInterval( 0) 333333
dwFrameInterval( 1) 400000
dwFrameInterval( 2) 500000
dwFrameInterval( 3) 666666
dwFrameInterval( 4) 1000000
dwFrameInterval( 5) 1333333
dwFrameInterval( 6) 2000000
VideoStreaming Interface Descriptor:
bLength 6
bDescriptorType 36
bDescriptorSubtype 13 (COLORFORMAT)
bColorPrimaries 1 (BT.709,sRGB)
bTransferCharacteristics 1 (BT.709)
bMatrixCoefficients 4 (SMPTE 170M (BT.601))
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 3
bAlternateSetting 1
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 1
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x0a80 2x 640 bytes
bInterval 1
Binary Object Store Descriptor:
bLength 5
bDescriptorType 15
wTotalLength 33
bNumDeviceCaps 1
Platform Device Capability:
bLength 28
bDescriptorType 16
bDevCapabilityType 5
bReserved 0
PlatformCapabilityUUID {d8dd60df-4589-4cc7-9cd2-659d9e648a9f}
CapabilityData[0] 0x00
CapabilityData[1] 0x00
CapabilityData[2] 0x00
CapabilityData[3] 0x0a
CapabilityData[4] 0x6a
CapabilityData[5] 0x03
CapabilityData[6] 0x01
CapabilityData[7] 0x00
Device Status: 0x0000
(Bus Powered)
On Fri, 30 Dec 2022 at 14:45, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Fri, Dec 02, 2022 at 06:02:43PM +0100, Ricardo Ribalda wrote:
> > Some Sunplus cameras took a borderline interpretation of the UVC 1.5
> > standard, and fill the PTS and SCR fields with invalid data if the
> > package does not contain data.
> >
> > "STC must be captured when the first video data of a video frame is put
> > on the USB bus."
> >
> > Eg:
> >
> > buffer: 0xa7755c00 len 000012 header:0x8c stc 00000000 sof 0000 pts 00000000
> > buffer: 0xa7755c00 len 000012 header:0x8c stc 00000000 sof 0000 pts 00000000
> > buffer: 0xa7755c00 len 000668 header:0x8c stc 73779dba sof 070c pts 7376d37a
> >
> > This borderline/buggy interpretation has been implemented in a variety
> > of devices, from directly Sunplus and from other OEMs that rebrand
> > Sunplus products.
> >
> > Luckily we can identify the affected modules by looking at the guid of
> > one of the extension units:
> >
> > VideoControl Interface Descriptor:
> > guidExtensionCode {82066163-7050-ab49-b8cc-b3855e8d221d}
> >
> > This patch adds a new quirk to take care of this.
> >
> > Complete lsusb of one of the affected cameras:
>
> That's not complete (but that's fine from a commit message point of
> view, the full descriptors would be too long). Can you share the full
> descriptors for all the devices you know are affected by this ?
>
> > Bus 001 Device 003: ID 1bcf:2a01 Sunplus Innovation Technology Inc.
> > Device Descriptor:
> > bLength 18
> > bDescriptorType 1
> > bcdUSB 2.01
> > bDeviceClass 239 Miscellaneous Device
> > bDeviceSubClass 2 ?
> > bDeviceProtocol 1 Interface Association
> > bMaxPacketSize0 64
> > idVendor 0x1bcf Sunplus Innovation Technology Inc.
> > idProduct 0x2a01
> > bcdDevice 0.02
> > iManufacturer 1 SunplusIT Inc
> > iProduct 2 HanChen Wise Camera
> > iSerial 3 01.00.00
> > bNumConfigurations 1
> >
> > Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
> > drivers/media/usb/uvc/uvc_video.c | 8 ++++++++
> > drivers/media/usb/uvc/uvcvideo.h | 1 +
> > 3 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index c63ecfd4617d..80ef0f0e04b0 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1497,6 +1497,17 @@ static const struct uvc_entity_quirk {
> > u8 guid[16];
> > u32 quirks;
> > } uvc_entity_quirk[] = {
> > + /*
> > + * Some SunPlus uvc 1.5 device firmware expects that packages with
>
> s/uvc/UVC/
>
> > + * no frame data are ignored by the host.
>
> If it's two paragraphs, you need a blank line in-between. If it's a
> single paragraph, you must not add a line break.
>
> > + * Therefore it does not clear the PTS/SCR bits in the header, and
> > + * breaks the timestamp decode algorithm.
> > + */
> > + {
> > + .guid = {0x82, 0x06, 0x61, 0x63, 0x70, 0x50, 0xab, 0x49,
> > + 0xb8, 0xcc, 0xb3, 0x85, 0x5e, 0x8d, 0x22, 0x1d},
> > + .quirks = UVC_QUIRK_IGNORE_EMPTY_TS,
> > + },
> > };
> >
> > static void uvc_entity_quirks(struct uvc_device *dev)
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index ab56e65ca324..a5b184e71ad7 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -500,6 +500,14 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> > if (len < header_size)
> > return;
> >
> > + /*
> > + * Ignore the hardware timestamp on frames with no data on
> > + * miss-behaving devices.
> > + */
>
> Could you please expand this comment to explain the issue ? Having to
> use git blame and read the commit message to understand why the check is
> here isn't very convenient.
>
> > + if (stream->dev->quirks & UVC_QUIRK_IGNORE_EMPTY_TS &&
> > + len == header_size)
> > + return;
> > +
> > /*
> > * Extract the timestamps:
> > *
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 24c911aeebce..f395b67fe95a 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -212,6 +212,7 @@
> > #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400
> > #define UVC_QUIRK_FORCE_Y8 0x00000800
> > #define UVC_QUIRK_FORCE_BPP 0x00001000
> > +#define UVC_QUIRK_IGNORE_EMPTY_TS 0x00002000
> >
> > /* Format flags */
> > #define UVC_FMT_FLAG_COMPRESSED 0x00000001
> >
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 3/8] media: uvc: Create UVC_QUIRK_IGNORE_EMPTY_TS quirk
2022-12-30 13:45 ` Laurent Pinchart
2023-01-03 16:00 ` Ricardo Ribalda
@ 2023-01-07 1:16 ` Laurent Pinchart
1 sibling, 0 replies; 25+ messages in thread
From: Laurent Pinchart @ 2023-01-07 1:16 UTC (permalink / raw)
To: Ricardo Ribalda; +Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel
Hi Ricardo,
Another comment.
On Fri, Dec 30, 2022 at 03:45:12PM +0200, Laurent Pinchart wrote:
> On Fri, Dec 02, 2022 at 06:02:43PM +0100, Ricardo Ribalda wrote:
> > Some Sunplus cameras took a borderline interpretation of the UVC 1.5
> > standard, and fill the PTS and SCR fields with invalid data if the
> > package does not contain data.
> >
> > "STC must be captured when the first video data of a video frame is put
> > on the USB bus."
> >
> > Eg:
> >
> > buffer: 0xa7755c00 len 000012 header:0x8c stc 00000000 sof 0000 pts 00000000
> > buffer: 0xa7755c00 len 000012 header:0x8c stc 00000000 sof 0000 pts 00000000
> > buffer: 0xa7755c00 len 000668 header:0x8c stc 73779dba sof 070c pts 7376d37a
> >
> > This borderline/buggy interpretation has been implemented in a variety
> > of devices, from directly Sunplus and from other OEMs that rebrand
> > Sunplus products.
> >
> > Luckily we can identify the affected modules by looking at the guid of
> > one of the extension units:
> >
> > VideoControl Interface Descriptor:
> > guidExtensionCode {82066163-7050-ab49-b8cc-b3855e8d221d}
> >
> > This patch adds a new quirk to take care of this.
> >
> > Complete lsusb of one of the affected cameras:
>
> That's not complete (but that's fine from a commit message point of
> view, the full descriptors would be too long). Can you share the full
> descriptors for all the devices you know are affected by this ?
>
> > Bus 001 Device 003: ID 1bcf:2a01 Sunplus Innovation Technology Inc.
> > Device Descriptor:
> > bLength 18
> > bDescriptorType 1
> > bcdUSB 2.01
> > bDeviceClass 239 Miscellaneous Device
> > bDeviceSubClass 2 ?
> > bDeviceProtocol 1 Interface Association
> > bMaxPacketSize0 64
> > idVendor 0x1bcf Sunplus Innovation Technology Inc.
> > idProduct 0x2a01
> > bcdDevice 0.02
> > iManufacturer 1 SunplusIT Inc
> > iProduct 2 HanChen Wise Camera
> > iSerial 3 01.00.00
> > bNumConfigurations 1
> >
> > Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
> > drivers/media/usb/uvc/uvc_video.c | 8 ++++++++
> > drivers/media/usb/uvc/uvcvideo.h | 1 +
> > 3 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index c63ecfd4617d..80ef0f0e04b0 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1497,6 +1497,17 @@ static const struct uvc_entity_quirk {
> > u8 guid[16];
> > u32 quirks;
> > } uvc_entity_quirk[] = {
> > + /*
> > + * Some SunPlus uvc 1.5 device firmware expects that packages with
>
> s/uvc/UVC/
>
> > + * no frame data are ignored by the host.
>
> If it's two paragraphs, you need a blank line in-between. If it's a
> single paragraph, you must not add a line break.
>
> > + * Therefore it does not clear the PTS/SCR bits in the header, and
> > + * breaks the timestamp decode algorithm.
> > + */
> > + {
> > + .guid = {0x82, 0x06, 0x61, 0x63, 0x70, 0x50, 0xab, 0x49,
> > + 0xb8, 0xcc, 0xb3, 0x85, 0x5e, 0x8d, 0x22, 0x1d},
> > + .quirks = UVC_QUIRK_IGNORE_EMPTY_TS,
> > + },
> > };
> >
> > static void uvc_entity_quirks(struct uvc_device *dev)
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index ab56e65ca324..a5b184e71ad7 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -500,6 +500,14 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> > if (len < header_size)
> > return;
> >
> > + /*
> > + * Ignore the hardware timestamp on frames with no data on
> > + * miss-behaving devices.
> > + */
>
> Could you please expand this comment to explain the issue ? Having to
> use git blame and read the commit message to understand why the check is
> here isn't very convenient.
>
> > + if (stream->dev->quirks & UVC_QUIRK_IGNORE_EMPTY_TS &&
> > + len == header_size)
> > + return;
Given that there may be no guarantee that the above GUID won't be used
by devices that don't exhibit this problem, I'm wondering if we couldn't
use a heuristics instead of a quirk. I'm thinking about something along
the lines of
if (buf->bytesused == 0 && len == header_size && has_scr &&
stc == 0 && sof == 0)
This will catch suspicious SCR values (stc == 0 && sof == 0) on empty
packets (len == header_size) sent before any data packet (buf->bytesused
== 0), which should handle the devices you have to support, and avoid
false positives (the stc == 0 && sof == 0 check is already quite
restrictive, adding buf->bytesused == 0 would limit the workaround to
packets before the first data packet).
With this we could drop patch 2/8.
> > +
> > /*
> > * Extract the timestamps:
> > *
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 24c911aeebce..f395b67fe95a 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -212,6 +212,7 @@
> > #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400
> > #define UVC_QUIRK_FORCE_Y8 0x00000800
> > #define UVC_QUIRK_FORCE_BPP 0x00001000
> > +#define UVC_QUIRK_IGNORE_EMPTY_TS 0x00002000
> >
> > /* Format flags */
> > #define UVC_FMT_FLAG_COMPRESSED 0x00000001
> >
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH RESEND v2 4/8] media: uvcvideo: Quirk for invalid dev_sof in Logi C922
2022-12-02 17:02 [PATCH RESEND v2 0/8] uvcvideo: Fixes for hw timestamping Ricardo Ribalda
` (2 preceding siblings ...)
2022-12-02 17:02 ` [PATCH RESEND v2 3/8] media: uvc: Create UVC_QUIRK_IGNORE_EMPTY_TS quirk Ricardo Ribalda
@ 2022-12-02 17:02 ` Ricardo Ribalda
2022-12-30 14:31 ` Laurent Pinchart
2022-12-02 17:02 ` [PATCH RESEND v2 5/8] media: uvcvideo: Quirk for autosuspend in Logi C910 Ricardo Ribalda
` (3 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Ricardo Ribalda @ 2022-12-02 17:02 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Laurent Pinchart
Cc: hn.chen, linux-media, Ricardo Ribalda, linux-kernel
Logitech C922 internal sof does not increases at a stable rate of 1KHz.
This causes that the device_sof and the host_sof run at different rates,
breaking the clock domain conversion algorithm. Eg:
30 (6) [-] none 30 614400 B 21.245557 21.395214 34.133 fps ts mono/SoE
31 (7) [-] none 31 614400 B 21.275327 21.427246 33.591 fps ts mono/SoE
32 (0) [-] none 32 614400 B 21.304739 21.459256 34.000 fps ts mono/SoE
33 (1) [-] none 33 614400 B 21.334324 21.495274 33.801 fps ts mono/SoE
* 34 (2) [-] none 34 614400 B 21.529237 21.527297 5.130 fps ts mono/SoE
* 35 (3) [-] none 35 614400 B 21.649416 21.559306 8.321 fps ts mono/SoE
36 (4) [-] none 36 614400 B 21.678789 21.595320 34.045 fps ts mono/SoE
...
99 (3) [-] none 99 614400 B 23.542226 23.696352 33.541 fps ts mono/SoE
100 (4) [-] none 100 614400 B 23.571578 23.728404 34.069 fps ts mono/SoE
101 (5) [-] none 101 614400 B 23.601425 23.760420 33.504 fps ts mono/SoE
* 102 (6) [-] none 102 614400 B 23.798324 23.796428 5.079 fps ts mono/SoE
* 103 (7) [-] none 103 614400 B 23.916271 23.828450 8.478 fps ts mono/SoE
104 (0) [-] none 104 614400 B 23.945720 23.860479 33.957 fps ts mono/SoE
Instead of disabling completely the hardware timestamping for such
hardware we take the assumption that the package handling jitter is
under 2ms and use the host_sof as dev_sof.
For the hardware tested, it provides a much better timestamping than a
pure software solution:
https://ibb.co/D1HJJ4x
https://ibb.co/8s9dBdk
https://ibb.co/QC9MgVK
This bug in the camera firmware has been confirmed by the vendor.
lsusb -v
Bus 001 Device 044: ID 046d:085c Logitech, Inc. C922 Pro Stream Webcam
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 239 Miscellaneous Device
bDeviceSubClass 2
bDeviceProtocol 1 Interface Association
bMaxPacketSize0 64
idVendor 0x046d Logitech, Inc.
idProduct 0x085c C922 Pro Stream Webcam
bcdDevice 0.16
iManufacturer 0
iProduct 2 C922 Pro Stream Webcam
iSerial 1 80B912DF
bNumConfigurations 1
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_driver.c | 9 +++++++++
drivers/media/usb/uvc/uvc_video.c | 11 +++++++++--
drivers/media/usb/uvc/uvcvideo.h | 1 +
3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 80ef0f0e04b0..4512316c8748 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2880,6 +2880,15 @@ static const struct usb_device_id uvc_ids[] = {
.bInterfaceSubClass = 1,
.bInterfaceProtocol = 0,
.driver_info = UVC_INFO_QUIRK(UVC_QUIRK_RESTORE_CTRLS_ON_INIT) },
+ /* Logitech HD Pro Webcam C922 */
+ { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
+ | USB_DEVICE_ID_MATCH_INT_INFO,
+ .idVendor = 0x046d,
+ .idProduct = 0x085c,
+ .bInterfaceClass = USB_CLASS_VIDEO,
+ .bInterfaceSubClass = 1,
+ .bInterfaceProtocol = 0,
+ .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_INVALID_DEVICE_SOF) },
/* Chicony CNF7129 (Asus EEE 100HE) */
{ .match_flags = USB_DEVICE_ID_MATCH_DEVICE
| USB_DEVICE_ID_MATCH_INT_INFO,
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index a5b184e71ad7..d387d6335344 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -529,13 +529,20 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
* data packages of the same frame contains the same sof. In that case
* only the first one will match the host_sof.
*/
- dev_sof = get_unaligned_le16(&data[header_size - 2]);
+ if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
+ dev_sof = usb_get_current_frame_number(stream->dev->udev);
+ else
+ dev_sof = get_unaligned_le16(&data[header_size - 2]);
+
if (dev_sof == stream->clock.last_sof)
return;
stream->clock.last_sof = dev_sof;
- host_sof = usb_get_current_frame_number(stream->dev->udev);
+ if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
+ host_sof = dev_sof;
+ else
+ host_sof = usb_get_current_frame_number(stream->dev->udev);
time = uvc_video_get_time();
/*
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index f395b67fe95a..e41289605d0e 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -213,6 +213,7 @@
#define UVC_QUIRK_FORCE_Y8 0x00000800
#define UVC_QUIRK_FORCE_BPP 0x00001000
#define UVC_QUIRK_IGNORE_EMPTY_TS 0x00002000
+#define UVC_QUIRK_INVALID_DEVICE_SOF 0x00004000
/* Format flags */
#define UVC_FMT_FLAG_COMPRESSED 0x00000001
--
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 4/8] media: uvcvideo: Quirk for invalid dev_sof in Logi C922
2022-12-02 17:02 ` [PATCH RESEND v2 4/8] media: uvcvideo: Quirk for invalid dev_sof in Logi C922 Ricardo Ribalda
@ 2022-12-30 14:31 ` Laurent Pinchart
0 siblings, 0 replies; 25+ messages in thread
From: Laurent Pinchart @ 2022-12-30 14:31 UTC (permalink / raw)
To: Ricardo Ribalda; +Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel
Hi Ricardo,
Thank you for the patch.
s/Logi/Logitech/ in the subject line (same in the previous patch)
On Fri, Dec 02, 2022 at 06:02:44PM +0100, Ricardo Ribalda wrote:
> Logitech C922 internal sof does not increases at a stable rate of 1KHz.
s/sof/SOF/
s/KHz/kHz/
>
No need for a blank line.
> This causes that the device_sof and the host_sof run at different rates,
s/that //
s/run/ to run/
> breaking the clock domain conversion algorithm. Eg:
>
> 30 (6) [-] none 30 614400 B 21.245557 21.395214 34.133 fps ts mono/SoE
> 31 (7) [-] none 31 614400 B 21.275327 21.427246 33.591 fps ts mono/SoE
> 32 (0) [-] none 32 614400 B 21.304739 21.459256 34.000 fps ts mono/SoE
> 33 (1) [-] none 33 614400 B 21.334324 21.495274 33.801 fps ts mono/SoE
> * 34 (2) [-] none 34 614400 B 21.529237 21.527297 5.130 fps ts mono/SoE
> * 35 (3) [-] none 35 614400 B 21.649416 21.559306 8.321 fps ts mono/SoE
> 36 (4) [-] none 36 614400 B 21.678789 21.595320 34.045 fps ts mono/SoE
> ...
> 99 (3) [-] none 99 614400 B 23.542226 23.696352 33.541 fps ts mono/SoE
> 100 (4) [-] none 100 614400 B 23.571578 23.728404 34.069 fps ts mono/SoE
> 101 (5) [-] none 101 614400 B 23.601425 23.760420 33.504 fps ts mono/SoE
> * 102 (6) [-] none 102 614400 B 23.798324 23.796428 5.079 fps ts mono/SoE
> * 103 (7) [-] none 103 614400 B 23.916271 23.828450 8.478 fps ts mono/SoE
> 104 (0) [-] none 104 614400 B 23.945720 23.860479 33.957 fps ts mono/SoE
>
> Instead of disabling completely the hardware timestamping for such
> hardware we take the assumption that the package handling jitter is
s/package/packet/
> under 2ms and use the host_sof as dev_sof.
You know, I think we should really move all this timestamp handling code
to userspace...
> For the hardware tested, it provides a much better timestamping than a
> pure software solution:
> https://ibb.co/D1HJJ4x
> https://ibb.co/8s9dBdk
> https://ibb.co/QC9MgVK
A bit of context would be nice, such as labelling the axes, mentioning
what nominal frame rate is expected, as telling what each link
corresponds to.
> This bug in the camera firmware has been confirmed by the vendor.
>
> lsusb -v
>
> Bus 001 Device 044: ID 046d:085c Logitech, Inc. C922 Pro Stream Webcam
> Device Descriptor:
> bLength 18
> bDescriptorType 1
> bcdUSB 2.00
> bDeviceClass 239 Miscellaneous Device
> bDeviceSubClass 2
> bDeviceProtocol 1 Interface Association
> bMaxPacketSize0 64
> idVendor 0x046d Logitech, Inc.
> idProduct 0x085c C922 Pro Stream Webcam
> bcdDevice 0.16
> iManufacturer 0
> iProduct 2 C922 Pro Stream Webcam
> iSerial 1 80B912DF
> bNumConfigurations 1
>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/usb/uvc/uvc_driver.c | 9 +++++++++
> drivers/media/usb/uvc/uvc_video.c | 11 +++++++++--
> drivers/media/usb/uvc/uvcvideo.h | 1 +
> 3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 80ef0f0e04b0..4512316c8748 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2880,6 +2880,15 @@ static const struct usb_device_id uvc_ids[] = {
> .bInterfaceSubClass = 1,
> .bInterfaceProtocol = 0,
> .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_RESTORE_CTRLS_ON_INIT) },
> + /* Logitech HD Pro Webcam C922 */
> + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> + | USB_DEVICE_ID_MATCH_INT_INFO,
> + .idVendor = 0x046d,
> + .idProduct = 0x085c,
> + .bInterfaceClass = USB_CLASS_VIDEO,
> + .bInterfaceSubClass = 1,
> + .bInterfaceProtocol = 0,
> + .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_INVALID_DEVICE_SOF) },
> /* Chicony CNF7129 (Asus EEE 100HE) */
> { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> | USB_DEVICE_ID_MATCH_INT_INFO,
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index a5b184e71ad7..d387d6335344 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -529,13 +529,20 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> * data packages of the same frame contains the same sof. In that case
> * only the first one will match the host_sof.
> */
> - dev_sof = get_unaligned_le16(&data[header_size - 2]);
> + if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> + dev_sof = usb_get_current_frame_number(stream->dev->udev);
> + else
> + dev_sof = get_unaligned_le16(&data[header_size - 2]);
> +
> if (dev_sof == stream->clock.last_sof)
> return;
>
> stream->clock.last_sof = dev_sof;
>
> - host_sof = usb_get_current_frame_number(stream->dev->udev);
> + if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> + host_sof = dev_sof;
> + else
> + host_sof = usb_get_current_frame_number(stream->dev->udev);
> time = uvc_video_get_time();
This effectively bypasses the SOF clock domain completely. If the SOF
values sent by the device are completely unrelated to the USB SOF
counter we can obviously not rely on the SOF clock domain, but I'd like
an explanation as to *why* this still provides better timestamps.
>
> /*
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index f395b67fe95a..e41289605d0e 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -213,6 +213,7 @@
> #define UVC_QUIRK_FORCE_Y8 0x00000800
> #define UVC_QUIRK_FORCE_BPP 0x00001000
> #define UVC_QUIRK_IGNORE_EMPTY_TS 0x00002000
> +#define UVC_QUIRK_INVALID_DEVICE_SOF 0x00004000
>
> /* Format flags */
> #define UVC_FMT_FLAG_COMPRESSED 0x00000001
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH RESEND v2 5/8] media: uvcvideo: Quirk for autosuspend in Logi C910
2022-12-02 17:02 [PATCH RESEND v2 0/8] uvcvideo: Fixes for hw timestamping Ricardo Ribalda
` (3 preceding siblings ...)
2022-12-02 17:02 ` [PATCH RESEND v2 4/8] media: uvcvideo: Quirk for invalid dev_sof in Logi C922 Ricardo Ribalda
@ 2022-12-02 17:02 ` Ricardo Ribalda
2022-12-30 13:52 ` Laurent Pinchart
2022-12-02 17:02 ` [PATCH RESEND v2 6/8] media: uvcvideo: Allow hw clock updates with buffers not full Ricardo Ribalda
` (2 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Ricardo Ribalda @ 2022-12-02 17:02 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Laurent Pinchart
Cc: hn.chen, linux-media, Ricardo Ribalda, linux-kernel
Logitech C910 firmware is unable to recover from a usb autosuspend. When
it resumes, the device is in a state where it only produces invalid
frames. Eg:
$ echo 0xFFFF > /sys/module/uvcvideo/parameters/trace # enable verbose log
$ yavta -c1 -n1 --file='frame#.jpg' --format MJPEG --size=1920x1080 /dev/video1
[350438.435219] uvcvideo: uvc_v4l2_open
[350438.529794] uvcvideo: Resuming interface 2
[350438.529801] uvcvideo: Resuming interface 3
[350438.529991] uvcvideo: Trying format 0x47504a4d (MJPG): 1920x1080.
[350438.529996] uvcvideo: Using default frame interval 33333.3 us (30.0 fps).
[350438.551496] uvcvideo: uvc_v4l2_mmap
[350438.555890] uvcvideo: Device requested 3060 B/frame bandwidth.
[350438.555896] uvcvideo: Selecting alternate setting 11 (3060 B/frame bandwidth).
[350438.556362] uvcvideo: Allocated 5 URB buffers of 32x3060 bytes each.
[350439.316468] uvcvideo: Marking buffer as bad (error bit set).
[350439.316475] uvcvideo: Frame complete (EOF found).
[350439.316477] uvcvideo: EOF in empty payload.
[350439.316484] uvcvideo: frame 1 stats: 149/261/417 packets, 1/149/417 pts (early initial), 416/417 scr, last pts/stc/sof 2976325734/2978107243/249
[350439.384510] uvcvideo: Marking buffer as bad (error bit set).
[350439.384516] uvcvideo: Frame complete (EOF found).
[350439.384518] uvcvideo: EOF in empty payload.
[350439.384525] uvcvideo: frame 2 stats: 265/379/533 packets, 1/265/533 pts (early initial), 532/533 scr, last pts/stc/sof 2979524454/2981305193/316
[350439.448472] uvcvideo: Marking buffer as bad (error bit set).
[350439.448478] uvcvideo: Frame complete (EOF found).
[350439.448480] uvcvideo: EOF in empty payload.
[350439.448487] uvcvideo: frame 3 stats: 265/377/533 packets, 1/265/533 pts (early initial), 532/533 scr, last pts/stc/sof 2982723174/2984503144/382
...(loop)...
The devices can leave this invalid state if its altstate is toggled.
This patch addes a quirk for this device so it can be autosuspended
properly.
lsusb -v:
Bus 001 Device 049: ID 046d:0821 Logitech, Inc. HD Webcam C910
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 239 Miscellaneous Device
bDeviceSubClass 2
bDeviceProtocol 1 Interface Association
bMaxPacketSize0 64
idVendor 0x046d Logitech, Inc.
idProduct 0x0821 HD Webcam C910
bcdDevice 0.10
iManufacturer 0
iProduct 0
iSerial 1 390022B0
bNumConfigurations 1
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_driver.c | 18 ++++++++++++++++++
drivers/media/usb/uvc/uvc_video.c | 5 +++++
drivers/media/usb/uvc/uvcvideo.h | 1 +
3 files changed, 24 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 4512316c8748..d2a158a1ce35 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2823,6 +2823,24 @@ static const struct usb_device_id uvc_ids[] = {
.bInterfaceSubClass = 1,
.bInterfaceProtocol = 0,
.driver_info = (kernel_ulong_t)&uvc_quirk_probe_minmax },
+ /* Logitech, Webcam C910 */
+ { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
+ | USB_DEVICE_ID_MATCH_INT_INFO,
+ .idVendor = 0x046d,
+ .idProduct = 0x0821,
+ .bInterfaceClass = USB_CLASS_VIDEO,
+ .bInterfaceSubClass = 1,
+ .bInterfaceProtocol = 0,
+ .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_WAKE_AUTOSUSPEND)},
+ /* Logitech, Webcam B910 */
+ { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
+ | USB_DEVICE_ID_MATCH_INT_INFO,
+ .idVendor = 0x046d,
+ .idProduct = 0x0823,
+ .bInterfaceClass = USB_CLASS_VIDEO,
+ .bInterfaceSubClass = 1,
+ .bInterfaceProtocol = 0,
+ .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_WAKE_AUTOSUSPEND)},
/* Logitech Quickcam Fusion */
{ .match_flags = USB_DEVICE_ID_MATCH_DEVICE
| USB_DEVICE_ID_MATCH_INT_INFO,
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index d387d6335344..75c32e232f5d 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1983,6 +1983,11 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
"Selecting alternate setting %u (%u B/frame bandwidth)\n",
altsetting, best_psize);
+ if (stream->dev->quirks & UVC_QUIRK_WAKE_AUTOSUSPEND) {
+ usb_set_interface(stream->dev->udev, intfnum,
+ altsetting);
+ usb_set_interface(stream->dev->udev, intfnum, 0);
+ }
ret = usb_set_interface(stream->dev->udev, intfnum, altsetting);
if (ret < 0)
return ret;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index e41289605d0e..14daa7111953 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -214,6 +214,7 @@
#define UVC_QUIRK_FORCE_BPP 0x00001000
#define UVC_QUIRK_IGNORE_EMPTY_TS 0x00002000
#define UVC_QUIRK_INVALID_DEVICE_SOF 0x00004000
+#define UVC_QUIRK_WAKE_AUTOSUSPEND 0x00008000
/* Format flags */
#define UVC_FMT_FLAG_COMPRESSED 0x00000001
--
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 5/8] media: uvcvideo: Quirk for autosuspend in Logi C910
2022-12-02 17:02 ` [PATCH RESEND v2 5/8] media: uvcvideo: Quirk for autosuspend in Logi C910 Ricardo Ribalda
@ 2022-12-30 13:52 ` Laurent Pinchart
2023-01-03 16:13 ` Ricardo Ribalda
0 siblings, 1 reply; 25+ messages in thread
From: Laurent Pinchart @ 2022-12-30 13:52 UTC (permalink / raw)
To: Ricardo Ribalda; +Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel
Hi Ricardo,
Thank you for the patch.
s/C910/B910 and C910/ in the subject line.
On Fri, Dec 02, 2022 at 06:02:45PM +0100, Ricardo Ribalda wrote:
> Logitech C910 firmware is unable to recover from a usb autosuspend. When
s/C910/B910 and C910/
s/usb/USB/
> it resumes, the device is in a state where it only produces invalid
> frames. Eg:
>
> $ echo 0xFFFF > /sys/module/uvcvideo/parameters/trace # enable verbose log
> $ yavta -c1 -n1 --file='frame#.jpg' --format MJPEG --size=1920x1080 /dev/video1
Is this true for YUYV frames too ?
> [350438.435219] uvcvideo: uvc_v4l2_open
> [350438.529794] uvcvideo: Resuming interface 2
> [350438.529801] uvcvideo: Resuming interface 3
> [350438.529991] uvcvideo: Trying format 0x47504a4d (MJPG): 1920x1080.
> [350438.529996] uvcvideo: Using default frame interval 33333.3 us (30.0 fps).
> [350438.551496] uvcvideo: uvc_v4l2_mmap
> [350438.555890] uvcvideo: Device requested 3060 B/frame bandwidth.
> [350438.555896] uvcvideo: Selecting alternate setting 11 (3060 B/frame bandwidth).
> [350438.556362] uvcvideo: Allocated 5 URB buffers of 32x3060 bytes each.
> [350439.316468] uvcvideo: Marking buffer as bad (error bit set).
> [350439.316475] uvcvideo: Frame complete (EOF found).
> [350439.316477] uvcvideo: EOF in empty payload.
> [350439.316484] uvcvideo: frame 1 stats: 149/261/417 packets, 1/149/417 pts (early initial), 416/417 scr, last pts/stc/sof 2976325734/2978107243/249
> [350439.384510] uvcvideo: Marking buffer as bad (error bit set).
> [350439.384516] uvcvideo: Frame complete (EOF found).
> [350439.384518] uvcvideo: EOF in empty payload.
> [350439.384525] uvcvideo: frame 2 stats: 265/379/533 packets, 1/265/533 pts (early initial), 532/533 scr, last pts/stc/sof 2979524454/2981305193/316
> [350439.448472] uvcvideo: Marking buffer as bad (error bit set).
> [350439.448478] uvcvideo: Frame complete (EOF found).
> [350439.448480] uvcvideo: EOF in empty payload.
> [350439.448487] uvcvideo: frame 3 stats: 265/377/533 packets, 1/265/533 pts (early initial), 532/533 scr, last pts/stc/sof 2982723174/2984503144/382
> ...(loop)...
>
> The devices can leave this invalid state if its altstate is toggled.
s/its altstate/the alternate setting of the streaming interface/
How did you figure this out ?
> This patch addes a quirk for this device so it can be autosuspended
> properly.
>
> lsusb -v:
> Bus 001 Device 049: ID 046d:0821 Logitech, Inc. HD Webcam C910
> Device Descriptor:
> bLength 18
> bDescriptorType 1
> bcdUSB 2.00
> bDeviceClass 239 Miscellaneous Device
> bDeviceSubClass 2
> bDeviceProtocol 1 Interface Association
> bMaxPacketSize0 64
> idVendor 0x046d Logitech, Inc.
> idProduct 0x0821 HD Webcam C910
> bcdDevice 0.10
> iManufacturer 0
> iProduct 0
> iSerial 1 390022B0
> bNumConfigurations 1
>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/usb/uvc/uvc_driver.c | 18 ++++++++++++++++++
> drivers/media/usb/uvc/uvc_video.c | 5 +++++
> drivers/media/usb/uvc/uvcvideo.h | 1 +
> 3 files changed, 24 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 4512316c8748..d2a158a1ce35 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2823,6 +2823,24 @@ static const struct usb_device_id uvc_ids[] = {
> .bInterfaceSubClass = 1,
> .bInterfaceProtocol = 0,
> .driver_info = (kernel_ulong_t)&uvc_quirk_probe_minmax },
> + /* Logitech, Webcam C910 */
> + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> + | USB_DEVICE_ID_MATCH_INT_INFO,
> + .idVendor = 0x046d,
> + .idProduct = 0x0821,
> + .bInterfaceClass = USB_CLASS_VIDEO,
> + .bInterfaceSubClass = 1,
> + .bInterfaceProtocol = 0,
> + .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_WAKE_AUTOSUSPEND)},
> + /* Logitech, Webcam B910 */
> + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> + | USB_DEVICE_ID_MATCH_INT_INFO,
> + .idVendor = 0x046d,
> + .idProduct = 0x0823,
> + .bInterfaceClass = USB_CLASS_VIDEO,
> + .bInterfaceSubClass = 1,
> + .bInterfaceProtocol = 0,
> + .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_WAKE_AUTOSUSPEND)},
> /* Logitech Quickcam Fusion */
> { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> | USB_DEVICE_ID_MATCH_INT_INFO,
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index d387d6335344..75c32e232f5d 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1983,6 +1983,11 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
> "Selecting alternate setting %u (%u B/frame bandwidth)\n",
> altsetting, best_psize);
>
Please add a comment to explain the issue.
> + if (stream->dev->quirks & UVC_QUIRK_WAKE_AUTOSUSPEND) {
> + usb_set_interface(stream->dev->udev, intfnum,
> + altsetting);
> + usb_set_interface(stream->dev->udev, intfnum, 0);
> + }
Missing blank line.
> ret = usb_set_interface(stream->dev->udev, intfnum, altsetting);
> if (ret < 0)
> return ret;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index e41289605d0e..14daa7111953 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -214,6 +214,7 @@
> #define UVC_QUIRK_FORCE_BPP 0x00001000
> #define UVC_QUIRK_IGNORE_EMPTY_TS 0x00002000
> #define UVC_QUIRK_INVALID_DEVICE_SOF 0x00004000
> +#define UVC_QUIRK_WAKE_AUTOSUSPEND 0x00008000
>
> /* Format flags */
> #define UVC_FMT_FLAG_COMPRESSED 0x00000001
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 5/8] media: uvcvideo: Quirk for autosuspend in Logi C910
2022-12-30 13:52 ` Laurent Pinchart
@ 2023-01-03 16:13 ` Ricardo Ribalda
0 siblings, 0 replies; 25+ messages in thread
From: Ricardo Ribalda @ 2023-01-03 16:13 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel
Hi Laurent
On Fri, 30 Dec 2022 at 14:52, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> s/C910/B910 and C910/ in the subject line.
>
> On Fri, Dec 02, 2022 at 06:02:45PM +0100, Ricardo Ribalda wrote:
> > Logitech C910 firmware is unable to recover from a usb autosuspend. When
>
> s/C910/B910 and C910/
> s/usb/USB/
>
> > it resumes, the device is in a state where it only produces invalid
> > frames. Eg:
> >
> > $ echo 0xFFFF > /sys/module/uvcvideo/parameters/trace # enable verbose log
> > $ yavta -c1 -n1 --file='frame#.jpg' --format MJPEG --size=1920x1080 /dev/video1
>
> Is this true for YUYV frames too ?
I believe so. This is just the script that I typically use to play
with my cameras.
>
> > [350438.435219] uvcvideo: uvc_v4l2_open
> > [350438.529794] uvcvideo: Resuming interface 2
> > [350438.529801] uvcvideo: Resuming interface 3
> > [350438.529991] uvcvideo: Trying format 0x47504a4d (MJPG): 1920x1080.
> > [350438.529996] uvcvideo: Using default frame interval 33333.3 us (30.0 fps).
> > [350438.551496] uvcvideo: uvc_v4l2_mmap
> > [350438.555890] uvcvideo: Device requested 3060 B/frame bandwidth.
> > [350438.555896] uvcvideo: Selecting alternate setting 11 (3060 B/frame bandwidth).
> > [350438.556362] uvcvideo: Allocated 5 URB buffers of 32x3060 bytes each.
> > [350439.316468] uvcvideo: Marking buffer as bad (error bit set).
> > [350439.316475] uvcvideo: Frame complete (EOF found).
> > [350439.316477] uvcvideo: EOF in empty payload.
> > [350439.316484] uvcvideo: frame 1 stats: 149/261/417 packets, 1/149/417 pts (early initial), 416/417 scr, last pts/stc/sof 2976325734/2978107243/249
> > [350439.384510] uvcvideo: Marking buffer as bad (error bit set).
> > [350439.384516] uvcvideo: Frame complete (EOF found).
> > [350439.384518] uvcvideo: EOF in empty payload.
> > [350439.384525] uvcvideo: frame 2 stats: 265/379/533 packets, 1/265/533 pts (early initial), 532/533 scr, last pts/stc/sof 2979524454/2981305193/316
> > [350439.448472] uvcvideo: Marking buffer as bad (error bit set).
> > [350439.448478] uvcvideo: Frame complete (EOF found).
> > [350439.448480] uvcvideo: EOF in empty payload.
> > [350439.448487] uvcvideo: frame 3 stats: 265/377/533 packets, 1/265/533 pts (early initial), 532/533 scr, last pts/stc/sof 2982723174/2984503144/382
> > ...(loop)...
> >
> > The devices can leave this invalid state if its altstate is toggled.
>
> s/its altstate/the alternate setting of the streaming interface/
>
> How did you figure this out ?
A deterministic process, where we analysed all the variables involved
in the streaming process....
(aka, educated guess + luck + trial and error :P)
I triggered a whole reconfiguration of the device and that worked, and
then I started removing parts of it until I ended up with the minimum
thing that made the device work.
>
> > This patch addes a quirk for this device so it can be autosuspended
> > properly.
> >
> > lsusb -v:
> > Bus 001 Device 049: ID 046d:0821 Logitech, Inc. HD Webcam C910
> > Device Descriptor:
> > bLength 18
> > bDescriptorType 1
> > bcdUSB 2.00
> > bDeviceClass 239 Miscellaneous Device
> > bDeviceSubClass 2
> > bDeviceProtocol 1 Interface Association
> > bMaxPacketSize0 64
> > idVendor 0x046d Logitech, Inc.
> > idProduct 0x0821 HD Webcam C910
> > bcdDevice 0.10
> > iManufacturer 0
> > iProduct 0
> > iSerial 1 390022B0
> > bNumConfigurations 1
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > drivers/media/usb/uvc/uvc_driver.c | 18 ++++++++++++++++++
> > drivers/media/usb/uvc/uvc_video.c | 5 +++++
> > drivers/media/usb/uvc/uvcvideo.h | 1 +
> > 3 files changed, 24 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 4512316c8748..d2a158a1ce35 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2823,6 +2823,24 @@ static const struct usb_device_id uvc_ids[] = {
> > .bInterfaceSubClass = 1,
> > .bInterfaceProtocol = 0,
> > .driver_info = (kernel_ulong_t)&uvc_quirk_probe_minmax },
> > + /* Logitech, Webcam C910 */
> > + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> > + | USB_DEVICE_ID_MATCH_INT_INFO,
> > + .idVendor = 0x046d,
> > + .idProduct = 0x0821,
> > + .bInterfaceClass = USB_CLASS_VIDEO,
> > + .bInterfaceSubClass = 1,
> > + .bInterfaceProtocol = 0,
> > + .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_WAKE_AUTOSUSPEND)},
> > + /* Logitech, Webcam B910 */
> > + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> > + | USB_DEVICE_ID_MATCH_INT_INFO,
> > + .idVendor = 0x046d,
> > + .idProduct = 0x0823,
> > + .bInterfaceClass = USB_CLASS_VIDEO,
> > + .bInterfaceSubClass = 1,
> > + .bInterfaceProtocol = 0,
> > + .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_WAKE_AUTOSUSPEND)},
> > /* Logitech Quickcam Fusion */
> > { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> > | USB_DEVICE_ID_MATCH_INT_INFO,
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index d387d6335344..75c32e232f5d 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -1983,6 +1983,11 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
> > "Selecting alternate setting %u (%u B/frame bandwidth)\n",
> > altsetting, best_psize);
> >
>
> Please add a comment to explain the issue.
>
> > + if (stream->dev->quirks & UVC_QUIRK_WAKE_AUTOSUSPEND) {
> > + usb_set_interface(stream->dev->udev, intfnum,
> > + altsetting);
> > + usb_set_interface(stream->dev->udev, intfnum, 0);
> > + }
>
> Missing blank line.
>
> > ret = usb_set_interface(stream->dev->udev, intfnum, altsetting);
> > if (ret < 0)
> > return ret;
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index e41289605d0e..14daa7111953 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -214,6 +214,7 @@
> > #define UVC_QUIRK_FORCE_BPP 0x00001000
> > #define UVC_QUIRK_IGNORE_EMPTY_TS 0x00002000
> > #define UVC_QUIRK_INVALID_DEVICE_SOF 0x00004000
> > +#define UVC_QUIRK_WAKE_AUTOSUSPEND 0x00008000
> >
> > /* Format flags */
> > #define UVC_FMT_FLAG_COMPRESSED 0x00000001
> >
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH RESEND v2 6/8] media: uvcvideo: Allow hw clock updates with buffers not full
2022-12-02 17:02 [PATCH RESEND v2 0/8] uvcvideo: Fixes for hw timestamping Ricardo Ribalda
` (4 preceding siblings ...)
2022-12-02 17:02 ` [PATCH RESEND v2 5/8] media: uvcvideo: Quirk for autosuspend in Logi C910 Ricardo Ribalda
@ 2022-12-02 17:02 ` Ricardo Ribalda
2022-12-30 14:37 ` Laurent Pinchart
2022-12-02 17:02 ` [PATCH RESEND v2 7/8] media: uvcvideo: Refactor clock circular buffer Ricardo Ribalda
2022-12-02 17:02 ` [PATCH RESEND v2 8/8] media: uvcvideo: Fix hw timestampt handling for slow FPS Ricardo Ribalda
7 siblings, 1 reply; 25+ messages in thread
From: Ricardo Ribalda @ 2022-12-02 17:02 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Laurent Pinchart
Cc: hn.chen, linux-media, Ricardo Ribalda, linux-kernel
With UVC 1.5 we get as little as one clock sample per frame. Which means
that it takes 32 frames to move from the software timestamp to the
hardware timestamp method.
This results in abrupt changes in the timestamping after 32 frames (~1
second), resulting in noticeable artifacts when used for encoding.
With this patch we modify the update algorithm to work with whatever
amount of values are available.
Tested-by: HungNien Chen <hn.chen@sunplusit.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_video.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 75c32e232f5d..7c6448c6d706 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -742,10 +742,10 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
spin_lock_irqsave(&clock->lock, flags);
- if (clock->count < clock->size)
+ if (clock->count < 2)
goto done;
- first = &clock->samples[clock->head];
+ first = &clock->samples[(clock->head - clock->count) % clock->size];
last = &clock->samples[(clock->head - 1) % clock->size];
/* First step, PTS to SOF conversion. */
@@ -760,6 +760,14 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
if (y2 < y1)
y2 += 2048 << 16;
+ /*
+ * Have at least 1/4 of a second of timestamps before we
+ * try to do any calculation. Otherwise we do not have enough
+ * precission.
+ */
+ if ((y2 - y1) < (256 << 16))
+ goto done;
+
y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2
- (u64)y2 * (u64)x1;
y = div_u64(y, x2 - x1);
--
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 6/8] media: uvcvideo: Allow hw clock updates with buffers not full
2022-12-02 17:02 ` [PATCH RESEND v2 6/8] media: uvcvideo: Allow hw clock updates with buffers not full Ricardo Ribalda
@ 2022-12-30 14:37 ` Laurent Pinchart
0 siblings, 0 replies; 25+ messages in thread
From: Laurent Pinchart @ 2022-12-30 14:37 UTC (permalink / raw)
To: Ricardo Ribalda; +Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel
Hi Ricardo,
Thank you for the patch.
On Fri, Dec 02, 2022 at 06:02:46PM +0100, Ricardo Ribalda wrote:
> With UVC 1.5 we get as little as one clock sample per frame. Which means
> that it takes 32 frames to move from the software timestamp to the
> hardware timestamp method.
>
> This results in abrupt changes in the timestamping after 32 frames (~1
> second), resulting in noticeable artifacts when used for encoding.
>
> With this patch we modify the update algorithm to work with whatever
> amount of values are available.
This too makes me thing that we should *really* move this to userspace.
It will but much easier to implement clock domain translation there,
with a better precision.
> Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/usb/uvc/uvc_video.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 75c32e232f5d..7c6448c6d706 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -742,10 +742,10 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
>
> spin_lock_irqsave(&clock->lock, flags);
>
> - if (clock->count < clock->size)
> + if (clock->count < 2)
> goto done;
>
> - first = &clock->samples[clock->head];
> + first = &clock->samples[(clock->head - clock->count) % clock->size];
> last = &clock->samples[(clock->head - 1) % clock->size];
>
> /* First step, PTS to SOF conversion. */
> @@ -760,6 +760,14 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
> if (y2 < y1)
> y2 += 2048 << 16;
>
> + /*
> + * Have at least 1/4 of a second of timestamps before we
> + * try to do any calculation. Otherwise we do not have enough
> + * precission.
s/precission/precision/
How did you determine 250ms was the right threshold ?
> + */
> + if ((y2 - y1) < (256 << 16))
> + goto done;
> +
> y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2
> - (u64)y2 * (u64)x1;
> y = div_u64(y, x2 - x1);
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH RESEND v2 7/8] media: uvcvideo: Refactor clock circular buffer
2022-12-02 17:02 [PATCH RESEND v2 0/8] uvcvideo: Fixes for hw timestamping Ricardo Ribalda
` (5 preceding siblings ...)
2022-12-02 17:02 ` [PATCH RESEND v2 6/8] media: uvcvideo: Allow hw clock updates with buffers not full Ricardo Ribalda
@ 2022-12-02 17:02 ` Ricardo Ribalda
2022-12-30 14:39 ` Laurent Pinchart
2022-12-02 17:02 ` [PATCH RESEND v2 8/8] media: uvcvideo: Fix hw timestampt handling for slow FPS Ricardo Ribalda
7 siblings, 1 reply; 25+ messages in thread
From: Ricardo Ribalda @ 2022-12-02 17:02 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Laurent Pinchart
Cc: hn.chen, linux-media, Ricardo Ribalda, linux-kernel
Move the addition to the buffer into its own function and remove the
stream dependency to all the clock functions.
Tested-by: HungNien Chen <hn.chen@sunplusit.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_video.c | 81 ++++++++++++++++++---------------------
1 file changed, 37 insertions(+), 44 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 7c6448c6d706..c81a8362d582 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -464,18 +464,29 @@ static inline ktime_t uvc_video_get_time(void)
return ktime_get_real();
}
+static void uvc_video_clock_add_sample(struct uvc_clock *clock,
+ const struct uvc_clock_sample *sample)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&clock->lock, flags);
+
+ memcpy(&clock->samples[clock->head], sample, sizeof(*sample));
+ clock->last_sof = sample->dev_sof;
+ clock->head = (clock->head + 1) % clock->size;
+ clock->count = min(clock->count + 1, clock->size);
+
+ spin_unlock_irqrestore(&clock->lock, flags);
+}
+
static void
uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
const u8 *data, int len)
{
- struct uvc_clock_sample *sample;
+ struct uvc_clock_sample sample;
unsigned int header_size;
bool has_pts = false;
bool has_scr = false;
- unsigned long flags;
- ktime_t time;
- u16 host_sof;
- u16 dev_sof;
switch (data[1] & (UVC_STREAM_PTS | UVC_STREAM_SCR)) {
case UVC_STREAM_PTS | UVC_STREAM_SCR:
@@ -530,20 +541,19 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
* only the first one will match the host_sof.
*/
if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
- dev_sof = usb_get_current_frame_number(stream->dev->udev);
+ sample.dev_sof =
+ usb_get_current_frame_number(stream->dev->udev);
else
- dev_sof = get_unaligned_le16(&data[header_size - 2]);
+ sample.dev_sof = get_unaligned_le16(&data[header_size - 2]);
- if (dev_sof == stream->clock.last_sof)
+ if (sample.dev_sof == stream->clock.last_sof)
return;
- stream->clock.last_sof = dev_sof;
-
if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
- host_sof = dev_sof;
+ sample.host_sof = sample.dev_sof;
else
- host_sof = usb_get_current_frame_number(stream->dev->udev);
- time = uvc_video_get_time();
+ sample.host_sof = usb_get_current_frame_number(stream->dev->udev);
+ sample.host_time = uvc_video_get_time();
/*
* The UVC specification allows device implementations that can't obtain
@@ -566,46 +576,29 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
* the 8 LSBs of the delta are kept.
*/
if (stream->clock.sof_offset == (u16)-1) {
- u16 delta_sof = (host_sof - dev_sof) & 255;
+ u16 delta_sof = (sample.host_sof - sample.dev_sof) & 255;
if (delta_sof >= 10)
stream->clock.sof_offset = delta_sof;
else
stream->clock.sof_offset = 0;
}
- dev_sof = (dev_sof + stream->clock.sof_offset) & 2047;
-
- spin_lock_irqsave(&stream->clock.lock, flags);
-
- sample = &stream->clock.samples[stream->clock.head];
- sample->dev_stc = get_unaligned_le32(&data[header_size - 6]);
- sample->dev_sof = dev_sof;
- sample->host_sof = host_sof;
- sample->host_time = time;
-
- /* Update the sliding window head and count. */
- stream->clock.head = (stream->clock.head + 1) % stream->clock.size;
+ sample.dev_sof = (sample.dev_sof + stream->clock.sof_offset) & 2047;
+ sample.dev_stc = get_unaligned_le32(&data[header_size - 6]);
- if (stream->clock.count < stream->clock.size)
- stream->clock.count++;
-
- spin_unlock_irqrestore(&stream->clock.lock, flags);
+ uvc_video_clock_add_sample(&stream->clock, &sample);
}
-static void uvc_video_clock_reset(struct uvc_streaming *stream)
+static void uvc_video_clock_reset(struct uvc_clock *clock)
{
- struct uvc_clock *clock = &stream->clock;
-
clock->head = 0;
clock->count = 0;
clock->last_sof = -1;
clock->sof_offset = -1;
}
-static int uvc_video_clock_init(struct uvc_streaming *stream)
+static int uvc_video_clock_init(struct uvc_clock *clock)
{
- struct uvc_clock *clock = &stream->clock;
-
spin_lock_init(&clock->lock);
clock->size = 32;
@@ -614,15 +607,15 @@ static int uvc_video_clock_init(struct uvc_streaming *stream)
if (clock->samples == NULL)
return -ENOMEM;
- uvc_video_clock_reset(stream);
+ uvc_video_clock_reset(clock);
return 0;
}
-static void uvc_video_clock_cleanup(struct uvc_streaming *stream)
+static void uvc_video_clock_cleanup(struct uvc_clock *clock)
{
- kfree(stream->clock.samples);
- stream->clock.samples = NULL;
+ kfree(clock->samples);
+ clock->samples = NULL;
}
/*
@@ -2085,7 +2078,7 @@ int uvc_video_resume(struct uvc_streaming *stream, int reset)
stream->frozen = 0;
- uvc_video_clock_reset(stream);
+ uvc_video_clock_reset(&stream->clock);
if (!uvc_queue_streaming(&stream->queue))
return 0;
@@ -2219,7 +2212,7 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
{
int ret;
- ret = uvc_video_clock_init(stream);
+ ret = uvc_video_clock_init(&stream->clock);
if (ret < 0)
return ret;
@@ -2237,7 +2230,7 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
error_video:
usb_set_interface(stream->dev->udev, stream->intfnum, 0);
error_commit:
- uvc_video_clock_cleanup(stream);
+ uvc_video_clock_cleanup(&stream->clock);
return ret;
}
@@ -2265,5 +2258,5 @@ void uvc_video_stop_streaming(struct uvc_streaming *stream)
usb_clear_halt(stream->dev->udev, pipe);
}
- uvc_video_clock_cleanup(stream);
+ uvc_video_clock_cleanup(&stream->clock);
}
--
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 7/8] media: uvcvideo: Refactor clock circular buffer
2022-12-02 17:02 ` [PATCH RESEND v2 7/8] media: uvcvideo: Refactor clock circular buffer Ricardo Ribalda
@ 2022-12-30 14:39 ` Laurent Pinchart
0 siblings, 0 replies; 25+ messages in thread
From: Laurent Pinchart @ 2022-12-30 14:39 UTC (permalink / raw)
To: Ricardo Ribalda; +Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel
Hi Ricardo,
Thank you for the patch.
On Fri, Dec 02, 2022 at 06:02:47PM +0100, Ricardo Ribalda wrote:
> Move the addition to the buffer into its own function and remove the
> stream dependency to all the clock functions.
Commit messages need to explain *why* the commit performs a change, not
just how.
> Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/usb/uvc/uvc_video.c | 81 ++++++++++++++++++---------------------
> 1 file changed, 37 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 7c6448c6d706..c81a8362d582 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -464,18 +464,29 @@ static inline ktime_t uvc_video_get_time(void)
> return ktime_get_real();
> }
>
> +static void uvc_video_clock_add_sample(struct uvc_clock *clock,
> + const struct uvc_clock_sample *sample)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&clock->lock, flags);
> +
> + memcpy(&clock->samples[clock->head], sample, sizeof(*sample));
> + clock->last_sof = sample->dev_sof;
> + clock->head = (clock->head + 1) % clock->size;
> + clock->count = min(clock->count + 1, clock->size);
> +
> + spin_unlock_irqrestore(&clock->lock, flags);
> +}
> +
> static void
> uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> const u8 *data, int len)
> {
> - struct uvc_clock_sample *sample;
> + struct uvc_clock_sample sample;
> unsigned int header_size;
> bool has_pts = false;
> bool has_scr = false;
> - unsigned long flags;
> - ktime_t time;
> - u16 host_sof;
> - u16 dev_sof;
>
> switch (data[1] & (UVC_STREAM_PTS | UVC_STREAM_SCR)) {
> case UVC_STREAM_PTS | UVC_STREAM_SCR:
> @@ -530,20 +541,19 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> * only the first one will match the host_sof.
> */
> if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> - dev_sof = usb_get_current_frame_number(stream->dev->udev);
> + sample.dev_sof =
> + usb_get_current_frame_number(stream->dev->udev);
> else
> - dev_sof = get_unaligned_le16(&data[header_size - 2]);
> + sample.dev_sof = get_unaligned_le16(&data[header_size - 2]);
>
> - if (dev_sof == stream->clock.last_sof)
> + if (sample.dev_sof == stream->clock.last_sof)
> return;
>
> - stream->clock.last_sof = dev_sof;
> -
> if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> - host_sof = dev_sof;
> + sample.host_sof = sample.dev_sof;
> else
> - host_sof = usb_get_current_frame_number(stream->dev->udev);
> - time = uvc_video_get_time();
> + sample.host_sof = usb_get_current_frame_number(stream->dev->udev);
> + sample.host_time = uvc_video_get_time();
>
> /*
> * The UVC specification allows device implementations that can't obtain
> @@ -566,46 +576,29 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> * the 8 LSBs of the delta are kept.
> */
> if (stream->clock.sof_offset == (u16)-1) {
> - u16 delta_sof = (host_sof - dev_sof) & 255;
> + u16 delta_sof = (sample.host_sof - sample.dev_sof) & 255;
> if (delta_sof >= 10)
> stream->clock.sof_offset = delta_sof;
> else
> stream->clock.sof_offset = 0;
> }
>
> - dev_sof = (dev_sof + stream->clock.sof_offset) & 2047;
> -
> - spin_lock_irqsave(&stream->clock.lock, flags);
> -
> - sample = &stream->clock.samples[stream->clock.head];
> - sample->dev_stc = get_unaligned_le32(&data[header_size - 6]);
> - sample->dev_sof = dev_sof;
> - sample->host_sof = host_sof;
> - sample->host_time = time;
> -
> - /* Update the sliding window head and count. */
> - stream->clock.head = (stream->clock.head + 1) % stream->clock.size;
> + sample.dev_sof = (sample.dev_sof + stream->clock.sof_offset) & 2047;
> + sample.dev_stc = get_unaligned_le32(&data[header_size - 6]);
>
> - if (stream->clock.count < stream->clock.size)
> - stream->clock.count++;
> -
> - spin_unlock_irqrestore(&stream->clock.lock, flags);
> + uvc_video_clock_add_sample(&stream->clock, &sample);
> }
>
> -static void uvc_video_clock_reset(struct uvc_streaming *stream)
> +static void uvc_video_clock_reset(struct uvc_clock *clock)
> {
> - struct uvc_clock *clock = &stream->clock;
> -
> clock->head = 0;
> clock->count = 0;
> clock->last_sof = -1;
> clock->sof_offset = -1;
> }
>
> -static int uvc_video_clock_init(struct uvc_streaming *stream)
> +static int uvc_video_clock_init(struct uvc_clock *clock)
> {
> - struct uvc_clock *clock = &stream->clock;
> -
> spin_lock_init(&clock->lock);
> clock->size = 32;
>
> @@ -614,15 +607,15 @@ static int uvc_video_clock_init(struct uvc_streaming *stream)
> if (clock->samples == NULL)
> return -ENOMEM;
>
> - uvc_video_clock_reset(stream);
> + uvc_video_clock_reset(clock);
>
> return 0;
> }
>
> -static void uvc_video_clock_cleanup(struct uvc_streaming *stream)
> +static void uvc_video_clock_cleanup(struct uvc_clock *clock)
> {
> - kfree(stream->clock.samples);
> - stream->clock.samples = NULL;
> + kfree(clock->samples);
> + clock->samples = NULL;
> }
>
> /*
> @@ -2085,7 +2078,7 @@ int uvc_video_resume(struct uvc_streaming *stream, int reset)
>
> stream->frozen = 0;
>
> - uvc_video_clock_reset(stream);
> + uvc_video_clock_reset(&stream->clock);
>
> if (!uvc_queue_streaming(&stream->queue))
> return 0;
> @@ -2219,7 +2212,7 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
> {
> int ret;
>
> - ret = uvc_video_clock_init(stream);
> + ret = uvc_video_clock_init(&stream->clock);
> if (ret < 0)
> return ret;
>
> @@ -2237,7 +2230,7 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
> error_video:
> usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> error_commit:
> - uvc_video_clock_cleanup(stream);
> + uvc_video_clock_cleanup(&stream->clock);
>
> return ret;
> }
> @@ -2265,5 +2258,5 @@ void uvc_video_stop_streaming(struct uvc_streaming *stream)
> usb_clear_halt(stream->dev->udev, pipe);
> }
>
> - uvc_video_clock_cleanup(stream);
> + uvc_video_clock_cleanup(&stream->clock);
> }
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH RESEND v2 8/8] media: uvcvideo: Fix hw timestampt handling for slow FPS
2022-12-02 17:02 [PATCH RESEND v2 0/8] uvcvideo: Fixes for hw timestamping Ricardo Ribalda
` (6 preceding siblings ...)
2022-12-02 17:02 ` [PATCH RESEND v2 7/8] media: uvcvideo: Refactor clock circular buffer Ricardo Ribalda
@ 2022-12-02 17:02 ` Ricardo Ribalda
2022-12-30 14:51 ` Laurent Pinchart
7 siblings, 1 reply; 25+ messages in thread
From: Ricardo Ribalda @ 2022-12-02 17:02 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Laurent Pinchart
Cc: hn.chen, linux-media, Ricardo Ribalda, linux-kernel
In UVC 1.5, when working with FPS under 32, there is a chance that the
circular buffer contains two dev_sof overflows, but the clock interpolator
is only capable of handle a single overflow.
Remove all the samples from the circular buffer that are two overflows
old.
Tested-by: HungNien Chen <hn.chen@sunplusit.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_video.c | 15 +++++++++++++++
drivers/media/usb/uvc/uvcvideo.h | 1 +
2 files changed, 16 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index c81a8362d582..6b6bd521d6c2 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -471,6 +471,20 @@ static void uvc_video_clock_add_sample(struct uvc_clock *clock,
spin_lock_irqsave(&clock->lock, flags);
+ /* Delete last overflows */
+ if (clock->head == clock->last_sof_overflow)
+ clock->last_sof_overflow = -1;
+
+ /* Handle overflows */
+ if (clock->count > 0 && clock->last_sof > sample->dev_sof) {
+ /* Remove data from the last^2 overflows */
+ if (clock->last_sof_overflow != -1)
+ clock->count = (clock->head - clock->last_sof_overflow)
+ % clock->count;
+ clock->last_sof_overflow = clock->head;
+ }
+
+ /* Add sample */
memcpy(&clock->samples[clock->head], sample, sizeof(*sample));
clock->last_sof = sample->dev_sof;
clock->head = (clock->head + 1) % clock->size;
@@ -594,6 +608,7 @@ static void uvc_video_clock_reset(struct uvc_clock *clock)
clock->head = 0;
clock->count = 0;
clock->last_sof = -1;
+ clock->last_sof_overflow = -1;
clock->sof_offset = -1;
}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 14daa7111953..d8c520ce5a86 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -647,6 +647,7 @@ struct uvc_streaming {
unsigned int head;
unsigned int count;
unsigned int size;
+ unsigned int last_sof_overflow;
u16 last_sof;
u16 sof_offset;
--
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 8/8] media: uvcvideo: Fix hw timestampt handling for slow FPS
2022-12-02 17:02 ` [PATCH RESEND v2 8/8] media: uvcvideo: Fix hw timestampt handling for slow FPS Ricardo Ribalda
@ 2022-12-30 14:51 ` Laurent Pinchart
2023-01-03 23:34 ` Ricardo Ribalda
0 siblings, 1 reply; 25+ messages in thread
From: Laurent Pinchart @ 2022-12-30 14:51 UTC (permalink / raw)
To: Ricardo Ribalda; +Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel
Hi Ricardo,
Thank you for the patch.
On Fri, Dec 02, 2022 at 06:02:48PM +0100, Ricardo Ribalda wrote:
> In UVC 1.5, when working with FPS under 32, there is a chance that the
> circular buffer contains two dev_sof overflows,
An explanation of why this is the case would be good in the commit
message. Also, by overflow, are you talking about the SOF counter
rolling over ?
> but the clock interpolator
> is only capable of handle a single overflow.
>
> Remove all the samples from the circular buffer that are two overflows
> old.
I would rather support multiple roll-over in the clock interpolator.
Dropping older sampls will lead to less predictable behaviour and harder
to debug issues.
> Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/usb/uvc/uvc_video.c | 15 +++++++++++++++
> drivers/media/usb/uvc/uvcvideo.h | 1 +
> 2 files changed, 16 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index c81a8362d582..6b6bd521d6c2 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -471,6 +471,20 @@ static void uvc_video_clock_add_sample(struct uvc_clock *clock,
>
> spin_lock_irqsave(&clock->lock, flags);
>
> + /* Delete last overflows */
> + if (clock->head == clock->last_sof_overflow)
> + clock->last_sof_overflow = -1;
> +
> + /* Handle overflows */
> + if (clock->count > 0 && clock->last_sof > sample->dev_sof) {
> + /* Remove data from the last^2 overflows */
> + if (clock->last_sof_overflow != -1)
> + clock->count = (clock->head - clock->last_sof_overflow)
> + % clock->count;
> + clock->last_sof_overflow = clock->head;
> + }
> +
> + /* Add sample */
> memcpy(&clock->samples[clock->head], sample, sizeof(*sample));
> clock->last_sof = sample->dev_sof;
> clock->head = (clock->head + 1) % clock->size;
> @@ -594,6 +608,7 @@ static void uvc_video_clock_reset(struct uvc_clock *clock)
> clock->head = 0;
> clock->count = 0;
> clock->last_sof = -1;
> + clock->last_sof_overflow = -1;
> clock->sof_offset = -1;
> }
>
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 14daa7111953..d8c520ce5a86 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -647,6 +647,7 @@ struct uvc_streaming {
> unsigned int head;
> unsigned int count;
> unsigned int size;
> + unsigned int last_sof_overflow;
>
> u16 last_sof;
> u16 sof_offset;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 8/8] media: uvcvideo: Fix hw timestampt handling for slow FPS
2022-12-30 14:51 ` Laurent Pinchart
@ 2023-01-03 23:34 ` Ricardo Ribalda
2023-01-07 0:54 ` Laurent Pinchart
0 siblings, 1 reply; 25+ messages in thread
From: Ricardo Ribalda @ 2023-01-03 23:34 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel
Hi Laurent
On Fri, 30 Dec 2022 at 15:51, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Fri, Dec 02, 2022 at 06:02:48PM +0100, Ricardo Ribalda wrote:
> > In UVC 1.5, when working with FPS under 32, there is a chance that the
> > circular buffer contains two dev_sof overflows,
>
> An explanation of why this is the case would be good in the commit
> message. Also, by overflow, are you talking about the SOF counter
> rolling over ?
>
> > but the clock interpolator
> > is only capable of handle a single overflow.
> >
> > Remove all the samples from the circular buffer that are two overflows
> > old.
>
> I would rather support multiple roll-over in the clock interpolator.
> Dropping older sampls will lead to less predictable behaviour and harder
> to debug issues.
>
Multiple roll-over would not necessarily mean better data here. We are
deleting data that is more than 1 second away, and our resolution is
1kHz, which means that we have enough data to provide good results, we
have measured that 250msec already provides good data.
From a logical point of view, this patch is fixing a bug, not adding a
new feature, and it has been validated. I would rather add multi
roll-over as a follow-up patch, or maybe suggest implementing it in
userspace ;).
> > Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > drivers/media/usb/uvc/uvc_video.c | 15 +++++++++++++++
> > drivers/media/usb/uvc/uvcvideo.h | 1 +
> > 2 files changed, 16 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index c81a8362d582..6b6bd521d6c2 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -471,6 +471,20 @@ static void uvc_video_clock_add_sample(struct uvc_clock *clock,
> >
> > spin_lock_irqsave(&clock->lock, flags);
> >
> > + /* Delete last overflows */
> > + if (clock->head == clock->last_sof_overflow)
> > + clock->last_sof_overflow = -1;
> > +
> > + /* Handle overflows */
> > + if (clock->count > 0 && clock->last_sof > sample->dev_sof) {
> > + /* Remove data from the last^2 overflows */
> > + if (clock->last_sof_overflow != -1)
> > + clock->count = (clock->head - clock->last_sof_overflow)
> > + % clock->count;
> > + clock->last_sof_overflow = clock->head;
> > + }
> > +
> > + /* Add sample */
> > memcpy(&clock->samples[clock->head], sample, sizeof(*sample));
> > clock->last_sof = sample->dev_sof;
> > clock->head = (clock->head + 1) % clock->size;
> > @@ -594,6 +608,7 @@ static void uvc_video_clock_reset(struct uvc_clock *clock)
> > clock->head = 0;
> > clock->count = 0;
> > clock->last_sof = -1;
> > + clock->last_sof_overflow = -1;
> > clock->sof_offset = -1;
> > }
> >
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 14daa7111953..d8c520ce5a86 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -647,6 +647,7 @@ struct uvc_streaming {
> > unsigned int head;
> > unsigned int count;
> > unsigned int size;
> > + unsigned int last_sof_overflow;
> >
> > u16 last_sof;
> > u16 sof_offset;
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 8/8] media: uvcvideo: Fix hw timestampt handling for slow FPS
2023-01-03 23:34 ` Ricardo Ribalda
@ 2023-01-07 0:54 ` Laurent Pinchart
2023-01-09 8:13 ` Ricardo Ribalda
0 siblings, 1 reply; 25+ messages in thread
From: Laurent Pinchart @ 2023-01-07 0:54 UTC (permalink / raw)
To: Ricardo Ribalda; +Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel
Hi Ricardo,
On Wed, Jan 04, 2023 at 12:34:34AM +0100, Ricardo Ribalda wrote:
> On Fri, 30 Dec 2022 at 15:51, Laurent Pinchart wrote:
> > On Fri, Dec 02, 2022 at 06:02:48PM +0100, Ricardo Ribalda wrote:
> > > In UVC 1.5, when working with FPS under 32, there is a chance that the
> > > circular buffer contains two dev_sof overflows,
> >
> > An explanation of why this is the case would be good in the commit
> > message. Also, by overflow, are you talking about the SOF counter
> > rolling over ?
> >
> > > but the clock interpolator
> > > is only capable of handle a single overflow.
> > >
> > > Remove all the samples from the circular buffer that are two overflows
> > > old.
> >
> > I would rather support multiple roll-over in the clock interpolator.
> > Dropping older sampls will lead to less predictable behaviour and harder
> > to debug issues.
>
> Multiple roll-over would not necessarily mean better data here. We are
> deleting data that is more than 1 second away, and our resolution is
> 1kHz, which means that we have enough data to provide good results, we
> have measured that 250msec already provides good data.
Do we ? We may get one SCR per frame only. With low frame rates (say,
5fps for instance, which is fairly common, I have 2092 out of 16475
frame descriptors supporting that format in my database of UVC
descriptors), we'll have 4 to 5 data points.
> From a logical point of view, this patch is fixing a bug, not adding a
> new feature, and it has been validated. I would rather add multi
> roll-over as a follow-up patch, or maybe suggest implementing it in
> userspace ;).
Would you give the latter a try ? :-)
> > > Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > > drivers/media/usb/uvc/uvc_video.c | 15 +++++++++++++++
> > > drivers/media/usb/uvc/uvcvideo.h | 1 +
> > > 2 files changed, 16 insertions(+)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > index c81a8362d582..6b6bd521d6c2 100644
> > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > @@ -471,6 +471,20 @@ static void uvc_video_clock_add_sample(struct uvc_clock *clock,
> > >
> > > spin_lock_irqsave(&clock->lock, flags);
> > >
> > > + /* Delete last overflows */
> > > + if (clock->head == clock->last_sof_overflow)
> > > + clock->last_sof_overflow = -1;
> > > +
> > > + /* Handle overflows */
> > > + if (clock->count > 0 && clock->last_sof > sample->dev_sof) {
> > > + /* Remove data from the last^2 overflows */
> > > + if (clock->last_sof_overflow != -1)
> > > + clock->count = (clock->head - clock->last_sof_overflow)
> > > + % clock->count;
> > > + clock->last_sof_overflow = clock->head;
> > > + }
> > > +
> > > + /* Add sample */
> > > memcpy(&clock->samples[clock->head], sample, sizeof(*sample));
> > > clock->last_sof = sample->dev_sof;
> > > clock->head = (clock->head + 1) % clock->size;
> > > @@ -594,6 +608,7 @@ static void uvc_video_clock_reset(struct uvc_clock *clock)
> > > clock->head = 0;
> > > clock->count = 0;
> > > clock->last_sof = -1;
> > > + clock->last_sof_overflow = -1;
> > > clock->sof_offset = -1;
> > > }
> > >
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index 14daa7111953..d8c520ce5a86 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -647,6 +647,7 @@ struct uvc_streaming {
> > > unsigned int head;
> > > unsigned int count;
> > > unsigned int size;
> > > + unsigned int last_sof_overflow;
> > >
> > > u16 last_sof;
> > > u16 sof_offset;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH RESEND v2 8/8] media: uvcvideo: Fix hw timestampt handling for slow FPS
2023-01-07 0:54 ` Laurent Pinchart
@ 2023-01-09 8:13 ` Ricardo Ribalda
0 siblings, 0 replies; 25+ messages in thread
From: Ricardo Ribalda @ 2023-01-09 8:13 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, hn.chen, linux-media, linux-kernel
Hi Laurent
On Sat, 7 Jan 2023 at 01:54, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Wed, Jan 04, 2023 at 12:34:34AM +0100, Ricardo Ribalda wrote:
> > On Fri, 30 Dec 2022 at 15:51, Laurent Pinchart wrote:
> > > On Fri, Dec 02, 2022 at 06:02:48PM +0100, Ricardo Ribalda wrote:
> > > > In UVC 1.5, when working with FPS under 32, there is a chance that the
> > > > circular buffer contains two dev_sof overflows,
> > >
> > > An explanation of why this is the case would be good in the commit
> > > message. Also, by overflow, are you talking about the SOF counter
> > > rolling over ?
> > >
> > > > but the clock interpolator
> > > > is only capable of handle a single overflow.
> > > >
> > > > Remove all the samples from the circular buffer that are two overflows
> > > > old.
> > >
> > > I would rather support multiple roll-over in the clock interpolator.
> > > Dropping older sampls will lead to less predictable behaviour and harder
> > > to debug issues.
> >
> > Multiple roll-over would not necessarily mean better data here. We are
> > deleting data that is more than 1 second away, and our resolution is
> > 1kHz, which means that we have enough data to provide good results, we
> > have measured that 250msec already provides good data.
>
> Do we ? We may get one SCR per frame only. With low frame rates (say,
> 5fps for instance, which is fairly common, I have 2092 out of 16475
> frame descriptors supporting that format in my database of UVC
> descriptors), we'll have 4 to 5 data points.
In the current algorithm, the accuracy is not given by the number of
samples, but the time between the first and the last sample.
Every sample has an average error error of (1/clkrate)/2, but the
errors do not add up.
This is: 2 samples at 2 fps is as accurate as 4 samples at 4 fps.
>
> > From a logical point of view, this patch is fixing a bug, not adding a
> > new feature, and it has been validated. I would rather add multi
> > roll-over as a follow-up patch, or maybe suggest implementing it in
> > userspace ;).
>
> Would you give the latter a try ? :-)
Aren't these two orthogonal problems? The kernel today provides an API
and that API is broken for fps < 32, which is a common fps.
Even if we re-implement the hwtimestamp in user space we need to fix
it in the kernel.
>
> > > > Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > > drivers/media/usb/uvc/uvc_video.c | 15 +++++++++++++++
> > > > drivers/media/usb/uvc/uvcvideo.h | 1 +
> > > > 2 files changed, 16 insertions(+)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > index c81a8362d582..6b6bd521d6c2 100644
> > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > @@ -471,6 +471,20 @@ static void uvc_video_clock_add_sample(struct uvc_clock *clock,
> > > >
> > > > spin_lock_irqsave(&clock->lock, flags);
> > > >
> > > > + /* Delete last overflows */
> > > > + if (clock->head == clock->last_sof_overflow)
> > > > + clock->last_sof_overflow = -1;
> > > > +
> > > > + /* Handle overflows */
> > > > + if (clock->count > 0 && clock->last_sof > sample->dev_sof) {
> > > > + /* Remove data from the last^2 overflows */
> > > > + if (clock->last_sof_overflow != -1)
> > > > + clock->count = (clock->head - clock->last_sof_overflow)
> > > > + % clock->count;
> > > > + clock->last_sof_overflow = clock->head;
> > > > + }
> > > > +
> > > > + /* Add sample */
> > > > memcpy(&clock->samples[clock->head], sample, sizeof(*sample));
> > > > clock->last_sof = sample->dev_sof;
> > > > clock->head = (clock->head + 1) % clock->size;
> > > > @@ -594,6 +608,7 @@ static void uvc_video_clock_reset(struct uvc_clock *clock)
> > > > clock->head = 0;
> > > > clock->count = 0;
> > > > clock->last_sof = -1;
> > > > + clock->last_sof_overflow = -1;
> > > > clock->sof_offset = -1;
> > > > }
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > index 14daa7111953..d8c520ce5a86 100644
> > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > @@ -647,6 +647,7 @@ struct uvc_streaming {
> > > > unsigned int head;
> > > > unsigned int count;
> > > > unsigned int size;
> > > > + unsigned int last_sof_overflow;
> > > >
> > > > u16 last_sof;
> > > > u16 sof_offset;
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 25+ messages in thread