diff mbox series

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

Message ID 20191231140007.31728-1-kai.vehmanen@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [v2] drm/i915: Limit audio CDCLK>=2*BCLK constraint back to GLK only | expand

Commit Message

Kai Vehmanen Dec. 31, 2019, 2 p.m. UTC
Revert changes done in commit f6ec9483091f ("drm/i915: extend audio
CDCLK>=2*BCLK constraint to more platforms"). Audio drivers
communicate with i915 over HDA bus multiple times during system
boot-up and each of these transactions result in matching
get_power/put_power calls to i915, and depending on the platform,
a modeset change causing visible flicker.

GLK is the only platform with minimum CDCLK significantly lower
than BCLK, and thus for GLK setting a higher CDCLK is mandatory.

For other platforms, minimum CDCLK is close but below 2*BCLK
(e.g. on ICL, CDCLK=176.4kHz with BCLK=96kHz). Spec-wise the constraint
should be set, but in practise no communication errors have been
reported and the downside if set is the flicker observed at boot-time.

Revert to old behaviour until better mechanism to manage
probe-time clocks is available.

The full CDCLK>=2*BCLK constraint is still enforced at pipe
enable time in intel_crtc_compute_min_cdclk().

Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/913
Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---

Notes:
    v2: d'oh, change put_power() as well

 drivers/gpu/drm/i915/display/intel_audio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Rodrigo Vivi Jan. 2, 2020, 6:28 p.m. UTC | #1
On Tue, Dec 31, 2019 at 04:00:07PM +0200, Kai Vehmanen wrote:
> Revert changes done in commit f6ec9483091f ("drm/i915: extend audio
> CDCLK>=2*BCLK constraint to more platforms"). Audio drivers
> communicate with i915 over HDA bus multiple times during system
> boot-up and each of these transactions result in matching
> get_power/put_power calls to i915, and depending on the platform,
> a modeset change causing visible flicker.
> 
> GLK is the only platform with minimum CDCLK significantly lower
> than BCLK, and thus for GLK setting a higher CDCLK is mandatory.
> 
> For other platforms, minimum CDCLK is close but below 2*BCLK
> (e.g. on ICL, CDCLK=176.4kHz with BCLK=96kHz). Spec-wise the constraint
> should be set, but in practise no communication errors have been
> reported and the downside if set is the flicker observed at boot-time.
> 
> Revert to old behaviour until better mechanism to manage
> probe-time clocks is available.
> 
> The full CDCLK>=2*BCLK constraint is still enforced at pipe
> enable time in intel_crtc_compute_min_cdclk().
> 
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/913
> Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> ---
> 
> Notes:
>     v2: d'oh, change put_power() as well
> 
>  drivers/gpu/drm/i915/display/intel_audio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
> index 27710098d056..e406719a6716 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> @@ -856,7 +856,7 @@ static unsigned long i915_audio_component_get_power(struct device *kdev)
>  		}
>  
>  		/* Force CDCLK to 2*BCLK as long as we need audio powered. */
> -		if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> +		if (IS_GEMINILAKE(dev_priv))

I believe for correctness we should at least say this is for display_10 but
since we don't have display gen defined probably the right thing to do here
is to at least replace this per:

IS_GEN(dev_priv, 10) || IS_GEMINILAKE(dev_priv)

>  			glk_force_audio_cdclk(dev_priv, true);
>  
>  		if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> @@ -875,7 +875,7 @@ static void i915_audio_component_put_power(struct device *kdev,
>  
>  	/* Stop forcing CDCLK to 2*BCLK if no need for audio to be powered. */
>  	if (--dev_priv->audio_power_refcount == 0)
> -		if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> +		if (IS_GEMINILAKE(dev_priv))
>  			glk_force_audio_cdclk(dev_priv, false);
>  
>  	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO, cookie);
> -- 
> 2.17.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Kai Vehmanen Jan. 3, 2020, 3:28 p.m. UTC | #2
Hi,

On Thu, 2 Jan 2020, Rodrigo Vivi wrote:

> On Tue, Dec 31, 2019 at 04:00:07PM +0200, Kai Vehmanen wrote:
>> Revert changes done in commit f6ec9483091f ("drm/i915: extend audio
>> CDCLK>=2*BCLK constraint to more platforms"). Audio drivers
[...]
>>  		/* Force CDCLK to 2*BCLK as long as we need audio powered. */
>> -		if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>> +		if (IS_GEMINILAKE(dev_priv))
> 
> I believe for correctness we should at least say this is for display_10 but
> since we don't have display gen defined probably the right thing to do here
> is to at least replace this per:
> 
> IS_GEN(dev_priv, 10) || IS_GEMINILAKE(dev_priv)

I checked the cdclk tables for CNL in intel_cdclk.c and minimum CDCLK
is 168kHz, so it is similar (>BCLK and close to 2*BCLK) as ICL and 
others and the workaround is not needed.

I do agree this still smells funny, but basicly my naive attempt to align 
with the spec failed in wider testing and it seems this original solution 
to have the WA for GLK only, is the least bad option at this point.

Possible longer term solutions for this: 
   (i) more clock configurations allowing to bump the freq without
       a mode change on all platforms
   (ii) avoid all HDA communication at probe time and only initialize
  	the HDA connection when a monitor is connected 
   (iii) guarantee min cdclk to be sufficient for HDA communication

Closing on feasibility of (i) and (iii) is going to be a longer 
discussion.

The (ii) option would be quite a big change on audio side and might
potentially require changes to drm_audio_component.h (and impact other
drivers). To me, this feels wrong, the HDA bus supports discovery of
codecs, so we should be able to use it as with any HDA codec, including
graphics. Unless we hit deadends with (i) and (iii), I'd rather 
not pursue this path.

Br, Kai
Matt Roper Jan. 6, 2020, 4:49 p.m. UTC | #3
On Fri, Jan 03, 2020 at 05:28:24PM +0200, Kai Vehmanen wrote:
> Hi,
> 
> On Thu, 2 Jan 2020, Rodrigo Vivi wrote:
> 
> > On Tue, Dec 31, 2019 at 04:00:07PM +0200, Kai Vehmanen wrote:
> >> Revert changes done in commit f6ec9483091f ("drm/i915: extend audio
> >> CDCLK>=2*BCLK constraint to more platforms"). Audio drivers
> [...]
> >>  		/* Force CDCLK to 2*BCLK as long as we need audio powered. */
> >> -		if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> >> +		if (IS_GEMINILAKE(dev_priv))
> > 
> > I believe for correctness we should at least say this is for display_10 but
> > since we don't have display gen defined probably the right thing to do here
> > is to at least replace this per:
> > 
> > IS_GEN(dev_priv, 10) || IS_GEMINILAKE(dev_priv)
> 
> I checked the cdclk tables for CNL in intel_cdclk.c and minimum CDCLK
> is 168kHz, so it is similar (>BCLK and close to 2*BCLK) as ICL and 
> others and the workaround is not needed.

Agreed; GLK's minimum cdclk goes down to 79.2 mhz so it makes sense that
it needs to be handled differently than CNL (and beyond).  I.e., this
isn't a pure revert of the original patch.

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

The only CI shard failure was an unrelated GEM false positive, so added
a Fixes: tag referencing the original patch and applied to dinq.  Thanks
for the patch!


Matt

> 
> I do agree this still smells funny, but basicly my naive attempt to align 
> with the spec failed in wider testing and it seems this original solution 
> to have the WA for GLK only, is the least bad option at this point.
> 
> Possible longer term solutions for this: 
>    (i) more clock configurations allowing to bump the freq without
>        a mode change on all platforms
>    (ii) avoid all HDA communication at probe time and only initialize
>   	the HDA connection when a monitor is connected 
>    (iii) guarantee min cdclk to be sufficient for HDA communication
> 
> Closing on feasibility of (i) and (iii) is going to be a longer 
> discussion.
> 
> The (ii) option would be quite a big change on audio side and might
> potentially require changes to drm_audio_component.h (and impact other
> drivers). To me, this feels wrong, the HDA bus supports discovery of
> codecs, so we should be able to use it as with any HDA codec, including
> graphics. Unless we hit deadends with (i) and (iii), I'd rather 
> not pursue this path.
> 
> Br, Kai
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Kai Vehmanen March 6, 2020, 4:45 p.m. UTC | #4
Hi folks,

[+Takashi from ALSA]

On Mon, 6 Jan 2020, Matt Roper wrote:
>>> On Tue, Dec 31, 2019 at 04:00:07PM +0200, Kai Vehmanen wrote:
>>>> Revert changes done in commit f6ec9483091f ("drm/i915: extend audio
>>>> CDCLK>=2*BCLK constraint to more platforms"). Audio drivers
>
> Agreed; GLK's minimum cdclk goes down to 79.2 mhz so it makes sense that
> it needs to be handled differently than CNL (and beyond).  I.e., this
> isn't a pure revert of the original patch.

unfortunately it seems this fix that was done is not holding up in wider 
testing. It now looks we need to enforce the constraint in one form or 
another for newer platforms as well. We can't revert the revert as that 
will bring the boot flicker back, so we'll need something else.

Now as we've gone back-and-forth multiple times, I want to get some early 
feedback before opting for one path or another.

To recap in short:
 - audio driver calls i915 acomp get_power() multiple times during boot
	-> this can cause annoying flicker at boot on platforms where
	   each get_power() leads to a modeset change
	- example: https://gitlab.freedesktop.org/drm/intel/issues/913
 - systems with more complex audio subsystems (DSP enabled) will be 
   calling get_power() many more times (as i915 iDisp link is needed in 
   multiple phases of audio controller, DSP and codecs initialization), 
   making the flicker worse

I've gone through (once more) possible options, and it starts to seem that 
trying to minimize # of get_power() cycles is not going to work well in 
the long run. We could certainly reduce number of distinct get_power 
calls e.g. during boot by refactoring the audio DSP setup, but there would 
still be more than a few, and it's not just the boot. We now need to call 
get_power() when the audio controller comes out from runtime suspend 
(otherwise the HDA link is not ok between i915 and audio). This can be pretty 
annoying if there are visible artifacts to the end-user in such a case
where potentially no HDMI/DP monitors are even connected).

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, 
but generally (depending on the available dividers provided), it's not 
going to be guaranteed to be flicker-free.

So how about: We move the glk_force_audio_cdclk() logic from 
intel_audio.c:i915_audio_component_get_power() to acomp init.
This has some notable implications:

 - audio driver can safely call get_power without worrying 
   about creating display artifacts,
 - on some platforms, with specific HDA link params in BIOS, 
   this will mean some lower CDCLK frequencies, will not be used
   at all
	- e.g. on ICL system with 96Mhz BCLK for iDisp, 172800 and
   	  180000 are blocked out, and 192000 is the practical minimum
	  unless you unload the audio driver
	- if BCLK is 48Mhz, no impact to CDCLK selection on ICL

Any chance to get this through? I understand this effectively removes the 
lower clocks from some systems, so this needs to be evaluated carefully.

I don't really have other options on the table now. We could maybe use 
idle-timers to delay powering off the audio domain until certain amount of 
inactivity, but this is both ugly and won't solve the full thing. Or we 
just keep reducing get_power() calls on audio side, and just mitigate the 
the severity of the flicker -- again not fully solving the problem.

Thoughts and feedback is welcome.

Br, Kai
Takashi Iwai March 9, 2020, 10:54 a.m. UTC | #5
On Fri, 06 Mar 2020 17:45:44 +0100,
Kai Vehmanen wrote:
> 
> Hi folks,
> 
> [+Takashi from ALSA]
> 
> On Mon, 6 Jan 2020, Matt Roper wrote:
> >>> On Tue, Dec 31, 2019 at 04:00:07PM +0200, Kai Vehmanen wrote:
> >>>> Revert changes done in commit f6ec9483091f ("drm/i915: extend audio
> >>>> CDCLK>=2*BCLK constraint to more platforms"). Audio drivers
> >
> > Agreed; GLK's minimum cdclk goes down to 79.2 mhz so it makes sense that
> > it needs to be handled differently than CNL (and beyond).  I.e., this
> > isn't a pure revert of the original patch.
> 
> unfortunately it seems this fix that was done is not holding up in wider 
> testing. It now looks we need to enforce the constraint in one form or 
> another for newer platforms as well. We can't revert the revert as that 
> will bring the boot flicker back, so we'll need something else.
> 
> Now as we've gone back-and-forth multiple times, I want to get some early 
> feedback before opting for one path or another.
> 
> To recap in short:
>  - audio driver calls i915 acomp get_power() multiple times during boot
> 	-> this can cause annoying flicker at boot on platforms where
> 	   each get_power() leads to a modeset change
> 	- example: https://gitlab.freedesktop.org/drm/intel/issues/913
>  - systems with more complex audio subsystems (DSP enabled) will be 
>    calling get_power() many more times (as i915 iDisp link is needed in 
>    multiple phases of audio controller, DSP and codecs initialization), 
>    making the flicker worse
> 
> I've gone through (once more) possible options, and it starts to seem that 
> trying to minimize # of get_power() cycles is not going to work well in 
> the long run. We could certainly reduce number of distinct get_power 
> calls e.g. during boot by refactoring the audio DSP setup, but there would 
> still be more than a few, and it's not just the boot. We now need to call 
> get_power() when the audio controller comes out from runtime suspend 
> (otherwise the HDA link is not ok between i915 and audio). This can be pretty 
> annoying if there are visible artifacts to the end-user in such a case
> where potentially no HDMI/DP monitors are even connected).
> 
> 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, 
> but generally (depending on the available dividers provided), it's not 
> going to be guaranteed to be flicker-free.
> 
> So how about: We move the glk_force_audio_cdclk() logic from 
> intel_audio.c:i915_audio_component_get_power() to acomp init.
> This has some notable implications:
> 
>  - audio driver can safely call get_power without worrying 
>    about creating display artifacts,
>  - on some platforms, with specific HDA link params in BIOS, 
>    this will mean some lower CDCLK frequencies, will not be used
>    at all
> 	- e.g. on ICL system with 96Mhz BCLK for iDisp, 172800 and
>    	  180000 are blocked out, and 192000 is the practical minimum
> 	  unless you unload the audio driver
> 	- if BCLK is 48Mhz, no impact to CDCLK selection on ICL
> 
> Any chance to get this through? I understand this effectively removes the 
> lower clocks from some systems, so this needs to be evaluated carefully.
> 
> I don't really have other options on the table now. We could maybe use 
> idle-timers to delay powering off the audio domain until certain amount of 
> inactivity, but this is both ugly and won't solve the full thing. Or we 
> just keep reducing get_power() calls on audio side, and just mitigate the 
> the severity of the flicker -- again not fully solving the problem.
> 
> Thoughts and feedback is welcome.
> 
> Br, Kai

That sounds reasonable to me.  But it's basically the i915 stuff, so
I'd leave the decision to you guys :)

My another quick thought after reading this mail is whether we can
simply remove glk_force_audio_cdclk(false) in
i915_audio_component_put_power().  In this way, a flicker should be
reduced, at most only once at boot time, and the CDCLK is lowered only
when the audio is really used (once).

Or, similarly, it can be put into *_component_bind() and *_unbind()
instead of *_get_power() and *_put_power().  This indicates that the
corresponding audio device really exists.


thanks,

Takashi
Kai Vehmanen March 10, 2020, 11:20 a.m. UTC | #6
Hey,

On Mon, 9 Mar 2020, Takashi Iwai wrote:

> On Fri, 06 Mar 2020 17:45:44 +0100, Kai Vehmanen wrote:
>> unfortunately it seems this fix that was done is not holding up in wider 
>> testing. It now looks we need to enforce the constraint in one form or 
[...]
>> So how about: We move the glk_force_audio_cdclk() logic from 
>> intel_audio.c:i915_audio_component_get_power() to acomp init.
>> This has some notable implications:
> 
> That sounds reasonable to me.  But it's basically the i915 stuff, so
> I'd leave the decision to you guys :)

thanks Takashi --let's wait for the comments. I'll add also Ville who 
wrote the original glk_force_audio() code diretly to the thread.

> My another quick thought after reading this mail is whether we can
> simply remove glk_force_audio_cdclk(false) in
> i915_audio_component_put_power().  In this way, a flicker should be
> reduced, at most only once at boot time, and the CDCLK is lowered only
> when the audio is really used (once).

If we could really limit this to actual first-time use (i.e. only if 
actual playback to HDMI/DP is done), that would be interesting compromise 
indeed, but as the ALSA side probe will call get_power, this will have 
limited benefit. I think this is in the end same as:

> Or, similarly, it can be put into *_component_bind() and *_unbind()
> instead of *_get_power() and *_put_power().  This indicates that the
> corresponding audio device really exists.

... doing it bind. But yes, you are right, bind() and unbind() would be 
the appropriate places. Then if audio driver is not loaded, the freq 
constraint is not put into effect, and similarly if audio driver is 
unloaded, cdclk constraint is released.

Br, Kai
Ville Syrjälä March 10, 2020, 1:41 p.m. UTC | #7
On Mon, Mar 09, 2020 at 11:54:52AM +0100, Takashi Iwai wrote:
> On Fri, 06 Mar 2020 17:45:44 +0100,
> Kai Vehmanen wrote:
> > 
> > Hi folks,
> > 
> > [+Takashi from ALSA]
> > 
> > On Mon, 6 Jan 2020, Matt Roper wrote:
> > >>> On Tue, Dec 31, 2019 at 04:00:07PM +0200, Kai Vehmanen wrote:
> > >>>> Revert changes done in commit f6ec9483091f ("drm/i915: extend audio
> > >>>> CDCLK>=2*BCLK constraint to more platforms"). Audio drivers
> > >
> > > Agreed; GLK's minimum cdclk goes down to 79.2 mhz so it makes sense that
> > > it needs to be handled differently than CNL (and beyond).  I.e., this
> > > isn't a pure revert of the original patch.
> > 
> > unfortunately it seems this fix that was done is not holding up in wider 
> > testing. It now looks we need to enforce the constraint in one form or 
> > another for newer platforms as well. We can't revert the revert as that 
> > will bring the boot flicker back, so we'll need something else.
> > 
> > Now as we've gone back-and-forth multiple times, I want to get some early 
> > feedback before opting for one path or another.
> > 
> > To recap in short:
> >  - audio driver calls i915 acomp get_power() multiple times during boot
> > 	-> this can cause annoying flicker at boot on platforms where
> > 	   each get_power() leads to a modeset change
> > 	- example: https://gitlab.freedesktop.org/drm/intel/issues/913
> >  - systems with more complex audio subsystems (DSP enabled) will be 
> >    calling get_power() many more times (as i915 iDisp link is needed in 
> >    multiple phases of audio controller, DSP and codecs initialization), 
> >    making the flicker worse
> > 
> > I've gone through (once more) possible options, and it starts to seem that 
> > trying to minimize # of get_power() cycles is not going to work well in 
> > the long run. We could certainly reduce number of distinct get_power 
> > calls e.g. during boot by refactoring the audio DSP setup, but there would 
> > still be more than a few, and it's not just the boot. We now need to call 
> > get_power() when the audio controller comes out from runtime suspend 
> > (otherwise the HDA link is not ok between i915 and audio). This can be pretty 
> > annoying if there are visible artifacts to the end-user in such a case
> > where potentially no HDMI/DP monitors are even connected).
> > 
> > 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, 
> > but generally (depending on the available dividers provided), it's not 
> > going to be guaranteed to be flicker-free.

There is new hw in the pipeline that should allow cdclk changes
without a full modeset.

> > 
> > So how about: We move the glk_force_audio_cdclk() logic from 
> > intel_audio.c:i915_audio_component_get_power() to acomp init.
> > This has some notable implications:
> > 
> >  - audio driver can safely call get_power without worrying 
> >    about creating display artifacts,
> >  - on some platforms, with specific HDA link params in BIOS, 
> >    this will mean some lower CDCLK frequencies, will not be used
> >    at all
> > 	- e.g. on ICL system with 96Mhz BCLK for iDisp, 172800 and
> >    	  180000 are blocked out, and 192000 is the practical minimum
> > 	  unless you unload the audio driver
> > 	- if BCLK is 48Mhz, no impact to CDCLK selection on ICL
> > 
> > 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.

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.
Kai Vehmanen March 10, 2020, 5:18 p.m. UTC | #8
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
Ville Syrjälä March 10, 2020, 6:25 p.m. UTC | #9
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?
Takashi Iwai March 10, 2020, 7:13 p.m. UTC | #10
On Tue, 10 Mar 2020 19:25:22 +0100,
Ville Syrjälä wrote:
> 
> On Tue, Mar 10, 2020 at 07:18:58PM +0200, Kai Vehmanen wrote:
> > 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 guess it doesn't happen -- at least with the legacy HD-audio and the
recent chip, if I understand correctly.  When the stream is on the
analog codec, the HDMI codec is kept closed / runtime-resumed.  And
the additional get_power() in the controller side is done only for
HSW/BDW (where the HDA-bus is dedicated to HDMI).


Takashi
Kai Vehmanen March 11, 2020, 12:16 p.m. UTC | #11
Hey,

On Tue, 10 Mar 2020, Takashi Iwai wrote:
> On Tue, 10 Mar 2020 19:25:22 +0100, Ville Syrjälä wrote:
>> On Tue, Mar 10, 2020 at 07:18:58PM +0200, Kai Vehmanen wrote:
>>> 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 guess it doesn't happen -- at least with the legacy HD-audio and the
> recent chip, if I understand correctly.  When the stream is on the
> analog codec, the HDMI codec is kept closed / runtime-resumed.  And
> the additional get_power() in the controller side is done only for
> HSW/BDW (where the HDA-bus is dedicated to HDMI).

I'm afraid it does -- I double checked the legacy driver code. Judging 
from history, since commit "ALSA: hda - Fix Skylake codec timeout", 
azx_runtime_resume() has called "display_power(chip, true)" on all 
platforms, even if the stream is on analog codec. I just recently fixed 
this logic to SOF driver as well. If you don't do this and the link 
parameters are different from hw defaults on i915 side (more likely with 
ICL and newer), you'll hit problems.

I don't currently know of a reliable way to recover. In theory, if HDMI/DP 
monitor is connected, we could do a delayed call to i915 get_power and 
reinitialize the HDA controller, but if we are actively streaming to 
analog codec when this happens, this wouldn't work.

And until very recent times, this has not really been an issue. With 
current i915, many conditions have to be met to actually hit this (only 
affects GLK platforms, you need to be using HDA analog codec, runtime PM 
needs to be enabled for HDA, etc). Problem is that going forward, there 
are going to be more platforms that are affected and this starts to show 
up in more places.

Ville: I'm checking your draft patch. I'll report back when I've 
completed testing.

Br, Kai
Takashi Iwai March 11, 2020, 4:01 p.m. UTC | #12
On Wed, 11 Mar 2020 13:16:56 +0100,
Kai Vehmanen wrote:
> 
> Hey,
> 
> On Tue, 10 Mar 2020, Takashi Iwai wrote:
> > On Tue, 10 Mar 2020 19:25:22 +0100, Ville Syrjälä wrote:
> >> On Tue, Mar 10, 2020 at 07:18:58PM +0200, Kai Vehmanen wrote:
> >>> 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 guess it doesn't happen -- at least with the legacy HD-audio and the
> > recent chip, if I understand correctly.  When the stream is on the
> > analog codec, the HDMI codec is kept closed / runtime-resumed.  And
> > the additional get_power() in the controller side is done only for
> > HSW/BDW (where the HDA-bus is dedicated to HDMI).
> 
> I'm afraid it does -- I double checked the legacy driver code. Judging 
> from history, since commit "ALSA: hda - Fix Skylake codec timeout", 
> azx_runtime_resume() has called "display_power(chip, true)" on all 
> platforms, even if the stream is on analog codec. I just recently fixed 
> this logic to SOF driver as well. If you don't do this and the link 
> parameters are different from hw defaults on i915 side (more likely with 
> ICL and newer), you'll hit problems.

Oh indeed, I overlooked that part.

> I don't currently know of a reliable way to recover. In theory, if HDMI/DP 
> monitor is connected, we could do a delayed call to i915 get_power and 
> reinitialize the HDA controller, but if we are actively streaming to 
> analog codec when this happens, this wouldn't work.
> 
> And until very recent times, this has not really been an issue. With 
> current i915, many conditions have to be met to actually hit this (only 
> affects GLK platforms, you need to be using HDA analog codec, runtime PM 
> needs to be enabled for HDA, etc). Problem is that going forward, there 
> are going to be more platforms that are affected and this starts to show 
> up in more places.

The remaining question is whether this display_power() call is still
needed for the recent chips.  Basically it's there for HSW/BDW type
chips that need already the power for the HDA link that is dedicated
to HDMI.  That is, a patch like below could work for the recent chips
that share both onboard and HDMI codecs.


Takashi

--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -992,9 +992,10 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt)
 	struct hda_codec *codec;
 	int status;
 
-	display_power(chip, true);
-	if (hda->need_i915_power)
+	if (hda->need_i915_power) {
+		display_power(chip, true);
 		snd_hdac_i915_set_bclk(bus);
+	}
 
 	/* Read STATESTS before controller reset */
 	status = azx_readw(chip, STATESTS);
@@ -1008,10 +1009,6 @@ static void __azx_runtime_resume(struct azx *chip, bool from_rt)
 				schedule_delayed_work(&codec->jackpoll_work,
 						      codec->jackpoll_interval);
 	}
-
-	/* power down again for link-controlled chips */
-	if (!hda->need_i915_power)
-		display_power(chip, false);
 }
 
 #ifdef CONFIG_PM_SLEEP
Kai Vehmanen March 11, 2020, 5:04 p.m. UTC | #13
Hey,

On Wed, 11 Mar 2020, Takashi Iwai wrote:

> The remaining question is whether this display_power() call is still
> needed for the recent chips.  Basically it's there for HSW/BDW type
> chips that need already the power for the HDA link that is dedicated
> to HDMI.  That is, a patch like below could work for the recent chips

yes, it is still needed. The newer chips (GLK, ICL and newer) have added 
sw logic in i915 get_power() that must be run before audio controller is 
initialized. So calling display_power() here is actually more critical 
than before, power up from codec driver is too late. If it's not done, 
you'll get verbs timing out (unless power management is disabled on i915 
side).

Br, Kai
Takashi Iwai March 11, 2020, 5:21 p.m. UTC | #14
On Wed, 11 Mar 2020 18:04:24 +0100,
Kai Vehmanen wrote:
> 
> Hey,
> 
> On Wed, 11 Mar 2020, Takashi Iwai wrote:
> 
> > The remaining question is whether this display_power() call is still
> > needed for the recent chips.  Basically it's there for HSW/BDW type
> > chips that need already the power for the HDA link that is dedicated
> > to HDMI.  That is, a patch like below could work for the recent chips
> 
> yes, it is still needed. The newer chips (GLK, ICL and newer) have added 
> sw logic in i915 get_power() that must be run before audio controller is 
> initialized. So calling display_power() here is actually more critical 
> than before, power up from codec driver is too late. If it's not done, 
> you'll get verbs timing out (unless power management is disabled on i915 
> side).

