[PATCHv2 0/4] Add USB typeC based DP support for BXT platform

[PATCHv2 0/4] Add USB typeC based DP support for BXT platform

[Durgadoss R]

This patch series adds upfront link training support to enable
USB type C based DP on BXT platform.

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.

Patches 1/4 :Refactoring/exporting DDI functions required
             to do upfront link train
Patch   2/4 :Moves finding unused crtc to a common function
Patch   3/4 :Make intel_ddi_get_crtc_encoder handle non-atomic paths
Patch   4/4 :Upfront implementation for DDI platforms
             (for now, tested on BXT A1).

Changes from v1:
* Using atomic_helper_dpms() to do DPMS off for upfront link
  training, instead of using load detect functions.
* Made intel_get_shared_dpll handle non-atomic paths by
  duplicating the required shared_dpll_config and then working
  on top of it. This helps in upfront link train code not
  directly touch the 'pll/pll->config' internals.
* Fixed the link_bw update logic in intel_dp_get_link_retry_params()
  for non-HBR2 panels.
* As per comments on earlier version, merged few patches
  (that added new functions) with the upfront link train patch,
  to facilitate easy review.

Link for v1:    https://patchwork.freedesktop.org/patch/67784/
Link for RFCv2: https://patchwork.freedesktop.org/patch/61776/
Link for RFCv1: https://patchwork.freedesktop.org/patch/59589/

Durgadoss R (4):
  drm/i915/dp: Export enable/disable_shared_dpll methods
  drm/i915: Make finding unused crtc as a generic function
  drm/i915/dp: Use legacy get_crtc_encoder in non-atomic paths
  drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

 drivers/gpu/drm/i915/intel_ddi.c     | 105 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |  79 ++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_dp.c      | 122 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  13 ++++
 4 files changed, 301 insertions(+), 18 deletions(-)

-- 
1.9.1

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

[PATCHv2 1/4] 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 4717de0..92cd5c6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1880,7 +1880,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;
@@ -1910,7 +1910,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 bc97012..c7a6e32 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1146,6 +1146,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

[PATCHv2 2/4] drm/i915: Make finding unused crtc as a generic function

[Durgadoss R]

Looping over the crtc list and finding an unused crtc
has users other than load_detect(). Hence move it to
a common function so that we can re-use the logic.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 92cd5c6..af50e61 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10417,6 +10417,27 @@ static int intel_modeset_setup_plane_state(struct drm_atomic_state *state,
 	return 0;
 }
 
+struct drm_crtc *intel_get_unused_crtc(struct drm_encoder *encoder)
+{
+	struct drm_crtc *possible_crtc;
+	struct drm_crtc *crtc = NULL;
+	struct drm_device *dev = encoder->dev;
+	int i = -1;
+
+	for_each_crtc(dev, possible_crtc) {
+		i++;
+		if (!(encoder->possible_crtcs & (1 << i)))
+			continue;
+		if (possible_crtc->state->enable)
+			continue;
+
+		crtc = possible_crtc;
+		break;
+	}
+
+	return crtc;
+}
+
 bool intel_get_load_detect_pipe(struct drm_connector *connector,
 				struct drm_display_mode *mode,
 				struct intel_load_detect_pipe *old,
@@ -10425,7 +10446,6 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 	struct intel_crtc *intel_crtc;
 	struct intel_encoder *intel_encoder =
 		intel_attached_encoder(connector);
-	struct drm_crtc *possible_crtc;
 	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_crtc *crtc = NULL;
 	struct drm_device *dev = encoder->dev;
@@ -10434,7 +10454,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 	struct drm_atomic_state *state = NULL;
 	struct drm_connector_state *connector_state;
 	struct intel_crtc_state *crtc_state;
-	int ret, i = -1;
+	int ret;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
 		      connector->base.id, connector->name,
@@ -10476,21 +10496,10 @@ retry:
 		return true;
 	}
 
-	/* Find an unused one (if possible) */
-	for_each_crtc(dev, possible_crtc) {
-		i++;
-		if (!(encoder->possible_crtcs & (1 << i)))
-			continue;
-		if (possible_crtc->state->enable)
-			continue;
-
-		crtc = possible_crtc;
-		break;
-	}
-
 	/*
 	 * If we didn't find an unused CRTC, don't use any.
 	 */
+	crtc = intel_get_unused_crtc(encoder);
 	if (!crtc) {
 		DRM_DEBUG_KMS("no pipe available for load-detect\n");
 		goto fail;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c7a6e32..9fe7c4b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1106,6 +1106,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 void intel_release_load_detect_pipe(struct drm_connector *connector,
 				    struct intel_load_detect_pipe *old,
 				    struct drm_modeset_acquire_ctx *ctx);
+struct drm_crtc *intel_get_unused_crtc(struct drm_encoder *encoder);
 int intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 			       struct drm_framebuffer *fb,
 			       const struct drm_plane_state *plane_state);
-- 
1.9.1

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

Re: [PATCHv2 2/4] drm/i915: Make finding unused crtc as a generic function

[Ander Conselvan De Oliveira]

On Mon, 2016-02-01 at 19:27 +0530, Durgadoss R wrote:
> Looping over the crtc list and finding an unused crtc
> has users other than load_detect(). 

Which other users? If there are other users they should be converted in this
patch. If the use will only come in a future patch, please make that clear in
the commit message.

> Hence move it to
> a common function so that we can re-use the logic.
> 
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 37 ++++++++++++++++++++++-------------
> -
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  2 files changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 92cd5c6..af50e61 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10417,6 +10417,27 @@ static int intel_modeset_setup_plane_state(struct
> drm_atomic_state *state,
>  	return 0;
>  }
>  
> +struct drm_crtc *intel_get_unused_crtc(struct drm_encoder *encoder)
> +{

This function is exported, so it needs some kernel doc.

> +	struct drm_crtc *possible_crtc;
> +	struct drm_crtc *crtc = NULL;
> +	struct drm_device *dev = encoder->dev;
> +	int i = -1;
> +
> +	for_each_crtc(dev, possible_crtc) {
> +		i++;
> +		if (!(encoder->possible_crtcs & (1 << i)))
> +			continue;
> +		if (possible_crtc->state->enable)
> +			continue;
> +
> +		crtc = possible_crtc;
> +		break;
> +	}
> +
> +	return crtc;
> +}
> +
>  bool intel_get_load_detect_pipe(struct drm_connector *connector,
>  				struct drm_display_mode *mode,
>  				struct intel_load_detect_pipe *old,
> @@ -10425,7 +10446,6 @@ bool intel_get_load_detect_pipe(struct drm_connector
> *connector,
>  	struct intel_crtc *intel_crtc;
>  	struct intel_encoder *intel_encoder =
>  		intel_attached_encoder(connector);
> -	struct drm_crtc *possible_crtc;
>  	struct drm_encoder *encoder = &intel_encoder->base;
>  	struct drm_crtc *crtc = NULL;
>  	struct drm_device *dev = encoder->dev;
> @@ -10434,7 +10454,7 @@ bool intel_get_load_detect_pipe(struct drm_connector
> *connector,
>  	struct drm_atomic_state *state = NULL;
>  	struct drm_connector_state *connector_state;
>  	struct intel_crtc_state *crtc_state;
> -	int ret, i = -1;
> +	int ret;
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
>  		      connector->base.id, connector->name,
> @@ -10476,21 +10496,10 @@ retry:
>  		return true;
>  	}
>  
> -	/* Find an unused one (if possible) */
> -	for_each_crtc(dev, possible_crtc) {
> -		i++;
> -		if (!(encoder->possible_crtcs & (1 << i)))
> -			continue;
> -		if (possible_crtc->state->enable)
> -			continue;
> -
> -		crtc = possible_crtc;
> -		break;
> -	}
> -
>  	/*
>  	 * If we didn't find an unused CRTC, don't use any.
>  	 */
> +	crtc = intel_get_unused_crtc(encoder);

The comment above looks out of place now. It is pretty obvious anyway, so maybe
just delete it.

With those fixed, this is 

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

 	if (!crtc) {
>  		DRM_DEBUG_KMS("no pipe available for load-detect\n");
>  		goto fail;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index c7a6e32..9fe7c4b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1106,6 +1106,7 @@ bool intel_get_load_detect_pipe(struct drm_connector
> *connector,
>  void intel_release_load_detect_pipe(struct drm_connector *connector,
>  				    struct intel_load_detect_pipe *old,
>  				    struct drm_modeset_acquire_ctx *ctx);
> +struct drm_crtc *intel_get_unused_crtc(struct drm_encoder *encoder);
>  int intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  			       struct drm_framebuffer *fb,
>  			       const struct drm_plane_state *plane_state);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCHv2 2/4] drm/i915: Make finding unused crtc as a generic function

[R, Durgadoss]

>-----Original Message-----
>From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com]
>Sent: Friday, February 12, 2016 10:23 PM
>To: R, Durgadoss <durgadoss.r@intel.com>; intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCHv2 2/4] drm/i915: Make finding unused crtc as a generic function
>
>On Mon, 2016-02-01 at 19:27 +0530, Durgadoss R wrote:
>> Looping over the crtc list and finding an unused crtc
>> has users other than load_detect().
>
>Which other users? If there are other users they should be converted in this
>patch. If the use will only come in a future patch, please make that clear in
>the commit message.

Thanks for the comments, Will update this patch with comments,
And send as part of next version.

Thanks,
Durga

>
>> Hence move it to
>> a common function so that we can re-use the logic.
>>
>> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 37 ++++++++++++++++++++++-------------
>> -
>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>  2 files changed, 24 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 92cd5c6..af50e61 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -10417,6 +10417,27 @@ static int intel_modeset_setup_plane_state(struct
>> drm_atomic_state *state,
>>  	return 0;
>>  }
>>
>> +struct drm_crtc *intel_get_unused_crtc(struct drm_encoder *encoder)
>> +{
>
>This function is exported, so it needs some kernel doc.
>
>> +	struct drm_crtc *possible_crtc;
>> +	struct drm_crtc *crtc = NULL;
>> +	struct drm_device *dev = encoder->dev;
>> +	int i = -1;
>> +
>> +	for_each_crtc(dev, possible_crtc) {
>> +		i++;
>> +		if (!(encoder->possible_crtcs & (1 << i)))
>> +			continue;
>> +		if (possible_crtc->state->enable)
>> +			continue;
>> +
>> +		crtc = possible_crtc;
>> +		break;
>> +	}
>> +
>> +	return crtc;
>> +}
>> +
>>  bool intel_get_load_detect_pipe(struct drm_connector *connector,
>>  				struct drm_display_mode *mode,
>>  				struct intel_load_detect_pipe *old,
>> @@ -10425,7 +10446,6 @@ bool intel_get_load_detect_pipe(struct drm_connector
>> *connector,
>>  	struct intel_crtc *intel_crtc;
>>  	struct intel_encoder *intel_encoder =
>>  		intel_attached_encoder(connector);
>> -	struct drm_crtc *possible_crtc;
>>  	struct drm_encoder *encoder = &intel_encoder->base;
>>  	struct drm_crtc *crtc = NULL;
>>  	struct drm_device *dev = encoder->dev;
>> @@ -10434,7 +10454,7 @@ bool intel_get_load_detect_pipe(struct drm_connector
>> *connector,
>>  	struct drm_atomic_state *state = NULL;
>>  	struct drm_connector_state *connector_state;
>>  	struct intel_crtc_state *crtc_state;
>> -	int ret, i = -1;
>> +	int ret;
>>
>>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
>>  		      connector->base.id, connector->name,
>> @@ -10476,21 +10496,10 @@ retry:
>>  		return true;
>>  	}
>>
>> -	/* Find an unused one (if possible) */
>> -	for_each_crtc(dev, possible_crtc) {
>> -		i++;
>> -		if (!(encoder->possible_crtcs & (1 << i)))
>> -			continue;
>> -		if (possible_crtc->state->enable)
>> -			continue;
>> -
>> -		crtc = possible_crtc;
>> -		break;
>> -	}
>> -
>>  	/*
>>  	 * If we didn't find an unused CRTC, don't use any.
>>  	 */
>> +	crtc = intel_get_unused_crtc(encoder);
>
>The comment above looks out of place now. It is pretty obvious anyway, so maybe
>just delete it.
>
>With those fixed, this is
>
>Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
>
> 	if (!crtc) {
>>  		DRM_DEBUG_KMS("no pipe available for load-detect\n");
>>  		goto fail;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index c7a6e32..9fe7c4b 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1106,6 +1106,7 @@ bool intel_get_load_detect_pipe(struct drm_connector
>> *connector,
>>  void intel_release_load_detect_pipe(struct drm_connector *connector,
>>  				    struct intel_load_detect_pipe *old,
>>  				    struct drm_modeset_acquire_ctx *ctx);
>> +struct drm_crtc *intel_get_unused_crtc(struct drm_encoder *encoder);
>>  int intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>>  			       struct drm_framebuffer *fb,
>>  			       const struct drm_plane_state *plane_state);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCHv2 3/4] drm/i915/dp: Use legacy get_crtc_encoder in non-atomic paths

[Durgadoss R]

This patch makes intel_crtc_get_new_crtc_encoder use get_crtc_encoder
when called from non-atomic paths. This helps when intel_get_shared_dpll
is called from non-atomic context.

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 1f9a368..3fb9a03 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -707,6 +707,9 @@ intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state)
 	int i;
 
 	state = crtc_state->base.state;
+	/* Use get_crtc_encoder() for non-atomic paths */
+	if (!state)
+		return intel_ddi_get_crtc_encoder(crtc_state->base.crtc);
 
 	for_each_connector_in_state(state, connector, connector_state, i) {
 		if (connector_state->crtc != crtc_state->base.crtc)
-- 
1.9.1

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

[PATCHv2 4/4] 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.
* 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, do upfront link training and then re-enable it back.
* All local changes made for upfront link training are reset
  to their previous values once it is done; so that the
  subsequent modeset is not aware of these changes.

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

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 3fb9a03..cc5cad5 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3217,6 +3217,108 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port)
 	return connector;
 }
 
+static void intel_ddi_commit_upfront_pll_config(struct intel_dp *intel_dp,
+					struct intel_shared_dpll *pll)
+{
+	struct intel_shared_dpll_config tmp_config;
+
+	/*
+	 * The shared_dpll_config is computed in intel_get_shared_dpll().
+	 * It is committed to 'pll->config' here to be used in
+	 * intel_enable/disable_shared_dpll functions. Before committing.
+	 * save the existing config, so that once upfront link training is
+	 * done, the original value of 'pll->config' can be restored.
+	 */
+	tmp_config = pll->config;
+	pll->config = intel_dp->upfront_pll_config;
+	intel_dp->upfront_pll_config = tmp_config;
+}
+
+static struct intel_shared_dpll *intel_ddi_select_upfront_pll_config(
+			struct intel_dp *intel_dp, struct intel_crtc *crtc)
+{
+	if (!intel_ddi_pll_select(crtc, crtc->config))
+		return NULL;
+
+	return intel_crtc_to_shared_dpll(crtc);
+}
+
+static void intel_ddi_clear_upfront_pll_config(struct intel_dp *intel_dp,
+					struct intel_shared_dpll *pll)
+{
+	pll->config = intel_dp->upfront_pll_config;
+}
+
+int intel_ddi_upfront_link_train(struct intel_dp *intel_dp,
+				struct intel_crtc *crtc)
+{
+	struct intel_connector *connector = intel_dp->attached_connector;
+	struct intel_encoder *encoder = connector->encoder;
+	struct intel_shared_dpll *pll = NULL;
+	struct drm_crtc *drm_crtc = NULL;
+	struct intel_crtc_state *tmp_crtc_config;
+	uint8_t link_bw, lane_count;
+
+	if (!crtc) {
+		drm_crtc = intel_get_unused_crtc(&encoder->base);
+		if (!drm_crtc) {
+			DRM_ERROR("No crtc for upfront link training\n");
+			return -EINVAL;
+		}
+		encoder->base.crtc = drm_crtc;
+		crtc = to_intel_crtc(drm_crtc);
+	}
+
+	/* Initialize with Max Link rate & lane count supported by panel */
+	link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
+	lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
+
+	/* Save the crtc->config */
+	tmp_crtc_config = crtc->config;
+
+	DRM_DEBUG_KMS("Using pipe %c\n", pipe_name(crtc->pipe));
+	do {
+		crtc->config->port_clock = drm_dp_bw_code_to_link_rate(link_bw);
+		crtc->config->lane_count = lane_count;
+
+		pll = intel_ddi_select_upfront_pll_config(intel_dp, crtc);
+		if (!pll) {
+			DRM_ERROR("Could not get shared DPLL\n");
+			goto exit;
+		}
+
+		intel_ddi_commit_upfront_pll_config(intel_dp, pll);
+
+		/* Enable PLL followed by port */
+		intel_enable_shared_dpll(crtc);
+		encoder->pre_enable(encoder);
+
+		/* Check if link training passed; if so update DPCD */
+		if (intel_dp->train_set_valid)
+			intel_dp_update_dpcd_params(intel_dp);
+
+		/* Disable port followed by PLL for next retry/clean up */
+		encoder->post_disable(encoder);
+		intel_disable_shared_dpll(crtc);
+
+		intel_ddi_clear_upfront_pll_config(intel_dp, pll);
+
+	} while (!intel_dp->train_set_valid &&
+		intel_dp_get_link_retry_params(intel_dp, &lane_count, &link_bw));
+exit:
+	/* Reset local associations made */
+	if (drm_crtc)
+		encoder->base.crtc = NULL;
+
+	/* Restore crtc config */
+	crtc->config = tmp_crtc_config;
+
+	DRM_DEBUG_KMS("Upfront link train %s: lanes:%d bw:%d\n",
+	intel_dp->train_set_valid ? "Passed" : "Failed", lane_count, link_bw);
+
+	return intel_dp->train_set_valid ? 0 : -1;
+}
+
 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_display.c b/drivers/gpu/drm/i915/intel_display.c
index af50e61..6a650aa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4238,16 +4238,45 @@ static void lpt_pch_enable(struct drm_crtc *crtc)
 	lpt_enable_pch_transcoder(dev_priv, cpu_transcoder);
 }
 
+static
+void intel_get_new_shared_dpll_config(struct drm_i915_private *dev_priv,
+				  struct intel_shared_dpll_config *shared_dpll)
+{
+	enum intel_dpll_id i;
+
+	/* Create a new shared dpll config by duplicating pll->config */
+	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
+		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
+		shared_dpll[i] = pll->config;
+		shared_dpll[i].crtc_mask = 0;
+	}
+}
+
 struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 						struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_shared_dpll *pll;
 	struct intel_shared_dpll_config *shared_dpll;
+	struct intel_shared_dpll_config tmp_config[I915_NUM_PLLS];
 	enum intel_dpll_id i;
 	int max = dev_priv->num_shared_dpll;
 
-	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state);
+	/*
+	 * intel_get_shared_dpll needs a shared_dpll[] to store the computed
+	 * dpll_config values. For atomic path, it is stored in
+	 * intel_atomic_state->shared_dpll[], which is later committed to
+	 * dev_priv->shared_dpll[] in atomic commit. For non-atomic path,
+	 * (where intel_atomic_state does not exist) the dpll_config values
+	 * are stored in 'tmp_config[]' and copied to encoder structures
+	 * for later use.
+	 */
+	if (crtc_state->base.state) {
+		shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state);
+	} else {
+		intel_get_new_shared_dpll_config(dev_priv, tmp_config);
+		shared_dpll = tmp_config;
+	}
 
 	if (HAS_PCH_IBX(dev_priv->dev)) {
 		/* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
@@ -4277,6 +4306,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 		pll = &dev_priv->shared_dplls[i];
 		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
 			crtc->base.base.id, pll->name);
+
 		WARN_ON(shared_dpll[i].crtc_mask);
 
 		goto found;
@@ -4325,6 +4355,12 @@ found:
 
 	shared_dpll[i].crtc_mask |= 1 << crtc->pipe;
 
+	/*
+	 * For DP, this shared dpll config is saved to intel_dp,
+	 * and used by upfront link train logic subsequently.
+	 */
+	intel_dp_update_shared_dpll_config(crtc_state, shared_dpll[i]);
+
 	return pll;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e2bea710..cc7b6f3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1679,6 +1679,55 @@ void intel_dp_set_link_params(struct intel_dp *intel_dp,
 	intel_dp->lane_count = pipe_config->lane_count;
 }
 
+void intel_dp_update_dpcd_params(struct intel_dp *intel_dp)
+{
+	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;
+
+	intel_dp->dpcd[DP_MAX_LINK_RATE] =
+			drm_dp_link_rate_to_bw_code(intel_dp->link_rate);
+}
+
+bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
+			uint8_t *lane_count, uint8_t *link_bw)
+{
+	/*
+	 * As per DP1.3 Spec, retry all link rates for a particular
+	 * lane count value, before reducing number of lanes.
+	 */
+	if (*link_bw == DP_LINK_BW_5_4) {
+		*link_bw = DP_LINK_BW_2_7;
+	} else if (*link_bw == DP_LINK_BW_2_7) {
+		*link_bw = DP_LINK_BW_1_62;
+	} else if (*lane_count == 4) {
+		*lane_count = 2;
+		*link_bw = intel_dp_max_link_bw(intel_dp);
+	} else if (*lane_count == 2) {
+		*lane_count = 1;
+		*link_bw = intel_dp_max_link_bw(intel_dp);
+	} else {
+		/* Tried all combinations, so exit */
+		return false;
+	}
+
+	return true;
+}
+
+void intel_dp_update_shared_dpll_config(struct intel_crtc_state *pipe_config,
+				struct intel_shared_dpll_config config)
+{
+	struct intel_encoder *encoder;
+	struct intel_dp *intel_dp;
+
+	encoder = intel_ddi_get_crtc_new_encoder(pipe_config);
+	if (!encoder || encoder->type != INTEL_OUTPUT_DISPLAYPORT)
+		return;
+
+	intel_dp = enc_to_intel_dp(&encoder->base);
+	intel_dp->upfront_pll_config = config;
+}
+
 static void intel_dp_prepare(struct intel_encoder *encoder)
 {
 	struct drm_device *dev = encoder->base.dev;
@@ -4601,6 +4650,66 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
 	intel_dp->has_audio = false;
 }
 
+static bool intel_dp_upfront_link_train(struct drm_connector *connector)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct drm_device *dev = intel_encoder->base.dev;
+	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) : NULL;
+	struct drm_modeset_acquire_ctx ctx, *old_ctx = NULL;
+	bool need_dpms_on = false;
+	int ret;
+
+	if (!IS_BROXTON(dev))
+		return true;
+
+	/*
+	 * 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 and then re-enable it back.
+	 */
+	if (crtc && crtc->enabled && intel_crtc->active) {
+		old_ctx = crtc->acquire_ctx;
+		drm_modeset_acquire_init(&ctx, 0);
+retry:
+		ret = drm_modeset_lock(&config->connection_mutex, &ctx);
+		if (ret == -EDEADLK) {
+			drm_modeset_backoff(&ctx);
+			goto retry;
+		}
+
+		crtc->acquire_ctx = &ctx;
+		ret = drm_atomic_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF);
+		if (ret) {
+			DRM_ERROR("DPMS off failed:%d\n", ret);
+			goto exit_unlock;
+		}
+
+		need_dpms_on = true;
+	}
+
+	if (HAS_DDI(dev))
+		ret = intel_ddi_upfront_link_train(intel_dp, intel_crtc);
+		/* Other platforms upfront link train call goes here..*/
+
+	if (!need_dpms_on)
+		return ret;
+
+	ret = drm_atomic_helper_connector_dpms(connector, DRM_MODE_DPMS_ON);
+	if (ret)
+		DRM_ERROR("DPMS on failed:%d\n", ret);
+
+exit_unlock:
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+	crtc->acquire_ctx = old_ctx;
+	return ret;
+}
+
 static enum drm_connector_status
 intel_dp_detect(struct drm_connector *connector, bool force)
 {
@@ -4610,7 +4719,8 @@ 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 do_upfront_link_train;
+	int ret;
 	u8 sink_irq_vector;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
@@ -4684,6 +4794,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(connector);
+		if (ret)
+			status = connector_status_disconnected;
+	}
 out:
 	intel_display_power_put(to_i915(dev), power_domain);
 	return status;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9fe7c4b..c5ca4ab 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -812,6 +812,9 @@ struct intel_dp {
 	unsigned long compliance_test_type;
 	unsigned long compliance_test_data;
 	bool compliance_test_active;
+
+	/* Used to store pll config for upfront link training */
+	struct intel_shared_dpll_config upfront_pll_config;
 };
 
 struct intel_digital_port {
@@ -1031,6 +1034,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);
+int intel_ddi_upfront_link_train(struct intel_dp *intel_dp,
+				struct intel_crtc *crtc);
 
 /* intel_frontbuffer.c */
 void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
@@ -1239,6 +1244,9 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 			     struct intel_connector *intel_connector);
 void intel_dp_set_link_params(struct intel_dp *intel_dp,
 			      const struct intel_crtc_state *pipe_config);
+void intel_dp_update_dpcd_params(struct intel_dp *intel_dp);
+bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
+				uint8_t *lane_count, uint8_t *link_bw);
 void intel_dp_start_link_train(struct intel_dp *intel_dp);
 void intel_dp_stop_link_train(struct intel_dp *intel_dp);
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
@@ -1246,6 +1254,8 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder);
 int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config);
+void intel_dp_update_shared_dpll_config(struct intel_crtc_state *pipe_config,
+				struct intel_shared_dpll_config config);
 bool intel_dp_is_edp(struct drm_device *dev, enum port port);
 enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
 				  bool long_hpd);
-- 
1.9.1

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

Re: [PATCHv2 4/4] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[Ander Conselvan De Oliveira]

On Mon, 2016-02-01 at 19:27 +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.
> * 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, do upfront link training and then re-enable it back.
> * All local changes made for upfront link training are reset
>   to their previous values once it is done; so that the
>   subsequent modeset is not aware of these changes.
> 

Please always include a changelog when sending a new version of a patch.

> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     | 102 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |  38 ++++++++++-
>  drivers/gpu/drm/i915/intel_dp.c      | 122 ++++++++++++++++++++++++++++++++++
> -
>  drivers/gpu/drm/i915/intel_drv.h     |  10 +++
>  4 files changed, 270 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 3fb9a03..cc5cad5 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3217,6 +3217,108 @@ intel_ddi_init_hdmi_connector(struct
> intel_digital_port *intel_dig_port)
>  	return connector;
>  }
>  
> +static void intel_ddi_commit_upfront_pll_config(struct intel_dp *intel_dp,
> +					struct intel_shared_dpll *pll)
> +{
> +	struct intel_shared_dpll_config tmp_config;
> +
> +	/*
> +	 * The shared_dpll_config is computed in intel_get_shared_dpll().
> +	 * It is committed to 'pll->config' here to be used in
> +	 * intel_enable/disable_shared_dpll functions. Before committing.
> +	 * save the existing config, so that once upfront link training is
> +	 * done, the original value of 'pll->config' can be restored.
> +	 */
> +	tmp_config = pll->config;
> +	pll->config = intel_dp->upfront_pll_config;
> +	intel_dp->upfront_pll_config = tmp_config;
> +}
> +
> +static struct intel_shared_dpll *intel_ddi_select_upfront_pll_config(
> +			struct intel_dp *intel_dp, struct intel_crtc *crtc)
> +{
> +	if (!intel_ddi_pll_select(crtc, crtc->config))
> +		return NULL;
> +
> +	return intel_crtc_to_shared_dpll(crtc);
> +}
> +
> +static void intel_ddi_clear_upfront_pll_config(struct intel_dp *intel_dp,
> +					struct intel_shared_dpll *pll)
> +{
> +	pll->config = intel_dp->upfront_pll_config;
> +}
> +

The shared pll interface is really getting in the way here. And BXT plls aren't
even shared. We are jumping through hoops to get the pll that matches the given
encoder and to call the code that calculates the dpll_hw_state based on the DP
link rate. I'd like to get that interface fixed but I reckon it will be tricky
to find something that works for all the platforms we support. That's the next
thing on my todo list.

If we have to live with some intermediary solution, I think it would be better
to (almost) completely bypass the shared dpll stuff. First we would need to
split bxt_ddi_pll_sel() so that we would have a function that takes the link
bandwidth and spits out a dpll_hw_state without looking at crtc_state. Then we
would just take the right pll based on dig_port->port, make sure it is not being
used and program it with the hw state we get from that new function.

I wrote a patch to do that split. With it, the PLL enable logic in the upfront
link train logic would look a bit like below. There is some potential for
problems with the state checker, but it should be fine as long as the old dpll
hw state is saved and restored when everything is over.


/* FIXME: this only works for BXT */
dpll_id = (enum intel_dpll_id) dig_port->port;
pll = dev_priv->shared_dplls[dpll_id];

if (WARN_ON(pll->config.crtc_mask) || WARN_ON(pll->active))
	goto exit;

bxt_ddi_dp_pll_dividers(crtc->config->port_clock, &clk_div);
if (!bxt_ddi_set_dpll_hw_state(clock, &clk_div,
			       &pll->config.hw_state)) {
	DRM_ERROR("Could not setup DPLL\n");
	goto exit;
}
                                                                                
      
/* Enable PLL followed by port */
pll->enable(dev_priv, pll);
encoder->pre_enable(encoder);


This solution would remove the need for patches 1 and 3.

What do you think?

> +int intel_ddi_upfront_link_train(struct intel_dp *intel_dp,
> +				struct intel_crtc *crtc)
> +{
> +	struct intel_connector *connector = intel_dp->attached_connector;
> +	struct intel_encoder *encoder = connector->encoder;
> +	struct intel_shared_dpll *pll = NULL;
> +	struct drm_crtc *drm_crtc = NULL;
> +	struct intel_crtc_state *tmp_crtc_config;
> +	uint8_t link_bw, lane_count;
> +
> +	if (!crtc) {
> +		drm_crtc = intel_get_unused_crtc(&encoder->base);
> +		if (!drm_crtc) {
> +			DRM_ERROR("No crtc for upfront link training\n");
> +			return -EINVAL;
> +		}
> +		encoder->base.crtc = drm_crtc;
> +		crtc = to_intel_crtc(drm_crtc);
> +	}
> +
> +	/* Initialize with Max Link rate & lane count supported by panel */
> +	link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
> +	lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
> +
> +	/* Save the crtc->config */
> +	tmp_crtc_config = crtc->config;
> +
> +	DRM_DEBUG_KMS("Using pipe %c\n", pipe_name(crtc->pipe));
> +	do {
> +		crtc->config->port_clock =
> drm_dp_bw_code_to_link_rate(link_bw);
> +		crtc->config->lane_count = lane_count;
> +
> +		pll = intel_ddi_select_upfront_pll_config(intel_dp, crtc);
> +		if (!pll) {
> +			DRM_ERROR("Could not get shared DPLL\n");
> +			goto exit;
> +		}
> +
> +		intel_ddi_commit_upfront_pll_config(intel_dp, pll);
> +
> +		/* Enable PLL followed by port */
> +		intel_enable_shared_dpll(crtc);
> +		encoder->pre_enable(encoder);
> +
> +		/* Check if link training passed; if so update DPCD */
> +		if (intel_dp->train_set_valid)
> +			intel_dp_update_dpcd_params(intel_dp);
> +
> +		/* Disable port followed by PLL for next retry/clean up */
> +		encoder->post_disable(encoder);
> +		intel_disable_shared_dpll(crtc);
> +
> +		intel_ddi_clear_upfront_pll_config(intel_dp, pll);
> +
> +	} while (!intel_dp->train_set_valid &&
> +		intel_dp_get_link_retry_params(intel_dp, &lane_count,
> &link_bw));
> +exit:
> +	/* Reset local associations made */
> +	if (drm_crtc)
> +		encoder->base.crtc = NULL;
> +
> +	/* Restore crtc config */
> +	crtc->config = tmp_crtc_config;
> +
> +	DRM_DEBUG_KMS("Upfront link train %s: lanes:%d bw:%d\n",
> +	intel_dp->train_set_valid ? "Passed" : "Failed", lane_count,
> link_bw);
> +
> +	return intel_dp->train_set_valid ? 0 : -1;
> +}
> +
>  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_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index af50e61..6a650aa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4238,16 +4238,45 @@ static void lpt_pch_enable(struct drm_crtc *crtc)
>  	lpt_enable_pch_transcoder(dev_priv, cpu_transcoder);
>  }
>  
> +static
> +void intel_get_new_shared_dpll_config(struct drm_i915_private *dev_priv,
> +				  struct intel_shared_dpll_config
> *shared_dpll)
> +{
> +	enum intel_dpll_id i;
> +
> +	/* Create a new shared dpll config by duplicating pll->config */
> +	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> +		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
> +		shared_dpll[i] = pll->config;
> +		shared_dpll[i].crtc_mask = 0;
> +	}
> +}
> +
>  struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  						struct intel_crtc_state
> *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>  	struct intel_shared_dpll *pll;
>  	struct intel_shared_dpll_config *shared_dpll;
> +	struct intel_shared_dpll_config tmp_config[I915_NUM_PLLS];
>  	enum intel_dpll_id i;
>  	int max = dev_priv->num_shared_dpll;
>  
> -	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state
> ->base.state);
> +	/*
> +	 * intel_get_shared_dpll needs a shared_dpll[] to store the computed
> +	 * dpll_config values. For atomic path, it is stored in
> +	 * intel_atomic_state->shared_dpll[], which is later committed to
> +	 * dev_priv->shared_dpll[] in atomic commit. For non-atomic path,
> +	 * (where intel_atomic_state does not exist) the dpll_config values
> +	 * are stored in 'tmp_config[]' and copied to encoder structures
> +	 * for later use.
> +	 */
> +	if (crtc_state->base.state) {
> +		shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state
> ->base.state);
> +	} else {
> +		intel_get_new_shared_dpll_config(dev_priv, tmp_config);
> +		shared_dpll = tmp_config;
> +	}
>  
>  	if (HAS_PCH_IBX(dev_priv->dev)) {
>  		/* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
> @@ -4277,6 +4306,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct
> intel_crtc *crtc,
>  		pll = &dev_priv->shared_dplls[i];
>  		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>  			crtc->base.base.id, pll->name);
> +
>  		WARN_ON(shared_dpll[i].crtc_mask);
>  
>  		goto found;
> @@ -4325,6 +4355,12 @@ found:
>  
>  	shared_dpll[i].crtc_mask |= 1 << crtc->pipe;
>  
> +	/*
> +	 * For DP, this shared dpll config is saved to intel_dp,
> +	 * and used by upfront link train logic subsequently.
> +	 */
> +	intel_dp_update_shared_dpll_config(crtc_state, shared_dpll[i]);
> +
>  	return pll;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e2bea710..cc7b6f3 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1679,6 +1679,55 @@ void intel_dp_set_link_params(struct intel_dp
> *intel_dp,
>  	intel_dp->lane_count = pipe_config->lane_count;
>  }
>  
> +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp)
> +{
> +	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;
> +
> +	intel_dp->dpcd[DP_MAX_LINK_RATE] =
> +			drm_dp_link_rate_to_bw_code(intel_dp->link_rate);

Maybe it would be good to have an explicit field for actual max lane count and
link rate. That way, reading the DPCD from debugfs will give the actual results
read from the sink instead of an edited value.

> +}
> +
> +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
> +			uint8_t *lane_count, uint8_t *link_bw)
> +{
> +	/*
> +	 * As per DP1.3 Spec, retry all link rates for a particular
> +	 * lane count value, before reducing number of lanes.
> +	 */
> +	if (*link_bw == DP_LINK_BW_5_4) {
> +		*link_bw = DP_LINK_BW_2_7;
> +	} else if (*link_bw == DP_LINK_BW_2_7) {
> +		*link_bw = DP_LINK_BW_1_62;
> +	} else if (*lane_count == 4) {
> +		*lane_count = 2;
> +		*link_bw = intel_dp_max_link_bw(intel_dp);
> +	} else if (*lane_count == 2) {
> +		*lane_count = 1;
> +		*link_bw = intel_dp_max_link_bw(intel_dp);
> +	} else {
> +		/* Tried all combinations, so exit */
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +void intel_dp_update_shared_dpll_config(struct intel_crtc_state *pipe_config,
> +				struct intel_shared_dpll_config config)
> +{
> +	struct intel_encoder *encoder;
> +	struct intel_dp *intel_dp;
> +
> +	encoder = intel_ddi_get_crtc_new_encoder(pipe_config);
> +	if (!encoder || encoder->type != INTEL_OUTPUT_DISPLAYPORT)
> +		return;
> +
> +	intel_dp = enc_to_intel_dp(&encoder->base);
> +	intel_dp->upfront_pll_config = config;
> +}
> +
>  static void intel_dp_prepare(struct intel_encoder *encoder)
>  {
>  	struct drm_device *dev = encoder->base.dev;
> @@ -4601,6 +4650,66 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>  	intel_dp->has_audio = false;
>  }
>  
> +static bool intel_dp_upfront_link_train(struct drm_connector *connector)
> +{
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> +	struct drm_device *dev = intel_encoder->base.dev;
> +	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) : NULL;
> +	struct drm_modeset_acquire_ctx ctx, *old_ctx = NULL;
> +	bool need_dpms_on = false;
> +	int ret;
> +
> +	if (!IS_BROXTON(dev))
> +		return true;
> +
> +	/*
> +	 * 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 and then re-enable it back.
> +	 */
> +	if (crtc && crtc->enabled && intel_crtc->active) {
> +		old_ctx = crtc->acquire_ctx;
> +		drm_modeset_acquire_init(&ctx, 0);
> +retry:
> +		ret = drm_modeset_lock(&config->connection_mutex, &ctx);
> +		if (ret == -EDEADLK) {
> +			drm_modeset_backoff(&ctx);
> +			goto retry;
> +		}

We need to acquire those locks even if the crtc is disabled, otherwise a modeset
could sneak past and wreak havoc while we are doing the link training and
fiddling with the plls.

> +
> +		crtc->acquire_ctx = &ctx;
> +		ret = drm_atomic_helper_connector_dpms(connector,
> DRM_MODE_DPMS_OFF);

The crtc->acquire_ctx override seems wrong. One thing that slipped by me when I
looked at the previous version of this patch is that we need to acquire some
locks even if the crtc is already off. So it is probably better to write a
separate function that takes an acquire context and uses atomic to set
crtc_state->active to false and calls drm_atomic_commit().

> +		if (ret) {
> +			DRM_ERROR("DPMS off failed:%d\n", ret);
> +			goto exit_unlock;
> +		}
> +
> +		need_dpms_on = true;
> +	}
> +
> +	if (HAS_DDI(dev))
> +		ret = intel_ddi_upfront_link_train(intel_dp, intel_crtc);
> +		/* Other platforms upfront link train call goes here..*/
> +
> +	if (!need_dpms_on)
> +		return ret;
> +
> +	ret = drm_atomic_helper_connector_dpms(connector, DRM_MODE_DPMS_ON);
> +	if (ret)
> +		DRM_ERROR("DPMS on failed:%d\n", ret);
> +
> +exit_unlock:
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +	crtc->acquire_ctx = old_ctx;
> +	return ret;

The return type of this function is bool. The conversion here will do the wrong
thing, since ret potentially has the return value of functions that return non
-zero on failure.

> +}
> +
>  static enum drm_connector_status
>  intel_dp_detect(struct drm_connector *connector, bool force)
>  {
> @@ -4610,7 +4719,8 @@ 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 do_upfront_link_train;
> +	int ret;
>  	u8 sink_irq_vector;
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> @@ -4684,6 +4794,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(connector);
> +		if (ret)
> +			status = connector_status_disconnected;
> +	}

So if upfront link training fails because there is no unused crtc we'll say that
the connector is disconnected?


Ander

>  out:
>  	intel_display_power_put(to_i915(dev), power_domain);
>  	return status;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 9fe7c4b..c5ca4ab 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -812,6 +812,9 @@ struct intel_dp {
>  	unsigned long compliance_test_type;
>  	unsigned long compliance_test_data;
>  	bool compliance_test_active;
> +
> +	/* Used to store pll config for upfront link training */
> +	struct intel_shared_dpll_config upfront_pll_config;
>  };
>  
>  struct intel_digital_port {
> @@ -1031,6 +1034,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);
> +int intel_ddi_upfront_link_train(struct intel_dp *intel_dp,
> +				struct intel_crtc *crtc);
>  
>  /* intel_frontbuffer.c */
>  void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> @@ -1239,6 +1244,9 @@ bool intel_dp_init_connector(struct intel_digital_port
> *intel_dig_port,
>  			     struct intel_connector *intel_connector);
>  void intel_dp_set_link_params(struct intel_dp *intel_dp,
>  			      const struct intel_crtc_state *pipe_config);
> +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp);
> +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
> +				uint8_t *lane_count, uint8_t *link_bw);
>  void intel_dp_start_link_train(struct intel_dp *intel_dp);
>  void intel_dp_stop_link_train(struct intel_dp *intel_dp);
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> @@ -1246,6 +1254,8 @@ void intel_dp_encoder_destroy(struct drm_encoder
> *encoder);
>  int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
>  bool intel_dp_compute_config(struct intel_encoder *encoder,
>  			     struct intel_crtc_state *pipe_config);
> +void intel_dp_update_shared_dpll_config(struct intel_crtc_state *pipe_config,
> +				struct intel_shared_dpll_config config);
>  bool intel_dp_is_edp(struct drm_device *dev, enum port port);
>  enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
>  				  bool long_hpd);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Split bxt_ddi_pll_select()

[Ander Conselvan de Oliveira]

Split out of bxt_ddi_pll_select() the logic that calculates the pll
dividers and dpll_hw_state into a new function that doesn't depend on
crtc state. This will be used for enabling the port pll when doing
upfront link training.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 150 ++++++++++++++++++++++++---------------
 1 file changed, 91 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 00bb299..35a2d96 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1583,6 +1583,8 @@ struct bxt_clk_div {
 	uint32_t m2_frac;
 	bool m2_frac_en;
 	uint32_t n;
+
+	int vco;
 };
 
 /* pre-calculated values for DP linkrates */
@@ -1597,54 +1599,61 @@ 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)
+bxt_ddi_hdmi_pll_dividers(struct intel_crtc *intel_crtc,
+			  struct intel_crtc_state *crtc_state, int clock,
+			  struct bxt_clk_div *clk_div)
 {
-	struct intel_shared_dpll *pll;
-	struct bxt_clk_div clk_div = {0};
-	int vco = 0;
-	uint32_t prop_coef, int_coef, gain_ctl, targ_cnt;
-	uint32_t lanestagger;
-	int clock = crtc_state->port_clock;
+	intel_clock_t best_clock;
 
-	if (intel_encoder->type == INTEL_OUTPUT_HDMI) {
-		intel_clock_t best_clock;
+	/* Calculate HDMI div */
+	/*
+	 * FIXME: tie the following calculation into
+	 * i9xx_crtc_compute_clock
+	 */
+	if (!bxt_find_best_dpll(crtc_state, clock, &best_clock)) {
+		DRM_DEBUG_DRIVER("no PLL dividers found for clock %d pipe %c\n",
+				 clock, pipe_name(intel_crtc->pipe));
+		return false;
+	}
 
-		/* Calculate HDMI div */
-		/*
-		 * FIXME: tie the following calculation into
-		 * i9xx_crtc_compute_clock
-		 */
-		if (!bxt_find_best_dpll(crtc_state, clock, &best_clock)) {
-			DRM_DEBUG_DRIVER("no PLL dividers found for clock %d pipe %c\n",
-					 clock, pipe_name(intel_crtc->pipe));
-			return false;
-		}
+	clk_div->p1 = best_clock.p1;
+	clk_div->p2 = best_clock.p2;
+	WARN_ON(best_clock.m1 != 2);
+	clk_div->n = best_clock.n;
+	clk_div->m2_int = best_clock.m2 >> 22;
+	clk_div->m2_frac = best_clock.m2 & ((1 << 22) - 1);
+	clk_div->m2_frac_en = clk_div->m2_frac != 0;
 
-		clk_div.p1 = best_clock.p1;
-		clk_div.p2 = best_clock.p2;
-		WARN_ON(best_clock.m1 != 2);
-		clk_div.n = best_clock.n;
-		clk_div.m2_int = best_clock.m2 >> 22;
-		clk_div.m2_frac = best_clock.m2 & ((1 << 22) - 1);
-		clk_div.m2_frac_en = clk_div.m2_frac != 0;
+	clk_div->vco = best_clock.vco;
 
-		vco = best_clock.vco;
-	} else if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
-			intel_encoder->type == INTEL_OUTPUT_EDP) {
-		int i;
+	return true;
+}
 
-		clk_div = bxt_dp_clk_val[0];
-		for (i = 0; i < ARRAY_SIZE(bxt_dp_clk_val); ++i) {
-			if (bxt_dp_clk_val[i].clock == clock) {
-				clk_div = bxt_dp_clk_val[i];
-				break;
-			}
+static void
+bxt_ddi_dp_pll_dividers(int clock, struct bxt_clk_div *clk_div)
+{
+	int i;
+
+	*clk_div = bxt_dp_clk_val[0];
+	for (i = 0; i < ARRAY_SIZE(bxt_dp_clk_val); ++i) {
+		if (bxt_dp_clk_val[i].clock == clock) {
+			*clk_div = bxt_dp_clk_val[i];
+			break;
 		}
-		vco = clock * 10 / 2 * clk_div.p1 * clk_div.p2;
 	}
 
+	clk_div->vco = clock * 10 / 2 * clk_div->p1 * clk_div->p2;
+}
+
+static bool
+bxt_ddi_set_dpll_hw_state(int clock,
+			  struct bxt_clk_div *clk_div,
+			  struct intel_dpll_hw_state *dpll_hw_state)
+{
+	int vco = clk_div->vco;
+	uint32_t prop_coef, int_coef, gain_ctl, targ_cnt;
+	uint32_t lanestagger;
+
 	if (vco >= 6200000 && vco <= 6700000) {
 		prop_coef = 4;
 		int_coef = 9;
@@ -1666,9 +1675,6 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
 		return false;
 	}
 
-	memset(&crtc_state->dpll_hw_state, 0,
-	       sizeof(crtc_state->dpll_hw_state));
-
 	if (clock > 270000)
 		lanestagger = 0x18;
 	else if (clock > 135000)
@@ -1680,33 +1686,59 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
 	else
 		lanestagger = 0x02;
 
-	crtc_state->dpll_hw_state.ebb0 =
-		PORT_PLL_P1(clk_div.p1) | PORT_PLL_P2(clk_div.p2);
-	crtc_state->dpll_hw_state.pll0 = clk_div.m2_int;
-	crtc_state->dpll_hw_state.pll1 = PORT_PLL_N(clk_div.n);
-	crtc_state->dpll_hw_state.pll2 = clk_div.m2_frac;
+	dpll_hw_state->ebb0 = PORT_PLL_P1(clk_div->p1) | PORT_PLL_P2(clk_div->p2);
+	dpll_hw_state->pll0 = clk_div->m2_int;
+	dpll_hw_state->pll1 = PORT_PLL_N(clk_div->n);
+	dpll_hw_state->pll2 = clk_div->m2_frac;
 
-	if (clk_div.m2_frac_en)
-		crtc_state->dpll_hw_state.pll3 =
+	if (clk_div->m2_frac_en)
+		dpll_hw_state->pll3 =
 			PORT_PLL_M2_FRAC_ENABLE;
 
-	crtc_state->dpll_hw_state.pll6 =
-		prop_coef | PORT_PLL_INT_COEFF(int_coef);
-	crtc_state->dpll_hw_state.pll6 |=
-		PORT_PLL_GAIN_CTL(gain_ctl);
+	dpll_hw_state->pll6 = prop_coef | PORT_PLL_INT_COEFF(int_coef);
+	dpll_hw_state->pll6 |= PORT_PLL_GAIN_CTL(gain_ctl);
 
-	crtc_state->dpll_hw_state.pll8 = targ_cnt;
+	dpll_hw_state->pll8 = targ_cnt;
 
-	crtc_state->dpll_hw_state.pll9 = 5 << PORT_PLL_LOCK_THRESHOLD_SHIFT;
+	dpll_hw_state->pll9 = 5 << PORT_PLL_LOCK_THRESHOLD_SHIFT;
 
-	crtc_state->dpll_hw_state.pll10 =
+	dpll_hw_state->pll10 =
 		PORT_PLL_DCO_AMP(PORT_PLL_DCO_AMP_DEFAULT)
 		| PORT_PLL_DCO_AMP_OVR_EN_H;
 
-	crtc_state->dpll_hw_state.ebb4 = PORT_PLL_10BIT_CLK_ENABLE;
+	dpll_hw_state->ebb4 = PORT_PLL_10BIT_CLK_ENABLE;
+
+	dpll_hw_state->pcsdw12 = LANESTAGGER_STRAP_OVRD | lanestagger;
+
+	return true;
+}
+
+static bool
+bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
+		   struct intel_crtc_state *crtc_state,
+		   struct intel_encoder *intel_encoder)
+{
+	struct bxt_clk_div clk_div = {0};
+	struct intel_dpll_hw_state dpll_hw_state = {0};
+	struct intel_shared_dpll *pll;
+	int clock = crtc_state->port_clock;
+
+	if (intel_encoder->type == INTEL_OUTPUT_HDMI
+	    && !bxt_ddi_hdmi_pll_dividers(intel_crtc, crtc_state,
+					  clock, &clk_div))
+			return false;
+
+	if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
+	    intel_encoder->type == INTEL_OUTPUT_EDP)
+		bxt_ddi_dp_pll_dividers(clock, &clk_div);
+
+	if (!bxt_ddi_set_dpll_hw_state(clock, &clk_div, &dpll_hw_state))
+		return false;
+
+	memset(&crtc_state->dpll_hw_state, 0,
+	       sizeof(crtc_state->dpll_hw_state));
 
-	crtc_state->dpll_hw_state.pcsdw12 =
-		LANESTAGGER_STRAP_OVRD | lanestagger;
+	crtc_state->dpll_hw_state = dpll_hw_state;
 
 	pll = intel_get_shared_dpll(intel_crtc, crtc_state);
 	if (pll == NULL) {
-- 
2.4.3

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

Re: [PATCHv2 4/4] 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: Friday, February 12, 2016 10:13 PM
>To: R, Durgadoss <durgadoss.r@intel.com>; intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCHv2 4/4] drm/i915/dp: Enable Upfront link training for typeC DP support on
>BXT
>
>On Mon, 2016-02-01 at 19:27 +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.
>> * 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, do upfront link training and then re-enable it back.
>> * All local changes made for upfront link training are reset
>>   to their previous values once it is done; so that the
>>   subsequent modeset is not aware of these changes.
>>
>
>Please always include a changelog when sending a new version of a patch.

Sure, will add it in the next version.

>
>> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c     | 102 +++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_display.c |  38 ++++++++++-
>>  drivers/gpu/drm/i915/intel_dp.c      | 122 ++++++++++++++++++++++++++++++++++
>> -
>>  drivers/gpu/drm/i915/intel_drv.h     |  10 +++
>>  4 files changed, 270 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>> b/drivers/gpu/drm/i915/intel_ddi.c
>> index 3fb9a03..cc5cad5 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -3217,6 +3217,108 @@ intel_ddi_init_hdmi_connector(struct
>> intel_digital_port *intel_dig_port)
>>  	return connector;
>>  }
>>
>> +static void intel_ddi_commit_upfront_pll_config(struct intel_dp *intel_dp,
>> +					struct intel_shared_dpll *pll)
>> +{
>> +	struct intel_shared_dpll_config tmp_config;
>> +
>> +	/*
>> +	 * The shared_dpll_config is computed in intel_get_shared_dpll().
>> +	 * It is committed to 'pll->config' here to be used in
>> +	 * intel_enable/disable_shared_dpll functions. Before committing.
>> +	 * save the existing config, so that once upfront link training is
>> +	 * done, the original value of 'pll->config' can be restored.
>> +	 */
>> +	tmp_config = pll->config;
>> +	pll->config = intel_dp->upfront_pll_config;
>> +	intel_dp->upfront_pll_config = tmp_config;
>> +}
>> +
>> +static struct intel_shared_dpll *intel_ddi_select_upfront_pll_config(
>> +			struct intel_dp *intel_dp, struct intel_crtc *crtc)
>> +{
>> +	if (!intel_ddi_pll_select(crtc, crtc->config))
>> +		return NULL;
>> +
>> +	return intel_crtc_to_shared_dpll(crtc);
>> +}
>> +
>> +static void intel_ddi_clear_upfront_pll_config(struct intel_dp *intel_dp,
>> +					struct intel_shared_dpll *pll)
>> +{
>> +	pll->config = intel_dp->upfront_pll_config;
>> +}
>> +
>
>The shared pll interface is really getting in the way here. And BXT plls aren't
>even shared. We are jumping through hoops to get the pll that matches the given
>encoder and to call the code that calculates the dpll_hw_state based on the DP
>link rate. I'd like to get that interface fixed but I reckon it will be tricky
>to find something that works for all the platforms we support. That's the next
>thing on my todo list.
>
>If we have to live with some intermediary solution, I think it would be better
>to (almost) completely bypass the shared dpll stuff. First we would need to
>split bxt_ddi_pll_sel() so that we would have a function that takes the link
>bandwidth and spits out a dpll_hw_state without looking at crtc_state. Then we
>would just take the right pll based on dig_port->port, make sure it is not being
>used and program it with the hw state we get from that new function.
>
>I wrote a patch to do that split. With it, the PLL enable logic in the upfront
>link train logic would look a bit like below. There is some potential for
>problems with the state checker, but it should be fine as long as the old dpll

This is similar to what we had in the earlier version except the
ddi_pll_select split.. I moved it this way because we wanted all the
dpll functionality within the dpll interfaces. But yes, as you said,
we could not get the whole thing cleaner.

>hw state is saved and restored when everything is over.
>
>
>/* FIXME: this only works for BXT */
>dpll_id = (enum intel_dpll_id) dig_port->port;
>pll = dev_priv->shared_dplls[dpll_id];
>
>if (WARN_ON(pll->config.crtc_mask) || WARN_ON(pll->active))
>	goto exit;
>
>bxt_ddi_dp_pll_dividers(crtc->config->port_clock, &clk_div);
>if (!bxt_ddi_set_dpll_hw_state(clock, &clk_div,
>			       &pll->config.hw_state)) {
>	DRM_ERROR("Could not setup DPLL\n");
>	goto exit;
>}
>
>
>/* Enable PLL followed by port */
>pll->enable(dev_priv, pll);
>encoder->pre_enable(encoder);
>
>
>This solution would remove the need for patches 1 and 3.
>
>What do you think?

This (and your other patch) looks ok to me. I will test and confirm once.

>
>> +int intel_ddi_upfront_link_train(struct intel_dp *intel_dp,
>> +				struct intel_crtc *crtc)
>> +{
>> +	struct intel_connector *connector = intel_dp->attached_connector;
>> +	struct intel_encoder *encoder = connector->encoder;
>> +	struct intel_shared_dpll *pll = NULL;
>> +	struct drm_crtc *drm_crtc = NULL;
>> +	struct intel_crtc_state *tmp_crtc_config;
>> +	uint8_t link_bw, lane_count;
>> +
>> +	if (!crtc) {
>> +		drm_crtc = intel_get_unused_crtc(&encoder->base);
>> +		if (!drm_crtc) {
>> +			DRM_ERROR("No crtc for upfront link training\n");
>> +			return -EINVAL;
>> +		}
>> +		encoder->base.crtc = drm_crtc;
>> +		crtc = to_intel_crtc(drm_crtc);
>> +	}
>> +
>> +	/* Initialize with Max Link rate & lane count supported by panel */
>> +	link_bw =  intel_dp->dpcd[DP_MAX_LINK_RATE];
>> +	lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
>> +
>> +	/* Save the crtc->config */
>> +	tmp_crtc_config = crtc->config;
>> +
>> +	DRM_DEBUG_KMS("Using pipe %c\n", pipe_name(crtc->pipe));
>> +	do {
>> +		crtc->config->port_clock =
>> drm_dp_bw_code_to_link_rate(link_bw);
>> +		crtc->config->lane_count = lane_count;
>> +
>> +		pll = intel_ddi_select_upfront_pll_config(intel_dp, crtc);
>> +		if (!pll) {
>> +			DRM_ERROR("Could not get shared DPLL\n");
>> +			goto exit;
>> +		}
>> +
>> +		intel_ddi_commit_upfront_pll_config(intel_dp, pll);
>> +
>> +		/* Enable PLL followed by port */
>> +		intel_enable_shared_dpll(crtc);
>> +		encoder->pre_enable(encoder);
>> +
>> +		/* Check if link training passed; if so update DPCD */
>> +		if (intel_dp->train_set_valid)
>> +			intel_dp_update_dpcd_params(intel_dp);
>> +
>> +		/* Disable port followed by PLL for next retry/clean up */
>> +		encoder->post_disable(encoder);
>> +		intel_disable_shared_dpll(crtc);
>> +
>> +		intel_ddi_clear_upfront_pll_config(intel_dp, pll);
>> +
>> +	} while (!intel_dp->train_set_valid &&
>> +		intel_dp_get_link_retry_params(intel_dp, &lane_count,
>> &link_bw));
>> +exit:
>> +	/* Reset local associations made */
>> +	if (drm_crtc)
>> +		encoder->base.crtc = NULL;
>> +
>> +	/* Restore crtc config */
>> +	crtc->config = tmp_crtc_config;
>> +
>> +	DRM_DEBUG_KMS("Upfront link train %s: lanes:%d bw:%d\n",
>> +	intel_dp->train_set_valid ? "Passed" : "Failed", lane_count,
>> link_bw);
>> +
>> +	return intel_dp->train_set_valid ? 0 : -1;
>> +}
>> +
>>  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_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index af50e61..6a650aa 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4238,16 +4238,45 @@ static void lpt_pch_enable(struct drm_crtc *crtc)
>>  	lpt_enable_pch_transcoder(dev_priv, cpu_transcoder);
>>  }
>>
>> +static
>> +void intel_get_new_shared_dpll_config(struct drm_i915_private *dev_priv,
>> +				  struct intel_shared_dpll_config
>> *shared_dpll)
>> +{
>> +	enum intel_dpll_id i;
>> +
>> +	/* Create a new shared dpll config by duplicating pll->config */
>> +	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>> +		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
>> +		shared_dpll[i] = pll->config;
>> +		shared_dpll[i].crtc_mask = 0;
>> +	}
>> +}
>> +
>>  struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>>  						struct intel_crtc_state
>> *crtc_state)
>>  {
>>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>>  	struct intel_shared_dpll *pll;
>>  	struct intel_shared_dpll_config *shared_dpll;
>> +	struct intel_shared_dpll_config tmp_config[I915_NUM_PLLS];
>>  	enum intel_dpll_id i;
>>  	int max = dev_priv->num_shared_dpll;
>>
>> -	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state
>> ->base.state);
>> +	/*
>> +	 * intel_get_shared_dpll needs a shared_dpll[] to store the computed
>> +	 * dpll_config values. For atomic path, it is stored in
>> +	 * intel_atomic_state->shared_dpll[], which is later committed to
>> +	 * dev_priv->shared_dpll[] in atomic commit. For non-atomic path,
>> +	 * (where intel_atomic_state does not exist) the dpll_config values
>> +	 * are stored in 'tmp_config[]' and copied to encoder structures
>> +	 * for later use.
>> +	 */
>> +	if (crtc_state->base.state) {
>> +		shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state
>> ->base.state);
>> +	} else {
>> +		intel_get_new_shared_dpll_config(dev_priv, tmp_config);
>> +		shared_dpll = tmp_config;
>> +	}
>>
>>  	if (HAS_PCH_IBX(dev_priv->dev)) {
>>  		/* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
>> @@ -4277,6 +4306,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct
>> intel_crtc *crtc,
>>  		pll = &dev_priv->shared_dplls[i];
>>  		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>>  			crtc->base.base.id, pll->name);
>> +
>>  		WARN_ON(shared_dpll[i].crtc_mask);
>>
>>  		goto found;
>> @@ -4325,6 +4355,12 @@ found:
>>
>>  	shared_dpll[i].crtc_mask |= 1 << crtc->pipe;
>>
>> +	/*
>> +	 * For DP, this shared dpll config is saved to intel_dp,
>> +	 * and used by upfront link train logic subsequently.
>> +	 */
>> +	intel_dp_update_shared_dpll_config(crtc_state, shared_dpll[i]);
>> +
>>  	return pll;
>>  }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index e2bea710..cc7b6f3 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1679,6 +1679,55 @@ void intel_dp_set_link_params(struct intel_dp
>> *intel_dp,
>>  	intel_dp->lane_count = pipe_config->lane_count;
>>  }
>>
>> +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp)
>> +{
>> +	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;
>> +
>> +	intel_dp->dpcd[DP_MAX_LINK_RATE] =
>> +			drm_dp_link_rate_to_bw_code(intel_dp->link_rate);
>
>Maybe it would be good to have an explicit field for actual max lane count and
>link rate. That way, reading the DPCD from debugfs will give the actual results
>read from the sink instead of an edited value.

Looks like the 'dpcd_show' interface reads the DPCD everytime and prints
It .. And does not seem to look up at intel_dp->dpcd[].
Or, are we talking about two different interfaces ?

>
>> +}
>> +
>> +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
>> +			uint8_t *lane_count, uint8_t *link_bw)
>> +{
>> +	/*
>> +	 * As per DP1.3 Spec, retry all link rates for a particular
>> +	 * lane count value, before reducing number of lanes.
>> +	 */
>> +	if (*link_bw == DP_LINK_BW_5_4) {
>> +		*link_bw = DP_LINK_BW_2_7;
>> +	} else if (*link_bw == DP_LINK_BW_2_7) {
>> +		*link_bw = DP_LINK_BW_1_62;
>> +	} else if (*lane_count == 4) {
>> +		*lane_count = 2;
>> +		*link_bw = intel_dp_max_link_bw(intel_dp);
>> +	} else if (*lane_count == 2) {
>> +		*lane_count = 1;
>> +		*link_bw = intel_dp_max_link_bw(intel_dp);
>> +	} else {
>> +		/* Tried all combinations, so exit */
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +void intel_dp_update_shared_dpll_config(struct intel_crtc_state *pipe_config,
>> +				struct intel_shared_dpll_config config)
>> +{
>> +	struct intel_encoder *encoder;
>> +	struct intel_dp *intel_dp;
>> +
>> +	encoder = intel_ddi_get_crtc_new_encoder(pipe_config);
>> +	if (!encoder || encoder->type != INTEL_OUTPUT_DISPLAYPORT)
>> +		return;
>> +
>> +	intel_dp = enc_to_intel_dp(&encoder->base);
>> +	intel_dp->upfront_pll_config = config;
>> +}
>> +
>>  static void intel_dp_prepare(struct intel_encoder *encoder)
>>  {
>>  	struct drm_device *dev = encoder->base.dev;
>> @@ -4601,6 +4650,66 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>>  	intel_dp->has_audio = false;
>>  }
>>
>> +static bool intel_dp_upfront_link_train(struct drm_connector *connector)
>> +{
>> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
>> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>> +	struct drm_device *dev = intel_encoder->base.dev;
>> +	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
>> +	struct drm_mode_config *config = &dev->mode_config;
>> +	struct intel_crtc *intel_crtc = crtc ? to_intel_crtc(crtc) : NULL;
>> +	struct drm_modeset_acquire_ctx ctx, *old_ctx = NULL;
>> +	bool need_dpms_on = false;
>> +	int ret;
>> +
>> +	if (!IS_BROXTON(dev))
>> +		return true;
>> +
>> +	/*
>> +	 * 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 and then re-enable it back.
>> +	 */
>> +	if (crtc && crtc->enabled && intel_crtc->active) {
>> +		old_ctx = crtc->acquire_ctx;
>> +		drm_modeset_acquire_init(&ctx, 0);
>> +retry:
>> +		ret = drm_modeset_lock(&config->connection_mutex, &ctx);
>> +		if (ret == -EDEADLK) {
>> +			drm_modeset_backoff(&ctx);
>> +			goto retry;
>> +		}
>
>We need to acquire those locks even if the crtc is disabled, otherwise a modeset
>could sneak past and wreak havoc while we are doing the link training and
>fiddling with the plls.
>
>> +
>> +		crtc->acquire_ctx = &ctx;
>> +		ret = drm_atomic_helper_connector_dpms(connector,
>> DRM_MODE_DPMS_OFF);
>
>The crtc->acquire_ctx override seems wrong. One thing that slipped by me when I
>looked at the previous version of this patch is that we need to acquire some
>locks even if the crtc is already off. So it is probably better to write a

Ok, I think this is what Ville is also mentioning. I did not notice it either ;-(
I am still learning the scheme of ww_* and ctx based locking..
So, yes, will re-work this.

>separate function that takes an acquire context and uses atomic to set
>crtc_state->active to false and calls drm_atomic_commit().
>
>> +		if (ret) {
>> +			DRM_ERROR("DPMS off failed:%d\n", ret);
>> +			goto exit_unlock;
>> +		}
>> +
>> +		need_dpms_on = true;
>> +	}
>> +
>> +	if (HAS_DDI(dev))
>> +		ret = intel_ddi_upfront_link_train(intel_dp, intel_crtc);
>> +		/* Other platforms upfront link train call goes here..*/
>> +
>> +	if (!need_dpms_on)
>> +		return ret;
>> +
>> +	ret = drm_atomic_helper_connector_dpms(connector, DRM_MODE_DPMS_ON);
>> +	if (ret)
>> +		DRM_ERROR("DPMS on failed:%d\n", ret);
>> +
>> +exit_unlock:
>> +	drm_modeset_drop_locks(&ctx);
>> +	drm_modeset_acquire_fini(&ctx);
>> +	crtc->acquire_ctx = old_ctx;
>> +	return ret;
>
>The return type of this function is bool. The conversion here will do the wrong
>thing, since ret potentially has the return value of functions that return non
>-zero on failure.

I wanted to make this as an 'int'. I missed this. Will fix it in next version.

>
>> +}
>> +
>>  static enum drm_connector_status
>>  intel_dp_detect(struct drm_connector *connector, bool force)
>>  {
>> @@ -4610,7 +4719,8 @@ 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 do_upfront_link_train;
>> +	int ret;
>>  	u8 sink_irq_vector;
>>
>>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>> @@ -4684,6 +4794,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(connector);
>> +		if (ret)
>> +			status = connector_status_disconnected;
>> +	}
>
>So if upfront link training fails because there is no unused crtc we'll say that
>the connector is disconnected?

I was not sure on what should be the stand we should take here..

Should we distinguish various causes of upfront failure and then
report status accordingly ? Not just crtc but pll selection, link training
failures, etc..

Please let me know what you think.

Thanks,
Durga

>
>
>Ander
>
>>  out:
>>  	intel_display_power_put(to_i915(dev), power_domain);
>>  	return status;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 9fe7c4b..c5ca4ab 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -812,6 +812,9 @@ struct intel_dp {
>>  	unsigned long compliance_test_type;
>>  	unsigned long compliance_test_data;
>>  	bool compliance_test_active;
>> +
>> +	/* Used to store pll config for upfront link training */
>> +	struct intel_shared_dpll_config upfront_pll_config;
>>  };
>>
>>  struct intel_digital_port {
>> @@ -1031,6 +1034,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);
>> +int intel_ddi_upfront_link_train(struct intel_dp *intel_dp,
>> +				struct intel_crtc *crtc);
>>
>>  /* intel_frontbuffer.c */
>>  void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
>> @@ -1239,6 +1244,9 @@ bool intel_dp_init_connector(struct intel_digital_port
>> *intel_dig_port,
>>  			     struct intel_connector *intel_connector);
>>  void intel_dp_set_link_params(struct intel_dp *intel_dp,
>>  			      const struct intel_crtc_state *pipe_config);
>> +void intel_dp_update_dpcd_params(struct intel_dp *intel_dp);
>> +bool intel_dp_get_link_retry_params(struct intel_dp *intel_dp,
>> +				uint8_t *lane_count, uint8_t *link_bw);
>>  void intel_dp_start_link_train(struct intel_dp *intel_dp);
>>  void intel_dp_stop_link_train(struct intel_dp *intel_dp);
>>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
>> @@ -1246,6 +1254,8 @@ void intel_dp_encoder_destroy(struct drm_encoder
>> *encoder);
>>  int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
>>  bool intel_dp_compute_config(struct intel_encoder *encoder,
>>  			     struct intel_crtc_state *pipe_config);
>> +void intel_dp_update_shared_dpll_config(struct intel_crtc_state *pipe_config,
>> +				struct intel_shared_dpll_config config);
>>  bool intel_dp_is_edp(struct drm_device *dev, enum port port);
>>  enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
>>  				  bool long_hpd);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCHv2 4/4] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[Ville Syrjälä]

On Mon, Feb 01, 2016 at 07:27:53PM +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.
> * 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, do upfront link training and then re-enable it back.
> * All local changes made for upfront link training are reset
>   to their previous values once it is done; so that the
>   subsequent modeset is not aware of these changes.

Glancing over this stuff, it doesn't look all that good on the locking
front. Mainly there doesn't seem to locking.

In general I'm not really convinced upfront link training is a very good
idea if it needs a pipe. What happens if we fail to find a free pipe?

I'd be much happier about this if we could make do without a pipe at
least on some modern platforms and actually limit the feature to
those platforms. PLLs are another concern, but I think modern platforms
often have some kind of way to always get the standard DP frequencies
from eg. the LCPLL.

If we do go with the "hope you find a free pipe" approach, then it
really needs to do that atomic locking stuff right. Otherwise I'd expect
fireworks when someone plugs in a display while there's modeset
happening.

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

Re: [PATCHv2 4/4] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[R, Durgadoss]

Hi Ville,

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, February 12, 2016 10:43 PM
>To: R, Durgadoss <durgadoss.r@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander
><ander.conselvan.de.oliveira@intel.com>
>Subject: Re: [Intel-gfx] [PATCHv2 4/4] drm/i915/dp: Enable Upfront link training for typeC DP support on
>BXT
>
>On Mon, Feb 01, 2016 at 07:27:53PM +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.
>> * 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, do upfront link training and then re-enable it back.
>> * All local changes made for upfront link training are reset
>>   to their previous values once it is done; so that the
>>   subsequent modeset is not aware of these changes.
>
>Glancing over this stuff, it doesn't look all that good on the locking
>front. Mainly there doesn't seem to locking.
>
>In general I'm not really convinced upfront link training is a very good
>idea if it needs a pipe. What happens if we fail to find a free pipe?
>
>I'd be much happier about this if we could make do without a pipe at

We actually don't enable/disable the pipe. We need the crtc structures
because values like port clock/link bw/dpll hw state are stored in
crtc->config structures. This is why we had to touch crtc related
data structures.

Ander has pulled the dpll_hw_states out of crtc structures and made them
independent. With this, things should improve.. I need to try this
implementation to see if we can/can't completely get away
with modifying crtc structures.

>least on some modern platforms and actually limit the feature to
>those platforms. PLLs are another concern, but I think modern platforms
>often have some kind of way to always get the standard DP frequencies
>from eg. the LCPLL.
>
>If we do go with the "hope you find a free pipe" approach, then it
>really needs to do that atomic locking stuff right. Otherwise I'd expect

Yes, we missed it. Ander pointed out the place where we need this
locking. So, we will re-work this.

Thanks,
Durga

>fireworks when someone plugs in a display while there's modeset
>happening.
>
>--
>Ville Syrjälä
>Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCHv2 4/4] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT

[Ville Syrjälä]

On Mon, Feb 15, 2016 at 06:55:07AM +0000, R, Durgadoss wrote:
> Hi Ville,
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Friday, February 12, 2016 10:43 PM
> >To: R, Durgadoss <durgadoss.r@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander
> ><ander.conselvan.de.oliveira@intel.com>
> >Subject: Re: [Intel-gfx] [PATCHv2 4/4] drm/i915/dp: Enable Upfront link training for typeC DP support on
> >BXT
> >
> >On Mon, Feb 01, 2016 at 07:27:53PM +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.
> >> * 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, do upfront link training and then re-enable it back.
> >> * All local changes made for upfront link training are reset
> >>   to their previous values once it is done; so that the
> >>   subsequent modeset is not aware of these changes.
> >
> >Glancing over this stuff, it doesn't look all that good on the locking
> >front. Mainly there doesn't seem to locking.
> >
> >In general I'm not really convinced upfront link training is a very good
> >idea if it needs a pipe. What happens if we fail to find a free pipe?
> >
> >I'd be much happier about this if we could make do without a pipe at
> 
> We actually don't enable/disable the pipe. We need the crtc structures
> because values like port clock/link bw/dpll hw state are stored in
> crtc->config structures. This is why we had to touch crtc related
> data structures.
> 
> Ander has pulled the dpll_hw_states out of crtc structures and made them
> independent. With this, things should improve.. I need to try this
> implementation to see if we can/can't completely get away
> with modifying crtc structures.

One option might involve adding a fake crtc if we can't easily detangle it
from the code. But obviously if we can remove the crtc dependency things
ought to be much cleaner.

I already had the thought that we might want to add fake crtcs for
dealing with the MST main link, and just use atomic to modeset both
the main link with its fake crtc and real encoder alongside any streams
with their real crtcs and fake encoders.

> 
> >least on some modern platforms and actually limit the feature to
> >those platforms. PLLs are another concern, but I think modern platforms
> >often have some kind of way to always get the standard DP frequencies
> >from eg. the LCPLL.
> >
> >If we do go with the "hope you find a free pipe" approach, then it
> >really needs to do that atomic locking stuff right. Otherwise I'd expect
> 
> Yes, we missed it. Ander pointed out the place where we need this
> locking. So, we will re-work this.

One concern is how whether it might block other threads from eg. page
flipping. I suppose if we have independent crtcs it should be fine since
page flip won't require the connetion_mutex.

> 
> Thanks,
> Durga
> 
> >fireworks when someone plugs in a display while there's modeset
> >happening.
> >
> >--
> >Ville Syrjälä
> >Intel OTC

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