[alsa-devel] [RFC] ASOC: Intel: Skylake: Broken HDaudio controller/link initialization sequences

[Pierre-Louis Bossart]

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 at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel