All of lore.kernel.org
 help / color / mirror / Atom feed
From: Takashi Iwai <tiwai@suse.de>
To: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Cc: linux-kernel@vger.kernel.org, "Jaroslav Kysela" <perex@perex.cz>,
	"Takashi Iwai" <tiwai@suse.com>,
	"Cássio Gabriel" <cassiogabrielcontato@gmail.com>,
	linux-sound@vger.kernel.org
Subject: Re: [PATCH] ALSA: aloop: Add 'hrtimer' option to timer_source
Date: Fri, 03 Jul 2026 13:12:03 +0200	[thread overview]
Message-ID: <87se6072f0.wl-tiwai@suse.de> (raw)
In-Reply-To: <20260702170356.3418432-1-yuhsuan@chromium.org>

On Thu, 02 Jul 2026 19:03:48 +0200,
Yu-Hsuan Hsu wrote:
> 
> The snd-aloop driver currently defaults to using the system jiffies
> timer (struct timer_list). On systems configured with a low timer
> interrupt frequency (e.g., CONFIG_HZ=250), the jiffies resolution
> (4ms per tick) is insufficient for precise audio timing. For
> example, a 10ms audio period requires 2.5 jiffies ticks, causing
> timing jitter that leads to capture underruns.
> 
> Introduce "hrtimer" as a supported timer_source option. When
> timer_source="hrtimer" is set, aloop uses high-resolution timers
> (hrtimer) to drive period updates. This provides nanosecond-level
> accuracy regardless of CONFIG_HZ and operates independently of other
> hardware audio cards.
> 
> Signed-off-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>

I guess the code depends on CONFIG_HIGH_RES_TIMERS, so we'd need
either an additional Kconfig dependency or some ifdefs to build
properly.

Other than that, I find the idea fine.


thanks,

Takashi

