[Intel-gfx] Timing issues between ALSA and i915 drivers

[Pierre-Louis Bossart]

>> I could use some feedback on HDMI audio issues exposed during the 4.21
>> merge window. By accident (misleading documentation) we ended up
>> enabling the Skylake driver instead of the HDaudio legacy, and broke
>> audio on a number of Skylake and ApolloLake devices where the
>> HDMI/iDISP codec was not detected (bit 2 not set in the
>> codec_mask). Linus' Dell XPS13 9350 was the first to be impacted of
>> course...
>>
>> After debugging a bit, this issue can be resolved by either
>>
>> a) compiling both SOUND and DRM as built-ins (y instead of m)
>>
>> b) moving the calls snd_hdac_i915_init() to the probe function instead
>> of the worker queue (code at
>> https://github.com/plbossart/sound/commits/fix/skl-hdmi)
> This isn't guaranteed to work, as request_module() might be involved,
> I'm afraid.

Sorry, what would be the impact of the request_module? it'd just delay 
the probe on the audio side, no?

And if the request_module failed then HDMI wouldn't be more broken 
anyways...

>
>> Both solutions point to timing issues.
>>
>> During internal reviews I was alerted to the fact that the suggested
>> fix essentially reverts patch ab1b732d53c18 ('ASoC: Intel: Skylake:
>> Move i915 registration to worker thread') which was introduced to
>> solve DRM lockup issues.
>>
>> In other words, we are at a point where we either have DRM lockups or
>> can't detect the HDMI audio codec, none of which are too good.
>>
>> I don't have the background for the DRM lockup stuff, nor do I
>> understand why snd_hdac_i915_init has to be called from a thread
>> context. Is this really a requirement?
>>
>> I also don't get what sort of timing issues would come from an
>> initialization. What happens on the i915 side and is there some sort
>> of mandatory delay between the completion of the i915_init and issuing
>> a snd_hdac_chip_readw(bus, STATESTS) to get the codec masks on the
>> HDaudio links?
> snd_hdac_i915_init() waits for the binding with the i915 audio
> component, so a possible solution would be just to delay the audio
> component registration at the really last stage, like the change
> below.
>
> If this still doesn't work, it'll be more deeply inside, and I have
> little clue for now...

I tried this suggestion and no luck, same error with the iDISP codec not 
exposed.

I added a delay after snd_hdac_i915_init(), doesn't seem to do anything.

One possibility is that this is a side effect of the Skylake driver 
initializing the link twice for some odd reason.

And I still don't get what the motivation for moving this init to a work 
queue was in the first place.

Doesn't seem like an easy one to fix...

>
>
> thanks,
>
> Takashi
>
> ---
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1577,8 +1577,6 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>   	if (IS_GEN5(dev_priv))
>   		intel_gpu_ips_init(dev_priv);
>   
> -	intel_audio_init(dev_priv);
> -
>   	/*
>   	 * Some ports require correctly set-up hpd registers for detection to
>   	 * work properly (leading to ghost connected connector status), e.g. VGA
> @@ -1597,6 +1595,8 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>   
>   	intel_power_domains_enable(dev_priv);
>   	intel_runtime_pm_enable(dev_priv);
> +
> +	intel_audio_init(dev_priv);
>   }
>   
>   /**