[PATCH v4] sound: rawmidi: Add framing mode
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)
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;@@ -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
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;@@ -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
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;@@ -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
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;@@ -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
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;@@ -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
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
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
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
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
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
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
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
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);elsektime_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);@@ -999,6 +1054,7 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream, runtime->xruns++; } } else {
spin_lock_irqsave(&runtime->lock, flags);
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
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
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
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);elsektime_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);@@ -999,6 +1054,7 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream, runtime->xruns++; } } else {
spin_lock_irqsave(&runtime->lock, flags);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
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
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
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
participants (3)
-
David Henningsson -
Jaroslav Kysela -
Takashi Iwai