[PATCH v4] sound: rawmidi: Add framing mode

[PATCH v4] sound: rawmidi: Add framing mode

[David Henningsson]

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>
---
 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 */
+	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)
+		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;
+	}
 	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);
+		}
+		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 +1024,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;
 
@@ -987,8 +1035,15 @@ 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_FRAMING_TSTAMP) {
+		if (substream->clock_type == CLOCK_MONOTONIC_RAW)
+			ktime_get_raw_ts64(&ts64);
+		else
+			ktime_get_ts64(&ts64);
+		spin_lock_irqsave(&runtime->lock, flags);
+		result = receive_with_tstamp_framing(substream, buffer, count, &ts64);
+	} else if (count == 1) {	/* special case, faster code */
+		spin_lock_irqsave(&runtime->lock, flags);
 		substream->bytes++;
 		if (runtime->avail < runtime->buffer_size) {
 			runtime->buffer[runtime->hw_ptr++] = buffer[0];
@@ -999,6 +1054,7 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream,
 			runtime->xruns++;
 		}
 	} else {
+		spin_lock_irqsave(&runtime->lock, flags);
 		substream->bytes += count;
 		count1 = runtime->buffer_size - runtime->hw_ptr;
 		if (count1 > count)
-- 
2.25.1

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

[Jaroslav Kysela]

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

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

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

> +	}
>  	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().

						Jaroslav

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

Re: [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@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

Re: [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@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@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

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

[David Henningsson]

Hi,

as a short reply to all of the review comments below:

I don't care either way. I can change things if it makes you happy. But 
at this point I have a hard time figuring out what are brainstorming 
suggestions, and what are things I need to change before the patch is 
merged.

Could both of you give a clear ack (or similar) so I know that I know 
that once I make a [PATCH v5] it is very likely to be merged? Or are 
there more discussions / reviews / etc to be had first?

// David


On 2021-04-11 19:52, Jaroslav Kysela wrote:
> 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@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
>

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

[Takashi Iwai]

On Sun, 11 Apr 2021 19:52:21 +0200,
Jaroslav Kysela wrote:
> >>>   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 */

The usage of bit fields in an ioctl struct is a bad idea from the
compat layer POV.  Let's avoid it.


thanks,

Takashi

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

[Jaroslav Kysela]

Dne 12. 04. 21 v 11:31 Takashi Iwai napsal(a):
> On Sun, 11 Apr 2021 19:52:21 +0200,
> Jaroslav Kysela wrote:
>>>>>   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 */
> 
> The usage of bit fields in an ioctl struct is a bad idea from the
> compat layer POV.  Let's avoid it.

What exactly do you mean?

					Jaroslav

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

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

[Takashi Iwai]

On Sat, 10 Apr 2021 14:02:29 +0200,
David Henningsson wrote:
> 
> +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 */

Please use u32 and u64 here instead of int and long long.
We really want to be specific about the field types.

> +static int receive_with_tstamp_framing(struct snd_rawmidi_substream *substream,
> +			const unsigned char *buffer, int src_count, struct timespec64 *tstamp)

Pass const to 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))

The frame_size check should be rather BUILD_BUG_ON() as it's constant.
And the buffer_size check is... well, not sure whether we need here.
snd_BUG_ON() is performed even if there is no debug option set (but
the debug message is suppressed).

