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

[Takashi Iwai]

On Fri, 06 Mar 2020 17:45:44 +0100,
Kai Vehmanen wrote:
> 
> 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

That sounds reasonable to me.  But it's basically the i915 stuff, so
I'd leave the decision to you guys :)

My another quick thought after reading this mail is whether we can
simply remove glk_force_audio_cdclk(false) in
i915_audio_component_put_power().  In this way, a flicker should be
reduced, at most only once at boot time, and the CDCLK is lowered only
when the audio is really used (once).

Or, similarly, it can be put into *_component_bind() and *_unbind()
instead of *_get_power() and *_put_power().  This indicates that the
corresponding audio device really exists.


thanks,

Takashi