From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [RFC: PATCH] ALSA: hda - clear IRQ statu twice on some Intel platforms Date: Fri, 26 Feb 2016 08:51:26 +0100 Message-ID: References: <1456126790-13480-1-git-send-email-libin.yang@linux.intel.com> <96A12704CE18D347B625EE2D4A099D1904670652@SHSMSX103.ccr.corp.intel.com> <96A12704CE18D347B625EE2D4A099D1904679BC8@SHSMSX103.ccr.corp.intel.com> <96A12704CE18D347B625EE2D4A099D190467D055@SHSMSX103.ccr.corp.intel.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id E11FE26526E for ; Fri, 26 Feb 2016 08:51:27 +0100 (CET) In-Reply-To: <96A12704CE18D347B625EE2D4A099D190467D055@SHSMSX103.ccr.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: "Yang, Libin" Cc: "Lin, Mengdong" , "libin.yang@linux.intel.com" , "alsa-devel@alsa-project.org" List-Id: alsa-devel@alsa-project.org On Fri, 26 Feb 2016 06:22:34 +0100, Yang, Libin wrote: > > Hi Takashi, > > Our QA has tested your patch. It works and the basic test cases passed. OK, I queued it up now. thanks, Takashi > > Regards, > Libin > > > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@suse.de] > > Sent: Tuesday, February 23, 2016 11:04 PM > > To: Yang, Libin > > Cc: Lin, Mengdong; libin.yang@linux.intel.com; alsa-devel@alsa- > > project.org > > Subject: Re: [alsa-devel] [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@alsa-project.org [mailto:alsa-devel- > > > > bounces@alsa-project.org] On Behalf Of Takashi Iwai > > > > Sent: Tuesday, February 23, 2016 5:28 PM > > > > To: Yang, Libin > > > > Cc: Lin, Mengdong; libin.yang@linux.intel.com; alsa-devel@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. > > > > OK, below is the revised patch. Let me know if this works. > > > > > > thanks, > > > > Takashi > > > > -- 8< -- > > From: Takashi Iwai > > 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 > > Cc: > > Signed-off-by: Takashi Iwai > > --- > > 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 >