Page MenuHomeFreeBSD

dev_submerge.ch (Florian Walpen)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 25 2021, 6:20 PM (168 w, 11 h)

Recent Activity

Tue, Feb 11

dev_submerge.ch added inline comments to D48926: sound tests: Fix downshift calculation in pcm_read_write test..
Tue, Feb 11, 11:10 PM
dev_submerge.ch updated the diff for D48926: sound tests: Fix downshift calculation in pcm_read_write test..

Put braces around if block as suggested.

Tue, Feb 11, 11:03 PM

Mon, Feb 10

dev_submerge.ch requested review of D48926: sound tests: Fix downshift calculation in pcm_read_write test..
Mon, Feb 10, 11:30 PM

Sun, Feb 2

dev_submerge.ch added a comment to D47638: sound: Implement AFMT_FLOAT support.

@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.

Sun, Feb 2, 6:20 PM
dev_submerge.ch added a comment to D48421: sound: Turn PCM_CLAMP_* macros into a function.

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

Sun, Feb 2, 5:09 PM
dev_submerge.ch accepted D48012: sound: Return if the new speed/format is the same as the current one.
Sun, Feb 2, 3:56 PM

Mon, Jan 27

dev_submerge.ch added inline comments to D47932: sound: Refactor the format conversion framework.
Mon, Jan 27, 4:07 PM

Sun, Jan 26

dev_submerge.ch added inline comments to D47932: sound: Refactor the format conversion framework.
Sun, Jan 26, 5:48 PM
dev_submerge.ch added a comment to D47932: sound: Refactor the format conversion framework.

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

Sun, Jan 26, 5:15 PM

Sat, Jan 25

dev_submerge.ch accepted D48336: sound: Handle multiple primary channel cases in vchan sysctls.

Reapprove.

Sat, Jan 25, 1:53 AM
dev_submerge.ch accepted D48335: sound: Cache vchanmode.

Reapprove, I already accepted this.

Sat, Jan 25, 1:44 AM
dev_submerge.ch accepted D48435: sound: Report actual vchanrate and vchanformat in sysctl.

Definitely better. The initial values of vchanformat and vchanrate are still off, but now they adopt the real channel parameters once set via sysctl. We can fix the initial values in a separate patch.

Sat, Jan 25, 1:39 AM

Thu, Jan 23

dev_submerge.ch added a comment to D48617: sound tests: Fix 32bit calculation detection in pcm_read_write.

To the best of my knowledge, this should fix the test on i386. I haven't spotted any other test failures yet on CI, but that's not conclusive - some architectures seem to have build failures and won't run the tests.

Thu, Jan 23, 3:48 AM
dev_submerge.ch requested review of D48617: sound tests: Fix 32bit calculation detection in pcm_read_write.
Thu, Jan 23, 3:43 AM

Wed, Jan 22

dev_submerge.ch added a comment to D48330: sound: Unit test the pcm sample read and write macros.

Ok, the CI builds already found something. I'll post an update for review this evening.

Wed, Jan 22, 3:19 PM
dev_submerge.ch added a comment to D48330: sound: Unit test the pcm sample read and write macros.
Wed, Jan 22, 2:19 PM

Tue, Jan 21

dev_submerge.ch added a comment to D48435: sound: Report actual vchanrate and vchanformat in sysctl.

Aren't vchanrate and vchanformat now completely disconnected from the actual parameters of the primary channel, until a vchan is opened? This would mean there's no useful feedback for the user until a channel is opened, at which point the parameters could suddenly change to something different?

Tue, Jan 21, 8:45 PM
dev_submerge.ch accepted D48434: sound: Initialize channels with sane default rate and format.

I see now that there's a chn_reset() following this for primary channels, which means the defaults here may be overridden if not compatible. Also my concern was in conjunction with D48435, but I think we have to fix it there.

Tue, Jan 21, 7:58 PM

Sun, Jan 19

dev_submerge.ch added inline comments to D48435: sound: Report actual vchanrate and vchanformat in sysctl.
Sun, Jan 19, 3:48 PM
dev_submerge.ch added inline comments to D48434: sound: Initialize channels with sane default rate and format.
Sun, Jan 19, 3:41 PM

Sat, Jan 18

