Re: [Intel-gfx] [PATCH v3 4/4] drm/i915/edp: Be less aggressive about changing link config on eDP
On Wed, Jul 12, 2017 at 02:53:36PM -0700, Manasi Navare wrote:
> On Wed, Jul 12, 2017 at 10:38:03PM +0100, Chris Wilson wrote:
> > Quoting Manasi Navare (2017-07-12 22:36:49)
> > > On Wed, Jul 12, 2017 at 12:16:13AM +0100, Chris Wilson wrote:
> > > > Quoting Jim Bride (2017-07-11 23:19:56)
> > > > > @@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct
> > > > > intel_dp *intel_dp)
> > > > >
> > > > > if (!intel_dp_get_link_status(intel_dp, link_status))
> > > > > {
> > > > > DRM_ERROR("failed to get link status\n");
> > > > > + intel_dp->train_set_valid = false;
> > > > > return false;
> > > > > }
> > > > >
> > > > > if (drm_dp_clock_recovery_ok(link_status,
> > > > > intel_dp->lane_count)) {
> > > > > DRM_DEBUG_KMS("clock recovery OK\n");
> > > > > + intel_dp->train_set_valid = is_edp(intel_dp);
> > > >
> > > > Ouch, that was hard to spot amongst the decoys. How about setting
> > > > intel_dp->train_set_valid = false at the very start of training, and
> > > > only on success set it to true, something like
> > > >
> > >
> > > Or like I suggested, just set train_set_valid to false in the
> > > failure_handling and set it to true only on success.
> >
> > It just looked a little crowded in the failure_handling: whereas at the
> > start of the function, there was plenty of whitespace for it to stand
> > out. That was all I was thinking.
> > -Chris
>
> But at the beginning of the function, it is anyway set to False since
> intel_dp_init_connector sets it to false.
> So we dont need to again set it to false at the beginning of the function
> right?
> Then we only set it to true when it succeeds so after 1 successful link
> training
> it will be set to true.
> And now since this code will also be used fro DP case, we need to make sure
> we set this to false in failure handling so that it resets link train during
> link train fallback.
> Makes sense?
What seems the easiest to me is to set intel_dp->train_set_valid
to false right before the main clock recovery loop (but after calling
intel_dp_reset_link_train(), which is what uses the value) in
intel_dp_link_training_clock_recovery(), and remove the
sets to false for the failing cases. If clock recovery is ok,
then we set it to true and return. This makes it much easier
to sort out when we succeed by reading the code without changing
any of the existing functionality. New patch coming as soon as I
get done testing it.
Jim
> Manasi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/4] drm/i915/edp: Be less aggressive about changing link config on eDP
On Wed, Jul 12, 2017 at 10:38:03PM +0100, Chris Wilson wrote:
> Quoting Manasi Navare (2017-07-12 22:36:49)
> > On Wed, Jul 12, 2017 at 12:16:13AM +0100, Chris Wilson wrote:
> > > Quoting Jim Bride (2017-07-11 23:19:56)
> > > > @@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct
> > > > intel_dp *intel_dp)
> > > >
> > > > if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > > > DRM_ERROR("failed to get link status\n");
> > > > + intel_dp->train_set_valid = false;
> > > > return false;
> > > > }
> > > >
> > > > if (drm_dp_clock_recovery_ok(link_status,
> > > > intel_dp->lane_count)) {
> > > > DRM_DEBUG_KMS("clock recovery OK\n");
> > > > + intel_dp->train_set_valid = is_edp(intel_dp);
> > >
> > > Ouch, that was hard to spot amongst the decoys. How about setting
> > > intel_dp->train_set_valid = false at the very start of training, and
> > > only on success set it to true, something like
> > >
> >
> > Or like I suggested, just set train_set_valid to false in the
> > failure_handling and set it to true only on success.
>
> It just looked a little crowded in the failure_handling: whereas at the
> start of the function, there was plenty of whitespace for it to stand
> out. That was all I was thinking.
> -Chris
But at the beginning of the function, it is anyway set to False since
intel_dp_init_connector sets it to false.
So we dont need to again set it to false at the beginning of the function right?
Then we only set it to true when it succeeds so after 1 successful link training
it will be set to true.
And now since this code will also be used fro DP case, we need to make sure
we set this to false in failure handling so that it resets link train during
link train fallback.
Makes sense?
Manasi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/4] drm/i915/edp: Be less aggressive about changing link config on eDP
Quoting Manasi Navare (2017-07-12 22:36:49)
> On Wed, Jul 12, 2017 at 12:16:13AM +0100, Chris Wilson wrote:
> > Quoting Jim Bride (2017-07-11 23:19:56)
> > > @@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct
> > > intel_dp *intel_dp)
> > >
> > > if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > > DRM_ERROR("failed to get link status\n");
> > > + intel_dp->train_set_valid = false;
> > > return false;
> > > }
> > >
> > > if (drm_dp_clock_recovery_ok(link_status,
> > > intel_dp->lane_count)) {
> > > DRM_DEBUG_KMS("clock recovery OK\n");
> > > + intel_dp->train_set_valid = is_edp(intel_dp);
> >
> > Ouch, that was hard to spot amongst the decoys. How about setting
> > intel_dp->train_set_valid = false at the very start of training, and
> > only on success set it to true, something like
> >
>
> Or like I suggested, just set train_set_valid to false in the
> failure_handling and set it to true only on success.
It just looked a little crowded in the failure_handling: whereas at the
start of the function, there was plenty of whitespace for it to stand
out. That was all I was thinking.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/4] drm/i915/edp: Be less aggressive about changing link config on eDP
On Wed, Jul 12, 2017 at 12:16:13AM +0100, Chris Wilson wrote:
> Quoting Jim Bride (2017-07-11 23:19:56)
> > @@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct intel_dp
> > *intel_dp)
> >
> > if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > DRM_ERROR("failed to get link status\n");
> > + intel_dp->train_set_valid = false;
> > return false;
> > }
> >
> > if (drm_dp_clock_recovery_ok(link_status,
> > intel_dp->lane_count)) {
> > DRM_DEBUG_KMS("clock recovery OK\n");
> > + intel_dp->train_set_valid = is_edp(intel_dp);
>
> Ouch, that was hard to spot amongst the decoys. How about setting
> intel_dp->train_set_valid = false at the very start of training, and
> only on success set it to true, something like
>
Or like I suggested, just set train_set_valid to false in the
failure_handling and set it to true only on success.
Manasi
> @@ -316,6 +316,8 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> {
> struct intel_connector *intel_connector =
> intel_dp->attached_connector;
>
> + intel_dp->train_set_valid = false;
> +
> if (!intel_dp_link_training_clock_recovery(intel_dp))
> goto failure_handling;
> if (!intel_dp_link_training_channel_equalization(intel_dp))
> @@ -323,6 +325,11 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>
> DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count =
> %d",
> intel_dp->link_rate, intel_dp->lane_count);
> + /*
> +* Record the last known working parameters for quick retraining
> +* next time. We only trust eDP for now.
> +*/
> + intel_dp->train_set_valid = is_edp(intel_dp);
> return;
>
> failure_handling:
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/4] drm/i915/edp: Be less aggressive about changing link config on eDP
On Tue, Jul 11, 2017 at 03:19:56PM -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
>
> 5fa836a9d859 ("drm/i915: DP link training optimization")
> 4e96c97742f4 ("drm/i915: eDP link training optimization")
>
> which were eventually hand-reverted by
>
> 34511dce4 ("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 4e96c97742f4201edf1b0f8e1b1b6b2ac6ff33e7
> Author: Mika Kahola
> Date: Wed Apr 29 09:17:39 2015 +0300
> 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: Rebase
>
> 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 | 4 +++-
> drivers/gpu/drm/i915/intel_dp_link_training.c | 11 ++-
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> 3 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index ee2a3db..b4cb5ba 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -106,7 +106,7 @@ static const int default_rates[] = { 162000, 27,
> 54 };
> * If a CPU or PCH DP output is attached to an eDP panel, this function
> * will return true, and false otherwise.
> */
> -static bool is_edp(struct intel_dp *intel_dp)
> +bool is_edp(struct intel_dp *intel_dp)
> {
> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>
> @@ -4764,6 +4764,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);
> @@ -6037,6 +6038,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 b79c1c0..60233e2 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,6 +163,7 @@ 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;
> }
>
> @@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct intel_dp
> *intel_dp)
>
> if (!intel_dp_get_link_status(intel_dp, link_status)) {
> DRM_ERROR("failed to get link status\n");
> + intel_dp->train_set_valid = false;
> return false;
> }
>
>
Re: [Intel-gfx] [PATCH v3 4/4] drm/i915/edp: Be less aggressive about changing link config on eDP
Quoting Jim Bride (2017-07-11 23:19:56)
> @@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct intel_dp
> *intel_dp)
>
> if (!intel_dp_get_link_status(intel_dp, link_status)) {
> DRM_ERROR("failed to get link status\n");
> + intel_dp->train_set_valid = false;
> return false;
> }
>
> if (drm_dp_clock_recovery_ok(link_status,
> intel_dp->lane_count)) {
> DRM_DEBUG_KMS("clock recovery OK\n");
> + intel_dp->train_set_valid = is_edp(intel_dp);
Ouch, that was hard to spot amongst the decoys. How about setting
intel_dp->train_set_valid = false at the very start of training, and
only on success set it to true, something like
@@ -316,6 +316,8 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
{
struct intel_connector *intel_connector = intel_dp->attached_connector;
+ intel_dp->train_set_valid = false;
+
if (!intel_dp_link_training_clock_recovery(intel_dp))
goto failure_handling;
if (!intel_dp_link_training_channel_equalization(intel_dp))
@@ -323,6 +325,11 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d",
intel_dp->link_rate, intel_dp->lane_count);
+ /*
+* Record the last known working parameters for quick retraining
+* next time. We only trust eDP for now.
+*/
+ intel_dp->train_set_valid = is_edp(intel_dp);
return;
failure_handling:
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 4/4] 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
5fa836a9d859 ("drm/i915: DP link training optimization")
4e96c97742f4 ("drm/i915: eDP link training optimization")
which were eventually hand-reverted by
34511dce4 ("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 4e96c97742f4201edf1b0f8e1b1b6b2ac6ff33e7
Author: Mika Kahola
Date: Wed Apr 29 09:17:39 2015 +0300
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: Rebase
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 | 4 +++-
drivers/gpu/drm/i915/intel_dp_link_training.c | 11 ++-
drivers/gpu/drm/i915/intel_drv.h | 2 ++
3 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ee2a3db..b4cb5ba 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -106,7 +106,7 @@ static const int default_rates[] = { 162000, 27, 54
};
* If a CPU or PCH DP output is attached to an eDP panel, this function
* will return true, and false otherwise.
*/
-static bool is_edp(struct intel_dp *intel_dp)
+bool is_edp(struct intel_dp *intel_dp)
{
struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -4764,6 +4764,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);
@@ -6037,6 +6038,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 b79c1c0..60233e2 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,6 +163,7 @@ 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;
}
@@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct intel_dp
*intel_dp)
if (!intel_dp_get_link_status(intel_dp, link_status)) {
DRM_ERROR("failed to get link status\n");
+ intel_dp->train_set_valid = false;
return false;
}
if (drm_dp_clock_recovery_ok(link_status,
intel_dp->lane_count)) {
DRM_DEBUG_KMS("clock recovery OK\n");
+ intel_dp->train_set_valid = is_edp(intel_dp);
