[Intel-gfx] [PATCH 1/2] drm/i915: use effective iDisp BCLK value for CDCLK calculation

[Intel-gfx] [PATCH 1/2] drm/i915: use effective iDisp BCLK value for CDCLK calculation

[Kai Vehmanen]

Instead of assuming maximum value of BCLK (96Mhz), use the actual value
as configured by BIOS.

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_audio.c | 13 ++++++++++++-
 drivers/gpu/drm/i915/display/intel_audio.h |  1 +
 drivers/gpu/drm/i915/display/intel_cdclk.c |  6 ++++--
 drivers/gpu/drm/i915/i915_reg.h            |  2 ++
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
index 19bf206037c2..e6389b9c2044 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -810,11 +810,22 @@ void intel_init_audio_hooks(struct drm_i915_private *dev_priv)
 	}
 }
 
+int intel_get_audio_bclk(struct drm_i915_private *dev_priv)
+{
+	if (INTEL_GEN(dev_priv) >= 9 &&
+	    (dev_priv->audio_freq_cntrl & AUD_FREQ_48M_BCLK))
+		return 48000;
+
+	return 96000;
+}
+
 static int glk_force_audio_cdclk_commit(struct intel_atomic_state *state,
 					struct intel_crtc *crtc,
 					bool enable)
 {
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	struct intel_cdclk_state *cdclk_state;
+	int bclk = intel_get_audio_bclk(dev_priv);
 	int ret;
 
 	/* need to hold at least one crtc lock for the global state */
@@ -827,7 +838,7 @@ static int glk_force_audio_cdclk_commit(struct intel_atomic_state *state,
 		return PTR_ERR(cdclk_state);
 
 	cdclk_state->force_min_cdclk_changed = true;
-	cdclk_state->force_min_cdclk = enable ? 2 * 96000 : 0;
+	cdclk_state->force_min_cdclk = enable ? 2 * bclk : 0;
 
 	ret = intel_atomic_lock_global_state(&cdclk_state->base);
 	if (ret)
diff --git a/drivers/gpu/drm/i915/display/intel_audio.h b/drivers/gpu/drm/i915/display/intel_audio.h
index a3657c7a7ba2..e4116d969d5e 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.h
+++ b/drivers/gpu/drm/i915/display/intel_audio.h
@@ -20,5 +20,6 @@ void intel_audio_codec_disable(struct intel_encoder *encoder,
 			       const struct drm_connector_state *old_conn_state);
 void intel_audio_init(struct drm_i915_private *dev_priv);
 void intel_audio_deinit(struct drm_i915_private *dev_priv);
+int intel_get_audio_bclk(struct drm_i915_private *dev_priv);
 
 #endif /* __INTEL_AUDIO_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 979a0241fdcb..98a45a296cbb 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -23,6 +23,7 @@
 
 #include "intel_atomic.h"
 #include "intel_cdclk.h"
+#include "intel_audio.h"
 #include "intel_display_types.h"
 #include "intel_sideband.h"
 
@@ -2001,6 +2002,7 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv =
 		to_i915(crtc_state->uapi.crtc->dev);
+	int bclk = intel_get_audio_bclk(dev_priv);
 	int min_cdclk;
 
 	if (!crtc_state->hw.enable)
@@ -2032,10 +2034,10 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
 
 	/*
 	 * According to BSpec, "The CD clock frequency must be at least twice
-	 * the frequency of the Azalia BCLK." and BCLK is 96 MHz by default.
+	 * the frequency of the Azalia BCLK.".
 	 */
 	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
-		min_cdclk = max(2 * 96000, min_cdclk);
+		min_cdclk = max(2 * bclk, min_cdclk);
 
 	/*
 	 * "For DP audio configuration, cdclk frequency shall be set to
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 79ae9654dac9..aabcc31de676 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9294,6 +9294,8 @@ enum {
 #define   SKL_AUD_CODEC_WAKE_SIGNAL		(1 << 15)
 
 #define AUD_FREQ_CNTRL			_MMIO(0x65900)
+#define   AUD_FREQ_48M_BCLK		REG_BIT(3)
+#define   AUD_FREQ_96M_BCLK		REG_BIT(4)
 #define AUD_PIN_BUF_CTL		_MMIO(0x48414)
 #define   AUD_PIN_BUF_ENABLE		REG_BIT(31)
 
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/2] drm/i915: move audio CDCLK constraint setup to bind/unbind

[Kai Vehmanen]

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(-)

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;
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: move audio CDCLK constraint setup to bind/unbind

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: use effective iDisp BCLK value for CDCLK calculation

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: use effective iDisp BCLK value for CDCLK calculation
URL   : https://patchwork.freedesktop.org/series/74682/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8135 -> Patchwork_16966
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_16966 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_16966, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16966/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_16966:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_parallel@basic:
    - fi-skl-6770hq:      [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8135/fi-skl-6770hq/igt@gem_exec_parallel@basic.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16966/fi-skl-6770hq/igt@gem_exec_parallel@basic.html

  
Known issues
------------

  Here are the changes found in Patchwork_16966 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@execlists:
    - fi-bxt-dsi:         [PASS][3] -> [INCOMPLETE][4] ([fdo#103927] / [i915#656])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8135/fi-bxt-dsi/igt@i915_selftest@live@execlists.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16966/fi-bxt-dsi/igt@i915_selftest@live@execlists.html

  * igt@i915_selftest@live@gem_contexts:
    - fi-cml-s:           [PASS][5] -> [DMESG-FAIL][6] ([i915#877])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8135/fi-cml-s/igt@i915_selftest@live@gem_contexts.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16966/fi-cml-s/igt@i915_selftest@live@gem_contexts.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@execlists:
    - fi-icl-dsi:         [DMESG-FAIL][7] ([fdo#108569]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8135/fi-icl-dsi/igt@i915_selftest@live@execlists.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16966/fi-icl-dsi/igt@i915_selftest@live@execlists.html

  * igt@i915_selftest@live@gem_contexts:
    - fi-cfl-8700k:       [INCOMPLETE][9] ([i915#424]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8135/fi-cfl-8700k/igt@i915_selftest@live@gem_contexts.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16966/fi-cfl-8700k/igt@i915_selftest@live@gem_contexts.html

  * igt@i915_selftest@live@hangcheck:
    - fi-ivb-3770:        [INCOMPLETE][11] ([i915#1405]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8135/fi-ivb-3770/igt@i915_selftest@live@hangcheck.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16966/fi-ivb-3770/igt@i915_selftest@live@hangcheck.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][13] ([fdo#111407]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8135/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16966/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_flip@basic-flip-vs-wf_vblank:
    - fi-bsw-n3050:       [FAIL][15] ([i915#34]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8135/fi-bsw-n3050/igt@kms_flip@basic-flip-vs-wf_vblank.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16966/fi-bsw-n3050/igt@kms_flip@basic-flip-vs-wf_vblank.html

  
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#111407]: https://bugs.freedesktop.org/show_bug.cgi?id=111407
  [i915#1405]: https://gitlab.freedesktop.org/drm/intel/issues/1405
  [i915#34]: https://gitlab.freedesktop.org/drm/intel/issues/34
  [i915#424]: https://gitlab.freedesktop.org/drm/intel/issues/424
  [i915#656]: https://gitlab.freedesktop.org/drm/intel/issues/656
  [i915#877]: https://gitlab.freedesktop.org/drm/intel/issues/877


Participating hosts (39 -> 44)
------------------------------

  Additional (11): fi-kbl-7560u fi-tgl-dsi fi-byt-j1900 fi-hsw-peppy fi-glk-dsi fi-snb-2520m fi-ilk-650 fi-gdg-551 fi-bsw-kefka fi-blb-e6850 fi-snb-2600 
  Missing    (6): fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-bsw-nick fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8135 -> Patchwork_16966

  CI-20190529: 20190529
  CI_DRM_8135: 17c19ee50a6cbb514b59f9f2d8d7d4f088a56300 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5508: 89d644600a1a9f08794cc7106b63758df40ce1d8 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16966: d64fcc34c0cd85d0e2013fe81eb0bf561c0d4786 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

d64fcc34c0cd drm/i915: move audio CDCLK constraint setup to bind/unbind
bdf38470761d drm/i915: use effective iDisp BCLK value for CDCLK calculation

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16966/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: move audio CDCLK constraint setup to bind/unbind

[Kai Vehmanen]

[-- Attachment #1: Type: text/plain, Size: 1892 bytes --]

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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: move audio CDCLK constraint setup to bind/unbind

[Kai Vehmanen]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: move audio CDCLK constraint setup to bind/unbind

[Kai Vehmanen]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx