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. So if 48 Mhz is chosen, you get the 
benefits with just a BIOS change and no need to muck around the 
kernel as well.

Br, Kai