On Thu, 2013-05-02 at 14:45 -0300, Paulo Zanoni wrote:
> 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?
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 at intel.com>
> Tested-by: Paulo Zanoni <paulo.r.zanoni at 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
