[PATCH] ALSA: add support for disabling period irq

[PATCH] ALSA: add support for disabling period irq

[Pierre-Louis Bossart]

Merged and cleaned patch based on earlier patches posted
on alsa-devel by Clemens Ladisch <clemens@ladisch.de> and
Pierre-Louis Bossart <pierre-louis.bossart@intel.com>

This patch disables period interrupts which are not
needed when the application relies on a system timer
to wake-up and refill the ring buffer. The behavior of
the driver is left unchanged, and interrupts are only
disabled if the application requests this configuration.
The behavior in case of underruns is slightly different,
instead of being detected during the period interrupts the
underruns are detected when the application calls
snd_pcm_update_avail, which in turns forces a refresh of the
hw pointer and shows the buffer is empty.

More specifically this patch makes a lot of sense when
PulseAudio relies on timer-based scheduling to access audio
devices such as HDAudio or Intel SST. Disabling interrupts
removes two unwanted wake-ups due to period elapsed events
in low-power playback modes. It also simplifies PulseAudio
voice modules used for speech calls.

To quote Lennart "This patch looks very interesting and
desirable. This is something have long been waiting for."

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
---
 core/pcm_lib.c          |    8 ++++++--
 core/pcm_native.c       |    6 ++++++
 include/asound.h        |    4 ++++
 include/pcm.h           |    1 +
 pci/hda/hda_intel.c     |    9 ++++++---
 pci/oxygen/oxygen_pcm.c |   14 ++++++++++----
 6 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/core/pcm_lib.c b/core/pcm_lib.c
index a1707cc..4963d8f 100644
--- a/core/pcm_lib.c
+++ b/core/pcm_lib.c
@@ -374,7 +374,8 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 			   (unsigned long)runtime->hw_ptr_base);
 	}
 	/* something must be really wrong */
-	if (delta >= runtime->buffer_size + runtime->period_size) {
+	if (delta >= runtime->buffer_size + runtime->period_size &&
+            !runtime->no_period_irq) {
 		hw_ptr_error(substream,
 			       "Unexpected hw_pointer value %s"
 			       "(stream=%i, pos=%ld, new_hw_ptr=%ld, "
@@ -395,6 +396,8 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 	 */
 	if (runtime->hw.info & SNDRV_PCM_INFO_BATCH)
 		goto no_jiffies_check;
+        if (runtime->no_period_irq)
+                goto no_jiffies_check;
 	hdelta = delta;
 	if (hdelta < runtime->delay)
 		goto no_jiffies_check;
@@ -431,7 +434,8 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 		hw_base = new_hw_ptr - (new_hw_ptr % runtime->buffer_size);
 	}
  no_jiffies_check:
-	if (delta > runtime->period_size + runtime->period_size / 2) {
+	if (delta > runtime->period_size + runtime->period_size / 2 &&
+            !runtime->no_period_irq) {
 		hw_ptr_error(substream,
 			     "Lost interrupts? %s"
 			     "(stream=%i, delta=%ld, new_hw_ptr=%ld, "
diff --git a/core/pcm_native.c b/core/pcm_native.c
index 8bc7cb3..92c8c59 100644
--- a/core/pcm_native.c
+++ b/core/pcm_native.c
@@ -412,6 +412,12 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
 			goto _error;
 	}
 
+        if (params->info & SNDRV_PCM_INFO_NO_PERIOD_IRQ) 
+                runtime->no_period_irq = !!(params->flags &
+                                            SNDRV_PCM_HW_PARAMS_NO_PERIOD_IRQ);
+        else
+                runtime->no_period_irq = 0;
+        
 	runtime->access = params_access(params);
 	runtime->format = params_format(params);
 	runtime->subformat = params_subformat(params);
diff --git a/include/asound.h b/include/asound.h
index a1803ec..d4b5f2d 100644
--- a/include/asound.h
+++ b/include/asound.h
@@ -259,6 +259,7 @@ typedef int __bitwise snd_pcm_subformat_t;
 #define SNDRV_PCM_INFO_HALF_DUPLEX	0x00100000	/* only half duplex */
 #define SNDRV_PCM_INFO_JOINT_DUPLEX	0x00200000	/* playback and capture stream are somewhat correlated */
 #define SNDRV_PCM_INFO_SYNC_START	0x00400000	/* pcm support some kind of sync go */
+#define SNDRV_PCM_INFO_NO_PERIOD_IRQ    0x00800000      /* period interrupt can be disabled */
 #define SNDRV_PCM_INFO_FIFO_IN_FRAMES	0x80000000	/* internal kernel flag - FIFO size is in frames */
 
 typedef int __bitwise snd_pcm_state_t;
@@ -334,6 +335,9 @@ typedef int snd_pcm_hw_param_t;
 #define	SNDRV_PCM_HW_PARAM_LAST_INTERVAL	SNDRV_PCM_HW_PARAM_TICK_TIME
 
 #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)	/* avoid rate resampling */
+#define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER      (1<<1)  /* export buffer */
+#define SNDRV_PCM_HW_PARAMS_NO_PERIOD_IRQ      (1<<2)  /* disable period
+                                                          interrupts */
 
 struct snd_interval {
 	unsigned int min, max;
diff --git a/include/pcm.h b/include/pcm.h
index dfd9b76..9abb4aa 100644
--- a/include/pcm.h
+++ b/include/pcm.h
@@ -297,6 +297,7 @@ struct snd_pcm_runtime {
 	unsigned int info;
 	unsigned int rate_num;
 	unsigned int rate_den;
+        bool no_period_irq;
 
 	/* -- SW params -- */
 	int tstamp_mode;		/* mmap timestamp is updated */
diff --git a/pci/hda/hda_intel.c b/pci/hda/hda_intel.c
index 38b063e..9f456a8 100644
--- a/pci/hda/hda_intel.c
+++ b/pci/hda/hda_intel.c
@@ -1227,7 +1227,8 @@ static int azx_setup_periods(struct azx *chip,
 			pos_adj = 0;
 		} else {
 			ofs = setup_bdle(substream, azx_dev,
-					 &bdl, ofs, pos_adj, 1);
+					 &bdl, ofs, pos_adj,
+                                         !substream->runtime->no_period_irq);
 			if (ofs < 0)
 				goto error;
 		}
@@ -1239,7 +1240,8 @@ static int azx_setup_periods(struct azx *chip,
 					 period_bytes - pos_adj, 0);
 		else
 			ofs = setup_bdle(substream, azx_dev, &bdl, ofs,
-					 period_bytes, 1);
+					 period_bytes,
+                                         !substream->runtime->no_period_irq);
 		if (ofs < 0)
 			goto error;
 	}
@@ -1507,7 +1509,8 @@ static struct snd_pcm_hardware azx_pcm_hw = {
 				 /* No full-resume yet implemented */
 				 /* SNDRV_PCM_INFO_RESUME |*/
 				 SNDRV_PCM_INFO_PAUSE |
-				 SNDRV_PCM_INFO_SYNC_START),
+				 SNDRV_PCM_INFO_SYNC_START |
+                                 SNDRV_PCM_INFO_NO_PERIOD_IRQ),
 	.formats =		SNDRV_PCM_FMTBIT_S16_LE,
 	.rates =		SNDRV_PCM_RATE_48000,
 	.rate_min =		48000,
diff --git a/pci/oxygen/oxygen_pcm.c b/pci/oxygen/oxygen_pcm.c
index 8146674..a32ee97 100644
--- a/pci/oxygen/oxygen_pcm.c
+++ b/pci/oxygen/oxygen_pcm.c
@@ -39,7 +39,8 @@ static const struct snd_pcm_hardware oxygen_stereo_hardware = {
 		SNDRV_PCM_INFO_MMAP_VALID |
 		SNDRV_PCM_INFO_INTERLEAVED |
 		SNDRV_PCM_INFO_PAUSE |
-		SNDRV_PCM_INFO_SYNC_START,
+		SNDRV_PCM_INFO_SYNC_START |
+                SNDRV_PCM_INFO_NO_PERIOD_IRQ,
 	.formats = SNDRV_PCM_FMTBIT_S16_LE |
 		   SNDRV_PCM_FMTBIT_S32_LE,
 	.rates = SNDRV_PCM_RATE_32000 |
@@ -65,7 +66,8 @@ static const struct snd_pcm_hardware oxygen_multichannel_hardware = {
 		SNDRV_PCM_INFO_MMAP_VALID |
 		SNDRV_PCM_INFO_INTERLEAVED |
 		SNDRV_PCM_INFO_PAUSE |
-		SNDRV_PCM_INFO_SYNC_START,
+		SNDRV_PCM_INFO_SYNC_START |
+                SNDRV_PCM_INFO_NO_PERIOD_IRQ,
 	.formats = SNDRV_PCM_FMTBIT_S16_LE |
 		   SNDRV_PCM_FMTBIT_S32_LE,
 	.rates = SNDRV_PCM_RATE_32000 |
@@ -91,7 +93,8 @@ static const struct snd_pcm_hardware oxygen_ac97_hardware = {
 		SNDRV_PCM_INFO_MMAP_VALID |
 		SNDRV_PCM_INFO_INTERLEAVED |
 		SNDRV_PCM_INFO_PAUSE |
-		SNDRV_PCM_INFO_SYNC_START,
+		SNDRV_PCM_INFO_SYNC_START |
+                SNDRV_PCM_INFO_NO_PERIOD_IRQ,
 	.formats = SNDRV_PCM_FMTBIT_S16_LE,
 	.rates = SNDRV_PCM_RATE_48000,
 	.rate_min = 48000,
@@ -530,7 +533,10 @@ static int oxygen_prepare(struct snd_pcm_substream *substream)
 	oxygen_set_bits8(chip, OXYGEN_DMA_FLUSH, channel_mask);
 	oxygen_clear_bits8(chip, OXYGEN_DMA_FLUSH, channel_mask);
 
-	chip->interrupt_mask |= channel_mask;
+        if (substream->runtime->no_period_irq)
+               chip->interrupt_mask &= ~channel_mask;
+        else
+                chip->interrupt_mask |= channel_mask;
 	oxygen_write16(chip, OXYGEN_INTERRUPT_MASK, chip->interrupt_mask);
 	spin_unlock_irq(&chip->reg_lock);
 	return 0;
-- 
1.7.2.3

Re: [PATCH] ALSA: add support for disabling period irq

[Jaroslav Kysela]

On Mon, 1 Nov 2010, Pierre-Louis Bossart wrote:

> Merged and cleaned patch based on earlier patches posted
> on alsa-devel by Clemens Ladisch <clemens@ladisch.de> and
> Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
>
> This patch disables period interrupts which are not
> needed when the application relies on a system timer
> to wake-up and refill the ring buffer. The behavior of
> the driver is left unchanged, and interrupts are only
> disabled if the application requests this configuration.
> The behavior in case of underruns is slightly different,
> instead of being detected during the period interrupts the
> underruns are detected when the application calls
> snd_pcm_update_avail, which in turns forces a refresh of the
> hw pointer and shows the buffer is empty.
>
> More specifically this patch makes a lot of sense when
> PulseAudio relies on timer-based scheduling to access audio
> devices such as HDAudio or Intel SST. Disabling interrupts
> removes two unwanted wake-ups due to period elapsed events
> in low-power playback modes. It also simplifies PulseAudio
> voice modules used for speech calls.
>
> To quote Lennart "This patch looks very interesting and
> desirable. This is something have long been waiting for."
>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
> ---
> core/pcm_lib.c          |    8 ++++++--
> core/pcm_native.c       |    6 ++++++
> include/asound.h        |    4 ++++
> include/pcm.h           |    1 +
> pci/hda/hda_intel.c     |    9 ++++++---
> pci/oxygen/oxygen_pcm.c |   14 ++++++++++----
> 6 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/core/pcm_lib.c b/core/pcm_lib.c
> index a1707cc..4963d8f 100644
> --- a/core/pcm_lib.c
> +++ b/core/pcm_lib.c
> @@ -374,7 +374,8 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
> 			   (unsigned long)runtime->hw_ptr_base);
> 	}
> 	/* something must be really wrong */
> -	if (delta >= runtime->buffer_size + runtime->period_size) {
> +	if (delta >= runtime->buffer_size + runtime->period_size &&
> +            !runtime->no_period_irq) {
> 		hw_ptr_error(substream,
> 			       "Unexpected hw_pointer value %s"
> 			       "(stream=%i, pos=%ld, new_hw_ptr=%ld, "
> @@ -395,6 +396,8 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
> 	 */
> 	if (runtime->hw.info & SNDRV_PCM_INFO_BATCH)
> 		goto no_jiffies_check;
> +        if (runtime->no_period_irq)
> +                goto no_jiffies_check;
> 	hdelta = delta;
> 	if (hdelta < runtime->delay)
> 		goto no_jiffies_check;
> @@ -431,7 +434,8 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
> 		hw_base = new_hw_ptr - (new_hw_ptr % runtime->buffer_size);
> 	}
>  no_jiffies_check:
> -	if (delta > runtime->period_size + runtime->period_size / 2) {
> +	if (delta > runtime->period_size + runtime->period_size / 2 &&
> +            !runtime->no_period_irq) {
> 		hw_ptr_error(substream,
> 			     "Lost interrupts? %s"
> 			     "(stream=%i, delta=%ld, new_hw_ptr=%ld, "

Use one 'if' and a goto behind the last condition to simplify things.

> diff --git a/core/pcm_native.c b/core/pcm_native.c
> index 8bc7cb3..92c8c59 100644
> --- a/core/pcm_native.c
> +++ b/core/pcm_native.c
> @@ -412,6 +412,12 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
> 			goto _error;
> 	}
>
> +        if (params->info & SNDRV_PCM_INFO_NO_PERIOD_IRQ)
> +                runtime->no_period_irq = !!(params->flags &
> +                                            SNDRV_PCM_HW_PARAMS_NO_PERIOD_IRQ);
> +        else
> +                runtime->no_period_irq = 0;
> +
> 	runtime->access = params_access(params);
> 	runtime->format = params_format(params);
> 	runtime->subformat = params_subformat(params);
> diff --git a/include/asound.h b/include/asound.h
> index a1803ec..d4b5f2d 100644
> --- a/include/asound.h
> +++ b/include/asound.h
> @@ -259,6 +259,7 @@ typedef int __bitwise snd_pcm_subformat_t;
> #define SNDRV_PCM_INFO_HALF_DUPLEX	0x00100000	/* only half duplex */
> #define SNDRV_PCM_INFO_JOINT_DUPLEX	0x00200000	/* playback and capture stream are somewhat correlated */
> #define SNDRV_PCM_INFO_SYNC_START	0x00400000	/* pcm support some kind of sync go */
> +#define SNDRV_PCM_INFO_NO_PERIOD_IRQ    0x00800000      /* period interrupt can be disabled */
> #define SNDRV_PCM_INFO_FIFO_IN_FRAMES	0x80000000	/* internal kernel flag - FIFO size is in frames */
>
> typedef int __bitwise snd_pcm_state_t;
> @@ -334,6 +335,9 @@ typedef int snd_pcm_hw_param_t;
> #define	SNDRV_PCM_HW_PARAM_LAST_INTERVAL	SNDRV_PCM_HW_PARAM_TICK_TIME
>
> #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)	/* avoid rate resampling */
> +#define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER      (1<<1)  /* export buffer */
> +#define SNDRV_PCM_HW_PARAMS_NO_PERIOD_IRQ      (1<<2)  /* disable period
> +                                                          interrupts */
>
> struct snd_interval {
> 	unsigned int min, max;

The PCM protocol version should be increased.

> diff --git a/include/pcm.h b/include/pcm.h
> index dfd9b76..9abb4aa 100644
> --- a/include/pcm.h
> +++ b/include/pcm.h
> @@ -297,6 +297,7 @@ struct snd_pcm_runtime {
> 	unsigned int info;
> 	unsigned int rate_num;
> 	unsigned int rate_den;
> +        bool no_period_irq;

I would use an unsigned int bit field rather than bool in this case to 
make addition in-line with other structures in this file.

 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: [PATCH] ALSA: add support for disabling period irq

[Raymond Yau]

2010/11/2 Pierre-Louis Bossart <bossart.nospam@gmail.com>

> Merged and cleaned patch based on earlier patches posted
> on alsa-devel by Clemens Ladisch <clemens@ladisch.de> and
> Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
>
> This patch disables period interrupts which are not
> needed when the application relies on a system timer
> to wake-up and refill the ring buffer. The behavior of
> the driver is left unchanged, and interrupts are only
> disabled if the application requests this configuration.
> The behavior in case of underruns is slightly different,
> instead of being detected during the period interrupts the
> underruns are detected when the application calls
> snd_pcm_update_avail, which in turns forces a refresh of the
> hw pointer and shows the buffer is empty.
>

But PA server set stop threshold to boundary, according to

http://www.alsa-project.org/alsa-doc/alsa-lib/group___p_c_m___s_w___params.html

PCM is automatically stopped in SND_PCM_STATE_XRUN state when available
frames is >= threshold. If the stop threshold is equal to boundary (also
software parameter - sw_param) then automatic stop will be disabled (thus
device will do the endless loop in the ring buffer).

The available buffer will not be empty (zero) but negative since device will
do the endless loop

Re: [PATCH] ALSA: add support for disabling period irq

[pl bossart]

Thanks Jaroslav for this quick review.

> Use one 'if' and a goto behind the last condition to simplify things.
will do

>> struct snd_interval {
>>        unsigned int min, max;
>
> The PCM protocol version should be increased.

Humm, this one I don't understand. I didn't change anything, just used
a bit in an existing flag? What are you specifically referring to?

>> @@ -297,6 +297,7 @@ struct snd_pcm_runtime {
>>        unsigned int info;
>>        unsigned int rate_num;
>>        unsigned int rate_den;
>> +        bool no_period_irq;
>
> I would use an unsigned int bit field rather than bool in this case to make
> addition in-line with other structures in this file.

will do.
-Pierre

Re: [PATCH] ALSA: add support for disabling period irq

[Jaroslav Kysela]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1229 bytes --]

On Mon, 1 Nov 2010, pl bossart wrote:

> Thanks Jaroslav for this quick review.
>
>> Use one 'if' and a goto behind the last condition to simplify things.
> will do
>
>>> struct snd_interval {
>>>        unsigned int min, max;
>>
>> The PCM protocol version should be increased.
>
> Humm, this one I don't understand. I didn't change anything, just used
> a bit in an existing flag? What are you specifically referring to?

Increase subminor in SNDRV_PCM_VERSION . It allows alsa-lib to check if 
the feature is implemented in the driver (although I admit that this 
change does not require special checks against the protocol version, but 
it's standard procedure to change the subminor when the API is extended).

>>> @@ -297,6 +297,7 @@ struct snd_pcm_runtime {
>>>        unsigned int info;
>>>        unsigned int rate_num;
>>>        unsigned int rate_den;
>>> +        bool no_period_irq;
>>
>> I would use an unsigned int bit field rather than bool in this case to make
>> addition in-line with other structures in this file.
>
> will do.

Thanks.

 					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ALSA: add support for disabling period irq

[Takashi Iwai]

At Mon,  1 Nov 2010 17:12:53 -0500,
Pierre-Louis Bossart wrote:
> 
> Merged and cleaned patch based on earlier patches posted
> on alsa-devel by Clemens Ladisch <clemens@ladisch.de> and
> Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
> 
> This patch disables period interrupts which are not
> needed when the application relies on a system timer
> to wake-up and refill the ring buffer. The behavior of
> the driver is left unchanged, and interrupts are only
> disabled if the application requests this configuration.
> The behavior in case of underruns is slightly different,
> instead of being detected during the period interrupts the
> underruns are detected when the application calls
> snd_pcm_update_avail, which in turns forces a refresh of the
> hw pointer and shows the buffer is empty.

So, this silently assumes that the applications do call
snd_pcm_update_avail() appropriately at the right timing?
If so, any sense to check XRUN in the driver at all...?

And, even more, any sense to report the incremental position by this
approach?  The only reliable information in this case is the offset in
the ring buffer.  The linear position as the current ALSA API provides
isn't guaranteed without the period irq.


Takashi

> More specifically this patch makes a lot of sense when
> PulseAudio relies on timer-based scheduling to access audio
> devices such as HDAudio or Intel SST. Disabling interrupts
> removes two unwanted wake-ups due to period elapsed events
> in low-power playback modes. It also simplifies PulseAudio
> voice modules used for speech calls.
> 
> To quote Lennart "This patch looks very interesting and
> desirable. This is something have long been waiting for."
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
> ---
>  core/pcm_lib.c          |    8 ++++++--
>  core/pcm_native.c       |    6 ++++++
>  include/asound.h        |    4 ++++
>  include/pcm.h           |    1 +
>  pci/hda/hda_intel.c     |    9 ++++++---
>  pci/oxygen/oxygen_pcm.c |   14 ++++++++++----
>  6 files changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/core/pcm_lib.c b/core/pcm_lib.c
> index a1707cc..4963d8f 100644
> --- a/core/pcm_lib.c
> +++ b/core/pcm_lib.c
> @@ -374,7 +374,8 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
>  			   (unsigned long)runtime->hw_ptr_base);
>  	}
>  	/* something must be really wrong */
> -	if (delta >= runtime->buffer_size + runtime->period_size) {
> +	if (delta >= runtime->buffer_size + runtime->period_size &&
> +            !runtime->no_period_irq) {
>  		hw_ptr_error(substream,
>  			       "Unexpected hw_pointer value %s"
>  			       "(stream=%i, pos=%ld, new_hw_ptr=%ld, "
> @@ -395,6 +396,8 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
>  	 */
>  	if (runtime->hw.info & SNDRV_PCM_INFO_BATCH)
>  		goto no_jiffies_check;
> +        if (runtime->no_period_irq)
> +                goto no_jiffies_check;
>  	hdelta = delta;
>  	if (hdelta < runtime->delay)
>  		goto no_jiffies_check;
> @@ -431,7 +434,8 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
>  		hw_base = new_hw_ptr - (new_hw_ptr % runtime->buffer_size);
>  	}
>   no_jiffies_check:
> -	if (delta > runtime->period_size + runtime->period_size / 2) {
> +	if (delta > runtime->period_size + runtime->period_size / 2 &&
> +            !runtime->no_period_irq) {
>  		hw_ptr_error(substream,
>  			     "Lost interrupts? %s"
>  			     "(stream=%i, delta=%ld, new_hw_ptr=%ld, "
> diff --git a/core/pcm_native.c b/core/pcm_native.c
> index 8bc7cb3..92c8c59 100644
> --- a/core/pcm_native.c
> +++ b/core/pcm_native.c
> @@ -412,6 +412,12 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
>  			goto _error;
>  	}
>  
> +        if (params->info & SNDRV_PCM_INFO_NO_PERIOD_IRQ) 
> +                runtime->no_period_irq = !!(params->flags &
> +                                            SNDRV_PCM_HW_PARAMS_NO_PERIOD_IRQ);
> +        else
> +                runtime->no_period_irq = 0;
> +        
>  	runtime->access = params_access(params);
>  	runtime->format = params_format(params);
>  	runtime->subformat = params_subformat(params);
> diff --git a/include/asound.h b/include/asound.h
> index a1803ec..d4b5f2d 100644
> --- a/include/asound.h
> +++ b/include/asound.h
> @@ -259,6 +259,7 @@ typedef int __bitwise snd_pcm_subformat_t;
>  #define SNDRV_PCM_INFO_HALF_DUPLEX	0x00100000	/* only half duplex */
>  #define SNDRV_PCM_INFO_JOINT_DUPLEX	0x00200000	/* playback and capture stream are somewhat correlated */
>  #define SNDRV_PCM_INFO_SYNC_START	0x00400000	/* pcm support some kind of sync go */
> +#define SNDRV_PCM_INFO_NO_PERIOD_IRQ    0x00800000      /* period interrupt can be disabled */
>  #define SNDRV_PCM_INFO_FIFO_IN_FRAMES	0x80000000	/* internal kernel flag - FIFO size is in frames */
>  
>  typedef int __bitwise snd_pcm_state_t;
> @@ -334,6 +335,9 @@ typedef int snd_pcm_hw_param_t;
>  #define	SNDRV_PCM_HW_PARAM_LAST_INTERVAL	SNDRV_PCM_HW_PARAM_TICK_TIME
>  
>  #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)	/* avoid rate resampling */
> +#define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER      (1<<1)  /* export buffer */
> +#define SNDRV_PCM_HW_PARAMS_NO_PERIOD_IRQ      (1<<2)  /* disable period
> +                                                          interrupts */
>  
>  struct snd_interval {
>  	unsigned int min, max;
> diff --git a/include/pcm.h b/include/pcm.h
> index dfd9b76..9abb4aa 100644
> --- a/include/pcm.h
> +++ b/include/pcm.h
> @@ -297,6 +297,7 @@ struct snd_pcm_runtime {
>  	unsigned int info;
>  	unsigned int rate_num;
>  	unsigned int rate_den;
> +        bool no_period_irq;
>  
>  	/* -- SW params -- */
>  	int tstamp_mode;		/* mmap timestamp is updated */
> diff --git a/pci/hda/hda_intel.c b/pci/hda/hda_intel.c
> index 38b063e..9f456a8 100644
> --- a/pci/hda/hda_intel.c
> +++ b/pci/hda/hda_intel.c
> @@ -1227,7 +1227,8 @@ static int azx_setup_periods(struct azx *chip,
>  			pos_adj = 0;
>  		} else {
>  			ofs = setup_bdle(substream, azx_dev,
> -					 &bdl, ofs, pos_adj, 1);
> +					 &bdl, ofs, pos_adj,
> +                                         !substream->runtime->no_period_irq);
>  			if (ofs < 0)
>  				goto error;
>  		}
> @@ -1239,7 +1240,8 @@ static int azx_setup_periods(struct azx *chip,
>  					 period_bytes - pos_adj, 0);
>  		else
>  			ofs = setup_bdle(substream, azx_dev, &bdl, ofs,
> -					 period_bytes, 1);
> +					 period_bytes,
> +                                         !substream->runtime->no_period_irq);
>  		if (ofs < 0)
>  			goto error;
>  	}
> @@ -1507,7 +1509,8 @@ static struct snd_pcm_hardware azx_pcm_hw = {
>  				 /* No full-resume yet implemented */
>  				 /* SNDRV_PCM_INFO_RESUME |*/
>  				 SNDRV_PCM_INFO_PAUSE |
> -				 SNDRV_PCM_INFO_SYNC_START),
> +				 SNDRV_PCM_INFO_SYNC_START |
> +                                 SNDRV_PCM_INFO_NO_PERIOD_IRQ),
>  	.formats =		SNDRV_PCM_FMTBIT_S16_LE,
>  	.rates =		SNDRV_PCM_RATE_48000,
>  	.rate_min =		48000,
> diff --git a/pci/oxygen/oxygen_pcm.c b/pci/oxygen/oxygen_pcm.c
> index 8146674..a32ee97 100644
> --- a/pci/oxygen/oxygen_pcm.c
> +++ b/pci/oxygen/oxygen_pcm.c
> @@ -39,7 +39,8 @@ static const struct snd_pcm_hardware oxygen_stereo_hardware = {
>  		SNDRV_PCM_INFO_MMAP_VALID |
>  		SNDRV_PCM_INFO_INTERLEAVED |
>  		SNDRV_PCM_INFO_PAUSE |
> -		SNDRV_PCM_INFO_SYNC_START,
> +		SNDRV_PCM_INFO_SYNC_START |
> +                SNDRV_PCM_INFO_NO_PERIOD_IRQ,
>  	.formats = SNDRV_PCM_FMTBIT_S16_LE |
>  		   SNDRV_PCM_FMTBIT_S32_LE,
>  	.rates = SNDRV_PCM_RATE_32000 |
> @@ -65,7 +66,8 @@ static const struct snd_pcm_hardware oxygen_multichannel_hardware = {
>  		SNDRV_PCM_INFO_MMAP_VALID |
>  		SNDRV_PCM_INFO_INTERLEAVED |
>  		SNDRV_PCM_INFO_PAUSE |
> -		SNDRV_PCM_INFO_SYNC_START,
> +		SNDRV_PCM_INFO_SYNC_START |
> +                SNDRV_PCM_INFO_NO_PERIOD_IRQ,
>  	.formats = SNDRV_PCM_FMTBIT_S16_LE |
>  		   SNDRV_PCM_FMTBIT_S32_LE,
>  	.rates = SNDRV_PCM_RATE_32000 |
> @@ -91,7 +93,8 @@ static const struct snd_pcm_hardware oxygen_ac97_hardware = {
>  		SNDRV_PCM_INFO_MMAP_VALID |
>  		SNDRV_PCM_INFO_INTERLEAVED |
>  		SNDRV_PCM_INFO_PAUSE |
> -		SNDRV_PCM_INFO_SYNC_START,
> +		SNDRV_PCM_INFO_SYNC_START |
> +                SNDRV_PCM_INFO_NO_PERIOD_IRQ,
>  	.formats = SNDRV_PCM_FMTBIT_S16_LE,
>  	.rates = SNDRV_PCM_RATE_48000,
>  	.rate_min = 48000,
> @@ -530,7 +533,10 @@ static int oxygen_prepare(struct snd_pcm_substream *substream)
>  	oxygen_set_bits8(chip, OXYGEN_DMA_FLUSH, channel_mask);
>  	oxygen_clear_bits8(chip, OXYGEN_DMA_FLUSH, channel_mask);
>  
> -	chip->interrupt_mask |= channel_mask;
> +        if (substream->runtime->no_period_irq)
> +               chip->interrupt_mask &= ~channel_mask;
> +        else
> +                chip->interrupt_mask |= channel_mask;
>  	oxygen_write16(chip, OXYGEN_INTERRUPT_MASK, chip->interrupt_mask);
>  	spin_unlock_irq(&chip->reg_lock);
>  	return 0;
> -- 
> 1.7.2.3
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH] ALSA: add support for disabling period irq

[Jaroslav Kysela]

On Tue, 2 Nov 2010, Takashi Iwai wrote:

> At Mon,  1 Nov 2010 17:12:53 -0500,
> Pierre-Louis Bossart wrote:
>>
>> Merged and cleaned patch based on earlier patches posted
>> on alsa-devel by Clemens Ladisch <clemens@ladisch.de> and
>> Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
>>
>> This patch disables period interrupts which are not
>> needed when the application relies on a system timer
>> to wake-up and refill the ring buffer. The behavior of
>> the driver is left unchanged, and interrupts are only
>> disabled if the application requests this configuration.
>> The behavior in case of underruns is slightly different,
>> instead of being detected during the period interrupts the
>> underruns are detected when the application calls
>> snd_pcm_update_avail, which in turns forces a refresh of the
>> hw pointer and shows the buffer is empty.
>
> So, this silently assumes that the applications do call
> snd_pcm_update_avail() appropriately at the right timing?
> If so, any sense to check XRUN in the driver at all...?
>
> And, even more, any sense to report the incremental position by this
> approach?  The only reliable information in this case is the offset in
> the ring buffer.  The linear position as the current ALSA API provides
> isn't guaranteed without the period irq.

We can detect the buffer size crossing using jiffies, but I agree, it's 
something which should be added to the patch to not break hw_ptr in 
case when the application does not call the update function in time.
We have both values in runtime->hw_ptr_jiffies and 
runtime->hw_ptr_buffer_jiffies.

Also, I would remove IRQ or interrupt from the API and use something 
like "no period ack" or so. IRQ is very hardware specific and some drivers 
does not use direct interrupts but another timing sources for period 
acks.

>> +#define SNDRV_PCM_INFO_NO_PERIOD_IRQ    0x00800000      /* period interrupt can be disabled */

#define SNDRV_PCM_INFO_NO_PERIOD_ACK	0x00800000	/* period transfer acknowledge can be disabled */


 				Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: [PATCH] ALSA: add support for disabling period irq

[Takashi Iwai]

At Tue, 2 Nov 2010 09:17:33 +0100 (CET),
Jaroslav Kysela wrote:
> 
> On Tue, 2 Nov 2010, Takashi Iwai wrote:
> 
> > At Mon,  1 Nov 2010 17:12:53 -0500,
> > Pierre-Louis Bossart wrote:
> >>
> >> Merged and cleaned patch based on earlier patches posted
> >> on alsa-devel by Clemens Ladisch <clemens@ladisch.de> and
> >> Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
> >>
> >> This patch disables period interrupts which are not
> >> needed when the application relies on a system timer
> >> to wake-up and refill the ring buffer. The behavior of
> >> the driver is left unchanged, and interrupts are only
> >> disabled if the application requests this configuration.
> >> The behavior in case of underruns is slightly different,
> >> instead of being detected during the period interrupts the
> >> underruns are detected when the application calls
> >> snd_pcm_update_avail, which in turns forces a refresh of the
> >> hw pointer and shows the buffer is empty.
> >
> > So, this silently assumes that the applications do call
> > snd_pcm_update_avail() appropriately at the right timing?
> > If so, any sense to check XRUN in the driver at all...?
> >
> > And, even more, any sense to report the incremental position by this
> > approach?  The only reliable information in this case is the offset in
> > the ring buffer.  The linear position as the current ALSA API provides
> > isn't guaranteed without the period irq.
> 
> We can detect the buffer size crossing using jiffies, but I agree, it's 
> something which should be added to the patch to not break hw_ptr in 
> case when the application does not call the update function in time.
> We have both values in runtime->hw_ptr_jiffies and 
> runtime->hw_ptr_buffer_jiffies.

But I thought this patch also disabled the jiffies check?

I feel that some bottom-line check is needed if we keep the linear
position over the buffer size even without the period irq.
If the app doesn't care, OK fine, the driver shouldn't care, too.
But then it doesn't make sense to keep the linear position, either.

> Also, I would remove IRQ or interrupt from the API and use something 
> like "no period ack" or so. IRQ is very hardware specific and some drivers 
> does not use direct interrupts but another timing sources for period 
> acks.
> 
> >> +#define SNDRV_PCM_INFO_NO_PERIOD_IRQ    0x00800000      /* period interrupt can be disabled */
> 
> #define SNDRV_PCM_INFO_NO_PERIOD_ACK	0x00800000	/* period transfer acknowledge can be disabled */

How about "period update"?
I don't mind much how it's called, though...


thanks,

Takashi

Re: [PATCH] ALSA: add support for disabling period irq

[Jaroslav Kysela]

On Tue, 2 Nov 2010, Takashi Iwai wrote:

> At Tue, 2 Nov 2010 09:17:33 +0100 (CET),
> Jaroslav Kysela wrote:
>>
>> On Tue, 2 Nov 2010, Takashi Iwai wrote:
>>
>>> At Mon,  1 Nov 2010 17:12:53 -0500,
>>> Pierre-Louis Bossart wrote:
>>>>
>>>> Merged and cleaned patch based on earlier patches posted
>>>> on alsa-devel by Clemens Ladisch <clemens@ladisch.de> and
>>>> Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
>>>>
>>>> This patch disables period interrupts which are not
>>>> needed when the application relies on a system timer
>>>> to wake-up and refill the ring buffer. The behavior of
>>>> the driver is left unchanged, and interrupts are only
>>>> disabled if the application requests this configuration.
>>>> The behavior in case of underruns is slightly different,
>>>> instead of being detected during the period interrupts the
>>>> underruns are detected when the application calls
>>>> snd_pcm_update_avail, which in turns forces a refresh of the
>>>> hw pointer and shows the buffer is empty.
>>>
>>> So, this silently assumes that the applications do call
>>> snd_pcm_update_avail() appropriately at the right timing?
>>> If so, any sense to check XRUN in the driver at all...?
>>>
>>> And, even more, any sense to report the incremental position by this
>>> approach?  The only reliable information in this case is the offset in
>>> the ring buffer.  The linear position as the current ALSA API provides
>>> isn't guaranteed without the period irq.
>>
>> We can detect the buffer size crossing using jiffies, but I agree, it's
>> something which should be added to the patch to not break hw_ptr in
>> case when the application does not call the update function in time.
>> We have both values in runtime->hw_ptr_jiffies and
>> runtime->hw_ptr_buffer_jiffies.
>
> But I thought this patch also disabled the jiffies check?

These values are not related only to the debug jiffies check. I'm using 
these values also to detect the double irq acks which were discovered
recently.

> I feel that some bottom-line check is needed if we keep the linear
> position over the buffer size even without the period irq.
> If the app doesn't care, OK fine, the driver shouldn't care, too.
> But then it doesn't make sense to keep the linear position, either.

But if we can keep the linear position using the light-weight jiffies 
check, it's probably OK to keep it rather than changing the hw_ptr 
behaviour.

>> Also, I would remove IRQ or interrupt from the API and use something
>> like "no period ack" or so. IRQ is very hardware specific and some drivers
>> does not use direct interrupts but another timing sources for period
>> acks.
>>
>>>> +#define SNDRV_PCM_INFO_NO_PERIOD_IRQ    0x00800000      /* period interrupt can be disabled */
>>
>> #define SNDRV_PCM_INFO_NO_PERIOD_ACK	0x00800000	/* period transfer acknowledge can be disabled */
>
> How about "period update"?
> I don't mind much how it's called, though...

ACK is smaller, but anything else than IRQ or INTERRUPT sounds good to me.

 					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: [PATCH] ALSA: add support for disabling period irq

[Takashi Iwai]

At Tue, 2 Nov 2010 09:26:48 +0100 (CET),
Jaroslav Kysela wrote:
> 
> On Tue, 2 Nov 2010, Takashi Iwai wrote:
> 
> > At Tue, 2 Nov 2010 09:17:33 +0100 (CET),
> > Jaroslav Kysela wrote:
> >>
> >> On Tue, 2 Nov 2010, Takashi Iwai wrote:
> >>
> >>> At Mon,  1 Nov 2010 17:12:53 -0500,
> >>> Pierre-Louis Bossart wrote:
> >>>>
> >>>> Merged and cleaned patch based on earlier patches posted
> >>>> on alsa-devel by Clemens Ladisch <clemens@ladisch.de> and
> >>>> Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
> >>>>
> >>>> This patch disables period interrupts which are not
> >>>> needed when the application relies on a system timer
> >>>> to wake-up and refill the ring buffer. The behavior of
> >>>> the driver is left unchanged, and interrupts are only
> >>>> disabled if the application requests this configuration.
> >>>> The behavior in case of underruns is slightly different,
> >>>> instead of being detected during the period interrupts the
> >>>> underruns are detected when the application calls
> >>>> snd_pcm_update_avail, which in turns forces a refresh of the
> >>>> hw pointer and shows the buffer is empty.
> >>>
> >>> So, this silently assumes that the applications do call
> >>> snd_pcm_update_avail() appropriately at the right timing?
> >>> If so, any sense to check XRUN in the driver at all...?
> >>>
> >>> And, even more, any sense to report the incremental position by this
> >>> approach?  The only reliable information in this case is the offset in
> >>> the ring buffer.  The linear position as the current ALSA API provides
> >>> isn't guaranteed without the period irq.
> >>
> >> We can detect the buffer size crossing using jiffies, but I agree, it's
> >> something which should be added to the patch to not break hw_ptr in
> >> case when the application does not call the update function in time.
> >> We have both values in runtime->hw_ptr_jiffies and
> >> runtime->hw_ptr_buffer_jiffies.
> >
> > But I thought this patch also disabled the jiffies check?
> 
> These values are not related only to the debug jiffies check. I'm using 
> these values also to detect the double irq acks which were discovered
> recently.

Ah, OK, I overlooked it.

> > I feel that some bottom-line check is needed if we keep the linear
> > position over the buffer size even without the period irq.
> > If the app doesn't care, OK fine, the driver shouldn't care, too.
> > But then it doesn't make sense to keep the linear position, either.
> 
> But if we can keep the linear position using the light-weight jiffies 
> check, it's probably OK to keep it rather than changing the hw_ptr 
> behaviour.

Well, but the point of the patch is to avoid wakeups as much as
possible.  The jiffies-check adds an unconditional wakeup for each
buffer size, thus it's against the purpose of the patch.

So, basically we need to decide whether
 - to check via jiffies as a bottom-line; then no big merit over period=1
 - let the driver not to care anything, apps are responsible for any
   wrong values; in this case, any sense to keep the linear position?

Maybe keeping the linear position would make sense only for the code
compatibility reason.  Many codes are written based on the linear
position behavior.  But then I feel somehow uneasy around this...

 
thanks,

Takashi

Re: [PATCH] ALSA: add support for disabling period irq

[Jaroslav Kysela]

On Tue, 2 Nov 2010, Takashi Iwai wrote:

> At Tue, 2 Nov 2010 09:26:48 +0100 (CET),
> Jaroslav Kysela wrote:
>>
>> On Tue, 2 Nov 2010, Takashi Iwai wrote:
>>
>>> At Tue, 2 Nov 2010 09:17:33 +0100 (CET),
>>> Jaroslav Kysela wrote:
>>>>
>>>> On Tue, 2 Nov 2010, Takashi Iwai wrote:
>>>>
>>>>> At Mon,  1 Nov 2010 17:12:53 -0500,
>>>>> Pierre-Louis Bossart wrote:
>>>>>>
>>>>>> Merged and cleaned patch based on earlier patches posted
>>>>>> on alsa-devel by Clemens Ladisch <clemens@ladisch.de> and
>>>>>> Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
>>>>>>
>>>>>> This patch disables period interrupts which are not
>>>>>> needed when the application relies on a system timer
>>>>>> to wake-up and refill the ring buffer. The behavior of
>>>>>> the driver is left unchanged, and interrupts are only
>>>>>> disabled if the application requests this configuration.
>>>>>> The behavior in case of underruns is slightly different,
>>>>>> instead of being detected during the period interrupts the
>>>>>> underruns are detected when the application calls
>>>>>> snd_pcm_update_avail, which in turns forces a refresh of the
>>>>>> hw pointer and shows the buffer is empty.
>>>>>
>>>>> So, this silently assumes that the applications do call
>>>>> snd_pcm_update_avail() appropriately at the right timing?
>>>>> If so, any sense to check XRUN in the driver at all...?
>>>>>
>>>>> And, even more, any sense to report the incremental position by this
>>>>> approach?  The only reliable information in this case is the offset in
>>>>> the ring buffer.  The linear position as the current ALSA API provides
>>>>> isn't guaranteed without the period irq.
>>>>
>>>> We can detect the buffer size crossing using jiffies, but I agree, it's
>>>> something which should be added to the patch to not break hw_ptr in
>>>> case when the application does not call the update function in time.
>>>> We have both values in runtime->hw_ptr_jiffies and
>>>> runtime->hw_ptr_buffer_jiffies.
>>>
>>> But I thought this patch also disabled the jiffies check?
>>
>> These values are not related only to the debug jiffies check. I'm using
>> these values also to detect the double irq acks which were discovered
>> recently.
>
> Ah, OK, I overlooked it.
>
>>> I feel that some bottom-line check is needed if we keep the linear
>>> position over the buffer size even without the period irq.
>>> If the app doesn't care, OK fine, the driver shouldn't care, too.
>>> But then it doesn't make sense to keep the linear position, either.
>>
>> But if we can keep the linear position using the light-weight jiffies
>> check, it's probably OK to keep it rather than changing the hw_ptr
>> behaviour.
>
> Well, but the point of the patch is to avoid wakeups as much as
> possible.  The jiffies-check adds an unconditional wakeup for each
> buffer size, thus it's against the purpose of the patch.

I don't see any wakeup. It's just about to compare the current jiffies 
with last hw_ptr update jiffies. The multiple buffer size crossing can be 
detected.

> So, basically we need to decide whether
> - to check via jiffies as a bottom-line; then no big merit over period=1

No, the difference is that no interrupts are really required.

 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: [PATCH] ALSA: add support for disabling period irq

[Takashi Iwai]

At Tue, 2 Nov 2010 10:02:49 +0100 (CET),
Jaroslav Kysela wrote:
> 
> On Tue, 2 Nov 2010, Takashi Iwai wrote:
> 
> > At Tue, 2 Nov 2010 09:26:48 +0100 (CET),
> > Jaroslav Kysela wrote:
> >>
> >> On Tue, 2 Nov 2010, Takashi Iwai wrote:
> >>
> >>> At Tue, 2 Nov 2010 09:17:33 +0100 (CET),
> >>> Jaroslav Kysela wrote:
> >>>>
> >>>> On Tue, 2 Nov 2010, Takashi Iwai wrote:
> >>>>
> >>>>> At Mon,  1 Nov 2010 17:12:53 -0500,
> >>>>> Pierre-Louis Bossart wrote:
> >>>>>>
> >>>>>> Merged and cleaned patch based on earlier patches posted
> >>>>>> on alsa-devel by Clemens Ladisch <clemens@ladisch.de> and
> >>>>>> Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
> >>>>>>
> >>>>>> This patch disables period interrupts which are not
> >>>>>> needed when the application relies on a system timer
> >>>>>> to wake-up and refill the ring buffer. The behavior of
> >>>>>> the driver is left unchanged, and interrupts are only
> >>>>>> disabled if the application requests this configuration.
> >>>>>> The behavior in case of underruns is slightly different,
> >>>>>> instead of being detected during the period interrupts the
> >>>>>> underruns are detected when the application calls
> >>>>>> snd_pcm_update_avail, which in turns forces a refresh of the
> >>>>>> hw pointer and shows the buffer is empty.
> >>>>>
> >>>>> So, this silently assumes that the applications do call
> >>>>> snd_pcm_update_avail() appropriately at the right timing?
> >>>>> If so, any sense to check XRUN in the driver at all...?
> >>>>>
> >>>>> And, even more, any sense to report the incremental position by this
> >>>>> approach?  The only reliable information in this case is the offset in
> >>>>> the ring buffer.  The linear position as the current ALSA API provides
> >>>>> isn't guaranteed without the period irq.
> >>>>
> >>>> We can detect the buffer size crossing using jiffies, but I agree, it's
> >>>> something which should be added to the patch to not break hw_ptr in
> >>>> case when the application does not call the update function in time.
> >>>> We have both values in runtime->hw_ptr_jiffies and
> >>>> runtime->hw_ptr_buffer_jiffies.
> >>>
> >>> But I thought this patch also disabled the jiffies check?
> >>
> >> These values are not related only to the debug jiffies check. I'm using
> >> these values also to detect the double irq acks which were discovered
> >> recently.
> >
> > Ah, OK, I overlooked it.
> >
> >>> I feel that some bottom-line check is needed if we keep the linear
> >>> position over the buffer size even without the period irq.
> >>> If the app doesn't care, OK fine, the driver shouldn't care, too.
> >>> But then it doesn't make sense to keep the linear position, either.
> >>
> >> But if we can keep the linear position using the light-weight jiffies
> >> check, it's probably OK to keep it rather than changing the hw_ptr
> >> behaviour.
> >
> > Well, but the point of the patch is to avoid wakeups as much as
> > possible.  The jiffies-check adds an unconditional wakeup for each
> > buffer size, thus it's against the purpose of the patch.
> 
> I don't see any wakeup.

The CPU is woken up.  This is to be avoided.


Takashi

Re: [PATCH] ALSA: add support for disabling period irq

[Jaroslav Kysela]

On Tue, 2 Nov 2010, Takashi Iwai wrote:

> At Tue, 2 Nov 2010 10:02:49 +0100 (CET),
> Jaroslav Kysela wrote:
>>
>> On Tue, 2 Nov 2010, Takashi Iwai wrote:
>>
>>> At Tue, 2 Nov 2010 09:26:48 +0100 (CET),
>>> Jaroslav Kysela wrote:
>>>>
>>>> On Tue, 2 Nov 2010, Takashi Iwai wrote:
>>>>
>>>>> At Tue, 2 Nov 2010 09:17:33 +0100 (CET),
>>>>> Jaroslav Kysela wrote:
>>>>>>
>>>>>> On Tue, 2 Nov 2010, Takashi Iwai wrote:
>>>>>>
>>>>>>> At Mon,  1 Nov 2010 17:12:53 -0500,
>>>>>>> Pierre-Louis Bossart wrote:
>>>>>>>>
>>>>>>>> Merged and cleaned patch based on earlier patches posted
>>>>>>>> on alsa-devel by Clemens Ladisch <clemens@ladisch.de> and
>>>>>>>> Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
>>>>>>>>
>>>>>>>> This patch disables period interrupts which are not
>>>>>>>> needed when the application relies on a system timer
>>>>>>>> to wake-up and refill the ring buffer. The behavior of
>>>>>>>> the driver is left unchanged, and interrupts are only
>>>>>>>> disabled if the application requests this configuration.
>>>>>>>> The behavior in case of underruns is slightly different,
>>>>>>>> instead of being detected during the period interrupts the
>>>>>>>> underruns are detected when the application calls
>>>>>>>> snd_pcm_update_avail, which in turns forces a refresh of the
>>>>>>>> hw pointer and shows the buffer is empty.
>>>>>>>
>>>>>>> So, this silently assumes that the applications do call
>>>>>>> snd_pcm_update_avail() appropriately at the right timing?
>>>>>>> If so, any sense to check XRUN in the driver at all...?
>>>>>>>
>>>>>>> And, even more, any sense to report the incremental position by this
>>>>>>> approach?  The only reliable information in this case is the offset in
>>>>>>> the ring buffer.  The linear position as the current ALSA API provides
>>>>>>> isn't guaranteed without the period irq.
>>>>>>
>>>>>> We can detect the buffer size crossing using jiffies, but I agree, it's
>>>>>> something which should be added to the patch to not break hw_ptr in
>>>>>> case when the application does not call the update function in time.
>>>>>> We have both values in runtime->hw_ptr_jiffies and
>>>>>> runtime->hw_ptr_buffer_jiffies.
>>>>>
>>>>> But I thought this patch also disabled the jiffies check?
>>>>
>>>> These values are not related only to the debug jiffies check. I'm using
>>>> these values also to detect the double irq acks which were discovered
>>>> recently.
>>>
>>> Ah, OK, I overlooked it.
>>>
>>>>> I feel that some bottom-line check is needed if we keep the linear
>>>>> position over the buffer size even without the period irq.
>>>>> If the app doesn't care, OK fine, the driver shouldn't care, too.
>>>>> But then it doesn't make sense to keep the linear position, either.
>>>>
>>>> But if we can keep the linear position using the light-weight jiffies
>>>> check, it's probably OK to keep it rather than changing the hw_ptr
>>>> behaviour.
>>>
>>> Well, but the point of the patch is to avoid wakeups as much as
>>> possible.  The jiffies-check adds an unconditional wakeup for each
>>> buffer size, thus it's against the purpose of the patch.
>>
>> I don't see any wakeup.
>
> The CPU is woken up.  This is to be avoided.

Nope. No timers are used. The jiffies check and hw_ptr correction should 
be done from the avail_update call invoked from an application.

 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: [PATCH] ALSA: add support for disabling period irq

[Takashi Iwai]

At Tue, 2 Nov 2010 10:22:32 +0100 (CET),
Jaroslav Kysela wrote:
> 
> On Tue, 2 Nov 2010, Takashi Iwai wrote:
> 
> > At Tue, 2 Nov 2010 10:02:49 +0100 (CET),
> > Jaroslav Kysela wrote:
> >>
> >> On Tue, 2 Nov 2010, Takashi Iwai wrote:
> >>
> >>> At Tue, 2 Nov 2010 09:26:48 +0100 (CET),
> >>> Jaroslav Kysela wrote:
> >>>>
> >>>> On Tue, 2 Nov 2010, Takashi Iwai wrote:
> >>>>
> >>>>> At Tue, 2 Nov 2010 09:17:33 +0100 (CET),
> >>>>> Jaroslav Kysela wrote:
> >>>>>>
> >>>>>> On Tue, 2 Nov 2010, Takashi Iwai wrote:
> >>>>>>
> >>>>>>> At Mon,  1 Nov 2010 17:12:53 -0500,
> >>>>>>> Pierre-Louis Bossart wrote:
> >>>>>>>>
> >>>>>>>> Merged and cleaned patch based on earlier patches posted
> >>>>>>>> on alsa-devel by Clemens Ladisch <clemens@ladisch.de> and
> >>>>>>>> Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
> >>>>>>>>
> >>>>>>>> This patch disables period interrupts which are not
> >>>>>>>> needed when the application relies on a system timer
> >>>>>>>> to wake-up and refill the ring buffer. The behavior of
> >>>>>>>> the driver is left unchanged, and interrupts are only
> >>>>>>>> disabled if the application requests this configuration.
> >>>>>>>> The behavior in case of underruns is slightly different,
> >>>>>>>> instead of being detected during the period interrupts the
> >>>>>>>> underruns are detected when the application calls
> >>>>>>>> snd_pcm_update_avail, which in turns forces a refresh of the
> >>>>>>>> hw pointer and shows the buffer is empty.
> >>>>>>>
> >>>>>>> So, this silently assumes that the applications do call
> >>>>>>> snd_pcm_update_avail() appropriately at the right timing?
> >>>>>>> If so, any sense to check XRUN in the driver at all...?
> >>>>>>>
> >>>>>>> And, even more, any sense to report the incremental position by this
> >>>>>>> approach?  The only reliable information in this case is the offset in
> >>>>>>> the ring buffer.  The linear position as the current ALSA API provides
> >>>>>>> isn't guaranteed without the period irq.
> >>>>>>
> >>>>>> We can detect the buffer size crossing using jiffies, but I agree, it's
> >>>>>> something which should be added to the patch to not break hw_ptr in
> >>>>>> case when the application does not call the update function in time.
> >>>>>> We have both values in runtime->hw_ptr_jiffies and
> >>>>>> runtime->hw_ptr_buffer_jiffies.
> >>>>>
> >>>>> But I thought this patch also disabled the jiffies check?
> >>>>
> >>>> These values are not related only to the debug jiffies check. I'm using
> >>>> these values also to detect the double irq acks which were discovered
> >>>> recently.
> >>>
> >>> Ah, OK, I overlooked it.
> >>>
> >>>>> I feel that some bottom-line check is needed if we keep the linear
> >>>>> position over the buffer size even without the period irq.
> >>>>> If the app doesn't care, OK fine, the driver shouldn't care, too.
> >>>>> But then it doesn't make sense to keep the linear position, either.
> >>>>
> >>>> But if we can keep the linear position using the light-weight jiffies
> >>>> check, it's probably OK to keep it rather than changing the hw_ptr
> >>>> behaviour.
> >>>
> >>> Well, but the point of the patch is to avoid wakeups as much as
> >>> possible.  The jiffies-check adds an unconditional wakeup for each
> >>> buffer size, thus it's against the purpose of the patch.
> >>
> >> I don't see any wakeup.
> >
> > The CPU is woken up.  This is to be avoided.
> 
> Nope. No timers are used. The jiffies check and hw_ptr correction should 
> be done from the avail_update call invoked from an application.

OK, point taken.


Takashi

Re: [PATCH] ALSA: add support for disabling period irq

[pl bossart]

>>>
>>> #define SNDRV_PCM_INFO_NO_PERIOD_ACK    0x00800000      /* period
>>> transfer acknowledge can be disabled */
>>
>> How about "period update"?
>> I don't mind much how it's called, though...
>
> ACK is smaller, but anything else than IRQ or INTERRUPT sounds good to me.

Anybody against WAKEUP?

Re: [PATCH] ALSA: add support for disabling period irq

[pl bossart]

[-- Attachment #1: Type: text/plain, Size: 1360 bytes --]

>>>>>> I feel that some bottom-line check is needed if we keep the linear
>>>>>> position over the buffer size even without the period irq.
>>>>>> If the app doesn't care, OK fine, the driver shouldn't care, too.
>>>>>> But then it doesn't make sense to keep the linear position, either.
>>>>>
>>>>> But if we can keep the linear position using the light-weight jiffies
>>>>> check, it's probably OK to keep it rather than changing the hw_ptr
>>>>> behaviour.
>>>>
>>>> Well, but the point of the patch is to avoid wakeups as much as
>>>> possible.  The jiffies-check adds an unconditional wakeup for each
>>>> buffer size, thus it's against the purpose of the patch.
>>>
>>> I don't see any wakeup.
>>
>> The CPU is woken up.  This is to be avoided.
>
> Nope. No timers are used. The jiffies check and hw_ptr correction should be
> done from the avail_update call invoked from an application.

ok, so I re-implemented all the points found in the reviews except for
this thread. There's already some code to see if we go beyond the
buffer boundaries, so I am not sure what the 'linear position' means.
Do I need to implement anything specific to check the validity of the
hw_ptr values? I am not clear if I need to do anything beyond the fix
attached, and I don't honestly understand all these checks with
jiffies.
Thanks.
-Pierre

[-- Attachment #2: pcm_lib.patch --]
[-- Type: application/octet-stream, Size: 749 bytes --]

diff --git a/core/pcm_lib.c b/core/pcm_lib.c
index a1707cc..1ccea4c 100644
--- a/core/pcm_lib.c
+++ b/core/pcm_lib.c
@@ -373,6 +373,10 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 			   (unsigned long)new_hw_ptr,
 			   (unsigned long)runtime->hw_ptr_base);
 	}
+
+        if (runtime->no_period_wakeup)
+                goto no_jiffies_check_if_no_period_wakeup;
+
 	/* something must be really wrong */
 	if (delta >= runtime->buffer_size + runtime->period_size) {
 		hw_ptr_error(substream,
@@ -442,6 +446,7 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 			     (long)old_hw_ptr);
 	}
 
+ no_jiffies_check_if_no_period_wakeup:
 	if (runtime->status->hw_ptr == new_hw_ptr)
 		return 0;
 

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ALSA: add support for disabling period irq

[Jaroslav Kysela]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1390 bytes --]

On Mon, 8 Nov 2010, pl bossart wrote:

>>>>>>> I feel that some bottom-line check is needed if we keep the linear
>>>>>>> position over the buffer size even without the period irq.
>>>>>>> If the app doesn't care, OK fine, the driver shouldn't care, too.
>>>>>>> But then it doesn't make sense to keep the linear position, either.
>>>>>>
>>>>>> But if we can keep the linear position using the light-weight jiffies
>>>>>> check, it's probably OK to keep it rather than changing the hw_ptr
>>>>>> behaviour.
>>>>>
>>>>> Well, but the point of the patch is to avoid wakeups as much as
>>>>> possible.  The jiffies-check adds an unconditional wakeup for each
>>>>> buffer size, thus it's against the purpose of the patch.
>>>>
>>>> I don't see any wakeup.
>>>
>>> The CPU is woken up.  This is to be avoided.
>>
>> Nope. No timers are used. The jiffies check and hw_ptr correction should be
>> done from the avail_update call invoked from an application.
>
> ok, so I re-implemented all the points found in the reviews except for
> this thread. There's already some code to see if we go beyond the
> buffer boundaries, so I am not sure what the 'linear position' means.

Linear means that new hw_ptr must be always behind the previous hw_ptr.

 					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ALSA: add support for disabling period irq

[pl bossart]

done from the avail_update call invoked from an application.
>>
>> ok, so I re-implemented all the points found in the reviews except for
>> this thread. There's already some code to see if we go beyond the
>> buffer boundaries, so I am not sure what the 'linear position' means.
>
> Linear means that new hw_ptr must be always behind the previous hw_ptr.

I got that. But there are already some checks in pcm_lib.c,

	/* new_hw_ptr might be lower than old_hw_ptr in case when */
	/* pointer crosses the end of the ring buffer */
	if (new_hw_ptr < old_hw_ptr) {
		hw_base += runtime->buffer_size;
		if (hw_base >= runtime->boundary)
			hw_base = 0;
		new_hw_ptr = hw_base + pos;
	}

So do I need to add any additional checks? Or did I miss something in
your discussion with Takashi?
-Pierre