[Intel-gfx] [PATCH v7] drm/i915/edp: Be less aggressive about changing link config on eDP
This set of changes has some history to them. There were several attempts
to add what was called "fast link training" to i915, which actually wasn't
fast link training as per the DP spec. These changes were:
commit 5fa836a9d859 ("drm/i915: DP link training optimization")
commit 4e96c97742f4 ("drm/i915: eDP link training optimization")
which were eventually hand-reverted by:
commit 34511dce4b35 ("drm/i915: Revert DisplayPort fast link training
feature")
in kernel 4.7-rc4. The eDP pieces of the above revert, however, had some
very bad side-effects on PSR functionality on Skylake. The issue at
hand is that when PSR exits i915 briefly emits TP1 followed by TP2/3
(depending on the original link configuration) in order to quickly get
the source and sink back in synchronization across the link before handing
control back to the i915. There's an assumption that none of the link
configuration information has changed (and thus it's still valid) since the
last full link training operation. The revert above was identified via a
bisect as the cause of some of Skylake's PSR woes. This patch, largely
based on commit 4e96c97742f4 ("drm/i915: eDP link training optimization")
puts the eDP portions of this patch back in place. None of the flickering
issues that spurred the revert have been seen, and I suspect the real
culprits here were addressed by some of the recent link training changes
that Manasi has implemented, and PSR on Skylake is definitely more happy
with these changes in-place.
v2 and v3: Rebase
v4: * Clean up accesses to train_set_valid a bit for easier
reading. (Chris)
* Rebase
v5: * Checkpatch cleanup
* Rebase
v6: * is_edp() => intel_dp_is_edp()
* rebase
v7: * Remove extraneous is_edp() prototype (Rodrigo)
Cc: Chris Wilson
Cc: Rodrigo Vivi
Cc: Paulo Zanoni
Cc: Manasi D Navare
Cc: Mika Kahola
Cc: Jani Nikula
Fixes: 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")
Signed-off-by: Jim Bride
---
drivers/gpu/drm/i915/intel_dp.c | 2 ++
drivers/gpu/drm/i915/intel_dp_link_training.c | 15 ++-
drivers/gpu/drm/i915/intel_drv.h | 1 +
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e385658..38bc7e0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4750,6 +4750,7 @@ intel_dp_long_pulse(struct intel_connector
*intel_connector)
intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
intel_dp->reset_link_params = false;
+ intel_dp->train_set_valid = false;
}
intel_dp_print_rates(intel_dp);
@@ -6019,6 +6020,7 @@ intel_dp_init_connector(struct intel_digital_port
*intel_dig_port,
intel_dp_set_source_rates(intel_dp);
intel_dp->reset_link_params = true;
+ intel_dp->train_set_valid = false;
intel_dp->pps_pipe = INVALID_PIPE;
intel_dp->active_pipe = INVALID_PIPE;
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 05907fa..79fe369 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -94,7 +94,8 @@ static bool
intel_dp_reset_link_train(struct intel_dp *intel_dp,
uint8_t dp_train_pat)
{
- memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
+ if (!intel_dp->train_set_valid)
+ memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
intel_dp_set_signal_levels(intel_dp);
return intel_dp_set_link_train(intel_dp, dp_train_pat);
}
@@ -162,9 +163,18 @@ intel_dp_link_training_clock_recovery(struct intel_dp
*intel_dp)
DP_TRAINING_PATTERN_1 |
DP_LINK_SCRAMBLING_DISABLE)) {
DRM_ERROR("failed to enable link training\n");
+ intel_dp->train_set_valid = false;
return false;
}
+ /*
+* The initial set of link parameters are set by this point, so go
+* ahead and set intel_dp->train_set_valid to false in case any of
+* the succeeding steps fail. It will be set back to true if we were
+* able to achieve clock recovery in the specified configuration.
+*/
+ intel_dp->train_set_valid = false;
+
voltage_tries = 1;
max_vswing_tries = 0;
for (;;) {
@@ -179,6 +189,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp
*intel_dp)
if (drm_dp_clock_recovery_ok(link_status,
intel_dp->lane_count)) {
DRM_DEBUG_KMS("clock recovery OK\n");
+ intel_dp->train_set_valid = intel_dp_is_edp(intel_dp);
return true;
}
@@ -256,6 +267,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp
*int
Re: [Intel-gfx] [PATCH v7] drm/i915/edp: Be less aggressive about changing link config on eDP
On Tue, 2017-09-19 at 12:55 -0700, Rodrigo Vivi wrote:
> On Fri, Sep 15, 2017 at 06:19:12PM +, Manasi Navare wrote:
> >
> > The patch looks good for eDP link training optimizations.
> >
> > Reviewed-by: Manasi Navare
> I haven't merged this patch yet because I'd like an Ack from Jani.
>
> Also I'd like to hear from Mika if he believes it is safe or not.
>
> On his revert commit he wrote:
> "It has been found out that in some HW combination the DisplayPort
> fast link training feature caused screen flickering. Let's revert
> this feature for now until we can ensure that the feature works for
> all platforms."
At the time of this revert there were quite a few bug reports of
flickering. The community felt that this "fast link training" was to
blame for flickering issues and therefore we decided to revert that.
However, it turned out later on that this revert didn't solve the
flickering issues. Flickering was gone once watermark work landed.
Therefore, I feel that this feature is safe to re-enable.
>
> I don't want to merge this patch to fix a feature that is disabled
> by default with the risk of bringing flickerings back.
>
> But even if we decide to go ahead and merge I believe we need to
> resend the test and collect a full round of CI that now runs
> all IGT tests.
>
> Thanks,
> Rodrigo.
>
>
> >
> >
> > Manasi
> >
> > On Tue, Aug 22, 2017 at 09:34:46AM -0700, Jim Bride wrote:
> > >
> > > This set of changes has some history to them. There were several
> > > attempts
> > > to add what was called "fast link training" to i915, which
> > > actually wasn't
> > > fast link training as per the DP spec. These changes were:
> > >
> > > commit 5fa836a9d859 ("drm/i915: DP link training optimization")
> > > commit 4e96c97742f4 ("drm/i915: eDP link training optimization")
> > >
> > > which were eventually hand-reverted by:
> > >
> > > commit 34511dce4b35 ("drm/i915: Revert DisplayPort fast link
> > > training
> > > feature")
> > >
> > > in kernel 4.7-rc4. The eDP pieces of the above revert, however,
> > > had some
> > > very bad side-effects on PSR functionality on Skylake. The issue
> > > at
> > > hand is that when PSR exits i915 briefly emits TP1 followed by
> > > TP2/3
> > > (depending on the original link configuration) in order to
> > > quickly get
> > > the source and sink back in synchronization across the link
> > > before handing
> > > control back to the i915. There's an assumption that none of the
> > > link
> > > configuration information has changed (and thus it's still valid)
> > > since the
> > > last full link training operation. The revert above was
> > > identified via a
> > > bisect as the cause of some of Skylake's PSR woes. This patch,
> > > largely
> > > based on commit 4e96c97742f4 ("drm/i915: eDP link training
> > > optimization")
> > > puts the eDP portions of this patch back in place. None of the
> > > flickering
> > > issues that spurred the revert have been seen, and I suspect the
> > > real
> > > culprits here were addressed by some of the recent link training
> > > changes
> > > that Manasi has implemented, and PSR on Skylake is definitely
> > > more happy
> > > with these changes in-place.
> > >
> > > v2 and v3: Rebase
> > > v4: * Clean up accesses to train_set_valid a bit for easier
> > > reading. (Chris)
> > > * Rebase
> > > v5: * Checkpatch cleanup
> > > * Rebase
> > > v6: * is_edp() => intel_dp_is_edp()
> > > * rebase
> > > v7: * Remove extraneous is_edp() prototype (Rodrigo)
> > >
> > > Cc: Chris Wilson
> > > Cc: Rodrigo Vivi
> > > Cc: Paulo Zanoni
> > > Cc: Manasi D Navare
> > > Cc: Mika Kahola
> > > Cc: Jani Nikula
> > > Fixes: 34511dce4 ("drm/i915: Revert DisplayPort fast link
> > > training feature")
> > > Signed-off-by: Jim Bride
> > > ---
> > > drivers/gpu/drm/i915/intel_dp.c | 2 ++
> > > drivers/gpu/drm/i915/intel_dp_link_training.c | 15
> > > ++-
> > > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > > 3 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index e385658..38bc7e0 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4750,6 +4750,7 @@ intel_dp_long_pulse(struct intel_connector
> > > *intel_connector)
> > > intel_dp->max_link_rate =
> > > intel_dp_max_common_rate(intel_dp);
> > >
> > > intel_dp->reset_link_params = false;
> > > + intel_dp->train_set_valid = false;
> > > }
> > >
> > > intel_dp_print_rates(intel_dp);
> > > @@ -6019,6 +6020,7 @@ intel_dp_init_connector(struct
> > > intel_digital_port *intel_dig_port,
> > > intel_dp_set_source_rates(intel_dp);
> > >
> > > intel_dp->reset_link_params = true;
> > > + intel_dp->train_set_valid = false;
> > > intel_dp->pps_pipe = INVALID_PIPE;
> > > intel_dp->active_pipe = INVALID_PIPE;
> > >
> > > diff --g
Re: [Intel-gfx] [PATCH v7] drm/i915/edp: Be less aggressive about changing link config on eDP
On Wed, Sep 27, 2017 at 12:23:32PM +, Mika Kahola wrote:
> On Tue, 2017-09-19 at 12:55 -0700, Rodrigo Vivi wrote:
> > On Fri, Sep 15, 2017 at 06:19:12PM +, Manasi Navare wrote:
> > >
> > > The patch looks good for eDP link training optimizations.
> > >
> > > Reviewed-by: Manasi Navare
> > I haven't merged this patch yet because I'd like an Ack from Jani.
> >
> > Also I'd like to hear from Mika if he believes it is safe or not.
> >
> > On his revert commit he wrote:
> > "It has been found out that in some HW combination the DisplayPort
> > fast link training feature caused screen flickering. Let's revert
> > this feature for now until we can ensure that the feature works for
> > all platforms."
> At the time of this revert there were quite a few bug reports of
> flickering. The community felt that this "fast link training" was to
> blame for flickering issues and therefore we decided to revert that.
> However, it turned out later on that this revert didn't solve the
> flickering issues. Flickering was gone once watermark work landed.
I know this feeling... like PSR blamed for VGA flickers hehe
>
> Therefore, I feel that this feature is safe to re-enable.
Jani, Ack?
>
> >
> > I don't want to merge this patch to fix a feature that is disabled
> > by default with the risk of bringing flickerings back.
> >
> > But even if we decide to go ahead and merge I believe we need to
> > resend the test and collect a full round of CI that now runs
> > all IGT tests.
> >
> > Thanks,
> > Rodrigo.
> >
> >
> > >
> > >
> > > Manasi
> > >
> > > On Tue, Aug 22, 2017 at 09:34:46AM -0700, Jim Bride wrote:
> > > >
> > > > This set of changes has some history to them. There were several
> > > > attempts
> > > > to add what was called "fast link training" to i915, which
> > > > actually wasn't
> > > > fast link training as per the DP spec. These changes were:
> > > >
> > > > commit 5fa836a9d859 ("drm/i915: DP link training optimization")
> > > > commit 4e96c97742f4 ("drm/i915: eDP link training optimization")
> > > >
> > > > which were eventually hand-reverted by:
> > > >
> > > > commit 34511dce4b35 ("drm/i915: Revert DisplayPort fast link
> > > > training
> > > > feature")
> > > >
> > > > in kernel 4.7-rc4. The eDP pieces of the above revert, however,
> > > > had some
> > > > very bad side-effects on PSR functionality on Skylake. The issue
> > > > at
> > > > hand is that when PSR exits i915 briefly emits TP1 followed by
> > > > TP2/3
> > > > (depending on the original link configuration) in order to
> > > > quickly get
> > > > the source and sink back in synchronization across the link
> > > > before handing
> > > > control back to the i915. There's an assumption that none of the
> > > > link
> > > > configuration information has changed (and thus it's still valid)
> > > > since the
> > > > last full link training operation. The revert above was
> > > > identified via a
> > > > bisect as the cause of some of Skylake's PSR woes. This patch,
> > > > largely
> > > > based on commit 4e96c97742f4 ("drm/i915: eDP link training
> > > > optimization")
> > > > puts the eDP portions of this patch back in place. None of the
> > > > flickering
> > > > issues that spurred the revert have been seen, and I suspect the
> > > > real
> > > > culprits here were addressed by some of the recent link training
> > > > changes
> > > > that Manasi has implemented, and PSR on Skylake is definitely
> > > > more happy
> > > > with these changes in-place.
> > > >
> > > > v2 and v3: Rebase
> > > > v4: * Clean up accesses to train_set_valid a bit for easier
> > > > reading. (Chris)
> > > > * Rebase
> > > > v5: * Checkpatch cleanup
> > > > * Rebase
> > > > v6: * is_edp() => intel_dp_is_edp()
> > > > * rebase
> > > > v7: * Remove extraneous is_edp() prototype (Rodrigo)
> > > >
> > > > Cc: Chris Wilson
> > > > Cc: Rodrigo Vivi
> > > > Cc: Paulo Zanoni
> > > > Cc: Manasi D Navare
> > > > Cc: Mika Kahola
> > > > Cc: Jani Nikula
> > > > Fixes: 34511dce4 ("drm/i915: Revert DisplayPort fast link
> > > > training feature")
> > > > Signed-off-by: Jim Bride
> > > > ---
> > > > drivers/gpu/drm/i915/intel_dp.c | 2 ++
> > > > drivers/gpu/drm/i915/intel_dp_link_training.c | 15
> > > > ++-
> > > > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > > > 3 files changed, 17 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index e385658..38bc7e0 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4750,6 +4750,7 @@ intel_dp_long_pulse(struct intel_connector
> > > > *intel_connector)
> > > > intel_dp->max_link_rate =
> > > > intel_dp_max_common_rate(intel_dp);
> > > >
> > > > intel_dp->reset_link_params = false;
> > > > + intel_dp->train_set_valid = false;
> > > >
Re: [Intel-gfx] [PATCH v7] drm/i915/edp: Be less aggressive about changing link config on eDP
The patch looks good for eDP link training optimizations.
Reviewed-by: Manasi Navare
Manasi
On Tue, Aug 22, 2017 at 09:34:46AM -0700, Jim Bride wrote:
> This set of changes has some history to them. There were several attempts
> to add what was called "fast link training" to i915, which actually wasn't
> fast link training as per the DP spec. These changes were:
>
> commit 5fa836a9d859 ("drm/i915: DP link training optimization")
> commit 4e96c97742f4 ("drm/i915: eDP link training optimization")
>
> which were eventually hand-reverted by:
>
> commit 34511dce4b35 ("drm/i915: Revert DisplayPort fast link training
> feature")
>
> in kernel 4.7-rc4. The eDP pieces of the above revert, however, had some
> very bad side-effects on PSR functionality on Skylake. The issue at
> hand is that when PSR exits i915 briefly emits TP1 followed by TP2/3
> (depending on the original link configuration) in order to quickly get
> the source and sink back in synchronization across the link before handing
> control back to the i915. There's an assumption that none of the link
> configuration information has changed (and thus it's still valid) since the
> last full link training operation. The revert above was identified via a
> bisect as the cause of some of Skylake's PSR woes. This patch, largely
> based on commit 4e96c97742f4 ("drm/i915: eDP link training optimization")
> puts the eDP portions of this patch back in place. None of the flickering
> issues that spurred the revert have been seen, and I suspect the real
> culprits here were addressed by some of the recent link training changes
> that Manasi has implemented, and PSR on Skylake is definitely more happy
> with these changes in-place.
>
> v2 and v3: Rebase
> v4: * Clean up accesses to train_set_valid a bit for easier
> reading. (Chris)
> * Rebase
> v5: * Checkpatch cleanup
> * Rebase
> v6: * is_edp() => intel_dp_is_edp()
> * rebase
> v7: * Remove extraneous is_edp() prototype (Rodrigo)
>
> Cc: Chris Wilson
> Cc: Rodrigo Vivi
> Cc: Paulo Zanoni
> Cc: Manasi D Navare
> Cc: Mika Kahola
> Cc: Jani Nikula
> Fixes: 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")
> Signed-off-by: Jim Bride
> ---
> drivers/gpu/drm/i915/intel_dp.c | 2 ++
> drivers/gpu/drm/i915/intel_dp_link_training.c | 15 ++-
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> 3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e385658..38bc7e0 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4750,6 +4750,7 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
> intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
>
> intel_dp->reset_link_params = false;
> + intel_dp->train_set_valid = false;
> }
>
> intel_dp_print_rates(intel_dp);
> @@ -6019,6 +6020,7 @@ intel_dp_init_connector(struct intel_digital_port
> *intel_dig_port,
> intel_dp_set_source_rates(intel_dp);
>
> intel_dp->reset_link_params = true;
> + intel_dp->train_set_valid = false;
> intel_dp->pps_pipe = INVALID_PIPE;
> intel_dp->active_pipe = INVALID_PIPE;
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index 05907fa..79fe369 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -94,7 +94,8 @@ static bool
> intel_dp_reset_link_train(struct intel_dp *intel_dp,
> uint8_t dp_train_pat)
> {
> - memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
> + if (!intel_dp->train_set_valid)
> + memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
> intel_dp_set_signal_levels(intel_dp);
> return intel_dp_set_link_train(intel_dp, dp_train_pat);
> }
> @@ -162,9 +163,18 @@ intel_dp_link_training_clock_recovery(struct intel_dp
> *intel_dp)
> DP_TRAINING_PATTERN_1 |
> DP_LINK_SCRAMBLING_DISABLE)) {
> DRM_ERROR("failed to enable link training\n");
> + intel_dp->train_set_valid = false;
> return false;
> }
>
> + /*
> + * The initial set of link parameters are set by this point, so go
> + * ahead and set intel_dp->train_set_valid to false in case any of
> + * the succeeding steps fail. It will be set back to true if we were
> + * able to achieve clock recovery in the specified configuration.
> + */
> + intel_dp->train_set_valid = false;
> +
> voltage_tries = 1;
> max_vswing_tries = 0;
> for (;;) {
> @@ -179,6 +189,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp
> *intel_dp)
>
> if (drm_dp_clock_recovery_ok(link_st
Re: [Intel-gfx] [PATCH v7] drm/i915/edp: Be less aggressive about changing link config on eDP
On Fri, Sep 15, 2017 at 06:19:12PM +, Manasi Navare wrote:
> The patch looks good for eDP link training optimizations.
>
> Reviewed-by: Manasi Navare
I haven't merged this patch yet because I'd like an Ack from Jani.
Also I'd like to hear from Mika if he believes it is safe or not.
On his revert commit he wrote:
"It has been found out that in some HW combination the DisplayPort
fast link training feature caused screen flickering. Let's revert
this feature for now until we can ensure that the feature works for
all platforms."
I don't want to merge this patch to fix a feature that is disabled
by default with the risk of bringing flickerings back.
But even if we decide to go ahead and merge I believe we need to
resend the test and collect a full round of CI that now runs
all IGT tests.
Thanks,
Rodrigo.
>
> Manasi
>
> On Tue, Aug 22, 2017 at 09:34:46AM -0700, Jim Bride wrote:
> > This set of changes has some history to them. There were several attempts
> > to add what was called "fast link training" to i915, which actually wasn't
> > fast link training as per the DP spec. These changes were:
> >
> > commit 5fa836a9d859 ("drm/i915: DP link training optimization")
> > commit 4e96c97742f4 ("drm/i915: eDP link training optimization")
> >
> > which were eventually hand-reverted by:
> >
> > commit 34511dce4b35 ("drm/i915: Revert DisplayPort fast link training
> > feature")
> >
> > in kernel 4.7-rc4. The eDP pieces of the above revert, however, had some
> > very bad side-effects on PSR functionality on Skylake. The issue at
> > hand is that when PSR exits i915 briefly emits TP1 followed by TP2/3
> > (depending on the original link configuration) in order to quickly get
> > the source and sink back in synchronization across the link before handing
> > control back to the i915. There's an assumption that none of the link
> > configuration information has changed (and thus it's still valid) since the
> > last full link training operation. The revert above was identified via a
> > bisect as the cause of some of Skylake's PSR woes. This patch, largely
> > based on commit 4e96c97742f4 ("drm/i915: eDP link training optimization")
> > puts the eDP portions of this patch back in place. None of the flickering
> > issues that spurred the revert have been seen, and I suspect the real
> > culprits here were addressed by some of the recent link training changes
> > that Manasi has implemented, and PSR on Skylake is definitely more happy
> > with these changes in-place.
> >
> > v2 and v3: Rebase
> > v4: * Clean up accesses to train_set_valid a bit for easier
> > reading. (Chris)
> > * Rebase
> > v5: * Checkpatch cleanup
> > * Rebase
> > v6: * is_edp() => intel_dp_is_edp()
> > * rebase
> > v7: * Remove extraneous is_edp() prototype (Rodrigo)
> >
> > Cc: Chris Wilson
> > Cc: Rodrigo Vivi
> > Cc: Paulo Zanoni
> > Cc: Manasi D Navare
> > Cc: Mika Kahola
> > Cc: Jani Nikula
> > Fixes: 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")
> > Signed-off-by: Jim Bride
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 2 ++
> > drivers/gpu/drm/i915/intel_dp_link_training.c | 15 ++-
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > 3 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index e385658..38bc7e0 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4750,6 +4750,7 @@ intel_dp_long_pulse(struct intel_connector
> > *intel_connector)
> > intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
> >
> > intel_dp->reset_link_params = false;
> > + intel_dp->train_set_valid = false;
> > }
> >
> > intel_dp_print_rates(intel_dp);
> > @@ -6019,6 +6020,7 @@ intel_dp_init_connector(struct intel_digital_port
> > *intel_dig_port,
> > intel_dp_set_source_rates(intel_dp);
> >
> > intel_dp->reset_link_params = true;
> > + intel_dp->train_set_valid = false;
> > intel_dp->pps_pipe = INVALID_PIPE;
> > intel_dp->active_pipe = INVALID_PIPE;
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index 05907fa..79fe369 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -94,7 +94,8 @@ static bool
> > intel_dp_reset_link_train(struct intel_dp *intel_dp,
> > uint8_t dp_train_pat)
> > {
> > - memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
> > + if (!intel_dp->train_set_valid)
> > + memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
> > intel_dp_set_signal_levels(intel_dp);
> > return intel_dp_set_link_train(intel_dp, dp_train_pat);
> > }
> > @@ -162,9 +163,18 @@ intel_dp_link_training_clock_recovery(struct intel_
Re: [Intel-gfx] [PATCH v7] drm/i915/edp: Be less aggressive about changing link config on eDP
On Tue, Sep 19, 2017 at 12:55:24PM -0700, Rodrigo Vivi wrote:
> On Fri, Sep 15, 2017 at 06:19:12PM +, Manasi Navare wrote:
> > The patch looks good for eDP link training optimizations.
> >
> > Reviewed-by: Manasi Navare
>
> I haven't merged this patch yet because I'd like an Ack from Jani.
>
> Also I'd like to hear from Mika if he believes it is safe or not.
Mika looked at it a few months ago and thought it would be ok. It
can't hurt to have him look at it again, though.
> On his revert commit he wrote:
> "It has been found out that in some HW combination the DisplayPort
> fast link training feature caused screen flickering. Let's revert
> this feature for now until we can ensure that the feature works for
> all platforms."
>
> I don't want to merge this patch to fix a feature that is disabled
> by default with the risk of bringing flickerings back.
>
> But even if we decide to go ahead and merge I believe we need to
> resend the test and collect a full round of CI that now runs
> all IGT tests.
I assume you mean resend the patch here. I'll do that.
Jim
> Thanks,
> Rodrigo.
>
>
> >
> > Manasi
> >
> > On Tue, Aug 22, 2017 at 09:34:46AM -0700, Jim Bride wrote:
> > > This set of changes has some history to them. There were several attempts
> > > to add what was called "fast link training" to i915, which actually wasn't
> > > fast link training as per the DP spec. These changes were:
> > >
> > > commit 5fa836a9d859 ("drm/i915: DP link training optimization")
> > > commit 4e96c97742f4 ("drm/i915: eDP link training optimization")
> > >
> > > which were eventually hand-reverted by:
> > >
> > > commit 34511dce4b35 ("drm/i915: Revert DisplayPort fast link training
> > > feature")
> > >
> > > in kernel 4.7-rc4. The eDP pieces of the above revert, however, had some
> > > very bad side-effects on PSR functionality on Skylake. The issue at
> > > hand is that when PSR exits i915 briefly emits TP1 followed by TP2/3
> > > (depending on the original link configuration) in order to quickly get
> > > the source and sink back in synchronization across the link before handing
> > > control back to the i915. There's an assumption that none of the link
> > > configuration information has changed (and thus it's still valid) since
> > > the
> > > last full link training operation. The revert above was identified via a
> > > bisect as the cause of some of Skylake's PSR woes. This patch, largely
> > > based on commit 4e96c97742f4 ("drm/i915: eDP link training optimization")
> > > puts the eDP portions of this patch back in place. None of the flickering
> > > issues that spurred the revert have been seen, and I suspect the real
> > > culprits here were addressed by some of the recent link training changes
> > > that Manasi has implemented, and PSR on Skylake is definitely more happy
> > > with these changes in-place.
> > >
> > > v2 and v3: Rebase
> > > v4: * Clean up accesses to train_set_valid a bit for easier
> > > reading. (Chris)
> > > * Rebase
> > > v5: * Checkpatch cleanup
> > > * Rebase
> > > v6: * is_edp() => intel_dp_is_edp()
> > > * rebase
> > > v7: * Remove extraneous is_edp() prototype (Rodrigo)
> > >
> > > Cc: Chris Wilson
> > > Cc: Rodrigo Vivi
> > > Cc: Paulo Zanoni
> > > Cc: Manasi D Navare
> > > Cc: Mika Kahola
> > > Cc: Jani Nikula
> > > Fixes: 34511dce4 ("drm/i915: Revert DisplayPort fast link training
> > > feature")
> > > Signed-off-by: Jim Bride
> > > ---
> > > drivers/gpu/drm/i915/intel_dp.c | 2 ++
> > > drivers/gpu/drm/i915/intel_dp_link_training.c | 15 ++-
> > > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > > 3 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index e385658..38bc7e0 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4750,6 +4750,7 @@ intel_dp_long_pulse(struct intel_connector
> > > *intel_connector)
> > > intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
> > >
> > > intel_dp->reset_link_params = false;
> > > + intel_dp->train_set_valid = false;
> > > }
> > >
> > > intel_dp_print_rates(intel_dp);
> > > @@ -6019,6 +6020,7 @@ intel_dp_init_connector(struct intel_digital_port
> > > *intel_dig_port,
> > > intel_dp_set_source_rates(intel_dp);
> > >
> > > intel_dp->reset_link_params = true;
> > > + intel_dp->train_set_valid = false;
> > > intel_dp->pps_pipe = INVALID_PIPE;
> > > intel_dp->active_pipe = INVALID_PIPE;
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > index 05907fa..79fe369 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > @@ -94,7 +94,8 @@ static bool
> > > intel_dp_reset_link_train(stru
