chn_getunr(): Use __assert_unreachable() instead of returning NULL.

An alternative approach using unr(9). Works with both ascending and descending order: https://reviews.freebsd.org/D46680

So, making this work when the list is sorted in descending order is quite more involved... The only viable solution I can think of is the following: 1) change from SLIST to TAILQ so that we can have TAILQ_FOREACH_REVERSE(), 2) introduce a CHN_FOREACH_REVERSE() macro, 3) store the sort type (i.e ascending or descending) in snddev_info, 4) check this sort type in chn_init(), and if it's ascending order, do what the patch does already, otherwise use CHN_FOREACH_REVERSE() instead instead of CHN_FOREACH(), so that the same code can keep working.

In D46550#1063996, @christos wrote:

In D46550#1063518, @dev_submerge.ch wrote:

This only works when the vchans per type are sorted by unit, in ascending order, right? Aren't the vchans sorted in descending order?

VCHANs are also sorted in ascending order in the channel list stored in snddev_info since vchan_create() calls pcm_chn_add(), which in turn calls CHN_INSERT_SORT_ASCEND(). The descending sort happens in the children list of pcm_channel.

In D46550#1063518, @dev_submerge.ch wrote:

This only works when the vchans per type are sorted by unit, in ascending order, right? Aren't the vchans sorted in descending order?

Fix.

In D46548#1063912, @dev_submerge.ch wrote:

I think the endless loop was caused by goto vchan_alloc;: It skips the retry check, successfully calls vchan_setnew() with unchanged values, and then loops back through goto retry_chnalloc;. There's nothing else in there that would change and break the loop.

That endless loop would be triggered when the vchan_num < c->unit condition is met. I'd expect that to happen before spawning vchan X + 1 though.

Introduce -t option to print "Timing Clock" messages on-demand, in order to
declutter output.

In D46418#1063564, @dev_submerge.ch wrote:

For real devices there's a "Timing clock" message coming at a certain frequency, too much noise to see the other MIDI messages. Maybe we could collect them and print them together, or add a flag to enable or disable printing them?

In D46520#1063489, @dev_submerge.ch wrote:

This is a clever solution for the maxautovchans sysctl. There's some aftermath though. With lots of channels and sysctl hw.snd.verbose=2, sndstat_prepare_pcm() does some memory allocation in sbuf_printf() while holding a lock:

uma_zalloc_debug: zone "malloc-65536" with the following non-sleepable locks held:
exclusive sleep mutex pcm0:virtual_record:dsp0.vr216 (pcm virtual record channel) r = 0 (0xfffff800015a8460) locked @ /usr/src/sys/dev/sound/pcm/sndstat.c:1261
stack backtrace:
#0 0xffffffff80bc423c at witness_debugger+0x6c
#1 0xffffffff80bc5433 at witness_warn+0x403
#2 0xffffffff80ef2e44 at uma_zalloc_debug+0x34
#3 0xffffffff80ef2967 at uma_zalloc_arg+0x27
#4 0xffffffff80b21e8d at malloc+0x7d
#5 0xffffffff80bac3eb at sbuf_extend+0x7b
#6 0xffffffff80bac9aa at sbuf_put_byte+0xda
#7 0xffffffff80ba6318 at kvprintf+0xe68
#8 0xffffffff80bac7ed at sbuf_vprintf+0x6d
#9 0xffffffff80bac89f at sbuf_printf+0x3f
#10 0xffffffff808df4b4 at sndstat_read+0x7c4
#11 0xffffffff809d8fd4 at devfs_read_f+0xe4
#12 0xffffffff80bc8f40 at dofileread+0x80
#13 0xffffffff80bc8ac7 at sys_read+0xb7
#14 0xffffffff810779e8 at amd64_syscall+0x158
#15 0xffffffff81049c3b at fast_syscall_common+0xf8

I think we fixed a similar case for the nvlist sndstat interface.

Remove snd_maxautovchans checks in sysctl_dev_pcm_vchans() and vchan_setnew()
to preserve previous behavior. This way we can still disable VCHANs globally
through hw.snd.maxautovchans and enable them on demand through
dev.pcm.X.[play|rec].vchans.

Handle vchan_setnew() error case.

In D46520#1061062, @dev_submerge.ch wrote:

This will break some use cases, e.g. turning off vchans by default through sysctl hw.snd.maxautovchans=0 and then turning it on for some specific device with sysctl dev.pcm.0.play.vchans=32. While that's also achievable the other way round, we're definitely going to break some people's workflows.

This can indeed break. However, I think it makes more intuitive sense to set dev.pcm.X.[play|rec].vchans to 0 to disable VCHANs, than hw.snd.maxautovchans. I know you said "by default", but still, that's just my humble opinion :)

I'm fine with that opinion. Problem is, it's far easier to set hw.snd.maxautovchans=0 than going for the individual pcm devices in both directions. And it's suggested for exactly this purpose by sound(4):

[...]

Therefore people are currently using it, and it's not a good idea to silently break their workflow.

Read 2 bytes (instead of 3) in case byte1 in SysEx is 0.

Any comment related to the patch?

In D46520#1060642, @dev_submerge.ch wrote:

We're changing semantics of hw.snd.maxautovchans here. Currently it's possible to override maxautovchans via the per pcm device vchans sysctl, with this patch it becomes a hard limit.

Update and depend on D46521.

I will also add a RELNOTES entry when I commit it.

Fix SysEx handling. In the case of 3-byte VendorID, I am not sure if we should
read 3 _additional_ bytes (what the patch does), or 2.

• read(2) on-demand. Introduce read_byte() function.
• Get rid of EVENT_MASK.

In D35772#943182, @asiciliano wrote:

I am interested to take/continue this feature. Are others already working?

In D35776#943183, @asiciliano wrote:

I am interested to take/continue this feature. Are others already working?

LGTM. I will commit the patch and add you as the author, as it needs some style(9) fix as well. Thanks!

Alright, didn't know we could use just GitHub for this. Thank you. :)

In D46377#1058133, @markj wrote:

In D46377#1058035, @christos wrote:

Bump.

Isn't this the same as the patch submitted by Ivan on github? https://github.com/freebsd/freebsd-src/pull/1374

If so, and you think the patch is reasonable, you can just approve it and we'll take care of landing it into src with the correct attribution etc..

Minor fix.

• Add bound check for ctls[].
• Use b2 instead of b1 for vendorid in SysEx.

Inspired by aseqdump, but more user-friendly (printing note names, frequencies, control names, ...), and also native so it does not require ALSA or any sound backend. In the future I would like to support MIDI 2.0 events as well, but I think for now we can roll with just MIDI 1.0.

Bump.

Bump.

Bump.

• Introduce play|rec.autoconv and realtime controls.
  • Maybe the names could be improved
  • See XXXs in mod functions
  • In mod_realtime() the values are hard-coded to the defaults for when realtime=0, but I am not sure if that’s right.
• play|rec.rate and play|rec.format: Report vchanrate/format but made it read-only when VCHANs are disabled.
• Is OSS caps useful info?
• Future goals:
  • Implement iostat -w <n>-like option as a future goal. I would like to think some more about a concise, yet useful/user-friendly output format, consider how many properties each channel has.
  • Think about Florian's comment: “We can still show the hardware rate / format, but we don't have to show mixdown or virtual channel properties when VCHANs are disabled, nor do we have to show different formats when bitperfect is set. Part of the simplification is to hide what is not relevant in a situation.”.
  • Improve the -v option in the future.
  • Write a test script

Make reads more robust.

Use snd_aftm2str() instead of plain hex value for the rest of the feeder
classes.

In D46307#1057113, @dev_submerge.ch wrote:

I suppose the original idea by @meka_tilda.center was to implement another example using mmap, at least there's code fragments for that. What's the plan regarding a mmap example?

Address Florian's comment.

In D46227#1055861, @dev_submerge.ch wrote:

Or something more concise to include more information, because this format will end up with too big lines if we want to include most info we can have about a channel. What would be nice is if this could run in a top(1)-like loop, but this would require us to call read_dev() constantly so that variable info like xruns, feedcount etc are updated. If you mean something like this then it could be a useful feature to troubleshoot problems.

Something like the -w or interval parameter of iostat(8) / zpool-iostat(8)?

In D46227#1055861, @dev_submerge.ch wrote:

In D46227#1055750, @christos wrote:

@dev_submerge.ch What do you think about the comment above? The only thing I'd add is, regarding the generic play|rec.rate and play|rec.format controls I proposed, it might be tricky to figure out what rate/format to report when VCHANs are disabled (in which case we can just report vchanrate and vchanformat).

Is there a vchanrate and vchanformat with vchans disabled? Because in sysctl(8) they just disappear. IIRC cat /dev/sndstat reports these parameters for the primary channel, so they should be accessible somehow. From a user's perspective there's actually two interesting infos to know, the mixdown format and rate, and the hardware format and rate.

Issue warning in case hw.snd.basename_clone is not set.

In D46253#1056061, @dev_submerge.ch wrote:

In D46253#1055964, @christos wrote:

I do not think it overrides the default unit. It turns off hw.snd.basename_clone, so that sound(4) disables access to the /dev/dsp node, in order for virtual_oss to create an actual /dev/dsp device. This is done so applications can still open /dev/dsp as they would normally, but in reality they open the virtual_oss device. It's a hack I guess.

As you explain, if /dev/dsp is created by virtual_oss, then setting the hw.snd.default_unit sysctl has no effect. Applications will open the virtual_oss device. Since we're integrating mixer(8) with virtual_oss here, I think we should issue a warning in that case. It's not obvious to the user. Could be done in a separate patch though.