[RFCv2 DP-typeC 0/6] Add support for USB typeC based DP

[RFCv2 DP-typeC 0/6] Add support for USB typeC based DP

[Durgadoss R]

This is an RFC series to start the review/discussion on approach
to support USB type C based DP panel.

To support USB type C alternate DP mode, the display driver needs to
know the number of lanes required by the DP panel as well as number
of lanes that can be supported by the type-C cable. Sometimes, the
type-C cable may limit the bandwidth even if Panel can support
more lanes.

The goal is to find out the number of lanes which can be supported
using a particular cable so that we can cap 'max_available_lanes'
to that number during modeset.

The BXT patches are based on 4.2-rc2 and tested on BXT A1.
CHV implementation is tested on 3.18. Both are tested only
in non-MST mode.

The 1/6 to 4/6 are refactoring/exporting functions required
to do upfront link train for DDI platforms from intel_ddi.c.
Patch 5/6 is upfront implementation for BXT and 6/6 is for CHV.

Brief summary of the approach taken:
-----------------------------------
1.As soon as DP-hotplug is detected, driver starts link training
  with highest number of lanes/bandwidth possible. If it fails,
  driver retries link training with lane/2 for same bandwidth.
  We continue this procedure until we find a working configuration
  of lane/bandwidth values. This 'number of lanes' is then
  set as the 'max_available_lanes', so that the following
  intel_dp_compute_config() during modeset picks it up as
  max_lane_count (instead of 4 always, from DPCD).

2.Since we do only link training on hotplug, only the port
  and its PLLs are enabled/disabled without touching pipe/
  planes etc.

3.For scenarios where we boot with DP connected (along with
  an LFP like MIPI/eDP) we disable the crtc and then start
  link training, since BIOS brings up DP. The crtc is
  enabled back during subsequent modeset. This needs some
  changes for latest -nightly branch since we do not have
  intel_crtc_control() anymore.

4.Since we are doing the link training on hotplug (as
  opposed to during modeset) we named the function
  '{chv/bxt/*}_upfront_link_train'. We can also think
  of a virtual func for this, inside intel_encoder.

As per Daniel's suggestion on RFCv1:
* Added the last patch 6/6 that has implementation for CHV
* Made intel_dp_upfront_link_train as common for all platforms
  and added a intel_ddi_upfront_* for all DDI platforms.
  Currently, have restricted it to only for SKL/BXT.
* Moved the upfront code for DDI platforms into intel_ddi.c
  from display.c, since that aligned better with other
  ddi* functions.
* Kept the CHV implementation in display.c as of now
  since we are using some pll functions defined in display.c
  We can discuss and finalize an appropriate place for this
  and then refactor/export required functions.

Durgadoss R (6):
  drm/i915/dp: Reuse encoder if it is already available
  drm/i915/dp: Reuse shared DPLL if it exists already
  drm/i915/dp: Abstract all get_ddi_pll methods
  drm/i915/dp: Export enable/disable_shared_dpll methods
  drm/i915/dp: Enable Upfront link training for typeC DP support on BXT
  drm/i915/dp: Enable Upfront link training for typeC DP support on CHV

 drivers/gpu/drm/i915/intel_ddi.c     | 231 ++++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_display.c | 159 ++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_dp.c      |  43 ++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  11 +-
 4 files changed, 401 insertions(+), 43 deletions(-)

-- 
1.9.1

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

[RFCv2 DP-typeC 1/6] drm/i915/dp: Reuse encoder if it is already available

[Durgadoss R]

We do not need to loop through crtc_state to get the
encoder if we already have a valid one available.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     | 11 ++++++++---
 drivers/gpu/drm/i915/intel_display.c |  2 +-
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index b25e99a..9098c12 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1762,11 +1762,16 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
  * function should be folded into compute_config() eventually.
  */
 bool intel_ddi_pll_select(struct intel_crtc *intel_crtc,
-			  struct intel_crtc_state *crtc_state)
+			  struct intel_crtc_state *crtc_state,
+			  struct intel_encoder *valid_encoder)
 {
 	struct drm_device *dev = intel_crtc->base.dev;
-	struct intel_encoder *intel_encoder =
-		intel_ddi_get_crtc_new_encoder(crtc_state);
+	struct intel_encoder *intel_encoder;
+
+	if (valid_encoder)
+		intel_encoder = valid_encoder;
+	else
+		intel_encoder = intel_ddi_get_crtc_new_encoder(crtc_state);
 
 	if (IS_SKYLAKE(dev))
 		return skl_ddi_pll_select(intel_crtc, crtc_state,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cddb0c6..8ae6d7b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9661,7 +9661,7 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state)
 static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
 				      struct intel_crtc_state *crtc_state)
 {
-	if (!intel_ddi_pll_select(crtc, crtc_state))
+	if (!intel_ddi_pll_select(crtc, crtc_state, NULL))
 		return -EINVAL;
 
 	crtc->lowfreq_avail = false;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 91b6b40..0822ab6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -989,7 +989,8 @@ void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
 void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc);
 void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc);
 bool intel_ddi_pll_select(struct intel_crtc *crtc,
-			  struct intel_crtc_state *crtc_state);
+			  struct intel_crtc_state *crtc_state,
+			  struct intel_encoder *encoder);
 void intel_ddi_set_pipe_settings(struct drm_crtc *crtc);
 void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder);
 bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
-- 
1.9.1

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

[RFCv2 DP-typeC 2/6] drm/i915/dp: Reuse shared DPLL if it exists already

[Durgadoss R]

Do not call intel_get_shared_dpll() if there exists a
valid shared DPLL already.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     | 70 ++++++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_display.c |  2 +-
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 3 files changed, 42 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 9098c12..8e4ea36 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1258,7 +1258,8 @@ hsw_ddi_calculate_wrpll(int clock /* in Hz */,
 static bool
 hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
 		   struct intel_crtc_state *crtc_state,
-		   struct intel_encoder *intel_encoder)
+		   struct intel_encoder *intel_encoder,
+		   bool find_dpll)
 {
 	int clock = crtc_state->port_clock;
 
@@ -1278,14 +1279,16 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
 
 		crtc_state->dpll_hw_state.wrpll = val;
 
-		pll = intel_get_shared_dpll(intel_crtc, crtc_state);
-		if (pll == NULL) {
-			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
-					 pipe_name(intel_crtc->pipe));
-			return false;
-		}
+		if (find_dpll) {
+			pll = intel_get_shared_dpll(intel_crtc, crtc_state);
+			if (pll == NULL) {
+				DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
+						 pipe_name(intel_crtc->pipe));
+				return false;
+			}
 
-		crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
+			crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
+		}
 	}
 
 	return true;
@@ -1540,7 +1543,8 @@ skip_remaining_dividers:
 static bool
 skl_ddi_pll_select(struct intel_crtc *intel_crtc,
 		   struct intel_crtc_state *crtc_state,
-		   struct intel_encoder *intel_encoder)
+		   struct intel_encoder *intel_encoder,
+		   bool find_dpll)
 {
 	struct intel_shared_dpll *pll;
 	uint32_t ctrl1, cfgcr1, cfgcr2;
@@ -1594,15 +1598,17 @@ skl_ddi_pll_select(struct intel_crtc *intel_crtc,
 	crtc_state->dpll_hw_state.cfgcr1 = cfgcr1;
 	crtc_state->dpll_hw_state.cfgcr2 = cfgcr2;
 
-	pll = intel_get_shared_dpll(intel_crtc, crtc_state);
-	if (pll == NULL) {
-		DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
-				 pipe_name(intel_crtc->pipe));
-		return false;
-	}
+	if (find_dpll) {
+		pll = intel_get_shared_dpll(intel_crtc, crtc_state);
+		if (pll == NULL) {
+			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
+					 pipe_name(intel_crtc->pipe));
+			return false;
+		}
 
-	/* shared DPLL id 0 is DPLL 1 */
-	crtc_state->ddi_pll_sel = pll->id + 1;
+		/* shared DPLL id 0 is DPLL 1 */
+		crtc_state->ddi_pll_sel = pll->id + 1;
+	}
 
 	return true;
 }
@@ -1632,7 +1638,8 @@ static const struct bxt_clk_div bxt_dp_clk_val[] = {
 static bool
 bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
 		   struct intel_crtc_state *crtc_state,
-		   struct intel_encoder *intel_encoder)
+		   struct intel_encoder *intel_encoder,
+		   bool find_pll)
 {
 	struct intel_shared_dpll *pll;
 	struct bxt_clk_div clk_div = {0};
@@ -1741,15 +1748,17 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
 	crtc_state->dpll_hw_state.pcsdw12 =
 		LANESTAGGER_STRAP_OVRD | lanestagger;
 
-	pll = intel_get_shared_dpll(intel_crtc, crtc_state);
-	if (pll == NULL) {
-		DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
-			pipe_name(intel_crtc->pipe));
-		return false;
-	}
+	if (find_pll) {
+		pll = intel_get_shared_dpll(intel_crtc, crtc_state);
+		if (pll == NULL) {
+			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
+				pipe_name(intel_crtc->pipe));
+			return false;
+		}
 
-	/* shared DPLL id 0 is DPLL A */
-	crtc_state->ddi_pll_sel = pll->id;
+		/* shared DPLL id 0 is DPLL A */
+		crtc_state->ddi_pll_sel = pll->id;
+	}
 
 	return true;
 }
@@ -1763,7 +1772,8 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
  */
 bool intel_ddi_pll_select(struct intel_crtc *intel_crtc,
 			  struct intel_crtc_state *crtc_state,
-			  struct intel_encoder *valid_encoder)
+			  struct intel_encoder *valid_encoder,
+			  bool find_dpll)
 {
 	struct drm_device *dev = intel_crtc->base.dev;
 	struct intel_encoder *intel_encoder;
@@ -1775,13 +1785,13 @@ bool intel_ddi_pll_select(struct intel_crtc *intel_crtc,
 
 	if (IS_SKYLAKE(dev))
 		return skl_ddi_pll_select(intel_crtc, crtc_state,
-					  intel_encoder);
+					  intel_encoder, find_dpll);
 	else if (IS_BROXTON(dev))
 		return bxt_ddi_pll_select(intel_crtc, crtc_state,
-					  intel_encoder);
+					  intel_encoder, find_dpll);
 	else
 		return hsw_ddi_pll_select(intel_crtc, crtc_state,
-					  intel_encoder);
+					  intel_encoder, find_dpll);
 }
 
 void intel_ddi_set_pipe_settings(struct drm_crtc *crtc)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8ae6d7b..5c2b4ce 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9661,7 +9661,7 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state)
 static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
 				      struct intel_crtc_state *crtc_state)
 {
-	if (!intel_ddi_pll_select(crtc, crtc_state, NULL))
+	if (!intel_ddi_pll_select(crtc, crtc_state, NULL, true))
 		return -EINVAL;
 
 	crtc->lowfreq_avail = false;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0822ab6..cbcfa14 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -990,7 +990,7 @@ void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc);
 void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc);
 bool intel_ddi_pll_select(struct intel_crtc *crtc,
 			  struct intel_crtc_state *crtc_state,
-			  struct intel_encoder *encoder);
+			  struct intel_encoder *encoder, bool find_dpll);
 void intel_ddi_set_pipe_settings(struct drm_crtc *crtc);
 void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder);
 bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
-- 
1.9.1

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

[RFCv2 DP-typeC 3/6] drm/i915/dp: Abstract all get_ddi_pll methods

[Durgadoss R]

This patch wraps the get_ddi_pll() methods for
SKL/BXT/HSW+ with a common intel_get_ddi_pll()
method, and exports it, so that it can be shared
by other users also.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++------
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5c2b4ce..e3f1201 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9738,6 +9738,17 @@ static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv,
 	}
 }
 
+void intel_get_ddi_pll(struct drm_i915_private *dev_priv, enum port port,
+			struct intel_crtc_state *pipe_config)
+{
+	if (IS_SKYLAKE(dev_priv))
+		skylake_get_ddi_pll(dev_priv, port, pipe_config);
+	else if (IS_BROXTON(dev_priv))
+		bxt_get_ddi_pll(dev_priv, port, pipe_config);
+	else
+		haswell_get_ddi_pll(dev_priv, port, pipe_config);
+}
+
 static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
 				       struct intel_crtc_state *pipe_config)
 {
@@ -9751,12 +9762,7 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
 
 	port = (tmp & TRANS_DDI_PORT_MASK) >> TRANS_DDI_PORT_SHIFT;
 
-	if (IS_SKYLAKE(dev))
-		skylake_get_ddi_pll(dev_priv, port, pipe_config);
-	else if (IS_BROXTON(dev))
-		bxt_get_ddi_pll(dev_priv, port, pipe_config);
-	else
-		haswell_get_ddi_pll(dev_priv, port, pipe_config);
+	intel_get_ddi_pll(dev_priv, port, pipe_config);
 
 	if (pipe_config->shared_dpll >= 0) {
 		pll = &dev_priv->shared_dplls[pipe_config->shared_dpll];
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cbcfa14..ee36ddf 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -991,6 +991,8 @@ void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc);
 bool intel_ddi_pll_select(struct intel_crtc *crtc,
 			  struct intel_crtc_state *crtc_state,
 			  struct intel_encoder *encoder, bool find_dpll);
+void intel_get_ddi_pll(struct drm_i915_private *dev_priv, enum port port,
+			struct intel_crtc_state *pipe_config);
 void intel_ddi_set_pipe_settings(struct drm_crtc *crtc);
 void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder);
 bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
-- 
1.9.1

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

[RFCv2 DP-typeC 4/6] drm/i915/dp: Export enable/disable_shared_dpll methods

[Durgadoss R]

This patch exports the intel_{enable/disable}_shared_dpll
methods so that they can be called from other files also.
Subsequent patches need to call this from intel_ddi.c

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 drivers/gpu/drm/i915/intel_drv.h     | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e3f1201..a2ec8cb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1910,7 +1910,7 @@ static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
  * The PCH PLL needs to be enabled before the PCH transcoder, since it
  * drives the transcoder clock.
  */