> +		return -EINVAL;
> +	while (src_count > 0) {
> +		if ((int)(runtime->buffer_size - runtime->avail) < frame_size) {
> +			runtime->xruns += src_count;
> +			return dest_frames * frame_size;

Better to break the loop for unifying the exit path.

> @@ -987,8 +1035,15 @@ 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_FRAMING_TSTAMP) {
> +		if (substream->clock_type == CLOCK_MONOTONIC_RAW)
> +			ktime_get_raw_ts64(&ts64);
> +		else
> +			ktime_get_ts64(&ts64);
> +		spin_lock_irqsave(&runtime->lock, flags);
> +		result = receive_with_tstamp_framing(substream, buffer, count, &ts64);
> +	} else if (count == 1) {	/* special case, faster code */
> +		spin_lock_irqsave(&runtime->lock, flags);
>  		substream->bytes++;
>  		if (runtime->avail < runtime->buffer_size) {
>  			runtime->buffer[runtime->hw_ptr++] = buffer[0];
> @@ -999,6 +1054,7 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream,
>  			runtime->xruns++;
>  		}
>  	} else {
> +		spin_lock_irqsave(&runtime->lock, flags);
>  		substream->bytes += count;
>  		count1 = runtime->buffer_size - runtime->hw_ptr;
>  		if (count1 > count)

It's error-prone to put the spin_lock_irqsave() in various places.
If you want to get the timestamp outside the spinlock inevitably, just
do it before the spin_lock_irqsave() line,

	if (substream->framing == SNDRV_RAWMIDI_FRAMING_TSTAMP) {
		if (substream->clock_type == CLOCK_MONOTONIC_RAW)
			ktime_get_raw_ts64(&ts64);
		else
			ktime_get_ts64(&ts64);
	}

	spin_lock_irqsave(&runtime->lock, flags);
	if (substream->framing == SNDRV_RAWMIDI_FRAMING_TSTAMP) {
		....
	} else if (count == 1) {	/* special case, faster code */
		....

And, the patch misses the change in rawmidi_compat.c.  It's also the
reason we'd like to avoid the bit fields usage, too.  (Not only that,
though.)

One thing I'm considering is how to inform the current framing and the
timestamp mode to user-space.  Currently we have only the ioctl to set
the values but not to inquiry.

Should we put those new information into the info or the status ioctl?
If so, it might be also helpful to inform the frame byte size
explicitly there, instead of assuming only a constant.


thanks,

Takashi

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

[Takashi Iwai]

On Sun, 11 Apr 2021 21:16:49 +0200,
David Henningsson wrote:
> 
> Hi,
> 
> as a short reply to all of the review comments below:
> 
> I don't care either way. I can change things if it makes you
> happy. But at this point I have a hard time figuring out what are
> brainstorming suggestions, and what are things I need to change before
> the patch is merged.

This particular part is non-substantial point, just a micro
optimization bike shed, so we don't need to stick with it at all.

> Could both of you give a clear ack (or similar) so I know that I know
> that once I make a [PATCH v5] it is very likely to be merged? Or are
> there more discussions / reviews / etc to be had first?

Obviously there are a few issues to be cleared beforehand.
The biggest open question was about the frame size, and there seems no
large voice opposing to the current value.  But we want to confirm
that before the merge.

The timing is bad for merge into 5.13, too, unfortunately; it's
already at a very late stage and I'd like to avoid the merge of a late
change that modifies the ABI.  So, it's likely a 5.14 merge material.


thanks,

Takashi


> 
> // David
> 
> 
> On 2021-04-11 19:52, Jaroslav Kysela wrote:
> > 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@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
> >
> 

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

[Jaroslav Kysela]

Dne 12. 04. 21 v 11:54 Takashi Iwai napsal(a):

> And, the patch misses the change in rawmidi_compat.c.  It's also the
> reason we'd like to avoid the bit fields usage, too.  (Not only that,
> though.)

Looking to snd_rawmidi_ioctl_params_compat(), I don't see a difference for a
version with and without bit fields. In both cases, the value should be copied
from one structure to another.

I see other structs in include/uapi which are using bit fields in public ioctl
calls.

					Jaroslav

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

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

[Takashi Iwai]

On Mon, 12 Apr 2021 11:43:02 +0200,
Jaroslav Kysela wrote:
> 
> Dne 12. 04. 21 v 11:31 Takashi Iwai napsal(a):
> > On Sun, 11 Apr 2021 19:52:21 +0200,
> > Jaroslav Kysela wrote:
> >>>>>   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 */
> > 
> > The usage of bit fields in an ioctl struct is a bad idea from the
> > compat layer POV.  Let's avoid it.
> 
> What exactly do you mean?

The compat layer has the hard time to deal with the conversion of bit
fields to a different struct.  And, it's a nightmare for the emulator
like QEMU.  If it has to convert the ioctl struct, it has to consider
about the byte order as well (and there is no strict definition how to
put the bit fields in C language).

That said, a bit field can be useful for the internal object
definition (if there is no racy access), but not for an object for the
communication that may be interpreted among different architectures.


Takashi

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

[Takashi Iwai]

On Mon, 12 Apr 2021 12:03:30 +0200,
Jaroslav Kysela wrote:
> 
> Dne 12. 04. 21 v 11:54 Takashi Iwai napsal(a):
> 
> > And, the patch misses the change in rawmidi_compat.c.  It's also the
> > reason we'd like to avoid the bit fields usage, too.  (Not only that,
> > though.)
> 
> Looking to snd_rawmidi_ioctl_params_compat(), I don't see a difference for a
> version with and without bit fields. In both cases, the value should be copied
> from one structure to another.
> 
> I see other structs in include/uapi which are using bit fields in public ioctl
> calls.

