All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
@ 2018-06-19 22:01 Abhay Kumar
  2018-06-19 22:13 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev6) Patchwork
                   ` (8 more replies)
  0 siblings, 9 replies; 17+ messages in thread
From: Abhay Kumar @ 2018-06-19 22:01 UTC (permalink / raw)
  To: intel-gfx, ville.syrjala; +Cc: jani.nikula

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

CDCLK has to be at least twice the BLCK regardless of audio. Audio
driver has to probe using this hook and increase the clock even in
absence of any display.

v2: Use atomic refcount for get_power, put_power so that we can
    call each once(Abhay).
v3: Reset power well 2 to avoid any transaction on iDisp link
    during cdclk change(Abhay).
v4: Remove Power well 2 reset workaround(Ville).
v5: Remove unwanted Power well 2 register defined in v4(Abhay).

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 ++
 drivers/gpu/drm/i915/intel_audio.c   | 67 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_cdclk.c   | 29 +++++-----------
 drivers/gpu/drm/i915/intel_display.c |  7 +++-
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 5 files changed, 83 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6104d7115054..a4a386a5db69 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1702,6 +1702,7 @@ struct drm_i915_private {
 	unsigned int hpll_freq;
 	unsigned int fdi_pll_freq;
 	unsigned int czclk_freq;
+	u32 get_put_refcount;
 
 	struct {
 		/*
@@ -1719,6 +1720,8 @@ struct drm_i915_private {
 		struct intel_cdclk_state actual;
 		/* The current hardware cdclk state */
 		struct intel_cdclk_state hw;
+
+		int force_min_cdclk;
 	} cdclk;
 
 	/**
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 3ea566f99450..ca8f04c7cbb3 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -618,7 +618,6 @@ void intel_audio_codec_enable(struct intel_encoder *encoder,
 
 	if (!connector->eld[0])
 		return;
-
 	DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
 			 connector->base.id,
 			 connector->name,
@@ -713,14 +712,74 @@ void intel_init_audio_hooks(struct drm_i915_private *dev_priv)
 	}
 }
 
+static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
+				bool enable)
+{
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_atomic_state *state;
+	int ret;
+
+	drm_modeset_acquire_init(&ctx, 0);
+	state = drm_atomic_state_alloc(&dev_priv->drm);
+	if (WARN_ON(!state))
+		return;
+
+	state->acquire_ctx = &ctx;
+
+retry:
+	to_intel_atomic_state(state)->modeset = true;
+	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
+		enable ? 2 * 96000 : 0;
+
+	/*
+	 * Protects dev_priv->cdclk.force_min_cdclk
+	 * Need to lock this here in case we have no active pipes
+	 * and thus wouldn't lock it during the commit otherwise.
+	 */
+	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, &ctx);
+	if (!ret)
+		ret = drm_atomic_commit(state);
+
+	if (ret == -EDEADLK) {
+		drm_atomic_state_clear(state);
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	}
+
+	WARN_ON(ret);
+
+	drm_atomic_state_put(state);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+}
+
 static void i915_audio_component_get_power(struct device *kdev)
 {
-	intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
+	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
+
+	dev_priv->get_put_refcount++;
+
+	/* Force cdclk to 2*BCLK during first time get power call */
+	if (dev_priv->get_put_refcount == 1)
+		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
+			glk_force_audio_cdclk(dev_priv, true);
+
+	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
 }
 
 static void i915_audio_component_put_power(struct device *kdev)
 {
-	intel_display_power_put(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
+	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
+
+	dev_priv->get_put_refcount--;
+
+	/* Force required cdclk during last time put power call */
+	if (dev_priv->get_put_refcount == 0)
+		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
+			glk_force_audio_cdclk(dev_priv, false);
+
+	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
 }
 
 static void i915_audio_component_codec_wake_override(struct device *kdev,
@@ -959,7 +1018,7 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv)
 		/* continue with reduced functionality */
 		return;
 	}
-
+	dev_priv->get_put_refcount = 0;
 	dev_priv->audio_component_registered = true;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 8ed7bd052e46..0f0aea900ceb 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -2153,19 +2153,8 @@ 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.
-	 *
-	 * FIXME: Check the actual, not default, BCLK being used.
-	 *
-	 * FIXME: This does not depend on ->has_audio because the higher CDCLK
-	 * is required for audio probe, also when there are no audio capable
-	 * displays connected at probe time. This leads to unnecessarily high
-	 * CDCLK when audio is not required.
-	 *
-	 * FIXME: This limit is only applied when there are displays connected
-	 * at probe time. If we probe without displays, we'll still end up using
-	 * the platform minimum CDCLK, failing audio probe.
 	 */
-	if (INTEL_GEN(dev_priv) >= 9)
+	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
 		min_cdclk = max(2 * 96000, min_cdclk);
 
 	/*
@@ -2205,7 +2194,7 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
 		intel_state->min_cdclk[i] = min_cdclk;
 	}
 
-	min_cdclk = 0;
+	min_cdclk = intel_state->cdclk.force_min_cdclk;
 	for_each_pipe(dev_priv, pipe)
 		min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
 
@@ -2266,7 +2255,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
 		vlv_calc_voltage_level(dev_priv, cdclk);
 
 	if (!intel_state->active_crtcs) {
-		cdclk = vlv_calc_cdclk(dev_priv, 0);
+		cdclk = vlv_calc_cdclk(dev_priv, intel_state->cdclk.force_min_cdclk);
 
 		intel_state->cdclk.actual.cdclk = cdclk;
 		intel_state->cdclk.actual.voltage_level =
@@ -2299,7 +2288,7 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
 		bdw_calc_voltage_level(cdclk);
 
 	if (!intel_state->active_crtcs) {
-		cdclk = bdw_calc_cdclk(0);
+		cdclk = bdw_calc_cdclk(intel_state->cdclk.force_min_cdclk);
 
 		intel_state->cdclk.actual.cdclk = cdclk;
 		intel_state->cdclk.actual.voltage_level =
@@ -2371,7 +2360,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
 		skl_calc_voltage_level(cdclk);
 
 	if (!intel_state->active_crtcs) {
-		cdclk = skl_calc_cdclk(0, vco);
+		cdclk = skl_calc_cdclk(intel_state->cdclk.force_min_cdclk, vco);
 
 		intel_state->cdclk.actual.vco = vco;
 		intel_state->cdclk.actual.cdclk = cdclk;
@@ -2410,10 +2399,10 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
 
 	if (!intel_state->active_crtcs) {
 		if (IS_GEMINILAKE(dev_priv)) {
-			cdclk = glk_calc_cdclk(0);
+			 cdclk = glk_calc_cdclk(intel_state->cdclk.force_min_cdclk);
 			vco = glk_de_pll_vco(dev_priv, cdclk);
 		} else {
-			cdclk = bxt_calc_cdclk(0);
+			cdclk = bxt_calc_cdclk(intel_state->cdclk.force_min_cdclk);
 			vco = bxt_de_pll_vco(dev_priv, cdclk);
 		}
 
@@ -2449,7 +2438,7 @@ static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
 		    cnl_compute_min_voltage_level(intel_state));
 
 	if (!intel_state->active_crtcs) {
-		cdclk = cnl_calc_cdclk(0);
+		cdclk = cnl_calc_cdclk(intel_state->cdclk.force_min_cdclk);
 		vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
 
 		intel_state->cdclk.actual.vco = vco;
@@ -2482,7 +2471,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
 	intel_state->cdclk.logical.cdclk = cdclk;
 
 	if (!intel_state->active_crtcs) {
-		cdclk = icl_calc_cdclk(0, ref);
+		cdclk = icl_calc_cdclk(intel_state->cdclk.force_min_cdclk, ref);
 		vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
 
 		intel_state->cdclk.actual.vco = vco;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 17c590b42fd7..3ee1c1f5419d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12162,6 +12162,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 		return -EINVAL;
 	}
 
+	/* keep the current setting */
+	if (!intel_state->modeset)
+		intel_state->cdclk.force_min_cdclk = dev_priv->cdclk.force_min_cdclk;
+
 	intel_state->modeset = true;
 	intel_state->active_crtcs = dev_priv->active_crtcs;
 	intel_state->cdclk.logical = dev_priv->cdclk.logical;
@@ -12257,7 +12261,7 @@ static int intel_atomic_check(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *crtc_state;
 	int ret, i;
-	bool any_ms = false;
+	bool any_ms = intel_state->modeset;
 
 	/* Catch I915_MODE_FLAG_INHERITED */
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
@@ -12805,6 +12809,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		dev_priv->active_crtcs = intel_state->active_crtcs;
 		dev_priv->cdclk.logical = intel_state->cdclk.logical;
 		dev_priv->cdclk.actual = intel_state->cdclk.actual;
+		dev_priv->cdclk.force_min_cdclk = intel_state->cdclk.force_min_cdclk;
 	}
 
 	drm_atomic_state_get(state);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8641583842be..0da17ad056ec 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -459,6 +459,8 @@ struct intel_atomic_state {
 		 * state only when all crtc's are DPMS off.
 		 */
 		struct intel_cdclk_state actual;
+
+		int force_min_cdclk;
 	} cdclk;
 
 	bool dpll_set, modeset;
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev6)
  2018-06-19 22:01 [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled Abhay Kumar
@ 2018-06-19 22:13 ` Patchwork
  2018-06-19 22:14 ` ✗ Fi.CI.SPARSE: " Patchwork
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-06-19 22:13 UTC (permalink / raw)
  To: Abhay Kumar; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev6)
URL   : https://patchwork.freedesktop.org/series/42459/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
61069992fd74 drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
-:62: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#62: FILE: drivers/gpu/drm/i915/intel_audio.c:726:
+static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
+				bool enable)

-:207: WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (16, 25)
#207: FILE: drivers/gpu/drm/i915/intel_cdclk.c:2436:
 		if (IS_GEMINILAKE(dev_priv)) {
+			 cdclk = glk_calc_cdclk(intel_state->cdclk.force_min_cdclk);

total: 0 errors, 1 warnings, 1 checks, 219 lines checked

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* ✗ Fi.CI.SPARSE: warning for drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev6)
  2018-06-19 22:01 [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled Abhay Kumar
  2018-06-19 22:13 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev6) Patchwork
@ 2018-06-19 22:14 ` Patchwork
  2018-06-19 22:29 ` ✓ Fi.CI.BAT: success " Patchwork
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-06-19 22:14 UTC (permalink / raw)
  To: Abhay Kumar; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev6)
URL   : https://patchwork.freedesktop.org/series/42459/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
-O:drivers/gpu/drm/i915/intel_cdclk.c:2204:29: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_cdclk.c:2193:29: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_cdclk.c:2245:29: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_cdclk.c:2245:29: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_cdclk.c:2234:29: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_cdclk.c:2234:29: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3690:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3693:16: warning: expression using sizeof(void)

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev6)
  2018-06-19 22:01 [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled Abhay Kumar
  2018-06-19 22:13 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev6) Patchwork
  2018-06-19 22:14 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2018-06-19 22:29 ` Patchwork
  2018-06-19 23:46 ` ✓ Fi.CI.IGT: " Patchwork
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-06-19 22:29 UTC (permalink / raw)
  To: Abhay Kumar; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev6)
URL   : https://patchwork.freedesktop.org/series/42459/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4344 -> Patchwork_9362 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42459/revisions/6/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)

    igt@gem_ctx_create@basic-files:
      fi-skl-gvtdvm:      PASS -> INCOMPLETE (fdo#105600)

    igt@gem_exec_gttfill@basic:
      fi-byt-n2820:       PASS -> FAIL (fdo#106744)

    igt@kms_pipe_crc_basic@read-crc-pipe-c:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106238)

    igt@pm_rpm@basic-rte:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106097)

    
    ==== Possible fixes ====

    igt@kms_flip@basic-flip-vs-modeset:
      fi-glk-j4005:       DMESG-WARN (fdo#106097) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-glk-j4005:       DMESG-WARN (fdo#106238) -> PASS

    
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105600 https://bugs.freedesktop.org/show_bug.cgi?id=105600
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
  fdo#106238 https://bugs.freedesktop.org/show_bug.cgi?id=106238
  fdo#106744 https://bugs.freedesktop.org/show_bug.cgi?id=106744


== Participating hosts (43 -> 37) ==

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-j1900 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


== Build changes ==

    * Linux: CI_DRM_4344 -> Patchwork_9362

  CI_DRM_4344: 922a029a1d0ecf5c7e5c86a372a5d3df3fd35483 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4524: 9ab9268fa7eeda0a7ea6eb2ab02bb6c5b9c91ba0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9362: 61069992fd7457cb12695c90199c205ce47d8f92 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

61069992fd74 drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled

== Logs ==

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* ✓ Fi.CI.IGT: success for drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev6)
  2018-06-19 22:01 [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled Abhay Kumar
                   ` (2 preceding siblings ...)
  2018-06-19 22:29 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-06-19 23:46 ` Patchwork
  2018-06-21 18:43 ` [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled Kumar, Abhay
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-06-19 23:46 UTC (permalink / raw)
  To: Abhay Kumar; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev6)
URL   : https://patchwork.freedesktop.org/series/42459/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4344_full -> Patchwork_9362_full =

