sound: rawmidi: Add framing mode

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>
---

Hi, it's been a while. Both because I've been busy with things that needed
to take priority, and because I wanted to do a little more than just compile
testing it before submitting; hopefully this one is mergable.

That testing showed that I needed to change from "u8" to "__u8" in the headers.
Other than that I think it is just the changes that Takashi Iwai pointed out in v6.

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

On Sat, 15 May 2021 09:15:33 +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>
> ---
> 
> Hi, it's been a while. Both because I've been busy with things that needed
> to take priority, and because I wanted to do a little more than just compile
> testing it before submitting; hopefully this one is mergable.
> 
> That testing showed that I needed to change from "u8" to "__u8" in the headers.
> Other than that I think it is just the changes that Takashi Iwai pointed out in v6.

Thanks, it looks good to me.

If anyone has no objection, I'm going to merge this soon later.


thanks,

Takashi

> 
>  include/sound/rawmidi.h     |  2 +
>  include/uapi/sound/asound.h | 30 +++++++++++-
>  sound/core/rawmidi.c        | 93 ++++++++++++++++++++++++++++++++++++-
>  sound/core/rawmidi_compat.c |  4 +-
>  4 files changed, 124 insertions(+), 5 deletions(-)
> 
> 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..d17c061950df 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];
> +} __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..4a6534db77d6 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,8 +723,24 @@ 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;
> +	int err;
> +
> +	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);
> -	return resize_runtime_buffer(substream->runtime, params, true);
> +	err = resize_runtime_buffer(substream->runtime, params, true);
> +	if (err < 0)
> +		return err;
> +
> +	substream->framing = framing;
> +	substream->clock_type = clock_type;
> +	return 0;
>  }
>  EXPORT_SYMBOL(snd_rawmidi_input_params);
>  
> @@ -963,6 +982,62 @@ 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 orig_count = src_count;
> +	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 orig_count - src_count;
> +}
> +
> +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 +1052,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 +1063,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];
> @@ -1541,6 +1620,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 +1677,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;
> -- 
> 2.25.1
>

Dne 15. 05. 21 v 9:15 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>

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

On Mon, 17 May 2021 11:04:28 +0200,
Takashi Iwai wrote:
> 
> On Sat, 15 May 2021 09:15:33 +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>
> > ---
> > 
> > Hi, it's been a while. Both because I've been busy with things that needed
> > to take priority, and because I wanted to do a little more than just compile
> > testing it before submitting; hopefully this one is mergable.
> > 
> > That testing showed that I needed to change from "u8" to "__u8" in the headers.
> > Other than that I think it is just the changes that Takashi Iwai pointed out in v6.
> 
> Thanks, it looks good to me.
> 
> If anyone has no objection, I'm going to merge this soon later.

Now applied.

Could you submit the corresponding patches for alsa-lib, too?


Thanks!

Takashi