* [PATCH] drm/i915: hsw: fix link training for DP/eDP on port-A
@ 2013-04-26 17:52 Imre Deak
2013-04-29 15:25 ` [PATCH v2] drm/i915: hsw: fix link training for eDP " Imre Deak
0 siblings, 1 reply; 5+ messages in thread
From: Imre Deak @ 2013-04-26 17:52 UTC (permalink / raw)
To: intel-gfx; +Cc: paulo.r.zanoni
According to BSpec the link training sequence on HSW port-A should be as
follows:
1. link training: clock recovery
2. link training: equalization
3. display pipe enable
4. link training: disable (set normal mode)
Contrary to this at the moment we do step 4. already after step 2. Fix
this by adding a new intel_dp_stop_link_training function, so that we'll
end up with the following functions corresponding to the above steps:
intel_dp_start_link_train -> step 1.
intel_dp_complete_link_train -> step 2.
intel_dp_stop_link_train -> step 4.
For port-A we'll call intel_dp_stop_link_train only after enabling the
pipe for everything else we'll call it right after
intel_dp_complete_link_train to preserve the current behavior.
Tested on HSW/HSW-ULT.
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/intel_ddi.c | 5 ++++
drivers/gpu/drm/i915/intel_dp.c | 49 ++++++++++++++++++++++++++------------
drivers/gpu/drm/i915/intel_drv.h | 1 +
3 files changed, 40 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index eef450b..d7da1ca 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1265,6 +1265,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
intel_dp_start_link_train(intel_dp);
intel_dp_complete_link_train(intel_dp);
+ if (port != PORT_A)
+ intel_dp_stop_link_train(intel_dp);
}
}
@@ -1326,6 +1328,9 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
} else if (type == INTEL_OUTPUT_EDP) {
struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+ if (port == PORT_A)
+ intel_dp_stop_link_train(intel_dp);
+
ironlake_edp_backlight_on(intel_dp);
}
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index dd4100f..57f4585 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1389,6 +1389,7 @@ static void intel_enable_dp(struct intel_encoder *encoder)
ironlake_edp_panel_on(intel_dp);
ironlake_edp_panel_vdd_off(intel_dp, true);
intel_dp_complete_link_train(intel_dp);
+ intel_dp_stop_link_train(intel_dp);
ironlake_edp_backlight_on(intel_dp);
if (IS_VALLEYVIEW(dev)) {
@@ -1887,10 +1888,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
struct drm_i915_private *dev_priv = dev->dev_private;
enum port port = intel_dig_port->port;
int ret;
- uint32_t temp;
if (HAS_DDI(dev)) {
- temp = I915_READ(DP_TP_CTL(port));
+ uint32_t temp = I915_READ(DP_TP_CTL(port));
if (dp_train_pat & DP_LINK_SCRAMBLING_DISABLE)
temp |= DP_TP_CTL_SCRAMBLE_DISABLE;
@@ -1900,18 +1900,6 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
switch (dp_train_pat & DP_TRAINING_PATTERN_MASK) {
case DP_TRAINING_PATTERN_DISABLE:
-
- if (port != PORT_A) {
- temp |= DP_TP_CTL_LINK_TRAIN_IDLE;
- I915_WRITE(DP_TP_CTL(port), temp);
-
- if (wait_for((I915_READ(DP_TP_STATUS(port)) &
- DP_TP_STATUS_IDLE_DONE), 1))
- DRM_ERROR("Timed out waiting for DP idle patterns\n");
-
- temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
- }
-
temp |= DP_TP_CTL_LINK_TRAIN_NORMAL;
break;
@@ -1987,6 +1975,27 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
return true;
}
+static void intel_dp_idle_link_train(struct intel_dp *intel_dp)
+{
+ struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+ struct drm_device *dev = intel_dig_port->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ enum port port = intel_dig_port->port;
+ uint32_t l;
+
+ if (!HAS_DDI(dev) || port == PORT_A)
+ return;
+
+ l = I915_READ(DP_TP_CTL(port));
+ l &= ~DP_TP_CTL_LINK_TRAIN_MASK;
+ l |= DP_TP_CTL_LINK_TRAIN_IDLE;
+ I915_WRITE(DP_TP_CTL(port), l);
+
+ if (wait_for((I915_READ(DP_TP_STATUS(port)) & DP_TP_STATUS_IDLE_DONE),
+ 1))
+ DRM_ERROR("Timed out waiting for DP idle patterns\n");
+}
+
/* Enable corresponding port and start training pattern 1 */
void
intel_dp_start_link_train(struct intel_dp *intel_dp)
@@ -2129,10 +2138,19 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
++tries;
}
+ intel_dp_idle_link_train(intel_dp);
+
+ intel_dp->DP = DP;
+
if (channel_eq)
DRM_DEBUG_KMS("Channel EQ done. DP Training successfull\n");
- intel_dp_set_link_train(intel_dp, DP, DP_TRAINING_PATTERN_DISABLE);
+}
+
+void intel_dp_stop_link_train(struct intel_dp *intel_dp)
+{
+ intel_dp_set_link_train(intel_dp, intel_dp->DP,
+ DP_TRAINING_PATTERN_DISABLE);
}
static void
@@ -2340,6 +2358,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
drm_get_encoder_name(&intel_encoder->base));
intel_dp_start_link_train(intel_dp);
intel_dp_complete_link_train(intel_dp);
+ intel_dp_stop_link_train(intel_dp);
}
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 66922f8..684a90e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -525,6 +525,7 @@ extern void intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
extern void intel_dp_init_link_config(struct intel_dp *intel_dp);
extern void intel_dp_start_link_train(struct intel_dp *intel_dp);
extern void intel_dp_complete_link_train(struct intel_dp *intel_dp);
+extern void intel_dp_stop_link_train(struct intel_dp *intel_dp);
extern void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
extern void intel_dp_encoder_destroy(struct drm_encoder *encoder);
extern void intel_dp_check_link_status(struct intel_dp *intel_dp);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2] drm/i915: hsw: fix link training for eDP on port-A
2013-04-26 17:52 [PATCH] drm/i915: hsw: fix link training for DP/eDP on port-A Imre Deak
@ 2013-04-29 15:25 ` Imre Deak
2013-05-02 17:45 ` Paulo Zanoni
2013-05-03 9:57 ` [PATCH v3] " Imre Deak
0 siblings, 2 replies; 5+ messages in thread
From: Imre Deak @ 2013-04-29 15:25 UTC (permalink / raw)
To: intel-gfx; +Cc: paulo.r.zanoni
According to BSpec the link training sequence for eDP on HSW port-A
should be as follows:
1. link training: clock recovery
2. link training: equalization
3. link training: set idle transmission mode
4. display pipe enable
5. link training: disable (set normal mode)
Contrary to this at the moment we don't do step 3. and we do step 5.
before step 4. Fix this by setting idle transmission mode for eDP at
the end of intel_dp_complete_link_train and adding a new
intel_dp_stop_link_training function to disable link training. With
these changes we'll end up with the following functions corresponding
to the above steps:
intel_dp_start_link_train -> step 1.
intel_dp_complete_link_train -> step 2., step 3.
intel_dp_stop_link_train -> step 5.
For port-A we'll call intel_dp_stop_link_train only after enabling the
pipe, for everything else we'll call it right after
intel_dp_complete_link_train to preserve the current behavior.
Tested on HSW/HSW-ULT.
In v2:
- Due to a HW issue we must set idle transmission mode for port-A too
before enabling the pipe. Thanks for Arthur Runyan for explaining
this.
- Update the patch subject to make it clear that it's an eDP fix, DP is
not affected.
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/intel_ddi.c | 5 ++++
drivers/gpu/drm/i915/intel_dp.c | 59 ++++++++++++++++++++++++++++----------
drivers/gpu/drm/i915/intel_drv.h | 1 +
3 files changed, 50 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 0d7cf31..f3d0f21 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1265,6 +1265,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
intel_dp_start_link_train(intel_dp);
intel_dp_complete_link_train(intel_dp);
+ if (port != PORT_A)
+ intel_dp_stop_link_train(intel_dp);
}
}
@@ -1326,6 +1328,9 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
} else if (type == INTEL_OUTPUT_EDP) {
struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+ if (port == PORT_A)
+ intel_dp_stop_link_train(intel_dp);
+
ironlake_edp_backlight_on(intel_dp);
}
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 13d2fd6..d4ddeb9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1437,6 +1437,7 @@ static void intel_enable_dp(struct intel_encoder *encoder)
ironlake_edp_panel_on(intel_dp);
ironlake_edp_panel_vdd_off(intel_dp, true);
intel_dp_complete_link_train(intel_dp);
+ intel_dp_stop_link_train(intel_dp);
ironlake_edp_backlight_on(intel_dp);
if (IS_VALLEYVIEW(dev)) {
@@ -1935,10 +1936,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
struct drm_i915_private *dev_priv = dev->dev_private;
enum port port = intel_dig_port->port;
int ret;
- uint32_t temp;
if (HAS_DDI(dev)) {
- temp = I915_READ(DP_TP_CTL(port));
+ uint32_t temp = I915_READ(DP_TP_CTL(port));
if (dp_train_pat & DP_LINK_SCRAMBLING_DISABLE)
temp |= DP_TP_CTL_SCRAMBLE_DISABLE;
@@ -1948,18 +1948,6 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
switch (dp_train_pat & DP_TRAINING_PATTERN_MASK) {
case DP_TRAINING_PATTERN_DISABLE:
-
- if (port != PORT_A) {
- temp |= DP_TP_CTL_LINK_TRAIN_IDLE;
- I915_WRITE(DP_TP_CTL(port), temp);
-
- if (wait_for((I915_READ(DP_TP_STATUS(port)) &
- DP_TP_STATUS_IDLE_DONE), 1))
- DRM_ERROR("Timed out waiting for DP idle patterns\n");
-
- temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
- }
-
temp |= DP_TP_CTL_LINK_TRAIN_NORMAL;
break;
@@ -2035,6 +2023,37 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
return true;
}
+static void intel_dp_idle_link_train(struct intel_dp *intel_dp)
+{
+ struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+ struct drm_device *dev = intel_dig_port->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ enum port port = intel_dig_port->port;
+ uint32_t l;
+
+ if (!HAS_DDI(dev))
+ return;
+
+ l = I915_READ(DP_TP_CTL(port));
+ l &= ~DP_TP_CTL_LINK_TRAIN_MASK;
+ l |= DP_TP_CTL_LINK_TRAIN_IDLE;
+ I915_WRITE(DP_TP_CTL(port), l);
+
+ /*
+ * On PORT_A we can have only eDP in SST mode. There the only reason
+ * we need to set idle transmission mode is to work around a HW issue
+ * where we enable the pipe while not in idle link-training mode.
+ * In this case there is requirement to wait for a minimum number of
+ * idle patterns to be sent.
+ */
+ if (port == PORT_A)
+ return;
+
+ if (wait_for((I915_READ(DP_TP_STATUS(port)) & DP_TP_STATUS_IDLE_DONE),
+ 1))
+ DRM_ERROR("Timed out waiting for DP idle patterns\n");
+}
+
/* Enable corresponding port and start training pattern 1 */
void
intel_dp_start_link_train(struct intel_dp *intel_dp)
@@ -2177,10 +2196,19 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
++tries;
}
+ intel_dp_idle_link_train(intel_dp);
+
+ intel_dp->DP = DP;
+
if (channel_eq)
DRM_DEBUG_KMS("Channel EQ done. DP Training successfull\n");
- intel_dp_set_link_train(intel_dp, DP, DP_TRAINING_PATTERN_DISABLE);
+}
+
+void intel_dp_stop_link_train(struct intel_dp *intel_dp)
+{
+ intel_dp_set_link_train(intel_dp, intel_dp->DP,
+ DP_TRAINING_PATTERN_DISABLE);
}
static void
@@ -2388,6 +2416,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
drm_get_encoder_name(&intel_encoder->base));
intel_dp_start_link_train(intel_dp);
intel_dp_complete_link_train(intel_dp);
+ intel_dp_stop_link_train(intel_dp);
}
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a8c6960..6d1e63a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -542,6 +542,7 @@ extern void intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
extern void intel_dp_init_link_config(struct intel_dp *intel_dp);
extern void intel_dp_start_link_train(struct intel_dp *intel_dp);
extern void intel_dp_complete_link_train(struct intel_dp *intel_dp);
+extern void intel_dp_stop_link_train(struct intel_dp *intel_dp);
extern void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
extern void intel_dp_encoder_destroy(struct drm_encoder *encoder);
extern void intel_dp_check_link_status(struct intel_dp *intel_dp);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/i915: hsw: fix link training for eDP on port-A
2013-04-29 15:25 ` [PATCH v2] drm/i915: hsw: fix link training for eDP " Imre Deak
@ 2013-05-02 17:45 ` Paulo Zanoni
2013-05-02 18:56 ` Imre Deak
2013-05-03 9:57 ` [PATCH v3] " Imre Deak
1 sibling, 1 reply; 5+ messages in thread
From: Paulo Zanoni @ 2013-05-02 17:45 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx, paulo.r.zanoni
Hi
2013/4/29 Imre Deak <imre.deak@intel.com>:
> According to BSpec the link training sequence for eDP on HSW port-A
> should be as follows:
>
> 1. link training: clock recovery
> 2. link training: equalization
> 3. link training: set idle transmission mode
> 4. display pipe enable
> 5. link training: disable (set normal mode)
>
> Contrary to this at the moment we don't do step 3. and we do step 5.
> before step 4. Fix this by setting idle transmission mode for eDP at
> the end of intel_dp_complete_link_train and adding a new
> intel_dp_stop_link_training function to disable link training. With
> these changes we'll end up with the following functions corresponding
> to the above steps:
>
> intel_dp_start_link_train -> step 1.
> intel_dp_complete_link_train -> step 2., step 3.
> intel_dp_stop_link_train -> step 5.
>
> For port-A we'll call intel_dp_stop_link_train only after enabling the
> pipe, for everything else we'll call it right after
> intel_dp_complete_link_train to preserve the current behavior.
>
> Tested on HSW/HSW-ULT.
>
> In v2:
> - Due to a HW issue we must set idle transmission mode for port-A too
> before enabling the pipe. Thanks for Arthur Runyan for explaining
> this.
> - Update the patch subject to make it clear that it's an eDP fix, DP is
> not affected.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 5 ++++
> drivers/gpu/drm/i915/intel_dp.c | 59 ++++++++++++++++++++++++++++----------
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> 3 files changed, 50 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 0d7cf31..f3d0f21 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1265,6 +1265,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> intel_dp_start_link_train(intel_dp);
> intel_dp_complete_link_train(intel_dp);
> + if (port != PORT_A)
> + intel_dp_stop_link_train(intel_dp);
> }
> }
>
> @@ -1326,6 +1328,9 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
> } else if (type == INTEL_OUTPUT_EDP) {
> struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>
> + if (port == PORT_A)
> + intel_dp_stop_link_train(intel_dp);
> +
> ironlake_edp_backlight_on(intel_dp);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 13d2fd6..d4ddeb9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1437,6 +1437,7 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> ironlake_edp_panel_on(intel_dp);
> ironlake_edp_panel_vdd_off(intel_dp, true);
> intel_dp_complete_link_train(intel_dp);
> + intel_dp_stop_link_train(intel_dp);
> ironlake_edp_backlight_on(intel_dp);
>
> if (IS_VALLEYVIEW(dev)) {
> @@ -1935,10 +1936,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
> struct drm_i915_private *dev_priv = dev->dev_private;
> enum port port = intel_dig_port->port;
> int ret;
> - uint32_t temp;
>
> if (HAS_DDI(dev)) {
> - temp = I915_READ(DP_TP_CTL(port));
> + uint32_t temp = I915_READ(DP_TP_CTL(port));
>
> if (dp_train_pat & DP_LINK_SCRAMBLING_DISABLE)
> temp |= DP_TP_CTL_SCRAMBLE_DISABLE;
> @@ -1948,18 +1948,6 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
> temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
> switch (dp_train_pat & DP_TRAINING_PATTERN_MASK) {
> case DP_TRAINING_PATTERN_DISABLE:
> -
> - if (port != PORT_A) {
> - temp |= DP_TP_CTL_LINK_TRAIN_IDLE;
> - I915_WRITE(DP_TP_CTL(port), temp);
> -
> - if (wait_for((I915_READ(DP_TP_STATUS(port)) &
> - DP_TP_STATUS_IDLE_DONE), 1))
> - DRM_ERROR("Timed out waiting for DP idle patterns\n");
> -
> - temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
> - }
> -
> temp |= DP_TP_CTL_LINK_TRAIN_NORMAL;
>
> break;
> @@ -2035,6 +2023,37 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
> return true;
> }
>
> +static void intel_dp_idle_link_train(struct intel_dp *intel_dp)
Maybe intel_dp_set_idle_link_train?
> +{
> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> + struct drm_device *dev = intel_dig_port->base.base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + enum port port = intel_dig_port->port;
> + uint32_t l;
We usually use "val", "tmp" or "temp". L is unusual.
> +
> + if (!HAS_DDI(dev))
> + return;
> +
> + l = I915_READ(DP_TP_CTL(port));
> + l &= ~DP_TP_CTL_LINK_TRAIN_MASK;
> + l |= DP_TP_CTL_LINK_TRAIN_IDLE;
> + I915_WRITE(DP_TP_CTL(port), l);
> +
> + /*
> + * On PORT_A we can have only eDP in SST mode. There the only reason
> + * we need to set idle transmission mode is to work around a HW issue
> + * where we enable the pipe while not in idle link-training mode.
> + * In this case there is requirement to wait for a minimum number of
> + * idle patterns to be sent.
> + */
> + if (port == PORT_A)
> + return;
> +
> + if (wait_for((I915_READ(DP_TP_STATUS(port)) & DP_TP_STATUS_IDLE_DONE),
> + 1))
> + DRM_ERROR("Timed out waiting for DP idle patterns\n");
> +}
> +
> /* Enable corresponding port and start training pattern 1 */
> void
> intel_dp_start_link_train(struct intel_dp *intel_dp)
> @@ -2177,10 +2196,19 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
> ++tries;
> }
>
> + intel_dp_idle_link_train(intel_dp);
> +
> + intel_dp->DP = DP;
I know this is not your fault, but I kinda _really_ don't like the
fact that we have 3 places to store the contents of the same register:
(i) the register itself, (ii) intel_dp->DP and (iii) these temporary
DP variables. Whoever writes a patch to improve that without adding
any regressions will get a free $favorite_beverage. I guess the first
step is to try to remove the temporary variables only.
So I've also booted your patch and both DP and eDP still work for me.
With or without the changes I proposed above:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> +
> if (channel_eq)
> DRM_DEBUG_KMS("Channel EQ done. DP Training successfull\n");
>
> - intel_dp_set_link_train(intel_dp, DP, DP_TRAINING_PATTERN_DISABLE);
> +}
> +
> +void intel_dp_stop_link_train(struct intel_dp *intel_dp)
> +{
> + intel_dp_set_link_train(intel_dp, intel_dp->DP,
> + DP_TRAINING_PATTERN_DISABLE);
> }
>
> static void
> @@ -2388,6 +2416,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
> drm_get_encoder_name(&intel_encoder->base));
> intel_dp_start_link_train(intel_dp);
> intel_dp_complete_link_train(intel_dp);
> + intel_dp_stop_link_train(intel_dp);
> }
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a8c6960..6d1e63a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -542,6 +542,7 @@ extern void intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> extern void intel_dp_init_link_config(struct intel_dp *intel_dp);
> extern void intel_dp_start_link_train(struct intel_dp *intel_dp);
> extern void intel_dp_complete_link_train(struct intel_dp *intel_dp);
> +extern void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> extern void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> extern void intel_dp_encoder_destroy(struct drm_encoder *encoder);
> extern void intel_dp_check_link_status(struct intel_dp *intel_dp);
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/i915: hsw: fix link training for eDP on port-A
2013-05-02 17:45 ` Paulo Zanoni
@ 2013-05-02 18:56 ` Imre Deak
0 siblings, 0 replies; 5+ messages in thread
From: Imre Deak @ 2013-05-02 18:56 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, paulo.r.zanoni
On Thu, 2013-05-02 at 14:45 -0300, Paulo Zanoni wrote:
> Hi
>
> 2013/4/29 Imre Deak <imre.deak@intel.com>:
> > According to BSpec the link training sequence for eDP on HSW port-A
> > should be as follows:
> >
> > 1. link training: clock recovery
> > 2. link training: equalization
> > 3. link training: set idle transmission mode
> > 4. display pipe enable
> > 5. link training: disable (set normal mode)
> >
> > Contrary to this at the moment we don't do step 3. and we do step 5.
> > before step 4. Fix this by setting idle transmission mode for eDP at
> > the end of intel_dp_complete_link_train and adding a new
> > intel_dp_stop_link_training function to disable link training. With
> > these changes we'll end up with the following functions corresponding
> > to the above steps:
> >
> > intel_dp_start_link_train -> step 1.
> > intel_dp_complete_link_train -> step 2., step 3.
> > intel_dp_stop_link_train -> step 5.
> >
> > For port-A we'll call intel_dp_stop_link_train only after enabling the
> > pipe, for everything else we'll call it right after
> > intel_dp_complete_link_train to preserve the current behavior.
> >
> > Tested on HSW/HSW-ULT.
> >
> > In v2:
> > - Due to a HW issue we must set idle transmission mode for port-A too
> > before enabling the pipe. Thanks for Arthur Runyan for explaining
> > this.
> > - Update the patch subject to make it clear that it's an eDP fix, DP is
> > not affected.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_ddi.c | 5 ++++
> > drivers/gpu/drm/i915/intel_dp.c | 59 ++++++++++++++++++++++++++++----------
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > 3 files changed, 50 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 0d7cf31..f3d0f21 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1265,6 +1265,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > intel_dp_start_link_train(intel_dp);
> > intel_dp_complete_link_train(intel_dp);
> > + if (port != PORT_A)
> > + intel_dp_stop_link_train(intel_dp);
> > }
> > }
> >
> > @@ -1326,6 +1328,9 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
> > } else if (type == INTEL_OUTPUT_EDP) {
> > struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >
> > + if (port == PORT_A)
> > + intel_dp_stop_link_train(intel_dp);
> > +
> > ironlake_edp_backlight_on(intel_dp);
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 13d2fd6..d4ddeb9 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1437,6 +1437,7 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> > ironlake_edp_panel_on(intel_dp);
> > ironlake_edp_panel_vdd_off(intel_dp, true);
> > intel_dp_complete_link_train(intel_dp);
> > + intel_dp_stop_link_train(intel_dp);
> > ironlake_edp_backlight_on(intel_dp);
> >
> > if (IS_VALLEYVIEW(dev)) {
> > @@ -1935,10 +1936,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > enum port port = intel_dig_port->port;
> > int ret;
> > - uint32_t temp;
> >
> > if (HAS_DDI(dev)) {
> > - temp = I915_READ(DP_TP_CTL(port));
> > + uint32_t temp = I915_READ(DP_TP_CTL(port));
> >
> > if (dp_train_pat & DP_LINK_SCRAMBLING_DISABLE)
> > temp |= DP_TP_CTL_SCRAMBLE_DISABLE;
> > @@ -1948,18 +1948,6 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
> > temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
> > switch (dp_train_pat & DP_TRAINING_PATTERN_MASK) {
> > case DP_TRAINING_PATTERN_DISABLE:
> > -
> > - if (port != PORT_A) {
> > - temp |= DP_TP_CTL_LINK_TRAIN_IDLE;
> > - I915_WRITE(DP_TP_CTL(port), temp);
> > -
> > - if (wait_for((I915_READ(DP_TP_STATUS(port)) &
> > - DP_TP_STATUS_IDLE_DONE), 1))
> > - DRM_ERROR("Timed out waiting for DP idle patterns\n");
> > -
> > - temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
> > - }
> > -
> > temp |= DP_TP_CTL_LINK_TRAIN_NORMAL;
> >
> > break;
> > @@ -2035,6 +2023,37 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
> > return true;
> > }
> >
> > +static void intel_dp_idle_link_train(struct intel_dp *intel_dp)
>
> Maybe intel_dp_set_idle_link_train?
Yea, that's more like the other link_train functions. Will change it.
>
>
> > +{
> > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > + struct drm_device *dev = intel_dig_port->base.base.dev;
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + enum port port = intel_dig_port->port;
> > + uint32_t l;
>
> We usually use "val", "tmp" or "temp". L is unusual.
Ok, will change.
>
>
> > +
> > + if (!HAS_DDI(dev))
> > + return;
> > +
> > + l = I915_READ(DP_TP_CTL(port));
> > + l &= ~DP_TP_CTL_LINK_TRAIN_MASK;
> > + l |= DP_TP_CTL_LINK_TRAIN_IDLE;
> > + I915_WRITE(DP_TP_CTL(port), l);
> > +
> > + /*
> > + * On PORT_A we can have only eDP in SST mode. There the only reason
> > + * we need to set idle transmission mode is to work around a HW issue
> > + * where we enable the pipe while not in idle link-training mode.
> > + * In this case there is requirement to wait for a minimum number of
> > + * idle patterns to be sent.
> > + */
> > + if (port == PORT_A)
> > + return;
> > +
> > + if (wait_for((I915_READ(DP_TP_STATUS(port)) & DP_TP_STATUS_IDLE_DONE),
> > + 1))
> > + DRM_ERROR("Timed out waiting for DP idle patterns\n");
> > +}
> > +
> > /* Enable corresponding port and start training pattern 1 */
> > void
> > intel_dp_start_link_train(struct intel_dp *intel_dp)
> > @@ -2177,10 +2196,19 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
> > ++tries;
> > }
> >
> > + intel_dp_idle_link_train(intel_dp);
> > +
> > + intel_dp->DP = DP;
>
> I know this is not your fault, but I kinda _really_ don't like the
> fact that we have 3 places to store the contents of the same register:
> (i) the register itself, (ii) intel_dp->DP and (iii) these temporary
> DP variables. Whoever writes a patch to improve that without adding
> any regressions will get a free $favorite_beverage. I guess the first
> step is to try to remove the temporary variables only.
Yea, I agree we should improve on this. You didn't insist, so I suggest
that we do it after this patch gets merged. If there's no one else I can
also volunteer to do this a bit later.
> So I've also booted your patch and both DP and eDP still work for me.
> With or without the changes I proposed above:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Thanks,
Imre
>
> > +
> > if (channel_eq)
> > DRM_DEBUG_KMS("Channel EQ done. DP Training successfull\n");
> >
> > - intel_dp_set_link_train(intel_dp, DP, DP_TRAINING_PATTERN_DISABLE);
> > +}
> > +
> > +void intel_dp_stop_link_train(struct intel_dp *intel_dp)
> > +{
> > + intel_dp_set_link_train(intel_dp, intel_dp->DP,
> > + DP_TRAINING_PATTERN_DISABLE);
> > }
> >
> > static void
> > @@ -2388,6 +2416,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
> > drm_get_encoder_name(&intel_encoder->base));
> > intel_dp_start_link_train(intel_dp);
> > intel_dp_complete_link_train(intel_dp);
> > + intel_dp_stop_link_train(intel_dp);
> > }
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index a8c6960..6d1e63a 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -542,6 +542,7 @@ extern void intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> > extern void intel_dp_init_link_config(struct intel_dp *intel_dp);
> > extern void intel_dp_start_link_train(struct intel_dp *intel_dp);
> > extern void intel_dp_complete_link_train(struct intel_dp *intel_dp);
> > +extern void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> > extern void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> > extern void intel_dp_encoder_destroy(struct drm_encoder *encoder);
> > extern void intel_dp_check_link_status(struct intel_dp *intel_dp);
> > --
> > 1.7.10.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3] drm/i915: hsw: fix link training for eDP on port-A
2013-04-29 15:25 ` [PATCH v2] drm/i915: hsw: fix link training for eDP " Imre Deak
2013-05-02 17:45 ` Paulo Zanoni
@ 2013-05-03 9:57 ` Imre Deak
1 sibling, 0 replies; 5+ messages in thread
From: Imre Deak @ 2013-05-03 9:57 UTC (permalink / raw)
To: intel-gfx; +Cc: paulo.r.zanoni
According to BSpec the link training sequence for eDP on HSW port-A
should be as follows:
1. link training: clock recovery
2. link training: equalization
3. link training: set idle transmission mode
4. display pipe enable
5. link training: disable (set normal mode)
Contrary to this at the moment we don't do step 3. and we do step 5.
before step 4. Fix this by setting idle transmission mode for eDP at
the end of intel_dp_complete_link_train and adding a new
intel_dp_stop_link_training function to disable link training. With
these changes we'll end up with the following functions corresponding
to the above steps:
intel_dp_start_link_train -> step 1.
intel_dp_complete_link_train -> step 2., step 3.
intel_dp_stop_link_train -> step 5.
For port-A we'll call intel_dp_stop_link_train only after enabling the
pipe, for everything else we'll call it right after
intel_dp_complete_link_train to preserve the current behavior.
Tested on HSW/HSW-ULT.
In v2:
- Due to a HW issue we must set idle transmission mode for port-A too
before enabling the pipe. Thanks for Arthur Runyan for explaining
this.
- Update the patch subject to make it clear that it's an eDP fix, DP is
not affected.
v3:
- rename intel_dp_link_train() to intel_dp_set_link_train(), use 'val'
instead 'l' as var name. (Paulo)
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_ddi.c | 5 ++++
drivers/gpu/drm/i915/intel_dp.c | 59 ++++++++++++++++++++++++++++----------
drivers/gpu/drm/i915/intel_drv.h | 1 +
3 files changed, 50 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 0d7cf31..f3d0f21 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1265,6 +1265,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
intel_dp_start_link_train(intel_dp);
intel_dp_complete_link_train(intel_dp);
+ if (port != PORT_A)
+ intel_dp_stop_link_train(intel_dp);
}
}
@@ -1326,6 +1328,9 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
} else if (type == INTEL_OUTPUT_EDP) {
struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+ if (port == PORT_A)
+ intel_dp_stop_link_train(intel_dp);
+
ironlake_edp_backlight_on(intel_dp);
}
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 13d2fd6..e991600 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1437,6 +1437,7 @@ static void intel_enable_dp(struct intel_encoder *encoder)
ironlake_edp_panel_on(intel_dp);
ironlake_edp_panel_vdd_off(intel_dp, true);
intel_dp_complete_link_train(intel_dp);
+ intel_dp_stop_link_train(intel_dp);
ironlake_edp_backlight_on(intel_dp);
if (IS_VALLEYVIEW(dev)) {
@@ -1935,10 +1936,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
struct drm_i915_private *dev_priv = dev->dev_private;
enum port port = intel_dig_port->port;
int ret;
- uint32_t temp;
if (HAS_DDI(dev)) {
- temp = I915_READ(DP_TP_CTL(port));
+ uint32_t temp = I915_READ(DP_TP_CTL(port));
if (dp_train_pat & DP_LINK_SCRAMBLING_DISABLE)
temp |= DP_TP_CTL_SCRAMBLE_DISABLE;
@@ -1948,18 +1948,6 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
switch (dp_train_pat & DP_TRAINING_PATTERN_MASK) {
case DP_TRAINING_PATTERN_DISABLE:
-
- if (port != PORT_A) {
- temp |= DP_TP_CTL_LINK_TRAIN_IDLE;
- I915_WRITE(DP_TP_CTL(port), temp);
-
- if (wait_for((I915_READ(DP_TP_STATUS(port)) &
- DP_TP_STATUS_IDLE_DONE), 1))
- DRM_ERROR("Timed out waiting for DP idle patterns\n");
-
- temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
- }
-
temp |= DP_TP_CTL_LINK_TRAIN_NORMAL;
break;
@@ -2035,6 +2023,37 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
return true;
}
+static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
+{
+ struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+ struct drm_device *dev = intel_dig_port->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ enum port port = intel_dig_port->port;
+ uint32_t val;
+
+ if (!HAS_DDI(dev))
+ return;
+
+ val = I915_READ(DP_TP_CTL(port));
+ val &= ~DP_TP_CTL_LINK_TRAIN_MASK;
+ val |= DP_TP_CTL_LINK_TRAIN_IDLE;
+ I915_WRITE(DP_TP_CTL(port), val);
+
+ /*
+ * On PORT_A we can have only eDP in SST mode. There the only reason
+ * we need to set idle transmission mode is to work around a HW issue
+ * where we enable the pipe while not in idle link-training mode.
+ * In this case there is requirement to wait for a minimum number of
+ * idle patterns to be sent.
+ */
+ if (port == PORT_A)
+ return;
+
+ if (wait_for((I915_READ(DP_TP_STATUS(port)) & DP_TP_STATUS_IDLE_DONE),
+ 1))
+ DRM_ERROR("Timed out waiting for DP idle patterns\n");
+}
+
/* Enable corresponding port and start training pattern 1 */
void
intel_dp_start_link_train(struct intel_dp *intel_dp)
@@ -2177,10 +2196,19 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
++tries;
}
+ intel_dp_set_idle_link_train(intel_dp);
+
+ intel_dp->DP = DP;
+
if (channel_eq)
DRM_DEBUG_KMS("Channel EQ done. DP Training successfull\n");
- intel_dp_set_link_train(intel_dp, DP, DP_TRAINING_PATTERN_DISABLE);
+}
+
+void intel_dp_stop_link_train(struct intel_dp *intel_dp)
+{
+ intel_dp_set_link_train(intel_dp, intel_dp->DP,
+ DP_TRAINING_PATTERN_DISABLE);
}
static void
@@ -2388,6 +2416,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
drm_get_encoder_name(&intel_encoder->base));
intel_dp_start_link_train(intel_dp);
intel_dp_complete_link_train(intel_dp);
+ intel_dp_stop_link_train(intel_dp);
}
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a8c6960..6d1e63a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -542,6 +542,7 @@ extern void intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
extern void intel_dp_init_link_config(struct intel_dp *intel_dp);
extern void intel_dp_start_link_train(struct intel_dp *intel_dp);
extern void intel_dp_complete_link_train(struct intel_dp *intel_dp);
+extern void intel_dp_stop_link_train(struct intel_dp *intel_dp);
extern void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
extern void intel_dp_encoder_destroy(struct drm_encoder *encoder);
extern void intel_dp_check_link_status(struct intel_dp *intel_dp);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-05-03 9:57 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-26 17:52 [PATCH] drm/i915: hsw: fix link training for DP/eDP on port-A Imre Deak
2013-04-29 15:25 ` [PATCH v2] drm/i915: hsw: fix link training for eDP " Imre Deak
2013-05-02 17:45 ` Paulo Zanoni
2013-05-02 18:56 ` Imre Deak
2013-05-03 9:57 ` [PATCH v3] " Imre Deak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox