Re: [PATCH 6/9] ALSA: emu10k1: fix PCM playback buffer size constraints

[Takashi Iwai <tiwai@suse.de>]

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
>