Re: [Intel-gfx] [PATCH 3/4] drm/i915/edp: Be less aggressive about changing link config on eDP

2017-05-08 Thread Jim Bride 
On Mon, May 08, 2017 at 11:41:25AM +0300, Jani Nikula wrote:
> On Sat, 06 May 2017, 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.
> 
> I'm weary of all the back and forth wrt PSR and DP, and wary of creating
> new ones. Would it make sense to restrict this patch to PSR?

Honestly, although it could be limited to PSR, I think it's beneficial
to leave it in place as it is.  For eDP, where panels generally only
support a small number of native modes, I think we end up doing a lot
of extra work by re-calculating all of the link information every
time.  Mika seems think it's pretty safe as well.

Jim

> 
> BR,
> Jani.
> 
> 
> >
> > Cc: Rodrigo Vivi 
> > Cc: Paulo Zanoni 
> > Cc: Manasi D Navare 
> > Cc: Mika Kahola 
> > 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 d46f72d..06b8bd4 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);
> >  
> > @@ -4690,6 +4690,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);
> > @@ -6052,6 +6053,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

Re: [Intel-gfx] [PATCH 3/4] drm/i915/edp: Be less aggressive about changing link config on eDP

2017-05-08 Thread Mika Kahola 
On Fri, 2017-05-05 at 14:02 -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.
We reverted this back in the day due to bug reports we received from
the community concerning flickering. However, you have to remember that
during that time also the watermark work was ongoing. We couldn't
pinpoint one issue or root cause that triggered these reported
flickering cases. Therefore we decided to revert these optimizations.
Personally, I feel that this would have been safe for eDP case.

Acked-by: Mika Kahola 

> 
> Cc: Rodrigo Vivi 
> Cc: Paulo Zanoni 
> Cc: Manasi D Navare 
> Cc: Mika Kahola 
> 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 d46f72d..06b8bd4 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);
>  
> @@ -4690,6 +4690,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);
> @@ -6052,6 +6053,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");
> +

Re: [Intel-gfx] [PATCH 3/4] drm/i915/edp: Be less aggressive about changing link config on eDP

2017-05-08 Thread Jani Nikula 
On Sat, 06 May 2017, 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.

I'm weary of all the back and forth wrt PSR and DP, and wary of creating
new ones. Would it make sense to restrict this patch to PSR?

BR,
Jani.


>
> Cc: Rodrigo Vivi 
> Cc: Paulo Zanoni 
> Cc: Manasi D Navare 
> Cc: Mika Kahola 
> 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 d46f72d..06b8bd4 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);
>  
> @@ -4690,6 +4690,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);
> @@ -6052,6 +6053,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-gfx] [PATCH 3/4] drm/i915/edp: Be less aggressive about changing link config on eDP

2017-05-05 Thread Jim Bride 
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.

Cc: Rodrigo Vivi 
Cc: Paulo Zanoni 
Cc: Manasi D Navare 
Cc: Mika Kahola 
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 d46f72d..06b8bd4 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);
 
@@ -4690,6 +4690,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);
@@ -6052,6 +6053,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);
return true;
}