| Message ID | 1596020585-11517-3-git-send-email-brent.lu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add period size constraint for Atom Chromebook | expand |
On 7/29/20 6:03 AM, Brent Lu wrote: > From: Yu-Hsuan Hsu <yuhsuan@chromium.org> > > The CRAS server does not set the period size in hw_param so ALSA will > calculate a value for period size which is based on the buffer size > and other parameters. The value may not always be aligned with Atom's > dsp design so a constraint is added to make sure the board always has > a good value. > > Cyan uses chtmax98090 and others(banon, celes, edgar, kefka...) use > rt5650. Is this patch required if you've already constrained the period sizes for the platform driver in patch1?
> > Is this patch required if you've already constrained the period sizes for the > platform driver in patch1? Yes or alsa will select 320 as default period size for it. Regards, Brent
>> Is this patch required if you've already constrained the period sizes for the >> platform driver in patch1? > > Yes or alsa will select 320 as default period size for it. ok, then that's a miss in your patch1. 320 samples is a multiple of 1ms for 48kHz rates. I think it was valid only for the 16kHz VoIP paths used in some versions of Android, but that we don't support in the upstream code. To build on Takashi's answer, the real ask here is to require that the period be a multiple of 1ms, because that's the fundamental design/limitation of firmware. It doesn't matter if it's 48, 96, 192, 240, 480, 960 samples.
On 7/30/20 10:27 AM, Pierre-Louis Bossart wrote: > > >>> Is this patch required if you've already constrained the period sizes >>> for the >>> platform driver in patch1? >> >> Yes or alsa will select 320 as default period size for it. > > ok, then that's a miss in your patch1. 320 samples is a multiple of 1ms typo: is NOT > for 48kHz rates. I think it was valid only for the 16kHz VoIP paths used > in some versions of Android, but that we don't support in the upstream > code. > > To build on Takashi's answer, the real ask here is to require that the > period be a multiple of 1ms, because that's the fundamental > design/limitation of firmware. It doesn't matter if it's 48, 96, 192, > 240, 480, 960 samples.
> >> > >> Yes or alsa will select 320 as default period size for it. > > > > ok, then that's a miss in your patch1. 320 samples is a multiple of > > 1ms > > typo: is NOT > > > for 48kHz rates. I think it was valid only for the 16kHz VoIP paths > > used in some versions of Android, but that we don't support in the > > upstream code. > > > > To build on Takashi's answer, the real ask here is to require that the > > period be a multiple of 1ms, because that's the fundamental > > design/limitation of firmware. It doesn't matter if it's 48, 96, 192, > > 240, 480, 960 samples. Yes 320 is for VoIP and the rates in CPU DAI are 8/16KHz. It's a mistake. Regards, Brent
On Thu, 30 Jul 2020 17:27:58 +0200, Pierre-Louis Bossart wrote: > > > > >> Is this patch required if you've already constrained the period sizes for the > >> platform driver in patch1? > > > > Yes or alsa will select 320 as default period size for it. > > ok, then that's a miss in your patch1. 320 samples is a multiple of > 1ms for 48kHz rates. I think it was valid only for the 16kHz VoIP > paths used in some versions of Android, but that we don't support in > the upstream code. > > To build on Takashi's answer, the real ask here is to require that the > period be a multiple of 1ms, because that's the fundamental > design/limitation of firmware. It doesn't matter if it's 48, 96, 192, > 240, 480, 960 samples. If the 1ms alignment is the condition, it can be better with a different hw_params constraint. We can use snd_pcm_hw_constraint_step() for such a purpose. thanks, Takashi
> > If the 1ms alignment is the condition, it can be better with a different > hw_params constraint. We can use > snd_pcm_hw_constraint_step() for such a purpose. Will fix. Thanks. Regards, Brent > > > thanks, > > Takashi
diff --git a/sound/soc/intel/boards/cht_bsw_max98090_ti.c b/sound/soc/intel/boards/cht_bsw_max98090_ti.c index 835e9bd..bf67254 100644 --- a/sound/soc/intel/boards/cht_bsw_max98090_ti.c +++ b/sound/soc/intel/boards/cht_bsw_max98090_ti.c @@ -283,8 +283,20 @@ static int cht_codec_fixup(struct snd_soc_pcm_runtime *rtd, static int cht_aif1_startup(struct snd_pcm_substream *substream) { - return snd_pcm_hw_constraint_single(substream->runtime, + int err; + + /* Set period size to 240 to align with Atom design */ + err = snd_pcm_hw_constraint_minmax(substream->runtime, + SNDRV_PCM_HW_PARAM_PERIOD_SIZE, 240, 240); + if (err < 0) + return err; + + err = snd_pcm_hw_constraint_single(substream->runtime, SNDRV_PCM_HW_PARAM_RATE, 48000); + if (err < 0) + return err; + + return 0; } static int cht_max98090_headset_init(struct snd_soc_component *component) diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c index b53c024..6e62f0d 100644 --- a/sound/soc/intel/boards/cht_bsw_rt5645.c +++ b/sound/soc/intel/boards/cht_bsw_rt5645.c @@ -414,8 +414,20 @@ static int cht_codec_fixup(struct snd_soc_pcm_runtime *rtd, static int cht_aif1_startup(struct snd_pcm_substream *substream) { - return snd_pcm_hw_constraint_single(substream->runtime, + int err; + + /* Set period size to 240 to align with Atom design */ + err = snd_pcm_hw_constraint_minmax(substream->runtime, + SNDRV_PCM_HW_PARAM_PERIOD_SIZE, 240, 240); + if (err < 0) + return err; + + err = snd_pcm_hw_constraint_single(substream->runtime, SNDRV_PCM_HW_PARAM_RATE, 48000); + if (err < 0) + return err; + + return 0; } static const struct snd_soc_ops cht_aif1_ops = {