[PATCH v5] sound: rawmidi: Add framing mode

[Takashi Iwai]

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.

If we think of the future-proof compatibility, you may align the
struct to a TLV pattern; the struct already shows a kind of that. But
it would work only if the kernel read side allows the partial read,
though.  So maybe it's not the case.

> >> +};
> >> +
> >>   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?

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.


BTW, regarding the portability: it'd be safer to put __packed
attribute to the newly defined frame struct.  Although it's certainly
fit in the designed size with gcc, essentially a compiler is still
free to put any padding.  Unlike the ioctl struct, the difference of
the access size couldn't be caught easily for read syscall (as we
often allow partial reads).


thanks,

Takashi