sound: Implement /dev/dsp as a router device

[christos]

Summary

/dev/dsp currently acts as a symlink to /dev/dsp${hw.snd.default_unit}.
A big issue with this, however, is that applications cannot use it as a
generic default device, which under the hood does the necessary routing
to the appropriate device(s). For this reason many users rely on
virtual_oss to override /dev/dsp with its own, which does act as a
router device.

This patch implements a new /dev/vdsp device which is responsible for
dispatching to /dev/dsp${hw.snd.default_unit}. For ease of use,
dsp_clone() is modified to clone /dev/vdsp instead of
/dev/dsp${hw.snd.default_unit}. The advantage of this approach is that,
applications can now open /dev/dsp and not have to worry about the
device going away, or the default unit changing and having to re-open
and re-configure the new device. They can simply open /dev/dsp and
sound(4) will do all the necessary routing to the default device.

This means we now have hot-swapping built into sound(4), and use of an
audio server (e.g., virtual_oss) for this purpose is no longer needed.
Hot-unplug also works with no problems.

The bulk of the mechanism is implemented in vdsp_getdev(). Please refer
to its comments for a more in-depth explanation.

Sponsored by: The FreeBSD Foundation
MFC after: 1 month

[christos]

dsp_clone() has been retired, however the OSSv4 compatibility aliases
are preserved, with the difference that they are now exposed using
make_dev_alias(9).

	root@freebsd:/mnt/src # ls -l /dev/dsp*
	crw-rw-rw-  1 root wheel 0x8d Mar  3 17:26 /dev/dsp
	crw-rw-rw-  1 root wheel 0x96 Mar  3 17:26 /dev/dsp0
	lrwxr-xr-x  1 root wheel    3 Mar  3 17:26 /dev/dsp_ac3 -> dsp
	lrwxr-xr-x  1 root wheel    3 Mar  3 17:26 /dev/dsp_mmap -> dsp
	lrwxr-xr-x  1 root wheel    3 Mar  3 17:26 /dev/dsp_multich -> dsp
	lrwxr-xr-x  1 root wheel    3 Mar  3 17:26 /dev/dsp_spdifin -> dsp
	lrwxr-xr-x  1 root wheel    3 Mar  3 17:26 /dev/dsp_spdifout -> dsp

hw.snd.basename_clone has also been retired.

This will need a RELNOTES entry, and also updates to virtual_oss.

I also need to update mixer.8 with regards to hot-swapping.

[ziaee]

This sounds amazing! I can't wait to test this!

share/man/man4/pcm.4

528

Macros in list widths do unpredictable things in different implementations, sometimes even rendering as negative width.

[christos]

share/man/man4/pcm.4

528

Thank you.

[mav]

sys/dev/sound/pcm/dsp.c

191–198

I suppose it may be just a step in that direction, but as I noted before, I think it would be good to have record and playback channels to select devices separately in the future.

206

Is this really enough? No manipulations with buffer/fragment parameters? No attempts to replicate current buffer position or content? In best case I suppose it will just lose a buffer of sound, which might (or not?) be acceptable, but I worry there can be more troubles.

I am surprised to not see here any mechanism to handle events from a device side, (or at this stage from the sysctl change). I wonder if some mmap-based application might not call any ioctl for some time, or they regularly poll the playback/record posistion?

1991–2006

Do we really need to do it for any IOCTL?

[christos]

sys/dev/sound/pcm/dsp.c

191–198

As mentioned in the email I sent earlier, this is already in the plans. I just didn't want to convolute this patch too much. It might be better to do that as a separate patch.

206

I haven't tried mmap yet, but regular IO works fine, at least so far that I tested with real audio. The buffer resetting happens in chn_reset().

1991–2006

No, but since the operations are quite cheap (the sndbuf_get*() functions are just simple getters), I thought it's better to just do it here and make it obvious, instead of hiding these inside specific IOCTL handlers.

[christos]

sys/dev/sound/pcm/dsp.c

206

I am surprised to not see here any mechanism to handle events from a device side, (or at this stage from the sysctl change). I wonder if some mmap-based application might not call any ioctl for some time, or they regularly poll the playback/record posistion?

Adding some event notification in the hw.snd.default_unit handler is a valid point. That's actually what I had in place initially, but scrapped it. I will re-think about it.

[christos]

@dev_submerge.ch If I'm not mistaken, your sosso library works with mmap. Could the test case it has be of any use to make sure this patch doesn't break anything?

