[PATCH 0/6] drm/i915/bxt: Fix DDI PHY setup for low resolutions

[PATCH 0/6] drm/i915/bxt: Fix DDI PHY setup for low resolutions

[Imre Deak]

There are two problems with the current way of enabling the DDI PHYs
during driver loading/resuming:
Relying on the HWs dynamic power gating may waste some power and part of
the PHY configuration is dependent on the mode specific DDI lane count.
To solve both of these issues split the PHY initialization, moving one
half of it to the power well code the other half to the modeset code,
similarly to the CHV code.

Kudos to Ville for explaining about the PHY power gating and other
quirks on CHV, it helped a lot to better understand the BXT PHY which is
quite similar.

This fixes modeset problems for modes with less than 4 lanes.

Imre Deak (6):
  drm/i915/bxt: Wait for PHY1 GRC calibration synchronously
  drm/i915: Factor out intel_power_well_get/put
  drm/i915/bxt: Move DDI PHY enabling/disabling to the power well code
  drm/i915/bxt: Set DDI PHY lane latency optimization during modeset
  drm/i915/bxt: Rename broxton to bxt in PHY/CDCLK function prefixes
  drm/i915/bxt: Sanitiy check the PHY lane power down status

 drivers/gpu/drm/i915/i915_reg.h         |  12 ++
 drivers/gpu/drm/i915/intel_ddi.c        | 222 ++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_display.c    |  33 +++--
 drivers/gpu/drm/i915/intel_drv.h        |  19 ++-
 drivers/gpu/drm/i915/intel_runtime_pm.c | 133 ++++++++++++++++---
 5 files changed, 291 insertions(+), 128 deletions(-)

-- 
2.5.0

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

[PATCH 1/6] drm/i915/bxt: Wait for PHY1 GRC calibration synchronously

[Imre Deak]

A follow-up patch moves the PHY enabling to the power well code where
enabling/disabling the PHYs will happen independently. Because of this
waiting for the GRC calibration in PHY1 asynchronously would need some
additional logic. Instead of adding that let's keep things simple for now
and wait synchronously. My measurements showed that the calibration
takes ~4ms.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 022b41d..b10c7b5 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1899,8 +1899,6 @@ static void broxton_phy_init(struct drm_i915_private *dev_priv,
 		 * the corresponding calibrated value from PHY1, and disable
 		 * the automatic calibration on PHY0.
 		 */
-		broxton_phy_wait_grc_done(dev_priv, DPIO_PHY1);
-
 		val = dev_priv->bxt_phy_grc = broxton_get_grc(dev_priv,
 							      DPIO_PHY1);
 		grc_code = val << GRC_CODE_FAST_SHIFT |
@@ -1912,14 +1910,13 @@ static void broxton_phy_init(struct drm_i915_private *dev_priv,
 		val |= GRC_DIS | GRC_RDY_OVRD;
 		I915_WRITE(BXT_PORT_REF_DW8(DPIO_PHY0), val);
 	}
-	/*
-	 * During PHY1 init delay waiting for GRC calibration to finish, since
-	 * it can happen in parallel with the subsequent PHY0 init.
-	 */
 
 	val = I915_READ(BXT_PHY_CTL_FAMILY(phy));
 	val |= COMMON_RESET_DIS;
 	I915_WRITE(BXT_PHY_CTL_FAMILY(phy), val);
+
+	if (phy == DPIO_PHY1)
+		broxton_phy_wait_grc_done(dev_priv, DPIO_PHY1);
 }
 
 void broxton_ddi_phy_init(struct drm_i915_private *dev_priv)
@@ -1927,12 +1924,6 @@ void broxton_ddi_phy_init(struct drm_i915_private *dev_priv)
 	/* Enable PHY1 first since it provides Rcomp for PHY0 */
 	broxton_phy_init(dev_priv, DPIO_PHY1);
 	broxton_phy_init(dev_priv, DPIO_PHY0);
-
-	/*
-	 * If BIOS enabled only PHY0 and not PHY1, we skipped waiting for the
-	 * PHY1 GRC calibration to finish, so wait for it here.
-	 */
-	broxton_phy_wait_grc_done(dev_priv, DPIO_PHY1);
 }
 
 static void broxton_phy_uninit(struct drm_i915_private *dev_priv,
-- 
2.5.0

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

[PATCH 2/6] drm/i915: Factor out intel_power_well_get/put

[Imre Deak]

These helpers will be needed by the next patch, so factor them out.

No functional change.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 2b75b30..d0d056a 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -151,6 +151,20 @@ static void intel_power_well_disable(struct drm_i915_private *dev_priv,
 	power_well->ops->disable(dev_priv, power_well);
 }
 
+static void intel_power_well_get(struct drm_i915_private *dev_priv,
+				 struct i915_power_well *power_well)
+{
+	if (!power_well->count++)
+		intel_power_well_enable(dev_priv, power_well);
+}
+
+static void intel_power_well_put(struct drm_i915_private *dev_priv,
+				 struct i915_power_well *power_well)
+{
+	if (!--power_well->count)
+		intel_power_well_disable(dev_priv, power_well);
+}
+
 /*
  * We should only use the power well if we explicitly asked the hardware to
  * enable it, so check if it's enabled and also check if we've requested it to
@@ -1518,10 +1532,8 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
 	struct i915_power_well *power_well;
 	int i;
 
-	for_each_power_well(i, power_well, BIT(domain), power_domains) {
-		if (!power_well->count++)
-			intel_power_well_enable(dev_priv, power_well);
-	}
+	for_each_power_well(i, power_well, BIT(domain), power_domains)
+		intel_power_well_get(dev_priv, power_well);
 
 	power_domains->domain_use_count[domain]++;
 }
@@ -1620,8 +1632,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 		     "Use count on power well %s is already zero",
 		     power_well->name);
 
-		if (!--power_well->count)
-			intel_power_well_disable(dev_priv, power_well);
+		intel_power_well_put(dev_priv, power_well);
 	}
 
 	mutex_unlock(&power_domains->lock);
-- 
2.5.0

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

Re: [PATCH 2/6] drm/i915: Factor out intel_power_well_get/put

[Ville Syrjälä]

On Tue, Jun 07, 2016 at 09:24:29PM +0300, Imre Deak wrote:
> These helpers will be needed by the next patch, so factor them out.
> 
> No functional change.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 2b75b30..d0d056a 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -151,6 +151,20 @@ static void intel_power_well_disable(struct drm_i915_private *dev_priv,
>  	power_well->ops->disable(dev_priv, power_well);
>  }
>  
> +static void intel_power_well_get(struct drm_i915_private *dev_priv,
> +				 struct i915_power_well *power_well)
> +{
> +	if (!power_well->count++)
> +		intel_power_well_enable(dev_priv, power_well);
> +}
> +
> +static void intel_power_well_put(struct drm_i915_private *dev_priv,
> +				 struct i915_power_well *power_well)
> +{
> +	if (!--power_well->count)
> +		intel_power_well_disable(dev_priv, power_well);
> +}
> +
>  /*
>   * We should only use the power well if we explicitly asked the hardware to
>   * enable it, so check if it's enabled and also check if we've requested it to
> @@ -1518,10 +1532,8 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
>  	struct i915_power_well *power_well;
>  	int i;
>  
> -	for_each_power_well(i, power_well, BIT(domain), power_domains) {
> -		if (!power_well->count++)
> -			intel_power_well_enable(dev_priv, power_well);
> -	}
> +	for_each_power_well(i, power_well, BIT(domain), power_domains)
> +		intel_power_well_get(dev_priv, power_well);
>  
>  	power_domains->domain_use_count[domain]++;
>  }
> @@ -1620,8 +1632,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  		     "Use count on power well %s is already zero",
>  		     power_well->name);

Move the WARN too?

>  
> -		if (!--power_well->count)
> -			intel_power_well_disable(dev_priv, power_well);
> +		intel_power_well_put(dev_priv, power_well);
>  	}
>  
>  	mutex_unlock(&power_domains->lock);
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH v2 2/6] drm/i915: Factor out intel_power_well_get/put

[Imre Deak]

These helpers will be needed by the next patch, so factor them out.

No functional change.

v2:
- Move the refcount==0 WARN to the new put helper. (Ville)

CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 2b75b30..10978cb 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -151,6 +151,23 @@ static void intel_power_well_disable(struct drm_i915_private *dev_priv,
 	power_well->ops->disable(dev_priv, power_well);
 }
 
+static void intel_power_well_get(struct drm_i915_private *dev_priv,
+				 struct i915_power_well *power_well)
+{
+	if (!power_well->count++)
+		intel_power_well_enable(dev_priv, power_well);
+}
+
+static void intel_power_well_put(struct drm_i915_private *dev_priv,
+				 struct i915_power_well *power_well)
+{
+	WARN(!power_well->count, "Use count on power well %s is already zero",
+	     power_well->name);
+
+	if (!--power_well->count)
+		intel_power_well_disable(dev_priv, power_well);
+}
+
 /*
  * We should only use the power well if we explicitly asked the hardware to
  * enable it, so check if it's enabled and also check if we've requested it to
@@ -1518,10 +1535,8 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
 	struct i915_power_well *power_well;
 	int i;
 
-	for_each_power_well(i, power_well, BIT(domain), power_domains) {
-		if (!power_well->count++)
-			intel_power_well_enable(dev_priv, power_well);
-	}
+	for_each_power_well(i, power_well, BIT(domain), power_domains)
+		intel_power_well_get(dev_priv, power_well);
 
 	power_domains->domain_use_count[domain]++;
 }
@@ -1615,14 +1630,8 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	     intel_display_power_domain_str(domain));
 	power_domains->domain_use_count[domain]--;
 
-	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
-		WARN(!power_well->count,
-		     "Use count on power well %s is already zero",
-		     power_well->name);
-
-		if (!--power_well->count)
-			intel_power_well_disable(dev_priv, power_well);
-	}
+	for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
+		intel_power_well_put(dev_priv, power_well);
 
 	mutex_unlock(&power_domains->lock);
 
-- 
2.5.0

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

[PATCH 3/6] drm/i915/bxt: Move DDI PHY enabling/disabling to the power well code

[Imre Deak]

So far we depended on the HW to dynamically power down unused PHYs and
so we enabled them manually once during driver loading/resuming. There
are indications however that we can achieve better power savings by
manual powering toggling. So make the PHY enabling/disabling to happen
on-demand whenever we need either the corresponding AUX or port
functionality. CHV does this already by enabling the PHY along the
corresponding PHY common lane power wells there, do the same on BXT by
adding virtual power wells for the same purpose.

Also sanity check the common lane power down ack signal from the PHY. Do
this only when the PHY is enabled, since it's not clear at what point
the HW power/clock gates things.

While at it rename broxton_ prefix to bxt_ in related function names to
better align with the SKL code.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         |   3 +
 drivers/gpu/drm/i915/intel_ddi.c        |  46 +++-----------
 drivers/gpu/drm/i915/intel_drv.h        |   9 ++-
 drivers/gpu/drm/i915/intel_runtime_pm.c | 106 +++++++++++++++++++++++++++++---
 4 files changed, 117 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8f0129d..81fc498 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -713,6 +713,9 @@ enum skl_disp_power_wells {
 	/* Not actual bit groups. Used as IDs for lookup_power_well() */
 	SKL_DISP_PW_ALWAYS_ON,
 	SKL_DISP_PW_DC_OFF,
+
+	BXT_DPIO_CMN_A,
+	BXT_DPIO_CMN_BC,
 };
 
 #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2))
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index b10c7b5..dee6dd0 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1742,8 +1742,8 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 	}
 }
 
-static bool broxton_phy_is_enabled(struct drm_i915_private *dev_priv,
-				   enum dpio_phy phy)
+bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
+			    enum dpio_phy phy)
 {
 	if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy)))
 		return false;
@@ -1787,21 +1787,17 @@ static void broxton_phy_wait_grc_done(struct drm_i915_private *dev_priv,
 		DRM_ERROR("timeout waiting for PHY%d GRC\n", phy);
 }
 
