[PATCH 0/2] Add period size constraint for Atom Chromebook

[PATCH 0/2] Add period size constraint for Atom Chromebook

[Brent Lu]

Two different constraints are implemented: one is in platform's CPU
DAI to enforce period sizes which are already used in Android BSP. The
other is in Atom Chromebook's machine driver to use 240 as period size.

Brent Lu (1):
  ASoC: intel: atom: Add period size constraint

Yu-Hsuan Hsu (1):
  ASoC: Intel: Add period size constraint on strago board

 sound/soc/intel/atom/sst-mfld-platform-pcm.c | 15 +++++++++++++++
 sound/soc/intel/boards/cht_bsw_max98090_ti.c | 14 +++++++++++++-
 sound/soc/intel/boards/cht_bsw_rt5645.c      | 14 +++++++++++++-
 3 files changed, 41 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH 1/2] ASoC: intel: atom: Add period size constraint

[Brent Lu]

Use constraint to enforce the period sizes which are validated in
Android BSP.

Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 sound/soc/intel/atom/sst-mfld-platform-pcm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
index 49b9f18..f614651 100644
--- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
+++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
@@ -300,6 +300,16 @@ static void power_down_sst(struct sst_runtime_stream *stream)
 	stream->ops->power(sst->dev, false);
 }
 
+static const unsigned int media_period_size[] = {
+	/* sizes validated on Android platform */
+	240, 320, 960, 3072
+};
+
+static const struct snd_pcm_hw_constraint_list constraints_media_period_size = {
+	.count = ARRAY_SIZE(media_period_size),
+	.list  = media_period_size,
+};
+
 static int sst_media_open(struct snd_pcm_substream *substream,
 		struct snd_soc_dai *dai)
 {
@@ -333,6 +343,11 @@ static int sst_media_open(struct snd_pcm_substream *substream,
 	if (ret_val < 0)
 		return ret_val;
 
+	/* Avoid using period size which is not validated */
+	snd_pcm_hw_constraint_list(substream->runtime, 0,
+			SNDRV_PCM_HW_PARAM_PERIOD_SIZE,
+			&constraints_media_period_size);
+
 	/* Make sure, that the period size is always even */
 	snd_pcm_hw_constraint_step(substream->runtime, 0,
 			   SNDRV_PCM_HW_PARAM_PERIODS, 2);
-- 
2.7.4

[PATCH 2/2] ASoC: Intel: Add period size constraint on strago board

[Brent Lu]

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.

Signed-off-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 sound/soc/intel/boards/cht_bsw_max98090_ti.c | 14 +++++++++++++-
 sound/soc/intel/boards/cht_bsw_rt5645.c      | 14 +++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

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 = {
-- 
2.7.4

Re: [PATCH 1/2] ASoC: intel: atom: Add period size constraint

[Andy Shevchenko]

On Wed, Jul 29, 2020 at 07:03:04PM +0800, Brent Lu wrote:
> Use constraint to enforce the period sizes which are validated in
> Android BSP.
> 
> Signed-off-by: Brent Lu <brent.lu@intel.com>
> ---
>  sound/soc/intel/atom/sst-mfld-platform-pcm.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> index 49b9f18..f614651 100644
> --- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> +++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> @@ -300,6 +300,16 @@ static void power_down_sst(struct sst_runtime_stream *stream)
>  	stream->ops->power(sst->dev, false);
>  }
>  
> +static const unsigned int media_period_size[] = {
> +	/* sizes validated on Android platform */

> +	240, 320, 960, 3072

Leave comma at the end.

> +};
> +
> +static const struct snd_pcm_hw_constraint_list constraints_media_period_size = {
> +	.count = ARRAY_SIZE(media_period_size),
> +	.list  = media_period_size,
> +};
> +
>  static int sst_media_open(struct snd_pcm_substream *substream,
>  		struct snd_soc_dai *dai)
>  {
> @@ -333,6 +343,11 @@ static int sst_media_open(struct snd_pcm_substream *substream,
>  	if (ret_val < 0)
>  		return ret_val;
>  
> +	/* Avoid using period size which is not validated */
> +	snd_pcm_hw_constraint_list(substream->runtime, 0,
> +			SNDRV_PCM_HW_PARAM_PERIOD_SIZE,
> +			&constraints_media_period_size);
> +
>  	/* Make sure, that the period size is always even */
>  	snd_pcm_hw_constraint_step(substream->runtime, 0,
>  			   SNDRV_PCM_HW_PARAM_PERIODS, 2);
> -- 
> 2.7.4
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] Add period size constraint for Atom Chromebook

[Andy Shevchenko]

On Wed, Jul 29, 2020 at 07:03:03PM +0800, Brent Lu wrote:
> Two different constraints are implemented: one is in platform's CPU
> DAI to enforce period sizes which are already used in Android BSP. The
> other is in Atom Chromebook's machine driver to use 240 as period size.

One nit to one patch.
Overall, LGTM and thus FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Brent Lu (1):
>   ASoC: intel: atom: Add period size constraint
> 
> Yu-Hsuan Hsu (1):
>   ASoC: Intel: Add period size constraint on strago board
> 
>  sound/soc/intel/atom/sst-mfld-platform-pcm.c | 15 +++++++++++++++
>  sound/soc/intel/boards/cht_bsw_max98090_ti.c | 14 +++++++++++++-
>  sound/soc/intel/boards/cht_bsw_rt5645.c      | 14 +++++++++++++-
>  3 files changed, 41 insertions(+), 2 deletions(-)
> 
> -- 
> 2.7.4
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] ASoC: Intel: Add period size constraint on strago board

[Pierre-Louis Bossart]

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?

RE: [PATCH 2/2] ASoC: Intel: Add period size constraint on strago board

[Lu, 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.


Regards,
Brent

Re: [PATCH 2/2] ASoC: Intel: Add period size constraint on strago board

[Pierre-Louis Bossart]

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

Re: [PATCH 2/2] ASoC: Intel: Add period size constraint on strago board

[Pierre-Louis Bossart]

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.

RE: [PATCH 2/2] ASoC: Intel: Add period size constraint on strago board

[Lu, Brent]

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

Re: [PATCH 2/2] ASoC: Intel: Add period size constraint on strago board

[Takashi Iwai]

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

[PATCH v3 0/2] Add period size constraint for Atom Chromebook

[Brent Lu]

Two different constraints are implemented: one is in platform's CPU
DAI to enforce the period to be multiple of 1ms to align with firmware
design. The other is in Atom Chromebook's machine driver to use 240 as
period size which is selected by google.


Changes since v1:
-Add comma at the end of media_period_size array declaration.

Changes since v2:
-Use snd_pcm_hw_constraint_step to enforce the 1ms period.


Brent Lu (1):
  ASoC: intel: atom: Add period size constraint

Yu-Hsuan Hsu (1):
  ASoC: Intel: Add period size constraint on strago board

 sound/soc/intel/atom/sst-mfld-platform-pcm.c | 11 +++++++++++
 sound/soc/intel/boards/cht_bsw_max98090_ti.c | 14 +++++++++++++-
 sound/soc/intel/boards/cht_bsw_rt5645.c      | 14 +++++++++++++-
 3 files changed, 37 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH v3 1/2] ASoC: intel: atom: Add period size constraint

[Brent Lu]

Use constraint to make sure the period size could always be multiple
of 1ms to align with the fundamental design/limitation of firmware.

Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 sound/soc/intel/atom/sst-mfld-platform-pcm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
index 49b9f18..5f89956 100644
--- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
+++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
@@ -333,6 +333,17 @@ static int sst_media_open(struct snd_pcm_substream *substream,
 	if (ret_val < 0)
 		return ret_val;
 
+	/*
+	 * Make sure the period to be multiple of 1ms to align the
+	 * design of firmware. Apply same rule to buffer size to make
+	 * sure alsa could always find a value for period size
+	 * regardless the buffer size given by user space.
+	 */
+	snd_pcm_hw_constraint_step(substream->runtime, 0,
+			   SNDRV_PCM_HW_PARAM_PERIOD_SIZE, 48);
+	snd_pcm_hw_constraint_step(substream->runtime, 0,
+			   SNDRV_PCM_HW_PARAM_BUFFER_SIZE, 48);
+
 	/* Make sure, that the period size is always even */
 	snd_pcm_hw_constraint_step(substream->runtime, 0,
 			   SNDRV_PCM_HW_PARAM_PERIODS, 2);
-- 
2.7.4

[PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Brent Lu]

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.

Signed-off-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 sound/soc/intel/boards/cht_bsw_max98090_ti.c | 14 +++++++++++++-
 sound/soc/intel/boards/cht_bsw_rt5645.c      | 14 +++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

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 = {
-- 
2.7.4

RE: [PATCH 2/2] ASoC: Intel: Add period size constraint on strago board

[Lu, Brent]

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

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Takashi Iwai]

On Fri, 31 Jul 2020 14:26:05 +0200,
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.
> 
> Signed-off-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
> Signed-off-by: Brent Lu <brent.lu@intel.com>
> ---
>  sound/soc/intel/boards/cht_bsw_max98090_ti.c | 14 +++++++++++++-
>  sound/soc/intel/boards/cht_bsw_rt5645.c      | 14 +++++++++++++-
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> 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;

Again, is this fixed 240 is a must?  Or is this also an alignment
issue?


thanks,

Takashi

RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Lu, Brent]

> 
> Again, is this fixed 240 is a must?  Or is this also an alignment issue?
Hi Takashi,

I think it's a must for Chromebooks. Google found this value works best
with their CRAS server running on their BSW products. They offered this
patch for their own Chromebooks.

> 
> 
> thanks,
> 
> Takashi

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Takashi Iwai]

On Sat, 01 Aug 2020 10:58:16 +0200,
Lu, Brent wrote:
> 
> > 
> > Again, is this fixed 240 is a must?  Or is this also an alignment issue?
> Hi Takashi,
> 
> I think it's a must for Chromebooks. Google found this value works best
> with their CRAS server running on their BSW products. They offered this
> patch for their own Chromebooks.

Hrm, but it's likely a worse choice on other sound backends.

Please double-check whether this fixed small period is a must, or it's
an alignment issue.


Takashi

RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Lu, Brent]

> > >
> > > Again, is this fixed 240 is a must?  Or is this also an alignment issue?
> > Hi Takashi,
> >
> > I think it's a must for Chromebooks. Google found this value works
> > best with their CRAS server running on their BSW products. They
> > offered this patch for their own Chromebooks.
> 
> Hrm, but it's likely a worse choice on other sound backends.
> 
> Please double-check whether this fixed small period is a must, or it's an
> alignment issue.
Hi Takashi,

I've double checked with google. It's a must for Chromebooks due to low
latency use case.


Regards,
Brent

> 
> 
> Takashi

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Pierre-Louis Bossart]

On 8/3/20 8:00 AM, Lu, Brent wrote:
>>>>
>>>> Again, is this fixed 240 is a must?  Or is this also an alignment issue?
>>> Hi Takashi,
>>>
>>> I think it's a must for Chromebooks. Google found this value works
>>> best with their CRAS server running on their BSW products. They
>>> offered this patch for their own Chromebooks.
>>
>> Hrm, but it's likely a worse choice on other sound backends.
>>
>> Please double-check whether this fixed small period is a must, or it's an
>> alignment issue.
> Hi Takashi,
> 
> I've double checked with google. It's a must for Chromebooks due to low
> latency use case.

I wonder if there's a misunderstanding here?

I believe Takashi's question was "is this a must to ONLY accept 240 
samples for the period size", there was no pushback on the value itself. 
Are those boards broken with e.g. 960 samples?

RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Lu, Brent]

> > Hi Takashi,
> >
> > I've double checked with google. It's a must for Chromebooks due to
> > low latency use case.
> 
> I wonder if there's a misunderstanding here?
> 
> I believe Takashi's question was "is this a must to ONLY accept 240 samples
> for the period size", there was no pushback on the value itself.
> Are those boards broken with e.g. 960 samples?

I've added google people to discuss directly.

Hi Yuhsuan,
Would you explain why CRAS needs to use such short period size? Thanks.


Regards,
Brent

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Takashi Iwai]

On Mon, 03 Aug 2020 18:45:29 +0200,
Lu, Brent wrote:
> 
> > > Hi Takashi,
> > >
> > > I've double checked with google. It's a must for Chromebooks due to
> > > low latency use case.
> > 
> > I wonder if there's a misunderstanding here?
> > 
> > I believe Takashi's question was "is this a must to ONLY accept 240 samples
> > for the period size", there was no pushback on the value itself.
> > Are those boards broken with e.g. 960 samples?
> 
> I've added google people to discuss directly.
> 
> Hi Yuhsuan,
> Would you explain why CRAS needs to use such short period size? Thanks.

For avoid further misunderstanding: it's fine that CRAS *uses* such a
short period.  It's often required for achieving a short latency.

