Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <ded0b788f5896621cffd8f64c679ef375231b6d1.camel@ndufresne.ca>
Subject: Re: [PATCH v2] media: uvcvideo: Implement V4L2_EVENT_FRAME_SYNC
From:   Nicolas Dufresne <nicolas@ndufresne.ca>
To:     Ricardo Ribalda <ribalda@chromium.org>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     Esker Wong <esker@google.com>,
        Kieran Bingham <kieran.bingham@ideasonboard.com>,
        Sakari Ailus <sakari.ailus@iki.fi>,
        Esker Wong <esker@chromium.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Thu, 23 Nov 2023 13:22:11 -0500
In-Reply-To: <CANiDSCuD=Z2FVaSnejCfLnRJXUSE99GQkK=4tRAXBbxpUqEVHg@mail.gmail.com>
References: <20231106-uvc-event-v2-1-7d8e36f0df16@chromium.org>
         <ZUjIlq0cxSv9Cut0@valkosipuli.retiisi.eu>
         <CAN_q1f_HV7Etb9i2c2_c6Trm2hAJUyd068UskJfMvT=OyiKXpA@mail.gmail.com>
         <fe672e31315b8f9c44a693c909d464a299e76093.camel@ndufresne.ca>
         <CAEZL83qR2bDq35yvCV-WvkaL6ZbPvSxQH+j=ViG6Kq8-0Mzq1Q@mail.gmail.com>
         <CANiDSCtDQ9Wg57YzVAJ1o5WQRmy1QPW8td8V2Scc08MmWtOwFg@mail.gmail.com>
         <03ac47742945cc04e4663b87563b47a96ed3ec1f.camel@ndufresne.ca>
         <CANiDSCunRRyod3Rg+L9ZsffOZyC7CKbMVTHX3u-n5iuNyZQk0w@mail.gmail.com>
         <20231109000327.GE21616@pendragon.ideasonboard.com>
         <CANiDSCuD=Z2FVaSnejCfLnRJXUSE99GQkK=4tRAXBbxpUqEVHg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

Le jeudi 09 novembre 2023 =C3=A0 01:27 +0100, Ricardo Ribalda a =C3=A9crit=
=C2=A0:
> Hi Laurent
>=20
> On Thu, 9 Nov 2023 at 01:03, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >=20
> > Hi Ricardo,
> >=20
> > On Wed, Nov 08, 2023 at 11:46:40PM +0100, Ricardo Ribalda wrote:
> > > On Wed, 8 Nov 2023 at 21:32, <nicolas@ndufresne.ca> wrote:
> > > >=20
> > > > The fact that you interpret the time from FRAME_SYNC to DQBUF (well=
 the
> > > > READ IO notification) as the actual latency is yours of course. It
> > > > assumes that the camera on the other end does not introduce other
> > >=20
> > > We want to use this signal to measure how much power is used since we
> > > start receiving the frame until we can use it.
> > > I agree with you that the latency between capture and dqbuf should be
> > > measured using the timestamp. That is not our use case here.
> > >=20
> > > > source of latency (or that these are negligible). You are also goin=
g to
> > > > introduce a lot of jitter, since it relies on when the OS decides t=
o
> > > > wake up your process.
> > >=20
> > > We have measured a jitter of around 2.5 msec, which is acceptable for=
 our needs.
> > >=20
> > > > I think my opinion resides in if you can accurately *enough* implem=
ent
> > > > what the spec says for FRAME_SYNC then do it, otherwise just don't =
lie.
> > >=20
> > > What the specs says is:
> > > ```
> > > Triggered immediately when the reception of a frame has begun
> > > ```
> > > In my opinion, that is true for usb devices, we are triggering it as
> > > soon as the transfer has started to the eyes of the driver. We cannot
> > > trigger earlier than that.
> > >=20
> > >=20
> > > > I think for ISO, "after the first chunk" i a small lie, but accepta=
ble.
> > > > But for BULK, the way it was explained is that it will be always ve=
ry
> > > > close to DQBUF time. and it should not emit FRAME_SYNC for this typ=
e of
> > > > UVC device. If it fits other events fine of course, I'm just making=
 a
> > > > judgment on if its fits V4L2_EVENT_FRAME_SYNC or not.
> > >=20
> > > nit: I believe that you have swapped iso and bulk on this description
> >=20
> > I've confused the USB packet size and the UVC payload size. The latter
> > is typically much bigger for bulk devices than isoc devices, but the
> > former will be in similar order of magnitudes in a large number of
> > cases, but not all cases.
> >=20
> > The URB size is the result of the USB packet size and number of packets
> > per URB. The uvcvideo driver currently sets the number of packets per
> > URB to 32 at most (and lowers it if the frame size is small, or if not
> > enough memory can be allocated). This could be increased or made dynami=
c
> > in the future, as higher speeds typically benefit from larger URB sizes=
.
> > The packet size differs between bulk and isoc endpoints.
> >=20
> > For bulk, the packet size can be up to 512 bytes for USB 2.0 and 1024
> > bytes for USB 3.0, and the device can select a smaller size. The larges=
t
> > URB size (again based on the current implementation of the uvcvideo
> > driver) is thus 32 KiB.
> >=20
> > For isochronous the situation is more complicated. The term "packet" as
> > used in the uvcvideo driver actually means all the data transferred in
> > one service interval, thus made of multiple isoc packets. It is heavily
> > dependent on the USB speed, and the device can advertise different
> > supported sizes (which translate directly to the reserved bandwidth for
> > the transfer), with the driver picking the smallest bandwidth large
> > enough for the data rate required by the resolution and frame rate. The
> > theoretical worst case is 1024 bytes per isoc packet * 16 isoc packets
> > per burst * 6 burst per interval * 32 "packets" per URB, equal to 3 MiB=
.
> >=20
> > Even with the largest URB size you have witnessed of ~1 MiB, we will en=
d
> > up lying quite a bit if we consider the URB completion callback for the
> > first URB of the frame as indicating the start of reception.
> >=20
> > > > In term of accuracy, if timestamp was passed with the FRAME_SYNC ev=
ent,
> > > > it would not matter how fast your process the event anymore and gre=
atly
> > > > improve accuracy.
> > >=20
> > > +1 to that. If we could easily change the uAPI for FRAME_SYNC that
> > > should definitely be implemented.
> > >=20
> > > > > Not to mention that the UVC timestamping requires a bit of love.
> > > > >=20
> > > > > @Laurent Pinchart, @Kieran Bingham  any progress reviewing :P :
> > > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=3D=
10083
> > > >=20
> > > > Thanks for working on this by the way, hope someone will find the t=
ime
> > > > to review this. The timestamps should in theory provide a jitter fr=
ee
> > >=20
> > > It already has a couple of Reviewed-by stamped in.... ;)
> > >=20
> > > > measurement of the delay Esker is trying to measure, and if it wasn=
't
> > > > of bugs (and crazy complexity) it would in the worst case match the
> > > > transfer time.
> > >=20
> > > Sorry to repeat myself, but just to avoid the confusion: Esker needs
> > > to know how much power is used since we start receiving a frame until
> > > it is available for dqbuf, not de frame latency.
> >=20
> > As I think everybody is aware, the earliest notification you get on the
> > CPU side is the *end* of reception of the first URB, which can possibly
> > be significantly later than the start of reception of the frame.
> >=20
> > Based on what I understand, the goal is to measure the CPU power
> > consumption related to CPU processing of the frame. If that's the case,
> > there's good and bad news. The good news is that the CPU doesn't proces=
s
> > the frame at all until the URB has been received (if you were to measur=
e
> > the power consumption of the USB host controller too, it would be a
> > different story), so the delay shouldn't be a problem. The bad news is
> > that I don't see how the information you're trying to get will help you=
,
> > as there's plenty of other things unrelated to the uvcvideo driver that
> > can take CPU time while a frame is being received. That may not be any
> > of my business, but from the point of view of the uvcvideo driver, I'm
> > less inclined to accept a possibly significant V4L2_EVENT_FRAME_SYNC li=
e
> > if the use case ends up making little sense :-)
>=20
> Just to add some numbers to add some context:
>=20
>  V4L2_EVENT_FRAME_SYNC for BULK and ISO will be delayed from:
> ```
> Triggered immediately when the reception of a frame has begun
> ```
> one urb.
>=20
> For bulk devices this is a maximum of 0.05 msec (32KiB/600MBps)

I lack a bit of knowledge on how to scale this to different devices, with
different speed/framesize. My only bulk device is:

https://inogeni.com/product/4k2usb3/

Which is USB 3.0, and have raw (NV12) resolution from 640x480 (max 60pfs) t=
o 4K
(max 30fps). What would the error look like with that ?

> For 1MiB transfer isoc devices (which is the biggest we have seen),
> that is 1.8 msec.
> In both cases, this is smaller than the jitter to process the event
> itself by userspace.
>=20
> The time from V4L2_EVENT_FRAME_SYNC to DQBUF is around 30 msec.
>=20
> I do not know how much delay is considered acceptable... but if we
> take the delay argument to the extreme, we could say that
> V4L2_EVENT_FRAME_SYNC can never be implemented, because the event will
> always be delayed by something.

We have v4l2_event.timestamp for all events, so the jitter to process the e=
vent
by userpace can be removed reliably already.

Nicolas

p.s. missed it earlier

>=20
> >=20
> > --
> > Regards,
> >=20
> > Laurent Pinchart
>=20
>=20
>=20