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

[Kai Vehmanen]

Hi,

On Tue, 10 Mar 2020, Ville Syrjälä wrote:

>> On Fri, 06 Mar 2020 17:45:44 +0100, Kai Vehmanen wrote:
>>> 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, 
> 
> There is new hw in the pipeline that should allow cdclk changes
> without a full modeset.

ok great, this is good to know. Especially we should not completely remove 
the CDCLK constraints code from get_power/put_power, as this will be 
later needed.

>>> intel_audio.c:i915_audio_component_get_power() to acomp init.
>>> This has some notable implications:
[...]
>>> Any chance to get this through? I understand this effectively removes the 
>>> lower clocks from some systems, so this needs to be evaluated carefully.
> 
> If we're going to effectively force cdclk to remain high all the time
> then we should just nuke the whole glk_force_audio_cdclk() thing. But
> at least I'll have to shed a few tears for the wasted milliwatts.
> 
> Well, I guess we might want to keep glk_force_audio_cdclk() in its
> current form for the upcoming hw that doesn't need the full modeset
> for cdclk changes.

Yeah, we probably should keep it in any case, because later it's going to 
be needed.

> I guess we could also make i915 force the cdclk to the min required by
> audio at init time. And we could maybe try to remove the modeset from the
> put_power() so that at least if you get a blink it's just the one. I did
> a similarsh thing for some other cdclk stuff recently where we want cdclk
> to go up as needed, but it will not come back down unless someone else
> already asked for a full modeset.

Hmm, this is interesting and maybe a better compromise for the in-between 
generations. Could it be as simple as not setting 
"cdclk.force_min_cdclk_changed" at put_power(), and just set the 
min_cdclk...? I was trying to follow the modeset code and it seems without 
the force set, this would avoid going to intel_modeset_checks(). If so, I 
can try this out.

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.

I just also noted if we keep the glk_force_audio function, we need to get 
rid of the hardcoded 96Mhz BCLK value that is used now, and instead dig up 
the effective used value (we do have this). This will at least offer the 
possibility to configure the HDA link to 48Mhz in BIOS and avoid the cdclk 
bump this way.

Br, Kai