| Message ID | 1424942770-14493-1-git-send-email-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 26, 2015 at 11:26:10AM +0200, Mika Kahola wrote: > In a case when DP link has been once trained we can reuse > the existing link training parameters i.e. voltage swing > and pre-emphasis levels from cache when there is a need to > restart link training. In a case of eDP we initially try > to train the link by using the parameter set that we can find > from VBT. The fallback on both cases is to reset the link > training parameters and restart. I don't think the fallback from bad VBT values works as on failure they just get reset. Care to enlighten me? -Chris
> -----Original Message----- > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > Sent: Thursday, February 26, 2015 12:51 PM > To: Kahola, Mika > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915: DP link training optimization > > On Thu, Feb 26, 2015 at 11:26:10AM +0200, Mika Kahola wrote: > > In a case when DP link has been once trained we can reuse the existing > > link training parameters i.e. voltage swing and pre-emphasis levels > > from cache when there is a need to restart link training. In a case of > > eDP we initially try to train the link by using the parameter set that > > we can find from VBT. The fallback on both cases is to reset the link > > training parameters and restart. > > I don't think the fallback from bad VBT values works as on failure they just > get reset. Care to enlighten me? > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre Thank you for the comment! This was my first patch to gfx development so all comments are welcomed. The idea that I had in mind was to initially start DP link training with the parameters that are stored in VBT. If we are unlucky and the link cannot be established at once by applying those initial parameters the parameters are reset to zero and link training is restarted. I do agree that the fallback in this case is unnecessary if VBT values are reset to zero in case of a failure. Cheers, Mika
On Thu, Feb 26, 2015 at 02:19:51PM +0000, Kahola, Mika wrote: > > -----Original Message----- > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > > Sent: Thursday, February 26, 2015 12:51 PM > > To: Kahola, Mika > > Cc: intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: DP link training optimization > > > > On Thu, Feb 26, 2015 at 11:26:10AM +0200, Mika Kahola wrote: > > > In a case when DP link has been once trained we can reuse the existing > > > link training parameters i.e. voltage swing and pre-emphasis levels > > > from cache when there is a need to restart link training. In a case of > > > eDP we initially try to train the link by using the parameter set that > > > we can find from VBT. The fallback on both cases is to reset the link > > > training parameters and restart. > > > > I don't think the fallback from bad VBT values works as on failure they just > > get reset. Care to enlighten me? > > -Chris > > > > -- > > Chris Wilson, Intel Open Source Technology Centre > > Thank you for the comment! This was my first patch to gfx development so all comments are welcomed. > > The idea that I had in mind was to initially start DP link training with the parameters that are stored in VBT. If we are unlucky and the link cannot be established at once by applying those initial parameters the parameters are reset to zero and link training is restarted. I do agree that the fallback in this case is unnecessary if VBT values are reset to zero in case of a failure. The VBT values themselves are not reset to zero. What I meant was the sequence for VBT recovery is: first time link training -> intel_dp_reset_link_train() -> use VBT values -> reuse_train_set = true clock recovery !ok -> reuse_train_set == true -> intel_dp_reset_link_train -> use VBT values again -> reuse_train_set = false i.e. every time we call reset_link_train we restore the VBT values and not zero. What you want is something like a intel_dp->edp_use_vbt_train_set flag that is initially set to is_edp(intel_edp) and then cleared if clock recovery ever fails. -Chris
Hi Mika, On 2/26/2015 2:26 AM, Mika Kahola wrote: > In a case when DP link has been once trained we can reuse > the existing link training parameters i.e. voltage swing > and pre-emphasis levels from cache when there is a need to > restart link training. In a case of eDP we initially try > to train the link by using the parameter set that we can find > from VBT. The fallback on both cases is to reset the link > training parameters and restart. I think you have a good idea here for the eDP case. But the way it's coded, there appear to be cases where the code will try to reuse the same training settings for regular DP as well. That will likely have some unfortunate results, as there's no guarantee for a normal external Displayport connection that training settings which worked once will work the next time. This would be of particular concern when disconnecting and reconnecting Displayport sink devices, as it appears as though it would try to retrain the newly connected device using the previously valid settings for the old device. IMHO, this really needs to be written as eDP only and such that it can never occur on an external Displayport device. Otherwise, it seems like there's a good chance of ending up with a black screen when you plug in a Displayport monitor. > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 93 +++++++++++++++++++++++++++++++++++----- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 2 files changed, 84 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index d1141d3..8482f58 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3288,8 +3288,39 @@ static bool > intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP, > uint8_t dp_train_pat) > { > - memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); > + 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; > + int i; > + > + /* in case of eDP, get link training parameters from VBT */ > + if (is_edp(intel_dp)) { > + for (i=0; i<intel_dp->lane_count; i++) > + intel_dp->train_set[i] = dev_priv->vbt.edp_vswing | dev_priv->vbt.edp_preemphasis; > + } > + else > + memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); > + > + intel_dp_set_signal_levels(intel_dp, DP); > + return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); > +} > + > +static bool > +intel_dp_reuse_link_train(struct intel_dp *intel_dp, uint32_t *DP, > + uint8_t dp_train_pat) > +{ > + 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; > + > intel_dp_set_signal_levels(intel_dp, DP); > + > + I915_WRITE(intel_dp->output_reg, *DP); > + POSTING_READ(intel_dp->output_reg); > + > + drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET, > + intel_dp->train_set, intel_dp->lane_count); > + > return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); > } > I'm not sure you want to be updating the sink device at this point. This should be checked against the spec to be sure it's not out of sequence. > @@ -3356,6 +3387,8 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) > int voltage_tries, loop_tries; > uint32_t DP = intel_dp->DP; > uint8_t link_config[2]; > + uint8_t link_status[DP_LINK_STATUS_SIZE]; > + bool reuse_train_set = false; > > if (HAS_DDI(dev)) > intel_ddi_prepare_link_retrain(encoder); > @@ -3373,20 +3406,44 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) > > DP |= DP_PORT_EN; > > - /* clock recovery */ > - if (!intel_dp_reset_link_train(intel_dp, &DP, > - DP_TRAINING_PATTERN_1 | > - DP_LINK_SCRAMBLING_DISABLE)) { > - DRM_ERROR("failed to enable link training\n"); > - return; > + /* > + * check if DP has already trained. Reset voltage swing and > + * pre-emphasis levels if that's not the case. > + */ > + if (intel_dp->link_trained) { > + if (!intel_dp_reuse_link_train(intel_dp, &DP, > + DP_TRAINING_PATTERN_1 | > + DP_LINK_SCRAMBLING_DISABLE)) { > + DRM_DEBUG_KMS("unable to set known link training values\n"); > + return; > + } > + DRM_DEBUG_KMS("reuse current link train set\n"); > + reuse_train_set = true; > + } > + else { > + /* reset link training values */ > + if (!intel_dp_reset_link_train(intel_dp, &DP, > + DP_TRAINING_PATTERN_1 | > + DP_LINK_SCRAMBLING_DISABLE)) { > + DRM_ERROR("failed to enable link training\n"); > + return; > + } > + DRM_DEBUG_KMS("reset link train set\n"); > + > + /* > + * in case of eDP, allow fallback to reset link training set > + * if VBT parameters doesn't apply > + */ > + if (is_edp(intel_dp)) > + reuse_train_set = true; > + else > + reuse_train_set = false; > } > > voltage = 0xff; > voltage_tries = 0; > loop_tries = 0; > for (;;) { > - uint8_t link_status[DP_LINK_STATUS_SIZE]; > - > drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd); > if (!intel_dp_get_link_status(intel_dp, link_status)) { > DRM_ERROR("failed to get link status\n"); > @@ -3397,6 +3454,20 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) > DRM_DEBUG_KMS("clock recovery OK\n"); > break; > } > + /* > + * if we used previously trained voltage and pre-emphasis values > + * and we don't get clock recovery, reset link training values > + */ > + if (reuse_train_set) { > + DRM_DEBUG_KMS("clock recovery not ok, resetting DP train set\n"); > + if (!intel_dp_reset_link_train(intel_dp, &DP, > + DP_TRAINING_PATTERN_1 | > + DP_LINK_SCRAMBLING_DISABLE)) { > + DRM_ERROR("failed to enable link training\n"); > + return; > + } > + reuse_train_set = false; > + } > > /* Check to see if we've tried the max voltage */ > for (i = 0; i < intel_dp->lane_count; i++) > @@ -3511,8 +3582,10 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) > > intel_dp->DP = DP; > > - if (channel_eq) > + if (channel_eq) { > DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n"); > + intel_dp->link_trained = true; The link_trained flag is set here but never initialized or cleared anywhere else. So it looks like once it's set, it's there forever. That being the case, this code would never hit the else() case where intel_dp->link_trained is checked further up. I'm pretty sure that's not what you intended... :) > + } > > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 1fb1529..32ece0d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -668,6 +668,7 @@ struct intel_dp { > bool has_aux_irq, > int send_bytes, > uint32_t aux_clock_divider); > + bool link_trained; /* flag to indicate if DP link has been trained or not */ > }; > > struct intel_digital_port {
On Thu, Feb 26, 2015 at 06:29:45PM -0700, Todd Previte wrote: > Hi Mika, > > On 2/26/2015 2:26 AM, Mika Kahola wrote: > > In a case when DP link has been once trained we can reuse > > the existing link training parameters i.e. voltage swing > > and pre-emphasis levels from cache when there is a need to > > restart link training. In a case of eDP we initially try > > to train the link by using the parameter set that we can find > > from VBT. The fallback on both cases is to reset the link > > training parameters and restart. > I think you have a good idea here for the eDP case. But the way it's > coded, there appear to be cases where the code will try to reuse the > same training settings for regular DP as well. That will likely have > some unfortunate results, as there's no guarantee for a normal external > Displayport connection that training settings which worked once will > work the next time. This would be of particular concern when > disconnecting and reconnecting Displayport sink devices, as it appears > as though it would try to retrain the newly connected device using the > previously valid settings for the old device. > > IMHO, this really needs to be written as eDP only and such that it can > never occur on an external Displayport device. Otherwise, it seems like > there's a good chance of ending up with a black screen when you plug in > a Displayport monitor. I've thought we should retrain with the same parameters we used the last time iff the link was succesully trained using those parameters in the past and the sink hasn't been disconnected in the meantime. We'd do this in the hopes of speeding up subsequent link trainings. So resetting the saved parameters on link training failure and long HPD should be good enough here, I think. And this way the VBT thing could just be a special case where we just sort of pretend that we succesfully trained the link using those parameters in the past. Oh and we could also read out the state from the hardware during init, and assuming the link was up and stable at the time we could populate the "previous good values" from whatever the BIOS actually programmed into the hardware. Anyway, that's just how I've imagined we should implement this kind of stuff.
On Fri, 27 Feb 2015, Todd Previte <tprevite@gmail.com> wrote: > Hi Mika, > > On 2/26/2015 2:26 AM, Mika Kahola wrote: >> In a case when DP link has been once trained we can reuse >> the existing link training parameters i.e. voltage swing >> and pre-emphasis levels from cache when there is a need to >> restart link training. In a case of eDP we initially try >> to train the link by using the parameter set that we can find >> from VBT. The fallback on both cases is to reset the link >> training parameters and restart. > I think you have a good idea here for the eDP case. But the way it's > coded, there appear to be cases where the code will try to reuse the > same training settings for regular DP as well. That will likely have > some unfortunate results, as there's no guarantee for a normal external > Displayport connection that training settings which worked once will > work the next time. This would be of particular concern when > disconnecting and reconnecting Displayport sink devices, as it appears > as though it would try to retrain the newly connected device using the > previously valid settings for the old device. Hmm, I thought it would be all right on external DP also if we ensure we reset the train set when the sink may have changed, i.e. at suspend and hotplug. BR, Jani. > > IMHO, this really needs to be written as eDP only and such that it can > never occur on an external Displayport device. Otherwise, it seems like > there's a good chance of ending up with a black screen when you plug in > a Displayport monitor. > >> Signed-off-by: Mika Kahola <mika.kahola@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 93 +++++++++++++++++++++++++++++++++++----- >> drivers/gpu/drm/i915/intel_drv.h | 1 + >> 2 files changed, 84 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index d1141d3..8482f58 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -3288,8 +3288,39 @@ static bool >> intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP, >> uint8_t dp_train_pat) >> { >> - memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); >> + 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; >> + int i; >> + >> + /* in case of eDP, get link training parameters from VBT */ >> + if (is_edp(intel_dp)) { >> + for (i=0; i<intel_dp->lane_count; i++) >> + intel_dp->train_set[i] = dev_priv->vbt.edp_vswing | dev_priv->vbt.edp_preemphasis; >> + } >> + else >> + memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); >> + >> + intel_dp_set_signal_levels(intel_dp, DP); >> + return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); >> +} >> + >> +static bool >> +intel_dp_reuse_link_train(struct intel_dp *intel_dp, uint32_t *DP, >> + uint8_t dp_train_pat) >> +{ >> + 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; >> + >> intel_dp_set_signal_levels(intel_dp, DP); >> + >> + I915_WRITE(intel_dp->output_reg, *DP); >> + POSTING_READ(intel_dp->output_reg); >> + >> + drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET, >> + intel_dp->train_set, intel_dp->lane_count); >> + >> return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); >> } >> > I'm not sure you want to be updating the sink device at this point. This > should be checked against the spec to be sure it's not out of sequence. > >> @@ -3356,6 +3387,8 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) >> int voltage_tries, loop_tries; >> uint32_t DP = intel_dp->DP; >> uint8_t link_config[2]; >> + uint8_t link_status[DP_LINK_STATUS_SIZE]; >> + bool reuse_train_set = false; >> >> if (HAS_DDI(dev)) >> intel_ddi_prepare_link_retrain(encoder); >> @@ -3373,20 +3406,44 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) >> >> DP |= DP_PORT_EN; >> >> - /* clock recovery */ >> - if (!intel_dp_reset_link_train(intel_dp, &DP, >> - DP_TRAINING_PATTERN_1 | >> - DP_LINK_SCRAMBLING_DISABLE)) { >> - DRM_ERROR("failed to enable link training\n"); >> - return; >> + /* >> + * check if DP has already trained. Reset voltage swing and >> + * pre-emphasis levels if that's not the case. >> + */ >> + if (intel_dp->link_trained) { >> + if (!intel_dp_reuse_link_train(intel_dp, &DP, >> + DP_TRAINING_PATTERN_1 | >> + DP_LINK_SCRAMBLING_DISABLE)) { >> + DRM_DEBUG_KMS("unable to set known link training values\n"); >> + return; >> + } >> + DRM_DEBUG_KMS("reuse current link train set\n"); >> + reuse_train_set = true; >> + } >> + else { >> + /* reset link training values */ >> + if (!intel_dp_reset_link_train(intel_dp, &DP, >> + DP_TRAINING_PATTERN_1 | >> + DP_LINK_SCRAMBLING_DISABLE)) { >> + DRM_ERROR("failed to enable link training\n"); >> + return; >> + } >> + DRM_DEBUG_KMS("reset link train set\n"); >> + >> + /* >> + * in case of eDP, allow fallback to reset link training set >> + * if VBT parameters doesn't apply >> + */ >> + if (is_edp(intel_dp)) >> + reuse_train_set = true; >> + else >> + reuse_train_set = false; >> } >> >> voltage = 0xff; >> voltage_tries = 0; >> loop_tries = 0; >> for (;;) { >> - uint8_t link_status[DP_LINK_STATUS_SIZE]; >> - >> drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd); >> if (!intel_dp_get_link_status(intel_dp, link_status)) { >> DRM_ERROR("failed to get link status\n"); >> @@ -3397,6 +3454,20 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) >> DRM_DEBUG_KMS("clock recovery OK\n"); >> break; >> } >> + /* >> + * if we used previously trained voltage and pre-emphasis values >> + * and we don't get clock recovery, reset link training values >> + */ >> + if (reuse_train_set) { >> + DRM_DEBUG_KMS("clock recovery not ok, resetting DP train set\n"); >> + if (!intel_dp_reset_link_train(intel_dp, &DP, >> + DP_TRAINING_PATTERN_1 | >> + DP_LINK_SCRAMBLING_DISABLE)) { >> + DRM_ERROR("failed to enable link training\n"); >> + return; >> + } >> + reuse_train_set = false; >> + } >> >> /* Check to see if we've tried the max voltage */ >> for (i = 0; i < intel_dp->lane_count; i++) >> @@ -3511,8 +3582,10 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) >> >> intel_dp->DP = DP; >> >> - if (channel_eq) >> + if (channel_eq) { >> DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n"); >> + intel_dp->link_trained = true; > The link_trained flag is set here but never initialized or cleared > anywhere else. So it looks like once it's set, it's there forever. That > being the case, this code would never hit the else() case where > intel_dp->link_trained is checked further up. I'm pretty sure that's not > what you intended... :) > >> + } >> >> } >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 1fb1529..32ece0d 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -668,6 +668,7 @@ struct intel_dp { >> bool has_aux_irq, >> int send_bytes, >> uint32_t aux_clock_divider); >> + bool link_trained; /* flag to indicate if DP link has been trained or not */ >> }; >> >> struct intel_digital_port { > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 2/27/2015 1:14 PM, Jani Nikula wrote: > On Fri, 27 Feb 2015, Todd Previte <tprevite@gmail.com> wrote: >> Hi Mika, >> >> On 2/26/2015 2:26 AM, Mika Kahola wrote: >>> In a case when DP link has been once trained we can reuse >>> the existing link training parameters i.e. voltage swing >>> and pre-emphasis levels from cache when there is a need to >>> restart link training. In a case of eDP we initially try >>> to train the link by using the parameter set that we can find >>> from VBT. The fallback on both cases is to reset the link >>> training parameters and restart. >> I think you have a good idea here for the eDP case. But the way it's >> coded, there appear to be cases where the code will try to reuse the >> same training settings for regular DP as well. That will likely have >> some unfortunate results, as there's no guarantee for a normal external >> Displayport connection that training settings which worked once will >> work the next time. This would be of particular concern when >> disconnecting and reconnecting Displayport sink devices, as it appears >> as though it would try to retrain the newly connected device using the >> previously valid settings for the old device. > Hmm, I thought it would be all right on external DP also if we ensure we > reset the train set when the sink may have changed, i.e. at suspend and > hotplug. > > BR, > Jani. Though VBT fields for Fast Link Training has been available for a long time it was not always filled and there was no way to know if the values in VBT are valid or not. Hence it is more than likely to be entering the failing condition. There was a field added in BDW timeline to say if the VBT fields for FLT values is valid or not, so please use them only after such a check. regards, Sivakumar > >> IMHO, this really needs to be written as eDP only and such that it can >> never occur on an external Displayport device. Otherwise, it seems like >> there's a good chance of ending up with a black screen when you plug in >> a Displayport monitor. >> >>> Signed-off-by: Mika Kahola <mika.kahola@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_dp.c | 93 +++++++++++++++++++++++++++++++++++----- >>> drivers/gpu/drm/i915/intel_drv.h | 1 + >>> 2 files changed, 84 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>> index d1141d3..8482f58 100644 >>> --- a/drivers/gpu/drm/i915/intel_dp.c >>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>> @@ -3288,8 +3288,39 @@ static bool >>> intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP, >>> uint8_t dp_train_pat) >>> { >>> - memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); >>> + 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; >>> + int i; >>> + >>> + /* in case of eDP, get link training parameters from VBT */ >>> + if (is_edp(intel_dp)) { >>> + for (i=0; i<intel_dp->lane_count; i++) >>> + intel_dp->train_set[i] = dev_priv->vbt.edp_vswing | dev_priv->vbt.edp_preemphasis; >>> + } >>> + else >>> + memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); >>> + >>> + intel_dp_set_signal_levels(intel_dp, DP); >>> + return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); >>> +} >>> + >>> +static bool >>> +intel_dp_reuse_link_train(struct intel_dp *intel_dp, uint32_t *DP, >>> + uint8_t dp_train_pat) >>> +{ >>> + 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; >>> + >>> intel_dp_set_signal_levels(intel_dp, DP); >>> + >>> + I915_WRITE(intel_dp->output_reg, *DP); >>> + POSTING_READ(intel_dp->output_reg); >>> + >>> + drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET, >>> + intel_dp->train_set, intel_dp->lane_count); >>> + >>> return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); >>> } >>> >> I'm not sure you want to be updating the sink device at this point. This >> should be checked against the spec to be sure it's not out of sequence. >> >>> @@ -3356,6 +3387,8 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) >>> int voltage_tries, loop_tries; >>> uint32_t DP = intel_dp->DP; >>> uint8_t link_config[2]; >>> + uint8_t link_status[DP_LINK_STATUS_SIZE]; >>> + bool reuse_train_set = false; >>> >>> if (HAS_DDI(dev)) >>> intel_ddi_prepare_link_retrain(encoder); >>> @@ -3373,20 +3406,44 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) >>> >>> DP |= DP_PORT_EN; >>> >>> - /* clock recovery */ >>> - if (!intel_dp_reset_link_train(intel_dp, &DP, >>> - DP_TRAINING_PATTERN_1 | >>> - DP_LINK_SCRAMBLING_DISABLE)) { >>> - DRM_ERROR("failed to enable link training\n"); >>> - return; >>> + /* >>> + * check if DP has already trained. Reset voltage swing and >>> + * pre-emphasis levels if that's not the case. >>> + */ >>> + if (intel_dp->link_trained) { >>> + if (!intel_dp_reuse_link_train(intel_dp, &DP, >>> + DP_TRAINING_PATTERN_1 | >>> + DP_LINK_SCRAMBLING_DISABLE)) { >>> + DRM_DEBUG_KMS("unable to set known link training values\n"); >>> + return; >>> + } >>> + DRM_DEBUG_KMS("reuse current link train set\n"); >>> + reuse_train_set = true; >>> + } >>> + else { >>> + /* reset link training values */ >>> + if (!intel_dp_reset_link_train(intel_dp, &DP, >>> + DP_TRAINING_PATTERN_1 | >>> + DP_LINK_SCRAMBLING_DISABLE)) { >>> + DRM_ERROR("failed to enable link training\n"); >>> + return; >>> + } >>> + DRM_DEBUG_KMS("reset link train set\n"); >>> + >>> + /* >>> + * in case of eDP, allow fallback to reset link training set >>> + * if VBT parameters doesn't apply >>> + */ >>> + if (is_edp(intel_dp)) >>> + reuse_train_set = true; >>> + else >>> + reuse_train_set = false; >>> } >>> >>> voltage = 0xff; >>> voltage_tries = 0; >>> loop_tries = 0; >>> for (;;) { >>> - uint8_t link_status[DP_LINK_STATUS_SIZE]; >>> - >>> drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd); >>> if (!intel_dp_get_link_status(intel_dp, link_status)) { >>> DRM_ERROR("failed to get link status\n"); >>> @@ -3397,6 +3454,20 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) >>> DRM_DEBUG_KMS("clock recovery OK\n"); >>> break; >>> } >>> + /* >>> + * if we used previously trained voltage and pre-emphasis values >>> + * and we don't get clock recovery, reset link training values >>> + */ >>> + if (reuse_train_set) { >>> + DRM_DEBUG_KMS("clock recovery not ok, resetting DP train set\n"); >>> + if (!intel_dp_reset_link_train(intel_dp, &DP, >>> + DP_TRAINING_PATTERN_1 | >>> + DP_LINK_SCRAMBLING_DISABLE)) { >>> + DRM_ERROR("failed to enable link training\n"); >>> + return; >>> + } >>> + reuse_train_set = false; >>> + } >>> >>> /* Check to see if we've tried the max voltage */ >>> for (i = 0; i < intel_dp->lane_count; i++) >>> @@ -3511,8 +3582,10 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) >>> >>> intel_dp->DP = DP; >>> >>> - if (channel_eq) >>> + if (channel_eq) { >>> DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n"); >>> + intel_dp->link_trained = true; >> The link_trained flag is set here but never initialized or cleared >> anywhere else. So it looks like once it's set, it's there forever. That >> being the case, this code would never hit the else() case where >> intel_dp->link_trained is checked further up. I'm pretty sure that's not >> what you intended... :) >> >>> + } >>> >>> } >>> >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >>> index 1fb1529..32ece0d 100644 >>> --- a/drivers/gpu/drm/i915/intel_drv.h >>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>> @@ -668,6 +668,7 @@ struct intel_dp { >>> bool has_aux_irq, >>> int send_bytes, >>> uint32_t aux_clock_divider); >>> + bool link_trained; /* flag to indicate if DP link has been trained or not */ >>> }; >>> >>> struct intel_digital_port { >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index d1141d3..8482f58 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3288,8 +3288,39 @@ static bool intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP, uint8_t dp_train_pat) { - memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); + 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; + int i; + + /* in case of eDP, get link training parameters from VBT */ + if (is_edp(intel_dp)) { + for (i=0; i<intel_dp->lane_count; i++) + intel_dp->train_set[i] = dev_priv->vbt.edp_vswing | dev_priv->vbt.edp_preemphasis; + } + else + memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); + + intel_dp_set_signal_levels(intel_dp, DP); + return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); +} + +static bool +intel_dp_reuse_link_train(struct intel_dp *intel_dp, uint32_t *DP, + uint8_t dp_train_pat) +{ + 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; + intel_dp_set_signal_levels(intel_dp, DP); + + I915_WRITE(intel_dp->output_reg, *DP); + POSTING_READ(intel_dp->output_reg); + + drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET, + intel_dp->train_set, intel_dp->lane_count); + return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); } @@ -3356,6 +3387,8 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) int voltage_tries, loop_tries; uint32_t DP = intel_dp->DP; uint8_t link_config[2]; + uint8_t link_status[DP_LINK_STATUS_SIZE]; + bool reuse_train_set = false; if (HAS_DDI(dev)) intel_ddi_prepare_link_retrain(encoder); @@ -3373,20 +3406,44 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) DP |= DP_PORT_EN; - /* clock recovery */ - if (!intel_dp_reset_link_train(intel_dp, &DP, - DP_TRAINING_PATTERN_1 | - DP_LINK_SCRAMBLING_DISABLE)) { - DRM_ERROR("failed to enable link training\n"); - return; + /* + * check if DP has already trained. Reset voltage swing and + * pre-emphasis levels if that's not the case. + */ + if (intel_dp->link_trained) { + if (!intel_dp_reuse_link_train(intel_dp, &DP, + DP_TRAINING_PATTERN_1 | + DP_LINK_SCRAMBLING_DISABLE)) { + DRM_DEBUG_KMS("unable to set known link training values\n"); + return; + } + DRM_DEBUG_KMS("reuse current link train set\n"); + reuse_train_set = true; + } + else { + /* reset link training values */ + if (!intel_dp_reset_link_train(intel_dp, &DP, + DP_TRAINING_PATTERN_1 | + DP_LINK_SCRAMBLING_DISABLE)) { + DRM_ERROR("failed to enable link training\n"); + return; + } + DRM_DEBUG_KMS("reset link train set\n"); + + /* + * in case of eDP, allow fallback to reset link training set + * if VBT parameters doesn't apply + */ + if (is_edp(intel_dp)) + reuse_train_set = true; + else + reuse_train_set = false; } voltage = 0xff; voltage_tries = 0; loop_tries = 0; for (;;) { - uint8_t link_status[DP_LINK_STATUS_SIZE]; - drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd); if (!intel_dp_get_link_status(intel_dp, link_status)) { DRM_ERROR("failed to get link status\n"); @@ -3397,6 +3454,20 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) DRM_DEBUG_KMS("clock recovery OK\n"); break; } + /* + * if we used previously trained voltage and pre-emphasis values + * and we don't get clock recovery, reset link training values + */ + if (reuse_train_set) { + DRM_DEBUG_KMS("clock recovery not ok, resetting DP train set\n"); + if (!intel_dp_reset_link_train(intel_dp, &DP, + DP_TRAINING_PATTERN_1 | + DP_LINK_SCRAMBLING_DISABLE)) { + DRM_ERROR("failed to enable link training\n"); + return; + } + reuse_train_set = false; + } /* Check to see if we've tried the max voltage */ for (i = 0; i < intel_dp->lane_count; i++) @@ -3511,8 +3582,10 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) intel_dp->DP = DP; - if (channel_eq) + if (channel_eq) { DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n"); + intel_dp->link_trained = true; + } } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 1fb1529..32ece0d 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -668,6 +668,7 @@ struct intel_dp { bool has_aux_irq, int send_bytes, uint32_t aux_clock_divider); + bool link_trained; /* flag to indicate if DP link has been trained or not */ }; struct intel_digital_port {
In a case when DP link has been once trained we can reuse the existing link training parameters i.e. voltage swing and pre-emphasis levels from cache when there is a need to restart link training. In a case of eDP we initially try to train the link by using the parameter set that we can find from VBT. The fallback on both cases is to reset the link training parameters and restart. Signed-off-by: Mika Kahola <mika.kahola@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 93 +++++++++++++++++++++++++++++++++++----- drivers/gpu/drm/i915/intel_drv.h | 1 + 2 files changed, 84 insertions(+), 10 deletions(-)