Alright, then it's an unavoidable case.  Then it seems rather natural
to limit the CDCLK statically when the audio device is present on the
system, rather than too frequent up and down...


Takashi
Kai Vehmanen March 12, 2020, 5:27 p.m. UTC | #15
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
Ville Syrjälä March 12, 2020, 5:48 p.m. UTC | #16
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ä March 12, 2020, 5:50 p.m. UTC | #17
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.

That would stop us from doing dynamic cdclk changes once we get the hw
that can do that properly. Rather I think I'd just hardcode the 2xbclk
requirement in i915 for the platforms that suck.
Kai Vehmanen March 13, 2020, 2:54 p.m. UTC | #18
Hey,

On Thu, 12 Mar 2020, Ville Syrjälä wrote:

> On Thu, Mar 12, 2020 at 07:27:58PM +0200, Kai Vehmanen wrote:
>> 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.
> 
> That would stop us from doing dynamic cdclk changes once we get the hw
> that can do that properly. Rather I think I'd just hardcode the 2xbclk
> requirement in i915 for the platforms that suck.

well, you can always code in both places -- i.e. have the clock 
constraints set up at bind() for older (the current) platforms, and have a 
different callplace in get_power() for newer platforms. This code is 
anyways conditional on the hardware that is used.

I now send a patch implementing this, plus code to at runtime figure
out the effective BCLK, to the list. Please have a look at comment.

Br, Kai
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
index 27710098d056..e406719a6716 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -856,7 +856,7 @@  static unsigned long i915_audio_component_get_power(struct device *kdev)
 		}
 
 		/* Force CDCLK to 2*BCLK as long as we need audio powered. */
-		if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
+		if (IS_GEMINILAKE(dev_priv))
 			glk_force_audio_cdclk(dev_priv, true);
 
 		if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
@@ -875,7 +875,7 @@  static void i915_audio_component_put_power(struct device *kdev,
 
 	/* Stop forcing CDCLK to 2*BCLK if no need for audio to be powered. */
 	if (--dev_priv->audio_power_refcount == 0)
-		if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
+		if (IS_GEMINILAKE(dev_priv))
 			glk_force_audio_cdclk(dev_priv, false);
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO, cookie);