audio: fix block size calculation

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

audio: fix block size calculation

Alexandre Ratchov-2
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.

OK?

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

Reply | Threaded
Open this post in threaded view
|

Re: audio: fix block size calculation

Martin Pieuchot
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
>

Reply | Threaded
Open this post in threaded view
|

Re: audio: fix block size calculation

Alexandre Ratchov-2
On Tue, Sep 03, 2019 at 02:03:52PM -0300, Martin Pieuchot wrote:

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

I totally agree, making this the default would be a mistake as it's
against the "least surprise" principle. It seems practical in few
specific cases though: for instance I use it to temporarilly connect a
usb headset in order to answer a phone call in firefox without the
need to restart sndiod and firefox and then call back the other
person.

> > OK?
>
> Diff is ok with me, if you think it makes sense to do this change anyway
> :)
>

Sure, the change is very useful.