Re: [alsa-devel] [RFC] ASOC: Intel: Skylake: Broken HDaudio controller/link initialization sequences
[ Corrected the ML address ]
On Wed, 09 Jan 2019 00:28:30 +0100, Pierre-Louis Bossart wrote:
Hi,
this is not a patch but a request for comments/feedback on the HDAudio controller/link initialization sequences. After the v4.20 merge window snafu w/ the HDaudio detection, I spent quite a bit of time to figure out why all our HDaudio development devices worked well with the Skylake driver, and why Linus' laptop and others didn't due to an HDMI initialization issue. Hans de Goede was kind enough to give me SSH remote access to his device and while I don't have a formal fix for now I found a couple of issues that really need to be fixed so that the Skylake driver works by design and not by accident.
See below the programming sequences for the legacy and Intel/Skylake drivers. There are obvious discrepancies or broken sequences such as :-
- the i915 component and display power being set in a work
queue. Forcing the init to be done upfront fixes the HDMI detection issue on Hans' laptop with the correct codec_mask detected.
This is hairy, and likely some timing issue...
- a call to snd_hdac_bus_reset_link() that doesn't seem to be useful
(done twice)
Better to avoid the unneeded call of this, yeah.
- an initialization of ML (multi-link) registers *before* reading ML
capabilities on Skylake (only meaningful in the second init) and not done in the legacy.
- Missing setups for multi-link (done in legacy, not done by Skylake
drivers)
I have little idea about the ML stuff, sorry.
- set_codec_wakeup() only initialized in legacy
This might be the cause of the missing HDMI codec.
The wakeup handling was added years ago for assuring that the i915 power-well turned on during probe and resume. Looking at the git log, the initial commit was 0a6735215350 ALSA: hda - reset display codec when power on
- init/stop/init pattern in Skylake driver. I can't think of any
reason why this is required both in the probe and the probe workqueue.
Maybe some workaround for messy timing issue? No much idea, either.
thanks,
Takashi
I don't have any background for most of this, I only took over support for HDaudio codecs after the initial contributor moved on, so sharing all my findings in the hope that others have memories of these different points. I'll get the same laptop as Linus later this week and will be able to work more on this, but in the meantime comments are welcome.
Thanks
-Pierre
azx_create -- azx_bus_init ---- snd_hdac_bus_init -- azx_probe_work ---- azx_probe_continue ------ snd_hdac_i915_init(bus) ------ snd_hdac_display_power(bus, true); ------ azx_first_init(chip); -------- snd_hdac_bus_parse_capabilities(bus) -------- azx_init_streams ---------- snd_hdac_stream_init -------- azx_alloc_stream_pages -------- azx_init_pci(chip) ---------- update_pci_byte(chip->pci, AZX_PCIREG_TCSEL, 0x07, 0); -------- hda_intel_init_chip ---------- snd_hdac_set_codec_wakeup(bus, true); !!! <<< is this needed? ---------- pci_write_config_dword(pci, INTEL_HDA_CGCTL, val); ---------- azx_init_chip(chip, full_reset) ------------ snd_hdac_bus_init_chip(chip, full_reset) -------------- snd_hdac_bus_reset_link(bus, full_reset); -------------- azx_int_clear -------------- snd_hdac_bus_init_cmd_io -------------- azx_int_enable <<<< codec_mask 0x5 is correct ---------- pci_write_config_dword(pci, INTEL_HDA_CGCTL, val) ---------- snd_hdac_set_codec_wakeup(bus, false); !!! <<< is this needed? ---------- bxt_reduce_dma_latency !!! <<< is this needed? ---------- intel_init_lctl !!! <<< is this needed? ------------ intel_ml_lctl_set_power(chip, 0) ------------ intel_get_lctl_scf ------------ intel_get_lctl_scf(chip, 1) ------ azx_probe_codecs ------ azx_codec_configure ------ snd_hdac_display_power(bus, false);
skl_probe -- skl_create ---- snd_hdac_ext_bus_init ------ snd_hdac_bus_init -- skl_first_init
<<< this sequence makes no sense. the i915/display is not initialized before reading the codec_masks. enabling the usual sequence of snd_hdac_i915_init and snd_hdac_display_power gives the right codec_mask
---- snd_hdac_bus_reset_link !!! <<< is this needed, it's done later as part of skl_init_chip? ------ snd_hdac_chip_writew(bus, STATESTS, STATESTS_INT_MASK); ------ snd_hdac_bus_enter_link_reset ------ usleep_range(500, 1000); ------ snd_hdac_bus_exit_link_reset ------ usleep_range(1000, 1200); ------ snd_hdac_chip_updatel(bus, GCTL, 0, AZX_GCTL_UNSOL); ------ snd_hdac_chip_readw(bus, STATESTS); <<<< codec_mask 0x1 is wrong, HDMI not detected ---- snd_hdac_bus_parse_capabilities ---- snd_hdac_ext_stream_init_all ---- skl_init_pci ------ skl_update_pci_byte(skl->pci, AZX_PCIREG_TCSEL, 0x07, 0); ---- skl_init_chip ------ skl_enable_miscbdcge(bus->dev, false); ------ snd_hdac_bus_init_chip(bus, full_reset); -------- snd_hdac_bus_reset_link(bus, full_reset); -------- azx_int_clear -------- snd_hdac_bus_init_cmd_io -------- azx_int_enable <<< next line makes no sense, it's done before the ML capabilities are read ------ bus->io_ops->reg_writel(0, hlink->ml_addr + AZX_REG_ML_LOSIDV); !!! <<< is this needed? ------ skl_enable_miscbdcge(bus->dev, true); <<< codec_mask 0x1 is wrong -- skl_nhlt_init -- skl_init_dsp ---- snd_hdac_ext_bus_ppcap_enable(bus, true); ---- snd_hdac_ext_bus_ppcap_int_enable(bus, true); -- snd_hdac_ext_bus_get_ml_capabilities -- snd_hdac_bus_stop_chip ---- azx_int_disable(bus); ---- azx_int_clear(bus); ---- snd_hdac_bus_stop_cmd_io(bus); -- skl_probe_work ---- skl_i915_init <<< this needs to be done earlier ------ snd_hdac_i915_init ------ snd_hdac_display_power ---- skl_init_chip <<< why do we need to re-initialize again? ------ skl_enable_miscbdcge(bus->dev, false); ------ snd_hdac_bus_init_chip(bus, full_reset); -------- snd_hdac_bus_reset_link(bus, full_reset); -------- azx_int_clear -------- snd_hdac_bus_init_cmd_io -------- azx_int_enable ------ bus->io_ops->reg_writel(0, hlink->ml_addr + AZX_REG_ML_LOSIDV); ------ skl_enable_miscbdcge(bus->dev, true); ---- skl_codec_create ------- snd_hdac_bus_stop_chip ------- skl_init_chip (on error) ---- skl_platform_register ---- skl_machine_device_register ---- snd_hdac_ext_bus_link_put ---- snd_hdac_display_power(bus, false);
Thanks for the comments Takashi, much appreciated.
On 1/11/19 5:42 AM, Takashi Iwai wrote:
[ Corrected the ML address ]
oops, thanks!
On Wed, 09 Jan 2019 00:28:30 +0100, Pierre-Louis Bossart wrote:
Hi,
this is not a patch but a request for comments/feedback on the HDAudio controller/link initialization sequences. After the v4.20 merge window snafu w/ the HDaudio detection, I spent quite a bit of time to figure out why all our HDaudio development devices worked well with the Skylake driver, and why Linus' laptop and others didn't due to an HDMI initialization issue. Hans de Goede was kind enough to give me SSH remote access to his device and while I don't have a formal fix for now I found a couple of issues that really need to be fixed so that the Skylake driver works by design and not by accident.
See below the programming sequences for the legacy and Intel/Skylake drivers. There are obvious discrepancies or broken sequences such as :-
- the i915 component and display power being set in a work
queue. Forcing the init to be done upfront fixes the HDMI detection issue on Hans' laptop with the correct codec_mask detected.
This is hairy, and likely some timing issue...
I can also reproduce a timing issue leading to the same error on the Dell XPS13 9350 with the DRM and SOUND configs set to m instead of y, so indeed we need to harden this.
- a call to snd_hdac_bus_reset_link() that doesn't seem to be useful
(done twice)
Better to avoid the unneeded call of this, yeah.
I'll remove it and ask for one round of validation.
- an initialization of ML (multi-link) registers *before* reading ML
capabilities on Skylake (only meaningful in the second init) and not done in the legacy.
- Missing setups for multi-link (done in legacy, not done by Skylake
drivers)
I have little idea about the ML stuff, sorry.
The good news is here that Libin Yang was the original author of this code and he's still in our team.
- set_codec_wakeup() only initialized in legacy
This might be the cause of the missing HDMI codec.
The wakeup handling was added years ago for assuring that the i915 power-well turned on during probe and resume. Looking at the git log, the initial commit was 0a6735215350 ALSA: hda - reset display codec when power on
Ah, very useful thanks! This codec_wakeup thingy seems to be required for all of GEN9 products, which as I understand it means Skylake, ApolloLake, KabyLake, GeminiLake and CoffeeLake. We also probably want to check if there is a similar issue with GEN10+.
- init/stop/init pattern in Skylake driver. I can't think of any
reason why this is required both in the probe and the probe workqueue.
Maybe some workaround for messy timing issue? No much idea, either.
this one is going to be painful to check, the original contributors have moved on. If the fixes for the power and wakeups are enough maybe we'll leave this sequence as is in a "Do no harm" way.
thanks,
Takashi
I don't have any background for most of this, I only took over support for HDaudio codecs after the initial contributor moved on, so sharing all my findings in the hope that others have memories of these different points. I'll get the same laptop as Linus later this week and will be able to work more on this, but in the meantime comments are welcome.
Thanks
-Pierre
azx_create -- azx_bus_init ---- snd_hdac_bus_init -- azx_probe_work ---- azx_probe_continue ------ snd_hdac_i915_init(bus) ------ snd_hdac_display_power(bus, true); ------ azx_first_init(chip); -------- snd_hdac_bus_parse_capabilities(bus) -------- azx_init_streams ---------- snd_hdac_stream_init -------- azx_alloc_stream_pages -------- azx_init_pci(chip) ---------- update_pci_byte(chip->pci, AZX_PCIREG_TCSEL, 0x07, 0); -------- hda_intel_init_chip ---------- snd_hdac_set_codec_wakeup(bus, true); !!! <<< is this needed? ---------- pci_write_config_dword(pci, INTEL_HDA_CGCTL, val); ---------- azx_init_chip(chip, full_reset) ------------ snd_hdac_bus_init_chip(chip, full_reset) -------------- snd_hdac_bus_reset_link(bus, full_reset); -------------- azx_int_clear -------------- snd_hdac_bus_init_cmd_io -------------- azx_int_enable <<<< codec_mask 0x5 is correct ---------- pci_write_config_dword(pci, INTEL_HDA_CGCTL, val) ---------- snd_hdac_set_codec_wakeup(bus, false); !!! <<< is this needed? ---------- bxt_reduce_dma_latency !!! <<< is this needed? ---------- intel_init_lctl !!! <<< is this needed? ------------ intel_ml_lctl_set_power(chip, 0) ------------ intel_get_lctl_scf ------------ intel_get_lctl_scf(chip, 1) ------ azx_probe_codecs ------ azx_codec_configure ------ snd_hdac_display_power(bus, false);
skl_probe -- skl_create ---- snd_hdac_ext_bus_init ------ snd_hdac_bus_init -- skl_first_init
<<< this sequence makes no sense. the i915/display is not initialized before reading the codec_masks. enabling the usual sequence of snd_hdac_i915_init and snd_hdac_display_power gives the right codec_mask
---- snd_hdac_bus_reset_link !!! <<< is this needed, it's done later as part of skl_init_chip? ------ snd_hdac_chip_writew(bus, STATESTS, STATESTS_INT_MASK); ------ snd_hdac_bus_enter_link_reset ------ usleep_range(500, 1000); ------ snd_hdac_bus_exit_link_reset ------ usleep_range(1000, 1200); ------ snd_hdac_chip_updatel(bus, GCTL, 0, AZX_GCTL_UNSOL); ------ snd_hdac_chip_readw(bus, STATESTS); <<<< codec_mask 0x1 is wrong, HDMI not detected ---- snd_hdac_bus_parse_capabilities ---- snd_hdac_ext_stream_init_all ---- skl_init_pci ------ skl_update_pci_byte(skl->pci, AZX_PCIREG_TCSEL, 0x07, 0); ---- skl_init_chip ------ skl_enable_miscbdcge(bus->dev, false); ------ snd_hdac_bus_init_chip(bus, full_reset); -------- snd_hdac_bus_reset_link(bus, full_reset); -------- azx_int_clear -------- snd_hdac_bus_init_cmd_io -------- azx_int_enable <<< next line makes no sense, it's done before the ML capabilities are read ------ bus->io_ops->reg_writel(0, hlink->ml_addr + AZX_REG_ML_LOSIDV); !!! <<< is this needed? ------ skl_enable_miscbdcge(bus->dev, true); <<< codec_mask 0x1 is wrong -- skl_nhlt_init -- skl_init_dsp ---- snd_hdac_ext_bus_ppcap_enable(bus, true); ---- snd_hdac_ext_bus_ppcap_int_enable(bus, true); -- snd_hdac_ext_bus_get_ml_capabilities -- snd_hdac_bus_stop_chip ---- azx_int_disable(bus); ---- azx_int_clear(bus); ---- snd_hdac_bus_stop_cmd_io(bus); -- skl_probe_work ---- skl_i915_init <<< this needs to be done earlier ------ snd_hdac_i915_init ------ snd_hdac_display_power ---- skl_init_chip <<< why do we need to re-initialize again? ------ skl_enable_miscbdcge(bus->dev, false); ------ snd_hdac_bus_init_chip(bus, full_reset); -------- snd_hdac_bus_reset_link(bus, full_reset); -------- azx_int_clear -------- snd_hdac_bus_init_cmd_io -------- azx_int_enable ------ bus->io_ops->reg_writel(0, hlink->ml_addr + AZX_REG_ML_LOSIDV); ------ skl_enable_miscbdcge(bus->dev, true); ---- skl_codec_create ------- snd_hdac_bus_stop_chip ------- skl_init_chip (on error) ---- skl_platform_register ---- skl_machine_device_register ---- snd_hdac_ext_bus_link_put ---- snd_hdac_display_power(bus, false);
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
participants (2)
-
Pierre-Louis Bossart -
Takashi Iwai