However, the question is whether the driver can set *only* this value
for making it working.  IOW, if we don't have this constraint, what
actually happens?  If the driver gives the period size alignment,
wouldn't CRAS choose 240?


Takashi

RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Lu, Brent]

> 
> For avoid further misunderstanding: it's fine that CRAS *uses* such a short
> period.  It's often required for achieving a short latency.
> 
> However, the question is whether the driver can set *only* this value for
> making it working.  IOW, if we don't have this constraint, what actually
> happens?  If the driver gives the period size alignment, wouldn't CRAS
> choose 240?

It won't. Without the constraint it becomes 432. Actually CRAS does not set
period size specifically so the value depends on the constraint rules.

[   52.011146] sound pcmC1D0p: hw_param
[   52.011152] sound pcmC1D0p:   ACCESS 0x1
[   52.011155] sound pcmC1D0p:   FORMAT 0x4
[   52.011158] sound pcmC1D0p:   SUBFORMAT 0x1
[   52.011161] sound pcmC1D0p:   SAMPLE_BITS [16:16]
[   52.011164] sound pcmC1D0p:   FRAME_BITS [32:32]
[   52.011167] sound pcmC1D0p:   CHANNELS [2:2]
[   52.011170] sound pcmC1D0p:   RATE [48000:48000]
[   52.011173] sound pcmC1D0p:   PERIOD_TIME [9000:9000]
[   52.011176] sound pcmC1D0p:   PERIOD_SIZE [432:432]
[   52.011179] sound pcmC1D0p:   PERIOD_BYTES [1728:1728]
[   52.011182] sound pcmC1D0p:   PERIODS [474:474]
[   52.011185] sound pcmC1D0p:   BUFFER_TIME [4266000:4266000]
[   52.011188] sound pcmC1D0p:   BUFFER_SIZE [204768:204768]
[   52.011191] sound pcmC1D0p:   BUFFER_BYTES [819072:819072]
[   52.011194] sound pcmC1D0p:   TICK_TIME [0:0]

Regards,
Brent

> 
> 
> Takashi

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Pierre-Louis Bossart]

On 8/3/20 11:33 PM, Lu, Brent wrote:
>>
>> For avoid further misunderstanding: it's fine that CRAS *uses* such a short
>> period.  It's often required for achieving a short latency.
>>
>> However, the question is whether the driver can set *only* this value for
>> making it working.  IOW, if we don't have this constraint, what actually
>> happens?  If the driver gives the period size alignment, wouldn't CRAS
>> choose 240?
> 
> It won't. Without the constraint it becomes 432. Actually CRAS does not set
> period size specifically so the value depends on the constraint rules.

I don't get this. If the platform driver already stated 240 and 960 
samples why would 432 be chosen? Doesn't this mean the constraint is not 
applied?

> [   52.011146] sound pcmC1D0p: hw_param
> [   52.011152] sound pcmC1D0p:   ACCESS 0x1
> [   52.011155] sound pcmC1D0p:   FORMAT 0x4
> [   52.011158] sound pcmC1D0p:   SUBFORMAT 0x1
> [   52.011161] sound pcmC1D0p:   SAMPLE_BITS [16:16]
> [   52.011164] sound pcmC1D0p:   FRAME_BITS [32:32]
> [   52.011167] sound pcmC1D0p:   CHANNELS [2:2]
> [   52.011170] sound pcmC1D0p:   RATE [48000:48000]
> [   52.011173] sound pcmC1D0p:   PERIOD_TIME [9000:9000]
> [   52.011176] sound pcmC1D0p:   PERIOD_SIZE [432:432]
> [   52.011179] sound pcmC1D0p:   PERIOD_BYTES [1728:1728]
> [   52.011182] sound pcmC1D0p:   PERIODS [474:474]
> [   52.011185] sound pcmC1D0p:   BUFFER_TIME [4266000:4266000]
> [   52.011188] sound pcmC1D0p:   BUFFER_SIZE [204768:204768]
> [   52.011191] sound pcmC1D0p:   BUFFER_BYTES [819072:819072]
> [   52.011194] sound pcmC1D0p:   TICK_TIME [0:0]
> 
> Regards,
> Brent
> 
>>
>>
>> Takashi
> 
> 

RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Lu, Brent]

> 
> I don't get this. If the platform driver already stated 240 and 960 samples why
> would 432 be chosen? Doesn't this mean the constraint is not applied?

Hi Pierre,

Sorry for late reply. I used following constraints in V3 patch so any period which
aligns 1ms would be accepted.

+	/*
+	 * Make sure the period to be multiple of 1ms to align the
+	 * design of firmware. Apply same rule to buffer size to make
+	 * sure alsa could always find a value for period size
+	 * regardless the buffer size given by user space.
+	 */
+	snd_pcm_hw_constraint_step(substream->runtime, 0,
+			   SNDRV_PCM_HW_PARAM_PERIOD_SIZE, 48);
+	snd_pcm_hw_constraint_step(substream->runtime, 0,
+			   SNDRV_PCM_HW_PARAM_BUFFER_SIZE, 48);

Regards,
Brent

> 
> > [   52.011146] sound pcmC1D0p: hw_param
> > [   52.011152] sound pcmC1D0p:   ACCESS 0x1
> > [   52.011155] sound pcmC1D0p:   FORMAT 0x4
> > [   52.011158] sound pcmC1D0p:   SUBFORMAT 0x1
> > [   52.011161] sound pcmC1D0p:   SAMPLE_BITS [16:16]
> > [   52.011164] sound pcmC1D0p:   FRAME_BITS [32:32]
> > [   52.011167] sound pcmC1D0p:   CHANNELS [2:2]
> > [   52.011170] sound pcmC1D0p:   RATE [48000:48000]
> > [   52.011173] sound pcmC1D0p:   PERIOD_TIME [9000:9000]
> > [   52.011176] sound pcmC1D0p:   PERIOD_SIZE [432:432]
> > [   52.011179] sound pcmC1D0p:   PERIOD_BYTES [1728:1728]
> > [   52.011182] sound pcmC1D0p:   PERIODS [474:474]
> > [   52.011185] sound pcmC1D0p:   BUFFER_TIME [4266000:4266000]
> > [   52.011188] sound pcmC1D0p:   BUFFER_SIZE [204768:204768]
> > [   52.011191] sound pcmC1D0p:   BUFFER_BYTES [819072:819072]
> > [   52.011194] sound pcmC1D0p:   TICK_TIME [0:0]
> >
> > Regards,
> > Brent
> >
> >>
> >>
> >> Takashi
> >
> >

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Pierre-Louis Bossart]

On 8/6/20 11:41 AM, Lu, Brent wrote:
>>
>> I don't get this. If the platform driver already stated 240 and 960 samples why
>> would 432 be chosen? Doesn't this mean the constraint is not applied?
> 
> Hi Pierre,
> 
> Sorry for late reply. I used following constraints in V3 patch so any period which
> aligns 1ms would be accepted.
> 
> +	/*
> +	 * Make sure the period to be multiple of 1ms to align the
> +	 * design of firmware. Apply same rule to buffer size to make
> +	 * sure alsa could always find a value for period size
> +	 * regardless the buffer size given by user space.
> +	 */
> +	snd_pcm_hw_constraint_step(substream->runtime, 0,
> +			   SNDRV_PCM_HW_PARAM_PERIOD_SIZE, 48);
> +	snd_pcm_hw_constraint_step(substream->runtime, 0,
> +			   SNDRV_PCM_HW_PARAM_BUFFER_SIZE, 48);

432 samples is 9ms, I don't have a clue why/how CRAS might ask for this 
value.

It'd be a bit odd to add constraints just for the purpose of letting 
userspace select a sensible value.

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Yu-Hsuan Hsu]

Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> 於
2020年8月10日 週一 下午11:03寫道：
>
>
>
> On 8/6/20 11:41 AM, Lu, Brent wrote:
> >>
> >> I don't get this. If the platform driver already stated 240 and 960 samples why
> >> would 432 be chosen? Doesn't this mean the constraint is not applied?
> >
> > Hi Pierre,
> >
> > Sorry for late reply. I used following constraints in V3 patch so any period which
> > aligns 1ms would be accepted.
> >
> > +     /*
> > +      * Make sure the period to be multiple of 1ms to align the
> > +      * design of firmware. Apply same rule to buffer size to make
> > +      * sure alsa could always find a value for period size
> > +      * regardless the buffer size given by user space.
> > +      */
> > +     snd_pcm_hw_constraint_step(substream->runtime, 0,
> > +                        SNDRV_PCM_HW_PARAM_PERIOD_SIZE, 48);
> > +     snd_pcm_hw_constraint_step(substream->runtime, 0,
> > +                        SNDRV_PCM_HW_PARAM_BUFFER_SIZE, 48);
>
> 432 samples is 9ms, I don't have a clue why/how CRAS might ask for this
> value.
>
> It'd be a bit odd to add constraints just for the purpose of letting
> userspace select a sensible value.

Sorry for the late reply. CRAS does not set the period size when using it.
The default period size is 256, which consumes the samples
quickly(about 49627 fps when the rate is 48000 fps) at the beginning
of the playback.
Since CRAS write samples with the fixed frequency, it triggers
underruns immidiately.

According to Brent, the DSP is using 240 period regardless the
hw_param. If the period size is 256, DSP will read 256 samples each
time but only consume 240 samples until the ring buffer of DSP is
full. This behavior makes the samples in the ring buffer of kernel
consumed quickly. (Not sure whether the explanation is correct. Need
Brent to confirm it.)

Unfortunately, we can not change the behavior of DSP. After some
experiments, we found that the issue can be fixed if we set the period
size to 240. With the same frequency as the DSP, the samples are
consumed stably. Because everyone can trigger this issue when using
the driver without setting the period size, we think it is a general
issue that should be fixed in the kernel.

Thanks,
Yu-Hsuan

RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Lu, Brent]

> 
> Sorry for the late reply. CRAS does not set the period size when using it.
> The default period size is 256, which consumes the samples quickly(about 49627
> fps when the rate is 48000 fps) at the beginning of the playback.
> Since CRAS write samples with the fixed frequency, it triggers underruns
> immidiately.
> 
> According to Brent, the DSP is using 240 period regardless the hw_param. If the
> period size is 256, DSP will read 256 samples each time but only consume 240
> samples until the ring buffer of DSP is full. This behavior makes the samples in
> the ring buffer of kernel consumed quickly. (Not sure whether the explanation is
> correct. Need Brent to confirm it.)
> 
> Unfortunately, we can not change the behavior of DSP. After some experiments,
> we found that the issue can be fixed if we set the period size to 240. With the
> same frequency as the DSP, the samples are consumed stably. Because everyone
> can trigger this issue when using the driver without setting the period size, we
> think it is a general issue that should be fixed in the kernel.

I check the code and just realized CRAS does nothing but request maximum buffer
size. As I know the application needs to decide the buffer time and period time so
ALSA could generate a hw_param structure with proper period size instead of using
fixed constraint in machine driver because driver has no idea about the latency you
want.

You can use snd_pcm_hw_params_set_buffer_time_near() and
snd_pcm_hw_params_set_period_time_near() to get a proper configuration of
buffer and period parameters according to the latency requirement. In the CRAS
code, there is a UCM variable to support this: DmaPeriodMicrosecs. I tested it on
Celes and it looks quite promising. It seems to me that adding constraint in machine
driver is not necessary.

