| Message ID | 20210418151217.208582-1-coding@diwic.se (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5] sound: rawmidi: Add framing mode | expand |
Hi David,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on sound/for-next]
[also build test WARNING on v5.12-rc7 next-20210416]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/David-Henningsson/sound-rawmidi-Add-framing-mode/20210418-231512
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: x86_64-randconfig-a001-20210418 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 1c10201d9660c1d6f43a7226ca7381bfa255105d)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/6b82e8d56896a3eb7f9024ae5838c4c3402bc4a4
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review David-Henningsson/sound-rawmidi-Add-framing-mode/20210418-231512
git checkout 6b82e8d56896a3eb7f9024ae5838c4c3402bc4a4
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> sound/core/rawmidi.c:1014:19: warning: no previous prototype for function 'get_framing_tstamp' [-Wmissing-prototypes]
struct timespec64 get_framing_tstamp(struct snd_rawmidi_substream *substream)
^
sound/core/rawmidi.c:1014:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
struct timespec64 get_framing_tstamp(struct snd_rawmidi_substream *substream)
^
static
1 warning generated.
vim +/get_framing_tstamp +1014 sound/core/rawmidi.c
1013
> 1014 struct timespec64 get_framing_tstamp(struct snd_rawmidi_substream *substream)
1015 {
1016 struct timespec64 ts64 = {0, 0};
1017
1018 if (substream->framing != SNDRV_RAWMIDI_FRAMING_TSTAMP)
1019 return ts64;
1020 if (substream->clock_type == CLOCK_MONOTONIC_RAW)
1021 ktime_get_raw_ts64(&ts64);
1022 else
1023 ktime_get_ts64(&ts64);
1024 return ts64;
1025 }
1026
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi David,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on sound/for-next]
[also build test WARNING on v5.12-rc7 next-20210416]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/David-Henningsson/sound-rawmidi-Add-framing-mode/20210418-231512
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: nios2-randconfig-r031-20210418 (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6b82e8d56896a3eb7f9024ae5838c4c3402bc4a4
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review David-Henningsson/sound-rawmidi-Add-framing-mode/20210418-231512
git checkout 6b82e8d56896a3eb7f9024ae5838c4c3402bc4a4
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=nios2
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> sound/core/rawmidi.c:1014:19: warning: no previous prototype for 'get_framing_tstamp' [-Wmissing-prototypes]
1014 | struct timespec64 get_framing_tstamp(struct snd_rawmidi_substream *substream)
| ^~~~~~~~~~~~~~~~~~
vim +/get_framing_tstamp +1014 sound/core/rawmidi.c
1013
> 1014 struct timespec64 get_framing_tstamp(struct snd_rawmidi_substream *substream)
1015 {
1016 struct timespec64 ts64 = {0, 0};
1017
1018 if (substream->framing != SNDRV_RAWMIDI_FRAMING_TSTAMP)
1019 return ts64;
1020 if (substream->clock_type == CLOCK_MONOTONIC_RAW)
1021 ktime_get_raw_ts64(&ts64);
1022 else
1023 ktime_get_ts64(&ts64);
1024 return ts64;
1025 }
1026
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi David,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on sound/for-next]
[also build test WARNING on v5.12-rc7 next-20210416]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/David-Henningsson/sound-rawmidi-Add-framing-mode/20210418-231512
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: sparc64-randconfig-m031-20210418 (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6b82e8d56896a3eb7f9024ae5838c4c3402bc4a4
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review David-Henningsson/sound-rawmidi-Add-framing-mode/20210418-231512
git checkout 6b82e8d56896a3eb7f9024ae5838c4c3402bc4a4
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=sparc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> sound/core/rawmidi.c:1014:19: warning: no previous prototype for 'get_framing_tstamp' [-Wmissing-prototypes]
1014 | struct timespec64 get_framing_tstamp(struct snd_rawmidi_substream *substream)
| ^~~~~~~~~~~~~~~~~~
vim +/get_framing_tstamp +1014 sound/core/rawmidi.c
1013
> 1014 struct timespec64 get_framing_tstamp(struct snd_rawmidi_substream *substream)
1015 {
1016 struct timespec64 ts64 = {0, 0};
1017
1018 if (substream->framing != SNDRV_RAWMIDI_FRAMING_TSTAMP)
1019 return ts64;
1020 if (substream->clock_type == CLOCK_MONOTONIC_RAW)
1021 ktime_get_raw_ts64(&ts64);
1022 else
1023 ktime_get_ts64(&ts64);
1024 return ts64;
1025 }
1026
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Dne 18. 04. 21 v 17:12 David Henningsson napsal(a): > +#define SND_RAWMIDI_FRAMING_DATA_LENGTH 16 SNDRV_ prefix should be here. > + > +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? > +}; > + > 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. > +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
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 >
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
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
diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h index 334842daa904..b0057a193c31 100644 --- a/include/sound/rawmidi.h +++ b/include/sound/rawmidi.h @@ -81,6 +81,8 @@ struct snd_rawmidi_substream { bool opened; /* open flag */ bool append; /* append flag (merge more streams) */ bool active_sensing; /* send active sensing when close */ + u8 framing; /* whether to frame input data */ + clockid_t clock_type; /* clock source to use for input framing */ int use_count; /* use counter (for output) */ size_t bytes; struct snd_rawmidi *rmidi; diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 535a7229e1d9..124ac74a13e9 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -710,7 +710,7 @@ enum { * Raw MIDI section - /dev/snd/midi?? */ -#define SNDRV_RAWMIDI_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 1) +#define SNDRV_RAWMIDI_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 2) enum { SNDRV_RAWMIDI_STREAM_OUTPUT = 0, @@ -736,12 +736,34 @@ struct snd_rawmidi_info { unsigned char reserved[64]; /* reserved for future use */ }; +enum { + SNDRV_RAWMIDI_FRAMING_NONE = 0, + SNDRV_RAWMIDI_FRAMING_TSTAMP, + SNDRV_RAWMIDI_FRAMING_LAST = SNDRV_RAWMIDI_FRAMING_TSTAMP, +}; + +#define SND_RAWMIDI_FRAMING_DATA_LENGTH 16 + +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]; +}; + 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 */ }; #ifndef __KERNEL__ diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c index aca00af93afe..c3a4940a919d 100644 --- a/sound/core/rawmidi.c +++ b/sound/core/rawmidi.c @@ -683,6 +683,8 @@ static int resize_runtime_buffer(struct snd_rawmidi_runtime *runtime, if (params->buffer_size < 32 || params->buffer_size > 1024L * 1024L) return -EINVAL; + if (params->framing == SNDRV_RAWMIDI_FRAMING_TSTAMP && (params->buffer_size & 0x1f) != 0) + return -EINVAL; if (params->avail_min < 1 || params->avail_min > params->buffer_size) return -EINVAL; if (params->buffer_size != runtime->buffer_size) { @@ -720,7 +722,16 @@ EXPORT_SYMBOL(snd_rawmidi_output_params); int snd_rawmidi_input_params(struct snd_rawmidi_substream *substream, struct snd_rawmidi_params *params) { + if (params->framing) { + if (params->framing > SNDRV_RAWMIDI_FRAMING_LAST) + return -EINVAL; + /* framing requires a valid clock type */ + if (params->clock_type != CLOCK_MONOTONIC_RAW && params->clock_type != CLOCK_MONOTONIC) + return -EINVAL; + } snd_rawmidi_drain_input(substream); + substream->framing = params->framing; + substream->clock_type = params->clock_type; return resize_runtime_buffer(substream->runtime, params, true); } EXPORT_SYMBOL(snd_rawmidi_input_params); @@ -963,6 +974,56 @@ static int snd_rawmidi_control_ioctl(struct snd_card *card, return -ENOIOCTLCMD; } +static int receive_with_tstamp_framing(struct snd_rawmidi_substream *substream, + const unsigned char *buffer, int src_count, const struct timespec64 *tstamp) +{ + struct snd_rawmidi_runtime *runtime = substream->runtime; + struct snd_rawmidi_framing_tstamp *dest_ptr; + struct snd_rawmidi_framing_tstamp frame = { .tv_sec = tstamp->tv_sec, .tv_nsec = tstamp->tv_nsec }; + int dest_frames = 0; + int frame_size = sizeof(struct snd_rawmidi_framing_tstamp); + + BUILD_BUG_ON(frame_size != 0x20); + if (snd_BUG_ON((runtime->hw_ptr & 0x1f) != 0)) + return -EINVAL; + + while (src_count > 0) { + if ((int)(runtime->buffer_size - runtime->avail) < frame_size) { + runtime->xruns += src_count; + break; + } + if (src_count >= SND_RAWMIDI_FRAMING_DATA_LENGTH) + frame.length = SND_RAWMIDI_FRAMING_DATA_LENGTH; + else { + frame.length = src_count; + memset(frame.data, 0, SND_RAWMIDI_FRAMING_DATA_LENGTH); + } + memcpy(frame.data, buffer, frame.length); + buffer += frame.length; + src_count -= frame.length; + dest_ptr = (struct snd_rawmidi_framing_tstamp *) (runtime->buffer + runtime->hw_ptr); + *dest_ptr = frame; + runtime->avail += frame_size; + runtime->hw_ptr += frame_size; + runtime->hw_ptr %= runtime->buffer_size; + dest_frames++; + } + return dest_frames * frame_size; +} + +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; +} + /** * snd_rawmidi_receive - receive the input data from the device * @substream: the rawmidi substream @@ -977,6 +1038,7 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream, const unsigned char *buffer, int count) { unsigned long flags; + struct timespec64 ts64 = get_framing_tstamp(substream); int result = 0, count1; struct snd_rawmidi_runtime *runtime = substream->runtime; @@ -987,8 +1049,11 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream, "snd_rawmidi_receive: input is not active!!!\n"); return -EINVAL; } + spin_lock_irqsave(&runtime->lock, flags); - if (count == 1) { /* special case, faster code */ + if (substream->framing == SNDRV_RAWMIDI_FRAMING_TSTAMP) { + result = receive_with_tstamp_framing(substream, buffer, count, &ts64); + } else if (count == 1) { /* special case, faster code */ substream->bytes++; if (runtime->avail < runtime->buffer_size) { runtime->buffer[runtime->hw_ptr++] = buffer[0]; @@ -1596,6 +1661,12 @@ static void snd_rawmidi_proc_info_read(struct snd_info_entry *entry, " Avail : %lu\n" " Overruns : %lu\n", buffer_size, avail, xruns); + if (substream->framing == SNDRV_RAWMIDI_FRAMING_TSTAMP) { + snd_iprintf(buffer, + " Framing : tstamp\n" + " Clock type : %s\n", + substream->clock_type == CLOCK_MONOTONIC_RAW ? "monotonic raw" : "monotonic"); + } } } } diff --git a/sound/core/rawmidi_compat.c b/sound/core/rawmidi_compat.c index 7397130976d0..2603d2dd8abb 100644 --- a/sound/core/rawmidi_compat.c +++ b/sound/core/rawmidi_compat.c @@ -13,7 +13,9 @@ struct snd_rawmidi_params32 { u32 buffer_size; u32 avail_min; unsigned int no_active_sensing; /* avoid bit-field */ - unsigned char reserved[16]; + unsigned char framing; + unsigned char clock_type; + unsigned char reserved[14]; } __attribute__((packed)); static int snd_rawmidi_ioctl_params_compat(struct snd_rawmidi_file *rfile, @@ -25,6 +27,8 @@ static int snd_rawmidi_ioctl_params_compat(struct snd_rawmidi_file *rfile, if (get_user(params.stream, &src->stream) || get_user(params.buffer_size, &src->buffer_size) || get_user(params.avail_min, &src->avail_min) || + get_user(params.framing, &src->framing) || + get_user(params.clock_type, &src->clock_type) || get_user(val, &src->no_active_sensing)) return -EFAULT; params.no_active_sensing = val;
This commit adds a new framing mode that frames all MIDI data into 32-byte frames with a timestamp. The main benefit is that we can get accurate timestamps even if userspace wakeup and processing is not immediate. Testing on a Celeron N3150 with this mode has a max jitter of 2.8 ms, compared to the in-kernel seq implementation which has a max jitter of 5 ms during idle and much worse when running scheduler stress tests in parallel. Signed-off-by: David Henningsson <coding@diwic.se> --- This version of the patch has been compile tested only, and compared to v4, has only the changes with no controversy. include/sound/rawmidi.h | 2 + include/uapi/sound/asound.h | 26 ++++++++++++- sound/core/rawmidi.c | 73 ++++++++++++++++++++++++++++++++++++- sound/core/rawmidi_compat.c | 6 ++- 4 files changed, 103 insertions(+), 4 deletions(-)