-static void intel_enable_shared_dpll(struct intel_crtc *crtc)
+void intel_enable_shared_dpll(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1940,7 +1940,7 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
 	pll->on = true;
 }
 
-static void intel_disable_shared_dpll(struct intel_crtc *crtc)
+void intel_disable_shared_dpll(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ee36ddf..5bcdd37 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1116,6 +1116,8 @@ void intel_create_rotation_property(struct drm_device *dev,
 
 /* shared dpll functions */
 struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
+void intel_enable_shared_dpll(struct intel_crtc *crtc);
+void intel_disable_shared_dpll(struct intel_crtc *crtc);
 void assert_shared_dpll(struct drm_i915_private *dev_priv,
 			struct intel_shared_dpll *pll,
 			bool state);
-- 
1.9.1

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

[RFCv2 DP-typeC 5/6] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[Durgadoss R]

To support USB type C alternate DP mode, the display driver needs to
know the number of lanes required by the DP panel as well as number
of lanes that can be supported by the type-C cable. Sometimes, the
type-C cable may limit the bandwidth even if Panel can support
more lanes. To address these scenarios, the display driver will
start link training with max lanes, and if that fails, the driver
falls back to x2 lanes; and repeats this procedure for all
bandwidth/lane configurations.

* Since link training is done before modeset only the port
  (and not pipe/planes) and its associated PLLs are enabled.
* Once link training is done, the port and its PLLs are disabled;
  so that the subsequent modeset is not aware of these changes.
* On DP hotplug: Directly start link training on the crtc
  associated with the DP encoder.
* On Connected boot scenarios: When booted with an LFP and a DP,
  typically, BIOS brings up DP. In these cases, we disable the
  crtc first and then start upfront link training. The crtc is
  re-enabled as part of a subsequent modeset.
* For BXT, ddi->enable/disable for DP only enable/disable
  audio codec and hence are not included in upfront link
  training sequence.
* As of now, this is tested only on BXT A1 platform, on
  kernel 4.2-rc2.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 152 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c  |  41 ++++++++++-
 drivers/gpu/drm/i915/intel_drv.h |   2 +
 3 files changed, 194 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 8e4ea36..b3a9bff 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3209,6 +3209,158 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port)
 	return connector;
 }
 
+bool intel_ddi_upfront_link_train(struct drm_device *dev,
+		struct intel_dp *intel_dp, struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct intel_connector *connector = intel_dp->attached_connector;
+	struct intel_encoder *tmp_encoder, *encoder = connector->encoder;
+	struct intel_shared_dpll *pll;
+	struct intel_crtc *tmp_crtc;
+	struct drm_crtc *tmp_drm_crtc;
+	uint8_t tmp_lane_count, tmp_link_bw;
+	bool ret, found, valid_crtc = false;
+
+	/* For now, we have only SKL and BXT supporting type-C */
+	if (!IS_BROXTON(dev) || !IS_SKYLAKE(dev))
+		return true;
+
+	if (!connector || !encoder) {
+		DRM_DEBUG_KMS("dp connector/encoder is NULL\n");
+		return false;
+	}
+
+	/* If we already have a crtc, start link training directly */
+	if (crtc) {
+		valid_crtc = true;
+		goto start_link_train;
+	}
+
+	/* Find an unused crtc and use it for link training */
+	for_each_intel_crtc(dev, crtc) {
+		if (intel_crtc_active(&crtc->base))
+			continue;
+
+		/* Make sure the new crtc will work with the encoder */
+		if (drm_encoder_crtc_ok(&encoder->base, &crtc->base)) {
+			found = true;
+
+			/* Save the existing values */
+			tmp_encoder = connector->new_encoder;
+			tmp_crtc = encoder->new_crtc;
+			tmp_drm_crtc = encoder->base.crtc;
+
+			connector->new_encoder = encoder;
+			encoder->new_crtc = crtc;
+			encoder->base.crtc = &crtc->base;
+
+			break;
+		}
+	}
+
+	if (!found) {
+		DRM_ERROR("Could not find crtc for upfront link training\n");
+		return false;
+	}
+
+start_link_train:
+	DRM_DEBUG_KMS("upfront link train on pipe:%c\n", pipe_name(crtc->pipe));
+	found = false;
+
+	/* Save the existing lane_count and link_bw values */
+	tmp_lane_count = intel_dp->lane_count;
+	tmp_link_bw = intel_dp->link_bw;
+
+	/* Initialize with Max Link rate & lane count supported by panel */
+	intel_dp->link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
+	intel_dp->lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] &
+					DP_MAX_LANE_COUNT_MASK;
+
+	/* Selects the shared DPLL to use for this port */
+	intel_get_ddi_pll(dev_priv, dig_port->port, crtc->config);
+	pll = intel_crtc_to_shared_dpll(crtc);
+	if (!pll) {
+		DRM_ERROR("Could not get shared DPLL\n");
+		goto exit;
+	}
+
+	do {
+		/* Find port clock from link_bw */
+		crtc->config->port_clock =
+				drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
+
+		ret = intel_ddi_pll_select(crtc, crtc->config, encoder, false);
+		if (!ret) {
+			DRM_ERROR("Could not select PLL\n");
+			goto exit;
+		}
+
+		pll->config.crtc_mask = (1 << crtc->pipe);
+		pll->config.hw_state = crtc->config->dpll_hw_state;
+
+		DRM_DEBUG_KMS("Using shared_dpll:%d\n", crtc->config->shared_dpll);
+
+		/* Enable PLL followed by port */
+		intel_enable_shared_dpll(crtc);
+		encoder->pre_enable(encoder);
+
+		/* Check if link training passed; if so update lane count */
+		if (intel_dp->train_set_valid) {
+			intel_dp->dpcd[DP_MAX_LANE_COUNT] &=
+						~DP_MAX_LANE_COUNT_MASK;
+			intel_dp->dpcd[DP_MAX_LANE_COUNT] |=
+				intel_dp->lane_count & DP_MAX_LANE_COUNT_MASK;
+
+			found = true;
+		}
+
+		/* Disable port followed by PLL for next retry/clean up */
+		encoder->post_disable(encoder);
+		intel_disable_shared_dpll(crtc);
+
+		if (found)
+			goto exit;
+
+		DRM_DEBUG_KMS("upfront link training failed. lanes:%d bw:%d\n",
+				intel_dp->lane_count, intel_dp->link_bw);
+
+		/* Go down to the next level and retry link training */
+		if (intel_dp->lane_count == 4) {
+			intel_dp->lane_count = 2;
+		} else if (intel_dp->lane_count == 2) {
+			intel_dp->lane_count = 1;
+		} else if (intel_dp->link_bw == DP_LINK_BW_5_4) {
+			intel_dp->link_bw = DP_LINK_BW_2_7;
+			intel_dp->lane_count = 4;
+		} else if (intel_dp->link_bw == DP_LINK_BW_2_7) {
+			intel_dp->link_bw = DP_LINK_BW_1_62;
+			intel_dp->lane_count = 4;
+		} else {
+			/* Tried all combinations, so exit */
+			break;
+		}
+
+	} while (1);
+
+exit:
+	/* Restore local associations made */
+	if (!valid_crtc) {
+		connector->new_encoder = tmp_encoder;
+		encoder->new_crtc = tmp_crtc;
+		encoder->base.crtc = tmp_drm_crtc;
+	}
+
+	if (found)
+		DRM_DEBUG_KMS("upfront link training passed. lanes:%d bw:%d\n",
+				intel_dp->lane_count, intel_dp->link_bw);
+	/* Restore lane_count and link_bw values */
+	intel_dp->lane_count = tmp_lane_count;
+	intel_dp->link_bw = tmp_link_bw;
+
+	return found;
+}
+
 void intel_ddi_init(struct drm_device *dev, enum port port)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 18bcfbe..8376b47 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4785,6 +4785,35 @@ intel_dp_power_put(struct intel_dp *dp,
 	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
 }
 
+static bool intel_dp_upfront_link_train(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct drm_device *dev = intel_encoder->base.dev;
+	struct intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) : NULL;
+
+	/*
+	 * On hotplug cases, we call _upfront_link_train directly.
+	 * In connected boot scenarios (boot with {MIPI/eDP} + DP),
+	 * BIOS typically brings up DP. Hence, we disable the crtc
+	 * to do _upfront_link_training. It gets re-enabled as part of
+	 * subsequent modeset.
+	 */
+	if (intel_encoder->connectors_active && crtc && crtc->enabled) {
+		DRM_DEBUG_KMS("Disabling crtc %c for upfront link training\n",
+				pipe_name(intel_crtc->pipe));
+		intel_crtc_control(crtc, false);
+	}
+
+	if (HAS_DDI(dev))
+		return intel_ddi_upfront_link_train(dev, intel_dp, intel_crtc);
+
+	/* Not a supported platform. Assume we don't need upfront_train */
+	return true;
+}
+
+
 static enum drm_connector_status
 intel_dp_detect(struct drm_connector *connector, bool force)
 {
@@ -4794,7 +4823,7 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	struct drm_device *dev = connector->dev;
 	enum drm_connector_status status;
 	enum intel_display_power_domain power_domain;
-	bool ret;
+	bool ret, do_upfront_link_train;
 	u8 sink_irq_vector;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
@@ -4852,6 +4881,16 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
 
+	/* Do not do upfront link train, if it is a compliance request */
+	do_upfront_link_train =
+		intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT &&
+		intel_dp->compliance_test_type != DP_TEST_LINK_TRAINING;
+
+	if (do_upfront_link_train) {
+		ret = intel_dp_upfront_link_train(intel_dp);
+		if (!ret)
+			status = connector_status_disconnected;
+	}
 out:
 	intel_dp_power_put(intel_dp, power_domain);
 	return status;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5bcdd37..82af4e6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1007,6 +1007,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder,
 			 struct intel_crtc_state *pipe_config);
 void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
 uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
+bool intel_ddi_upfront_link_train(struct drm_device *dev,
+		struct intel_dp *intel_dp, struct intel_crtc *crtc);
 
 /* intel_frontbuffer.c */
 void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
-- 
1.9.1

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

[RFCv2 DP-typeC 6/6] drm/i915/dp: Enable Upfront link training for typeC DP support on CHV

[Durgadoss R]

To support USB type C alternate DP mode, the display driver needs to know the
number of lanes required by DP panel as well as number of lanes that can be
supported by the type-C cable. Sometimes, the type-C cable may limit the
bandwidth even if Panel can support more lanes. To address these scenarios,
the display driver will start link training with max lanes, and if the link
training fails, the driver then falls back to x2 lanes.

* Since link training is done before modeset, planes are not enabled. Only
  encoder and the its associated PLLs are enabled.
* Once link training is done, the encoder and its PLLs are disabled; so that
  the subsequent modeset is not aware of these changes.
* As of now, this is tested only on CHV.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 135 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c      |   2 +
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 3 files changed, 139 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a2ec8cb..17b3c7a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15751,3 +15751,138 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file)
 		spin_unlock_irq(&dev->event_lock);
 	}
 }
+
+bool chv_upfront_link_train(struct drm_device *dev,
+		struct intel_dp *intel_dp, struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_connector *connector = intel_dp->attached_connector;
+	struct intel_encoder *tmp_encoder, *encoder = connector->encoder;
+	struct intel_crtc *tmp_crtc;
+	struct drm_crtc *tmp_drm_crtc;
+	bool found = false, valid_crtc = false;
+	uint8_t tmp_lane_count, tmp_link_bw;
+
+	if (!connector || !encoder) {
+		DRM_DEBUG_KMS("dp connector/encoder is NULL\n");
+		return false;
+	}
+
+	/* If we already have a crtc, start link training directly */
+	if (crtc) {
+		valid_crtc = true;
+		goto start_link_train;
+	}
+
+	/* Find an unused crtc and use it for link training */
+	for_each_intel_crtc(dev, crtc) {
+		if (intel_crtc_active(&crtc->base))
+			continue;
+
+		/* Make sure the new crtc will work with the encoder */
+		if (drm_encoder_crtc_ok(&encoder->base, &crtc->base)) {
+			/* Save the existing values */
+			tmp_encoder = connector->new_encoder;
+			tmp_crtc = encoder->new_crtc;
+			tmp_drm_crtc = encoder->base.crtc;
+
+			connector->new_encoder = encoder;
+			encoder->new_crtc = crtc;
+			encoder->base.crtc = &crtc->base;
+
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		DRM_ERROR("Could not find crtc for upfront link training\n");
+		return false;
+	}
+
+start_link_train:
+
+	DRM_DEBUG_KMS("upfront link training on pipe:%c\n",
+					pipe_name(crtc->pipe));
+	found = false;
+
+	/* Save the existing lane_count and link_bw values */
+	tmp_lane_count = intel_dp->lane_count;
+	tmp_link_bw = intel_dp->link_bw;
+
+	/* Initialize with Max Link rate & lane count supported by panel */
+	intel_dp->link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
+	intel_dp->lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] &
+					DP_MAX_LANE_COUNT_MASK;
+
+	/* CHV does not support HBR2 */
+	if (intel_dp->link_bw == DP_LINK_BW_5_4)
+		intel_dp->link_bw = DP_LINK_BW_2_7;
+
+	do {
+		/* Find port clock from link_bw */
+		crtc->config.port_clock =
+				drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
+
+		/* Enable PLL followed by port */
+		intel_dp_set_clock(encoder, &crtc->config, intel_dp->link_bw);
+		chv_update_pll(crtc);
+		encoder->pre_pll_enable(encoder);
+		chv_enable_pll(crtc);
+		encoder->pre_enable(encoder);
+
+		/* Check if link training passed; if so update lane count */
+		/* TODO: Newer code has this variable as 'train_set_valid */
+		if (intel_dp->has_fast_link_train) {
+			intel_dp->dpcd[DP_MAX_LANE_COUNT] &=
+						~DP_MAX_LANE_COUNT_MASK;
+			intel_dp->dpcd[DP_MAX_LANE_COUNT] |=
+				intel_dp->lane_count & DP_MAX_LANE_COUNT_MASK;
+
+			found = true;
+		}
+
+		/* Reset encoder for next retry or for clean up */
+		encoder->disable(encoder);
+		encoder->post_disable(encoder);
+		chv_disable_pll(dev_priv, crtc->pipe);
+
+		if (found)
+			goto exit;
+
+		DRM_DEBUG_KMS("upfront link training failed. lanes:%d bw:%d\n",
+				intel_dp->lane_count, intel_dp->link_bw);
+
+		/* Go down to the next level and retry link training */
+		if (intel_dp->lane_count == 4) {
+			intel_dp->lane_count = 2;
+		} else if (intel_dp->lane_count == 2) {
+			intel_dp->lane_count = 1;
+		} else if (intel_dp->link_bw == DP_LINK_BW_2_7) {
+			intel_dp->link_bw = DP_LINK_BW_1_62;
+			intel_dp->lane_count = 4;
+		} else {
+			/* Tried all combinations, so exit */
+			break;
+		}
+
+	} while (1);
+
+exit:
+	/* Clear local associations made */
+	if (!valid_crtc) {
+		connector->new_encoder = tmp_encoder;
+		encoder->new_crtc = tmp_crtc;
+		encoder->base.crtc = tmp_drm_crtc;
+	}
+
+	if (found)
+		DRM_DEBUG_KMS("upfront link training passed. lanes:%d bw:%d\n",
+				intel_dp->lane_count, intel_dp->link_bw);
+
+	/* Restore lane_count and link_bw values */
+	intel_dp->lane_count = tmp_lane_count;
+	intel_dp->link_bw = tmp_link_bw;
+
+	return found;
+}
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8376b47..0254395 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4808,6 +4808,8 @@ static bool intel_dp_upfront_link_train(struct intel_dp *intel_dp)
 
 	if (HAS_DDI(dev))
 		return intel_ddi_upfront_link_train(dev, intel_dp, intel_crtc);
