[PATCH v2] drm/i915/ddi: Get AUX power domain for DP main link too

[PATCH v2] drm/i915/ddi: Get AUX power domain for DP main link too

[Imre Deak]

So far we got an AUX power domain reference only for the duration of DP
AUX transfers. However, the following suggests that we also need these
for main link functionality:
- The specification doesn't state whether it's needed or not for main
  link functionality, but suggests that these power wells need to be
  enabled already during display core initialization (Sequences to
  Initialize Display).
- For PSR we need to keep the AUX power well enabled.
- On ICL combo PHY ports (non-TC) the AUX power well is needed for
  link training too: while the port is enabled with a DP link training
  test pattern trying to toggle the AUX power well will time out.
- On ICL MG PHY ports (TC) the AUX power well is needed also for main
  link functionality (both in DP and HDMI modes).
- Windows enables these power wells both for main and AUX lane
  functionality.

Based on the above take an AUX power reference for main link
functionality too. This makes a difference only on GEN10+ (GLK+)
platforms, where we have separate port specific AUX power wells.

For PSR we still need to distinguish between port A and the other
ports, since on port A DC states must stay enabled for main link
functionality, but DC states must be disabled for driver initiated
AUX transfers. So re-use the corresponding helper from intel_psr.c.

Since we take now a rereference for main link functionality on all DP
ports we can forgo taking the seperate power ref for PSR functionality.

v2:
- Make sure DC states stay enabled when taking the ref on port A.
  (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c        | 54 ++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_display.c    | 12 +++++++-
 drivers/gpu/drm/i915/intel_drv.h        |  3 +-
 drivers/gpu/drm/i915/intel_psr.c        | 41 -------------------------
 drivers/gpu/drm/i915/intel_runtime_pm.c |  1 +
 5 files changed, 64 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 044fe1fb9872..146a9daa6564 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1983,15 +1983,55 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
 	return ret;
 }
 
-static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder)
+static inline enum intel_display_power_domain
+intel_ddi_main_link_aux_domain(struct intel_dp *intel_dp)
+{
+	/* CNL HW requires corresponding AUX IOs to be powered up for PSR with
+	 * DC states enabled at the same time, while for driver initiated AUX
+	 * transfers we need the same AUX IOs to be powered but with DC states
+	 * disabled. Accordingly use the AUX power domain here which leaves DC
+	 * states enabled.
+	 * However, for non-A AUX ports the corresponding non-EDP transcoders
+	 * would have already enabled power well 2 and DC_OFF. This means we can
+	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference instead of a
+	 * specific AUX_IO reference without powering up any extra wells.
+	 * Note that PSR is enabled only on Port A even though this function
+	 * returns the correct domain for other ports too.
+	 */
+	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
+					      intel_dp->aux_power_domain;
+}
+
+static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder,
+				       struct intel_crtc_state *crtc_state)
 {
 	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
 	enum pipe pipe;
+	u64 domains;
 
-	if (intel_ddi_get_hw_state(encoder, &pipe))
-		return BIT_ULL(dig_port->ddi_io_power_domain);
+	if (!intel_ddi_get_hw_state(encoder, &pipe))
+		return 0;
 
-	return 0;
+	domains = BIT_ULL(dig_port->ddi_io_power_domain);
+	if (!crtc_state)
+		return domains;
+
+	/*
+	 * TODO: Add support for MST encoders. Atm, the following should never
+	 * happen since this function will be called only for the primary
+	 * encoder which doesn't have its own pipe/crtc_state.
+	 */
+	if (WARN_ON(intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)))
+		return domains;
+
+	/* AUX power is only needed for (e)DP mode, not for HDMI. */
+	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
+		struct intel_dp *intel_dp = &dig_port->dp;
+
+		domains |= BIT_ULL(intel_ddi_main_link_aux_domain(intel_dp));
+	}
+
+	return domains;
 }
 
 void intel_ddi_enable_pipe_clock(const struct intel_crtc_state *crtc_state)
@@ -2631,6 +2671,9 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 
 	WARN_ON(is_mst && (port == PORT_A || port == PORT_E));
 
+	intel_display_power_get(dev_priv,
+				intel_ddi_main_link_aux_domain(intel_dp));
+
 	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
 				 crtc_state->lane_count, is_mst);
 
@@ -2775,6 +2818,9 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder,
 	intel_display_power_put(dev_priv, dig_port->ddi_io_power_domain);
 
 	intel_ddi_clk_disable(encoder);
+
+	intel_display_power_put(dev_priv,
+				intel_ddi_main_link_aux_domain(intel_dp));
 }
 
 static void intel_ddi_post_disable_hdmi(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2c8fef3ede54..d9fefcec4b1a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv)
 	for_each_intel_encoder(&dev_priv->drm, encoder) {
 		u64 get_domains;
 		enum intel_display_power_domain domain;
+		struct intel_crtc_state *crtc_state;
 
 		if (!encoder->get_power_domains)
 			continue;
 
-		get_domains = encoder->get_power_domains(encoder);
+		/*
+		 * In case of a dangling encoder without a crtc we still need
+		 * to get power domain refs. Such encoders will be disabled
+		 * dropping these references.
+		 */
+		crtc_state = encoder->base.crtc ?
+			     to_intel_crtc_state(encoder->base.crtc->state) :
+			     NULL;
+
+		get_domains = encoder->get_power_domains(encoder, crtc_state);
 		for_each_power_domain(domain, get_domains)
 			intel_display_power_get(dev_priv, domain);
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0c3ac0eafde0..7174429d930a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -254,7 +254,8 @@ struct intel_encoder {
 			   struct intel_crtc_state *pipe_config);
 	/* Returns a mask of power domains that need to be referenced as part
 	 * of the hardware state readout code. */
-	u64 (*get_power_domains)(struct intel_encoder *encoder);
+	u64 (*get_power_domains)(struct intel_encoder *encoder,
+				 struct intel_crtc_state *crtc_state);
 	/*
 	 * Called during system suspend after all pending requests for the
 	 * encoder are flushed (for example for DP AUX transactions) and
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index d4cd19fea148..eecdd8b5b5e0 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -56,43 +56,6 @@
 #include "intel_drv.h"
 #include "i915_drv.h"
 
-static inline enum intel_display_power_domain
-psr_aux_domain(struct intel_dp *intel_dp)
-{
-	/* CNL HW requires corresponding AUX IOs to be powered up for PSR.
-	 * However, for non-A AUX ports the corresponding non-EDP transcoders
-	 * would have already enabled power well 2 and DC_OFF. This means we can
-	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference instead of a
-	 * specific AUX_IO reference without powering up any extra wells.
-	 * Note that PSR is enabled only on Port A even though this function
-	 * returns the correct domain for other ports too.
-	 */
-	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
-					      intel_dp->aux_power_domain;
-}
-
-static void psr_aux_io_power_get(struct intel_dp *intel_dp)
-{
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
-
-	if (INTEL_GEN(dev_priv) < 10)
-		return;
-
-	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
-}
-
-static void psr_aux_io_power_put(struct intel_dp *intel_dp)
-{
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
-
-	if (INTEL_GEN(dev_priv) < 10)
-		return;
-
-	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
-}
-
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug)
 {
 	u32 debug_mask, mask;
@@ -595,8 +558,6 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
 
-	psr_aux_io_power_get(intel_dp);
-
 	/* Only HSW and BDW have PSR AUX registers that need to be setup. SKL+
 	 * use hardcoded values PSR AUX transactions
 	 */
@@ -717,8 +678,6 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
 		else
 			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
 	}
-
-	psr_aux_io_power_put(intel_dp);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index de3a81034f77..2969787201ef 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1824,6 +1824,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	BIT_ULL(POWER_DOMAIN_INIT))
 #define GLK_DISPLAY_AUX_A_POWER_DOMAINS (		\
 	BIT_ULL(POWER_DOMAIN_AUX_A) |		\
+	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
 	BIT_ULL(POWER_DOMAIN_INIT))
 #define GLK_DISPLAY_AUX_B_POWER_DOMAINS (		\
 	BIT_ULL(POWER_DOMAIN_AUX_B) |		\
-- 
2.13.2

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

✗ Fi.CI.CHECKPATCH: warning for drm/i915/ddi: Get AUX power domain for DP main link too

[Patchwork]

== Series Details ==

Series: drm/i915/ddi: Get AUX power domain for DP main link too
URL   : https://patchwork.freedesktop.org/series/45188/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
e686fff14a1c drm/i915/ddi: Get AUX power domain for DP main link too
-:35: WARNING:TYPO_SPELLING: 'seperate' may be misspelled - perhaps 'separate'?
#35: 
ports we can forgo taking the seperate power ref for PSR functionality.

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

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

✓ Fi.CI.BAT: success for drm/i915/ddi: Get AUX power domain for DP main link too

[Patchwork]

== Series Details ==

Series: drm/i915/ddi: Get AUX power domain for DP main link too
URL   : https://patchwork.freedesktop.org/series/45188/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4362 -> Patchwork_9386 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45188/revisions/1/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ctx_create@basic-files:
      fi-skl-guc:         PASS -> DMESG-WARN (fdo#106954)

    
    ==== Possible fixes ====

    igt@gem_ctx_create@basic-files:
      fi-skl-6700k2:      INCOMPLETE (fdo#106988) -> PASS

    
  fdo#106954 https://bugs.freedesktop.org/show_bug.cgi?id=106954
  fdo#106988 https://bugs.freedesktop.org/show_bug.cgi?id=106988


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

  Additional (1): fi-bxt-dsi 
  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-glk-dsi fi-bsw-cyan fi-ctg-p8600 fi-kbl-x1275 


== Build changes ==

    * Linux: CI_DRM_4362 -> Patchwork_9386

  CI_DRM_4362: 7159c27349b070831dfb3512ca055c8dbcf9e1fc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4528: 6be300d405de5974b262e8b93a445be4ac618e6a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9386: e686fff14a1c09cd8625de6fe968c0feea20f97c @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

e686fff14a1c drm/i915/ddi: Get AUX power domain for DP main link too

== Logs ==

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

[PATCH v3] drm/i915/ddi: Get AUX power domain for DP main link too

[Imre Deak]

So far we got an AUX power domain reference only for the duration of DP
AUX transfers. However, the following suggests that we also need these
for main link functionality:
- The specification doesn't state whether it's needed or not for main
  link functionality, but suggests that these power wells need to be
  enabled already during display core initialization (Sequences to
  Initialize Display).
- For PSR we need to keep the AUX power well enabled.
- On ICL combo PHY ports (non-TC) the AUX power well is needed for
  link training too: while the port is enabled with a DP link training
  test pattern trying to toggle the AUX power well will time out.
- On ICL MG PHY ports (TC) the AUX power well is needed also for main
  link functionality (both in DP and HDMI modes).
- Windows enables these power wells both for main and AUX lane
  functionality.

Based on the above take an AUX power reference for main link
functionality too. This makes a difference only on GEN10+ (GLK+)
platforms, where we have separate port specific AUX power wells.

For PSR we still need to distinguish between port A and the other
ports, since on port A DC states must stay enabled for main link
functionality, but DC states must be disabled for driver initiated
AUX transfers. So re-use the corresponding helper from intel_psr.c.

Since we take now a reference for main link functionality on all DP
ports we can forgo taking the separate power ref for PSR functionality.

v2:
- Make sure DC states stay enabled when taking the ref on port A.
  (Ville)

v3: (Ville)
- Fix comment about logic for encoders without a crtc state and
  add FIXME note for a simplification to avoid calling get_power_domains
  in such cases.
- Use intel_crtc_has_dp_encoder() instead !intel_crtc_has_type(HDMI).

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c        | 54 ++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_display.c    | 12 +++++++-
 drivers/gpu/drm/i915/intel_drv.h        |  3 +-
 drivers/gpu/drm/i915/intel_psr.c        | 41 -------------------------
 drivers/gpu/drm/i915/intel_runtime_pm.c |  1 +
 5 files changed, 64 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 044fe1fb9872..0319825b725b 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1983,15 +1983,55 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
 	return ret;
 }
 
-static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder)
+static inline enum intel_display_power_domain
+intel_ddi_main_link_aux_domain(struct intel_dp *intel_dp)
+{
+	/* CNL HW requires corresponding AUX IOs to be powered up for PSR with
+	 * DC states enabled at the same time, while for driver initiated AUX
+	 * transfers we need the same AUX IOs to be powered but with DC states
+	 * disabled. Accordingly use the AUX power domain here which leaves DC
+	 * states enabled.
+	 * However, for non-A AUX ports the corresponding non-EDP transcoders
+	 * would have already enabled power well 2 and DC_OFF. This means we can
+	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference instead of a
+	 * specific AUX_IO reference without powering up any extra wells.
+	 * Note that PSR is enabled only on Port A even though this function
+	 * returns the correct domain for other ports too.
+	 */
+	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
+					      intel_dp->aux_power_domain;
+}
+
+static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder,
+				       struct intel_crtc_state *crtc_state)
 {
 	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
 	enum pipe pipe;
+	u64 domains;
 
-	if (intel_ddi_get_hw_state(encoder, &pipe))
-		return BIT_ULL(dig_port->ddi_io_power_domain);
+	if (!intel_ddi_get_hw_state(encoder, &pipe))
+		return 0;
 
-	return 0;
+	domains = BIT_ULL(dig_port->ddi_io_power_domain);
+	if (!crtc_state)
+		return domains;
+
+	/*
+	 * TODO: Add support for MST encoders. Atm, the following should never
+	 * happen since this function will be called only for the primary
+	 * encoder which doesn't have its own pipe/crtc_state.
+	 */
+	if (WARN_ON(intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)))
+		return domains;
+
+	/* AUX power is only needed for (e)DP mode, not for HDMI. */
+	if (intel_crtc_has_dp_encoder(crtc_state)) {
+		struct intel_dp *intel_dp = &dig_port->dp;
+
+		domains |= BIT_ULL(intel_ddi_main_link_aux_domain(intel_dp));
+	}
+
+	return domains;
 }
 
 void intel_ddi_enable_pipe_clock(const struct intel_crtc_state *crtc_state)
@@ -2631,6 +2671,9 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 
 	WARN_ON(is_mst && (port == PORT_A || port == PORT_E));
 
+	intel_display_power_get(dev_priv,
+				intel_ddi_main_link_aux_domain(intel_dp));
+
 	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
 				 crtc_state->lane_count, is_mst);
 
@@ -2775,6 +2818,9 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder,
 	intel_display_power_put(dev_priv, dig_port->ddi_io_power_domain);
 
 	intel_ddi_clk_disable(encoder);
+
+	intel_display_power_put(dev_priv,
+				intel_ddi_main_link_aux_domain(intel_dp));
 }
 
 static void intel_ddi_post_disable_hdmi(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2c8fef3ede54..ce615f89b43a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv)
 	for_each_intel_encoder(&dev_priv->drm, encoder) {
 		u64 get_domains;
 		enum intel_display_power_domain domain;
+		struct intel_crtc_state *crtc_state;
 
 		if (!encoder->get_power_domains)
 			continue;
 
-		get_domains = encoder->get_power_domains(encoder);
+		/*
+		 * For MST and inactive encoders we don't have a crtc state.
+		 * FIXME: no need to call get_power_domains in such cases, it
+		 * will always return 0.
+		 */
+		crtc_state = encoder->base.crtc ?
+			     to_intel_crtc_state(encoder->base.crtc->state) :
+			     NULL;
+
+		get_domains = encoder->get_power_domains(encoder, crtc_state);
 		for_each_power_domain(domain, get_domains)
 			intel_display_power_get(dev_priv, domain);
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0c3ac0eafde0..7174429d930a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -254,7 +254,8 @@ struct intel_encoder {
 			   struct intel_crtc_state *pipe_config);
 	/* Returns a mask of power domains that need to be referenced as part
 	 * of the hardware state readout code. */
-	u64 (*get_power_domains)(struct intel_encoder *encoder);
+	u64 (*get_power_domains)(struct intel_encoder *encoder,
+				 struct intel_crtc_state *crtc_state);
 	/*
 	 * Called during system suspend after all pending requests for the
 	 * encoder are flushed (for example for DP AUX transactions) and
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index d4cd19fea148..eecdd8b5b5e0 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -56,43 +56,6 @@
 #include "intel_drv.h"
 #include "i915_drv.h"
 
-static inline enum intel_display_power_domain
-psr_aux_domain(struct intel_dp *intel_dp)
-{
-	/* CNL HW requires corresponding AUX IOs to be powered up for PSR.
-	 * However, for non-A AUX ports the corresponding non-EDP transcoders
-	 * would have already enabled power well 2 and DC_OFF. This means we can
-	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference instead of a
-	 * specific AUX_IO reference without powering up any extra wells.
-	 * Note that PSR is enabled only on Port A even though this function
-	 * returns the correct domain for other ports too.
-	 */
-	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
-					      intel_dp->aux_power_domain;
-}
-
-static void psr_aux_io_power_get(struct intel_dp *intel_dp)
-{
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
-
-	if (INTEL_GEN(dev_priv) < 10)
-		return;
-
-	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
-}
-
-static void psr_aux_io_power_put(struct intel_dp *intel_dp)
-{
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
-
-	if (INTEL_GEN(dev_priv) < 10)
-		return;
-
-	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
-}
-
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug)
 {
 	u32 debug_mask, mask;
@@ -595,8 +558,6 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
 
-	psr_aux_io_power_get(intel_dp);
-
 	/* Only HSW and BDW have PSR AUX registers that need to be setup. SKL+
 	 * use hardcoded values PSR AUX transactions
 	 */
@@ -717,8 +678,6 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
 		else
 			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
 	}
-
-	psr_aux_io_power_put(intel_dp);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index de3a81034f77..2969787201ef 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1824,6 +1824,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	BIT_ULL(POWER_DOMAIN_INIT))
 #define GLK_DISPLAY_AUX_A_POWER_DOMAINS (		\
 	BIT_ULL(POWER_DOMAIN_AUX_A) |		\
+	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
 	BIT_ULL(POWER_DOMAIN_INIT))
 #define GLK_DISPLAY_AUX_B_POWER_DOMAINS (		\
 	BIT_ULL(POWER_DOMAIN_AUX_B) |		\
-- 
2.13.2

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

