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/incl...
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