[alsa-devel] [RFC: PATCH] ALSA: hda - clear IRQ statu twice on some Intel platforms

[Takashi Iwai]

On Tue, 23 Feb 2016 09:57:13 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> I'm testing your patch in stress level. So far, it seems to works smoothly.
> 
> Which patch do you like? And your patch seems to impact on all the platforms?

One potential problem in your patch is that the later triggered event
shall be ignored by the post-irq ack.

OTOH, my patch would impact to all platforms, yes.  However, in
theory, all platforms may have this issue, so it's anyway good in one
side.  In another flip, of course, it might introduce some regression.

We can limit this loop check only for some known platforms, or we let
it tested more :)

I'm going to cook the patch a bit more and give you for testing.


thanks,

Takashi

> 
> Regards,
> Libin
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Monday, February 22, 2016 4:44 PM
> > To: libin.yang at linux.intel.com
> > Cc: alsa-devel at alsa-project.org; Lin, Mengdong; Yang, Libin
> > Subject: Re: [alsa-devel] [RFC: PATCH] ALSA: hda - clear IRQ statu twice on
> > some Intel platforms
> > 
> > On Mon, 22 Feb 2016 08:39:50 +0100,
> > libin.yang at linux.intel.com wrote:
> > >
> > > From: Libin Yang <libin.yang at linux.intel.com>
> > >
> > > On some Intel platforms, we found the interrupt issue in
> > > the below scenario:
> > > 1. driver is in irq handler
> > > 2. there is another interrupt from HW after interrupt status
> > >    is cleared and before exiting from interrupt handler
> > > 3. exit from the current irq handling
> > >
> > > After exiting the irq handler, it should raise another
> > > interrupt for driver to handle the new interrupt. But actually,
> > > it failed to raise the interrupt and driver will never have
> > > chance to clear the interrupt status.
> > >
> > > The patch clears the interrupt status again just before exiting
> > > for interrupt handler. This can reduce the contest dramatically.
> > >
> > > Signed-off-by: Libin Yang <libin.yang at linux.intel.com>
> > 
> > An alternative way is to loop while the status bit is reenabled.  An
> > untested patch is below.  Not sure which is better yet.  Just an idea.
> > 
> > 
> > Takashi
> > 
> > ---
> > diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> > index e2b712c90d3f..b74d9c8dceff 100644
> > --- a/include/sound/hdaudio.h
> > +++ b/include/sound/hdaudio.h
> > @@ -343,7 +343,7 @@ void snd_hdac_bus_enter_link_reset(struct
> > hdac_bus *bus);
> >  void snd_hdac_bus_exit_link_reset(struct hdac_bus *bus);
> > 
> >  void snd_hdac_bus_update_rirb(struct hdac_bus *bus);
> > -void snd_hdac_bus_handle_stream_irq(struct hdac_bus *bus, unsigned
> > int status,
> > +bool snd_hdac_bus_handle_stream_irq(struct hdac_bus *bus, unsigned
> > int status,
> >  				    void (*ack)(struct hdac_bus *,
> >  						struct hdac_stream *));
> > 
> > diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
> > index b5a17cb510a0..448cd8c990f0 100644
> > --- a/sound/hda/hdac_controller.c
> > +++ b/sound/hda/hdac_controller.c
> > @@ -427,17 +427,19 @@
> > EXPORT_SYMBOL_GPL(snd_hdac_bus_stop_chip);
> >   * @status: INTSTS register value
> >   * @ask: callback to be called for woken streams
> >   */
> > -void snd_hdac_bus_handle_stream_irq(struct hdac_bus *bus, unsigned
> > int status,
> > +bool snd_hdac_bus_handle_stream_irq(struct hdac_bus *bus, unsigned
> > int status,
> >  				    void (*ack)(struct hdac_bus *,
> >  						struct hdac_stream *))
> >  {
> >  	struct hdac_stream *azx_dev;
> >  	u8 sd_status;
> > +	bool handled = false;
> > 
> >  	list_for_each_entry(azx_dev, &bus->stream_list, list) {
> >  		if (status & azx_dev->sd_int_sta_mask) {
> >  			sd_status = snd_hdac_stream_readb(azx_dev,
> > SD_STS);
> >  			snd_hdac_stream_writeb(azx_dev, SD_STS,
> > SD_INT_MASK);
> > +			handled = true;
> >  			if (!azx_dev->substream || !azx_dev->running
> > ||
> >  			    !(sd_status & SD_INT_COMPLETE))
> >  				continue;
> > @@ -445,6 +447,7 @@ void snd_hdac_bus_handle_stream_irq(struct
> > hdac_bus *bus, unsigned int status,
> >  				ack(bus, azx_dev);
> >  		}
> >  	}
> > +	return handled;
> >  }
> >  EXPORT_SYMBOL_GPL(snd_hdac_bus_handle_stream_irq);
> > 
> > diff --git a/sound/pci/hda/hda_controller.c
> > b/sound/pci/hda/hda_controller.c
> > index 37cf9cee9835..a4226026b1dc 100644
> > --- a/sound/pci/hda/hda_controller.c
> > +++ b/sound/pci/hda/hda_controller.c
> > @@ -930,6 +930,7 @@ irqreturn_t azx_interrupt(int irq, void *dev_id)
> >  	struct azx *chip = dev_id;
> >  	struct hdac_bus *bus = azx_bus(chip);
> >  	u32 status;
> > +	bool handled = false;
> > 
> >  #ifdef CONFIG_PM
> >  	if (azx_has_pm_runtime(chip))
> > @@ -944,28 +945,35 @@ irqreturn_t azx_interrupt(int irq, void *dev_id)
> >  		return IRQ_NONE;
> >  	}
> > 
> > -	status = azx_readl(chip, INTSTS);
> > -	if (status == 0 || status == 0xffffffff) {
> > -		spin_unlock(&bus->reg_lock);
> > -		return IRQ_NONE;
> > -	}
> > +	for (;;) {
> > +		bool active;
> > 
> > -	snd_hdac_bus_handle_stream_irq(bus, status, stream_update);
> > +		status = azx_readl(chip, INTSTS);
> > +		if (status == 0 || status == 0xffffffff)
> > +			break;
> > 
> > -	/* clear rirb int */
> > -	status = azx_readb(chip, RIRBSTS);
> > -	if (status & RIRB_INT_MASK) {
> > -		if (status & RIRB_INT_RESPONSE) {
> > -			if (chip->driver_caps &
> > AZX_DCAPS_CTX_WORKAROUND)
> > -				udelay(80);
> > -			snd_hdac_bus_update_rirb(bus);
> > +		active = snd_hdac_bus_handle_stream_irq(bus, status,
> > stream_update);
> > +
> > +		/* clear rirb int */
> > +		status = azx_readb(chip, RIRBSTS);
> > +		if (status & RIRB_INT_MASK) {
> > +			active = true;
> > +			if (status & RIRB_INT_RESPONSE) {
> > +				if (chip->driver_caps &
> > AZX_DCAPS_CTX_WORKAROUND)
> > +					udelay(80);
> > +				snd_hdac_bus_update_rirb(bus);
> > +			}
> > +			azx_writeb(chip, RIRBSTS, RIRB_INT_MASK);
> >  		}
> > -		azx_writeb(chip, RIRBSTS, RIRB_INT_MASK);
> > +
> > +		if (!active)
> > +			break;
> > +		handled = true;
> >  	}
> > 
> >  	spin_unlock(&bus->reg_lock);
> > 
> > -	return IRQ_HANDLED;
> > +	return IRQ_RETVAL(handled);
> >  }
> >  EXPORT_SYMBOL_GPL(azx_interrupt);
> > 
>