✓ Fi.CI.BAT: success for drm/i915/ddi: Get AUX power domain for DP main link too (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/ddi: Get AUX power domain for DP main link too (rev2)
URL   : https://patchwork.freedesktop.org/series/45188/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4363 -> Patchwork_9388 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45188/revisions/2/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

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

    
    ==== Possible fixes ====

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       DMESG-FAIL (fdo#102614, fdo#106103) -> PASS

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103


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

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-glk-dsi fi-ctg-p8600 fi-kbl-x1275 


== Build changes ==

    * Linux: CI_DRM_4363 -> Patchwork_9388

  CI_DRM_4363: 9e52e63f91e95af0f7475ccce206d4bdfca8933a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4528: 6be300d405de5974b262e8b93a445be4ac618e6a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9388: 6df9a07560c7c1343343e5d9332972e84a4c27bb @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

6df9a07560c7 drm/i915/ddi: Get AUX power domain for DP main link too

== Logs ==

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

Re: [PATCH v3] drm/i915/ddi: Get AUX power domain for DP main link too

[Imre Deak]

On Thu, Jun 21, 2018 at 01:14:30PM -0700, Dhinakaran Pandiyan wrote:
> On Thu, 2018-06-21 at 21:44 +0300, Imre Deak wrote:
> > So far we got an AUX power domain reference only for the duration of
> > DP
> > AUX transfers. However, the following suggests that we also need
> > these
> > for main link functionality:
> > - The specification doesn't state whether it's needed or not for main
> >   link functionality, but suggests that these power wells need to be
> >   enabled already during display core initialization (Sequences to
> >   Initialize Display).
> Hmm. The sequence also says "If an Aux channel will not be used, it
> does not need to be powered up."

Well, I consider that hints at not using the port at all for DP, since
for DP you will need AUX.

> 
> > - For PSR we need to keep the AUX power well enabled.
> > - On ICL combo PHY ports (non-TC) the AUX power well is needed for
> >   link training too: while the port is enabled with a DP link
> > training
> I don't see this in the spec, is this something you observed?

Yes.

> 
> >   test pattern trying to toggle the AUX power well will time out.
> 
> Can we enable AUX power well before link training starts and disable
> after we are done training? Is that enough?

That was my first idea too, but given the rest of the points I consider
it a safer option to enable it for main link functionality.

> 
> > - On ICL MG PHY ports (TC) the AUX power well is needed also for main
> >   link functionality (both in DP and HDMI modes).
> > - Windows enables these power wells both for main and AUX lane
> >   functionality.
> > 
> > Based on the above take an AUX power reference for main link
> > functionality too. This makes a difference only on GEN10+ (GLK+)
> > platforms, where we have separate port specific AUX power wells.
> > 
> > For PSR we still need to distinguish between port A and the other
> > ports, since on port A DC states must stay enabled for main link
> > functionality, but DC states must be disabled for driver initiated
> > AUX transfers. So re-use the corresponding helper from intel_psr.c.
> > 
> > Since we take now a reference for main link functionality on all DP
> > ports we can forgo taking the separate power ref for PSR
> > functionality.
> > 
> > v2:
> > - Make sure DC states stay enabled when taking the ref on port A.
> >   (Ville)
> > 
> > v3: (Ville)
> > - Fix comment about logic for encoders without a crtc state and
> >   add FIXME note for a simplification to avoid calling
> > get_power_domains
> >   in such cases.
> > - Use intel_crtc_has_dp_encoder() instead !intel_crtc_has_type(HDMI).
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c        | 54
> > ++++++++++++++++++++++++++++++---
> >  drivers/gpu/drm/i915/intel_display.c    | 12 +++++++-
> >  drivers/gpu/drm/i915/intel_drv.h        |  3 +-
> >  drivers/gpu/drm/i915/intel_psr.c        | 41 ---------------------
> > ----
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  1 +
> >  5 files changed, 64 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 044fe1fb9872..0319825b725b 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1983,15 +1983,55 @@ bool intel_ddi_get_hw_state(struct
> > intel_encoder *encoder,
> >  	return ret;
> >  }
> >  
> > -static u64 intel_ddi_get_power_domains(struct intel_encoder
> > *encoder)
> > +static inline enum intel_display_power_domain
> > +intel_ddi_main_link_aux_domain(struct intel_dp *intel_dp)
> > +{
> > +	/* CNL HW requires corresponding AUX IOs to be powered up
> > for PSR with
> > +	 * DC states enabled at the same time, while for driver
> > initiated AUX
> > +	 * transfers we need the same AUX IOs to be powered but with
> > DC states
> > +	 * disabled. Accordingly use the AUX power domain here which
> > leaves DC
> > +	 * states enabled.
> > +	 * However, for non-A AUX ports the corresponding non-EDP
> > transcoders
> > +	 * would have already enabled power well 2 and DC_OFF. This
> > means we can
> > +	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> > instead of a
> > +	 * specific AUX_IO reference without powering up any extra
> > wells.
> > +	 * Note that PSR is enabled only on Port A even though this
> > function
> > +	 * returns the correct domain for other ports too.
> > +	 */
> > +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A
> > :
> > +					      intel_dp-
> > >aux_power_domain;
> > +}
> > +
> > +static u64 intel_ddi_get_power_domains(struct intel_encoder
> > *encoder,
> > +				       struct intel_crtc_state
> > *crtc_state)
> >  {
> >  	struct intel_digital_port *dig_port =
> > enc_to_dig_port(&encoder->base);
> >  	enum pipe pipe;
> > +	u64 domains;
> >  
> > -	if (intel_ddi_get_hw_state(encoder, &pipe))
> > -		return BIT_ULL(dig_port->ddi_io_power_domain);
> > +	if (!intel_ddi_get_hw_state(encoder, &pipe))
> > +		return 0;
> >  
> > -	return 0;
> > +	domains = BIT_ULL(dig_port->ddi_io_power_domain);
> > +	if (!crtc_state)
> > +		return domains;
> > +
> > +	/*
> > +	 * TODO: Add support for MST encoders. Atm, the following
> > should never
> > +	 * happen since this function will be called only for the
> > primary
> > +	 * encoder which doesn't have its own pipe/crtc_state.
> > +	 */
> > +	if (WARN_ON(intel_crtc_has_type(crtc_state,
> > INTEL_OUTPUT_DP_MST)))
> > +		return domains;
> > +
> > +	/* AUX power is only needed for (e)DP mode, not for HDMI. */
> > +	if (intel_crtc_has_dp_encoder(crtc_state)) {
> > +		struct intel_dp *intel_dp = &dig_port->dp;
> > +
> > +		domains |=
> > BIT_ULL(intel_ddi_main_link_aux_domain(intel_dp));
> > +	}
> > +
> > +	return domains;
> >  }
> >  
> >  void intel_ddi_enable_pipe_clock(const struct intel_crtc_state
> > *crtc_state)
> > @@ -2631,6 +2671,9 @@ static void intel_ddi_pre_enable_dp(struct
> > intel_encoder *encoder,
> >  
> >  	WARN_ON(is_mst && (port == PORT_A || port == PORT_E));
> >  
> > +	intel_display_power_get(dev_priv,
> > +				intel_ddi_main_link_aux_domain(intel
> > _dp));
> > +
> >  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> >  				 crtc_state->lane_count, is_mst);
> >  
> > @@ -2775,6 +2818,9 @@ static void intel_ddi_post_disable_dp(struct
> > intel_encoder *encoder,
> >  	intel_display_power_put(dev_priv, dig_port-
> > >ddi_io_power_domain);
> >  
> >  	intel_ddi_clk_disable(encoder);
> > +
> > +	intel_display_power_put(dev_priv,
> > +				intel_ddi_main_link_aux_domain(intel
> > _dp));
> >  }
> >  
> >  static void intel_ddi_post_disable_hdmi(struct intel_encoder
> > *encoder,
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 2c8fef3ede54..ce615f89b43a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct
> > drm_i915_private *dev_priv)
> >  	for_each_intel_encoder(&dev_priv->drm, encoder) {
> >  		u64 get_domains;
> >  		enum intel_display_power_domain domain;
> > +		struct intel_crtc_state *crtc_state;
> >  
> >  		if (!encoder->get_power_domains)
> >  			continue;
> >  
> > -		get_domains = encoder->get_power_domains(encoder);
> > +		/*
> > +		 * For MST and inactive encoders we don't have a
> > crtc state.
> > +		 * FIXME: no need to call get_power_domains in such
> > cases, it
> > +		 * will always return 0.
> > +		 */
> > +		crtc_state = encoder->base.crtc ?
> > +			     to_intel_crtc_state(encoder->base.crtc-
> > >state) :
> > +			     NULL;
> > +
> > +		get_domains = encoder->get_power_domains(encoder,
> > crtc_state);
> >  		for_each_power_domain(domain, get_domains)
> >  			intel_display_power_get(dev_priv, domain);
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 0c3ac0eafde0..7174429d930a 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -254,7 +254,8 @@ struct intel_encoder {
> >  			   struct intel_crtc_state *pipe_config);
> >  	/* Returns a mask of power domains that need to be
> > referenced as part
> >  	 * of the hardware state readout code. */
> > -	u64 (*get_power_domains)(struct intel_encoder *encoder);
> > +	u64 (*get_power_domains)(struct intel_encoder *encoder,
> > +				 struct intel_crtc_state
> > *crtc_state);
> >  	/*
> >  	 * Called during system suspend after all pending requests
> > for the
> >  	 * encoder are flushed (for example for DP AUX transactions)
> > and
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index d4cd19fea148..eecdd8b5b5e0 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -56,43 +56,6 @@
> >  #include "intel_drv.h"
> >  #include "i915_drv.h"
> >  
> > -static inline enum intel_display_power_domain
> > -psr_aux_domain(struct intel_dp *intel_dp)
> > -{
> > -	/* CNL HW requires corresponding AUX IOs to be powered up
> > for PSR.
> > -	 * However, for non-A AUX ports the corresponding non-EDP
> > transcoders
> > -	 * would have already enabled power well 2 and DC_OFF. This
> > means we can
> > -	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> > instead of a
> > -	 * specific AUX_IO reference without powering up any extra
> > wells.
> > -	 * Note that PSR is enabled only on Port A even though this
> > function
> > -	 * returns the correct domain for other ports too.
> > -	 */
> > -	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A
> > :
> > -					      intel_dp-
> > >aux_power_domain;
> > -}
> > -
> > -static void psr_aux_io_power_get(struct intel_dp *intel_dp)
> > -{
> > -	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > -	struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
> > >base.base.dev);
> > -
> > -	if (INTEL_GEN(dev_priv) < 10)
> > -		return;
> > -
> > -	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> > -}
> > -
> > -static void psr_aux_io_power_put(struct intel_dp *intel_dp)
> > -{
> > -	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > -	struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
> > >base.base.dev);
> > -
> > -	if (INTEL_GEN(dev_priv) < 10)
> > -		return;
> > -
> > -	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> > -}
> > -
> >  void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool
> > debug)
> >  {
> >  	u32 debug_mask, mask;
> > @@ -595,8 +558,6 @@ static void hsw_psr_enable_source(struct intel_dp
> > *intel_dp,
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> >  
> > -	psr_aux_io_power_get(intel_dp);
> > -
> >  	/* Only HSW and BDW have PSR AUX registers that need to be
> > setup. SKL+
> >  	 * use hardcoded values PSR AUX transactions
> >  	 */
> > @@ -717,8 +678,6 @@ static void hsw_psr_disable(struct intel_dp
> > *intel_dp,
> >  		else
> >  			WARN_ON(I915_READ(EDP_PSR_CTL) &
> > EDP_PSR_ENABLE);
> >  	}
> > -
> > -	psr_aux_io_power_put(intel_dp);
> >  }
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index de3a81034f77..2969787201ef 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -1824,6 +1824,7 @@ void intel_display_power_put(struct
> > drm_i915_private *dev_priv,
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  #define GLK_DISPLAY_AUX_A_POWER_DOMAINS (		\
> >  	BIT_ULL(POWER_DOMAIN_AUX_A) |		\
> > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  #define GLK_DISPLAY_AUX_B_POWER_DOMAINS (		\
> >  	BIT_ULL(POWER_DOMAIN_AUX_B) |		\
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/ddi: Get AUX power domain for DP main link too

[Imre Deak]

On Thu, Jun 21, 2018 at 01:28:40PM -0700, Dhinakaran Pandiyan wrote:
> On Thu, 2018-06-21 at 22:54 +0300, Imre Deak wrote:
> > On Thu, Jun 21, 2018 at 01:14:30PM -0700, Dhinakaran Pandiyan wrote:
> > > 
> > > On Thu, 2018-06-21 at 21:44 +0300, Imre Deak wrote:
> > > > 
> > > > So far we got an AUX power domain reference only for the duration
> > > > of
> > > > DP
> > > > AUX transfers. However, the following suggests that we also need
> > > > these
> > > > for main link functionality:
> > > > - The specification doesn't state whether it's needed or not for
> > > > main
> > > >   link functionality, but suggests that these power wells need to
> > > > be
> > > >   enabled already during display core initialization (Sequences
> > > > to
> > > >   Initialize Display).
> > > Hmm. The sequence also says "If an Aux channel will not be used, it
> > > does not need to be powered up."
> > Well, I consider that hints at not using the port at all for DP,
> > since
> > for DP you will need AUX.
> > 
> 
> I also see an instruction to "set PORT_CL_DW12_<port letter> Lane
> Enable Aux to 1b" for aux channels on combo ports. A quick check shows
> the register is not defined in the driver. My concern is we might be
> missing a step for combo ports.

It is added in the ICL power well enabling patch (see internal branch),
and is something I tested the above scenario with.

> 
> 
> > > 
> > > 
> > > > 
> > > > - For PSR we need to keep the AUX power well enabled.
> > > > - On ICL combo PHY ports (non-TC) the AUX power well is needed
> > > > for
> > > >   link training too: while the port is enabled with a DP link
> > > > training
> > > I don't see this in the spec, is this something you observed?
> > Yes.
> > 
> > > 
> > > 
> > > > 
> > > >   test pattern trying to toggle the AUX power well will time out.
> > > Can we enable AUX power well before link training starts and
> > > disable
> > > after we are done training? Is that enough?
> > That was my first idea too, but given the rest of the points I
> > consider
> > it a safer option to enable it for main link functionality.
> > 
> > > 
> > > 
> > > > 
> > > > - On ICL MG PHY ports (TC) the AUX power well is needed also for
> > > > main
> > > >   link functionality (both in DP and HDMI modes).
> > > > - Windows enables these power wells both for main and AUX lane
> > > >   functionality.
> > > > 
> > > > Based on the above take an AUX power reference for main link
> > > > functionality too. This makes a difference only on GEN10+ (GLK+)
> > > > platforms, where we have separate port specific AUX power wells.
> > > > 
> > > > For PSR we still need to distinguish between port A and the other
> > > > ports, since on port A DC states must stay enabled for main link
> > > > functionality, but DC states must be disabled for driver
> > > > initiated
> > > > AUX transfers. So re-use the corresponding helper from
> > > > intel_psr.c.
> > > > 
> > > > Since we take now a reference for main link functionality on all
> > > > DP
> > > > ports we can forgo taking the separate power ref for PSR
> > > > functionality.
> > > > 
> > > > v2:
> > > > - Make sure DC states stay enabled when taking the ref on port A.
> > > >   (Ville)
> > > > 
> > > > v3: (Ville)
> > > > - Fix comment about logic for encoders without a crtc state and
> > > >   add FIXME note for a simplification to avoid calling
> > > > get_power_domains
> > > >   in such cases.
> > > > - Use intel_crtc_has_dp_encoder() instead
> > > > !intel_crtc_has_type(HDMI).
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_ddi.c        | 54
> > > > ++++++++++++++++++++++++++++++---
> > > >  drivers/gpu/drm/i915/intel_display.c    | 12 +++++++-
> > > >  drivers/gpu/drm/i915/intel_drv.h        |  3 +-
> > > >  drivers/gpu/drm/i915/intel_psr.c        | 41 -----------------
> > > > ----
> > > > ----
> > > >  drivers/gpu/drm/i915/intel_runtime_pm.c |  1 +
> > > >  5 files changed, 64 insertions(+), 47 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index 044fe1fb9872..0319825b725b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -1983,15 +1983,55 @@ bool intel_ddi_get_hw_state(struct
> > > > intel_encoder *encoder,
> > > >  	return ret;
> > > >  }
> > > >  
> > > > -static u64 intel_ddi_get_power_domains(struct intel_encoder
> > > > *encoder)
> > > > +static inline enum intel_display_power_domain
> > > > +intel_ddi_main_link_aux_domain(struct intel_dp *intel_dp)
> > > > +{
> > > > +	/* CNL HW requires corresponding AUX IOs to be powered
> > > > up
> > > > for PSR with
> > > > +	 * DC states enabled at the same time, while for driver
> > > > initiated AUX
> > > > +	 * transfers we need the same AUX IOs to be powered but
> > > > with
> > > > DC states
> > > > +	 * disabled. Accordingly use the AUX power domain here
> > > > which
> > > > leaves DC
> > > > +	 * states enabled.
> > > > +	 * However, for non-A AUX ports the corresponding non-
> > > > EDP
> > > > transcoders
> > > > +	 * would have already enabled power well 2 and DC_OFF.
> > > > This
> > > > means we can
> > > > +	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> > > > instead of a
> > > > +	 * specific AUX_IO reference without powering up any
> > > > extra
> > > > wells.
> > > > +	 * Note that PSR is enabled only on Port A even though
> > > > this
> > > > function
> > > > +	 * returns the correct domain for other ports too.
> > > > +	 */
> > > > +	return intel_dp->aux_ch == AUX_CH_A ?
> > > > POWER_DOMAIN_AUX_IO_A
> > > > :
> > > > +					      intel_dp-
> > > > > 
> > > > > aux_power_domain;
> > > > +}
> > > > +
> > > > +static u64 intel_ddi_get_power_domains(struct intel_encoder
> > > > *encoder,
> > > > +				       struct intel_crtc_state
> > > > *crtc_state)
> > > >  {
> > > >  	struct intel_digital_port *dig_port =
> > > > enc_to_dig_port(&encoder->base);
> > > >  	enum pipe pipe;
> > > > +	u64 domains;
> > > >  
> > > > -	if (intel_ddi_get_hw_state(encoder, &pipe))
> > > > -		return BIT_ULL(dig_port->ddi_io_power_domain);
> > > > +	if (!intel_ddi_get_hw_state(encoder, &pipe))
> > > > +		return 0;
> > > >  
> > > > -	return 0;
> > > > +	domains = BIT_ULL(dig_port->ddi_io_power_domain);
> > > > +	if (!crtc_state)
> > > > +		return domains;
> > > > +
> > > > +	/*
> > > > +	 * TODO: Add support for MST encoders. Atm, the
> > > > following
> > > > should never
> > > > +	 * happen since this function will be called only for
> > > > the
> > > > primary
> > > > +	 * encoder which doesn't have its own pipe/crtc_state.
> > > > +	 */
> > > > +	if (WARN_ON(intel_crtc_has_type(crtc_state,
> > > > INTEL_OUTPUT_DP_MST)))
> > > > +		return domains;
> > > > +
> > > > +	/* AUX power is only needed for (e)DP mode, not for
> > > > HDMI. */
> > > > +	if (intel_crtc_has_dp_encoder(crtc_state)) {
> > > > +		struct intel_dp *intel_dp = &dig_port->dp;
> > > > +
> > > > +		domains |=
> > > > BIT_ULL(intel_ddi_main_link_aux_domain(intel_dp));
> > > > +	}
> > > > +
> > > > +	return domains;
> > > >  }
> > > >  
> > > >  void intel_ddi_enable_pipe_clock(const struct intel_crtc_state
> > > > *crtc_state)
> > > > @@ -2631,6 +2671,9 @@ static void intel_ddi_pre_enable_dp(struct
> > > > intel_encoder *encoder,
> > > >  
> > > >  	WARN_ON(is_mst && (port == PORT_A || port == PORT_E));
> > > >  
> > > > +	intel_display_power_get(dev_priv,
> > > > +				intel_ddi_main_link_aux_domain(i
> > > > ntel
> > > > _dp));
> > > > +
> > > >  	intel_dp_set_link_params(intel_dp, crtc_state-
> > > > >port_clock,
> > > >  				 crtc_state->lane_count,
> > > > is_mst);
> > > >  
> > > > @@ -2775,6 +2818,9 @@ static void
> > > > intel_ddi_post_disable_dp(struct
> > > > intel_encoder *encoder,
> > > >  	intel_display_power_put(dev_priv, dig_port-
> > > > > 
> > > > > ddi_io_power_domain);
> > > >  
> > > >  	intel_ddi_clk_disable(encoder);
> > > > +
> > > > +	intel_display_power_put(dev_priv,
> > > > +				intel_ddi_main_link_aux_domain(i
> > > > ntel
> > > > _dp));
> > > >  }
> > > >  
> > > >  static void intel_ddi_post_disable_hdmi(struct intel_encoder
> > > > *encoder,
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index 2c8fef3ede54..ce615f89b43a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct
> > > > drm_i915_private *dev_priv)
> > > >  	for_each_intel_encoder(&dev_priv->drm, encoder) {
> > > >  		u64 get_domains;
> > > >  		enum intel_display_power_domain domain;
> > > > +		struct intel_crtc_state *crtc_state;
> > > >  
> > > >  		if (!encoder->get_power_domains)
> > > >  			continue;
> > > >  
> > > > -		get_domains = encoder-
> > > > >get_power_domains(encoder);
> > > > +		/*
> > > > +		 * For MST and inactive encoders we don't have a
> > > > crtc state.
> > > > +		 * FIXME: no need to call get_power_domains in
> > > > such
> > > > cases, it
> > > > +		 * will always return 0.
> > > > +		 */
> > > > +		crtc_state = encoder->base.crtc ?
> > > > +			     to_intel_crtc_state(encoder-
> > > > >base.crtc-
> > > > > 
> > > > > state) :
> > > > +			     NULL;
> > > > +
> > > > +		get_domains = encoder-
> > > > >get_power_domains(encoder,
> > > > crtc_state);
> > > >  		for_each_power_domain(domain, get_domains)
> > > >  			intel_display_power_get(dev_priv,
> > > > domain);
> > > >  	}
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 0c3ac0eafde0..7174429d930a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -254,7 +254,8 @@ struct intel_encoder {
> > > >  			   struct intel_crtc_state
> > > > *pipe_config);
> > > >  	/* Returns a mask of power domains that need to be
> > > > referenced as part
> > > >  	 * of the hardware state readout code. */
> > > > -	u64 (*get_power_domains)(struct intel_encoder *encoder);
> > > > +	u64 (*get_power_domains)(struct intel_encoder *encoder,
> > > > +				 struct intel_crtc_state
> > > > *crtc_state);
> > > >  	/*
> > > >  	 * Called during system suspend after all pending
> > > > requests
> > > > for the
> > > >  	 * encoder are flushed (for example for DP AUX
> > > > transactions)
> > > > and
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index d4cd19fea148..eecdd8b5b5e0 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -56,43 +56,6 @@
> > > >  #include "intel_drv.h"
> > > >  #include "i915_drv.h"
> > > >  
> > > > -static inline enum intel_display_power_domain
> > > > -psr_aux_domain(struct intel_dp *intel_dp)
> > > > -{
> > > > -	/* CNL HW requires corresponding AUX IOs to be powered
> > > > up
> > > > for PSR.
> > > > -	 * However, for non-A AUX ports the corresponding non-
> > > > EDP
> > > > transcoders
> > > > -	 * would have already enabled power well 2 and DC_OFF.
> > > > This
> > > > means we can
> > > > -	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> > > > instead of a
> > > > -	 * specific AUX_IO reference without powering up any
> > > > extra
> > > > wells.
> > > > -	 * Note that PSR is enabled only on Port A even though
> > > > this
> > > > function
> > > > -	 * returns the correct domain for other ports too.
> > > > -	 */
> > > > -	return intel_dp->aux_ch == AUX_CH_A ?
> > > > POWER_DOMAIN_AUX_IO_A
> > > > :
> > > > -					      intel_dp-
> > > > > 
> > > > > aux_power_domain;
> > > > -}
> > > > -
> > > > -static void psr_aux_io_power_get(struct intel_dp *intel_dp)
> > > > -{
> > > > -	struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > -	struct drm_i915_private *dev_priv =
> > > > to_i915(intel_dig_port-
> > > > > 
> > > > > base.base.dev);
> > > > -
> > > > -	if (INTEL_GEN(dev_priv) < 10)
> > > > -		return;
> > > > -
> > > > -	intel_display_power_get(dev_priv,
> > > > psr_aux_domain(intel_dp));
> > > > -}
> > > > -
> > > > -static void psr_aux_io_power_put(struct intel_dp *intel_dp)
> > > > -{
> > > > -	struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > -	struct drm_i915_private *dev_priv =
> > > > to_i915(intel_dig_port-
> > > > > 
> > > > > base.base.dev);
> > > > -
> > > > -	if (INTEL_GEN(dev_priv) < 10)
> > > > -		return;
> > > > -
> > > > -	intel_display_power_put(dev_priv,
> > > > psr_aux_domain(intel_dp));
> > > > -}
> > > > -
> > > >  void intel_psr_irq_control(struct drm_i915_private *dev_priv,
> > > > bool
> > > > debug)
> > > >  {
> > > >  	u32 debug_mask, mask;
> > > > @@ -595,8 +558,6 @@ static void hsw_psr_enable_source(struct
> > > > intel_dp
> > > > *intel_dp,
> > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > >  	enum transcoder cpu_transcoder = crtc_state-
> > > > >cpu_transcoder;
> > > >  
> > > > -	psr_aux_io_power_get(intel_dp);
> > > > -
> > > >  	/* Only HSW and BDW have PSR AUX registers that need to
> > > > be
> > > > setup. SKL+
> > > >  	 * use hardcoded values PSR AUX transactions
> > > >  	 */
> > > > @@ -717,8 +678,6 @@ static void hsw_psr_disable(struct intel_dp
> > > > *intel_dp,
> > > >  		else
> > > >  			WARN_ON(I915_READ(EDP_PSR_CTL) &
> > > > EDP_PSR_ENABLE);
> > > >  	}
> > > > -
> > > > -	psr_aux_io_power_put(intel_dp);
> > > >  }
> > > >  
> > > >  /**
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > index de3a81034f77..2969787201ef 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > @@ -1824,6 +1824,7 @@ void intel_display_power_put(struct
> > > > drm_i915_private *dev_priv,
> > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > >  #define GLK_DISPLAY_AUX_A_POWER_DOMAINS (		\
> > > >  	BIT_ULL(POWER_DOMAIN_AUX_A) |		\
> > > > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > >  #define GLK_DISPLAY_AUX_B_POWER_DOMAINS (		\
> > > >  	BIT_ULL(POWER_DOMAIN_AUX_B) |		\
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/ddi: Get AUX power domain for DP main link too

[Dhinakaran Pandiyan]

On Thu, 2018-06-21 at 21:44 +0300, Imre Deak wrote:
> So far we got an AUX power domain reference only for the duration of
> DP
> AUX transfers. However, the following suggests that we also need
> these
> for main link functionality:
> - The specification doesn't state whether it's needed or not for main
>   link functionality, but suggests that these power wells need to be
>   enabled already during display core initialization (Sequences to
>   Initialize Display).
Hmm. The sequence also says "If an Aux channel will not be used, it
does not need to be powered up."

> - For PSR we need to keep the AUX power well enabled.
> - On ICL combo PHY ports (non-TC) the AUX power well is needed for
>   link training too: while the port is enabled with a DP link
> training
I don't see this in the spec, is this something you observed?

>   test pattern trying to toggle the AUX power well will time out.

Can we enable AUX power well before link training starts and disable
after we are done training? Is that enough?

> - On ICL MG PHY ports (TC) the AUX power well is needed also for main
>   link functionality (both in DP and HDMI modes).
> - Windows enables these power wells both for main and AUX lane
>   functionality.
> 
> Based on the above take an AUX power reference for main link
> functionality too. This makes a difference only on GEN10+ (GLK+)
> platforms, where we have separate port specific AUX power wells.
> 
> For PSR we still need to distinguish between port A and the other
> ports, since on port A DC states must stay enabled for main link
> functionality, but DC states must be disabled for driver initiated
> AUX transfers. So re-use the corresponding helper from intel_psr.c.
> 
> Since we take now a reference for main link functionality on all DP
> ports we can forgo taking the separate power ref for PSR
> functionality.
> 
> v2:
> - Make sure DC states stay enabled when taking the ref on port A.
>   (Ville)
> 
> v3: (Ville)
> - Fix comment about logic for encoders without a crtc state and
>   add FIXME note for a simplification to avoid calling
> get_power_domains
>   in such cases.
> - Use intel_crtc_has_dp_encoder() instead !intel_crtc_has_type(HDMI).
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c        | 54
> ++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_display.c    | 12 +++++++-
>  drivers/gpu/drm/i915/intel_drv.h        |  3 +-
>  drivers/gpu/drm/i915/intel_psr.c        | 41 ---------------------
> ----
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  1 +
>  5 files changed, 64 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 044fe1fb9872..0319825b725b 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1983,15 +1983,55 @@ bool intel_ddi_get_hw_state(struct
> intel_encoder *encoder,
>  	return ret;
>  }
>  
> -static u64 intel_ddi_get_power_domains(struct intel_encoder
> *encoder)
> +static inline enum intel_display_power_domain
> +intel_ddi_main_link_aux_domain(struct intel_dp *intel_dp)
> +{
> +	/* CNL HW requires corresponding AUX IOs to be powered up
> for PSR with
> +	 * DC states enabled at the same time, while for driver
> initiated AUX
> +	 * transfers we need the same AUX IOs to be powered but with
> DC states
> +	 * disabled. Accordingly use the AUX power domain here which
> leaves DC
> +	 * states enabled.
> +	 * However, for non-A AUX ports the corresponding non-EDP
> transcoders
> +	 * would have already enabled power well 2 and DC_OFF. This
> means we can
> +	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> instead of a
> +	 * specific AUX_IO reference without powering up any extra
> wells.
> +	 * Note that PSR is enabled only on Port A even though this
> function
> +	 * returns the correct domain for other ports too.
> +	 */
> +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A
> :
> +					      intel_dp-
> >aux_power_domain;
> +}
> +
> +static u64 intel_ddi_get_power_domains(struct intel_encoder
> *encoder,
> +				       struct intel_crtc_state
> *crtc_state)
>  {
>  	struct intel_digital_port *dig_port =
> enc_to_dig_port(&encoder->base);
>  	enum pipe pipe;
> +	u64 domains;
>  
> -	if (intel_ddi_get_hw_state(encoder, &pipe))
> -		return BIT_ULL(dig_port->ddi_io_power_domain);
> +	if (!intel_ddi_get_hw_state(encoder, &pipe))
> +		return 0;
>  
> -	return 0;
> +	domains = BIT_ULL(dig_port->ddi_io_power_domain);
> +	if (!crtc_state)
> +		return domains;
> +
> +	/*
> +	 * TODO: Add support for MST encoders. Atm, the following
> should never
> +	 * happen since this function will be called only for the
> primary
> +	 * encoder which doesn't have its own pipe/crtc_state.
> +	 */
> +	if (WARN_ON(intel_crtc_has_type(crtc_state,
> INTEL_OUTPUT_DP_MST)))
> +		return domains;
> +
> +	/* AUX power is only needed for (e)DP mode, not for HDMI. */
> +	if (intel_crtc_has_dp_encoder(crtc_state)) {
> +		struct intel_dp *intel_dp = &dig_port->dp;
> +
> +		domains |=
> BIT_ULL(intel_ddi_main_link_aux_domain(intel_dp));
> +	}
> +
> +	return domains;
>  }
>  
>  void intel_ddi_enable_pipe_clock(const struct intel_crtc_state
> *crtc_state)
> @@ -2631,6 +2671,9 @@ static void intel_ddi_pre_enable_dp(struct
> intel_encoder *encoder,
>  
>  	WARN_ON(is_mst && (port == PORT_A || port == PORT_E));
>  
> +	intel_display_power_get(dev_priv,
> +				intel_ddi_main_link_aux_domain(intel
> _dp));
> +
>  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
>  				 crtc_state->lane_count, is_mst);
>  
> @@ -2775,6 +2818,9 @@ static void intel_ddi_post_disable_dp(struct
> intel_encoder *encoder,
>  	intel_display_power_put(dev_priv, dig_port-
> >ddi_io_power_domain);
>  
>  	intel_ddi_clk_disable(encoder);
> +
> +	intel_display_power_put(dev_priv,
> +				intel_ddi_main_link_aux_domain(intel
> _dp));
>  }
>  
>  static void intel_ddi_post_disable_hdmi(struct intel_encoder
> *encoder,
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 2c8fef3ede54..ce615f89b43a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct
> drm_i915_private *dev_priv)
>  	for_each_intel_encoder(&dev_priv->drm, encoder) {
>  		u64 get_domains;
>  		enum intel_display_power_domain domain;
> +		struct intel_crtc_state *crtc_state;
>  
>  		if (!encoder->get_power_domains)
>  			continue;
>  
> -		get_domains = encoder->get_power_domains(encoder);
> +		/*
> +		 * For MST and inactive encoders we don't have a
> crtc state.
> +		 * FIXME: no need to call get_power_domains in such
> cases, it
> +		 * will always return 0.
> +		 */
> +		crtc_state = encoder->base.crtc ?
> +			     to_intel_crtc_state(encoder->base.crtc-
> >state) :
> +			     NULL;
> +
> +		get_domains = encoder->get_power_domains(encoder,
> crtc_state);
>  		for_each_power_domain(domain, get_domains)
>  			intel_display_power_get(dev_priv, domain);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 0c3ac0eafde0..7174429d930a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -254,7 +254,8 @@ struct intel_encoder {
>  			   struct intel_crtc_state *pipe_config);
>  	/* Returns a mask of power domains that need to be
> referenced as part
>  	 * of the hardware state readout code. */
> -	u64 (*get_power_domains)(struct intel_encoder *encoder);
> +	u64 (*get_power_domains)(struct intel_encoder *encoder,
> +				 struct intel_crtc_state
> *crtc_state);
>  	/*
>  	 * Called during system suspend after all pending requests
> for the
>  	 * encoder are flushed (for example for DP AUX transactions)
> and
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index d4cd19fea148..eecdd8b5b5e0 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -56,43 +56,6 @@
>  #include "intel_drv.h"
>  #include "i915_drv.h"
>  
> -static inline enum intel_display_power_domain
> -psr_aux_domain(struct intel_dp *intel_dp)
> -{
> -	/* CNL HW requires corresponding AUX IOs to be powered up
> for PSR.
> -	 * However, for non-A AUX ports the corresponding non-EDP
> transcoders
> -	 * would have already enabled power well 2 and DC_OFF. This
> means we can
> -	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> instead of a
> -	 * specific AUX_IO reference without powering up any extra
> wells.
> -	 * Note that PSR is enabled only on Port A even though this
> function
> -	 * returns the correct domain for other ports too.
> -	 */
> -	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A
> :
> -					      intel_dp-
> >aux_power_domain;
> -}
> -
> -static void psr_aux_io_power_get(struct intel_dp *intel_dp)
> -{
> -	struct intel_digital_port *intel_dig_port =
> dp_to_dig_port(intel_dp);
> -	struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
> >base.base.dev);
> -
> -	if (INTEL_GEN(dev_priv) < 10)
> -		return;
> -
> -	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> -}
> -
> -static void psr_aux_io_power_put(struct intel_dp *intel_dp)
> -{
> -	struct intel_digital_port *intel_dig_port =
> dp_to_dig_port(intel_dp);
> -	struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
> >base.base.dev);
> -
> -	if (INTEL_GEN(dev_priv) < 10)
> -		return;
> -
> -	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> -}
> -
>  void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool
> debug)
>  {
>  	u32 debug_mask, mask;
> @@ -595,8 +558,6 @@ static void hsw_psr_enable_source(struct intel_dp
> *intel_dp,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>  
> -	psr_aux_io_power_get(intel_dp);
> -
>  	/* Only HSW and BDW have PSR AUX registers that need to be
> setup. SKL+
>  	 * use hardcoded values PSR AUX transactions
>  	 */
> @@ -717,8 +678,6 @@ static void hsw_psr_disable(struct intel_dp
> *intel_dp,
>  		else
>  			WARN_ON(I915_READ(EDP_PSR_CTL) &
> EDP_PSR_ENABLE);
>  	}
> -
> -	psr_aux_io_power_put(intel_dp);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index de3a81034f77..2969787201ef 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1824,6 +1824,7 @@ void intel_display_power_put(struct
> drm_i915_private *dev_priv,
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  #define GLK_DISPLAY_AUX_A_POWER_DOMAINS (		\
>  	BIT_ULL(POWER_DOMAIN_AUX_A) |		\
> +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  #define GLK_DISPLAY_AUX_B_POWER_DOMAINS (		\
>  	BIT_ULL(POWER_DOMAIN_AUX_B) |		\
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/ddi: Get AUX power domain for DP main link too

[Dhinakaran Pandiyan]

On Thu, 2018-06-21 at 22:54 +0300, Imre Deak wrote:
> On Thu, Jun 21, 2018 at 01:14:30PM -0700, Dhinakaran Pandiyan wrote:
> > 
> > On Thu, 2018-06-21 at 21:44 +0300, Imre Deak wrote:
> > > 
> > > So far we got an AUX power domain reference only for the duration
> > > of
> > > DP
> > > AUX transfers. However, the following suggests that we also need
> > > these
> > > for main link functionality:
> > > - The specification doesn't state whether it's needed or not for
> > > main
> > >   link functionality, but suggests that these power wells need to
> > > be
> > >   enabled already during display core initialization (Sequences
> > > to
> > >   Initialize Display).
> > Hmm. The sequence also says "If an Aux channel will not be used, it
> > does not need to be powered up."
> Well, I consider that hints at not using the port at all for DP,
> since
> for DP you will need AUX.
> 

I also see an instruction to "set PORT_CL_DW12_<port letter> Lane
Enable Aux to 1b" for aux channels on combo ports. A quick check shows
the register is not defined in the driver. My concern is we might be
missing a step for combo ports.


> > 
> > 
> > > 
> > > - For PSR we need to keep the AUX power well enabled.
> > > - On ICL combo PHY ports (non-TC) the AUX power well is needed
> > > for
> > >   link training too: while the port is enabled with a DP link
> > > training
> > I don't see this in the spec, is this something you observed?
> Yes.
> 
> > 
> > 
> > > 
> > >   test pattern trying to toggle the AUX power well will time out.
> > Can we enable AUX power well before link training starts and
> > disable
> > after we are done training? Is that enough?
> That was my first idea too, but given the rest of the points I
> consider
> it a safer option to enable it for main link functionality.
> 
> > 
> > 
> > > 
> > > - On ICL MG PHY ports (TC) the AUX power well is needed also for
> > > main
> > >   link functionality (both in DP and HDMI modes).
> > > - Windows enables these power wells both for main and AUX lane
> > >   functionality.
> > > 
> > > Based on the above take an AUX power reference for main link
> > > functionality too. This makes a difference only on GEN10+ (GLK+)
> > > platforms, where we have separate port specific AUX power wells.
> > > 
> > > For PSR we still need to distinguish between port A and the other
> > > ports, since on port A DC states must stay enabled for main link
> > > functionality, but DC states must be disabled for driver
> > > initiated
> > > AUX transfers. So re-use the corresponding helper from
> > > intel_psr.c.
> > > 
> > > Since we take now a reference for main link functionality on all
> > > DP
> > > ports we can forgo taking the separate power ref for PSR
> > > functionality.
> > > 
> > > v2:
> > > - Make sure DC states stay enabled when taking the ref on port A.
> > >   (Ville)
> > > 
> > > v3: (Ville)
> > > - Fix comment about logic for encoders without a crtc state and
> > >   add FIXME note for a simplification to avoid calling
> > > get_power_domains
> > >   in such cases.
> > > - Use intel_crtc_has_dp_encoder() instead
> > > !intel_crtc_has_type(HDMI).
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c        | 54
> > > ++++++++++++++++++++++++++++++---
> > >  drivers/gpu/drm/i915/intel_display.c    | 12 +++++++-
> > >  drivers/gpu/drm/i915/intel_drv.h        |  3 +-
> > >  drivers/gpu/drm/i915/intel_psr.c        | 41 -----------------
> > > ----
> > > ----
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c |  1 +
> > >  5 files changed, 64 insertions(+), 47 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 044fe1fb9872..0319825b725b 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -1983,15 +1983,55 @@ bool intel_ddi_get_hw_state(struct
> > > intel_encoder *encoder,
> > >  	return ret;
> > >  }
> > >  
> > > -static u64 intel_ddi_get_power_domains(struct intel_encoder
> > > *encoder)
> > > +static inline enum intel_display_power_domain
> > > +intel_ddi_main_link_aux_domain(struct intel_dp *intel_dp)
> > > +{
> > > +	/* CNL HW requires corresponding AUX IOs to be powered
> > > up
> > > for PSR with
> > > +	 * DC states enabled at the same time, while for driver
> > > initiated AUX
> > > +	 * transfers we need the same AUX IOs to be powered but
> > > with
> > > DC states
> > > +	 * disabled. Accordingly use the AUX power domain here
> > > which
> > > leaves DC
> > > +	 * states enabled.
> > > +	 * However, for non-A AUX ports the corresponding non-
> > > EDP
> > > transcoders
> > > +	 * would have already enabled power well 2 and DC_OFF.
> > > This
> > > means we can
> > > +	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> > > instead of a
> > > +	 * specific AUX_IO reference without powering up any
> > > extra
> > > wells.
> > > +	 * Note that PSR is enabled only on Port A even though
> > > this
> > > function
> > > +	 * returns the correct domain for other ports too.
> > > +	 */
> > > +	return intel_dp->aux_ch == AUX_CH_A ?
> > > POWER_DOMAIN_AUX_IO_A
> > > :
> > > +					      intel_dp-
> > > > 
> > > > aux_power_domain;
> > > +}
> > > +
> > > +static u64 intel_ddi_get_power_domains(struct intel_encoder
> > > *encoder,
> > > +				       struct intel_crtc_state
> > > *crtc_state)
> > >  {
> > >  	struct intel_digital_port *dig_port =
> > > enc_to_dig_port(&encoder->base);
> > >  	enum pipe pipe;
> > > +	u64 domains;
> > >  
> > > -	if (intel_ddi_get_hw_state(encoder, &pipe))
> > > -		return BIT_ULL(dig_port->ddi_io_power_domain);
> > > +	if (!intel_ddi_get_hw_state(encoder, &pipe))
> > > +		return 0;
> > >  
> > > -	return 0;
> > > +	domains = BIT_ULL(dig_port->ddi_io_power_domain);
> > > +	if (!crtc_state)
> > > +		return domains;
> > > +
> > > +	/*
> > > +	 * TODO: Add support for MST encoders. Atm, the
> > > following
> > > should never
> > > +	 * happen since this function will be called only for
> > > the
> > > primary
> > > +	 * encoder which doesn't have its own pipe/crtc_state.
> > > +	 */
> > > +	if (WARN_ON(intel_crtc_has_type(crtc_state,
> > > INTEL_OUTPUT_DP_MST)))
> > > +		return domains;
> > > +
> > > +	/* AUX power is only needed for (e)DP mode, not for
> > > HDMI. */
> > > +	if (intel_crtc_has_dp_encoder(crtc_state)) {
> > > +		struct intel_dp *intel_dp = &dig_port->dp;
> > > +
> > > +		domains |=
> > > BIT_ULL(intel_ddi_main_link_aux_domain(intel_dp));
> > > +	}
> > > +
> > > +	return domains;
> > >  }
> > >  
> > >  void intel_ddi_enable_pipe_clock(const struct intel_crtc_state
> > > *crtc_state)
> > > @@ -2631,6 +2671,9 @@ static void intel_ddi_pre_enable_dp(struct
> > > intel_encoder *encoder,
> > >  
> > >  	WARN_ON(is_mst && (port == PORT_A || port == PORT_E));
> > >  
> > > +	intel_display_power_get(dev_priv,
> > > +				intel_ddi_main_link_aux_domain(i
> > > ntel
> > > _dp));
> > > +
> > >  	intel_dp_set_link_params(intel_dp, crtc_state-
> > > >port_clock,
> > >  				 crtc_state->lane_count,
> > > is_mst);
> > >  
> > > @@ -2775,6 +2818,9 @@ static void
> > > intel_ddi_post_disable_dp(struct
> > > intel_encoder *encoder,
> > >  	intel_display_power_put(dev_priv, dig_port-
> > > > 
> > > > ddi_io_power_domain);
> > >  
> > >  	intel_ddi_clk_disable(encoder);
> > > +
> > > +	intel_display_power_put(dev_priv,
> > > +				intel_ddi_main_link_aux_domain(i
> > > ntel
> > > _dp));
> > >  }
> > >  
> > >  static void intel_ddi_post_disable_hdmi(struct intel_encoder
> > > *encoder,
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 2c8fef3ede54..ce615f89b43a 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct
> > > drm_i915_private *dev_priv)
> > >  	for_each_intel_encoder(&dev_priv->drm, encoder) {
> > >  		u64 get_domains;
> > >  		enum intel_display_power_domain domain;
> > > +		struct intel_crtc_state *crtc_state;
> > >  
> > >  		if (!encoder->get_power_domains)
> > >  			continue;
> > >  
> > > -		get_domains = encoder-
> > > >get_power_domains(encoder);
> > > +		/*
> > > +		 * For MST and inactive encoders we don't have a
> > > crtc state.
> > > +		 * FIXME: no need to call get_power_domains in
> > > such
> > > cases, it
> > > +		 * will always return 0.
> > > +		 */
> > > +		crtc_state = encoder->base.crtc ?
> > > +			     to_intel_crtc_state(encoder-
> > > >base.crtc-
> > > > 
> > > > state) :
> > > +			     NULL;
> > > +
> > > +		get_domains = encoder-
> > > >get_power_domains(encoder,
> > > crtc_state);
> > >  		for_each_power_domain(domain, get_domains)
> > >  			intel_display_power_get(dev_priv,
> > > domain);
> > >  	}
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 0c3ac0eafde0..7174429d930a 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -254,7 +254,8 @@ struct intel_encoder {
> > >  			   struct intel_crtc_state
> > > *pipe_config);
> > >  	/* Returns a mask of power domains that need to be
> > > referenced as part
> > >  	 * of the hardware state readout code. */
> > > -	u64 (*get_power_domains)(struct intel_encoder *encoder);
> > > +	u64 (*get_power_domains)(struct intel_encoder *encoder,
> > > +				 struct intel_crtc_state
> > > *crtc_state);
> > >  	/*
> > >  	 * Called during system suspend after all pending
> > > requests
> > > for the
> > >  	 * encoder are flushed (for example for DP AUX
> > > transactions)
> > > and
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index d4cd19fea148..eecdd8b5b5e0 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -56,43 +56,6 @@
> > >  #include "intel_drv.h"
> > >  #include "i915_drv.h"
> > >  
> > > -static inline enum intel_display_power_domain
> > > -psr_aux_domain(struct intel_dp *intel_dp)
> > > -{
> > > -	/* CNL HW requires corresponding AUX IOs to be powered
> > > up
> > > for PSR.
> > > -	 * However, for non-A AUX ports the corresponding non-
> > > EDP
> > > transcoders
> > > -	 * would have already enabled power well 2 and DC_OFF.
> > > This
> > > means we can
> > > -	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> > > instead of a
> > > -	 * specific AUX_IO reference without powering up any
> > > extra
> > > wells.
> > > -	 * Note that PSR is enabled only on Port A even though
> > > this
> > > function
> > > -	 * returns the correct domain for other ports too.
> > > -	 */
> > > -	return intel_dp->aux_ch == AUX_CH_A ?
> > > POWER_DOMAIN_AUX_IO_A
> > > :
> > > -					      intel_dp-
> > > > 
> > > > aux_power_domain;
> > > -}
> > > -
> > > -static void psr_aux_io_power_get(struct intel_dp *intel_dp)
> > > -{
> > > -	struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > -	struct drm_i915_private *dev_priv =
> > > to_i915(intel_dig_port-
> > > > 
> > > > base.base.dev);
> > > -
> > > -	if (INTEL_GEN(dev_priv) < 10)
> > > -		return;
> > > -
> > > -	intel_display_power_get(dev_priv,
> > > psr_aux_domain(intel_dp));
> > > -}
> > > -
> > > -static void psr_aux_io_power_put(struct intel_dp *intel_dp)
> > > -{
> > > -	struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > -	struct drm_i915_private *dev_priv =
> > > to_i915(intel_dig_port-
> > > > 
> > > > base.base.dev);
> > > -
> > > -	if (INTEL_GEN(dev_priv) < 10)
> > > -		return;
> > > -
> > > -	intel_display_power_put(dev_priv,
> > > psr_aux_domain(intel_dp));
> > > -}
> > > -
> > >  void intel_psr_irq_control(struct drm_i915_private *dev_priv,
> > > bool
> > > debug)
> > >  {
> > >  	u32 debug_mask, mask;
> > > @@ -595,8 +558,6 @@ static void hsw_psr_enable_source(struct
> > > intel_dp
> > > *intel_dp,
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	enum transcoder cpu_transcoder = crtc_state-
> > > >cpu_transcoder;
> > >  
> > > -	psr_aux_io_power_get(intel_dp);
> > > -
> > >  	/* Only HSW and BDW have PSR AUX registers that need to
> > > be
> > > setup. SKL+
> > >  	 * use hardcoded values PSR AUX transactions
> > >  	 */
> > > @@ -717,8 +678,6 @@ static void hsw_psr_disable(struct intel_dp
> > > *intel_dp,
> > >  		else
> > >  			WARN_ON(I915_READ(EDP_PSR_CTL) &
> > > EDP_PSR_ENABLE);
> > >  	}
> > > -
> > > -	psr_aux_io_power_put(intel_dp);
> > >  }
> > >  
> > >  /**
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index de3a81034f77..2969787201ef 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -1824,6 +1824,7 @@ void intel_display_power_put(struct
> > > drm_i915_private *dev_priv,
> > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > >  #define GLK_DISPLAY_AUX_A_POWER_DOMAINS (		\
> > >  	BIT_ULL(POWER_DOMAIN_AUX_A) |		\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > >  #define GLK_DISPLAY_AUX_B_POWER_DOMAINS (		\
> > >  	BIT_ULL(POWER_DOMAIN_AUX_B) |		\
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915/ddi: Get AUX power domain for DP main link too (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/ddi: Get AUX power domain for DP main link too (rev2)
URL   : https://patchwork.freedesktop.org/series/45188/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4363_full -> Patchwork_9388_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9388_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9388_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_9388_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-vebox:
      shard-kbl:          SKIP -> PASS

    igt@perf_pmu@rc6:
      shard-kbl:          PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

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

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665, fdo#106023)

    igt@gem_workarounds@suspend-resume-context:
      shard-apl:          PASS -> FAIL (fdo#103375) +1

    igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          PASS -> FAIL (fdo#106509, fdo#105454)

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

    igt@kms_sysfs_edid_timing:
      shard-glk:          NOTRUN -> WARN (fdo#100047)

    igt@perf_pmu@busy-idle-check-all-rcs0:
      shard-snb:          PASS -> INCOMPLETE (fdo#105411)

    
    ==== Possible fixes ====

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

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

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

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

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

    
  fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105189 https://bugs.freedesktop.org/show_bug.cgi?id=105189
  fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
  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_4363 -> Patchwork_9388

  CI_DRM_4363: 9e52e63f91e95af0f7475ccce206d4bdfca8933a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4528: 6be300d405de5974b262e8b93a445be4ac618e6a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9388: 6df9a07560c7c1343343e5d9332972e84a4c27bb @ 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_9388/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/ddi: Get AUX power domain for DP main link too

[Souza, Jose]

On Thu, 2018-06-21 at 21:44 +0300, Imre Deak wrote:
> So far we got an AUX power domain reference only for the duration of
> DP
> AUX transfers. However, the following suggests that we also need
> these
> for main link functionality:
> - The specification doesn't state whether it's needed or not for main
>   link functionality, but suggests that these power wells need to be
>   enabled already during display core initialization (Sequences to
>   Initialize Display).
> - For PSR we need to keep the AUX power well enabled.
> - On ICL combo PHY ports (non-TC) the AUX power well is needed for
>   link training too: while the port is enabled with a DP link
> training
>   test pattern trying to toggle the AUX power well will time out.
> - On ICL MG PHY ports (TC) the AUX power well is needed also for main
>   link functionality (both in DP and HDMI modes).
> - Windows enables these power wells both for main and AUX lane
>   functionality.
> 
> Based on the above take an AUX power reference for main link
> functionality too. This makes a difference only on GEN10+ (GLK+)
> platforms, where we have separate port specific AUX power wells.
> 
> For PSR we still need to distinguish between port A and the other
> ports, since on port A DC states must stay enabled for main link
> functionality, but DC states must be disabled for driver initiated
> AUX transfers. So re-use the corresponding helper from intel_psr.c.
> 
> Since we take now a reference for main link functionality on all DP
> ports we can forgo taking the separate power ref for PSR
> functionality.
> 
> v2:
> - Make sure DC states stay enabled when taking the ref on port A.
>   (Ville)
> 
> v3: (Ville)
> - Fix comment about logic for encoders without a crtc state and
>   add FIXME note for a simplification to avoid calling
> get_power_domains
>   in such cases.
> - Use intel_crtc_has_dp_encoder() instead !intel_crtc_has_type(HDMI).
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

The spec is not clear but this fix the "aux power well" enable timeouts
that I was getting in aux B so looks like your interpretation is right.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_ddi.c        | 54
> ++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_display.c    | 12 +++++++-
>  drivers/gpu/drm/i915/intel_drv.h        |  3 +-
>  drivers/gpu/drm/i915/intel_psr.c        | 41 ---------------------
> ----
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  1 +
>  5 files changed, 64 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 044fe1fb9872..0319825b725b 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1983,15 +1983,55 @@ bool intel_ddi_get_hw_state(struct
> intel_encoder *encoder,
>  	return ret;
>  }
>  
> -static u64 intel_ddi_get_power_domains(struct intel_encoder
> *encoder)
> +static inline enum intel_display_power_domain
> +intel_ddi_main_link_aux_domain(struct intel_dp *intel_dp)
> +{
> +	/* CNL HW requires corresponding AUX IOs to be powered up
> for PSR with
> +	 * DC states enabled at the same time, while for driver
> initiated AUX
> +	 * transfers we need the same AUX IOs to be powered but with
> DC states
> +	 * disabled. Accordingly use the AUX power domain here which
> leaves DC
> +	 * states enabled.
> +	 * However, for non-A AUX ports the corresponding non-EDP
> transcoders
> +	 * would have already enabled power well 2 and DC_OFF. This
> means we can
> +	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> instead of a
> +	 * specific AUX_IO reference without powering up any extra
> wells.
> +	 * Note that PSR is enabled only on Port A even though this
> function
> +	 * returns the correct domain for other ports too.
> +	 */
> +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A
> :
> +					      intel_dp-
> >aux_power_domain;
> +}
> +
> +static u64 intel_ddi_get_power_domains(struct intel_encoder
> *encoder,
> +				       struct intel_crtc_state
> *crtc_state)
>  {
>  	struct intel_digital_port *dig_port =
> enc_to_dig_port(&encoder->base);
>  	enum pipe pipe;
> +	u64 domains;
>  
> -	if (intel_ddi_get_hw_state(encoder, &pipe))
> -		return BIT_ULL(dig_port->ddi_io_power_domain);
> +	if (!intel_ddi_get_hw_state(encoder, &pipe))
> +		return 0;
>  
> -	return 0;
> +	domains = BIT_ULL(dig_port->ddi_io_power_domain);
> +	if (!crtc_state)
> +		return domains;
> +
> +	/*
> +	 * TODO: Add support for MST encoders. Atm, the following
> should never
> +	 * happen since this function will be called only for the
> primary
> +	 * encoder which doesn't have its own pipe/crtc_state.
> +	 */
> +	if (WARN_ON(intel_crtc_has_type(crtc_state,
> INTEL_OUTPUT_DP_MST)))
> +		return domains;
> +
> +	/* AUX power is only needed for (e)DP mode, not for HDMI. */
> +	if (intel_crtc_has_dp_encoder(crtc_state)) {
> +		struct intel_dp *intel_dp = &dig_port->dp;
> +
> +		domains |=
> BIT_ULL(intel_ddi_main_link_aux_domain(intel_dp));
> +	}
> +
> +	return domains;
>  }
>  
>  void intel_ddi_enable_pipe_clock(const struct intel_crtc_state
> *crtc_state)
> @@ -2631,6 +2671,9 @@ static void intel_ddi_pre_enable_dp(struct
> intel_encoder *encoder,
>  
>  	WARN_ON(is_mst && (port == PORT_A || port == PORT_E));
>  
> +	intel_display_power_get(dev_priv,
> +				intel_ddi_main_link_aux_domain(intel
> _dp));
> +
>  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
>  				 crtc_state->lane_count, is_mst);
>  
> @@ -2775,6 +2818,9 @@ static void intel_ddi_post_disable_dp(struct
> intel_encoder *encoder,
>  	intel_display_power_put(dev_priv, dig_port-
> >ddi_io_power_domain);
>  
>  	intel_ddi_clk_disable(encoder);
> +
> +	intel_display_power_put(dev_priv,
> +				intel_ddi_main_link_aux_domain(intel
> _dp));
>  }
>  
>  static void intel_ddi_post_disable_hdmi(struct intel_encoder
> *encoder,
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 2c8fef3ede54..ce615f89b43a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct
> drm_i915_private *dev_priv)
>  	for_each_intel_encoder(&dev_priv->drm, encoder) {
>  		u64 get_domains;
>  		enum intel_display_power_domain domain;
> +		struct intel_crtc_state *crtc_state;
>  
>  		if (!encoder->get_power_domains)
>  			continue;
>  
> -		get_domains = encoder->get_power_domains(encoder);
> +		/*
> +		 * For MST and inactive encoders we don't have a
> crtc state.
> +		 * FIXME: no need to call get_power_domains in such
> cases, it
> +		 * will always return 0.
> +		 */
> +		crtc_state = encoder->base.crtc ?
> +			     to_intel_crtc_state(encoder->base.crtc-
> >state) :
> +			     NULL;
> +
> +		get_domains = encoder->get_power_domains(encoder,
> crtc_state);
>  		for_each_power_domain(domain, get_domains)
>  			intel_display_power_get(dev_priv, domain);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 0c3ac0eafde0..7174429d930a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -254,7 +254,8 @@ struct intel_encoder {
>  			   struct intel_crtc_state *pipe_config);
>  	/* Returns a mask of power domains that need to be
> referenced as part
>  	 * of the hardware state readout code. */
> -	u64 (*get_power_domains)(struct intel_encoder *encoder);
> +	u64 (*get_power_domains)(struct intel_encoder *encoder,
> +				 struct intel_crtc_state
> *crtc_state);
>  	/*
>  	 * Called during system suspend after all pending requests
> for the
>  	 * encoder are flushed (for example for DP AUX transactions)
> and
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index d4cd19fea148..eecdd8b5b5e0 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -56,43 +56,6 @@
>  #include "intel_drv.h"
>  #include "i915_drv.h"
>  
> -static inline enum intel_display_power_domain
> -psr_aux_domain(struct intel_dp *intel_dp)
> -{
> -	/* CNL HW requires corresponding AUX IOs to be powered up
> for PSR.
> -	 * However, for non-A AUX ports the corresponding non-EDP
> transcoders
> -	 * would have already enabled power well 2 and DC_OFF. This
> means we can
> -	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> instead of a
> -	 * specific AUX_IO reference without powering up any extra
> wells.
> -	 * Note that PSR is enabled only on Port A even though this
> function
> -	 * returns the correct domain for other ports too.
> -	 */
> -	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A
> :
> -					      intel_dp-
> >aux_power_domain;
> -}
> -
> -static void psr_aux_io_power_get(struct intel_dp *intel_dp)
> -{
> -	struct intel_digital_port *intel_dig_port =
> dp_to_dig_port(intel_dp);
> -	struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
> >base.base.dev);
> -
> -	if (INTEL_GEN(dev_priv) < 10)
> -		return;
> -
> -	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> -}
> -
> -static void psr_aux_io_power_put(struct intel_dp *intel_dp)
> -{
> -	struct intel_digital_port *intel_dig_port =
> dp_to_dig_port(intel_dp);
> -	struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
> >base.base.dev);
> -
> -	if (INTEL_GEN(dev_priv) < 10)
> -		return;
> -
> -	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> -}
> -
>  void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool
> debug)
>  {
>  	u32 debug_mask, mask;
> @@ -595,8 +558,6 @@ static void hsw_psr_enable_source(struct intel_dp
> *intel_dp,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>  
> -	psr_aux_io_power_get(intel_dp);
> -
>  	/* Only HSW and BDW have PSR AUX registers that need to be
> setup. SKL+
>  	 * use hardcoded values PSR AUX transactions
>  	 */
> @@ -717,8 +678,6 @@ static void hsw_psr_disable(struct intel_dp
> *intel_dp,
>  		else
>  			WARN_ON(I915_READ(EDP_PSR_CTL) &
> EDP_PSR_ENABLE);
>  	}
> -
> -	psr_aux_io_power_put(intel_dp);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index de3a81034f77..2969787201ef 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1824,6 +1824,7 @@ void intel_display_power_put(struct
> drm_i915_private *dev_priv,
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  #define GLK_DISPLAY_AUX_A_POWER_DOMAINS (		\
>  	BIT_ULL(POWER_DOMAIN_AUX_A) |		\
> +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  #define GLK_DISPLAY_AUX_B_POWER_DOMAINS (		\
>  	BIT_ULL(POWER_DOMAIN_AUX_B) |		\
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.IGT: success for drm/i915/ddi: Get AUX power domain for DP main link too (rev2)

[Imre Deak]

On Fri, Jun 22, 2018 at 04:12:29AM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915/ddi: Get AUX power domain for DP main link too (rev2)
> URL   : https://patchwork.freedesktop.org/series/45188/
> State : success

Pushed to -dinq, thanks for the reviews.

> 
> == Summary ==
> 
> = CI Bug Log - changes from CI_DRM_4363_full -> Patchwork_9388_full =
> 
> == Summary - WARNING ==
> 
>   Minor unknown changes coming with Patchwork_9388_full need to be verified
>   manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_9388_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_9388_full:
> 
>   === IGT changes ===
> 
>     ==== Warnings ====
> 
>     igt@gem_mocs_settings@mocs-rc6-vebox:
>       shard-kbl:          SKIP -> PASS
> 
>     igt@perf_pmu@rc6:
>       shard-kbl:          PASS -> SKIP
> 
>     
> == Known issues ==
> 
>   Here are the changes found in Patchwork_9388_full that come from known issues:
> 
>   === IGT changes ===
> 
>     ==== Issues hit ====
> 
>     igt@drv_suspend@shrink:
>       shard-apl:          PASS -> FAIL (fdo#106886)
> 
>     igt@gem_ppgtt@blt-vs-render-ctx0:
>       shard-kbl:          PASS -> INCOMPLETE (fdo#103665, fdo#106023)
> 
>     igt@gem_workarounds@suspend-resume-context:
>       shard-apl:          PASS -> FAIL (fdo#103375) +1
> 
>     igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
>       shard-glk:          PASS -> FAIL (fdo#106509, fdo#105454)
> 
>     igt@kms_flip@plain-flip-ts-check-interruptible:
>       shard-glk:          PASS -> FAIL (fdo#100368)
> 
>     igt@kms_sysfs_edid_timing:
>       shard-glk:          NOTRUN -> WARN (fdo#100047)
> 
>     igt@perf_pmu@busy-idle-check-all-rcs0:
>       shard-snb:          PASS -> INCOMPLETE (fdo#105411)
> 
>     
>     ==== Possible fixes ====
> 
>     igt@drv_selftest@live_gtt:
>       shard-kbl:          FAIL (fdo#105347) -> PASS
> 
>     igt@drv_selftest@live_hangcheck:
>       shard-kbl:          DMESG-FAIL (fdo#106560, fdo#106947) -> PASS
>       shard-apl:          DMESG-FAIL (fdo#106560, fdo#106947) -> PASS
> 
>     igt@kms_flip@flip-vs-expired-vblank:
>       shard-glk:          FAIL (fdo#105189) -> PASS
> 
>     igt@kms_flip_tiling@flip-to-x-tiled:
>       shard-glk:          FAIL (fdo#104724) -> PASS +1
> 
>     igt@kms_setmode@basic:
>       shard-apl:          FAIL (fdo#99912) -> PASS
> 
>     
>   fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
>   fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
>   fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
>   fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
>   fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
>   fdo#105189 https://bugs.freedesktop.org/show_bug.cgi?id=105189
>   fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
>   fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
>   fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
>   fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
>   fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
>   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_4363 -> Patchwork_9388
> 
>   CI_DRM_4363: 9e52e63f91e95af0f7475ccce206d4bdfca8933a @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_4528: 6be300d405de5974b262e8b93a445be4ac618e6a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_9388: 6df9a07560c7c1343343e5d9332972e84a4c27bb @ 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_9388/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/ddi: Get AUX power domain for DP main link too

[Paulo Zanoni]

Em Seg, 2018-06-25 às 16:55 -0700, Souza, Jose escreveu:
> On Thu, 2018-06-21 at 21:44 +0300, Imre Deak wrote:
> > So far we got an AUX power domain reference only for the duration
> > of
> > DP
> > AUX transfers. However, the following suggests that we also need
> > these
> > for main link functionality:
> > - The specification doesn't state whether it's needed or not for
> > main
> >   link functionality, but suggests that these power wells need to
> > be
> >   enabled already during display core initialization (Sequences to
> >   Initialize Display).

No, the specification states it's needed. Page "Sequences for
DisplayPort" (the ICL version, of course), step 1.b:

"TC: Note that AUX power is required for running main link."

Same for the HDMI page.

> > - For PSR we need to keep the AUX power well enabled.
> > - On ICL combo PHY ports (non-TC) the AUX power well is needed for
> >   link training too: while the port is enabled with a DP link
> > training
> >   test pattern trying to toggle the AUX power well will time out.
> > - On ICL MG PHY ports (TC) the AUX power well is needed also for
> > main
> >   link functionality (both in DP and HDMI modes).
> > - Windows enables these power wells both for main and AUX lane
> >   functionality.
> > 
> > Based on the above take an AUX power reference for main link
> > functionality too. This makes a difference only on GEN10+ (GLK+)
> > platforms, where we have separate port specific AUX power wells.
> > 
> > For PSR we still need to distinguish between port A and the other
> > ports, since on port A DC states must stay enabled for main link
> > functionality, but DC states must be disabled for driver initiated
> > AUX transfers. So re-use the corresponding helper from intel_psr.c.
> > 
> > Since we take now a reference for main link functionality on all DP
> > ports we can forgo taking the separate power ref for PSR
> > functionality.
> > 
> > v2:
> > - Make sure DC states stay enabled when taking the ref on port A.
> >   (Ville)
> > 
> > v3: (Ville)
> > - Fix comment about logic for encoders without a crtc state and
> >   add FIXME note for a simplification to avoid calling
> > get_power_domains
> >   in such cases.
> > - Use intel_crtc_has_dp_encoder() instead
> > !intel_crtc_has_type(HDMI).
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> The spec is not clear but this fix the "aux power well" enable
> timeouts
> that I was getting in aux B so looks like your interpretation is
> right.
> 
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c        | 54
> > ++++++++++++++++++++++++++++++---
> >  drivers/gpu/drm/i915/intel_display.c    | 12 +++++++-
> >  drivers/gpu/drm/i915/intel_drv.h        |  3 +-
> >  drivers/gpu/drm/i915/intel_psr.c        | 41 ---------------------
> > ----
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  1 +
> >  5 files changed, 64 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 044fe1fb9872..0319825b725b 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1983,15 +1983,55 @@ bool intel_ddi_get_hw_state(struct
> > intel_encoder *encoder,
> >  	return ret;
> >  }
> >  
> > -static u64 intel_ddi_get_power_domains(struct intel_encoder
> > *encoder)
> > +static inline enum intel_display_power_domain
> > +intel_ddi_main_link_aux_domain(struct intel_dp *intel_dp)
> > +{
> > +	/* CNL HW requires corresponding AUX IOs to be powered up
> > for PSR with
> > +	 * DC states enabled at the same time, while for driver
> > initiated AUX
> > +	 * transfers we need the same AUX IOs to be powered but
> > with
> > DC states
> > +	 * disabled. Accordingly use the AUX power domain here
> > which
> > leaves DC
> > +	 * states enabled.
> > +	 * However, for non-A AUX ports the corresponding non-EDP
> > transcoders
> > +	 * would have already enabled power well 2 and DC_OFF.
> > This
> > means we can
> > +	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> > instead of a
> > +	 * specific AUX_IO reference without powering up any extra
> > wells.
> > +	 * Note that PSR is enabled only on Port A even though
> > this
> > function
> > +	 * returns the correct domain for other ports too.
> > +	 */
> > +	return intel_dp->aux_ch == AUX_CH_A ?
> > POWER_DOMAIN_AUX_IO_A
> > :
> > +					      intel_dp-
> > > aux_power_domain;
> > 
> > +}
> > +
> > +static u64 intel_ddi_get_power_domains(struct intel_encoder
> > *encoder,
> > +				       struct intel_crtc_state
> > *crtc_state)
> >  {
> >  	struct intel_digital_port *dig_port =
> > enc_to_dig_port(&encoder->base);
> >  	enum pipe pipe;
> > +	u64 domains;
> >  
> > -	if (intel_ddi_get_hw_state(encoder, &pipe))
> > -		return BIT_ULL(dig_port->ddi_io_power_domain);
> > +	if (!intel_ddi_get_hw_state(encoder, &pipe))
> > +		return 0;
> >  
> > -	return 0;
> > +	domains = BIT_ULL(dig_port->ddi_io_power_domain);
> > +	if (!crtc_state)
> > +		return domains;
> > +
> > +	/*
> > +	 * TODO: Add support for MST encoders. Atm, the following
> > should never
> > +	 * happen since this function will be called only for the
> > primary
> > +	 * encoder which doesn't have its own pipe/crtc_state.
> > +	 */
> > +	if (WARN_ON(intel_crtc_has_type(crtc_state,
> > INTEL_OUTPUT_DP_MST)))
> > +		return domains;
> > +
> > +	/* AUX power is only needed for (e)DP mode, not for HDMI.
> > */
> > +	if (intel_crtc_has_dp_encoder(crtc_state)) {
> > +		struct intel_dp *intel_dp = &dig_port->dp;
> > +
> > +		domains |=
> > BIT_ULL(intel_ddi_main_link_aux_domain(intel_dp));
> > +	}
> > +
> > +	return domains;
> >  }
> >  
> >  void intel_ddi_enable_pipe_clock(const struct intel_crtc_state
> > *crtc_state)
> > @@ -2631,6 +2671,9 @@ static void intel_ddi_pre_enable_dp(struct
> > intel_encoder *encoder,
> >  
> >  	WARN_ON(is_mst && (port == PORT_A || port == PORT_E));
> >  
> > +	intel_display_power_get(dev_priv,
> > +				intel_ddi_main_link_aux_domain(int
> > el
> > _dp));
> > +
> >  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> >  				 crtc_state->lane_count, is_mst);
> >  
> > @@ -2775,6 +2818,9 @@ static void intel_ddi_post_disable_dp(struct
> > intel_encoder *encoder,
> >  	intel_display_power_put(dev_priv, dig_port-
> > > ddi_io_power_domain);
> > 
> >  
> >  	intel_ddi_clk_disable(encoder);
> > +
> > +	intel_display_power_put(dev_priv,
> > +				intel_ddi_main_link_aux_domain(int
> > el
> > _dp));
> >  }
> >  
> >  static void intel_ddi_post_disable_hdmi(struct intel_encoder
> > *encoder,
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 2c8fef3ede54..ce615f89b43a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct
> > drm_i915_private *dev_priv)
> >  	for_each_intel_encoder(&dev_priv->drm, encoder) {
> >  		u64 get_domains;
> >  		enum intel_display_power_domain domain;
> > +		struct intel_crtc_state *crtc_state;
> >  
> >  		if (!encoder->get_power_domains)
> >  			continue;
> >  
> > -		get_domains = encoder->get_power_domains(encoder);
> > +		/*
> > +		 * For MST and inactive encoders we don't have a
> > crtc state.
> > +		 * FIXME: no need to call get_power_domains in
> > such
> > cases, it
> > +		 * will always return 0.
> > +		 */
> > +		crtc_state = encoder->base.crtc ?
> > +			     to_intel_crtc_state(encoder-
> > >base.crtc-
> > > state) :
> > 
> > +			     NULL;
> > +
> > +		get_domains = encoder->get_power_domains(encoder,
> > crtc_state);
> >  		for_each_power_domain(domain, get_domains)
> >  			intel_display_power_get(dev_priv, domain);
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 0c3ac0eafde0..7174429d930a 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -254,7 +254,8 @@ struct intel_encoder {
> >  			   struct intel_crtc_state *pipe_config);
> >  	/* Returns a mask of power domains that need to be
> > referenced as part
> >  	 * of the hardware state readout code. */
> > -	u64 (*get_power_domains)(struct intel_encoder *encoder);
> > +	u64 (*get_power_domains)(struct intel_encoder *encoder,
> > +				 struct intel_crtc_state
> > *crtc_state);
> >  	/*
> >  	 * Called during system suspend after all pending requests
> > for the
> >  	 * encoder are flushed (for example for DP AUX
> > transactions)
> > and
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index d4cd19fea148..eecdd8b5b5e0 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -56,43 +56,6 @@
> >  #include "intel_drv.h"
> >  #include "i915_drv.h"
> >  
> > -static inline enum intel_display_power_domain
> > -psr_aux_domain(struct intel_dp *intel_dp)
> > -{
> > -	/* CNL HW requires corresponding AUX IOs to be powered up
> > for PSR.
> > -	 * However, for non-A AUX ports the corresponding non-EDP
> > transcoders
> > -	 * would have already enabled power well 2 and DC_OFF.
> > This
> > means we can
> > -	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> > instead of a
> > -	 * specific AUX_IO reference without powering up any extra
> > wells.
> > -	 * Note that PSR is enabled only on Port A even though
> > this
> > function
> > -	 * returns the correct domain for other ports too.
> > -	 */
> > -	return intel_dp->aux_ch == AUX_CH_A ?
> > POWER_DOMAIN_AUX_IO_A
> > :
> > -					      intel_dp-
> > > aux_power_domain;
> > 
> > -}
> > -
> > -static void psr_aux_io_power_get(struct intel_dp *intel_dp)
> > -{
> > -	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > -	struct drm_i915_private *dev_priv =
> > to_i915(intel_dig_port-
> > > base.base.dev);
> > 
> > -
> > -	if (INTEL_GEN(dev_priv) < 10)
> > -		return;
> > -
> > -	intel_display_power_get(dev_priv,
> > psr_aux_domain(intel_dp));
> > -}
> > -
> > -static void psr_aux_io_power_put(struct intel_dp *intel_dp)
> > -{
> > -	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > -	struct drm_i915_private *dev_priv =
> > to_i915(intel_dig_port-
> > > base.base.dev);
> > 
> > -
> > -	if (INTEL_GEN(dev_priv) < 10)
> > -		return;
> > -
> > -	intel_display_power_put(dev_priv,
> > psr_aux_domain(intel_dp));
> > -}
> > -
> >  void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool
> > debug)
> >  {
> >  	u32 debug_mask, mask;
> > @@ -595,8 +558,6 @@ static void hsw_psr_enable_source(struct
> > intel_dp
> > *intel_dp,
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	enum transcoder cpu_transcoder = crtc_state-
> > >cpu_transcoder;
> >  
> > -	psr_aux_io_power_get(intel_dp);
> > -
> >  	/* Only HSW and BDW have PSR AUX registers that need to be
> > setup. SKL+
> >  	 * use hardcoded values PSR AUX transactions
> >  	 */
> > @@ -717,8 +678,6 @@ static void hsw_psr_disable(struct intel_dp
> > *intel_dp,
> >  		else
> >  			WARN_ON(I915_READ(EDP_PSR_CTL) &
> > EDP_PSR_ENABLE);
> >  	}
> > -
> > -	psr_aux_io_power_put(intel_dp);
> >  }
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index de3a81034f77..2969787201ef 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -1824,6 +1824,7 @@ void intel_display_power_put(struct
> > drm_i915_private *dev_priv,
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  #define GLK_DISPLAY_AUX_A_POWER_DOMAINS (		\
> >  	BIT_ULL(POWER_DOMAIN_AUX_A) |		\
> > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  #define GLK_DISPLAY_AUX_B_POWER_DOMAINS (		\
> >  	BIT_ULL(POWER_DOMAIN_AUX_B) |		\
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/ddi: Get AUX power domain for DP main link too

[Imre Deak]

On Tue, Jun 26, 2018 at 10:40:26AM -0700, Paulo Zanoni wrote:
> Em Seg, 2018-06-25 às 16:55 -0700, Souza, Jose escreveu:
> > On Thu, 2018-06-21 at 21:44 +0300, Imre Deak wrote:
> > > So far we got an AUX power domain reference only for the duration
> > > of
> > > DP
> > > AUX transfers. However, the following suggests that we also need
> > > these
> > > for main link functionality:
> > > - The specification doesn't state whether it's needed or not for
> > > main
> > >   link functionality, but suggests that these power wells need to
> > > be
> > >   enabled already during display core initialization (Sequences to
> > >   Initialize Display).
> 
> No, the specification states it's needed. Page "Sequences for
> DisplayPort" (the ICL version, of course), step 1.b:
> 
> "TC: Note that AUX power is required for running main link."
> 
> Same for the HDMI page.

Yes, for ICL MG PHY ports (in all modes) it's clearly stated as I also
wrote it below. The above is about the ICL combo PHY ports and the other
platforms. For those the only hint is that enabling the AUX power wells
is listed as a step in the 'Sequences to Initialize Display".

> 
> > > - For PSR we need to keep the AUX power well enabled.
> > > - On ICL combo PHY ports (non-TC) the AUX power well is needed for
> > >   link training too: while the port is enabled with a DP link
> > > training
> > >   test pattern trying to toggle the AUX power well will time out.
> > > - On ICL MG PHY ports (TC) the AUX power well is needed also for
> > > main
> > >   link functionality (both in DP and HDMI modes).
> > > - Windows enables these power wells both for main and AUX lane
> > >   functionality.
> > > 
> > > Based on the above take an AUX power reference for main link
> > > functionality too. This makes a difference only on GEN10+ (GLK+)
> > > platforms, where we have separate port specific AUX power wells.
> > > 
> > > For PSR we still need to distinguish between port A and the other
> > > ports, since on port A DC states must stay enabled for main link
> > > functionality, but DC states must be disabled for driver initiated
> > > AUX transfers. So re-use the corresponding helper from intel_psr.c.
> > > 
> > > Since we take now a reference for main link functionality on all DP
> > > ports we can forgo taking the separate power ref for PSR
> > > functionality.
> > > 
> > > v2:
> > > - Make sure DC states stay enabled when taking the ref on port A.
> > >   (Ville)
> > > 
> > > v3: (Ville)
> > > - Fix comment about logic for encoders without a crtc state and
> > >   add FIXME note for a simplification to avoid calling
> > > get_power_domains
> > >   in such cases.
> > > - Use intel_crtc_has_dp_encoder() instead
> > > !intel_crtc_has_type(HDMI).
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > 
> > The spec is not clear but this fix the "aux power well" enable
> > timeouts
> > that I was getting in aux B so looks like your interpretation is
> > right.
> > 
> > Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c        | 54
> > > ++++++++++++++++++++++++++++++---
> > >  drivers/gpu/drm/i915/intel_display.c    | 12 +++++++-
> > >  drivers/gpu/drm/i915/intel_drv.h        |  3 +-
> > >  drivers/gpu/drm/i915/intel_psr.c        | 41 ---------------------
> > > ----
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c |  1 +
> > >  5 files changed, 64 insertions(+), 47 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 044fe1fb9872..0319825b725b 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -1983,15 +1983,55 @@ bool intel_ddi_get_hw_state(struct
> > > intel_encoder *encoder,
> > >  	return ret;
> > >  }
> > >  
> > > -static u64 intel_ddi_get_power_domains(struct intel_encoder
> > > *encoder)
> > > +static inline enum intel_display_power_domain
> > > +intel_ddi_main_link_aux_domain(struct intel_dp *intel_dp)
> > > +{
> > > +	/* CNL HW requires corresponding AUX IOs to be powered up
> > > for PSR with
> > > +	 * DC states enabled at the same time, while for driver
> > > initiated AUX
> > > +	 * transfers we need the same AUX IOs to be powered but
> > > with
> > > DC states
> > > +	 * disabled. Accordingly use the AUX power domain here
> > > which
> > > leaves DC
> > > +	 * states enabled.
> > > +	 * However, for non-A AUX ports the corresponding non-EDP
> > > transcoders
> > > +	 * would have already enabled power well 2 and DC_OFF.
> > > This
> > > means we can
> > > +	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> > > instead of a
> > > +	 * specific AUX_IO reference without powering up any extra
> > > wells.
> > > +	 * Note that PSR is enabled only on Port A even though
> > > this
> > > function
> > > +	 * returns the correct domain for other ports too.
> > > +	 */
> > > +	return intel_dp->aux_ch == AUX_CH_A ?
> > > POWER_DOMAIN_AUX_IO_A
> > > :
> > > +					      intel_dp-
> > > > aux_power_domain;
> > > 
> > > +}
> > > +
> > > +static u64 intel_ddi_get_power_domains(struct intel_encoder
> > > *encoder,
> > > +				       struct intel_crtc_state
> > > *crtc_state)
> > >  {
> > >  	struct intel_digital_port *dig_port =
> > > enc_to_dig_port(&encoder->base);
> > >  	enum pipe pipe;
> > > +	u64 domains;
> > >  
> > > -	if (intel_ddi_get_hw_state(encoder, &pipe))
> > > -		return BIT_ULL(dig_port->ddi_io_power_domain);
> > > +	if (!intel_ddi_get_hw_state(encoder, &pipe))
> > > +		return 0;
> > >  
> > > -	return 0;
> > > +	domains = BIT_ULL(dig_port->ddi_io_power_domain);
> > > +	if (!crtc_state)
> > > +		return domains;
> > > +
> > > +	/*
> > > +	 * TODO: Add support for MST encoders. Atm, the following
> > > should never
> > > +	 * happen since this function will be called only for the
> > > primary
> > > +	 * encoder which doesn't have its own pipe/crtc_state.
> > > +	 */
> > > +	if (WARN_ON(intel_crtc_has_type(crtc_state,
> > > INTEL_OUTPUT_DP_MST)))
> > > +		return domains;
> > > +
> > > +	/* AUX power is only needed for (e)DP mode, not for HDMI.
> > > */
> > > +	if (intel_crtc_has_dp_encoder(crtc_state)) {
> > > +		struct intel_dp *intel_dp = &dig_port->dp;
> > > +
> > > +		domains |=
> > > BIT_ULL(intel_ddi_main_link_aux_domain(intel_dp));
> > > +	}
> > > +
> > > +	return domains;
> > >  }
> > >  
> > >  void intel_ddi_enable_pipe_clock(const struct intel_crtc_state
> > > *crtc_state)
> > > @@ -2631,6 +2671,9 @@ static void intel_ddi_pre_enable_dp(struct
> > > intel_encoder *encoder,
> > >  
> > >  	WARN_ON(is_mst && (port == PORT_A || port == PORT_E));
> > >  
> > > +	intel_display_power_get(dev_priv,
> > > +				intel_ddi_main_link_aux_domain(int
> > > el
> > > _dp));
> > > +
> > >  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> > >  				 crtc_state->lane_count, is_mst);
> > >  
> > > @@ -2775,6 +2818,9 @@ static void intel_ddi_post_disable_dp(struct
> > > intel_encoder *encoder,
> > >  	intel_display_power_put(dev_priv, dig_port-
> > > > ddi_io_power_domain);
> > > 
> > >  
> > >  	intel_ddi_clk_disable(encoder);
> > > +
> > > +	intel_display_power_put(dev_priv,
> > > +				intel_ddi_main_link_aux_domain(int
> > > el
> > > _dp));
> > >  }
> > >  
> > >  static void intel_ddi_post_disable_hdmi(struct intel_encoder
> > > *encoder,
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 2c8fef3ede54..ce615f89b43a 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct
> > > drm_i915_private *dev_priv)
> > >  	for_each_intel_encoder(&dev_priv->drm, encoder) {
> > >  		u64 get_domains;
> > >  		enum intel_display_power_domain domain;
> > > +		struct intel_crtc_state *crtc_state;
> > >  
> > >  		if (!encoder->get_power_domains)
> > >  			continue;
> > >  
> > > -		get_domains = encoder->get_power_domains(encoder);
> > > +		/*
> > > +		 * For MST and inactive encoders we don't have a
> > > crtc state.
> > > +		 * FIXME: no need to call get_power_domains in
> > > such
> > > cases, it
> > > +		 * will always return 0.
> > > +		 */
> > > +		crtc_state = encoder->base.crtc ?
> > > +			     to_intel_crtc_state(encoder-
> > > >base.crtc-
> > > > state) :
> > > 
> > > +			     NULL;
> > > +
> > > +		get_domains = encoder->get_power_domains(encoder,
> > > crtc_state);
> > >  		for_each_power_domain(domain, get_domains)
> > >  			intel_display_power_get(dev_priv, domain);
> > >  	}
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 0c3ac0eafde0..7174429d930a 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -254,7 +254,8 @@ struct intel_encoder {
> > >  			   struct intel_crtc_state *pipe_config);
> > >  	/* Returns a mask of power domains that need to be
> > > referenced as part
> > >  	 * of the hardware state readout code. */
> > > -	u64 (*get_power_domains)(struct intel_encoder *encoder);
> > > +	u64 (*get_power_domains)(struct intel_encoder *encoder,
> > > +				 struct intel_crtc_state
> > > *crtc_state);
> > >  	/*
> > >  	 * Called during system suspend after all pending requests
> > > for the
> > >  	 * encoder are flushed (for example for DP AUX
> > > transactions)
> > > and
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index d4cd19fea148..eecdd8b5b5e0 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -56,43 +56,6 @@
> > >  #include "intel_drv.h"
> > >  #include "i915_drv.h"
> > >  
> > > -static inline enum intel_display_power_domain
> > > -psr_aux_domain(struct intel_dp *intel_dp)
> > > -{
> > > -	/* CNL HW requires corresponding AUX IOs to be powered up
> > > for PSR.
> > > -	 * However, for non-A AUX ports the corresponding non-EDP
> > > transcoders
> > > -	 * would have already enabled power well 2 and DC_OFF.
> > > This
> > > means we can
> > > -	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> > > instead of a
> > > -	 * specific AUX_IO reference without powering up any extra
> > > wells.
> > > -	 * Note that PSR is enabled only on Port A even though
> > > this
> > > function
> > > -	 * returns the correct domain for other ports too.
> > > -	 */
> > > -	return intel_dp->aux_ch == AUX_CH_A ?
> > > POWER_DOMAIN_AUX_IO_A
> > > :
> > > -					      intel_dp-
> > > > aux_power_domain;
> > > 
> > > -}
> > > -
> > > -static void psr_aux_io_power_get(struct intel_dp *intel_dp)
> > > -{
> > > -	struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > -	struct drm_i915_private *dev_priv =
> > > to_i915(intel_dig_port-
> > > > base.base.dev);
> > > 
> > > -
> > > -	if (INTEL_GEN(dev_priv) < 10)
> > > -		return;
> > > -
> > > -	intel_display_power_get(dev_priv,
> > > psr_aux_domain(intel_dp));
> > > -}
> > > -
> > > -static void psr_aux_io_power_put(struct intel_dp *intel_dp)
> > > -{
> > > -	struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > -	struct drm_i915_private *dev_priv =
> > > to_i915(intel_dig_port-
> > > > base.base.dev);
> > > 
> > > -
> > > -	if (INTEL_GEN(dev_priv) < 10)
> > > -		return;
> > > -
> > > -	intel_display_power_put(dev_priv,
> > > psr_aux_domain(intel_dp));
> > > -}
> > > -
> > >  void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool
> > > debug)
> > >  {
> > >  	u32 debug_mask, mask;
> > > @@ -595,8 +558,6 @@ static void hsw_psr_enable_source(struct
> > > intel_dp
> > > *intel_dp,
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	enum transcoder cpu_transcoder = crtc_state-
> > > >cpu_transcoder;
> > >  
> > > -	psr_aux_io_power_get(intel_dp);
> > > -
> > >  	/* Only HSW and BDW have PSR AUX registers that need to be
> > > setup. SKL+
> > >  	 * use hardcoded values PSR AUX transactions
> > >  	 */
> > > @@ -717,8 +678,6 @@ static void hsw_psr_disable(struct intel_dp
> > > *intel_dp,
> > >  		else
> > >  			WARN_ON(I915_READ(EDP_PSR_CTL) &
> > > EDP_PSR_ENABLE);
> > >  	}
> > > -
> > > -	psr_aux_io_power_put(intel_dp);
> > >  }
> > >  
> > >  /**
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index de3a81034f77..2969787201ef 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -1824,6 +1824,7 @@ void intel_display_power_put(struct
> > > drm_i915_private *dev_priv,
> > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > >  #define GLK_DISPLAY_AUX_A_POWER_DOMAINS (		\
> > >  	BIT_ULL(POWER_DOMAIN_AUX_A) |		\
> > > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > >  #define GLK_DISPLAY_AUX_B_POWER_DOMAINS (		\
> > >  	BIT_ULL(POWER_DOMAIN_AUX_B) |		\
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/ddi: Get AUX power domain for DP main link too

[Manasi Navare]

On Tue, Jun 26, 2018 at 09:03:48PM +0300, Imre Deak wrote:
> On Tue, Jun 26, 2018 at 10:40:26AM -0700, Paulo Zanoni wrote:
> > Em Seg, 2018-06-25 às 16:55 -0700, Souza, Jose escreveu:
> > > On Thu, 2018-06-21 at 21:44 +0300, Imre Deak wrote:
> > > > So far we got an AUX power domain reference only for the duration
> > > > of
> > > > DP
> > > > AUX transfers. However, the following suggests that we also need
> > > > these
> > > > for main link functionality:
> > > > - The specification doesn't state whether it's needed or not for
> > > > main
> > > >   link functionality, but suggests that these power wells need to
> > > > be
> > > >   enabled already during display core initialization (Sequences to
> > > >   Initialize Display).
> > 
> > No, the specification states it's needed. Page "Sequences for
> > DisplayPort" (the ICL version, of course), step 1.b:
> > 
> > "TC: Note that AUX power is required for running main link."
> > 
> > Same for the HDMI page.
> 
> Yes, for ICL MG PHY ports (in all modes) it's clearly stated as I also
> wrote it below. The above is about the ICL combo PHY ports and the other
> platforms. For those the only hint is that enabling the AUX power wells
> is listed as a step in the 'Sequences to Initialize Display".
>

I tested this patch with the latest stepping with DP on Combo PHY port.
Without this patch it was giving me AUX timeouts and could never succeed with link
training. I needed this patch to successfully train the link and get display out
on Combo PHy Port B DP.
So,

Tested-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

> > 
> > > > - For PSR we need to keep the AUX power well enabled.
> > > > - On ICL combo PHY ports (non-TC) the AUX power well is needed for
> > > >   link training too: while the port is enabled with a DP link
> > > > training
> > > >   test pattern trying to toggle the AUX power well will time out.
> > > > - On ICL MG PHY ports (TC) the AUX power well is needed also for
> > > > main
> > > >   link functionality (both in DP and HDMI modes).
> > > > - Windows enables these power wells both for main and AUX lane
> > > >   functionality.
> > > > 
> > > > Based on the above take an AUX power reference for main link
> > > > functionality too. This makes a difference only on GEN10+ (GLK+)
> > > > platforms, where we have separate port specific AUX power wells.
> > > > 
> > > > For PSR we still need to distinguish between port A and the other
> > > > ports, since on port A DC states must stay enabled for main link
> > > > functionality, but DC states must be disabled for driver initiated
> > > > AUX transfers. So re-use the corresponding helper from intel_psr.c.
> > > > 
> > > > Since we take now a reference for main link functionality on all DP
> > > > ports we can forgo taking the separate power ref for PSR
> > > > functionality.
> > > > 
> > > > v2:
> > > > - Make sure DC states stay enabled when taking the ref on port A.
> > > >   (Ville)
> > > > 
> > > > v3: (Ville)
> > > > - Fix comment about logic for encoders without a crtc state and
> > > >   add FIXME note for a simplification to avoid calling
> > > > get_power_domains
> > > >   in such cases.
> > > > - Use intel_crtc_has_dp_encoder() instead
> > > > !intel_crtc_has_type(HDMI).
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > 
> > > The spec is not clear but this fix the "aux power well" enable
> > > timeouts
> > > that I was getting in aux B so looks like your interpretation is
> > > right.
> > > 
> > > Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_ddi.c        | 54
> > > > ++++++++++++++++++++++++++++++---
> > > >  drivers/gpu/drm/i915/intel_display.c    | 12 +++++++-
> > > >  drivers/gpu/drm/i915/intel_drv.h        |  3 +-
> > > >  drivers/gpu/drm/i915/intel_psr.c        | 41 ---------------------
> > > > ----
> > > >  drivers/gpu/drm/i915/intel_runtime_pm.c |  1 +
> > > >  5 files changed, 64 insertions(+), 47 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index 044fe1fb9872..0319825b725b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -1983,15 +1983,55 @@ bool intel_ddi_get_hw_state(struct
> > > > intel_encoder *encoder,
> > > >  	return ret;
> > > >  }
> > > >  
> > > > -static u64 intel_ddi_get_power_domains(struct intel_encoder
> > > > *encoder)
> > > > +static inline enum intel_display_power_domain
> > > > +intel_ddi_main_link_aux_domain(struct intel_dp *intel_dp)
> > > > +{
> > > > +	/* CNL HW requires corresponding AUX IOs to be powered up
> > > > for PSR with
> > > > +	 * DC states enabled at the same time, while for driver
> > > > initiated AUX
> > > > +	 * transfers we need the same AUX IOs to be powered but
> > > > with
> > > > DC states
> > > > +	 * disabled. Accordingly use the AUX power domain here
> > > > which
> > > > leaves DC
> > > > +	 * states enabled.
> > > > +	 * However, for non-A AUX ports the corresponding non-EDP
> > > > transcoders
> > > > +	 * would have already enabled power well 2 and DC_OFF.
> > > > This
> > > > means we can
> > > > +	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> > > > instead of a
> > > > +	 * specific AUX_IO reference without powering up any extra
> > > > wells.
> > > > +	 * Note that PSR is enabled only on Port A even though
> > > > this
> > > > function
> > > > +	 * returns the correct domain for other ports too.
> > > > +	 */
> > > > +	return intel_dp->aux_ch == AUX_CH_A ?
> > > > POWER_DOMAIN_AUX_IO_A
> > > > :
> > > > +					      intel_dp-
> > > > > aux_power_domain;
> > > > 
> > > > +}
> > > > +
> > > > +static u64 intel_ddi_get_power_domains(struct intel_encoder
> > > > *encoder,
> > > > +				       struct intel_crtc_state
> > > > *crtc_state)
> > > >  {
> > > >  	struct intel_digital_port *dig_port =
> > > > enc_to_dig_port(&encoder->base);
> > > >  	enum pipe pipe;
> > > > +	u64 domains;
> > > >  
> > > > -	if (intel_ddi_get_hw_state(encoder, &pipe))
> > > > -		return BIT_ULL(dig_port->ddi_io_power_domain);
> > > > +	if (!intel_ddi_get_hw_state(encoder, &pipe))
> > > > +		return 0;
> > > >  
> > > > -	return 0;
> > > > +	domains = BIT_ULL(dig_port->ddi_io_power_domain);
> > > > +	if (!crtc_state)
> > > > +		return domains;
> > > > +
> > > > +	/*
> > > > +	 * TODO: Add support for MST encoders. Atm, the following
> > > > should never
> > > > +	 * happen since this function will be called only for the
> > > > primary
> > > > +	 * encoder which doesn't have its own pipe/crtc_state.
> > > > +	 */
> > > > +	if (WARN_ON(intel_crtc_has_type(crtc_state,
> > > > INTEL_OUTPUT_DP_MST)))
> > > > +		return domains;
> > > > +
> > > > +	/* AUX power is only needed for (e)DP mode, not for HDMI.
> > > > */
> > > > +	if (intel_crtc_has_dp_encoder(crtc_state)) {
> > > > +		struct intel_dp *intel_dp = &dig_port->dp;
> > > > +
> > > > +		domains |=
> > > > BIT_ULL(intel_ddi_main_link_aux_domain(intel_dp));
> > > > +	}
> > > > +
> > > > +	return domains;
> > > >  }
> > > >  
> > > >  void intel_ddi_enable_pipe_clock(const struct intel_crtc_state
> > > > *crtc_state)
> > > > @@ -2631,6 +2671,9 @@ static void intel_ddi_pre_enable_dp(struct
> > > > intel_encoder *encoder,
> > > >  
> > > >  	WARN_ON(is_mst && (port == PORT_A || port == PORT_E));
> > > >  
> > > > +	intel_display_power_get(dev_priv,
> > > > +				intel_ddi_main_link_aux_domain(int
> > > > el
> > > > _dp));
> > > > +
> > > >  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> > > >  				 crtc_state->lane_count, is_mst);
> > > >  
> > > > @@ -2775,6 +2818,9 @@ static void intel_ddi_post_disable_dp(struct
> > > > intel_encoder *encoder,
> > > >  	intel_display_power_put(dev_priv, dig_port-
> > > > > ddi_io_power_domain);
> > > > 
> > > >  
> > > >  	intel_ddi_clk_disable(encoder);
> > > > +
> > > > +	intel_display_power_put(dev_priv,
> > > > +				intel_ddi_main_link_aux_domain(int
> > > > el
> > > > _dp));
> > > >  }
> > > >  
> > > >  static void intel_ddi_post_disable_hdmi(struct intel_encoder
> > > > *encoder,
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index 2c8fef3ede54..ce615f89b43a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct
> > > > drm_i915_private *dev_priv)
> > > >  	for_each_intel_encoder(&dev_priv->drm, encoder) {
> > > >  		u64 get_domains;
> > > >  		enum intel_display_power_domain domain;
> > > > +		struct intel_crtc_state *crtc_state;
> > > >  
> > > >  		if (!encoder->get_power_domains)
> > > >  			continue;
> > > >  
> > > > -		get_domains = encoder->get_power_domains(encoder);
> > > > +		/*
> > > > +		 * For MST and inactive encoders we don't have a
> > > > crtc state.
> > > > +		 * FIXME: no need to call get_power_domains in
> > > > such
> > > > cases, it
> > > > +		 * will always return 0.
> > > > +		 */
> > > > +		crtc_state = encoder->base.crtc ?
> > > > +			     to_intel_crtc_state(encoder-
> > > > >base.crtc-
> > > > > state) :
> > > > 
> > > > +			     NULL;
> > > > +
> > > > +		get_domains = encoder->get_power_domains(encoder,
> > > > crtc_state);
> > > >  		for_each_power_domain(domain, get_domains)
> > > >  			intel_display_power_get(dev_priv, domain);
> > > >  	}
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 0c3ac0eafde0..7174429d930a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -254,7 +254,8 @@ struct intel_encoder {
> > > >  			   struct intel_crtc_state *pipe_config);
> > > >  	/* Returns a mask of power domains that need to be
> > > > referenced as part
> > > >  	 * of the hardware state readout code. */
> > > > -	u64 (*get_power_domains)(struct intel_encoder *encoder);
> > > > +	u64 (*get_power_domains)(struct intel_encoder *encoder,
> > > > +				 struct intel_crtc_state
> > > > *crtc_state);
> > > >  	/*
> > > >  	 * Called during system suspend after all pending requests
> > > > for the
> > > >  	 * encoder are flushed (for example for DP AUX
> > > > transactions)
> > > > and
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index d4cd19fea148..eecdd8b5b5e0 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -56,43 +56,6 @@
> > > >  #include "intel_drv.h"
> > > >  #include "i915_drv.h"
> > > >  
> > > > -static inline enum intel_display_power_domain
> > > > -psr_aux_domain(struct intel_dp *intel_dp)
> > > > -{
> > > > -	/* CNL HW requires corresponding AUX IOs to be powered up
> > > > for PSR.
> > > > -	 * However, for non-A AUX ports the corresponding non-EDP
> > > > transcoders
> > > > -	 * would have already enabled power well 2 and DC_OFF.
> > > > This
> > > > means we can
> > > > -	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> > > > instead of a
> > > > -	 * specific AUX_IO reference without powering up any extra
> > > > wells.
> > > > -	 * Note that PSR is enabled only on Port A even though
> > > > this
> > > > function
> > > > -	 * returns the correct domain for other ports too.
> > > > -	 */
> > > > -	return intel_dp->aux_ch == AUX_CH_A ?
> > > > POWER_DOMAIN_AUX_IO_A
> > > > :
> > > > -					      intel_dp-
> > > > > aux_power_domain;
> > > > 
> > > > -}
> > > > -
> > > > -static void psr_aux_io_power_get(struct intel_dp *intel_dp)
> > > > -{
> > > > -	struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > -	struct drm_i915_private *dev_priv =
> > > > to_i915(intel_dig_port-
> > > > > base.base.dev);
> > > > 
> > > > -
> > > > -	if (INTEL_GEN(dev_priv) < 10)
> > > > -		return;
> > > > -
> > > > -	intel_display_power_get(dev_priv,
> > > > psr_aux_domain(intel_dp));
> > > > -}
> > > > -
> > > > -static void psr_aux_io_power_put(struct intel_dp *intel_dp)
> > > > -{
> > > > -	struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > -	struct drm_i915_private *dev_priv =
> > > > to_i915(intel_dig_port-
> > > > > base.base.dev);
> > > > 
> > > > -
> > > > -	if (INTEL_GEN(dev_priv) < 10)
> > > > -		return;
> > > > -
> > > > -	intel_display_power_put(dev_priv,
> > > > psr_aux_domain(intel_dp));
> > > > -}
> > > > -
> > > >  void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool
> > > > debug)
> > > >  {
> > > >  	u32 debug_mask, mask;
> > > > @@ -595,8 +558,6 @@ static void hsw_psr_enable_source(struct
> > > > intel_dp
> > > > *intel_dp,
> > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > >  	enum transcoder cpu_transcoder = crtc_state-
> > > > >cpu_transcoder;
> > > >  
> > > > -	psr_aux_io_power_get(intel_dp);
> > > > -
> > > >  	/* Only HSW and BDW have PSR AUX registers that need to be
> > > > setup. SKL+
> > > >  	 * use hardcoded values PSR AUX transactions
> > > >  	 */
> > > > @@ -717,8 +678,6 @@ static void hsw_psr_disable(struct intel_dp
> > > > *intel_dp,
> > > >  		else
> > > >  			WARN_ON(I915_READ(EDP_PSR_CTL) &
> > > > EDP_PSR_ENABLE);
> > > >  	}
> > > > -
> > > > -	psr_aux_io_power_put(intel_dp);
> > > >  }
> > > >  
> > > >  /**
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > index de3a81034f77..2969787201ef 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > @@ -1824,6 +1824,7 @@ void intel_display_power_put(struct
> > > > drm_i915_private *dev_priv,
> > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > >  #define GLK_DISPLAY_AUX_A_POWER_DOMAINS (		\
> > > >  	BIT_ULL(POWER_DOMAIN_AUX_A) |		\
> > > > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> > > >  	BIT_ULL(POWER_DOMAIN_INIT))
> > > >  #define GLK_DISPLAY_AUX_B_POWER_DOMAINS (		\
> > > >  	BIT_ULL(POWER_DOMAIN_AUX_B) |		\
> _______________________________________________
> 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