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@xxxxxx>
> ---
> 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
>