== Summary - WARNING ==

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

  

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-ctx-dirty-render:
      shard-kbl:          SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_gtt:
      shard-glk:          PASS -> FAIL (fdo#105347)

    igt@drv_selftest@live_hangcheck:
      shard-kbl:          PASS -> DMESG-FAIL (fdo#106560, fdo#106947)

    igt@kms_flip@2x-plain-flip-fb-recreate-interruptible:
      shard-glk:          PASS -> FAIL (fdo#100368)

    igt@kms_flip@dpms-vs-vblank-race:
      shard-hsw:          PASS -> FAIL (fdo#103060)

    igt@kms_flip@flip-vs-expired-vblank:
      shard-glk:          PASS -> FAIL (fdo#102887)

    igt@kms_flip_tiling@flip-x-tiled:
      shard-glk:          PASS -> FAIL (fdo#104724, fdo#103822)

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@drv_suspend@shrink:
      shard-snb:          FAIL (fdo#106886) -> PASS
      shard-apl:          INCOMPLETE (fdo#103927) -> PASS
      shard-glk:          FAIL (fdo#106886) -> PASS

    igt@gem_ctx_switch@basic-all-light:
      shard-hsw:          INCOMPLETE (fdo#103540) -> PASS

    igt@kms_cursor_legacy@cursora-vs-flipa-toggle:
      shard-glk:          DMESG-WARN (fdo#105763) -> PASS

    igt@kms_flip@2x-plain-flip-ts-check:
      shard-glk:          FAIL (fdo#100368) -> PASS

    igt@kms_flip@dpms-vs-vblank-race-interruptible:
      shard-glk:          FAIL (fdo#103060) -> PASS

    igt@kms_flip_tiling@flip-to-y-tiled:
      shard-glk:          FAIL (fdo#104724, fdo#103822) -> PASS +1

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-shrfb-plflip-blt:
      shard-glk:          FAIL (fdo#103167, fdo#104724) -> PASS

    igt@kms_setmode@basic:
      shard-hsw:          FAIL (fdo#99912) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4344 -> Patchwork_9362

  CI_DRM_4344: 922a029a1d0ecf5c7e5c86a372a5d3df3fd35483 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4524: 9ab9268fa7eeda0a7ea6eb2ab02bb6c5b9c91ba0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9362: 61069992fd7457cb12695c90199c205ce47d8f92 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
  2018-06-19 22:01 [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled Abhay Kumar
                   ` (3 preceding siblings ...)
  2018-06-19 23:46 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-06-21 18:43 ` Kumar, Abhay
  2018-06-21 20:16   ` Du,Wenkai
  2018-09-12 13:18 ` Imre Deak
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Kumar, Abhay @ 2018-06-21 18:43 UTC (permalink / raw)
  To: Kumar, Abhay, intel-gfx, Syrjala, Ville, Du, Wenkai; +Cc: Nikula, Jani

+ Wenkai

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Abhay Kumar
Sent: Tuesday, June 19, 2018 3:01 PM
To: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>
Cc: Nikula, Jani <jani.nikula@intel.com>
Subject: [Intel-gfx] [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

CDCLK has to be at least twice the BLCK regardless of audio. Audio driver has to probe using this hook and increase the clock even in absence of any display.

v2: Use atomic refcount for get_power, put_power so that we can
    call each once(Abhay).
v3: Reset power well 2 to avoid any transaction on iDisp link
    during cdclk change(Abhay).
v4: Remove Power well 2 reset workaround(Ville).
v5: Remove unwanted Power well 2 register defined in v4(Abhay).

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 ++
 drivers/gpu/drm/i915/intel_audio.c   | 67 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_cdclk.c   | 29 +++++-----------
 drivers/gpu/drm/i915/intel_display.c |  7 +++-
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 5 files changed, 83 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 6104d7115054..a4a386a5db69 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1702,6 +1702,7 @@ struct drm_i915_private {
 	unsigned int hpll_freq;
 	unsigned int fdi_pll_freq;
 	unsigned int czclk_freq;
+	u32 get_put_refcount;
 
 	struct {
 		/*
@@ -1719,6 +1720,8 @@ struct drm_i915_private {
 		struct intel_cdclk_state actual;
 		/* The current hardware cdclk state */
 		struct intel_cdclk_state hw;
+
+		int force_min_cdclk;
 	} cdclk;
 
 	/**
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 3ea566f99450..ca8f04c7cbb3 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -618,7 +618,6 @@ void intel_audio_codec_enable(struct intel_encoder *encoder,
 
 	if (!connector->eld[0])
 		return;
-
 	DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
 			 connector->base.id,
 			 connector->name,
@@ -713,14 +712,74 @@ void intel_init_audio_hooks(struct drm_i915_private *dev_priv)
 	}
 }
 
+static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
+				bool enable)
+{
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_atomic_state *state;
+	int ret;
+
+	drm_modeset_acquire_init(&ctx, 0);
+	state = drm_atomic_state_alloc(&dev_priv->drm);
+	if (WARN_ON(!state))
+		return;
+
+	state->acquire_ctx = &ctx;
+
+retry:
+	to_intel_atomic_state(state)->modeset = true;
+	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
+		enable ? 2 * 96000 : 0;
+
+	/*
+	 * Protects dev_priv->cdclk.force_min_cdclk
+	 * Need to lock this here in case we have no active pipes
+	 * and thus wouldn't lock it during the commit otherwise.
+	 */
+	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, &ctx);
+	if (!ret)
+		ret = drm_atomic_commit(state);
+
+	if (ret == -EDEADLK) {
+		drm_atomic_state_clear(state);
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	}
+
+	WARN_ON(ret);
+
+	drm_atomic_state_put(state);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+}
+
 static void i915_audio_component_get_power(struct device *kdev)  {
-	intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
+	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
+
+	dev_priv->get_put_refcount++;
+
+	/* Force cdclk to 2*BCLK during first time get power call */
+	if (dev_priv->get_put_refcount == 1)
+		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
+			glk_force_audio_cdclk(dev_priv, true);
+
+	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
 }
 
 static void i915_audio_component_put_power(struct device *kdev)  {
-	intel_display_power_put(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
+	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
+
+	dev_priv->get_put_refcount--;
+
+	/* Force required cdclk during last time put power call */
+	if (dev_priv->get_put_refcount == 0)
+		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
+			glk_force_audio_cdclk(dev_priv, false);
+
+	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
 }
 
 static void i915_audio_component_codec_wake_override(struct device *kdev, @@ -959,7 +1018,7 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv)
 		/* continue with reduced functionality */
 		return;
 	}
-
+	dev_priv->get_put_refcount = 0;
 	dev_priv->audio_component_registered = true;  }
 
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 8ed7bd052e46..0f0aea900ceb 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -2153,19 +2153,8 @@ 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.
-	 *
-	 * FIXME: Check the actual, not default, BCLK being used.
-	 *
-	 * FIXME: This does not depend on ->has_audio because the higher CDCLK
-	 * is required for audio probe, also when there are no audio capable
-	 * displays connected at probe time. This leads to unnecessarily high
-	 * CDCLK when audio is not required.
-	 *
-	 * FIXME: This limit is only applied when there are displays connected
-	 * at probe time. If we probe without displays, we'll still end up using
-	 * the platform minimum CDCLK, failing audio probe.
 	 */
-	if (INTEL_GEN(dev_priv) >= 9)
+	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
 		min_cdclk = max(2 * 96000, min_cdclk);
 
 	/*
@@ -2205,7 +2194,7 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
 		intel_state->min_cdclk[i] = min_cdclk;
 	}
 
-	min_cdclk = 0;
+	min_cdclk = intel_state->cdclk.force_min_cdclk;
 	for_each_pipe(dev_priv, pipe)
 		min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
 
@@ -2266,7 +2255,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
 		vlv_calc_voltage_level(dev_priv, cdclk);
 
 	if (!intel_state->active_crtcs) {
-		cdclk = vlv_calc_cdclk(dev_priv, 0);
+		cdclk = vlv_calc_cdclk(dev_priv, intel_state->cdclk.force_min_cdclk);
 
 		intel_state->cdclk.actual.cdclk = cdclk;
 		intel_state->cdclk.actual.voltage_level = @@ -2299,7 +2288,7 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
 		bdw_calc_voltage_level(cdclk);
 
 	if (!intel_state->active_crtcs) {
-		cdclk = bdw_calc_cdclk(0);
+		cdclk = bdw_calc_cdclk(intel_state->cdclk.force_min_cdclk);
 
 		intel_state->cdclk.actual.cdclk = cdclk;
 		intel_state->cdclk.actual.voltage_level = @@ -2371,7 +2360,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
 		skl_calc_voltage_level(cdclk);
 
 	if (!intel_state->active_crtcs) {
-		cdclk = skl_calc_cdclk(0, vco);
+		cdclk = skl_calc_cdclk(intel_state->cdclk.force_min_cdclk, vco);
 
 		intel_state->cdclk.actual.vco = vco;
 		intel_state->cdclk.actual.cdclk = cdclk; @@ -2410,10 +2399,10 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
 
 	if (!intel_state->active_crtcs) {
 		if (IS_GEMINILAKE(dev_priv)) {
-			cdclk = glk_calc_cdclk(0);
+			 cdclk = glk_calc_cdclk(intel_state->cdclk.force_min_cdclk);
 			vco = glk_de_pll_vco(dev_priv, cdclk);
 		} else {
-			cdclk = bxt_calc_cdclk(0);
+			cdclk = bxt_calc_cdclk(intel_state->cdclk.force_min_cdclk);
 			vco = bxt_de_pll_vco(dev_priv, cdclk);
 		}
 
@@ -2449,7 +2438,7 @@ static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
 		    cnl_compute_min_voltage_level(intel_state));
 
 	if (!intel_state->active_crtcs) {
-		cdclk = cnl_calc_cdclk(0);
+		cdclk = cnl_calc_cdclk(intel_state->cdclk.force_min_cdclk);
 		vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
 
 		intel_state->cdclk.actual.vco = vco;
@@ -2482,7 +2471,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
 	intel_state->cdclk.logical.cdclk = cdclk;
 
 	if (!intel_state->active_crtcs) {
-		cdclk = icl_calc_cdclk(0, ref);
+		cdclk = icl_calc_cdclk(intel_state->cdclk.force_min_cdclk, ref);
 		vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
 
 		intel_state->cdclk.actual.vco = vco;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 17c590b42fd7..3ee1c1f5419d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12162,6 +12162,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 		return -EINVAL;
 	}
 
+	/* keep the current setting */
+	if (!intel_state->modeset)
+		intel_state->cdclk.force_min_cdclk = dev_priv->cdclk.force_min_cdclk;
+
 	intel_state->modeset = true;
 	intel_state->active_crtcs = dev_priv->active_crtcs;
 	intel_state->cdclk.logical = dev_priv->cdclk.logical; @@ -12257,7 +12261,7 @@ static int intel_atomic_check(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *crtc_state;
 	int ret, i;
-	bool any_ms = false;
+	bool any_ms = intel_state->modeset;
 
 	/* Catch I915_MODE_FLAG_INHERITED */
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, @@ -12805,6 +12809,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		dev_priv->active_crtcs = intel_state->active_crtcs;
 		dev_priv->cdclk.logical = intel_state->cdclk.logical;
 		dev_priv->cdclk.actual = intel_state->cdclk.actual;
+		dev_priv->cdclk.force_min_cdclk = intel_state->cdclk.force_min_cdclk;
 	}
 
 	drm_atomic_state_get(state);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8641583842be..0da17ad056ec 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -459,6 +459,8 @@ struct intel_atomic_state {
 		 * state only when all crtc's are DPMS off.
 		 */
 		struct intel_cdclk_state actual;
+
+		int force_min_cdclk;
 	} cdclk;
 
 	bool dpll_set, modeset;
--
2.7.4

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
  2018-06-21 18:43 ` [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled Kumar, Abhay
@ 2018-06-21 20:16   ` Du,Wenkai
  2018-07-03  5:06     ` Kumar, Abhay
  0 siblings, 1 reply; 17+ messages in thread
From: Du,Wenkai @ 2018-06-21 20:16 UTC (permalink / raw)
  To: Kumar, Abhay, intel-gfx, Syrjala, Ville; +Cc: Nikula, Jani



On 6/21/2018 11:43 AM, Kumar, Abhay wrote:
> + Wenkai
> 
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Abhay Kumar
> Sent: Tuesday, June 19, 2018 3:01 PM
> To: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>
> Cc: Nikula, Jani <jani.nikula@intel.com>
> Subject: [Intel-gfx] [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> CDCLK has to be at least twice the BLCK regardless of audio. Audio driver has to probe using this hook and increase the clock even in absence of any display.
> 
> v2: Use atomic refcount for get_power, put_power so that we can
>      call each once(Abhay).
> v3: Reset power well 2 to avoid any transaction on iDisp link
>      during cdclk change(Abhay).
> v4: Remove Power well 2 reset workaround(Ville).
> v5: Remove unwanted Power well 2 register defined in v4(Abhay).
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
Tested-by: Wenkai Du <wenkai.du@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h      |  3 ++
>   drivers/gpu/drm/i915/intel_audio.c   | 67 +++++++++++++++++++++++++++++++++---
>   drivers/gpu/drm/i915/intel_cdclk.c   | 29 +++++-----------
>   drivers/gpu/drm/i915/intel_display.c |  7 +++-
>   drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>   5 files changed, 83 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 6104d7115054..a4a386a5db69 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1702,6 +1702,7 @@ struct drm_i915_private {
>   	unsigned int hpll_freq;
>   	unsigned int fdi_pll_freq;
>   	unsigned int czclk_freq;
> +	u32 get_put_refcount;
>   
>   	struct {
>   		/*
> @@ -1719,6 +1720,8 @@ struct drm_i915_private {
>   		struct intel_cdclk_state actual;
>   		/* The current hardware cdclk state */
>   		struct intel_cdclk_state hw;
> +
> +		int force_min_cdclk;
>   	} cdclk;
>   
>   	/**
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 3ea566f99450..ca8f04c7cbb3 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -618,7 +618,6 @@ void intel_audio_codec_enable(struct intel_encoder *encoder,
>   
>   	if (!connector->eld[0])
>   		return;
> -
>   	DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
>   			 connector->base.id,
>   			 connector->name,
> @@ -713,14 +712,74 @@ void intel_init_audio_hooks(struct drm_i915_private *dev_priv)
>   	}
>   }
>   
> +static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
> +				bool enable)
> +{
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_atomic_state *state;
> +	int ret;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +	state = drm_atomic_state_alloc(&dev_priv->drm);
> +	if (WARN_ON(!state))
> +		return;
> +
> +	state->acquire_ctx = &ctx;
> +
> +retry:
> +	to_intel_atomic_state(state)->modeset = true;
> +	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
> +		enable ? 2 * 96000 : 0;
> +
> +	/*
> +	 * Protects dev_priv->cdclk.force_min_cdclk
> +	 * Need to lock this here in case we have no active pipes
> +	 * and thus wouldn't lock it during the commit otherwise.
> +	 */
> +	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, &ctx);
> +	if (!ret)
> +		ret = drm_atomic_commit(state);
> +
> +	if (ret == -EDEADLK) {
> +		drm_atomic_state_clear(state);
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +
> +	WARN_ON(ret);
> +
> +	drm_atomic_state_put(state);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
> +
>   static void i915_audio_component_get_power(struct device *kdev)  {
> -	intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> +
> +	dev_priv->get_put_refcount++;
> +
> +	/* Force cdclk to 2*BCLK during first time get power call */
> +	if (dev_priv->get_put_refcount == 1)
> +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> +			glk_force_audio_cdclk(dev_priv, true);
> +
> +	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
>   }
>   
>   static void i915_audio_component_put_power(struct device *kdev)  {
> -	intel_display_power_put(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> +
> +	dev_priv->get_put_refcount--;
> +
> +	/* Force required cdclk during last time put power call */
> +	if (dev_priv->get_put_refcount == 0)
> +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> +			glk_force_audio_cdclk(dev_priv, false);
> +
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
>   }
>   
>   static void i915_audio_component_codec_wake_override(struct device *kdev, @@ -959,7 +1018,7 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv)
>   		/* continue with reduced functionality */
>   		return;
>   	}
> -
> +	dev_priv->get_put_refcount = 0;
>   	dev_priv->audio_component_registered = true;  }
>   
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 8ed7bd052e46..0f0aea900ceb 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -2153,19 +2153,8 @@ 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.
> -	 *
> -	 * FIXME: Check the actual, not default, BCLK being used.
> -	 *
> -	 * FIXME: This does not depend on ->has_audio because the higher CDCLK
> -	 * is required for audio probe, also when there are no audio capable
> -	 * displays connected at probe time. This leads to unnecessarily high
> -	 * CDCLK when audio is not required.
> -	 *
> -	 * FIXME: This limit is only applied when there are displays connected
> -	 * at probe time. If we probe without displays, we'll still end up using
> -	 * the platform minimum CDCLK, failing audio probe.
>   	 */
> -	if (INTEL_GEN(dev_priv) >= 9)
> +	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
>   		min_cdclk = max(2 * 96000, min_cdclk);
>   
>   	/*
> @@ -2205,7 +2194,7 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
>   		intel_state->min_cdclk[i] = min_cdclk;
>   	}
>   
> -	min_cdclk = 0;
> +	min_cdclk = intel_state->cdclk.force_min_cdclk;
>   	for_each_pipe(dev_priv, pipe)
>   		min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
>   
> @@ -2266,7 +2255,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
>   		vlv_calc_voltage_level(dev_priv, cdclk);
>   
>   	if (!intel_state->active_crtcs) {
> -		cdclk = vlv_calc_cdclk(dev_priv, 0);
> +		cdclk = vlv_calc_cdclk(dev_priv, intel_state->cdclk.force_min_cdclk);
>   
>   		intel_state->cdclk.actual.cdclk = cdclk;
>   		intel_state->cdclk.actual.voltage_level = @@ -2299,7 +2288,7 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
>   		bdw_calc_voltage_level(cdclk);
>   
>   	if (!intel_state->active_crtcs) {
> -		cdclk = bdw_calc_cdclk(0);
> +		cdclk = bdw_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>   
>   		intel_state->cdclk.actual.cdclk = cdclk;
>   		intel_state->cdclk.actual.voltage_level = @@ -2371,7 +2360,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>   		skl_calc_voltage_level(cdclk);
>   
>   	if (!intel_state->active_crtcs) {
> -		cdclk = skl_calc_cdclk(0, vco);
> +		cdclk = skl_calc_cdclk(intel_state->cdclk.force_min_cdclk, vco);
>   
>   		intel_state->cdclk.actual.vco = vco;
>   		intel_state->cdclk.actual.cdclk = cdclk; @@ -2410,10 +2399,10 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
>   
>   	if (!intel_state->active_crtcs) {
>   		if (IS_GEMINILAKE(dev_priv)) {
> -			cdclk = glk_calc_cdclk(0);
> +			 cdclk = glk_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>   			vco = glk_de_pll_vco(dev_priv, cdclk);
>   		} else {
> -			cdclk = bxt_calc_cdclk(0);
> +			cdclk = bxt_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>   			vco = bxt_de_pll_vco(dev_priv, cdclk);
>   		}
>   
> @@ -2449,7 +2438,7 @@ static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
>   		    cnl_compute_min_voltage_level(intel_state));
>   
>   	if (!intel_state->active_crtcs) {
> -		cdclk = cnl_calc_cdclk(0);
> +		cdclk = cnl_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>   		vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
>   
>   		intel_state->cdclk.actual.vco = vco;
> @@ -2482,7 +2471,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
>   	intel_state->cdclk.logical.cdclk = cdclk;
>   
>   	if (!intel_state->active_crtcs) {
> -		cdclk = icl_calc_cdclk(0, ref);
> +		cdclk = icl_calc_cdclk(intel_state->cdclk.force_min_cdclk, ref);
>   		vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
>   
>   		intel_state->cdclk.actual.vco = vco;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 17c590b42fd7..3ee1c1f5419d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12162,6 +12162,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>   		return -EINVAL;
>   	}
>   
> +	/* keep the current setting */
> +	if (!intel_state->modeset)
> +		intel_state->cdclk.force_min_cdclk = dev_priv->cdclk.force_min_cdclk;
> +
>   	intel_state->modeset = true;
>   	intel_state->active_crtcs = dev_priv->active_crtcs;
>   	intel_state->cdclk.logical = dev_priv->cdclk.logical; @@ -12257,7 +12261,7 @@ static int intel_atomic_check(struct drm_device *dev,
>   	struct drm_crtc *crtc;
>   	struct drm_crtc_state *old_crtc_state, *crtc_state;
>   	int ret, i;
> -	bool any_ms = false;
> +	bool any_ms = intel_state->modeset;
>   
>   	/* Catch I915_MODE_FLAG_INHERITED */
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, @@ -12805,6 +12809,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>   		dev_priv->active_crtcs = intel_state->active_crtcs;
>   		dev_priv->cdclk.logical = intel_state->cdclk.logical;
>   		dev_priv->cdclk.actual = intel_state->cdclk.actual;
> +		dev_priv->cdclk.force_min_cdclk = intel_state->cdclk.force_min_cdclk;
>   	}
>   
>   	drm_atomic_state_get(state);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8641583842be..0da17ad056ec 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -459,6 +459,8 @@ struct intel_atomic_state {
>   		 * state only when all crtc's are DPMS off.
>   		 */
>   		struct intel_cdclk_state actual;
> +
> +		int force_min_cdclk;
>   	} cdclk;
>   
>   	bool dpll_set, modeset;
> --
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
  2018-06-21 20:16   ` Du,Wenkai
@ 2018-07-03  5:06     ` Kumar, Abhay
  0 siblings, 0 replies; 17+ messages in thread
From: Kumar, Abhay @ 2018-07-03  5:06 UTC (permalink / raw)
  To: Du, Wenkai, intel-gfx, Syrjala, Ville; +Cc: Nikula, Jani

Hi Ville,
 
  Can we please get this merged to DINQ?

Regards,
Abhay

-----Original Message-----
From: Du, Wenkai 
Sent: Thursday, June 21, 2018 1:16 PM
To: Kumar, Abhay <abhay.kumar@intel.com>; intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>
Cc: Nikula, Jani <jani.nikula@intel.com>
Subject: Re: [Intel-gfx] [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled



On 6/21/2018 11:43 AM, Kumar, Abhay wrote:
> + Wenkai
> 
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On 
> Behalf Of Abhay Kumar
> Sent: Tuesday, June 19, 2018 3:01 PM
> To: intel-gfx@lists.freedesktop.org; Syrjala, Ville 
> <ville.syrjala@intel.com>
> Cc: Nikula, Jani <jani.nikula@intel.com>
> Subject: [Intel-gfx] [PATCH v5] drm/i915: Force 2*96 MHz cdclk on 
> glk/cnl when audio power is enabled
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> CDCLK has to be at least twice the BLCK regardless of audio. Audio driver has to probe using this hook and increase the clock even in absence of any display.
> 
> v2: Use atomic refcount for get_power, put_power so that we can
>      call each once(Abhay).
> v3: Reset power well 2 to avoid any transaction on iDisp link
>      during cdclk change(Abhay).
> v4: Remove Power well 2 reset workaround(Ville).
> v5: Remove unwanted Power well 2 register defined in v4(Abhay).
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
Tested-by: Wenkai Du <wenkai.du@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h      |  3 ++
>   drivers/gpu/drm/i915/intel_audio.c   | 67 +++++++++++++++++++++++++++++++++---
>   drivers/gpu/drm/i915/intel_cdclk.c   | 29 +++++-----------
>   drivers/gpu/drm/i915/intel_display.c |  7 +++-
>   drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>   5 files changed, 83 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> b/drivers/gpu/drm/i915/i915_drv.h index 6104d7115054..a4a386a5db69 
> 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1702,6 +1702,7 @@ struct drm_i915_private {
>   	unsigned int hpll_freq;
>   	unsigned int fdi_pll_freq;
>   	unsigned int czclk_freq;
> +	u32 get_put_refcount;
>   
>   	struct {
>   		/*
> @@ -1719,6 +1720,8 @@ struct drm_i915_private {
>   		struct intel_cdclk_state actual;
>   		/* The current hardware cdclk state */
>   		struct intel_cdclk_state hw;
> +
> +		int force_min_cdclk;
>   	} cdclk;
>   
>   	/**
> diff --git a/drivers/gpu/drm/i915/intel_audio.c 
> b/drivers/gpu/drm/i915/intel_audio.c
> index 3ea566f99450..ca8f04c7cbb3 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -618,7 +618,6 @@ void intel_audio_codec_enable(struct intel_encoder 
> *encoder,
>   
>   	if (!connector->eld[0])
>   		return;
> -
>   	DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
>   			 connector->base.id,
>   			 connector->name,
> @@ -713,14 +712,74 @@ void intel_init_audio_hooks(struct drm_i915_private *dev_priv)
>   	}
>   }
>   
> +static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
> +				bool enable)
> +{
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_atomic_state *state;
> +	int ret;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +	state = drm_atomic_state_alloc(&dev_priv->drm);
> +	if (WARN_ON(!state))
> +		return;
> +
> +	state->acquire_ctx = &ctx;
> +
> +retry:
> +	to_intel_atomic_state(state)->modeset = true;
> +	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
> +		enable ? 2 * 96000 : 0;
> +
> +	/*
> +	 * Protects dev_priv->cdclk.force_min_cdclk
> +	 * Need to lock this here in case we have no active pipes
> +	 * and thus wouldn't lock it during the commit otherwise.
> +	 */
> +	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, &ctx);
> +	if (!ret)
> +		ret = drm_atomic_commit(state);
> +
> +	if (ret == -EDEADLK) {
> +		drm_atomic_state_clear(state);
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +
> +	WARN_ON(ret);
> +
> +	drm_atomic_state_put(state);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
> +
>   static void i915_audio_component_get_power(struct device *kdev)  {
> -	intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> +
> +	dev_priv->get_put_refcount++;
> +
> +	/* Force cdclk to 2*BCLK during first time get power call */
> +	if (dev_priv->get_put_refcount == 1)
> +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> +			glk_force_audio_cdclk(dev_priv, true);
> +
> +	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
>   }
>   
>   static void i915_audio_component_put_power(struct device *kdev)  {
> -	intel_display_power_put(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> +
> +	dev_priv->get_put_refcount--;
> +
> +	/* Force required cdclk during last time put power call */
> +	if (dev_priv->get_put_refcount == 0)
> +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> +			glk_force_audio_cdclk(dev_priv, false);
> +
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
>   }
>   
>   static void i915_audio_component_codec_wake_override(struct device *kdev, @@ -959,7 +1018,7 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv)
>   		/* continue with reduced functionality */
>   		return;
>   	}
> -
> +	dev_priv->get_put_refcount = 0;
>   	dev_priv->audio_component_registered = true;  }
>   
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c 
> b/drivers/gpu/drm/i915/intel_cdclk.c
> index 8ed7bd052e46..0f0aea900ceb 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -2153,19 +2153,8 @@ 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.
> -	 *
> -	 * FIXME: Check the actual, not default, BCLK being used.
> -	 *
> -	 * FIXME: This does not depend on ->has_audio because the higher CDCLK
> -	 * is required for audio probe, also when there are no audio capable
> -	 * displays connected at probe time. This leads to unnecessarily high
> -	 * CDCLK when audio is not required.
> -	 *
> -	 * FIXME: This limit is only applied when there are displays connected
> -	 * at probe time. If we probe without displays, we'll still end up using
> -	 * the platform minimum CDCLK, failing audio probe.
>   	 */
> -	if (INTEL_GEN(dev_priv) >= 9)
> +	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
>   		min_cdclk = max(2 * 96000, min_cdclk);
>   
>   	/*
> @@ -2205,7 +2194,7 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
>   		intel_state->min_cdclk[i] = min_cdclk;
>   	}
>   
> -	min_cdclk = 0;
> +	min_cdclk = intel_state->cdclk.force_min_cdclk;
>   	for_each_pipe(dev_priv, pipe)
>   		min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
>   
> @@ -2266,7 +2255,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
>   		vlv_calc_voltage_level(dev_priv, cdclk);
>   
>   	if (!intel_state->active_crtcs) {
> -		cdclk = vlv_calc_cdclk(dev_priv, 0);
> +		cdclk = vlv_calc_cdclk(dev_priv, 
> +intel_state->cdclk.force_min_cdclk);
>   
>   		intel_state->cdclk.actual.cdclk = cdclk;
>   		intel_state->cdclk.actual.voltage_level = @@ -2299,7 +2288,7 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
>   		bdw_calc_voltage_level(cdclk);
>   
>   	if (!intel_state->active_crtcs) {
> -		cdclk = bdw_calc_cdclk(0);
> +		cdclk = bdw_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>   
>   		intel_state->cdclk.actual.cdclk = cdclk;
>   		intel_state->cdclk.actual.voltage_level = @@ -2371,7 +2360,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>   		skl_calc_voltage_level(cdclk);
>   
>   	if (!intel_state->active_crtcs) {
> -		cdclk = skl_calc_cdclk(0, vco);
> +		cdclk = skl_calc_cdclk(intel_state->cdclk.force_min_cdclk, vco);
>   
>   		intel_state->cdclk.actual.vco = vco;
>   		intel_state->cdclk.actual.cdclk = cdclk; @@ -2410,10 +2399,10 @@ 
> static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
>   
>   	if (!intel_state->active_crtcs) {
>   		if (IS_GEMINILAKE(dev_priv)) {
> -			cdclk = glk_calc_cdclk(0);
> +			 cdclk = glk_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>   			vco = glk_de_pll_vco(dev_priv, cdclk);
>   		} else {
> -			cdclk = bxt_calc_cdclk(0);
> +			cdclk = bxt_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>   			vco = bxt_de_pll_vco(dev_priv, cdclk);
>   		}
>   
> @@ -2449,7 +2438,7 @@ static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
>   		    cnl_compute_min_voltage_level(intel_state));
>   
>   	if (!intel_state->active_crtcs) {
> -		cdclk = cnl_calc_cdclk(0);
> +		cdclk = cnl_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>   		vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
>   
>   		intel_state->cdclk.actual.vco = vco; @@ -2482,7 +2471,7 @@ static 
> int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
>   	intel_state->cdclk.logical.cdclk = cdclk;
>   
>   	if (!intel_state->active_crtcs) {
> -		cdclk = icl_calc_cdclk(0, ref);
> +		cdclk = icl_calc_cdclk(intel_state->cdclk.force_min_cdclk, ref);
>   		vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
>   
>   		intel_state->cdclk.actual.vco = vco; diff --git 
> a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 17c590b42fd7..3ee1c1f5419d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12162,6 +12162,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>   		return -EINVAL;
>   	}
>   
> +	/* keep the current setting */
> +	if (!intel_state->modeset)
> +		intel_state->cdclk.force_min_cdclk = 
> +dev_priv->cdclk.force_min_cdclk;
> +
>   	intel_state->modeset = true;
>   	intel_state->active_crtcs = dev_priv->active_crtcs;
>   	intel_state->cdclk.logical = dev_priv->cdclk.logical; @@ -12257,7 +12261,7 @@ static int intel_atomic_check(struct drm_device *dev,
>   	struct drm_crtc *crtc;
>   	struct drm_crtc_state *old_crtc_state, *crtc_state;
>   	int ret, i;
> -	bool any_ms = false;
> +	bool any_ms = intel_state->modeset;
>   
>   	/* Catch I915_MODE_FLAG_INHERITED */
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, @@ -12805,6 +12809,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>   		dev_priv->active_crtcs = intel_state->active_crtcs;
>   		dev_priv->cdclk.logical = intel_state->cdclk.logical;
>   		dev_priv->cdclk.actual = intel_state->cdclk.actual;
> +		dev_priv->cdclk.force_min_cdclk = 
> +intel_state->cdclk.force_min_cdclk;
>   	}
>   
>   	drm_atomic_state_get(state);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 8641583842be..0da17ad056ec 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -459,6 +459,8 @@ struct intel_atomic_state {
>   		 * state only when all crtc's are DPMS off.
>   		 */
>   		struct intel_cdclk_state actual;
> +
> +		int force_min_cdclk;
>   	} cdclk;
>   
>   	bool dpll_set, modeset;
> --
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
  2018-06-19 22:01 [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled Abhay Kumar
                   ` (4 preceding siblings ...)
  2018-06-21 18:43 ` [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled Kumar, Abhay
@ 2018-09-12 13:18 ` Imre Deak
  2018-09-12 13:18   ` Imre Deak
  2018-09-12 13:49   ` Ville Syrjälä
  2018-09-13 15:07 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev8) Patchwork
                   ` (2 subsequent siblings)
  8 siblings, 2 replies; 17+ messages in thread
From: Imre Deak @ 2018-09-12 13:18 UTC (permalink / raw)
  To: Abhay Kumar; +Cc: jani.nikula, intel-gfx, ville.syrjala

Hi Kumar, Takashi,

On Tue, Jun 19, 2018 at 03:01:11PM -0700, Abhay Kumar wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> CDCLK has to be at least twice the BLCK regardless of audio. Audio
> driver has to probe using this hook and increase the clock even in
> absence of any display.
> 
> v2: Use atomic refcount for get_power, put_power so that we can
>     call each once(Abhay).
> v3: Reset power well 2 to avoid any transaction on iDisp link
>     during cdclk change(Abhay).
> v4: Remove Power well 2 reset workaround(Ville).
> v5: Remove unwanted Power well 2 register defined in v4(Abhay).
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  3 ++
>  drivers/gpu/drm/i915/intel_audio.c   | 67 +++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_cdclk.c   | 29 +++++-----------
>  drivers/gpu/drm/i915/intel_display.c |  7 +++-
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  5 files changed, 83 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6104d7115054..a4a386a5db69 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1702,6 +1702,7 @@ struct drm_i915_private {
>  	unsigned int hpll_freq;
>  	unsigned int fdi_pll_freq;
>  	unsigned int czclk_freq;
> +	u32 get_put_refcount;
>  
>  	struct {
>  		/*
> @@ -1719,6 +1720,8 @@ struct drm_i915_private {
>  		struct intel_cdclk_state actual;
>  		/* The current hardware cdclk state */
>  		struct intel_cdclk_state hw;
> +
> +		int force_min_cdclk;
>  	} cdclk;
>  
>  	/**
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 3ea566f99450..ca8f04c7cbb3 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -618,7 +618,6 @@ void intel_audio_codec_enable(struct intel_encoder *encoder,
>  
>  	if (!connector->eld[0])
>  		return;
> -
>  	DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
>  			 connector->base.id,
>  			 connector->name,
> @@ -713,14 +712,74 @@ void intel_init_audio_hooks(struct drm_i915_private *dev_priv)
>  	}
>  }
>  
> +static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
> +				bool enable)
> +{
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_atomic_state *state;
> +	int ret;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +	state = drm_atomic_state_alloc(&dev_priv->drm);
> +	if (WARN_ON(!state))
> +		return;
> +
> +	state->acquire_ctx = &ctx;
> +
> +retry:
> +	to_intel_atomic_state(state)->modeset = true;
> +	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
> +		enable ? 2 * 96000 : 0;
> +
> +	/*
> +	 * Protects dev_priv->cdclk.force_min_cdclk
> +	 * Need to lock this here in case we have no active pipes
> +	 * and thus wouldn't lock it during the commit otherwise.
> +	 */
> +	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, &ctx);
> +	if (!ret)
> +		ret = drm_atomic_commit(state);

Ville mentioned a potential dead-lock problem here: a normal modeset
enabling an HDMI/DP output with an audio sink will call the
drm_audio_component_audio_ops::pin_eld_notify() callback with the
connection_mutex already held. This callback in turn could call
drm_audio_component_ops::get_power()/put_power() callbacks and
dead-lock at the above drm_modeset_lock() call.

Looks like that

   sound/pci/hda/patch_hdmi.c / intel_pin_eld_notify()->
   check_presence_and_report()->
   hdmi_present_sense()

would prevent this, but for instance

   sound/soc/codecs/hdac_hdmi.c / hdac_hdmi_eld_notify_cb()->
   hdac_hdmi_present_sense()->
   hdac_hdmi_jack_report()->
   snd_soc_jack_report()->
   snd_soc_dapm_sync()->
   snd_soc_dapm_sync_unlocked()->
   dapm_power_widgets()->
   dapm_pre_sequence_async()

could call pm_runtime_get_sync() and so I guess also the aops
get_power() hook.

Could someone in your team check if the above can indeed happen?

> +
> +	if (ret == -EDEADLK) {
> +		drm_atomic_state_clear(state);
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +
> +	WARN_ON(ret);
> +
> +	drm_atomic_state_put(state);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
> +
>  static void i915_audio_component_get_power(struct device *kdev)
>  {
> -	intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> +
> +	dev_priv->get_put_refcount++;
> +
> +	/* Force cdclk to 2*BCLK during first time get power call */
> +	if (dev_priv->get_put_refcount == 1)
> +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> +			glk_force_audio_cdclk(dev_priv, true);
> +
> +	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);

We can't guarantee that the power wells belonging to the audio power
domain are off at this point (since an external output can keep them on).
But according to the tests you did Kumar, this doesn't even seem to be
required, the only requirement is that CDCLK is raised.

So to make the sequence behave the same all the time (and not hide some
problem happening occasionally) and to have it symmetric with the one
in i915_audio_component_put_power() we should get the audio power domain
before bumping CDCLK.

>  }
>  
>  static void i915_audio_component_put_power(struct device *kdev)
>  {
> -	intel_display_power_put(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> +
> +	dev_priv->get_put_refcount--;
> +
> +	/* Force required cdclk during last time put power call */
> +	if (dev_priv->get_put_refcount == 0)
> +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> +			glk_force_audio_cdclk(dev_priv, false);
> +
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
>  }
>  
>  static void i915_audio_component_codec_wake_override(struct device *kdev,
> @@ -959,7 +1018,7 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv)
>  		/* continue with reduced functionality */
>  		return;
>  	}
> -
> +	dev_priv->get_put_refcount = 0;
>  	dev_priv->audio_component_registered = true;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 8ed7bd052e46..0f0aea900ceb 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -2153,19 +2153,8 @@ 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.
> -	 *
> -	 * FIXME: Check the actual, not default, BCLK being used.
> -	 *
> -	 * FIXME: This does not depend on ->has_audio because the higher CDCLK
> -	 * is required for audio probe, also when there are no audio capable
> -	 * displays connected at probe time. This leads to unnecessarily high
> -	 * CDCLK when audio is not required.
> -	 *
> -	 * FIXME: This limit is only applied when there are displays connected
> -	 * at probe time. If we probe without displays, we'll still end up using
> -	 * the platform minimum CDCLK, failing audio probe.
>  	 */
> -	if (INTEL_GEN(dev_priv) >= 9)
> +	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
>  		min_cdclk = max(2 * 96000, min_cdclk);
>  
>  	/*
> @@ -2205,7 +2194,7 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
>  		intel_state->min_cdclk[i] = min_cdclk;
>  	}
>  
> -	min_cdclk = 0;
> +	min_cdclk = intel_state->cdclk.force_min_cdclk;
>  	for_each_pipe(dev_priv, pipe)
>  		min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
>  
> @@ -2266,7 +2255,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
>  		vlv_calc_voltage_level(dev_priv, cdclk);
>  
>  	if (!intel_state->active_crtcs) {
> -		cdclk = vlv_calc_cdclk(dev_priv, 0);
> +		cdclk = vlv_calc_cdclk(dev_priv, intel_state->cdclk.force_min_cdclk);
>  
>  		intel_state->cdclk.actual.cdclk = cdclk;
>  		intel_state->cdclk.actual.voltage_level =
> @@ -2299,7 +2288,7 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
>  		bdw_calc_voltage_level(cdclk);
>  
>  	if (!intel_state->active_crtcs) {
> -		cdclk = bdw_calc_cdclk(0);
> +		cdclk = bdw_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>  
>  		intel_state->cdclk.actual.cdclk = cdclk;
>  		intel_state->cdclk.actual.voltage_level =
> @@ -2371,7 +2360,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  		skl_calc_voltage_level(cdclk);
>  
>  	if (!intel_state->active_crtcs) {
> -		cdclk = skl_calc_cdclk(0, vco);
> +		cdclk = skl_calc_cdclk(intel_state->cdclk.force_min_cdclk, vco);
>  
>  		intel_state->cdclk.actual.vco = vco;
>  		intel_state->cdclk.actual.cdclk = cdclk;
> @@ -2410,10 +2399,10 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
>  
>  	if (!intel_state->active_crtcs) {
>  		if (IS_GEMINILAKE(dev_priv)) {
> -			cdclk = glk_calc_cdclk(0);
> +			 cdclk = glk_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>  			vco = glk_de_pll_vco(dev_priv, cdclk);
>  		} else {
> -			cdclk = bxt_calc_cdclk(0);
> +			cdclk = bxt_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>  			vco = bxt_de_pll_vco(dev_priv, cdclk);
>  		}
>  
> @@ -2449,7 +2438,7 @@ static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  		    cnl_compute_min_voltage_level(intel_state));
>  
>  	if (!intel_state->active_crtcs) {
> -		cdclk = cnl_calc_cdclk(0);
> +		cdclk = cnl_calc_cdclk(intel_state->cdclk.force_min_cdclk);
>  		vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
>  
>  		intel_state->cdclk.actual.vco = vco;
> @@ -2482,7 +2471,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	intel_state->cdclk.logical.cdclk = cdclk;
>  
>  	if (!intel_state->active_crtcs) {
> -		cdclk = icl_calc_cdclk(0, ref);
> +		cdclk = icl_calc_cdclk(intel_state->cdclk.force_min_cdclk, ref);
>  		vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
>  
>  		intel_state->cdclk.actual.vco = vco;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 17c590b42fd7..3ee1c1f5419d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12162,6 +12162,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>  		return -EINVAL;
>  	}
>  
> +	/* keep the current setting */
> +	if (!intel_state->modeset)

Ville, I'd use here a dedicated flag instead.

> +		intel_state->cdclk.force_min_cdclk = dev_priv->cdclk.force_min_cdclk;
> +
>  	intel_state->modeset = true;
>  	intel_state->active_crtcs = dev_priv->active_crtcs;
>  	intel_state->cdclk.logical = dev_priv->cdclk.logical;
> @@ -12257,7 +12261,7 @@ static int intel_atomic_check(struct drm_device *dev,
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state, *crtc_state;
>  	int ret, i;
> -	bool any_ms = false;
> +	bool any_ms = intel_state->modeset;
>  
>  	/* Catch I915_MODE_FLAG_INHERITED */
>  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> @@ -12805,6 +12809,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		dev_priv->active_crtcs = intel_state->active_crtcs;
>  		dev_priv->cdclk.logical = intel_state->cdclk.logical;
>  		dev_priv->cdclk.actual = intel_state->cdclk.actual;
> +		dev_priv->cdclk.force_min_cdclk = intel_state->cdclk.force_min_cdclk;
>  	}
>  
>  	drm_atomic_state_get(state);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8641583842be..0da17ad056ec 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -459,6 +459,8 @@ struct intel_atomic_state {
>  		 * state only when all crtc's are DPMS off.
>  		 */
>  		struct intel_cdclk_state actual;
> +
> +		int force_min_cdclk;
>  	} cdclk;
>  
>  	bool dpll_set, modeset;
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
  2018-09-12 13:18 ` Imre Deak
@ 2018-09-12 13:18   ` Imre Deak
  2018-09-12 18:18     ` Takashi Iwai
  2018-09-12 13:49   ` Ville Syrjälä
  1 sibling, 1 reply; 17+ messages in thread
From: Imre Deak @ 2018-09-12 13:18 UTC (permalink / raw)
  To: Abhay Kumar, Takashi Iwai; +Cc: jani.nikula, intel-gfx, ville.syrjala

+Takashi

On Wed, Sep 12, 2018 at 04:18:12PM +0300, Imre Deak wrote:
> Hi Kumar, Takashi,
> 
> On Tue, Jun 19, 2018 at 03:01:11PM -0700, Abhay Kumar wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > CDCLK has to be at least twice the BLCK regardless of audio. Audio
> > driver has to probe using this hook and increase the clock even in
> > absence of any display.
> > 
> > v2: Use atomic refcount for get_power, put_power so that we can
> >     call each once(Abhay).
> > v3: Reset power well 2 to avoid any transaction on iDisp link
> >     during cdclk change(Abhay).
> > v4: Remove Power well 2 reset workaround(Ville).
> > v5: Remove unwanted Power well 2 register defined in v4(Abhay).
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  3 ++
> >  drivers/gpu/drm/i915/intel_audio.c   | 67 +++++++++++++++++++++++++++++++++---
> >  drivers/gpu/drm/i915/intel_cdclk.c   | 29 +++++-----------
> >  drivers/gpu/drm/i915/intel_display.c |  7 +++-
> >  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> >  5 files changed, 83 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 6104d7115054..a4a386a5db69 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1702,6 +1702,7 @@ struct drm_i915_private {
> >  	unsigned int hpll_freq;
> >  	unsigned int fdi_pll_freq;
> >  	unsigned int czclk_freq;
> > +	u32 get_put_refcount;
> >  
> >  	struct {
> >  		/*
> > @@ -1719,6 +1720,8 @@ struct drm_i915_private {
> >  		struct intel_cdclk_state actual;
> >  		/* The current hardware cdclk state */
> >  		struct intel_cdclk_state hw;
> > +
> > +		int force_min_cdclk;
> >  	} cdclk;
> >  
> >  	/**
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index 3ea566f99450..ca8f04c7cbb3 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -618,7 +618,6 @@ void intel_audio_codec_enable(struct intel_encoder *encoder,
> >  
> >  	if (!connector->eld[0])
> >  		return;
> > -
> >  	DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
> >  			 connector->base.id,
> >  			 connector->name,
> > @@ -713,14 +712,74 @@ void intel_init_audio_hooks(struct drm_i915_private *dev_priv)
> >  	}
> >  }
> >  
> > +static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
> > +				bool enable)
> > +{
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	struct drm_atomic_state *state;
> > +	int ret;
> > +
> > +	drm_modeset_acquire_init(&ctx, 0);
> > +	state = drm_atomic_state_alloc(&dev_priv->drm);
> > +	if (WARN_ON(!state))
> > +		return;
> > +
> > +	state->acquire_ctx = &ctx;
> > +
> > +retry:
> > +	to_intel_atomic_state(state)->modeset = true;
> > +	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
> > +		enable ? 2 * 96000 : 0;
> > +
> > +	/*
> > +	 * Protects dev_priv->cdclk.force_min_cdclk
> > +	 * Need to lock this here in case we have no active pipes
> > +	 * and thus wouldn't lock it during the commit otherwise.
> > +	 */
> > +	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, &ctx);
> > +	if (!ret)
> > +		ret = drm_atomic_commit(state);
> 
> Ville mentioned a potential dead-lock problem here: a normal modeset
> enabling an HDMI/DP output with an audio sink will call the
> drm_audio_component_audio_ops::pin_eld_notify() callback with the
> connection_mutex already held. This callback in turn could call
> drm_audio_component_ops::get_power()/put_power() callbacks and
> dead-lock at the above drm_modeset_lock() call.
> 
> Looks like that
> 
>    sound/pci/hda/patch_hdmi.c / intel_pin_eld_notify()->
>    check_presence_and_report()->
>    hdmi_present_sense()
> 
> would prevent this, but for instance
> 
>    sound/soc/codecs/hdac_hdmi.c / hdac_hdmi_eld_notify_cb()->
>    hdac_hdmi_present_sense()->
>    hdac_hdmi_jack_report()->
>    snd_soc_jack_report()->
>    snd_soc_dapm_sync()->
>    snd_soc_dapm_sync_unlocked()->
>    dapm_power_widgets()->
>    dapm_pre_sequence_async()
> 
> could call pm_runtime_get_sync() and so I guess also the aops
> get_power() hook.
> 
> Could someone in your team check if the above can indeed happen?
> 
> > +
> > +	if (ret == -EDEADLK) {
> > +		drm_atomic_state_clear(state);
> > +		drm_modeset_backoff(&ctx);
> > +		goto retry;
> > +	}
> > +
> > +	WARN_ON(ret);
> > +
> > +	drm_atomic_state_put(state);
> > +
> > +	drm_modeset_drop_locks(&ctx);
> > +	drm_modeset_acquire_fini(&ctx);
> > +}
> > +
> >  static void i915_audio_component_get_power(struct device *kdev)
> >  {
> > -	intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> > +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> > +
> > +	dev_priv->get_put_refcount++;
> > +
> > +	/* Force cdclk to 2*BCLK during first time get power call */
> > +	if (dev_priv->get_put_refcount == 1)
> > +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > +			glk_force_audio_cdclk(dev_priv, true);
> > +
> > +	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> 
> We can't guarantee that the power wells belonging to the audio power
> domain are off at this point (since an external output can keep them on).
> But according to the tests you did Kumar, this doesn't even seem to be
> required, the only requirement is that CDCLK is raised.
> 
> So to make the sequence behave the same all the time (and not hide some
> problem happening occasionally) and to have it symmetric with the one
> in i915_audio_component_put_power() we should get the audio power domain
> before bumping CDCLK.
> 
> >  }
> >  
> >  static void i915_audio_component_put_power(struct device *kdev)
> >  {
> > -	intel_display_power_put(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> > +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> > +
> > +	dev_priv->get_put_refcount--;
> > +
> > +	/* Force required cdclk during last time put power call */
> > +	if (dev_priv->get_put_refcount == 0)
> > +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > +			glk_force_audio_cdclk(dev_priv, false);
> > +
> > +	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> >  }
> >  
> >  static void i915_audio_component_codec_wake_override(struct device *kdev,
> > @@ -959,7 +1018,7 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv)
> >  		/* continue with reduced functionality */
> >  		return;
> >  	}
> > -
> > +	dev_priv->get_put_refcount = 0;
> >  	dev_priv->audio_component_registered = true;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 8ed7bd052e46..0f0aea900ceb 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -2153,19 +2153,8 @@ 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.
> > -	 *
> > -	 * FIXME: Check the actual, not default, BCLK being used.
> > -	 *
> > -	 * FIXME: This does not depend on ->has_audio because the higher CDCLK
> > -	 * is required for audio probe, also when there are no audio capable
> > -	 * displays connected at probe time. This leads to unnecessarily high
> > -	 * CDCLK when audio is not required.
> > -	 *
> > -	 * FIXME: This limit is only applied when there are displays connected
> > -	 * at probe time. If we probe without displays, we'll still end up using
> > -	 * the platform minimum CDCLK, failing audio probe.
> >  	 */
> > -	if (INTEL_GEN(dev_priv) >= 9)
> > +	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
> >  		min_cdclk = max(2 * 96000, min_cdclk);
> >  
> >  	/*
> > @@ -2205,7 +2194,7 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> >  		intel_state->min_cdclk[i] = min_cdclk;
> >  	}
> >  
> > -	min_cdclk = 0;
> > +	min_cdclk = intel_state->cdclk.force_min_cdclk;
> >  	for_each_pipe(dev_priv, pipe)
> >  		min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
> >  
> > @@ -2266,7 +2255,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  		vlv_calc_voltage_level(dev_priv, cdclk);
> >  
> >  	if (!intel_state->active_crtcs) {
> > -		cdclk = vlv_calc_cdclk(dev_priv, 0);
> > +		cdclk = vlv_calc_cdclk(dev_priv, intel_state->cdclk.force_min_cdclk);
> >  
> >  		intel_state->cdclk.actual.cdclk = cdclk;
> >  		intel_state->cdclk.actual.voltage_level =
> > @@ -2299,7 +2288,7 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  		bdw_calc_voltage_level(cdclk);
> >  
> >  	if (!intel_state->active_crtcs) {
> > -		cdclk = bdw_calc_cdclk(0);
> > +		cdclk = bdw_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> >  
> >  		intel_state->cdclk.actual.cdclk = cdclk;
> >  		intel_state->cdclk.actual.voltage_level =
> > @@ -2371,7 +2360,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  		skl_calc_voltage_level(cdclk);
> >  
> >  	if (!intel_state->active_crtcs) {
> > -		cdclk = skl_calc_cdclk(0, vco);
> > +		cdclk = skl_calc_cdclk(intel_state->cdclk.force_min_cdclk, vco);
> >  
> >  		intel_state->cdclk.actual.vco = vco;
> >  		intel_state->cdclk.actual.cdclk = cdclk;
> > @@ -2410,10 +2399,10 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  
> >  	if (!intel_state->active_crtcs) {
> >  		if (IS_GEMINILAKE(dev_priv)) {
> > -			cdclk = glk_calc_cdclk(0);
> > +			 cdclk = glk_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> >  			vco = glk_de_pll_vco(dev_priv, cdclk);
> >  		} else {
> > -			cdclk = bxt_calc_cdclk(0);
> > +			cdclk = bxt_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> >  			vco = bxt_de_pll_vco(dev_priv, cdclk);
> >  		}
> >  
> > @@ -2449,7 +2438,7 @@ static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  		    cnl_compute_min_voltage_level(intel_state));
> >  
> >  	if (!intel_state->active_crtcs) {
> > -		cdclk = cnl_calc_cdclk(0);
> > +		cdclk = cnl_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> >  		vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
> >  
> >  		intel_state->cdclk.actual.vco = vco;
> > @@ -2482,7 +2471,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	intel_state->cdclk.logical.cdclk = cdclk;
> >  
> >  	if (!intel_state->active_crtcs) {
> > -		cdclk = icl_calc_cdclk(0, ref);
> > +		cdclk = icl_calc_cdclk(intel_state->cdclk.force_min_cdclk, ref);
> >  		vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
> >  
> >  		intel_state->cdclk.actual.vco = vco;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 17c590b42fd7..3ee1c1f5419d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12162,6 +12162,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> >  		return -EINVAL;
> >  	}
> >  
> > +	/* keep the current setting */
> > +	if (!intel_state->modeset)
> 
> Ville, I'd use here a dedicated flag instead.
> 
> > +		intel_state->cdclk.force_min_cdclk = dev_priv->cdclk.force_min_cdclk;
> > +
> >  	intel_state->modeset = true;
> >  	intel_state->active_crtcs = dev_priv->active_crtcs;
> >  	intel_state->cdclk.logical = dev_priv->cdclk.logical;
> > @@ -12257,7 +12261,7 @@ static int intel_atomic_check(struct drm_device *dev,
> >  	struct drm_crtc *crtc;
> >  	struct drm_crtc_state *old_crtc_state, *crtc_state;
> >  	int ret, i;
> > -	bool any_ms = false;
> > +	bool any_ms = intel_state->modeset;
> >  
> >  	/* Catch I915_MODE_FLAG_INHERITED */
> >  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> > @@ -12805,6 +12809,7 @@ static int intel_atomic_commit(struct drm_device *dev,
> >  		dev_priv->active_crtcs = intel_state->active_crtcs;
> >  		dev_priv->cdclk.logical = intel_state->cdclk.logical;
> >  		dev_priv->cdclk.actual = intel_state->cdclk.actual;
> > +		dev_priv->cdclk.force_min_cdclk = intel_state->cdclk.force_min_cdclk;
> >  	}
> >  
> >  	drm_atomic_state_get(state);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 8641583842be..0da17ad056ec 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -459,6 +459,8 @@ struct intel_atomic_state {
> >  		 * state only when all crtc's are DPMS off.
> >  		 */
> >  		struct intel_cdclk_state actual;
> > +
> > +		int force_min_cdclk;
> >  	} cdclk;
> >  
> >  	bool dpll_set, modeset;
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
  2018-09-12 13:18 ` Imre Deak
  2018-09-12 13:18   ` Imre Deak
@ 2018-09-12 13:49   ` Ville Syrjälä
  1 sibling, 0 replies; 17+ messages in thread
From: Ville Syrjälä @ 2018-09-12 13:49 UTC (permalink / raw)
  To: Imre Deak; +Cc: jani.nikula, intel-gfx, ville.syrjala

On Wed, Sep 12, 2018 at 04:18:12PM +0300, Imre Deak wrote:
> Hi Kumar, Takashi,
> 
> On Tue, Jun 19, 2018 at 03:01:11PM -0700, Abhay Kumar wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > CDCLK has to be at least twice the BLCK regardless of audio. Audio
> > driver has to probe using this hook and increase the clock even in
> > absence of any display.
> > 
> > v2: Use atomic refcount for get_power, put_power so that we can
> >     call each once(Abhay).
> > v3: Reset power well 2 to avoid any transaction on iDisp link
> >     during cdclk change(Abhay).
> > v4: Remove Power well 2 reset workaround(Ville).
> > v5: Remove unwanted Power well 2 register defined in v4(Abhay).
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  3 ++
> >  drivers/gpu/drm/i915/intel_audio.c   | 67 +++++++++++++++++++++++++++++++++---
> >  drivers/gpu/drm/i915/intel_cdclk.c   | 29 +++++-----------
> >  drivers/gpu/drm/i915/intel_display.c |  7 +++-
> >  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> >  5 files changed, 83 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 6104d7115054..a4a386a5db69 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1702,6 +1702,7 @@ struct drm_i915_private {
> >  	unsigned int hpll_freq;
> >  	unsigned int fdi_pll_freq;
> >  	unsigned int czclk_freq;
> > +	u32 get_put_refcount;
> >  
> >  	struct {
> >  		/*
> > @@ -1719,6 +1720,8 @@ struct drm_i915_private {
> >  		struct intel_cdclk_state actual;
> >  		/* The current hardware cdclk state */
> >  		struct intel_cdclk_state hw;
> > +
> > +		int force_min_cdclk;
> >  	} cdclk;
> >  
> >  	/**
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index 3ea566f99450..ca8f04c7cbb3 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -618,7 +618,6 @@ void intel_audio_codec_enable(struct intel_encoder *encoder,
> >  
> >  	if (!connector->eld[0])
> >  		return;
> > -
> >  	DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
> >  			 connector->base.id,
> >  			 connector->name,
> > @@ -713,14 +712,74 @@ void intel_init_audio_hooks(struct drm_i915_private *dev_priv)
> >  	}
> >  }
> >  
> > +static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
> > +				bool enable)
> > +{
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	struct drm_atomic_state *state;
> > +	int ret;
> > +
> > +	drm_modeset_acquire_init(&ctx, 0);
> > +	state = drm_atomic_state_alloc(&dev_priv->drm);
> > +	if (WARN_ON(!state))
> > +		return;
> > +
> > +	state->acquire_ctx = &ctx;
> > +
> > +retry:
> > +	to_intel_atomic_state(state)->modeset = true;
> > +	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
> > +		enable ? 2 * 96000 : 0;
> > +
> > +	/*
> > +	 * Protects dev_priv->cdclk.force_min_cdclk
> > +	 * Need to lock this here in case we have no active pipes
> > +	 * and thus wouldn't lock it during the commit otherwise.
> > +	 */
> > +	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, &ctx);
> > +	if (!ret)
> > +		ret = drm_atomic_commit(state);
> 
> Ville mentioned a potential dead-lock problem here: a normal modeset
> enabling an HDMI/DP output with an audio sink will call the
> drm_audio_component_audio_ops::pin_eld_notify() callback with the
> connection_mutex already held. This callback in turn could call
> drm_audio_component_ops::get_power()/put_power() callbacks and
> dead-lock at the above drm_modeset_lock() call.
> 
> Looks like that
> 
>    sound/pci/hda/patch_hdmi.c / intel_pin_eld_notify()->
>    check_presence_and_report()->
>    hdmi_present_sense()
> 
> would prevent this, but for instance
> 
>    sound/soc/codecs/hdac_hdmi.c / hdac_hdmi_eld_notify_cb()->
>    hdac_hdmi_present_sense()->
>    hdac_hdmi_jack_report()->
>    snd_soc_jack_report()->
>    snd_soc_dapm_sync()->
>    snd_soc_dapm_sync_unlocked()->
>    dapm_power_widgets()->
>    dapm_pre_sequence_async()
> 
> could call pm_runtime_get_sync() and so I guess also the aops
> get_power() hook.
> 
> Could someone in your team check if the above can indeed happen?
> 
> > +
> > +	if (ret == -EDEADLK) {
> > +		drm_atomic_state_clear(state);
> > +		drm_modeset_backoff(&ctx);
> > +		goto retry;
> > +	}
> > +
> > +	WARN_ON(ret);
> > +
> > +	drm_atomic_state_put(state);
> > +
> > +	drm_modeset_drop_locks(&ctx);
> > +	drm_modeset_acquire_fini(&ctx);
> > +}
> > +
> >  static void i915_audio_component_get_power(struct device *kdev)
> >  {
> > -	intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> > +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> > +
> > +	dev_priv->get_put_refcount++;
> > +
> > +	/* Force cdclk to 2*BCLK during first time get power call */
> > +	if (dev_priv->get_put_refcount == 1)
> > +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > +			glk_force_audio_cdclk(dev_priv, true);
> > +
> > +	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> 
> We can't guarantee that the power wells belonging to the audio power
> domain are off at this point (since an external output can keep them on).
> But according to the tests you did Kumar, this doesn't even seem to be
> required, the only requirement is that CDCLK is raised.
> 
> So to make the sequence behave the same all the time (and not hide some
> problem happening occasionally) and to have it symmetric with the one
> in i915_audio_component_put_power() we should get the audio power domain
> before bumping CDCLK.
> 
> >  }
> >  
> >  static void i915_audio_component_put_power(struct device *kdev)
> >  {
> > -	intel_display_power_put(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> > +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> > +
> > +	dev_priv->get_put_refcount--;
> > +
> > +	/* Force required cdclk during last time put power call */
> > +	if (dev_priv->get_put_refcount == 0)
> > +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > +			glk_force_audio_cdclk(dev_priv, false);
> > +
> > +	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> >  }
> >  
> >  static void i915_audio_component_codec_wake_override(struct device *kdev,
> > @@ -959,7 +1018,7 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv)
> >  		/* continue with reduced functionality */
> >  		return;
> >  	}
> > -
> > +	dev_priv->get_put_refcount = 0;
> >  	dev_priv->audio_component_registered = true;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 8ed7bd052e46..0f0aea900ceb 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -2153,19 +2153,8 @@ 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.
> > -	 *
> > -	 * FIXME: Check the actual, not default, BCLK being used.
> > -	 *
> > -	 * FIXME: This does not depend on ->has_audio because the higher CDCLK
> > -	 * is required for audio probe, also when there are no audio capable
> > -	 * displays connected at probe time. This leads to unnecessarily high
> > -	 * CDCLK when audio is not required.
> > -	 *
> > -	 * FIXME: This limit is only applied when there are displays connected
> > -	 * at probe time. If we probe without displays, we'll still end up using
> > -	 * the platform minimum CDCLK, failing audio probe.
> >  	 */
> > -	if (INTEL_GEN(dev_priv) >= 9)
> > +	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
> >  		min_cdclk = max(2 * 96000, min_cdclk);
> >  
> >  	/*
> > @@ -2205,7 +2194,7 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> >  		intel_state->min_cdclk[i] = min_cdclk;
> >  	}
> >  
> > -	min_cdclk = 0;
> > +	min_cdclk = intel_state->cdclk.force_min_cdclk;
> >  	for_each_pipe(dev_priv, pipe)
> >  		min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
> >  
> > @@ -2266,7 +2255,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  		vlv_calc_voltage_level(dev_priv, cdclk);
> >  
> >  	if (!intel_state->active_crtcs) {
> > -		cdclk = vlv_calc_cdclk(dev_priv, 0);
> > +		cdclk = vlv_calc_cdclk(dev_priv, intel_state->cdclk.force_min_cdclk);
> >  
> >  		intel_state->cdclk.actual.cdclk = cdclk;
> >  		intel_state->cdclk.actual.voltage_level =
> > @@ -2299,7 +2288,7 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  		bdw_calc_voltage_level(cdclk);
> >  
> >  	if (!intel_state->active_crtcs) {
> > -		cdclk = bdw_calc_cdclk(0);
> > +		cdclk = bdw_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> >  
> >  		intel_state->cdclk.actual.cdclk = cdclk;
> >  		intel_state->cdclk.actual.voltage_level =
> > @@ -2371,7 +2360,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  		skl_calc_voltage_level(cdclk);
> >  
> >  	if (!intel_state->active_crtcs) {
> > -		cdclk = skl_calc_cdclk(0, vco);
> > +		cdclk = skl_calc_cdclk(intel_state->cdclk.force_min_cdclk, vco);
> >  
> >  		intel_state->cdclk.actual.vco = vco;
> >  		intel_state->cdclk.actual.cdclk = cdclk;
> > @@ -2410,10 +2399,10 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  
> >  	if (!intel_state->active_crtcs) {
> >  		if (IS_GEMINILAKE(dev_priv)) {
> > -			cdclk = glk_calc_cdclk(0);
> > +			 cdclk = glk_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> >  			vco = glk_de_pll_vco(dev_priv, cdclk);
> >  		} else {
> > -			cdclk = bxt_calc_cdclk(0);
> > +			cdclk = bxt_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> >  			vco = bxt_de_pll_vco(dev_priv, cdclk);
> >  		}
> >  
> > @@ -2449,7 +2438,7 @@ static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  		    cnl_compute_min_voltage_level(intel_state));
> >  
> >  	if (!intel_state->active_crtcs) {
> > -		cdclk = cnl_calc_cdclk(0);
> > +		cdclk = cnl_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> >  		vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
> >  
> >  		intel_state->cdclk.actual.vco = vco;
> > @@ -2482,7 +2471,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	intel_state->cdclk.logical.cdclk = cdclk;
> >  
> >  	if (!intel_state->active_crtcs) {
> > -		cdclk = icl_calc_cdclk(0, ref);
> > +		cdclk = icl_calc_cdclk(intel_state->cdclk.force_min_cdclk, ref);
> >  		vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
> >  
> >  		intel_state->cdclk.actual.vco = vco;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 17c590b42fd7..3ee1c1f5419d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12162,6 +12162,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> >  		return -EINVAL;
> >  	}
> >  
> > +	/* keep the current setting */
> > +	if (!intel_state->modeset)
> 
> Ville, I'd use here a dedicated flag instead.

Sure.

> 
> > +		intel_state->cdclk.force_min_cdclk = dev_priv->cdclk.force_min_cdclk;
> > +
> >  	intel_state->modeset = true;
> >  	intel_state->active_crtcs = dev_priv->active_crtcs;
> >  	intel_state->cdclk.logical = dev_priv->cdclk.logical;
> > @@ -12257,7 +12261,7 @@ static int intel_atomic_check(struct drm_device *dev,
> >  	struct drm_crtc *crtc;
> >  	struct drm_crtc_state *old_crtc_state, *crtc_state;
> >  	int ret, i;
> > -	bool any_ms = false;
> > +	bool any_ms = intel_state->modeset;
> >  
> >  	/* Catch I915_MODE_FLAG_INHERITED */
> >  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> > @@ -12805,6 +12809,7 @@ static int intel_atomic_commit(struct drm_device *dev,
> >  		dev_priv->active_crtcs = intel_state->active_crtcs;
> >  		dev_priv->cdclk.logical = intel_state->cdclk.logical;
> >  		dev_priv->cdclk.actual = intel_state->cdclk.actual;
> > +		dev_priv->cdclk.force_min_cdclk = intel_state->cdclk.force_min_cdclk;
> >  	}
> >  
> >  	drm_atomic_state_get(state);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 8641583842be..0da17ad056ec 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -459,6 +459,8 @@ struct intel_atomic_state {
> >  		 * state only when all crtc's are DPMS off.
> >  		 */
> >  		struct intel_cdclk_state actual;
> > +
> > +		int force_min_cdclk;
> >  	} cdclk;
> >  
> >  	bool dpll_set, modeset;
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
  2018-09-12 13:18   ` Imre Deak
@ 2018-09-12 18:18     ` Takashi Iwai
  2018-09-13 13:54       ` Imre Deak
  0 siblings, 1 reply; 17+ messages in thread
From: Takashi Iwai @ 2018-09-12 18:18 UTC (permalink / raw)
  To: imre.deak; +Cc: jani.nikula, intel-gfx, ville.syrjala

On Wed, 12 Sep 2018 15:18:47 +0200,
Imre Deak wrote:
> 
> +Takashi
> 
> On Wed, Sep 12, 2018 at 04:18:12PM +0300, Imre Deak wrote:
> > Hi Kumar, Takashi,
> > 
> > On Tue, Jun 19, 2018 at 03:01:11PM -0700, Abhay Kumar wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > CDCLK has to be at least twice the BLCK regardless of audio. Audio
> > > driver has to probe using this hook and increase the clock even in
> > > absence of any display.
> > > 
> > > v2: Use atomic refcount for get_power, put_power so that we can
> > >     call each once(Abhay).
> > > v3: Reset power well 2 to avoid any transaction on iDisp link
> > >     during cdclk change(Abhay).
> > > v4: Remove Power well 2 reset workaround(Ville).
> > > v5: Remove unwanted Power well 2 register defined in v4(Abhay).
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h      |  3 ++
> > >  drivers/gpu/drm/i915/intel_audio.c   | 67 +++++++++++++++++++++++++++++++++---
> > >  drivers/gpu/drm/i915/intel_cdclk.c   | 29 +++++-----------
> > >  drivers/gpu/drm/i915/intel_display.c |  7 +++-
> > >  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> > >  5 files changed, 83 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 6104d7115054..a4a386a5db69 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1702,6 +1702,7 @@ struct drm_i915_private {
> > >  	unsigned int hpll_freq;
> > >  	unsigned int fdi_pll_freq;
> > >  	unsigned int czclk_freq;
> > > +	u32 get_put_refcount;
> > >  
> > >  	struct {
> > >  		/*
> > > @@ -1719,6 +1720,8 @@ struct drm_i915_private {
> > >  		struct intel_cdclk_state actual;
> > >  		/* The current hardware cdclk state */
> > >  		struct intel_cdclk_state hw;
> > > +
> > > +		int force_min_cdclk;
> > >  	} cdclk;
> > >  
> > >  	/**
> > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > index 3ea566f99450..ca8f04c7cbb3 100644
> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > @@ -618,7 +618,6 @@ void intel_audio_codec_enable(struct intel_encoder *encoder,
> > >  
> > >  	if (!connector->eld[0])
> > >  		return;
> > > -
> > >  	DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
> > >  			 connector->base.id,
> > >  			 connector->name,
> > > @@ -713,14 +712,74 @@ void intel_init_audio_hooks(struct drm_i915_private *dev_priv)
> > >  	}
> > >  }
> > >  
> > > +static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
> > > +				bool enable)
> > > +{
> > > +	struct drm_modeset_acquire_ctx ctx;
> > > +	struct drm_atomic_state *state;
> > > +	int ret;
> > > +
> > > +	drm_modeset_acquire_init(&ctx, 0);
> > > +	state = drm_atomic_state_alloc(&dev_priv->drm);
> > > +	if (WARN_ON(!state))
> > > +		return;
> > > +
> > > +	state->acquire_ctx = &ctx;
> > > +
> > > +retry:
> > > +	to_intel_atomic_state(state)->modeset = true;
> > > +	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
> > > +		enable ? 2 * 96000 : 0;
> > > +
> > > +	/*
> > > +	 * Protects dev_priv->cdclk.force_min_cdclk
> > > +	 * Need to lock this here in case we have no active pipes
> > > +	 * and thus wouldn't lock it during the commit otherwise.
> > > +	 */
> > > +	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, &ctx);
> > > +	if (!ret)
> > > +		ret = drm_atomic_commit(state);
> > 
> > Ville mentioned a potential dead-lock problem here: a normal modeset
> > enabling an HDMI/DP output with an audio sink will call the
> > drm_audio_component_audio_ops::pin_eld_notify() callback with the
> > connection_mutex already held. This callback in turn could call
> > drm_audio_component_ops::get_power()/put_power() callbacks and
> > dead-lock at the above drm_modeset_lock() call.
> > 
> > Looks like that
> > 
> >    sound/pci/hda/patch_hdmi.c / intel_pin_eld_notify()->
> >    check_presence_and_report()->
> >    hdmi_present_sense()
> > 
> > would prevent this, but for instance
> > 
> >    sound/soc/codecs/hdac_hdmi.c / hdac_hdmi_eld_notify_cb()->
> >    hdac_hdmi_present_sense()->
> >    hdac_hdmi_jack_report()->
> >    snd_soc_jack_report()->
> >    snd_soc_dapm_sync()->
> >    snd_soc_dapm_sync_unlocked()->
> >    dapm_power_widgets()->
> >    dapm_pre_sequence_async()
> > 
> > could call pm_runtime_get_sync() and so I guess also the aops
> > get_power() hook.
> > 
> > Could someone in your team check if the above can indeed happen?

Through a quick glance, I'm afraid that it's indeed possible.  We need
to off-load either the hdac_hdmi jack handling or this new
force_audio_cdclk stuff, I suppose.


thanks,

Takashi

> > > +
> > > +	if (ret == -EDEADLK) {
> > > +		drm_atomic_state_clear(state);
> > > +		drm_modeset_backoff(&ctx);
> > > +		goto retry;
> > > +	}
> > > +
> > > +	WARN_ON(ret);
> > > +
> > > +	drm_atomic_state_put(state);
> > > +
> > > +	drm_modeset_drop_locks(&ctx);
> > > +	drm_modeset_acquire_fini(&ctx);
> > > +}
> > > +
> > >  static void i915_audio_component_get_power(struct device *kdev)
> > >  {
> > > -	intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> > > +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> > > +
> > > +	dev_priv->get_put_refcount++;
> > > +
> > > +	/* Force cdclk to 2*BCLK during first time get power call */
> > > +	if (dev_priv->get_put_refcount == 1)
> > > +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > > +			glk_force_audio_cdclk(dev_priv, true);
> > > +
> > > +	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> > 
> > We can't guarantee that the power wells belonging to the audio power
> > domain are off at this point (since an external output can keep them on).
> > But according to the tests you did Kumar, this doesn't even seem to be
> > required, the only requirement is that CDCLK is raised.
> > 
> > So to make the sequence behave the same all the time (and not hide some
> > problem happening occasionally) and to have it symmetric with the one
> > in i915_audio_component_put_power() we should get the audio power domain
> > before bumping CDCLK.
> > 
> > >  }
> > >  
> > >  static void i915_audio_component_put_power(struct device *kdev)
> > >  {
> > > -	intel_display_power_put(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> > > +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> > > +
> > > +	dev_priv->get_put_refcount--;
> > > +
> > > +	/* Force required cdclk during last time put power call */
> > > +	if (dev_priv->get_put_refcount == 0)
> > > +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > > +			glk_force_audio_cdclk(dev_priv, false);
> > > +
> > > +	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> > >  }
> > >  
> > >  static void i915_audio_component_codec_wake_override(struct device *kdev,
> > > @@ -959,7 +1018,7 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv)
> > >  		/* continue with reduced functionality */
> > >  		return;
> > >  	}
> > > -
> > > +	dev_priv->get_put_refcount = 0;
> > >  	dev_priv->audio_component_registered = true;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > > index 8ed7bd052e46..0f0aea900ceb 100644
> > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > @@ -2153,19 +2153,8 @@ 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.
> > > -	 *
> > > -	 * FIXME: Check the actual, not default, BCLK being used.
> > > -	 *
> > > -	 * FIXME: This does not depend on ->has_audio because the higher CDCLK
> > > -	 * is required for audio probe, also when there are no audio capable
> > > -	 * displays connected at probe time. This leads to unnecessarily high
> > > -	 * CDCLK when audio is not required.
> > > -	 *
> > > -	 * FIXME: This limit is only applied when there are displays connected
> > > -	 * at probe time. If we probe without displays, we'll still end up using
> > > -	 * the platform minimum CDCLK, failing audio probe.
> > >  	 */
> > > -	if (INTEL_GEN(dev_priv) >= 9)
> > > +	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
> > >  		min_cdclk = max(2 * 96000, min_cdclk);
> > >  
> > >  	/*
> > > @@ -2205,7 +2194,7 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> > >  		intel_state->min_cdclk[i] = min_cdclk;
> > >  	}
> > >  
> > > -	min_cdclk = 0;
> > > +	min_cdclk = intel_state->cdclk.force_min_cdclk;
> > >  	for_each_pipe(dev_priv, pipe)
> > >  		min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
> > >  
> > > @@ -2266,7 +2255,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  		vlv_calc_voltage_level(dev_priv, cdclk);
> > >  
> > >  	if (!intel_state->active_crtcs) {
> > > -		cdclk = vlv_calc_cdclk(dev_priv, 0);
> > > +		cdclk = vlv_calc_cdclk(dev_priv, intel_state->cdclk.force_min_cdclk);
> > >  
> > >  		intel_state->cdclk.actual.cdclk = cdclk;
> > >  		intel_state->cdclk.actual.voltage_level =
> > > @@ -2299,7 +2288,7 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  		bdw_calc_voltage_level(cdclk);
> > >  
> > >  	if (!intel_state->active_crtcs) {
> > > -		cdclk = bdw_calc_cdclk(0);
> > > +		cdclk = bdw_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> > >  
> > >  		intel_state->cdclk.actual.cdclk = cdclk;
> > >  		intel_state->cdclk.actual.voltage_level =
> > > @@ -2371,7 +2360,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  		skl_calc_voltage_level(cdclk);
> > >  
> > >  	if (!intel_state->active_crtcs) {
> > > -		cdclk = skl_calc_cdclk(0, vco);
> > > +		cdclk = skl_calc_cdclk(intel_state->cdclk.force_min_cdclk, vco);
> > >  
> > >  		intel_state->cdclk.actual.vco = vco;
> > >  		intel_state->cdclk.actual.cdclk = cdclk;
> > > @@ -2410,10 +2399,10 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  
> > >  	if (!intel_state->active_crtcs) {
> > >  		if (IS_GEMINILAKE(dev_priv)) {
> > > -			cdclk = glk_calc_cdclk(0);
> > > +			 cdclk = glk_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> > >  			vco = glk_de_pll_vco(dev_priv, cdclk);
> > >  		} else {
> > > -			cdclk = bxt_calc_cdclk(0);
> > > +			cdclk = bxt_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> > >  			vco = bxt_de_pll_vco(dev_priv, cdclk);
> > >  		}
> > >  
> > > @@ -2449,7 +2438,7 @@ static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  		    cnl_compute_min_voltage_level(intel_state));
> > >  
> > >  	if (!intel_state->active_crtcs) {
> > > -		cdclk = cnl_calc_cdclk(0);
> > > +		cdclk = cnl_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> > >  		vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
> > >  
> > >  		intel_state->cdclk.actual.vco = vco;
> > > @@ -2482,7 +2471,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > >  	intel_state->cdclk.logical.cdclk = cdclk;
> > >  
> > >  	if (!intel_state->active_crtcs) {
> > > -		cdclk = icl_calc_cdclk(0, ref);
> > > +		cdclk = icl_calc_cdclk(intel_state->cdclk.force_min_cdclk, ref);
> > >  		vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
> > >  
> > >  		intel_state->cdclk.actual.vco = vco;
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 17c590b42fd7..3ee1c1f5419d 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -12162,6 +12162,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > +	/* keep the current setting */
> > > +	if (!intel_state->modeset)
> > 
> > Ville, I'd use here a dedicated flag instead.
> > 
> > > +		intel_state->cdclk.force_min_cdclk = dev_priv->cdclk.force_min_cdclk;
> > > +
> > >  	intel_state->modeset = true;
> > >  	intel_state->active_crtcs = dev_priv->active_crtcs;
> > >  	intel_state->cdclk.logical = dev_priv->cdclk.logical;
> > > @@ -12257,7 +12261,7 @@ static int intel_atomic_check(struct drm_device *dev,
> > >  	struct drm_crtc *crtc;
> > >  	struct drm_crtc_state *old_crtc_state, *crtc_state;
> > >  	int ret, i;
> > > -	bool any_ms = false;
> > > +	bool any_ms = intel_state->modeset;
> > >  
> > >  	/* Catch I915_MODE_FLAG_INHERITED */
> > >  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> > > @@ -12805,6 +12809,7 @@ static int intel_atomic_commit(struct drm_device *dev,
> > >  		dev_priv->active_crtcs = intel_state->active_crtcs;
> > >  		dev_priv->cdclk.logical = intel_state->cdclk.logical;
> > >  		dev_priv->cdclk.actual = intel_state->cdclk.actual;
> > > +		dev_priv->cdclk.force_min_cdclk = intel_state->cdclk.force_min_cdclk;
> > >  	}
> > >  
> > >  	drm_atomic_state_get(state);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 8641583842be..0da17ad056ec 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -459,6 +459,8 @@ struct intel_atomic_state {
> > >  		 * state only when all crtc's are DPMS off.
> > >  		 */
> > >  		struct intel_cdclk_state actual;
> > > +
> > > +		int force_min_cdclk;
> > >  	} cdclk;
> > >  
> > >  	bool dpll_set, modeset;
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
  2018-09-12 18:18     ` Takashi Iwai
@ 2018-09-13 13:54       ` Imre Deak
  2018-09-13 14:49         ` Takashi Iwai
  0 siblings, 1 reply; 17+ messages in thread
From: Imre Deak @ 2018-09-13 13:54 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: jani.nikula, intel-gfx, ville.syrjala

On Wed, Sep 12, 2018 at 08:18:37PM +0200, Takashi Iwai wrote:
> On Wed, 12 Sep 2018 15:18:47 +0200,
> Imre Deak wrote:
> > 
> > +Takashi
> > 
> > On Wed, Sep 12, 2018 at 04:18:12PM +0300, Imre Deak wrote:
> > > Hi Kumar, Takashi,
> > > 
> > > On Tue, Jun 19, 2018 at 03:01:11PM -0700, Abhay Kumar wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > CDCLK has to be at least twice the BLCK regardless of audio. Audio
> > > > driver has to probe using this hook and increase the clock even in
> > > > absence of any display.
> > > > 
> > > > v2: Use atomic refcount for get_power, put_power so that we can
> > > >     call each once(Abhay).
> > > > v3: Reset power well 2 to avoid any transaction on iDisp link
> > > >     during cdclk change(Abhay).
> > > > v4: Remove Power well 2 reset workaround(Ville).
> > > > v5: Remove unwanted Power well 2 register defined in v4(Abhay).
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h      |  3 ++
> > > >  drivers/gpu/drm/i915/intel_audio.c   | 67 +++++++++++++++++++++++++++++++++---
> > > >  drivers/gpu/drm/i915/intel_cdclk.c   | 29 +++++-----------
> > > >  drivers/gpu/drm/i915/intel_display.c |  7 +++-
> > > >  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> > > >  5 files changed, 83 insertions(+), 25 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 6104d7115054..a4a386a5db69 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -1702,6 +1702,7 @@ struct drm_i915_private {
> > > >  	unsigned int hpll_freq;
> > > >  	unsigned int fdi_pll_freq;
> > > >  	unsigned int czclk_freq;
> > > > +	u32 get_put_refcount;
> > > >  
> > > >  	struct {
> > > >  		/*
> > > > @@ -1719,6 +1720,8 @@ struct drm_i915_private {
> > > >  		struct intel_cdclk_state actual;
> > > >  		/* The current hardware cdclk state */
> > > >  		struct intel_cdclk_state hw;
> > > > +
> > > > +		int force_min_cdclk;
> > > >  	} cdclk;
> > > >  
> > > >  	/**
> > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > > index 3ea566f99450..ca8f04c7cbb3 100644
> > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > @@ -618,7 +618,6 @@ void intel_audio_codec_enable(struct intel_encoder *encoder,
> > > >  
> > > >  	if (!connector->eld[0])
> > > >  		return;
> > > > -
> > > >  	DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
> > > >  			 connector->base.id,
> > > >  			 connector->name,
> > > > @@ -713,14 +712,74 @@ void intel_init_audio_hooks(struct drm_i915_private *dev_priv)
> > > >  	}
> > > >  }
> > > >  
> > > > +static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
> > > > +				bool enable)
> > > > +{
> > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > +	struct drm_atomic_state *state;
> > > > +	int ret;
> > > > +
> > > > +	drm_modeset_acquire_init(&ctx, 0);
> > > > +	state = drm_atomic_state_alloc(&dev_priv->drm);
> > > > +	if (WARN_ON(!state))
> > > > +		return;
> > > > +
> > > > +	state->acquire_ctx = &ctx;
> > > > +
> > > > +retry:
> > > > +	to_intel_atomic_state(state)->modeset = true;
> > > > +	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
> > > > +		enable ? 2 * 96000 : 0;
> > > > +
> > > > +	/*
> > > > +	 * Protects dev_priv->cdclk.force_min_cdclk
> > > > +	 * Need to lock this here in case we have no active pipes
> > > > +	 * and thus wouldn't lock it during the commit otherwise.
> > > > +	 */
> > > > +	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, &ctx);
> > > > +	if (!ret)
> > > > +		ret = drm_atomic_commit(state);
> > > 
> > > Ville mentioned a potential dead-lock problem here: a normal modeset
> > > enabling an HDMI/DP output with an audio sink will call the
> > > drm_audio_component_audio_ops::pin_eld_notify() callback with the
> > > connection_mutex already held. This callback in turn could call
> > > drm_audio_component_ops::get_power()/put_power() callbacks and
> > > dead-lock at the above drm_modeset_lock() call.
> > > 
> > > Looks like that
> > > 
> > >    sound/pci/hda/patch_hdmi.c / intel_pin_eld_notify()->
> > >    check_presence_and_report()->
> > >    hdmi_present_sense()
> > > 
> > > would prevent this, but for instance
> > > 
> > >    sound/soc/codecs/hdac_hdmi.c / hdac_hdmi_eld_notify_cb()->
> > >    hdac_hdmi_present_sense()->
> > >    hdac_hdmi_jack_report()->
> > >    snd_soc_jack_report()->
> > >    snd_soc_dapm_sync()->
> > >    snd_soc_dapm_sync_unlocked()->
> > >    dapm_power_widgets()->
> > >    dapm_pre_sequence_async()
> > > 
> > > could call pm_runtime_get_sync() and so I guess also the aops
> > > get_power() hook.
> > > 
> > > Could someone in your team check if the above can indeed happen?
> 
> Through a quick glance, I'm afraid that it's indeed possible.  We need
> to off-load either the hdac_hdmi jack handling or this new
> force_audio_cdclk stuff, I suppose.

Ok. We need the force_audio_cdclk thing whenever the audio driver needs
power and calls the aops::get_power() hook. IIUC this happens from various
places in the HDA driver and get_power() should enable power
synchronously, so not sure how we could off-load that..

How about the same for the jack handling, perhaps only in the case it's
done from hdac_hdmi_eld_notify_cb()?

Also could you explain why patch_hdmi.c can do without get_power()?
Could the same thing be done in hdac_hdmi.c?

Thanks,
Imre

> 
> 
> thanks,
> 
> Takashi
> 
> > > > +
> > > > +	if (ret == -EDEADLK) {
> > > > +		drm_atomic_state_clear(state);
> > > > +		drm_modeset_backoff(&ctx);
> > > > +		goto retry;
> > > > +	}
> > > > +
> > > > +	WARN_ON(ret);
> > > > +
> > > > +	drm_atomic_state_put(state);
> > > > +
> > > > +	drm_modeset_drop_locks(&ctx);
> > > > +	drm_modeset_acquire_fini(&ctx);
> > > > +}
> > > > +
> > > >  static void i915_audio_component_get_power(struct device *kdev)
> > > >  {
> > > > -	intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> > > > +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> > > > +
> > > > +	dev_priv->get_put_refcount++;
> > > > +
> > > > +	/* Force cdclk to 2*BCLK during first time get power call */
> > > > +	if (dev_priv->get_put_refcount == 1)
> > > > +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > > > +			glk_force_audio_cdclk(dev_priv, true);
> > > > +
> > > > +	intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> > > 
> > > We can't guarantee that the power wells belonging to the audio power
> > > domain are off at this point (since an external output can keep them on).
> > > But according to the tests you did Kumar, this doesn't even seem to be
> > > required, the only requirement is that CDCLK is raised.
> > > 
> > > So to make the sequence behave the same all the time (and not hide some
> > > problem happening occasionally) and to have it symmetric with the one
> > > in i915_audio_component_put_power() we should get the audio power domain
> > > before bumping CDCLK.
> > > 
> > > >  }
> > > >  
> > > >  static void i915_audio_component_put_power(struct device *kdev)
> > > >  {
> > > > -	intel_display_power_put(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> > > > +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> > > > +
> > > > +	dev_priv->get_put_refcount--;
> > > > +
> > > > +	/* Force required cdclk during last time put power call */
> > > > +	if (dev_priv->get_put_refcount == 0)
> > > > +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > > > +			glk_force_audio_cdclk(dev_priv, false);
> > > > +
> > > > +	intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> > > >  }
> > > >  
> > > >  static void i915_audio_component_codec_wake_override(struct device *kdev,
> > > > @@ -959,7 +1018,7 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv)
> > > >  		/* continue with reduced functionality */
> > > >  		return;
> > > >  	}
> > > > -
> > > > +	dev_priv->get_put_refcount = 0;
> > > >  	dev_priv->audio_component_registered = true;
> > > >  }
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > > > index 8ed7bd052e46..0f0aea900ceb 100644
> > > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > > @@ -2153,19 +2153,8 @@ 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.
> > > > -	 *
> > > > -	 * FIXME: Check the actual, not default, BCLK being used.
> > > > -	 *
> > > > -	 * FIXME: This does not depend on ->has_audio because the higher CDCLK
> > > > -	 * is required for audio probe, also when there are no audio capable
> > > > -	 * displays connected at probe time. This leads to unnecessarily high
> > > > -	 * CDCLK when audio is not required.
> > > > -	 *
> > > > -	 * FIXME: This limit is only applied when there are displays connected
> > > > -	 * at probe time. If we probe without displays, we'll still end up using
> > > > -	 * the platform minimum CDCLK, failing audio probe.
> > > >  	 */
> > > > -	if (INTEL_GEN(dev_priv) >= 9)
> > > > +	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
> > > >  		min_cdclk = max(2 * 96000, min_cdclk);
> > > >  
> > > >  	/*
> > > > @@ -2205,7 +2194,7 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> > > >  		intel_state->min_cdclk[i] = min_cdclk;
> > > >  	}
> > > >  
> > > > -	min_cdclk = 0;
> > > > +	min_cdclk = intel_state->cdclk.force_min_cdclk;
> > > >  	for_each_pipe(dev_priv, pipe)
> > > >  		min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
> > > >  
> > > > @@ -2266,7 +2255,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  		vlv_calc_voltage_level(dev_priv, cdclk);
> > > >  
> > > >  	if (!intel_state->active_crtcs) {
> > > > -		cdclk = vlv_calc_cdclk(dev_priv, 0);
> > > > +		cdclk = vlv_calc_cdclk(dev_priv, intel_state->cdclk.force_min_cdclk);
> > > >  
> > > >  		intel_state->cdclk.actual.cdclk = cdclk;
> > > >  		intel_state->cdclk.actual.voltage_level =
> > > > @@ -2299,7 +2288,7 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  		bdw_calc_voltage_level(cdclk);
> > > >  
> > > >  	if (!intel_state->active_crtcs) {
> > > > -		cdclk = bdw_calc_cdclk(0);
> > > > +		cdclk = bdw_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> > > >  
> > > >  		intel_state->cdclk.actual.cdclk = cdclk;
> > > >  		intel_state->cdclk.actual.voltage_level =
> > > > @@ -2371,7 +2360,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  		skl_calc_voltage_level(cdclk);
> > > >  
> > > >  	if (!intel_state->active_crtcs) {
> > > > -		cdclk = skl_calc_cdclk(0, vco);
> > > > +		cdclk = skl_calc_cdclk(intel_state->cdclk.force_min_cdclk, vco);
> > > >  
> > > >  		intel_state->cdclk.actual.vco = vco;
> > > >  		intel_state->cdclk.actual.cdclk = cdclk;
> > > > @@ -2410,10 +2399,10 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  
> > > >  	if (!intel_state->active_crtcs) {
> > > >  		if (IS_GEMINILAKE(dev_priv)) {
> > > > -			cdclk = glk_calc_cdclk(0);
> > > > +			 cdclk = glk_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> > > >  			vco = glk_de_pll_vco(dev_priv, cdclk);
> > > >  		} else {
> > > > -			cdclk = bxt_calc_cdclk(0);
> > > > +			cdclk = bxt_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> > > >  			vco = bxt_de_pll_vco(dev_priv, cdclk);
> > > >  		}
> > > >  
> > > > @@ -2449,7 +2438,7 @@ static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  		    cnl_compute_min_voltage_level(intel_state));
> > > >  
> > > >  	if (!intel_state->active_crtcs) {
> > > > -		cdclk = cnl_calc_cdclk(0);
> > > > +		cdclk = cnl_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> > > >  		vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
> > > >  
> > > >  		intel_state->cdclk.actual.vco = vco;
> > > > @@ -2482,7 +2471,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >  	intel_state->cdclk.logical.cdclk = cdclk;
> > > >  
> > > >  	if (!intel_state->active_crtcs) {
> > > > -		cdclk = icl_calc_cdclk(0, ref);
> > > > +		cdclk = icl_calc_cdclk(intel_state->cdclk.force_min_cdclk, ref);
> > > >  		vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
> > > >  
> > > >  		intel_state->cdclk.actual.vco = vco;
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 17c590b42fd7..3ee1c1f5419d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -12162,6 +12162,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> > > >  		return -EINVAL;
> > > >  	}
> > > >  
> > > > +	/* keep the current setting */
> > > > +	if (!intel_state->modeset)
> > > 
> > > Ville, I'd use here a dedicated flag instead.
> > > 
> > > > +		intel_state->cdclk.force_min_cdclk = dev_priv->cdclk.force_min_cdclk;
> > > > +
> > > >  	intel_state->modeset = true;
> > > >  	intel_state->active_crtcs = dev_priv->active_crtcs;
> > > >  	intel_state->cdclk.logical = dev_priv->cdclk.logical;
> > > > @@ -12257,7 +12261,7 @@ static int intel_atomic_check(struct drm_device *dev,
> > > >  	struct drm_crtc *crtc;
> > > >  	struct drm_crtc_state *old_crtc_state, *crtc_state;
> > > >  	int ret, i;
> > > > -	bool any_ms = false;
> > > > +	bool any_ms = intel_state->modeset;
> > > >  
> > > >  	/* Catch I915_MODE_FLAG_INHERITED */
> > > >  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> > > > @@ -12805,6 +12809,7 @@ static int intel_atomic_commit(struct drm_device *dev,
> > > >  		dev_priv->active_crtcs = intel_state->active_crtcs;
> > > >  		dev_priv->cdclk.logical = intel_state->cdclk.logical;
> > > >  		dev_priv->cdclk.actual = intel_state->cdclk.actual;
> > > > +		dev_priv->cdclk.force_min_cdclk = intel_state->cdclk.force_min_cdclk;
> > > >  	}
> > > >  
> > > >  	drm_atomic_state_get(state);
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 8641583842be..0da17ad056ec 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -459,6 +459,8 @@ struct intel_atomic_state {
> > > >  		 * state only when all crtc's are DPMS off.
> > > >  		 */
> > > >  		struct intel_cdclk_state actual;
> > > > +
> > > > +		int force_min_cdclk;
> > > >  	} cdclk;
> > > >  
> > > >  	bool dpll_set, modeset;
> > > > -- 
> > > > 2.7.4
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
  2018-09-13 13:54       ` Imre Deak
@ 2018-09-13 14:49         ` Takashi Iwai
  0 siblings, 0 replies; 17+ messages in thread
From: Takashi Iwai @ 2018-09-13 14:49 UTC (permalink / raw)
  To: imre.deak; +Cc: jani.nikula, intel-gfx, ville.syrjala

On Thu, 13 Sep 2018 15:54:37 +0200,
Imre Deak wrote:
> 
> On Wed, Sep 12, 2018 at 08:18:37PM +0200, Takashi Iwai wrote:
> > On Wed, 12 Sep 2018 15:18:47 +0200,
> > Imre Deak wrote:
> > > 
> > > +Takashi
> > > 
> > > On Wed, Sep 12, 2018 at 04:18:12PM +0300, Imre Deak wrote:
> > > > Hi Kumar, Takashi,
> > > > 
> > > > On Tue, Jun 19, 2018 at 03:01:11PM -0700, Abhay Kumar wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > CDCLK has to be at least twice the BLCK regardless of audio. Audio
> > > > > driver has to probe using this hook and increase the clock even in
> > > > > absence of any display.
> > > > > 
> > > > > v2: Use atomic refcount for get_power, put_power so that we can
> > > > >     call each once(Abhay).
> > > > > v3: Reset power well 2 to avoid any transaction on iDisp link
> > > > >     during cdclk change(Abhay).
> > > > > v4: Remove Power well 2 reset workaround(Ville).
> > > > > v5: Remove unwanted Power well 2 register defined in v4(Abhay).
> > > > > 
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.h      |  3 ++
> > > > >  drivers/gpu/drm/i915/intel_audio.c   | 67 +++++++++++++++++++++++++++++++++---
> > > > >  drivers/gpu/drm/i915/intel_cdclk.c   | 29 +++++-----------
> > > > >  drivers/gpu/drm/i915/intel_display.c |  7 +++-
> > > > >  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> > > > >  5 files changed, 83 insertions(+), 25 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 6104d7115054..a4a386a5db69 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -1702,6 +1702,7 @@ struct drm_i915_private {
> > > > >  	unsigned int hpll_freq;
> > > > >  	unsigned int fdi_pll_freq;
> > > > >  	unsigned int czclk_freq;
> > > > > +	u32 get_put_refcount;
> > > > >  
> > > > >  	struct {
> > > > >  		/*
> > > > > @@ -1719,6 +1720,8 @@ struct drm_i915_private {
> > > > >  		struct intel_cdclk_state actual;
> > > > >  		/* The current hardware cdclk state */
> > > > >  		struct intel_cdclk_state hw;
> > > > > +
> > > > > +		int force_min_cdclk;
> > > > >  	} cdclk;
> > > > >  
> > > > >  	/**
> > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > > > index 3ea566f99450..ca8f04c7cbb3 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > > @@ -618,7 +618,6 @@ void intel_audio_codec_enable(struct intel_encoder *encoder,
> > > > >  
> > > > >  	if (!connector->eld[0])
> > > > >  		return;
> > > > > -
> > > > >  	DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
> > > > >  			 connector->base.id,
> > > > >  			 connector->name,
> > > > > @@ -713,14 +712,74 @@ void intel_init_audio_hooks(struct drm_i915_private *dev_priv)
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
> > > > > +				bool enable)
> > > > > +{
> > > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > > +	struct drm_atomic_state *state;
> > > > > +	int ret;
> > > > > +
> > > > > +	drm_modeset_acquire_init(&ctx, 0);
> > > > > +	state = drm_atomic_state_alloc(&dev_priv->drm);
> > > > > +	if (WARN_ON(!state))
> > > > > +		return;
> > > > > +
> > > > > +	state->acquire_ctx = &ctx;
> > > > > +
> > > > > +retry:
> > > > > +	to_intel_atomic_state(state)->modeset = true;
> > > > > +	to_intel_atomic_state(state)->cdclk.force_min_cdclk =
> > > > > +		enable ? 2 * 96000 : 0;
> > > > > +
> > > > > +	/*
> > > > > +	 * Protects dev_priv->cdclk.force_min_cdclk
> > > > > +	 * Need to lock this here in case we have no active pipes
> > > > > +	 * and thus wouldn't lock it during the commit otherwise.
> > > > > +	 */
> > > > > +	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, &ctx);
> > > > > +	if (!ret)
> > > > > +		ret = drm_atomic_commit(state);
> > > > 
> > > > Ville mentioned a potential dead-lock problem here: a normal modeset
> > > > enabling an HDMI/DP output with an audio sink will call the
> > > > drm_audio_component_audio_ops::pin_eld_notify() callback with the
> > > > connection_mutex already held. This callback in turn could call
> > > > drm_audio_component_ops::get_power()/put_power() callbacks and
> > > > dead-lock at the above drm_modeset_lock() call.
> > > > 
> > > > Looks like that
> > > > 
> > > >    sound/pci/hda/patch_hdmi.c / intel_pin_eld_notify()->
> > > >    check_presence_and_report()->
> > > >    hdmi_present_sense()
> > > > 
> > > > would prevent this, but for instance
> > > > 
> > > >    sound/soc/codecs/hdac_hdmi.c / hdac_hdmi_eld_notify_cb()->
> > > >    hdac_hdmi_present_sense()->
> > > >    hdac_hdmi_jack_report()->
> > > >    snd_soc_jack_report()->
> > > >    snd_soc_dapm_sync()->
> > > >    snd_soc_dapm_sync_unlocked()->
> > > >    dapm_power_widgets()->
> > > >    dapm_pre_sequence_async()
> > > > 
> > > > could call pm_runtime_get_sync() and so I guess also the aops
> > > > get_power() hook.
> > > > 
> > > > Could someone in your team check if the above can indeed happen?
> > 
> > Through a quick glance, I'm afraid that it's indeed possible.  We need
> > to off-load either the hdac_hdmi jack handling or this new
> > force_audio_cdclk stuff, I suppose.
> 
> Ok. We need the force_audio_cdclk thing whenever the audio driver needs
> power and calls the aops::get_power() hook. IIUC this happens from various
> places in the HDA driver and get_power() should enable power
> synchronously, so not sure how we could off-load that..
> 
> How about the same for the jack handling, perhaps only in the case it's
> done from hdac_hdmi_eld_notify_cb()?

Yes, this shouldn't be too hard.  A totally untested patch is below.

> Also could you explain why patch_hdmi.c can do without get_power()?
> Could the same thing be done in hdac_hdmi.c?

In the legacy HD-audio driver, what driver does is essentially only
the update of the jack and the ELD ctl elements.  The state is already
stored in GPU driver, hence we need no GPU power up, but just read
it and set to the sound ctl values.

OTOH, on ASoC hdac_hdmi driver, it kicks off the DAPM update depending
the jack state, and it's involved with the power up because it touches
the whole graph.


Takashi

--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -96,8 +96,10 @@ struct hdac_hdmi_port {
 	hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
 	struct hdac_hdmi_eld eld;
 	const char *jack_pin;
+	bool is_connect;
 	struct snd_soc_dapm_context *dapm;
 	const char *output_pin;
+	struct work_struct dapm_work;
 };
 
 struct hdac_hdmi_pcm {
@@ -158,16 +160,12 @@ hdac_hdmi_get_pcm_from_cvt(struct hdac_hdmi_priv *hdmi,
 	return pcm;
 }
 
-static void hdac_hdmi_jack_report(struct hdac_hdmi_pcm *pcm,
+static void __hdac_hdmi_jack_report(struct hdac_hdmi_pcm *pcm,
 		struct hdac_hdmi_port *port, bool is_connect)
 {
 	struct hdac_device *hdev = port->pin->hdev;
 
-	if (is_connect)
-		snd_soc_dapm_enable_pin(port->dapm, port->jack_pin);
-	else
-		snd_soc_dapm_disable_pin(port->dapm, port->jack_pin);
-
+	port->is_connect = is_connect;
 	if (is_connect) {
 		/*
 		 * Report Jack connect event when a device is connected
@@ -193,10 +191,32 @@ static void hdac_hdmi_jack_report(struct hdac_hdmi_pcm *pcm,
 		if (pcm->jack_event > 0)
 			pcm->jack_event--;
 	}
+}
 
+static void hdac_hdmi_port_dapm_update(struct hdac_hdmi_port *port)
+{
+	if (port->is_connect)
+		snd_soc_dapm_enable_pin(port->dapm, port->jack_pin);
+	else
+		snd_soc_dapm_disable_pin(port->dapm, port->jack_pin);
 	snd_soc_dapm_sync(port->dapm);
 }
 
+static void hdac_hdmi_jack_dapm_work(struct work_struct *work)
+{
+	struct hdac_hdmi_port *port;
+
+	port = container_of(work, struct hdac_hdmi_port, dapm_work);
+	hdac_hdmi_port_dapm_update(port);
+}
+
+static void hdac_hdmi_jack_report(struct hdac_hdmi_pcm *pcm,
+		struct hdac_hdmi_port *port, bool is_connect)
+{
+	__hdac_hdmi_jack_report(pcm, port, is_connect);
+	hdac_hdmi_port_dapm_update(port);
+}
+
 /* MST supported verbs */
 /*
  * Get the no devices that can be connected to a port on the Pin widget.
@@ -1261,16 +1281,20 @@ static void hdac_hdmi_present_sense(struct hdac_hdmi_pin *pin,
 		 * report jack here. It will be done in usermode mux
 		 * control select.
 		 */
-		if (pcm)
-			hdac_hdmi_jack_report(pcm, port, false);
+		if (pcm) {
+			__hdac_hdmi_jack_report(pcm, port, false);
+			schedule_work(&port->dapm_work);
+		}
 
 		mutex_unlock(&hdmi->pin_mutex);
 		return;
 	}
 
 	if (port->eld.monitor_present && port->eld.eld_valid) {
-		if (pcm)
-			hdac_hdmi_jack_report(pcm, port, true);
+		if (pcm) {
+			__hdac_hdmi_jack_report(pcm, port, true);
+			schedule_work(&port->dapm_work);
+		}
 
 		print_hex_dump_debug("ELD: ", DUMP_PREFIX_OFFSET, 16, 1,
 			  port->eld.eld_buffer, port->eld.eld_size, false);
@@ -1299,6 +1323,7 @@ static int hdac_hdmi_add_ports(struct hdac_hdmi_priv *hdmi,
 	for (i = 0; i < max_ports; i++) {
 		ports[i].id = i;
 		ports[i].pin = pin;
+		INIT_WORK(&ports[i].dapm_work, hdac_hdmi_jack_dapm_work);
 	}
 	pin->ports = ports;
 	pin->num_ports = max_ports;
@@ -2062,6 +2087,10 @@ static int hdac_hdmi_dev_remove(struct hdac_device *hdev)
 	struct hdac_hdmi_port *port, *port_next;
 	int i;
 
+	list_for_each_entry(pin, &hdmi->pin_list, head)
+		for (i = 0; i < pin->num_ports; i++)
+			cancel_work_sync(& pin->ports[i].dapm_work);
+
 	list_for_each_entry_safe(pcm, pcm_next, &hdmi->pcm_list, head) {
 		pcm->cvt = NULL;
 		if (list_empty(&pcm->port_list))
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev8)
  2018-06-19 22:01 [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled Abhay Kumar
                   ` (5 preceding siblings ...)
  2018-09-12 13:18 ` Imre Deak
@ 2018-09-13 15:07 ` Patchwork
  2018-09-13 15:26 ` ✓ Fi.CI.BAT: success " Patchwork
  2018-09-13 16:16 ` ✓ Fi.CI.IGT: " Patchwork
  8 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-09-13 15:07 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev8)
URL   : https://patchwork.freedesktop.org/series/42459/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
8d4f7a2b425b drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
-:25: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#25: 
> > > > > CDCLK has to be at least twice the BLCK regardless of audio. Audio

-:189: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#189: FILE: sound/soc/codecs/hdac_hdmi.c:164:
+static void __hdac_hdmi_jack_report(struct hdac_hdmi_pcm *pcm,
 		struct hdac_hdmi_port *port, bool is_connect)

-:226: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#226: FILE: sound/soc/codecs/hdac_hdmi.c:214:
+static void hdac_hdmi_jack_report(struct hdac_hdmi_pcm *pcm,
+		struct hdac_hdmi_port *port, bool is_connect)

-:274: ERROR:SPACING: space prohibited after that '&' (ctx:BxW)
#274: FILE: sound/soc/codecs/hdac_hdmi.c:2092:
+			cancel_work_sync(& pin->ports[i].dapm_work);
 			                 ^

-:278: ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s)

total: 2 errors, 1 warnings, 2 checks, 101 lines checked

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev8)
  2018-06-19 22:01 [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled Abhay Kumar
                   ` (6 preceding siblings ...)
  2018-09-13 15:07 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev8) Patchwork
@ 2018-09-13 15:26 ` Patchwork
  2018-09-13 16:16 ` ✓ Fi.CI.IGT: " Patchwork
  8 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-09-13 15:26 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev8)
URL   : https://patchwork.freedesktop.org/series/42459/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4816 -> Patchwork_10169 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42459/revisions/8/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_psr@primary_mmap_gtt:
      {fi-cnl-u}:         NOTRUN -> FAIL (fdo#107383) +3

    igt@kms_psr@primary_page_flip:
      fi-cfl-s3:          PASS -> FAIL (fdo#107336)
      fi-kbl-r:           PASS -> FAIL (fdo#107336)

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
      fi-byt-clapper:     FAIL (fdo#107362) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-byt-clapper:     FAIL (fdo#103191, fdo#107362) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107383 https://bugs.freedesktop.org/show_bug.cgi?id=107383


== Participating hosts (49 -> 45) ==

  Additional (2): fi-cnl-u fi-icl-u 
  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus 


== Build changes ==

    * Linux: CI_DRM_4816 -> Patchwork_10169

  CI_DRM_4816: 5bebc54ac552e3716bfe0f1f7eb0babfbda49f09 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4640: 9a8da36e708f9ed15b20689dfe305e41f9a19008 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10169: 8d4f7a2b425b893afe5b2b4ecea3d185e45e399f @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

8d4f7a2b425b drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled

== Logs ==

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* ✓ Fi.CI.IGT: success for drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev8)
  2018-06-19 22:01 [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled Abhay Kumar
                   ` (7 preceding siblings ...)
  2018-09-13 15:26 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-09-13 16:16 ` Patchwork
  8 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-09-13 16:16 UTC (permalink / raw)
  To: >Takashi Iwai; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev8)
URL   : https://patchwork.freedesktop.org/series/42459/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4816_full -> Patchwork_10169_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@shrink:
      shard-glk:          PASS -> INCOMPLETE (k.org#198133, fdo#106886, fdo#103359)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-shrfb-draw-mmap-gtt:
      shard-glk:          PASS -> FAIL (fdo#103167)

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)
      shard-kbl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@gem_exec_await@wide-contexts:
      shard-glk:          FAIL (fdo#106680) -> PASS

    igt@kms_color@pipe-c-ctm-max:
      shard-kbl:          DMESG-WARN (fdo#105602, fdo#103558) -> PASS +34

    igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
      shard-hsw:          FAIL (fdo#105767) -> PASS

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
  fdo#106680 https://bugs.freedesktop.org/show_bug.cgi?id=106680
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4816 -> Patchwork_10169

  CI_DRM_4816: 5bebc54ac552e3716bfe0f1f7eb0babfbda49f09 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4640: 9a8da36e708f9ed15b20689dfe305e41f9a19008 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10169: 8d4f7a2b425b893afe5b2b4ecea3d185e45e399f @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2018-09-13 16:16 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-19 22:01 [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled Abhay Kumar
2018-06-19 22:13 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev6) Patchwork
2018-06-19 22:14 ` ✗ Fi.CI.SPARSE: " Patchwork
2018-06-19 22:29 ` ✓ Fi.CI.BAT: success " Patchwork
2018-06-19 23:46 ` ✓ Fi.CI.IGT: " Patchwork
2018-06-21 18:43 ` [PATCH v5] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled Kumar, Abhay
2018-06-21 20:16   ` Du,Wenkai
2018-07-03  5:06     ` Kumar, Abhay
2018-09-12 13:18 ` Imre Deak
2018-09-12 13:18   ` Imre Deak
2018-09-12 18:18     ` Takashi Iwai
2018-09-13 13:54       ` Imre Deak
2018-09-13 14:49         ` Takashi Iwai
2018-09-12 13:49   ` Ville Syrjälä
2018-09-13 15:07 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled (rev8) Patchwork
2018-09-13 15:26 ` ✓ Fi.CI.BAT: success " Patchwork
2018-09-13 16:16 ` ✓ Fi.CI.IGT: " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.