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

[Ville Syrjälä]

On Thu, Mar 12, 2020 at 07:27:58PM +0200, Kai Vehmanen wrote:
> Hey,
> 
> On Tue, 10 Mar 2020, Ville Syrjälä wrote:
> 
> > On Tue, Mar 10, 2020 at 07:18:58PM +0200, Kai Vehmanen wrote:
> >> On Tue, 10 Mar 2020, Ville Syrjälä wrote:
> >>> 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
> >>
> >> Hmm, this is interesting and maybe a better compromise for the in-between 
> >> generations. Could it be as simple as not setting 
> > 
> > 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
> 
> I tested this today and no issues found. I can see clock bumped if there 
> is audio activity, but clock is kept after audio goes back to sleep. 
> But then e.g. at next display-off timeout, clk is brought back down.
> So works as expected.
> 
> But, but, then I also tested...
> 
> >> 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 
> 
> Now actually hitting this requires some effort. On most systems I tried, 
> with display active, the clock will stay above the limit for other 
> reasons, but yup, when this happens, it is pretty, pretty bad.
> 
> Your no_cdclk_in_audio_put_power patch does reduce the level of annoyance 
> also in this case -- you only get one flash instead of two. But does not 
> seem acceptable still. If you happen to have a system where the conditions 
> are met, then this happens all the time. In case of UI notification sounds 
> being the trigger, we could consider the visual flash as a feature, but 
> probably not widely appreciated. ;) .. and especially as you cannot turn 
> it off.
> 
> So I think this starts to look that we should move calling glk_force_audio 
> to bind/unbind pair. I can make a patch for this.
> 
> >> 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 
> >
> > 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?
> 
> Indeed the recommendation still is 96 Mhz and that will be the dominant 
> value, but 48 Mhz is still an option. To keep the system open for 
> configurability, I think the bind/unbind restriction should take the 
> effective BCLK value into account.

IMO no. We should just figure out what the bclk is and let the display
driver enfoce the 2xbclk requirement on its own regardless of the bclk
frequency. I don't want the audio driver start assuming cdclk can
never go below the 2*48MHz magic limit.

I think there was a register somewhere where we could read the BCLK.
And if not we should extend the interface between the drivers so the
audio driver can pass in that information.

-- 
Ville Syrjälä
Intel