[PATCH v4] sound: rawmidi: Add framing mode

[Jaroslav Kysela]

Dne 11. 04. 21 v 6:34 David Henningsson napsal(a):
> 
> On 2021-04-10 14:23, Jaroslav Kysela wrote:
>> Dne 10. 04. 21 v 14:02 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 at diwic.se>
>>> ---
>>>   include/sound/rawmidi.h     |  2 ++
>>>   include/uapi/sound/asound.h | 26 ++++++++++++++--
>>>   sound/core/rawmidi.c        | 60 +++++++++++++++++++++++++++++++++++--
>>>   3 files changed, 84 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h
>>> index 334842daa904..b0057a193c31 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 */
>>> +	u8 framing;			/* whether to frame input data */
>>> +	clockid_t 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..af8e60740218 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,34 @@ struct snd_rawmidi_info {
>>>   	unsigned char reserved[64];	/* reserved for future use */
>>>   };
>>>   
>>> +enum {
>>> +	SNDRV_RAWMIDI_FRAMING_NONE = 0,
>>> +	SNDRV_RAWMIDI_FRAMING_TSTAMP,
>>> +	SNDRV_RAWMIDI_FRAMING_LAST = SNDRV_RAWMIDI_FRAMING_TSTAMP,
>>> +};
>>> +
>>> +#define SND_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.
>>> +	 */
>>> +	unsigned char frame_type;
>>> +	unsigned char length; /* number of valid bytes in data field */
>>> +	unsigned char reserved[2];
>>> +	unsigned int tv_nsec;		/* nanoseconds */
>>> +	unsigned long long tv_sec;	/* seconds */
>>> +	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 */
>> We can move this flag to above 32-bit word (no_active_sensing). I'm not sure,
>> if we need 8 bits for this. It's first change after 20 years. Another flag may
>> obsolete this one.
> 
> Not sure what you mean by this, could you show the code? Framing is an 
> enum rather than a flag, in case we find other framing formats with 
> other sizes that would obsolete this one.

unsigned int no_active_sensing: 1;
unsigned int framing32: 1;
unsigned int framing64: 1; /* future extension, framing32 == 0 in this case */

or:

unsigned int framing_mode: 3;	/* three bits for future types */

perhaps also:

unsigned int clock_type: 3;	/* three bits for the clock type selection */

>>> +		return -EINVAL;
>>>   	if (params->avail_min < 1 || params->avail_min > params->buffer_size)
>>>   		return -EINVAL;
>>>   	if (params->buffer_size != runtime->buffer_size) {
>>> @@ -720,7 +722,16 @@ EXPORT_SYMBOL(snd_rawmidi_output_params);
>>>   int snd_rawmidi_input_params(struct snd_rawmidi_substream *substream,
>>>   			     struct snd_rawmidi_params *params)
>>>   {
>>> +	if (params->framing) {
>>> +		if (params->framing > SNDRV_RAWMIDI_FRAMING_LAST)
>>> +			return -EINVAL;
>>> +		/* framing requires a valid clock type */
>>> +		if (params->clock_type != CLOCK_MONOTONIC_RAW && params->clock_type != CLOCK_MONOTONIC)
>>> +			return -EINVAL;
>> The CLOCK_REALTIME may be supported, too. For example, the input subsystem
>> supports those three timestamps and we support this in the PCM interface, too.
> OTOH, the seq subsystem supports only the monotonic clock. And nobody 
> has complained so far. This can be added in a later patch if there is a 
> need for it.

The monotonic clock source is used only internally for diffs - the seq timer
starts with zero. So the monotonic clock is _NOT_ exported.

In PCM interface, we have also all three clock sources. We're using own enum
there, too (SNDRV_PCM_TSTAMP_TYPE_...).

>>> +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 *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);
>>> +
>>> +	if (snd_BUG_ON(runtime->hw_ptr & 0x1f || runtime->buffer_size & 0x1f || frame_size != 0x20))
>>> +		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);
>> We know the length here, so we can skip the zeroing the copied bytes with
>> memcpy().
> 
> True, but I believe this would generate slightly faster code because 
> SND_RAWMIDI_FRAMING_DATA_LENGTH is a constant.

It's questionable.

					Jaroslav

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