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