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@austad.us wrote:

quoted

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!

quoted

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.

quoted

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.

quoted

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