From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Takashi Iwai <tiwai@suse.de>,
Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Cc: "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
"commit_signer:6/16=38%, authored:6/16=38%,
added_lines:123/248=50%, removed_lines:36/84=43%,
Kai Vehmanen DRIVERS \)" <kai.vehmanen@linux.intel.com>,
Bard Liao <yung-chuan.liao@linux.intel.com>,
"Rojewski, Cezary" <cezary.rojewski@intel.com>,
Takashi Iwai <tiwai@suse.com>,
Keyon Jie <yang.jie@linux.intel.com>, "authored:2/16=12%,
added_lines:21/248=8%, removed_lines:5/84=6%, \),
Liam Girdwood DRIVERS \)" <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>,
Zhu Yingjiang <yingjiang.zhu@linux.intel.com>,
"sound-open-firmware@alsa-project.orgDRIVERS"
<sound-open-firmware@alsa-project.orgDRIVERS>,
"Daniel Baluta DRIVERS \)" <daniel.baluta@nxp.com>,
"Lu, Brent" <brent.lu@intel.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ASoC: SOF: Intel: hda: unsolicited RIRB response
Date: Thu, 11 Jun 2020 15:36:58 -0500 [thread overview]
Message-ID: <b7e98ae0-ea42-bced-1c0f-caa73e798908@linux.intel.com> (raw)
In-Reply-To: <s5hzh99cqc0.wl-tiwai@suse.de>
>>>> I added debug messages to print the RIRBWP register and realize
>>>> that
>>>> response could come between the read of RIRBWP in the
>>>> snd_hdac_bus_update_rirb() function and the interrupt clear in the
>>>> hda_dsp_stream_interrupt() function. The response is not handled
>>>> but
>>>> the interrupt is already cleared. It will cause timeout unless more
>>>> responses coming to RIRB.
>>>
>>> Now I noticed that the legacy driver already addressed it recently
>>> via
>>> commit 6d011d5057ff
>>> ALSA: hda: Clear RIRB status before reading WP
>>>
>>> We should have checked SOF at the same time, too...
>>
>> Thanks, Takashi. But the legacy driver but doesnt remove the loop. The
>> loop added in the SOF driver was based on the legacy driver and
>> specifically to handle missed stream interrupts. Is there any harm in
>> keeping the loop?
>
> A loop there might be safer to keep, indeed. That's basically for a
> difference kind of race, and it can still happen theoretically.
>
> Though, SOF is with the threaded interrupt, and it's interesting how
> the behavior differs. I can imagine that, if a thread irq is running
> while a new IRQ is re-triggered, the hard irq handler won't queue it
> again. But I might be wrong here, need some checks.
IIRC we added this loop before merging all interrupt handling in one
thread, somehow the MSI mode never worked reliably without this change,
so maybe we don't need this loop any longer.
I'd really prefer it if we didn't tie the RIRB handing change to this
loop change, removing the loop should only be done with *a lot of testing*.
WARNING: multiple messages have this Message-ID (diff)
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Takashi Iwai <tiwai@suse.de>,
Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Cc: "Lu, Brent" <brent.lu@intel.com>,
"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
"authored:2/16=12%,added_lines:21/248=8%,removed_lines:5/84=6%,),Liam"
Girdwood DRIVERS ")" <lgirdwood@gmail.com>,
"commit_signer:6/16=38%,authored:6/16=38%,added_lines:123/248=50%
,removed_lines:36/84=43%,Kai" Vehmanen DRIVERS ")"
<kai.vehmanen@linux.intel.com>,
"Daniel Baluta DRIVERS )" <daniel.baluta@nxp.com>,
Mark Brown <broonie@kernel.org>, Jaroslav Kysela <perex@perex.cz>,
Takashi Iwai <tiwai@suse.com>,
"Rojewski, Cezary" <cezary.rojewski@intel.com>,
Zhu Yingjiang <yingjiang.zhu@linux.intel.com>,
Keyon Jie <yang.jie@linux.intel.com>,
Bard Liao <yung-chuan.liao@linux.intel.com>,
"sound-open-firmware@alsa-project.orgDRIVERS"
<sound-open-firmware@alsa-project.orgDRIVERS>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ASoC: SOF: Intel: hda: unsolicited RIRB response
Date: Thu, 11 Jun 2020 15:36:58 -0500 [thread overview]
Message-ID: <b7e98ae0-ea42-bced-1c0f-caa73e798908@linux.intel.com> (raw)
In-Reply-To: <s5hzh99cqc0.wl-tiwai@suse.de>
>>>> I added debug messages to print the RIRBWP register and realize
>>>> that
>>>> response could come between the read of RIRBWP in the
>>>> snd_hdac_bus_update_rirb() function and the interrupt clear in the
>>>> hda_dsp_stream_interrupt() function. The response is not handled
>>>> but
>>>> the interrupt is already cleared. It will cause timeout unless more
>>>> responses coming to RIRB.
>>>
>>> Now I noticed that the legacy driver already addressed it recently
>>> via
>>> commit 6d011d5057ff
>>> ALSA: hda: Clear RIRB status before reading WP
>>>
>>> We should have checked SOF at the same time, too...
>>
>> Thanks, Takashi. But the legacy driver but doesnt remove the loop. The
>> loop added in the SOF driver was based on the legacy driver and
>> specifically to handle missed stream interrupts. Is there any harm in
>> keeping the loop?
>
> A loop there might be safer to keep, indeed. That's basically for a
> difference kind of race, and it can still happen theoretically.
>
> Though, SOF is with the threaded interrupt, and it's interesting how
> the behavior differs. I can imagine that, if a thread irq is running
> while a new IRQ is re-triggered, the hard irq handler won't queue it
> again. But I might be wrong here, need some checks.
IIRC we added this loop before merging all interrupt handling in one
thread, somehow the MSI mode never worked reliably without this change,
so maybe we don't need this loop any longer.
I'd really prefer it if we didn't tie the RIRB handing change to this
loop change, removing the loop should only be done with *a lot of testing*.
next prev parent reply other threads:[~2020-06-11 20:38 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-11 13:44 [PATCH] ASoC: SOF: Intel: hda: unsolicited RIRB response Brent Lu
2020-06-11 14:26 ` Ranjani Sridharan
2020-06-11 17:09 ` Lu, Brent
2020-06-11 17:09 ` Lu, Brent
2020-06-11 17:59 ` Takashi Iwai
2020-06-11 17:59 ` Takashi Iwai
2020-06-11 18:12 ` Ranjani Sridharan
2020-06-11 18:12 ` Ranjani Sridharan
2020-06-11 20:14 ` Takashi Iwai
2020-06-11 20:14 ` Takashi Iwai
2020-06-11 20:36 ` Pierre-Louis Bossart [this message]
2020-06-11 20:36 ` Pierre-Louis Bossart
2020-06-11 23:33 ` Lu, Brent
2020-06-11 23:33 ` Lu, Brent
2020-06-12 6:15 ` Lu, Brent
2020-06-12 6:15 ` Lu, Brent
2020-06-11 18:01 ` Pierre-Louis Bossart
2020-06-11 18:01 ` Pierre-Louis Bossart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b7e98ae0-ea42-bced-1c0f-caa73e798908@linux.intel.com \
--to=pierre-louis.bossart@linux.intel.com \
--cc=alsa-devel@alsa-project.org \
--cc=brent.lu@intel.com \
--cc=broonie@kernel.org \
--cc=cezary.rojewski@intel.com \
--cc=daniel.baluta@nxp.com \
--cc=kai.vehmanen@linux.intel.com \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=ranjani.sridharan@linux.intel.com \
--cc=sound-open-firmware@alsa-project.orgDRIVERS \
--cc=tiwai@suse.com \
--cc=tiwai@suse.de \
--cc=yang.jie@linux.intel.com \
--cc=yingjiang.zhu@linux.intel.com \
--cc=yung-chuan.liao@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.