+	else if (IS_CHERRYVIEW(dev))
+		return intel_chv_upfront_link_train(dev, intel_dp, intel_crtc);
 
 	/* Not a supported platform. Assume we don't need upfront_train */
 	return true;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 82af4e6..c214797 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1166,6 +1166,8 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
 void intel_dp_get_m_n(struct intel_crtc *crtc,
 		      struct intel_crtc_state *pipe_config);
 void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n);
+bool intel_chv_upfront_link_train(struct drm_device *dev,
+		struct intel_dp *intel_dp, struct intel_crtc *crtc);
 int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n);
 void
 ironlake_check_encoder_dotclock(const struct intel_crtc_state *pipe_config,
-- 
1.9.1

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

Re: [RFCv2 DP-typeC 2/6] drm/i915/dp: Reuse shared DPLL if it exists already

[Jani Nikula]

On Wed, 14 Oct 2015, Durgadoss R <durgadoss.r@intel.com> wrote:
> Do not call intel_get_shared_dpll() if there exists a
> valid shared DPLL already.
>
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     | 70 ++++++++++++++++++++----------------
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>  3 files changed, 42 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 9098c12..8e4ea36 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1258,7 +1258,8 @@ hsw_ddi_calculate_wrpll(int clock /* in Hz */,
>  static bool
>  hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>  		   struct intel_crtc_state *crtc_state,
> -		   struct intel_encoder *intel_encoder)
> +		   struct intel_encoder *intel_encoder,
> +		   bool find_dpll)
>  {
>  	int clock = crtc_state->port_clock;
>  
> @@ -1278,14 +1279,16 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>  
>  		crtc_state->dpll_hw_state.wrpll = val;
>  
> -		pll = intel_get_shared_dpll(intel_crtc, crtc_state);
> -		if (pll == NULL) {
> -			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> -					 pipe_name(intel_crtc->pipe));
> -			return false;
> -		}
> +		if (find_dpll) {
> +			pll = intel_get_shared_dpll(intel_crtc, crtc_state);
> +			if (pll == NULL) {
> +				DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> +						 pipe_name(intel_crtc->pipe));
> +				return false;
> +			}

You have to do a *lot* of passing around parameters here. I wonder (and
really, I don't know) if we could make intel_get_shared_dpll() grow
smarts about reusing the pll with intel_crtc_to_shared_dpll() when it's
there already.

BR,
Jani.


>  
> -		crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
> +			crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
> +		}
>  	}
>  
>  	return true;
> @@ -1540,7 +1543,8 @@ skip_remaining_dividers:
>  static bool
>  skl_ddi_pll_select(struct intel_crtc *intel_crtc,
>  		   struct intel_crtc_state *crtc_state,
> -		   struct intel_encoder *intel_encoder)
> +		   struct intel_encoder *intel_encoder,
> +		   bool find_dpll)
>  {
>  	struct intel_shared_dpll *pll;
>  	uint32_t ctrl1, cfgcr1, cfgcr2;
> @@ -1594,15 +1598,17 @@ skl_ddi_pll_select(struct intel_crtc *intel_crtc,
>  	crtc_state->dpll_hw_state.cfgcr1 = cfgcr1;
>  	crtc_state->dpll_hw_state.cfgcr2 = cfgcr2;
>  
> -	pll = intel_get_shared_dpll(intel_crtc, crtc_state);
> -	if (pll == NULL) {
> -		DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> -				 pipe_name(intel_crtc->pipe));
> -		return false;
> -	}
> +	if (find_dpll) {
> +		pll = intel_get_shared_dpll(intel_crtc, crtc_state);
> +		if (pll == NULL) {
> +			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> +					 pipe_name(intel_crtc->pipe));
> +			return false;
> +		}
>  
> -	/* shared DPLL id 0 is DPLL 1 */
> -	crtc_state->ddi_pll_sel = pll->id + 1;
> +		/* shared DPLL id 0 is DPLL 1 */
> +		crtc_state->ddi_pll_sel = pll->id + 1;
> +	}
>  
>  	return true;
>  }
> @@ -1632,7 +1638,8 @@ static const struct bxt_clk_div bxt_dp_clk_val[] = {
>  static bool
>  bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>  		   struct intel_crtc_state *crtc_state,
> -		   struct intel_encoder *intel_encoder)
> +		   struct intel_encoder *intel_encoder,
> +		   bool find_pll)
>  {
>  	struct intel_shared_dpll *pll;
>  	struct bxt_clk_div clk_div = {0};
> @@ -1741,15 +1748,17 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>  	crtc_state->dpll_hw_state.pcsdw12 =
>  		LANESTAGGER_STRAP_OVRD | lanestagger;
>  
> -	pll = intel_get_shared_dpll(intel_crtc, crtc_state);
> -	if (pll == NULL) {
> -		DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> -			pipe_name(intel_crtc->pipe));
> -		return false;
> -	}
> +	if (find_pll) {
> +		pll = intel_get_shared_dpll(intel_crtc, crtc_state);
> +		if (pll == NULL) {
> +			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> +				pipe_name(intel_crtc->pipe));
> +			return false;
> +		}
>  
> -	/* shared DPLL id 0 is DPLL A */
> -	crtc_state->ddi_pll_sel = pll->id;
> +		/* shared DPLL id 0 is DPLL A */
> +		crtc_state->ddi_pll_sel = pll->id;
> +	}
>  
>  	return true;
>  }
> @@ -1763,7 +1772,8 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>   */
>  bool intel_ddi_pll_select(struct intel_crtc *intel_crtc,
>  			  struct intel_crtc_state *crtc_state,
> -			  struct intel_encoder *valid_encoder)
> +			  struct intel_encoder *valid_encoder,
> +			  bool find_dpll)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
>  	struct intel_encoder *intel_encoder;
> @@ -1775,13 +1785,13 @@ bool intel_ddi_pll_select(struct intel_crtc *intel_crtc,
>  
>  	if (IS_SKYLAKE(dev))
>  		return skl_ddi_pll_select(intel_crtc, crtc_state,
> -					  intel_encoder);
> +					  intel_encoder, find_dpll);
>  	else if (IS_BROXTON(dev))
>  		return bxt_ddi_pll_select(intel_crtc, crtc_state,
> -					  intel_encoder);
> +					  intel_encoder, find_dpll);
>  	else
>  		return hsw_ddi_pll_select(intel_crtc, crtc_state,
> -					  intel_encoder);
> +					  intel_encoder, find_dpll);
>  }
>  
>  void intel_ddi_set_pipe_settings(struct drm_crtc *crtc)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8ae6d7b..5c2b4ce 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9661,7 +9661,7 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state)
>  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>  				      struct intel_crtc_state *crtc_state)
>  {
> -	if (!intel_ddi_pll_select(crtc, crtc_state, NULL))
> +	if (!intel_ddi_pll_select(crtc, crtc_state, NULL, true))
>  		return -EINVAL;
>  
>  	crtc->lowfreq_avail = false;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0822ab6..cbcfa14 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -990,7 +990,7 @@ void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc);
>  void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc);
>  bool intel_ddi_pll_select(struct intel_crtc *crtc,
>  			  struct intel_crtc_state *crtc_state,
> -			  struct intel_encoder *encoder);
> +			  struct intel_encoder *encoder, bool find_dpll);
>  void intel_ddi_set_pipe_settings(struct drm_crtc *crtc);
>  void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder);
>  bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFCv2 DP-typeC 5/6] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[kbuild test robot]

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

Hi Durgadoss,