SectionDevice."Speaker".0 {
	Value {
		PlaybackPCM "hw:chtrt5650,0"
		DmaPeriodMicrosecs "5000"
...

[   52.434761] sound pcmC1D0p: hw_param
[   52.434767] sound pcmC1D0p:   ACCESS 0x1
[   52.434770] sound pcmC1D0p:   FORMAT 0x4
[   52.434772] sound pcmC1D0p:   SUBFORMAT 0x1
[   52.434776] sound pcmC1D0p:   SAMPLE_BITS [16:16]
[   52.434779] sound pcmC1D0p:   FRAME_BITS [32:32]
[   52.434782] sound pcmC1D0p:   CHANNELS [2:2]
[   52.434785] sound pcmC1D0p:   RATE [48000:48000]
[   52.434788] sound pcmC1D0p:   PERIOD_TIME [5000:5000]
[   52.434791] sound pcmC1D0p:   PERIOD_SIZE [240:240]
[   52.434794] sound pcmC1D0p:   PERIOD_BYTES [960:960]
[   52.434797] sound pcmC1D0p:   PERIODS [852:852]
[   52.434799] sound pcmC1D0p:   BUFFER_TIME [4260000:4260000]
[   52.434802] sound pcmC1D0p:   BUFFER_SIZE [204480:204480]
[   52.434805] sound pcmC1D0p:   BUFFER_BYTES [817920:817920]
[   52.434808] sound pcmC1D0p:   TICK_TIME [0:0]

Regards,
Brent

> 
> Thanks,
> Yu-Hsuan

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Yu-Hsuan Hsu]

Lu, Brent <brent.lu@intel.com> 於 2020年8月11日 週二 上午10:17寫道：
>
> >
> > Sorry for the late reply. CRAS does not set the period size when using it.
> > The default period size is 256, which consumes the samples quickly(about 49627
> > fps when the rate is 48000 fps) at the beginning of the playback.
> > Since CRAS write samples with the fixed frequency, it triggers underruns
> > immidiately.
> >
> > According to Brent, the DSP is using 240 period regardless the hw_param. If the
> > period size is 256, DSP will read 256 samples each time but only consume 240
> > samples until the ring buffer of DSP is full. This behavior makes the samples in
> > the ring buffer of kernel consumed quickly. (Not sure whether the explanation is
> > correct. Need Brent to confirm it.)
> >
> > Unfortunately, we can not change the behavior of DSP. After some experiments,
> > we found that the issue can be fixed if we set the period size to 240. With the
> > same frequency as the DSP, the samples are consumed stably. Because everyone
> > can trigger this issue when using the driver without setting the period size, we
> > think it is a general issue that should be fixed in the kernel.
>
> I check the code and just realized CRAS does nothing but request maximum buffer
> size. As I know the application needs to decide the buffer time and period time so
> ALSA could generate a hw_param structure with proper period size instead of using
> fixed constraint in machine driver because driver has no idea about the latency you
> want.
>
> You can use snd_pcm_hw_params_set_buffer_time_near() and
> snd_pcm_hw_params_set_period_time_near() to get a proper configuration of
> buffer and period parameters according to the latency requirement. In the CRAS
> code, there is a UCM variable to support this: DmaPeriodMicrosecs. I tested it on
> Celes and it looks quite promising. It seems to me that adding constraint in machine
> driver is not necessary.
>
> SectionDevice."Speaker".0 {
>         Value {
>                 PlaybackPCM "hw:chtrt5650,0"
>                 DmaPeriodMicrosecs "5000"
> ...
>
> [   52.434761] sound pcmC1D0p: hw_param
> [   52.434767] sound pcmC1D0p:   ACCESS 0x1
> [   52.434770] sound pcmC1D0p:   FORMAT 0x4
> [   52.434772] sound pcmC1D0p:   SUBFORMAT 0x1
> [   52.434776] sound pcmC1D0p:   SAMPLE_BITS [16:16]
> [   52.434779] sound pcmC1D0p:   FRAME_BITS [32:32]
> [   52.434782] sound pcmC1D0p:   CHANNELS [2:2]
> [   52.434785] sound pcmC1D0p:   RATE [48000:48000]
> [   52.434788] sound pcmC1D0p:   PERIOD_TIME [5000:5000]
> [   52.434791] sound pcmC1D0p:   PERIOD_SIZE [240:240]
> [   52.434794] sound pcmC1D0p:   PERIOD_BYTES [960:960]
> [   52.434797] sound pcmC1D0p:   PERIODS [852:852]
> [   52.434799] sound pcmC1D0p:   BUFFER_TIME [4260000:4260000]
> [   52.434802] sound pcmC1D0p:   BUFFER_SIZE [204480:204480]
> [   52.434805] sound pcmC1D0p:   BUFFER_BYTES [817920:817920]
> [   52.434808] sound pcmC1D0p:   TICK_TIME [0:0]
>
> Regards,
> Brent
Hi Brent,

Yes, I know we can do it to fix the issue as well. As I mentioned
before, we wanted to fix it in kernel because it is a real issue,
isn't it? Basically, a driver should work with any param it supports.
But in this case, everyone can trigger underrun if he or she does not
the period size to 240. If you still think it's not necessary, I can
modify UCM to make CRAS set the appropriate period size.

Thanks,
Yu-Hsuan

>
> >
> > Thanks,
> > Yu-Hsuan

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Takashi Iwai]

On Tue, 11 Aug 2020 04:29:24 +0200,
Yu-Hsuan Hsu wrote:
> 
> Lu, Brent <brent.lu@intel.com> 於 2020年8月11日 週二 上午10:17寫道：
> >
> > >
> > > Sorry for the late reply. CRAS does not set the period size when using it.
> > > The default period size is 256, which consumes the samples quickly(about 49627
> > > fps when the rate is 48000 fps) at the beginning of the playback.
> > > Since CRAS write samples with the fixed frequency, it triggers underruns
> > > immidiately.
> > >
> > > According to Brent, the DSP is using 240 period regardless the hw_param. If the
> > > period size is 256, DSP will read 256 samples each time but only consume 240
> > > samples until the ring buffer of DSP is full. This behavior makes the samples in
> > > the ring buffer of kernel consumed quickly. (Not sure whether the explanation is
> > > correct. Need Brent to confirm it.)
> > >
> > > Unfortunately, we can not change the behavior of DSP. After some experiments,
> > > we found that the issue can be fixed if we set the period size to 240. With the
> > > same frequency as the DSP, the samples are consumed stably. Because everyone
> > > can trigger this issue when using the driver without setting the period size, we
> > > think it is a general issue that should be fixed in the kernel.
> >
> > I check the code and just realized CRAS does nothing but request maximum buffer
> > size. As I know the application needs to decide the buffer time and period time so
> > ALSA could generate a hw_param structure with proper period size instead of using
> > fixed constraint in machine driver because driver has no idea about the latency you
> > want.
> >
> > You can use snd_pcm_hw_params_set_buffer_time_near() and
> > snd_pcm_hw_params_set_period_time_near() to get a proper configuration of
> > buffer and period parameters according to the latency requirement. In the CRAS
> > code, there is a UCM variable to support this: DmaPeriodMicrosecs. I tested it on
> > Celes and it looks quite promising. It seems to me that adding constraint in machine
> > driver is not necessary.
> >
> > SectionDevice."Speaker".0 {
> >         Value {
> >                 PlaybackPCM "hw:chtrt5650,0"
> >                 DmaPeriodMicrosecs "5000"
> > ...
> >
> > [   52.434761] sound pcmC1D0p: hw_param
> > [   52.434767] sound pcmC1D0p:   ACCESS 0x1
> > [   52.434770] sound pcmC1D0p:   FORMAT 0x4
> > [   52.434772] sound pcmC1D0p:   SUBFORMAT 0x1
> > [   52.434776] sound pcmC1D0p:   SAMPLE_BITS [16:16]
> > [   52.434779] sound pcmC1D0p:   FRAME_BITS [32:32]
> > [   52.434782] sound pcmC1D0p:   CHANNELS [2:2]
> > [   52.434785] sound pcmC1D0p:   RATE [48000:48000]
> > [   52.434788] sound pcmC1D0p:   PERIOD_TIME [5000:5000]
> > [   52.434791] sound pcmC1D0p:   PERIOD_SIZE [240:240]
> > [   52.434794] sound pcmC1D0p:   PERIOD_BYTES [960:960]
> > [   52.434797] sound pcmC1D0p:   PERIODS [852:852]
> > [   52.434799] sound pcmC1D0p:   BUFFER_TIME [4260000:4260000]
> > [   52.434802] sound pcmC1D0p:   BUFFER_SIZE [204480:204480]
> > [   52.434805] sound pcmC1D0p:   BUFFER_BYTES [817920:817920]
> > [   52.434808] sound pcmC1D0p:   TICK_TIME [0:0]
> >
> > Regards,
> > Brent
> Hi Brent,
> 
> Yes, I know we can do it to fix the issue as well. As I mentioned
> before, we wanted to fix it in kernel because it is a real issue,
> isn't it? Basically, a driver should work with any param it supports.
> But in this case, everyone can trigger underrun if he or she does not
> the period size to 240. If you still think it's not necessary, I can
> modify UCM to make CRAS set the appropriate period size.

How does it *not* work if you set other than period size 240, more
exactly?

The hw_constraint to a fixed period size must be really an exception.
If you look at other drivers, you won't find any other doing such.
It already indicates that something is wrong.

Usually the fixed period size comes from the hardware limitation and
defined in snd_pcm_hardware.  Or, sometimes it's an alignment issue.
If you need more than that, you should doubt what's really not
working.


Takashi

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Yu-Hsuan Hsu]

Takashi Iwai <tiwai@suse.de> 於 2020年8月11日 週二 下午3:43寫道：
>
> On Tue, 11 Aug 2020 04:29:24 +0200,
> Yu-Hsuan Hsu wrote:
> >
> > Lu, Brent <brent.lu@intel.com> 於 2020年8月11日 週二 上午10:17寫道：
> > >
> > > >
> > > > Sorry for the late reply. CRAS does not set the period size when using it.
> > > > The default period size is 256, which consumes the samples quickly(about 49627
> > > > fps when the rate is 48000 fps) at the beginning of the playback.
> > > > Since CRAS write samples with the fixed frequency, it triggers underruns
> > > > immidiately.
> > > >
> > > > According to Brent, the DSP is using 240 period regardless the hw_param. If the
> > > > period size is 256, DSP will read 256 samples each time but only consume 240
> > > > samples until the ring buffer of DSP is full. This behavior makes the samples in
> > > > the ring buffer of kernel consumed quickly. (Not sure whether the explanation is
> > > > correct. Need Brent to confirm it.)
> > > >
> > > > Unfortunately, we can not change the behavior of DSP. After some experiments,
> > > > we found that the issue can be fixed if we set the period size to 240. With the
> > > > same frequency as the DSP, the samples are consumed stably. Because everyone
> > > > can trigger this issue when using the driver without setting the period size, we
> > > > think it is a general issue that should be fixed in the kernel.
> > >
> > > I check the code and just realized CRAS does nothing but request maximum buffer
> > > size. As I know the application needs to decide the buffer time and period time so
> > > ALSA could generate a hw_param structure with proper period size instead of using
> > > fixed constraint in machine driver because driver has no idea about the latency you
> > > want.
> > >
> > > You can use snd_pcm_hw_params_set_buffer_time_near() and
> > > snd_pcm_hw_params_set_period_time_near() to get a proper configuration of
> > > buffer and period parameters according to the latency requirement. In the CRAS
> > > code, there is a UCM variable to support this: DmaPeriodMicrosecs. I tested it on
> > > Celes and it looks quite promising. It seems to me that adding constraint in machine
> > > driver is not necessary.
> > >
> > > SectionDevice."Speaker".0 {
> > >         Value {
> > >                 PlaybackPCM "hw:chtrt5650,0"
> > >                 DmaPeriodMicrosecs "5000"
> > > ...
> > >
> > > [   52.434761] sound pcmC1D0p: hw_param
> > > [   52.434767] sound pcmC1D0p:   ACCESS 0x1
> > > [   52.434770] sound pcmC1D0p:   FORMAT 0x4
> > > [   52.434772] sound pcmC1D0p:   SUBFORMAT 0x1
> > > [   52.434776] sound pcmC1D0p:   SAMPLE_BITS [16:16]
> > > [   52.434779] sound pcmC1D0p:   FRAME_BITS [32:32]
> > > [   52.434782] sound pcmC1D0p:   CHANNELS [2:2]
> > > [   52.434785] sound pcmC1D0p:   RATE [48000:48000]
> > > [   52.434788] sound pcmC1D0p:   PERIOD_TIME [5000:5000]
> > > [   52.434791] sound pcmC1D0p:   PERIOD_SIZE [240:240]
> > > [   52.434794] sound pcmC1D0p:   PERIOD_BYTES [960:960]
> > > [   52.434797] sound pcmC1D0p:   PERIODS [852:852]
> > > [   52.434799] sound pcmC1D0p:   BUFFER_TIME [4260000:4260000]
> > > [   52.434802] sound pcmC1D0p:   BUFFER_SIZE [204480:204480]
> > > [   52.434805] sound pcmC1D0p:   BUFFER_BYTES [817920:817920]
> > > [   52.434808] sound pcmC1D0p:   TICK_TIME [0:0]
> > >
> > > Regards,
> > > Brent
> > Hi Brent,
> >
> > Yes, I know we can do it to fix the issue as well. As I mentioned
> > before, we wanted to fix it in kernel because it is a real issue,
> > isn't it? Basically, a driver should work with any param it supports.
> > But in this case, everyone can trigger underrun if he or she does not
> > the period size to 240. If you still think it's not necessary, I can
> > modify UCM to make CRAS set the appropriate period size.
>
> How does it *not* work if you set other than period size 240, more
> exactly?
>
> The hw_constraint to a fixed period size must be really an exception.
> If you look at other drivers, you won't find any other doing such.
> It already indicates that something is wrong.
>
> Usually the fixed period size comes from the hardware limitation and
> defined in snd_pcm_hardware.  Or, sometimes it's an alignment issue.
> If you need more than that, you should doubt what's really not
> working.
>
>
> Takashi
Thank Takashi,

As I mentioned before, if the period size is set to 256, the measured
rate of sample-consuming will be around 49627 fps. It causes underrun
because the rate we set is 48000 fps. This behavior also happen on the
other period rate except for 240.

Thanks,
Yu-Hsuan

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Takashi Iwai]

On Tue, 11 Aug 2020 10:25:22 +0200,
Yu-Hsuan Hsu wrote:
> 
> Takashi Iwai <tiwai@suse.de> 於 2020年8月11日 週二 下午3:43寫道：
> >
> > On Tue, 11 Aug 2020 04:29:24 +0200,
> > Yu-Hsuan Hsu wrote:
> > >
> > > Lu, Brent <brent.lu@intel.com> 於 2020年8月11日 週二 上午10:17寫道：
> > > >
> > > > >
> > > > > Sorry for the late reply. CRAS does not set the period size when using it.
> > > > > The default period size is 256, which consumes the samples quickly(about 49627
> > > > > fps when the rate is 48000 fps) at the beginning of the playback.
> > > > > Since CRAS write samples with the fixed frequency, it triggers underruns
> > > > > immidiately.
> > > > >
> > > > > According to Brent, the DSP is using 240 period regardless the hw_param. If the
> > > > > period size is 256, DSP will read 256 samples each time but only consume 240
> > > > > samples until the ring buffer of DSP is full. This behavior makes the samples in
> > > > > the ring buffer of kernel consumed quickly. (Not sure whether the explanation is
> > > > > correct. Need Brent to confirm it.)
> > > > >
> > > > > Unfortunately, we can not change the behavior of DSP. After some experiments,
> > > > > we found that the issue can be fixed if we set the period size to 240. With the
> > > > > same frequency as the DSP, the samples are consumed stably. Because everyone
> > > > > can trigger this issue when using the driver without setting the period size, we
> > > > > think it is a general issue that should be fixed in the kernel.
> > > >
> > > > I check the code and just realized CRAS does nothing but request maximum buffer
> > > > size. As I know the application needs to decide the buffer time and period time so
> > > > ALSA could generate a hw_param structure with proper period size instead of using
> > > > fixed constraint in machine driver because driver has no idea about the latency you
> > > > want.
> > > >
> > > > You can use snd_pcm_hw_params_set_buffer_time_near() and
> > > > snd_pcm_hw_params_set_period_time_near() to get a proper configuration of
> > > > buffer and period parameters according to the latency requirement. In the CRAS
> > > > code, there is a UCM variable to support this: DmaPeriodMicrosecs. I tested it on
> > > > Celes and it looks quite promising. It seems to me that adding constraint in machine
> > > > driver is not necessary.
> > > >
> > > > SectionDevice."Speaker".0 {
> > > >         Value {
> > > >                 PlaybackPCM "hw:chtrt5650,0"
> > > >                 DmaPeriodMicrosecs "5000"
> > > > ...
> > > >
> > > > [   52.434761] sound pcmC1D0p: hw_param
> > > > [   52.434767] sound pcmC1D0p:   ACCESS 0x1
> > > > [   52.434770] sound pcmC1D0p:   FORMAT 0x4
> > > > [   52.434772] sound pcmC1D0p:   SUBFORMAT 0x1
> > > > [   52.434776] sound pcmC1D0p:   SAMPLE_BITS [16:16]
> > > > [   52.434779] sound pcmC1D0p:   FRAME_BITS [32:32]
> > > > [   52.434782] sound pcmC1D0p:   CHANNELS [2:2]
> > > > [   52.434785] sound pcmC1D0p:   RATE [48000:48000]
> > > > [   52.434788] sound pcmC1D0p:   PERIOD_TIME [5000:5000]
> > > > [   52.434791] sound pcmC1D0p:   PERIOD_SIZE [240:240]
> > > > [   52.434794] sound pcmC1D0p:   PERIOD_BYTES [960:960]
> > > > [   52.434797] sound pcmC1D0p:   PERIODS [852:852]
> > > > [   52.434799] sound pcmC1D0p:   BUFFER_TIME [4260000:4260000]
> > > > [   52.434802] sound pcmC1D0p:   BUFFER_SIZE [204480:204480]
> > > > [   52.434805] sound pcmC1D0p:   BUFFER_BYTES [817920:817920]
> > > > [   52.434808] sound pcmC1D0p:   TICK_TIME [0:0]
> > > >
> > > > Regards,
> > > > Brent
> > > Hi Brent,
> > >
> > > Yes, I know we can do it to fix the issue as well. As I mentioned
> > > before, we wanted to fix it in kernel because it is a real issue,
> > > isn't it? Basically, a driver should work with any param it supports.
> > > But in this case, everyone can trigger underrun if he or she does not
> > > the period size to 240. If you still think it's not necessary, I can
> > > modify UCM to make CRAS set the appropriate period size.
> >
> > How does it *not* work if you set other than period size 240, more
> > exactly?
> >
> > The hw_constraint to a fixed period size must be really an exception.
> > If you look at other drivers, you won't find any other doing such.
> > It already indicates that something is wrong.
> >
> > Usually the fixed period size comes from the hardware limitation and
> > defined in snd_pcm_hardware.  Or, sometimes it's an alignment issue.
> > If you need more than that, you should doubt what's really not
> > working.
> >
> >
> > Takashi
> Thank Takashi,
> 
> As I mentioned before, if the period size is set to 256, the measured
> rate of sample-consuming will be around 49627 fps. It causes underrun
> because the rate we set is 48000 fps.

But this explanation rather sounds like the alignment problem.
However...

> This behavior also happen on the
> other period rate except for 240.

... Why only 240?  That's the next logical question.
If you have a clarification for it, it may be the rigid reason to
introduce such a hw constraint.


Takashi

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Yu-Hsuan Hsu]

Takashi Iwai <tiwai@suse.de> 於 2020年8月11日 週二 下午4:39寫道：
>
> On Tue, 11 Aug 2020 10:25:22 +0200,
> Yu-Hsuan Hsu wrote:
> >
> > Takashi Iwai <tiwai@suse.de> 於 2020年8月11日 週二 下午3:43寫道：
> > >
> > > On Tue, 11 Aug 2020 04:29:24 +0200,
> > > Yu-Hsuan Hsu wrote:
> > > >
> > > > Lu, Brent <brent.lu@intel.com> 於 2020年8月11日 週二 上午10:17寫道：
> > > > >
> > > > > >
> > > > > > Sorry for the late reply. CRAS does not set the period size when using it.
> > > > > > The default period size is 256, which consumes the samples quickly(about 49627
> > > > > > fps when the rate is 48000 fps) at the beginning of the playback.
> > > > > > Since CRAS write samples with the fixed frequency, it triggers underruns
> > > > > > immidiately.
> > > > > >
> > > > > > According to Brent, the DSP is using 240 period regardless the hw_param. If the
> > > > > > period size is 256, DSP will read 256 samples each time but only consume 240
> > > > > > samples until the ring buffer of DSP is full. This behavior makes the samples in
> > > > > > the ring buffer of kernel consumed quickly. (Not sure whether the explanation is
> > > > > > correct. Need Brent to confirm it.)
> > > > > >
> > > > > > Unfortunately, we can not change the behavior of DSP. After some experiments,
> > > > > > we found that the issue can be fixed if we set the period size to 240. With the
> > > > > > same frequency as the DSP, the samples are consumed stably. Because everyone
> > > > > > can trigger this issue when using the driver without setting the period size, we
> > > > > > think it is a general issue that should be fixed in the kernel.
> > > > >
> > > > > I check the code and just realized CRAS does nothing but request maximum buffer
> > > > > size. As I know the application needs to decide the buffer time and period time so
> > > > > ALSA could generate a hw_param structure with proper period size instead of using
> > > > > fixed constraint in machine driver because driver has no idea about the latency you
> > > > > want.
> > > > >
> > > > > You can use snd_pcm_hw_params_set_buffer_time_near() and
> > > > > snd_pcm_hw_params_set_period_time_near() to get a proper configuration of
> > > > > buffer and period parameters according to the latency requirement. In the CRAS
> > > > > code, there is a UCM variable to support this: DmaPeriodMicrosecs. I tested it on
> > > > > Celes and it looks quite promising. It seems to me that adding constraint in machine
> > > > > driver is not necessary.
> > > > >
> > > > > SectionDevice."Speaker".0 {
> > > > >         Value {
> > > > >                 PlaybackPCM "hw:chtrt5650,0"
> > > > >                 DmaPeriodMicrosecs "5000"
> > > > > ...
> > > > >
> > > > > [   52.434761] sound pcmC1D0p: hw_param
> > > > > [   52.434767] sound pcmC1D0p:   ACCESS 0x1
> > > > > [   52.434770] sound pcmC1D0p:   FORMAT 0x4
> > > > > [   52.434772] sound pcmC1D0p:   SUBFORMAT 0x1
> > > > > [   52.434776] sound pcmC1D0p:   SAMPLE_BITS [16:16]
> > > > > [   52.434779] sound pcmC1D0p:   FRAME_BITS [32:32]
> > > > > [   52.434782] sound pcmC1D0p:   CHANNELS [2:2]
> > > > > [   52.434785] sound pcmC1D0p:   RATE [48000:48000]
> > > > > [   52.434788] sound pcmC1D0p:   PERIOD_TIME [5000:5000]
> > > > > [   52.434791] sound pcmC1D0p:   PERIOD_SIZE [240:240]
> > > > > [   52.434794] sound pcmC1D0p:   PERIOD_BYTES [960:960]
> > > > > [   52.434797] sound pcmC1D0p:   PERIODS [852:852]
> > > > > [   52.434799] sound pcmC1D0p:   BUFFER_TIME [4260000:4260000]
> > > > > [   52.434802] sound pcmC1D0p:   BUFFER_SIZE [204480:204480]
> > > > > [   52.434805] sound pcmC1D0p:   BUFFER_BYTES [817920:817920]
> > > > > [   52.434808] sound pcmC1D0p:   TICK_TIME [0:0]
> > > > >
> > > > > Regards,
> > > > > Brent
> > > > Hi Brent,
> > > >
> > > > Yes, I know we can do it to fix the issue as well. As I mentioned
> > > > before, we wanted to fix it in kernel because it is a real issue,
> > > > isn't it? Basically, a driver should work with any param it supports.
> > > > But in this case, everyone can trigger underrun if he or she does not
> > > > the period size to 240. If you still think it's not necessary, I can
> > > > modify UCM to make CRAS set the appropriate period size.
> > >
> > > How does it *not* work if you set other than period size 240, more
> > > exactly?
> > >
> > > The hw_constraint to a fixed period size must be really an exception.
> > > If you look at other drivers, you won't find any other doing such.
> > > It already indicates that something is wrong.
> > >
> > > Usually the fixed period size comes from the hardware limitation and
> > > defined in snd_pcm_hardware.  Or, sometimes it's an alignment issue.
> > > If you need more than that, you should doubt what's really not
> > > working.
> > >
> > >
> > > Takashi
> > Thank Takashi,
> >
> > As I mentioned before, if the period size is set to 256, the measured
> > rate of sample-consuming will be around 49627 fps. It causes underrun
> > because the rate we set is 48000 fps.
>
> But this explanation rather sounds like the alignment problem.
> However...
>
> > This behavior also happen on the
> > other period rate except for 240.
>
> ... Why only 240?  That's the next logical question.
> If you have a clarification for it, it may be the rigid reason to
> introduce such a hw constraint.
>
>
> Takashi

According to Brent, the DSP is using 240 period regardless the
hw_param. If the period size is 256, DSP will read 256 samples each
time but only consume 240 samples until the ring buffer of DSP is
full. This behavior makes the samples in the ring buffer of kernel
consumed quickly.

Not sure whether the explanation is correct. Hi Brent, can you confirm it?

Thanks,
Yu-Hsuan

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1206 bytes --]

On Tue, Aug 11, 2020 at 05:35:45PM +0800, Yu-Hsuan Hsu wrote:
> Takashi Iwai <tiwai@suse.de> 於 2020年8月11日 週二 下午4:39寫道：

> > ... Why only 240?  That's the next logical question.
> > If you have a clarification for it, it may be the rigid reason to
> > introduce such a hw constraint.

> According to Brent, the DSP is using 240 period regardless the
> hw_param. If the period size is 256, DSP will read 256 samples each
> time but only consume 240 samples until the ring buffer of DSP is
> full. This behavior makes the samples in the ring buffer of kernel
> consumed quickly.

> Not sure whether the explanation is correct. Hi Brent, can you confirm it?

This seems to be going round and round in circles.  Userspace lets the
kernel pick the period size, if the period size isn't 240 (or a multiple
of it?) the DSP doesn't properly pay attention to that apparently due to
internal hard coding in the DSP firmware which we can't change so the
constraint logic needs to know about this DSP limitation - it seems like
none of this is going to change without something new going into the
mix?  We at least need a new question to ask about the DSP firmware I
think.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Pierre-Louis Bossart]

>>> ... Why only 240?  That's the next logical question.
>>> If you have a clarification for it, it may be the rigid reason to
>>> introduce such a hw constraint.
> 
>> According to Brent, the DSP is using 240 period regardless the
>> hw_param. If the period size is 256, DSP will read 256 samples each
>> time but only consume 240 samples until the ring buffer of DSP is
>> full. This behavior makes the samples in the ring buffer of kernel
>> consumed quickly.
> 
>> Not sure whether the explanation is correct. Hi Brent, can you confirm it?
> 
> This seems to be going round and round in circles.  Userspace lets the
> kernel pick the period size, if the period size isn't 240 (or a multiple
> of it?) the DSP doesn't properly pay attention to that apparently due to
> internal hard coding in the DSP firmware which we can't change so the
> constraint logic needs to know about this DSP limitation - it seems like
> none of this is going to change without something new going into the
> mix?  We at least need a new question to ask about the DSP firmware I
> think.

I just tested aplay -Dhw: on a Cyan Chromebook with the Ubuntu kernel 
5.4, and I see no issues with the 240 sample period. Same with 432, 960, 
9600, etc.

I also tried just for fun what happens with 256 samples, and I don't see 
any underflows thrown either, so I am wondering what exactly the problem 
is? Something's not adding up. I would definitively favor multiple of 
1ms periods, since it's the only case that was productized, but there's 
got to me something a side effect of how CRAS programs the hw_params.

root@chrx:~# aplay -Dhw:0,0 --period-size=240 --buffer-size=480 -v 1.wav
Playing WAVE '1.wav' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo
Hardware PCM card 0 'chtmax98090' device 0 subdevice 0
Its setup is:
   stream       : PLAYBACK
   access       : RW_INTERLEAVED
   format       : S16_LE
   subformat    : STD
   channels     : 2
   rate         : 48000
   exact rate   : 48000 (48000/1)
   msbits       : 16
   buffer_size  : 480
   period_size  : 240
   period_time  : 5000
   tstamp_mode  : NONE
   tstamp_type  : MONOTONIC
   period_step  : 1
   avail_min    : 240
   period_event : 0
   start_threshold  : 480
   stop_threshold   : 480
   silence_threshold: 0
   silence_size : 0
   boundary     : 8646911284551352320
   appl_ptr     : 0
   hw_ptr       : 0

root@chrx:~# aplay -Dhw:0,0 --period-size=256 --buffer-size=512 -v 1.wav
Playing WAVE '1.wav' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo
Hardware PCM card 0 'chtmax98090' device 0 subdevice 0
Its setup is:
   stream       : PLAYBACK
   access       : RW_INTERLEAVED
   format       : S16_LE
   subformat    : STD
   channels     : 2
   rate         : 48000
   exact rate   : 48000 (48000/1)
   msbits       : 16
   buffer_size  : 512
   period_size  : 256
   period_time  : 5333
   tstamp_mode  : NONE
   tstamp_type  : MONOTONIC
   period_step  : 1
   avail_min    : 256
   period_event : 0
   start_threshold  : 512
   stop_threshold   : 512
   silence_threshold: 0
   silence_size : 0
   boundary     : 4611686018427387904
   appl_ptr     : 0
   hw_ptr       : 0

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 952 bytes --]

On Tue, Aug 11, 2020 at 11:54:38AM -0500, Pierre-Louis Bossart wrote:

> > constraint logic needs to know about this DSP limitation - it seems like
> > none of this is going to change without something new going into the
> > mix?  We at least need a new question to ask about the DSP firmware I
> > think.

> I just tested aplay -Dhw: on a Cyan Chromebook with the Ubuntu kernel 5.4,
> and I see no issues with the 240 sample period. Same with 432, 960, 9600,
> etc.

> I also tried just for fun what happens with 256 samples, and I don't see any
> underflows thrown either, so I am wondering what exactly the problem is?
> Something's not adding up. I would definitively favor multiple of 1ms
> periods, since it's the only case that was productized, but there's got to
> me something a side effect of how CRAS programs the hw_params.

Is it something that goes wrong with longer playbacks possibly (eg,
someone watching a feature film or something)?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Yu-Hsuan Hsu]

Mark Brown <broonie@kernel.org> 於 2020年8月12日 週三 上午1:22寫道：
>
> On Tue, Aug 11, 2020 at 11:54:38AM -0500, Pierre-Louis Bossart wrote:
>
> > > constraint logic needs to know about this DSP limitation - it seems like
> > > none of this is going to change without something new going into the
> > > mix?  We at least need a new question to ask about the DSP firmware I
> > > think.
>
> > I just tested aplay -Dhw: on a Cyan Chromebook with the Ubuntu kernel 5.4,
> > and I see no issues with the 240 sample period. Same with 432, 960, 9600,
> > etc.
>
> > I also tried just for fun what happens with 256 samples, and I don't see any
> > underflows thrown either, so I am wondering what exactly the problem is?
> > Something's not adding up. I would definitively favor multiple of 1ms
> > periods, since it's the only case that was productized, but there's got to
> > me something a side effect of how CRAS programs the hw_params.
>
> Is it something that goes wrong with longer playbacks possibly (eg,
> someone watching a feature film or something)?

Thanks for testing!

After doing some experiments, I think I can identify the problem more precisely.
1. aplay can not reproduce this issue because it writes samples
immediately when there are some space in the buffer. However, you can
add --test-position to see how the delay grows with period size 256.
> aplay -Dhw:1,0 --period-size=256 --buffer-size=480 /dev/zero -d 1 -f dat --test-position
Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
Hz, Stereo
Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer = 512
Suspicious buffer position (2 total): avail = 0, delay = 2064, buffer = 512
Suspicious buffer position (3 total): avail = 0, delay = 2096, buffer = 512
...

2. Since many samples are moved to DSP(delay), the measured rate of
the ring-buffer is high. (I measured it by alsa_conformance_test,
which only test the sampling rate in the ring buffer of kernel not
DSP)

3. Since CRAS writes samples with a fixed frequency, this behavior
will take all samples from the ring buffer, which is seen as underrun
by CRAS. (It seems that it is not a real underrun because that avail
does not larger than buffer size. Maybe CRAS should also take dalay
into account.)

4. In spite of it is not a real underrun, the large delay is still a
big problem. Can we apply the constraint to fix it? Or any better
idea?

Thanks,
Yu-Hsuan

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Takashi Iwai]

