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

[David Henningsson <coding@xxxxxxxx>]

On 2021-03-24 11:03, Takashi Iwai wrote:
> On Wed, 24 Mar 2021 06:42:53 +0100,
> David Henningsson wrote:
> > 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.
> >
> > Signed-off-by: David Henningsson <coding@xxxxxxxx>
> Thanks for the patch!
Thanks for the review :-)
> I seem to have overlooked your previous post, sorry.
>
> This looks like a good middle ground solution, while we still need to
> address the sequencer timestamp (basically we should be able to send
> an event with the timestamp prepared from the rawmidi side).

I believe the new framing mode would be useful both for readers of 
rawmidi devices, and the seq kernel module.

I have also been thinking of doing something in usb-midi (because I 
assume that's the most common way to do midi input these days), to 
improve performance for packets with more than three bytes in them. 
Right now a sysex would be cut off in chunks of three bytes, each one 
with its own timestamp. If so, that would be a later patch.

> The implementation itself looks good to me.  But this needs to bump
> the SNDRV_RAWMIDI_VERSION for indicating the new API.

Sure, I'll send a v3 with a bumped SNDRV_RAWMIDI_VERSION.

I'm also considering adding "time when the stream started" in the 
snd_rawmidi_status timestamp to get a fixed starting point for the 
timestamps, unless the field was reserved for some other purpose? The 
status timestamp would only be added if the framing mode is enabled. If 
so, that change would go into the same version bump. Does that sound 
good to you?

// David

> Takashi
>
> > ---
> >
> > v2: Fixed checkpatch errors.
> >
> >   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..4ba5d2deec18 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..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,
> > +};
> > +
> > +#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..cd927ba178a6 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 (src_count >= SND_RAWMIDI_FRAMING_DATA_LENGTH)
> > +			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
> > @@ -977,6 +1016,7 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream,
> >   			const unsigned char *buffer, int count)
> >   {
> >   	unsigned long flags;
> > +	struct timespec64 ts64;
> >   	int result = 0, count1;
> >   	struct snd_rawmidi_runtime *runtime = substream->runtime;
> >   
> > @@ -988,7 +1028,10 @@ 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) {
> > +		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