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

[Kai Vehmanen]

Hi folks,

[+Takashi from ALSA]

On Mon, 6 Jan 2020, Matt Roper wrote:
>>> On Tue, Dec 31, 2019 at 04:00:07PM +0200, Kai Vehmanen wrote:
>>>> Revert changes done in commit f6ec9483091f ("drm/i915: extend audio
>>>> CDCLK>=2*BCLK constraint to more platforms"). Audio drivers
>
> Agreed; GLK's minimum cdclk goes down to 79.2 mhz so it makes sense that
> it needs to be handled differently than CNL (and beyond).  I.e., this
> isn't a pure revert of the original patch.

unfortunately it seems this fix that was done is not holding up in wider 
testing. It now looks we need to enforce the constraint in one form or 
another for newer platforms as well. We can't revert the revert as that 
will bring the boot flicker back, so we'll need something else.

Now as we've gone back-and-forth multiple times, I want to get some early 
feedback before opting for one path or another.

To recap in short:
 - audio driver calls i915 acomp get_power() multiple times during boot
	-> this can cause annoying flicker at boot on platforms where
	   each get_power() leads to a modeset change
	- example: https://gitlab.freedesktop.org/drm/intel/issues/913
 - systems with more complex audio subsystems (DSP enabled) will be 
   calling get_power() many more times (as i915 iDisp link is needed in 
   multiple phases of audio controller, DSP and codecs initialization), 
   making the flicker worse

I've gone through (once more) possible options, and it starts to seem that 
trying to minimize # of get_power() cycles is not going to work well in 
the long run. We could certainly reduce number of distinct get_power 
calls e.g. during boot by refactoring the audio DSP setup, but there would 
still be more than a few, and it's not just the boot. We now need to call 
get_power() when the audio controller comes out from runtime suspend 
(otherwise the HDA link is not ok between i915 and audio). This can be pretty 
annoying if there are visible artifacts to the end-user in such a case
where potentially no HDMI/DP monitors are even connected).

Similarly on i915 side, it would seem pretty unlikely that we are going
to get smooth changes of CDCLK. It might work better on some platforms, 
but generally (depending on the available dividers provided), it's not 
going to be guaranteed to be flicker-free.

So how about: We move the glk_force_audio_cdclk() logic from 
intel_audio.c:i915_audio_component_get_power() to acomp init.
This has some notable implications:

 - audio driver can safely call get_power without worrying 
   about creating display artifacts,
 - on some platforms, with specific HDA link params in BIOS, 
   this will mean some lower CDCLK frequencies, will not be used
   at all
	- e.g. on ICL system with 96Mhz BCLK for iDisp, 172800 and
   	  180000 are blocked out, and 192000 is the practical minimum
	  unless you unload the audio driver
	- if BCLK is 48Mhz, no impact to CDCLK selection on ICL

Any chance to get this through? I understand this effectively removes the 
lower clocks from some systems, so this needs to be evaluated carefully.

I don't really have other options on the table now. We could maybe use 
idle-timers to delay powering off the audio domain until certain amount of 
inactivity, but this is both ugly and won't solve the full thing. Or we 
just keep reducing get_power() calls on audio side, and just mitigate the 
the severity of the flicker -- again not fully solving the problem.

Thoughts and feedback is welcome.

Br, Kai