| Message ID | 20200313144821.29592-2-kai.vehmanen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: use effective iDisp BCLK value for CDCLK calculation | expand |
On Fri, Mar 13, 2020 at 04:48:21PM +0200, Kai Vehmanen wrote: > When the iDisp HDA interface between display and audio is brought > out from reset, the link parameters must be correctly set before > reset. This requires the audio driver to call i915 get_power() > whenever it brings the HDA audio controller from reset. This is > e.g. done every time audio controller is resumed from runtime > suspend. > > The current solution of modifying min_cdclk in get_power()/put_power() > can lead to display flicker as events such as playback of UI sounds > may indirectly cause a modeset change. > > As we need to extend the CDCLK>=2*BCLK constraint to more platforms > beyond GLK, a more robust solution is needed to this problem. > > This patch moves modifying the min_cdclk at audio component bind > phase and extends coverage to all gen9+ platforms. This effectively > guarantees that whenever audio driver is loaded and bound to i915, > CDCLK is guaranteed to be such that calls to get_power()/put_power() > do not result in display artifacts. So this will now force BXT to never use the 144 MHz CDCLK too. Is that actually required? I don't remember any reports of failures on BXT. > > If 2*BCLK is below lowest CDCLK, this patch has no effect. > > If a future platform provides means to change CDCLK without > a modeset, the constraint code can be moved to get/put_power() > for these platforms. > > Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_audio.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c > index e6389b9c2044..4e4832741ecf 100644 > --- a/drivers/gpu/drm/i915/display/intel_audio.c > +++ b/drivers/gpu/drm/i915/display/intel_audio.c > @@ -902,10 +902,6 @@ static unsigned long i915_audio_component_get_power(struct device *kdev) > dev_priv->audio_freq_cntrl); > } > > - /* Force CDCLK to 2*BCLK as long as we need audio powered. */ > - if (IS_GEMINILAKE(dev_priv)) > - glk_force_audio_cdclk(dev_priv, true); > - > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > intel_de_write(dev_priv, AUD_PIN_BUF_CTL, > (intel_de_read(dev_priv, AUD_PIN_BUF_CTL) | AUD_PIN_BUF_ENABLE)); > @@ -919,11 +915,7 @@ static void i915_audio_component_put_power(struct device *kdev, > { > struct drm_i915_private *dev_priv = kdev_to_i915(kdev); > > - /* Stop forcing CDCLK to 2*BCLK if no need for audio to be powered. */ > - if (--dev_priv->audio_power_refcount == 0) > - if (IS_GEMINILAKE(dev_priv)) > - glk_force_audio_cdclk(dev_priv, false); > - > + dev_priv->audio_power_refcount--; > intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO, cookie); > } > > @@ -1114,6 +1106,13 @@ static int i915_audio_component_bind(struct device *i915_kdev, > DL_FLAG_STATELESS))) > return -ENOMEM; > > + /* > + * To avoid display flicker due to frequent CDCLK changes from > + * get/put_power(), set up CDCLK>=2*BCLK constraint here. > + */ > + if (INTEL_GEN(dev_priv) >= 9) > + glk_force_audio_cdclk(dev_priv, true); Ah, so we still keep it on the i915 side. That seems fine. We can then stop doing this once we get nicer hardware and put it back into to get/put power. The name of function is rather misleading now though. I guess we should just s/glk/intel/ since there's nothing actually hardware specific in there. > + > drm_modeset_lock_all(&dev_priv->drm); > acomp->base.ops = &i915_audio_component_ops; > acomp->base.dev = i915_kdev; > @@ -1132,6 +1131,9 @@ static void i915_audio_component_unbind(struct device *i915_kdev, > struct i915_audio_component *acomp = data; > struct drm_i915_private *dev_priv = kdev_to_i915(i915_kdev); > > + if (INTEL_GEN(dev_priv) >= 9) > + glk_force_audio_cdclk(dev_priv, false); > + > drm_modeset_lock_all(&dev_priv->drm); > acomp->base.ops = NULL; > acomp->base.dev = NULL; > -- > 2.17.1
Hey, On Fri, 13 Mar 2020, Ville Syrjälä wrote: > On Fri, Mar 13, 2020 at 04:48:21PM +0200, Kai Vehmanen wrote: >> This patch moves modifying the min_cdclk at audio component bind >> phase and extends coverage to all gen9+ platforms. This effectively > > So this will now force BXT to never use the 144 MHz CDCLK too. Is that > actually required? I don't remember any reports of failures on BXT. yes it will force it higher. I can't really explain why it works. 144Mhz is well above BCLK, so most of the time it will work, but it is still out of spec. I do know that on more recent hardware (gen12), I will get failures if I don't strictly follow the requirement. GLK is a special case as it has the 79Mhz low cdclk. I've not been able to trigger the problem on other old hardware though. So where to draw the line? It's a fair point that if the old platforms have worked so far, we should not make the change unless there is at least one data point supporting it. So the constraint would then apply for gen12+ and glk. Now thinking of another possibility, is it possible to hook code to power-up of power domains? E.g. can I hook custom code which is executed in sync with power wells going up and down? If we could reprogram AUD_FREQ_CNTRL outside the get/put_power() flow (i.e. independently from audio driver), and guarantee that if the display side is powered on, the link params are always correct, it might be possible to get away without calling get_power() from audio controller reset. Worth a shot probably before we merge this. >> + if (INTEL_GEN(dev_priv) >= 9) >> + glk_force_audio_cdclk(dev_priv, true); > > Ah, so we still keep it on the i915 side. That seems fine. We can then > stop doing this once we get nicer hardware and put it back into to > get/put power. OTOH, if we restrict this to gen12+-and-glk, the glk_ prefix makes sense again. Br, Kai
Hey Ville and others, On Fri, 13 Mar 2020, Kai Vehmanen wrote: > On Fri, 13 Mar 2020, Ville Syrjälä wrote: > Now thinking of another possibility, is it possible to hook code to > power-up of power domains? E.g. can I hook custom code which is executed [...] > If we could reprogram AUD_FREQ_CNTRL outside the get/put_power() flow > (i.e. independently from audio driver), and guarantee that if the display > side is powered on, the link params are always correct, it might be > possible to get away without calling get_power() from audio controller ... no need to answer this. I made an ugly hack directly to intel_display_power.c and added a hook to audio restore, but, but, this does not seem to help. I can still hit hangs unless I bump cdclk before the reset -- i.e. restoring AUD_FREQ_CNTRL is not enough. Br, Kai
Hi Ville and others, On Fri, 13 Mar 2020, Kai Vehmanen wrote: > I do know that on more recent hardware (gen12), I will get failures if I > don't strictly follow the requirement. GLK is a special case as it has the > 79Mhz low cdclk. I've not been able to trigger the problem on other old > hardware though. So where to draw the line? > > It's a fair point that if the old platforms have worked so far, we should > not make the change unless there is at least one data point supporting it. > So the constraint would then apply for gen12+ and glk. given the not-so-great options on the table, I went back (again) looking for other solutions and came with a curious finding. The forced Codec Wake interface added to gen9 in 2015, seems to fix the gen12 codec probe issues as well, without the cdclk bump. So it looks like the problem is not about the CDCLK being high enough, but something in hardware state that is fixed by doing a modeset. Or, as it seems to be the case, by using the chickebits to force the codec wake (and no need for modeset). Based on hw specs, there is no valid reason to limit the forced codec wake only to gen9, so I went and sent a patch for this now. I've tested this on various gen9/10/11/12 platforms and also asked our validation to test it out, and seems everything seems good. I also tried an experiment where I added the codec wake patch, but removed the forced cdclk bump on GLK, but that still fails, so on GLK, the CDCLK bump is still mandatory, and likely a separate problem. So this doesn't solve all the problems, but at least we can fix the currently reported bugs without causing any new compromises at system level. Br, Kai
diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c index e6389b9c2044..4e4832741ecf 100644 --- a/drivers/gpu/drm/i915/display/intel_audio.c +++ b/drivers/gpu/drm/i915/display/intel_audio.c @@ -902,10 +902,6 @@ static unsigned long i915_audio_component_get_power(struct device *kdev) dev_priv->audio_freq_cntrl); } - /* Force CDCLK to 2*BCLK as long as we need audio powered. */ - if (IS_GEMINILAKE(dev_priv)) - glk_force_audio_cdclk(dev_priv, true); - if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) intel_de_write(dev_priv, AUD_PIN_BUF_CTL, (intel_de_read(dev_priv, AUD_PIN_BUF_CTL) | AUD_PIN_BUF_ENABLE)); @@ -919,11 +915,7 @@ static void i915_audio_component_put_power(struct device *kdev, { struct drm_i915_private *dev_priv = kdev_to_i915(kdev); - /* Stop forcing CDCLK to 2*BCLK if no need for audio to be powered. */ - if (--dev_priv->audio_power_refcount == 0) - if (IS_GEMINILAKE(dev_priv)) - glk_force_audio_cdclk(dev_priv, false); - + dev_priv->audio_power_refcount--; intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO, cookie); } @@ -1114,6 +1106,13 @@ static int i915_audio_component_bind(struct device *i915_kdev, DL_FLAG_STATELESS))) return -ENOMEM; + /* + * To avoid display flicker due to frequent CDCLK changes from + * get/put_power(), set up CDCLK>=2*BCLK constraint here. + */ + if (INTEL_GEN(dev_priv) >= 9) + glk_force_audio_cdclk(dev_priv, true); + drm_modeset_lock_all(&dev_priv->drm); acomp->base.ops = &i915_audio_component_ops; acomp->base.dev = i915_kdev; @@ -1132,6 +1131,9 @@ static void i915_audio_component_unbind(struct device *i915_kdev, struct i915_audio_component *acomp = data; struct drm_i915_private *dev_priv = kdev_to_i915(i915_kdev); + if (INTEL_GEN(dev_priv) >= 9) + glk_force_audio_cdclk(dev_priv, false); + drm_modeset_lock_all(&dev_priv->drm); acomp->base.ops = NULL; acomp->base.dev = NULL;
When the iDisp HDA interface between display and audio is brought out from reset, the link parameters must be correctly set before reset. This requires the audio driver to call i915 get_power() whenever it brings the HDA audio controller from reset. This is e.g. done every time audio controller is resumed from runtime suspend. The current solution of modifying min_cdclk in get_power()/put_power() can lead to display flicker as events such as playback of UI sounds may indirectly cause a modeset change. As we need to extend the CDCLK>=2*BCLK constraint to more platforms beyond GLK, a more robust solution is needed to this problem. This patch moves modifying the min_cdclk at audio component bind phase and extends coverage to all gen9+ platforms. This effectively guarantees that whenever audio driver is loaded and bound to i915, CDCLK is guaranteed to be such that calls to get_power()/put_power() do not result in display artifacts. If 2*BCLK is below lowest CDCLK, this patch has no effect. If a future platform provides means to change CDCLK without a modeset, the constraint code can be moved to get/put_power() for these platforms. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> --- drivers/gpu/drm/i915/display/intel_audio.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)