Received-SPF: pass (google.com: domain of linux-kernel+bounces-111120-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20220920-resend-hwtimestamp-v9-0-55a89f46f6be@chromium.org>
 <20220920-resend-hwtimestamp-v9-2-55a89f46f6be@chromium.org> <20240321232602.GB20938@pendragon.ideasonboard.com>
In-Reply-To: <20240321232602.GB20938@pendragon.ideasonboard.com>
From: Ricardo Ribalda <ribalda@chromium.org>
Date: Fri, 22 Mar 2024 09:22:39 +0100
Message-ID: <CANiDSCt7Y4v3MUCoVyuzwLg6rq1=4MTUGtJ1+HkMMRY7sfjYjA@mail.gmail.com>
Subject: Re: [PATCH v9 2/6] media: uvcvideo: Ignore empty TS packets
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>, Sergey Senozhatsky <senozhatsky@chromium.org>, 
	linux-kernel@vger.kernel.org, "hn.chen" <hn.chen@sunplusit.com>, 
	linux-media@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

Hi Laurent

Hi, I added some minor modifications, hope that it is fine with you.

Thanks!!

On Fri, 22 Mar 2024 at 00:26, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Wed, Mar 15, 2023 at 02:30:13PM +0100, Ricardo Ribalda wrote:
> > Some SunplusIT 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:
>
> "Some SunplusIT devices send, e.g.,"
>
> >
> > 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
>
> "while the UVC specification meant that the first two packets shouldn't
> have had the SCR bit set in the header."
>
> >
> > This borderline/buggy interpretation has been implemented in a variety
> > of devices, from directly SunplusIT and from other OEMs that rebrand
> > SunplusIT products. So quirking based on VID:PID will be problematic.
> >
> > All the affected modules have the following extension unit:
> > VideoControl Interface Descriptor:
> >   guidExtensionCode         {82066163-7050-ab49-b8cc-b3855e8d221d}
> >
> > But the vendor plans to use that GUID in the future and fix the bug,
> > this means that we should use heuristic to figure out the broken
> > packets.
>
> Because it would have been too easy otherwise of course :-)
>
> >
> > This patch takes care of this.
> >
> > 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>
> > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index 4ff4ab4471fe..1f416c494acc 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -478,6 +478,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> >       ktime_t time;
> >       u16 host_sof;
> >       u16 dev_sof;
> > +     u32 dev_stc;
> >
> >       switch (data[1] & (UVC_STREAM_PTS | UVC_STREAM_SCR)) {
> >       case UVC_STREAM_PTS | UVC_STREAM_SCR:
> > @@ -526,6 +527,23 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> >       if (dev_sof == stream->clock.last_sof)
> >               return;
> >
> > +     dev_stc = get_unaligned_le32(&data[header_size - 6]);
> > +
> > +     /*
> > +      * STC (Source Time Clock) is the clock used by the camera. The UVC 1.5
> > +      * standard states that it "must be captured when the first video data
> > +      * of a video frame is put on the USB bus".
> > +      * Most of the vendors, clear the `UVC_STREAM_SCR` bit when the data is
> > +      * not valid, other vendors always set the `UVC_STREAM_SCR` bit and
> > +      * expect that the driver only samples the stc if there is data on the
> > +      * packet.
> > +      * Ignore all the hardware timestamp information if there is no data
> > +      * and stc and sof are zero.
> > +      */
>
> I'd like to expand this a bit (partly to make sure I understand the
> issue correctly):
>
>         /*
>          * STC (Source Time Clock) is the clock used by the camera. The UVC 1.5
>          * standard states that it "must be captured when the first video data
>          * of a video frame is put on the USB bus". This is generally understood
>          * as requiring devices to clear the payload header's SCR bit before
>          * the first packet containing video data.
>          *
>          * Most vendors follow that interpretation, but some (namely SunplusIT)
namely SunplusIT on some devices
>          * always set the `UVC_STREAM_SCR` bit, fill the SCR field with 0's,
>          * and expect that the driver only processes the SCR if there is data in
>          * the packet.
>          *
>          * Ignore all the hardware timestamp information if we haven't received
>          * any data for this frame yet, the packet contains no data, and both
>          * STC and SOF are zero. This heuristics should be safe on compliant
>          * devices. This should be safe with compliant devices, as in the very
>          * unlikely case where a UVC 1.1 device would send timing information
>          * only before the first packet containing data, and both STC and SOF
>          * happen to be zero for a particular frame, we would only miss one
>          * clock sample and the clock recovery algorithm wouldn't suffer from
one clock sample from many
>          * this condition.
>          */
>
> Is this correct (and fine with you) ? If so,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > +     if (buf && buf->bytesused == 0 && len == header_size &&
> > +         dev_stc == 0 && dev_sof == 0)
> > +             return;
> > +
> >       stream->clock.last_sof = dev_sof;
> >
> >       host_sof = usb_get_current_frame_number(stream->dev->udev);
> > @@ -564,7 +582,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> >       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_stc = dev_stc;
> >       sample->dev_sof = dev_sof;
> >       sample->host_sof = host_sof;
> >       sample->host_time = time;
> >
>
> --
> Regards,
>
> Laurent Pinchart


-- 
Ricardo Ribalda