sound: Scale PCM secondary buffers by byte rate

[kbowling]

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

[kbowling]

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

[christos]

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.

1677

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.

1924

Why are we using CHN_2NDBUFMAXSIZE here and not maxsize?

sys/dev/sound/pcm/channel.h

439

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

444

How was this derived?

sys/dev/sound/pcm/dsp.c

1271

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]

sys/dev/sound/pcm/channel.c

1677

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.

1924

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

439

Yeah good point. I renamed these

444

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

1271

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

[christos]

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

432–433

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.

[kbowling]

sys/dev/sound/pcm/channel.c

1671

removed

1922

That would alter the cadence to the hw.. maybe or maybe not, I haven't gotten that far and don't want to conflate the patch with device level exposure. Clarified comment.

sys/dev/sound/pcm/dsp.c

130–136

Simplified

[christos]

sys/dev/sound/pcm/channel.c

1670–1673

These 2 ifs can be rewritten as:

RANGE(maxsize, CHN_2NDBUFSIZE_MIN, CHN_2NDBUFSIZE_MAX);

But it's fine if you find the macro ugly and prefer to write it explicitly. :-)

sys/dev/sound/pcm/dsp.c

127–129

I don't see a scenario where lw is negative; the ioctls will set it to either arg_i if it's larger than 1, or 1 if not. It can also be set in chn_resizebuf() to bs->blksz, which has checks as well. I guess you have the check for robustness, hence why my recommendation used RANGE.

[kbowling]

sys/dev/sound/pcm/channel.c

1670–1673

replaced with RANGE

sys/dev/sound/pcm/dsp.c

127–129

replaced with RANGE

[christos]

This looks good. The only thing I am still skeptical about are the various casts you've introduced, not sure if they are needed. I will be testing the patch as well.

[christos]

Tested with my HW, no problems. Is there some particular way you are testing this?

[kbowling]

Thanks. I tested this directly on an HDSPe AIO ADAT PCM with vchans disabled and bit-perfect mode enabled. I opened it full-duplex as S32_LE, 8 channels at 48 kHz, then requested SNDCTL_DSP_SETFRAGMENT with 4 KiB fragments and maxfrags = 0. It returned 64 × 4096 bytes. Verbose /dev/sndstat showed [b:131072/1024/128|bs:262144/4096/64] for both playback and capture, confirming that the secondary buffer grew to 256 KiB while the hardware buffer remained unchanged. You need something with enough channel and rate to go beyond the prior limit.

Test configuration:

/dev/dsp5, O_RDWR
AFMT_S32_LE
8 channels
48000 Hz
SETFRAGMENT: maxfrags=0, fragment size=4096
returned: 64 fragments × 4096 = 262144 bytes

Relevant verbose /dev/sndstat capture:

pcm5: <HDSPe AIO [adat]> (1p:0v/1r:0v)
        snddev flags=...<BITPERFECT>

        [dsp5.play.0]: spd 48000, fmt 0x08801000
                [b:131072/1024/128|bs:262144/4096/64]
                {userland} -> feeder_root(0x08801000) -> {hardware}

        [dsp5.record.0]: spd 48000, fmt 0x08801000
                [b:131072/1024/128|bs:262144/4096/64]
                {hardware} -> feeder_root(0x08801000) -> {userland}

This confirms:

- The hardware buffer stayed at 128 KiB.
- The secondary buffer grew to 256 KiB.
- It worked for both playback and capture.
- The path was bit-perfect with no virtual channels or matrix conversion.

Here's a sample test program to open as described:

#include <sys/ioctl.h>
#include <sys/soundcard.h>

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

static void
set_int(int fd, unsigned long request, int *value, const char *name)
{
        if (ioctl(fd, request, value) == -1)
                err(1, "%s", name);
}

int
main(int argc, char **argv)
{
        audio_buf_info info;
        const char *device;
        int channels, fd, format, fragment, rate;

        device = argc > 1 ? argv[1] : "/dev/dsp5";
        fd = open(device, O_RDWR);
        if (fd == -1)
                err(1, "%s", device);

        format = AFMT_S32_LE;
        set_int(fd, SNDCTL_DSP_SETFMT, &format, "SNDCTL_DSP_SETFMT");
        channels = 8;
        set_int(fd, SNDCTL_DSP_CHANNELS, &channels, "SNDCTL_DSP_CHANNELS");
        rate = 48000;
        set_int(fd, SNDCTL_DSP_SPEED, &rate, "SNDCTL_DSP_SPEED");

        /* maxfrags=0 asks for the largest permitted ring; fragshift=12 is 4 KiB. */
        fragment = 12;
        set_int(fd, SNDCTL_DSP_SETFRAGMENT, &fragment,
            "SNDCTL_DSP_SETFRAGMENT");

        printf("device=%s format=%#x channels=%d rate=%d\n",
            device, format, channels, rate);
        printf("SETFRAGMENT returned %#x: %u fragments, %u bytes each\n",
            fragment, (uint32_t)fragment >> 16,
            1U << ((uint32_t)fragment & 0xffff));

        if (ioctl(fd, SNDCTL_DSP_GETOSPACE, &info) == 0)
                printf("GETOSPACE: fragments=%d fragstotal=%d fragsize=%d bytes=%d\n",
                    info.fragments, info.fragstotal, info.fragsize, info.bytes);
        if (ioctl(fd, SNDCTL_DSP_GETISPACE, &info) == 0)
                printf("GETISPACE: fragments=%d fragstotal=%d fragsize=%d bytes=%d\n",
                    info.fragments, info.fragstotal, info.fragsize, info.bytes);

        printf("pid=%d; descriptor is open; press Enter to close\n", getpid());
        fflush(stdout);
        (void)getchar();
        close(fd);
        return (0);
}

