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