[alsa-devel] [RFC: PATCH] ALSA: hda - clear IRQ statu twice on some Intel platforms
Yang, Libin
libin.yang at intel.com
Tue Feb 23 15:27:50 CET 2016
> -----Original Message-----
> From: alsa-devel-bounces at alsa-project.org [mailto:alsa-devel-
> bounces at alsa-project.org] On Behalf Of Takashi Iwai
> Sent: Tuesday, February 23, 2016 5:28 PM
> To: Yang, Libin
> Cc: Lin, Mengdong; libin.yang at linux.intel.com; alsa-devel at alsa-
> project.org
> Subject: Re: [alsa-devel] [RFC: PATCH] ALSA: hda - clear IRQ statu twice on
> some Intel platforms
>
> 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.
Yes. So it seems your patch is better.
>
> 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.
I'd like to test your updated patch.
Regards,
Libin
>
>
> 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);
> > >
> >
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
More information about the Alsa-devel
mailing list