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