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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