[christos]

sys/dev/sound/pcm/dsp.c

206

@mav So, the whole mechanism works because we make use of DEVFS_CDEVPRIV(9). vdsp_getdev() is called from within the cdevsw context. That means that currently, a hot-swap cannot triggered from a sysctl, or, as you stated, if an application doesn't make a devfs call for some time.

In order to implement some kind of event notification in sysctl_hw_snd_default_unit() or pcm_unregister() for example, we'd need to also trigger vdsp_getdev() somehow. But if we call vdsp_getdev() from outside the cdevsw context, we won't be able to fetch the cdevpriv. Alternatively, we can keep pointers to the cdevpriv structures so that we can fetch them from anywhere, but I'd like to avoid that if possible.

What do you think?

[ziaee]

Patch does not apply to CURRENT. Also, may I have your permission to post this on the Reddit?{F111680852}

[christos]

In D49216#1123613, @ziaee wrote:

Patch does not apply to CURRENT. Also, may I have your permission to post this on the Reddit?{F111680852}

I thought it'd apply cleanly without D46700. Just apply it first and it'll work fine. You can post it on reddit, although it's still a work-in-progress.

[christos]

This comment was removed by christos.

[christos]

• Address ziaee@'s man page comment.
• Restored hw.snd.basename_clone and dsp_clone(), but instead of cloning /dev/dsp${hw.snd.default_unit}, we are cloning vdsp_cdev, which is defined as /dev/vdsp. This way we preserve backwards compatibility with virtual_oss, since it can override /dev/dsp with its own. Update man page accordingly.
• Check whether the channels are not NULL, as opposed to checking priv->flags and assuming the channels will be non-NULL.
• Check for !DSP_REGISTERED() after acquiring the new cdevpriv in vdsp_getdev(), as well as in vdsp_open().

TODO:

• Be able to trigger vdsp_getdev() from outside the cdevsw context. See https://reviews.freebsd.org/D49216#inline-293492 for more.
• Figure out if mmap needs special handling.
• Is it necessary to lock anything in vdsp_getdev() and/or dsp_clone(), like we did in https://cgit.freebsd.org/src/commit/?id=b18b990d8e7b15d25243d85ea22374dfdde8a18b?
• Once committed, revert https://cgit.freebsd.org/src/commit/?id=9aac27599acaffa21ff69c5be8a2d71d29cc3d6b

[christos]

@mav @markj I have gotten down the functionality of splitting the default into playback and recording already, so I can confirm this is possible, but I still want to finalize this patch first before I submit the next one for review. The main problem currently is how we can trigger vdsp_getdev() from outside the cdevsw context, so that we can account for cases where an application may not call a cdevsw function for a while. Again, as I described in a previous comment, a possible scenario, which sounds a bit ugly IMHO, is to keep our own list of cdevprivs, so that we can fetch them whenever we want. I am not sure however if devfs_get_cdevpriv() would work in those cases, where the cdevsw function is triggered from outside the cdevsw context, though -- I'll need to test this. Do you have any ideas with regards to this?

[christos]

• bus_topo_lock() around uses of devclass_*().
• Pass the unit as argument to vdsp_getdev(). Will be useful in the follow-up patch.

[christos]

Do not ignore value from dsp_poll() in vdsp_poll(), otherwise we might block
there forever.

I've also been testing mmap, and as is expected, during hot-swap sound will not
transfer to the new device.

[christos]

So from a quick investigation, the problem with mmap seems to be the fact that the memory used by the mmap is actually a channel's buffer:

static int
dsp_mmap_single(struct cdev *i_dev, vm_ooffset_t *offset,
    vm_size_t size, struct vm_object **object, int nprot)
{
        ...
	*offset = (uintptr_t)sndbuf_getbufofs(c->bufsoft, *offset);
        ...
}

Which means that when we hot-swap, and as a result, we close the previous device's channels, the buffer also gets deleted. I am wondering what a solution to this could be while also avoiding overcomplicating things. Perhaps we could use global (i.e., not per-channel) buffers so that we can point each new device to them?

[shurd]

sys/dev/sound/pcm/dsp.c

356

I don't really know anything about the subsystem, but this bit makes me wonder...

Do you not need to check PCM_DETACHING() here if the device is detached when a descriptor is open? I don't know how many times my dmesg has been spammed by a pcm device not being able to detach because something still has it opened.

[shurd]

sys/dev/sound/pcm/dsp.c

