Abandoning in favor of a more sane approach. Related discussion: https://lists.freebsd.org/archives/freebsd-multimedia/2025-February/002845.html

In D47932#1117964, @dev_submerge.ch wrote:

In D47932#1117452, @christos wrote:

Remove __always_inline. Related discussion in D47638.

Aren't we mixing up things here? Regarding performance, we definitely want __always_inline (or __attribute__((__always_inline__))) I suppose. The only issue with that is the unused warnings, which are fully legitimate because we define static functions that are unused in some translation units. There's other ways to fix that, and it seems strange to me that an additional __inline would prevent those warnings.

The commit message says "ALC257 codecs initialization", but in hdaa_patch_direct() and in pin_patch_realtek.h we seem to be affecting other codecs as well. Shouldn't the commit message reflect that?

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

Reflect changes in D47932.

Replace __always_inline __unused with __inline. Related discussion in
D47638.

Remove __always_inline. Related discussion in D47638.

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?

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.

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
      |                        ^
[...]

Add more printfs.

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

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

Address Mark's comments.

Typo.

In D47917#1106908, @dev_submerge.ch wrote:

In D47917#1106149, @christos wrote:

I'm still a bit conflicted as to what approach is the best for this patch specifically. It's probably better not to over-complicate this patch, and instead either leave the vchanrate and vchanformat sysctls uninitialized until the first vchan is created, or just hardcode some value.
@dev_submerge.ch What do you think?

While the practical consequences of this issue are somewhat minor, what bothers me is the disconnectedness of sysctl settings and current state. We should either make the vchan* sysctls reflect the current software format of the primary channel. Or, as an alternative, make the vchan* settings a template for newly created vchans, independent of the current primary channel setup. It doesn't help to decide here that vchanrate only accepts hardware sample rates now, whereas the vchanformat can be chosen freely.

That being said, I'm fine with postponing these fixes, and I would recommend to merge this patch to get it exposed to more real-world testing. It's quite a fundamental change in behavior, but I'm done with reviewing from my side. Please take note of the issues I mentioned so we can tackle them in a future change.

Use __inline instead __unused to silence warnings.

I think with the recent fixes to the tests, and since all comments here have been addressed, we could go ahead with this?

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

Tested it by undefing SND_PCM_64 and it works fine. Thank you.

Rebase on top of recent patches.

In D48421#1113048, @dev_submerge.ch wrote:

I have a proposal to simplify this code, may be worth considering. The general idea would be to

• Use normalized, full 32bit magnitude values for calculation.
• In feeder_mixer.c, for 32bit calculation, check potential overflow before doing the addition, substitute PCM_S32_MIN or PCM_S32_MAX if it would overflow.
• In feeder_volume.c, always use 64bit multiplication but make it clear to the compiler that the volume value is 16bit.

For the mixer this just takes a bit more logic on 32bit architectures, and for the volume multiplying a 16bit sample with a 16bit volume would now be two multiplication ops. On the positive side, we can always maintain the full 32bit resolution, and get rid of the whole *_calc() sample handling.

Does that make sense?

• Fix a typo.
• break at the end of the loop.

Address Ed's comment.

This is still an experimental patch and not finished.

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

Related discussion in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=279787. The issue is solved with this patch.

Fix big endian regressions introduced in last diff.

Use the right pcm_sample_* variants.

Make z intpcm32_t.

Bump.

Reflect changes in D47932.

Bump.

Bump.

Bump.

@dev_submerge.ch @emaste Can you re-accept this patch?

Address comments.

In D48693#1110663, @ziaee wrote:

Thanks! That's how I create them, but this one changed too much for git-arc.sh update HEAD.

In D48693#1110642, @ziaee wrote:

Oops, I forgot to make the patch with -U9999 like bz taught me.
That, is an excellent example which I think is very appropriate to put in development(7).

@dev_submerge.ch Both 32 and 64 bit tests pass normally (with the fix mentioned in D48330). Are we done with this one?

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

Reflect changes in D48435. Hopefully last change.

Assign to primary channel parameters.

In D48435#1108253, @dev_submerge.ch wrote:

Aren't vchanrate and vchanformat now completely disconnected from the actual parameters of the primary channel, until a vchan is opened?

LGTM. I just saw that https://ci.freebsd.org/job/FreeBSD-main-i386-test/lastUnstableBuild/testReport/junit/sys.sound/pcm_read_write/pcm_read/ reported this error, as a result of the 32 -> 24 bit conversion not happening.

In D48330#1108350, @dev_submerge.ch wrote:

In D48330#1108326, @christos wrote:

Pushed a fix for gcc builds: https://cgit.freebsd.org/src/commit/?id=f6631da0b581b28c2bfeea1199b52013bb46aa41

@christos, thanks - do we have some feedback on running the tests by now? I only know of ci.freebsd.org, and I couldn't find anything about test runs.

I think we're missing the note I suggested: "I think it'd also make sense to mention that this can only work if the module is not built into the kernel."

Pushed a fix for gcc builds: https://cgit.freebsd.org/src/commit/?id=f6631da0b581b28c2bfeea1199b52013bb46aa41