Mailing List Archive

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

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

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


Takashi

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

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

Yu-Hsuan

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

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

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


Takashi

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

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

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

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

The existing code has this:

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

return snd_pcm_hw_constraint_integer(runtime,
SNDRV_PCM_HW_PARAM_PERIODS);

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

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