[Intel-gfx] [PATCH v2] drm/i915: Limit audio CDCLK>=2*BCLK constraint back to GLK only

[Takashi Iwai]

On Wed, 11 Mar 2020 13:16:56 +0100,
Kai Vehmanen wrote:
> 
> Hey,
> 
> On Tue, 10 Mar 2020, Takashi Iwai wrote:
> > On Tue, 10 Mar 2020 19:25:22 +0100, Ville Syrjälä wrote:
> >> On Tue, Mar 10, 2020 at 07:18:58PM +0200, Kai Vehmanen wrote:
> >>> One problematic scenario that this doesn't cover:
> >>>  - a single display is used (at low cdclk), and 
> >>>  - audio block goes to runtime suspend while display stays up. 
> >>> 
> >>> Upon resume (for e.g. UI notification sound), audio will initialize the 
> >>> HDA bus and call get_power() on i915, even if the notification goes to 
> >>> internal speaker. A modeset at this point is potentially very annoying.
> >> 
> >> :( That seems much harder to deal with.
> > 
> > I guess it doesn't happen -- at least with the legacy HD-audio and the
> > recent chip, if I understand correctly.  When the stream is on the
> > analog codec, the HDMI codec is kept closed / runtime-resumed.  And
> > the additional get_power() in the controller side is done only for
> > HSW/BDW (where the HDA-bus is dedicated to HDMI).
> 
> I'm afraid it does -- I double checked the legacy driver code. Judging 
> from history, since commit "ALSA: hda - Fix Skylake codec timeout", 
> azx_runtime_resume() has called "display_power(chip, true)" on all 
> platforms, even if the stream is on analog codec. I just recently fixed 
> this logic to SOF driver as well. If you don't do this and the link 
> parameters are different from hw defaults on i915 side (more likely with 
> ICL and newer), you'll hit problems.

Oh indeed, I overlooked that part.

> I don't currently know of a reliable way to recover. In theory, if HDMI/DP 
> monitor is connected, we could do a delayed call to i915 get_power and 
> reinitialize the HDA controller, but if we are actively streaming to 
> analog codec when this happens, this wouldn't work.
> 
> And until very recent times, this has not really been an issue. With 
> current i915, many conditions have to be met to actually hit this (only 
> affects GLK platforms, you need to be using HDA analog codec, runtime PM 
> needs to be enabled for HDA, etc). Problem is that going forward, there 
> are going to be more platforms that are affected and this starts to show 
> up in more places.

The remaining question is whether this display_power() call is still
needed for the recent chips.  Basically it's there for HSW/BDW type
chips that need already the power for the HDA link that is dedicated
to HDMI.  That is, a patch like below could work for the recent chips
that share both onboard and HDMI codecs.


Takashi

--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -992,9 +992,10 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt)
 	struct hda_codec *codec;
 	int status;
 
-	display_power(chip, true);
-	if (hda->need_i915_power)
+	if (hda->need_i915_power) {
+		display_power(chip, true);
 		snd_hdac_i915_set_bclk(bus);
+	}
 
 	/* Read STATESTS before controller reset */
 	status = azx_readw(chip, STATESTS);
@@ -1008,10 +1009,6 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt)
 				schedule_delayed_work(&codec->jackpoll_work,
 						      codec->jackpoll_interval);
 	}
-
-	/* power down again for link-controlled chips */
-	if (!hda->need_i915_power)
-		display_power(chip, false);
 }
 
 #ifdef CONFIG_PM_SLEEP