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
- the SNDRV_RAWMIDI_MODE_FRAMING_32BYTES indicates that the frame
size is 32 bytes and the first byte of that frame is frame_type
- 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