sound: Implement AFMT_FLOAT support

[christos]

Summary

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.

[1] http://manuals.opensound.com/developer/AFMT_FLOAT.html

PR: 157050, 184380, 264973, 280612, 281390
Sponsored by: The FreeBSD Foundation
MFC after: 1 week

Test Plan

Apply D47639 to test sound.

---

Below is a benchmark for AFMT_FLOAT operations in pcm_sample_read() and pcm_sample_write(). The results are in cycles, and are measured by adding 2 rdtscp() calls in pcm_sample_read() and pcm_sample_write() respectively - one at the start and one at the end, and calculating the difference.

The test program is a simple loopback program that reads and writes zeroes 512 times:

#include <sys/soundcard.h>

#include <err.h>
#include <fcntl.h>
#include <unistd.h>

int
main(int argc, char *argv[])
{
        uint32_t sample;
        int fd, fmt, chans, rate, n;

        if ((fd = open(argv[1], O_RDWR)) < 0)
                err(1, "open(%s)", argv[1]);

        chans = 1;
        if (ioctl(fd, SNDCTL_DSP_CHANNELS, &chans) < 0)
                err(1, "ioctl(SNDCTL_DSP_CHANNELS)");

        fmt = AFMT_FLOAT;
        if (ioctl(fd, SNDCTL_DSP_SETFMT, &fmt) < 0)
                err(1, "ioctl(SNDCTL_DSP_SETFMT)");

        rate = 48000;
        if (ioctl(fd, SNDCTL_DSP_SPEED, &rate) < 0)
                err(1, "ioctl(SNDCTL_DSP_SPEED)");

        for (n = 512; n--;) {
                sample = 0;
                if (read(fd, &sample, sizeof(sample)) < 0)
                        err(1, "read");
                if (write(fd, &sample, sizeof(sample)) < 0)
                        err(1, "write");
        }

        close(fd);
}

The sound card used is a Focusrite Scarlett Solo (snd_uaudio(4)), with VCHANs disabled to simplify the test.

function	cycles
pcm_sample_read(AFMT_S32_LE)	330.806
pcm_sample_write(AFMT_S32_LE)	183.298
pcm_sample_read(AFMT_F32_LE)	3246.6
pcm_sample_write(AFMT_F32_LE)	1893.8

The AFMT_FLOAT operations seem to be ~10x more expensive.

[christos]

sys/dev/sound/pcm/feeder_format.c

68 ↗

(On Diff #146593)

I think we can move the discussion to D47932. I cleaned up the framework quite a lot IMHO so we can work on it easier now.

[christos]

Remove early return in pcm_sample_read().

[christos]

Bump.

[christos]

Fix shifting error I accidentally introduced while rewriting some parts.

[christos]

Use __bswap32() instead of doing the LE <-> BE conversion manually.

As per kevans@ and imp@ on IRC, since v is an int32_t, it shouldn't be a
problem that __bswap32() expects and returns a uint32_t, because the return
value is implicitly casted.

[imp]

sys/dev/sound/pcm/feeder_format.c

141 ↗

(On Diff #147814)

Why not just bswap32?

258 ↗

(On Diff #147814)

Bswap32?

300 ↗

(On Diff #147814)

Bswap32?

[dev_submerge.ch]

I have a hard time to apply these interdependent patches cleanly to my tree. @christos, do you update all affected patches in the series when doing a rebase?

sys/dev/sound/pcm/feeder_format.c

141 ↗

(On Diff #147814)

There's discussion relevant to this in D47932.

[christos]

• Make implementation alignment-agnostic.
• Add support in feeder_chain.c, feeder_rate.c and feeder_volume.c.
• Rebase to work with D47932.
• Define AFMT_F32_OE.
• Guard fpu_kern(9) calls with ifdef _KERNEL so that the unit test can run.

I think the handling in pcm_sample_read() is quite ugly. Maybe it can be
simplified a bit.

Also, according to the benchmarks I provided, it seems that the fpu_kern(9)
calls do introduce a significant overhead, but I am not sure whether that's an amount worth caring about. Perhaps someone can provide more feedback.
It might be worth trying to do these operations using only integers.

I will also add tests to the unit test implemented in D48330.

[christos]

Rebase on top of D48421.

[christos]

Use __DECONST to silence const warning in pcm_sample_read(). A bit ugly...

[christos]

• Fix bugs.
• Write unit tests.
• Re-arrange fpu_kern(9) calls.

There are a few sound tests I haven't done yet, but I am posting as a WIP in
case someone wants to leave a comment.

[christos]

sys/dev/sound/pcm/pcm.h

218

Fixed.

[christos]

Fix big endian regressions introduced in last diff.

I tried recording and playback with real audio (both LE and BE) and things seem
to work fine.

@dev_submerge.ch The big endian tests fail, because on a little endian machine,
converting 0x3f000000 (0.5) to 0x0000003f will result in the float value being
0, so the pcm_sample_* functions return will return wrong values. For
example, pcm_sample_read() reads the input as float (i.e 0x3f000000 ->
0.5), then converts it to big endian (0x3f000000 -> 0x0000003f -> 0), and
lastly converts it to S32 (0 * PCM_S32_MAX = 0), in which case the return
value will be 0, hence the test failure. I am not sure yet how we should adapt
the test program to accomodate floating-point big endian tests.

[dev_submerge.ch]

In D47638#1111768, @christos wrote:

@dev_submerge.ch The big endian tests fail, because on a little endian machine,
converting 0x3f000000 (0.5) to 0x0000003f will result in the float value being
0, so the pcm_sample_* functions return will return wrong values. For
example, pcm_sample_read() reads the input as float (i.e 0x3f000000 ->
0.5), then converts it to big endian (0x3f000000 -> 0x0000003f -> 0), and
lastly converts it to S32 (0 * PCM_S32_MAX = 0), in which case the return
value will be 0, hence the test failure. I am not sure yet how we should adapt
the test program to accomodate floating-point big endian tests.

Well, you do have the test data backwards for the big-endian floats? Just fill in the byte array in big-endian order and check for the correct sample values.

sys/dev/sound/pcm/pcm.h

199–210

Isn't this a bit complicated? Just read it as a 32bit integer from src, reinterpret that as a float, and then calculate the integer value from that float?

[christos]

• Simplify read.
• Make big-endian tests pass. Hopefully it will all work properly on actual big-endian architectures as well.

[christos]

@Alexander88207_protonmail.com I remember you tested the patch when it was still a draft. Can you re-test please?

[Alexander88207_protonmail.com]

Thanks for the ping, sure!

Can you please update your patch so i can apply it cleanly?

error: patch failed: sys/dev/sound/pcm/pcm.h:124
error: sys/dev/sound/pcm/pcm.h: patch does not apply

Thanks.

[christos]

Reflect minor changes in D47932 so that patch can apply cleanly.

[christos]

@Alexander88207_protonmail.com now it should apply fine. Do not forget to apply D47932 and D48421 first.

[Alexander88207_protonmail.com]

[Creating objdir /usr/obj/usr/src/amd64.amd64/sys/GENERIC/modules/usr/src/sys/modules/sgx...]
--- machine ---
--- feeder_eq.o ---
In file included from /usr/src/sys/dev/sound/pcm/feeder_eq.c:43:
/usr/src/sys/dev/sound/pcm/pcm.h:124:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
--- modules-all ---
machine -> /usr/src/sys/amd64/include
--- feeder_eq.o ---
  124 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:220:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  220 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:229:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  229 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:256:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  256 | static __always_inline __inline void
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:361:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  361 | static __always_inline __inline void
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:370:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  370 | static __always_inline __inline void
      |                        ^
--- modules-all ---
--- x86 ---
x86 -> /usr/src/sys/x86/include
--- i386 ---
i386 -> /usr/src/sys/i386/include
--- feeder_eq.o ---
6 errors generated.
--- modules-all ---
--- opt_hwpmc_hooks.h ---
ln -sf /usr/obj/usr/src/amd64.amd64/sys/GENERIC/opt_hwpmc_hooks.h opt_hwpmc_hooks.h
--- opt_kstack_pages.h ---
--- all_subdir_sge ---
ctfconvert -L VERSION -g if_sge.o
--- all_subdir_sgx ---
ln -sf /usr/obj/usr/src/amd64.amd64/sys/GENERIC/opt_kstack_pages.h opt_kstack_pages.h
--- feeder_eq.o ---
*** [feeder_eq.o] Error code 1

make[2]: stopped in /usr/obj/usr/src/amd64.amd64/sys/GENERIC
--- modules-all ---
*** [modules-all] Error code 6

make[2]: stopped in /usr/obj/usr/src/amd64.amd64/sys/GENERIC
--- hdaa.o ---
ctfconvert -L VERSION -g hdaa.o
2 errors

[christos]

In D47638#1117351, @Alexander88207_protonmail.com wrote:

[...]
  124 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:220:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  220 | static __always_inline __inline intpcm_t
      |                        ^
[...]

Interesting, it builds fine for me. @markj do you have any clue as to why that is the case?

[markj]

In D47638#1117352, @christos wrote:

In D47638#1117351, @Alexander88207_protonmail.com wrote:

[...]
  124 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:220:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  220 | static __always_inline __inline intpcm_t
      |                        ^
[...]

Interesting, it builds fine for me. @markj do you have any clue as to why that is the case?

I'm not sure. __always_inline apparently includes the __inline qualifier as well, but that's not enough to suppress unused warnings with clang. See commit d25f0bdceb3ab for the justification of adding that qualifier in the first place. Perhaps it's a bad idea to use __always_inline for functions defined in a header file. I can't see any other examples of that in the kernel.

[christos]

In D47638#1117357, @markj wrote:

In D47638#1117352, @christos wrote:

In D47638#1117351, @Alexander88207_protonmail.com wrote:

[...]
  124 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:220:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  220 | static __always_inline __inline intpcm_t
      |                        ^
[...]

Interesting, it builds fine for me. @markj do you have any clue as to why that is the case?

I'm not sure. __always_inline apparently includes the __inline qualifier as well, but that's not enough to suppress unused warnings with clang. See commit d25f0bdceb3ab for the justification of adding that qualifier in the first place. Perhaps it's a bad idea to use __always_inline for functions defined in a header file. I can't see any other examples of that in the kernel.

Do you think defining it as just __inline will suffice?

[christos]

In D47638#1117358, @christos wrote:

In D47638#1117357, @markj wrote:

In D47638#1117352, @christos wrote:

In D47638#1117351, @Alexander88207_protonmail.com wrote:

[...]
  124 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:220:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  220 | static __always_inline __inline intpcm_t
      |                        ^
[...]

Interesting, it builds fine for me. @markj do you have any clue as to why that is the case?

I'm not sure. __always_inline apparently includes the __inline qualifier as well, but that's not enough to suppress unused warnings with clang. See commit d25f0bdceb3ab for the justification of adding that qualifier in the first place. Perhaps it's a bad idea to use __always_inline for functions defined in a header file. I can't see any other examples of that in the kernel.

Do you think defining it as just __inline will suffice?

It compiles fine.

[markj]

In D47638#1117374, @christos wrote:

In D47638#1117358, @christos wrote:

In D47638#1117357, @markj wrote:

In D47638#1117352, @christos wrote:

In D47638#1117351, @Alexander88207_protonmail.com wrote:

[...]
  124 | static __always_inline __inline intpcm_t
      |                        ^
/usr/src/sys/dev/sound/pcm/pcm.h:220:24: error: duplicate 'inline' declaration specifier [-Werror,-Wduplicate-decl-specifier]
  220 | static __always_inline __inline intpcm_t
      |                        ^
[...]

Interesting, it builds fine for me. @markj do you have any clue as to why that is the case?

I'm not sure. __always_inline apparently includes the __inline qualifier as well, but that's not enough to suppress unused warnings with clang. See commit d25f0bdceb3ab for the justification of adding that qualifier in the first place. Perhaps it's a bad idea to use __always_inline for functions defined in a header file. I can't see any other examples of that in the kernel.

Do you think defining it as just __inline will suffice?

It compiles fine.

I think that's reasonable.

[christos]

Reflect changes in D47932.

[christos]

@Alexander88207_protonmail.com could you apply D47932, D48421 and D47638 again and see if everything works fine?

[christos]

Reflect changes in D47932.

[theron.tarigo_gmail.com]

sys/dev/sound/pcm/pcm.h

207

Rounding toward zero introduces a crossover distortion bias. Although quantization error itself is unavoidable here, we shouldn't do that.
lrintf or roundf or equivalent is needed.

Tested values -> results -> errors
{F111904629}

Error with INTPCM_T(fv * (float)PCM_S32_MAX);
{F111904009}

Error with INTPCM_T(roundf(fv * (float)PCM_S32_MAX));
{F111904015}

Test programs
{F111903997}
{F111904003}

[christos]

sys/dev/sound/pcm/pcm.h

207

That's very useful information, which I admittedly didn't take into consideration. However, I cannot access those files. Can you share them somehow else?

[theron.tarigo_gmail.com]

sys/dev/sound/pcm/pcm.h

207

https://gist.github.com/therontarigo/7c7f642d7baa4c053285f6eaf4be2f1f

[christos]

sys/dev/sound/pcm/pcm.h

207

Thank you very much for this. :-)

Updating the diff in a moment.

[christos]

sys/dev/sound/pcm/pcm.h

207

Hmm, while we can call roundf() in userland, it cannot be used in the kernel, which where this code is executed in real-world scenarios.

[theron.tarigo_gmail.com]

sys/dev/sound/pcm/pcm.h

207

Considering that use of AFMT_FLOAT is discouraged in new programs and that the motivation for this review is to support certain closed libraries (please correct me if I am mistaken), is it reasonable to only support this on CPUs with SSE2, which introduced the efficient rounding instructions?

Otherwise, I can provide a slower though still reasonably efficient non-FPU conversion routine. Even if fpu_kern_enter()/fpu_kern_leave() is moved outside the per-sample loop, avoiding FPU may still be a win depending on write size.

[christos]

sys/dev/sound/pcm/pcm.h

207

Feel free to share the non-FPU solution and we can definitely discuss it. I have no objection towards not using fpu_kern(9), in fact, in the crude benchmarks I've posted above you can see that using it makes things around 10 times more expensive for each sample.

[darkvincentdude_yahoo.com]

It looks like the patch fails to apply against main:

$ git apply D47638.diff
error: patch failed: sys/dev/sound/pcm/feeder_mixer.c:73
error: sys/dev/sound/pcm/feeder_mixer.c: patch does not apply

[christos]

Rebase.

[christos]

In D47638#1125021, @darkvincentdude_yahoo.com wrote:

It looks like the patch fails to apply against main:

$ git apply D47638.diff
error: patch failed: sys/dev/sound/pcm/feeder_mixer.c:73
error: sys/dev/sound/pcm/feeder_mixer.c: patch does not apply

I forgot to rebase it since I committed some patches. Try again. :-)

[darkvincentdude_yahoo.com]

I forgot to rebase it since I committed some patches. Try again. :-)

Thanks! I can confirm that the patches fix game crashing and sound issues in multiple games on LSU Steam + FreeBSD Proton, such as Tell Me Why, Dishonored 2, and Republique.

However, Cyberpunk 2077 still has broken, static sound output, even with the patch.

[christos]

In D47638#1125221, @darkvincentdude_yahoo.com wrote:

I forgot to rebase it since I committed some patches. Try again. :-)

Thanks! I can confirm that the patches fix game crashing and sound issues in multiple games on LSU Steam + FreeBSD Proton, such as Tell Me Why, Dishonored 2, and Republique.

Glad to hear that! :-)

However, Cyberpunk 2077 still has broken, static sound output, even with the patch.

Is this related to the sound system? If yes, please open a bug report on Bugzilla and we can investigate it there.

[darkvincentdude_yahoo.com]

In D47638#1125224, @christos wrote:

In D47638#1125221, @darkvincentdude_yahoo.com wrote:

I forgot to rebase it since I committed some patches. Try again. :-)

Thanks! I can confirm that the patches fix game crashing and sound issues in multiple games on LSU Steam + FreeBSD Proton, such as Tell Me Why, Dishonored 2, and Republique.

Glad to hear that! :-)

However, Cyberpunk 2077 still has broken, static sound output, even with the patch.

Is this related to the sound system? If yes, please open a bug report on Bugzilla and we can investigate it there.

Well, audio works correctly in the game if I choose PulseAudio or Alsa as the Wine sound backends (via winetricks) in the game. So it's only by using the OSS audio backend that sound is all static. Other games with fixed audio (with the patch) are using the OSS backend.

[christos]

In D47638#1125676, @darkvincentdude_yahoo.com wrote:

In D47638#1125224, @christos wrote:

In D47638#1125221, @darkvincentdude_yahoo.com wrote:

I forgot to rebase it since I committed some patches. Try again. :-)

Thanks! I can confirm that the patches fix game crashing and sound issues in multiple games on LSU Steam + FreeBSD Proton, such as Tell Me Why, Dishonored 2, and Republique.

Glad to hear that! :-)

However, Cyberpunk 2077 still has broken, static sound output, even with the patch.

Is this related to the sound system? If yes, please open a bug report on Bugzilla and we can investigate it there.

Well, audio works correctly in the game if I choose PulseAudio or Alsa as the Wine sound backends (via winetricks) in the game. So it's only by using the OSS audio backend that sound is all static. Other games with fixed audio (with the patch) are using the OSS backend.

That could also be a fault in Cyberpunks OSS code though. But you could try setting hw.snd.verbose=2 and seeing if there any sound(4) warnings.