At Fri, 9 Nov 2012 17:19:32 -0800, Trent Piepho wrote:
On Fri, Nov 9, 2012 at 8:57 AM, Takashi Iwai tiwai@suse.de wrote:
At Thu, 8 Nov 2012 13:47:52 -0800, Trent Piepho wrote:
snd_pcm_wait() doesn't sleep, because it's based on the status of the hw buffer and that buffer does have free space. snd_pcm_write_areas() would write to the hw buffer and not try to sleep. But it's not looking at the hw buffer when it decides to sleep or write, it's looking at the rate buffer. And the rate buffer's fill level says to sleep, not write. And if snd_pcm_wait() was looking at the rate buffer, it would sleep.
Yeah, this is a known longtime PITA in the rate plugin.
Tweaking avail_min is also an option, but I'm afraid it's too hackish.
The primary problem is the discrepancy of the state "I have to wait until period size becomes available" between the rate plugin and the underlying PCM. Since the poll is performed only on the slave PCM, checking the rate plugin's avail is bogus in such a state. In other words, if the rate plugin puts the decision for the sleep simply depending on the slave state, there will be no inconsistency between them.
Below is a quick fix. Does it work for you?
It adds an extra call to the kernel to get the HW pointer position yet again. snd_pcm_avail_update() is called initially by snd_pcm_write_areas.
Oops, I forgot to replace it with snd_pcm_mmap_avail(), which has no extra update call.
The avail_update method for the rate plugin calls snd_pcm_avail_update on the slave. It's worth noting at this point that we know that avail(rate) + unresampled_data = avail(slave). Then check_for_wait() is called right after, which calls the rate plugin's check_for_wait method, which calls snd_pcm_avail_update() on the slave a second time.
It seems like you have a race if a period elapses between the two checks of the slave's avail. Suppose upon entering snd_pcm_write_areas both buffers are 100% full. avail = snd_pcm_update_avail(pcm /*rate*/) returns zero, which is up to date and correct. We get to the code for check_for_wait() and it calls snd_pcm_avail_update(slave) and a period has elapsed since "avail" was found. So check_for_wait() returns not to wait, which is correct based on the current state of the slave. Now the rest of the snd_pcm_write_areas() code runs, but avail=0 still and it's not going to work, it just blows up if avail = 0.
If you think about it, tweaking avail_min does the same thing as using the slave's avail and avail_min.
We had a wait check of "rate->avail < rate->avail_min". What we want is "slave->avail < slave->avail_min". Ignoring the resampling factor, we know that: rate->avail + rate->unresampled_data = slave->avail rate->avail_min = slave->avail_min
So if we do a little algebra: slave->avail < slave->avail_min rate->avail + rate->unresampled_data < slave->avail_min rate->avail + rate->unresampled_data < rate->avail_min rate->avail < rate->avail_min - rate->unresampled_data
In other words, subtracting the unresampled data from the avail_min for the rate pcm produces the exact same comparison as looking at the slave's avail and avail_min, which is what we actually wanted.
Well, thinking more on this, I don't think fiddling avail_min always works.
First off, avail_min is a value set by the user and can be referred by the outside during operation (until user again changes via sw_params call). Changing this internally may cause an unexpected side effect in other codes -- remember that there can be an external plugin too. (We do some ugly avail_min manipulations in the rate drain callback, but it's closed only in the function, thus the impact must be minimum.)
Secondly, the avail_min modification doesn't work if another plugin is deployed over the rate plugin. For example, using PCM foo below with S8 format in your test program will again hog CPU.
pcm.foo { type linear slave.format S8 slave.pcm { type plug; slave { pcm "hw:0,0"; rate 48000; } } }
It's because the inconsistent state of the rate plugin isn't considered in the upper layer. Obviously, for fixing this, we need to propagate the workaround for the avail_min check.
My previous patch doesn't work for this case, too, but it's easy to make it working. A revised patch is attached below. It still doesn't cover all plugins (e.g. multi plugin), but for testing it should suffice.
thanks,
Takashi
--- diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c index 5880057..393c357 100644 --- a/src/pcm/pcm.c +++ b/src/pcm/pcm.c @@ -2352,7 +2352,7 @@ int snd_pcm_open_named_slave(snd_pcm_t **pcmp, const char *name, */ int snd_pcm_wait(snd_pcm_t *pcm, int timeout) { - if (snd_pcm_mmap_avail(pcm) >= pcm->avail_min) { + if (snd_pcm_may_wait_for_avail_min(pcm, snd_pcm_mmap_avail(pcm))) { /* check more precisely */ switch (snd_pcm_state(pcm)) { case SND_PCM_STATE_XRUN: @@ -6771,14 +6771,14 @@ snd_pcm_sframes_t snd_pcm_write_areas(snd_pcm_t *pcm, const snd_pcm_channel_area goto _end; } if ((state == SND_PCM_STATE_RUNNING && - (snd_pcm_uframes_t)avail < pcm->avail_min && - size > (snd_pcm_uframes_t)avail)) { + size > (snd_pcm_uframes_t)avail && + snd_pcm_may_wait_for_avail_min(pcm, avail))) { if (pcm->mode & SND_PCM_NONBLOCK) { err = -EAGAIN; goto _end; }
- err = snd_pcm_wait(pcm, -1); + err = snd_pcm_wait_nocheck(pcm, -1); if (err < 0) break; goto _again; diff --git a/src/pcm/pcm_generic.c b/src/pcm/pcm_generic.c index 0436439..6b26822 100644 --- a/src/pcm/pcm_generic.c +++ b/src/pcm/pcm_generic.c @@ -341,4 +341,10 @@ int snd_pcm_generic_set_chmap(snd_pcm_t *pcm, const snd_pcm_chmap_t *map) return snd_pcm_set_chmap(generic->slave, map); }
+int snd_pcm_generic_may_wait_for_avail_min(snd_pcm_t *pcm, snd_pcm_uframes_t avail) +{ + snd_pcm_generic_t *generic = pcm->private_data; + return snd_pcm_may_wait_for_avail_min(generic->slave, snd_pcm_mmap_avail(generic->slave)); +} + #endif /* DOC_HIDDEN */ diff --git a/src/pcm/pcm_generic.h b/src/pcm/pcm_generic.h index 916c6ec..f06f35f 100644 --- a/src/pcm/pcm_generic.h +++ b/src/pcm/pcm_generic.h @@ -109,6 +109,8 @@ typedef struct { snd1_pcm_generic_get_chmap #define snd_pcm_generic_set_chmap \ snd1_pcm_generic_set_chmap +#define snd_pcm_generic_may_wait_for_avail_min \ + snd1_pcm_generic_may_wait_for_avail_min
int snd_pcm_generic_close(snd_pcm_t *pcm); int snd_pcm_generic_nonblock(snd_pcm_t *pcm, int nonblock); @@ -158,5 +160,5 @@ int snd_pcm_generic_munmap(snd_pcm_t *pcm); snd_pcm_chmap_query_t **snd_pcm_generic_query_chmaps(snd_pcm_t *pcm); snd_pcm_chmap_t *snd_pcm_generic_get_chmap(snd_pcm_t *pcm); int snd_pcm_generic_set_chmap(snd_pcm_t *pcm, const snd_pcm_chmap_t *map); - +int snd_pcm_generic_may_wait_for_avail_min(snd_pcm_t *pcm, snd_pcm_uframes_t avail);
diff --git a/src/pcm/pcm_hooks.c b/src/pcm/pcm_hooks.c index 0feb4a3..8ea4d2a 100644 --- a/src/pcm/pcm_hooks.c +++ b/src/pcm/pcm_hooks.c @@ -199,6 +199,7 @@ static const snd_pcm_fast_ops_t snd_pcm_hooks_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, };
/** diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index e7798fd..830035e 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -177,6 +177,7 @@ typedef struct { int (*poll_descriptors_count)(snd_pcm_t *pcm); int (*poll_descriptors)(snd_pcm_t *pcm, struct pollfd *pfds, unsigned int space); int (*poll_revents)(snd_pcm_t *pcm, struct pollfd *pfds, unsigned int nfds, unsigned short *revents); + int (*may_wait_for_avail_min)(snd_pcm_t *pcm, snd_pcm_uframes_t avail); } snd_pcm_fast_ops_t;
struct _snd_pcm { @@ -984,3 +985,12 @@ _snd_pcm_parse_config_chmaps(snd_config_t *conf); snd_pcm_chmap_t * _snd_pcm_choose_fixed_chmap(snd_pcm_t *pcm, snd_pcm_chmap_query_t * const *maps);
+/* return true if the PCM stream may wait to get avail_min space */ +static inline int snd_pcm_may_wait_for_avail_min(snd_pcm_t *pcm, snd_pcm_uframes_t avail) +{ + if (pcm->fast_ops->may_wait_for_avail_min) + return pcm->fast_ops->may_wait_for_avail_min(pcm, avail); + else + return avail < pcm->avail_min; +} + diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c index d88e117..f228f3b 100644 --- a/src/pcm/pcm_plugin.c +++ b/src/pcm/pcm_plugin.c @@ -570,6 +570,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, };
#endif diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c index 4ba8521..f0bf218 100644 --- a/src/pcm/pcm_rate.c +++ b/src/pcm/pcm_rate.c @@ -1234,6 +1234,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, };
static const snd_pcm_ops_t snd_pcm_rate_ops = {