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

217–224

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.

232

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?

2081–2096

Do we really need to do it for any IOCTL?

[christos]

sys/dev/sound/pcm/dsp.c

217–224

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.

232

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().

2081–2096

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

232

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

232

@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

168

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

168

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

232

@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

165

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();

283

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

310

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

165

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

310

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

165

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

310

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

310

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

310

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.