[PATCH 0/2] alsa-lib: dynamically adapt the avail_min on the slave

[PATCH 0/2] alsa-lib: dynamically adapt the avail_min on the slave

[Jiada Wang]

When configuring avail_min to multiple of slave period size it can happen
that user waits one slave period longer than needed for available data. 
Root cause is implicit grabbing of slave samples in avail_update operation. 
On next entering poll, the slave will wait for the avail_min threshold 
reached again, as he is not aware that there are already pending samples 
in the above layer which are not yet provided to user. 
Solution is to dynamically adapt the avail_min on the slave. 


Andreas Pape (2):
  plugin: dynamically update avail_min on slave
  rate: dynamic update avail_min on slave

 src/pcm/pcm_plugin.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 src/pcm/pcm_plugin.h |  3 +++
 src/pcm/pcm_rate.c   |  2 +-
 3 files changed, 51 insertions(+), 2 deletions(-)

-- 
2.9.3

[PATCH 1/2] plugin: dynamically update avail_min on slave

[Jiada Wang]

From: Andreas Pape <apape@de.adit-jv.com>

mmapped capture access on some plugins can fetch data from slave in the 'background'.
A subsequent snd_pcm_wait waits for too long time to reach avail_min threshold again.
Waiting too long leads to xruns on other devices waiting for the capture data.

As a fix the avail_min on slave is recalculated dynamically.

Signed-off-by: Andreas Pape <apape@de.adit-jv.com>
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
 src/pcm/pcm_plugin.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c
index e53c5bb..c78316b 100644
--- a/src/pcm/pcm_plugin.c
+++ b/src/pcm/pcm_plugin.c
@@ -535,6 +535,52 @@ static int snd_pcm_plugin_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
 	return 0;
 }
 
+static int snd_pcm_plugin_may_wait_for_avail_min(snd_pcm_t *pcm, snd_pcm_uframes_t avail)
+{
+	if (pcm->stream == SND_PCM_STREAM_CAPTURE &&
+	    pcm->access != SND_PCM_ACCESS_RW_INTERLEAVED &&
+	    pcm->access != SND_PCM_ACCESS_RW_NONINTERLEAVED) {
+		/* mmap access on capture device already consumes data from slave in avail_update operation (@see snd_pcm_plugin_avail_update).
+		  Entering snd_pcm_wait after having already consumed some fragments leads to waiting for too long time,
+		  as slave will unnecessarily wait for avail_min condition reached again.
+		  To avoid unnecessary wait times we adapt the avail_min threshold on slave dynamically.
+		  Just modifying slave->avail_min as a shortcut and lightweight solution does not work for all slave plugin types
+		  and in addition it will not propagate the change through all downstream plugins, so we have to use the sw_params API.
+		  note: reading fragmental parts from slave will only happen in case
+			a) the slave can provide contineous hw_ptr between periods
+			b) avail_min does not match one slave_period
+		*/
+		snd_pcm_plugin_t *plugin = pcm->private_data;
+		snd_pcm_t *slave = plugin->gen.slave;
+		snd_pcm_uframes_t needed_slave_avail_min;
+		snd_pcm_sframes_t available;
+
+		/* update, as it might have changed. This will also call avail_update on slave and also can return error*/
+		available = snd_pcm_avail_update(pcm);
+		if (available < 0)
+			return 0;
+
+		if (available >= pcm->avail_min)
+			/*don't wait at all. As we can't configure avail_min of slave to 0 return here*/
+			return 0;
+
+		needed_slave_avail_min = pcm->avail_min - available;
+		if (slave->avail_min != needed_slave_avail_min) {
+			snd_pcm_sw_params_t *swparams;
+			snd_pcm_sw_params_alloca(&swparams);
+			/* pray that changing sw_params while running is properly implemented in all downstream plugins... it's legal but not commonly used.*/
+			snd_pcm_sw_params_current(slave, swparams);
+			/* snd_pcm_sw_params_set_avail_min() restricts setting to >= period size.
+			This conflicts at least with our dshare patch which allows combining multiple periods or with slaves which return hw postions between periods
+			-> set directly in sw_param structure */
+			swparams->avail_min = needed_slave_avail_min;
+			snd_pcm_sw_params(slave, swparams);
+		}
+		avail = available;
+	}
+	return snd_pcm_generic_may_wait_for_avail_min(pcm, avail);
+}
+
 const snd_pcm_fast_ops_t snd_pcm_plugin_fast_ops = {
 	.status = snd_pcm_plugin_status,
 	.state = snd_pcm_generic_state,
@@ -564,7 +610,7 @@ const snd_pcm_fast_ops_t snd_pcm_plugin_fast_ops = {
 	.poll_descriptors_count = snd_pcm_generic_poll_descriptors_count,
 	.poll_descriptors = snd_pcm_generic_poll_descriptors,
 	.poll_revents = snd_pcm_generic_poll_revents,
-	.may_wait_for_avail_min = snd_pcm_generic_may_wait_for_avail_min,
+	.may_wait_for_avail_min = snd_pcm_plugin_may_wait_for_avail_min,
 };
 
 #endif
-- 
2.9.3

[PATCH 2/2] rate: dynamic update avail_min on slave

[Jiada Wang]

From: Andreas Pape <apape@de.adit-jv.com>

Signed-off-by: Andreas Pape <apape@de.adit-jv.com>
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
 src/pcm/pcm_plugin.c | 2 +-
 src/pcm/pcm_plugin.h | 3 +++
 src/pcm/pcm_rate.c   | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c
index c78316b..e045732 100644
--- a/src/pcm/pcm_plugin.c
+++ b/src/pcm/pcm_plugin.c
@@ -535,7 +535,7 @@ static int snd_pcm_plugin_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
 	return 0;
 }
 
-static int snd_pcm_plugin_may_wait_for_avail_min(snd_pcm_t *pcm, snd_pcm_uframes_t avail)
+int snd_pcm_plugin_may_wait_for_avail_min(snd_pcm_t *pcm, snd_pcm_uframes_t avail)
 {
 	if (pcm->stream == SND_PCM_STREAM_CAPTURE &&
 	    pcm->access != SND_PCM_ACCESS_RW_INTERLEAVED &&
diff --git a/src/pcm/pcm_plugin.h b/src/pcm/pcm_plugin.h
index 217f075..95aacb3 100644
--- a/src/pcm/pcm_plugin.h
+++ b/src/pcm/pcm_plugin.h
@@ -50,6 +50,8 @@ typedef struct {
 /* make local functions really local */
 #define snd_pcm_plugin_init \
 	snd1_pcm_plugin_init
+#define snd_pcm_plugin_may_wait_for_avail_min \
+	snd1_pcm_plugin_may_wait_for_avail_min
 #define snd_pcm_plugin_fast_ops \
 	snd1_pcm_plugin_fast_ops
 #define snd_pcm_plugin_undo_read_generic \
@@ -64,6 +66,7 @@ typedef struct {
 void snd_pcm_plugin_init(snd_pcm_plugin_t *plugin);
 snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames);
 snd_pcm_sframes_t snd_pcm_plugin_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames);
+int snd_pcm_plugin_may_wait_for_avail_min(snd_pcm_t *pcm, snd_pcm_uframes_t avail);
 
 extern const snd_pcm_fast_ops_t snd_pcm_plugin_fast_ops;
 
diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
index 6184def..b0a1a48 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -1156,7 +1156,7 @@ static const snd_pcm_fast_ops_t snd_pcm_rate_fast_ops = {
 	.poll_descriptors_count = snd_pcm_generic_poll_descriptors_count,
 	.poll_descriptors = snd_pcm_generic_poll_descriptors,
 	.poll_revents = snd_pcm_rate_poll_revents,
-	.may_wait_for_avail_min = snd_pcm_generic_may_wait_for_avail_min,
+	.may_wait_for_avail_min = snd_pcm_plugin_may_wait_for_avail_min,
 };
 
 static const snd_pcm_ops_t snd_pcm_rate_ops = {
-- 
2.9.3

Re: [PATCH 0/2] alsa-lib: dynamically adapt the avail_min on the slave

[Takashi Iwai]

On Thu, 10 Nov 2016 08:34:41 +0100,
Jiada Wang wrote:
> 
> When configuring avail_min to multiple of slave period size it can happen
> that user waits one slave period longer than needed for available data. 
> Root cause is implicit grabbing of slave samples in avail_update operation. 
> On next entering poll, the slave will wait for the avail_min threshold 
> reached again, as he is not aware that there are already pending samples 
> in the above layer which are not yet provided to user. 
> Solution is to dynamically adapt the avail_min on the slave. 

Can you give a test case?  Then it's easier to check what's going on.

In general, I prefer having this only in the rate plugin, as the rate
plugin is the only one that suffers from the problem for now.
If the issue is really reproduced on other plugins, make it generic in
the plugin code later.

About your fix: the dynamic changing sw_params is hackish, so I wonder
whether it's the best solution.  Let's see.

Last but not least, please try to fit within 80 chars like kernel
patches.  It's not strictly required for alsa-lib codes, but still the
too long lines like in your patches are difficult to read.


thanks,

Takashi

Re: [PATCH 0/2] alsa-lib: dynamically adapt the avail_min on the slave

[Jiada Wang]

Hello Takashi-san

On 11/11/2016 08:10 PM, Takashi Iwai wrote:
> On Thu, 10 Nov 2016 08:34:41 +0100,
> Jiada Wang wrote:
>>
>> When configuring avail_min to multiple of slave period size it can happen
>> that user waits one slave period longer than needed for available data.
>> Root cause is implicit grabbing of slave samples in avail_update operation.
>> On next entering poll, the slave will wait for the avail_min threshold
>> reached again, as he is not aware that there are already pending samples
>> in the above layer which are not yet provided to user.
>> Solution is to dynamically adapt the avail_min on the slave.
>
> Can you give a test case?  Then it's easier to check what's going on.
>
Sorry, I missed your comment
Following is the error scenario
use plugin which uses the generic plugin implementation from pcm_plugin.c
(e.g linear plugin or the easiest one: 'copy' plugin)
-use MMAP capture access on that plugin
-configure slave hardware to 4ms period
-configure avail_min to 8ms
-enter snd_pcm_wait after 4ms are already available.
-->user may unexpectedly wait for 4ms too long

> In general, I prefer having this only in the rate plugin, as the rate
> plugin is the only one that suffers from the problem for now.
> If the issue is really reproduced on other plugins, make it generic in
> the plugin code later.
>
> About your fix: the dynamic changing sw_params is hackish, so I wonder
> whether it's the best solution.  Let's see.
>
> Last but not least, please try to fit within 80 chars like kernel
> patches.  It's not strictly required for alsa-lib codes, but still the
> too long lines like in your patches are difficult to read.
>
Sure, I will update commit message in next update

Thanks,
Jiada

>
> thanks,
>
> Takashi
>

Re: [PATCH 0/2] alsa-lib: dynamically adapt the avail_min on the slave

[Jiada Wang]

Hi Takashi

On 11/11/2016 08:10 PM, Takashi Iwai wrote:
> On Thu, 10 Nov 2016 08:34:41 +0100,
> Jiada Wang wrote:
>>
>> When configuring avail_min to multiple of slave period size it can happen
>> that user waits one slave period longer than needed for available data.
>> Root cause is implicit grabbing of slave samples in avail_update operation.
>> On next entering poll, the slave will wait for the avail_min threshold
>> reached again, as he is not aware that there are already pending samples
>> in the above layer which are not yet provided to user.
>> Solution is to dynamically adapt the avail_min on the slave.
>
> Can you give a test case?  Then it's easier to check what's going on.
>
> In general, I prefer having this only in the rate plugin, as the rate
> plugin is the only one that suffers from the problem for now.
> If the issue is really reproduced on other plugins, make it generic in
> the plugin code later.
>
not only the rate plugin is suffering from this issue,
but also other plugins which use the generic plugin implementation
(for example linear and copy plugin), so only have the fix in rate 
plugin, won't prevent the issue we are seeing.

> About your fix: the dynamic changing sw_params is hackish, so I wonder
> whether it's the best solution.  Let's see.
>
Yes, we also think this change is risky, and would like to have it be 
reviewed by community, so that we can have a more robust solution

> Last but not least, please try to fit within 80 chars like kernel
> patches.  It's not strictly required for alsa-lib codes, but still the
> too long lines like in your patches are difficult to read.
>
I will update ChangeLog in next update

Thanks,
Jiada
>
> thanks,
>
> Takashi
>