Date: Mon, 27 Jul 2026 01:08:45 +0300 From: "Christos Margiolis" <christos@FreeBSD.org> To: "Ronald Klop" <ronald-lists@klop.ws> Cc: <dev-commits-src-all@FreeBSD.org>, <src-committers@FreeBSD.org>, "Kevin Bowling" <kbowling@FreeBSD.org>, <dev-commits-src-main@FreeBSD.org> Subject: Re: git: 967e86d1ef2a - main - sound: Scale PCM secondary buffers by byte rate Message-ID: <DK8UGIJQ1GO9.2XPOWIG8HM79G@FreeBSD.org> In-Reply-To: <20302724.1251.1784904383819@localhost> References: <6a636fb1.26412.26a07c1d@gitrepo.freebsd.org> <20302724.1251.1784904383819@localhost>
index | next in thread | previous in thread | raw e-mail
On Fri Jul 24, 2026 at 5:46 PM EEST, Ronald Klop wrote: > Is this a good candidate for > Relnotes: yes > in the commit message? I don't think so to be honest. > Regards, > Ronald. > > > > Van: Christos Margiolis <christos@FreeBSD.org> > Datum: vrijdag, 24 juli 2026 15:59 > Aan: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org > CC: Kevin Bowling <kbowling@FreeBSD.org> > Onderwerp: git: 967e86d1ef2a - main - sound: Scale PCM secondary buffers by byte rate >> >> The branch main has been updated by christos: >> >> URL: https://cgit.FreeBSD.org/src/commit/?id=967e86d1ef2ac8711c0ae7be353a9c08186f4e6f >> >> commit 967e86d1ef2ac8711c0ae7be353a9c08186f4e6f >> Author: Kevin Bowling <kbowling@FreeBSD.org> >> AuthorDate: 2026-07-24 13:57:27 +0000 >> Commit: Christos Margiolis <christos@FreeBSD.org> >> CommitDate: 2026-07-24 13:58:14 +0000 >> >> sound: Scale PCM secondary buffers by byte rate >> >> 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. >> >> MFC after: 3 weeks >> Reviewed by: christos >> Differential Revision: https://reviews.freebsd.org/D58064 >> --- >> sys/dev/sound/pcm/channel.c | 62 +++++++++++++++++++++++++++++++++------------ >> sys/dev/sound/pcm/channel.h | 30 +++++++++++++++++----- >> sys/dev/sound/pcm/dsp.c | 38 +++++++++++++++++++-------- >> 3 files changed, 96 insertions(+), 34 deletions(-) >> >> diff --git a/sys/dev/sound/pcm/channel.c b/sys/dev/sound/pcm/channel.c >> index ff7620c772c3..21c223c61205 100644 >> --- a/sys/dev/sound/pcm/channel.c >> +++ b/sys/dev/sound/pcm/channel.c >> @@ -1657,15 +1657,30 @@ round_pow2(u_int32_t v) >> return ret; >> } >> >> +u_int32_t >> +chn_2ndbufmaxsize(struct pcm_channel *c) >> +{ >> + struct snd_dbuf *bs; >> + uint64_t maxsize; >> + >> + CHN_LOCKASSERT(c); >> + >> + bs = c->bufsoft; >> + maxsize = (uint64_t)bs->align * bs->spd * CHN_2NDBUFTIME_MS / 1000; >> + RANGE(maxsize, CHN_2NDBUFSIZE_MIN, CHN_2NDBUFSIZE_MAX); >> + >> + return ((u_int32_t)maxsize); >> +} >> + >> static u_int32_t >> -round_blksz(u_int32_t v, int round) >> +round_blksz(u_int32_t v, int round, u_int32_t maxsize) >> { >> u_int32_t ret, tmp; >> >> if (round < 1) >> round = 1; >> >> - ret = min(round_pow2(v), CHN_2NDBUFMAXSIZE >> 1); >> + ret = min(round_pow2(v), maxsize >> 1); >> >> if (ret > v && (ret >> 1) > 0 && (ret >> 1) >= ((v * 3) >> 2)) >> ret >>= 1; >> @@ -1780,14 +1795,16 @@ chn_calclatency(int dir, int latency, int bps, u_int32_t datarate, >> if (latency < CHN_LATENCY_MIN || latency > CHN_LATENCY_MAX || >> bps < 1 || datarate < 1 || >> !(dir == PCMDIR_PLAY || dir == PCMDIR_REC)) { >> + if (max < CHN_2NDBUFSIZE_MIN) >> + max = CHN_2NDBUFSIZE_MIN; >> if (rblksz != NULL) >> - *rblksz = CHN_2NDBUFMAXSIZE >> 1; >> + *rblksz = max >> 1; >> if (rblkcnt != NULL) >> *rblkcnt = 2; >> printf("%s(): FAILED dir=%d latency=%d bps=%d " >> "datarate=%u max=%u\n", >> __func__, dir, latency, bps, datarate, max); >> - return CHN_2NDBUFMAXSIZE; >> + return max; >> } >> >> lprofile = chn_latency_profile; >> @@ -1804,7 +1821,7 @@ chn_calclatency(int dir, int latency, int bps, u_int32_t datarate, >> datarate)); >> if (bufsz > max) >> bufsz = max; >> - blksz = round_blksz(bufsz >> blkcnt, bps); >> + blksz = round_blksz(bufsz >> blkcnt, bps, max); >> >> if (rblksz != NULL) >> *rblksz = blksz; >> @@ -1821,6 +1838,7 @@ chn_resizebuf(struct pcm_channel *c, int latency, >> struct snd_dbuf *b, *bs, *pb; >> int sblksz, sblkcnt, hblksz, hblkcnt, limit = 0, nsblksz, nsblkcnt; >> int ret; >> + u_int32_t maxsize; >> >> CHN_LOCKASSERT(c); >> >> @@ -1843,14 +1861,15 @@ chn_resizebuf(struct pcm_channel *c, int latency, >> >> bs = c->bufsoft; >> b = c->bufhard; >> + maxsize = chn_2ndbufmaxsize(c); >> >> if (!(blksz == 0 || blkcnt == -1) && >> (blksz < 16 || blksz < bs->align || blkcnt < 2 || >> - (blksz * blkcnt) > CHN_2NDBUFMAXSIZE)) >> + (uint64_t)blksz * blkcnt > maxsize)) >> return EINVAL; >> >> chn_calclatency(c->direction, latency, bs->align, >> - bs->align * bs->spd, CHN_2NDBUFMAXSIZE, >> + bs->align * bs->spd, maxsize, >> &sblksz, &sblkcnt); >> >> if (blksz == 0 || blkcnt == -1) { >> @@ -1871,7 +1890,7 @@ chn_resizebuf(struct pcm_channel *c, int latency, >> * defeat the purpose of having custom control. The least >> * we can do is round it to the nearest ^2 and align it. >> */ >> - sblksz = round_blksz(blksz, bs->align); >> + sblksz = round_blksz(blksz, bs->align, maxsize); >> sblkcnt = round_pow2(blkcnt); >> } >> >> @@ -1890,18 +1909,28 @@ chn_resizebuf(struct pcm_channel *c, int latency, >> sndbuf_xbytes(pb->blksz, pb, bs) * 2 : 0; >> } >> } else { >> + /* >> + * The byte-rate-scaled cap applies to the secondary buffer >> + * only. It exists to absorb userland scheduling latency, >> + * which the secondary buffer alone must cover; hardware >> + * buffer geometry keeps the historical cap, since enlarging >> + * it would change fragment sizes and interrupt cadence >> + * visible to drivers, and remains bounded by b->maxsize >> + * below. >> + */ >> hblkcnt = 2; >> if (c->flags & CHN_F_HAS_SIZE) { >> hblksz = round_blksz(sndbuf_xbytes(sblksz, bs, b), >> - b->align); >> + b->align, CHN_2NDBUFSIZE_MIN); >> hblkcnt = round_pow2(bs->blkcnt); >> } else >> chn_calclatency(c->direction, latency, >> b->align, b->align * b->spd, >> - CHN_2NDBUFMAXSIZE, &hblksz, &hblkcnt); >> + CHN_2NDBUFSIZE_MIN, &hblksz, &hblkcnt); >> >> if ((hblksz << 1) > b->maxsize) >> - hblksz = round_blksz(b->maxsize >> 1, b->align); >> + hblksz = round_blksz(b->maxsize >> 1, b->align, >> + CHN_2NDBUFSIZE_MIN); >> >> while ((hblksz * hblkcnt) > b->maxsize) { >> if (hblkcnt < 4) >> @@ -1922,7 +1951,8 @@ chn_resizebuf(struct pcm_channel *c, int latency, >> >> if (!CHN_EMPTY(c, children)) { >> nsblksz = round_blksz( >> - sndbuf_xbytes(b->blksz, b, bs), bs->align); >> + sndbuf_xbytes(b->blksz, b, bs), bs->align, >> + maxsize); >> nsblkcnt = b->blkcnt; >> if (c->direction == PCMDIR_PLAY) { >> do { >> @@ -1938,13 +1968,13 @@ chn_resizebuf(struct pcm_channel *c, int latency, >> limit = sndbuf_xbytes(b->blksz, b, bs) * 2; >> } >> >> - if (limit > CHN_2NDBUFMAXSIZE) >> - limit = CHN_2NDBUFMAXSIZE; >> + if ((u_int32_t)limit > maxsize) >> + limit = maxsize; >> >> - while ((sblksz * sblkcnt) < limit) >> + while ((uint64_t)sblksz * sblkcnt < (uint64_t)limit) >> sblkcnt <<= 1; >> >> - while ((sblksz * sblkcnt) > CHN_2NDBUFMAXSIZE) { >> + while ((uint64_t)sblksz * sblkcnt > maxsize) { >> if (sblkcnt < 4) >> sblksz >>= 1; >> else >> diff --git a/sys/dev/sound/pcm/channel.h b/sys/dev/sound/pcm/channel.h >> index c7f5bf93b8e5..c78c25dac13b 100644 >> --- a/sys/dev/sound/pcm/channel.h >> +++ b/sys/dev/sound/pcm/channel.h >> @@ -430,15 +430,31 @@ enum { >> #define CHN_TIMEOUT_MIN 1 >> #define CHN_TIMEOUT_MAX 10 >> >> -/* >> - * This should be large enough to hold all pcm data between >> - * tsleeps in chn_{read,write} at the highest sample rate. >> - * (which is usually 48kHz * 16bit * stereo = 192000 bytes/sec) >> - */ >> +/* Default block size for the secondary buffer. */ >> #define CHN_2NDBUFBLKSIZE (2 * 1024) >> /* The total number of blocks per secondary bufhard. */ >> #define CHN_2NDBUFBLKNUM (32) >> -/* The size of a whole secondary bufhard. */ >> -#define CHN_2NDBUFMAXSIZE (131072) >> +/* >> + * The secondary buffer cap scales with the channel byte rate, targeting >> + * CHN_2NDBUFTIME_MS of stream so that all pcm data between tsleeps in >> + * chn_{read,write} fits, clamped to [CHN_2NDBUFSIZE_MIN, >> + * CHN_2NDBUFSIZE_MAX]. >> + * >> + * The floor is the historical secondary buffer size and preserves memory >> + * use for low byte-rate streams; it holds ~680 ms at the once-typical >> + * 48kHz * 16bit * stereo rate (192000 bytes/sec), well above the target >> + * (~38 KiB there). >> + * >> + * The ceiling bounds per-channel buffer memory. Buffers are allocated at >> + * the derived size, so only streams that actually run at high byte rates >> + * approach it. It holds the full target through MADI-class streams >> + * (64ch * 32-bit * 48kHz, ~12.3 MB/s); beyond that, coverage shrinks >> + * proportionally (e.g. ~85 ms at 64ch * 32-bit * 192kHz). >> + */ >> +#define CHN_2NDBUFSIZE_MIN (131072) >> +#define CHN_2NDBUFSIZE_MAX (4 * 1024 * 1024) >> +#define CHN_2NDBUFTIME_MS 200 >> + >> +u_int32_t chn_2ndbufmaxsize(struct pcm_channel *); >> >> #define CHANNEL_DECLARE(name) static DEFINE_CLASS(name, name ## _methods, sizeof(struct kobj)) >> diff --git a/sys/dev/sound/pcm/dsp.c b/sys/dev/sound/pcm/dsp.c >> index 0a5063410d24..f49df6e8a8c6 100644 >> --- a/sys/dev/sound/pcm/dsp.c >> +++ b/sys/dev/sound/pcm/dsp.c >> @@ -109,6 +109,25 @@ static int dsp_oss_setsong(struct pcm_channel *wrch, struct pcm_channel *rdch, o >> static int dsp_oss_setname(struct pcm_channel *wrch, struct pcm_channel *rdch, oss_longname_t *name); >> #endif >> >> +static uint32_t >> +dsp_clamp_fragments(uint32_t maxfrags, uint32_t fragsz, uint32_t maxsize) >> +{ >> + if (maxfrags == 0) >> + maxfrags = maxsize / fragsz; >> + if (maxfrags < 2) >> + maxfrags = 2; >> + if ((uint64_t)maxfrags * fragsz > maxsize) >> + maxfrags = maxsize / fragsz; >> + return (maxfrags); >> +} >> + >> +static unsigned int >> +dsp_low_water(struct pcm_channel *ch, int lw) >> +{ >> + RANGE(lw, 1, ch->bufsoft->bufsize); >> + return ((unsigned int)lw); >> +} >> + >> int >> dsp_make_dev(device_t dev) >> { >> @@ -1244,18 +1263,13 @@ dsp_ioctl(struct cdev *i_dev, u_long cmd, caddr_t arg, int mode, >> RANGE(fragln, 4, 16); >> fragsz = 1 << fragln; >> >> - if (maxfrags == 0) >> - maxfrags = CHN_2NDBUFMAXSIZE / fragsz; >> - if (maxfrags < 2) >> - maxfrags = 2; >> - if (maxfrags * fragsz > CHN_2NDBUFMAXSIZE) >> - maxfrags = CHN_2NDBUFMAXSIZE / fragsz; >> - >> DEB(printf("SNDCTL_DSP_SETFRAGMENT %d frags, %d sz\n", maxfrags, fragsz)); >> PCM_ACQUIRE_QUICK(d); >> if (rdch) { >> CHN_LOCK(rdch); >> - ret = chn_setblocksize(rdch, maxfrags, fragsz); >> + ret = chn_setblocksize(rdch, >> + dsp_clamp_fragments(maxfrags, fragsz, >> + chn_2ndbufmaxsize(rdch)), fragsz); >> r_maxfrags = rdch->bufsoft->blkcnt; >> r_fragsz = rdch->bufsoft->blksz; >> CHN_UNLOCK(rdch); >> @@ -1265,7 +1279,9 @@ dsp_ioctl(struct cdev *i_dev, u_long cmd, caddr_t arg, int mode, >> } >> if (wrch && ret == 0) { >> CHN_LOCK(wrch); >> - ret = chn_setblocksize(wrch, maxfrags, fragsz); >> + ret = chn_setblocksize(wrch, >> + dsp_clamp_fragments(maxfrags, fragsz, >> + chn_2ndbufmaxsize(wrch)), fragsz); >> maxfrags = wrch->bufsoft->blkcnt; >> fragsz = wrch->bufsoft->blksz; >> CHN_UNLOCK(wrch); >> @@ -1653,12 +1669,12 @@ dsp_ioctl(struct cdev *i_dev, u_long cmd, caddr_t arg, int mode, >> */ >> if (wrch != NULL) { >> CHN_LOCK(wrch); >> - wrch->lw = (*arg_i > 1) ? *arg_i : 1; >> + wrch->lw = dsp_low_water(wrch, *arg_i); >> CHN_UNLOCK(wrch); >> } >> if (rdch != NULL) { >> CHN_LOCK(rdch); >> - rdch->lw = (*arg_i > 1) ? *arg_i : 1; >> + rdch->lw = dsp_low_water(rdch, *arg_i); >> CHN_UNLOCK(rdch); >> } >> break; >> >> >> >> > >home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?DK8UGIJQ1GO9.2XPOWIG8HM79G>