[alsa-devel] [PATCH v2 0/6]
- Updated the patches by incorporating review comments from Takashi Iwai-san
[PATCH 1/5] pcm: direct: Add generic hw_ptr_alignment function for dmix, dshare and dsnoop [Takashi Iwai:]
The patch description needs rephrasing. What actually this does is to move the code from pcm_dmix.c to pcm_direct.c and its header so that the helper code can be re-used by other direct-PCM plugins.
- Commit message is rephrased as suggested. - New commit: [PATCH v2 1/6] pcm: direct: Add generic hw_ptr_alignment function for
[PATCH 2/5] pcm: dshare: Added "hw_ptr_alignment" option in configuration for alignment of slave pointers [Takashi Iwai:]
Again, this patch description is too ambiguous. And, if it enables the hw_ptr_alignment option, update the documentation as well.
- Commit message is explained in detail for the changes done. - Documentation updated. - New commit: [PATCH v2 2/6] pcm: dshare: Added "hw_ptr_alignment" option in
[PATCH 3/5] pcm: dsnoop: Added "hw_ptr_alignment" option in configuration for slave pointer alignment [Takashi Iwai:]
Ditto as patch 2, the description is too ambiguous, and the update of documentation is missing. It's not good to change the helper function semantics out of sudden, even without any description.
- Commit message is explained in detail for the changes done. - Documentation updated. - Divided the patch with commit ("pcm: dsnoop: Add hw_ptr_alignment option in configuration") into additional patch commit ("pcm: direct: Round up of slave_app_ptr pointer if buffer") - Usecase scenario is described for the changes done in helper function. - New commit: [PATCH v2 3/6] pcm: dsnoop: Added "hw_ptr_alignment" option in [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer
[PATCH 4/5] pcm: restructuring sw params function [Takashi Iwai:]
I see no reason to do that. Please describe.
- Commit message is explained in detail why reformating was done. - New commit: [PATCH v2 5/6] pcm: restructuring sw params function
[PATCH 5/5] pcm: Update pcm->avail_min with needed_slave_avail_min, after reading unaligned frames [Takashi Iwai:]
This kind of changes in the core code should be avoided as much as possible, especially if it's only relevant with the specific plugins. Sorry, this isn't convincing enough. If this is a MUST, please clarify better.
- Commit message is explained in detail with the generic usecase and specific use case we came across. - New commit: [PATCH v2 6/6] pcm: Update pcm->avail_min with needed_slave_avail_min,
From: Vanitha Channaiah vanitha.channaiah@in.bosch.com
Move the code snd_pcm_direct_reset_slave_ptr() from pcm_dmix.c to pcm_direct.c and its header so that the helper function can be re-used by other direct-pcm plugins. There is no change in the behavior or the functionality.
Signed-off-by: Vanitha Channaiah vanitha.channaiah@in.bosch.com --- src/pcm/pcm_direct.c | 19 +++++++++++++++++++ src/pcm/pcm_direct.h | 8 ++++++++ src/pcm/pcm_dmix.c | 25 ++----------------------- 3 files changed, 29 insertions(+), 23 deletions(-)
diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 666a8ce..411a035 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -2040,3 +2040,22 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf,
return 0; } + +void snd_pcm_direct_reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) +{ + dmix->slave_appl_ptr = dmix->slave_hw_ptr = *dmix->spcm->hw.ptr; + + if (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_ROUNDUP || + (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_AUTO && + pcm->buffer_size <= pcm->period_size * 2)) + dmix->slave_appl_ptr = + ((dmix->slave_appl_ptr + dmix->slave_period_size - 1) / + dmix->slave_period_size) * dmix->slave_period_size; + else if (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_ROUNDDOWN || + (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_AUTO && + (dmix->slave_period_size * SEC_TO_MS) / + pcm->rate < LOW_LATENCY_PERIOD_TIME)) + dmix->slave_appl_ptr = dmix->slave_hw_ptr = + ((dmix->slave_hw_ptr / dmix->slave_period_size) * + dmix->slave_period_size); +} diff --git a/src/pcm/pcm_direct.h b/src/pcm/pcm_direct.h index da5e280..a71aab1 100644 --- a/src/pcm/pcm_direct.h +++ b/src/pcm/pcm_direct.h @@ -24,6 +24,11 @@
#define DIRECT_IPC_SEMS 1 #define DIRECT_IPC_SEM_CLIENT 0 +/* Seconds representing in Milli seconds */ +#define SEC_TO_MS 1000 +/* slave_period time for low latency requirements in ms */ +#define LOW_LATENCY_PERIOD_TIME 10 +
typedef void (mix_areas_t)(unsigned int size, volatile void *dst, void *src, @@ -257,6 +262,8 @@ struct snd_pcm_direct { snd1_pcm_direct_get_chmap #define snd_pcm_direct_set_chmap \ snd1_pcm_direct_set_chmap +#define snd_pcm_direct_reset_slave_ptr \ + snd1_pcm_direct_reset_slave_ptr
int snd_pcm_direct_semaphore_create_or_connect(snd_pcm_direct_t *dmix);
@@ -341,6 +348,7 @@ int snd_pcm_direct_slave_recover(snd_pcm_direct_t *direct); int snd_pcm_direct_client_chk_xrun(snd_pcm_direct_t *direct, snd_pcm_t *pcm); int snd_timer_async(snd_timer_t *timer, int sig, pid_t pid); struct timespec snd_pcm_hw_fast_tstamp(snd_pcm_t *pcm); +void snd_pcm_direct_reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix);
struct snd_pcm_direct_open_conf { key_t ipc_key; diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c index c5592cd..dcde40d 100644 --- a/src/pcm/pcm_dmix.c +++ b/src/pcm/pcm_dmix.c @@ -55,9 +55,6 @@ const char *_snd_module_pcm_dmix = ""; #define STATE_RUN_PENDING 1024 #endif
-#define SEC_TO_MS 1000 /* Seconds representing in Milli seconds */ -#define LOW_LATENCY_PERIOD_TIME 10 /* slave_period time for low latency requirements in ms */ - /* * */ @@ -560,30 +557,12 @@ static int snd_pcm_dmix_hwsync(snd_pcm_t *pcm) } }
-static void reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) -{ - dmix->slave_appl_ptr = dmix->slave_hw_ptr = *dmix->spcm->hw.ptr; - - if (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_ROUNDUP || - (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_AUTO && - pcm->buffer_size <= pcm->period_size * 2)) - dmix->slave_appl_ptr = - ((dmix->slave_appl_ptr + dmix->slave_period_size - 1) - / dmix->slave_period_size) * dmix->slave_period_size; - else if (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_ROUNDDOWN || - (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_AUTO && - (dmix->slave_period_size * SEC_TO_MS) / pcm->rate < LOW_LATENCY_PERIOD_TIME)) - dmix->slave_appl_ptr = dmix->slave_hw_ptr = - ((dmix->slave_hw_ptr / dmix->slave_period_size) * - dmix->slave_period_size); -} - static int snd_pcm_dmix_reset(snd_pcm_t *pcm) { snd_pcm_direct_t *dmix = pcm->private_data; dmix->hw_ptr %= pcm->period_size; dmix->appl_ptr = dmix->last_appl_ptr = dmix->hw_ptr; - reset_slave_ptr(pcm, dmix); + snd_pcm_direct_reset_slave_ptr(pcm, dmix); return 0; }
@@ -592,7 +571,7 @@ static int snd_pcm_dmix_start_timer(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) int err;
snd_pcm_hwsync(dmix->spcm); - reset_slave_ptr(pcm, dmix); + snd_pcm_direct_reset_slave_ptr(pcm, dmix); err = snd_timer_start(dmix->timer); if (err < 0) return err;
On Wed, 15 May 2019 08:26:32 +0200, vanitha.channaiah@in.bosch.com wrote:
From: Vanitha Channaiah vanitha.channaiah@in.bosch.com
Move the code snd_pcm_direct_reset_slave_ptr() from pcm_dmix.c to pcm_direct.c and its header so that the helper function can be re-used by other direct-pcm plugins. There is no change in the behavior or the functionality.
Signed-off-by: Vanitha Channaiah vanitha.channaiah@in.bosch.com
Thanks, the patch description is much more understandable in this version. Applied this one as well as patch 2 & 3 now.
Takashi
From: Vanitha Channaiah vanitha.channaiah@in.bosch.com
This change adapt the fix commit 6b058fda9dce ("pcm: dmix: Add option to allow alignment of slave pointers") for dshare plugin
Issue is that snd_pcm_wait() goes back to waiting because the hw_ptr is not period aligned. Therefore snd_pcm_wait() will block for a longer time as required.
With these rcar driver changes the exact position of the dma is returned. During snd_pcm_start they read hw_ptr as reference, and this hw_ptr is now not period aligned, and is a little ahead over the period while it is read. Therefore when the avail is calculated during snd_pcm_wait(), it is missing the avail_min by a few frames.
An additional option hw_ptr_alignment is provided to dshare configuration, to allow the user to configure the slave application and hw pointer alignment at startup
Signed-off-by: Vanitha Channaiah vanitha.channaiah@in.bosch.com --- src/pcm/pcm_dshare.c | 40 +++++++++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-)
diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c index f135b5d..cf8a863 100644 --- a/src/pcm/pcm_dshare.c +++ b/src/pcm/pcm_dshare.c @@ -333,16 +333,16 @@ static int snd_pcm_dshare_reset(snd_pcm_t *pcm) snd_pcm_direct_t *dshare = pcm->private_data; dshare->hw_ptr %= pcm->period_size; dshare->appl_ptr = dshare->last_appl_ptr = dshare->hw_ptr; - dshare->slave_appl_ptr = dshare->slave_hw_ptr = *dshare->spcm->hw.ptr; + snd_pcm_direct_reset_slave_ptr(pcm, dshare); return 0; }
-static int snd_pcm_dshare_start_timer(snd_pcm_direct_t *dshare) +static int snd_pcm_dshare_start_timer(snd_pcm_t *pcm, snd_pcm_direct_t *dshare) { int err;
snd_pcm_hwsync(dshare->spcm); - dshare->slave_appl_ptr = dshare->slave_hw_ptr = *dshare->spcm->hw.ptr; + snd_pcm_direct_reset_slave_ptr(pcm, dshare); err = snd_timer_start(dshare->timer); if (err < 0) return err; @@ -364,7 +364,8 @@ static int snd_pcm_dshare_start(snd_pcm_t *pcm) else if (avail < 0) return 0; else { - if ((err = snd_pcm_dshare_start_timer(dshare)) < 0) + err = snd_pcm_dshare_start_timer(pcm, dshare); + if (err < 0) return err; snd_pcm_dshare_sync_area(pcm); } @@ -547,7 +548,8 @@ static snd_pcm_sframes_t snd_pcm_dshare_mmap_commit(snd_pcm_t *pcm, return 0; snd_pcm_mmap_appl_forward(pcm, size); if (dshare->state == STATE_RUN_PENDING) { - if ((err = snd_pcm_dshare_start_timer(dshare)) < 0) + err = snd_pcm_dshare_start_timer(pcm, dshare); + if (err < 0) return err; } else if (dshare->state == SND_PCM_STATE_RUNNING || dshare->state == SND_PCM_STATE_DRAINING) { @@ -755,6 +757,7 @@ int snd_pcm_dshare_open(snd_pcm_t **pcmp, const char *name, dshare->slowptr = opts->slowptr; dshare->max_periods = opts->max_periods; dshare->var_periodsize = opts->var_periodsize; + dshare->hw_ptr_alignment = opts->hw_ptr_alignment; dshare->sync_ptr = snd_pcm_dshare_sync_ptr;
retry: @@ -912,6 +915,12 @@ pcm.name { ipc_key INT # unique IPC key ipc_key_add_uid BOOL # add current uid to unique IPC key ipc_perm INT # IPC permissions (octal, default 0600) + hw_ptr_alignment STR # Slave application and hw pointer alignment type + # STR can be one of the below strings : + # no + # roundup + # rounddown + # auto (default) slave STR # or slave { # Slave definition @@ -936,6 +945,27 @@ pcm.name { } \endcode
+<code>hw_ptr_alignment</code> specifies slave application and hw +pointer alignment type. By default hw_ptr_alignment is auto. Below are +the possible configurations: +- no: minimal latency with minimal frames dropped at startup. But + wakeup of application (return from snd_pcm_wait() or poll()) can + take up to 2 * period. +- roundup: It is guaranteed that all frames will be played at + startup. But the latency will increase upto period-1 frames. +- rounddown: It is guaranteed that a wakeup will happen for each + period and frames can be written from application. But on startup + upto period-1 frames will be dropped. +- auto: Selects the best approach depending on the used period and + buffer size. + If the application buffer size is < 2 * application period, + "roundup" will be selected to avoid under runs. If the slave_period + is < 10ms we could expect that there are low latency + requirements. Therefore "rounddown" will be chosen to avoid long + wakeup times. Such wakeup delay could otherwise end up with Xruns in + case of a dependency to another sound device (e.g. forwarding of + microphone to speaker). Else "no" will be chosen. + \subsection pcm_plugins_dshare_funcref Function reference
<UL>
From: Vanitha Channaiah vanitha.channaiah@in.bosch.com
This change adapt the fix commit 6b058fda9dce ("pcm: dmix: Add option to allow alignment of slave pointers") for dsnoop plugin
Issue is that snd_pcm_wait() goes back to waiting because the hw_ptr is not period aligned. Therefore snd_pcm_wait() will block for a longer time as required.
With these rcar driver changes the exact position of the dma is returned. During snd_pcm_start they read hw_ptr as reference, and this hw_ptr is now not period aligned, and is a little ahead over the period while it is read. Therefore when the avail is calculated during snd_pcm_wait(), it is missing the avail_min by a few frames.
An additional option hw_ptr_alignment is provided to dsnoop configuration, to allow the user to configure the slave application and hw pointer alignment at startup
Signed-off-by: Vanitha Channaiah vanitha.channaiah@in.bosch.com --- src/pcm/pcm_direct.c | 1 - src/pcm/pcm_dmix.c | 2 ++ src/pcm/pcm_dshare.c | 2 ++ src/pcm/pcm_dsnoop.c | 30 +++++++++++++++++++++++++++++- 4 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 411a035..54d9900 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -2043,7 +2043,6 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf,
void snd_pcm_direct_reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) { - dmix->slave_appl_ptr = dmix->slave_hw_ptr = *dmix->spcm->hw.ptr;
if (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_ROUNDUP || (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_AUTO && diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c index dcde40d..274726e 100644 --- a/src/pcm/pcm_dmix.c +++ b/src/pcm/pcm_dmix.c @@ -562,6 +562,7 @@ static int snd_pcm_dmix_reset(snd_pcm_t *pcm) snd_pcm_direct_t *dmix = pcm->private_data; dmix->hw_ptr %= pcm->period_size; dmix->appl_ptr = dmix->last_appl_ptr = dmix->hw_ptr; + dmix->slave_appl_ptr = dmix->slave_hw_ptr = *dmix->spcm->hw.ptr; snd_pcm_direct_reset_slave_ptr(pcm, dmix); return 0; } @@ -571,6 +572,7 @@ static int snd_pcm_dmix_start_timer(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) int err;
snd_pcm_hwsync(dmix->spcm); + dmix->slave_appl_ptr = dmix->slave_hw_ptr = *dmix->spcm->hw.ptr; snd_pcm_direct_reset_slave_ptr(pcm, dmix); err = snd_timer_start(dmix->timer); if (err < 0) diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c index cf8a863..b75809c 100644 --- a/src/pcm/pcm_dshare.c +++ b/src/pcm/pcm_dshare.c @@ -333,6 +333,7 @@ static int snd_pcm_dshare_reset(snd_pcm_t *pcm) snd_pcm_direct_t *dshare = pcm->private_data; dshare->hw_ptr %= pcm->period_size; dshare->appl_ptr = dshare->last_appl_ptr = dshare->hw_ptr; + dshare->slave_appl_ptr = dshare->slave_hw_ptr = *dshare->spcm->hw.ptr; snd_pcm_direct_reset_slave_ptr(pcm, dshare); return 0; } @@ -342,6 +343,7 @@ static int snd_pcm_dshare_start_timer(snd_pcm_t *pcm, snd_pcm_direct_t *dshare) int err;
snd_pcm_hwsync(dshare->spcm); + dshare->slave_appl_ptr = dshare->slave_hw_ptr = *dshare->spcm->hw.ptr; snd_pcm_direct_reset_slave_ptr(pcm, dshare); err = snd_timer_start(dshare->timer); if (err < 0) diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index d08b624..58b1e53 100644 --- a/src/pcm/pcm_dsnoop.c +++ b/src/pcm/pcm_dsnoop.c @@ -278,6 +278,7 @@ static int snd_pcm_dsnoop_reset(snd_pcm_t *pcm) dsnoop->hw_ptr %= pcm->period_size; dsnoop->appl_ptr = dsnoop->hw_ptr; dsnoop->slave_appl_ptr = dsnoop->slave_hw_ptr; + snd_pcm_direct_reset_slave_ptr(pcm, dsnoop); return 0; }
@@ -285,12 +286,13 @@ static int snd_pcm_dsnoop_start(snd_pcm_t *pcm) { snd_pcm_direct_t *dsnoop = pcm->private_data; int err; - + if (dsnoop->state != SND_PCM_STATE_PREPARED) return -EBADFD; snd_pcm_hwsync(dsnoop->spcm); snoop_timestamp(pcm); dsnoop->slave_appl_ptr = dsnoop->slave_hw_ptr; + snd_pcm_direct_reset_slave_ptr(pcm, dsnoop); err = snd_timer_start(dsnoop->timer); if (err < 0) return err; @@ -627,6 +629,7 @@ int snd_pcm_dsnoop_open(snd_pcm_t **pcmp, const char *name, dsnoop->max_periods = opts->max_periods; dsnoop->var_periodsize = opts->var_periodsize; dsnoop->sync_ptr = snd_pcm_dsnoop_sync_ptr; + dsnoop->hw_ptr_alignment = opts->hw_ptr_alignment;
retry: if (first_instance) { @@ -771,6 +774,12 @@ pcm.name { ipc_key INT # unique IPC key ipc_key_add_uid BOOL # add current uid to unique IPC key ipc_perm INT # IPC permissions (octal, default 0600) + hw_ptr_alignment STR # Slave application and hw pointer alignment type + # STR can be one of the below strings : + # no + # roundup + # rounddown + # auto (default) slave STR # or slave { # Slave definition @@ -795,6 +804,25 @@ pcm.name { } \endcode
+<code>hw_ptr_alignment</code> specifies slave application and hw +pointer alignment type. By default hw_ptr_alignment is auto. Below are +the possible configurations: +- no: minimal latency with minimal frames dropped at startup. But + wakeup of application (return from snd_pcm_wait() or poll()) can + take up to 2 * period. +- roundup: It is guaranteed that all frames will be played at + startup. But the latency will increase upto period-1 frames. +- rounddown: It is guaranteed that a wakeup will happen for each + period and frames can be written from application. But on startup + upto period-1 frames will be dropped. +- auto: Selects the best approach depending on the used period and + buffer size. + If the application buffer size is < 2 * application period, + "roundup" will be selected to avoid over runs. If the slave_period + is < 10ms we could expect that there are low latency + requirements. Therefore "rounddown" will be chosen to avoid long + wakeup times. Else "no" will be chosen. + \subsection pcm_plugins_dsnoop_funcref Function reference
<UL>
From: Vanitha Channaiah vanitha.channaiah@in.bosch.com
For buffer size less than two period size, the start position of slave_app_ptr is rounded up in order to avoid xruns For e.g.: Considering below parameters and its values Period size = 96 Buffer size = 191 slave_appl_ptr = slave_hw_ptr = unaligned value
Issue: - During the start of the stream, app_ptr = hw_ptr = 0 - Application writes one period of data in the buffer i.e app_ptr = 96, hw_ptr = 0 - Now, the avail is just period-1 frames available. avail = hw_ptr + buffer_size - app_ptr = 95 i.e. shortage of 1 frame space - so application is waiting for the 1frame space to be available. - slave_app_ptr and slave_hw_ptr would get updated to lower values - This could lead to under run to occur.
Fix: If we round Up the slave_app_ptr pointer, - During the start of the stream, app_ptr = hw_ptr = 0 - Application writes one period of data in the buffer i.e app_ptr = 96, hw_ptr = 0 - Round Up of slave_app_ptr pointer leads to below calculation: - slave_app_ptr rounded to 96 - slave_app_ptr and slave_hw_ptr would get updated to larger value nearing to 2 period size - avail = greater than period size. - Here, there is a lower chance of under run.
Signed-off-by: Vanitha Channaiah vanitha.channaiah@in.bosch.com --- src/pcm/pcm_direct.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 54d9900..b56da85 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -2043,10 +2043,12 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf,
void snd_pcm_direct_reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) { - + /* For buffer size less than two period size, the start position + * of slave app ptr is rounded up in order to avoid xruns + */ if (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_ROUNDUP || (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_AUTO && - pcm->buffer_size <= pcm->period_size * 2)) + pcm->buffer_size < pcm->period_size * 2)) dmix->slave_appl_ptr = ((dmix->slave_appl_ptr + dmix->slave_period_size - 1) / dmix->slave_period_size) * dmix->slave_period_size;
On Wed, 15 May 2019 08:26:35 +0200, vanitha.channaiah@in.bosch.com wrote:
From: Vanitha Channaiah vanitha.channaiah@in.bosch.com
For buffer size less than two period size, the start position of slave_app_ptr is rounded up in order to avoid xruns For e.g.: Considering below parameters and its values Period size = 96 Buffer size = 191 slave_appl_ptr = slave_hw_ptr = unaligned value
Issue:
- During the start of the stream, app_ptr = hw_ptr = 0
- Application writes one period of data in the buffer i.e app_ptr = 96, hw_ptr = 0
- Now, the avail is just period-1 frames available. avail = hw_ptr + buffer_size - app_ptr = 95 i.e. shortage of 1 frame space
- so application is waiting for the 1frame space to be available.
- slave_app_ptr and slave_hw_ptr would get updated to lower values
- This could lead to under run to occur.
Fix: If we round Up the slave_app_ptr pointer,
- During the start of the stream, app_ptr = hw_ptr = 0
- Application writes one period of data in the buffer i.e app_ptr = 96, hw_ptr = 0
- Round Up of slave_app_ptr pointer leads to below calculation:
- slave_app_ptr rounded to 96
- slave_app_ptr and slave_hw_ptr would get updated to larger value nearing to 2 period size
- avail = greater than period size.
- Here, there is a lower chance of under run.
Signed-off-by: Vanitha Channaiah vanitha.channaiah@in.bosch.com
src/pcm/pcm_direct.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 54d9900..b56da85 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -2043,10 +2043,12 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf,
void snd_pcm_direct_reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) {
- /* For buffer size less than two period size, the start position
* of slave app ptr is rounded up in order to avoid xruns*/
pcm->buffer_size <= pcm->period_size * 2))
pcm->buffer_size < pcm->period_size * 2))
It's still not clear to me why this change is made.
The example mentioned in the above (period_size=96, buffer_size=191) also matches with the condition before the change, so there should be behavior change by the patch.
IOW, your patch does nothing but modifying the condition to drop the case buffer_size == period_size * 2. Why this condition can't (shouldn't) be a target of the round up? That needs the clarification in the patch description.
thanks,
Takashi
Hello Takashi-san,
It's still not clear to me why this change is made. The example mentioned in the above (period_size=96, buffer_size=191) also matches with the condition before the change, so there should be behavior change by the patch. IOW, your patch does nothing but modifying the condition to drop the case buffer_size == period_size * 2. Why this condition can't (shouldn't) be a target of the round up? That needs the clarification in the patch description.
In case of Buffer_size = 2 * period_size, round down of slave_hw_ptr was necessary which otherwise leads to Blocking of snd_pcm_wait() for longer time(i.e. more than 1n period) An example of capture case is explained here:
Issue occurs in case of round up:
- During the start, slave_hw_ptr = 128 - After round up: slave_app_ptr: 192 slave_hw_ptr: 128 - avail:0 app_ptr:0 hw_ptr:0 - snd_pcm_wait() locks - new slave hw ptr updated to plugins: new_slave_hw_ptr = 192 - hw_ptr = new_slave_hw_ptr - old_slave_hw_ptr = 192 - 128 = 64 - avail:64 app_ptr:0 hw_ptr:64 - snd_pcm_wait() still blocked --------------------> [issue occurs] - new slave hw ptr updated to plugins: new_slave_hw_ptr = 288 - avail:160 app_ptr:0 hw_ptr:160(64+96) - snd_pcm_wait() is released
In case of round down:
- During the start, slave_hw_ptr = 128 - After round up: slave_app_ptr:96 slave_hw_ptr:96 - avail:0 app_ptr:0 hw_ptr:0 - snd_pcm_wait() locks - new slave hw ptr updated to plugins: new_slave_hw_ptr = 192 - hw_ptr = new_slave_hw_ptr - old_slave_hw_ptr = 192 - 96 = 96 - avail:96 app_ptr:0 hw_ptr:96 - snd_pcm_wait() is released--------------------> [issue does not occurs] - avail:160 app_ptr:0 hw_ptr:160(64+96)
Also, No other issue is introduced in case of playback scenario.
Best regards, Vanitha Channaiah RBEI/ECF3
Tel. +91 80 6136-4436
-----Original Message----- From: Takashi Iwai tiwai@suse.de Sent: Wednesday, May 15, 2019 2:16 PM To: Channaiah Vanitha (RBEI/ECF3) Vanitha.Channaiah@in.bosch.com Cc: alsa-devel@alsa-project.org; Wischer Timo (ADITG/ESS) twischer@de.adit-jv.com Subject: Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.
On Wed, 15 May 2019 08:26:35 +0200, <vanitha.channaiah@in.bosch.commailto:vanitha.channaiah@in.bosch.com> wrote:
From: Vanitha Channaiah <vanitha.channaiah@in.bosch.commailto:vanitha.channaiah@in.bosch.com>
For buffer size less than two period size, the start position of slave_app_ptr is rounded up in order to avoid xruns For e.g.: Considering below parameters and its values Period size = 96 Buffer size = 191 slave_appl_ptr = slave_hw_ptr = unaligned value
Issue:
- During the start of the stream, app_ptr = hw_ptr = 0
- Application writes one period of data in the buffer i.e app_ptr = 96, hw_ptr = 0
- Now, the avail is just period-1 frames available. avail = hw_ptr + buffer_size - app_ptr = 95 i.e. shortage of 1 frame space
- so application is waiting for the 1frame space to be available.
- slave_app_ptr and slave_hw_ptr would get updated to lower values
- This could lead to under run to occur.
Fix: If we round Up the slave_app_ptr pointer,
- During the start of the stream, app_ptr = hw_ptr = 0
- Application writes one period of data in the buffer i.e app_ptr = 96, hw_ptr = 0
- Round Up of slave_app_ptr pointer leads to below calculation:
- slave_app_ptr rounded to 96
- slave_app_ptr and slave_hw_ptr would get updated to larger value
nearing to 2 period size
- avail = greater than period size.
- Here, there is a lower chance of under run.
Signed-off-by: Vanitha Channaiah <vanitha.channaiah@in.bosch.commailto:vanitha.channaiah@in.bosch.com>
src/pcm/pcm_direct.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 54d9900..b56da85 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -2043,10 +2043,12 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf,
void snd_pcm_direct_reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) {
/* For buffer size less than two period size, the start position* of slave app ptr is rounded up in order to avoid xruns*/ if (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_ROUNDUP || (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_AUTO &&
pcm->buffer_size <= pcm->period_size * 2))
pcm->buffer_size < pcm->period_size * 2)) dmix->slave_appl_ptr = ((dmix->slave_appl_ptr + dmix->slave_period_size - 1) / dmix->slave_period_size) * dmix->slave_period_size;
It's still not clear to me why this change is made.
The example mentioned in the above (period_size=96, buffer_size=191) also matches with the condition before the change, so there should be behavior change by the patch.
IOW, your patch does nothing but modifying the condition to drop the case buffer_size == period_size * 2. Why this condition can't (shouldn't) be a target of the round up? That needs the clarification in the patch description.
thanks,
Takashi
On Thu, 16 May 2019 19:40:35 +0200, Channaiah Vanitha (RBEI/ECF3) wrote:
Hello Takashi-san,
It's still not clear to me why this change is made. The example mentioned in the above (period_size=96, buffer_size=191) also
matches with the condition before the change, so there should be behavior change by the patch.
IOW, your patch does nothing but modifying the condition to drop the case
buffer_size == period_size * 2. Why this condition can't
(shouldn't) be a target of the round up? That needs the clarification in
the patch description.
In case of Buffer_size = 2 * period_size, round down of slave_hw_ptr was necessary which otherwise leads to Blocking of snd_pcm_wait() for longer time(i.e. more than 1n period)
An example of capture case is explained here:
Issue occurs in case of round up:
- During the start, slave_hw_ptr = 128
- After round up: slave_app_ptr: 192 slave_hw_ptr: 128
- avail:0 app_ptr:0 hw_ptr:0
- snd_pcm_wait() locks
- new slave hw ptr updated to plugins: new_slave_hw_ptr = 192
- hw_ptr = new_slave_hw_ptr - old_slave_hw_ptr = 192 - 128 = 64
- avail:64 app_ptr:0 hw_ptr:64
- snd_pcm_wait() still blocked ------------------à [issue occurs]
- new slave hw ptr updated to plugins: new_slave_hw_ptr = 288
- avail:160 app_ptr:0 hw_ptr:160(64+96)
- snd_pcm_wait() is released
In case of round down:
- During the start, slave_hw_ptr = 128
- After round up: slave_app_ptr:96 slave_hw_ptr:96
- avail:0 app_ptr:0 hw_ptr:0
- snd_pcm_wait() locks
- new slave hw ptr updated to plugins: new_slave_hw_ptr = 192
- hw_ptr = new_slave_hw_ptr - old_slave_hw_ptr = 192 - 96 = 96
- avail:96 app_ptr:0 hw_ptr:96
- snd_pcm_wait() is released------------------à [issue does not occurs]
- avail:160 app_ptr:0 hw_ptr:160(64+96)
Also, No other issue is introduced in case of playback scenario.
But the forced alignment has another drawback, namely it shifts the streaming. That is sometimes worse than the longer wakeup latency. You can't guess which behavior is preferred by user in the case of "auto" policy.
The current condition was chosen because otherwise it'll cause underrun errors. If the round down is needed for avoiding errors, it should be changed, yes. Otherwise, it needs a careful evaluation.
In anyway, the description in the patch doesn't match with the change. Please update it to fit with the actual change if we still need to take this change inevitably.
thanks,
Takashi
Best regards, Vanitha Channaiah RBEI/ECF3
Tel. +91 80 6136-4436
-----Original Message----- From: Takashi Iwai tiwai@suse.de Sent: Wednesday, May 15, 2019 2:16 PM To: Channaiah Vanitha (RBEI/ECF3) Vanitha.Channaiah@in.bosch.com Cc: alsa-devel@alsa-project.org; Wischer Timo (ADITG/ESS) twischer@de.adit-jv.com Subject: Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.
On Wed, 15 May 2019 08:26:35 +0200, vanitha.channaiah@in.bosch.com wrote:
From: Vanitha Channaiah vanitha.channaiah@in.bosch.com
For buffer size less than two period size, the start position of slave_app_ptr is rounded up in order to avoid xruns For e.g.: Considering below parameters and its values Period size = 96 Buffer size = 191 slave_appl_ptr = slave_hw_ptr = unaligned value
Issue:
- During the start of the stream, app_ptr = hw_ptr = 0
- Application writes one period of data in the buffer i.e app_ptr = 96, hw_ptr = 0
- Now, the avail is just period-1 frames available. avail = hw_ptr + buffer_size - app_ptr = 95 i.e. shortage of 1 frame space
- so application is waiting for the 1frame space to be available.
- slave_app_ptr and slave_hw_ptr would get updated to lower values
- This could lead to under run to occur.
Fix: If we round Up the slave_app_ptr pointer,
- During the start of the stream, app_ptr = hw_ptr = 0
- Application writes one period of data in the buffer i.e app_ptr = 96, hw_ptr = 0
- Round Up of slave_app_ptr pointer leads to below calculation:
- slave_app_ptr rounded to 96
- slave_app_ptr and slave_hw_ptr would get updated to larger value
nearing to 2 period size
- avail = greater than period size.
- Here, there is a lower chance of under run.
Signed-off-by: Vanitha Channaiah vanitha.channaiah@in.bosch.com
src/pcm/pcm_direct.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 54d9900..b56da85 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -2043,10 +2043,12 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf,
void snd_pcm_direct_reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) {
/* For buffer size less than two period size, the start position* of slave app ptr is rounded up in order to avoid xruns*/ if (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_ROUNDUP || (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_AUTO &&
pcm->buffer_size <= pcm->period_size * 2))
pcm->buffer_size < pcm->period_size * 2)) dmix->slave_appl_ptr = ((dmix->slave_appl_ptr + dmix->slave_period_size - 1)/
dmix->slave_period_size) * dmix->slave_period_size;It's still not clear to me why this change is made.
The example mentioned in the above (period_size=96, buffer_size=191) also matches with the condition before the change, so there should be behavior change by the patch.
IOW, your patch does nothing but modifying the condition to drop the case buffer_size == period_size * 2. Why this condition can't (shouldn't) be a target of the round up? That needs the clarification in the patch description.
thanks,
Takashi
On Thu, 16 May 2019 19:56:20 +0200, Takashi Iwai wrote:
On Thu, 16 May 2019 19:40:35 +0200, Channaiah Vanitha (RBEI/ECF3) wrote:
Hello Takashi-san,
It's still not clear to me why this change is made. The example mentioned in the above (period_size=96, buffer_size=191) also
matches with the condition before the change, so there should be behavior change by the patch.
IOW, your patch does nothing but modifying the condition to drop the case
buffer_size == period_size * 2. Why this condition can't
(shouldn't) be a target of the round up? That needs the clarification in
the patch description.
In case of Buffer_size = 2 * period_size, round down of slave_hw_ptr was necessary which otherwise leads to Blocking of snd_pcm_wait() for longer time(i.e. more than 1n period)
An example of capture case is explained here:
Issue occurs in case of round up:
- During the start, slave_hw_ptr = 128
- After round up: slave_app_ptr: 192 slave_hw_ptr: 128
- avail:0 app_ptr:0 hw_ptr:0
- snd_pcm_wait() locks
- new slave hw ptr updated to plugins: new_slave_hw_ptr = 192
- hw_ptr = new_slave_hw_ptr - old_slave_hw_ptr = 192 - 128 = 64
- avail:64 app_ptr:0 hw_ptr:64
- snd_pcm_wait() still blocked ------------------à [issue occurs]
- new slave hw ptr updated to plugins: new_slave_hw_ptr = 288
- avail:160 app_ptr:0 hw_ptr:160(64+96)
- snd_pcm_wait() is released
In case of round down:
- During the start, slave_hw_ptr = 128
- After round up: slave_app_ptr:96 slave_hw_ptr:96
- avail:0 app_ptr:0 hw_ptr:0
- snd_pcm_wait() locks
- new slave hw ptr updated to plugins: new_slave_hw_ptr = 192
- hw_ptr = new_slave_hw_ptr - old_slave_hw_ptr = 192 - 96 = 96
- avail:96 app_ptr:0 hw_ptr:96
- snd_pcm_wait() is released------------------à [issue does not occurs]
- avail:160 app_ptr:0 hw_ptr:160(64+96)
Also, No other issue is introduced in case of playback scenario.
But the forced alignment has another drawback, namely it shifts the streaming. That is sometimes worse than the longer wakeup latency. You can't guess which behavior is preferred by user in the case of "auto" policy.
Erm, scratch this, it makes further confusion, sorry. But the below still applies:
The current condition was chosen because otherwise it'll cause underrun errors. If the round down is needed for avoiding errors, it should be changed, yes. Otherwise, it needs a careful evaluation.
If buffer=2*period, the chance to slip the update is quite high unless you align the start. And the instability with 2xperiod is the very reason we've added this hack at the beginning.
thanks,
Takashi
In anyway, the description in the patch doesn't match with the change. Please update it to fit with the actual change if we still need to take this change inevitably.
thanks,
Takashi
Best regards, Vanitha Channaiah RBEI/ECF3
Tel. +91 80 6136-4436
-----Original Message----- From: Takashi Iwai tiwai@suse.de Sent: Wednesday, May 15, 2019 2:16 PM To: Channaiah Vanitha (RBEI/ECF3) Vanitha.Channaiah@in.bosch.com Cc: alsa-devel@alsa-project.org; Wischer Timo (ADITG/ESS) twischer@de.adit-jv.com Subject: Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.
On Wed, 15 May 2019 08:26:35 +0200, vanitha.channaiah@in.bosch.com wrote:
From: Vanitha Channaiah vanitha.channaiah@in.bosch.com
For buffer size less than two period size, the start position of slave_app_ptr is rounded up in order to avoid xruns For e.g.: Considering below parameters and its values Period size = 96 Buffer size = 191 slave_appl_ptr = slave_hw_ptr = unaligned value
Issue:
- During the start of the stream, app_ptr = hw_ptr = 0
- Application writes one period of data in the buffer i.e app_ptr = 96, hw_ptr = 0
- Now, the avail is just period-1 frames available. avail = hw_ptr + buffer_size - app_ptr = 95 i.e. shortage of 1 frame space
- so application is waiting for the 1frame space to be available.
- slave_app_ptr and slave_hw_ptr would get updated to lower values
- This could lead to under run to occur.
Fix: If we round Up the slave_app_ptr pointer,
- During the start of the stream, app_ptr = hw_ptr = 0
- Application writes one period of data in the buffer i.e app_ptr = 96, hw_ptr = 0
- Round Up of slave_app_ptr pointer leads to below calculation:
- slave_app_ptr rounded to 96
- slave_app_ptr and slave_hw_ptr would get updated to larger value
nearing to 2 period size
- avail = greater than period size.
- Here, there is a lower chance of under run.
Signed-off-by: Vanitha Channaiah vanitha.channaiah@in.bosch.com
src/pcm/pcm_direct.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 54d9900..b56da85 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -2043,10 +2043,12 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf,
void snd_pcm_direct_reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) {
/* For buffer size less than two period size, the start position* of slave app ptr is rounded up in order to avoid xruns*/ if (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_ROUNDUP || (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_AUTO &&
pcm->buffer_size <= pcm->period_size * 2))
pcm->buffer_size < pcm->period_size * 2)) dmix->slave_appl_ptr = ((dmix->slave_appl_ptr + dmix->slave_period_size - 1)/
dmix->slave_period_size) * dmix->slave_period_size;It's still not clear to me why this change is made.
The example mentioned in the above (period_size=96, buffer_size=191) also matches with the condition before the change, so there should be behavior change by the patch.
IOW, your patch does nothing but modifying the condition to drop the case buffer_size == period_size * 2. Why this condition can't (shouldn't) be a target of the round up? That needs the clarification in the patch description.
thanks,
Takashi
Hello Takashi-san,
Firstly, very sorry for the late reply.
The current condition was chosen because otherwise it'll cause underrun errors. If the round down is needed for avoiding errors, it should be changed, yes. Otherwise, it needs a careful evaluation.
If buffer=2*period, the chance to slip the update is quite high unless you align the start. And the instability with 2xperiod is the very reason we've added this hack at the beginning.
If this is the case, current condition would also solve under run errors for the condition buffer=2*period+1frame. For, buffer=2*period+1frame, snd_pcm_wait() is waiting for more than a period and this could lead to xruns. The chance to slip the condition is more in case of buffer=2*period-1frame. Because, after first write (one period write), avail is less than one period by 1 frame. It has to wait until next DMA interrupt. There is more chance of xruns. In case of buffer=2*period, after first write (one period write) avail is still one period size and there is less chance of xruns.
In anyway, the description in the patch doesn't match with the change. Please update it to fit with the actual change if we still need to take this change inevitably.
For buffer>=2*period, round down of slave pointers and for buffer=2*period-1frame, round up of slave pointers will avoid xruns. Which otherwise causes snd_pcm_wait() to block for more than a period time and leads to xruns.
Best regards, Vanitha Channaiah RBEI/ECF3
Tel. +91 80 6136-4436
-----Original Message----- From: Takashi Iwai tiwai@suse.de Sent: Friday, May 17, 2019 4:20 PM To: Channaiah Vanitha (RBEI/ECF3) Vanitha.Channaiah@in.bosch.com Cc: alsa-devel@alsa-project.org; Wischer Timo (ADITG/ESS) twischer@de.adit-jv.com Subject: Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.
On Thu, 16 May 2019 19:56:20 +0200, Takashi Iwai wrote:
On Thu, 16 May 2019 19:40:35 +0200, Channaiah Vanitha (RBEI/ECF3) wrote:
Hello Takashi-san,
It's still not clear to me why this change is made. The example mentioned in the above (period_size=96, buffer_size=191) also
matches with the condition before the change, so there should be behavior change by the patch.
IOW, your patch does nothing but modifying the condition to drop the case
buffer_size == period_size * 2. Why this condition can't
(shouldn't) be a target of the round up? That needs the clarification in
the patch description.
In case of Buffer_size = 2 * period_size, round down of slave_hw_ptr was necessary which otherwise leads to Blocking of snd_pcm_wait() for longer time(i.e. more than 1n period)
An example of capture case is explained here:
Issue occurs in case of round up:
- During the start, slave_hw_ptr = 128
- After round up: slave_app_ptr: 192 slave_hw_ptr: 128
- avail:0 app_ptr:0 hw_ptr:0
- snd_pcm_wait() locks
- new slave hw ptr updated to plugins: new_slave_hw_ptr = 192
- hw_ptr = new_slave_hw_ptr - old_slave_hw_ptr = 192 - 128 = 64
- avail:64 app_ptr:0 hw_ptr:64
- snd_pcm_wait() still blocked ------------------à [issue occurs]
- new slave hw ptr updated to plugins: new_slave_hw_ptr = 288
- avail:160 app_ptr:0 hw_ptr:160(64+96)
- snd_pcm_wait() is released
In case of round down:
- During the start, slave_hw_ptr = 128
- After round up: slave_app_ptr:96 slave_hw_ptr:96
- avail:0 app_ptr:0 hw_ptr:0
- snd_pcm_wait() locks
- new slave hw ptr updated to plugins: new_slave_hw_ptr = 192
- hw_ptr = new_slave_hw_ptr - old_slave_hw_ptr = 192 - 96 = 96
- avail:96 app_ptr:0 hw_ptr:96
- snd_pcm_wait() is released------------------à [issue does not
occurs]
- avail:160 app_ptr:0 hw_ptr:160(64+96)
Also, No other issue is introduced in case of playback scenario.
But the forced alignment has another drawback, namely it shifts the streaming. That is sometimes worse than the longer wakeup latency. You can't guess which behavior is preferred by user in the case of "auto" policy.
Erm, scratch this, it makes further confusion, sorry. But the below still applies:
The current condition was chosen because otherwise it'll cause underrun errors. If the round down is needed for avoiding errors, it should be changed, yes. Otherwise, it needs a careful evaluation.
If buffer=2*period, the chance to slip the update is quite high unless you align the start. And the instability with 2xperiod is the very reason we've added this hack at the beginning.
thanks,
Takashi
In anyway, the description in the patch doesn't match with the change. Please update it to fit with the actual change if we still need to take this change inevitably.
thanks,
Takashi
Best regards, Vanitha Channaiah RBEI/ECF3
Tel. +91 80 6136-4436
-----Original Message----- From: Takashi Iwai <tiwai@suse.demailto:tiwai@suse.de> Sent: Wednesday, May 15, 2019 2:16 PM To: Channaiah Vanitha (RBEI/ECF3) <Vanitha.Channaiah@in.bosch.commailto:Vanitha.Channaiah@in.bosch.com> Cc: alsa-devel@alsa-project.orgmailto:alsa-devel@alsa-project.org; Wischer Timo (ADITG/ESS) <twischer@de.adit-jv.commailto:twischer@de.adit-jv.com> Subject: Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.
On Wed, 15 May 2019 08:26:35 +0200, <vanitha.channaiah@in.bosch.commailto:vanitha.channaiah@in.bosch.com> wrote:
From: Vanitha Channaiah <vanitha.channaiah@in.bosch.commailto:vanitha.channaiah@in.bosch.com>
For buffer size less than two period size, the start position of slave_app_ptr is rounded up in order to avoid xruns For e.g.: Considering below parameters and its values Period size = 96 Buffer size = 191 slave_appl_ptr = slave_hw_ptr = unaligned value
Issue:
- During the start of the stream, app_ptr = hw_ptr = 0
- Application writes one period of data in the buffer i.e app_ptr = 96, hw_ptr = 0
- Now, the avail is just period-1 frames available. avail = hw_ptr + buffer_size - app_ptr = 95 i.e. shortage of 1 frame space
- so application is waiting for the 1frame space to be available.
- slave_app_ptr and slave_hw_ptr would get updated to lower values
- This could lead to under run to occur.
Fix: If we round Up the slave_app_ptr pointer,
- During the start of the stream, app_ptr = hw_ptr = 0
- Application writes one period of data in the buffer i.e app_ptr = 96, hw_ptr = 0
- Round Up of slave_app_ptr pointer leads to below calculation:
- slave_app_ptr rounded to 96
- slave_app_ptr and slave_hw_ptr would get updated to larger value
nearing to 2 period size
- avail = greater than period size.
- Here, there is a lower chance of under run.
Signed-off-by: Vanitha Channaiah <vanitha.channaiah@in.bosch.commailto:vanitha.channaiah@in.bosch.com>
src/pcm/pcm_direct.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 54d9900..b56da85 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -2043,10 +2043,12 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf,
void snd_pcm_direct_reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) {
/* For buffer size less than two period size, the start position* of slave app ptr is rounded up in order to avoid xruns*/ if (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_ROUNDUP || (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_AUTO &&
pcm->buffer_size <= pcm->period_size * 2))
pcm->buffer_size < pcm->period_size * 2)) dmix->slave_appl_ptr = ((dmix->slave_appl_ptr +dmix->slave_period_size - 1)
/
dmix->slave_period_size) *dmix->slave_period_size;
It's still not clear to me why this change is made.
The example mentioned in the above (period_size=96, buffer_size=191) also matches with the condition before the change, so there should be behavior change by the patch.
IOW, your patch does nothing but modifying the condition to drop the case buffer_size == period_size * 2. Why this condition can't (shouldn't) be a target of the round up? That needs the clarification in the patch description.
thanks,
Takashi
Hello Takashi-san,
Can you please reply your feedback for below mail chain.
Best regards, Vanitha Channaiah RBEI/ECF3
_____________________________________________ From: Channaiah Vanitha (RBEI/ECF3) Sent: Tuesday, June 18, 2019 4:44 AM To: 'Takashi Iwai' tiwai@suse.de Cc: alsa-devel@alsa-project.org; Wischer Timo (ADITG/ESS) twischer@de.adit-jv.com Subject: RE: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.
Hello Takashi-san,
Firstly, very sorry for the late reply.
The current condition was chosen because otherwise it'll cause underrun errors. If the round down is needed for avoiding errors, it should be changed, yes. Otherwise, it needs a careful evaluation.
If buffer=2*period, the chance to slip the update is quite high unless you align the start. And the instability with 2xperiod is the very reason we've added this hack at the beginning.
If this is the case, current condition would also solve under run errors for the condition buffer=2*period+1frame. For, buffer=2*period+1frame, snd_pcm_wait() is waiting for more than a period and this could lead to xruns. The chance to slip the condition is more in case of buffer=2*period-1frame. Because, after first write (one period write), avail is less than one period by 1 frame. It has to wait until next DMA interrupt. There is more chance of xruns. In case of buffer=2*period, after first write (one period write) avail is still one period size and there is less chance of xruns.
In anyway, the description in the patch doesn't match with the change. Please update it to fit with the actual change if we still need to take this change inevitably.
For buffer>=2*period, round down of slave pointers and for buffer=2*period-1frame, round up of slave pointers will avoid xruns. Which otherwise causes snd_pcm_wait() to block for more than a period time and leads to xruns.
Best regards, Vanitha Channaiah RBEI/ECF3
Tel. +91 80 6136-4436
-----Original Message----- From: Takashi Iwai mailto:tiwai@suse.de Sent: Friday, May 17, 2019 4:20 PM To: Channaiah Vanitha (RBEI/ECF3) mailto:Vanitha.Channaiah@in.bosch.com Cc: mailto:alsa-devel@alsa-project.org; Wischer Timo (ADITG/ESS) mailto:twischer@de.adit-jv.com Subject: Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.
On Thu, 16 May 2019 19:56:20 +0200, Takashi Iwai wrote:
On Thu, 16 May 2019 19:40:35 +0200, Channaiah Vanitha (RBEI/ECF3) wrote:
Hello Takashi-san,
It's still not clear to me why this change is made. The example mentioned in the above (period_size=96, buffer_size=191) also
matches with the condition before the change, so there should be behavior change by the patch.
IOW, your patch does nothing but modifying the condition to drop the case
buffer_size == period_size * 2. Why this condition can't
(shouldn't) be a target of the round up? That needs the clarification in
the patch description.
In case of Buffer_size = 2 * period_size, round down of slave_hw_ptr was necessary which otherwise leads to Blocking of snd_pcm_wait() for longer time(i.e. more than 1n period)
An example of capture case is explained here:
Issue occurs in case of round up:
- During the start, slave_hw_ptr = 128
- After round up: slave_app_ptr: 192 slave_hw_ptr: 128
- avail:0 app_ptr:0 hw_ptr:0
- snd_pcm_wait() locks
- new slave hw ptr updated to plugins: new_slave_hw_ptr = 192
- hw_ptr = new_slave_hw_ptr - old_slave_hw_ptr = 192 - 128 = 64
- avail:64 app_ptr:0 hw_ptr:64
- snd_pcm_wait() still blocked ------------------à [issue occurs]
- new slave hw ptr updated to plugins: new_slave_hw_ptr = 288
- avail:160 app_ptr:0 hw_ptr:160(64+96)
- snd_pcm_wait() is released
In case of round down:
- During the start, slave_hw_ptr = 128
- After round up: slave_app_ptr:96 slave_hw_ptr:96
- avail:0 app_ptr:0 hw_ptr:0
- snd_pcm_wait() locks
- new slave hw ptr updated to plugins: new_slave_hw_ptr = 192
- hw_ptr = new_slave_hw_ptr - old_slave_hw_ptr = 192 - 96 = 96
- avail:96 app_ptr:0 hw_ptr:96
- snd_pcm_wait() is released------------------à [issue does not
occurs]
- avail:160 app_ptr:0 hw_ptr:160(64+96)
Also, No other issue is introduced in case of playback scenario.
But the forced alignment has another drawback, namely it shifts the streaming. That is sometimes worse than the longer wakeup latency. You can't guess which behavior is preferred by user in the case of "auto" policy.
Erm, scratch this, it makes further confusion, sorry. But the below still applies:
The current condition was chosen because otherwise it'll cause underrun errors. If the round down is needed for avoiding errors, it should be changed, yes. Otherwise, it needs a careful evaluation.
If buffer=2*period, the chance to slip the update is quite high unless you align the start. And the instability with 2xperiod is the very reason we've added this hack at the beginning.
thanks,
Takashi
In anyway, the description in the patch doesn't match with the change. Please update it to fit with the actual change if we still need to take this change inevitably.
thanks,
Takashi
Best regards, Vanitha Channaiah RBEI/ECF3
Tel. +91 80 6136-4436
-----Original Message----- From: Takashi Iwai mailto:tiwai@suse.de Sent: Wednesday, May 15, 2019 2:16 PM To: Channaiah Vanitha (RBEI/ECF3) mailto:Vanitha.Channaiah@in.bosch.com Cc: mailto:alsa-devel@alsa-project.org; Wischer Timo (ADITG/ESS) mailto:twischer@de.adit-jv.com Subject: Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.
On Wed, 15 May 2019 08:26:35 +0200, mailto:vanitha.channaiah@in.bosch.com wrote:
From: Vanitha Channaiah mailto:vanitha.channaiah@in.bosch.com
For buffer size less than two period size, the start position of slave_app_ptr is rounded up in order to avoid xruns For e.g.: Considering below parameters and its values Period size = 96 Buffer size = 191 slave_appl_ptr = slave_hw_ptr = unaligned value
Issue:
- During the start of the stream, app_ptr = hw_ptr = 0
- Application writes one period of data in the buffer i.e app_ptr = 96, hw_ptr = 0
- Now, the avail is just period-1 frames available. avail = hw_ptr + buffer_size - app_ptr = 95 i.e. shortage of 1 frame space
- so application is waiting for the 1frame space to be available.
- slave_app_ptr and slave_hw_ptr would get updated to lower values
- This could lead to under run to occur.
Fix: If we round Up the slave_app_ptr pointer,
- During the start of the stream, app_ptr = hw_ptr = 0
- Application writes one period of data in the buffer i.e app_ptr = 96, hw_ptr = 0
- Round Up of slave_app_ptr pointer leads to below calculation:
- slave_app_ptr rounded to 96
- slave_app_ptr and slave_hw_ptr would get updated to larger value
nearing to 2 period size
- avail = greater than period size.
- Here, there is a lower chance of under run.
Signed-off-by: Vanitha Channaiah mailto:vanitha.channaiah@in.bosch.com
src/pcm/pcm_direct.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 54d9900..b56da85 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -2043,10 +2043,12 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf,
void snd_pcm_direct_reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) {
/* For buffer size less than two period size, the start position* of slave app ptr is rounded up in order to avoid xruns*/ if (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_ROUNDUP || (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_AUTO &&
pcm->buffer_size <= pcm->period_size * 2))
pcm->buffer_size < pcm->period_size * 2)) dmix->slave_appl_ptr = ((dmix->slave_appl_ptr +dmix->slave_period_size - 1)
/
dmix->slave_period_size) *dmix->slave_period_size;
It's still not clear to me why this change is made.
The example mentioned in the above (period_size=96, buffer_size=191) also matches with the condition before the change, so there should be behavior change by the patch.
IOW, your patch does nothing but modifying the condition to drop the case buffer_size == period_size * 2. Why this condition can't (shouldn't) be a target of the round up? That needs the clarification in the patch description.
thanks,
Takashi
On Tue, 16 Jul 2019 05:57:51 +0200, Channaiah Vanitha (RBEI/ECF3) wrote:
Hello Takashi-san,
Can you please reply your feedback for below mail chain.
Best regards, Vanitha Channaiah RBEI/ECF3
From: Channaiah Vanitha (RBEI/ECF3) Sent: Tuesday, June 18, 2019 4:44 AM To: 'Takashi Iwai' tiwai@suse.de Cc: alsa-devel@alsa-project.org; Wischer Timo (ADITG/ESS) twischer@de.adit-jv.com Subject: RE: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.
Hello Takashi-san,
Firstly, very sorry for the late reply.
The current condition was chosen because otherwise it'll cause underrun errors. If the round down is needed for avoiding errors, it should be changed, yes. Otherwise, it needs a careful evaluation.
If buffer=2*period, the chance to slip the update is quite high unless you align the start. And the instability with 2xperiod is the very reason we've added this hack at the beginning.
If this is the case, current condition would also solve under run errors for the condition buffer=2*period+1frame. For, buffer=2*period+1frame, snd_pcm_wait() is waiting for more than a period and this could lead to xruns. The chance to slip the condition is more in case of buffer=2*period-1frame. Because, after first write (one period write), avail is less than one period by 1 frame. It has to wait until next DMA interrupt. There is more chance of xruns. In case of buffer=2*period, after first write (one period write) avail is still one period size and there is less chance of xruns.
In anyway, the description in the patch doesn't match with the change. Please update it to fit with the actual change if we still need to take this change inevitably.
For buffer>=2*period, round down of slave pointers and for buffer=2*period-1frame, round up of slave pointers will avoid xruns. Which otherwise causes snd_pcm_wait() to block for more than a period time and leads to xruns.
Hmm, it's still not clear at all. Please repost the patch with a more elaborated and correct description that matches with the code change.
thanks,
Takashi
From: Vanitha Channaiah vanitha.channaiah@in.bosch.com
snd_pcm_sw_params() is reformatted by using _snd_pcm_sw_params_internal() function. Critical section of snd_pcm_sw_param() is moved to _snd_pcm_sw_params_internal(). This allows to call the snd_pcm_sw_params() function from an internal function which has already locked the API mutex. Calling snd_pcm_sw_params() from an internal function with locked API mutex would end up in an deadlock because recursive locking is not supported. This patch doesnot change the behavior or the functionality. To avoid double lock conditions, a separate _snd_pcm_sw_params_internal() function is added which can be used internally by any other functions in alsa-lib
Signed-off-by: Vanitha Channaiah vanitha.channaiah@in.bosch.com --- src/pcm/pcm.c | 12 +----------- src/pcm/pcm_local.h | 1 + src/pcm/pcm_params.c | 21 +++++++++++++++++++++ 3 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c index 3a71d79..f0db545 100644 --- a/src/pcm/pcm.c +++ b/src/pcm/pcm.c @@ -968,21 +968,11 @@ int snd_pcm_sw_params(snd_pcm_t *pcm, snd_pcm_sw_params_t *params) } #endif __snd_pcm_lock(pcm); /* forced lock due to pcm field change */ - err = pcm->ops->sw_params(pcm->op_arg, params); + err = _snd_pcm_sw_params_internal(pcm, params); if (err < 0) { __snd_pcm_unlock(pcm); return err; } - pcm->tstamp_mode = params->tstamp_mode; - pcm->tstamp_type = params->tstamp_type; - pcm->period_step = params->period_step; - pcm->avail_min = params->avail_min; - pcm->period_event = sw_get_period_event(params); - pcm->start_threshold = params->start_threshold; - pcm->stop_threshold = params->stop_threshold; - pcm->silence_threshold = params->silence_threshold; - pcm->silence_size = params->silence_size; - pcm->boundary = params->boundary; __snd_pcm_unlock(pcm); return 0; } diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index d52229d..e103f72 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -661,6 +661,7 @@ static inline int muldiv_near(int a, int b, int c) return n; }
+int _snd_pcm_sw_params_internal(snd_pcm_t *pcm, snd_pcm_sw_params_t *params); int snd_pcm_hw_refine(snd_pcm_t *pcm, snd_pcm_hw_params_t *params); int _snd_pcm_hw_params_internal(snd_pcm_t *pcm, snd_pcm_hw_params_t *params); #undef _snd_pcm_hw_params diff --git a/src/pcm/pcm_params.c b/src/pcm/pcm_params.c index 8826bc3..3ba05fb 100644 --- a/src/pcm/pcm_params.c +++ b/src/pcm/pcm_params.c @@ -2439,3 +2439,24 @@ int _snd_pcm_hw_params_internal(snd_pcm_t *pcm, snd_pcm_hw_params_t *params) return 0; }
+int _snd_pcm_sw_params_internal(snd_pcm_t *pcm, snd_pcm_sw_params_t *params) +{ + int err; + + assert(pcm && params); + assert(pcm->setup); + err = pcm->ops->sw_params(pcm->op_arg, params); + if (err < 0) + return err; + pcm->tstamp_mode = params->tstamp_mode; + pcm->tstamp_type = params->tstamp_type; + pcm->period_step = params->period_step; + pcm->avail_min = params->avail_min; + pcm->period_event = sw_get_period_event(params); + pcm->start_threshold = params->start_threshold; + pcm->stop_threshold = params->stop_threshold; + pcm->silence_threshold = params->silence_threshold; + pcm->silence_size = params->silence_size; + pcm->boundary = params->boundary; + return 0; +}
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. - 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 - 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); + } offset += frames; size -= frames; xfer += frames; 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;
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
Hello Takashi-san,
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.
Issue can be seen without direct plugins too with involvement of only hw which reports unaligned hw ptr. As I have explained in below detailed description: "Also, this issue can be seen without dsnoop plugin, when HW reports unaligned hw_ptr For e.g. "
Best regards, Vanitha Channaiah RBEI/ECF3
Tel. +91 80 6136-4436
-----Original Message----- From: Takashi Iwai tiwai@suse.de Sent: Wednesday, May 15, 2019 9:03 PM To: Channaiah Vanitha (RBEI/ECF3) Vanitha.Channaiah@in.bosch.com Cc: alsa-devel@alsa-project.org; Wischer Timo (ADITG/ESS) twischer@de.adit-jv.com Subject: Re: [PATCH v2 6/6] pcm: Update pcm->avail_min with needed_slave_avail_min, after reading unaligned frames
On Wed, 15 May 2019 08:26:37 +0200, <vanitha.channaiah@in.bosch.commailto:vanitha.channaiah@in.bosch.com> wrote:
From: Vanitha Channaiah <vanitha.channaiah@in.bosch.commailto: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.commailto: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);} offset += frames; size -= frames; xfer += frames;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
Dne 16. 05. 19 v 19:26 Channaiah Vanitha (RBEI/ECF3) napsal(a):
Hello Takashi-san,
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.
Issue can be seen without direct plugins too with involvement of only hw which reports unaligned hw ptr. As I have explained in below detailed description: "Also, this issue can be seen without dsnoop plugin, when HW reports unaligned hw_ptr For e.g. "
Which hardware exactly? The hw_ptr should be reset when the streaming starts. It appears that the problem is specific to the direct plugins only when the period wakeups are a bit different than for the direct hardware access.
Jaroslav
Hello Jaroslav-san, Takashi-san,
Which hardware exactly? The hw_ptr should be reset when the streaming starts. It appears that the problem is specific to the direct plugins only when the period wakeups are a bit different than for the direct hardware access.
Firstly, sorry for late reply.
Issue is seen in RCar Kingfischer H3 es2.0 target.
The issue was seen during the below commit : commit 07b7acb51d283d8469696c906b91f1882696a4d4 ("ASoC: rsnd: update pointer more accurate") https://patchwork.kernel.org/patch/9772671/
There could be a non-uniform jitter exists between when interrupt raised [rcar_dmac_isr_channel(), rcar_dmac_isr_channel_thread()] and the interrupt is processed to read/calculate the DMA position [dma_set_residue()] This could result in unaligned hw_ptr reported at user-space alsa lib.
Best regards, Vanitha Channaiah RBEI/ECF3
Tel. +91 80 6136-4436
-----Original Message----- From: Jaroslav Kysela perex@perex.cz Sent: Thursday, May 16, 2019 11:05 PM To: Channaiah Vanitha (RBEI/ECF3) Vanitha.Channaiah@in.bosch.com; Takashi Iwai tiwai@suse.de Cc: Wischer Timo (ADITG/ESS) twischer@de.adit-jv.com; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] [PATCH v2 6/6] pcm: Update pcm->avail_min with needed_slave_avail_min, after reading unaligned frames
Dne 16. 05. 19 v 19:26 Channaiah Vanitha (RBEI/ECF3) napsal(a):
Hello Takashi-san,
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.
Issue can be seen without direct plugins too with involvement of only hw which reports unaligned hw ptr. As I have explained in below detailed description: "Also, this issue can be seen without dsnoop plugin, when HW reports unaligned hw_ptr For e.g. "
Which hardware exactly? The hw_ptr should be reset when the streaming starts. It appears that the problem is specific to the direct plugins only when the period wakeups are a bit different than for the direct hardware access.
Jaroslav
-- Jaroslav Kysela <perex@perex.czmailto:perex@perex.cz> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
Hello Jaroslav-san, Takashi-san,
Can you please reply your feedback for below mail chain.
Best regards, Vanitha Channaiah RBEI/ECF3
_____________________________________________ From: Channaiah Vanitha (RBEI/ECF3) Sent: Tuesday, June 18, 2019 4:45 AM To: 'Jaroslav Kysela' perex@perex.cz; Takashi Iwai tiwai@suse.de Cc: Wischer Timo (ADITG/ESS) twischer@de.adit-jv.com; alsa-devel@alsa-project.org Subject: RE: [alsa-devel] [PATCH v2 6/6] pcm: Update pcm->avail_min with needed_slave_avail_min, after reading unaligned frames
Hello Jaroslav-san, Takashi-san,
Which hardware exactly? The hw_ptr should be reset when the streaming starts. It appears that the problem is specific to the direct plugins only when the period wakeups are a bit different than for the direct hardware access.
Firstly, sorry for late reply.
Issue is seen in RCar Kingfischer H3 es2.0 target.
The issue was seen during the below commit : commit 07b7acb51d283d8469696c906b91f1882696a4d4 ("ASoC: rsnd: update pointer more accurate") https://patchwork.kernel.org/patch/9772671/
There could be a non-uniform jitter exists between when interrupt raised [rcar_dmac_isr_channel(), rcar_dmac_isr_channel_thread()] and the interrupt is processed to read/calculate the DMA position [dma_set_residue()] This could result in unaligned hw_ptr reported at user-space alsa lib.
Best regards, Vanitha Channaiah RBEI/ECF3
Tel. +91 80 6136-4436
-----Original Message----- From: Jaroslav Kysela mailto:perex@perex.cz Sent: Thursday, May 16, 2019 11:05 PM To: Channaiah Vanitha (RBEI/ECF3) mailto:Vanitha.Channaiah@in.bosch.com; Takashi Iwai mailto:tiwai@suse.de Cc: Wischer Timo (ADITG/ESS) mailto:twischer@de.adit-jv.com; mailto:alsa-devel@alsa-project.org Subject: Re: [alsa-devel] [PATCH v2 6/6] pcm: Update pcm->avail_min with needed_slave_avail_min, after reading unaligned frames
Dne 16. 05. 19 v 19:26 Channaiah Vanitha (RBEI/ECF3) napsal(a):
Hello Takashi-san,
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.
Issue can be seen without direct plugins too with involvement of only hw which reports unaligned hw ptr. As I have explained in below detailed description: "Also, this issue can be seen without dsnoop plugin, when HW reports unaligned hw_ptr For e.g. "
Which hardware exactly? The hw_ptr should be reset when the streaming starts. It appears that the problem is specific to the direct plugins only when the period wakeups are a bit different than for the direct hardware access.
Jaroslav
-- Jaroslav Kysela mailto:perex@perex.cz Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
On Tue, 16 Jul 2019 05:58:15 +0200, Channaiah Vanitha (RBEI/ECF3) wrote:
Hello Jaroslav-san, Takashi-san,
Can you please reply your feedback for below mail chain.
Best regards, Vanitha Channaiah RBEI/ECF3
From: Channaiah Vanitha (RBEI/ECF3) Sent: Tuesday, June 18, 2019 4:45 AM To: 'Jaroslav Kysela' perex@perex.cz; Takashi Iwai tiwai@suse.de Cc: Wischer Timo (ADITG/ESS) twischer@de.adit-jv.com; alsa-devel@alsa-project.org Subject: RE: [alsa-devel] [PATCH v2 6/6] pcm: Update pcm->avail_min with needed_slave_avail_min, after reading unaligned frames
Hello Jaroslav-san, Takashi-san,
Which hardware exactly? The hw_ptr should be reset when the streaming starts. It appears that the problem is specific to the direct plugins only when the period wakeups are a bit different than for the direct hardware access.
Firstly, sorry for late reply.
Issue is seen in RCar Kingfischer H3 es2.0 target.
The issue was seen during the below commit : commit 07b7acb51d283d8469696c906b91f1882696a4d4 ("ASoC: rsnd: update pointer more accurate") https://patchwork.kernel.org/patch/9772671/
There could be a non-uniform jitter exists between when interrupt raised [rcar_dmac_isr_channel(), rcar_dmac_isr_channel_thread()] and the interrupt is processed to read/calculate the DMA position [dma_set_residue()] This could result in unaligned hw_ptr reported at user-space alsa lib.
It looks rather like a workaround for the bug in driver. Better fix the driver instead.
thanks,
Takashi
participants (4)
-
Channaiah Vanitha (RBEI/ECF3) -
Jaroslav Kysela -
Takashi Iwai -
vanitha.channaiah＠in.bosch.com