linux icon indicating copy to clipboard operation
linux copied to clipboard

deepbuffer PCM not following period size setting

Open kv2019i opened this issue 1 year ago • 7 comments

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 avatar Jan 22 '24 16:01 kv2019i

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.

kv2019i avatar Jan 22 '24 17:01 kv2019i

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

plbossart avatar Feb 02 '24 20:02 plbossart

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.

yongzhi1 avatar Feb 28 '24 16:02 yongzhi1

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

plbossart avatar Apr 24 '24 17:04 plbossart

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

ujfalusi avatar Apr 25 '24 06:04 ujfalusi

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

plbossart avatar May 01 '24 17:05 plbossart

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

yongzhi1 avatar May 02 '24 21:05 yongzhi1