[PATCH v5] sound: rawmidi: Add framing mode

[Jaroslav Kysela]

Dne 19. 04. 21 v 9:34 Takashi Iwai napsal(a):
> On Mon, 19 Apr 2021 08:22:50 +0200,
> David Henningsson wrote:
>>
>>
>> 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.
> 
> I thought of the use of union, but concluded that it doesn't give any
> good benefit.  Practically seen, defining two structs would be far
> easier, and if you want to code something in user-space for another
> new frame format, you would just use the new struct as is, as long as
> the size fits correctly.

Ok.

>>> 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?
> 
> Oh, don't get me wrong, I have no objection to the bit flags at all.
> My objection was against the use of C language bit-fields
> (e.g. unsigned int foo:5) as Jaroslav suggested in his earlier reply.
> That's not portable, hence unsuitable for ioctl structs.  OTOH, the
> explicit bit flags are very common in ABI definitions.
> 
> And I have no strong opinion on the flag definitions.  I find it OK to
> keep two uchar fields (that are mostly enough for near future
> extensions), but having a 32bit full bit flag would be of course OK
> from ABI design POV (and one less code in the compat function).
> 
> That said, there is no disagreement about the flag definition at all.
> You can pick up what you believe the best.

I have two reasons to pick the flags rather uchar fields (a bit non-standard
in the ALSA ioctl API):

1) save bits for future use
2) align to 4 bytes - with 2 uchar, we have 2 bytes pad

So, instead to split the 4 bytes from the reserved area, allocate the whole
32-bit word and allocate bits from it.

					Jaroslav

-- 
Jaroslav Kysela <perex at perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.