On Wed, 12 Aug 2020 05:09:58 +0200,
Yu-Hsuan Hsu wrote:
> 
> Mark Brown <broonie@kernel.org> 於 2020年8月12日 週三 上午1:22寫道：
> >
> > On Tue, Aug 11, 2020 at 11:54:38AM -0500, Pierre-Louis Bossart wrote:
> >
> > > > constraint logic needs to know about this DSP limitation - it seems like
> > > > none of this is going to change without something new going into the
> > > > mix?  We at least need a new question to ask about the DSP firmware I
> > > > think.
> >
> > > I just tested aplay -Dhw: on a Cyan Chromebook with the Ubuntu kernel 5.4,
> > > and I see no issues with the 240 sample period. Same with 432, 960, 9600,
> > > etc.
> >
> > > I also tried just for fun what happens with 256 samples, and I don't see any
> > > underflows thrown either, so I am wondering what exactly the problem is?
> > > Something's not adding up. I would definitively favor multiple of 1ms
> > > periods, since it's the only case that was productized, but there's got to
> > > me something a side effect of how CRAS programs the hw_params.
> >
> > Is it something that goes wrong with longer playbacks possibly (eg,
> > someone watching a feature film or something)?
> 
> Thanks for testing!
> 
> After doing some experiments, I think I can identify the problem more precisely.
> 1. aplay can not reproduce this issue because it writes samples
> immediately when there are some space in the buffer. However, you can
> add --test-position to see how the delay grows with period size 256.
> > aplay -Dhw:1,0 --period-size=256 --buffer-size=480 /dev/zero -d 1 -f dat --test-position
> Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
> Hz, Stereo
> Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer = 512
> Suspicious buffer position (2 total): avail = 0, delay = 2064, buffer = 512
> Suspicious buffer position (3 total): avail = 0, delay = 2096, buffer = 512
> ...

Isn't this about the alignment of the buffer size against the period
size, not the period size itself?  i.e. in the example above, the
buffer size isn't a multiple of period size, and DSP can't handle if
the position overlaps the buffer size in a half way.

If that's the problem (and it's an oft-seen restriction), the right
constraint is
  snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);


Takashi

> 2. Since many samples are moved to DSP(delay), the measured rate of
> the ring-buffer is high. (I measured it by alsa_conformance_test,
> which only test the sampling rate in the ring buffer of kernel not
> DSP)
> 
> 3. Since CRAS writes samples with a fixed frequency, this behavior
> will take all samples from the ring buffer, which is seen as underrun
> by CRAS. (It seems that it is not a real underrun because that avail
> does not larger than buffer size. Maybe CRAS should also take dalay
> into account.)
> 
> 4. In spite of it is not a real underrun, the large delay is still a
> big problem. Can we apply the constraint to fix it? Or any better
> idea?
> 
> Thanks,
> Yu-Hsuan
> 

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Yu-Hsuan Hsu]

Takashi Iwai <tiwai@suse.de> 於 2020年8月12日 週三 下午2:14寫道：
>
> On Wed, 12 Aug 2020 05:09:58 +0200,
> Yu-Hsuan Hsu wrote:
> >
> > Mark Brown <broonie@kernel.org> 於 2020年8月12日 週三 上午1:22寫道：
> > >
> > > On Tue, Aug 11, 2020 at 11:54:38AM -0500, Pierre-Louis Bossart wrote:
> > >
> > > > > constraint logic needs to know about this DSP limitation - it seems like
> > > > > none of this is going to change without something new going into the
> > > > > mix?  We at least need a new question to ask about the DSP firmware I
> > > > > think.
> > >
> > > > I just tested aplay -Dhw: on a Cyan Chromebook with the Ubuntu kernel 5.4,
> > > > and I see no issues with the 240 sample period. Same with 432, 960, 9600,
> > > > etc.
> > >
> > > > I also tried just for fun what happens with 256 samples, and I don't see any
> > > > underflows thrown either, so I am wondering what exactly the problem is?
> > > > Something's not adding up. I would definitively favor multiple of 1ms
> > > > periods, since it's the only case that was productized, but there's got to
> > > > me something a side effect of how CRAS programs the hw_params.
> > >
> > > Is it something that goes wrong with longer playbacks possibly (eg,
> > > someone watching a feature film or something)?
> >
> > Thanks for testing!
> >
> > After doing some experiments, I think I can identify the problem more precisely.
> > 1. aplay can not reproduce this issue because it writes samples
> > immediately when there are some space in the buffer. However, you can
> > add --test-position to see how the delay grows with period size 256.
> > > aplay -Dhw:1,0 --period-size=256 --buffer-size=480 /dev/zero -d 1 -f dat --test-position
> > Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
> > Hz, Stereo
> > Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer = 512
> > Suspicious buffer position (2 total): avail = 0, delay = 2064, buffer = 512
> > Suspicious buffer position (3 total): avail = 0, delay = 2096, buffer = 512
> > ...
>
> Isn't this about the alignment of the buffer size against the period
> size, not the period size itself?  i.e. in the example above, the
> buffer size isn't a multiple of period size, and DSP can't handle if
> the position overlaps the buffer size in a half way.
>
> If that's the problem (and it's an oft-seen restriction), the right
> constraint is
>   snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
>
>
> Takashi
Oh sorry for my typo. The issue happens no matter what buffer size is
set. Actually, even if I want to set 480, it will change to 512
automatically.
Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer
= 512 <-this one is the buffer size

>
> > 2. Since many samples are moved to DSP(delay), the measured rate of
> > the ring-buffer is high. (I measured it by alsa_conformance_test,
> > which only test the sampling rate in the ring buffer of kernel not
> > DSP)
> >
> > 3. Since CRAS writes samples with a fixed frequency, this behavior
> > will take all samples from the ring buffer, which is seen as underrun
> > by CRAS. (It seems that it is not a real underrun because that avail
> > does not larger than buffer size. Maybe CRAS should also take dalay
> > into account.)
> >
> > 4. In spite of it is not a real underrun, the large delay is still a
> > big problem. Can we apply the constraint to fix it? Or any better
> > idea?
> >
> > Thanks,
> > Yu-Hsuan
> >

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Takashi Iwai]

