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

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

Jul 31, 2020, 6:15 AM

Post #1 of 39 (1637 views)

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 v3 2/2] ASoC: Intel: Add period size constraint on strago board [ In reply to ]

tiwai at suse

Jul 31, 2020, 8:15 AM

Post #2 of 39 (1637 views)

Permalink

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 [ In reply to ]

brent.lu at intel

Aug 1, 2020, 2:15 AM

Post #3 of 39 (1637 views)

Permalink

>
> 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 [ In reply to ]

tiwai at suse

Aug 1, 2020, 4:15 AM

Post #4 of 39 (1637 views)

Permalink

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 [ In reply to ]

brent.lu at intel

Aug 3, 2020, 10:15 AM

Post #5 of 39 (1636 views)

Permalink

> > >
> > > 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 [ In reply to ]

pierre-louis.bossart at linux

Aug 3, 2020, 10:15 AM

Post #6 of 39 (1636 views)

Permalink

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 [ In reply to ]

brent.lu at intel

Aug 3, 2020, 10:15 AM

Post #7 of 39 (1636 views)

Permalink

> > 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 [ In reply to ]

tiwai at suse

Aug 3, 2020, 10:15 AM

Post #8 of 39 (1636 views)

Permalink

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 [ In reply to ]

brent.lu at intel

Aug 3, 2020, 10:15 PM

Post #9 of 39 (1633 views)

Permalink

>
> 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 [ In reply to ]

pierre-louis.bossart at linux

Aug 4, 2020, 10:15 AM

Post #10 of 39 (1632 views)

Permalink

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 [ In reply to ]

brent.lu at intel

Aug 6, 2020, 10:15 AM

Post #11 of 39 (1632 views)

Permalink

>
> 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 [ In reply to ]

pierre-louis.bossart at linux

Aug 10, 2020, 8:15 AM

Post #12 of 39 (1632 views)

Permalink

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 [ In reply to ]

yuhsuan at chromium

Aug 10, 2020, 12:15 PM

Post #13 of 39 (1631 views)

Permalink

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 [ In reply to ]

brent.lu at intel

Aug 10, 2020, 8:15 PM

Post #14 of 39 (1631 views)

Permalink

>
> 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 [ In reply to ]

yuhsuan at chromium

Aug 10, 2020, 8:15 PM

Post #15 of 39 (1631 views)

Permalink

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 [ In reply to ]

tiwai at suse

Aug 11, 2020, 1:05 AM

Post #16 of 39 (1629 views)

Permalink

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 [ In reply to ]

yuhsuan at chromium

Aug 11, 2020, 2:15 AM

Post #17 of 39 (1627 views)

Permalink

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 [ In reply to ]

tiwai at suse

Aug 11, 2020, 2:15 AM

Post #18 of 39 (1627 views)

Permalink

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 [ In reply to ]

yuhsuan at chromium

Aug 11, 2020, 4:15 AM

Post #19 of 39 (1613 views)

Permalink

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 [ In reply to ]

broonie at kernel

Aug 11, 2020, 8:15 AM

Post #20 of 39 (1610 views)

Permalink

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.

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

pierre-louis.bossart at linux

Aug 11, 2020, 10:15 AM

Post #21 of 39 (1595 views)

Permalink

>>> ... 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 [ In reply to ]

broonie at kernel

Aug 11, 2020, 12:15 PM

Post #22 of 39 (1594 views)

Permalink

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

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

yuhsuan at chromium

Aug 11, 2020, 10:15 PM

Post #23 of 39 (1591 views)

Permalink

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 [ In reply to ]

tiwai at suse

Aug 12, 2020, 12:15 AM

Post #24 of 39 (1591 views)

Permalink

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 [ In reply to ]

yuhsuan at chromium

Aug 12, 2020, 12:15 AM

Post #25 of 39 (1593 views)

Permalink

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

Mailing List Archive

Attached Files:

Attached Files: