On 2021-03-24 13:44, Takashi Sakamoto wrote:
Hi,
On Wed, Mar 24, 2021 at 06:42:53AM +0100, David Henningsson wrote:
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 535a7229e1d9..f33076755025 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -736,12 +736,28 @@ struct snd_rawmidi_info {
unsigned char reserved[64]; /* reserved for future use */
};
+enum {
+ SNDRV_RAWMIDI_FRAMING_NONE = 0,
+ SNDRV_RAWMIDI_FRAMING_TSTAMP_MONOTONIC_RAW,
+ SNDRV_RAWMIDI_FRAMING_LAST = SNDRV_RAWMIDI_FRAMING_TSTAMP_MONOTONIC_RAW,
+};
Hi and thanks for the review!
In C language specification, enumeration is for value of int storage. In
my opinion, int type should be used for the framing member, perhaps.
In this case, I was following how the rest of the enum declarations were
in the same header. In addition, the only place it is used, is as an
unsigned char. So I'm not sure defining it as an int here would make sense.
(I think you can easily understand my insistent since you're Rust
programmer.)
I am, and as a side point: for those who don't know, I've written (and
maintaining) alsa-lib bindings for Rust in
https://github.com/diwic/alsa-rs
I note that in UAPI of Linux kernel, we have some macros to represent
system clocks; e.g. CLOCK_REALTIME, CLOCK_MONOTONIC:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/time.h#n46
We can use the series of macro, instead of defining the specific
enumerations. However I have one concern that the 'None' value cannot be
zero in the case since CLOCK_REALTIME is zero. This is a bit inconvenient
since we need initializer function in both of kernel space and user
space...
Interesting point. So I guess we could add a bit in the existing
bitfield that says on/off, and then have an unsigned char that decides
the clock type. But as you point out in your other reply, if we can get
a timestamp from closer to the source somehow, that would be even
better, and then that would be a clock that is something different from
the existing clock defines in time.h.
[snip from your other reply]
However, the timing jitter of IRQ handler invocation is issued in this
case, as well as PCM interface, even if the data rate of MIDI physical
layer is quite low nowadays (31.25 Kbit / sec ~= 3906.25 byte / sec).
As long as I experienced, in actual running Linux system, the invocation
of IRQ handler has no guarantee for timing jitter, mainly due to CPU level
IRQ mask (like spin_lock). Therefore the interval of each invocation is not
so precise to decide event timestamp, at least for time slot comes from
MIDI physical layer.
Nevertheless, I think your idea is enough interesting, with conditions to
deliver information from driver (or driver developer) to applications
(ALSA Sequencer or userspace applications). Even if we have some
limitation and restriction to precise timestamp, it's worth to work for
it. It seems to be required that improvements at interface level and
documentation about how to use the frame timestamp you implemented.
Right, so first, I believe the most common way to transport midi these
days is through USB, where the 31.25 Kbit/sec limit does not apply:
instead we have a granularity of 1/8 ms but many messages can fit in
each one. So that limit is - for many if not most cases - gone.
Second; you're probably right in that there is still some jitter w r t
when the IRQ fires. That is regrettable, but the earlier we get that
timestamp the better, so a timestamp in IRQ context would at least be
better than in a workqueue that fires after the IRQ, or in userspace
that possibly has scheduling delays.
Btw, I checked the "struct urb" and there was no timestamp in there, so
I don't know how to get a better timestamp than in snd_rawmidi_receive.
But maybe other interfaces (PCI, Firewire etc) offers something better.
// David