On Wed, 12 Aug 2020 08:53:42 +0200,
Yu-Hsuan Hsu wrote:
> 
> Takashi Iwai <tiwai@suse.de> 於 2020年8月12日 週三 下午2:14寫道：
> >
> > On Wed, 12 Aug 2020 05:09:58 +0200,
> > Yu-Hsuan Hsu wrote:
> > >
> > > Mark Brown <broonie@kernel.org> 於 2020年8月12日 週三 上午1:22寫道：
> > > >
> > > > On Tue, Aug 11, 2020 at 11:54:38AM -0500, Pierre-Louis Bossart wrote:
> > > >
> > > > > > constraint logic needs to know about this DSP limitation - it seems like
> > > > > > none of this is going to change without something new going into the
> > > > > > mix?  We at least need a new question to ask about the DSP firmware I
> > > > > > think.
> > > >
> > > > > I just tested aplay -Dhw: on a Cyan Chromebook with the Ubuntu kernel 5.4,
> > > > > and I see no issues with the 240 sample period. Same with 432, 960, 9600,
> > > > > etc.
> > > >
> > > > > I also tried just for fun what happens with 256 samples, and I don't see any
> > > > > underflows thrown either, so I am wondering what exactly the problem is?
> > > > > Something's not adding up. I would definitively favor multiple of 1ms
> > > > > periods, since it's the only case that was productized, but there's got to
> > > > > me something a side effect of how CRAS programs the hw_params.
> > > >
> > > > Is it something that goes wrong with longer playbacks possibly (eg,
> > > > someone watching a feature film or something)?
> > >
> > > Thanks for testing!
> > >
> > > After doing some experiments, I think I can identify the problem more precisely.
> > > 1. aplay can not reproduce this issue because it writes samples
> > > immediately when there are some space in the buffer. However, you can
> > > add --test-position to see how the delay grows with period size 256.
> > > > aplay -Dhw:1,0 --period-size=256 --buffer-size=480 /dev/zero -d 1 -f dat --test-position
> > > Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
> > > Hz, Stereo
> > > Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer = 512
> > > Suspicious buffer position (2 total): avail = 0, delay = 2064, buffer = 512
> > > Suspicious buffer position (3 total): avail = 0, delay = 2096, buffer = 512
> > > ...
> >
> > Isn't this about the alignment of the buffer size against the period
> > size, not the period size itself?  i.e. in the example above, the
> > buffer size isn't a multiple of period size, and DSP can't handle if
> > the position overlaps the buffer size in a half way.
> >
> > If that's the problem (and it's an oft-seen restriction), the right
> > constraint is
> >   snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
> >
> >
> > Takashi
> Oh sorry for my typo. The issue happens no matter what buffer size is
> set. Actually, even if I want to set 480, it will change to 512
> automatically.
> Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer
> = 512 <-this one is the buffer size

OK, then it means that the buffer size alignment is already in place.

And this large delay won't happen if you use period size 240?


Takashi

> > > 2. Since many samples are moved to DSP(delay), the measured rate of
> > > the ring-buffer is high. (I measured it by alsa_conformance_test,
> > > which only test the sampling rate in the ring buffer of kernel not
> > > DSP)
> > >
> > > 3. Since CRAS writes samples with a fixed frequency, this behavior
> > > will take all samples from the ring buffer, which is seen as underrun
> > > by CRAS. (It seems that it is not a real underrun because that avail
> > > does not larger than buffer size. Maybe CRAS should also take dalay
> > > into account.)
> > >
> > > 4. In spite of it is not a real underrun, the large delay is still a
> > > big problem. Can we apply the constraint to fix it? Or any better
> > > idea?
> > >
> > > Thanks,
> > > Yu-Hsuan
> > >
> 

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Yu-Hsuan Hsu]

Takashi Iwai <tiwai@suse.de> 於 2020年8月12日 週三 下午2:58寫道：
>
> On Wed, 12 Aug 2020 08:53:42 +0200,
> Yu-Hsuan Hsu wrote:
> >
> > Takashi Iwai <tiwai@suse.de> 於 2020年8月12日 週三 下午2:14寫道：
> > >
> > > On Wed, 12 Aug 2020 05:09:58 +0200,
> > > Yu-Hsuan Hsu wrote:
> > > >
> > > > Mark Brown <broonie@kernel.org> 於 2020年8月12日 週三 上午1:22寫道：
> > > > >
> > > > > On Tue, Aug 11, 2020 at 11:54:38AM -0500, Pierre-Louis Bossart wrote:
> > > > >
> > > > > > > constraint logic needs to know about this DSP limitation - it seems like
> > > > > > > none of this is going to change without something new going into the
> > > > > > > mix?  We at least need a new question to ask about the DSP firmware I
> > > > > > > think.
> > > > >
> > > > > > I just tested aplay -Dhw: on a Cyan Chromebook with the Ubuntu kernel 5.4,
> > > > > > and I see no issues with the 240 sample period. Same with 432, 960, 9600,
> > > > > > etc.
> > > > >
> > > > > > I also tried just for fun what happens with 256 samples, and I don't see any
> > > > > > underflows thrown either, so I am wondering what exactly the problem is?
> > > > > > Something's not adding up. I would definitively favor multiple of 1ms
> > > > > > periods, since it's the only case that was productized, but there's got to
> > > > > > me something a side effect of how CRAS programs the hw_params.
> > > > >
> > > > > Is it something that goes wrong with longer playbacks possibly (eg,
> > > > > someone watching a feature film or something)?
> > > >
> > > > Thanks for testing!
> > > >
> > > > After doing some experiments, I think I can identify the problem more precisely.
> > > > 1. aplay can not reproduce this issue because it writes samples
> > > > immediately when there are some space in the buffer. However, you can
> > > > add --test-position to see how the delay grows with period size 256.
> > > > > aplay -Dhw:1,0 --period-size=256 --buffer-size=480 /dev/zero -d 1 -f dat --test-position
> > > > Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
> > > > Hz, Stereo
> > > > Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer = 512
> > > > Suspicious buffer position (2 total): avail = 0, delay = 2064, buffer = 512
> > > > Suspicious buffer position (3 total): avail = 0, delay = 2096, buffer = 512
> > > > ...
> > >
> > > Isn't this about the alignment of the buffer size against the period
> > > size, not the period size itself?  i.e. in the example above, the
> > > buffer size isn't a multiple of period size, and DSP can't handle if
> > > the position overlaps the buffer size in a half way.
> > >
> > > If that's the problem (and it's an oft-seen restriction), the right
> > > constraint is
> > >   snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
> > >
> > >
> > > Takashi
> > Oh sorry for my typo. The issue happens no matter what buffer size is
> > set. Actually, even if I want to set 480, it will change to 512
> > automatically.
> > Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer
> > = 512 <-this one is the buffer size
>
> OK, then it means that the buffer size alignment is already in place.
>
> And this large delay won't happen if you use period size 240?
>
>
> Takashi
Yes! If I set the period size to 240, it will not print "Suspicious
buffer position ..."

Yu-Hsuan

>
> > > > 2. Since many samples are moved to DSP(delay), the measured rate of
> > > > the ring-buffer is high. (I measured it by alsa_conformance_test,
> > > > which only test the sampling rate in the ring buffer of kernel not
> > > > DSP)
> > > >
> > > > 3. Since CRAS writes samples with a fixed frequency, this behavior
> > > > will take all samples from the ring buffer, which is seen as underrun
> > > > by CRAS. (It seems that it is not a real underrun because that avail
> > > > does not larger than buffer size. Maybe CRAS should also take dalay
> > > > into account.)
> > > >
> > > > 4. In spite of it is not a real underrun, the large delay is still a
> > > > big problem. Can we apply the constraint to fix it? Or any better
> > > > idea?
> > > >
> > > > Thanks,
> > > > Yu-Hsuan
> > > >
> >

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Takashi Iwai]

On Wed, 12 Aug 2020 09:43:22 +0200,
Yu-Hsuan Hsu wrote:
> 
> Takashi Iwai <tiwai@suse.de> 於 2020年8月12日 週三 下午2:58寫道：
> >
> > On Wed, 12 Aug 2020 08:53:42 +0200,
> > Yu-Hsuan Hsu wrote:
> > >
> > > Takashi Iwai <tiwai@suse.de> 於 2020年8月12日 週三 下午2:14寫道：
> > > >
> > > > On Wed, 12 Aug 2020 05:09:58 +0200,
> > > > Yu-Hsuan Hsu wrote:
> > > > >
> > > > > Mark Brown <broonie@kernel.org> 於 2020年8月12日 週三 上午1:22寫道：
> > > > > >
> > > > > > On Tue, Aug 11, 2020 at 11:54:38AM -0500, Pierre-Louis Bossart wrote:
> > > > > >
> > > > > > > > constraint logic needs to know about this DSP limitation - it seems like
> > > > > > > > none of this is going to change without something new going into the
> > > > > > > > mix?  We at least need a new question to ask about the DSP firmware I
> > > > > > > > think.
> > > > > >
> > > > > > > I just tested aplay -Dhw: on a Cyan Chromebook with the Ubuntu kernel 5.4,
> > > > > > > and I see no issues with the 240 sample period. Same with 432, 960, 9600,
> > > > > > > etc.
> > > > > >
> > > > > > > I also tried just for fun what happens with 256 samples, and I don't see any
> > > > > > > underflows thrown either, so I am wondering what exactly the problem is?
> > > > > > > Something's not adding up. I would definitively favor multiple of 1ms
> > > > > > > periods, since it's the only case that was productized, but there's got to
> > > > > > > me something a side effect of how CRAS programs the hw_params.
> > > > > >
> > > > > > Is it something that goes wrong with longer playbacks possibly (eg,
> > > > > > someone watching a feature film or something)?
> > > > >
> > > > > Thanks for testing!
> > > > >
> > > > > After doing some experiments, I think I can identify the problem more precisely.
> > > > > 1. aplay can not reproduce this issue because it writes samples
> > > > > immediately when there are some space in the buffer. However, you can
> > > > > add --test-position to see how the delay grows with period size 256.
> > > > > > aplay -Dhw:1,0 --period-size=256 --buffer-size=480 /dev/zero -d 1 -f dat --test-position
> > > > > Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
> > > > > Hz, Stereo
> > > > > Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer = 512
> > > > > Suspicious buffer position (2 total): avail = 0, delay = 2064, buffer = 512
> > > > > Suspicious buffer position (3 total): avail = 0, delay = 2096, buffer = 512
> > > > > ...
> > > >
> > > > Isn't this about the alignment of the buffer size against the period
> > > > size, not the period size itself?  i.e. in the example above, the
> > > > buffer size isn't a multiple of period size, and DSP can't handle if
> > > > the position overlaps the buffer size in a half way.
> > > >
> > > > If that's the problem (and it's an oft-seen restriction), the right
> > > > constraint is
> > > >   snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
> > > >
> > > >
> > > > Takashi
> > > Oh sorry for my typo. The issue happens no matter what buffer size is
> > > set. Actually, even if I want to set 480, it will change to 512
> > > automatically.
> > > Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer
> > > = 512 <-this one is the buffer size
> >
> > OK, then it means that the buffer size alignment is already in place.
> >
> > And this large delay won't happen if you use period size 240?
> >
> >
> > Takashi
> Yes! If I set the period size to 240, it will not print "Suspicious
> buffer position ..."

So it sounds like DSP handles the delay report incorrectly.
Then it comes to another question: the driver supports both SOF and
SST.  Is there the behavior difference between both DSPs wrt this
delay issue?


Takashi

> 
> Yu-Hsuan
> 
> >
> > > > > 2. Since many samples are moved to DSP(delay), the measured rate of
> > > > > the ring-buffer is high. (I measured it by alsa_conformance_test,
> > > > > which only test the sampling rate in the ring buffer of kernel not
> > > > > DSP)
> > > > >
> > > > > 3. Since CRAS writes samples with a fixed frequency, this behavior
> > > > > will take all samples from the ring buffer, which is seen as underrun
> > > > > by CRAS. (It seems that it is not a real underrun because that avail
> > > > > does not larger than buffer size. Maybe CRAS should also take dalay
> > > > > into account.)
> > > > >
> > > > > 4. In spite of it is not a real underrun, the large delay is still a
> > > > > big problem. Can we apply the constraint to fix it? Or any better
> > > > > idea?
> > > > >
> > > > > Thanks,
> > > > > Yu-Hsuan
> > > > >
> > >
> 

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Pierre-Louis Bossart]

>>>>>> After doing some experiments, I think I can identify the problem more precisely.
>>>>>> 1. aplay can not reproduce this issue because it writes samples
>>>>>> immediately when there are some space in the buffer. However, you can
>>>>>> add --test-position to see how the delay grows with period size 256.
>>>>>>> aplay -Dhw:1,0 --period-size=256 --buffer-size=480 /dev/zero -d 1 -f dat --test-position
>>>>>> Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
>>>>>> Hz, Stereo
>>>>>> Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer = 512
>>>>>> Suspicious buffer position (2 total): avail = 0, delay = 2064, buffer = 512
>>>>>> Suspicious buffer position (3 total): avail = 0, delay = 2096, buffer = 512
>>>>>> ...
>>>>>
>>>>> Isn't this about the alignment of the buffer size against the period
>>>>> size, not the period size itself?  i.e. in the example above, the
>>>>> buffer size isn't a multiple of period size, and DSP can't handle if
>>>>> the position overlaps the buffer size in a half way.
>>>>>
>>>>> If that's the problem (and it's an oft-seen restriction), the right
>>>>> constraint is
>>>>>    snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
>>>>>
>>>>>
>>>>> Takashi
>>>> Oh sorry for my typo. The issue happens no matter what buffer size is
>>>> set. Actually, even if I want to set 480, it will change to 512
>>>> automatically.
>>>> Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer
>>>> = 512 <-this one is the buffer size
>>>
>>> OK, then it means that the buffer size alignment is already in place.
>>>
>>> And this large delay won't happen if you use period size 240?
>>>
>>>
>>> Takashi
>> Yes! If I set the period size to 240, it will not print "Suspicious
>> buffer position ..."
> 
> So it sounds like DSP handles the delay report incorrectly.
> Then it comes to another question: the driver supports both SOF and
> SST.  Is there the behavior difference between both DSPs wrt this
> delay issue?

