[PATCH] ALSA: HDA: Lessen CPU usage when waiting for chip to respond

[PATCH] ALSA: HDA: Lessen CPU usage when waiting for chip to respond

[David Henningsson]

When an IRQ for some reason gets lost, we wait up to a second using
udelay, which is CPU intensive. This patch improves the situation by
waiting about 30 ms in the CPU intensive mode, then stepping down to
using msleep(2) instead. In essence, we trade some granularity in
exchange for less CPU consumption when the waiting time is a bit longer.

As a result, PulseAudio should no longer be killed by the kernel
for taking up to much RT-prio CPU time. At least not for *this* reason.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/hda_intel.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Hi Arun,

Can you check if this patch resolves your problem with PulseAudio getting
killed by the kernel? If so, we should apply it to the kernel, perhaps even
to stable.

// David


diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 7b6a823..0e7c3f1 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -783,11 +783,13 @@ static unsigned int azx_rirb_get_response(struct hda_bus *bus,
 {
 	struct azx *chip = bus->private_data;
 	unsigned long timeout;
+	unsigned long loopcounter;
 	int do_poll = 0;
 
  again:
 	timeout = jiffies + msecs_to_jiffies(1000);
-	for (;;) {
+	
+	for (loopcounter = 0;; loopcounter++) {
 		if (chip->polling_mode || do_poll) {
 			spin_lock_irq(&chip->reg_lock);
 			azx_update_rirb(chip);
@@ -803,7 +805,7 @@ static unsigned int azx_rirb_get_response(struct hda_bus *bus,
 		}
 		if (time_after(jiffies, timeout))
 			break;
-		if (bus->needs_damn_long_delay)
+		if (bus->needs_damn_long_delay || loopcounter > 3000)
 			msleep(2); /* temporary workaround */
 		else {
 			udelay(10);
-- 
1.7.9.5

Re: [PATCH] ALSA: HDA: Lessen CPU usage when waiting for chip to respond

[Takashi Iwai]

At Fri,  4 May 2012 11:05:55 +0200,
David Henningsson wrote:
> 
> When an IRQ for some reason gets lost, we wait up to a second using
> udelay, which is CPU intensive. This patch improves the situation by
> waiting about 30 ms in the CPU intensive mode, then stepping down to
> using msleep(2) instead. In essence, we trade some granularity in
> exchange for less CPU consumption when the waiting time is a bit longer.
> 
> As a result, PulseAudio should no longer be killed by the kernel
> for taking up to much RT-prio CPU time. At least not for *this* reason.
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  sound/pci/hda/hda_intel.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Hi Arun,
> 
> Can you check if this patch resolves your problem with PulseAudio getting
> killed by the kernel? If so, we should apply it to the kernel, perhaps even
> to stable.

The patch looks good to me.  I'm going to apply it for 3.4 kernel
tree (with Cc to stable, too) if it's confirmed to improve actually.
Even if not, it's still good to apply, but this kind of change is a
3.5 kernel material.

So, Arun, please let me know whether it works for you.


thanks,

Takashi

> 
> // David
> 
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 7b6a823..0e7c3f1 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -783,11 +783,13 @@ static unsigned int azx_rirb_get_response(struct hda_bus *bus,
>  {
>  	struct azx *chip = bus->private_data;
>  	unsigned long timeout;
> +	unsigned long loopcounter;
>  	int do_poll = 0;
>  
>   again:
>  	timeout = jiffies + msecs_to_jiffies(1000);
> -	for (;;) {
> +	
> +	for (loopcounter = 0;; loopcounter++) {
>  		if (chip->polling_mode || do_poll) {
>  			spin_lock_irq(&chip->reg_lock);
>  			azx_update_rirb(chip);
> @@ -803,7 +805,7 @@ static unsigned int azx_rirb_get_response(struct hda_bus *bus,
>  		}
>  		if (time_after(jiffies, timeout))
>  			break;
> -		if (bus->needs_damn_long_delay)
> +		if (bus->needs_damn_long_delay || loopcounter > 3000)
>  			msleep(2); /* temporary workaround */
>  		else {
>  			udelay(10);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH] ALSA: HDA: Lessen CPU usage when waiting for chip to respond

[Arun Raghavan]

On Fri, 2012-05-04 at 11:05 +0200, David Henningsson wrote:
> When an IRQ for some reason gets lost, we wait up to a second using
> udelay, which is CPU intensive. This patch improves the situation by
> waiting about 30 ms in the CPU intensive mode, then stepping down to
> using msleep(2) instead. In essence, we trade some granularity in
> exchange for less CPU consumption when the waiting time is a bit longer.
> 
> As a result, PulseAudio should no longer be killed by the kernel
> for taking up to much RT-prio CPU time. At least not for *this* reason.
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  sound/pci/hda/hda_intel.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Hi Arun,
> 
> Can you check if this patch resolves your problem with PulseAudio getting
> killed by the kernel? If so, we should apply it to the kernel, perhaps even
> to stable.
> 
> // David

Thanks, this fixes the problem for me. Don't know what the official
procedure for these things is, but fwiw:

Signed-off-by: Arun Raghavan <arun.raghavan@collabora.co.uk>

> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 7b6a823..0e7c3f1 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -783,11 +783,13 @@ static unsigned int azx_rirb_get_response(struct hda_bus *bus,
>  {
>  	struct azx *chip = bus->private_data;
>  	unsigned long timeout;
> +	unsigned long loopcounter;
>  	int do_poll = 0;
>  
>   again:
>  	timeout = jiffies + msecs_to_jiffies(1000);
> -	for (;;) {
> +	
> +	for (loopcounter = 0;; loopcounter++) {
>  		if (chip->polling_mode || do_poll) {
>  			spin_lock_irq(&chip->reg_lock);
>  			azx_update_rirb(chip);
> @@ -803,7 +805,7 @@ static unsigned int azx_rirb_get_response(struct hda_bus *bus,
>  		}
>  		if (time_after(jiffies, timeout))
>  			break;
> -		if (bus->needs_damn_long_delay)
> +		if (bus->needs_damn_long_delay || loopcounter > 3000)
>  			msleep(2); /* temporary workaround */
>  		else {
>  			udelay(10);

Re: [PATCH] ALSA: HDA: Lessen CPU usage when waiting for chip to respond

[Takashi Iwai]

At Wed, 09 May 2012 01:35:52 +0530,
Arun Raghavan wrote:
> 
> On Fri, 2012-05-04 at 11:05 +0200, David Henningsson wrote:
> > When an IRQ for some reason gets lost, we wait up to a second using
> > udelay, which is CPU intensive. This patch improves the situation by
> > waiting about 30 ms in the CPU intensive mode, then stepping down to
> > using msleep(2) instead. In essence, we trade some granularity in
> > exchange for less CPU consumption when the waiting time is a bit longer.
> > 
> > As a result, PulseAudio should no longer be killed by the kernel
> > for taking up to much RT-prio CPU time. At least not for *this* reason.
> > 
> > Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> > ---
> >  sound/pci/hda/hda_intel.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > Hi Arun,
> > 
> > Can you check if this patch resolves your problem with PulseAudio getting
> > killed by the kernel? If so, we should apply it to the kernel, perhaps even
> > to stable.
> > 
> > // David
> 
> Thanks, this fixes the problem for me. Don't know what the official
> procedure for these things is, but fwiw:
> 
> Signed-off-by: Arun Raghavan <arun.raghavan@collabora.co.uk>

I guess you meant tested-by tag?
In anyway I took the patch now with Cc to stable.  It'll be included
in the next pull request to Linus.


thanks,

Takashi

> 
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 7b6a823..0e7c3f1 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -783,11 +783,13 @@ static unsigned int azx_rirb_get_response(struct hda_bus *bus,
> >  {
> >  	struct azx *chip = bus->private_data;
> >  	unsigned long timeout;
> > +	unsigned long loopcounter;
> >  	int do_poll = 0;
> >  
> >   again:
> >  	timeout = jiffies + msecs_to_jiffies(1000);
> > -	for (;;) {
> > +	
> > +	for (loopcounter = 0;; loopcounter++) {
> >  		if (chip->polling_mode || do_poll) {
> >  			spin_lock_irq(&chip->reg_lock);
> >  			azx_update_rirb(chip);
> > @@ -803,7 +805,7 @@ static unsigned int azx_rirb_get_response(struct hda_bus *bus,
> >  		}
> >  		if (time_after(jiffies, timeout))
> >  			break;
> > -		if (bus->needs_damn_long_delay)
> > +		if (bus->needs_damn_long_delay || loopcounter > 3000)
> >  			msleep(2); /* temporary workaround */
> >  		else {
> >  			udelay(10);
> 
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH] ALSA: HDA: Lessen CPU usage when waiting for chip to respond

[Arun Raghavan]

On Wed, 2012-05-09 at 12:51 +0200, Takashi Iwai wrote:
> At Wed, 09 May 2012 01:35:52 +0530,
> Arun Raghavan wrote:
> > 
> > On Fri, 2012-05-04 at 11:05 +0200, David Henningsson wrote:
> > > When an IRQ for some reason gets lost, we wait up to a second using
> > > udelay, which is CPU intensive. This patch improves the situation by
> > > waiting about 30 ms in the CPU intensive mode, then stepping down to
> > > using msleep(2) instead. In essence, we trade some granularity in
> > > exchange for less CPU consumption when the waiting time is a bit longer.
> > > 
> > > As a result, PulseAudio should no longer be killed by the kernel
> > > for taking up to much RT-prio CPU time. At least not for *this* reason.
> > > 
> > > Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> > > ---
> > >  sound/pci/hda/hda_intel.c |    6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > Hi Arun,
> > > 
> > > Can you check if this patch resolves your problem with PulseAudio getting
> > > killed by the kernel? If so, we should apply it to the kernel, perhaps even
> > > to stable.
> > > 
> > > // David
> > 
> > Thanks, this fixes the problem for me. Don't know what the official
> > procedure for these things is, but fwiw:
> > 
> > Signed-off-by: Arun Raghavan <arun.raghavan@collabora.co.uk>
> 
> I guess you meant tested-by tag?
> In anyway I took the patch now with Cc to stable.  It'll be included
> in the next pull request to Linus.

I did, and now I know what tag to use the next time. :)

Thanks,
Arun