thesofproject / linux

Linux kernel source tree
Other
89 stars 129 forks source link

deepbuffer PCM not following period size setting #4795

Open kv2019i opened 7 months ago

kv2019i commented 7 months ago

Testing with a new extension to alsa-conformance-test for delay reporting (https://github.com/kv2019i/cros-audiotest/tree/topic/pcm-delay-debug), shows unexpected wake-up behavior for the SOF deepbuffer PCM on Intel MTL platform.

Environment

kernel: 66ee247636e52ea6ebb136edec9abed6891d7d6c (sof-dev) SOF: 6c188298b958 (SOF main) - https://github.com/thesofproject/sof/pull/8756 needed for correct delay values but not mandatory to reproduce this bug platform: MTL topoplogy: sof-mtl-rt713-l0-rt1316-l12.tplg

Expected outcome

The driver should wake up user-space as per the configured period-size.

Additionally, INFO_BATCH should be set to indicate hw_ptr will move in jumps (see https://github.com/thesofproject/sof/issues/8717 ).

Actual outcome

If application sets a 10ms period size and 100ms buffer-size, the hw-ptr will jump 90ms at a time.

Example log (with above alsa-conformance-test version). Note, below log with FW patch https://github.com/thesofproject/sof/pull/8756 to address https://github.com/thesofproject/linux/issues/4781 -- otherwise the delay values are not correct. Not mandatory to reproduce this bug.

$ aplay -l |grep -A2 "device 31"
card 0: sofsoundwire [sof-soundwire], device 31: Deepbuffer Jack Out (*) []
  Subdevices: 1/1
  Subdevice #0: subdevice #0
$ alsa_conformance_test -Phw:0,31 -p 480 -B 2400 --merge_threshold_sz=4800 -d 60 --debug 
TIME_DIFF(s)    HW_LEVEL     PLAYED       DIFF               RATE      DELAY PLAYED_ADJ       DIFF
0.000060024            0       4800       4800    79968012.794882 7493989779944510264 -7493989779944505464 -7493989779944505464
0.095043928         4552       5048        248        2609.319766       4913       4687 7493989779944510151
0.000023466         3346       6254       1206    51393505.497315       4927       4673        -14 [Merged]
0.000021303         2296       7304       1050    49288832.558795       4921       4679          6 [Merged]
0.000019454         3744       8256        952    48935951.475275       7321       4679          0 [Merged]
0.000018708         2840       9160        904    48321573.658328       7323       4677         -2 [Merged]
0.000025679         2592       9408        248     9657696.950816       7513       4487       -190 [Merged]
0.095874426         2544       9456         48         500.654888       2681       9319       4832
0.000022952         1374      10626       1170    50975949.808296       2715       9285        -34 [Merged]
0.000020586         2760      11640       1014    49256776.450015       5121       9279         -6 [Merged]
0.000020071         1776      12624        984    49025957.849634       5113       9287          8 [Merged]
0.000020940         3144      13656       1032    49283667.621777       7513       9287          0 [Merged]
0.000026964         2784      14016        360    13351134.846462       7705       9095       -192 [Merged]
0.095880649         2544      14256        240        2503.111968       2873      13927       4832

Notes

With defaults settings alsa-conformance-test will hit undderuns with the deepbuffer PCM and the results will not be valid. To avoid this, the block size (-B 2400) is manually set. This is the amount of data user-space writes at a time to PCM and is separate from the configured period-size.

kv2019i commented 7 months ago

FYI @RanderWang @ranj063 @ujfalusi @plbossart -- the deepbuffer PCM has some unexpected behaviour towards the user-space. Not necessarily a problem for apps that can handle this, but e.g. this won't pass the conformance test.

plbossart commented 7 months ago

@kv2019i for my education, why would an application set a 100ms buffer and then ask to be awaken 10 times? The canonical ALSA behavior is for 4 periods, and the goal would actually be to use no period wake-ups and instead wake-up with a timer at the 80 or 90ms watermark.

Just trying to understand if we are dealing with a validation corner case that has no direct bearing on actual apps, or something with a direct app/product impact.

yongzhi1 commented 6 months ago

FYI @RanderWang @ranj063 @ujfalusi @plbossart -- the deepbuffer PCM has some unexpected behaviour towards the user-space. Not necessarily a problem for apps that can handle this, but e.g. this won't pass the conformance test.

The audio position has an impact for AV synchronization, assume the latency is constant if queried every 100ms, the conformance rate test should pass when --merge_threshold_t set to 0.1 sec right? but it still fails.

plbossart commented 4 months ago

@kv2019i @ujfalusi @yongzhi1 I think this is solved, no? can we close?

ujfalusi commented 4 months ago

@plbossart, with an improved alsaconformance test the reported delay is taken into account for the rate validation and it passes fine: https://github.com/ujfalusi/chromeos-audiotest/tree/topic/pcm-delay

I really don't know how that can be upstreamed back to Chrome...

plbossart commented 4 months ago

@yongzhi1 @sathya-nujella can you help propagate the patches back to the Chrome repo?

yongzhi1 commented 4 months ago

@yongzhi1 @sathya-nujella can you help propagate the patches back to the Chrome repo?

Sure, maybe after the kernel series get merged to Google tree.