Yes, and those were bad ideas, too.


Takashi

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

[Jaroslav Kysela]

Dne 12. 04. 21 v 12:04 Takashi Iwai napsal(a):
> On Mon, 12 Apr 2021 11:43:02 +0200,
> Jaroslav Kysela wrote:
>>
>> Dne 12. 04. 21 v 11:31 Takashi Iwai napsal(a):
>>> On Sun, 11 Apr 2021 19:52:21 +0200,
>>> Jaroslav Kysela wrote:
>>>>>>>   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 */
>>>
>>> The usage of bit fields in an ioctl struct is a bad idea from the
>>> compat layer POV.  Let's avoid it.
>>
>> What exactly do you mean?
> 
> The compat layer has the hard time to deal with the conversion of bit
> fields to a different struct.  And, it's a nightmare for the emulator
> like QEMU.  If it has to convert the ioctl struct, it has to consider
> about the byte order as well (and there is no strict definition how to
> put the bit fields in C language).

The endianness of the 32-bit word can be changed quite easily - and the
situation is similar to bit flags stored in the 32-bit word. Nobody prevents
QEMU to work with the whole 32-bit word instead bit fields in this case.

The linux compat layer may copy the whole 32-bit word, too.

I think that your argument is not so strong here.

> That said, a bit field can be useful for the internal object
> definition (if there is no racy access), but not for an object for the
> communication that may be interpreted among different architectures.

In this case, we have 31 free bits and this information can be stored there. I
would prefer to keep the reserved bytes for some large fields.

					Jaroslav

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

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

[Takashi Iwai]

On Mon, 12 Apr 2021 12:26:49 +0200,
Jaroslav Kysela wrote:
> 
> Dne 12. 04. 21 v 12:04 Takashi Iwai napsal(a):
> > On Mon, 12 Apr 2021 11:43:02 +0200,
> > Jaroslav Kysela wrote:
> >>
> >> Dne 12. 04. 21 v 11:31 Takashi Iwai napsal(a):
> >>> On Sun, 11 Apr 2021 19:52:21 +0200,
> >>> Jaroslav Kysela wrote:
> >>>>>>>   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 */
> >>>
> >>> The usage of bit fields in an ioctl struct is a bad idea from the
> >>> compat layer POV.  Let's avoid it.
> >>
> >> What exactly do you mean?
> > 
> > The compat layer has the hard time to deal with the conversion of bit
> > fields to a different struct.  And, it's a nightmare for the emulator
> > like QEMU.  If it has to convert the ioctl struct, it has to consider
> > about the byte order as well (and there is no strict definition how to
> > put the bit fields in C language).
> 
> The endianness of the 32-bit word can be changed quite easily - and the
> situation is similar to bit flags stored in the 32-bit word. Nobody prevents
> QEMU to work with the whole 32-bit word instead bit fields in this case.

And how you assure that the bit field is set to which bit position?
There is no guarantee defined in C language about that.

> The linux compat layer may copy the whole 32-bit word, too.
> 
> I think that your argument is not so strong here.

Oh please, this is my experience years ago while discussing with the
QEMU developers in the past.  Using a bit field for an ioctl struct is
a horrible idea, after all.

> > That said, a bit field can be useful for the internal object
> > definition (if there is no racy access), but not for an object for the
> > communication that may be interpreted among different architectures.
> 
> In this case, we have 31 free bits and this information can be stored there. I
> would prefer to keep the reserved bytes for some large fields.

Again, C language doesn't define the position of the bit fields.
That's the primary problem.

If we really have to save the space, it's a nice workaround.  But for
other purposes, there is really little merit and more flip side by
that.


Takashi

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

[Jaroslav Kysela]

Dne 12. 04. 21 v 13:17 Takashi Iwai napsal(a):

>> In this case, we have 31 free bits and this information can be stored there. I
>> would prefer to keep the reserved bytes for some large fields.
> 
> Again, C language doesn't define the position of the bit fields.
> That's the primary problem.

Yes and no. It seems that it's not a big C compiler implementation problem,
because other drivers are using the bitfields in the uapi structures, too.
Yes, it may need some extra care.

> If we really have to save the space, it's a nice workaround.  But for
> other purposes, there is really little merit and more flip side by
> that.

I see your reluctance to talk about extension of the bit field word, but
perhaps, we may be clever and define the flag word for the newly added
parameters now. The space may be used more precisely in the future with this
change.

					Jaroslav

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

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

[Takashi Iwai]

On Mon, 12 Apr 2021 13:44:45 +0200,
Jaroslav Kysela wrote:
> 
> Dne 12. 04. 21 v 13:17 Takashi Iwai napsal(a):
> 
> >> In this case, we have 31 free bits and this information can be stored there. I
> >> would prefer to keep the reserved bytes for some large fields.
> > 
> > Again, C language doesn't define the position of the bit fields.
> > That's the primary problem.
> 
> Yes and no. It seems that it's not a big C compiler implementation problem,
> because other drivers are using the bitfields in the uapi structures, too.
> Yes, it may need some extra care.

Right, and the need for care is mostly for little merit -- that's the
point.  I'm not against about the bit flags at all, but pointing that
the C bit fields are too error-prone for ioctl structs.

> > If we really have to save the space, it's a nice workaround.  But for
> > other purposes, there is really little merit and more flip side by
> > that.
> 
> I see your reluctance to talk about extension of the bit field word, but
> perhaps, we may be clever and define the flag word for the newly added
> parameters now. The space may be used more precisely in the future with this
> change.

Sure, that pattern is common and recommended for bit-wise flags.


thanks,

Takashi

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

[David Henningsson]

