[PATCH v4] sound: rawmidi: Add framing mode

[David Henningsson]

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 char clock_type;	/* Type of clock to use for framing, same as clockid_t */
>> +	unsigned char reserved[14];	/* reserved for future use */
>>   };
>>   
>>   #ifndef __KERNEL__
>> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
>> index aca00af93afe..d4b6b9b5c0e4 100644
>> --- a/sound/core/rawmidi.c
>> +++ b/sound/core/rawmidi.c
>> @@ -683,6 +683,8 @@ static int resize_runtime_buffer(struct snd_rawmidi_runtime *runtime,
>>   
>>   	if (params->buffer_size < 32 || params->buffer_size > 1024L * 1024L)
>>   		return -EINVAL;
>> +	if (params->framing == SNDRV_RAWMIDI_FRAMING_TSTAMP && params->buffer_size & 0x1f)
> I would use '(a & b) != 0' here. It's more readable.

Ok; if v4 is not merged I'll change this for v5.


>
>> +		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.
>
>> +	}
>>   	snd_rawmidi_drain_input(substream);
>> +	substream->framing = params->framing;
>> +	substream->clock_type = params->clock_type;
>>   	return resize_runtime_buffer(substream->runtime, params, true);
>>   }
>>   EXPORT_SYMBOL(snd_rawmidi_input_params);
>> @@ -963,6 +974,42 @@ 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 *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.

// David