[PATCH v4] sound: rawmidi: Add framing mode
David Henningsson
coding at diwic.se
Sun Apr 11 21:16:49 CEST 2021
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 at diwic.se>
>>>> ---
>>>> include/sound/rawmidi.h | 2 ++
>>>> include/uapi/sound/asound.h | 26 ++++++++++++++--
>>>> sound/core/rawmidi.c | 60 +++++++++++++++++++++++++++++++++++--
>>>> 3 files changed, 84 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h
>>>> index 334842daa904..b0057a193c31 100644
>>>> --- a/include/sound/rawmidi.h
>>>> +++ b/include/sound/rawmidi.h
>>>> @@ -81,6 +81,8 @@ struct snd_rawmidi_substream {
>>>> bool opened; /* open flag */
>>>> bool append; /* append flag (merge more streams) */
>>>> bool active_sensing; /* send active sensing when close */
>>>> + u8 framing; /* whether to frame input data */
>>>> + clockid_t clock_type; /* clock source to use for input framing */
>>>> int use_count; /* use counter (for output) */
>>>> size_t bytes;
>>>> struct snd_rawmidi *rmidi;
>>>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>>>> index 535a7229e1d9..af8e60740218 100644
>>>> --- a/include/uapi/sound/asound.h
>>>> +++ b/include/uapi/sound/asound.h
>>>> @@ -710,7 +710,7 @@ enum {
>>>> * Raw MIDI section - /dev/snd/midi??
>>>> */
>>>>
>>>> -#define SNDRV_RAWMIDI_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 1)
>>>> +#define SNDRV_RAWMIDI_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 2)
>>>>
>>>> enum {
>>>> SNDRV_RAWMIDI_STREAM_OUTPUT = 0,
>>>> @@ -736,12 +736,34 @@ struct snd_rawmidi_info {
>>>> unsigned char reserved[64]; /* reserved for future use */
>>>> };
>>>>
>>>> +enum {
>>>> + SNDRV_RAWMIDI_FRAMING_NONE = 0,
>>>> + SNDRV_RAWMIDI_FRAMING_TSTAMP,
>>>> + SNDRV_RAWMIDI_FRAMING_LAST = SNDRV_RAWMIDI_FRAMING_TSTAMP,
>>>> +};
>>>> +
>>>> +#define SND_RAWMIDI_FRAMING_DATA_LENGTH 16
>>>> +
>>>> +struct snd_rawmidi_framing_tstamp {
>>>> + /* For now, frame_type is always 0. Midi 2.0 is expected to add new
>>>> + * types here. Applications are expected to skip unknown frame types.
>>>> + */
>>>> + unsigned char frame_type;
>>>> + unsigned char length; /* number of valid bytes in data field */
>>>> + unsigned char reserved[2];
>>>> + unsigned int tv_nsec; /* nanoseconds */
>>>> + unsigned long long tv_sec; /* seconds */
>>>> + unsigned char data[SND_RAWMIDI_FRAMING_DATA_LENGTH];
>>>> +};
>>>> +
>>>> struct snd_rawmidi_params {
>>>> int stream;
>>>> size_t buffer_size; /* queue size in bytes */
>>>> size_t avail_min; /* minimum avail bytes for wakeup */
>>>> unsigned int no_active_sensing: 1; /* do not send active sensing byte in close() */
>>>> - unsigned char reserved[16]; /* reserved for future use */
>>>> + unsigned char framing; /* For input data only, frame incoming data */
>>> We can move this flag to above 32-bit word (no_active_sensing). I'm not sure,
>>> if we need 8 bits for this. It's first change after 20 years. Another flag may
>>> obsolete this one.
>> Not sure what you mean by this, could you show the code? Framing is an
>> enum rather than a flag, in case we find other framing formats with
>> other sizes that would obsolete this one.
> unsigned int no_active_sensing: 1;
> unsigned int framing32: 1;
> unsigned int framing64: 1; /* future extension, framing32 == 0 in this case */
>
> or:
>
> unsigned int framing_mode: 3; /* three bits for future types */
>
> perhaps also:
>
> unsigned int clock_type: 3; /* three bits for the clock type selection */
>
>>>> + return -EINVAL;
>>>> if (params->avail_min < 1 || params->avail_min > params->buffer_size)
>>>> return -EINVAL;
>>>> if (params->buffer_size != runtime->buffer_size) {
>>>> @@ -720,7 +722,16 @@ EXPORT_SYMBOL(snd_rawmidi_output_params);
>>>> int snd_rawmidi_input_params(struct snd_rawmidi_substream *substream,
>>>> struct snd_rawmidi_params *params)
>>>> {
>>>> + if (params->framing) {
>>>> + if (params->framing > SNDRV_RAWMIDI_FRAMING_LAST)
>>>> + return -EINVAL;
>>>> + /* framing requires a valid clock type */
>>>> + if (params->clock_type != CLOCK_MONOTONIC_RAW && params->clock_type != CLOCK_MONOTONIC)
>>>> + return -EINVAL;
>>> The CLOCK_REALTIME may be supported, too. For example, the input subsystem
>>> supports those three timestamps and we support this in the PCM interface, too.
>> OTOH, the seq subsystem supports only the monotonic clock. And nobody
>> has complained so far. This can be added in a later patch if there is a
>> need for it.
> The monotonic clock source is used only internally for diffs - the seq timer
> starts with zero. So the monotonic clock is _NOT_ exported.
>
> In PCM interface, we have also all three clock sources. We're using own enum
> there, too (SNDRV_PCM_TSTAMP_TYPE_...).
>
>>>> +static int receive_with_tstamp_framing(struct snd_rawmidi_substream *substream,
>>>> + const unsigned char *buffer, int src_count, struct timespec64 *tstamp)
>>>> +{
>>>> + struct snd_rawmidi_runtime *runtime = substream->runtime;
>>>> + struct snd_rawmidi_framing_tstamp *dest_ptr;
>>>> + struct snd_rawmidi_framing_tstamp frame = { .tv_sec = tstamp->tv_sec, .tv_nsec = tstamp->tv_nsec };
>>>> +
>>>> + int dest_frames = 0;
>>>> + int frame_size = sizeof(struct snd_rawmidi_framing_tstamp);
>>>> +
>>>> + if (snd_BUG_ON(runtime->hw_ptr & 0x1f || runtime->buffer_size & 0x1f || frame_size != 0x20))
>>>> + return -EINVAL;
>>>> + while (src_count > 0) {
>>>> + if ((int)(runtime->buffer_size - runtime->avail) < frame_size) {
>>>> + runtime->xruns += src_count;
>>>> + return dest_frames * frame_size;
>>>> + }
>>>> + if (src_count >= SND_RAWMIDI_FRAMING_DATA_LENGTH)
>>>> + frame.length = SND_RAWMIDI_FRAMING_DATA_LENGTH;
>>>> + else {
>>>> + frame.length = src_count;
>>>> + memset(frame.data, 0, SND_RAWMIDI_FRAMING_DATA_LENGTH);
>>> We know the length here, so we can skip the zeroing the copied bytes with
>>> memcpy().
>> True, but I believe this would generate slightly faster code because
>> SND_RAWMIDI_FRAMING_DATA_LENGTH is a constant.
> It's questionable.
>
> Jaroslav
>
More information about the Alsa-devel
mailing list