[auto build test WARNING on drm-intel/for-linux-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Durgadoss-R/Add-support-for-USB-typeC-based-DP/20151014-193613
config: x86_64-randconfig-s5-10142016 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/uapi/linux/stddef.h:1:0,
                    from include/linux/stddef.h:4,
                    from include/uapi/linux/posix_types.h:4,
                    from include/uapi/linux/types.h:13,
                    from include/linux/types.h:5,
                    from include/linux/mod_devicetable.h:11,
                    from include/linux/i2c.h:29,
                    from drivers/gpu/drm/i915/intel_dp.c:28:
   drivers/gpu/drm/i915/intel_dp.c: In function 'intel_dp_upfront_link_train':
   drivers/gpu/drm/i915/intel_dp.c:4803:19: error: 'struct intel_encoder' has no member named 'connectors_active'
     if (intel_encoder->connectors_active && crtc && crtc->enabled) {
                      ^
   include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                               ^
>> drivers/gpu/drm/i915/intel_dp.c:4803:2: note: in expansion of macro 'if'
     if (intel_encoder->connectors_active && crtc && crtc->enabled) {
     ^
   drivers/gpu/drm/i915/intel_dp.c:4803:19: error: 'struct intel_encoder' has no member named 'connectors_active'
     if (intel_encoder->connectors_active && crtc && crtc->enabled) {
                      ^
   include/linux/compiler.h:147:40: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                                           ^
>> drivers/gpu/drm/i915/intel_dp.c:4803:2: note: in expansion of macro 'if'
     if (intel_encoder->connectors_active && crtc && crtc->enabled) {
     ^
   drivers/gpu/drm/i915/intel_dp.c:4803:19: error: 'struct intel_encoder' has no member named 'connectors_active'
     if (intel_encoder->connectors_active && crtc && crtc->enabled) {
                      ^
   include/linux/compiler.h:158:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^
>> drivers/gpu/drm/i915/intel_dp.c:4803:2: note: in expansion of macro 'if'
     if (intel_encoder->connectors_active && crtc && crtc->enabled) {
     ^
   drivers/gpu/drm/i915/intel_dp.c:4806:3: error: implicit declaration of function 'intel_crtc_control' [-Werror=implicit-function-declaration]
      intel_crtc_control(crtc, false);
      ^
   cc1: some warnings being treated as errors

vim +/if +4803 drivers/gpu/drm/i915/intel_dp.c

  4787	
  4788	static bool intel_dp_upfront_link_train(struct intel_dp *intel_dp)
  4789	{
  4790		struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
  4791		struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
  4792		struct intel_encoder *intel_encoder = &intel_dig_port->base;
  4793		struct drm_device *dev = intel_encoder->base.dev;
  4794		struct intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) : NULL;
  4795	
  4796		/*
  4797		 * On hotplug cases, we call _upfront_link_train directly.
  4798		 * In connected boot scenarios (boot with {MIPI/eDP} + DP),
  4799		 * BIOS typically brings up DP. Hence, we disable the crtc
  4800		 * to do _upfront_link_training. It gets re-enabled as part of
  4801		 * subsequent modeset.
  4802		 */
> 4803		if (intel_encoder->connectors_active && crtc && crtc->enabled) {
  4804			DRM_DEBUG_KMS("Disabling crtc %c for upfront link training\n",
  4805					pipe_name(intel_crtc->pipe));
  4806			intel_crtc_control(crtc, false);
  4807		}
  4808	
  4809		if (HAS_DDI(dev))
  4810			return intel_ddi_upfront_link_train(dev, intel_dp, intel_crtc);
  4811	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24235 bytes --]

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

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

Re: [RFCv2 DP-typeC 6/6] drm/i915/dp: Enable Upfront link training for typeC DP support on CHV

[kbuild test robot]

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

Hi Durgadoss,

[auto build test WARNING on drm-intel/for-linux-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Durgadoss-R/Add-support-for-USB-typeC-based-DP/20151014-193613
config: x86_64-randconfig-s5-10142016 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/i915/intel_display.c: In function 'chv_upfront_link_train':
   drivers/gpu/drm/i915/intel_display.c:15741:27: error: 'struct intel_connector' has no member named 'new_encoder'
       tmp_encoder = connector->new_encoder;
                              ^
   drivers/gpu/drm/i915/intel_display.c:15742:22: error: 'struct intel_encoder' has no member named 'new_crtc'
       tmp_crtc = encoder->new_crtc;
                         ^
   drivers/gpu/drm/i915/intel_display.c:15745:13: error: 'struct intel_connector' has no member named 'new_encoder'
       connector->new_encoder = encoder;
                ^
   drivers/gpu/drm/i915/intel_display.c:15746:11: error: 'struct intel_encoder' has no member named 'new_crtc'
       encoder->new_crtc = crtc;
              ^
   drivers/gpu/drm/i915/intel_display.c:15767:24: error: 'struct intel_dp' has no member named 'link_bw'
     tmp_link_bw = intel_dp->link_bw;
                           ^
   drivers/gpu/drm/i915/intel_display.c:15770:10: error: 'struct intel_dp' has no member named 'link_bw'
     intel_dp->link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
             ^
   In file included from include/uapi/linux/stddef.h:1:0,
                    from include/linux/stddef.h:4,
                    from include/uapi/linux/posix_types.h:4,
                    from include/uapi/linux/types.h:13,
                    from include/linux/types.h:5,
                    from include/linux/list.h:4,
                    from include/linux/dmi.h:4,
                    from drivers/gpu/drm/i915/intel_display.c:27:
   drivers/gpu/drm/i915/intel_display.c:15775:14: error: 'struct intel_dp' has no member named 'link_bw'
     if (intel_dp->link_bw == DP_LINK_BW_5_4)
                 ^
   include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                               ^
>> drivers/gpu/drm/i915/intel_display.c:15775:2: note: in expansion of macro 'if'
     if (intel_dp->link_bw == DP_LINK_BW_5_4)
     ^
   drivers/gpu/drm/i915/intel_display.c:15775:14: error: 'struct intel_dp' has no member named 'link_bw'
     if (intel_dp->link_bw == DP_LINK_BW_5_4)
                 ^
   include/linux/compiler.h:147:40: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                                           ^
>> drivers/gpu/drm/i915/intel_display.c:15775:2: note: in expansion of macro 'if'
     if (intel_dp->link_bw == DP_LINK_BW_5_4)
     ^
   drivers/gpu/drm/i915/intel_display.c:15775:14: error: 'struct intel_dp' has no member named 'link_bw'
     if (intel_dp->link_bw == DP_LINK_BW_5_4)
                 ^
   include/linux/compiler.h:158:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^
>> drivers/gpu/drm/i915/intel_display.c:15775:2: note: in expansion of macro 'if'
     if (intel_dp->link_bw == DP_LINK_BW_5_4)
     ^
   drivers/gpu/drm/i915/intel_display.c:15776:11: error: 'struct intel_dp' has no member named 'link_bw'
      intel_dp->link_bw = DP_LINK_BW_2_7;
              ^
   drivers/gpu/drm/i915/intel_display.c:15780:15: error: request for member 'port_clock' in something not a structure or union
      crtc->config.port_clock =
                  ^
   drivers/gpu/drm/i915/intel_display.c:15781:41: error: 'struct intel_dp' has no member named 'link_bw'
        drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
                                            ^
   drivers/gpu/drm/i915/intel_display.c:15784:3: error: implicit declaration of function 'intel_dp_set_clock' [-Werror=implicit-function-declaration]
      intel_dp_set_clock(encoder, &crtc->config, intel_dp->link_bw);
      ^
   drivers/gpu/drm/i915/intel_display.c:15784:54: error: 'struct intel_dp' has no member named 'link_bw'
      intel_dp_set_clock(encoder, &crtc->config, intel_dp->link_bw);
                                                         ^
   drivers/gpu/drm/i915/intel_display.c:15785:3: error: implicit declaration of function 'chv_update_pll' [-Werror=implicit-function-declaration]
      chv_update_pll(crtc);
      ^
   drivers/gpu/drm/i915/intel_display.c:15787:3: error: too few arguments to function 'chv_enable_pll'
      chv_enable_pll(crtc);
      ^
   drivers/gpu/drm/i915/intel_display.c:1636:13: note: declared here
    static void chv_enable_pll(struct intel_crtc *crtc,
                ^
   In file included from include/uapi/linux/stddef.h:1:0,
                    from include/linux/stddef.h:4,
                    from include/uapi/linux/posix_types.h:4,
                    from include/uapi/linux/types.h:13,
                    from include/linux/types.h:5,
                    from include/linux/list.h:4,
                    from include/linux/dmi.h:4,
                    from drivers/gpu/drm/i915/intel_display.c:27:
   drivers/gpu/drm/i915/intel_display.c:15792:15: error: 'struct intel_dp' has no member named 'has_fast_link_train'
      if (intel_dp->has_fast_link_train) {
                  ^
   include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                               ^
   drivers/gpu/drm/i915/intel_display.c:15792:3: note: in expansion of macro 'if'
      if (intel_dp->has_fast_link_train) {
      ^
   drivers/gpu/drm/i915/intel_display.c:15792:15: error: 'struct intel_dp' has no member named 'has_fast_link_train'
      if (intel_dp->has_fast_link_train) {
                  ^
   include/linux/compiler.h:147:40: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                                           ^
   drivers/gpu/drm/i915/intel_display.c:15792:3: note: in expansion of macro 'if'
      if (intel_dp->has_fast_link_train) {
      ^
   drivers/gpu/drm/i915/intel_display.c:15792:15: error: 'struct intel_dp' has no member named 'has_fast_link_train'
      if (intel_dp->has_fast_link_train) {
                  ^
   include/linux/compiler.h:158:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^
   drivers/gpu/drm/i915/intel_display.c:15792:3: note: in expansion of macro 'if'
      if (intel_dp->has_fast_link_train) {
      ^
   In file included from drivers/gpu/drm/i915/intel_display.c:35:0:
   drivers/gpu/drm/i915/intel_display.c:15810:35: error: 'struct intel_dp' has no member named 'link_bw'
        intel_dp->lane_count, intel_dp->link_bw);
                                      ^
   include/drm/drmP.h:208:41: note: in definition of macro 'DRM_DEBUG_KMS'
       drm_ut_debug_printk(__func__, fmt, ##args); \
                                            ^
   In file included from include/uapi/linux/stddef.h:1:0,
                    from include/linux/stddef.h:4,
                    from include/uapi/linux/posix_types.h:4,
                    from include/uapi/linux/types.h:13,
                    from include/linux/types.h:5,
                    from include/linux/list.h:4,
                    from include/linux/dmi.h:4,
                    from drivers/gpu/drm/i915/intel_display.c:27:
   drivers/gpu/drm/i915/intel_display.c:15817:22: error: 'struct intel_dp' has no member named 'link_bw'
      } else if (intel_dp->link_bw == DP_LINK_BW_2_7) {
                         ^
   include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                               ^
   drivers/gpu/drm/i915/intel_display.c:15817:10: note: in expansion of macro 'if'
      } else if (intel_dp->link_bw == DP_LINK_BW_2_7) {
             ^
   drivers/gpu/drm/i915/intel_display.c:15817:22: error: 'struct intel_dp' has no member named 'link_bw'
      } else if (intel_dp->link_bw == DP_LINK_BW_2_7) {
                         ^
   include/linux/compiler.h:147:40: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                                           ^
   drivers/gpu/drm/i915/intel_display.c:15817:10: note: in expansion of macro 'if'
      } else if (intel_dp->link_bw == DP_LINK_BW_2_7) {
             ^
   drivers/gpu/drm/i915/intel_display.c:15817:22: error: 'struct intel_dp' has no member named 'link_bw'
      } else if (intel_dp->link_bw == DP_LINK_BW_2_7) {
                         ^
   include/linux/compiler.h:158:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^

vim +/if +15775 drivers/gpu/drm/i915/intel_display.c

 15759	start_link_train:
 15760	
 15761		DRM_DEBUG_KMS("upfront link training on pipe:%c\n",
 15762						pipe_name(crtc->pipe));
 15763		found = false;
 15764	
 15765		/* Save the existing lane_count and link_bw values */
 15766		tmp_lane_count = intel_dp->lane_count;
 15767		tmp_link_bw = intel_dp->link_bw;
 15768	
 15769		/* Initialize with Max Link rate & lane count supported by panel */
 15770		intel_dp->link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
 15771		intel_dp->lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] &
 15772						DP_MAX_LANE_COUNT_MASK;
 15773	
 15774		/* CHV does not support HBR2 */
 15775		if (intel_dp->link_bw == DP_LINK_BW_5_4)
 15776			intel_dp->link_bw = DP_LINK_BW_2_7;
 15777	
 15778		do {
 15779			/* Find port clock from link_bw */
 15780			crtc->config.port_clock =
 15781					drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
 15782	
 15783			/* Enable PLL followed by port */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24235 bytes --]

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

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

Re: [RFCv2 DP-typeC 2/6] drm/i915/dp: Reuse shared DPLL if it exists already

[R, Durgadoss]

>-----Original Message-----
>From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>Sent: Wednesday, October 14, 2015 6:53 PM
>To: R, Durgadoss; intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [RFCv2 DP-typeC 2/6] drm/i915/dp: Reuse shared DPLL if it exists already
>
>On Wed, 14 Oct 2015, Durgadoss R <durgadoss.r@intel.com> wrote:
>> Do not call intel_get_shared_dpll() if there exists a
>> valid shared DPLL already.
>>
>> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c     | 70 ++++++++++++++++++++----------------
>>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>>  3 files changed, 42 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 9098c12..8e4ea36 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1258,7 +1258,8 @@ hsw_ddi_calculate_wrpll(int clock /* in Hz */,
>>  static bool
>>  hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>>  		   struct intel_crtc_state *crtc_state,
>> -		   struct intel_encoder *intel_encoder)
>> +		   struct intel_encoder *intel_encoder,
>> +		   bool find_dpll)
>>  {
>>  	int clock = crtc_state->port_clock;
>>
>> @@ -1278,14 +1279,16 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>>
>>  		crtc_state->dpll_hw_state.wrpll = val;
>>
>> -		pll = intel_get_shared_dpll(intel_crtc, crtc_state);
>> -		if (pll == NULL) {
>> -			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
>> -					 pipe_name(intel_crtc->pipe));
>> -			return false;
>> -		}
>> +		if (find_dpll) {
>> +			pll = intel_get_shared_dpll(intel_crtc, crtc_state);
>> +			if (pll == NULL) {
>> +				DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
>> +						 pipe_name(intel_crtc->pipe));
>> +				return false;
>> +			}
>
>You have to do a *lot* of passing around parameters here. I wonder (and
>really, I don't know) if we could make intel_get_shared_dpll() grow
>smarts about reusing the pll with intel_crtc_to_shared_dpll() when it's
>there already.

Sure. I will try it out and do this change if it works without issues.

Thanks,
Durga

>
>BR,
>Jani.
>
>
>>
>> -		crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
>> +			crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
>> +		}
>>  	}
>>
>>  	return true;
>> @@ -1540,7 +1543,8 @@ skip_remaining_dividers:
>>  static bool
>>  skl_ddi_pll_select(struct intel_crtc *intel_crtc,
>>  		   struct intel_crtc_state *crtc_state,
>> -		   struct intel_encoder *intel_encoder)
>> +		   struct intel_encoder *intel_encoder,
>> +		   bool find_dpll)
>>  {
>>  	struct intel_shared_dpll *pll;
>>  	uint32_t ctrl1, cfgcr1, cfgcr2;
>> @@ -1594,15 +1598,17 @@ skl_ddi_pll_select(struct intel_crtc *intel_crtc,
>>  	crtc_state->dpll_hw_state.cfgcr1 = cfgcr1;
>>  	crtc_state->dpll_hw_state.cfgcr2 = cfgcr2;
>>
>> -	pll = intel_get_shared_dpll(intel_crtc, crtc_state);
>> -	if (pll == NULL) {
>> -		DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
>> -				 pipe_name(intel_crtc->pipe));
>> -		return false;
>> -	}
>> +	if (find_dpll) {
>> +		pll = intel_get_shared_dpll(intel_crtc, crtc_state);
>> +		if (pll == NULL) {
>> +			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
>> +					 pipe_name(intel_crtc->pipe));
>> +			return false;
>> +		}
>>
>> -	/* shared DPLL id 0 is DPLL 1 */
>> -	crtc_state->ddi_pll_sel = pll->id + 1;
>> +		/* shared DPLL id 0 is DPLL 1 */
>> +		crtc_state->ddi_pll_sel = pll->id + 1;
>> +	}
>>
>>  	return true;
>>  }
>> @@ -1632,7 +1638,8 @@ static const struct bxt_clk_div bxt_dp_clk_val[] = {
>>  static bool
>>  bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>>  		   struct intel_crtc_state *crtc_state,
>> -		   struct intel_encoder *intel_encoder)
>> +		   struct intel_encoder *intel_encoder,
>> +		   bool find_pll)
>>  {
>>  	struct intel_shared_dpll *pll;
>>  	struct bxt_clk_div clk_div = {0};
>> @@ -1741,15 +1748,17 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>>  	crtc_state->dpll_hw_state.pcsdw12 =
>>  		LANESTAGGER_STRAP_OVRD | lanestagger;
>>
>> -	pll = intel_get_shared_dpll(intel_crtc, crtc_state);
>> -	if (pll == NULL) {
>> -		DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
>> -			pipe_name(intel_crtc->pipe));
>> -		return false;
>> -	}
>> +	if (find_pll) {
>> +		pll = intel_get_shared_dpll(intel_crtc, crtc_state);
>> +		if (pll == NULL) {
>> +			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
>> +				pipe_name(intel_crtc->pipe));
>> +			return false;
>> +		}
>>
>> -	/* shared DPLL id 0 is DPLL A */
>> -	crtc_state->ddi_pll_sel = pll->id;
>> +		/* shared DPLL id 0 is DPLL A */
>> +		crtc_state->ddi_pll_sel = pll->id;
>> +	}
>>
>>  	return true;
>>  }
>> @@ -1763,7 +1772,8 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>>   */
>>  bool intel_ddi_pll_select(struct intel_crtc *intel_crtc,
>>  			  struct intel_crtc_state *crtc_state,
>> -			  struct intel_encoder *valid_encoder)
>> +			  struct intel_encoder *valid_encoder,
>> +			  bool find_dpll)
>>  {
>>  	struct drm_device *dev = intel_crtc->base.dev;
>>  	struct intel_encoder *intel_encoder;
>> @@ -1775,13 +1785,13 @@ bool intel_ddi_pll_select(struct intel_crtc *intel_crtc,
>>
>>  	if (IS_SKYLAKE(dev))
>>  		return skl_ddi_pll_select(intel_crtc, crtc_state,
>> -					  intel_encoder);
>> +					  intel_encoder, find_dpll);
>>  	else if (IS_BROXTON(dev))
>>  		return bxt_ddi_pll_select(intel_crtc, crtc_state,
>> -					  intel_encoder);
>> +					  intel_encoder, find_dpll);
>>  	else
>>  		return hsw_ddi_pll_select(intel_crtc, crtc_state,
>> -					  intel_encoder);
>> +					  intel_encoder, find_dpll);
>>  }
>>
>>  void intel_ddi_set_pipe_settings(struct drm_crtc *crtc)
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 8ae6d7b..5c2b4ce 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9661,7 +9661,7 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state
>*old_state)
>>  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>>  				      struct intel_crtc_state *crtc_state)
>>  {
>> -	if (!intel_ddi_pll_select(crtc, crtc_state, NULL))
>> +	if (!intel_ddi_pll_select(crtc, crtc_state, NULL, true))
>>  		return -EINVAL;
>>
>>  	crtc->lowfreq_avail = false;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 0822ab6..cbcfa14 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -990,7 +990,7 @@ void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc);
>>  void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc);
>>  bool intel_ddi_pll_select(struct intel_crtc *crtc,
>>  			  struct intel_crtc_state *crtc_state,
>> -			  struct intel_encoder *encoder);
>> +			  struct intel_encoder *encoder, bool find_dpll);
>>  void intel_ddi_set_pipe_settings(struct drm_crtc *crtc);
>>  void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder);
>>  bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFCv2 DP-typeC 5/6] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[Ander Conselvan De Oliveira]

