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

[Ville Syrjälä]

On Tue, Mar 10, 2020 at 07:18:58PM +0200, Kai Vehmanen wrote:
> 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.

The logic around the cdclk computation is still a bit messy.

First draft of just doing the lazy force_min_cdclk reduction in put_power():
git://github.com/vsyrjala/linux.git no_cdclk_in_audio_put_power

Very lightly smoke tested, but not sure if it achieves anything useful :P

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

I think when I last complained about the assumed 96 MHz BCLK
people said "48 MHz never happens". But I guess it can be made
to happen?

-- 
Ville Syrjälä
Intel