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
:-
1. 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.
2. 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.
3. 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.
4. 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.
5. 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+.
6. 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@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
