Re: [PATCH v2] drm/i915: hsw: fix link training for eDP on port-A

[Imre Deak <imre.deak@intel.com>]

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
>