[PATCH v5] sound: rawmidi: Add framing mode

[David Henningsson]

On 2021-04-18 20:24, Jaroslav Kysela wrote:
> Dne 18. 04. 21 v 17:12 David Henningsson napsal(a):
>
>> +#define SND_RAWMIDI_FRAMING_DATA_LENGTH 16
> SNDRV_ prefix should be here.

Ack

>
>> +
>> +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.
>> +	 */
>> +	u8 frame_type;
>> +	u8 length; /* number of valid bytes in data field */
>> +	u8 reserved[2];
>> +	u32 tv_nsec;		/* nanoseconds */
>> +	u64 tv_sec;		/* seconds */
>> +	u8 data[SND_RAWMIDI_FRAMING_DATA_LENGTH];
> What about to move the fields to union (except for frame_type) like we do for
> 'struct snd_ctl_event' in case when we need to reorganize the contents for
> future types?

So the two degrees of freedom would be

1) the SNDRV_RAWMIDI_MODE_FRAMING_32BYTES indicates that the frame size 
is 32 bytes and the first byte of that frame is frame_type

2) the frame_type of every frame indicates the format of the other 31 
bytes, and an application is expected to ignore unknown frame_types, so 
we can add new frame_types in a backwards compatible way.

We'll end up with:

struct snd_rawmidi_framing_32bytes {
     u8 frame_type;
     union {
         struct {
             u8 length; /* number of valid bytes in data field */
             u8 reserved[2];
             u32 tv_nsec;        /* nanoseconds */
             u64 tv_sec;        /* seconds */
             u8 data[SNDRV_RAWMIDI_FRAMING_32BYTES_FOO_LENGTH];
         } foo;
         u8 reserved[31];
     } data;
};

...except I don't know what we should replace foo with. We can't call it 
"midi1" or "type0" or such because many different frame_types might 
share the same interior format.


>
>> +};
>> +
>>   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 */
>> +	unsigned char clock_type;	/* Type of clock to use for framing, same as clockid_t */
>> +	unsigned char reserved[14];	/* reserved for future use */
> As I noted, I would prefer to add 'unsigned int mode;' and define
> SNDRV_RAWMID_MODE_XXX bit flags and groups with framing and clock_type groups.
> There's no reason to stick with 'clockid_t' (which is integer anyway). We're
> using just a subset.
>
> #define SNDRV_RAWMIDI_MODE_FRAMING_MASK        (7<<0)
> #define SNDRV_RAWMIDI_MODE_FRAMING_SHIFT       0
> #define SNDRV_RAWMIDI_MODE_FRAMING_NONE	       (0<<0)
> #define SNDRV_RAWMIDI_MODE_FRAMING_32BYTES     (1<<0)
> #define SNDRV_RAWMIDI_MODE_CLOCK_MASK          (7<<3)
> #define SNDRV_RAWMIDI_MODE_CLOCK_SHIFT         3
> #define SNDRV_RAWMIDI_MODE_CLOCK_NONE	       (0<<3)
> #define SNDRV_RAWMIDI_MODE_CLOCK_REALTIME      (1<<3)
> #define SNDRV_RAWMIDI_MODE_CLOCK_MONOTONIC     (2<<3)
> #define SNDRV_RAWMIDI_MODE_CLOCK_MONOTONIC_RAW (3<<3)
>
> In this case, we can use 26-bits in future for extensions.

Well, for me this is mostly bikeshedding. But as long as you and Takashi 
can't agree on whether bitfields/bimasks/etc are good or bad, I'm just 
stuck between the two of you and can't actually improve Linux's 
capability to be a great pro audio OS, and that is utterly frustrating. 
I don't care if the two of you decides who's going to win this through 
this list, a conference call, a game of SuperTuxKart or thumb wrestling, 
just reach consensus somehow. Okay?

>
>> +struct timespec64 get_framing_tstamp(struct snd_rawmidi_substream *substream)
>> +{
>> +	struct timespec64 ts64 = {0, 0};
>> +
>> +	if (substream->framing != SNDRV_RAWMIDI_FRAMING_TSTAMP)
>> +		return ts64;
>> +	if (substream->clock_type == CLOCK_MONOTONIC_RAW)
>> +		ktime_get_raw_ts64(&ts64);
>> +	else
>> +		ktime_get_ts64(&ts64);
>> +	return ts64;
>> +}
> Missing the realtime clock type here.
>
> 					Jaroslav
>