Timing issues between ALSA and i915 drivers

Submitted by Takashi Iwai on Jan. 16, 2019, 7:17 p.m.

Details

Message ID s5hef9ce7iz.wl-tiwai@suse.de
State New
Headers show
Series "Timing issues between ALSA and i915 drivers" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Takashi Iwai Jan. 16, 2019, 7:17 p.m.
On Wed, 16 Jan 2019 18:48:25 +0100,
Pierre-Louis Bossart wrote:
> 
> Hi,
> 
> 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.

> 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...


thanks,

Takashi

---

Patch hide | download patch | download mbox

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);
 }
 
 /**

Comments

>> 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);
>   }
>   
>   /**
>>>
>>> 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)

I added DRM+audio dmesg logs at the following link for reference:

https://github.com/thesofproject/linux/issues/549