Re: [PATCH v2] sound: rawmidi: Add framing mode

[David Henningsson <coding@diwic.se>]

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