| Message ID | 20200618144051.7415-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda: Enable sync-write operation as default for all controllers | expand |
Hi Takashi, On 18/06/2020 15:40, Takashi Iwai wrote: > In the end we already enabled the sync-write mode for most of HD-audio > controllers including Intel, and it's no big merit to keep the async > write mode for the rest. Let's make it as default and drop the > superfluous AZX_DCAPS_SYNC_WRITE bit flag. > > Also, avoid to set the allow_bus_reset flag, which is a quite unstable > and hackish behavior that was needed only for some early platforms > (decades ago). The straight fallback to the single cmd mode is more > robust. > > Signed-off-by: Takashi Iwai <tiwai@suse.de> I have noticed a regression in HDA playback on our Tegra186 Jetson TX2 platform. Bisect is pointing to this patch and reverting this does appear to fix it. Interestingly, I am not seeing any problems on other Tegra platforms, however, Tegra186 does have the IOMMU enabled for HDA which is one different between the other platforms. We can take a closer look at this for Tegra, but I am wondering if we revert this for Tegra for now. Cheers Jon
On 14/07/2020 09:08, Jon Hunter wrote: > Hi Takashi, > > On 18/06/2020 15:40, Takashi Iwai wrote: >> In the end we already enabled the sync-write mode for most of HD-audio >> controllers including Intel, and it's no big merit to keep the async >> write mode for the rest. Let's make it as default and drop the >> superfluous AZX_DCAPS_SYNC_WRITE bit flag. >> >> Also, avoid to set the allow_bus_reset flag, which is a quite unstable >> and hackish behavior that was needed only for some early platforms >> (decades ago). The straight fallback to the single cmd mode is more >> robust. >> >> Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > I have noticed a regression in HDA playback on our Tegra186 Jetson TX2 > platform. Bisect is pointing to this patch and reverting this does > appear to fix it. Interestingly, I am not seeing any problems on other > Tegra platforms, however, Tegra186 does have the IOMMU enabled for HDA > which is one different between the other platforms. > > We can take a closer look at this for Tegra, but I am wondering if we > revert this for Tegra for now. By revert, I don't mean revert the entire change, but just disable the sync-write for Tegra for now. Jon
On Tue, 14 Jul 2020 10:08:02 +0200, Jon Hunter wrote: > > Hi Takashi, > > On 18/06/2020 15:40, Takashi Iwai wrote: > > In the end we already enabled the sync-write mode for most of HD-audio > > controllers including Intel, and it's no big merit to keep the async > > write mode for the rest. Let's make it as default and drop the > > superfluous AZX_DCAPS_SYNC_WRITE bit flag. > > > > Also, avoid to set the allow_bus_reset flag, which is a quite unstable > > and hackish behavior that was needed only for some early platforms > > (decades ago). The straight fallback to the single cmd mode is more > > robust. > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > I have noticed a regression in HDA playback on our Tegra186 Jetson TX2 > platform. Bisect is pointing to this patch and reverting this does > appear to fix it. Interestingly, I am not seeing any problems on other > Tegra platforms, however, Tegra186 does have the IOMMU enabled for HDA > which is one different between the other platforms. > > We can take a closer look at this for Tegra, but I am wondering if we > revert this for Tegra for now. It's a deja vu, we (or someone else in Nvidia?) discussed it in the past? The patch below should cure the problem temporarily, as it sets the polling mode as default for Tegra. But it'd be appreciated if you can find the root cause. thanks, Takashi --- a/sound/pci/hda/hda_tegra.c +++ b/sound/pci/hda/hda_tegra.c @@ -394,6 +394,7 @@ static int hda_tegra_create(struct snd_card *card, if (err < 0) return err; + chip->bus.core.polling = 1; chip->bus.core.needs_damn_long_delay = 1; err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
On 14/07/2020 09:30, Takashi Iwai wrote: > On Tue, 14 Jul 2020 10:08:02 +0200, > Jon Hunter wrote: >> >> Hi Takashi, >> >> On 18/06/2020 15:40, Takashi Iwai wrote: >>> In the end we already enabled the sync-write mode for most of HD-audio >>> controllers including Intel, and it's no big merit to keep the async >>> write mode for the rest. Let's make it as default and drop the >>> superfluous AZX_DCAPS_SYNC_WRITE bit flag. >>> >>> Also, avoid to set the allow_bus_reset flag, which is a quite unstable >>> and hackish behavior that was needed only for some early platforms >>> (decades ago). The straight fallback to the single cmd mode is more >>> robust. >>> >>> Signed-off-by: Takashi Iwai <tiwai@suse.de> >> >> >> I have noticed a regression in HDA playback on our Tegra186 Jetson TX2 >> platform. Bisect is pointing to this patch and reverting this does >> appear to fix it. Interestingly, I am not seeing any problems on other >> Tegra platforms, however, Tegra186 does have the IOMMU enabled for HDA >> which is one different between the other platforms. >> >> We can take a closer look at this for Tegra, but I am wondering if we >> revert this for Tegra for now. > > It's a deja vu, we (or someone else in Nvidia?) discussed it in the > past? > > The patch below should cure the problem temporarily, as it sets the > polling mode as default for Tegra. But it'd be appreciated if you can > find the root cause. > > > thanks, > > Takashi > > --- a/sound/pci/hda/hda_tegra.c > +++ b/sound/pci/hda/hda_tegra.c > @@ -394,6 +394,7 @@ static int hda_tegra_create(struct snd_card *card, > if (err < 0) > return err; > > + chip->bus.core.polling = 1; > chip->bus.core.needs_damn_long_delay = 1; > > err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops); > Did you mean ... diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c index 0cc5fad1af8a..5637f0129932 100644 --- a/sound/pci/hda/hda_tegra.c +++ b/sound/pci/hda/hda_tegra.c @@ -443,6 +443,7 @@ static int hda_tegra_create(struct snd_card *card, if (err < 0) return err; + chip->bus.core.sync_write = 0; chip->bus.core.needs_damn_long_delay = 1; chip->bus.core.aligned_mmio = 1; The above works for me. Cheers Jon
On Tue, 14 Jul 2020 11:08:18 +0200, Jon Hunter wrote: > > > On 14/07/2020 09:30, Takashi Iwai wrote: > > On Tue, 14 Jul 2020 10:08:02 +0200, > > Jon Hunter wrote: > >> > >> Hi Takashi, > >> > >> On 18/06/2020 15:40, Takashi Iwai wrote: > >>> In the end we already enabled the sync-write mode for most of HD-audio > >>> controllers including Intel, and it's no big merit to keep the async > >>> write mode for the rest. Let's make it as default and drop the > >>> superfluous AZX_DCAPS_SYNC_WRITE bit flag. > >>> > >>> Also, avoid to set the allow_bus_reset flag, which is a quite unstable > >>> and hackish behavior that was needed only for some early platforms > >>> (decades ago). The straight fallback to the single cmd mode is more > >>> robust. > >>> > >>> Signed-off-by: Takashi Iwai <tiwai@suse.de> > >> > >> > >> I have noticed a regression in HDA playback on our Tegra186 Jetson TX2 > >> platform. Bisect is pointing to this patch and reverting this does > >> appear to fix it. Interestingly, I am not seeing any problems on other > >> Tegra platforms, however, Tegra186 does have the IOMMU enabled for HDA > >> which is one different between the other platforms. > >> > >> We can take a closer look at this for Tegra, but I am wondering if we > >> revert this for Tegra for now. > > > > It's a deja vu, we (or someone else in Nvidia?) discussed it in the > > past? > > > > The patch below should cure the problem temporarily, as it sets the > > polling mode as default for Tegra. But it'd be appreciated if you can > > find the root cause. > > > > > > thanks, > > > > Takashi > > > > --- a/sound/pci/hda/hda_tegra.c > > +++ b/sound/pci/hda/hda_tegra.c > > @@ -394,6 +394,7 @@ static int hda_tegra_create(struct snd_card *card, > > if (err < 0) > > return err; > > > > + chip->bus.core.polling = 1; > > chip->bus.core.needs_damn_long_delay = 1; > > > > err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops); > > > > Did you mean ... > > diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c > index 0cc5fad1af8a..5637f0129932 100644 > --- a/sound/pci/hda/hda_tegra.c > +++ b/sound/pci/hda/hda_tegra.c > @@ -443,6 +443,7 @@ static int hda_tegra_create(struct snd_card *card, > if (err < 0) > return err; > > + chip->bus.core.sync_write = 0; > chip->bus.core.needs_damn_long_delay = 1; > chip->bus.core.aligned_mmio = 1; > > The above works for me. Yeah, sorry, that was a wrong patch :) I'm fine for applying it with some more comments. Care to submit a proper patch? thanks, Takashi
On 14/07/2020 10:12, Takashi Iwai wrote: ... >> Did you mean ... >> >> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c >> index 0cc5fad1af8a..5637f0129932 100644 >> --- a/sound/pci/hda/hda_tegra.c >> +++ b/sound/pci/hda/hda_tegra.c >> @@ -443,6 +443,7 @@ static int hda_tegra_create(struct snd_card *card, >> if (err < 0) >> return err; >> >> + chip->bus.core.sync_write = 0; >> chip->bus.core.needs_damn_long_delay = 1; >> chip->bus.core.aligned_mmio = 1; >> >> The above works for me. > > Yeah, sorry, that was a wrong patch :) > I'm fine for applying it with some more comments. > > Care to submit a proper patch? No problem. I will submit a patch. Jon
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c index 9765652a73d7..80016b7b6849 100644 --- a/sound/pci/hda/hda_controller.c +++ b/sound/pci/hda/hda_controller.c @@ -1202,15 +1202,8 @@ int azx_bus_init(struct azx *chip, const char *model) if (chip->driver_caps & AZX_DCAPS_4K_BDLE_BOUNDARY) bus->core.align_bdle_4k = true; - /* AMD chipsets often cause the communication stalls upon certain - * sequence like the pin-detection. It seems that forcing the synced - * access works around the stall. Grrr... - */ - if (chip->driver_caps & AZX_DCAPS_SYNC_WRITE) { - dev_dbg(chip->card->dev, "Enable sync_write for stable communication\n"); - bus->core.sync_write = 1; - bus->allow_bus_reset = 1; - } + /* enable sync_write flag for stable communication as default */ + bus->core.sync_write = 1; return 0; } diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h index 82e26442724b..fe171685492d 100644 --- a/sound/pci/hda/hda_controller.h +++ b/sound/pci/hda/hda_controller.h @@ -33,7 +33,7 @@ #define AZX_DCAPS_POSFIX_LPIB (1 << 16) /* Use LPIB as default */ #define AZX_DCAPS_AMD_WORKAROUND (1 << 17) /* AMD-specific workaround */ #define AZX_DCAPS_NO_64BIT (1 << 18) /* No 64bit address */ -#define AZX_DCAPS_SYNC_WRITE (1 << 19) /* sync each cmd write */ +/* 19 unused */ #define AZX_DCAPS_OLD_SSYNC (1 << 20) /* Old SSYNC reg for ICH */ #define AZX_DCAPS_NO_ALIGN_BUFSIZE (1 << 21) /* no buffer size alignment */ /* 22 unused */ diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 3565e2ab0965..b2bd21f5a52e 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -283,13 +283,12 @@ enum { /* quirks for old Intel chipsets */ #define AZX_DCAPS_INTEL_ICH \ - (AZX_DCAPS_OLD_SSYNC | AZX_DCAPS_NO_ALIGN_BUFSIZE |\ - AZX_DCAPS_SYNC_WRITE) + (AZX_DCAPS_OLD_SSYNC | AZX_DCAPS_NO_ALIGN_BUFSIZE) /* quirks for Intel PCH */ #define AZX_DCAPS_INTEL_PCH_BASE \ (AZX_DCAPS_NO_ALIGN_BUFSIZE | AZX_DCAPS_COUNT_LPIB_DELAY |\ - AZX_DCAPS_SNOOP_TYPE(SCH) | AZX_DCAPS_SYNC_WRITE) + AZX_DCAPS_SNOOP_TYPE(SCH)) /* PCH up to IVB; no runtime PM; bind with i915 gfx */ #define AZX_DCAPS_INTEL_PCH_NOPM \ @@ -304,13 +303,13 @@ enum { #define AZX_DCAPS_INTEL_HASWELL \ (/*AZX_DCAPS_ALIGN_BUFSIZE |*/ AZX_DCAPS_COUNT_LPIB_DELAY |\ AZX_DCAPS_PM_RUNTIME | AZX_DCAPS_I915_COMPONENT |\ - AZX_DCAPS_SNOOP_TYPE(SCH) | AZX_DCAPS_SYNC_WRITE) + AZX_DCAPS_SNOOP_TYPE(SCH)) /* Broadwell HDMI can't use position buffer reliably, force to use LPIB */ #define AZX_DCAPS_INTEL_BROADWELL \ (/*AZX_DCAPS_ALIGN_BUFSIZE |*/ AZX_DCAPS_POSFIX_LPIB |\ AZX_DCAPS_PM_RUNTIME | AZX_DCAPS_I915_COMPONENT |\ - AZX_DCAPS_SNOOP_TYPE(SCH) | AZX_DCAPS_SYNC_WRITE) + AZX_DCAPS_SNOOP_TYPE(SCH)) #define AZX_DCAPS_INTEL_BAYTRAIL \ (AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_I915_COMPONENT) @@ -321,19 +320,18 @@ enum { #define AZX_DCAPS_INTEL_SKYLAKE \ (AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME |\ - AZX_DCAPS_SYNC_WRITE |\ AZX_DCAPS_SEPARATE_STREAM_TAG | AZX_DCAPS_I915_COMPONENT) #define AZX_DCAPS_INTEL_BROXTON AZX_DCAPS_INTEL_SKYLAKE /* quirks for ATI SB / AMD Hudson */ #define AZX_DCAPS_PRESET_ATI_SB \ - (AZX_DCAPS_NO_TCSEL | AZX_DCAPS_SYNC_WRITE | AZX_DCAPS_POSFIX_LPIB |\ + (AZX_DCAPS_NO_TCSEL | AZX_DCAPS_POSFIX_LPIB |\ AZX_DCAPS_SNOOP_TYPE(ATI)) /* quirks for ATI/AMD HDMI */ #define AZX_DCAPS_PRESET_ATI_HDMI \ - (AZX_DCAPS_NO_TCSEL | AZX_DCAPS_SYNC_WRITE | AZX_DCAPS_POSFIX_LPIB|\ + (AZX_DCAPS_NO_TCSEL | AZX_DCAPS_POSFIX_LPIB|\ AZX_DCAPS_NO_MSI64) /* quirks for ATI HDMI with snoop off */ @@ -342,7 +340,7 @@ enum { /* quirks for AMD SB */ #define AZX_DCAPS_PRESET_AMD_SB \ - (AZX_DCAPS_NO_TCSEL | AZX_DCAPS_SYNC_WRITE | AZX_DCAPS_AMD_WORKAROUND |\ + (AZX_DCAPS_NO_TCSEL | AZX_DCAPS_AMD_WORKAROUND |\ AZX_DCAPS_SNOOP_TYPE(ATI) | AZX_DCAPS_PM_RUNTIME) /* quirks for Nvidia */
In the end we already enabled the sync-write mode for most of HD-audio controllers including Intel, and it's no big merit to keep the async write mode for the rest. Let's make it as default and drop the superfluous AZX_DCAPS_SYNC_WRITE bit flag. Also, avoid to set the allow_bus_reset flag, which is a quite unstable and hackish behavior that was needed only for some early platforms (decades ago). The straight fallback to the single cmd mode is more robust. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/pci/hda/hda_controller.c | 11 ++--------- sound/pci/hda/hda_controller.h | 2 +- sound/pci/hda/hda_intel.c | 16 +++++++--------- 3 files changed, 10 insertions(+), 19 deletions(-)