-static bool broxton_phy_verify_state(struct drm_i915_private *dev_priv,
-				     enum dpio_phy phy);
-
-static void broxton_phy_init(struct drm_i915_private *dev_priv,
-			     enum dpio_phy phy)
+void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
 {
 	enum port port;
 	u32 ports, val;
 
-	if (broxton_phy_is_enabled(dev_priv, phy)) {
+	if (bxt_ddi_phy_is_enabled(dev_priv, phy)) {
 		/* Still read out the GRC value for state verification */
 		if (phy == DPIO_PHY0)
 			dev_priv->bxt_phy_grc = broxton_get_grc(dev_priv, phy);
 
-		if (broxton_phy_verify_state(dev_priv, phy)) {
+		if (bxt_ddi_phy_verify_state(dev_priv, phy)) {
 			DRM_DEBUG_DRIVER("DDI PHY %d already enabled, "
 					 "won't reprogram it\n", phy);
 
@@ -1810,8 +1806,6 @@ static void broxton_phy_init(struct drm_i915_private *dev_priv,
 
 		DRM_DEBUG_DRIVER("DDI PHY %d enabled with invalid state, "
 				 "force reprogramming it\n", phy);
-	} else {
-		DRM_DEBUG_DRIVER("DDI PHY %d not enabled, enabling it\n", phy);
 	}
 
 	val = I915_READ(BXT_P_CR_GT_DISP_PWRON);
@@ -1919,15 +1913,7 @@ static void broxton_phy_init(struct drm_i915_private *dev_priv,
 		broxton_phy_wait_grc_done(dev_priv, DPIO_PHY1);
 }
 
-void broxton_ddi_phy_init(struct drm_i915_private *dev_priv)
-{
-	/* Enable PHY1 first since it provides Rcomp for PHY0 */
-	broxton_phy_init(dev_priv, DPIO_PHY1);
-	broxton_phy_init(dev_priv, DPIO_PHY0);
-}
-
-static void broxton_phy_uninit(struct drm_i915_private *dev_priv,
-			       enum dpio_phy phy)
+void bxt_ddi_phy_uninit(struct drm_i915_private *dev_priv, enum dpio_phy phy)
 {
 	uint32_t val;
 
@@ -1940,12 +1926,6 @@ static void broxton_phy_uninit(struct drm_i915_private *dev_priv,
 	I915_WRITE(BXT_P_CR_GT_DISP_PWRON, val);
 }
 
-void broxton_ddi_phy_uninit(struct drm_i915_private *dev_priv)
-{
-	broxton_phy_uninit(dev_priv, DPIO_PHY1);
-	broxton_phy_uninit(dev_priv, DPIO_PHY0);
-}
-
 static bool __printf(6, 7)
 __phy_reg_verify_state(struct drm_i915_private *dev_priv, enum dpio_phy phy,
 		       i915_reg_t reg, u32 mask, u32 expected,
@@ -1973,8 +1953,8 @@ __phy_reg_verify_state(struct drm_i915_private *dev_priv, enum dpio_phy phy,
 	return false;
 }
 
-static bool broxton_phy_verify_state(struct drm_i915_private *dev_priv,
-				     enum dpio_phy phy)
+bool bxt_ddi_phy_verify_state(struct drm_i915_private *dev_priv,
+			      enum dpio_phy phy)
 {
 	enum port port;
 	u32 ports;
@@ -1985,8 +1965,7 @@ static bool broxton_phy_verify_state(struct drm_i915_private *dev_priv,
 	__phy_reg_verify_state(dev_priv, phy, reg, mask, exp, fmt,	\
 			       ## __VA_ARGS__)
 
-	/* We expect the PHY to be always enabled */
-	if (!broxton_phy_is_enabled(dev_priv, phy))
+	if (!bxt_ddi_phy_is_enabled(dev_priv, phy))
 		return false;
 
 	ok = true;
@@ -2049,13 +2028,6 @@ static bool broxton_phy_verify_state(struct drm_i915_private *dev_priv,
 #undef _CHK
 }
 
-void broxton_ddi_phy_verify_state(struct drm_i915_private *dev_priv)
-{
-	if (!broxton_phy_verify_state(dev_priv, DPIO_PHY0) ||
-	    !broxton_phy_verify_state(dev_priv, DPIO_PHY1))
-		i915_report_error(dev_priv, "DDI PHY state mismatch\n");
-}
-
 void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ebe7b34..17445d7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1263,9 +1263,12 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv);
 void hsw_disable_pc8(struct drm_i915_private *dev_priv);
 void broxton_init_cdclk(struct drm_i915_private *dev_priv);
 void broxton_uninit_cdclk(struct drm_i915_private *dev_priv);
-void broxton_ddi_phy_init(struct drm_i915_private *dev_priv);
-void broxton_ddi_phy_uninit(struct drm_i915_private *dev_priv);
-void broxton_ddi_phy_verify_state(struct drm_i915_private *dev_priv);
+void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy);
+void bxt_ddi_phy_uninit(struct drm_i915_private *dev_priv, enum dpio_phy phy);
+bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
+			    enum dpio_phy phy);
+bool bxt_ddi_phy_verify_state(struct drm_i915_private *dev_priv,
+			      enum dpio_phy phy);
 void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv);
 void bxt_enable_dc9(struct drm_i915_private *dev_priv);
 void bxt_disable_dc9(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index d0d056a..1dd937a 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -65,6 +65,9 @@
 bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
 				    int power_well_id);
 
+static struct i915_power_well *
+lookup_power_well(struct drm_i915_private *dev_priv, int power_well_id);
+
 const char *
 intel_display_power_domain_str(enum intel_display_power_domain domain)
 {
@@ -433,6 +436,16 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
 	BIT(POWER_DOMAIN_MODESET) |			\
 	BIT(POWER_DOMAIN_AUX_A) |			\
 	BIT(POWER_DOMAIN_INIT))
+#define BXT_DPIO_CMN_A_POWER_DOMAINS (			\
+	BIT(POWER_DOMAIN_PORT_DDI_A_LANES) |		\
+	BIT(POWER_DOMAIN_AUX_A) |			\
+	BIT(POWER_DOMAIN_INIT))
+#define BXT_DPIO_CMN_BC_POWER_DOMAINS (			\
+	BIT(POWER_DOMAIN_PORT_DDI_B_LANES) |		\
+	BIT(POWER_DOMAIN_PORT_DDI_C_LANES) |		\
+	BIT(POWER_DOMAIN_AUX_B) |			\
+	BIT(POWER_DOMAIN_AUX_C) |			\
+	BIT(POWER_DOMAIN_INIT))
 
 static void assert_can_enable_dc9(struct drm_i915_private *dev_priv)
 {
@@ -814,6 +827,72 @@ static void skl_power_well_disable(struct drm_i915_private *dev_priv,
 	skl_set_power_well(dev_priv, power_well, false);
 }
 
+static enum dpio_phy bxt_power_well_to_phy(struct i915_power_well *power_well)
+{
+	enum skl_disp_power_wells power_well_id = power_well->data;
+
+	return power_well_id == BXT_DPIO_CMN_A ? DPIO_PHY1 : DPIO_PHY0;
+}
+
+static void bxt_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
+					   struct i915_power_well *power_well)
+{
+	enum skl_disp_power_wells power_well_id = power_well->data;
+	struct i915_power_well *cmn_a_well;
+
+	if (power_well_id == BXT_DPIO_CMN_BC) {
+		/*
+		 * We need to copy the GRC calibration value from the eDP PHY,
+		 * so make sure it's powered up.
+		 */
+		cmn_a_well = lookup_power_well(dev_priv, BXT_DPIO_CMN_A);
+		intel_power_well_get(dev_priv, cmn_a_well);
+	}
+
+	bxt_ddi_phy_init(dev_priv, bxt_power_well_to_phy(power_well));
+
+	if (power_well_id == BXT_DPIO_CMN_BC)
+		intel_power_well_put(dev_priv, cmn_a_well);
+}
+
+static void bxt_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
+					    struct i915_power_well *power_well)
+{
+	bxt_ddi_phy_uninit(dev_priv, bxt_power_well_to_phy(power_well));
+}
+
+static bool bxt_dpio_cmn_power_well_enabled(struct drm_i915_private *dev_priv,
+					    struct i915_power_well *power_well)
+{
+	return bxt_ddi_phy_is_enabled(dev_priv,
+				      bxt_power_well_to_phy(power_well));
+}
+
+static void bxt_dpio_cmn_power_well_sync_hw(struct drm_i915_private *dev_priv,
+					    struct i915_power_well *power_well)
+{
+	if (power_well->count > 0)
+		bxt_dpio_cmn_power_well_enable(dev_priv, power_well);
+	else
+		bxt_dpio_cmn_power_well_disable(dev_priv, power_well);
+}
+
+
+static void bxt_verify_ddi_phy_power_wells(struct drm_i915_private *dev_priv)
+{
+	struct i915_power_well *power_well;
+
+	power_well = lookup_power_well(dev_priv, BXT_DPIO_CMN_A);
+	if (power_well->count > 0)
+		bxt_ddi_phy_verify_state(dev_priv,
+					 bxt_power_well_to_phy(power_well));
+
+	power_well = lookup_power_well(dev_priv, BXT_DPIO_CMN_BC);
+	if (power_well->count > 0)
+		bxt_ddi_phy_verify_state(dev_priv,
+					 bxt_power_well_to_phy(power_well));
+}
+
 static bool gen9_dc_off_power_well_enabled(struct drm_i915_private *dev_priv,
 					   struct i915_power_well *power_well)
 {
@@ -840,7 +919,7 @@ static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
 	gen9_assert_dbuf_enabled(dev_priv);
 
 	if (IS_BROXTON(dev_priv))
-		broxton_ddi_phy_verify_state(dev_priv);
+		bxt_verify_ddi_phy_power_wells(dev_priv);
 }
 
 static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
@@ -1804,6 +1883,13 @@ static const struct i915_power_well_ops gen9_dc_off_power_well_ops = {
 	.is_enabled = gen9_dc_off_power_well_enabled,
 };
 
+static const struct i915_power_well_ops bxt_dpio_cmn_power_well_ops = {
+	.sync_hw = bxt_dpio_cmn_power_well_sync_hw,
+	.enable = bxt_dpio_cmn_power_well_enable,
+	.disable = bxt_dpio_cmn_power_well_disable,
+	.is_enabled = bxt_dpio_cmn_power_well_enabled,
+};
+
 static struct i915_power_well hsw_power_wells[] = {
 	{
 		.name = "always-on",
@@ -2040,6 +2126,18 @@ static struct i915_power_well bxt_power_wells[] = {
 		.ops = &skl_power_well_ops,
 		.data = SKL_DISP_PW_2,
 	},
+	{
+		.name = "dpio-common-a",
+		.domains = BXT_DPIO_CMN_A_POWER_DOMAINS,
+		.ops = &bxt_dpio_cmn_power_well_ops,
+		.data = BXT_DPIO_CMN_A,
+	},
+	{
+		.name = "dpio-common-bc",
+		.domains = BXT_DPIO_CMN_BC_POWER_DOMAINS,
+		.ops = &bxt_dpio_cmn_power_well_ops,
+		.data = BXT_DPIO_CMN_BC,
+	},
 };
 
 static int
@@ -2309,10 +2407,6 @@ void bxt_display_core_init(struct drm_i915_private *dev_priv,
 
 	gen9_dbuf_enable(dev_priv);
 
-	broxton_ddi_phy_init(dev_priv);
-
-	broxton_ddi_phy_verify_state(dev_priv);
-
 	if (resume && dev_priv->csr.dmc_payload)
 		intel_csr_load_program(dev_priv);
 }
@@ -2324,8 +2418,6 @@ void bxt_display_core_uninit(struct drm_i915_private *dev_priv)
 
 	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
 
-	broxton_ddi_phy_uninit(dev_priv);
-
 	gen9_dbuf_disable(dev_priv);
 
 	broxton_uninit_cdclk(dev_priv);
-- 
2.5.0

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

[PATCH 4/6] drm/i915/bxt: Set DDI PHY lane latency optimization during modeset

[Imre Deak]

So far we configured a static lane latency optimization during driver
loading/resuming. The specification changed at one point and now this
configuration depends on the lane count, so move the configuration
to modeset time accordingly.

It's not clear when this lane configuration takes effect. The
specification only requires that the programming is done before enabling
the port. On CHV OTOH the lanes start to power up already right after
enabling the PLL. To be safe preserve the current order and set things
up already before enabling the PLL.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95476
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     | 131 +++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_display.c |   5 ++
 drivers/gpu/drm/i915/intel_drv.h     |   6 ++
 3 files changed, 99 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index dee6dd0..f46117a 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1789,8 +1789,7 @@ static void broxton_phy_wait_grc_done(struct drm_i915_private *dev_priv,
 
 void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
 {
-	enum port port;
-	u32 ports, val;
+	u32 val;
 
 	if (bxt_ddi_phy_is_enabled(dev_priv, phy)) {
 		/* Still read out the GRC value for state verification */
@@ -1825,28 +1824,6 @@ void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
 		DRM_ERROR("timeout during PHY%d power on\n", phy);
 	}
 
-	if (phy == DPIO_PHY0)
-		ports = BIT(PORT_B) | BIT(PORT_C);
-	else
-		ports = BIT(PORT_A);
-
-	for_each_port_masked(port, ports) {
-		int lane;
-
-		for (lane = 0; lane < 4; lane++) {
-			val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
-			/*
-			 * Note that on CHV this flag is called UPAR, but has
-			 * the same function.
-			 */
-			val &= ~LATENCY_OPTIM;
-			if (lane != 1)
-				val |= LATENCY_OPTIM;
-
-			I915_WRITE(BXT_PORT_TX_DW14_LN(port, lane), val);
-		}
-	}
-
 	/* Program PLL Rcomp code offset */
 	val = I915_READ(BXT_PORT_CL1CM_DW9(phy));
 	val &= ~IREF0RC_OFFSET_MASK;
@@ -1956,8 +1933,6 @@ __phy_reg_verify_state(struct drm_i915_private *dev_priv, enum dpio_phy phy,
 bool bxt_ddi_phy_verify_state(struct drm_i915_private *dev_priv,
 			      enum dpio_phy phy)
 {
-	enum port port;
-	u32 ports;
 	uint32_t mask;
 	bool ok;
 
@@ -1970,21 +1945,6 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private *dev_priv,
 
 	ok = true;
 
-	if (phy == DPIO_PHY0)
-		ports = BIT(PORT_B) | BIT(PORT_C);
-	else
-		ports = BIT(PORT_A);
-
-	for_each_port_masked(port, ports) {
-		int lane;
-
-		for (lane = 0; lane < 4; lane++)
-			ok &= _CHK(BXT_PORT_TX_DW14_LN(port, lane),
-				    LATENCY_OPTIM,
-				    lane != 1 ? LATENCY_OPTIM : 0,
-				    "BXT_PORT_TX_DW14_LN(%d, %d)", port, lane);
-	}
-
 	/* PLL Rcomp code offset */
 	ok &= _CHK(BXT_PORT_CL1CM_DW9(phy),
 		    IREF0RC_OFFSET_MASK, 0xe4 << IREF0RC_OFFSET_SHIFT,
@@ -2028,6 +1988,75 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private *dev_priv,
 #undef _CHK
 }
 
+static uint8_t
+bxt_ddi_phy_calc_lane_lat_optim_mask(struct intel_encoder *encoder,
+				     struct intel_crtc_state *pipe_config)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	int optim_lane_count;
+
+	switch (pipe_config->lane_count) {
+	case 1:
+		optim_lane_count = 0;
+		break;
+	case 2:
+		optim_lane_count = 3;
+		break;
+	case 4:
+		optim_lane_count = 4;
+		break;
+	default:
+		MISSING_CASE(intel_crtc->config->lane_count);
+
+		return 0;
+	}
+
+	return (BIT(optim_lane_count) - 1) & ~BIT(1);
+}
+
+static void bxt_ddi_pre_pll_enable(struct intel_encoder *encoder)
+{
+	struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
+	struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev);
+	enum port port = dport->port;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	int lane;
+
+	for (lane = 0; lane < 4; lane++) {
+		u32 val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
+
+		/*
+		 * Note that on CHV this flag is called UPAR, but has
+		 * the same function.
+		 */
+		val &= ~LATENCY_OPTIM;
+		if (intel_crtc->config->lane_lat_optim_mask & BIT(lane))
+			val |= LATENCY_OPTIM;
+
+		I915_WRITE(BXT_PORT_TX_DW14_LN(port, lane), val);
+	}
+}
+
+static uint8_t
+bxt_ddi_phy_get_lane_lat_optim_mask(struct intel_encoder *encoder)
+{
+	struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
+	struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev);
+	enum port port = dport->port;
+	int lane;
+	uint8_t mask;
+
+	mask = 0;
+	for (lane = 0; lane < 4; lane++) {
+		u32 val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
+
+		if (val & LATENCY_OPTIM)
+			mask |= BIT(lane);
+	}
+
+	return mask;
+}
+
 void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -2199,13 +2228,19 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
 	}
 
 	intel_ddi_clock_get(encoder, pipe_config);
+
+	if (IS_BROXTON(dev_priv))
+		pipe_config->lane_lat_optim_mask =
+			bxt_ddi_phy_get_lane_lat_optim_mask(encoder);
 }
 
 static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 				     struct intel_crtc_state *pipe_config)
 {
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
 	int type = encoder->type;
 	int port = intel_ddi_get_encoder_port(encoder);
+	int ret;
 
 	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
 
@@ -2213,9 +2248,17 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 		pipe_config->cpu_transcoder = TRANSCODER_EDP;
 
 	if (type == INTEL_OUTPUT_HDMI)
-		return intel_hdmi_compute_config(encoder, pipe_config);
+		ret = intel_hdmi_compute_config(encoder, pipe_config);
 	else
-		return intel_dp_compute_config(encoder, pipe_config);
+		ret = intel_dp_compute_config(encoder, pipe_config);
+
+	if (IS_BROXTON(dev_priv) && ret)
+		pipe_config->lane_lat_optim_mask =
+			bxt_ddi_phy_calc_lane_lat_optim_mask(encoder,
+							     pipe_config);
+
+	return ret;
+
 }
 
 static const struct drm_encoder_funcs intel_ddi_funcs = {
@@ -2314,6 +2357,8 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 
 	intel_encoder->compute_config = intel_ddi_compute_config;
 	intel_encoder->enable = intel_enable_ddi;
+	if (IS_BROXTON(dev_priv))
+		intel_encoder->pre_pll_enable = bxt_ddi_pre_pll_enable;
 	intel_encoder->pre_enable = intel_ddi_pre_enable;
 	intel_encoder->disable = intel_disable_ddi;
 	intel_encoder->post_disable = intel_ddi_post_disable;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 60cba19..4bc7c48 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4841,6 +4841,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
 						      false);
 
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		if (encoder->pre_pll_enable)
+			encoder->pre_pll_enable(encoder);
+
 	if (intel_crtc->config->shared_dpll)
 		intel_enable_shared_dpll(intel_crtc);
 
@@ -12816,6 +12820,7 @@ intel_pipe_config_compare(struct drm_device *dev,
 
 	PIPE_CONF_CHECK_I(has_dp_encoder);
 	PIPE_CONF_CHECK_I(lane_count);
+	PIPE_CONF_CHECK_X(lane_lat_optim_mask);
 
 	if (INTEL_INFO(dev)->gen < 8) {
 		PIPE_CONF_CHECK_M_N(dp_m_n);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 17445d7..ca98416 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -579,6 +579,12 @@ struct intel_crtc_state {
 
 	uint8_t lane_count;
 
+	/*
+	 * Used by platforms having DP/HDMI PHY with programmable lane
+	 * latency optimization.
+	 */
+	uint8_t lane_lat_optim_mask;
+
 	/* Panel fitter controls for gen2-gen4 + VLV */
 	struct {
 		u32 control;
-- 
2.5.0

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

Re: [PATCH 4/6] drm/i915/bxt: Set DDI PHY lane latency optimization during modeset

[Ander Conselvan De Oliveira]

On Tue, 2016-06-07 at 21:24 +0300, Imre Deak wrote:
> So far we configured a static lane latency optimization during driver
> loading/resuming. The specification changed at one point and now this
> configuration depends on the lane count, so move the configuration
> to modeset time accordingly.
> 
> It's not clear when this lane configuration takes effect. The
> specification only requires that the programming is done before enabling
> the port. On CHV OTOH the lanes start to power up already right after
> enabling the PLL. To be safe preserve the current order and set things
> up already before enabling the PLL.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95476
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     | 131 +++++++++++++++++++++++-----------
> -
>  drivers/gpu/drm/i915/intel_display.c |   5 ++
>  drivers/gpu/drm/i915/intel_drv.h     |   6 ++
>  3 files changed, 99 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index dee6dd0..f46117a 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1789,8 +1789,7 @@ static void broxton_phy_wait_grc_done(struct
> drm_i915_private *dev_priv,
>  
>  void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
>  {
> -	enum port port;
> -	u32 ports, val;
> +	u32 val;
>  
>  	if (bxt_ddi_phy_is_enabled(dev_priv, phy)) {
>  		/* Still read out the GRC value for state verification */
> @@ -1825,28 +1824,6 @@ void bxt_ddi_phy_init(struct drm_i915_private
> *dev_priv, enum dpio_phy phy)
>  		DRM_ERROR("timeout during PHY%d power on\n", phy);
>  	}
>  
> -	if (phy == DPIO_PHY0)
> -		ports = BIT(PORT_B) | BIT(PORT_C);
> -	else
> -		ports = BIT(PORT_A);
> -
> -	for_each_port_masked(port, ports) {
> -		int lane;
> -
> -		for (lane = 0; lane < 4; lane++) {
> -			val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
> -			/*
> -			 * Note that on CHV this flag is called UPAR, but has
> -			 * the same function.
> -			 */
> -			val &= ~LATENCY_OPTIM;
> -			if (lane != 1)
> -				val |= LATENCY_OPTIM;
> -
> -			I915_WRITE(BXT_PORT_TX_DW14_LN(port, lane), val);
> -		}
> -	}
> -
>  	/* Program PLL Rcomp code offset */
>  	val = I915_READ(BXT_PORT_CL1CM_DW9(phy));
>  	val &= ~IREF0RC_OFFSET_MASK;
> @@ -1956,8 +1933,6 @@ __phy_reg_verify_state(struct drm_i915_private
> *dev_priv, enum dpio_phy phy,
>  bool bxt_ddi_phy_verify_state(struct drm_i915_private *dev_priv,
>  			      enum dpio_phy phy)
>  {
> -	enum port port;
> -	u32 ports;
>  	uint32_t mask;
>  	bool ok;
>  
> @@ -1970,21 +1945,6 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private
> *dev_priv,
>  
>  	ok = true;
>  
> -	if (phy == DPIO_PHY0)
> -		ports = BIT(PORT_B) | BIT(PORT_C);
> -	else
> -		ports = BIT(PORT_A);
> -
> -	for_each_port_masked(port, ports) {
> -		int lane;
> -
> -		for (lane = 0; lane < 4; lane++)
> -			ok &= _CHK(BXT_PORT_TX_DW14_LN(port, lane),
> -				    LATENCY_OPTIM,
> -				    lane != 1 ? LATENCY_OPTIM : 0,
> -				    "BXT_PORT_TX_DW14_LN(%d, %d)", port,
> lane);
> -	}
> -
>  	/* PLL Rcomp code offset */
>  	ok &= _CHK(BXT_PORT_CL1CM_DW9(phy),
>  		    IREF0RC_OFFSET_MASK, 0xe4 << IREF0RC_OFFSET_SHIFT,
> @@ -2028,6 +1988,75 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private
> *dev_priv,
>  #undef _CHK
>  }
>  
> +static uint8_t
> +bxt_ddi_phy_calc_lane_lat_optim_mask(struct intel_encoder *encoder,
> +				     struct intel_crtc_state *pipe_config)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	int optim_lane_count;
> +
> +	switch (pipe_config->lane_count) {
> +	case 1:
> +		optim_lane_count = 0;
> +		break;
> +	case 2:
> +		optim_lane_count = 3;
> +		break;
> +	case 4:
> +		optim_lane_count = 4;
> +		break;
> +	default:
> +		MISSING_CASE(intel_crtc->config->lane_count);

Since this is called from compute config, pipe_config and intel_crtc->config
will be different. But anyway, I think this belongs in the dpll code. See below.

> +
> +		return 0;
> +	}
> +
> +	return (BIT(optim_lane_count) - 1) & ~BIT(1);

Wouldn't it be easier to just return the right mask from the switch? There's
already a case for each possible lane count.

> +}
> +
> +static void bxt_ddi_pre_pll_enable(struct intel_encoder *encoder)
> +{
> +	struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
> +	struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev);
> +	enum port port = dport->port;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	int lane;
> +
> +	for (lane = 0; lane < 4; lane++) {
> +		u32 val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
> +
> +		/*
> +		 * Note that on CHV this flag is called UPAR, but has
> +		 * the same function.
> +		 */
> +		val &= ~LATENCY_OPTIM;
> +		if (intel_crtc->config->lane_lat_optim_mask & BIT(lane))
> +			val |= LATENCY_OPTIM;
> +
> +		I915_WRITE(BXT_PORT_TX_DW14_LN(port, lane), val);
> +	}
> +}
> +
> +static uint8_t
> +bxt_ddi_phy_get_lane_lat_optim_mask(struct intel_encoder *encoder)
> +{
> +	struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
> +	struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev);
> +	enum port port = dport->port;
> +	int lane;
> +	uint8_t mask;
> +
> +	mask = 0;
> +	for (lane = 0; lane < 4; lane++) {
> +		u32 val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
> +
> +		if (val & LATENCY_OPTIM)
> +			mask |= BIT(lane);
> +	}
> +
> +	return mask;
> +}
> +
>  void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> @@ -2199,13 +2228,19 @@ void intel_ddi_get_config(struct intel_encoder
> *encoder,
>  	}
>  
>  	intel_ddi_clock_get(encoder, pipe_config);
> +
> +	if (IS_BROXTON(dev_priv))
> +		pipe_config->lane_lat_optim_mask =
> +			bxt_ddi_phy_get_lane_lat_optim_mask(encoder);
>  }
>  
>  static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>  				     struct intel_crtc_state *pipe_config)
>  {
> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>  	int type = encoder->type;
>  	int port = intel_ddi_get_encoder_port(encoder);
> +	int ret;
>  
>  	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown
> output!\n");
>  
> @@ -2213,9 +2248,17 @@ static bool intel_ddi_compute_config(struct
> intel_encoder *encoder,
>  		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>  
>  	if (type == INTEL_OUTPUT_HDMI)
> -		return intel_hdmi_compute_config(encoder, pipe_config);
> +		ret = intel_hdmi_compute_config(encoder, pipe_config);
>  	else
> -		return intel_dp_compute_config(encoder, pipe_config);
> +		ret = intel_dp_compute_config(encoder, pipe_config);
> +
> +	if (IS_BROXTON(dev_priv) && ret)
> +		pipe_config->lane_lat_optim_mask =
> +			bxt_ddi_phy_calc_lane_lat_optim_mask(encoder,
> +							     pipe_config);
> +
> +	return ret;
> +
>  }
>  
>  static const struct drm_encoder_funcs intel_ddi_funcs = {
> @@ -2314,6 +2357,8 @@ void intel_ddi_init(struct drm_device *dev, enum port
> port)
>  
>  	intel_encoder->compute_config = intel_ddi_compute_config;
>  	intel_encoder->enable = intel_enable_ddi;
> +	if (IS_BROXTON(dev_priv))
> +		intel_encoder->pre_pll_enable = bxt_ddi_pre_pll_enable;
>  	intel_encoder->pre_enable = intel_ddi_pre_enable;
>  	intel_encoder->disable = intel_disable_ddi;
>  	intel_encoder->post_disable = intel_ddi_post_disable;
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 60cba19..4bc7c48 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4841,6 +4841,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
>  						      false);
>  
> +	for_each_encoder_on_crtc(dev, crtc, encoder)
> +		if (encoder->pre_pll_enable)
> +			encoder->pre_pll_enable(encoder);
> +
>  	if (intel_crtc->config->shared_dpll)
>  		intel_enable_shared_dpll(intel_crtc);

Effectively this writes BXT_PORT_TX_DW14_LN before bxt_ddi_pll_enable() is
called. If that would be the first register write in that function, it would
avoid the extra detour. So we can calculate the optimization in bxt_get_dpll(),
store it in struct intel_dpll_hw_state and use the pll state checking to verify
the right values were programmed. That way there's no need to add a value
specific to the bxt phy to struct intel_crtc_state or implement the
pre_pll_enable hook.

Ander

>  
> @@ -12816,6 +12820,7 @@ intel_pipe_config_compare(struct drm_device *dev,
>  
>  	PIPE_CONF_CHECK_I(has_dp_encoder);
>  	PIPE_CONF_CHECK_I(lane_count);
> +	PIPE_CONF_CHECK_X(lane_lat_optim_mask);
>  
>  	if (INTEL_INFO(dev)->gen < 8) {
>  		PIPE_CONF_CHECK_M_N(dp_m_n);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 17445d7..ca98416 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -579,6 +579,12 @@ struct intel_crtc_state {
>  
>  	uint8_t lane_count;
>  
> +	/*
> +	 * Used by platforms having DP/HDMI PHY with programmable lane
> +	 * latency optimization.
> +	 */
> +	uint8_t lane_lat_optim_mask;
> +
>  	/* Panel fitter controls for gen2-gen4 + VLV */
>  	struct {
>  		u32 control;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/i915/bxt: Set DDI PHY lane latency optimization during modeset

[Imre Deak]

On ke, 2016-06-08 at 09:41 +0300, Ander Conselvan De Oliveira wrote:
> On Tue, 2016-06-07 at 21:24 +0300, Imre Deak wrote:
> > So far we configured a static lane latency optimization during driver
> > loading/resuming. The specification changed at one point and now this
> > configuration depends on the lane count, so move the configuration
> > to modeset time accordingly.
> > 
> > It's not clear when this lane configuration takes effect. The
> > specification only requires that the programming is done before enabling
> > the port. On CHV OTOH the lanes start to power up already right after
> > enabling the PLL. To be safe preserve the current order and set things
> > up already before enabling the PLL.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95476
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c     | 131 +++++++++++++++++++++++-----------
> > -
> >  drivers/gpu/drm/i915/intel_display.c |   5 ++
> >  drivers/gpu/drm/i915/intel_drv.h     |   6 ++
> >  3 files changed, 99 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index dee6dd0..f46117a 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1789,8 +1789,7 @@ static void broxton_phy_wait_grc_done(struct
> > drm_i915_private *dev_priv,
> >  
> >  void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
> >  {
> > -	enum port port;
> > -	u32 ports, val;
> > +	u32 val;
> >  
> >  	if (bxt_ddi_phy_is_enabled(dev_priv, phy)) {
> >  		/* Still read out the GRC value for state verification */
> > @@ -1825,28 +1824,6 @@ void bxt_ddi_phy_init(struct drm_i915_private
> > *dev_priv, enum dpio_phy phy)
> >  		DRM_ERROR("timeout during PHY%d power on\n", phy);
> >  	}
> >  
> > -	if (phy == DPIO_PHY0)
> > -		ports = BIT(PORT_B) | BIT(PORT_C);
> > -	else
> > -		ports = BIT(PORT_A);
> > -
> > -	for_each_port_masked(port, ports) {
> > -		int lane;
> > -
> > -		for (lane = 0; lane < 4; lane++) {
> > -			val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
> > -			/*
> > -			 * Note that on CHV this flag is called UPAR, but has
> > -			 * the same function.
> > -			 */
> > -			val &= ~LATENCY_OPTIM;
> > -			if (lane != 1)
> > -				val |= LATENCY_OPTIM;
> > -
> > -			I915_WRITE(BXT_PORT_TX_DW14_LN(port, lane), val);
> > -		}
> > -	}
> > -
> >  	/* Program PLL Rcomp code offset */
> >  	val = I915_READ(BXT_PORT_CL1CM_DW9(phy));
> >  	val &= ~IREF0RC_OFFSET_MASK;
> > @@ -1956,8 +1933,6 @@ __phy_reg_verify_state(struct drm_i915_private
> > *dev_priv, enum dpio_phy phy,
> >  bool bxt_ddi_phy_verify_state(struct drm_i915_private *dev_priv,
> >  			      enum dpio_phy phy)
> >  {
> > -	enum port port;
> > -	u32 ports;
> >  	uint32_t mask;
> >  	bool ok;
> >  
> > @@ -1970,21 +1945,6 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private
> > *dev_priv,
> >  
> >  	ok = true;
> >  
> > -	if (phy == DPIO_PHY0)
> > -		ports = BIT(PORT_B) | BIT(PORT_C);
> > -	else
> > -		ports = BIT(PORT_A);
> > -
> > -	for_each_port_masked(port, ports) {
> > -		int lane;
> > -
> > -		for (lane = 0; lane < 4; lane++)
> > -			ok &= _CHK(BXT_PORT_TX_DW14_LN(port, lane),
> > -				    LATENCY_OPTIM,
> > -				    lane != 1 ? LATENCY_OPTIM : 0,
> > -				    "BXT_PORT_TX_DW14_LN(%d, %d)", port,
> > lane);
> > -	}
> > -
> >  	/* PLL Rcomp code offset */
> >  	ok &= _CHK(BXT_PORT_CL1CM_DW9(phy),
> >  		    IREF0RC_OFFSET_MASK, 0xe4 << IREF0RC_OFFSET_SHIFT,
> > @@ -2028,6 +1988,75 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private
> > *dev_priv,
> >  #undef _CHK
> >  }
> >  
> > +static uint8_t
> > +bxt_ddi_phy_calc_lane_lat_optim_mask(struct intel_encoder *encoder,
> > +				     struct intel_crtc_state *pipe_config)
> > +{
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> > +	int optim_lane_count;
> > +
> > +	switch (pipe_config->lane_count) {
> > +	case 1:
> > +		optim_lane_count = 0;
> > +		break;
> > +	case 2:
> > +		optim_lane_count = 3;
> > +		break;
> > +	case 4:
> > +		optim_lane_count = 4;
> > +		break;
> > +	default:
> > +		MISSING_CASE(intel_crtc->config->lane_count);
> 
> Since this is called from compute config, pipe_config and intel_crtc->config
> will be different. But anyway, I think this belongs in the dpll code. See below.

Yep, this is a typo left-over before I realized that the two things are
different:) Thanks for catching it.

> 
> > +
> > +		return 0;
> > +	}
> > +
> > +	return (BIT(optim_lane_count) - 1) & ~BIT(1);
> 
> Wouldn't it be easier to just return the right mask from the switch? There's
> already a case for each possible lane count.

Ok, will simplify it.

> > +}
> > +
> > +static void bxt_ddi_pre_pll_enable(struct intel_encoder *encoder)
> > +{
> > +	struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
> > +	struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev);
> > +	enum port port = dport->port;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> > +	int lane;
> > +
> > +	for (lane = 0; lane < 4; lane++) {
> > +		u32 val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
> > +
> > +		/*
> > +		 * Note that on CHV this flag is called UPAR, but has
> > +		 * the same function.
> > +		 */
> > +		val &= ~LATENCY_OPTIM;
> > +		if (intel_crtc->config->lane_lat_optim_mask & BIT(lane))
> > +			val |= LATENCY_OPTIM;
> > +
> > +		I915_WRITE(BXT_PORT_TX_DW14_LN(port, lane), val);
> > +	}
> > +}
> > +
> > +static uint8_t
> > +bxt_ddi_phy_get_lane_lat_optim_mask(struct intel_encoder *encoder)
> > +{
> > +	struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
> > +	struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev);
> > +	enum port port = dport->port;
> > +	int lane;
> > +	uint8_t mask;
> > +
> > +	mask = 0;
> > +	for (lane = 0; lane < 4; lane++) {
> > +		u32 val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
> > +
> > +		if (val & LATENCY_OPTIM)
> > +			mask |= BIT(lane);
> > +	}
> > +
> > +	return mask;
> > +}
> > +
> >  void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
> >  {
> >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > @@ -2199,13 +2228,19 @@ void intel_ddi_get_config(struct intel_encoder
> > *encoder,
> >  	}
> >  
> >  	intel_ddi_clock_get(encoder, pipe_config);
> > +
> > +	if (IS_BROXTON(dev_priv))
> > +		pipe_config->lane_lat_optim_mask =
> > +			bxt_ddi_phy_get_lane_lat_optim_mask(encoder);
> >  }
> >  
> >  static bool intel_ddi_compute_config(struct intel_encoder *encoder,
> >  				     struct intel_crtc_state *pipe_config)
> >  {
> > +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> >  	int type = encoder->type;
> >  	int port = intel_ddi_get_encoder_port(encoder);
> > +	int ret;
> >  
> >  	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown
> > output!\n");
> >  
> > @@ -2213,9 +2248,17 @@ static bool intel_ddi_compute_config(struct
> > intel_encoder *encoder,
> >  		pipe_config->cpu_transcoder = TRANSCODER_EDP;
> >  
> >  	if (type == INTEL_OUTPUT_HDMI)
> > -		return intel_hdmi_compute_config(encoder, pipe_config);
> > +		ret = intel_hdmi_compute_config(encoder, pipe_config);
> >  	else
> > -		return intel_dp_compute_config(encoder, pipe_config);
> > +		ret = intel_dp_compute_config(encoder, pipe_config);
> > +
> > +	if (IS_BROXTON(dev_priv) && ret)
> > +		pipe_config->lane_lat_optim_mask =
> > +			bxt_ddi_phy_calc_lane_lat_optim_mask(encoder,
> > +							     pipe_config);
> > +
> > +	return ret;
> > +
> >  }
> >  
> >  static const struct drm_encoder_funcs intel_ddi_funcs = {
> > @@ -2314,6 +2357,8 @@ void intel_ddi_init(struct drm_device *dev, enum port
> > port)
> >  
> >  	intel_encoder->compute_config = intel_ddi_compute_config;
> >  	intel_encoder->enable = intel_enable_ddi;
> > +	if (IS_BROXTON(dev_priv))
> > +		intel_encoder->pre_pll_enable = bxt_ddi_pre_pll_enable;
> >  	intel_encoder->pre_enable = intel_ddi_pre_enable;
> >  	intel_encoder->disable = intel_disable_ddi;
> >  	intel_encoder->post_disable = intel_ddi_post_disable;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 60cba19..4bc7c48 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4841,6 +4841,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> >  		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> >  						      false);
> >  
> > +	for_each_encoder_on_crtc(dev, crtc, encoder)
> > +		if (encoder->pre_pll_enable)
> > +			encoder->pre_pll_enable(encoder);
> > +
> >  	if (intel_crtc->config->shared_dpll)
> >  		intel_enable_shared_dpll(intel_crtc);
> 
> Effectively this writes BXT_PORT_TX_DW14_LN before bxt_ddi_pll_enable() is
> called. If that would be the first register write in that function, it would
> avoid the extra detour. So we can calculate the optimization in bxt_get_dpll(),
> store it in struct intel_dpll_hw_state and use the pll state checking to verify
> the right values were programmed. That way there's no need to add a value
> specific to the bxt phy to struct intel_crtc_state or implement the
> pre_pll_enable hook.

It's not a PLL specific thing though, rather a property of the PHY
channel. For instance we could share one PLL for two channels in which 
case we'd need to enable only one PLL but configure the lanes for both
channels.

--Imre

> 
> Ander
> 
> >  
> > @@ -12816,6 +12820,7 @@ intel_pipe_config_compare(struct drm_device *dev,
> >  
> >  	PIPE_CONF_CHECK_I(has_dp_encoder);
> >  	PIPE_CONF_CHECK_I(lane_count);
> > +	PIPE_CONF_CHECK_X(lane_lat_optim_mask);
> >  
> >  	if (INTEL_INFO(dev)->gen < 8) {
> >  		PIPE_CONF_CHECK_M_N(dp_m_n);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 17445d7..ca98416 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -579,6 +579,12 @@ struct intel_crtc_state {
> >  
> >  	uint8_t lane_count;
> >  
> > +	/*
> > +	 * Used by platforms having DP/HDMI PHY with programmable lane
> > +	 * latency optimization.
> > +	 */
> > +	uint8_t lane_lat_optim_mask;
> > +
> >  	/* Panel fitter controls for gen2-gen4 + VLV */
> >  	struct {
> >  		u32 control;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 4/6] drm/i915/bxt: Set DDI PHY lane latency optimization during modeset

[Imre Deak]

So far we configured a static lane latency optimization during driver
loading/resuming. The specification changed at one point and now this
configuration depends on the lane count, so move the configuration
to modeset time accordingly.

It's not clear when this lane configuration takes effect. The
specification only requires that the programming is done before enabling
the port. On CHV OTOH the lanes start to power up already right after
enabling the PLL. To be safe preserve the current order and set things
up already before enabling the PLL.

v2: (Ander)
- Simplify the optimization mask calculation.
- Use the correct pipe_config always during the calculation instead
  of the bogus intel_crtc->config.

CC: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95476
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     | 123 +++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_display.c |   5 ++
 drivers/gpu/drm/i915/intel_drv.h     |   6 ++
 3 files changed, 91 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index dee6dd0..e7edeec 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1789,8 +1789,7 @@ static void broxton_phy_wait_grc_done(struct drm_i915_private *dev_priv,
 
 void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
 {
-	enum port port;
-	u32 ports, val;
+	u32 val;
 
 	if (bxt_ddi_phy_is_enabled(dev_priv, phy)) {
 		/* Still read out the GRC value for state verification */
@@ -1825,28 +1824,6 @@ void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
 		DRM_ERROR("timeout during PHY%d power on\n", phy);
 	}
 
-	if (phy == DPIO_PHY0)
-		ports = BIT(PORT_B) | BIT(PORT_C);
-	else
-		ports = BIT(PORT_A);
-
-	for_each_port_masked(port, ports) {
-		int lane;
-
-		for (lane = 0; lane < 4; lane++) {
-			val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
-			/*
-			 * Note that on CHV this flag is called UPAR, but has
-			 * the same function.
-			 */
-			val &= ~LATENCY_OPTIM;
-			if (lane != 1)
-				val |= LATENCY_OPTIM;
-
-			I915_WRITE(BXT_PORT_TX_DW14_LN(port, lane), val);
-		}
-	}
-
 	/* Program PLL Rcomp code offset */
 	val = I915_READ(BXT_PORT_CL1CM_DW9(phy));
 	val &= ~IREF0RC_OFFSET_MASK;
@@ -1956,8 +1933,6 @@ __phy_reg_verify_state(struct drm_i915_private *dev_priv, enum dpio_phy phy,
 bool bxt_ddi_phy_verify_state(struct drm_i915_private *dev_priv,
 			      enum dpio_phy phy)
 {
-	enum port port;
-	u32 ports;
 	uint32_t mask;
 	bool ok;
 
@@ -1970,21 +1945,6 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private *dev_priv,
 
 	ok = true;
 
-	if (phy == DPIO_PHY0)
-		ports = BIT(PORT_B) | BIT(PORT_C);
-	else
-		ports = BIT(PORT_A);
-
-	for_each_port_masked(port, ports) {
-		int lane;
-
-		for (lane = 0; lane < 4; lane++)
-			ok &= _CHK(BXT_PORT_TX_DW14_LN(port, lane),
-				    LATENCY_OPTIM,
-				    lane != 1 ? LATENCY_OPTIM : 0,
-				    "BXT_PORT_TX_DW14_LN(%d, %d)", port, lane);
-	}
-
 	/* PLL Rcomp code offset */
 	ok &= _CHK(BXT_PORT_CL1CM_DW9(phy),
 		    IREF0RC_OFFSET_MASK, 0xe4 << IREF0RC_OFFSET_SHIFT,
@@ -2028,6 +1988,67 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private *dev_priv,
 #undef _CHK
 }
 
+static uint8_t
+bxt_ddi_phy_calc_lane_lat_optim_mask(struct intel_encoder *encoder,
+				     struct intel_crtc_state *pipe_config)
+{
+	switch (pipe_config->lane_count) {
+	case 1:
+		return 0;
+	case 2:
+		return BIT(2) | BIT(0);
+	case 4:
+		return BIT(3) | BIT(2) | BIT(0);
+	default:
+		MISSING_CASE(pipe_config->lane_count);
+
+		return 0;
+	}
+}
+
+static void bxt_ddi_pre_pll_enable(struct intel_encoder *encoder)
+{
+	struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
+	struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev);
+	enum port port = dport->port;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	int lane;
+
+	for (lane = 0; lane < 4; lane++) {
+		u32 val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
+
+		/*
+		 * Note that on CHV this flag is called UPAR, but has
+		 * the same function.
+		 */
+		val &= ~LATENCY_OPTIM;
+		if (intel_crtc->config->lane_lat_optim_mask & BIT(lane))
+			val |= LATENCY_OPTIM;
+
+		I915_WRITE(BXT_PORT_TX_DW14_LN(port, lane), val);
+	}
+}
+
+static uint8_t
+bxt_ddi_phy_get_lane_lat_optim_mask(struct intel_encoder *encoder)
+{
+	struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
+	struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev);
+	enum port port = dport->port;
+	int lane;
+	uint8_t mask;
+
+	mask = 0;
+	for (lane = 0; lane < 4; lane++) {
+		u32 val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
+
+		if (val & LATENCY_OPTIM)
+			mask |= BIT(lane);
+	}
+
+	return mask;
+}
+
 void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -2199,13 +2220,19 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
 	}
 
 	intel_ddi_clock_get(encoder, pipe_config);
+
+	if (IS_BROXTON(dev_priv))
+		pipe_config->lane_lat_optim_mask =
+			bxt_ddi_phy_get_lane_lat_optim_mask(encoder);
 }
 
 static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 				     struct intel_crtc_state *pipe_config)
 {
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
 	int type = encoder->type;
 	int port = intel_ddi_get_encoder_port(encoder);
+	int ret;
 
 	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
 
@@ -2213,9 +2240,17 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 		pipe_config->cpu_transcoder = TRANSCODER_EDP;
 
 	if (type == INTEL_OUTPUT_HDMI)
-		return intel_hdmi_compute_config(encoder, pipe_config);
+		ret = intel_hdmi_compute_config(encoder, pipe_config);
 	else
-		return intel_dp_compute_config(encoder, pipe_config);
+		ret = intel_dp_compute_config(encoder, pipe_config);
+
+	if (IS_BROXTON(dev_priv) && ret)
+		pipe_config->lane_lat_optim_mask =
+			bxt_ddi_phy_calc_lane_lat_optim_mask(encoder,
+							     pipe_config);
+
+	return ret;
+
 }
 
 static const struct drm_encoder_funcs intel_ddi_funcs = {
@@ -2314,6 +2349,8 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 
 	intel_encoder->compute_config = intel_ddi_compute_config;
 	intel_encoder->enable = intel_enable_ddi;
+	if (IS_BROXTON(dev_priv))
+		intel_encoder->pre_pll_enable = bxt_ddi_pre_pll_enable;
 	intel_encoder->pre_enable = intel_ddi_pre_enable;
 	intel_encoder->disable = intel_disable_ddi;
 	intel_encoder->post_disable = intel_ddi_post_disable;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 60cba19..4bc7c48 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4841,6 +4841,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
 						      false);
 
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		if (encoder->pre_pll_enable)
+			encoder->pre_pll_enable(encoder);
+
 	if (intel_crtc->config->shared_dpll)
 		intel_enable_shared_dpll(intel_crtc);
 
@@ -12816,6 +12820,7 @@ intel_pipe_config_compare(struct drm_device *dev,
 
 	PIPE_CONF_CHECK_I(has_dp_encoder);
 	PIPE_CONF_CHECK_I(lane_count);
+	PIPE_CONF_CHECK_X(lane_lat_optim_mask);
 
 	if (INTEL_INFO(dev)->gen < 8) {
 		PIPE_CONF_CHECK_M_N(dp_m_n);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 17445d7..ca98416 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -579,6 +579,12 @@ struct intel_crtc_state {
 
 	uint8_t lane_count;
 
+	/*
+	 * Used by platforms having DP/HDMI PHY with programmable lane
+	 * latency optimization.
+	 */
+	uint8_t lane_lat_optim_mask;
+
 	/* Panel fitter controls for gen2-gen4 + VLV */
 	struct {
 		u32 control;
-- 
2.5.0

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

[PATCH 5/6] drm/i915/bxt: Rename broxton to bxt in PHY/CDCLK function prefixes

[Imre Deak]

Rename these remaining function prefixes to better align with the
corresponding SKL functions.

No functional change.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c        | 13 ++++++-------
 drivers/gpu/drm/i915/intel_display.c    | 28 ++++++++++++++--------------
 drivers/gpu/drm/i915/intel_drv.h        |  4 ++--
 drivers/gpu/drm/i915/intel_runtime_pm.c |  4 ++--
 4 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index f46117a..acf0a7a 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1773,15 +1773,15 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
 	return true;
 }
 
-static u32 broxton_get_grc(struct drm_i915_private *dev_priv, enum dpio_phy phy)
+static u32 bxt_get_grc(struct drm_i915_private *dev_priv, enum dpio_phy phy)
 {
 	u32 val = I915_READ(BXT_PORT_REF_DW6(phy));
 
 	return (val & GRC_CODE_MASK) >> GRC_CODE_SHIFT;
 }
 
-static void broxton_phy_wait_grc_done(struct drm_i915_private *dev_priv,
-				      enum dpio_phy phy)
+static void bxt_phy_wait_grc_done(struct drm_i915_private *dev_priv,
+				  enum dpio_phy phy)
 {
 	if (wait_for(I915_READ(BXT_PORT_REF_DW3(phy)) & GRC_DONE, 10))
 		DRM_ERROR("timeout waiting for PHY%d GRC\n", phy);
@@ -1794,7 +1794,7 @@ void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
 	if (bxt_ddi_phy_is_enabled(dev_priv, phy)) {
 		/* Still read out the GRC value for state verification */
 		if (phy == DPIO_PHY0)
-			dev_priv->bxt_phy_grc = broxton_get_grc(dev_priv, phy);
+			dev_priv->bxt_phy_grc = bxt_get_grc(dev_priv, phy);
 
 		if (bxt_ddi_phy_verify_state(dev_priv, phy)) {
 			DRM_DEBUG_DRIVER("DDI PHY %d already enabled, "
@@ -1870,8 +1870,7 @@ void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
 		 * the corresponding calibrated value from PHY1, and disable
 		 * the automatic calibration on PHY0.
 		 */
-		val = dev_priv->bxt_phy_grc = broxton_get_grc(dev_priv,
-							      DPIO_PHY1);
+		val = dev_priv->bxt_phy_grc = bxt_get_grc(dev_priv, DPIO_PHY1);
 		grc_code = val << GRC_CODE_FAST_SHIFT |
 			   val << GRC_CODE_SLOW_SHIFT |
 			   val;
@@ -1887,7 +1886,7 @@ void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
 	I915_WRITE(BXT_PHY_CTL_FAMILY(phy), val);
 
 	if (phy == DPIO_PHY1)
-		broxton_phy_wait_grc_done(dev_priv, DPIO_PHY1);
+		bxt_phy_wait_grc_done(dev_priv, DPIO_PHY1);
 }
 
 void bxt_ddi_phy_uninit(struct drm_i915_private *dev_priv, enum dpio_phy phy)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4bc7c48..2bfd219 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -123,7 +123,7 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc);
 static void intel_modeset_setup_hw_state(struct drm_device *dev);
 static void intel_pre_disable_primary_noatomic(struct drm_crtc *crtc);
 static int ilk_max_pixel_rate(struct drm_atomic_state *state);
-static int broxton_calc_cdclk(int max_pixclk);
+static int bxt_calc_cdclk(int max_pixclk);
 
 struct intel_limit {
 	struct {
@@ -5420,7 +5420,7 @@ static void bxt_de_pll_enable(struct drm_i915_private *dev_priv, int vco)
 	dev_priv->cdclk_pll.vco = vco;
 }
 
-static void broxton_set_cdclk(struct drm_i915_private *dev_priv, int cdclk)
+static void bxt_set_cdclk(struct drm_i915_private *dev_priv, int cdclk)
 {
 	u32 val, divider;
 	int vco, ret;
@@ -5545,7 +5545,7 @@ sanitize:
 	dev_priv->cdclk_pll.vco = -1;
 }
 
-void broxton_init_cdclk(struct drm_i915_private *dev_priv)
+void bxt_init_cdclk(struct drm_i915_private *dev_priv)
 {
 	bxt_sanitize_cdclk(dev_priv);
 
@@ -5557,12 +5557,12 @@ void broxton_init_cdclk(struct drm_i915_private *dev_priv)
 	 * - The initial CDCLK needs to be read from VBT.
 	 *   Need to make this change after VBT has changes for BXT.
 	 */
-	broxton_set_cdclk(dev_priv, broxton_calc_cdclk(0));
+	bxt_set_cdclk(dev_priv, bxt_calc_cdclk(0));
 }
 
-void broxton_uninit_cdclk(struct drm_i915_private *dev_priv)
+void bxt_uninit_cdclk(struct drm_i915_private *dev_priv)
 {
-	broxton_set_cdclk(dev_priv, dev_priv->cdclk_pll.ref);
+	bxt_set_cdclk(dev_priv, dev_priv->cdclk_pll.ref);
 }
 
 static int skl_calc_cdclk(int max_pixclk, int vco)
@@ -5988,7 +5988,7 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv,
 		return 200000;
 }
 
-static int broxton_calc_cdclk(int max_pixclk)
+static int bxt_calc_cdclk(int max_pixclk)
 {
 	if (max_pixclk > 576000)
 		return 624000;
@@ -6048,17 +6048,17 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
 	return 0;
 }
 
-static int broxton_modeset_calc_cdclk(struct drm_atomic_state *state)
+static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
 {
 	int max_pixclk = ilk_max_pixel_rate(state);
 	struct intel_atomic_state *intel_state =
 		to_intel_atomic_state(state);
 
 	intel_state->cdclk = intel_state->dev_cdclk =
-		broxton_calc_cdclk(max_pixclk);
+		bxt_calc_cdclk(max_pixclk);
 
 	if (!intel_state->active_crtcs)
-		intel_state->dev_cdclk = broxton_calc_cdclk(0);
+		intel_state->dev_cdclk = bxt_calc_cdclk(0);
 
 	return 0;
 }
@@ -9677,14 +9677,14 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
 	}
 }
 
-static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state)
+static void bxt_modeset_commit_cdclk(struct drm_atomic_state *old_state)
 {
 	struct drm_device *dev = old_state->dev;
 	struct intel_atomic_state *old_intel_state =
 		to_intel_atomic_state(old_state);
 	unsigned int req_cdclk = old_intel_state->dev_cdclk;
 
-	broxton_set_cdclk(to_i915(dev), req_cdclk);
+	bxt_set_cdclk(to_i915(dev), req_cdclk);
 }
 
 /* compute the max rate for new configuration */
@@ -15217,9 +15217,9 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 			valleyview_modeset_calc_cdclk;
 	} else if (IS_BROXTON(dev_priv)) {
 		dev_priv->display.modeset_commit_cdclk =
-			broxton_modeset_commit_cdclk;
+			bxt_modeset_commit_cdclk;
 		dev_priv->display.modeset_calc_cdclk =
-			broxton_modeset_calc_cdclk;
+			bxt_modeset_calc_cdclk;
 	} else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
 		dev_priv->display.modeset_commit_cdclk =
 			skl_modeset_commit_cdclk;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ca98416..4f05341 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1267,8 +1267,8 @@ void intel_prepare_reset(struct drm_i915_private *dev_priv);
 void intel_finish_reset(struct drm_i915_private *dev_priv);
 void hsw_enable_pc8(struct drm_i915_private *dev_priv);
 void hsw_disable_pc8(struct drm_i915_private *dev_priv);
-void broxton_init_cdclk(struct drm_i915_private *dev_priv);
-void broxton_uninit_cdclk(struct drm_i915_private *dev_priv);
+void bxt_init_cdclk(struct drm_i915_private *dev_priv);
+void bxt_uninit_cdclk(struct drm_i915_private *dev_priv);
 void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy);
 void bxt_ddi_phy_uninit(struct drm_i915_private *dev_priv, enum dpio_phy phy);
 bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 1dd937a..e03078e 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2403,7 +2403,7 @@ void bxt_display_core_init(struct drm_i915_private *dev_priv,
 
 	mutex_unlock(&power_domains->lock);
 
-	broxton_init_cdclk(dev_priv);
+	bxt_init_cdclk(dev_priv);
 
 	gen9_dbuf_enable(dev_priv);
 
@@ -2420,7 +2420,7 @@ void bxt_display_core_uninit(struct drm_i915_private *dev_priv)
 
 	gen9_dbuf_disable(dev_priv);
 
-	broxton_uninit_cdclk(dev_priv);
+	bxt_uninit_cdclk(dev_priv);
 
 	/* The spec doesn't call for removing the reset handshake flag */
 
-- 
2.5.0

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

[PATCH 6/6] drm/i915/bxt: Sanitiy check the PHY lane power down status

[Imre Deak]

We can check the power state of the PHY data and common lanes as
reported by the PHY. Do this in case we need to debug problems where the
PHY gets stuck in an unexpected state.

Note that I only check these when the lanes are expected to be powered
on purpose, since it's not clear at what point the PHY power/clock gates
things.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  9 +++++++++
 drivers/gpu/drm/i915/intel_ddi.c | 27 +++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 81fc498..e904818 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1276,6 +1276,15 @@ enum skl_disp_power_wells {
 #define BXT_P_CR_GT_DISP_PWRON		_MMIO(0x138090)
 #define   GT_DISPLAY_POWER_ON(phy)	(1 << (phy))
 
+#define _BXT_PHY_CTL_DDI_A		0x64C00
+#define _BXT_PHY_CTL_DDI_B		0x64C10
+#define _BXT_PHY_CTL_DDI_C		0x64C20
+#define   BXT_PHY_CMNLANE_POWERDOWN_ACK	(1 << 10)
+#define   BXT_PHY_LANE_POWERDOWN_ACK	(1 << 9)
+#define   BXT_PHY_LANE_ENABLED		(1 << 8)
+#define BXT_PHY_CTL(port)		_MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \
+							 _BXT_PHY_CTL_DDI_B)
+
 #define _PHY_CTL_FAMILY_EDP		0x64C80
 #define _PHY_CTL_FAMILY_DDI		0x64C90
 #define   COMMON_RESET_DIS		(1 << 31)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index acf0a7a..afa28a1 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1342,6 +1342,16 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
 	DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port));
 
 out:
+	if (ret && IS_BROXTON(dev_priv)) {
+		tmp = I915_READ(BXT_PHY_CTL(port));
+		if ((tmp & (BXT_PHY_LANE_POWERDOWN_ACK |
+			    BXT_PHY_LANE_ENABLED)) != BXT_PHY_LANE_ENABLED) {
+			DRM_DEBUG_KMS("Port %c enabled but PHY powered down? "
+				      "(PHY_CTL %08x)\n", port_name(port), tmp);
+			ret = false;
+		}
+	}
+
 	intel_display_power_put(dev_priv, power_domain);
 
 	return ret;
@@ -1745,6 +1755,8 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
 			    enum dpio_phy phy)
 {
+	enum port port;
+
 	if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy)))
 		return false;
 
@@ -1770,6 +1782,21 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
 		return false;
 	}
 
+	for_each_port_masked(port,
+			     phy == DPIO_PHY0 ? BIT(PORT_B) | BIT(PORT_C) :
+					        BIT(PORT_A)) {
+		u32 tmp = I915_READ(BXT_PHY_CTL(port));
+
+		if (tmp & BXT_PHY_CMNLANE_POWERDOWN_ACK) {
+			DRM_DEBUG_DRIVER("DDI PHY %d powered, but common lane "
+					 "for port %c powered down "
+					 "(PHY_CTL %08x)\n",
+					 phy, port_name(port), tmp);
+
+			return false;
+		}
+	}
+
 	return true;
 }
 
-- 
2.5.0

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

Re: [PATCH 6/6] drm/i915/bxt: Sanitiy check the PHY lane power down status

[Ville Syrjälä]

On Tue, Jun 07, 2016 at 09:24:33PM +0300, Imre Deak wrote:
> We can check the power state of the PHY data and common lanes as
> reported by the PHY. Do this in case we need to debug problems where the
> PHY gets stuck in an unexpected state.
> 
> Note that I only check these when the lanes are expected to be powered
> on purpose, since it's not clear at what point the PHY power/clock gates
> things.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  9 +++++++++
>  drivers/gpu/drm/i915/intel_ddi.c | 27 +++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 81fc498..e904818 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1276,6 +1276,15 @@ enum skl_disp_power_wells {
>  #define BXT_P_CR_GT_DISP_PWRON		_MMIO(0x138090)
>  #define   GT_DISPLAY_POWER_ON(phy)	(1 << (phy))
>  
> +#define _BXT_PHY_CTL_DDI_A		0x64C00
> +#define _BXT_PHY_CTL_DDI_B		0x64C10
> +#define _BXT_PHY_CTL_DDI_C		0x64C20
> +#define   BXT_PHY_CMNLANE_POWERDOWN_ACK	(1 << 10)
> +#define   BXT_PHY_LANE_POWERDOWN_ACK	(1 << 9)
> +#define   BXT_PHY_LANE_ENABLED		(1 << 8)
> +#define BXT_PHY_CTL(port)		_MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \
> +							 _BXT_PHY_CTL_DDI_B)
> +
>  #define _PHY_CTL_FAMILY_EDP		0x64C80
>  #define _PHY_CTL_FAMILY_DDI		0x64C90
>  #define   COMMON_RESET_DIS		(1 << 31)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index acf0a7a..afa28a1 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1342,6 +1342,16 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
>  	DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port));
>  
>  out:
> +	if (ret && IS_BROXTON(dev_priv)) {
> +		tmp = I915_READ(BXT_PHY_CTL(port));
> +		if ((tmp & (BXT_PHY_LANE_POWERDOWN_ACK |
> +			    BXT_PHY_LANE_ENABLED)) != BXT_PHY_LANE_ENABLED) {
> +			DRM_DEBUG_KMS("Port %c enabled but PHY powered down? "
> +				      "(PHY_CTL %08x)\n", port_name(port), tmp);
> +			ret = false;

Hmm. I'm not sure I'd change the return value here because then we'd
fail to follow the proper shutdown sequence, perhaps even messing
up the next modeset?

So maybe just ERROR/WARN in these cases?

> +		}
> +	}
> +
>  	intel_display_power_put(dev_priv, power_domain);
>  
>  	return ret;
> @@ -1745,6 +1755,8 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>  bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
>  			    enum dpio_phy phy)
>  {
> +	enum port port;
> +
>  	if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy)))
>  		return false;
>  
> @@ -1770,6 +1782,21 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
>  		return false;
>  	}
>  
> +	for_each_port_masked(port,
> +			     phy == DPIO_PHY0 ? BIT(PORT_B) | BIT(PORT_C) :
> +					        BIT(PORT_A)) {
> +		u32 tmp = I915_READ(BXT_PHY_CTL(port));
> +
> +		if (tmp & BXT_PHY_CMNLANE_POWERDOWN_ACK) {
> +			DRM_DEBUG_DRIVER("DDI PHY %d powered, but common lane "
> +					 "for port %c powered down "
> +					 "(PHY_CTL %08x)\n",
> +					 phy, port_name(port), tmp);
> +
> +			return false;
> +		}
> +	}
> +
>  	return true;
>  }
>  
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 6/6] drm/i915/bxt: Sanitiy check the PHY lane power down status

[Imre Deak]

On ke, 2016-06-08 at 17:19 +0300, Ville Syrjälä wrote:
> On Tue, Jun 07, 2016 at 09:24:33PM +0300, Imre Deak wrote:
> > We can check the power state of the PHY data and common lanes as
> > reported by the PHY. Do this in case we need to debug problems where the
> > PHY gets stuck in an unexpected state.
> > 
> > Note that I only check these when the lanes are expected to be powered
> > on purpose, since it's not clear at what point the PHY power/clock gates
> > things.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  |  9 +++++++++
> >  drivers/gpu/drm/i915/intel_ddi.c | 27 +++++++++++++++++++++++++++
> >  2 files changed, 36 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 81fc498..e904818 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1276,6 +1276,15 @@ enum skl_disp_power_wells {
> >  #define BXT_P_CR_GT_DISP_PWRON		_MMIO(0x138090)
> >  #define   GT_DISPLAY_POWER_ON(phy)	(1 << (phy))
> >  
> > +#define _BXT_PHY_CTL_DDI_A		0x64C00
> > +#define _BXT_PHY_CTL_DDI_B		0x64C10
> > +#define _BXT_PHY_CTL_DDI_C		0x64C20
> > +#define   BXT_PHY_CMNLANE_POWERDOWN_ACK	(1 << 10)
> > +#define   BXT_PHY_LANE_POWERDOWN_ACK	(1 << 9)
> > +#define   BXT_PHY_LANE_ENABLED		(1 << 8)
> > +#define BXT_PHY_CTL(port)		_MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \
> > +							 _BXT_PHY_CTL_DDI_B)
> > +
> >  #define _PHY_CTL_FAMILY_EDP		0x64C80
> >  #define _PHY_CTL_FAMILY_DDI		0x64C90
> >  #define   COMMON_RESET_DIS		(1 << 31)
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index acf0a7a..afa28a1 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1342,6 +1342,16 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
> >  	DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port));
> >  
> >  out:
> > +	if (ret && IS_BROXTON(dev_priv)) {
> > +		tmp = I915_READ(BXT_PHY_CTL(port));
> > +		if ((tmp & (BXT_PHY_LANE_POWERDOWN_ACK |
> > +			    BXT_PHY_LANE_ENABLED)) != BXT_PHY_LANE_ENABLED) {
> > +			DRM_DEBUG_KMS("Port %c enabled but PHY powered down? "
> > +				      "(PHY_CTL %08x)\n", port_name(port), tmp);
> > +			ret = false;
> 
> Hmm. I'm not sure I'd change the return value here because then we'd
> fail to follow the proper shutdown sequence, perhaps even messing
> up the next modeset?

What I wanted is to force a reprogramming in this case, but yea we
would miss then the proper disable sequence. AFAICS we can't mark the
state here as enabled but not valid, so for now I will just change the
above to DRM_ERROR then and report an enabled state as you suggested.

> So maybe just ERROR/WARN in these cases?
> 
> > +		}
> > +	}
> > +
> >  	intel_display_power_put(dev_priv, power_domain);
> >  
> >  	return ret;
> > @@ -1745,6 +1755,8 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
> >  bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
> >  			    enum dpio_phy phy)
> >  {
> > +	enum port port;
> > +
> >  	if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy)))
> >  		return false;
> >  
> > @@ -1770,6 +1782,21 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
> >  		return false;
> >  	}
> >  
> > +	for_each_port_masked(port,
> > +			     phy == DPIO_PHY0 ? BIT(PORT_B) | BIT(PORT_C) :
> > +					        BIT(PORT_A)) {
> > +		u32 tmp = I915_READ(BXT_PHY_CTL(port));
> > +
> > +		if (tmp & BXT_PHY_CMNLANE_POWERDOWN_ACK) {
> > +			DRM_DEBUG_DRIVER("DDI PHY %d powered, but common lane "
> > +					 "for port %c powered down "
> > +					 "(PHY_CTL %08x)\n",
> > +					 phy, port_name(port), tmp);
> > +
> > +			return false;
> > +		}
> > +	}
> > +
> >  	return true;
> >  }
> >  
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > 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