dev_submerge.ch accepted D47917: sound: Allocate vchans on-demand.

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?

Sat, Jan 18, 7:54 PM

Fri, Jan 17

dev_submerge.ch added a comment to D48330: sound: Unit test the pcm sample read and write macros.

Hopefully the last update, sorry for the breakage in the commits based on this. If there's no further issues, we should get this into CI for all supported architectures - then verify that this test passes everywhere before merging the followup commits with the refactorings.

Fri, Jan 17, 7:37 PM
dev_submerge.ch updated the diff for D48330: sound: Unit test the pcm sample read and write macros.

Extend tests and restructure test data:

  • Label each row of test data explicitly.
  • Replace normalized test values with non-UB normalize function.
  • Test read / write macros that use 24 bit samples for 32 bit calculation.
  • Rename some helper functions for clarity.
Fri, Jan 17, 7:30 PM

Jan 13 2025

dev_submerge.ch added inline comments to D47932: sound: Refactor the format conversion framework.
Jan 13 2025, 12:00 AM

Jan 12 2025

dev_submerge.ch added inline comments to D47932: sound: Refactor the format conversion framework.
Jan 12 2025, 11:43 PM
dev_submerge.ch added inline comments to D48421: sound: Turn PCM_CLAMP_* macros into a function.
Jan 12 2025, 11:12 PM
dev_submerge.ch added a comment to D47932: sound: Refactor the format conversion framework.

There's actually three different sample value types involved here:

Jan 12 2025, 2:16 AM
dev_submerge.ch added a comment to D48421: sound: Turn PCM_CLAMP_* macros into a function.

This currently shifts 32bit samples out of the 32bit range in case SND_PCM_64 is not set. sample << PCM_FXSHIFT happens twice. The root cause is in D47932 though.

Jan 12 2025, 1:36 AM

Jan 11 2025

dev_submerge.ch accepted D48424: snd_uaudio: Remove undefined functions.
Jan 11 2025, 10:15 PM
dev_submerge.ch added a comment to D47917: sound: Allocate vchans on-demand.

Unfortunately the last two changes leave the vchanformat uninitialized until the channel's first use:

Jan 11 2025, 7:57 PM

Jan 10 2025

dev_submerge.ch added inline comments to D48330: sound: Unit test the pcm sample read and write macros.
Jan 10 2025, 11:57 AM
dev_submerge.ch updated the diff for D48330: sound: Unit test the pcm sample read and write macros.

Address some of the comments:

  • Turn test data into a const array.
  • Read samples from the local buffer, as intended.
  • Test u-law and A-law sample formats.
  • Extra test case to check the AFMT_BIT macro for correctness.
  • Add a high level comment on the scope of the tests.
Jan 10 2025, 11:20 AM

Jan 9 2025

dev_submerge.ch added inline comments to D48394: sound: Simplify pcm/feeder_mixer.c.
Jan 9 2025, 4:34 PM
dev_submerge.ch added a comment to D47917: sound: Allocate vchans on-demand.

I guess that's because of D47917, which assigns vchanrate to VCHAN_DEFAULT_RATE (48000), during attach in pcm_init(). Perhaps we should initialize these values once the primary channels have been created, and use their rates instead of defaulting to VCHAN_DEFAULT_RATE. I suppose the same should apply to the format.

Original discussion on D48336. Apart from that I think we're done, at least from my side.

Do you agree with this approach? The patch will need some refactoring but I think it should work.

Jan 9 2025, 12:59 PM
dev_submerge.ch added a comment to D47917: sound: Allocate vchans on-demand.

I guess that's because of D47917, which assigns vchanrate to VCHAN_DEFAULT_RATE (48000), during attach in pcm_init(). Perhaps we should initialize these values once the primary channels have been created, and use their rates instead of defaulting to VCHAN_DEFAULT_RATE. I suppose the same should apply to the format.

Jan 9 2025, 12:36 PM
dev_submerge.ch accepted D48336: sound: Handle multiple primary channel cases in vchan sysctls.
Jan 9 2025, 12:05 PM
dev_submerge.ch accepted D48335: sound: Cache vchanmode.
Jan 9 2025, 12:03 PM
dev_submerge.ch accepted D48185: sound: Do not fail from vchan_destroy() if children list is empty.
Jan 9 2025, 11:55 AM