On Wed, 2015-10-14 at 17:30 +0530, Durgadoss R wrote:
> To support USB type C alternate DP mode, the display driver needs to
> know the number of lanes required by the DP panel as well as number
> of lanes that can be supported by the type-C cable. Sometimes, the
> type-C cable may limit the bandwidth even if Panel can support
> more lanes. To address these scenarios, the display driver will
> start link training with max lanes, and if that fails, the driver
> falls back to x2 lanes; and repeats this procedure for all
> bandwidth/lane configurations.
> 
> * Since link training is done before modeset only the port
>   (and not pipe/planes) and its associated PLLs are enabled.
> * Once link training is done, the port and its PLLs are disabled;
>   so that the subsequent modeset is not aware of these changes.
> * On DP hotplug: Directly start link training on the crtc
>   associated with the DP encoder.
> * On Connected boot scenarios: When booted with an LFP and a DP,
>   typically, BIOS brings up DP. In these cases, we disable the
>   crtc first and then start upfront link training. The crtc is
>   re-enabled as part of a subsequent modeset.
> * For BXT, ddi->enable/disable for DP only enable/disable
>   audio codec and hence are not included in upfront link
>   training sequence.
> * As of now, this is tested only on BXT A1 platform, on
>   kernel 4.2-rc2.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 152
> +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c  |  41 ++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h |   2 +
>  3 files changed, 194 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 8e4ea36..b3a9bff 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3209,6 +3209,158 @@ intel_ddi_init_hdmi_connector(struct
> intel_digital_port *intel_dig_port)
>  	return connector;
>  }
>  
> +bool intel_ddi_upfront_link_train(struct drm_device *dev,
> +		struct intel_dp *intel_dp, struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_connector *connector = intel_dp->attached_connector;
> +	struct intel_encoder *tmp_encoder, *encoder = connector->encoder;
> +	struct intel_shared_dpll *pll;
> +	struct intel_crtc *tmp_crtc;
> +	struct drm_crtc *tmp_drm_crtc;
> +	uint8_t tmp_lane_count, tmp_link_bw;
> +	bool ret, found, valid_crtc = false;
> +
> +	/* For now, we have only SKL and BXT supporting type-C */
> +	if (!IS_BROXTON(dev) || !IS_SKYLAKE(dev))
> +		return true;
> +
> +	if (!connector || !encoder) {
> +		DRM_DEBUG_KMS("dp connector/encoder is NULL\n");
> +		return false;
> +	}
> +
> +	/* If we already have a crtc, start link training directly */
> +	if (crtc) {
> +		valid_crtc = true;
> +		goto start_link_train;
> +	}
> +
> +	/* Find an unused crtc and use it for link training */
> +	for_each_intel_crtc(dev, crtc) {
> +		if (intel_crtc_active(&crtc->base))
> +			continue;
> +
> +		/* Make sure the new crtc will work with the encoder */
> +		if (drm_encoder_crtc_ok(&encoder->base, &crtc->base)) {
> +			found = true;
> +
> +			/* Save the existing values */
> +			tmp_encoder = connector->new_encoder;
> +			tmp_crtc = encoder->new_crtc;
> +			tmp_drm_crtc = encoder->base.crtc;

In which case are these different than NULL? I thought at this point there
hasn't been a modeset in the hotplug case and you disable the crtc on the
connected on boot case. This will also need to be rebased on atomic.

> +
> +			connector->new_encoder = encoder;
> +			encoder->new_crtc = crtc;
> +			encoder->base.crtc = &crtc->base;
> +
> +			break;
> +		}
> +	}

I think it would be a good idea to split the search for an unused crtc to a
separate function. Also, there's similar code in intel_get_load_detect_pipe(),
it would be nice if that could use the same function.

> +
> +	if (!found) {
> +		DRM_ERROR("Could not find crtc for upfront link training\n");
> +		return false;
> +	}
> +
> +start_link_train:
> +	DRM_DEBUG_KMS("upfront link train on pipe:%c\n", pipe_name(crtc
> ->pipe));
> +	found = false;
> +
> +	/* Save the existing lane_count and link_bw values */
> +	tmp_lane_count = intel_dp->lane_count;
> +	tmp_link_bw = intel_dp->link_bw;
> +
> +	/* Initialize with Max Link rate & lane count supported by panel */
> +	intel_dp->link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
> +	intel_dp->lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] &
> +					DP_MAX_LANE_COUNT_MASK;
> +
> +	/* Selects the shared DPLL to use for this port */
> +	intel_get_ddi_pll(dev_priv, dig_port->port, crtc->config);
> +	pll = intel_crtc_to_shared_dpll(crtc);
> +	if (!pll) {
> +		DRM_ERROR("Could not get shared DPLL\n");
> +		goto exit;
> +	}
> +
> +	do {
> +		/* Find port clock from link_bw */
> +		crtc->config->port_clock =
> +				drm_dp_bw_code_to_link_rate(intel_dp
> ->link_bw);
> +
> +		ret = intel_ddi_pll_select(crtc, crtc->config, encoder,
> false);
> +		if (!ret) {
> +			DRM_ERROR("Could not select PLL\n");
> +			goto exit;
> +		}
> +
> +		pll->config.crtc_mask = (1 << crtc->pipe);

Is it possible that this pll is being used by another active crtc? In that case
you steal the pll and change the configuration behind its back.

> +		pll->config.hw_state = crtc->config->dpll_hw_state;
> +
> +		DRM_DEBUG_KMS("Using shared_dpll:%d\n", crtc->config
> ->shared_dpll);
> +
> +		/* Enable PLL followed by port */
> +		intel_enable_shared_dpll(crtc);
> +		encoder->pre_enable(encoder);
> +
> +		/* Check if link training passed; if so update lane count */
> +		if (intel_dp->train_set_valid) {
> +			intel_dp->dpcd[DP_MAX_LANE_COUNT] &=
> +						~DP_MAX_LANE_COUNT_MASK;
> +			intel_dp->dpcd[DP_MAX_LANE_COUNT] |=
> +				intel_dp->lane_count &
> DP_MAX_LANE_COUNT_MASK;
> +
> +			found = true;
> +		}
> +
> +		/* Disable port followed by PLL for next retry/clean up */
> +		encoder->post_disable(encoder);
> +		intel_disable_shared_dpll(crtc);
> +
> +		if (found)
> +			goto exit;
> +
> +		DRM_DEBUG_KMS("upfront link training failed. lanes:%d
> bw:%d\n",
> +				intel_dp->lane_count, intel_dp->link_bw);
> +
> +		/* Go down to the next level and retry link training */
> +		if (intel_dp->lane_count == 4) {
> +			intel_dp->lane_count = 2;
> +		} else if (intel_dp->lane_count == 2) {
> +			intel_dp->lane_count = 1;
> +		} else if (intel_dp->link_bw == DP_LINK_BW_5_4) {
> +			intel_dp->link_bw = DP_LINK_BW_2_7;
> +			intel_dp->lane_count = 4;
> +		} else if (intel_dp->link_bw == DP_LINK_BW_2_7) {
> +			intel_dp->link_bw = DP_LINK_BW_1_62;
> +			intel_dp->lane_count = 4;
> +		} else {
> +			/* Tried all combinations, so exit */
> +			break;
> +		}

I wonder what happens to the lane status dpcd registers when the cable only
supports a reduced number of lanes. The DP standard (at least the 1.3 version)
says that if clock recovery fails with RBR, the source device should check if
the lower number lanes have the CR_DONE bit set, and in that case reduce the
number of lanes, go back to the highest rate desired and continue the link
training.

> +
> +	} while (1);

Maybe make this into a for (;;) loop. That way one can spot the (lack of) end
condition earlier when reading top to bottom.


Ander

> +
> +exit:
> +	/* Restore local associations made */
> +	if (!valid_crtc) {
> +		connector->new_encoder = tmp_encoder;
> +		encoder->new_crtc = tmp_crtc;
> +		encoder->base.crtc = tmp_drm_crtc;
> +	}
> +
> +	if (found)
> +		DRM_DEBUG_KMS("upfront link training passed. lanes:%d
> bw:%d\n",
> +				intel_dp->lane_count, intel_dp->link_bw);
> +	/* Restore lane_count and link_bw values */
> +	intel_dp->lane_count = tmp_lane_count;
> +	intel_dp->link_bw = tmp_link_bw;
> +
> +	return found;
> +}
> +
>  void intel_ddi_init(struct drm_device *dev, enum port port)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 18bcfbe..8376b47 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4785,6 +4785,35 @@ intel_dp_power_put(struct intel_dp *dp,
>  	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
>  }
>  
> +static bool intel_dp_upfront_link_train(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> +	struct drm_device *dev = intel_encoder->base.dev;
> +	struct intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) : NULL;
> +
> +	/*
> +	 * On hotplug cases, we call _upfront_link_train directly.
> +	 * In connected boot scenarios (boot with {MIPI/eDP} + DP),
> +	 * BIOS typically brings up DP. Hence, we disable the crtc
> +	 * to do _upfront_link_training. It gets re-enabled as part of
> +	 * subsequent modeset.
> +	 */
> +	if (intel_encoder->connectors_active && crtc && crtc->enabled) {
> +		DRM_DEBUG_KMS("Disabling crtc %c for upfront link
> training\n",
> +				pipe_name(intel_crtc->pipe));
> +		intel_crtc_control(crtc, false);
> +	}
> +
> +	if (HAS_DDI(dev))
> +		return intel_ddi_upfront_link_train(dev, intel_dp,
> intel_crtc);
> +
> +	/* Not a supported platform. Assume we don't need upfront_train */
> +	return true;
> +}
> +
> +
>  static enum drm_connector_status
>  intel_dp_detect(struct drm_connector *connector, bool force)
>  {
> @@ -4794,7 +4823,7 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  	struct drm_device *dev = connector->dev;
>  	enum drm_connector_status status;
>  	enum intel_display_power_domain power_domain;
> -	bool ret;
> +	bool ret, do_upfront_link_train;
>  	u8 sink_irq_vector;
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> @@ -4852,6 +4881,16 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  			DRM_DEBUG_DRIVER("CP or sink specific irq
> unhandled\n");
>  	}
>  
> +	/* Do not do upfront link train, if it is a compliance request */
> +	do_upfront_link_train =
> +		intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT &&
> +		intel_dp->compliance_test_type != DP_TEST_LINK_TRAINING;
> +
> +	if (do_upfront_link_train) {
> +		ret = intel_dp_upfront_link_train(intel_dp);
> +		if (!ret)
> +			status = connector_status_disconnected;
> +	}
>  out:
>  	intel_dp_power_put(intel_dp, power_domain);
>  	return status;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 5bcdd37..82af4e6 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1007,6 +1007,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder,
>  			 struct intel_crtc_state *pipe_config);
>  void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
>  uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
> +bool intel_ddi_upfront_link_train(struct drm_device *dev,
> +		struct intel_dp *intel_dp, struct intel_crtc *crtc);
>  
>  /* intel_frontbuffer.c */
>  void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFCv2 DP-typeC 5/6] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[R, Durgadoss]

