[pulseaudio-discuss] [RFC PATCH] alsa-sink: Reduce hardware pointer update syscalls
David Henningsson
david.henningsson at canonical.com
Tue Sep 23 23:17:06 PDT 2014
On 2014-09-23 22:34, Pierre-Louis Bossart wrote:
> On 9/18/14, 2:13 AM, David Henningsson wrote:
>> Calling snd_pcm_avail/delay causes a syscall to the kernel, which
>> communicates with the audio hardware, and can therefore be expensive
>> on some cards.
>>
>> By only updating this value after a sleep and after unusual events,
>> we can reduce calls to update the hardware pointer.
>> In particular, if a write goes well, we will now assume that the
>> buffer has been filled up, instead of re-asking the hardware that
>> this is actually the case.
>
> You can reduce the number of syscall by using snd_pcm_status, which
> provides all the information at once. I thought I contributed a patch
> for this long back.
I believe that patch is in - pa_alsa_safe_delay uses snd_pcm_status
internally. But there are other calls as well, one to pa_alsa_safe_avail
and the other one just to get a timestamp.
> Making assumptions on how the write went is problematic in my opinion,
> and there can be a case where you want to write more data while awake if
> there is enough room. Maybe this ought to be split in two parts for
> testing/bisecting.
When the code decides to fill up, it's always less than half in the
buffer and it fills it completely up.
If filling up the buffer itself takes so long time that the buffer
becomes more than half empty, then we have serious issues anyway (and in
extension, the risk of us being killed for spending too much time in RT
context seems pretty high).
Or, if I'm misunderstanding you, could you give a real world example
where it would be important to continue writing even though the buffer
was successfully filled up?
>
>> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
>> ---
>> src/modules/alsa/alsa-sink.c | 92
>> +++++++++++++++++++++++++++++---------------
>> 1 file changed, 62 insertions(+), 30 deletions(-)
>>
>> Hi,
>>
>> This patch is not really finished, but I'd like you to test it and see
>> if you have any performance improvements, before I go ahead and try to
>> make
>> it more robust.
>> When I tested it, I didn't see much performance gain, at least not
>> consistently.
>> But maybe this is because the sound cards I tested with have quick hw
>> pointer updates.
>>
>> diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
>> index 9e9b863..5330c78 100644
>> --- a/src/modules/alsa/alsa-sink.c
>> +++ b/src/modules/alsa/alsa-sink.c
>> @@ -155,6 +155,11 @@ struct userdata {
>> pa_reserve_monitor_wrapper *monitor;
>> pa_hook_slot *monitor_slot;
>>
>> + snd_pcm_status_t *pcm_status;
>> + snd_pcm_sframes_t pcm_status_delay;
>> + snd_pcm_sframes_t pcm_status_avail;
>> + bool pcm_status_valid;
>> +
>> /* ucm context */
>> pa_alsa_ucm_mapping_context *ucm_context;
>> };
>> @@ -440,6 +445,7 @@ static int try_recover(struct userdata *u, const
>> char *call, int err) {
>> if (err == -ESTRPIPE)
>> pa_log_debug("%s: System suspended!", call);
>>
>> + u->pcm_status_valid = false;
>> if ((err = snd_pcm_recover(u->pcm_handle, err, 1)) < 0) {
>> pa_log("%s: %s", call, pa_alsa_strerror(err));
>> return -1;
>> @@ -450,6 +456,29 @@ static int try_recover(struct userdata *u, const
>> char *call, int err) {
>> return 0;
>> }
>>
>> +static bool ensure_pcm_status(struct userdata *u) {
>> + int err;
>> +
>> + if (u->pcm_status_valid)
>> + return true;
>> +#ifdef DEBUG_TIMING
>> + pa_log_debug("Calling snd_pcm_status (hwsync)");
>> +#endif
>> + if (PA_UNLIKELY((err = pa_alsa_safe_delay(u->pcm_handle,
>> u->pcm_status, &u->pcm_status_delay, u->hwbuf_size,
>> &u->sink->sample_spec, false)) < 0)) {
>> + pa_log_warn("Failed to retrieve current pcm status: %s",
>> pa_alsa_strerror(err));
>> + if (try_recover(u, "snd_pcm_status", err) < 0)
>> + return false;
>> + if ((err = pa_alsa_safe_delay(u->pcm_handle, u->pcm_status,
>> &u->pcm_status_delay, u->hwbuf_size, &u->sink->sample_spec, false)) <
>> 0) {
>> + pa_log_warn("Failed to re-retrieve current pcm status:
>> %s", pa_alsa_strerror(err));
>> + return false;
>> + }
>> + }
>> +
>> + u->pcm_status_avail = snd_pcm_status_get_avail(u->pcm_status); /*
>> FIXME: This should be checked for weird values, just like delay */
>> + u->pcm_status_valid = true;
>> + return true;
>> +}
>> +
>> static size_t check_left_to_play(struct userdata *u, size_t n_bytes,
>> bool on_timeout) {
>> size_t left_to_play;
>> bool underrun = false;
>> @@ -521,23 +550,16 @@ static int mmap_write(struct userdata *u,
>> pa_usec_t *sleep_usec, bool polled, bo
>> hw_sleep_time(u, &max_sleep_usec, &process_usec);
>>
>> for (;;) {
>> - snd_pcm_sframes_t n;
>> size_t n_bytes;
>> int r;
>> bool after_avail = true;
>>
>> /* First we determine how many samples are missing to fill the
>> * buffer up to 100% */
>> + if (PA_UNLIKELY(!ensure_pcm_status(u)))
>> + return -1;
>>
>> - if (PA_UNLIKELY((n = pa_alsa_safe_avail(u->pcm_handle,
>> u->hwbuf_size, &u->sink->sample_spec)) < 0)) {
>> -
>> - if ((r = try_recover(u, "snd_pcm_avail", (int) n)) == 0)
>> - continue;
>> -
>> - return r;
>> - }
>> -
>> - n_bytes = (size_t) n * u->frame_size;
>> + n_bytes = (size_t) u->pcm_status_avail * u->frame_size;
>>
>> #ifdef DEBUG_TIMING
>> pa_log_debug("avail: %lu", (unsigned long) n_bytes);
>> @@ -609,6 +631,7 @@ static int mmap_write(struct userdata *u,
>> pa_usec_t *sleep_usec, bool polled, bo
>>
>> if (PA_UNLIKELY((err =
>> pa_alsa_safe_mmap_begin(u->pcm_handle, &areas, &offset, &frames,
>> u->hwbuf_size, &u->sink->sample_spec)) < 0)) {
>>
>> + u->pcm_status_valid = false;
>> if (!after_avail && err == -EAGAIN)
>> break;
>>
>> @@ -647,6 +670,7 @@ static int mmap_write(struct userdata *u,
>> pa_usec_t *sleep_usec, bool polled, bo
>> pa_memblock_unref_fixed(chunk.memblock);
>>
>> if (PA_UNLIKELY((sframes =
>> snd_pcm_mmap_commit(u->pcm_handle, offset, frames)) < 0)) {
>> + u->pcm_status_valid = false;
>>
>> if (!after_avail && (int) sframes == -EAGAIN)
>> break;
>> @@ -661,6 +685,8 @@ static int mmap_write(struct userdata *u,
>> pa_usec_t *sleep_usec, bool polled, bo
>>
>> u->write_count += written;
>> u->since_start += written;
>> + u->pcm_status_avail -= sframes;
>> + u->pcm_status_delay += sframes;
>>
>> #ifdef DEBUG_TIMING
>> pa_log_debug("Wrote %lu bytes (of possible %lu bytes)",
>> (unsigned long) written, (unsigned long) n_bytes);
>> @@ -706,20 +732,14 @@ static int unix_write(struct userdata *u,
>> pa_usec_t *sleep_usec, bool polled, bo
>> hw_sleep_time(u, &max_sleep_usec, &process_usec);
>>
>> for (;;) {
>> - snd_pcm_sframes_t n;
>> size_t n_bytes;
>> int r;
>> bool after_avail = true;
>>
>> - if (PA_UNLIKELY((n = pa_alsa_safe_avail(u->pcm_handle,
>> u->hwbuf_size, &u->sink->sample_spec)) < 0)) {
>> + if (PA_UNLIKELY(!ensure_pcm_status(u)))
>> + return -1;
>>
>> - if ((r = try_recover(u, "snd_pcm_avail", (int) n)) == 0)
>> - continue;
>> -
>> - return r;
>> - }
>> -
>> - n_bytes = (size_t) n * u->frame_size;
>> + n_bytes = (size_t) u->pcm_status_avail * u->frame_size;
>>
>> #ifdef DEBUG_TIMING
>> pa_log_debug("avail: %lu", (unsigned long) n_bytes);
>> @@ -789,6 +809,7 @@ static int unix_write(struct userdata *u,
>> pa_usec_t *sleep_usec, bool polled, bo
>>
>> if (PA_UNLIKELY(frames < 0)) {
>>
>> + u->pcm_status_valid = false;
>> if (!after_avail && (int) frames == -EAGAIN)
>> break;
>>
>> @@ -815,6 +836,8 @@ static int unix_write(struct userdata *u,
>> pa_usec_t *sleep_usec, bool polled, bo
>>
>> work_done = true;
>>
>> + u->pcm_status_avail -= frames;
>> + u->pcm_status_delay += frames;
>> u->write_count += written;
>> u->since_start += written;
>>
>> @@ -848,26 +871,19 @@ static int unix_write(struct userdata *u,
>> pa_usec_t *sleep_usec, bool polled, bo
>> }
>>
>> static void update_smoother(struct userdata *u) {
>> - snd_pcm_sframes_t delay = 0;
>> int64_t position;
>> - int err;
>> pa_usec_t now1 = 0, now2;
>> - snd_pcm_status_t *status;
>> snd_htimestamp_t htstamp = { 0, 0 };
>>
>> - snd_pcm_status_alloca(&status);
>> -
>> pa_assert(u);
>> pa_assert(u->pcm_handle);
>> -
>> /* Let's update the time smoother */
>> -
>> - if (PA_UNLIKELY((err = pa_alsa_safe_delay(u->pcm_handle, status,
>> &delay, u->hwbuf_size, &u->sink->sample_spec, false)) < 0)) {
>> - pa_log_warn("Failed to query DSP status data: %s",
>> pa_alsa_strerror(err));
>> + if (!ensure_pcm_status(u)) {
>> + pa_log_warn("Failed to query DSP status data");
>> return;
>> }
>>
>> - snd_pcm_status_get_htstamp(status, &htstamp);
>> + snd_pcm_status_get_htstamp(u->pcm_status, &htstamp);
>> now1 = pa_timespec_load(&htstamp);
>>
>> /* Hmm, if the timestamp is 0, then it wasn't set and we take
>> the current time */
>> @@ -879,7 +895,7 @@ static void update_smoother(struct userdata *u) {
>> if (u->last_smoother_update + u->smoother_interval > now1)
>> return;
>>
>> - position = (int64_t) u->write_count - ((int64_t) delay *
>> (int64_t) u->frame_size);
>> + position = (int64_t) u->write_count - ((int64_t)
>> u->pcm_status_delay * (int64_t) u->frame_size);
>>
>> if (PA_UNLIKELY(position < 0))
>> position = 0;
>> @@ -935,6 +951,7 @@ static int suspend(struct userdata *u) {
>>
>> /* Let's suspend -- we don't call snd_pcm_drain() here since
>> that might
>> * take awfully long with our long buffer sizes today. */
>> + u->pcm_status_valid = false;
>> snd_pcm_close(u->pcm_handle);
>> u->pcm_handle = NULL;
>>
>> @@ -1002,6 +1019,7 @@ static int update_sw_params(struct userdata *u) {
>>
>> pa_log_debug("setting avail_min=%lu", (unsigned long) avail_min);
>>
>> + u->pcm_status_valid = false;
>> if ((err = pa_alsa_set_sw_params(u->pcm_handle, avail_min,
>> !u->use_tsched)) < 0) {
>> pa_log("Failed to set software parameters: %s",
>> pa_alsa_strerror(err));
>> return err;
>> @@ -1073,6 +1091,7 @@ static int unsuspend(struct userdata *u) {
>> pa_snprintf(device_name, len, "%s,AES0=6", u->device_name);
>> }
>>
>> + u->pcm_status_valid = false;
>> if ((err = snd_pcm_open(&u->pcm_handle, device_name ?
>> device_name : u->device_name, SND_PCM_STREAM_PLAYBACK,
>> SND_PCM_NONBLOCK|
>> SND_PCM_NO_AUTO_RESAMPLE|
>> @@ -1136,6 +1155,7 @@ static int unsuspend(struct userdata *u) {
>>
>> fail:
>> if (u->pcm_handle) {
>> + u->pcm_status_valid = false;
>> snd_pcm_close(u->pcm_handle);
>> u->pcm_handle = NULL;
>> }
>> @@ -1653,6 +1673,7 @@ static int process_rewind(struct userdata *u) {
>>
>> in_frames = (snd_pcm_sframes_t) (rewind_nbytes /
>> u->frame_size);
>> pa_log_debug("before: %lu", (unsigned long) in_frames);
>> + u->pcm_status_valid = false;
>> if ((out_frames = snd_pcm_rewind(u->pcm_handle,
>> (snd_pcm_uframes_t) in_frames)) < 0) {
>> pa_log("snd_pcm_rewind() failed: %s",
>> pa_alsa_strerror((int) out_frames));
>> if (try_recover(u, "process_rewind", out_frames) < 0)
>> @@ -1727,6 +1748,7 @@ static void thread_func(void *userdata) {
>>
>> if (u->first) {
>> pa_log_info("Starting playback.");
>> + u->pcm_status_valid = false;
>> snd_pcm_start(u->pcm_handle);
>>
>> pa_smoother_resume(u->smoother,
>> pa_rtclock_now(), true);
>> @@ -1811,6 +1833,7 @@ static void thread_func(void *userdata) {
>> (double) (real_sleep - rtpoll_sleep) /
>> PA_USEC_PER_MSEC,
>> (double) (u->tsched_watermark_usec) /
>> PA_USEC_PER_MSEC);
>> }
>> + u->pcm_status_valid = false;
>>
>> if (u->sink->flags & PA_SINK_DEFERRED_VOLUME)
>> pa_sink_volume_change_apply(u->sink, NULL);
>> @@ -1827,11 +1850,14 @@ static void thread_func(void *userdata) {
>> pollfd = pa_rtpoll_item_get_pollfd(u->alsa_rtpoll_item,
>> &n);
>>
>> if ((err =
>> snd_pcm_poll_descriptors_revents(u->pcm_handle, pollfd, n, &revents))
>> < 0) {
>> + u->pcm_status_valid = false;
>> pa_log("snd_pcm_poll_descriptors_revents() failed:
>> %s", pa_alsa_strerror(err));
>> goto fail;
>> }
>>
>> if (revents & ~POLLOUT) {
>> + u->pcm_status_valid = false;
>> +
>> if (pa_alsa_recover_from_poll(u->pcm_handle,
>> revents) < 0)
>> goto fail;
>>
>> @@ -2141,6 +2167,9 @@ pa_sink *pa_alsa_sink_new(pa_module *m,
>> pa_modargs *ma, const char*driver, pa_ca
>> if (reserve_monitor_init(u, dev_id) < 0)
>> goto fail;
>>
>> + if (snd_pcm_status_malloc(&u->pcm_status) < 0)
>> + goto fail;
>> +
>> b = use_mmap;
>> d = use_tsched;
>>
>> @@ -2465,6 +2494,9 @@ static void userdata_free(struct userdata *u) {
>> snd_pcm_close(u->pcm_handle);
>> }
>>
>> + if (u->pcm_status)
>> + snd_pcm_status_free(u->pcm_status);
>> +
>> if (u->mixer_fdl)
>> pa_alsa_fdlist_free(u->mixer_fdl);
>>
>>
>
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
More information about the pulseaudio-discuss
mailing list