[christos]

I don't have a device like that right now. But everything works as expected? Playback, recording, latencies, etc?

[kbowling]

You do not need comparable multichannel hardware if it can do 192 kHz. A native stereo S32_LE device at 192 kHz has the same byte rate as 8-channel S32_LE at 48 kHz, so it should exercise the same scaling. With vchans disabled, try maxfrags = 0 and 4 KiB fragments; /dev/sndstat should show a 256 KiB bs while b remains unchanged.

[kbowling]

Here is a simple self contained 192 kHz stereo test with audio output. No underruns seen on my hw sounds fine.

#include <sys/ioctl.h>
#include <sys/soundcard.h>
#include <sys/time.h>

#include <err.h>
#include <fcntl.h>
#include <math.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

#define RATE            192000
#define CHANNELS        2
#define SECONDS         10
#define AMPLITUDE       0.005
#define TONE_HZ         440.0
#define FRAMES_PER_WRITE 512

static void
set_int(int fd, unsigned long request, int *value, const char *name)
{
        if (ioctl(fd, request, value) == -1)
                err(1, "%s", name);
}

static void
write_all(int fd, const void *buffer, size_t length)
{
        const uint8_t *p;
        ssize_t written;

        p = buffer;
        while (length != 0) {
                written = write(fd, p, length);
                if (written == -1)
                        err(1, "write");
                p += written;
                length -= (size_t)written;
        }
}

int
main(void)
{
        int32_t samples[FRAMES_PER_WRITE * CHANNELS];
        int channels, delay, fd, format, fragment, rate;
        uint64_t frame, frames, i;
        double envelope, phase;

        fd = open("/dev/dsp0", O_WRONLY);
        if (fd == -1)
                err(1, "/dev/dsp0");

        format = AFMT_S32_LE;
        set_int(fd, SNDCTL_DSP_SETFMT, &format, "SNDCTL_DSP_SETFMT");
        channels = CHANNELS;
        set_int(fd, SNDCTL_DSP_CHANNELS, &channels, "SNDCTL_DSP_CHANNELS");
        rate = RATE;
        set_int(fd, SNDCTL_DSP_SPEED, &rate, "SNDCTL_DSP_SPEED");

        /* maxfrags=0, fragshift=12 (4096 bytes). */
        fragment = 12;
        set_int(fd, SNDCTL_DSP_SETFRAGMENT, &fragment,
            "SNDCTL_DSP_SETFRAGMENT");

        printf("format=%#x channels=%d rate=%d\n", format, channels, rate);
        printf("SETFRAGMENT=%#x: %u fragments x %u bytes = %u bytes\n",
            fragment, (uint32_t)fragment >> 16,
            1U << ((uint32_t)fragment & 0xffff),
            ((uint32_t)fragment >> 16) *
            (1U << ((uint32_t)fragment & 0xffff)));
        printf("tone=%.1f Hz amplitude=%.4f duration=%d seconds\n",
            TONE_HZ, AMPLITUDE, SECONDS);
        printf("press Enter to start\n");
        fflush(stdout);
        (void)getchar();

        frames = (uint64_t)rate * SECONDS;
        for (frame = 0; frame < frames; frame += FRAMES_PER_WRITE) {
                uint64_t batch;

                batch = frames - frame;
                if (batch > FRAMES_PER_WRITE)
                        batch = FRAMES_PER_WRITE;
                for (i = 0; i < batch; i++) {
                        uint64_t n;
                        int32_t sample;

                        n = frame + i;
                        envelope = 1.0;
                        if (n < (uint64_t)rate / 10)
                                envelope = (double)n / ((double)rate / 10.0);
                        if (frames - n < (uint64_t)rate / 10)
                                envelope = (double)(frames - n) /
                                    ((double)rate / 10.0);
                        phase = 2.0 * M_PI * TONE_HZ * (double)n /
                            (double)rate;
                        sample = (int32_t)(sin(phase) * envelope *
                            AMPLITUDE * INT32_MAX);
                        samples[i * 2] = sample;
                        samples[i * 2 + 1] = sample;
                }
                write_all(fd, samples, batch * CHANNELS * sizeof(samples[0]));
        }

        if (ioctl(fd, SNDCTL_DSP_GETODELAY, &delay) == 0)
                printf("queued before drain: %d bytes (%.2f ms)\n", delay,
                    1000.0 * delay /
                    (rate * CHANNELS * (double)sizeof(samples[0])));
        if (ioctl(fd, SNDCTL_DSP_SYNC, NULL) == -1)
                err(1, "SNDCTL_DSP_SYNC");
        printf("playback and drain completed\n");
        close(fd);
        return (0);
}