[PATCH 0/1] sound: rawmidi: Add framing mode

[PATCH 0/1] sound: rawmidi: Add framing mode

[David Henningsson]

Hi,

When writing an application that records midi (and e g saves it to disk), ultra-low latency isn't really needed, all we need to know is exactly when the midi message came in. The application can then wake up once a second or so, to write the incoming data, including an accurate timestamp for every event, to disk.

As far as I can see, the rawmidi interface does not support such a feature at all. There is a snd_rawmidi_status syscall, but its timestamp field is not even filled by the kernel (!). But even if that was fixed, it would not fix the problem as there could be several midi events in the buffer with different timestamps.

You could use the seq interface, it does support timestamps, but I can see at least two potential problems with this:

First, the seq code runs in a work queue, not in the actual IRQ. This means that midi event is timestamped too late, especially so if the work is delayed for some reason.

Second, seq hard-codes the timestamp type to monotonic - there is no monotonic_raw, so the timestamp would be affected by NTP changes.

Also, the timespec uses 32-bit for sec and nsec, but I suspect this is less of a problem (unless people constantly record midi for sixty years or so...).

So here's a patch that adds a new "framing" mode that stuffs all MIDI data into 16 byte frames, 8 bytes of timestamp, one byte length, up to seven bytes of data. I'll follow up with an alsa-lib patch if this gets merged. 

David Henningsson (1):
  sound: rawmidi: Add framing mode

 include/sound/rawmidi.h     |  1 +
 include/uapi/sound/asound.h | 18 ++++++++++++++-
 sound/core/rawmidi.c        | 45 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 62 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH 1/1] sound: rawmidi: Add framing mode

[David Henningsson]

This commit adds a new framing mode that frames all MIDI data into
16-byte frames with a timestamp from the monotonic_raw clock.

The main benefit is that we can get accurate timestamps even if
userspace wakeup and processing is not immediate.
---
 include/sound/rawmidi.h     |  1 +
 include/uapi/sound/asound.h | 18 ++++++++++++++-
 sound/core/rawmidi.c        | 45 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h
index 334842daa904..ea4d88d513e1 100644
--- a/include/sound/rawmidi.h
+++ b/include/sound/rawmidi.h
@@ -81,6 +81,7 @@ struct snd_rawmidi_substream {
 	bool opened;			/* open flag */
 	bool append;			/* append flag (merge more streams) */
 	bool active_sensing;		/* send active sensing when close */
+	u8 framing; /* whether to frame data (for input) */
 	int use_count;			/* use counter (for output) */
 	size_t bytes;
 	struct snd_rawmidi *rmidi;
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 535a7229e1d9..13c3865a818e 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,
+};
+
+#define SND_RAWMIDI_FRAMING_DATA_LENGTH 7
+
+struct snd_rawmidi_framing_tstamp {
+	unsigned int tv_sec;	/* seconds */
+	unsigned int tv_nsec;	/* nanoseconds */
+	unsigned char length;
+	unsigned char data[SND_RAWMIDI_FRAMING_DATA_LENGTH];
+};
+
 struct snd_rawmidi_params {
 	int stream;
 	size_t buffer_size;		/* queue size in bytes */
 	size_t avail_min;		/* minimum avail bytes for wakeup */
 	unsigned int no_active_sensing: 1; /* do not send active sensing byte in close() */
-	unsigned char reserved[16];	/* reserved for future use */
+	unsigned char framing; /* For input data only, frame incoming data */
+	unsigned char reserved[15];	/* reserved for future use */
 };
 
 #ifndef __KERNEL__
diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index aca00af93afe..fefa7d9b70a6 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -721,6 +721,7 @@ int snd_rawmidi_input_params(struct snd_rawmidi_substream *substream,
 			     struct snd_rawmidi_params *params)
 {
 	snd_rawmidi_drain_input(substream);
+	substream->framing = params->framing;
 	return resize_runtime_buffer(substream->runtime, params, true);
 }
 EXPORT_SYMBOL(snd_rawmidi_input_params);
@@ -963,6 +964,44 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card,
 	return -ENOIOCTLCMD;
 }
 
+static int receive_with_tstamp_framing(struct snd_rawmidi_substream *substream,
+			const unsigned char *buffer, int src_count, struct timespec64 *tstamp)
+{
+	struct snd_rawmidi_runtime *runtime = substream->runtime;
+	struct snd_rawmidi_framing_tstamp frame;
+	struct snd_rawmidi_framing_tstamp *dest_ptr;
+
+	int dest_frames = 0;
+	int frame_size = sizeof(struct snd_rawmidi_framing_tstamp);
+	frame.tv_sec = tstamp->tv_sec;
+	frame.tv_nsec = tstamp->tv_nsec;
+
+	if (snd_BUG_ON(runtime->hw_ptr & 15 || runtime->buffer_size & 15 || frame_size != 16))
+		return -EINVAL;
+	while (src_count > 0) {
+		if ((int)(runtime->buffer_size - runtime->avail) < frame_size) {
+			runtime->xruns += src_count;
+			return dest_frames * frame_size;
+		}
+		if (SND_RAWMIDI_FRAMING_DATA_LENGTH < src_count)
+			frame.length = SND_RAWMIDI_FRAMING_DATA_LENGTH;
+		else {
+			frame.length = src_count;
+			memset(frame.data, 0, SND_RAWMIDI_FRAMING_DATA_LENGTH);
+		}
+		memcpy(frame.data, buffer, frame.length);
+		buffer += frame.length;
+		src_count -= frame.length;
+		dest_ptr = (struct snd_rawmidi_framing_tstamp *) (runtime->buffer + runtime->hw_ptr);
+		*dest_ptr = frame;
+		runtime->avail += frame_size;
+		runtime->hw_ptr += frame_size;
+		runtime->hw_ptr %= runtime->buffer_size;
+		dest_frames++;
+	}
+	return dest_frames * frame_size;
+}
+
 /**
  * snd_rawmidi_receive - receive the input data from the device
  * @substream: the rawmidi substream
@@ -988,7 +1027,11 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream,
 		return -EINVAL;
 	}
 	spin_lock_irqsave(&runtime->lock, flags);
-	if (count == 1) {	/* special case, faster code */
+	if (substream->framing == SNDRV_RAWMIDI_FRAMING_TSTAMP_MONOTONIC_RAW) {
+		struct timespec64 ts64;
+		ktime_get_raw_ts64(&ts64);
+		result = receive_with_tstamp_framing(substream, buffer, count, &ts64);
+	} else if (count == 1) {	/* special case, faster code */
 		substream->bytes++;
 		if (runtime->avail < runtime->buffer_size) {
 			runtime->buffer[runtime->hw_ptr++] = buffer[0];
-- 
2.25.1

Re: [PATCH 1/1] sound: rawmidi: Add framing mode

[Jaroslav Kysela]

Dne 24. 03. 21 v 6:31 David Henningsson napsal(a):
> This commit adds a new framing mode that frames all MIDI data into
> 16-byte frames with a timestamp from the monotonic_raw clock.

I would add support for monotonic timestamps, too. The NTP drifts are usually
small, so it may make sense to support those timestamps, too. It may be handy
for the synchronization among multiple machines (timing sources).

The timestamp mode should be selected separately than the framing mode.

> +#define SND_RAWMIDI_FRAMING_DATA_LENGTH 7
> +
> +struct snd_rawmidi_framing_tstamp {
> +	unsigned int tv_sec;	/* seconds */
> +	unsigned int tv_nsec;	/* nanoseconds */
> +	unsigned char length;
> +	unsigned char data[SND_RAWMIDI_FRAMING_DATA_LENGTH];
> +};

Perhaps, we should consider to have a fixed header and variable data length
here. For MIDI, the standard messages have only few bytes usually. It would be
better to use this space for the seconds field:

header {
	unsigned long long tv_sec;
	unsigned int tv_nsec;
	unsigned int len;
	unsigned char data[0];
};

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH 1/1] sound: rawmidi: Add framing mode

[David Henningsson]

On 2021-03-24 17:06, Jaroslav Kysela wrote:
> Dne 24. 03. 21 v 6:31 David Henningsson napsal(a):
>> This commit adds a new framing mode that frames all MIDI data into
>> 16-byte frames with a timestamp from the monotonic_raw clock.
> I would add support for monotonic timestamps, too. The NTP drifts are usually
> small, so it may make sense to support those timestamps, too. It may be handy
> for the synchronization among multiple machines (timing sources).
>
> The timestamp mode should be selected separately than the framing mode.
Okay, noted for v3.
>
>> +#define SND_RAWMIDI_FRAMING_DATA_LENGTH 7
>> +
>> +struct snd_rawmidi_framing_tstamp {
>> +	unsigned int tv_sec;	/* seconds */
>> +	unsigned int tv_nsec;	/* nanoseconds */
>> +	unsigned char length;
>> +	unsigned char data[SND_RAWMIDI_FRAMING_DATA_LENGTH];
>> +};
> Perhaps, we should consider to have a fixed header and variable data length
> here. For MIDI, the standard messages have only few bytes usually. It would be
> better to use this space for the seconds field:
>
> header {
> 	unsigned long long tv_sec;
> 	unsigned int tv_nsec;
> 	unsigned int len;
> 	unsigned char data[0];
> };

I considered that, but it has problems with alignment. If you have a 
normal midi message of 3 bytes, now your second tv_sec will end up 
starting on an odd byte, unless you add padding, and then that padding 
needs to be specified and so on. In addition, half of the header could 
end up in the end of the ring buffer and the other half in the 
beginning. So I found the 16 byte fixed version to be simpler and easier 
to implement correctly.

However if you like we could change the tv_sec to 64 bit and end up with:

#define SND_RAWMIDI_FRAMING_DATA_LENGTH 3

struct snd_rawmidi_framing_tstamp {
	unsigned long long tv_sec;	/* seconds */
	unsigned int tv_nsec;	/* nanoseconds */
	unsigned char length;
	unsigned char data[SND_RAWMIDI_FRAMING_DATA_LENGTH];
};

We'll then have only three bytes for the actual data, but since that is what most midi messages are anyway, it would be okay, I assume.

// David

Re: [PATCH 1/1] sound: rawmidi: Add framing mode

[Jaroslav Kysela]

Dne 24. 03. 21 v 17:17 David Henningsson napsal(a):
> 
> On 2021-03-24 17:06, Jaroslav Kysela wrote:
>> Dne 24. 03. 21 v 6:31 David Henningsson napsal(a):
>>> This commit adds a new framing mode that frames all MIDI data into
>>> 16-byte frames with a timestamp from the monotonic_raw clock.
>> I would add support for monotonic timestamps, too. The NTP drifts are usually
>> small, so it may make sense to support those timestamps, too. It may be handy
>> for the synchronization among multiple machines (timing sources).
>>
>> The timestamp mode should be selected separately than the framing mode.
> Okay, noted for v3.
>>
>>> +#define SND_RAWMIDI_FRAMING_DATA_LENGTH 7
>>> +
>>> +struct snd_rawmidi_framing_tstamp {
>>> +	unsigned int tv_sec;	/* seconds */
>>> +	unsigned int tv_nsec;	/* nanoseconds */
>>> +	unsigned char length;
>>> +	unsigned char data[SND_RAWMIDI_FRAMING_DATA_LENGTH];
>>> +};
>> Perhaps, we should consider to have a fixed header and variable data length
>> here. For MIDI, the standard messages have only few bytes usually. It would be
>> better to use this space for the seconds field:
>>
>> header {
>> 	unsigned long long tv_sec;
>> 	unsigned int tv_nsec;
>> 	unsigned int len;
>> 	unsigned char data[0];
>> };
> 
> I considered that, but it has problems with alignment. If you have a 
> normal midi message of 3 bytes, now your second tv_sec will end up 
> starting on an odd byte, unless you add padding, and then that padding 
> needs to be specified and so on. In addition, half of the header could 
> end up in the end of the ring buffer and the other half in the 
> beginning. So I found the 16 byte fixed version to be simpler and easier 
> to implement correctly.

I see. I agree that the fixed frame is easier to handle.

> However if you like we could change the tv_sec to 64 bit and end up with:
> 
> #define SND_RAWMIDI_FRAMING_DATA_LENGTH 3
> 
> struct snd_rawmidi_framing_tstamp {
> 	unsigned long long tv_sec;	/* seconds */
> 	unsigned int tv_nsec;	/* nanoseconds */
> 	unsigned char length;
> 	unsigned char data[SND_RAWMIDI_FRAMING_DATA_LENGTH];
> };
> 
> We'll then have only three bytes for the actual data, but since that is what most midi messages are anyway, it would be okay, I assume.

We can use the free bits in tv_nsec. It may be possible to carry 4 midi bytes
with the 64-bit tv_sec field, too.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.