Page MenuHomeFreeBSD

sound: Scale PCM secondary buffers by byte rate
Needs ReviewPublic

Authored by kbowling on Mon, Jul 6, 10:19 PM.
Tags
None
Referenced Files
F161974355: D58064.diff
Wed, Jul 8, 1:16 PM
F161943806: D58064.diff
Wed, Jul 8, 5:30 AM
Unknown Object (File)
Tue, Jul 7, 2:25 AM
Subscribers

Details

Reviewers
christos
Summary

The fixed 128 KiB secondary buffer cap dates from stereo-sized
streams. High channel-count or high sample-width OSS streams can
consume most of that budget in one graph quantum, leaving too little
room for capture catch-up or playback headroom.

Keep 128 KiB as the low-rate floor, but derive the effective soft-ring
cap from the channel byte rate, clamped to 1 MiB. Use that per-channel
cap when resizing the soft buffer and when clamping
SNDCTL_DSP_SETFRAGMENT requests.

Also clamp SNDCTL_DSP_LOW_WATER to the current soft-buffer size so an
impossible readiness threshold cannot make poll/select wait forever.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

Some notes/potential followups..

This scales 2nd buf capacity only. It doesn't touch OSS fragment or block size, so large strides may need LOW_WATER or some other changes if they want readiness to correspond to large quantums.

The cap is computed from channel's current byte rate, so if an application sizes fragments before setting the format/channels/rate they may keep a small buffer unless resizing again. This may be a documentation followup.

Full duplex SETFRAGMENT doesn't return potential read/write sizing differences

sys/dev/sound/pcm/channel.c
120

I'm hesitant to introduce more sysctls, especially given this is a quite niche thing. Is there a good reason to provide this?

If we keep this, it should be documented in sound(4)'s man page.

1704

Why are we operating on uint64_ts when eventually we'll truncate to uint32_t? I have the same questions about the uint64_t casts below.

1946

Why are we using CHN_2NDBUFMAXSIZE here and not maxsize?

sys/dev/sound/pcm/channel.h
453

Is there a reason you didn't redefine CHN_2NDBUFMAXSIZE and instead re-introduced a max-max variable?

458

How was this derived?

sys/dev/sound/pcm/dsp.c
1286

You are setting req_maxfrags = maxfrags and then maxfrags = req_maxfrags and req_maxfrags remains unused. Is there a reason we don't use maxfrags only?

kbowling edited the summary of this revision. (Show Details)
sys/dev/sound/pcm/channel.c
1704

I moved this into a helper, it's in the control path so it doesn't seem like something to micro-optimize. It prevents theoretical overflow before division at a high enough channel/bit/kHz.

1946

Intentional, added a block comment to describe that those are hardware buffer sizes unrelated to this change for now. Future work to consider the impact and driver b->maxsize?

sys/dev/sound/pcm/channel.h
453

Yeah good point. I renamed these

458

The block comment above now attempts to explain it, to hopefully fit up to this target for the channel/bit/rate config but there are degenerate cases with high enough channel count and pro audio rates. By MADI-class I mean something like this https://www.ferrofish.com/a32pro-converter-multimode

sys/dev/sound/pcm/dsp.c
1286

the read size modified req_maxfrags, and the write side needed the original value. I cleaned this up with a helper.

sys/dev/sound/pcm/channel.c
1671

It seems like we are always calling this with c locked. Is this a meaningful check?

1922

Is there a rationale for preserving the historical cap for the primary buffer?

sys/dev/sound/pcm/channel.h
436

This comment could be removed, or incorporated into CHN_2NDBUFSIZE_MIN, as in, it could just explain why we set it to 131072. It doesn't make much sense anymore to include it like this.

sys/dev/sound/pcm/dsp.c
130–136

We can simplify this. The check for max < 1 seems redundant. I don't think ch->bufsoft->bufsize can be less than 1.