Re: [PATCH 6/6] drm/i915/bxt: Sanitiy check the PHY lane power down status

[Ville Syrjälä]

On Wed, Jun 08, 2016 at 05:41:00PM +0300, Imre Deak wrote:
> On ke, 2016-06-08 at 17:19 +0300, Ville Syrjälä wrote:
> > On Tue, Jun 07, 2016 at 09:24:33PM +0300, Imre Deak wrote:
> > > We can check the power state of the PHY data and common lanes as
> > > reported by the PHY. Do this in case we need to debug problems where the
> > > PHY gets stuck in an unexpected state.
> > > 
> > > Note that I only check these when the lanes are expected to be powered
> > > on purpose, since it's not clear at what point the PHY power/clock gates
> > > things.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h  |  9 +++++++++
> > >  drivers/gpu/drm/i915/intel_ddi.c | 27 +++++++++++++++++++++++++++
> > >  2 files changed, 36 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 81fc498..e904818 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -1276,6 +1276,15 @@ enum skl_disp_power_wells {
> > >  #define BXT_P_CR_GT_DISP_PWRON		_MMIO(0x138090)
> > >  #define   GT_DISPLAY_POWER_ON(phy)	(1 << (phy))
> > >  
> > > +#define _BXT_PHY_CTL_DDI_A		0x64C00
> > > +#define _BXT_PHY_CTL_DDI_B		0x64C10
> > > +#define _BXT_PHY_CTL_DDI_C		0x64C20
> > > +#define   BXT_PHY_CMNLANE_POWERDOWN_ACK	(1 << 10)
> > > +#define   BXT_PHY_LANE_POWERDOWN_ACK	(1 << 9)
> > > +#define   BXT_PHY_LANE_ENABLED		(1 << 8)
> > > +#define BXT_PHY_CTL(port)		_MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \
> > > +							 _BXT_PHY_CTL_DDI_B)
> > > +
> > >  #define _PHY_CTL_FAMILY_EDP		0x64C80
> > >  #define _PHY_CTL_FAMILY_DDI		0x64C90
> > >  #define   COMMON_RESET_DIS		(1 << 31)
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index acf0a7a..afa28a1 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -1342,6 +1342,16 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
> > >  	DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port));
> > >  
> > >  out:
> > > +	if (ret && IS_BROXTON(dev_priv)) {
> > > +		tmp = I915_READ(BXT_PHY_CTL(port));
> > > +		if ((tmp & (BXT_PHY_LANE_POWERDOWN_ACK |
> > > +			    BXT_PHY_LANE_ENABLED)) != BXT_PHY_LANE_ENABLED) {
> > > +			DRM_DEBUG_KMS("Port %c enabled but PHY powered down? "
> > > +				      "(PHY_CTL %08x)\n", port_name(port), tmp);
> > > +			ret = false;
> > 
> > Hmm. I'm not sure I'd change the return value here because then we'd
> > fail to follow the proper shutdown sequence, perhaps even messing
> > up the next modeset?
> 
> What I wanted is to force a reprogramming in this case, but yea we
> would miss then the proper disable sequence. AFAICS we can't mark the
> state here as enabled but not valid, so for now I will just change the
> above to DRM_ERROR then and report an enabled state as you suggested.

Yeah. So far we've done any reprogramming in the sanitize phase, but
that may require splitting/duplicating some parts of state readout. Maybe
we should allow .get_config() & co. to flag the state as "this needs a
forced modeset to sanitize things"?

> 
> > So maybe just ERROR/WARN in these cases?
> > 
> > > +		}
> > > +	}
> > > +
> > >  	intel_display_power_put(dev_priv, power_domain);
> > >  
> > >  	return ret;
> > > @@ -1745,6 +1755,8 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
> > >  bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
> > >  			    enum dpio_phy phy)
> > >  {
> > > +	enum port port;
> > > +
> > >  	if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy)))
> > >  		return false;
> > >  
> > > @@ -1770,6 +1782,21 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
> > >  		return false;
> > >  	}
> > >  
> > > +	for_each_port_masked(port,
> > > +			     phy == DPIO_PHY0 ? BIT(PORT_B) | BIT(PORT_C) :
> > > +					        BIT(PORT_A)) {
> > > +		u32 tmp = I915_READ(BXT_PHY_CTL(port));
> > > +
> > > +		if (tmp & BXT_PHY_CMNLANE_POWERDOWN_ACK) {
> > > +			DRM_DEBUG_DRIVER("DDI PHY %d powered, but common lane "
> > > +					 "for port %c powered down "
> > > +					 "(PHY_CTL %08x)\n",
> > > +					 phy, port_name(port), tmp);
> > > +
> > > +			return false;
> > > +		}
> > > +	}
> > > +
> > >  	return true;
> > >  }
> > >  
> > > -- 
> > > 2.5.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

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