356

Nevermind, that doesn't exist anymore.

[kib]

I think that it is better to do all that (if doing at all) at the devfs layer.

First, when you assign default dsp device, you should dev_ref() it, as it was done before your patch. Then you can be sure that the struct cdev memory is not going away while you have the pointer to it memoized.

Second, in your vdsp cdev methods, you _must_ call dev_refthread() around calls to lower cdevsw, and the successful dev_refthread() provides you with the right cdevsw. If dev_refthread() failed, you can for instance dev_rel() the reference to the unit.

[christos]

sys/dev/sound/pcm/dsp.c

206

@mav I'm keep thinking about this issue, but I'm starting to wonder if what we're discussing is a real scenario in the first place IMHO. What the patch currently does is, for every cdev call, it checks if the unit has changed, and if it has, does what it has to do. What you mentioned initially is, what if an application _doesn't_ (e.g., mmap) doesn't call any cdev method for some time, and so we'll keep using the old default device until it does. But even mmap applications frequently call ioctls, to get the current pointer position, for example.

In the worst case, an application that doesn't call cdev methods frequently, will just switch to the new unit slower. But I am wondering whether complicating the code is worth the benefit we'll get.

[christos]

In D49216#1127765, @kib wrote:

I think that it is better to do all that (if doing at all) at the devfs layer.

First, when you assign default dsp device, you should dev_ref() it, as it was done before your patch. Then you can be sure that the struct cdev memory is not going away while you have the pointer to it memoized.

Second, in your vdsp cdev methods, you _must_ call dev_refthread() around calls to lower cdevsw, and the successful dev_refthread() provides you with the right cdevsw. If dev_refthread() failed, you can for instance dev_rel() the reference to the unit.

So, I've added dev_refthread() and dev_relthread() around the vdsp cdev methods, like we do in sys/kern/kern_conf for the Giant ones. I am not sure however where the dev_ref() and dev_rel() should go. Would it make sense to dev_ref() after setting *dev = d at the end of vdsp_getdev(), and dev_rel() before devfs_clear_cdevpriv() in vdsp_getdev() again?

[kib]

In D49216#1128877, @christos wrote:

In D49216#1127765, @kib wrote:

I think that it is better to do all that (if doing at all) at the devfs layer.

First, when you assign default dsp device, you should dev_ref() it, as it was done before your patch. Then you can be sure that the struct cdev memory is not going away while you have the pointer to it memoized.

Second, in your vdsp cdev methods, you _must_ call dev_refthread() around calls to lower cdevsw, and the successful dev_refthread() provides you with the right cdevsw. If dev_refthread() failed, you can for instance dev_rel() the reference to the unit.

So, I've added dev_refthread() and dev_relthread() around the vdsp cdev methods, like we do in sys/kern/kern_conf for the Giant ones. I am not sure however where the dev_ref() and dev_rel() should go. Would it make sense to dev_ref() after setting *dev = d at the end of vdsp_getdev(), and dev_rel() before devfs_clear_cdevpriv() in vdsp_getdev() again?

dev_ref() ensures validity of the cdev memory (not its content). So yes, after (or before, does not matter) you memoized the reference, you should dev_ref() it. dev_rel() must be only done after you completely finished with using the pointer, so most likely after devfs_clear_cdevpriv() is called.

[christos]

• Create helper vdsp_open_dev().
• Use dev_ref() and dev_rel().
• Use dev_refthread() and dev_relthread() in vdsp cdev calls.
• Preserve device (flags, mode) and channel (rates, formats) across hot-swaps. This also fixes a bug with duplex -> simplex -> duplex hot-swaps.
• Rename vdsp_getdev() to vdsp_get_dev().

[kib]

sys/dev/sound/pcm/dsp.c

353

I think that this if() should be at the very beginning as the if-else part under the bus_topo_lock(). Like

bus_topo_lock();
d = devclass_get_softc(pcm_devclass, snd_unit);
if (!DSP_REGISTERED(d)) {
        bus_topo_unlock();
        return (EBADF);
} else if (dref) {
    dev_ref(d->dsp_dev);
}
bus_topo_unlock();

471

This is too late/too many things can happen between vdsp_open_dev() and there. You need to dev_ref() in vdsp_open_dev().

498

I do not understand this code. If vdsp_get_dev() returned error, you silently drop it and do nothing, returning success()/no bytes?

[christos]

sys/dev/sound/pcm/dsp.c