>-----Original Message-----
>From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com]
>Sent: Wednesday, October 21, 2015 9:08 PM
>To: R, Durgadoss; intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [RFCv2 DP-typeC 5/6] drm/i915/dp: Enable Upfront link training for typeC DP
>support on BXT
>
>On Wed, 2015-10-14 at 17:30 +0530, Durgadoss R wrote:
>> To support USB type C alternate DP mode, the display driver needs to
>> know the number of lanes required by the DP panel as well as number
>> of lanes that can be supported by the type-C cable. Sometimes, the
>> type-C cable may limit the bandwidth even if Panel can support
>> more lanes. To address these scenarios, the display driver will
>> start link training with max lanes, and if that fails, the driver
>> falls back to x2 lanes; and repeats this procedure for all
>> bandwidth/lane configurations.
>>
>> * Since link training is done before modeset only the port
>>   (and not pipe/planes) and its associated PLLs are enabled.
>> * Once link training is done, the port and its PLLs are disabled;
>>   so that the subsequent modeset is not aware of these changes.
>> * On DP hotplug: Directly start link training on the crtc
>>   associated with the DP encoder.
>> * On Connected boot scenarios: When booted with an LFP and a DP,
>>   typically, BIOS brings up DP. In these cases, we disable the
>>   crtc first and then start upfront link training. The crtc is
>>   re-enabled as part of a subsequent modeset.
>> * For BXT, ddi->enable/disable for DP only enable/disable
>>   audio codec and hence are not included in upfront link
>>   training sequence.
>> * As of now, this is tested only on BXT A1 platform, on
>>   kernel 4.2-rc2.
>>
>> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 152
>> +++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_dp.c  |  41 ++++++++++-
>>  drivers/gpu/drm/i915/intel_drv.h |   2 +
>>  3 files changed, 194 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>> b/drivers/gpu/drm/i915/intel_ddi.c
>> index 8e4ea36..b3a9bff 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -3209,6 +3209,158 @@ intel_ddi_init_hdmi_connector(struct
>> intel_digital_port *intel_dig_port)
>>  	return connector;
>>  }
>>
>> +bool intel_ddi_upfront_link_train(struct drm_device *dev,
>> +		struct intel_dp *intel_dp, struct intel_crtc *crtc)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> +	struct intel_connector *connector = intel_dp->attached_connector;
>> +	struct intel_encoder *tmp_encoder, *encoder = connector->encoder;
>> +	struct intel_shared_dpll *pll;
>> +	struct intel_crtc *tmp_crtc;
>> +	struct drm_crtc *tmp_drm_crtc;
>> +	uint8_t tmp_lane_count, tmp_link_bw;
>> +	bool ret, found, valid_crtc = false;
>> +
>> +	/* For now, we have only SKL and BXT supporting type-C */
>> +	if (!IS_BROXTON(dev) || !IS_SKYLAKE(dev))
>> +		return true;
>> +
>> +	if (!connector || !encoder) {
>> +		DRM_DEBUG_KMS("dp connector/encoder is NULL\n");
>> +		return false;
>> +	}
>> +
>> +	/* If we already have a crtc, start link training directly */
>> +	if (crtc) {
>> +		valid_crtc = true;
>> +		goto start_link_train;
>> +	}
>> +
>> +	/* Find an unused crtc and use it for link training */
>> +	for_each_intel_crtc(dev, crtc) {
>> +		if (intel_crtc_active(&crtc->base))
>> +			continue;
>> +
>> +		/* Make sure the new crtc will work with the encoder */
>> +		if (drm_encoder_crtc_ok(&encoder->base, &crtc->base)) {
>> +			found = true;
>> +
>> +			/* Save the existing values */
>> +			tmp_encoder = connector->new_encoder;
>> +			tmp_crtc = encoder->new_crtc;
>> +			tmp_drm_crtc = encoder->base.crtc;
>
>In which case are these different than NULL? I thought at this point there
>hasn't been a modeset in the hotplug case and you disable the crtc on the
>connected on boot case. This will also need to be rebased on atomic.

As far as I tested these, they are NULL in both Hotplug and connected boot
Cases. There was one issue during suspend/resume where it was not NULL.
But later we figured out, we always have a valid crtc during resume and hence
Should not take this path. So, yes, with all our testing so far, NULL works fine here.

Agreed, this need to be rebased on atomic. Will do this in next version.

>
>> +
>> +			connector->new_encoder = encoder;
>> +			encoder->new_crtc = crtc;
>> +			encoder->base.crtc = &crtc->base;
>> +
>> +			break;
>> +		}
>> +	}
>
>I think it would be a good idea to split the search for an unused crtc to a
>separate function. Also, there's similar code in intel_get_load_detect_pipe(),
>it would be nice if that could use the same function.

Yes, I also had a similar thought, but did not get to _load_detect_pipe()
Function. Will look at it and try to use it..

>
>> +
>> +	if (!found) {
>> +		DRM_ERROR("Could not find crtc for upfront link training\n");
>> +		return false;
>> +	}
>> +
>> +start_link_train:
>> +	DRM_DEBUG_KMS("upfront link train on pipe:%c\n", pipe_name(crtc
>> ->pipe));
>> +	found = false;
>> +
>> +	/* Save the existing lane_count and link_bw values */
>> +	tmp_lane_count = intel_dp->lane_count;
>> +	tmp_link_bw = intel_dp->link_bw;
>> +
>> +	/* Initialize with Max Link rate & lane count supported by panel */
>> +	intel_dp->link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
>> +	intel_dp->lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] &
>> +					DP_MAX_LANE_COUNT_MASK;
>> +
>> +	/* Selects the shared DPLL to use for this port */
>> +	intel_get_ddi_pll(dev_priv, dig_port->port, crtc->config);
>> +	pll = intel_crtc_to_shared_dpll(crtc);
>> +	if (!pll) {
>> +		DRM_ERROR("Could not get shared DPLL\n");
>> +		goto exit;
>> +	}
>> +
>> +	do {
>> +		/* Find port clock from link_bw */
>> +		crtc->config->port_clock =
>> +				drm_dp_bw_code_to_link_rate(intel_dp
>> ->link_bw);
>> +
>> +		ret = intel_ddi_pll_select(crtc, crtc->config, encoder,
>> false);
>> +		if (!ret) {
>> +			DRM_ERROR("Could not select PLL\n");
>> +			goto exit;
>> +		}
>> +
>> +		pll->config.crtc_mask = (1 << crtc->pipe);
>
>Is it possible that this pll is being used by another active crtc? In that case
>you steal the pll and change the configuration behind its back.

I am not sure either. When we tested on BXT, these were always
used by one crtc only. So, is this a valid case in BXT or in some other DDI
based platforms ?

>
>> +		pll->config.hw_state = crtc->config->dpll_hw_state;
>> +
>> +		DRM_DEBUG_KMS("Using shared_dpll:%d\n", crtc->config
>> ->shared_dpll);
>> +
>> +		/* Enable PLL followed by port */
>> +		intel_enable_shared_dpll(crtc);
>> +		encoder->pre_enable(encoder);
>> +
>> +		/* Check if link training passed; if so update lane count */
>> +		if (intel_dp->train_set_valid) {
>> +			intel_dp->dpcd[DP_MAX_LANE_COUNT] &=
>> +						~DP_MAX_LANE_COUNT_MASK;
>> +			intel_dp->dpcd[DP_MAX_LANE_COUNT] |=
>> +				intel_dp->lane_count &
>> DP_MAX_LANE_COUNT_MASK;
>> +
>> +			found = true;
>> +		}
>> +
>> +		/* Disable port followed by PLL for next retry/clean up */
>> +		encoder->post_disable(encoder);
>> +		intel_disable_shared_dpll(crtc);
>> +
>> +		if (found)
>> +			goto exit;
>> +
>> +		DRM_DEBUG_KMS("upfront link training failed. lanes:%d
>> bw:%d\n",
>> +				intel_dp->lane_count, intel_dp->link_bw);
>> +
>> +		/* Go down to the next level and retry link training */
>> +		if (intel_dp->lane_count == 4) {
>> +			intel_dp->lane_count = 2;
>> +		} else if (intel_dp->lane_count == 2) {
>> +			intel_dp->lane_count = 1;
>> +		} else if (intel_dp->link_bw == DP_LINK_BW_5_4) {
>> +			intel_dp->link_bw = DP_LINK_BW_2_7;
>> +			intel_dp->lane_count = 4;
>> +		} else if (intel_dp->link_bw == DP_LINK_BW_2_7) {
>> +			intel_dp->link_bw = DP_LINK_BW_1_62;
>> +			intel_dp->lane_count = 4;
>> +		} else {
>> +			/* Tried all combinations, so exit */
>> +			break;
>> +		}
>
>I wonder what happens to the lane status dpcd registers when the cable only
>supports a reduced number of lanes. The DP standard (at least the 1.3 version)
>says that if clock recovery fails with RBR, the source device should check if
>the lower number lanes have the CR_DONE bit set, and in that case reduce the
>number of lanes, go back to the highest rate desired and continue the link
>training.

I believe you are talking about DPCD 202h and 203h i.e which one of them is
actually being used/reported correctly. I will check on this with few different
cables during my next round of testing.

>
>> +
>> +	} while (1);
>
>Maybe make this into a for (;;) loop. That way one can spot the (lack of) end
>condition earlier when reading top to bottom.

Ok, will try this implementation..

Thank you for having a look at this patch Ander..

Thanks,
Durga

>
>
>Ander
>
>> +
>> +exit:
>> +	/* Restore local associations made */
>> +	if (!valid_crtc) {
>> +		connector->new_encoder = tmp_encoder;
>> +		encoder->new_crtc = tmp_crtc;
>> +		encoder->base.crtc = tmp_drm_crtc;
>> +	}
>> +
>> +	if (found)
>> +		DRM_DEBUG_KMS("upfront link training passed. lanes:%d
>> bw:%d\n",
>> +				intel_dp->lane_count, intel_dp->link_bw);
>> +	/* Restore lane_count and link_bw values */
>> +	intel_dp->lane_count = tmp_lane_count;
>> +	intel_dp->link_bw = tmp_link_bw;
>> +
>> +	return found;
>> +}
>> +
>>  void intel_ddi_init(struct drm_device *dev, enum port port)
>>  {
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 18bcfbe..8376b47 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4785,6 +4785,35 @@ intel_dp_power_put(struct intel_dp *dp,
>>  	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
>>  }
>>
>> +static bool intel_dp_upfront_link_train(struct intel_dp *intel_dp)
>> +{
>> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> +	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
>> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>> +	struct drm_device *dev = intel_encoder->base.dev;
>> +	struct intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) : NULL;
>> +
>> +	/*
>> +	 * On hotplug cases, we call _upfront_link_train directly.
>> +	 * In connected boot scenarios (boot with {MIPI/eDP} + DP),
>> +	 * BIOS typically brings up DP. Hence, we disable the crtc
>> +	 * to do _upfront_link_training. It gets re-enabled as part of
>> +	 * subsequent modeset.
>> +	 */
>> +	if (intel_encoder->connectors_active && crtc && crtc->enabled) {
>> +		DRM_DEBUG_KMS("Disabling crtc %c for upfront link
>> training\n",
>> +				pipe_name(intel_crtc->pipe));
>> +		intel_crtc_control(crtc, false);
>> +	}
>> +
>> +	if (HAS_DDI(dev))
>> +		return intel_ddi_upfront_link_train(dev, intel_dp,
>> intel_crtc);
>> +
>> +	/* Not a supported platform. Assume we don't need upfront_train */
>> +	return true;
>> +}
>> +
>> +
>>  static enum drm_connector_status
>>  intel_dp_detect(struct drm_connector *connector, bool force)
>>  {
>> @@ -4794,7 +4823,7 @@ intel_dp_detect(struct drm_connector *connector, bool
>> force)
>>  	struct drm_device *dev = connector->dev;
>>  	enum drm_connector_status status;
>>  	enum intel_display_power_domain power_domain;
>> -	bool ret;
>> +	bool ret, do_upfront_link_train;
>>  	u8 sink_irq_vector;
>>
>>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>> @@ -4852,6 +4881,16 @@ intel_dp_detect(struct drm_connector *connector, bool
>> force)
>>  			DRM_DEBUG_DRIVER("CP or sink specific irq
>> unhandled\n");
>>  	}
>>
>> +	/* Do not do upfront link train, if it is a compliance request */
>> +	do_upfront_link_train =
>> +		intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT &&
>> +		intel_dp->compliance_test_type != DP_TEST_LINK_TRAINING;
>> +
>> +	if (do_upfront_link_train) {
>> +		ret = intel_dp_upfront_link_train(intel_dp);
>> +		if (!ret)
>> +			status = connector_status_disconnected;
>> +	}
>>  out:
>>  	intel_dp_power_put(intel_dp, power_domain);
>>  	return status;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 5bcdd37..82af4e6 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1007,6 +1007,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder,
>>  			 struct intel_crtc_state *pipe_config);
>>  void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
>>  uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
>> +bool intel_ddi_upfront_link_train(struct drm_device *dev,
>> +		struct intel_dp *intel_dp, struct intel_crtc *crtc);
>>
>>  /* intel_frontbuffer.c */
>>  void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFCv2 DP-typeC 5/6] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[Thulasimani, Sivakumar]

