[PATCH v2 0/6]

[PATCH v2 0/6]

[vanitha.channaiah]

- 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,

[PATCH v2 1/6] pcm: direct: Add generic hw_ptr_alignment function for dmix, dshare and dsnoop

[vanitha.channaiah]

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;
-- 
2.7.4

Re: [PATCH v2 1/6] pcm: direct: Add generic hw_ptr_alignment function for dmix, dshare and dsnoop

[Takashi Iwai]

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

[PATCH v2 2/6] pcm: dshare: Added "hw_ptr_alignment" option in configuration for alignment of slave pointers

[vanitha.channaiah]

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>
-- 
2.7.4

[PATCH v2 3/6] pcm: dsnoop: Added "hw_ptr_alignment" option in configuration for slave pointer alignment

[vanitha.channaiah]

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>
-- 
2.7.4

[PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.

[vanitha.channaiah]

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;
-- 
2.7.4

Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.

[Takashi Iwai]

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

Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.

[Channaiah Vanitha (RBEI/ECF3)]

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.com<mailto:vanitha.channaiah@in.bosch.com>> wrote:
>
> From: Vanitha Channaiah <vanitha.channaiah@in.bosch.com<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 <vanitha.channaiah@in.bosch.com<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

Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.

[Takashi Iwai]

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
> 
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.

[Takashi Iwai]

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
> > 
> > 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.

[Channaiah Vanitha (RBEI/ECF3)]

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.de<mailto:tiwai@suse.de>>
> > Sent: Wednesday, May 15, 2019 2:16 PM
> > To: Channaiah Vanitha (RBEI/ECF3) <Vanitha.Channaiah@in.bosch.com<mailto:Vanitha.Channaiah@in.bosch.com>>
> > Cc: alsa-devel@alsa-project.org<mailto:alsa-devel@alsa-project.org>; Wischer Timo (ADITG/ESS)
> > <twischer@de.adit-jv.com<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,
> > <vanitha.channaiah@in.bosch.com<mailto:vanitha.channaiah@in.bosch.com>> wrote:
> > >
> > > From: Vanitha Channaiah <vanitha.channaiah@in.bosch.com<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 <vanitha.channaiah@in.bosch.com<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
> >
> >

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.

[Channaiah Vanitha (RBEI/ECF3)]

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
> > 
> > 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.

[Takashi Iwai]

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

[PATCH v2 5/6] pcm: restructuring sw params function

[vanitha.channaiah]

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;
+}
-- 
2.7.4

[PATCH v2 6/6] pcm: Update pcm->avail_min with needed_slave_avail_min, after reading unaligned frames

[vanitha.channaiah]

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;
-- 
2.7.4

Re: [PATCH v2 6/6] pcm: Update pcm->avail_min with needed_slave_avail_min, after reading unaligned frames

[Takashi Iwai]

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

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

Re: [PATCH v2 6/6] pcm: Update pcm->avail_min with needed_slave_avail_min, after reading unaligned frames

[Channaiah Vanitha (RBEI/ECF3)]

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.com<mailto:vanitha.channaiah@in.bosch.com>> wrote:
>
> From: Vanitha Channaiah <vanitha.channaiah@in.bosch.com<mailto: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<mailto: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

Re: [PATCH v2 6/6] pcm: Update pcm->avail_min with needed_slave_avail_min, after reading unaligned frames

[Jaroslav Kysela]

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.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH v2 6/6] pcm: Update pcm->avail_min with needed_slave_avail_min, after reading unaligned frames

[Channaiah Vanitha (RBEI/ECF3)]

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.cz<mailto:perex@perex.cz>>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH v2 6/6] pcm: Update pcm->avail_min with needed_slave_avail_min, after reading unaligned frames

[Channaiah Vanitha (RBEI/ECF3)]

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.

Re: [PATCH v2 6/6] pcm: Update pcm->avail_min with needed_slave_avail_min, after reading unaligned frames

[Takashi Iwai]

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