On 18/08/19(Sun) 08:02, Alexandre Ratchov wrote:
> Currently the block size calculations are broken by design: the driver
> provides a round_blocksize() function which must retrun a valid block
> size in *bytes*. Unfortunately, since the driver doesn't know if it's
> called for the play or for the record block size, it's mathematically
> impossible to calculate the block size in all cases if play and record
> number of channels are different. As a consequence, there are
> half-working and weired hacks to find a usable block sizes.
>
> The diff below addresses this by adding two new driver functions,
> which are very simple to use:
>
> set_blksz() - calculate and set the block size in *frames*, it's
> necessarily common to play and recording directions no matter
> the number of channels,
>
> set_nblks() - calculate the number of blocks per buffer for the given
> direction.
>
> the diff below shows how to properly calculate the block size in
> azalia and uaudio. The plan is to convert all drivers from
> round_blocksize() to the new functions and to delete
> round_blocksize().
>
> Why is this important? besides for removing ugly (and risky) hacks, we
> want all our drivers to support common block sizes in the 5ms-50ms
> range. This would allow to implement switching between audio devices:
> for instance, start playback on a USB device, unplug the cable and
> continue on azalia.
While the cleanup in itself makes sense to me. I'm unsure if continuing
to play on a secondary audio device is what we want. Nowadays phones
seems to stop music players if an audio device is disconnected.
Let's assume I'm in a hackathon hearing music via a USB device, if I
unplug it, accidentally or not, I'd find more logical that the player
stop instead of forcing the whole room to listen my music.
When gilles@ and/or jcs@ will be ready with their Bluetooth code, we
could do the same there :o)
> OK?
Diff is ok with me, if you think it makes sense to do this change anyway
:)
>
> Index: share/man/man9/audio.9
> ===================================================================
> RCS file: /cvs/src/share/man/man9/audio.9,v
> retrieving revision 1.27
> diff -u -p -r1.27 audio.9
> --- share/man/man9/audio.9 12 Mar 2019 08:18:34 -0000 1.27
> +++ share/man/man9/audio.9 18 Aug 2019 05:28:35 -0000
> @@ -82,6 +82,10 @@ struct audio_hw_if {
> void (*)(void *), void *, struct audio_params *);
> void (*copy_output)(void *hdl, size_t bytes);
> void (*underrun)(void *hdl);
> + int (*set_blksz)(void *, int,
> + struct audio_params *, struct audio_params *, int);
> + int (*set_nblks)(void *, int, int,
> + struct audio_params *, int);
> };
>
> struct audio_params {
> @@ -417,6 +421,56 @@ Drivers using bounce buffers for transfe
> ring buffer and the device must implement this method to skip
> one block from the audio ring buffer and transfer the
> corresponding amount of silence to the device.
> +.It Fn "int (*set_blksz)" "void *hdl" "int mode" \
> +"struct audio_params *play" "struct audio_params *rec" "int blksz"
> +This function is called to set the audio block size.
> +.Fa mode
> +is a combination of the
> +.Dv AUMODE_RECORD
> +and
> +.Dv AUMODE_PLAY
> +flags indicating the current mode set with the
> +.Fn open
> +function.
> +The
> +.Fa play
> +and
> +.Fa rec
> +structures contain the current encoding set with the
> +.Fn set_params
> +function.
> +.Fa blksz
> +is the desired block size in frames.
> +It may be adjusted to match hardware constraints.
> +This function returns the adjusted block size.
> +.It Fn "int (*set_nblks)" "void *hdl" "int dir" "int blksz" \
> +"struct audio_params *params" "int nblks"
> +This function is called to set the number of audio blocks in
> +the ring buffer.
> +.Fa dir
> +is either the
> +.Dv AUMODE_RECORD
> +or the
> +.Dv AUMODE_PLAY
> +flag, indicating which ring buffer size is set.
> +The
> +.Fa params
> +structure contains the encoding parameters set by the
> +.Fn set_params
> +method.
> +.Fa blksz
> +is the current block size in frames set with the
> +.Fa set_params
> +function.
> +The
> +.Fa params
> +structure is the current encoding parameters, set with the
> +.Fn set_params
> +function.
> +.Fa nblks
> +is the desired number of blocks in the ring buffer.
> +It may be lowered to at least two, to match hardware constraints.
> +This function returns the adjusted number of blocks.
> .El
> .Pp
> If the audio hardware is capable of input from more
> Index: sys/dev/audio.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/audio.c,v
> retrieving revision 1.180
> diff -u -p -r1.180 audio.c
> --- sys/dev/audio.c 17 Aug 2019 05:04:56 -0000 1.180
> +++ sys/dev/audio.c 18 Aug 2019 05:28:36 -0000
> @@ -193,6 +193,31 @@ audio_gcd(unsigned int a, unsigned int b
> return a;
> }
>
> +/*
> + * Calculate the least block size (in frames) such that both the
> + * corresponding play and/or record block sizes (in bytes) are multiple
> + * of the given number of bytes.
> + */
> +int
> +audio_blksz_bytes(int mode,
> + struct audio_params *p, struct audio_params *r, int bytes)
> +{
> + unsigned int np, nr;
> +
> + if (mode & AUMODE_PLAY) {
> + np = bytes / audio_gcd(p->bps * p->channels, bytes);
> + if (!(mode & AUMODE_RECORD))
> + nr = np;
> + }
> + if (mode & AUMODE_RECORD) {
> + nr = bytes / audio_gcd(r->bps * r->channels, bytes);
> + if (!(mode & AUMODE_PLAY))
> + np = nr;
> + }
> +
> + return nr * np / audio_gcd(nr, np);
> +}
> +
> int
> audio_buf_init(struct audio_softc *sc, struct audio_buf *buf, int dir)
> {
> @@ -625,11 +650,19 @@ audio_canstart(struct audio_softc *sc)
> }
>
> int
> -audio_setpar_blksz(struct audio_softc *sc)
> +audio_setpar_blksz(struct audio_softc *sc,
> + struct audio_params *p, struct audio_params *r)
> {
> unsigned int nr, np, max, min, mult;
> unsigned int blk_mult, blk_max;
>
> + if (sc->ops->set_blksz) {
> + sc->round = sc->ops->set_blksz(sc->arg, sc->mode,
> + p, r, sc->round);
> + DPRINTF("%s: block size set to: %u\n", DEVNAME(sc), sc->round);
> + return 0;
> + }
> +
> /*
> * get least multiplier of the number of frames per block
> */
> @@ -706,7 +739,8 @@ audio_setpar_blksz(struct audio_softc *s
> }
>
> int
> -audio_setpar_nblks(struct audio_softc *sc)
> +audio_setpar_nblks(struct audio_softc *sc,
> + struct audio_params *p, struct audio_params *r)
> {
> unsigned int max;
>
> @@ -719,6 +753,12 @@ audio_setpar_nblks(struct audio_softc *s
> sc->play.nblks = max;
> else if (sc->play.nblks < 2)
> sc->play.nblks = 2;
> + if (sc->ops->set_nblks) {
> + sc->play.nblks = sc->ops->set_nblks(sc->arg, sc->mode,
> + p, sc->round, sc->play.nblks);
> + DPRINTF("%s: play nblks -> %u\n", DEVNAME(sc),
> + sc->play.nblks);
> + }
> }
> if (sc->mode & AUMODE_RECORD) {
> /*
> @@ -727,6 +767,11 @@ audio_setpar_nblks(struct audio_softc *s
> * size of maximum reliability during xruns
> */
> max = sc->rec.datalen / (sc->round * sc->rchan * sc->bps);
> + if (sc->ops->set_nblks) {
> + max = sc->ops->set_nblks(sc->arg, sc->mode,
> + r, sc->round, max);
> + DPRINTF("%s: rec nblks -> %u\n", DEVNAME(sc), max);
> + }
> sc->rec.nblks = max;
> }
> return 0;
> @@ -874,11 +919,11 @@ audio_setpar(struct audio_softc *sc)
> }
> audio_calc_sil(sc);
>
> - error = audio_setpar_blksz(sc);
> + error = audio_setpar_blksz(sc, &p, &r);
> if (error)
> return error;
>
> - error = audio_setpar_nblks(sc);
> + error = audio_setpar_nblks(sc, &p, &r);
> if (error)
> return error;
>
> Index: sys/dev/audio_if.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/audio_if.h,v
> retrieving revision 1.35
> diff -u -p -r1.35 audio_if.h
> --- sys/dev/audio_if.h 12 Mar 2019 08:16:29 -0000 1.35
> +++ sys/dev/audio_if.h 18 Aug 2019 05:28:36 -0000
> @@ -133,6 +133,10 @@ struct audio_hw_if {
> void (*)(void *), void *, struct audio_params *);
> void (*copy_output)(void *, size_t);
> void (*underrun)(void *);
> + unsigned int (*set_blksz)(void *, int,
> + struct audio_params *, struct audio_params *, unsigned int);
> + unsigned int (*set_nblks)(void *, int,
> + struct audio_params *, unsigned int, unsigned int);
> };
>
> struct audio_attach_args {
> @@ -149,6 +153,8 @@ struct audio_attach_args {
> /* Attach the MI driver(s) to the MD driver. */
> struct device *audio_attach_mi(struct audio_hw_if *, void *, struct device
> *);
> int audioprint(void *, const char *);
> +int audio_blksz_bytes(int,
> + struct audio_params *, struct audio_params *, int);
>
> extern struct mutex audio_lock;
>
> Index: sys/dev/pci/azalia.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/azalia.c,v
> retrieving revision 1.250
> diff -u -p -r1.250 azalia.c
> --- sys/dev/pci/azalia.c 13 Aug 2019 15:28:12 -0000 1.250
> +++ sys/dev/pci/azalia.c 18 Aug 2019 05:28:36 -0000
> @@ -252,7 +252,10 @@ int azalia_open(void *, int);
> void azalia_close(void *);
> int azalia_set_params(void *, int, int, audio_params_t *,
> audio_params_t *);
> -int azalia_round_blocksize(void *, int);
> +unsigned int azalia_set_blksz(void *, int,
> + struct audio_params *, struct audio_params *, unsigned int);
> +unsigned int azalia_set_nblks(void *, int,
> + struct audio_params *, unsigned int, unsigned int);
> int azalia_halt_output(void *);
> int azalia_halt_input(void *);
> int azalia_set_port(void *, mixer_ctrl_t *);
> @@ -290,7 +293,7 @@ struct audio_hw_if azalia_hw_if = {
> azalia_open,
> azalia_close,
> azalia_set_params,
> - azalia_round_blocksize,
> + NULL, /* round_blocksize */
> NULL, /* commit_settings */
> NULL, /* init_output */
> NULL, /* init_input */
> @@ -308,7 +311,11 @@ struct audio_hw_if azalia_hw_if = {
> azalia_round_buffersize,
> azalia_get_props,
> azalia_trigger_output,
> - azalia_trigger_input
> + azalia_trigger_input,
> + NULL, /* copy_output */
> + NULL, /* underrun */
> + azalia_set_blksz,
> + azalia_set_nblks
> };
>
> static const char *pin_devices[16] = {
> @@ -3962,25 +3969,30 @@ azalia_set_params(void *v, int smode, in
> return (0);
> }
>
> -int
> -azalia_round_blocksize(void *v, int blk)
> +unsigned int
> +azalia_set_blksz(void *v, int mode,
> + struct audio_params *p, struct audio_params *r, unsigned int blksz)
> {
> - azalia_t *az;
> - size_t size;
> + int mult;
> +
> + /* must be multiple of 128 bytes */
> + mult = audio_blksz_bytes(mode, p, r, 128);
> +
> + blksz += mult - 1;
> + blksz -= blksz % mult;
> +
> + return blksz;
> +}
>
> - blk &= ~0x7f; /* must be multiple of 128 */
> - if (blk <= 0)
> - blk = 128;
> +unsigned int
> +azalia_set_nblks(void *v, int mode,
> + struct audio_params *params, unsigned int blksz, unsigned int nblks)
> +{
> /* number of blocks must be <= HDA_BDL_MAX */
> - az = v;
> - size = az->pstream.buffer.size;
> - if (size > HDA_BDL_MAX * blk) {
> - blk = size / HDA_BDL_MAX;
> - if (blk & 0x7f)
> - blk = (blk + 0x7f) & ~0x7f;
> - }
> - DPRINTFN(1,("%s: resultant block size = %d\n", __func__, blk));
> - return blk;
> + if (nblks > HDA_BDL_MAX)
> + nblks = HDA_BDL_MAX;
> +
> + return nblks;
> }
>
> int
> Index: sys/dev/usb/uaudio.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uaudio.c,v
> retrieving revision 1.144
> diff -u -p -r1.144 uaudio.c
> --- sys/dev/usb/uaudio.c 9 May 2019 07:09:04 -0000 1.144
> +++ sys/dev/usb/uaudio.c 18 Aug 2019 05:28:37 -0000
> @@ -403,7 +403,8 @@ int uaudio_open(void *, int);
> void uaudio_close(void *);
> int uaudio_set_params(void *, int, int, struct audio_params *,
> struct audio_params *);
> -int uaudio_round_blocksize(void *, int);
> +unsigned int uaudio_set_blksz(void *, int,
> + struct audio_params *, struct audio_params *, unsigned int);
> int uaudio_trigger_output(void *, void *, void *, int,
> void (*)(void *), void *, struct audio_params *);
> int uaudio_trigger_input(void *, void *, void *, int,
> @@ -457,7 +458,7 @@ struct audio_hw_if uaudio_hw_if = {
> uaudio_open, /* open */
> uaudio_close, /* close */
> uaudio_set_params, /* set_params */
> - uaudio_round_blocksize, /* round_blocksize */
> + NULL, /* round_blocksize */
> NULL, /* commit_settings */
> NULL, /* init_output */
> NULL, /* init_input */
> @@ -477,7 +478,8 @@ struct audio_hw_if uaudio_hw_if = {
> uaudio_trigger_output, /* trigger_output */
> uaudio_trigger_input, /* trigger_input */
> uaudio_copy_output, /* copy_output */
> - uaudio_underrun /* underrun */
> + uaudio_underrun, /* underrun */
> + uaudio_set_blksz /* set_blksz */
> };
>
> /*
> @@ -3962,57 +3964,43 @@ uaudio_set_params(void *self, int setmod
> return 0;
> }
>
> -int
> -uaudio_round_blocksize(void *self, int blksz)
> +unsigned int
> +uaudio_set_blksz(void *self, int mode,
> + struct audio_params *p, struct audio_params *r, unsigned int blksz)
> {
> struct uaudio_softc *sc = self;
> - struct uaudio_alt *a;
> - unsigned int rbpf, pbpf;
> - unsigned int blksz_max;
> + unsigned int fps, fps_min;
> + unsigned int blksz_max, blksz_min;
>
> /*
> - * XXX: We don't know if we're called for the play or record
> - * direction, so we can't calculate maximum blksz. This would
> - * require a change in the audio(9) interface. Meanwhile, we
> - * use the direction with the greatest sample size; it gives
> - * the correct result: indeed, if we return:
> - *
> - * blksz_max = max(pbpf, rbpf) * nsamp_max
> - *
> - * in turn the audio(4) layer will use:
> - *
> - * min(blksz_max / pbpf, blksz_max / rbpf)
> - *
> - * which is exactly nsamp_max.
> + * minimum block size is two transfers, see uaudio_stream_open()
> */
> -
> - if (sc->mode & AUMODE_PLAY) {
> - a = sc->params->palt;
> - pbpf = a->bps * a->nch;
> - } else
> - pbpf = 1;
> -
> - if (sc->mode & AUMODE_RECORD) {
> - a = sc->params->ralt;
> - rbpf = a->bps * a->nch;
> - } else
> - rbpf = 1;
> + fps_min = sc->ufps;
> + if (mode & AUMODE_PLAY) {
> + fps = sc->params->palt->fps;
> + if (fps_min > fps)
> + fps_min = fps;
> + }
> + if (mode & AUMODE_RECORD) {
> + fps = sc->params->ralt->fps;
> + if (fps_min > fps)
> + fps_min = fps;
> + }
> + blksz_min = (sc->rate * 2 + fps_min - 1) / fps_min;
>
> /*
> - * Limit the block size to (slightly more than):
> - *
> - * sc->host_nframes / UAUDIO_NXFERS_MIN
> - *
> - * (micro-)frames of audio. Transfers are slightly larger than
> - * the audio block size (few bytes to make the "safe" block
> - * size plus one extra millisecond). We reserve an extra 15%
> - * for that.
> - */
> - blksz_max = (pbpf > rbpf ? pbpf : rbpf) *
> - sc->rate * (sc->host_nframes / UAUDIO_NXFERS_MIN) / sc->ufps *
> - 85 / 100;
> + * max block size is only limited by the number of frames the
> + * host can schedule
> + */
> + blksz_max = sc->rate * (sc->host_nframes / UAUDIO_NXFERS_MIN) /
> + sc->ufps * 85 / 100;
> +
> + if (blksz > blksz_max)
> + blksz = blksz_max;
> + else if (blksz < blksz_min)
> + blksz = blksz_min;
>
> - return blksz < blksz_max ? blksz : blksz_max;
> + return blksz;
> }
>
> int
>
