| Message ID | 20230517174256.3657060-6-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/9] ALSA: emu10k1: pass frame instead of byte addresses | expand |
On Wed, 17 May 2023 19:42:53 +0200, Oswald Buddenhagen wrote: > > The period_bytes_min parameter made no sense at all, as it didn't > correlate with any hardware limitation. Does the device really work with less than 64 bytes period size? I meant not in theory but in practice. Without any value set, dumb applications may try to set 4 bytes for period size, so setting some practical limit still makes sense. Takashi > The same can be said of the buffer_bytes minimum constraint. > Instead, apply a buffer_size constraint, which works on frames. > > Sync up the constraints of the EFX playback with those of the regular > playback, as there is no reason for them to diverge. > > Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> > --- > sound/pci/emu10k1/emupcm.c | 38 ++++++++++++++++++++++++++------------ > 1 file changed, 26 insertions(+), 12 deletions(-) > > diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c > index feb575922738..5226f0978408 100644 > --- a/sound/pci/emu10k1/emupcm.c > +++ b/sound/pci/emu10k1/emupcm.c > @@ -457,9 +457,8 @@ static const struct snd_pcm_hardware snd_emu10k1_efx_playback = > .rate_max = 48000, > .channels_min = NUM_EFX_PLAYBACK, > .channels_max = NUM_EFX_PLAYBACK, > - .buffer_bytes_max = (64*1024), > - .period_bytes_min = 64, > - .period_bytes_max = (64*1024), > + .buffer_bytes_max = (128*1024), > + .period_bytes_max = (128*1024), > .periods_min = 2, > .periods_max = 2, > .fifo_size = 0, > @@ -851,7 +850,6 @@ static const struct snd_pcm_hardware snd_emu10k1_playback = > .channels_min = 1, > .channels_max = 2, > .buffer_bytes_max = (128*1024), > - .period_bytes_min = 64, > .period_bytes_max = (128*1024), > .periods_min = 1, > .periods_max = 1024, > @@ -956,25 +954,46 @@ static int snd_emu10k1_efx_playback_close(struct snd_pcm_substream *substream) > return 0; > } > > +static int snd_emu10k1_playback_set_constraints(struct snd_pcm_runtime *runtime) > +{ > + int err; > + > + // The buffer size must be a multiple of the period size, to avoid a > + // mismatch between the extra voice and the regular voices. > + err = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS); > + if (err < 0) > + return err; > + // The hardware is typically the cache's size of 64 frames ahead. > + // Leave enough time for actually filling up the buffer. > + err = snd_pcm_hw_constraint_minmax( > + runtime, SNDRV_PCM_HW_PARAM_BUFFER_SIZE, 256, UINT_MAX); > + return err; > +} > + > static int snd_emu10k1_efx_playback_open(struct snd_pcm_substream *substream) > { > struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream); > struct snd_emu10k1_pcm *epcm; > struct snd_emu10k1_pcm_mixer *mix; > struct snd_pcm_runtime *runtime = substream->runtime; > - int i, j; > + int i, j, err; > > epcm = kzalloc(sizeof(*epcm), GFP_KERNEL); > if (epcm == NULL) > return -ENOMEM; > epcm->emu = emu; > epcm->type = PLAYBACK_EFX; > epcm->substream = substream; > > runtime->private_data = epcm; > runtime->private_free = snd_emu10k1_pcm_free_substream; > runtime->hw = snd_emu10k1_efx_playback; > - > + err = snd_emu10k1_playback_set_constraints(runtime); > + if (err < 0) { > + kfree(epcm); > + return err; > + } > + > for (i = 0; i < NUM_EFX_PLAYBACK; i++) { > mix = &emu->efx_pcm_mixer[i]; > for (j = 0; j < 8; j++) > @@ -1005,12 +1024,7 @@ static int snd_emu10k1_playback_open(struct snd_pcm_substream *substream) > runtime->private_data = epcm; > runtime->private_free = snd_emu10k1_pcm_free_substream; > runtime->hw = snd_emu10k1_playback; > - err = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS); > - if (err < 0) { > - kfree(epcm); > - return err; > - } > - err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 256, UINT_MAX); > + err = snd_emu10k1_playback_set_constraints(runtime); > if (err < 0) { > kfree(epcm); > return err; > -- > 2.40.0.152.g15d061e6df >
On Wed, May 17, 2023 at 10:25:00PM +0200, Takashi Iwai wrote: >On Wed, 17 May 2023 19:42:53 +0200, >Oswald Buddenhagen wrote: >> >> The period_bytes_min parameter made no sense at all, as it didn't >> correlate with any hardware limitation. > >Does the device really work with less than 64 bytes period size? >I meant not in theory but in practice. > somewhat predictably, not. >Without any value set, >dumb applications may try to set 4 bytes for period size, > the "try to" is key here. it will fail, because the frame-based constraint will prevent it from doing so. alsa's constraint system is really quite impressive, probably the technically most interesting part of the whole. :-) regards
On Thu, 18 May 2023 09:57:51 +0200, Oswald Buddenhagen wrote: > > On Wed, May 17, 2023 at 10:25:00PM +0200, Takashi Iwai wrote: > > On Wed, 17 May 2023 19:42:53 +0200, > > Oswald Buddenhagen wrote: > >> > >> The period_bytes_min parameter made no sense at all, as it didn't > >> correlate with any hardware limitation. > > > > Does the device really work with less than 64 bytes period size? > > I meant not in theory but in practice. > > > somewhat predictably, not. > > > Without any value set, > > dumb applications may try to set 4 bytes for period size, > > > the "try to" is key here. it will fail, because the frame-based > constraint will prevent it from doing so. Ah OK, this should be commented that the lower bound is set in a different way. Takashi
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c index feb575922738..5226f0978408 100644 --- a/sound/pci/emu10k1/emupcm.c +++ b/sound/pci/emu10k1/emupcm.c @@ -457,9 +457,8 @@ static const struct snd_pcm_hardware snd_emu10k1_efx_playback = .rate_max = 48000, .channels_min = NUM_EFX_PLAYBACK, .channels_max = NUM_EFX_PLAYBACK, - .buffer_bytes_max = (64*1024), - .period_bytes_min = 64, - .period_bytes_max = (64*1024), + .buffer_bytes_max = (128*1024), + .period_bytes_max = (128*1024), .periods_min = 2, .periods_max = 2, .fifo_size = 0, @@ -851,7 +850,6 @@ static const struct snd_pcm_hardware snd_emu10k1_playback = .channels_min = 1, .channels_max = 2, .buffer_bytes_max = (128*1024), - .period_bytes_min = 64, .period_bytes_max = (128*1024), .periods_min = 1, .periods_max = 1024, @@ -956,25 +954,46 @@ static int snd_emu10k1_efx_playback_close(struct snd_pcm_substream *substream) return 0; } +static int snd_emu10k1_playback_set_constraints(struct snd_pcm_runtime *runtime) +{ + int err; + + // The buffer size must be a multiple of the period size, to avoid a + // mismatch between the extra voice and the regular voices. + err = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS); + if (err < 0) + return err; + // The hardware is typically the cache's size of 64 frames ahead. + // Leave enough time for actually filling up the buffer. + err = snd_pcm_hw_constraint_minmax( + runtime, SNDRV_PCM_HW_PARAM_BUFFER_SIZE, 256, UINT_MAX); + return err; +} + static int snd_emu10k1_efx_playback_open(struct snd_pcm_substream *substream) { struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream); struct snd_emu10k1_pcm *epcm; struct snd_emu10k1_pcm_mixer *mix; struct snd_pcm_runtime *runtime = substream->runtime; - int i, j; + int i, j, err; epcm = kzalloc(sizeof(*epcm), GFP_KERNEL); if (epcm == NULL) return -ENOMEM; epcm->emu = emu; epcm->type = PLAYBACK_EFX; epcm->substream = substream; runtime->private_data = epcm; runtime->private_free = snd_emu10k1_pcm_free_substream; runtime->hw = snd_emu10k1_efx_playback; - + err = snd_emu10k1_playback_set_constraints(runtime); + if (err < 0) { + kfree(epcm); + return err; + } + for (i = 0; i < NUM_EFX_PLAYBACK; i++) { mix = &emu->efx_pcm_mixer[i]; for (j = 0; j < 8; j++) @@ -1005,12 +1024,7 @@ static int snd_emu10k1_playback_open(struct snd_pcm_substream *substream) runtime->private_data = epcm; runtime->private_free = snd_emu10k1_pcm_free_substream; runtime->hw = snd_emu10k1_playback; - err = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS); - if (err < 0) { - kfree(epcm); - return err; - } - err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 256, UINT_MAX); + err = snd_emu10k1_playback_set_constraints(runtime); if (err < 0) { kfree(epcm); return err;
The period_bytes_min parameter made no sense at all, as it didn't correlate with any hardware limitation. The same can be said of the buffer_bytes minimum constraint. Instead, apply a buffer_size constraint, which works on frames. Sync up the constraints of the EFX playback with those of the regular playback, as there is no reason for them to diverge. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- sound/pci/emu10k1/emupcm.c | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-)