[PATCH 0/2] Add period size constraint for Atom Chromebook
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(-)
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);
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
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 = {
On 7/29/20 6:03 AM, Brent Lu wrote:
From: Yu-Hsuan Hsu yuhsuan@chromium.org
The CRAS server does not set the period size in hw_param so ALSA will calculate a value for period size which is based on the buffer size and other parameters. The value may not always be aligned with Atom's dsp design so a constraint is added to make sure the board always has a good value.
Cyan uses chtmax98090 and others(banon, celes, edgar, kefka...) use rt5650.
Is this patch required if you've already constrained the period sizes for the platform driver in patch1?
Is this patch required if you've already constrained the period sizes for the platform driver in patch1?
Yes or alsa will select 320 as default period size for it.
ok, then that's a miss in your patch1. 320 samples is a multiple of 1ms for 48kHz rates. I think it was valid only for the 16kHz VoIP paths used in some versions of Android, but that we don't support in the upstream code.
To build on Takashi's answer, the real ask here is to require that the period be a multiple of 1ms, because that's the fundamental design/limitation of firmware. It doesn't matter if it's 48, 96, 192, 240, 480, 960 samples.
On 7/30/20 10:27 AM, Pierre-Louis Bossart wrote:
Is this patch required if you've already constrained the period sizes for the platform driver in patch1?
Yes or alsa will select 320 as default period size for it.
ok, then that's a miss in your patch1. 320 samples is a multiple of 1ms
typo: is NOT
for 48kHz rates. I think it was valid only for the 16kHz VoIP paths used in some versions of Android, but that we don't support in the upstream code.
To build on Takashi's answer, the real ask here is to require that the period be a multiple of 1ms, because that's the fundamental design/limitation of firmware. It doesn't matter if it's 48, 96, 192, 240, 480, 960 samples.
Yes or alsa will select 320 as default period size for it.
ok, then that's a miss in your patch1. 320 samples is a multiple of 1ms
typo: is NOT
for 48kHz rates. I think it was valid only for the 16kHz VoIP paths used in some versions of Android, but that we don't support in the upstream code.
To build on Takashi's answer, the real ask here is to require that the period be a multiple of 1ms, because that's the fundamental design/limitation of firmware. It doesn't matter if it's 48, 96, 192, 240, 480, 960 samples.
Yes 320 is for VoIP and the rates in CPU DAI are 8/16KHz. It's a mistake.
Regards, Brent
On Thu, 30 Jul 2020 17:27:58 +0200, Pierre-Louis Bossart wrote:
Is this patch required if you've already constrained the period sizes for the platform driver in patch1?
Yes or alsa will select 320 as default period size for it.
ok, then that's a miss in your patch1. 320 samples is a multiple of 1ms for 48kHz rates. I think it was valid only for the 16kHz VoIP paths used in some versions of Android, but that we don't support in the upstream code.
To build on Takashi's answer, the real ask here is to require that the period be a multiple of 1ms, because that's the fundamental design/limitation of firmware. It doesn't matter if it's 48, 96, 192, 240, 480, 960 samples.
If the 1ms alignment is the condition, it can be better with a different hw_params constraint. We can use snd_pcm_hw_constraint_step() for such a purpose.
thanks,
Takashi
If the 1ms alignment is the condition, it can be better with a different hw_params constraint. We can use snd_pcm_hw_constraint_step() for such a purpose.
Will fix. Thanks.
Regards, Brent
thanks,
Takashi
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
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(-)
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);
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 = {
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
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
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
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
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?
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
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
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
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
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
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.
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
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
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
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
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
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
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
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.
... 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
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)?
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
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.
- 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
- 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)
- 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.)
- 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
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.
- 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
- 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)
- 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.)
- 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
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.
- 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
- 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)
- 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.)
- 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
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.
- 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
- 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)
- 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.)
- 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
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.
- 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
- 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)
- 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.)
- 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
After doing some experiments, I think I can identify the problem more precisely.
- 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?
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
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 :-)
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
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.
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.)
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
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
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
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.
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
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
participants (7)
-
Andy Shevchenko -
Brent Lu -
Lu, Brent
-
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai -
Yu-Hsuan Hsu