When a channel is closed, dsp_close() either calls vchan_destroy() on vchans,
or chn_abort()/chn_flush() on primary channels. However, the problem with this
is that, when closing a vchan, we end up not terminating the stream properly.
The call sequence we are interested in is the following:
vchan_destroy(vchan) -> chn_kill(vchan) -> chn_trigger(vchan) ->
vchan_trigger(vchan) -> chn_notify(parent)
Even though chn_notify() contains codepaths which call chn_abort(parent),
apparently we do not execute any of those codepaths in this case, so the
DMA remains unterminated, hence why we keep seeing the primary
channel(s) being interrupted even once the application has exited:
root@freebsd:~ # sndctl interrupts
dsp0.play.0.interrupts=1139
dsp0.record.0.interrupts=0
root@freebsd:~ # sndctl interrupts
dsp0.play.0.interrupts=1277
dsp0.record.0.interrupts=0
root@freebsd:~ # sndctl interrupts
dsp0.play.0.interrupts=1394
dsp0.record.0.interrupts=0
The only applications that do not have this issue are those (e.g., mpv) that
manually call ioctls which end up calling chn_abort(), like SNDCTL_DSP_HALT, to
abort the channel(s) during shutdown. For all other applications that do not
manually abort the channel(s), we can confirm that chn_abort()/chn_flush(), or
even chn_trigger(PCMTRIG_ABORT) on the parent, doesn't happen during shutdown.
root@freebsd:~ # dtrace -n 'fbt::chn_abort:entry,fbt::chn_flush:entry { printf("%s", args[0]->name); stack(); }'
dtrace: description 'fbt::chn_abort:entry,fbt::chn_flush:entry ' matched 2 probes
dtrace: buffer size lowered to 1m
^C
[...]
root@freebsd:~ # dtrace -n 'fbt::chn_trigger:entry /args[1] == -1/ { printf("%s", args[0]->name); stack(); }'
dtrace: description 'fbt::chn_trigger:entry ' matched 1 probe
dtrace: buffer size lowered to 1m
CPU ID FUNCTION:NAME
0 68037 chn_trigger:entry dsp0.virtual_play.0
sound.ko`chn_kill+0x134
sound.ko`vchan_destroy+0x94
sound.ko`dsp_close+0x39b
kernel`devfs_destroy_cdevpriv+0xab
kernel`devfs_close_f+0x63
kernel`_fdrop+0x1a
kernel`closef+0x1e3
kernel`closefp_impl+0x76
kernel`amd64_syscall+0x151
kernel`0xffffffff8103841b1
To fix this, modify dsp_close() to execute the primary channel case on both
primary and virtual channels. While what we really care about are the
chn_abort()/chn_flush() calls, it shouldn't hurt to call the rest of the
functions on the vchans as well, to avoid complicating things; they get deleted
right below, anyway.
With the patch applied:
root@freebsd:~ # dtrace -n 'fbt::chn_trigger:entry /args[1] == -1/ { printf("%s", args[0]->name); stack(); }'
dtrace: description 'fbt::chn_trigger:entry ' matched 1 probe
dtrace: buffer size lowered to 1m
CPU ID FUNCTION:NAME
1 68037 chn_trigger:entry dsp0.virtual_play.0
sound.ko`chn_flush+0x2a
sound.ko`dsp_close+0x330
kernel`devfs_destroy_cdevpriv+0xab
kernel`devfs_close_f+0x63
kernel`_fdrop+0x1a
kernel`closef+0x1e3
kernel`closefp_impl+0x76
kernel`amd64_syscall+0x151
kernel`0xffffffff8103841b
0 68037 chn_trigger:entry dsp0.play.0
sound.ko`chn_notify+0x4ce
sound.ko`vchan_trigger+0x105
sound.ko`chn_trigger+0xb4
sound.ko`chn_flush+0x2a
sound.ko`dsp_close+0x330
kernel`devfs_destroy_cdevpriv+0xab
kernel`devfs_close_f+0x63
kernel`_fdrop+0x1a
kernel`closef+0x1e3
kernel`closefp_impl+0x76
kernel`amd64_syscall+0x151
kernel`0xffffffff8103841b
Above we can see a chn_trigger(PCMTRIG_ABORT) on the parent (dsp0.play.0),
which is coming from the chn_abort() (inlined) in chn_notify():
root@freebsd:~ # dtrace -n 'kinst::chn_abort:entry { stack(); }'
dtrace: description 'kinst::chn_abort:entry ' matched 5 probes
dtrace: buffer size lowered to 1m
CPU ID FUNCTION:NAME
1 72580 chn_notify:1192
sound.ko`0xffffffff8296cab4
sound.ko`vchan_trigger+0x105
sound.ko`chn_trigger+0xb4
sound.ko`chn_flush+0x2a
sound.ko`dsp_close+0x330
kernel`devfs_destroy_cdevpriv+0xab
kernel`devfs_close_f+0x63
kernel`_fdrop+0x1a
kernel`closef+0x1e3
kernel`closefp_impl+0x76
kernel`amd64_syscall+0x151
kernel`0xffffffff8103841b
We can also confirm the primary channel(s) are not interrupted anymore:
root@freebsd:/mnt/src # sndctl interrupts
dsp0.play.0.interrupts=0
dsp0.record.0.interrupts=0
In collaboration with: adrian
Tested by: adrian, christos, thj
Sponsored by: The FreeBSD Foundation
MFC after: 2 days
Reviewed by: thj, adrian, emaste
Differential Revision: https://reviews.freebsd.org/D50488
(cherry picked from commit f6430bc61df78be070209d52b4452ae9cf4cd015)
(cherry picked from commit 0c6aa445ec0c85e7c9653d20562907742569de6f)
Approved by: re (cperciva)
Power down the device on shutdown similar to what is done in the case
of suspend. The device may fail to attach on next boot without this.
PR: 286385
Reviewed by: christos, adrian
Differential Revision: https://reviews.freebsd.org/D50306
(cherry picked from commit d9900b9ea2b27f7a0c2eda97841b9499e02e3ea7)
(cherry picked from commit 77521692f4c71213c5419268657e696532c28325)
Approved by: re (cperciva)
The lock is already not held while deleting child devices, and the
bus_topo_lock is already held when child devices are created.
Reviewed by: imp
Differential Revision: https://reviews.freebsd.org/D49272
(cherry picked from commit 02d61f27585f52d422fc1e235ac6226e27145162)
This call was meant to be the default case in the first place, but
somehow missed this.
Reported by: glebius
Fixes: 4021fa32d92d ("sound: Simplify pcm/feeder_mixer.c")
MFC after: 1 week
Sponsored by: The FreeBSD Foundation
(cherry picked from commit b6420b5ea5bcdeb859a2b3357e5dbaafe7aaff88)
Lock around uses of devclass_*() and replace leftover
CTLFLAG_NEEDGIANTs with CTLFLAG_MPSAFE.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Reviewed by: imp, jhb
Differential Revision: https://reviews.freebsd.org/D46700
(cherry picked from commit 35400672df83e337f8792df1972a15003b603930)
Reduce ifdefs, and add aliases for the unsigned formats.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Reviewed by: dev_submerge.ch
Differential Revision: https://reviews.freebsd.org/D48009
(cherry picked from commit a4aff024fd53a38ba08bbf5309589e1865ffe024)
Even though the OSS manual [1] advises against using AFMT_FLOAT, there
are applications that expect the sound driver to support it, and might
not work properly without it.
This patch adds AFMT_F32_LE|BE (as well as AFMT_FLOAT for OSS
compatibility) in sys/soundcard.h and implements AFMT_F32_LE|BE <->
AFMT_S32_LE|BE conversion functions. As a result, applications can
write/read floats to/from sound(4), but internally, because sound(4)
works with integers, we convert floating point samples to integer ones,
before doing any processing.
The reason for encoding/decoding IEEE754s manually, instead of using
fpu_kern(9), is that fpu_kern(9) is not supported by all architectures,
and also introduces significant overhead.
The IEEE754 encoding/decoding implementation has been written by Ariff
Abdullah [2].
[1] http://manuals.opensound.com/developer/AFMT_FLOAT.html
[2] https://people.freebsd.org/~ariff/utils/ieee754.c
PR: 157050, 184380, 264973, 280612, 281390
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Reviewed by: emaste
Differential Revision: https://reviews.freebsd.org/D47638
(cherry picked from commit e1bbaa71d62c8681a576f9f5bedf475c7541bd35)
Make vchanrate and vchanformat reflect the primary channel's software
buffer's rate and format respectively. Fix previous inconsistencies.
Get rid of the initializations in vchan_create() and move them to
chn_init().
Without the feeder_rate_round check in sysctl_dev_pcm_vchanrate(), we
can set the software rate to anything between feeder_rate_min and
feeder_rate_max. If we keep the check, however, the rate is limited to
whatever the driver's min/max is, which can be a problem if, for
example, the driver supports only a single rate, in which case we won't
be able to set anything other than the driver rate.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Differential Revision: https://reviews.freebsd.org/D48961
(cherry picked from commit e372211be5c56e218e974a4478be9aa80bfca064)
Redirect sound to headphone jack when plugged in.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Reviewed by: emaste
Differential Revision: https://reviews.freebsd.org/D49346
(cherry picked from commit 68b6567d09ddd29a16443ad709e2609a9902a190)
PCM_REGISTERED() does this check through PCM_ALIVE().
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
(cherry picked from commit cabf76fde8368222a68865953521e1c3668b5298)
Only to files I've currently made significant contributions to.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
(cherry picked from commit c824383b269d8abe175ea4751194660716d5600e)
Currently dev.pcm.X.mode is calculated only once in pcm_sysinit(), which
is called by pcm_register() during attach, but this can result in
inconsistencies.
For some context, what pcm_mode_init() does is, it checks if "playcount"
is positive, in which case we assume the device supports playback. The
same is done for "reccount" for recording, and if "mixer_dev" is not
NULL, we know the device has a mixer.
The "playcount" and "reccount" variables correspond to the number of
_primary_ playback/recording channels, so we can assume that the primary
channels have been created before reaching pcm_mode_init(). However, for
the mixer that's not always the case. If the mixer is created _after_
pcm_register(), as is the case for snd_dummy(4) for example,
pcm_mode_init() will see that "mixer_dev" is NULL, and report that the
device does not have a mixer, whereas in reality we just created it
afterwards.
While this could be fixed by simply creating the mixers always before
pcm_register(), it is better to be robust and calculate the mode
dynamically.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Differential Revision: https://reviews.freebsd.org/D49024
(cherry picked from commit ab95710f30f7255d3a6be22a1a2c375ee0f96868)
This flag is redundant and essentially a no-op, as it is set when the
device supports at least playback or recording, which is almost always
the case. But even if the device is mixer-only (i.e., 0 channels), there
is no reason to keep this flag; it is only used to bail out of the vchan
sysctl handlers, but we already bail out anyway if we try to use the
sysctl in a vchan direction that is not supported.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Differential Revision: https://reviews.freebsd.org/D49021
(cherry picked from commit b768f2c7773b6e306fb43687657414f9f42a27d4)
Not used outside of pcm/dsp.c.
MFC after: 1 week
Sponsored by: The FreeBSD Foundation
Reviewed by: imp, markj
Differential Revision: https://reviews.freebsd.org/D49217
(cherry picked from commit 2fda8597116e760c32f07af43d260041f975b650)
- Get rid of macro magic.
- Make feed_mixer_info handling similar to most feeders.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Differential Revision: https://reviews.freebsd.org/D48394
(cherry picked from commit 4021fa32d92d656d3d43186cc231695c7ad53d33)
It does what feed_matrix_apply() already does, so it is redundant.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Differential Revision: https://reviews.freebsd.org/D48036
(cherry picked from commit b73b5f70e9f6be951e66531f6f57249eea4ba4dc)
Turn the FEEDMATRIX_DECLARE macro into a single inline function
(feed_matrix_apply()). There is no reason to have this as a macro, it
only complicated the code. An advantage of this patch is that, because
we no longer call the functions created by the macro through function
pointers (apply field of feed_matrix_info), we can call
feed_matrix_apply() directly in feed_matrix_feed().
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Differential Revision: https://reviews.freebsd.org/D48035
(cherry picked from commit ac24c9da8bb7af731646dd7924841a28e2ad7ad7)
Turn the FEEDEQ_DECLARE macro into a single inline function
(feed_eq_biquad()). There is no reason to have this as a macro, and it
only complicates the code. An advantage of this patch is that, because
we no longer call the functions created by the macro through function
pointers (biquad_op), we can call feed_eq_biquad() directly in
feed_eq_feed().
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Differential Revision: https://reviews.freebsd.org/D48032
(cherry picked from commit e18d66d9c515abc729f5c4740b9b999d28abc333)
This makes some subsequent feeder refactors easier to implement.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Differential Revision: https://reviews.freebsd.org/D48421
(cherry picked from commit 4918fc2e238b581aaf1f63e20003d5fa957f0b09)
Merge the PCM_READ|WRITE_* macros defined in pcm/pcm.h, as well as the
intpcm_read|write_* macros defined in pcm/feeder_format.c, into six
inline functions: pcm_sample_read|write[_norm|calc](). The absence of
macro magic makes the code significantly easier to read, use and modify.
Since these functions take the input/output format as a parameter, get
rid of the read() and write() function pointers defined in struct
feed_format_info, as well as the feeder_format_read|write_op()
functions, and use the new read/write functions directly.
Sponsored by: The FreeBSD Fondation
MFC after: 1 week
Reviewed by: markj
Differential Revision: https://reviews.freebsd.org/D47932
(cherry picked from commit 433e270f341cf660b2fe125c2e0f733073829188)
The comment and rationale behind choosing CHN_INSERT_SORT_DESCEND()
instead of CHN_INSERT_SORT_ASCEND() are no longer relevant as of FILLME
("sound: Allocate vchans on-demand").
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Differential Revision: https://reviews.freebsd.org/D48962
(cherry picked from commit 2868776c9c6f5313a5676786f642b766f103d780)
The mechanism of acquiring SD_F_BUSY in pcm_init() and releasing it in
pcm_register() is a leftover from the previous device creation scheme,
where pcm_init() (previously pcm_register()) would create the sysctl
nodes, as well as the device node. In this scenario, acquiring SD_F_BUSY
was necessary, in order to avoid races in case the device was accessed
before it was ready for use. Commit 66f3eb14e955 ("sound: Move sysctl
and /dev/dspX creation to pcm_setstatus()") fixed this issue, so we can
simplify things now. Only acquire SD_F_BUSY in pcm_addchan(), because
chn_init() expects to be called with SD_F_BUSY acquired.
While here, move the sndstat_register() call further down to be more
robust.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Reviewed by: markj
Differential Revision: https://reviews.freebsd.org/D48482
(cherry picked from commit fd906e47b18f53b09524647bf8431dc6170b8dfd)
This gives us a slight performance boost since we do not have to go
through chn_setparam() and the driver functions.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Reviewed by: dev_submerge.ch, emaste
Differential Revision: https://reviews.freebsd.org/D48012
(cherry picked from commit 40616b7e41ff96b6d7522445b00f27f26d593a85)
It's a no-op. There is no lock associated with the buffer.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Reviewed by: dev_submerge.ch, markj
Differential Revision: https://reviews.freebsd.org/D48008
(cherry picked from commit 6524d2a5affd02623e6909fff0ca03ba21882a0c)
vchan_getparentchannel() is used by various vchan sysctl functions to
fetch the first primary channel. However, this assumes that all devices
have only one primary channel per direction. If a device does not meet
this assumption, then the sysctl functions will be applying the
configurations on the first primary channel only.
Since we now have the "primary" channel sublist, we can retire
vchan_getparentchannel() and iterate through the "primary" list in each
sysctl function and apply the settings to all primary channels.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Reviewed by: dev_submerge.ch
Differential Revision: https://reviews.freebsd.org/D48336
(cherry picked from commit 1cbafcd13796934a7896064cdb23fd4e37d58821)
Refactor pcm_chnalloc() and merge with parts of vchan_setnew() (now
removed) and dsp_open()’s channel creation into a new dsp_chn_alloc()
function. The function is responsible for either using a free HW channel
(if vchans are disabled), or allocating a new vchan.
Clean up allocated vchans associated with a given dsp_cdevpriv on
dsp_close() instead of leaving them unused.
hw.snd.vchans_enable (previously hw.snd.maxautovchans) and
dev.pcm.X.{play|rec}.vchans now work as tunables to only enable/disable
vchans, as opposed to setting their number and/or (de-)allocating
vchans. Since these sysctls do not trigger any (de-)allocations anymore,
their effect is instantaneous, whereas before we could have frozen the
machine (when trying to allocate new vchans) when setting
dev.pcm.X.{play|rec}.vchans to a very large value.
Create a new "primary" channel sublist so that we do not waste time
looping through all channels in dsp_chn_alloc(), since we are only
looking for a parent channel to either use, or add a new vchan to. This
guarantees a steady traversal speed, as the parent channels are most
likely going to be just a handful (2). What was currently in place was a
loop through the whole channel list, which meant that the traversal
would take longer the more channels were added to that list.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Reviewed by: dev_submerge.ch
Differential Revision: https://reviews.freebsd.org/D47917
(cherry picked from commit 02d4eeabfd73e6a827f5d42601e99aad92060b04)
There is no good reason to initialize with AFMT_U8 at 8000Hz.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Reviewed by: dev_submerge.ch
Differential Revision: https://reviews.freebsd.org/D48434
(cherry picked from commit e89ee05b7cf1a54d0d6ed56e4d451fdd9a10db43)
vchan_destroy() should be able to call chn_kill() regardless of whether
the channel has been added to the children list yet or not. Also remove
the CHN_F_BUSY check. There is no reason we shouldn't execute the code
below if the parent is not busy.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Reviewed by: dev_submerge.ch
Differential Revision: https://reviews.freebsd.org/D48185
(cherry picked from commit 405b1e50926d4162fb9503644073c0184a8b37d9)
An empty children list does not mean that there is an error.
Reported by: Florian Walpen <dev@submerge.ch>
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Reviewed by: dev_submerge.ch
Differential Revision: https://reviews.freebsd.org/D48156
(cherry picked from commit 164651f16c860049a4ad2098206def8ca7a39b4a)
Note that hdsp and hdspe were just leaking the ivars on detach
previously.
While here, use M_WAITOK to allocate ivars since attach routines are
sleepable. hdsp and hdspe were using M_NOWAIT without checking the
return value.
Reviewed by: imp
Differential Revision: https://reviews.freebsd.org/D47366
(cherry picked from commit a17a41ffde0d1c579dcd7cef7900c3b44be0ec81)
hsdp(4) doesn't exist in stable/14 so is not patched
If a format is unsupported, the feeder_build_* functions will fail
anyway, so bail out early to avoid unnecessary computation.
This is also needed by a series of upcoming patches to the feeder
framework.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Differential Revision: https://reviews.freebsd.org/D48031
(cherry picked from commit 18457e7e25c55b8834e886fd925ec3b11b79d1b6)
Commit 27b932e32fab ("sound: Safely remove channel from list in one
pass") optimized CHN_REMOVE_SAFE() to perform almost equally to
CHN_REMOVE(), so we can turn CHN_REMOVE_SAFE() into CHN_REMOVE() without
fears of performance regressions, while also being more robust.
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Reviewed by: dev_submerge.ch
Differential Revision: https://reviews.freebsd.org/D48249
(cherry picked from commit ffcefe5310e084415a2f292a00f4637d4059c40f)
The CHN_REMOVE_SAFE() macro did two traversals of the channel list to
remove a channel, one to check whether the channel is an element of the
list, and a second traversal through SLIST_REMOVE(). Reduce this to one
traversal, while still preventing a NULL dereference in case the channel
in question is not present in the list.
While here, rename the macro arguments to something more descriptive.
MFC after: 1 week
Reviewed by: christos
Differential Revision: https://reviews.freebsd.org/D48207
(cherry picked from commit 27b932e32faba1137ff307d05b787d837ccadda8)
Main goal is to have a unit test, with sample test data that is verified
against the current macro implementation of pcm sample read and write
functions. With a test in place, we can proceed on a planned refactoring
of the sample read and write code, and confidently check the new code
for regressions.
Implementation of the unit test itself has to avoid any cast or
conversion affected by endianness, to make the tests compatible with all
machine architectures.
MFC after: 1 week
Reviewed by: christos, markj
Differential Revision: https://reviews.freebsd.org/D48330
(cherry picked from commit 27ef5d48c729defb83a8822143dc71ab17f9d68b)
It uses the same audio codec as 12th gen (PCI ID 0x0002).
Actually everything is the same, except the CPU.
Signed-off-by: Daniel Schaefer <dhs@frame.work>
(cherry picked from commit 48b9d78a0a9d795cfdeb56895a27309aadd50c77)
Remove uses of cv_waiters in PCM_RELEASE and CHN_BROADCAST, and also use
a counter around cv_timedwait_sig() in chn_sleep(), which is checked in
pcm_killchans(), as opposed to reading cv_waiters directly, which is a
layering violation.
While here, move CHN_BROADCAST below the channel lock operations.
Reported by: avg, jhb, markj
Sponsored by: The FreeBSD Foundation
MFC after: 2 days
Reviewed by: dev_submerge.ch, avg
Differential Revision: https://reviews.freebsd.org/D47780
(cherry picked from commit 46a97b9cd6fd4415270afe4070082ae69ee21035)
