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@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