On 2021-04-12 11:54, Takashi Iwai wrote:
> On Sat, 10 Apr 2021 14:02:29 +0200,
> David Henningsson wrote:
>> +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 */
> Please use u32 and u64 here instead of int and long long.
> We really want to be specific about the field types.
>
>> +static int receive_with_tstamp_framing(struct snd_rawmidi_substream *substream,
>> +			const unsigned char *buffer, int src_count, struct timespec64 *tstamp)
> Pass const to 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))
> The frame_size check should be rather BUILD_BUG_ON() as it's constant.
> And the buffer_size check is... well, not sure whether we need here.
> snd_BUG_ON() is performed even if there is no debug option set (but
> the debug message is suppressed).
>
>> +		return -EINVAL;
>> +	while (src_count > 0) {
>> +		if ((int)(runtime->buffer_size - runtime->avail) < frame_size) {
>> +			runtime->xruns += src_count;
>> +			return dest_frames * frame_size;
> Better to break the loop for unifying the exit path.
>
>> @@ -987,8 +1035,15 @@ 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_FRAMING_TSTAMP) {
>> +		if (substream->clock_type == CLOCK_MONOTONIC_RAW)
>> +			ktime_get_raw_ts64(&ts64);
>> +		else
>> +			ktime_get_ts64(&ts64);
>> +		spin_lock_irqsave(&runtime->lock, flags);
>> +		result = receive_with_tstamp_framing(substream, buffer, count, &ts64);
>> +	} else if (count == 1) {	/* special case, faster code */
>> +		spin_lock_irqsave(&runtime->lock, flags);
>>   		substream->bytes++;
>>   		if (runtime->avail < runtime->buffer_size) {
>>   			runtime->buffer[runtime->hw_ptr++] = buffer[0];
>> @@ -999,6 +1054,7 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream,
>>   			runtime->xruns++;
>>   		}
>>   	} else {
>> +		spin_lock_irqsave(&runtime->lock, flags);
>>   		substream->bytes += count;
>>   		count1 = runtime->buffer_size - runtime->hw_ptr;
>>   		if (count1 > count)
> It's error-prone to put the spin_lock_irqsave() in various places.
> If you want to get the timestamp outside the spinlock inevitably, just
> do it before the spin_lock_irqsave() line,
>
> 	if (substream->framing == SNDRV_RAWMIDI_FRAMING_TSTAMP) {
> 		if (substream->clock_type == CLOCK_MONOTONIC_RAW)
> 			ktime_get_raw_ts64(&ts64);
> 		else
> 			ktime_get_ts64(&ts64);
> 	}

Thanks for the review.

I'll refactor this part into a separate function. The rest I'll just fix 
the way you suggest.

>
> 	spin_lock_irqsave(&runtime->lock, flags);
> 	if (substream->framing == SNDRV_RAWMIDI_FRAMING_TSTAMP) {
> 		....
> 	} else if (count == 1) {	/* special case, faster code */
> 		....
>
> And, the patch misses the change in rawmidi_compat.c.  It's also the
> reason we'd like to avoid the bit fields usage, too.  (Not only that,
> though.)
>
> One thing I'm considering is how to inform the current framing and the
> timestamp mode to user-space.  Currently we have only the ioctl to set
> the values but not to inquiry.

Yes, this is the same as we do with PCM. There is no ioctl to get 
current hw/sw params either.

> Should we put those new information into the info or the status ioctl?

I would prefer neither, because it is not necessary and creates an 
inconsistency with PCM.

> If so, it might be also helpful to inform the frame byte size
> explicitly there, instead of assuming only a constant.

If userspace has verified the kernel protocol version and successfully 
calledSNDRV_RAWMIDI_IOCTL_PARAMS with the framing byte/bitfield/whatever 
set, then userspace can be sure that the frames are according to the 
snd_rawmidi_framing_tstamp struct. Knowing the frame byte size but not 
knowing the exact format is of no use to userspace anyway, right?

// David

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

[Takashi Iwai]

On Mon, 12 Apr 2021 21:32:37 +0200,
David Henningsson wrote:
> 
> > One thing I'm considering is how to inform the current framing and the
> > timestamp mode to user-space.  Currently we have only the ioctl to set
> > the values but not to inquiry.
> 
> Yes, this is the same as we do with PCM. There is no ioctl to get
> current hw/sw params either.
> 
> > Should we put those new information into the info or the status ioctl?
> 
> I would prefer neither, because it is not necessary and creates an
> inconsistency with PCM.

Well, honestly speaking, ALSA PCM API design is no best one you should
refer to...  IMO, it should have had the symmetric get function, too.
I guess it worked without such ioctl because the current PCM status is
exposed via proc file.  Without a way for inquiry of the current
status, we may have had trouble for debugging.

In that sense, it might make sense to extend the proc entry of the
rawmidi status output, too.

> > If so, it might be also helpful to inform the frame byte size
> > explicitly there, instead of assuming only a constant.
> 
> If userspace has verified the kernel protocol version and successfully
> calledSNDRV_RAWMIDI_IOCTL_PARAMS with the framing
> byte/bitfield/whatever set, then userspace can be sure that the frames
> are according to the snd_rawmidi_framing_tstamp struct. Knowing the
> frame byte size but not knowing the exact format is of no use to
> userspace anyway, right?

Sure, if any, the kernel should inform all stuff, the frame type, the
clock type, and the frame size.  But practically seen, this might be
not often inquired, if we define the frame struct explicitly as a part
of user-space API, too.  Then sizeof() already gives the proper size.
Of course, that depends on how to provide the user-space API.  We may
provide again an opaque API, too.


Takashi

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

[Jaroslav Kysela]

Dne 13. 04. 21 v 17:27 Takashi Iwai napsal(a):
> On Mon, 12 Apr 2021 21:32:37 +0200,
> David Henningsson wrote:
>>
>>> One thing I'm considering is how to inform the current framing and the
>>> timestamp mode to user-space.  Currently we have only the ioctl to set
>>> the values but not to inquiry.
>>
>> Yes, this is the same as we do with PCM. There is no ioctl to get
>> current hw/sw params either.
>>
>>> Should we put those new information into the info or the status ioctl?
>>
>> I would prefer neither, because it is not necessary and creates an
>> inconsistency with PCM.
> 
> Well, honestly speaking, ALSA PCM API design is no best one you should
> refer to...  IMO, it should have had the symmetric get function, too.
> I guess it worked without such ioctl because the current PCM status is
> exposed via proc file.  Without a way for inquiry of the current
> status, we may have had trouble for debugging.
> 
> In that sense, it might make sense to extend the proc entry of the
> rawmidi status output, too.

The parameters can be cached in the user space. The "set" ioctl should return
an error, if the parameters are not accepted and the caller should check the
protocol version, if the newly added extensions are supported. So, this type
of ioctl can be added only when really necessary.

I see the only use when you pass the file descriptor to another process, but
usually, there's another channel to pass the setup, too.

The proc file is much better, because you can get the information without any
special tool and anytime.

					Jaroslav

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

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

[David Henningsson]

On 2021-04-13 17:27, Takashi Iwai wrote:
> On Mon, 12 Apr 2021 21:32:37 +0200,
> David Henningsson wrote:
>>> One thing I'm considering is how to inform the current framing and the
>>> timestamp mode to user-space.  Currently we have only the ioctl to set
>>> the values but not to inquiry.
>> Yes, this is the same as we do with PCM. There is no ioctl to get
>> current hw/sw params either.
>>
>>> Should we put those new information into the info or the status ioctl?
>> I would prefer neither, because it is not necessary and creates an
>> inconsistency with PCM.
> Well, honestly speaking, ALSA PCM API design is no best one you should
> refer to...  IMO, it should have had the symmetric get function, too.
> I guess it worked without such ioctl because the current PCM status is
> exposed via proc file.  Without a way for inquiry of the current
> status, we may have had trouble for debugging.

Whether or not the ALSA pcm/rawmidi apis should have get functions to 
get its current parameters, seems like a separate discussion, and 
separate patch. It can be done later if there is such a need.

>
> In that sense, it might make sense to extend the proc entry of the
> rawmidi status output, too.
Okay, I can add rows about framing and clock type in the proc file for v5.
>>> If so, it might be also helpful to inform the frame byte size
>>> explicitly there, instead of assuming only a constant.
>> If userspace has verified the kernel protocol version and successfully
>> calledSNDRV_RAWMIDI_IOCTL_PARAMS with the framing
>> byte/bitfield/whatever set, then userspace can be sure that the frames
>> are according to the snd_rawmidi_framing_tstamp struct. Knowing the
>> frame byte size but not knowing the exact format is of no use to
>> userspace anyway, right?
> Sure, if any, the kernel should inform all stuff, the frame type, the
> clock type, and the frame size.  But practically seen, this might be
> not often inquired, if we define the frame struct explicitly as a part
> of user-space API, too.  Then sizeof() already gives the proper size.
> Of course, that depends on how to provide the user-space API.  We may
> provide again an opaque API, too.

The frame struct will be part of the userspace and alsa-lib APIs. I 
intend to follow up with patches to alsa-lib after this patch gets merged.

// David