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