I still don't get what the issue is. The two following cases work fine 
with the SST/Atom driver:

root@chrx:~# aplay -Dhw:0,0 --period-size=240 --buffer-size=480 
/dev/zero -d 2 -f dat --test-position
Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000 
Hz, Stereo
root@chrx:~# aplay -Dhw:0,0 --period-size=960 --buffer-size=4800 
/dev/zero -d 2 -f dat --test-position
Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000 
Hz, Stereo

The existing code has this:

	/* Make sure, that the period size is always even */
	snd_pcm_hw_constraint_step(substream->runtime, 0,
			   SNDRV_PCM_HW_PARAM_PERIODS, 2);

	return snd_pcm_hw_constraint_integer(runtime,
			 SNDRV_PCM_HW_PARAM_PERIODS);

and with the addition of period size being a multiple of 1ms all 
requirements should be met?

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Takashi Iwai]

On Wed, 12 Aug 2020 16:46:40 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> >>>>>> After doing some experiments, I think I can identify the problem more precisely.
> >>>>>> 1. aplay can not reproduce this issue because it writes samples
> >>>>>> immediately when there are some space in the buffer. However, you can
> >>>>>> add --test-position to see how the delay grows with period size 256.
> >>>>>>> aplay -Dhw:1,0 --period-size=256 --buffer-size=480 /dev/zero -d 1 -f dat --test-position
> >>>>>> Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
> >>>>>> Hz, Stereo
> >>>>>> Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer = 512
> >>>>>> Suspicious buffer position (2 total): avail = 0, delay = 2064, buffer = 512
> >>>>>> Suspicious buffer position (3 total): avail = 0, delay = 2096, buffer = 512
> >>>>>> ...
> >>>>>
> >>>>> Isn't this about the alignment of the buffer size against the period
> >>>>> size, not the period size itself?  i.e. in the example above, the
> >>>>> buffer size isn't a multiple of period size, and DSP can't handle if
> >>>>> the position overlaps the buffer size in a half way.
> >>>>>
> >>>>> If that's the problem (and it's an oft-seen restriction), the right
> >>>>> constraint is
> >>>>>    snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
> >>>>>
> >>>>>
> >>>>> Takashi
> >>>> Oh sorry for my typo. The issue happens no matter what buffer size is
> >>>> set. Actually, even if I want to set 480, it will change to 512
> >>>> automatically.
> >>>> Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer
> >>>> = 512 <-this one is the buffer size
> >>>
> >>> OK, then it means that the buffer size alignment is already in place.
> >>>
> >>> And this large delay won't happen if you use period size 240?
> >>>
> >>>
> >>> Takashi
> >> Yes! If I set the period size to 240, it will not print "Suspicious
> >> buffer position ..."
> >
> > So it sounds like DSP handles the delay report incorrectly.
> > Then it comes to another question: the driver supports both SOF and
> > SST.  Is there the behavior difference between both DSPs wrt this
> > delay issue?
> 
> I still don't get what the issue is. The two following cases work fine
> with the SST/Atom driver:
> 
> root@chrx:~# aplay -Dhw:0,0 --period-size=240 --buffer-size=480
> /dev/zero -d 2 -f dat --test-position
> Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
> Hz, Stereo
> root@chrx:~# aplay -Dhw:0,0 --period-size=960 --buffer-size=4800
> /dev/zero -d 2 -f dat --test-position
> Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
> Hz, Stereo

What if with --period-size=256 --buffer-size=512 and --test-position?
Can you reproduce the problem in your side?

> The existing code has this:
> 
> 	/* Make sure, that the period size is always even */
> 	snd_pcm_hw_constraint_step(substream->runtime, 0,
> 			   SNDRV_PCM_HW_PARAM_PERIODS, 2);
> 
> 	return snd_pcm_hw_constraint_integer(runtime,
> 			 SNDRV_PCM_HW_PARAM_PERIODS);
> 
> and with the addition of period size being a multiple of 1ms all
> requirements should be met?

I also wonder what's really missing, too :)

BTW, I took a look back at the thread, and CRAS seems using a very
large buffer, namely:
[   52.434791] sound pcmC1D0p:   PERIOD_SIZE [240:240]
[   52.434802] sound pcmC1D0p:   BUFFER_SIZE [204480:204480]


Takashi

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Pierre-Louis Bossart]

On 8/12/20 9:55 AM, Takashi Iwai wrote:
> On Wed, 12 Aug 2020 16:46:40 +0200,
> Pierre-Louis Bossart wrote:
>>
>>
>>>>>>>> After doing some experiments, I think I can identify the problem more precisely.
>>>>>>>> 1. aplay can not reproduce this issue because it writes samples
>>>>>>>> immediately when there are some space in the buffer. However, you can
>>>>>>>> add --test-position to see how the delay grows with period size 256.
>>>>>>>>> aplay -Dhw:1,0 --period-size=256 --buffer-size=480 /dev/zero -d 1 -f dat --test-position
>>>>>>>> Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
>>>>>>>> Hz, Stereo
>>>>>>>> Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer = 512
>>>>>>>> Suspicious buffer position (2 total): avail = 0, delay = 2064, buffer = 512
>>>>>>>> Suspicious buffer position (3 total): avail = 0, delay = 2096, buffer = 512
>>>>>>>> ...
>>>>>>>
>>>>>>> Isn't this about the alignment of the buffer size against the period
>>>>>>> size, not the period size itself?  i.e. in the example above, the
>>>>>>> buffer size isn't a multiple of period size, and DSP can't handle if
>>>>>>> the position overlaps the buffer size in a half way.
>>>>>>>
>>>>>>> If that's the problem (and it's an oft-seen restriction), the right
>>>>>>> constraint is
>>>>>>>     snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
>>>>>>>
>>>>>>>
>>>>>>> Takashi
>>>>>> Oh sorry for my typo. The issue happens no matter what buffer size is
>>>>>> set. Actually, even if I want to set 480, it will change to 512
>>>>>> automatically.
>>>>>> Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer
>>>>>> = 512 <-this one is the buffer size
>>>>>
>>>>> OK, then it means that the buffer size alignment is already in place.
>>>>>
>>>>> And this large delay won't happen if you use period size 240?
>>>>>
>>>>>
>>>>> Takashi
>>>> Yes! If I set the period size to 240, it will not print "Suspicious
>>>> buffer position ..."
>>>
>>> So it sounds like DSP handles the delay report incorrectly.
>>> Then it comes to another question: the driver supports both SOF and
>>> SST.  Is there the behavior difference between both DSPs wrt this
>>> delay issue?
>>
>> I still don't get what the issue is. The two following cases work fine
>> with the SST/Atom driver:
>>
>> root@chrx:~# aplay -Dhw:0,0 --period-size=240 --buffer-size=480
>> /dev/zero -d 2 -f dat --test-position
>> Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
>> Hz, Stereo
>> root@chrx:~# aplay -Dhw:0,0 --period-size=960 --buffer-size=4800
>> /dev/zero -d 2 -f dat --test-position
>> Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
>> Hz, Stereo
> 
> What if with --period-size=256 --buffer-size=512 and --test-position?
> Can you reproduce the problem in your side?

Yes indeed with the existing driver:

root@chrx:~# aplay -Dhw:0,0 --period-size=256 --buffer-size=512 
/dev/zero -d 2 -f dat --test-position
Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000 
Hz, Stereo
underrun!!! (at least 0.312 ms long)
underrun!!! (at least 0.326 ms long)
Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer = 512
Suspicious buffer position (2 total): avail = 0, delay = 2064, buffer = 512
Suspicious buffer position (3 total): avail = 0, delay = 2080, buffer = 512
Suspicious buffer position (4 total): avail = 0, delay = 2080, buffer = 512
Suspicious buffer position (5 total): avail = 0, delay = 2096, buffer = 512
Suspicious buffer position (6 total): avail = 0, delay = 2096, buffer = 512

but the new constraint to force a 1ms step added in the patch1 should 
preclude this from happening.

>> The existing code has this:
>>
>> 	/* Make sure, that the period size is always even */
>> 	snd_pcm_hw_constraint_step(substream->runtime, 0,
>> 			   SNDRV_PCM_HW_PARAM_PERIODS, 2);
>>
>> 	return snd_pcm_hw_constraint_integer(runtime,
>> 			 SNDRV_PCM_HW_PARAM_PERIODS);
>>
>> and with the addition of period size being a multiple of 1ms all
>> requirements should be met?
> 
> I also wonder what's really missing, too :)
> 
> BTW, I took a look back at the thread, and CRAS seems using a very
> large buffer, namely:
> [   52.434791] sound pcmC1D0p:   PERIOD_SIZE [240:240]
> [   52.434802] sound pcmC1D0p:   BUFFER_SIZE [204480:204480]

yes, that's 852 periods and 4.260 seconds. Never seen such values :-)

RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Lu, Brent]

> >
> > I also wonder what's really missing, too :)
> >
> > BTW, I took a look back at the thread, and CRAS seems using a very
> > large buffer, namely:
> > [   52.434791] sound pcmC1D0p:   PERIOD_SIZE [240:240]
> > [   52.434802] sound pcmC1D0p:   BUFFER_SIZE [204480:204480]
> 
> yes, that's 852 periods and 4.260 seconds. Never seen such values :-)

CRAS calls snd_pcm_hw_params_set_buffer_size_max() to use as large
buffer as possible. So the period size is an arbitrary number in different
platforms. Atom SST platform happens to be 256, and CML SOF platform
is 1056 for example.


Regards,
Brent

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Pierre-Louis Bossart]

On 8/12/20 11:08 AM, Lu, Brent wrote:
>>>
>>> I also wonder what's really missing, too :)
>>>
>>> BTW, I took a look back at the thread, and CRAS seems using a very
>>> large buffer, namely:
>>> [   52.434791] sound pcmC1D0p:   PERIOD_SIZE [240:240]
>>> [   52.434802] sound pcmC1D0p:   BUFFER_SIZE [204480:204480]
>>
>> yes, that's 852 periods and 4.260 seconds. Never seen such values :-)
> 
> CRAS calls snd_pcm_hw_params_set_buffer_size_max() to use as large
> buffer as possible. So the period size is an arbitrary number in different
> platforms. Atom SST platform happens to be 256, and CML SOF platform
> is 1056 for example.

ok, but earlier in this thread it was mentioned that values such as 432 
are not suitable. the statement above seems to mean the period actual 
value is a "don't care", so I don't quite see why this specific patch2 
restricting the value to 240 is necessary. Patch1 is needed for sure, 
Patch2 is where Takashi and I are not convinced.

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Yu-Hsuan Hsu]

Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> 於
2020年8月13日 週四 上午12:38寫道：
>
>
>
> On 8/12/20 11:08 AM, Lu, Brent wrote:
> >>>
> >>> I also wonder what's really missing, too :)
> >>>
> >>> BTW, I took a look back at the thread, and CRAS seems using a very
> >>> large buffer, namely:
> >>> [   52.434791] sound pcmC1D0p:   PERIOD_SIZE [240:240]
> >>> [   52.434802] sound pcmC1D0p:   BUFFER_SIZE [204480:204480]
> >>
> >> yes, that's 852 periods and 4.260 seconds. Never seen such values :-)
> >
> > CRAS calls snd_pcm_hw_params_set_buffer_size_max() to use as large
> > buffer as possible. So the period size is an arbitrary number in different
> > platforms. Atom SST platform happens to be 256, and CML SOF platform
> > is 1056 for example.
>
> ok, but earlier in this thread it was mentioned that values such as 432
> are not suitable. the statement above seems to mean the period actual
> value is a "don't care", so I don't quite see why this specific patch2
> restricting the value to 240 is necessary. Patch1 is needed for sure,
> Patch2 is where Takashi and I are not convinced.

I have downloaded the patch1 but it does not work. After applying
patch1, the default period size changes to 320. However, it also has
the same issue with period size 320. (It can be verified by aplay.)

RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Lu, Brent]

> > >
> > > CRAS calls snd_pcm_hw_params_set_buffer_size_max() to use as large
> > > buffer as possible. So the period size is an arbitrary number in
> > > different platforms. Atom SST platform happens to be 256, and CML
> > > SOF platform is 1056 for example.
> >
> > ok, but earlier in this thread it was mentioned that values such as
> > 432 are not suitable. the statement above seems to mean the period
> > actual value is a "don't care", so I don't quite see why this specific
> > patch2 restricting the value to 240 is necessary. Patch1 is needed for
> > sure,
> > Patch2 is where Takashi and I are not convinced.
> 
> I have downloaded the patch1 but it does not work. After applying patch1,
> the default period size changes to 320. However, it also has the same issue
> with period size 320. (It can be verified by aplay.)

The period_size is related to the audio latency so it's decided by application
according to the use case it's running. That's why there are concerns about
patch 2 and also you cannot find similar constraints in other machine driver.

Another problem is the buffer size. Too large buffer is not just wasting memories.
It also creates problems to memory allocator since continuous pages are not
always there. Using a small period_count like 2 or 4 should be sufficient for audio
data transfer.

buffer_size = period_size * period_count * 1000000 / sample_rate;
snd_pcm_hw_params_set_buffer_time_near(mPcmDevice, params, &buffer_size, NULL);

And one more problem here: you need to decide period_size and period_count
first in order to calculate the buffer size...


Regards,
Brent

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Yu-Hsuan Hsu]

Lu, Brent <brent.lu@intel.com> 於 2020年8月13日 週四 下午3:55寫道：
>
> > > >
> > > > CRAS calls snd_pcm_hw_params_set_buffer_size_max() to use as large
> > > > buffer as possible. So the period size is an arbitrary number in
> > > > different platforms. Atom SST platform happens to be 256, and CML
> > > > SOF platform is 1056 for example.
> > >
> > > ok, but earlier in this thread it was mentioned that values such as
> > > 432 are not suitable. the statement above seems to mean the period
> > > actual value is a "don't care", so I don't quite see why this specific
> > > patch2 restricting the value to 240 is necessary. Patch1 is needed for
> > > sure,
> > > Patch2 is where Takashi and I are not convinced.
> >
> > I have downloaded the patch1 but it does not work. After applying patch1,
> > the default period size changes to 320. However, it also has the same issue
> > with period size 320. (It can be verified by aplay.)
>
> The period_size is related to the audio latency so it's decided by application
> according to the use case it's running. That's why there are concerns about
> patch 2 and also you cannot find similar constraints in other machine driver.
You're right. However, the problem here is the provided period size
does not work. Like 256, setting the period size to 320 also makes
users have big latency in the DSP ring buffer.

localhost ~ # aplay -Dhw:1,0 --period-size=320 --buffer-size=640
/dev/zero -d 1 -f dat --test-position
Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
Hz, Stereo
Suspicious buffer position (1 total): avail = 0, delay = 2640, buffer = 640
Suspicious buffer position (2 total): avail = 0, delay = 2640, buffer = 640
Suspicious buffer position (3 total): avail = 0, delay = 2720, buffer = 640
...

>
> Another problem is the buffer size. Too large buffer is not just wasting memories.
> It also creates problems to memory allocator since continuous pages are not
> always there. Using a small period_count like 2 or 4 should be sufficient for audio
> data transfer.
>
> buffer_size = period_size * period_count * 1000000 / sample_rate;
> snd_pcm_hw_params_set_buffer_time_near(mPcmDevice, params, &buffer_size, NULL);
>
> And one more problem here: you need to decide period_size and period_count
> first in order to calculate the buffer size...
It's a good point. I will bring it up to our team and see whether we
can use the smaller buffer size. Thanks!
>
>
> Regards,
> Brent

Thanks,
Yu-Hsuan

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Takashi Iwai]

On Thu, 13 Aug 2020 10:36:57 +0200,
Yu-Hsuan Hsu wrote:
> 
> Lu, Brent <brent.lu@intel.com> 於 2020年8月13日 週四 下午3:55寫道：
> >
> > > > >
> > > > > CRAS calls snd_pcm_hw_params_set_buffer_size_max() to use as large
> > > > > buffer as possible. So the period size is an arbitrary number in
> > > > > different platforms. Atom SST platform happens to be 256, and CML
> > > > > SOF platform is 1056 for example.
> > > >
> > > > ok, but earlier in this thread it was mentioned that values such as
> > > > 432 are not suitable. the statement above seems to mean the period
> > > > actual value is a "don't care", so I don't quite see why this specific
> > > > patch2 restricting the value to 240 is necessary. Patch1 is needed for
> > > > sure,
> > > > Patch2 is where Takashi and I are not convinced.
> > >
> > > I have downloaded the patch1 but it does not work. After applying patch1,
> > > the default period size changes to 320. However, it also has the same issue
> > > with period size 320. (It can be verified by aplay.)
> >
> > The period_size is related to the audio latency so it's decided by application
> > according to the use case it's running. That's why there are concerns about
> > patch 2 and also you cannot find similar constraints in other machine driver.
> You're right. However, the problem here is the provided period size
> does not work. Like 256, setting the period size to 320 also makes
> users have big latency in the DSP ring buffer.
> 
> localhost ~ # aplay -Dhw:1,0 --period-size=320 --buffer-size=640
> /dev/zero -d 1 -f dat --test-position
> Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
> Hz, Stereo
> Suspicious buffer position (1 total): avail = 0, delay = 2640, buffer = 640
> Suspicious buffer position (2 total): avail = 0, delay = 2640, buffer = 640
> Suspicious buffer position (3 total): avail = 0, delay = 2720, buffer = 640
> ...

It means that the delay value returned from the driver is bogus.
I suppose it comes pcm_delay value calculated in sst_calc_tstamp(),
but haven't followed the code closely yet.  Maybe checking the debug
outputs can help to trace what's going wrong.


Takashi

> 
> >
> > Another problem is the buffer size. Too large buffer is not just wasting memories.
> > It also creates problems to memory allocator since continuous pages are not
> > always there. Using a small period_count like 2 or 4 should be sufficient for audio
> > data transfer.
> >
> > buffer_size = period_size * period_count * 1000000 / sample_rate;
> > snd_pcm_hw_params_set_buffer_time_near(mPcmDevice, params, &buffer_size, NULL);
> >
> > And one more problem here: you need to decide period_size and period_count
> > first in order to calculate the buffer size...
> It's a good point. I will bring it up to our team and see whether we
> can use the smaller buffer size. Thanks!
> >
> >
> > Regards,
> > Brent
> 
> Thanks,
> Yu-Hsuan
> 

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Pierre-Louis Bossart]

On 8/13/20 3:45 AM, Takashi Iwai wrote:
> On Thu, 13 Aug 2020 10:36:57 +0200,
> Yu-Hsuan Hsu wrote:
>>
>> Lu, Brent <brent.lu@intel.com> 於 2020年8月13日 週四 下午3:55寫道：
>>>
>>>>>>
>>>>>> CRAS calls snd_pcm_hw_params_set_buffer_size_max() to use as large
>>>>>> buffer as possible. So the period size is an arbitrary number in
>>>>>> different platforms. Atom SST platform happens to be 256, and CML
>>>>>> SOF platform is 1056 for example.
>>>>>
>>>>> ok, but earlier in this thread it was mentioned that values such as
>>>>> 432 are not suitable. the statement above seems to mean the period
>>>>> actual value is a "don't care", so I don't quite see why this specific
>>>>> patch2 restricting the value to 240 is necessary. Patch1 is needed for
>>>>> sure,
>>>>> Patch2 is where Takashi and I are not convinced.
>>>>
>>>> I have downloaded the patch1 but it does not work. After applying patch1,
>>>> the default period size changes to 320. However, it also has the same issue
>>>> with period size 320. (It can be verified by aplay.)
>>>
>>> The period_size is related to the audio latency so it's decided by application
>>> according to the use case it's running. That's why there are concerns about
>>> patch 2 and also you cannot find similar constraints in other machine driver.
>> You're right. However, the problem here is the provided period size
>> does not work. Like 256, setting the period size to 320 also makes
>> users have big latency in the DSP ring buffer.
>>
>> localhost ~ # aplay -Dhw:1,0 --period-size=320 --buffer-size=640
>> /dev/zero -d 1 -f dat --test-position
>> Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
>> Hz, Stereo
>> Suspicious buffer position (1 total): avail = 0, delay = 2640, buffer = 640
>> Suspicious buffer position (2 total): avail = 0, delay = 2640, buffer = 640
>> Suspicious buffer position (3 total): avail = 0, delay = 2720, buffer = 640
>> ...
> 
> It means that the delay value returned from the driver is bogus.
> I suppose it comes pcm_delay value calculated in sst_calc_tstamp(),
> but haven't followed the code closely yet.  Maybe checking the debug
> outputs can help to trace what's going wrong.

the problem is really that we add a constraint that the period size be a 
multiple of 1ms, and it's not respected. 320 samples is not a valid 
choice, I don't get how it ends-up being selected? there's a glitch in 
the matrix here.

Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

[Yu-Hsuan Hsu]

Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> 於
2020年8月13日 週四 下午8:57寫道：
>
>
>
> On 8/13/20 3:45 AM, Takashi Iwai wrote:
> > On Thu, 13 Aug 2020 10:36:57 +0200,
> > Yu-Hsuan Hsu wrote:
> >>
> >> Lu, Brent <brent.lu@intel.com> 於 2020年8月13日 週四 下午3:55寫道：
> >>>
> >>>>>>
> >>>>>> CRAS calls snd_pcm_hw_params_set_buffer_size_max() to use as large
> >>>>>> buffer as possible. So the period size is an arbitrary number in
> >>>>>> different platforms. Atom SST platform happens to be 256, and CML
> >>>>>> SOF platform is 1056 for example.
> >>>>>
> >>>>> ok, but earlier in this thread it was mentioned that values such as
> >>>>> 432 are not suitable. the statement above seems to mean the period
> >>>>> actual value is a "don't care", so I don't quite see why this specific
> >>>>> patch2 restricting the value to 240 is necessary. Patch1 is needed for
> >>>>> sure,
> >>>>> Patch2 is where Takashi and I are not convinced.
> >>>>
> >>>> I have downloaded the patch1 but it does not work. After applying patch1,
> >>>> the default period size changes to 320. However, it also has the same issue
> >>>> with period size 320. (It can be verified by aplay.)
> >>>
> >>> The period_size is related to the audio latency so it's decided by application
> >>> according to the use case it's running. That's why there are concerns about
> >>> patch 2 and also you cannot find similar constraints in other machine driver.
> >> You're right. However, the problem here is the provided period size
> >> does not work. Like 256, setting the period size to 320 also makes
> >> users have big latency in the DSP ring buffer.
> >>
> >> localhost ~ # aplay -Dhw:1,0 --period-size=320 --buffer-size=640
> >> /dev/zero -d 1 -f dat --test-position
> >> Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000
> >> Hz, Stereo
> >> Suspicious buffer position (1 total): avail = 0, delay = 2640, buffer = 640
> >> Suspicious buffer position (2 total): avail = 0, delay = 2640, buffer = 640
> >> Suspicious buffer position (3 total): avail = 0, delay = 2720, buffer = 640
> >> ...
> >
> > It means that the delay value returned from the driver is bogus.
> > I suppose it comes pcm_delay value calculated in sst_calc_tstamp(),
> > but haven't followed the code closely yet.  Maybe checking the debug
> > outputs can help to trace what's going wrong.
>
> the problem is really that we add a constraint that the period size be a
> multiple of 1ms, and it's not respected. 320 samples is not a valid
> choice, I don't get how it ends-up being selected? there's a glitch in
> the matrix here.
>
>
Oh sorry that I applied the wrong patch. With the correct patch, the
default period size is 432.
With period size 432, running aplay with --test-position does not show
any errors. However, by cat `/proc/asound/card1/pcm0p/sub0/status`. We
can see the delay is around 3000.
Here are all period sizes I have tried. All buffer sizes are set to 2
* period size.
period size: 192,  delay is a negative number. Not sure what happened.
period size: 240, delay is fixed at 960
period size: 288, delay is around 27XX
period size: 336, delay is around 27XX
period size: 384, delay is around 24XX (no errors from aplay)
period size: 432, delay is around 30XX (no errors from aplay)
period size: 480, delay is fixed at 3120 (no errors from aplay)
period size: 524, delay is around 31XX (no errors from aplay)

Not sure why the delay is around 50ms except for the period size 240.
Is it normal?

Thanks,
Yu-Hsuan

Re: [PATCH v3 0/2] Add period size constraint for Atom Chromebook

[Mark Brown]

On Fri, 31 Jul 2020 20:26:03 +0800, Brent Lu wrote:
> Two different constraints are implemented: one is in platform's CPU
> DAI to enforce the period to be multiple of 1ms to align with firmware
> design. The other is in Atom Chromebook's machine driver to use 240 as
> period size which is selected by google.
> 
> 
> Changes since v1:
> -Add comma at the end of media_period_size array declaration.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: intel: atom: Add period size constraint
      commit: 5e7820e369248f880767c4c4079b414529bc2125

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark