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
