diff mbox series

[v6] sound: rawmidi: Add framing mode

Message ID 20210419164023.159967-1-coding@diwic.se (mailing list archive)
State Superseded
Headers show
Series [v6] sound: rawmidi: Add framing mode | expand

Commit Message

David Henningsson April 19, 2021, 4:40 p.m. UTC
This commit adds a new framing mode that frames all MIDI data into
32-byte frames with a timestamp.

The main benefit is that we can get accurate timestamps even if
userspace wakeup and processing is not immediate.

Testing on a Celeron N3150 with this mode has a max jitter of 2.8 ms,
compared to the in-kernel seq implementation which has a max jitter
of 5 ms during idle and much worse when running scheduler stress tests
in parallel.

Signed-off-by: David Henningsson <coding@diwic.se>
---

Changes since v5: Added realtime clock and changed params struct according to
Jaroslav's wishes.

This version of the patch has been compile tested only.

 include/sound/rawmidi.h     |  2 +
 include/uapi/sound/asound.h | 30 ++++++++++++-
 sound/core/rawmidi.c        | 86 ++++++++++++++++++++++++++++++++++++-
 sound/core/rawmidi_compat.c |  4 +-
 4 files changed, 118 insertions(+), 4 deletions(-)

Comments

Jaroslav Kysela April 19, 2021, 6 p.m. UTC | #1
Dne 19. 04. 21 v 18:40 David Henningsson napsal(a):
> This commit adds a new framing mode that frames all MIDI data into
> 32-byte frames with a timestamp.
> 
> The main benefit is that we can get accurate timestamps even if
> userspace wakeup and processing is not immediate.
> 
> Testing on a Celeron N3150 with this mode has a max jitter of 2.8 ms,
> compared to the in-kernel seq implementation which has a max jitter
> of 5 ms during idle and much worse when running scheduler stress tests
> in parallel.
> 
> Signed-off-by: David Henningsson <coding@diwic.se>

Nice. Thank you.

Reviewed-by: Jaroslav Kysela <perex@perex.cz>