> ---
>  sound/drivers/aloop.c | 101 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 98 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c
> index 06bfe09eae1a..35018c33a92b 100644
> --- a/sound/drivers/aloop.c
> +++ b/sound/drivers/aloop.c
> @@ -23,6 +23,7 @@
>  #include <linux/wait.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/hrtimer.h>
>  #include <sound/core.h>
>  #include <sound/control.h>
>  #include <sound/pcm.h>
> @@ -55,7 +56,7 @@ MODULE_PARM_DESC(pcm_substreams, "PCM substreams # (1-8) for loopback driver.");
>  module_param_array(pcm_notify, int, NULL, 0444);
>  MODULE_PARM_DESC(pcm_notify, "Break capture when PCM format/rate/channels changes.");
>  module_param_array(timer_source, charp, NULL, 0444);
> -MODULE_PARM_DESC(timer_source, "Sound card name or number and device/subdevice number of timer to be used. Empty string for jiffies timer [default].");
> +MODULE_PARM_DESC(timer_source, "Sound card name or number and device/subdevice number of timer to be used. Empty string for jiffies timer [default], 'hrtimer' for high-resolution timer.");
>  
>  #define NO_PITCH 100000
>  
> @@ -161,8 +162,9 @@ struct loopback_pcm {
>  	unsigned int period_size_frac;	/* period size in jiffies ticks */
>  	unsigned int last_drift;
>  	unsigned long last_jiffies;
> -	/* If jiffies timer is used */
> +	/* If jiffies / hrtimer is used */
>  	struct timer_list timer;
> +	struct hrtimer hrtimer;
>  
>  	/* size of per channel buffer in case of non-interleaved access */
>  	unsigned int channel_buf_n;
> @@ -232,6 +234,29 @@ static int loopback_jiffies_timer_start(struct loopback_pcm *dpcm)
>  	return 0;
>  }
>  
> +/* call in cable->lock */
> +static int loopback_hrtimer_start(struct loopback_pcm *dpcm)
> +{
> +	unsigned long tick;
> +	unsigned int rate_shift = get_rate_shift(dpcm);
> +
> +	if (rate_shift != dpcm->pcm_rate_shift) {
> +		dpcm->pcm_rate_shift = rate_shift;
> +		dpcm->period_size_frac = frac_pos(dpcm, dpcm->pcm_period_size);
> +	}
> +	if (dpcm->period_size_frac <= dpcm->irq_pos) {
> +		dpcm->irq_pos %= dpcm->period_size_frac;
> +		dpcm->period_update_pending = 1;
> +	}
> +	tick = dpcm->period_size_frac - dpcm->irq_pos;
> +	tick = DIV_ROUND_UP(tick, dpcm->pcm_bps);
> +	hrtimer_start(&dpcm->hrtimer,
> +		      ns_to_ktime(div_u64((u64)tick * NSEC_PER_SEC, HZ)),
> +		      HRTIMER_MODE_REL_SOFT);
> +
> +	return 0;
> +}
> +
>  /* call in cable->lock */
>  static int loopback_snd_timer_start(struct loopback_pcm *dpcm)
>  {
> @@ -270,6 +295,14 @@ static inline int loopback_jiffies_timer_stop(struct loopback_pcm *dpcm)
>  	return 0;
>  }
>  
> +/* call in cable->lock */
> +static inline int loopback_hrtimer_stop(struct loopback_pcm *dpcm)
> +{
> +	hrtimer_cancel(&dpcm->hrtimer);
> +
> +	return 0;
> +}
> +
>  /* call in cable->lock */
>  static int loopback_snd_timer_stop(struct loopback_pcm *dpcm)
>  {
> @@ -300,6 +333,13 @@ static inline int loopback_jiffies_timer_stop_sync(struct loopback_pcm *dpcm)
>  	return 0;
>  }
>  
> +static inline int loopback_hrtimer_stop_sync(struct loopback_pcm *dpcm)
> +{
> +	hrtimer_cancel(&dpcm->hrtimer);
> +
> +	return 0;
> +}
> +
>  /* call in loopback->cable_lock */
>  static int loopback_snd_timer_close_cable(struct loopback_pcm *dpcm)
>  {
> @@ -734,6 +774,28 @@ static void loopback_jiffies_timer_function(struct timer_list *t)
>  		snd_pcm_period_elapsed(dpcm->substream);
>  }
>  
> +static enum hrtimer_restart loopback_hrtimer_function(struct hrtimer *t)
> +{
> +	struct loopback_pcm *dpcm = container_of(t, struct loopback_pcm, hrtimer);
> +	bool period_elapsed = false;
> +
> +	scoped_guard(spinlock_irqsave, &dpcm->cable->lock) {
> +		if (loopback_jiffies_timer_pos_update(dpcm->cable) &
> +		    (1 << dpcm->substream->stream)) {
> +			loopback_hrtimer_start(dpcm);
> +			if (dpcm->period_update_pending) {
> +				dpcm->period_update_pending = 0;
> +				period_elapsed = true;
> +			}
> +		}
> +	}
> +
> +	if (period_elapsed)
> +		snd_pcm_period_elapsed(dpcm->substream);
> +
> +	return HRTIMER_NORESTART;
> +}
> +
>  /* call in cable->lock */
>  static int loopback_snd_timer_check_resolution(struct snd_pcm_runtime *runtime,
>  					       unsigned long resolution)
> @@ -907,6 +969,19 @@ static void loopback_jiffies_timer_dpcm_info(struct loopback_pcm *dpcm,
>  	snd_iprintf(buffer, "    timer_expires:\t%lu\n", dpcm->timer.expires);
>  }
>  
> +static void loopback_hrtimer_dpcm_info(struct loopback_pcm *dpcm,
> +				       struct snd_info_buffer *buffer)
> +{
> +	snd_iprintf(buffer, "    update_pending:\t%u\n",
> +		    dpcm->period_update_pending);
> +	snd_iprintf(buffer, "    irq_pos:\t\t%u\n", dpcm->irq_pos);
> +	snd_iprintf(buffer, "    period_frac:\t%u\n", dpcm->period_size_frac);
> +	snd_iprintf(buffer, "    last_jiffies:\t%lu (%lu)\n",
> +		    dpcm->last_jiffies, jiffies);
> +	snd_iprintf(buffer, "    timer_expires:\t%llu\n",
> +		    ktime_to_ns(hrtimer_get_expires(&dpcm->hrtimer)));
> +}
> +
>  static void loopback_snd_timer_dpcm_info(struct loopback_pcm *dpcm,
>  					 struct snd_info_buffer *buffer)
>  {
> @@ -1097,6 +1172,24 @@ static const struct loopback_ops loopback_jiffies_timer_ops = {
>  	.dpcm_info = loopback_jiffies_timer_dpcm_info,
>  };
>  
> +static int loopback_hrtimer_open(struct loopback_pcm *dpcm)
> +{
> +	hrtimer_setup(&dpcm->hrtimer, loopback_hrtimer_function,
> +		      CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
> +
> +	return 0;
> +}
> +
> +static const struct loopback_ops loopback_hrtimer_ops = {
> +	.open = loopback_hrtimer_open,
> +	.start = loopback_hrtimer_start,
> +	.stop = loopback_hrtimer_stop,
> +	.stop_sync = loopback_hrtimer_stop_sync,
> +	.close_substream = loopback_hrtimer_stop_sync,
> +	.pos_update = loopback_jiffies_timer_pos_update,
> +	.dpcm_info = loopback_hrtimer_dpcm_info,
> +};
> +
>  static int loopback_parse_timer_id(const char *str,
>  				   struct snd_timer_id *tid)
>  {
> @@ -1274,7 +1367,9 @@ static int loopback_open(struct snd_pcm_substream *substream)
>  		spin_lock_init(&cable->lock);
>  		snd_refcount_init(&cable->stop_count);
>  		cable->hw = loopback_pcm_hardware;
> -		if (loopback->timer_source)
> +		if (loopback->timer_source && !strcmp(loopback->timer_source, "hrtimer"))
> +			cable->ops = &loopback_hrtimer_ops;
> +		else if (loopback->timer_source && loopback->timer_source[0])
>  			cable->ops = &loopback_snd_timer_ops;
>  		else
>  			cable->ops = &loopback_jiffies_timer_ops;
> -- 
> 2.55.0.rc0.799.gd6f94ed593-goog
> 

  reply	other threads:[~2026-07-03 11:12 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-02 17:03 [PATCH] ALSA: aloop: Add 'hrtimer' option to timer_source Yu-Hsuan Hsu
2026-07-03 11:12 ` Takashi Iwai [this message]
2026-07-05 12:59 ` [PATCH v2] " Yu-Hsuan Hsu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87se6072f0.wl-tiwai@suse.de \
    --to=tiwai@suse.de \
    --cc=cassiogabrielcontato@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sound@vger.kernel.org \
    --cc=perex@perex.cz \
    --cc=tiwai@suse.com \
    --cc=yuhsuan@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.