[Intel-gfx] [PATCH v2] drm/i915: hsw: fix link training for eDP on port-A
Paulo Zanoni
przanoni at gmail.com
Thu May 2 19:45:09 CEST 2013
Hi
2013/4/29 Imre Deak <imre.deak at 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 at 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 at intel.com>
Tested-by: Paulo Zanoni <paulo.r.zanoni at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
More information about the Intel-gfx
mailing list