| Message ID | 20220818141517.109280-3-amadeuszx.slawinski@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hda: Minor cleanups | expand |
On 18/08/2022 15:15, Amadeusz Sławiński wrote: > We can use existing macros to poll and update register values instead of > open coding the functionality. > > Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> > Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > --- > sound/hda/hdac_stream.c | 26 ++++++-------------------- > 1 file changed, 6 insertions(+), 20 deletions(-) > > diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c > index f3582012d22f..bdf6d4db6769 100644 > --- a/sound/hda/hdac_stream.c > +++ b/sound/hda/hdac_stream.c > @@ -165,7 +165,6 @@ EXPORT_SYMBOL_GPL(snd_hdac_stop_streams_and_chip); > void snd_hdac_stream_reset(struct hdac_stream *azx_dev) > { > unsigned char val; > - int timeout; > int dma_run_state; > > snd_hdac_stream_clear(azx_dev); > @@ -173,30 +172,17 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) > dma_run_state = snd_hdac_stream_readb(azx_dev, SD_CTL) & SD_CTL_DMA_START; > > snd_hdac_stream_updateb(azx_dev, SD_CTL, 0, SD_CTL_STREAM_RESET); > - udelay(3); > - timeout = 300; > - do { > - val = snd_hdac_stream_readb(azx_dev, SD_CTL) & > - SD_CTL_STREAM_RESET; > - if (val) > - break; > - } while (--timeout); > + > + /* wait for hardware to report that the stream entered reset */ > + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, (val & SD_CTL_STREAM_RESET), 3, 300); > > if (azx_dev->bus->dma_stop_delay && dma_run_state) > udelay(azx_dev->bus->dma_stop_delay); > > - val &= ~SD_CTL_STREAM_RESET; > - snd_hdac_stream_writeb(azx_dev, SD_CTL, val); > - udelay(3); > + snd_hdac_stream_updateb(azx_dev, SD_CTL, SD_CTL_STREAM_RESET, 0); > > - timeout = 300; > - /* waiting for hardware to report that the stream is out of reset */ > - do { > - val = snd_hdac_stream_readb(azx_dev, SD_CTL) & > - SD_CTL_STREAM_RESET; > - if (!val) > - break; > - } while (--timeout); > + /* wait for hardware to report that the stream is out of reset */ > + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, !(val & SD_CTL_STREAM_RESET), 3, 300); > > /* reset first position - may not be synced with hw at this time */ > if (azx_dev->posbuf) HDA playback is failing on -next for various Tegra boards. Bisect is point to this commit and reverting it fixes the problem. I was a bit puzzled why this change is causing a problem, but looking closer there is a difference between the previous code that was calling snd_hdac_stream_readb() and the new code that is calling snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO which Tegra does and then would call snd_hdac_aligned_read(). However, now the code always call readb() and this is breaking Tegra. So it is either necessary to update snd_hdac_stream_readb_poll() to handle this or revert this change. Cheers Jon
On Wed, 05 Oct 2022 14:10:04 +0200, Jon Hunter wrote: > > > On 18/08/2022 15:15, Amadeusz Sławiński wrote: > > We can use existing macros to poll and update register values instead of > > open coding the functionality. > > > > Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> > > Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > > --- > > sound/hda/hdac_stream.c | 26 ++++++-------------------- > > 1 file changed, 6 insertions(+), 20 deletions(-) > > > > diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c > > index f3582012d22f..bdf6d4db6769 100644 > > --- a/sound/hda/hdac_stream.c > > +++ b/sound/hda/hdac_stream.c > > @@ -165,7 +165,6 @@ EXPORT_SYMBOL_GPL(snd_hdac_stop_streams_and_chip); > > void snd_hdac_stream_reset(struct hdac_stream *azx_dev) > > { > > unsigned char val; > > - int timeout; > > int dma_run_state; > > snd_hdac_stream_clear(azx_dev); > > @@ -173,30 +172,17 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) > > dma_run_state = snd_hdac_stream_readb(azx_dev, SD_CTL) & SD_CTL_DMA_START; > > snd_hdac_stream_updateb(azx_dev, SD_CTL, 0, > > SD_CTL_STREAM_RESET); > > - udelay(3); > > - timeout = 300; > > - do { > > - val = snd_hdac_stream_readb(azx_dev, SD_CTL) & > > - SD_CTL_STREAM_RESET; > > - if (val) > > - break; > > - } while (--timeout); > > + > > + /* wait for hardware to report that the stream entered reset */ > > + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, (val & SD_CTL_STREAM_RESET), 3, 300); > > if (azx_dev->bus->dma_stop_delay && dma_run_state) > > udelay(azx_dev->bus->dma_stop_delay); > > - val &= ~SD_CTL_STREAM_RESET; > > - snd_hdac_stream_writeb(azx_dev, SD_CTL, val); > > - udelay(3); > > + snd_hdac_stream_updateb(azx_dev, SD_CTL, SD_CTL_STREAM_RESET, 0); > > - timeout = 300; > > - /* waiting for hardware to report that the stream is out of reset */ > > - do { > > - val = snd_hdac_stream_readb(azx_dev, SD_CTL) & > > - SD_CTL_STREAM_RESET; > > - if (!val) > > - break; > > - } while (--timeout); > > + /* wait for hardware to report that the stream is out of reset */ > > + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, !(val & SD_CTL_STREAM_RESET), 3, 300); > > /* reset first position - may not be synced with hw at this > > time */ > > if (azx_dev->posbuf) > > > HDA playback is failing on -next for various Tegra boards. Bisect is > point to this commit and reverting it fixes the problem. I was a bit > puzzled why this change is causing a problem, but looking closer there > is a difference between the previous code that was calling > snd_hdac_stream_readb() and the new code that is calling > snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() > calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO > which Tegra does and then would call snd_hdac_aligned_read(). However, > now the code always call readb() and this is breaking Tegra. > > So it is either necessary to update snd_hdac_stream_readb_poll() to > handle this or revert this change. Does the patch below work? thanks, Takashi -- 8< -- --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus, #define snd_hdac_stream_readb(dev, reg) \ snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \ - readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ - delay_us, timeout_us) + read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, timeout_us,\ + false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readl_poll(dev, reg, val, cond, delay_us, timeout_us) \ readl_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ delay_us, timeout_us)
On 05/10/2022 13:29, Takashi Iwai wrote: ... >> HDA playback is failing on -next for various Tegra boards. Bisect is >> point to this commit and reverting it fixes the problem. I was a bit >> puzzled why this change is causing a problem, but looking closer there >> is a difference between the previous code that was calling >> snd_hdac_stream_readb() and the new code that is calling >> snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() >> calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO >> which Tegra does and then would call snd_hdac_aligned_read(). However, >> now the code always call readb() and this is breaking Tegra. >> >> So it is either necessary to update snd_hdac_stream_readb_poll() to >> handle this or revert this change. > > Does the patch below work? > > > thanks, > > Takashi > > -- 8< -- > --- a/include/sound/hdaudio.h > +++ b/include/sound/hdaudio.h > @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus, > #define snd_hdac_stream_readb(dev, reg) \ > snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) > #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \ > - readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ > - delay_us, timeout_us) > + read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, timeout_us,\ > + false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) > #define snd_hdac_stream_readl_poll(dev, reg, val, cond, delay_us, timeout_us) \ > readl_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ > delay_us, timeout_us) Amazingly it does not work. I would have thought that would, but it does not. I am a bit puzzled by that? Jon
On Wed, 05 Oct 2022 15:52:01 +0200, Jon Hunter wrote: > > > On 05/10/2022 13:29, Takashi Iwai wrote: > > ... > > >> HDA playback is failing on -next for various Tegra boards. Bisect is > >> point to this commit and reverting it fixes the problem. I was a bit > >> puzzled why this change is causing a problem, but looking closer there > >> is a difference between the previous code that was calling > >> snd_hdac_stream_readb() and the new code that is calling > >> snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() > >> calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO > >> which Tegra does and then would call snd_hdac_aligned_read(). However, > >> now the code always call readb() and this is breaking Tegra. > >> > >> So it is either necessary to update snd_hdac_stream_readb_poll() to > >> handle this or revert this change. > > > > Does the patch below work? > > > > > > thanks, > > > > Takashi > > > > -- 8< -- > > --- a/include/sound/hdaudio.h > > +++ b/include/sound/hdaudio.h > > @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus, > > #define snd_hdac_stream_readb(dev, reg) \ > > snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) > > #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \ > > - readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ > > - delay_us, timeout_us) > > + read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, timeout_us,\ > > + false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) > > #define snd_hdac_stream_readl_poll(dev, reg, val, cond, delay_us, timeout_us) \ > > readl_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ > > delay_us, timeout_us) > > > Amazingly it does not work. I would have thought that would, but it > does not. I am a bit puzzled by that? Interesting, it must be a subtle difference. What about passing true? It seems that the original code has the udelay(3) before the loop. Takashi -- 8< -- --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus, #define snd_hdac_stream_readb(dev, reg) \ snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \ - readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ - delay_us, timeout_us) + read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, timeout_us,\ + true, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readl_poll(dev, reg, val, cond, delay_us, timeout_us) \ readl_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ delay_us, timeout_us)
On 05/10/2022 15:07, Takashi Iwai wrote: > On Wed, 05 Oct 2022 15:52:01 +0200, > Jon Hunter wrote: >> >> >> On 05/10/2022 13:29, Takashi Iwai wrote: >> >> ... >> >>>> HDA playback is failing on -next for various Tegra boards. Bisect is >>>> point to this commit and reverting it fixes the problem. I was a bit >>>> puzzled why this change is causing a problem, but looking closer there >>>> is a difference between the previous code that was calling >>>> snd_hdac_stream_readb() and the new code that is calling >>>> snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() >>>> calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO >>>> which Tegra does and then would call snd_hdac_aligned_read(). However, >>>> now the code always call readb() and this is breaking Tegra. >>>> >>>> So it is either necessary to update snd_hdac_stream_readb_poll() to >>>> handle this or revert this change. >>> >>> Does the patch below work? >>> >>> >>> thanks, >>> >>> Takashi >>> >>> -- 8< -- >>> --- a/include/sound/hdaudio.h >>> +++ b/include/sound/hdaudio.h >>> @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus, >>> #define snd_hdac_stream_readb(dev, reg) \ >>> snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) >>> #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \ >>> - readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ >>> - delay_us, timeout_us) >>> + read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, timeout_us,\ >>> + false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) >>> #define snd_hdac_stream_readl_poll(dev, reg, val, cond, delay_us, timeout_us) \ >>> readl_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ >>> delay_us, timeout_us) >> >> >> Amazingly it does not work. I would have thought that would, but it >> does not. I am a bit puzzled by that? > > Interesting, it must be a subtle difference. > What about passing true? It seems that the original code has the > udelay(3) before the loop. I wondered the same and tried that, but still not working. Jon
On 10/5/2022 4:26 PM, Jon Hunter wrote: > > > On 05/10/2022 15:07, Takashi Iwai wrote: >> On Wed, 05 Oct 2022 15:52:01 +0200, >> Jon Hunter wrote: >>> >>> >>> On 05/10/2022 13:29, Takashi Iwai wrote: >>> >>> ... >>> >>>>> HDA playback is failing on -next for various Tegra boards. Bisect is >>>>> point to this commit and reverting it fixes the problem. I was a bit >>>>> puzzled why this change is causing a problem, but looking closer there >>>>> is a difference between the previous code that was calling >>>>> snd_hdac_stream_readb() and the new code that is calling >>>>> snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() >>>>> calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO >>>>> which Tegra does and then would call snd_hdac_aligned_read(). However, >>>>> now the code always call readb() and this is breaking Tegra. >>>>> >>>>> So it is either necessary to update snd_hdac_stream_readb_poll() to >>>>> handle this or revert this change. >>>> >>>> Does the patch below work? >>>> >>>> >>>> thanks, >>>> >>>> Takashi >>>> >>>> -- 8< -- >>>> --- a/include/sound/hdaudio.h >>>> +++ b/include/sound/hdaudio.h >>>> @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct >>>> hdac_bus *bus, >>>> #define snd_hdac_stream_readb(dev, reg) \ >>>> snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) >>>> #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, >>>> timeout_us) \ >>>> - readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ >>>> - delay_us, timeout_us) >>>> + read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, >>>> timeout_us,\ >>>> + false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) >>>> #define snd_hdac_stream_readl_poll(dev, reg, val, cond, delay_us, >>>> timeout_us) \ >>>> readl_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, >>>> cond, \ >>>> delay_us, timeout_us) >>> >>> >>> Amazingly it does not work. I would have thought that would, but it >>> does not. I am a bit puzzled by that? >> >> Interesting, it must be a subtle difference. >> What about passing true? It seems that the original code has the >> udelay(3) before the loop. > > > I wondered the same and tried that, but still not working. > > Jon > Well in worse case we can revert the patch in question, but I would like to get it working... Maybe also try to raise timeout to 1000, as what original code called timeout, was actually number of retries? So 300 * udelay(3) which is more or less 900us, so we can round it up for test? I mean, something like: --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -176,7 +176,7 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) snd_hdac_stream_updateb(azx_dev, SD_CTL, 0, SD_CTL_STREAM_RESET); /* wait for hardware to report that the stream entered reset */ - snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, (val & SD_CTL_STREAM_RESET), 3, 300); + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, (val & SD_CTL_STREAM_RESET), 3, 1000); if (azx_dev->bus->dma_stop_delay && dma_run_state) udelay(azx_dev->bus->dma_stop_delay); @@ -184,7 +184,7 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) snd_hdac_stream_updateb(azx_dev, SD_CTL, SD_CTL_STREAM_RESET, 0); /* wait for hardware to report that the stream is out of reset */ - snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, !(val & SD_CTL_STREAM_RESET), 3, 300); + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, !(val & SD_CTL_STREAM_RESET), 3, 1000); /* reset first position - may not be synced with hw at this time */ if (azx_dev->posbuf) in addition to Takashi suggestion?
On 05/10/2022 15:47, Amadeusz Sławiński wrote: ... > Well in worse case we can revert the patch in question, but I would like > to get it working... > > Maybe also try to raise timeout to 1000, as what original code called > timeout, was actually number of retries? So 300 * udelay(3) which is > more or less 900us, so we can round it up for test? > > I mean, something like: > > --- a/sound/hda/hdac_stream.c > +++ b/sound/hda/hdac_stream.c > @@ -176,7 +176,7 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) > snd_hdac_stream_updateb(azx_dev, SD_CTL, 0, SD_CTL_STREAM_RESET); > > /* wait for hardware to report that the stream entered reset */ > - snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, (val & > SD_CTL_STREAM_RESET), 3, 300); > + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, (val & > SD_CTL_STREAM_RESET), 3, 1000); > > if (azx_dev->bus->dma_stop_delay && dma_run_state) > udelay(azx_dev->bus->dma_stop_delay); > @@ -184,7 +184,7 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) > snd_hdac_stream_updateb(azx_dev, SD_CTL, SD_CTL_STREAM_RESET, 0); > > /* wait for hardware to report that the stream is out of reset */ > - snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, !(val & > SD_CTL_STREAM_RESET), 3, 300); > + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, !(val & > SD_CTL_STREAM_RESET), 3, 1000); > > /* reset first position - may not be synced with hw at this > time */ > if (azx_dev->posbuf) > > > in addition to Takashi suggestion? Thanks. Tried that on top of Takaski's patch but still not working :-( Jon
On 05/10/2022 13:29, Takashi Iwai wrote: ... >> HDA playback is failing on -next for various Tegra boards. Bisect is >> point to this commit and reverting it fixes the problem. I was a bit >> puzzled why this change is causing a problem, but looking closer there >> is a difference between the previous code that was calling >> snd_hdac_stream_readb() and the new code that is calling >> snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() >> calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO >> which Tegra does and then would call snd_hdac_aligned_read(). However, >> now the code always call readb() and this is breaking Tegra. >> >> So it is either necessary to update snd_hdac_stream_readb_poll() to >> handle this or revert this change. > > Does the patch below work? > > > thanks, > > Takashi > > -- 8< -- > --- a/include/sound/hdaudio.h > +++ b/include/sound/hdaudio.h > @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus, > #define snd_hdac_stream_readb(dev, reg) \ > snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) > #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \ > - readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ > - delay_us, timeout_us) > + read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, timeout_us,\ > + false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) > #define snd_hdac_stream_readl_poll(dev, reg, val, cond, delay_us, timeout_us) \ > readl_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ > delay_us, timeout_us) So looking at this a bit more I see ... [ 199.422773] bad: scheduling from the idle thread! [ 199.427610] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G D C 6.0.0-rc7-next-20220930-00007-gd6ae4ed0a78f-dirty #23 [ 199.438880] Hardware name: NVIDIA Jetson Nano Developer Kit (DT) [ 199.444899] Call trace: [ 199.447357] dump_backtrace.part.7+0xe8/0xf8 [ 199.451680] show_stack+0x14/0x38 [ 199.455024] dump_stack_lvl+0x64/0x7c [ 199.458715] dump_stack+0x14/0x2c [ 199.462067] dequeue_task_idle+0x2c/0x58 [ 199.466038] dequeue_task+0x38/0x98 [ 199.469565] __schedule+0x4a4/0x6d8 [ 199.473104] schedule+0x58/0xc0 [ 199.476292] schedule_hrtimeout_range_clock+0x98/0x108 [ 199.481472] schedule_hrtimeout_range+0x10/0x18 [ 199.486039] usleep_range_state+0x7c/0xb0 [ 199.490084] snd_hdac_stream_reset+0xe8/0x328 [snd_hda_core] [ 199.495913] snd_hdac_stream_sync+0xe4/0x190 [snd_hda_core] [ 199.501627] azx_pcm_trigger+0x1d8/0x250 [snd_hda_codec] [ 199.507109] snd_pcm_do_stop+0x54/0x70 [ 199.510904] snd_pcm_action_single+0x44/0xa0 [ 199.515215] snd_pcm_drain_done+0x20/0x28 [ 199.519267] snd_pcm_update_state+0x114/0x128 [ 199.523670] snd_pcm_update_hw_ptr0+0x22c/0x3a0 [ 199.528246] snd_pcm_period_elapsed_under_stream_lock+0x44/0x88 [ 199.534216] snd_pcm_period_elapsed+0x24/0x48 [ 199.538617] stream_update+0x3c/0x50 [snd_hda_codec] [ 199.543737] snd_hdac_bus_handle_stream_irq+0xe8/0x150 [snd_hda_core] [ 199.550320] azx_interrupt+0xb4/0x1b0 [snd_hda_codec] [ 199.555524] __handle_irq_event_percpu+0x74/0x140 [ 199.560281] handle_irq_event_percpu+0x14/0x48 [ 199.564772] handle_irq_event+0x44/0x88 [ 199.568653] handle_fasteoi_irq+0xa8/0x130 [ 199.572788] generic_handle_domain_irq+0x28/0x40 [ 199.577452] gic_handle_irq+0x9c/0xb8 [ 199.581168] call_on_irq_stack+0x2c/0x40 [ 199.585129] do_interrupt_handler+0x7c/0x80 [ 199.589355] el1_interrupt+0x34/0x68 [ 199.592969] el1h_64_irq_handler+0x14/0x20 [ 199.597107] el1h_64_irq+0x64/0x68 [ 199.600540] cpuidle_enter_state+0x130/0x2f8 [ 199.604853] cpuidle_enter+0x38/0x50 [ 199.608463] call_cpuidle+0x18/0x38 [ 199.611991] do_idle+0x1f8/0x248 [ 199.615259] cpu_startup_entry+0x20/0x28 [ 199.619224] kernel_init+0x0/0x128 [ 199.622669] arch_post_acpi_subsys_init+0x0/0x8 [ 199.627240] start_kernel+0x630/0x668 [ 199.630933] __primary_switched+0xb4/0xbc If I change your patch to be read_poll_timeout_atomic, then it works \o/ Can we make that update? Jon
On 10/6/2022 10:45 AM, Jon Hunter wrote: > > On 05/10/2022 13:29, Takashi Iwai wrote: > > ... > >>> HDA playback is failing on -next for various Tegra boards. Bisect is >>> point to this commit and reverting it fixes the problem. I was a bit >>> puzzled why this change is causing a problem, but looking closer there >>> is a difference between the previous code that was calling >>> snd_hdac_stream_readb() and the new code that is calling >>> snd_hdac_stream_readb_poll(). The function snd_hdac_stream_readb() >>> calls snd_hdac_aligned_mmio() is see if the device has an aligned MMIO >>> which Tegra does and then would call snd_hdac_aligned_read(). However, >>> now the code always call readb() and this is breaking Tegra. >>> >>> So it is either necessary to update snd_hdac_stream_readb_poll() to >>> handle this or revert this change. >> >> Does the patch below work? >> >> >> thanks, >> >> Takashi >> >> -- 8< -- >> --- a/include/sound/hdaudio.h >> +++ b/include/sound/hdaudio.h >> @@ -592,8 +592,8 @@ int snd_hdac_get_stream_stripe_ctl(struct hdac_bus >> *bus, >> #define snd_hdac_stream_readb(dev, reg) \ >> snd_hdac_reg_readb((dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) >> #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, >> timeout_us) \ >> - readb_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ >> - delay_us, timeout_us) >> + read_poll_timeout(snd_hdac_reg_readb, val, cond, delay_us, >> timeout_us,\ >> + false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) >> #define snd_hdac_stream_readl_poll(dev, reg, val, cond, delay_us, >> timeout_us) \ >> readl_poll_timeout((dev)->sd_addr + AZX_REG_ ## reg, val, cond, \ >> delay_us, timeout_us) > > > So looking at this a bit more I see ... > > [ 199.422773] bad: scheduling from the idle thread! > [ 199.427610] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G D > C 6.0.0-rc7-next-20220930-00007-gd6ae4ed0a78f-dirty #23 > [ 199.438880] Hardware name: NVIDIA Jetson Nano Developer Kit (DT) > [ 199.444899] Call trace: > [ 199.447357] dump_backtrace.part.7+0xe8/0xf8 > [ 199.451680] show_stack+0x14/0x38 > [ 199.455024] dump_stack_lvl+0x64/0x7c > [ 199.458715] dump_stack+0x14/0x2c > [ 199.462067] dequeue_task_idle+0x2c/0x58 > [ 199.466038] dequeue_task+0x38/0x98 > [ 199.469565] __schedule+0x4a4/0x6d8 > [ 199.473104] schedule+0x58/0xc0 > [ 199.476292] schedule_hrtimeout_range_clock+0x98/0x108 > [ 199.481472] schedule_hrtimeout_range+0x10/0x18 > [ 199.486039] usleep_range_state+0x7c/0xb0 > [ 199.490084] snd_hdac_stream_reset+0xe8/0x328 [snd_hda_core] > [ 199.495913] snd_hdac_stream_sync+0xe4/0x190 [snd_hda_core] > [ 199.501627] azx_pcm_trigger+0x1d8/0x250 [snd_hda_codec] > [ 199.507109] snd_pcm_do_stop+0x54/0x70 > [ 199.510904] snd_pcm_action_single+0x44/0xa0 > [ 199.515215] snd_pcm_drain_done+0x20/0x28 > [ 199.519267] snd_pcm_update_state+0x114/0x128 > [ 199.523670] snd_pcm_update_hw_ptr0+0x22c/0x3a0 > [ 199.528246] snd_pcm_period_elapsed_under_stream_lock+0x44/0x88 > [ 199.534216] snd_pcm_period_elapsed+0x24/0x48 > [ 199.538617] stream_update+0x3c/0x50 [snd_hda_codec] > [ 199.543737] snd_hdac_bus_handle_stream_irq+0xe8/0x150 [snd_hda_core] > [ 199.550320] azx_interrupt+0xb4/0x1b0 [snd_hda_codec] > [ 199.555524] __handle_irq_event_percpu+0x74/0x140 > [ 199.560281] handle_irq_event_percpu+0x14/0x48 > [ 199.564772] handle_irq_event+0x44/0x88 > [ 199.568653] handle_fasteoi_irq+0xa8/0x130 > [ 199.572788] generic_handle_domain_irq+0x28/0x40 > [ 199.577452] gic_handle_irq+0x9c/0xb8 > [ 199.581168] call_on_irq_stack+0x2c/0x40 > [ 199.585129] do_interrupt_handler+0x7c/0x80 > [ 199.589355] el1_interrupt+0x34/0x68 > [ 199.592969] el1h_64_irq_handler+0x14/0x20 > [ 199.597107] el1h_64_irq+0x64/0x68 > [ 199.600540] cpuidle_enter_state+0x130/0x2f8 > [ 199.604853] cpuidle_enter+0x38/0x50 > [ 199.608463] call_cpuidle+0x18/0x38 > [ 199.611991] do_idle+0x1f8/0x248 > [ 199.615259] cpu_startup_entry+0x20/0x28 > [ 199.619224] kernel_init+0x0/0x128 > [ 199.622669] arch_post_acpi_subsys_init+0x0/0x8 > [ 199.627240] start_kernel+0x630/0x668 > [ 199.630933] __primary_switched+0xb4/0xbc > > > If I change your patch to be read_poll_timeout_atomic, then it works \o/ > > Can we make that update? > > Jon > Yes, it makes sense, as it uses udelay instead of usleep, same as original code. I've send patch which updates the macros. It passed validation on our side. Thanks for report! Amadeusz
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c index f3582012d22f..bdf6d4db6769 100644 --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -165,7 +165,6 @@ EXPORT_SYMBOL_GPL(snd_hdac_stop_streams_and_chip); void snd_hdac_stream_reset(struct hdac_stream *azx_dev) { unsigned char val; - int timeout; int dma_run_state; snd_hdac_stream_clear(azx_dev); @@ -173,30 +172,17 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev) dma_run_state = snd_hdac_stream_readb(azx_dev, SD_CTL) & SD_CTL_DMA_START; snd_hdac_stream_updateb(azx_dev, SD_CTL, 0, SD_CTL_STREAM_RESET); - udelay(3); - timeout = 300; - do { - val = snd_hdac_stream_readb(azx_dev, SD_CTL) & - SD_CTL_STREAM_RESET; - if (val) - break; - } while (--timeout); + + /* wait for hardware to report that the stream entered reset */ + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, (val & SD_CTL_STREAM_RESET), 3, 300); if (azx_dev->bus->dma_stop_delay && dma_run_state) udelay(azx_dev->bus->dma_stop_delay); - val &= ~SD_CTL_STREAM_RESET; - snd_hdac_stream_writeb(azx_dev, SD_CTL, val); - udelay(3); + snd_hdac_stream_updateb(azx_dev, SD_CTL, SD_CTL_STREAM_RESET, 0); - timeout = 300; - /* waiting for hardware to report that the stream is out of reset */ - do { - val = snd_hdac_stream_readb(azx_dev, SD_CTL) & - SD_CTL_STREAM_RESET; - if (!val) - break; - } while (--timeout); + /* wait for hardware to report that the stream is out of reset */ + snd_hdac_stream_readb_poll(azx_dev, SD_CTL, val, !(val & SD_CTL_STREAM_RESET), 3, 300); /* reset first position - may not be synced with hw at this time */ if (azx_dev->posbuf)