353

And I suppose we should dev_rel() for every failure point as well?

498

Yes. My idea is that we want this device to be persistent to not very "critical" errors. For example, suppose you start mpv, and at some point you unplug the only (or all) sound card in the machine. If I opened the device directly (e.g., /dev/dsp0) and it went away, then it'd make sense to just return an error and possibly have mpv killed. However, because /dev/vdsp is a router/virtual device, I would expect it to _keep_ running even after a failure from a particular device. The advantage of this is that in my example, if the device came back at some point, then sound would come back again automatically without needing to restart mpv or anything. I confirmed this during testing.

The reason I do not ignore the return value of vdsp_open() or vdsp_poll() is that for vdsp_open() it doesn't make sense to continue if we cannot even open the device in the first place. For vdsp_poll() returning 0 means timeout, so we want to return the actual value of poll().

[kib]

sys/dev/sound/pcm/dsp.c

353

Yes, but why is it a problem? Otherwise you have a race where the cdev memory can be freed under you.

498

Well, read() and write() must return the amount of bytes read/written. If they return 0, it means eof/no space etc. I do not think that apps expect such response.

[christos]

sys/dev/sound/pcm/dsp.c

498

You are right. I must have tested something different that ended up not working. Returning the bytes normally works as I expected as well. Thanks. :-)

However, I think ignoring the return value of vdsp_ioctl() and vdsp_mmap*() is still sensible. Or?

[kib]

sys/dev/sound/pcm/dsp.c

498

If read/write return real result, why ioctl or mmap should be different. This is esp. weird for mmap, where mmaping failure would result in accessing invalid VA.

[christos]

• Do not ignore return values in vdsp cdev methods.
• Address kib's comments.

[christos]

@kib Any comments regarding the use of dev_ref() and dev_rel()?

[kib]

sys/dev/sound/pcm/dsp.c

450

Do you need to somehow invalidate priv->sc->dsp_dev?

500

If d->dsp_dev is destroyed, do you still need to hold a reference and a pointer to it?

[christos]

sys/dev/sound/pcm/dsp.c

450

This part failing does not imply that the cdev is unusable necessarily, so I think we shouldn't touch that.

500

Do you mean if it's destroyed right before dev_refthread()? I'm not sure I understand exactly.

[christos]

Remove unit argument from vdsp_open_dev() and vdsp_get_dev() and figure
it out inside the functions instead. It's cleaner in combination with the
follow-up patch.

[kib]

sys/dev/sound/pcm/dsp.c

357

I still do not understand the approach. If the nested call to d_open() failed, you still return success, and release d->dsp_dev. This allows d->dsp_dev to become freed at any moment. Also, if d_open failed, the code would marshal other d_ calls to the device which did not receive its open().

[dev_submerge.ch]

Hi there, I'm still short on spare time but did some very brief tests.

First this lock order reversal, don't know how to reproduce yet and whether it is related to this patch:

lock order reversal:
 1st 0xfffff800030fae60 dsp0.virtual_record.0 (pcm virtual record channel, sleep mutex) @ /usr/src/sys/dev/sound/pcm/dsp.c:2880
 2nd 0xfffff800030f9040 pcm0 (sound cdev, sleep mutex) @ /usr/src/sys/dev/sound/pcm/channel.c:2289
lock order sound cdev -> pcm virtual record channel established at:
#0 0xffffffff80bce3e4 at witness_checkorder+0x364
#1 0xffffffff80b31781 at __mtx_lock_flags+0x91
#2 0xffffffff808e964d at sound_oss_sysinfo+0x1fd
#3 0xffffffff808d08b0 at dsp_ioctl+0x950
#4 0xffffffff808d4cdd at vdsp_ioctl+0x5d
#5 0xffffffff809dd681 at devfs_ioctl+0xd1
#6 0xffffffff80c6bba6 at vn_ioctl+0xb6
#7 0xffffffff809ddd4e at devfs_ioctl_f+0x1e
#8 0xffffffff80bd4ad6 at kern_ioctl+0x286
#9 0xffffffff80bd47ef at sys_ioctl+0x12f
#10 0xffffffff81098a2a at amd64_syscall+0x15a
#11 0xffffffff8106a01b at fast_syscall_common+0xf8
lock order pcm virtual record channel -> sound cdev attempted at:
#0 0xffffffff80bcec61 at witness_checkorder+0xbe1
#1 0xffffffff80b31781 at __mtx_lock_flags+0x91
#2 0xffffffff808ca410 at chn_trigger+0x110
#3 0xffffffff808ebfbe at vchan_trigger+0x11e
#4 0xffffffff808ca3cc at chn_trigger+0xcc
#5 0xffffffff808d70eb at dsp_oss_syncstart+0x1cb
#6 0xffffffff808d1bbf at dsp_ioctl+0x1c5f
#7 0xffffffff808d4cdd at vdsp_ioctl+0x5d
#8 0xffffffff809dd681 at devfs_ioctl+0xd1
#9 0xffffffff80c6bba6 at vn_ioctl+0xb6
#10 0xffffffff809ddd4e at devfs_ioctl_f+0x1e
#11 0xffffffff80bd4ad6 at kern_ioctl+0x286
#12 0xffffffff80bd47ef at sys_ioctl+0x12f
#13 0xffffffff81098a2a at amd64_syscall+0x15a
#14 0xffffffff8106a01b at fast_syscall_common+0xf8

Then I tried to disable vchans on one device, could it be that vchans are silently reenabled somehow? It first (correctly) refused to play an incompatible format in cooked mode, but after playing some file in sox it accepted the incompatible format by converting it. The vchan sysctl remained set to 0.

What's the idea for error handling in case of incompatible formats?
Should bitperfect operation be possible with /dev/dsp at all?
How about switching to a device without vchans set?

I think this needs some serious testing, maybe even a unit test using the dummy device?

[christos]

In D49216#1135617, @dev_submerge.ch wrote:

Hi there, I'm still short on spare time but did some very brief tests.

First this lock order reversal, don't know how to reproduce yet and whether it is related to this patch:

This LOR is a known bug and not related to the patch. I'm pretty sure it's been there since before I even started working on sound(4).

Then I tried to disable vchans on one device, could it be that vchans are silently reenabled somehow? It first (correctly) refused to play an incompatible format in cooked mode, but after playing some file in sox it accepted the incompatible format by converting it. The vchan sysctl remained set to 0.

Can you share the commands you are running exactly? I tested the following with mpv without problems:

1. Plug USB card, disable vchans, enable bitperfect. So we're using S32 at 48000.
2. Load dummy driver, default settings.
3. Set dummy as default device and start mpv track to it with the samplerate set to 44100.
4. Hot-swap to USB card. It correctly uses the primary channel, and the sound is wrong since the card is set at 48000 and not 44100.

What's the idea for error handling in case of incompatible formats?

So, the current code preserves the configuration set by userland across devices, that is, the format and rate is whatever was set through the ioctl interface. Now, it can be the case that the application will request X sample rate, but sound(4) will set it to Y instead. In that case from that point onwards, Y is our sample rate.

Should bitperfect operation be possible with /dev/dsp at all?

Why not? As I illustrated in the example above, the behavior, at least in my opinion, is what you'd expect. If you are currently playing audio to a non-bitperfect device, and then switch to a bitperfect one, but with the wrong format/rate, then you'd expect to get bad sound. Is there a reason we shouldn't support bitperfect?

How about switching to a device without vchans set?

Ditto, see example above.

I think this needs some serious testing, maybe even a unit test using the dummy device?

I'm not sure how we can use the dummy device to test this. If there is no swapping involved, then the patch mostly works exactly the same as accessing /dev/dspX directly.

sys/dev/sound/pcm/dsp.c

357

Yeah, this was actually a typo. I've now modified the last return to return ret instead of 0, so if there's an error from d_open() we'll propagate it normally.

[dev_submerge.ch]

On a side note, I tried to test exclusive and bitperfect mode yesterday, as on request by the application through ioctl. Both are completely ineffective. When did we break this, and was that intentional?

In D49216#1136393, @christos wrote:

In D49216#1135617, @dev_submerge.ch wrote:

Hi there, I'm still short on spare time but did some very brief tests.

First this lock order reversal, don't know how to reproduce yet and whether it is related to this patch:

This LOR is a known bug and not related to the patch. I'm pretty sure it's been there since before I even started working on sound(4).

Then I tried to disable vchans on one device, could it be that vchans are silently reenabled somehow? It first (correctly) refused to play an incompatible format in cooked mode, but after playing some file in sox it accepted the incompatible format by converting it. The vchan sysctl remained set to 0.

Can you share the commands you are running exactly? I tested the following with mpv without problems:

No swapping involved, a single device, just what I wrote:

1. Open /dev/dsp with incompatible settings in JACK or sosso_test, fails.
2. Open /dev/dsp with a more tolerant application like sox, succeeds.
3. Open /dev/dsp again with incompatible settings (same as sox play format), succeeds now.

I'd not expect opening the device in step 2 to change the behavior of the other, since vchans are disabled and the hardware is fixed.

What's the idea for error handling in case of incompatible formats?

So, the current code preserves the configuration set by userland across devices, that is, the format and rate is whatever was set through the ioctl interface. Now, it can be the case that the application will request X sample rate, but sound(4) will set it to Y instead. In that case from that point onwards, Y is our sample rate.

That"s consistent with userland, but sort of implies vchans - otherwise you can't swap to another device. If the device we swap to is incompatible (vchans disabled through settings), where and how does it fail? Does the application get an error (we cannot update the format), or does the swap fail?

Should bitperfect operation be possible with /dev/dsp at all?

Why not? As I illustrated in the example above, the behavior, at least in my opinion, is what you'd expect. If you are currently playing audio to a non-bitperfect device, and then switch to a bitperfect one, but with the wrong format/rate, then you'd expect to get bad sound. Is there a reason we shouldn't support bitperfect?

I'd expect bitperfect to fail with error because of the incompatible format, like it does when opening the device directly. Just throwing the incompatible audio data at the bitperfect device would be careless and possibly ear-damaging.

How about switching to a device without vchans set?

Ditto, see example above.

Ditto, as above ;-)

I think this needs some serious testing, maybe even a unit test using the dummy device?

I'm not sure how we can use the dummy device to test this. If there is no swapping involved, then the patch mostly works exactly the same as accessing /dev/dspX directly.

Some basic tests could be performed, but yes, we'd need to create multiple dummy devices for swapping which isn't supported (yet).

[christos]

In D49216#1136549, @dev_submerge.ch wrote:

On a side note, I tried to test exclusive and bitperfect mode yesterday, as on request by the application through ioctl. Both are completely ineffective. When did we break this, and was that intentional?

Can you share what you do to reproduce this? I want to look into it, although bitperfect hasn't been touched that I can remember.

Even without (but with it as well) the patch included, if I disable vchans and set bitperfect, and play audio with an incompatible format through mpv, I do get sound, but of course, wrong one:

root@freebsd:~ # mpv hendrix.mp3 --audio-format=s16 &
root@freebsd:~ #  (+) Audio --aid=1 (mp3 2ch 48000Hz)
AO: [oss] 48000Hz stereo 2ch s16

root@freebsd:~ # cat /dev/sndstat
FreeBSD Audio Driver
Installed devices:
pcm0: <Focusrite Scarlett Solo USB> on uaudio0 (1p:0v/1r:0v) default
        snddev flags=0xfc<SOFTPCMVOL,BUSY,MPSAFE,REGISTERED,BITPERFECT,VPC>
        [dsp0.play.0]: spd 48000, fmt 0x00201000, flags 0x2000010c, 0x00000001, pid 1565 (mpv)
                interrupts 202, underruns 0, feed 201, ready 11776
                [b:3072/1536/2|bs:16384/2048/8]
                channel flags=0x2000010c<RUNNING,TRIGGERED,BUSY,BITPERFECT>
                {userland} -> feeder_root(0x00201000) -> {hardware}
        [dsp0.record.0]: spd 48000, fmt 0x00200010/0x00201000, flags 0x00000000, 0x00000023
                interrupts 0, overruns 0, feed 0, hfree 3072, sfree 32768
                [b:3072/1536/2|bs:32768/256/128]
                channel flags=0x0
                {hardware} -> feeder_root(0x00201000) -> feeder_format(0x00201000 -> 0x00200010) -> feeder_volume(0x00200010) -> {userland}
No devices installed from userspace.

There is no additional feeder added, and the format/rate stays 48000/s32le, even though I play the track with 44100/s16le.

No swapping involved, a single device, just what I wrote:

1. Open /dev/dsp with incompatible settings in JACK or sosso_test, fails.
2. Open /dev/dsp with a more tolerant application like sox, succeeds.
3. Open /dev/dsp again with incompatible settings (same as sox play format), succeeds now.

I'd not expect opening the device in step 2 to change the behavior of the other, since vchans are disabled and the hardware is fixed.

For me even opening jack succeeds. Can you share the commands + output? It'd also be great to see /dev/sndstat when sox is running.

What's the idea for error handling in case of incompatible formats?

So, the current code preserves the configuration set by userland across devices, that is, the format and rate is whatever was set through the ioctl interface. Now, it can be the case that the application will request X sample rate, but sound(4) will set it to Y instead. In that case from that point onwards, Y is our sample rate.

That"s consistent with userland, but sort of implies vchans - otherwise you can't swap to another device. If the device we swap to is incompatible (vchans disabled through settings), where and how does it fail? Does the application get an error (we cannot update the format), or does the swap fail?

The high-level control flow of this patch is:

For the first ever /dev/dsp open call, essentially just call open() on the default dsp device. If it fails, we're done, there's nothing more we can do. If it succeeds however, we do the following for every vdsp cdev method:

1. In vdsp_get_dev(), fetch the default device.
2. If the default device is already registered and we already have an allocated priv for this FD, just do nothing and succeed. We simply call the respective dsp cdev method (e.g., read(), write(), ...). If there's no hot-swapping or detach involved, this is going to be what always happens.
3. If the device either detached, or we switched default unit, fetch the new default device, call open() on it with the userland's config, and use this one from now on.

In essence, as long as you use the same device, the functionality is more or less the same as if you used /dev/dspX directly.

To answer your question, if vdsp_get_dev() fails (see the various points of failure), then we do nothing. In other words, we'll keep trying to open the default device until we eventually swap to one that is possible to be opened. If we get past vdsp_get_dev(), we simply return whatever the dsp cdev method returned, at this point, it's the dsp layer's job, so if we failed at some ioctl, vdsp will just propagate the error, and as a result, fail as well.

Should bitperfect operation be possible with /dev/dsp at all?

Why not? As I illustrated in the example above, the behavior, at least in my opinion, is what you'd expect. If you are currently playing audio to a non-bitperfect device, and then switch to a bitperfect one, but with the wrong format/rate, then you'd expect to get bad sound. Is there a reason we shouldn't support bitperfect?

I'd expect bitperfect to fail with error because of the incompatible format, like it does when opening the device directly. Just throwing the incompatible audio data at the bitperfect device would be careless and possibly ear-damaging.

I haven't made up my mind on this to be honest. I think that if someone wants to deliberately use bitperfect, they probably already know the side effects of using it incorrectly. Also, I don't know if it's our job to add more caveats to the code than let the user be responsible. Even from an experimental point of view it might be beneficial to allow more things.

I think this needs some serious testing, maybe even a unit test using the dummy device?

I'm not sure how we can use the dummy device to test this. If there is no swapping involved, then the patch mostly works exactly the same as accessing /dev/dspX directly.

Some basic tests could be performed, but yes, we'd need to create multiple dummy devices for swapping which isn't supported (yet).

Making the dummy device be loaded multiple times is in my list already, but haven't worked on it at all yet. :-)

[dev_submerge.ch]

In D49216#1136683, @christos wrote:

In D49216#1136549, @dev_submerge.ch wrote:

On a side note, I tried to test exclusive and bitperfect mode yesterday, as on request by the application through ioctl. Both are completely ineffective. When did we break this, and was that intentional?

Can you share what you do to reproduce this? I want to look into it, although bitperfect hasn't been touched that I can remember.

Just use an application like JACK and set it to request exclusive / bitperfect mode. Then try to play something in parallel on the same device, which shouldn't be possible. Or set JACK to use a hardware incompatible format, which should also fail.

Even without (but with it as well) the patch included, if I disable vchans and set bitperfect, and play audio with an incompatible format through mpv, I do get sound, but of course, wrong one:

Not the sysctl settings, O_EXCL on open() and OSSv4 ioctl requests by the application.

root@freebsd:~ # mpv hendrix.mp3 --audio-format=s16 &
root@freebsd:~ #  (+) Audio --aid=1 (mp3 2ch 48000Hz)
AO: [oss] 48000Hz stereo 2ch s16