On 10/23/2015 5:37 PM, R, Durgadoss wrote:
>> -----Original Message-----
>> From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com]
>> Sent: Wednesday, October 21, 2015 9:08 PM
>> To: R, Durgadoss; intel-gfx@lists.freedesktop.org
>> Subject: Re: [Intel-gfx] [RFCv2 DP-typeC 5/6] drm/i915/dp: Enable Upfront link training for typeC DP
>> support on BXT
>>
>> On Wed, 2015-10-14 at 17:30 +0530, Durgadoss R wrote:
>>> To support USB type C alternate DP mode, the display driver needs to
>>> know the number of lanes required by the DP panel as well as number
>>> of lanes that can be supported by the type-C cable. Sometimes, the
>>> type-C cable may limit the bandwidth even if Panel can support
>>> more lanes. To address these scenarios, the display driver will
>>> start link training with max lanes, and if that fails, the driver
>>> falls back to x2 lanes; and repeats this procedure for all
>>> bandwidth/lane configurations.
>>>
>>> * Since link training is done before modeset only the port
>>>    (and not pipe/planes) and its associated PLLs are enabled.
>>> * Once link training is done, the port and its PLLs are disabled;
>>>    so that the subsequent modeset is not aware of these changes.
>>> * On DP hotplug: Directly start link training on the crtc
>>>    associated with the DP encoder.
>>> * On Connected boot scenarios: When booted with an LFP and a DP,
>>>    typically, BIOS brings up DP. In these cases, we disable the
>>>    crtc first and then start upfront link training. The crtc is
>>>    re-enabled as part of a subsequent modeset.
>>> * For BXT, ddi->enable/disable for DP only enable/disable
>>>    audio codec and hence are not included in upfront link
>>>    training sequence.
>>> * As of now, this is tested only on BXT A1 platform, on
>>>    kernel 4.2-rc2.
>>>
>>> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_ddi.c | 152
>>> +++++++++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/intel_dp.c  |  41 ++++++++++-
>>>   drivers/gpu/drm/i915/intel_drv.h |   2 +
>>>   3 files changed, 194 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>>> b/drivers/gpu/drm/i915/intel_ddi.c
>>> index 8e4ea36..b3a9bff 100644
>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>> @@ -3209,6 +3209,158 @@ intel_ddi_init_hdmi_connector(struct
>>> intel_digital_port *intel_dig_port)
>>>   	return connector;
>>>   }
>>>
>>> +bool intel_ddi_upfront_link_train(struct drm_device *dev,
>>> +		struct intel_dp *intel_dp, struct intel_crtc *crtc)
>>> +{
>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>>> +	struct intel_connector *connector = intel_dp->attached_connector;
>>> +	struct intel_encoder *tmp_encoder, *encoder = connector->encoder;
>>> +	struct intel_shared_dpll *pll;
>>> +	struct intel_crtc *tmp_crtc;
>>> +	struct drm_crtc *tmp_drm_crtc;
>>> +	uint8_t tmp_lane_count, tmp_link_bw;
>>> +	bool ret, found, valid_crtc = false;
>>> +
>>> +	/* For now, we have only SKL and BXT supporting type-C */
>>> +	if (!IS_BROXTON(dev) || !IS_SKYLAKE(dev))
>>> +		return true;
>>> +
>>> +	if (!connector || !encoder) {
>>> +		DRM_DEBUG_KMS("dp connector/encoder is NULL\n");
>>> +		return false;
>>> +	}
>>> +
>>> +	/* If we already have a crtc, start link training directly */
>>> +	if (crtc) {
>>> +		valid_crtc = true;
>>> +		goto start_link_train;
>>> +	}
>>> +
>>> +	/* Find an unused crtc and use it for link training */
>>> +	for_each_intel_crtc(dev, crtc) {
>>> +		if (intel_crtc_active(&crtc->base))
>>> +			continue;
>>> +
>>> +		/* Make sure the new crtc will work with the encoder */
>>> +		if (drm_encoder_crtc_ok(&encoder->base, &crtc->base)) {
>>> +			found = true;
>>> +
>>> +			/* Save the existing values */
>>> +			tmp_encoder = connector->new_encoder;
>>> +			tmp_crtc = encoder->new_crtc;
>>> +			tmp_drm_crtc = encoder->base.crtc;
>> In which case are these different than NULL? I thought at this point there
>> hasn't been a modeset in the hotplug case and you disable the crtc on the
>> connected on boot case. This will also need to be rebased on atomic.
> As far as I tested these, they are NULL in both Hotplug and connected boot
> Cases. There was one issue during suspend/resume where it was not NULL.
> But later we figured out, we always have a valid crtc during resume and hence
> Should not take this path. So, yes, with all our testing so far, NULL works fine here.
>
> Agreed, this need to be rebased on atomic. Will do this in next version.
>
>>> +
>>> +			connector->new_encoder = encoder;
>>> +			encoder->new_crtc = crtc;
>>> +			encoder->base.crtc = &crtc->base;
>>> +
>>> +			break;
>>> +		}
>>> +	}
>> I think it would be a good idea to split the search for an unused crtc to a
>> separate function. Also, there's similar code in intel_get_load_detect_pipe(),
>> it would be nice if that could use the same function.
> Yes, I also had a similar thought, but did not get to _load_detect_pipe()
> Function. Will look at it and try to use it..
>
>>> +
>>> +	if (!found) {
>>> +		DRM_ERROR("Could not find crtc for upfront link training\n");
>>> +		return false;
>>> +	}
>>> +
>>> +start_link_train:
>>> +	DRM_DEBUG_KMS("upfront link train on pipe:%c\n", pipe_name(crtc
>>> ->pipe));
>>> +	found = false;
>>> +
>>> +	/* Save the existing lane_count and link_bw values */
>>> +	tmp_lane_count = intel_dp->lane_count;
>>> +	tmp_link_bw = intel_dp->link_bw;
>>> +
>>> +	/* Initialize with Max Link rate & lane count supported by panel */
>>> +	intel_dp->link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
>>> +	intel_dp->lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] &
>>> +					DP_MAX_LANE_COUNT_MASK;
>>> +
>>> +	/* Selects the shared DPLL to use for this port */
>>> +	intel_get_ddi_pll(dev_priv, dig_port->port, crtc->config);
>>> +	pll = intel_crtc_to_shared_dpll(crtc);
>>> +	if (!pll) {
>>> +		DRM_ERROR("Could not get shared DPLL\n");
>>> +		goto exit;
>>> +	}
>>> +
>>> +	do {
>>> +		/* Find port clock from link_bw */
>>> +		crtc->config->port_clock =
>>> +				drm_dp_bw_code_to_link_rate(intel_dp
>>> ->link_bw);
>>> +
>>> +		ret = intel_ddi_pll_select(crtc, crtc->config, encoder,
>>> false);
>>> +		if (!ret) {
>>> +			DRM_ERROR("Could not select PLL\n");
>>> +			goto exit;
>>> +		}
>>> +
>>> +		pll->config.crtc_mask = (1 << crtc->pipe);
>> Is it possible that this pll is being used by another active crtc? In that case
>> you steal the pll and change the configuration behind its back.
> I am not sure either. When we tested on BXT, these were always
> used by one crtc only. So, is this a valid case in BXT or in some other DDI
> based platforms ?
yes, we should handle this. the two platforms this is tested in did not 
have PLL
sharing so it was not considered.
>>> +		pll->config.hw_state = crtc->config->dpll_hw_state;
>>> +
>>> +		DRM_DEBUG_KMS("Using shared_dpll:%d\n", crtc->config
>>> ->shared_dpll);
>>> +
>>> +		/* Enable PLL followed by port */
>>> +		intel_enable_shared_dpll(crtc);
>>> +		encoder->pre_enable(encoder);
>>> +
>>> +		/* Check if link training passed; if so update lane count */
>>> +		if (intel_dp->train_set_valid) {
>>> +			intel_dp->dpcd[DP_MAX_LANE_COUNT] &=
>>> +						~DP_MAX_LANE_COUNT_MASK;
>>> +			intel_dp->dpcd[DP_MAX_LANE_COUNT] |=
>>> +				intel_dp->lane_count &
>>> DP_MAX_LANE_COUNT_MASK;
>>> +
>>> +			found = true;
>>> +		}
>>> +
>>> +		/* Disable port followed by PLL for next retry/clean up */
>>> +		encoder->post_disable(encoder);
>>> +		intel_disable_shared_dpll(crtc);
>>> +
>>> +		if (found)
>>> +			goto exit;
>>> +
>>> +		DRM_DEBUG_KMS("upfront link training failed. lanes:%d
>>> bw:%d\n",
>>> +				intel_dp->lane_count, intel_dp->link_bw);
>>> +
>>> +		/* Go down to the next level and retry link training */
>>> +		if (intel_dp->lane_count == 4) {
>>> +			intel_dp->lane_count = 2;
>>> +		} else if (intel_dp->lane_count == 2) {
>>> +			intel_dp->lane_count = 1;
>>> +		} else if (intel_dp->link_bw == DP_LINK_BW_5_4) {
>>> +			intel_dp->link_bw = DP_LINK_BW_2_7;
>>> +			intel_dp->lane_count = 4;
>>> +		} else if (intel_dp->link_bw == DP_LINK_BW_2_7) {
>>> +			intel_dp->link_bw = DP_LINK_BW_1_62;
>>> +			intel_dp->lane_count = 4;
>>> +		} else {
>>> +			/* Tried all combinations, so exit */
>>> +			break;
>>> +		}
>> I wonder what happens to the lane status dpcd registers when the cable only
>> supports a reduced number of lanes. The DP standard (at least the 1.3 version)
>> says that if clock recovery fails with RBR, the source device should check if
>> the lower number lanes have the CR_DONE bit set, and in that case reduce the
>> number of lanes, go back to the highest rate desired and continue the link
>> training.
> I believe you are talking about DPCD 202h and 203h i.e which one of them is
> actually being used/reported correctly. I will check on this with few different
> cables during my next round of testing.
can you please share where is it mentioned in the spec that if CR fails
we should retry with higher rate and lower lanes  ? i am not aware of 
such a requirement.

The expectation is that if cable supports lesser no of lanes than the 
number supported by panel
CR should fail for the additional lanes. resulting in the next link 
training with lower lane count
to pass. that is the basic assumption of this patch :)

regards,
Sivakumar
>>> +
>>> +	} while (1);
>> Maybe make this into a for (;;) loop. That way one can spot the (lack of) end
>> condition earlier when reading top to bottom.
> Ok, will try this implementation..
>
> Thank you for having a look at this patch Ander..
>
> Thanks,
> Durga
>
>>
>> Ander
>>
>>> +
>>> +exit:
>>> +	/* Restore local associations made */
>>> +	if (!valid_crtc) {
>>> +		connector->new_encoder = tmp_encoder;
>>> +		encoder->new_crtc = tmp_crtc;
>>> +		encoder->base.crtc = tmp_drm_crtc;
>>> +	}
>>> +
>>> +	if (found)
>>> +		DRM_DEBUG_KMS("upfront link training passed. lanes:%d
>>> bw:%d\n",
>>> +				intel_dp->lane_count, intel_dp->link_bw);
>>> +	/* Restore lane_count and link_bw values */
>>> +	intel_dp->lane_count = tmp_lane_count;
>>> +	intel_dp->link_bw = tmp_link_bw;
>>> +
>>> +	return found;
>>> +}
>>> +
>>>   void intel_ddi_init(struct drm_device *dev, enum port port)
>>>   {
>>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 18bcfbe..8376b47 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -4785,6 +4785,35 @@ intel_dp_power_put(struct intel_dp *dp,
>>>   	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
>>>   }
>>>
>>> +static bool intel_dp_upfront_link_train(struct intel_dp *intel_dp)
>>> +{
>>> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>>> +	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
>>> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>>> +	struct drm_device *dev = intel_encoder->base.dev;
>>> +	struct intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) : NULL;
>>> +
>>> +	/*
>>> +	 * On hotplug cases, we call _upfront_link_train directly.
>>> +	 * In connected boot scenarios (boot with {MIPI/eDP} + DP),
>>> +	 * BIOS typically brings up DP. Hence, we disable the crtc
>>> +	 * to do _upfront_link_training. It gets re-enabled as part of
>>> +	 * subsequent modeset.
>>> +	 */
>>> +	if (intel_encoder->connectors_active && crtc && crtc->enabled) {
>>> +		DRM_DEBUG_KMS("Disabling crtc %c for upfront link
>>> training\n",
>>> +				pipe_name(intel_crtc->pipe));
>>> +		intel_crtc_control(crtc, false);
>>> +	}
>>> +
>>> +	if (HAS_DDI(dev))
>>> +		return intel_ddi_upfront_link_train(dev, intel_dp,
>>> intel_crtc);
>>> +
>>> +	/* Not a supported platform. Assume we don't need upfront_train */
>>> +	return true;
>>> +}
>>> +
>>> +
>>>   static enum drm_connector_status
>>>   intel_dp_detect(struct drm_connector *connector, bool force)
>>>   {
>>> @@ -4794,7 +4823,7 @@ intel_dp_detect(struct drm_connector *connector, bool
>>> force)
>>>   	struct drm_device *dev = connector->dev;
>>>   	enum drm_connector_status status;
>>>   	enum intel_display_power_domain power_domain;
>>> -	bool ret;
>>> +	bool ret, do_upfront_link_train;
>>>   	u8 sink_irq_vector;
>>>
>>>   	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>>> @@ -4852,6 +4881,16 @@ intel_dp_detect(struct drm_connector *connector, bool
>>> force)
>>>   			DRM_DEBUG_DRIVER("CP or sink specific irq
>>> unhandled\n");
>>>   	}
>>>
>>> +	/* Do not do upfront link train, if it is a compliance request */
>>> +	do_upfront_link_train =
>>> +		intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT &&
>>> +		intel_dp->compliance_test_type != DP_TEST_LINK_TRAINING;
>>> +
>>> +	if (do_upfront_link_train) {
>>> +		ret = intel_dp_upfront_link_train(intel_dp);
>>> +		if (!ret)
>>> +			status = connector_status_disconnected;
>>> +	}
>>>   out:
>>>   	intel_dp_power_put(intel_dp, power_domain);
>>>   	return status;
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>> b/drivers/gpu/drm/i915/intel_drv.h
>>> index 5bcdd37..82af4e6 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -1007,6 +1007,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder,
>>>   			 struct intel_crtc_state *pipe_config);
>>>   void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
>>>   uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
>>> +bool intel_ddi_upfront_link_train(struct drm_device *dev,
>>> +		struct intel_dp *intel_dp, struct intel_crtc *crtc);
>>>
>>>   /* intel_frontbuffer.c */
>>>   void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [RFCv2 DP-typeC 5/6] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[Ander Conselvan De Oliveira]

On Mon, 2015-10-26 at 08:27 +0530, Thulasimani, Sivakumar wrote:
> 
> On 10/23/2015 5:37 PM, R, Durgadoss wrote:
> > > -----Original Message-----
> > > From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com]
> > > Sent: Wednesday, October 21, 2015 9:08 PM
> > > To: R, Durgadoss; intel-gfx@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [RFCv2 DP-typeC 5/6] drm/i915/dp: Enable Upfront
> > > link training for typeC DP
> > > support on BXT
> > > 
> > > On Wed, 2015-10-14 at 17:30 +0530, Durgadoss R wrote:
> > > > To support USB type C alternate DP mode, the display driver needs to
> > > > know the number of lanes required by the DP panel as well as number
> > > > of lanes that can be supported by the type-C cable. Sometimes, the
> > > > type-C cable may limit the bandwidth even if Panel can support
> > > > more lanes. To address these scenarios, the display driver will
> > > > start link training with max lanes, and if that fails, the driver
> > > > falls back to x2 lanes; and repeats this procedure for all
> > > > bandwidth/lane configurations.
> > > > 
> > > > * Since link training is done before modeset only the port
> > > >    (and not pipe/planes) and its associated PLLs are enabled.
> > > > * Once link training is done, the port and its PLLs are disabled;
> > > >    so that the subsequent modeset is not aware of these changes.
> > > > * On DP hotplug: Directly start link training on the crtc
> > > >    associated with the DP encoder.
> > > > * On Connected boot scenarios: When booted with an LFP and a DP,
> > > >    typically, BIOS brings up DP. In these cases, we disable the
> > > >    crtc first and then start upfront link training. The crtc is
> > > >    re-enabled as part of a subsequent modeset.
> > > > * For BXT, ddi->enable/disable for DP only enable/disable
> > > >    audio codec and hence are not included in upfront link
> > > >    training sequence.
> > > > * As of now, this is tested only on BXT A1 platform, on
> > > >    kernel 4.2-rc2.
> > > > 
> > > > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/intel_ddi.c | 152
> > > > +++++++++++++++++++++++++++++++++++++++
> > > >   drivers/gpu/drm/i915/intel_dp.c  |  41 ++++++++++-
> > > >   drivers/gpu/drm/i915/intel_drv.h |   2 +
> > > >   3 files changed, 194 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index 8e4ea36..b3a9bff 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -3209,6 +3209,158 @@ intel_ddi_init_hdmi_connector(struct
> > > > intel_digital_port *intel_dig_port)
> > > >   	return connector;
> > > >   }
> > > > 
> > > > +bool intel_ddi_upfront_link_train(struct drm_device *dev,
> > > > +		struct intel_dp *intel_dp, struct intel_crtc *crtc)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > > +	struct intel_connector *connector = intel_dp
> > > > ->attached_connector;
> > > > +	struct intel_encoder *tmp_encoder, *encoder = connector
> > > > ->encoder;
> > > > +	struct intel_shared_dpll *pll;
> > > > +	struct intel_crtc *tmp_crtc;
> > > > +	struct drm_crtc *tmp_drm_crtc;
> > > > +	uint8_t tmp_lane_count, tmp_link_bw;
> > > > +	bool ret, found, valid_crtc = false;
> > > > +
> > > > +	/* For now, we have only SKL and BXT supporting type-C */
> > > > +	if (!IS_BROXTON(dev) || !IS_SKYLAKE(dev))
> > > > +		return true;
> > > > +
> > > > +	if (!connector || !encoder) {
> > > > +		DRM_DEBUG_KMS("dp connector/encoder is NULL\n");
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	/* If we already have a crtc, start link training directly */
> > > > +	if (crtc) {
> > > > +		valid_crtc = true;
> > > > +		goto start_link_train;
> > > > +	}
> > > > +
> > > > +	/* Find an unused crtc and use it for link training */
> > > > +	for_each_intel_crtc(dev, crtc) {
> > > > +		if (intel_crtc_active(&crtc->base))
> > > > +			continue;
> > > > +
> > > > +		/* Make sure the new crtc will work with the encoder */
> > > > +		if (drm_encoder_crtc_ok(&encoder->base, &crtc->base)) {
> > > > +			found = true;
> > > > +
> > > > +			/* Save the existing values */
> > > > +			tmp_encoder = connector->new_encoder;
> > > > +			tmp_crtc = encoder->new_crtc;
> > > > +			tmp_drm_crtc = encoder->base.crtc;
> > > In which case are these different than NULL? I thought at this point there
> > > hasn't been a modeset in the hotplug case and you disable the crtc on the
> > > connected on boot case. This will also need to be rebased on atomic.
> > As far as I tested these, they are NULL in both Hotplug and connected boot
> > Cases. There was one issue during suspend/resume where it was not NULL.
> > But later we figured out, we always have a valid crtc during resume and
> > hence
> > Should not take this path. So, yes, with all our testing so far, NULL works
> > fine here.
> > 
> > Agreed, this need to be rebased on atomic. Will do this in next version.
> > 
> > > > +
> > > > +			connector->new_encoder = encoder;
> > > > +			encoder->new_crtc = crtc;
> > > > +			encoder->base.crtc = &crtc->base;
> > > > +
> > > > +			break;
> > > > +		}
> > > > +	}
> > > I think it would be a good idea to split the search for an unused crtc to
> > > a
> > > separate function. Also, there's similar code in
> > > intel_get_load_detect_pipe(),
> > > it would be nice if that could use the same function.
> > Yes, I also had a similar thought, but did not get to _load_detect_pipe()
> > Function. Will look at it and try to use it..
> > 
> > > > +
> > > > +	if (!found) {
> > > > +		DRM_ERROR("Could not find crtc for upfront link
> > > > training\n");
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +start_link_train:
> > > > +	DRM_DEBUG_KMS("upfront link train on pipe:%c\n", pipe_name(crtc
> > > > ->pipe));
> > > > +	found = false;
> > > > +
> > > > +	/* Save the existing lane_count and link_bw values */
> > > > +	tmp_lane_count = intel_dp->lane_count;
> > > > +	tmp_link_bw = intel_dp->link_bw;
> > > > +
> > > > +	/* Initialize with Max Link rate & lane count supported by
> > > > panel */
> > > > +	intel_dp->link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
> > > > +	intel_dp->lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] &
> > > > +					DP_MAX_LANE_COUNT_MASK;
> > > > +
> > > > +	/* Selects the shared DPLL to use for this port */
> > > > +	intel_get_ddi_pll(dev_priv, dig_port->port, crtc->config);
> > > > +	pll = intel_crtc_to_shared_dpll(crtc);
> > > > +	if (!pll) {
> > > > +		DRM_ERROR("Could not get shared DPLL\n");
> > > > +		goto exit;
> > > > +	}
> > > > +
> > > > +	do {
> > > > +		/* Find port clock from link_bw */
> > > > +		crtc->config->port_clock =
> > > > +				drm_dp_bw_code_to_link_rate(intel_dp
> > > > ->link_bw);
> > > > +
> > > > +		ret = intel_ddi_pll_select(crtc, crtc->config, encoder,
> > > > false);
> > > > +		if (!ret) {
> > > > +			DRM_ERROR("Could not select PLL\n");
> > > > +			goto exit;
> > > > +		}
> > > > +
> > > > +		pll->config.crtc_mask = (1 << crtc->pipe);
> > > Is it possible that this pll is being used by another active crtc? In that
> > > case
> > > you steal the pll and change the configuration behind its back.
> > I am not sure either. When we tested on BXT, these were always
> > used by one crtc only. So, is this a valid case in BXT or in some other DDI
> > based platforms ?
> yes, we should handle this. the two platforms this is tested in did not 
> have PLL
> sharing so it was not considered.
> > > > +		pll->config.hw_state = crtc->config->dpll_hw_state;
> > > > +
> > > > +		DRM_DEBUG_KMS("Using shared_dpll:%d\n", crtc->config
> > > > ->shared_dpll);
> > > > +
> > > > +		/* Enable PLL followed by port */
> > > > +		intel_enable_shared_dpll(crtc);
> > > > +		encoder->pre_enable(encoder);
> > > > +
> > > > +		/* Check if link training passed; if so update lane
> > > > count */
> > > > +		if (intel_dp->train_set_valid) {
> > > > +			intel_dp->dpcd[DP_MAX_LANE_COUNT] &=
> > > > +						~DP_MAX_LANE_COUNT_MASK
> > > > ;
> > > > +			intel_dp->dpcd[DP_MAX_LANE_COUNT] |=
> > > > +				intel_dp->lane_count &
> > > > DP_MAX_LANE_COUNT_MASK;
> > > > +
> > > > +			found = true;
> > > > +		}
> > > > +
> > > > +		/* Disable port followed by PLL for next retry/clean up
> > > > */
> > > > +		encoder->post_disable(encoder);
> > > > +		intel_disable_shared_dpll(crtc);
> > > > +
> > > > +		if (found)
> > > > +			goto exit;
> > > > +
> > > > +		DRM_DEBUG_KMS("upfront link training failed. lanes:%d
> > > > bw:%d\n",
> > > > +				intel_dp->lane_count, intel_dp
> > > > ->link_bw);
> > > > +
> > > > +		/* Go down to the next level and retry link training */
> > > > +		if (intel_dp->lane_count == 4) {
> > > > +			intel_dp->lane_count = 2;
> > > > +		} else if (intel_dp->lane_count == 2) {
> > > > +			intel_dp->lane_count = 1;
> > > > +		} else if (intel_dp->link_bw == DP_LINK_BW_5_4) {
> > > > +			intel_dp->link_bw = DP_LINK_BW_2_7;
> > > > +			intel_dp->lane_count = 4;
> > > > +		} else if (intel_dp->link_bw == DP_LINK_BW_2_7) {
> > > > +			intel_dp->link_bw = DP_LINK_BW_1_62;
> > > > +			intel_dp->lane_count = 4;
> > > > +		} else {
> > > > +			/* Tried all combinations, so exit */
> > > > +			break;
> > > > +		}
> > > I wonder what happens to the lane status dpcd registers when the cable
> > > only
> > > supports a reduced number of lanes. The DP standard (at least the 1.3
> > > version)
> > > says that if clock recovery fails with RBR, the source device should check
> > > if
> > > the lower number lanes have the CR_DONE bit set, and in that case reduce
> > > the
> > > number of lanes, go back to the highest rate desired and continue the link
> > > training.
> > I believe you are talking about DPCD 202h and 203h i.e which one of them is
> > actually being used/reported correctly. I will check on this with few
> > different
> > cables during my next round of testing.
> can you please share where is it mentioned in the spec that if CR fails
> we should retry with higher rate and lower lanes  ? i am not aware of 
> such a requirement.

Section 3.5.1.2.2.1 of the 1.3 version.


Ander


> The expectation is that if cable supports lesser no of lanes than the 
> number supported by panel
> CR should fail for the additional lanes. resulting in the next link 
> training with lower lane count
> to pass. that is the basic assumption of this patch :)

> regards,
> Sivakumar
> > > > +
> > > > +	} while (1);
> > > Maybe make this into a for (;;) loop. That way one can spot the (lack of)
> > > end
> > > condition earlier when reading top to bottom.
> > Ok, will try this implementation..
> > 
> > Thank you for having a look at this patch Ander..
> > 
> > Thanks,
> > Durga
> > 
> > > 
> > > Ander
> > > 
> > > > +
> > > > +exit:
> > > > +	/* Restore local associations made */
> > > > +	if (!valid_crtc) {
> > > > +		connector->new_encoder = tmp_encoder;
> > > > +		encoder->new_crtc = tmp_crtc;
> > > > +		encoder->base.crtc = tmp_drm_crtc;
> > > > +	}
> > > > +
> > > > +	if (found)
> > > > +		DRM_DEBUG_KMS("upfront link training passed. lanes:%d
> > > > bw:%d\n",
> > > > +				intel_dp->lane_count, intel_dp
> > > > ->link_bw);
> > > > +	/* Restore lane_count and link_bw values */
> > > > +	intel_dp->lane_count = tmp_lane_count;
> > > > +	intel_dp->link_bw = tmp_link_bw;
> > > > +
> > > > +	return found;
> > > > +}
> > > > +
> > > >   void intel_ddi_init(struct drm_device *dev, enum port port)
> > > >   {
> > > >   	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 18bcfbe..8376b47 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4785,6 +4785,35 @@ intel_dp_power_put(struct intel_dp *dp,
> > > >   	intel_display_power_put(to_i915(encoder->base.dev),
> > > > power_domain);
> > > >   }
> > > > 
> > > > +static bool intel_dp_upfront_link_train(struct intel_dp *intel_dp)
> > > > +{
> > > > +	struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > +	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
> > > > +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> > > > +	struct drm_device *dev = intel_encoder->base.dev;
> > > > +	struct intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) :
> > > > NULL;
> > > > +
> > > > +	/*
> > > > +	 * On hotplug cases, we call _upfront_link_train directly.
> > > > +	 * In connected boot scenarios (boot with {MIPI/eDP} + DP),
> > > > +	 * BIOS typically brings up DP. Hence, we disable the crtc
> > > > +	 * to do _upfront_link_training. It gets re-enabled as part of
> > > > +	 * subsequent modeset.
> > > > +	 */
> > > > +	if (intel_encoder->connectors_active && crtc && crtc->enabled)
> > > > {
> > > > +		DRM_DEBUG_KMS("Disabling crtc %c for upfront link
> > > > training\n",
> > > > +				pipe_name(intel_crtc->pipe));
> > > > +		intel_crtc_control(crtc, false);
> > > > +	}
> > > > +
> > > > +	if (HAS_DDI(dev))
> > > > +		return intel_ddi_upfront_link_train(dev, intel_dp,
> > > > intel_crtc);
> > > > +
> > > > +	/* Not a supported platform. Assume we don't need upfront_train
> > > > */
> > > > +	return true;
> > > > +}
> > > > +
> > > > +
> > > >   static enum drm_connector_status
> > > >   intel_dp_detect(struct drm_connector *connector, bool force)
> > > >   {
> > > > @@ -4794,7 +4823,7 @@ intel_dp_detect(struct drm_connector *connector,
> > > > bool
> > > > force)
> > > >   	struct drm_device *dev = connector->dev;
> > > >   	enum drm_connector_status status;
> > > >   	enum intel_display_power_domain power_domain;
> > > > -	bool ret;
> > > > +	bool ret, do_upfront_link_train;
> > > >   	u8 sink_irq_vector;
> > > > 
> > > >   	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > > > @@ -4852,6 +4881,16 @@ intel_dp_detect(struct drm_connector *connector,
> > > > bool
> > > > force)
> > > >   			DRM_DEBUG_DRIVER("CP or sink specific irq
> > > > unhandled\n");
> > > >   	}
> > > > 
> > > > +	/* Do not do upfront link train, if it is a compliance request
> > > > */
> > > > +	do_upfront_link_train =
> > > > +		intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT &&
> > > > +		intel_dp->compliance_test_type !=
> > > > DP_TEST_LINK_TRAINING;
> > > > +
> > > > +	if (do_upfront_link_train) {
> > > > +		ret = intel_dp_upfront_link_train(intel_dp);
> > > > +		if (!ret)
> > > > +			status = connector_status_disconnected;
> > > > +	}
> > > >   out:
> > > >   	intel_dp_power_put(intel_dp, power_domain);
> > > >   	return status;
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 5bcdd37..82af4e6 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1007,6 +1007,8 @@ void intel_ddi_clock_get(struct intel_encoder
> > > > *encoder,
> > > >   			 struct intel_crtc_state *pipe_config);
> > > >   void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool
> > > > state);
> > > >   uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
> > > > +bool intel_ddi_upfront_link_train(struct drm_device *dev,
> > > > +		struct intel_dp *intel_dp, struct intel_crtc *crtc);
> > > > 
> > > >   /* intel_frontbuffer.c */
> > > >   void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx