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