Re: [PATCH 6/6] drm/i915/bxt: Sanitiy check the PHY lane power down status

[Imre Deak]

On ke, 2016-06-08 at 17:50 +0300, Ville Syrjälä wrote:
> On Wed, Jun 08, 2016 at 05:41:00PM +0300, Imre Deak wrote:
> > On ke, 2016-06-08 at 17:19 +0300, Ville Syrjälä wrote:
> > > On Tue, Jun 07, 2016 at 09:24:33PM +0300, Imre Deak wrote:
> > > > We can check the power state of the PHY data and common lanes as
> > > > reported by the PHY. Do this in case we need to debug problems where the
> > > > PHY gets stuck in an unexpected state.
> > > > 
> > > > Note that I only check these when the lanes are expected to be powered
> > > > on purpose, since it's not clear at what point the PHY power/clock gates
> > > > things.
> > > > 
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_reg.h  |  9 +++++++++
> > > >  drivers/gpu/drm/i915/intel_ddi.c | 27 +++++++++++++++++++++++++++
> > > >  2 files changed, 36 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 81fc498..e904818 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -1276,6 +1276,15 @@ enum skl_disp_power_wells {
> > > >  #define BXT_P_CR_GT_DISP_PWRON		_MMIO(0x138090)
> > > >  #define   GT_DISPLAY_POWER_ON(phy)	(1 << (phy))
> > > >  
> > > > +#define _BXT_PHY_CTL_DDI_A		0x64C00
> > > > +#define _BXT_PHY_CTL_DDI_B		0x64C10
> > > > +#define _BXT_PHY_CTL_DDI_C		0x64C20
> > > > +#define   BXT_PHY_CMNLANE_POWERDOWN_ACK	(1 << 10)
> > > > +#define   BXT_PHY_LANE_POWERDOWN_ACK	(1 << 9)
> > > > +#define   BXT_PHY_LANE_ENABLED		(1 << 8)
> > > > +#define BXT_PHY_CTL(port)		_MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \
> > > > +							 _BXT_PHY_CTL_DDI_B)
> > > > +
> > > >  #define _PHY_CTL_FAMILY_EDP		0x64C80
> > > >  #define _PHY_CTL_FAMILY_DDI		0x64C90
> > > >  #define   COMMON_RESET_DIS		(1 << 31)
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index acf0a7a..afa28a1 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -1342,6 +1342,16 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
> > > >  	DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port));
> > > >  
> > > >  out:
> > > > +	if (ret && IS_BROXTON(dev_priv)) {
> > > > +		tmp = I915_READ(BXT_PHY_CTL(port));
> > > > +		if ((tmp & (BXT_PHY_LANE_POWERDOWN_ACK |
> > > > +			    BXT_PHY_LANE_ENABLED)) != BXT_PHY_LANE_ENABLED) {
> > > > +			DRM_DEBUG_KMS("Port %c enabled but PHY powered down? "
> > > > +				      "(PHY_CTL %08x)\n", port_name(port), tmp);
> > > > +			ret = false;
> > > 
> > > Hmm. I'm not sure I'd change the return value here because then we'd
> > > fail to follow the proper shutdown sequence, perhaps even messing
> > > up the next modeset?
> > 
> > What I wanted is to force a reprogramming in this case, but yea we
> > would miss then the proper disable sequence. AFAICS we can't mark the
> > state here as enabled but not valid, so for now I will just change the
> > above to DRM_ERROR then and report an enabled state as you suggested.
> 
> Yeah. So far we've done any reprogramming in the sanitize phase, but
> that may require splitting/duplicating some parts of state readout. Maybe
> we should allow .get_config() & co. to flag the state as "this needs a
> forced modeset to sanitize things"?

Yea, sounds good to me. Since this a new check in any case could we add
this as a follow-up?

> > > So maybe just ERROR/WARN in these cases?
> > > 
> > > > +		}
> > > > +	}
> > > > +
> > > >  	intel_display_power_put(dev_priv, power_domain);
> > > >  
> > > >  	return ret;
> > > > @@ -1745,6 +1755,8 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
> > > >  bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
> > > >  			    enum dpio_phy phy)
> > > >  {
> > > > +	enum port port;
> > > > +
> > > >  	if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy)))
> > > >  		return false;
> > > >  
> > > > @@ -1770,6 +1782,21 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
> > > >  		return false;
> > > >  	}
> > > >  
> > > > +	for_each_port_masked(port,
> > > > +			     phy == DPIO_PHY0 ? BIT(PORT_B) | BIT(PORT_C) :
> > > > +					        BIT(PORT_A)) {
> > > > +		u32 tmp = I915_READ(BXT_PHY_CTL(port));
> > > > +
> > > > +		if (tmp & BXT_PHY_CMNLANE_POWERDOWN_ACK) {
> > > > +			DRM_DEBUG_DRIVER("DDI PHY %d powered, but common lane "
> > > > +					 "for port %c powered down "
> > > > +					 "(PHY_CTL %08x)\n",
> > > > +					 phy, port_name(port), tmp);
> > > > +
> > > > +			return false;
> > > > +		}
> > > > +	}
> > > > +
> > > >  	return true;
> > > >  }
> > > >  
> > > > -- 
> > > > 2.5.0
> > > > 
> > > > _______________________________________________
> > > > 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

