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@xxxxxxxx>
>>> ---
>>> 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@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
