Message-ID: <20161217090554.GA19737@icarus.home.austad.us>
Date:   Sat, 17 Dec 2016 10:05:54 +0100
From:   Henrik Austad <henrik@...tad.us>
To:     Richard Cochran <richardcochran@...il.com>
Cc:     linux-kernel@...r.kernel.org, Henrik Austad <haustad@...co.com>,
        linux-media@...r.kernel.org, alsa-devel@...r.kernel.org,
        netdev@...r.kernel.org
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, henrik@...tad.us 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

Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)

Powered by blists - more mailing lists