Hi Takashi, Our QA has tested your patch. It works and the basic test cases passed. Regards, Libin > -----Original Message----- > From: Takashi Iwai [mailto:tiwai@xxxxxxx] > Sent: Tuesday, February 23, 2016 11:04 PM > To: Yang, Libin > Cc: Lin, Mengdong; libin.yang@xxxxxxxxxxxxxxx; alsa-devel@alsa- > project.org > Subject: Re: [RFC: PATCH] ALSA: hda - clear IRQ statu twice on > some Intel platforms > > On Tue, 23 Feb 2016 15:27:50 +0100, > Yang, Libin wrote: > > > > > -----Original Message----- > > > From: alsa-devel-bounces@xxxxxxxxxxxxxxxx [mailto:alsa-devel- > > > bounces@xxxxxxxxxxxxxxxx] On Behalf Of Takashi Iwai > > > Sent: Tuesday, February 23, 2016 5:28 PM > > > To: Yang, Libin > > > Cc: Lin, Mengdong; libin.yang@xxxxxxxxxxxxxxx; alsa-devel@alsa- > > > project.org > > > Subject: Re: [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. > > OK, below is the revised patch. Let me know if this works. > > > thanks, > > Takashi > > -- 8< -- > From: Takashi Iwai <tiwai@xxxxxxx> > Subject: [PATCH] ALSA: hda - Loop interrupt handling until really cleared > > Currently the interrupt handler of HD-audio driver assumes that no irq > update is needed while processing the irq. But in reality, it has > been confirmed that the HW irq is issued even during the irq > handling. Since we clear the irq status at the beginning, process the > interrupt, then exits from the handler, the lately issued interrupt is > left untouched without being properly processed. > > This patch changes the interrupt handler code to loop over the > check-and-process. The handler tries repeatedly as long as the IRQ > status are turned on, and either stream or CORB/RIRB is handled. > > For checking the stream handling, snd_hdac_bus_handle_stream_irq() > returns a value indicating the stream indices bits. Other than that, > the change is only in the irq handler itself. > > Reported-by: Libin Yang <libin.yang@xxxxxxxxxxxxxxx> > Cc: <stable@xxxxxxxxxxxxxxx> > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> > --- > include/sound/hdaudio.h | 2 +- > sound/hda/hdac_controller.c | 7 ++++++- > sound/pci/hda/hda_controller.c | 47 +++++++++++++++++++++++------- > ------------ > 3 files changed, 33 insertions(+), 23 deletions(-) > > diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h > index e2b712c90d3f..c21c38ce7450 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, > +int 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..8c486235c905 100644 > --- a/sound/hda/hdac_controller.c > +++ b/sound/hda/hdac_controller.c > @@ -426,18 +426,22 @@ > EXPORT_SYMBOL_GPL(snd_hdac_bus_stop_chip); > * @bus: HD-audio core bus > * @status: INTSTS register value > * @ask: callback to be called for woken streams > + * > + * Returns the bits of handled streams, or zero if no stream is handled. > */ > -void snd_hdac_bus_handle_stream_irq(struct hdac_bus *bus, unsigned > int status, > +int 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; > + int handled = 0; > > 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 |= 1 << azx_dev->index; > if (!azx_dev->substream || !azx_dev->running > || > !(sd_status & SD_INT_COMPLETE)) > continue; > @@ -445,6 +449,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..27de8015717d 100644 > --- a/sound/pci/hda/hda_controller.c > +++ b/sound/pci/hda/hda_controller.c > @@ -930,6 +930,8 @@ irqreturn_t azx_interrupt(int irq, void *dev_id) > struct azx *chip = dev_id; > struct hdac_bus *bus = azx_bus(chip); > u32 status; > + bool active, handled = false; > + int repeat = 0; /* count for avoiding endless loop */ > > #ifdef CONFIG_PM > if (azx_has_pm_runtime(chip)) > @@ -939,33 +941,36 @@ irqreturn_t azx_interrupt(int irq, void *dev_id) > > spin_lock(&bus->reg_lock); > > - if (chip->disabled) { > - spin_unlock(&bus->reg_lock); > - return IRQ_NONE; > - } > - > - status = azx_readl(chip, INTSTS); > - if (status == 0 || status == 0xffffffff) { > - spin_unlock(&bus->reg_lock); > - return IRQ_NONE; > - } > + if (chip->disabled) > + goto unlock; > > - snd_hdac_bus_handle_stream_irq(bus, status, stream_update); > + do { > + 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); > + handled = true; > + active = false; > + if (snd_hdac_bus_handle_stream_irq(bus, status, > stream_update)) > + active = true; > + > + /* 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); > - } > + } while (active && ++repeat < 10); > > + unlock: > spin_unlock(&bus->reg_lock); > > - return IRQ_HANDLED; > + return IRQ_RETVAL(handled); > } > EXPORT_SYMBOL_GPL(azx_interrupt); > > -- > 2.7.1 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
