sound: Retire unit.*

[christos]

Summary

The unit.* code is largely obsolete and imposes limits that are no
longer needed nowadays.

• Capping the maximum allowed soundcards in a given machine. By default, the limit is 512 (snd_max_u() in unit.c), and the maximum possible is 2048 (SND_UNIT_UMAX in unit.h). It can also be tuned through the hw.snd.maxunit loader(8) tunable. Even though these limits are large enough that they should never cause problems, there is no need for this limit to exist in the first place.
• Capping the available device/channel types. By default, this is 32 (snd_max_d() in unit.c). However, these types are pre-defined in pcm/sound.h (see SND_DEV_*), so the cap is unnecessary when we know that their number is constant.
• Capping the number of channels per-device. By default, the limit 1024 (snd_max_c() in unit.c). This is probably the most problematic of the limits mentioned, because this limit can never be reached, as the maximum is hard-capped at either hw.snd.maxautovchans (16 by default), or SND_MAXHWCHAN and SND_MAXVCHANS.

These limtits are encoded in masks (see SND_U_MASK, SND_D_MASK,
SND_C_MASK in unit.h) and are used to construct a bitfield of the form
[dsp_unit, type, channel_unit] in snd_mkunit() which is assigned to
pcm_channel->unit.

This patch gets rid of everything unit.*-related and makes a slightly
different use of the "unit" field to only contain the channel unit
number. The channel type is stored in a new pcm_channel->type field, and
the DSP unit number need not be stored at all, since we can fetch it
from device_get_unit(pcm_channel->parentsnddev->dev). This change has
the effect that we no longer need to impose caps on the number of
soundcards, device/channel types and per-device channels. As a result
the code is noticeably simplified and more readable.

Apart from the fact that the hw.snd.maxunit loader(8) tunable is also
retired as a side-effect of this patch, sound(4)'s behavior remains the
same.

Sponsored by: The FreeBSD Foundation
MFC after: 1 month

Test Plan

/dev/sndstat output is the same:

root@freebsd:~ # cat /dev/sndstat
FreeBSD Audio Driver (64bit 2009061500/amd64)
Installed devices:
pcm0: <Focusrite Scarlett Solo USB> on uaudio0 (1p:1v/1r:1v) default
        snddev flags=0x2e6<AUTOVCHAN,SOFTPCMVOL,BUSY,MPSAFE,REGISTERED,VPC>
        [pcm0:play:dsp0.p0]: spd 48000, fmt 0x00200010/0x00201000, flags 0x00002100, 0x00000006
        interrupts 0, underruns 0, feed 0, ready 0 [b:3072/1536/2|bs:2048/1024/2]
        channel flags=0x2100<BUSY,HAS_VCHAN>
        {userland} -> feeder_mixer(0x00200010) -> feeder_format(0x00200010 -> 0x00201000) -> {hardware}
        pcm0:play:dsp0.p0[pcm0:virtual_play:dsp0.vp0]: spd 8000, fmt 0x00100008, flags 0x10000000, 0x00000000
        interrupts 0, underruns 0, feed 0, ready 0 [b:0/0/0|bs:0/0/0]
        channel flags=0x10000000<VIRTUAL>
        {userland} -> feeder_root(0x00000000) -> {hardware}
        [pcm0:record:dsp0.r0]: spd 48000, fmt 0x00200010/0x00201000, flags 0x00002100, 0x00000007
        interrupts 0, overruns 0, feed 0, hfree 3072, sfree 2048 [b:3072/1536/2|bs:2048/1024/2]
        channel flags=0x2100<BUSY,HAS_VCHAN>
        {hardware} -> feeder_root(0x00201000) -> feeder_format(0x00201000 -> 0x00200010) -> feeder_mixer(0x00200010) -> {userland}
        pcm0:record:dsp0.r0[pcm0:virtual_record:dsp0.vr0]: spd 8000, fmt 0x00100008, flags 0x10000000, 0x00000000
        interrupts 0, overruns 0, feed 0, hfree 0, sfree 0 [b:0/0/0|bs:0/0/0]
        channel flags=0x10000000<VIRTUAL>
        {hardware} -> feeder_root(0x00000000) -> {userland}
No devices installed from userspace.

I have made sure that both the CHN_INSERT_SORT_ASCEND and CHN_INSERT_SORT_DESCEND methods works properly with this patch.

[emaste]

Is hw.snd.maxunit documented anywhere today? I suspect we should have an entry in UPDATING at least reporting that it's removed.

[christos]

In D44912#1024008, @emaste wrote:

Is hw.snd.maxunit documented anywhere today? I suspect we should have an entry in UPDATING at least reporting that it's removed.

It's not documented anywhere AFAIK. I will add the entry shortly.

[christos]

Address Ed's comment: Add an UPDATING for hw.snd.maxunit being gone after this
patch. Will fill the date and commit hash once committed.

[markj]

UPDATING

32 ↗

(On Diff #137549)

IMO this belongs in RELNOTES, not UPDATING. The latter is essentially for people who track main, the former is for users that will eventually upgrade to 15.0.

sys/dev/sound/pcm/dsp.c

2155

Why can't we just pass ch to dsp_unit2name()`? Would it be a layering violation?

sys/dev/sound/pcm/sound.c

483–484

This line looks too long.

[christos]

Address Mark's comments:

• Move hw.snd.maxunit retirement notification from UPDATING to RELNOTES.
• Simplify dsp_unit2name() arguments and pass the channel directly.

[christos]

sys/dev/sound/pcm/dsp.c

2155

That's a good idea. There are a couple of layering violations (in my opinion at least) in sound(4) already, but this one I think isn't a violation.

[dev_submerge.ch]

sys/dev/sound/pcm/sound.c

246–247

What's the use case for type and unit parameters in pcm_chnalloc()? I don't think the code below does what is expected if someone specifies type = -1 and some actual unit number, for example.

[christos]

sys/dev/sound/pcm/sound.c

246–247

Currently, because devunit is a bitfield containing both the type and the channel unit, devunit = -1 would mean that we haven't supplied neither the type nor the type. This is why I added an ntu variable ("no type and unit"?) below, which is used in place of devunit through pcm_chnalloc() to replicate this behavior.

Or am I missing something? I could be wrong.

[christos]

sys/dev/sound/pcm/sound.c

246–247

s/neither the type nor the type/neither the type nor the channel unit/

[dev_submerge.ch]

sys/dev/sound/pcm/sound.c

246–247

I do understand your refactoring intent, but now the function signature suggests that there are two independent parameters, whereas in the implementation they are not. AFAICT pcm_chnalloc() is only ever called with -1, means we do not currently use those parameters. I suppose we could decide to either

1. Keep the lookup logic in this function, define how type and unit parameters should affect said logic, and fix the implementation accordingly.
2. Scrap the lookup logic because we don't use it, clean up the implementation.

[christos]

sys/dev/sound/pcm/sound.c

246–247

I fully support your 2nd proposal to clean up the implementation and get rid of those 2 parameters altogether.

[christos]

After suggestion from Florian, clean up pcm_chnalloc(): get rid of the type
and unit (pre-patch devunit) parameters and related code as there is no
caller that uses them.

[meka_tilda.center]

I had to remove dev/sound/unit.c optional sound from sys/conf/files but otherwise it works.

[christos]

Remove unit.c entry from /sys/conf/files.

[christos]

@dev_submerge.ch @markj Do you think this one is good to go? So far seems to go work fine, and Meka has confirmed as well.

[dev_submerge.ch]

Did some testing, works fine for me.