[PATCH v4] sound: rawmidi: Add framing mode

[David Henningsson]

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);
>> +		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);
> 	}

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