root@freebsd:~ # cat /dev/sndstat
FreeBSD Audio Driver
Installed devices:
pcm0: <Focusrite Scarlett Solo USB> on uaudio0 (1p:0v/1r:0v) default
        snddev flags=0xfc<SOFTPCMVOL,BUSY,MPSAFE,REGISTERED,BITPERFECT,VPC>
        [dsp0.play.0]: spd 48000, fmt 0x00201000, flags 0x2000010c, 0x00000001, pid 1565 (mpv)
                interrupts 202, underruns 0, feed 201, ready 11776
                [b:3072/1536/2|bs:16384/2048/8]
                channel flags=0x2000010c<RUNNING,TRIGGERED,BUSY,BITPERFECT>
                {userland} -> feeder_root(0x00201000) -> {hardware}
        [dsp0.record.0]: spd 48000, fmt 0x00200010/0x00201000, flags 0x00000000, 0x00000023
                interrupts 0, overruns 0, feed 0, hfree 3072, sfree 32768
                [b:3072/1536/2|bs:32768/256/128]
                channel flags=0x0
                {hardware} -> feeder_root(0x00201000) -> feeder_format(0x00201000 -> 0x00200010) -> feeder_volume(0x00200010) -> {userland}
No devices installed from userspace.

There is no additional feeder added, and the format/rate stays 48000/s32le, even though I play the track with 44100/s16le.

You actually play the track with 48kHz? mpv may be able to convert by itself.

No swapping involved, a single device, just what I wrote:

1. Open /dev/dsp with incompatible settings in JACK or sosso_test, fails.
2. Open /dev/dsp with a more tolerant application like sox, succeeds.
3. Open /dev/dsp again with incompatible settings (same as sox play format), succeeds now.

I'd not expect opening the device in step 2 to change the behavior of the other, since vchans are disabled and the hardware is fixed.

For me even opening jack succeeds. Can you share the commands + output? It'd also be great to see /dev/sndstat when sox is running.

That's the point, it shouldn't, at least not if you use a hardware incompatible sample rate. But I'll see to that I can reproduce this properly when I'm back at my workstation at home, some time this weekend.

What's the idea for error handling in case of incompatible formats?

So, the current code preserves the configuration set by userland across devices, that is, the format and rate is whatever was set through the ioctl interface. Now, it can be the case that the application will request X sample rate, but sound(4) will set it to Y instead. In that case from that point onwards, Y is our sample rate.

That"s consistent with userland, but sort of implies vchans - otherwise you can't swap to another device. If the device we swap to is incompatible (vchans disabled through settings), where and how does it fail? Does the application get an error (we cannot update the format), or does the swap fail?

The high-level control flow of this patch is:

For the first ever /dev/dsp open call, essentially just call open() on the default dsp device. If it fails, we're done, there's nothing more we can do. If it succeeds however, we do the following for every vdsp cdev method:

1. In vdsp_get_dev(), fetch the default device.
2. If the default device is already registered and we already have an allocated priv for this FD, just do nothing and succeed. We simply call the respective dsp cdev method (e.g., read(), write(), ...). If there's no hot-swapping or detach involved, this is going to be what always happens.
3. If the device either detached, or we switched default unit, fetch the new default device, call open() on it with the userland's config, and use this one from now on.

In essence, as long as you use the same device, the functionality is more or less the same as if you used /dev/dspX directly.

Thanks for the explanation. I suppose this hot-swapping works for simple applications, but not for anything where latency or buffer sizes matter?

To answer your question, if vdsp_get_dev() fails (see the various points of failure), then we do nothing. In other words, we'll keep trying to open the default device until we eventually swap to one that is possible to be opened. If we get past vdsp_get_dev(), we simply return whatever the dsp cdev method returned, at this point, it's the dsp layer's job, so if we failed at some ioctl, vdsp will just propagate the error, and as a result, fail as well.

Should bitperfect operation be possible with /dev/dsp at all?

Why not? As I illustrated in the example above, the behavior, at least in my opinion, is what you'd expect. If you are currently playing audio to a non-bitperfect device, and then switch to a bitperfect one, but with the wrong format/rate, then you'd expect to get bad sound. Is there a reason we shouldn't support bitperfect?

I'd expect bitperfect to fail with error because of the incompatible format, like it does when opening the device directly. Just throwing the incompatible audio data at the bitperfect device would be careless and possibly ear-damaging.

I haven't made up my mind on this to be honest. I think that if someone wants to deliberately use bitperfect, they probably already know the side effects of using it incorrectly. Also, I don't know if it's our job to add more caveats to the code than let the user be responsible. Even from an experimental point of view it might be beneficial to allow more things.

A well written application checks the format in use even for bitperfect audio. If your hot-swap implementation doesn't check that in the same manner, it is broken. I know OSSv4 isn't particularly well designed regarding format negotiation between Application and Audio hardware, but the way your implementation works removes this possibility entirely when actually used to change the device.