| Message ID | 1448450801-3927-1-git-send-email-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, > drm/i915: Disable FLT if DP config changes Please just spell out fast link training. On Wed, 2015-11-25 at 13:26 +0200, Mika Kahola wrote: > Disable DP fast link training if DP link configuration > changes. If one of the DP link parameters i.e. link > bandwidth, lane count, rate selection, port clock or bpp > changes the link training does no longer apply the > previously computed voltage swing and pre-emphasis values. > Instead, the link training is started with zero values. I think there is a more fundamental problem with our fast link training implementation. The requirements imposed by the spec for it are: - HPD line was high the whole time since the link was in full operation (except for IRQ_HPD); - the NO_AUX_TRANSACTION_LINK_TRAINING bit is set in DP_MAX_DOWNSPREAD. If both those conditions are met, then link training can skip the AUX transactions and start with the last known good voltage swing and pre-emphasis. Ville has seen an issue with a dongle that causes a lot of DPCD defers when link training starts with non-zero values [1]. I wonder if we should disable fast lin k training completely until we can meet those requirements. [1] http://lists.freedesktop.org/archives/intel-gfx/2015-November/079277.html Ander > > The patch is fix for reported screen flickering issue in > > https://bugs.freedesktop.org/show_bug.cgi?id=91393 > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 6 ++++++ > drivers/gpu/drm/i915/intel_dp_link_training.c | 27 > +++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 6 +++++- > 3 files changed, 38 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 2805f0d..3694e3f 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1621,6 +1621,12 @@ found: > intel_dp_compute_rate(intel_dp, pipe_config->port_clock, > &link_bw, &rate_select); > > + intel_dp->link_bw = link_bw; > + intel_dp->rate_select = rate_select; > + intel_dp->lane_count = lane_count; > + intel_dp->port_clock = pipe_config->port_clock; > + intel_dp->bpp = bpp; > + > DRM_DEBUG_KMS("DP link bw %02x rate select %02x lane count %d clock > %d bpp %d\n", > link_bw, rate_select, pipe_config->lane_count, > pipe_config->port_clock, bpp); > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c > b/drivers/gpu/drm/i915/intel_dp_link_training.c > index 8888793..36a5294 100644 > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > @@ -82,9 +82,31 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, > } > > static bool > +intel_dp_check_conf(struct intel_dp *intel_dp) > +{ > + if (intel_dp->link_bw != intel_dp->old_link_bw) > + return false; > + else if (intel_dp->lane_count != intel_dp->old_lane_count) > + return false; > + else if (intel_dp->rate_select != intel_dp->old_rate_select) > + return false; > + else if (intel_dp->port_clock != intel_dp->old_port_clock) > + return false; > + else if (intel_dp->bpp != intel_dp->old_bpp) > + return false; > + else > + return true; > +} > + > +static bool > intel_dp_reset_link_train(struct intel_dp *intel_dp, > uint8_t dp_train_pat) > { > + intel_dp->train_set_valid &= intel_dp_check_conf(intel_dp); > + > + DRM_DEBUG_KMS("flt enabled: %s\n", > + intel_dp->train_set_valid ? "true" : "false"); > + > if (!intel_dp->train_set_valid) > memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); > intel_dp_set_signal_levels(intel_dp); > @@ -305,6 +327,11 @@ intel_dp_link_training_channel_equalization(struct > intel_dp *intel_dp) > > if (channel_eq) { > intel_dp->train_set_valid = true; > + intel_dp->old_link_bw = intel_dp->link_bw; > + intel_dp->old_rate_select = intel_dp->rate_select; > + intel_dp->old_lane_count = intel_dp->lane_count; > + intel_dp->old_port_clock = intel_dp->port_clock; > + intel_dp->old_bpp = intel_dp->bpp; > DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n"); > } > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 8fae824..8db9288 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -742,7 +742,11 @@ struct intel_dp { > i915_reg_t aux_ch_data_reg[5]; > uint32_t DP; > int link_rate; > - uint8_t lane_count; > + uint8_t lane_count, old_lane_count; > + uint8_t link_bw, old_link_bw; > + uint8_t rate_select, old_rate_select; > + int port_clock, old_port_clock; > + int bpp, old_bpp; > bool has_audio; > enum hdmi_force_audio force_audio; > bool limited_color_range;
On Fri, Nov 27, 2015 at 02:01:31PM +0200, Ander Conselvan De Oliveira wrote: > Hi, > > > drm/i915: Disable FLT if DP config changes > > Please just spell out fast link training. > > On Wed, 2015-11-25 at 13:26 +0200, Mika Kahola wrote: > > Disable DP fast link training if DP link configuration > > changes. If one of the DP link parameters i.e. link > > bandwidth, lane count, rate selection, port clock or bpp > > changes the link training does no longer apply the > > previously computed voltage swing and pre-emphasis values. > > Instead, the link training is started with zero values. > > I think there is a more fundamental problem with our fast link training > implementation. The requirements imposed by the spec for it are: > > - HPD line was high the whole time since the link was in full operation > (except for IRQ_HPD); > > - the NO_AUX_TRANSACTION_LINK_TRAINING bit is set in DP_MAX_DOWNSPREAD. > > If both those conditions are met, then link training can skip the AUX > transactions and start with the last known good voltage swing and pre-emphasis. > > Ville has seen an issue with a dongle that causes a lot of DPCD defers when link > training starts with non-zero values [1]. I wonder if we should disable fast lin > k training completely until we can meet those requirements. Well the spec does say "The upstream device may start Full Link Training with non-minimum differential voltage swing and/or non-zero pre-emphasis if the optimal setting is already known, for example, as is the case in embedded application. In this case, the upstream device must write the swing and pre-emphasis settings at which it is starting training to the TRAINING_LANEx_SET register(s) as part of the burst write in which it writes 21h to TRAINING_PATTERN_SET register" We do use a burst write as described, so in theory what we do is fully spec compliant (it's just not what the spec calls "fast link training"). But I suppose it's possible people have taken the "embedded application" note to mean that normal DP devices don't need to support this sort of thing, and maybe no one else even tries to do this and so some DP devices might get confused by it. However I definitely agree with the idea in this patch. If the link parameters change we should throw away our previous "known good" swing/pre-emphasis settings. > > [1] http://lists.freedesktop.org/archives/intel-gfx/2015-November/079277.html > > Ander > > > > > > The patch is fix for reported screen flickering issue in > > > > https://bugs.freedesktop.org/show_bug.cgi?id=91393 > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++++ > > drivers/gpu/drm/i915/intel_dp_link_training.c | 27 > > +++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_drv.h | 6 +++++- > > 3 files changed, 38 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 2805f0d..3694e3f 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1621,6 +1621,12 @@ found: > > intel_dp_compute_rate(intel_dp, pipe_config->port_clock, > > &link_bw, &rate_select); > > > > + intel_dp->link_bw = link_bw; > > + intel_dp->rate_select = rate_select; > > + intel_dp->lane_count = lane_count; > > + intel_dp->port_clock = pipe_config->port_clock; > > + intel_dp->bpp = bpp; I'm thinking that only port_clock and lane_count would have relevance here. Eg. if bpp changes, it won't actually affect the link in any physical sense. > > + > > DRM_DEBUG_KMS("DP link bw %02x rate select %02x lane count %d clock > > %d bpp %d\n", > > link_bw, rate_select, pipe_config->lane_count, > > pipe_config->port_clock, bpp); > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c > > b/drivers/gpu/drm/i915/intel_dp_link_training.c > > index 8888793..36a5294 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > > @@ -82,9 +82,31 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, > > } > > > > static bool > > +intel_dp_check_conf(struct intel_dp *intel_dp) > > +{ > > + if (intel_dp->link_bw != intel_dp->old_link_bw) > > + return false; > > + else if (intel_dp->lane_count != intel_dp->old_lane_count) > > + return false; > > + else if (intel_dp->rate_select != intel_dp->old_rate_select) > > + return false; > > + else if (intel_dp->port_clock != intel_dp->old_port_clock) > > + return false; > > + else if (intel_dp->bpp != intel_dp->old_bpp) > > + return false; > > + else > > + return true; > > +} > > + > > +static bool > > intel_dp_reset_link_train(struct intel_dp *intel_dp, > > uint8_t dp_train_pat) > > { > > + intel_dp->train_set_valid &= intel_dp_check_conf(intel_dp); > > + > > + DRM_DEBUG_KMS("flt enabled: %s\n", > > + intel_dp->train_set_valid ? "true" : "false"); > > + > > if (!intel_dp->train_set_valid) > > memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); > > intel_dp_set_signal_levels(intel_dp); > > @@ -305,6 +327,11 @@ intel_dp_link_training_channel_equalization(struct > > intel_dp *intel_dp) > > > > if (channel_eq) { > > intel_dp->train_set_valid = true; > > + intel_dp->old_link_bw = intel_dp->link_bw; > > + intel_dp->old_rate_select = intel_dp->rate_select; > > + intel_dp->old_lane_count = intel_dp->lane_count; > > + intel_dp->old_port_clock = intel_dp->port_clock; > > + intel_dp->old_bpp = intel_dp->bpp; > > DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n"); > > } > > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 8fae824..8db9288 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -742,7 +742,11 @@ struct intel_dp { > > i915_reg_t aux_ch_data_reg[5]; > > uint32_t DP; > > int link_rate; > > - uint8_t lane_count; > > + uint8_t lane_count, old_lane_count; > > + uint8_t link_bw, old_link_bw; > > + uint8_t rate_select, old_rate_select; > > + int port_clock, old_port_clock; > > + int bpp, old_bpp; > > bool has_audio; > > enum hdmi_force_audio force_audio; > > bool limited_color_range;
> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Friday, November 27, 2015 2:13 PM > To: Ander Conselvan De Oliveira > Cc: Kahola, Mika; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Disable FLT if DP config changes > > On Fri, Nov 27, 2015 at 02:01:31PM +0200, Ander Conselvan De Oliveira > wrote: > > Hi, > > > > > drm/i915: Disable FLT if DP config changes > > > > Please just spell out fast link training. I can do that. Although, this patch is not exactly what the spec means by fast link training but could be considered as one as we try to train the link with known good values. This patch adds an additional check if we have any point of trying out the known good values in the first place. > > > > On Wed, 2015-11-25 at 13:26 +0200, Mika Kahola wrote: > > > Disable DP fast link training if DP link configuration changes. If > > > one of the DP link parameters i.e. link bandwidth, lane count, rate > > > selection, port clock or bpp changes the link training does no > > > longer apply the previously computed voltage swing and pre-emphasis > > > values. > > > Instead, the link training is started with zero values. > > > > I think there is a more fundamental problem with our fast link > > training implementation. The requirements imposed by the spec for it are: > > > > - HPD line was high the whole time since the link was in full > > operation (except for IRQ_HPD); > > > > - the NO_AUX_TRANSACTION_LINK_TRAINING bit is set in > DP_MAX_DOWNSPREAD. > > > > If both those conditions are met, then link training can skip the AUX > > transactions and start with the last known good voltage swing and pre- > emphasis. I could add these checks for the patch. Especially if the NO_AUX_TRANSACTION_LINK_TRAINING bit is set. What should we do in case of suspend/resume cycle? The link has been trained before and we know the good parameters so why wouldn't we be able try this set first? At least with the eDP case the display is there. > > > > Ville has seen an issue with a dongle that causes a lot of DPCD defers > > when link training starts with non-zero values [1]. I wonder if we > > should disable fast lin k training completely until we can meet those > requirements. I wouldn't want give up on this yet. I think at the moment we are just a bit too optimistic on when we can reuse the precomputed link training parameters. Thanks guys for the review! Cheers, Mika > > Well the spec does say > "The upstream device may start Full Link Training with non-minimum > differential voltage swing and/or non-zero pre-emphasis if the optimal > setting is already known, for example, as is the case in embedded > application. > In this case, the upstream device must write the swing and pre-emphasis > settings at which it is starting training to the TRAINING_LANEx_SET > register(s) as part of the burst write in which it writes 21h to > TRAINING_PATTERN_SET register" > > We do use a burst write as described, so in theory what we do is fully spec > compliant (it's just not what the spec calls "fast link training"). But I suppose > it's possible people have taken the "embedded application" note to mean > that normal DP devices don't need to support this sort of thing, and maybe > no one else even tries to do this and so some DP devices might get confused > by it. > > However I definitely agree with the idea in this patch. If the link parameters > change we should throw away our previous "known good" > swing/pre-emphasis settings. > > > > > [1] > > http://lists.freedesktop.org/archives/intel-gfx/2015-November/079277.h > > tml > > > > Ander > > > > > > > > > > The patch is fix for reported screen flickering issue in > > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=91393 > > > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 6 ++++++ > > > drivers/gpu/drm/i915/intel_dp_link_training.c | 27 > > > +++++++++++++++++++++++++++ > > > drivers/gpu/drm/i915/intel_drv.h | 6 +++++- > > > 3 files changed, 38 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c index 2805f0d..3694e3f 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -1621,6 +1621,12 @@ found: > > > intel_dp_compute_rate(intel_dp, pipe_config->port_clock, > > > &link_bw, &rate_select); > > > > > > + intel_dp->link_bw = link_bw; > > > + intel_dp->rate_select = rate_select; > > > + intel_dp->lane_count = lane_count; > > > + intel_dp->port_clock = pipe_config->port_clock; > > > + intel_dp->bpp = bpp; > > I'm thinking that only port_clock and lane_count would have relevance here. > Eg. if bpp changes, it won't actually affect the link in any physical sense. > > > > + > > > DRM_DEBUG_KMS("DP link bw %02x rate select %02x lane count %d > > > clock %d bpp %d\n", > > > link_bw, rate_select, pipe_config->lane_count, > > > pipe_config->port_clock, bpp); diff --git > > > a/drivers/gpu/drm/i915/intel_dp_link_training.c > > > b/drivers/gpu/drm/i915/intel_dp_link_training.c > > > index 8888793..36a5294 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > > > @@ -82,9 +82,31 @@ intel_dp_set_link_train(struct intel_dp > > > *intel_dp, } > > > > > > static bool > > > +intel_dp_check_conf(struct intel_dp *intel_dp) { > > > + if (intel_dp->link_bw != intel_dp->old_link_bw) > > > + return false; > > > + else if (intel_dp->lane_count != intel_dp->old_lane_count) > > > + return false; > > > + else if (intel_dp->rate_select != intel_dp->old_rate_select) > > > + return false; > > > + else if (intel_dp->port_clock != intel_dp->old_port_clock) > > > + return false; > > > + else if (intel_dp->bpp != intel_dp->old_bpp) > > > + return false; > > > + else > > > + return true; > > > +} > > > + > > > +static bool > > > intel_dp_reset_link_train(struct intel_dp *intel_dp, > > > uint8_t dp_train_pat) > > > { > > > + intel_dp->train_set_valid &= intel_dp_check_conf(intel_dp); > > > + > > > + DRM_DEBUG_KMS("flt enabled: %s\n", > > > + intel_dp->train_set_valid ? "true" : "false"); > > > + > > > if (!intel_dp->train_set_valid) > > > memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); > > > intel_dp_set_signal_levels(intel_dp); > > > @@ -305,6 +327,11 @@ > > > intel_dp_link_training_channel_equalization(struct > > > intel_dp *intel_dp) > > > > > > if (channel_eq) { > > > intel_dp->train_set_valid = true; > > > + intel_dp->old_link_bw = intel_dp->link_bw; > > > + intel_dp->old_rate_select = intel_dp->rate_select; > > > + intel_dp->old_lane_count = intel_dp->lane_count; > > > + intel_dp->old_port_clock = intel_dp->port_clock; > > > + intel_dp->old_bpp = intel_dp->bpp; > > > DRM_DEBUG_KMS("Channel EQ done. DP Training > successful\n"); > > > } > > > } > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index 8fae824..8db9288 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -742,7 +742,11 @@ struct intel_dp { > > > i915_reg_t aux_ch_data_reg[5]; > > > uint32_t DP; > > > int link_rate; > > > - uint8_t lane_count; > > > + uint8_t lane_count, old_lane_count; > > > + uint8_t link_bw, old_link_bw; > > > + uint8_t rate_select, old_rate_select; > > > + int port_clock, old_port_clock; > > > + int bpp, old_bpp; > > > bool has_audio; > > > enum hdmi_force_audio force_audio; > > > bool limited_color_range; > > -- > Ville Syrjälä > Intel OTC
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 2805f0d..3694e3f 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1621,6 +1621,12 @@ found: intel_dp_compute_rate(intel_dp, pipe_config->port_clock, &link_bw, &rate_select); + intel_dp->link_bw = link_bw; + intel_dp->rate_select = rate_select; + intel_dp->lane_count = lane_count; + intel_dp->port_clock = pipe_config->port_clock; + intel_dp->bpp = bpp; + DRM_DEBUG_KMS("DP link bw %02x rate select %02x lane count %d clock %d bpp %d\n", link_bw, rate_select, pipe_config->lane_count, pipe_config->port_clock, bpp); diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c index 8888793..36a5294 100644 --- a/drivers/gpu/drm/i915/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c @@ -82,9 +82,31 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, } static bool +intel_dp_check_conf(struct intel_dp *intel_dp) +{ + if (intel_dp->link_bw != intel_dp->old_link_bw) + return false; + else if (intel_dp->lane_count != intel_dp->old_lane_count) + return false; + else if (intel_dp->rate_select != intel_dp->old_rate_select) + return false; + else if (intel_dp->port_clock != intel_dp->old_port_clock) + return false; + else if (intel_dp->bpp != intel_dp->old_bpp) + return false; + else + return true; +} + +static bool intel_dp_reset_link_train(struct intel_dp *intel_dp, uint8_t dp_train_pat) { + intel_dp->train_set_valid &= intel_dp_check_conf(intel_dp); + + DRM_DEBUG_KMS("flt enabled: %s\n", + intel_dp->train_set_valid ? "true" : "false"); + if (!intel_dp->train_set_valid) memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); intel_dp_set_signal_levels(intel_dp); @@ -305,6 +327,11 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) if (channel_eq) { intel_dp->train_set_valid = true; + intel_dp->old_link_bw = intel_dp->link_bw; + intel_dp->old_rate_select = intel_dp->rate_select; + intel_dp->old_lane_count = intel_dp->lane_count; + intel_dp->old_port_clock = intel_dp->port_clock; + intel_dp->old_bpp = intel_dp->bpp; DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n"); } } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 8fae824..8db9288 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -742,7 +742,11 @@ struct intel_dp { i915_reg_t aux_ch_data_reg[5]; uint32_t DP; int link_rate; - uint8_t lane_count; + uint8_t lane_count, old_lane_count; + uint8_t link_bw, old_link_bw; + uint8_t rate_select, old_rate_select; + int port_clock, old_port_clock; + int bpp, old_bpp; bool has_audio; enum hdmi_force_audio force_audio; bool limited_color_range;
Disable DP fast link training if DP link configuration changes. If one of the DP link parameters i.e. link bandwidth, lane count, rate selection, port clock or bpp changes the link training does no longer apply the previously computed voltage swing and pre-emphasis values. Instead, the link training is started with zero values. The patch is fix for reported screen flickering issue in https://bugs.freedesktop.org/show_bug.cgi?id=91393 Signed-off-by: Mika Kahola <mika.kahola@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 6 ++++++ drivers/gpu/drm/i915/intel_dp_link_training.c | 27 +++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 6 +++++- 3 files changed, 38 insertions(+), 1 deletion(-)