On Wed, 15 May 2019 08:26:37 +0200, vanitha.channaiah@in.bosch.com wrote:
From: Vanitha Channaiah vanitha.channaiah@in.bosch.com
This Fix was analyzed for below usecase :
alsa configuration: pcm.line_in { type dsnoop ipc_key INT slave { pcm hardware channels 2 period_time 8000 rate 48000 format S16_LE } bindings { 0 0 1 1 } } pcm.hardware { type hw card "gmd-card" device 0 subdevice 0 channels 2 period_time 2000 rate 48000 format S16_LE }
command: $arecord -v -D line_in -r 48000 -c 2 -f S16_LE recordfile.wav Direct Snoop PCM Its setup is: stream : CAPTURE access : RW_INTERLEAVED format : S16_LE subformat : STD channels : 2 rate : 48000 exact rate : 48000 (48000/1) msbits : 16 buffer_size : 1536 period_size : 384 period_time : 8000 tstamp_mode : NONE tstamp_type : MONOTONIC period_step : 1 avail_min : 384 period_event : 0 start_threshold : 1 stop_threshold : 1536 silence_threshold: 0 silence_size : 0 boundary : huge value Hardware PCM card 3 'gmd-card' device 0 subdevice 0 Its setup is: stream : CAPTURE access : MMAP_INTERLEAVED format : S16_LE subformat : STD channels : 2 rate : 48000 exact rate : 48000 (48000/1) msbits : 16 buffer_size : 1536 period_size : 96 period_time : 2000 tstamp_mode : ENABLE tstamp_type : MONOTONIC period_step : 1 avail_min : 96 period_event : 0 start_threshold : 1 stop_threshold : huge value silence_threshold: 0 silence_size : 0 boundary : huge value appl_ptr : 0 hw_ptr : 576
Here, there are no other plugins apart from Dsnoop.
Issue: After partial read of unaligned frames(not one period frames), snd_pcm_wait() would block for the pcm->avail_min which would result in blocking for longer periods i.e more than one period as specified by pcm->avail_min
For e.g.: Slave period size = 0x60 Client period-size=0x180 No. of Ticks = 4 pcm->avail_min = one period size = 0x180
Issue:
- During the start of streaming, the position of slave hw_ptr returned from the driver is 0x20.
- avail is 0x20
- hw_ptr is calculated by new_slave_hw_ptr - old_slave_hw_ptr i.e. 0x20 - 0 = 0x20
- hw_ptr updated to 0x20
- avail is 0x20
- app_ptr updated to 0x20
- Now, avail = 0
- snd_pcm_wait() waits till avail=0x180 because pcm->avail_min = 0x180
- After 4 snd_pcm_elapsed(), slave_hw_ptr = 0x180
- Since app_ptr has updated with 0x20, avail becomes 0x160 There is a shortage of 0x20 frames and hence snd_pcm_wait() goes back to wait again.
- Now, snd_pcm_wait is locked.
- After another 4 snd_pcm_elapsed() slave_hw_ptr = 0x300
- avail = 0x2e0
- snd_pcm_wait() unlocks.
So, here snd_pcm_wait() is locked for more than 1 period(exactly 2 periods)
Also, this issue can be seen without dsnoop plugin, when HW reports unaligned hw_ptr For e.g. period size = 0x60 pcm->avail_min = 0x60
- During the start of streaming, the position of slave hw_ptr returned from the driver is 0x20.
- hw_ptr is calculated by new_slave_hw_ptr - old_slave_hw_ptr i.e. 0x20 - 0 = 0x20
- hw_ptr updated to 0x20
- avail is 0x20
- app_ptr updated to 0x20
- Now, avail = 0
- snd_pcm_wait() waits till avail=0x60 because pcm->avail_min=0x60
- After 1 snd_pcm_elapsed(), slave_hw_ptr = 0x60
- Since app_ptr has updated with 0x20, avail becomes 0x40 There is a shortage of 0x20 frames and hence snd_pcm_wait() goes back to wait again.
- Now, snd_pcm_wait is locked.
- After another 1 snd_pcm_elapsed(), slave_hw_ptr = 0x120
- avail = 0xe0
- snd_pcm_wait() unlocks.
So, here snd_pcm_wait() is locked for more than 1 period (exactly 2 periods)
Fix: After reading unaligned frames(not one period frames), set the pcm->avail_min to the needed_avail_slave_min frames so that snd_pcm_wait() blocks till needed_avail_slave_min available Once needed_avail_slave_min frames are read, set back the original pcm->avail_min
For ex: Slave period size = 0x60 Client period-size=0x180 No. of Ticks = 4 pcm->avail_min = one period size = 0x180
Fix:
- During the start of streaming, the position of slave_hw_ptr returned from the driver is 0x20.
i.e. 0x20 - 0 = 0x20
- hw_ptr is calculated by new_slave_hw_ptr - old_slave_hw_ptr
- hw_ptr updated to 0x20
- avail is 0x20
- app_ptr updated to 0x20
- Now, avail = 0
- calculate needed_avail_slave_min = 0x160
- update the needed_avail_slave_min to pcm->avail_min i.e. pcm->avail_min = 0x160
- snd_pcm_wait() waits till avail=0x160
- After 4 snd_pcm_elapsed(), slave_hw_ptr = 0x180
- snd_pcm_wait() unlocks.
- Once needed_avail_slave_min frames are read, set back the original pcm->avail_min to 0x180
So, here snd_pcm_wait() is locked for 1 period only.
Signed-off-by: Vanitha Channaiah vanitha.channaiah@in.bosch.com
src/pcm/pcm.c | 21 +++++++++++++++++++++ src/pcm/pcm_local.h | 2 ++ 2 files changed, 23 insertions(+)
diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c index f0db545..f361eb1 100644 --- a/src/pcm/pcm.c +++ b/src/pcm/pcm.c @@ -973,6 +973,7 @@ int snd_pcm_sw_params(snd_pcm_t *pcm, snd_pcm_sw_params_t *params) __snd_pcm_unlock(pcm); return err; }
- pcm->original_avail_min = pcm->avail_min; __snd_pcm_unlock(pcm); return 0;
} @@ -7267,6 +7268,17 @@ void snd_pcm_areas_from_bufs(snd_pcm_t *pcm, snd_pcm_channel_area_t *areas, snd_pcm_unlock(pcm); }
+static void snd_pcm_set_avail_min(snd_pcm_t *pcm, snd_pcm_uframes_t avail) +{
- if (avail != pcm->avail_min) {
snd_pcm_sw_params_t swparams;snd_pcm_sw_params_current(pcm, &swparams);swparams.avail_min = avail;_snd_pcm_sw_params_internal(pcm, &swparams);- }
+}
snd_pcm_sframes_t snd_pcm_read_areas(snd_pcm_t *pcm, const snd_pcm_channel_area_t *areas, snd_pcm_uframes_t offset, snd_pcm_uframes_t size, snd_pcm_xfer_areas_func_t func) @@ -7274,6 +7286,7 @@ snd_pcm_sframes_t snd_pcm_read_areas(snd_pcm_t *pcm, const snd_pcm_channel_area_ snd_pcm_uframes_t xfer = 0; snd_pcm_sframes_t err = 0; snd_pcm_state_t state;
snd_pcm_uframes_t needed_slave_avail_min = 0;
if (size == 0) return 0;
@@ -7332,6 +7345,14 @@ snd_pcm_sframes_t snd_pcm_read_areas(snd_pcm_t *pcm, const snd_pcm_channel_area_ if (err < 0) break; frames = err;
pcm->unaligned_frames += frames;pcm->unaligned_frames %= pcm->period_size;if (pcm->unaligned_frames) {needed_slave_avail_min = pcm->period_size - pcm->unaligned_frames;snd_pcm_set_avail_min(pcm, needed_slave_avail_min);} else {snd_pcm_set_avail_min(pcm, pcm->original_avail_min);}diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index e103f72..3fdffb4 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -210,6 +210,8 @@ struct _snd_pcm { snd_pcm_tstamp_type_t tstamp_type; /* timestamp type */ unsigned int period_step; snd_pcm_uframes_t avail_min; /* min avail frames for wakeup */
- snd_pcm_uframes_t unaligned_frames;
- snd_pcm_uframes_t original_avail_min; int period_event; snd_pcm_uframes_t start_threshold; snd_pcm_uframes_t stop_threshold;
Can we avoid adding such a hack in the core code?
Basically the issue is very specific to direct-plugins, so this sort of workaround should be implemented locally there instead. I guess we can do similar tricks by overriding the calls in the callbacks.
thanks,
Takashi