* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros [not found] ` <20220818141517.109280-3-amadeuszx.slawinski@linux.intel.com> @ 2022-10-05 12:10 ` Jon Hunter 2022-10-05 12:29 ` Takashi Iwai 0 siblings, 1 reply; 9+ messages in thread From: Jon Hunter @ 2022-10-05 12:10 UTC (permalink / raw) To: Amadeusz Sławiński, Takashi Iwai, alsa-devel Cc: linux-kernel, Cezary Rojewski, Pierre-Louis Bossart, Jaroslav Kysela, linux-tegra@vger.kernel.org, Mohan Kumar D 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 -- nvpublic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros 2022-10-05 12:10 ` [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros Jon Hunter @ 2022-10-05 12:29 ` Takashi Iwai 2022-10-05 13:52 ` Jon Hunter 2022-10-06 8:45 ` Jon Hunter 0 siblings, 2 replies; 9+ messages in thread From: Takashi Iwai @ 2022-10-05 12:29 UTC (permalink / raw) To: Jon Hunter Cc: Amadeusz Sławiński, Takashi Iwai, alsa-devel, linux-kernel, Cezary Rojewski, Pierre-Louis Bossart, Jaroslav Kysela, linux-tegra@vger.kernel.org, Mohan Kumar D 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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros 2022-10-05 12:29 ` Takashi Iwai @ 2022-10-05 13:52 ` Jon Hunter 2022-10-05 14:07 ` Takashi Iwai 2022-10-06 8:45 ` Jon Hunter 1 sibling, 1 reply; 9+ messages in thread From: Jon Hunter @ 2022-10-05 13:52 UTC (permalink / raw) To: Takashi Iwai Cc: Amadeusz Sławiński, Takashi Iwai, alsa-devel, linux-kernel, Cezary Rojewski, Pierre-Louis Bossart, Jaroslav Kysela, linux-tegra@vger.kernel.org, Mohan Kumar D 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 -- nvpublic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros 2022-10-05 13:52 ` Jon Hunter @ 2022-10-05 14:07 ` Takashi Iwai 2022-10-05 14:26 ` Jon Hunter 0 siblings, 1 reply; 9+ messages in thread From: Takashi Iwai @ 2022-10-05 14:07 UTC (permalink / raw) To: Jon Hunter Cc: Amadeusz Sławiński, Takashi Iwai, alsa-devel, linux-kernel, Cezary Rojewski, Pierre-Louis Bossart, Jaroslav Kysela, linux-tegra@vger.kernel.org, Mohan Kumar D 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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros 2022-10-05 14:07 ` Takashi Iwai @ 2022-10-05 14:26 ` Jon Hunter 2022-10-05 14:47 ` Amadeusz Sławiński 0 siblings, 1 reply; 9+ messages in thread From: Jon Hunter @ 2022-10-05 14:26 UTC (permalink / raw) To: Takashi Iwai Cc: Amadeusz Sławiński, Takashi Iwai, alsa-devel, linux-kernel, Cezary Rojewski, Pierre-Louis Bossart, Jaroslav Kysela, linux-tegra@vger.kernel.org, Mohan Kumar D 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 -- nvpublic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros 2022-10-05 14:26 ` Jon Hunter @ 2022-10-05 14:47 ` Amadeusz Sławiński 2022-10-05 20:16 ` Jon Hunter 0 siblings, 1 reply; 9+ messages in thread From: Amadeusz Sławiński @ 2022-10-05 14:47 UTC (permalink / raw) To: Jon Hunter, Takashi Iwai Cc: alsa-devel, Cezary Rojewski, Takashi Iwai, linux-kernel, Pierre-Louis Bossart, linux-tegra@vger.kernel.org, Mohan Kumar D 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros 2022-10-05 14:47 ` Amadeusz Sławiński @ 2022-10-05 20:16 ` Jon Hunter 0 siblings, 0 replies; 9+ messages in thread From: Jon Hunter @ 2022-10-05 20:16 UTC (permalink / raw) To: Amadeusz Sławiński, Takashi Iwai Cc: alsa-devel, Cezary Rojewski, Takashi Iwai, linux-kernel, Pierre-Louis Bossart, linux-tegra@vger.kernel.org, Mohan Kumar D 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 -- nvpublic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros 2022-10-05 12:29 ` Takashi Iwai 2022-10-05 13:52 ` Jon Hunter @ 2022-10-06 8:45 ` Jon Hunter 2022-10-07 8:49 ` Amadeusz Sławiński 1 sibling, 1 reply; 9+ messages in thread From: Jon Hunter @ 2022-10-06 8:45 UTC (permalink / raw) To: Takashi Iwai Cc: Amadeusz Sławiński, Takashi Iwai, alsa-devel, linux-kernel, Cezary Rojewski, Pierre-Louis Bossart, Jaroslav Kysela, linux-tegra@vger.kernel.org, Mohan Kumar D 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 -- nvpublic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros 2022-10-06 8:45 ` Jon Hunter @ 2022-10-07 8:49 ` Amadeusz Sławiński 0 siblings, 0 replies; 9+ messages in thread From: Amadeusz Sławiński @ 2022-10-07 8:49 UTC (permalink / raw) To: Jon Hunter, Takashi Iwai Cc: alsa-devel, Cezary Rojewski, Takashi Iwai, linux-kernel, Pierre-Louis Bossart, linux-tegra@vger.kernel.org, Mohan Kumar D 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-10-07 8:50 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220818141517.109280-1-amadeuszx.slawinski@linux.intel.com>
[not found] ` <20220818141517.109280-3-amadeuszx.slawinski@linux.intel.com>
2022-10-05 12:10 ` [PATCH v2 2/4] ALSA: hda: Rework snd_hdac_stream_reset() to use macros Jon Hunter
2022-10-05 12:29 ` Takashi Iwai
2022-10-05 13:52 ` Jon Hunter
2022-10-05 14:07 ` Takashi Iwai
2022-10-05 14:26 ` Jon Hunter
2022-10-05 14:47 ` Amadeusz Sławiński
2022-10-05 20:16 ` Jon Hunter
2022-10-06 8:45 ` Jon Hunter
2022-10-07 8:49 ` Amadeusz Sławiński
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).