On Fri, Dec 16, 2016 at 11:09:38PM +0100, Richard Cochran wrote:
> On Fri, Dec 16, 2016 at 06:59:09PM +0100, [email protected] wrote:
> > +/*
> > + * List of current subtype fields in the common header of AVTPDU
> > + *
> > + * Note: AVTPDU is a remnant of the standards from when it was AVB.
> > + *
> > + * The list has been updated with the recent values from IEEE 1722, draft 16.
> > + */
> > +enum avtp_subtype {
> > + TSN_61883_IIDC = 0, /* IEC 61883/IIDC Format */
> > + TSN_MMA_STREAM, /* MMA Streams */
> > + TSN_AAF, /* AVTP Audio Format */
> > + TSN_CVF, /* Compressed Video Format */
> > + TSN_CRF, /* Clock Reference Format */
> > + TSN_TSCF, /* Time-Synchronous Control Format */
> > + TSN_SVF, /* SDI Video Format */
> > + TSN_RVF, /* Raw Video Format */
> > + /* 0x08 - 0x6D reserved */
> > + TSN_AEF_CONTINOUS = 0x6e, /* AES Encrypted Format Continous */
> > + TSN_VSF_STREAM, /* Vendor Specific Format Stream */
> > + /* 0x70 - 0x7e reserved */
> > + TSN_EF_STREAM = 0x7f, /* Experimental Format Stream */
> > + /* 0x80 - 0x81 reserved */
> > + TSN_NTSCF = 0x82, /* Non Time-Synchronous Control Format */
> > + /* 0x83 - 0xed reserved */
> > + TSN_ESCF = 0xec, /* ECC Signed Control Format */
> > + TSN_EECF, /* ECC Encrypted Control Format */
> > + TSN_AEF_DISCRETE, /* AES Encrypted Format Discrete */
> > + /* 0xef - 0xf9 reserved */
> > + TSN_ADP = 0xfa, /* AVDECC Discovery Protocol */
> > + TSN_AECP, /* AVDECC Enumeration and Control Protocol */
> > + TSN_ACMP, /* AVDECC Connection Management Protocol */
> > + /* 0xfd reserved */
> > + TSN_MAAP = 0xfe, /* MAAP Protocol */
> > + TSN_EF_CONTROL, /* Experimental Format Control */
> > +};
>
> The kernel shouldn't be in the business of assembling media packets.

No, but assembling the packets and shipping frames to a destination is not
neccessarily the same thing.

A nice workflow would be to signal to the shim that "I'm sending a
compressed video format" and then the shim/tsn_core will ship out the
frames over the network - and then you need to set TSN_CVF as subtype in
each header.

That does not that mean you should do H.264 encode/decode *in* the kernel

Perhaps this is better placed in include/uapi/tsn.h so that userspace and
kernel share the same header?

--
Henrik Austad

Attachments:

(No filename) (2.23 kB)
signature.asc (181.00 B)
Download all attachments

2016-12-17 09:06:12

by Henrik Austad

[permalink] [raw]

Subject: Re: [TSN RFC v2 0/9] TSN driver for the kernel

Hi Richard,

On Fri, Dec 16, 2016 at 11:05:30PM +0100, Richard Cochran wrote:
> On Fri, Dec 16, 2016 at 06:59:04PM +0100, [email protected] wrote:
> > The driver is directed via ConfigFS as we need userspace to handle
> > stream-reservation (MSRP), discovery and enumeration (IEEE 1722.1) and
> > whatever other management is needed.
>
> I complained about configfs before, but you didn't listen.

Yes you did, I remember quite well, and no, I didn't listen :)

At the time, there were other issues that I had to address. The
configfs-part is fairly isolated. As I tried to explain the last round,
the *reason* I've used ConfigFS thus far, is because it makes it pretty
easy from userspace to signal the driver to create a new alsa-device.

And the reason I haven't changed configfs, is because so far, that part has
worked fairly well and have made testing quite easy. At this stage, *this*
is what is helpful, not a perfect interface. This does not mean that
configfs is set in stone.

To clearify:
I'm sending out a new set now because, what I have works _fairly_ well for
testing and a way to see what you can do with AVB. Using spotify to play
music on random machines is quite entertaining.

It is by no means -done-, nor do I consider it done. I have been tight on
time, and instead of sitting in an office polishing on some code, I thought
it better to send out a new (and not done) set of patches so that others
could see it still being worked on. If this turned out to be noise-only, I
appologize!

> > 2 new fields in netdev_ops have been introduced, and the Intel
> > igb-driver has been updated (as this an AVB-capable NIC which is
> > available as a PCI-e card).
>
> The igb hacks show that you are on the wrong track. We can and should
> be able to support TSN without resorting to driver specific hacks and
> module parameters.

I was not able to find a sane way to change the mode of the NIC, some of
the settings required to enable Qav-mode must be done when bringing the
NIC up, so I needed hooks in _probe().

ANother elemnt needed is a way for tsn_core to ascertain if a NIC is
capable of TSN or not (this would be ndo_tsn_capable)

Then finally, you need to update values in a per-tx-queue manner when a new
stream is ready (hence ndo_tsn_link_configure).

What you mean by 'driver specific hacks' is not obvious though, TSN
requires a set of fairly standardized parameters (priority code points,
size of frames to send in a new stream and so on), adding this to the
hw-registers in the NIC is an operation that will be common for all
TSN-capable NICs.

> > Before reading on - this is not even beta, but I'd really appreciate if
> > people would comment on the overall architecture and perhaps provide
> > some pointers to where I should improve/fix/update
>
> As I said before about V1, this architecture stinks.

I like feedback when it's short, sweet and to the point
2 out of 3 ain't that bad ;)

> You appear to have continued hacking along and posted the same design
> again. Did you even address any of the points I raised back then?

So you did raise a lot of good points the last round, and no, I have not
had the time to address them properly. That does not mean I do not *want*
to (apart from configfs actually having worked quite nicely thus far and
'shim' being a name I like ;)

From the last round of discussion:

> 1. A proper userland stack for AVDECC, MAAP, FQTSS, and so on. The
> OpenAVB project does not offer much beyond simple examples.

Yes, I fully agree, as far as I know, no-one is working on this. That being
said, I have not paid much attention the userspace tooling lately. But all
of this must be handled in userspace, having avdecc in the kernel would be
an utter nightmare :)

> 2. A user space audio application that puts it all together, making
> use of the services in #1, the linuxptp gPTP service, the ALSA
> services, and the network connections. This program will have all
> the knowledge about packet formats, AV encodings, and the local HW
> capabilities. This program cannot yet be written, as we still need
> some kernel work in the audio and networking subsystems.

And therein lies the problem. It cannot yet be written, so we have to start
in *some* end. And as I repeatedly stated in June, I'm at an RFC here,
trying to spark some interest and lure other developers in :)

Also, I really do not want a media-application to care about _where_ the
frames are going. Sure, I see the issue of configuring a link, but that can
be done from _outside_ the media-application. VLC (or aplay, or totem, or
.. take your pick) should not have to worry about this.

Applications that require finer control over timestamping is easier to
adapt to AVB than all the others, I'd rather add special knobs for those
who are interested than adding a set of knobs that -all- applications must
be aware of.

Could be that we are talking about the same thing, just from different
perspectives.

> * Kernel Space
>
> 1. Providing frames with a future transmit time. For normal sockets,
> this can be in the CMESG data. For mmap'ed buffers, we will need a
> new format. (I think Arnd is working on a new layout.)

I need to revisit that discussion again I think.

> 2. Time based qdisc for transmitted frames. For MACs that support
> this (like the i210), we only have to place the frame into the
> correct queue. For normal HW, we want to be able to reserve a time
> window in which non-TSN frames are blocked. This is some work, but
> in the end it should be a generic solution that not only works
> "perfectly" with TSN HW but also provides best effort service using
> any NIC.

Yes, indeed, that would be one good solution, and quite a lot of work.

> 3. ALSA support for tunable AD/DA clocks. The rate of the Listener's
> DA clock must match that of the Talker and the other Listeners.

To nitpick a bit, all AD/DAs should match that of the gPTP grandmaster
(which in most settings would be the Talker). But yes, you need to adjust
the AD/DA. SRC is slow and painful, best to avoid.

> Either you adjust it in HW using a VCO or similar, or you do
> adaptive sample rate conversion in the application. (And that is
> another reason for *not* having a shared kernel buffer.) For the
> Talker, either you adjust the AD clock to match the PTP time, or
> you measure the frequency offset.

Yes, some hook into adjusting the clock is needed, I wonder if this is
possible via V4L2, or of the monitor-world is a completely different beast.

> 4. ALSA support for time triggered playback. The patch series
> completely ignore the critical issue of media clock recovery. The
> Listener must buffer the stream in order to play it exactly at a
> specified time. It cannot simply send the stream ASAP to the audio
> HW, because some other Listener might need longer. AFAICT, there is
> nothing in ALSA that allows you to say, sample X should be played at
> time Y.

Yes, and this requires a lot of change to ALSA (and probably something in
V4L2 as well?), so before we get to that, perhaps have a set of patches
that does this best effort and *then* work on getting time-triggered
playback into the kernel?

Another item that was brought up last round was getting timing-information
to/from ALSA, See driver/media/avb/avb_alsa.c, as a start it updates the
time for last incoming/outgoing frame so that userspace can get that
information. Probably buggy as heck :)

* Back to your email from last night*

> You are trying to put tons of code into the kernel that really belongs
> in user space, and at the same time, you omit critical functions that
> only the kernel can provide.

Some (well, to be honest, most) of the of the critical functions that my
driver omits, are omitted because they require substantial effort to
implement - and befor there's a need for this, that won't happen. So,
consider the TSN-driver such a need!

I'd love to use a qdisc that uses a time-triggered transmit, that would
drop the need for a lot of the stuff in tsn_core.c. The same goes for
time-triggered playback in media.

> > There are at least one AVB-driver (the AV-part of TSN) in the kernel
> > already.
>
> And which driver is that?

Ah, a proverbial slip of the changelog, we visited this the last iteration,
that would be the ravb-driver (which is an AVB capable NIC), but it does
not include much in the way of AVB-support *In* kernel. Sorry about that!

Since then, the iMX7 from NXP has arrived, and this also has HW-support for
TSN, but not in the kernel AFAICT.

So, the next issue I plan to tackle, is how I do buffers, the current
approach where tsn_core allocates memory is on its way out and I'll let the
shim (which means alsa/v4l2) will provide a buffer. Then I'll start looking
at qdisc.

Thanks!

--
Henrik Austad

Attachments:

(No filename) (8.70 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2016-12-17 20:09:42

by Richard Cochran

[permalink] [raw]

Subject: Re: [TSN RFC v2 0/9] TSN driver for the kernel

On Sat, Dec 17, 2016 at 10:05:54AM +0100, Henrik Austad wrote:
> I'm sending out a new set now because, what I have works _fairly_ well for
> testing and a way to see what you can do with AVB. Using spotify to play
> music on random machines is quite entertaining.

You have missed the point about TSN entirely. Unless your demo has
synchronized playback (in the low microsecond range), then it really
is pointless. We can already play music over the LAN using gstreamer,
without a single kernel change. Heck, gstreamer even has its own
rudimentary synchronization, so your series is a step backwards.

> And therein lies the problem. It cannot yet be written, so we have to start
> in *some* end. And as I repeatedly stated in June, I'm at an RFC here,
> trying to spark some interest and lure other developers in :)

The best way to attract interest is to provide the critical
infrastructure missing in the kernel. Coding a media player in kernel
space is not very interesting in my view.

> Yes, and this requires a lot of change to ALSA (and probably something in
> V4L2 as well?), so before we get to that, perhaps have a set of patches
> that does this best effort and *then* work on getting time-triggered
> playback into the kernel?

No, we don't need a best effort implementation. That is gstreamer and Co.

> So, the next issue I plan to tackle, is how I do buffers, the current
> approach where tsn_core allocates memory is on its way out and I'll let the
> shim (which means alsa/v4l2) will provide a buffer. Then I'll start looking
> at qdisc.

More than once you wrote something like, "I know that's needed, but it
is just too hard ATM." Please start with qdisc and tc. That
shouldn't be too hard. If we had the AVB shaping rules with one or
two drivers supporting them, that would be one piece already done.

Thanks,
Richard