Re: [PATCH 6/6] drm/i915/bxt: Sanitiy check the PHY lane power down status

[Ville Syrjälä]

On Wed, Jun 08, 2016 at 05:55:23PM +0300, Imre Deak wrote:
> On ke, 2016-06-08 at 17:50 +0300, Ville Syrjälä wrote:
> > On Wed, Jun 08, 2016 at 05:41:00PM +0300, Imre Deak wrote:
> > > On ke, 2016-06-08 at 17:19 +0300, Ville Syrjälä wrote:
> > > > On Tue, Jun 07, 2016 at 09:24:33PM +0300, Imre Deak wrote:
> > > > > We can check the power state of the PHY data and common lanes as
> > > > > reported by the PHY. Do this in case we need to debug problems where the
> > > > > PHY gets stuck in an unexpected state.
> > > > > 
> > > > > Note that I only check these when the lanes are expected to be powered
> > > > > on purpose, since it's not clear at what point the PHY power/clock gates
> > > > > things.
> > > > > 
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_reg.h  |  9 +++++++++
> > > > >  drivers/gpu/drm/i915/intel_ddi.c | 27 +++++++++++++++++++++++++++
> > > > >  2 files changed, 36 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index 81fc498..e904818 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -1276,6 +1276,15 @@ enum skl_disp_power_wells {
> > > > >  #define BXT_P_CR_GT_DISP_PWRON		_MMIO(0x138090)
> > > > >  #define   GT_DISPLAY_POWER_ON(phy)	(1 << (phy))
> > > > >  
> > > > > +#define _BXT_PHY_CTL_DDI_A		0x64C00
> > > > > +#define _BXT_PHY_CTL_DDI_B		0x64C10
> > > > > +#define _BXT_PHY_CTL_DDI_C		0x64C20
> > > > > +#define   BXT_PHY_CMNLANE_POWERDOWN_ACK	(1 << 10)
> > > > > +#define   BXT_PHY_LANE_POWERDOWN_ACK	(1 << 9)
> > > > > +#define   BXT_PHY_LANE_ENABLED		(1 << 8)
> > > > > +#define BXT_PHY_CTL(port)		_MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \
> > > > > +							 _BXT_PHY_CTL_DDI_B)
> > > > > +
> > > > >  #define _PHY_CTL_FAMILY_EDP		0x64C80
> > > > >  #define _PHY_CTL_FAMILY_DDI		0x64C90
> > > > >  #define   COMMON_RESET_DIS		(1 << 31)
> > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > index acf0a7a..afa28a1 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > @@ -1342,6 +1342,16 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
> > > > >  	DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port));
> > > > >  
> > > > >  out:
> > > > > +	if (ret && IS_BROXTON(dev_priv)) {
> > > > > +		tmp = I915_READ(BXT_PHY_CTL(port));
> > > > > +		if ((tmp & (BXT_PHY_LANE_POWERDOWN_ACK |
> > > > > +			    BXT_PHY_LANE_ENABLED)) != BXT_PHY_LANE_ENABLED) {
> > > > > +			DRM_DEBUG_KMS("Port %c enabled but PHY powered down? "
> > > > > +				      "(PHY_CTL %08x)\n", port_name(port), tmp);
> > > > > +			ret = false;
> > > > 
> > > > Hmm. I'm not sure I'd change the return value here because then we'd
> > > > fail to follow the proper shutdown sequence, perhaps even messing
> > > > up the next modeset?
> > > 
> > > What I wanted is to force a reprogramming in this case, but yea we
> > > would miss then the proper disable sequence. AFAICS we can't mark the
> > > state here as enabled but not valid, so for now I will just change the
> > > above to DRM_ERROR then and report an enabled state as you suggested.
> > 
> > Yeah. So far we've done any reprogramming in the sanitize phase, but
> > that may require splitting/duplicating some parts of state readout. Maybe
> > we should allow .get_config() & co. to flag the state as "this needs a
> > forced modeset to sanitize things"?
> 
> Yea, sounds good to me. Since this a new check in any case could we add
> this as a follow-up?

Yes. This was more of a food for thought type of suggestion, so wasn't
expecting that it would happen right away.

> 
> > > > So maybe just ERROR/WARN in these cases?
> > > > 
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > >  	intel_display_power_put(dev_priv, power_domain);
> > > > >  
> > > > >  	return ret;
> > > > > @@ -1745,6 +1755,8 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
> > > > >  bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
> > > > >  			    enum dpio_phy phy)
> > > > >  {
> > > > > +	enum port port;
> > > > > +
> > > > >  	if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy)))
> > > > >  		return false;
> > > > >  
> > > > > @@ -1770,6 +1782,21 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
> > > > >  		return false;
> > > > >  	}
> > > > >  
> > > > > +	for_each_port_masked(port,
> > > > > +			     phy == DPIO_PHY0 ? BIT(PORT_B) | BIT(PORT_C) :
> > > > > +					        BIT(PORT_A)) {
> > > > > +		u32 tmp = I915_READ(BXT_PHY_CTL(port));
> > > > > +
> > > > > +		if (tmp & BXT_PHY_CMNLANE_POWERDOWN_ACK) {
> > > > > +			DRM_DEBUG_DRIVER("DDI PHY %d powered, but common lane "
> > > > > +					 "for port %c powered down "
> > > > > +					 "(PHY_CTL %08x)\n",
> > > > > +					 phy, port_name(port), tmp);
> > > > > +
> > > > > +			return false;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > >  	return true;
> > > > >  }
> > > > >  
> > > > > -- 
> > > > > 2.5.0
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > 

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

[PATCH v2 6/6] drm/i915/bxt: Sanitiy check the PHY lane power down status

[Imre Deak]

We can check the power state of the PHY data and common lanes as
reported by the PHY. Do this in case we need to debug problems where the
PHY gets stuck in an unexpected state.

Note that I only check these when the lanes are expected to be powered
on purpose, since it's not clear at what point the PHY power/clock gates
things.

v2:
- Don't report the encoder as disabled when the sanity check fails.
  (Ville)

CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  9 +++++++++
 drivers/gpu/drm/i915/intel_ddi.c | 25 +++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 81fc498..e904818 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1276,6 +1276,15 @@ enum skl_disp_power_wells {
 #define BXT_P_CR_GT_DISP_PWRON		_MMIO(0x138090)
 #define   GT_DISPLAY_POWER_ON(phy)	(1 << (phy))
 
+#define _BXT_PHY_CTL_DDI_A		0x64C00
+#define _BXT_PHY_CTL_DDI_B		0x64C10
+#define _BXT_PHY_CTL_DDI_C		0x64C20
+#define   BXT_PHY_CMNLANE_POWERDOWN_ACK	(1 << 10)
+#define   BXT_PHY_LANE_POWERDOWN_ACK	(1 << 9)
+#define   BXT_PHY_LANE_ENABLED		(1 << 8)
+#define BXT_PHY_CTL(port)		_MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \
+							 _BXT_PHY_CTL_DDI_B)
+
 #define _PHY_CTL_FAMILY_EDP		0x64C80
 #define _PHY_CTL_FAMILY_DDI		0x64C90
 #define   COMMON_RESET_DIS		(1 << 31)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index cb48b0d..bd0c46a 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1342,6 +1342,14 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
 	DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port));
 
 out:
+	if (ret && IS_BROXTON(dev_priv)) {
+		tmp = I915_READ(BXT_PHY_CTL(port));
+		if ((tmp & (BXT_PHY_LANE_POWERDOWN_ACK |
+			    BXT_PHY_LANE_ENABLED)) != BXT_PHY_LANE_ENABLED)
+			DRM_ERROR("Port %c enabled but PHY powered down? "
+				  "(PHY_CTL %08x)\n", port_name(port), tmp);
+	}
+
 	intel_display_power_put(dev_priv, power_domain);
 
 	return ret;
@@ -1745,6 +1753,8 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
 			    enum dpio_phy phy)
 {
+	enum port port;
+
 	if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy)))
 		return false;
 
@@ -1770,6 +1780,21 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
 		return false;
 	}
 
+	for_each_port_masked(port,
+			     phy == DPIO_PHY0 ? BIT(PORT_B) | BIT(PORT_C) :
+					        BIT(PORT_A)) {
+		u32 tmp = I915_READ(BXT_PHY_CTL(port));
+
+		if (tmp & BXT_PHY_CMNLANE_POWERDOWN_ACK) {
+			DRM_DEBUG_DRIVER("DDI PHY %d powered, but common lane "
+					 "for port %c powered down "
+					 "(PHY_CTL %08x)\n",
+					 phy, port_name(port), tmp);
+
+			return false;
+		}
+	}
+
 	return true;
 }
 
-- 
2.5.0

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

✓ Ro.CI.BAT: success for drm/i915/bxt: Fix DDI PHY setup for low resolutions

[Patchwork]

== Series Details ==

Series: drm/i915/bxt: Fix DDI PHY setup for low resolutions
URL   : https://patchwork.freedesktop.org/series/8414/
State : success

== Summary ==

Series 8414v1 drm/i915/bxt: Fix DDI PHY setup for low resolutions
http://patchwork.freedesktop.org/api/1.0/series/8414/revisions/1/mbox


fi-bdw-i7-5557u  total:102  pass:93   dwarn:0   dfail:0   fail:0   skip:8  
fi-skl-i5-6260u  total:209  pass:198  dwarn:0   dfail:0   fail:0   skip:11 
fi-skl-i7-6700k  total:209  pass:184  dwarn:0   dfail:0   fail:0   skip:25 
fi-snb-i7-2600   total:209  pass:170  dwarn:0   dfail:0   fail:0   skip:39 
ro-bdw-i5-5250u  total:183  pass:172  dwarn:0   dfail:0   fail:0   skip:10 
ro-bdw-i7-5600u  total:93   pass:85   dwarn:0   dfail:0   fail:0   skip:8  
ro-bsw-n3050     total:208  pass:167  dwarn:0   dfail:0   fail:2   skip:39 
ro-byt-n2820     total:208  pass:168  dwarn:0   dfail:0   fail:3   skip:37 
ro-hsw-i3-4010u  total:208  pass:185  dwarn:0   dfail:0   fail:0   skip:23 
ro-hsw-i7-4770r  total:183  pass:161  dwarn:0   dfail:0   fail:0   skip:21 
ro-ilk1-i5-650   total:204  pass:146  dwarn:0   dfail:0   fail:1   skip:57 
ro-ivb-i7-3770   total:183  pass:154  dwarn:0   dfail:0   fail:0   skip:28 
ro-ivb2-i7-3770  total:183  pass:158  dwarn:0   dfail:0   fail:0   skip:24 
ro-snb-i7-2620M  total:183  pass:151  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-i7-4770k failed to connect after reboot
ro-bdw-i7-5557U failed to connect after reboot
ro-bdw-i7-5600u failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1133/

131a54b drm-intel-nightly: 2016y-06m-07d-19h-46m-33s UTC integration manifest
9c8fe8f drm/i915/bxt: Sanitiy check the PHY lane power down status
11da1b8 drm/i915/bxt: Rename broxton to bxt in PHY/CDCLK function prefixes
218803a drm/i915/bxt: Set DDI PHY lane latency optimization during modeset
bda698f drm/i915/bxt: Move DDI PHY enabling/disabling to the power well code
765d6b6 drm/i915: Factor out intel_power_well_get/put
82df3ef drm/i915/bxt: Wait for PHY1 GRC calibration synchronously

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

✗ Ro.CI.BAT: failure for drm/i915/bxt: Fix DDI PHY setup for low resolutions (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915/bxt: Fix DDI PHY setup for low resolutions (rev4)
URL   : https://patchwork.freedesktop.org/series/8414/
State : failure

== Summary ==

Applying: drm/i915/bxt: Wait for PHY1 GRC calibration synchronously
Applying: drm/i915/bxt: Sanitiy check the PHY lane power down status
fatal: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_ddi.c).
error: could not build fake ancestor
Patch failed at 0002 drm/i915/bxt: Sanitiy check the PHY lane power down status
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

Re: ✗ Ro.CI.BAT: failure for drm/i915/bxt: Fix DDI PHY setup for low resolutions (rev4)

[Imre Deak]

Somehow threading seems to be broken, v2 of 6/6 is applied out of order
after 1/6. Also I can't see v2 of 2/6 and v2 of 4/6 at [1] which I sent
along with v2 of 6/6. AFAICS the In-reply-to fields were correct in the
v2 versions I sent.

--Imre

[1] https://patchwork.freedesktop.org/series/8414/

> == Series Details ==
> 
> Series: drm/i915/bxt: Fix DDI PHY setup for low resolutions (rev4)
> URL   : https://patchwork.freedesktop.org/series/8414/
> State : failure
>
> == Summary ==
>
> Applying: drm/i915/bxt: Wait for PHY1 GRC calibration synchronously
> Applying: drm/i915/bxt: Sanitiy check the PHY lane power down status
> fatal: sha1 information is lacking or useless(drivers/gpu/drm/i915/intel_ddi.c).
> error: could not build fake ancestor
> Patch failed at 0002 drm/i915/bxt: Sanitiy check the PHY lane power down status
> The copy of the patch that failed is found in: .git/rebase-apply/patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/6] drm/i915/bxt: Fix DDI PHY setup for low resolutions

[Ville Syrjälä]

On Tue, Jun 07, 2016 at 09:24:27PM +0300, Imre Deak wrote:
> There are two problems with the current way of enabling the DDI PHYs
> during driver loading/resuming:
> Relying on the HWs dynamic power gating may waste some power and part of
> the PHY configuration is dependent on the mode specific DDI lane count.
> To solve both of these issues split the PHY initialization, moving one
> half of it to the power well code the other half to the modeset code,
> similarly to the CHV code.
> 
> Kudos to Ville for explaining about the PHY power gating and other
> quirks on CHV, it helped a lot to better understand the BXT PHY which is
> quite similar.
> 
> This fixes modeset problems for modes with less than 4 lanes.
> 
> Imre Deak (6):
>   drm/i915/bxt: Wait for PHY1 GRC calibration synchronously
>   drm/i915: Factor out intel_power_well_get/put
>   drm/i915/bxt: Move DDI PHY enabling/disabling to the power well code
>   drm/i915/bxt: Set DDI PHY lane latency optimization during modeset
>   drm/i915/bxt: Rename broxton to bxt in PHY/CDCLK function prefixes
>   drm/i915/bxt: Sanitiy check the PHY lane power down status

Series lgtm

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
>  drivers/gpu/drm/i915/i915_reg.h         |  12 ++
>  drivers/gpu/drm/i915/intel_ddi.c        | 222 ++++++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_display.c    |  33 +++--
>  drivers/gpu/drm/i915/intel_drv.h        |  19 ++-
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 133 ++++++++++++++++---
>  5 files changed, 291 insertions(+), 128 deletions(-)
> 
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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