| 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 |
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
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
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
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
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
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
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.
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
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?
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
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
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
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
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
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
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.
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.
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 --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);
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(-)