Jan 8 2025

dev_submerge.ch added a comment to D48336: sound: Handle multiple primary channel cases in vchan sysctls.

Regarding the vchanrate setting, the main difference is probably that this USB interface supports only one sample rate (selected via hardware switch). If I set it to 44.1kHz, the behavior is somewhat strange:

root@current:~ # sysctl dev.pcm.0.rec.vchanrate
dev.pcm.0.rec.vchanrate: 48000
root@current:~ # sysctl dev.pcm.0.play.vchanrate=96000
dev.pcm.0.play.vchanrate: 48000 -> 44100
root@current:~ # sysctl dev.pcm.0.rec.vchanrate=96000
dev.pcm.0.rec.vchanrate: 48000 -> 44100
root@current:~ # sysctl dev.pcm.0.rec.vchanrate=48000
dev.pcm.0.rec.vchanrate: 44100 -> 44100
root@current:~ # sysctl dev.pcm.0.rec.vchanrate=96000
dev.pcm.0.rec.vchanrate: 44100 -> 44100

It seems to adopt the hardware sample rate, instead of accepting the user defined one. I'm not sure whether this is actually a bad thing, but it's neither consistent with the other settings nor with previous behavior.

Jan 8 2025, 10:25 AM

Jan 7 2025

dev_submerge.ch added a comment to D48336: sound: Handle multiple primary channel cases in vchan sysctls.

It seems to adopt the hardware sample rate, instead of accepting the user defined one. I'm not sure whether this is actually a bad thing, but it's neither consistent with the other settings nor with previous behavior.

I am trying to understand how the patch would affect this behavior though. The previous implementation picks the first primary channel in a given direction, and this one loops through them, discarding the opposite direction's ones. With 1 primary channel per direction the behavior should be exactly the same. Are you sure this isn't the case without the patches, or at least not this one?

Jan 7 2025, 4:48 PM
dev_submerge.ch added a comment to D48336: sound: Handle multiple primary channel cases in vchan sysctls.

No problem, I suppose you just wanted to test the attention of us reviewers ;-)

Jan 7 2025, 4:31 PM
dev_submerge.ch added a comment to D48336: sound: Handle multiple primary channel cases in vchan sysctls.

With this stack of changes I'm not able to set a different vchanrate than the 48000 default value. Didn't investigate yet, Christos can you reproduce this?

Jan 7 2025, 12:10 AM

Jan 6 2025

dev_submerge.ch added inline comments to D47917: sound: Allocate vchans on-demand.
Jan 6 2025, 2:03 PM
dev_submerge.ch added a comment to D48330: sound: Unit test the pcm sample read and write macros.

Christos, I hope it's ok for me to "hijack" your refactoring efforts with this unit test, but I think it's the proper way forward with D47932. If this test succeeds now on all supported architectures, I'll be much more confident about the correctness of the test and test data. We can then adapt it to test your refactored pcm_sample_read() and pcm_sample_write() functions.

Jan 6 2025, 1:21 AM
dev_submerge.ch requested review of D48330: sound: Unit test the pcm sample read and write macros.
Jan 6 2025, 12:42 AM

Jan 3 2025

dev_submerge.ch added a comment to D47932: sound: Refactor the format conversion framework.

Please note that I'm working on a unit test which exercises the original sample read / write macros, to be committed before this refactoring. This way we can verify the test procedure and test data with the original code, then adapt it to the refactored code. It's still WIP, but I can already tell that we definitely have some regressions here.

Jan 3 2025, 3:08 PM

Dec 31 2024

dev_submerge.ch added inline comments to D47932: sound: Refactor the format conversion framework.
Dec 31 2024, 2:31 AM

Dec 30 2024

dev_submerge.ch added a comment to D48032: sound: Remove macro magic from pcm/feeder_eq.c.

I suspect that to make sure this works as expected, you would also want to introduce functions like

static void __noinline
feed_eq_biquad_s16(...)
{
    feed_eq_biquad(..., AFMT_S16_NE);
}

and call those from the switch statement instead. That would make the programmer's intent more obvious, and prevent the compiler from trying to deduplicate the inlined code.

Dec 30 2024, 3:49 PM

Dec 29 2024

dev_submerge.ch accepted D48249: sound: Make CHN_REMOVE_SAFE() the default.
Dec 29 2024, 10:23 PM
dev_submerge.ch added a comment to D48032: sound: Remove macro magic from pcm/feeder_eq.c.

Bump.

Dec 29 2024, 9:57 PM
dev_submerge.ch added inline comments to D48185: sound: Do not fail from vchan_destroy() if children list is empty.
Dec 29 2024, 9:52 PM
dev_submerge.ch added a comment to D47918: sound: Simplify vchan_getparentchannel().

Not a consequence of your changes here, but it seems we only set the vchan settings on the first channel per direction. There could be multiple, right?

IIRC, there's only one or two sound drivers that support multiple channels per direction. Maybe we should evaluate whether that's actually required and worth the complication it causes throughout the sound module.

There are more places in pcm/ where we always pick the first primary channel. I guess we could implement some heuristics, like choosing the first non-busy channel, or else fall back to the first channel, or the channel with the least vchans, in order to balance things out, but that would require multiple changes throughout sound(4), so we could discuss this further in an email.

Dec 29 2024, 9:22 PM

Dec 28 2024

dev_submerge.ch accepted D48183: sound: Call vchan_destroy() on vchan_create() failure.
Dec 28 2024, 4:15 PM
dev_submerge.ch added inline comments to D48185: sound: Do not fail from vchan_destroy() if children list is empty.
Dec 28 2024, 2:55 PM
dev_submerge.ch added a comment to D47918: sound: Simplify vchan_getparentchannel().

Not a consequence of your changes here, but it seems we only set the vchan settings on the first channel per direction. There could be multiple, right?

Dec 28 2024, 1:39 PM
dev_submerge.ch added a comment to D47932: sound: Refactor the format conversion framework.

Thanks for your input. I agree that we should keep an eye on performance, but I'd like to clarify: In our current refactoring of the format conversion we're not touching anything that changes the buffer sizes. And that's the only thing that could possibly affect audio latency. If latency really is of concern, then we'd have to look at smaller buffer sizes and how to make timers and scheduling more reliable. Also I doubt that we ever had the lowest overall latency, if you count in ASIO on Windows. I suspect that was about the latency introduced by the resampler.

We have the possibility to modify the latency / buffer size already. See https://meka.rs/blog/2017/01/25/sing-beastie-sing/

Dec 28 2024, 2:54 AM

Dec 27 2024

dev_submerge.ch added inline comments to D47917: sound: Allocate vchans on-demand.
Dec 27 2024, 11:52 PM
dev_submerge.ch added inline comments to D47917: sound: Allocate vchans on-demand.
Dec 27 2024, 4:50 PM
dev_submerge.ch added a comment to D47932: sound: Refactor the format conversion framework.

FYI: at some point we had the sound system with the lowest latency. Ariff was comparing MS, OS X and Linux. I do not know if they have catched-up since then, but it may be worth the effort to check the performance / latency for such changes. Unfortunately I haven't found the info about the latency stuff he did, only his resampling quality comparison with other resampler implementations (https://people.freebsd.org/~ariff/z_comparison/). In https://people.freebsd.org/~ariff/old/ he has some old low latency diffs, so some interested soul could check which parts he modified to get lower latency.

Dec 27 2024, 2:45 PM

Dec 26 2024

dev_submerge.ch added a comment to D48207: sound: Safely remove channel from list in one pass.

I stumbled upon that repeated list traversal when reviewing D48185. The implementation is roughly based on SLIST_REMOVE(). Compared to CHN_REMOVE(), the difference in performance should be only minuscule, doing one additional NULL pointer check per iteration. Therefore we may want to consider using the safe variant everywhere.

Dec 26 2024, 11:53 PM
dev_submerge.ch requested review of D48207: sound: Safely remove channel from list in one pass.
Dec 26 2024, 11:38 PM
dev_submerge.ch added inline comments to D48185: sound: Do not fail from vchan_destroy() if children list is empty.
Dec 26 2024, 3:57 PM

Dec 24 2024

dev_submerge.ch accepted D48156: sound: Do not return an error from chn_notify() if the children list is empty.

I can confirm that this fixes the panic experienced with D47917, and I don't see anything else that could be affected by the change in return value. Thank you!

Dec 24 2024, 12:38 PM
dev_submerge.ch added inline comments to D47932: sound: Refactor the format conversion framework.
Dec 24 2024, 2:15 AM

Dec 19 2024

dev_submerge.ch added a comment to D47917: sound: Allocate vchans on-demand.

Interesting. Does calling chn_abort() on the vchan in dsp_close() before the call to vchan_destroy() fix this?

Dec 19 2024, 9:27 PM
dev_submerge.ch added a comment to D47932: sound: Refactor the format conversion framework.

First of all, could you please update these reviews when you rebase? The diff here still has D47868 included, and thus fails to apply cleanly.

Dec 19 2024, 9:13 PM

Dec 17 2024

dev_submerge.ch added a comment to D47917: sound: Allocate vchans on-demand.

Ok, I did some more debugging, my current theory is this:

  1. The first (and only) playback vchan is closed while running, we call vchan_destroy().
  2. vchan_destroy() removes the vchan from its parent's children list.
  3. vchan_destroy() calls chn_kill(), which in turn calls chn_trigger(ABORT).
  4. chn_trigger(ABORT) calls vchan_trigger() through the object method.
  5. vchan_trigger() calls chn_notify() on the parent.
  6. chn_notify() fails with ENODEV because the parent's children list is empty now.
  7. Thus vchan_trigger() also fails, ENODEV.
  8. chn_trigger(ABORT) fails early and doesn't remove the vchan from the channels.pcm.busy list.
  9. chn_kill() frees the vchan data, leaving a stale entry in channels.pcm.busy.
  10. Next occasion to traverse channels.pcm.busy list -> panic!

So a precondition of this panic would be that there's one vchan open which gets closed abruptly, instead of being stopped. This seems to hold in my experiments, as I cannot trigger the panic while keeping a vchan open.

Dec 17 2024, 7:30 PM
dev_submerge.ch added a comment to D47917: sound: Allocate vchans on-demand.

Ok, I did some more debugging, my current theory is this:

Dec 17 2024, 7:04 PM
dev_submerge.ch added a comment to D47917: sound: Allocate vchans on-demand.

Kernel panics when I play something, stop it, cat /dev/sndstat, and then try to play something again. My tree has D47868, D47917 and D47918 on top of main branch, I suspect it's this one.

Fatal trap 9: general protection fault while in kernel mode
...

Interesting, I am not able to reproduce this. Does this happen with a different player than sox as well? Not sure how this patch could be responsible for this, though. All it does is essentially allocate vchans on open() and delete them on close(), instead of keeping them around after close().

I now isolated the panic to this patch (on top of D47868 which is required to build). It also happens with cat /dev/zero > /dev/dsp instead of sox. The cat /dev/sndstat is not needed to reproduce it. I did a full system build / install with and without this patch, consistent results.

I applied just D47868 and this one and still cannot reproduce the panic... Tried all the tests I have in the test section here, as well as what you mentioned, with both vchans enabled or disabled, including bitperfect. Judging from your backtraces, it seems that the panics most likely occur at the CHN_INSERT_HEAD_SAFE()/CHN_REMOVE_SAFE() calls. Is there something worth noting in your setup/test environment?

I will also work on addressing the issues in the inline comments.

Dec 17 2024, 4:40 PM
dev_submerge.ch added inline comments to D47932: sound: Refactor the format conversion framework.
Dec 17 2024, 3:24 PM
dev_submerge.ch added inline comments to D47932: sound: Refactor the format conversion framework.
Dec 17 2024, 2:57 PM
dev_submerge.ch added inline comments to D47932: sound: Refactor the format conversion framework.
Dec 17 2024, 2:54 PM

Dec 16 2024

dev_submerge.ch added a comment to D48036: sound: Remove feed_matrix_apply_generic().

Yeap... I shuffled some patches around to re-organize them in a more intuitive way, so it's quite a mess right now to apply the patches cleanly. The current sequence of patches in my local tree is (from oldest to newest):

sound: Clean up pcm/ includes
sound: Bail out early if a format is not supported
sound: Refactor the format conversion framework
sound: Remove macro magic from pcm/feeder_eq.c
sound: Remove FEEDEQ_CLAMP()
sound: Cache bps in pcm/feeder_eq.c
sound: Remove macro magic from pcm/feeder_matrix.c
sound: Remove feed_matrix_apply_generic()
sound: Implement AFMT_FLOAT support
beep(1): Use AFMT_FLOAT
sound: Allocate vchans on-demand
sound: Simplify vchan_getparentchannel()
sound: Remove SNDBUF_LOCKASSERT()
sound: Improve afmt_tab
sound: Get rid of redundant variables in chn_setspeed() and chn_setformat()
sound: Return if the new speed/format is the same as the current one
Dec 16 2024, 2:16 PM
dev_submerge.ch added a comment to D47917: sound: Allocate vchans on-demand.

FYI, the panic also happens on close:

Dec 16 2024, 11:49 AM
dev_submerge.ch added inline comments to D47917: sound: Allocate vchans on-demand.
Dec 16 2024, 11:24 AM
dev_submerge.ch added a comment to D47917: sound: Allocate vchans on-demand.

Kernel panics when I play something, stop it, cat /dev/sndstat, and then try to play something again. My tree has D47868, D47917 and D47918 on top of main branch, I suspect it's this one.

Fatal trap 9: general protection fault while in kernel mode
...

Interesting, I am not able to reproduce this. Does this happen with a different player than sox as well? Not sure how this patch could be responsible for this, though. All it does is essentially allocate vchans on open() and delete them on close(), instead of keeping them around after close().

Dec 16 2024, 10:15 AM
dev_submerge.ch added a comment to D48032: sound: Remove macro magic from pcm/feeder_eq.c.

If you want to make your function equal to the macro in terms of performance, you have to call it with literal format parameters somehow. E.g. through a switch on the format parameters.

I do not have a benchmark right now to prove the opposite, but do you think such a performance hit is noticeable, if at all existent? Currently we use a function pointer to a specialized function (i.e one for each format), and the patch uses a generic function which fetches the format directly from the struct. I am not really well-versed with compiler optimizations, but I would suppose that the generic one is more likely to be inlined, and thus give us a performance boost (I did see the comment about CPU-branch prediction in D47932).

Dec 16 2024, 1:05 AM
dev_submerge.ch added a comment to D48033: sound: Remove FEEDEQ_CLAMP().

Hm, PCM_CLAMP_S32 does something different when the SND_PCM_64 option is not set?

It does, but I am wondering whether the non-SND_PCM_64 is useful. I am not exactly sure why it does what it does.

Dec 16 2024, 12:04 AM

Dec 15 2024

dev_submerge.ch added a comment to D48036: sound: Remove feed_matrix_apply_generic().

There's more #ifdef FEEDMATRIX_GENERIC code which references this, no?

No. D48035 removes the other code which assigns feed_matrix_apply_generic() as the callback.

Dec 15 2024, 11:35 PM

Dec 13 2024

dev_submerge.ch added a comment to D48035: sound: Remove macro magic from pcm/feeder_matrix.c.

Same as D48032, you may want to call your function with literal format parameters, for performance reasons.

Dec 13 2024, 3:01 PM
dev_submerge.ch added a comment to D48032: sound: Remove macro magic from pcm/feeder_eq.c.

If you want to make your function equal to the macro in terms of performance, you have to call it with literal format parameters somehow. E.g. through a switch on the format parameters.

Dec 13 2024, 2:59 PM
dev_submerge.ch added a comment to D48036: sound: Remove feed_matrix_apply_generic().

There's more #ifdef FEEDMATRIX_GENERIC code which references this, no?

Dec 13 2024, 2:50 PM
dev_submerge.ch added a comment to D48034: sound: Cache bps in pcm/feeder_eq.c.

Why not just cache it in a local variable? That's probably cheaper, even though the "computation" here is trivial anyway.

Dec 13 2024, 2:44 PM
dev_submerge.ch added a comment to D48033: sound: Remove FEEDEQ_CLAMP().

Hm, PCM_CLAMP_S32 does something different when the SND_PCM_64 option is not set?

Dec 13 2024, 2:38 PM
dev_submerge.ch added inline comments to D48031: sound: Bail out early if a format is not supported.
Dec 13 2024, 2:17 PM
dev_submerge.ch accepted D48012: sound: Return if the new speed/format is the same as the current one.
Dec 13 2024, 12:52 AM
dev_submerge.ch accepted D48011: sound: Get rid of redundant variables in chn_setspeed() and chn_setformat().
Dec 13 2024, 12:52 AM

Dec 12 2024

dev_submerge.ch accepted D48008: sound: Remove SNDBUF_LOCKASSERT().
Dec 12 2024, 11:43 PM
dev_submerge.ch accepted D48009: sound: Improve afmt_tab.
Dec 12 2024, 11:41 PM
dev_submerge.ch added a comment to D47917: sound: Allocate vchans on-demand.

Kernel panics when I play something, stop it, cat /dev/sndstat, and then try to play something again. My tree has D47868, D47917 and D47918 on top of main branch, I suspect it's this one.

Dec 12 2024, 8:41 PM
dev_submerge.ch added inline comments to D47917: sound: Allocate vchans on-demand.
Dec 12 2024, 6:57 PM
dev_submerge.ch added a comment to D47638: sound: Implement AFMT_FLOAT support.

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?

Dec 12 2024, 3:11 PM
dev_submerge.ch added inline comments to D47932: sound: Refactor the format conversion framework.
Dec 12 2024, 2:59 PM
dev_submerge.ch added a comment to D47932: sound: Refactor the format conversion framework.
  • I strongly suggest a unit test for the format conversions.

What exactly do we want to test? If converting a sample X from format1 to format2 will give us the intended Y result?

Dec 12 2024, 2:16 PM

Dec 6 2024

dev_submerge.ch added a comment to D47932: sound: Refactor the format conversion framework.

Overall I like the direction of this change, but I still see some major issues:

Dec 6 2024, 12:54 PM

Dec 5 2024

dev_submerge.ch added a comment to D47917: sound: Allocate vchans on-demand.

Some notes:

  • I would like a different name than hw.snd.maxautovchans since the functionality is different now. Maybe hw.snd.vchans? That would break compatibility though. In any case, I will add a RELNOTES entry when it's time to commit.

This implementation is more like a default value for the dev.pcm.X.vchans, maybe default_vchans or autovchans?

The patch makes it so that maxautovchans behaves like a global switch for vchans, which can be overriden by the individual dev.pcm.X.[play|rec].vchans controls. So IMHO default_vchans doesn't really apply here. autovchans could work, but I think simply vchans is more intuitive. What do you think?

Dec 5 2024, 3:55 PM
dev_submerge.ch added a comment to D47917: sound: Allocate vchans on-demand.

Some notes:

  • I would like a different name than hw.snd.maxautovchans since the functionality is different now. Maybe hw.snd.vchans? That would break compatibility though. In any case, I will add a RELNOTES entry when it's time to commit.
Dec 5 2024, 2:08 AM

Dec 3 2024

dev_submerge.ch added a comment to D47868: sound: Clean up pcm/ includes.

I have to admit the centralization through sound.h goes against my intuition - usually I would only include what is needed, where it's actually used. While this change reduces the number of includes, it makes it even harder to see the dependencies and layers in the sound module. But that's just my opinion, I suppose @markj and @emaste may be the better judges in this case.

While I kind of agree, I notice that some of the includes are simply repeated over and over, so IMHO it's better to just have them in one place. After all, we include pcm/sound.h everywhere anyway. Regarding dependencies and layering, I intentionally excluded pcm/sndstat.c-specific includes since they are not needed anywhere else, as well the foo_if.h ones, which I kept/moved to their respective header files.

Dec 3 2024, 1:11 PM
dev_submerge.ch added a comment to D47868: sound: Clean up pcm/ includes.

I have to admit the centralization through sound.h goes against my intuition - usually I would only include what is needed, where it's actually used. While this change reduces the number of includes, it makes it even harder to see the dependencies and layers in the sound module. But that's just my opinion, I suppose @markj and @emaste may be the better judges in this case.

Dec 3 2024, 12:40 PM

Nov 28 2024

dev_submerge.ch accepted D47780: sound: Do not access cv_waiters.
Nov 28 2024, 9:57 AM