> @@ -1534,6 +1607,7 @@ static __poll_t snd_rawmidi_poll(struct file *file, poll_table *wait)
>  /*
>   */
>  
> +
>  static void snd_rawmidi_proc_info_read(struct snd_info_entry *entry,
>  				       struct snd_info_buffer *buffer)
>  {

This is probably a typo chunk?

						Jaroslav
Takashi Iwai April 20, 2021, 9:07 a.m. UTC | #2
On Mon, 19 Apr 2021 18:40:23 +0200,
David Henningsson wrote:
> 
> This commit adds a new framing mode that frames all MIDI data into
> 32-byte frames with a timestamp.
> 
> The main benefit is that we can get accurate timestamps even if
> userspace wakeup and processing is not immediate.
> 
> Testing on a Celeron N3150 with this mode has a max jitter of 2.8 ms,
> compared to the in-kernel seq implementation which has a max jitter
> of 5 ms during idle and much worse when running scheduler stress tests
> in parallel.
> 
> Signed-off-by: David Henningsson <coding@diwic.se>
> ---
> 
> Changes since v5: Added realtime clock and changed params struct according to
> Jaroslav's wishes.
> 
> This version of the patch has been compile tested only.

Thanks for the updated patch.  It looks almost good, just some minor
issues below:

> +struct snd_rawmidi_framing_tstamp {
> +	/* For now, frame_type is always 0. Midi 2.0 is expected to add new
> +	 * types here. Applications are expected to skip unknown frame types.
> +	 */
> +	u8 frame_type;
> +	u8 length; /* number of valid bytes in data field */
> +	u8 reserved[2];
> +	u32 tv_nsec;		/* nanoseconds */
> +	u64 tv_sec;		/* seconds */
> +	u8 data[SNDRV_RAWMIDI_FRAMING_DATA_LENGTH];
> +} __attribute__((packed));

Use __packed instead.


> @@ -720,7 +723,18 @@ EXPORT_SYMBOL(snd_rawmidi_output_params);
>  int snd_rawmidi_input_params(struct snd_rawmidi_substream *substream,
>  			     struct snd_rawmidi_params *params)
>  {
> +	unsigned int framing = params->mode & SNDRV_RAWMIDI_MODE_FRAMING_MASK;
> +	unsigned int clock_type = params->mode & SNDRV_RAWMIDI_MODE_CLOCK_MASK;
> +
> +	if (framing == SNDRV_RAWMIDI_MODE_FRAMING_NONE && clock_type != SNDRV_RAWMIDI_MODE_CLOCK_NONE)
> +		return -EINVAL;
> +	else if (clock_type > SNDRV_RAWMIDI_MODE_CLOCK_MONOTONIC_RAW)
> +		return -EINVAL;
> +	if (framing > SNDRV_RAWMIDI_MODE_FRAMING_TSTAMP)
> +		return -EINVAL;
>  	snd_rawmidi_drain_input(substream);
> +	substream->framing = framing;
> +	substream->clock_type = clock_type;
>  	return resize_runtime_buffer(substream->runtime, params, true);

The stale framing and clock_type are set in the case of errors, which
will confuse the later operations.


> +static int receive_with_tstamp_framing(struct snd_rawmidi_substream *substream,
> +			const unsigned char *buffer, int src_count, const struct timespec64 *tstamp)
> +{
....
> +	return dest_frames * frame_size;

The snd_rawmidi_receive() function is supposed to return the processed
read size, hence this would become incompatible.  Though, I don't find
any code that actually checks the return value from
snd_rawmidi_receive(), but a definition is a definition...


thanks,

Takashi
diff mbox series

Patch

diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h
index 334842daa904..989e1517332d 100644
--- a/include/sound/rawmidi.h
+++ b/include/sound/rawmidi.h
@@ -81,6 +81,8 @@  struct snd_rawmidi_substream {
 	bool opened;			/* open flag */
 	bool append;			/* append flag (merge more streams) */
 	bool active_sensing;		/* send active sensing when close */
+	unsigned int framing;		/* whether to frame input data */
+	unsigned int clock_type;	/* clock source to use for input framing */
 	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..773a00c0a1d8 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -710,7 +710,7 @@  enum {
  *  Raw MIDI section - /dev/snd/midi??
  */
 
-#define SNDRV_RAWMIDI_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 1)
+#define SNDRV_RAWMIDI_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 2)
 
 enum {
 	SNDRV_RAWMIDI_STREAM_OUTPUT = 0,
@@ -736,12 +736,38 @@  struct snd_rawmidi_info {
 	unsigned char reserved[64];	/* reserved for future use */
 };
 
+#define SNDRV_RAWMIDI_MODE_FRAMING_MASK		(7<<0)
+#define SNDRV_RAWMIDI_MODE_FRAMING_SHIFT	0
+#define SNDRV_RAWMIDI_MODE_FRAMING_NONE		(0<<0)
+#define SNDRV_RAWMIDI_MODE_FRAMING_TSTAMP	(1<<0)
+#define SNDRV_RAWMIDI_MODE_CLOCK_MASK		(7<<3)
+#define SNDRV_RAWMIDI_MODE_CLOCK_SHIFT		3
+#define SNDRV_RAWMIDI_MODE_CLOCK_NONE		(0<<3)
+#define SNDRV_RAWMIDI_MODE_CLOCK_REALTIME	(1<<3)
+#define SNDRV_RAWMIDI_MODE_CLOCK_MONOTONIC	(2<<3)
+#define SNDRV_RAWMIDI_MODE_CLOCK_MONOTONIC_RAW	(3<<3)
+
+#define SNDRV_RAWMIDI_FRAMING_DATA_LENGTH 16
+
+struct snd_rawmidi_framing_tstamp {
+	/* For now, frame_type is always 0. Midi 2.0 is expected to add new
+	 * types here. Applications are expected to skip unknown frame types.
+	 */
+	u8 frame_type;
+	u8 length; /* number of valid bytes in data field */
+	u8 reserved[2];
+	u32 tv_nsec;		/* nanoseconds */
+	u64 tv_sec;		/* seconds */
+	u8 data[SNDRV_RAWMIDI_FRAMING_DATA_LENGTH];
+} __attribute__((packed));
+
 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 int mode;		/* For input data only, frame incoming data */
+	unsigned char reserved[12];	/* reserved for future use */
 };
 
 #ifndef __KERNEL__
diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index aca00af93afe..5d5f4363e887 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -680,9 +680,12 @@  static int resize_runtime_buffer(struct snd_rawmidi_runtime *runtime,
 				 bool is_input)
 {
 	char *newbuf, *oldbuf;
+	unsigned int framing = params->mode & SNDRV_RAWMIDI_MODE_FRAMING_MASK;
 
 	if (params->buffer_size < 32 || params->buffer_size > 1024L * 1024L)
 		return -EINVAL;
+	if (framing == SNDRV_RAWMIDI_MODE_FRAMING_TSTAMP && (params->buffer_size & 0x1f) != 0)
+		return -EINVAL;
 	if (params->avail_min < 1 || params->avail_min > params->buffer_size)
 		return -EINVAL;
 	if (params->buffer_size != runtime->buffer_size) {
@@ -720,7 +723,18 @@  EXPORT_SYMBOL(snd_rawmidi_output_params);
 int snd_rawmidi_input_params(struct snd_rawmidi_substream *substream,
 			     struct snd_rawmidi_params *params)
 {
+	unsigned int framing = params->mode & SNDRV_RAWMIDI_MODE_FRAMING_MASK;
+	unsigned int clock_type = params->mode & SNDRV_RAWMIDI_MODE_CLOCK_MASK;
+
+	if (framing == SNDRV_RAWMIDI_MODE_FRAMING_NONE && clock_type != SNDRV_RAWMIDI_MODE_CLOCK_NONE)
+		return -EINVAL;
+	else if (clock_type > SNDRV_RAWMIDI_MODE_CLOCK_MONOTONIC_RAW)
+		return -EINVAL;
+	if (framing > SNDRV_RAWMIDI_MODE_FRAMING_TSTAMP)
+		return -EINVAL;
 	snd_rawmidi_drain_input(substream);
+	substream->framing = framing;
+	substream->clock_type = clock_type;
 	return resize_runtime_buffer(substream->runtime, params, true);
 }
 EXPORT_SYMBOL(snd_rawmidi_input_params);
@@ -963,6 +977,61 @@  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, const struct timespec64 *tstamp)
+{
+	struct snd_rawmidi_runtime *runtime = substream->runtime;
+	struct snd_rawmidi_framing_tstamp *dest_ptr;
+	struct snd_rawmidi_framing_tstamp frame = { .tv_sec = tstamp->tv_sec, .tv_nsec = tstamp->tv_nsec };
+	int dest_frames = 0;
+	int frame_size = sizeof(struct snd_rawmidi_framing_tstamp);
+
+	BUILD_BUG_ON(frame_size != 0x20);
+	if (snd_BUG_ON((runtime->hw_ptr & 0x1f) != 0))
+		return -EINVAL;
+
+	while (src_count > 0) {
+		if ((int)(runtime->buffer_size - runtime->avail) < frame_size) {
+			runtime->xruns += src_count;
+			break;
+		}
+		if (src_count >= SNDRV_RAWMIDI_FRAMING_DATA_LENGTH)
+			frame.length = SNDRV_RAWMIDI_FRAMING_DATA_LENGTH;
+		else {
+			frame.length = src_count;
+			memset(frame.data, 0, SNDRV_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;
+}
+
+static struct timespec64 get_framing_tstamp(struct snd_rawmidi_substream *substream)
+{
+	struct timespec64 ts64 = {0, 0};
+
+	switch (substream->clock_type) {
+	case SNDRV_RAWMIDI_MODE_CLOCK_MONOTONIC_RAW:
+		ktime_get_raw_ts64(&ts64);
+		break;
+	case SNDRV_RAWMIDI_MODE_CLOCK_MONOTONIC:
+		ktime_get_ts64(&ts64);
+		break;
+	case SNDRV_RAWMIDI_MODE_CLOCK_REALTIME:
+		ktime_get_real_ts64(&ts64);
+		break;
+	}
+	return ts64;
+}
+
 /**
  * snd_rawmidi_receive - receive the input data from the device
  * @substream: the rawmidi substream
@@ -977,6 +1046,7 @@  int snd_rawmidi_receive(struct snd_rawmidi_substream *substream,
 			const unsigned char *buffer, int count)
 {
 	unsigned long flags;
+	struct timespec64 ts64 = get_framing_tstamp(substream);
 	int result = 0, count1;
 	struct snd_rawmidi_runtime *runtime = substream->runtime;
 
@@ -987,8 +1057,11 @@  int snd_rawmidi_receive(struct snd_rawmidi_substream *substream,
 			  "snd_rawmidi_receive: input is not active!!!\n");
 		return -EINVAL;
 	}
+
 	spin_lock_irqsave(&runtime->lock, flags);
-	if (count == 1) {	/* special case, faster code */
+	if (substream->framing == SNDRV_RAWMIDI_MODE_FRAMING_TSTAMP) {
+		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];
@@ -1534,6 +1607,7 @@  static __poll_t snd_rawmidi_poll(struct file *file, poll_table *wait)
 /*
  */
 
+
 static void snd_rawmidi_proc_info_read(struct snd_info_entry *entry,
 				       struct snd_info_buffer *buffer)
 {
@@ -1541,6 +1615,8 @@  static void snd_rawmidi_proc_info_read(struct snd_info_entry *entry,
 	struct snd_rawmidi_substream *substream;
 	struct snd_rawmidi_runtime *runtime;
 	unsigned long buffer_size, avail, xruns;
+	unsigned int clock_type;
+	static const char *clock_names[4] = { "none", "realtime", "monotonic", "monotonic raw" };
 
 	rmidi = entry->private_data;
 	snd_iprintf(buffer, "%s\n\n", rmidi->name);
@@ -1596,6 +1672,14 @@  static void snd_rawmidi_proc_info_read(struct snd_info_entry *entry,
 					    "  Avail        : %lu\n"
 					    "  Overruns     : %lu\n",
 					    buffer_size, avail, xruns);
+				if (substream->framing == SNDRV_RAWMIDI_MODE_FRAMING_TSTAMP) {
+					clock_type = substream->clock_type >> SNDRV_RAWMIDI_MODE_CLOCK_SHIFT;
+					if (!snd_BUG_ON(clock_type >= sizeof(clock_names)))
+						snd_iprintf(buffer,
+							    "  Framing      : tstamp\n"
+							    "  Clock type   : %s\n",
+							    clock_names[clock_type]);
+				}
 			}
 		}
 	}
diff --git a/sound/core/rawmidi_compat.c b/sound/core/rawmidi_compat.c
index 7397130976d0..68a93443583c 100644
--- a/sound/core/rawmidi_compat.c
+++ b/sound/core/rawmidi_compat.c
@@ -13,7 +13,8 @@  struct snd_rawmidi_params32 {
 	u32 buffer_size;
 	u32 avail_min;
 	unsigned int no_active_sensing; /* avoid bit-field */
-	unsigned char reserved[16];
+	unsigned int mode;
+	unsigned char reserved[12];
 } __attribute__((packed));
 
 static int snd_rawmidi_ioctl_params_compat(struct snd_rawmidi_file *rfile,
@@ -25,6 +26,7 @@  static int snd_rawmidi_ioctl_params_compat(struct snd_rawmidi_file *rfile,
 	if (get_user(params.stream, &src->stream) ||
 	    get_user(params.buffer_size, &src->buffer_size) ||
 	    get_user(params.avail_min, &src->avail_min) ||
+	    get_user(params.mode, &src->mode) ||
 	    get_user(val, &src->no_active_sensing))
 		return -EFAULT;
 	params.no_active_sensing = val;