[PATCH 0/4] Kernel PSR Fix-ups

[PATCH 0/4] Kernel PSR Fix-ups

[Jim Bride]

These patches, along with an upcoming series for IGT, enable our
PSR IGT tests to run reliably once again.  The first change
enables us to run the PSR tests on SKL and KBL RVP platforms,
whose panels have too slow of a setup time when running in their
preferred mode.  The second fixes a minor problem with the way that
we were initializing SRD_CTL that caused us to clobber a bit that we
are not supposed to change in that register on SKL and KBL.  The third
change re-introduces some changes to our link training code to be less
aggressive about changing link state for eDP, because PSR depends on
the link state being the same at PSR exit as it was at PSR entry.
The fourth change greatly increases the reliability of reading the
sink CRC generated by the eDP panel.  

Jim Bride (4):
  drm/i915/edp: Allow alternate fixed mode for eDP if available.
  drm/i915/psr: Clean-up intel_enable_source_psr1()
  drm/i915/edp: Be less aggressive about changing link config on eDP
  drm/i915/psr: Account for sink CRC raciness on some panels

 drivers/gpu/drm/i915/i915_debugfs.c           | 14 +++-
 drivers/gpu/drm/i915/i915_reg.h               |  4 ++
 drivers/gpu/drm/i915/intel_dp.c               | 95 +++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_dp_link_training.c | 11 +++-
 drivers/gpu/drm/i915/intel_drv.h              |  4 ++
 drivers/gpu/drm/i915/intel_dsi.c              |  2 +-
 drivers/gpu/drm/i915/intel_dvo.c              |  2 +-
 drivers/gpu/drm/i915/intel_lvds.c             |  3 +-
 drivers/gpu/drm/i915/intel_panel.c            |  2 +
 drivers/gpu/drm/i915/intel_psr.c              | 21 +++++-
 10 files changed, 136 insertions(+), 22 deletions(-)

-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/4] drm/i915/edp: Allow alternate fixed mode for eDP if available.

[Jim Bride]

Some fixed resolution panels actually support more than one mode,
with the only thing different being the refresh rate.  Having this
alternate mode available to us is desirable, because it allows us to
test PSR on panels whose setup time at the preferred mode is too long.
With this patch we allow the use of the alternate mode if it's
available and it was specifically requested.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c    | 34 +++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_drv.h   |  2 ++
 drivers/gpu/drm/i915/intel_dsi.c   |  2 +-
 drivers/gpu/drm/i915/intel_dvo.c   |  2 +-
 drivers/gpu/drm/i915/intel_lvds.c  |  3 ++-
 drivers/gpu/drm/i915/intel_panel.c |  2 ++
 6 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 08834f7..d46f72d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1637,6 +1637,19 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 	return bpp;
 }
 
+static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
+				       struct drm_display_mode *m2)
+{
+	return (m1->hdisplay == m2->hdisplay &&
+		m1->hsync_start == m2->hsync_start &&
+		m1->hsync_end == m2->hsync_end &&
+		m1->htotal == m2->htotal &&
+		m1->vdisplay == m2->vdisplay &&
+		m1->vsync_start == m2->vsync_start &&
+		m1->vsync_end == m2->vsync_end &&
+		m1->vtotal == m2->vtotal);
+}
+
 bool
 intel_dp_compute_config(struct intel_encoder *encoder,
 			struct intel_crtc_state *pipe_config,
@@ -1674,8 +1687,16 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	pipe_config->has_audio = intel_dp->has_audio && port != PORT_A;
 
 	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
-		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
-				       adjusted_mode);
+		struct drm_display_mode *panel_mode =
+			intel_connector->panel.alt_fixed_mode;
+		struct drm_display_mode *req_mode = &pipe_config->base.mode;
+
+		if (!intel_edp_compare_alt_mode(req_mode, panel_mode))
+			panel_mode = intel_connector->panel.fixed_mode;
+
+		drm_mode_debug_printmodeline(panel_mode);
+
+		intel_fixed_panel_mode(panel_mode, adjusted_mode);
 
 		if (INTEL_GEN(dev_priv) >= 9) {
 			int ret;
@@ -5829,6 +5850,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	struct drm_device *dev = intel_encoder->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_display_mode *fixed_mode = NULL;
+	struct drm_display_mode *alt_fixed_mode = NULL;
 	struct drm_display_mode *downclock_mode = NULL;
 	bool has_dpcd;
 	struct drm_display_mode *scan;
@@ -5884,13 +5906,14 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	}
 	intel_connector->edid = edid;
 
-	/* prefer fixed mode from EDID if available */
+	/* prefer fixed mode from EDID if available, save an alt mode also */
 	list_for_each_entry(scan, &connector->probed_modes, head) {
 		if ((scan->type & DRM_MODE_TYPE_PREFERRED)) {
 			fixed_mode = drm_mode_duplicate(dev, scan);
 			downclock_mode = intel_dp_drrs_init(
 						intel_connector, fixed_mode);
-			break;
+		} else {
+			alt_fixed_mode = drm_mode_duplicate(dev, scan);
 		}
 	}
 
@@ -5927,7 +5950,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 			      pipe_name(pipe));
 	}
 
-	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
+	intel_panel_init(&intel_connector->panel, fixed_mode, alt_fixed_mode,
+			 downclock_mode);
 	intel_connector->panel.backlight.power = intel_edp_backlight_power;
 	intel_panel_setup_backlight(connector, pipe);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 54f3ff8..19d0c8f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -265,6 +265,7 @@ struct intel_encoder {
 
 struct intel_panel {
 	struct drm_display_mode *fixed_mode;
+	struct drm_display_mode *alt_fixed_mode;
 	struct drm_display_mode *downclock_mode;
 	int fitting_mode;
 
@@ -1676,6 +1677,7 @@ void intel_overlay_reset(struct drm_i915_private *dev_priv);
 /* intel_panel.c */
 int intel_panel_init(struct intel_panel *panel,
 		     struct drm_display_mode *fixed_mode,
+		     struct drm_display_mode *alt_fixed_mode,
 		     struct drm_display_mode *downclock_mode);
 void intel_panel_fini(struct intel_panel *panel);
 void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index fc0ef49..a938f08 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -1852,7 +1852,7 @@ void intel_dsi_init(struct drm_i915_private *dev_priv)
 	connector->display_info.width_mm = fixed_mode->width_mm;
 	connector->display_info.height_mm = fixed_mode->height_mm;
 
-	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
+	intel_panel_init(&intel_connector->panel, fixed_mode, NULL, NULL);
 	intel_panel_setup_backlight(connector, INVALID_PIPE);
 
 	intel_dsi_add_properties(intel_connector);
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index c1544a5..39fd4f3 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -554,7 +554,7 @@ void intel_dvo_init(struct drm_i915_private *dev_priv)
 			 */
 			intel_panel_init(&intel_connector->panel,
 					 intel_dvo_get_current_mode(connector),
-					 NULL);
+					 NULL, NULL);
 			intel_dvo->panel_wants_dither = true;
 		}
 
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 8b942ef..b1e6743 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -1181,7 +1181,8 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
 out:
 	mutex_unlock(&dev->mode_config.mutex);
 
-	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
+	intel_panel_init(&intel_connector->panel, fixed_mode, NULL,
+			 downclock_mode);
 	intel_panel_setup_backlight(connector, INVALID_PIPE);
 
 	lvds_encoder->is_dual_link = compute_is_dual_link_lvds(lvds_encoder);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index cb50c52..41eb7b0 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1801,11 +1801,13 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel)
 
 int intel_panel_init(struct intel_panel *panel,
 		     struct drm_display_mode *fixed_mode,
+		     struct drm_display_mode *alt_fixed_mode,
 		     struct drm_display_mode *downclock_mode)
 {
 	intel_panel_init_backlight_funcs(panel);
 
 	panel->fixed_mode = fixed_mode;
+	panel->alt_fixed_mode = alt_fixed_mode;
 	panel->downclock_mode = downclock_mode;
 
 	return 0;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915/edp: Allow alternate fixed mode for eDP if available.

[Jani Nikula]

On Sat, 06 May 2017, Jim Bride <jim.bride@linux.intel.com> wrote:
> Some fixed resolution panels actually support more than one mode,
> with the only thing different being the refresh rate.  Having this
> alternate mode available to us is desirable, because it allows us to
> test PSR on panels whose setup time at the preferred mode is too long.
> With this patch we allow the use of the alternate mode if it's
> available and it was specifically requested.

All in all this feels like a hack. The generic solution would be to
allow any mode to be set again.

A few specific comments inline.

BR,
Jani.

> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c    | 34 +++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
>  drivers/gpu/drm/i915/intel_dsi.c   |  2 +-
>  drivers/gpu/drm/i915/intel_dvo.c   |  2 +-
>  drivers/gpu/drm/i915/intel_lvds.c  |  3 ++-
>  drivers/gpu/drm/i915/intel_panel.c |  2 ++
>  6 files changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 08834f7..d46f72d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1637,6 +1637,19 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  	return bpp;
>  }
>  
> +static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
> +				       struct drm_display_mode *m2)
> +{
> +	return (m1->hdisplay == m2->hdisplay &&
> +		m1->hsync_start == m2->hsync_start &&
> +		m1->hsync_end == m2->hsync_end &&
> +		m1->htotal == m2->htotal &&
> +		m1->vdisplay == m2->vdisplay &&
> +		m1->vsync_start == m2->vsync_start &&
> +		m1->vsync_end == m2->vsync_end &&
> +		m1->vtotal == m2->vtotal);
> +}

See drm_mode_equal and friends.

> +
>  bool
>  intel_dp_compute_config(struct intel_encoder *encoder,
>  			struct intel_crtc_state *pipe_config,
> @@ -1674,8 +1687,16 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	pipe_config->has_audio = intel_dp->has_audio && port != PORT_A;
>  
>  	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
> -		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
> -				       adjusted_mode);
> +		struct drm_display_mode *panel_mode =
> +			intel_connector->panel.alt_fixed_mode;
> +		struct drm_display_mode *req_mode = &pipe_config->base.mode;
> +
> +		if (!intel_edp_compare_alt_mode(req_mode, panel_mode))
> +			panel_mode = intel_connector->panel.fixed_mode;
> +
> +		drm_mode_debug_printmodeline(panel_mode);
> +
> +		intel_fixed_panel_mode(panel_mode, adjusted_mode);
>  
>  		if (INTEL_GEN(dev_priv) >= 9) {
>  			int ret;
> @@ -5829,6 +5850,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	struct drm_device *dev = intel_encoder->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_display_mode *fixed_mode = NULL;
> +	struct drm_display_mode *alt_fixed_mode = NULL;
>  	struct drm_display_mode *downclock_mode = NULL;
>  	bool has_dpcd;
>  	struct drm_display_mode *scan;
> @@ -5884,13 +5906,14 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	}
>  	intel_connector->edid = edid;
>  
> -	/* prefer fixed mode from EDID if available */
> +	/* prefer fixed mode from EDID if available, save an alt mode also */
>  	list_for_each_entry(scan, &connector->probed_modes, head) {
>  		if ((scan->type & DRM_MODE_TYPE_PREFERRED)) {
>  			fixed_mode = drm_mode_duplicate(dev, scan);
>  			downclock_mode = intel_dp_drrs_init(
>  						intel_connector, fixed_mode);
> -			break;
> +		} else {
> +			alt_fixed_mode = drm_mode_duplicate(dev, scan);
>  		}

This changes the fixed mode if there are more than one preferred
mode. This will also leak all but the two modes.

>  	}
>  
> @@ -5927,7 +5950,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  			      pipe_name(pipe));
>  	}
>  
> -	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
> +	intel_panel_init(&intel_connector->panel, fixed_mode, alt_fixed_mode,
> +			 downclock_mode);
>  	intel_connector->panel.backlight.power = intel_edp_backlight_power;
>  	intel_panel_setup_backlight(connector, pipe);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 54f3ff8..19d0c8f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -265,6 +265,7 @@ struct intel_encoder {
>  
>  struct intel_panel {
>  	struct drm_display_mode *fixed_mode;
> +	struct drm_display_mode *alt_fixed_mode;
>  	struct drm_display_mode *downclock_mode;
>  	int fitting_mode;
>  
> @@ -1676,6 +1677,7 @@ void intel_overlay_reset(struct drm_i915_private *dev_priv);
>  /* intel_panel.c */
>  int intel_panel_init(struct intel_panel *panel,
>  		     struct drm_display_mode *fixed_mode,
> +		     struct drm_display_mode *alt_fixed_mode,
>  		     struct drm_display_mode *downclock_mode);
>  void intel_panel_fini(struct intel_panel *panel);
>  void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index fc0ef49..a938f08 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -1852,7 +1852,7 @@ void intel_dsi_init(struct drm_i915_private *dev_priv)
>  	connector->display_info.width_mm = fixed_mode->width_mm;
>  	connector->display_info.height_mm = fixed_mode->height_mm;
>  
> -	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
> +	intel_panel_init(&intel_connector->panel, fixed_mode, NULL, NULL);
>  	intel_panel_setup_backlight(connector, INVALID_PIPE);
>  
>  	intel_dsi_add_properties(intel_connector);
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index c1544a5..39fd4f3 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -554,7 +554,7 @@ void intel_dvo_init(struct drm_i915_private *dev_priv)
>  			 */
>  			intel_panel_init(&intel_connector->panel,
>  					 intel_dvo_get_current_mode(connector),
> -					 NULL);
> +					 NULL, NULL);
>  			intel_dvo->panel_wants_dither = true;
>  		}
>  
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 8b942ef..b1e6743 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -1181,7 +1181,8 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
>  out:
>  	mutex_unlock(&dev->mode_config.mutex);
>  
> -	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
> +	intel_panel_init(&intel_connector->panel, fixed_mode, NULL,
> +			 downclock_mode);
>  	intel_panel_setup_backlight(connector, INVALID_PIPE);
>  
>  	lvds_encoder->is_dual_link = compute_is_dual_link_lvds(lvds_encoder);
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index cb50c52..41eb7b0 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1801,11 +1801,13 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel)
>  
>  int intel_panel_init(struct intel_panel *panel,
>  		     struct drm_display_mode *fixed_mode,
> +		     struct drm_display_mode *alt_fixed_mode,
>  		     struct drm_display_mode *downclock_mode)
>  {
>  	intel_panel_init_backlight_funcs(panel);
>  
>  	panel->fixed_mode = fixed_mode;
> +	panel->alt_fixed_mode = alt_fixed_mode;
>  	panel->downclock_mode = downclock_mode;
>  
>  	return 0;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915/edp: Allow alternate fixed mode for eDP if available.

[Jim Bride]

On Mon, May 08, 2017 at 11:54:15AM +0300, Jani Nikula wrote:
> On Sat, 06 May 2017, Jim Bride <jim.bride@linux.intel.com> wrote:
> > Some fixed resolution panels actually support more than one mode,
> > with the only thing different being the refresh rate.  Having this
> > alternate mode available to us is desirable, because it allows us to
> > test PSR on panels whose setup time at the preferred mode is too long.
> > With this patch we allow the use of the alternate mode if it's
> > available and it was specifically requested.
> 
> All in all this feels like a hack. The generic solution would be to
> allow any mode to be set again.

To an extent, I agree with you.  I did things this way in an attempt
to change the current behavior as little as possible.  Personally,
I'd rather see us allow any supported mode to be set.

> A few specific comments inline.
> 
> BR,
> Jani.
> 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c    | 34 +++++++++++++++++++++++++++++-----
> >  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
> >  drivers/gpu/drm/i915/intel_dsi.c   |  2 +-
> >  drivers/gpu/drm/i915/intel_dvo.c   |  2 +-
> >  drivers/gpu/drm/i915/intel_lvds.c  |  3 ++-
> >  drivers/gpu/drm/i915/intel_panel.c |  2 ++
> >  6 files changed, 37 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 08834f7..d46f72d 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1637,6 +1637,19 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
> >  	return bpp;
> >  }
> >  
> > +static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
> > +				       struct drm_display_mode *m2)
> > +{
> > +	return (m1->hdisplay == m2->hdisplay &&
> > +		m1->hsync_start == m2->hsync_start &&
> > +		m1->hsync_end == m2->hsync_end &&
> > +		m1->htotal == m2->htotal &&
> > +		m1->vdisplay == m2->vdisplay &&
> > +		m1->vsync_start == m2->vsync_start &&
> > +		m1->vsync_end == m2->vsync_end &&
> > +		m1->vtotal == m2->vtotal);
> > +}
> 
> See drm_mode_equal and friends.

I did.  The problem is that I needed a comparison without vscan being
involved (see drm_mode_equal_no_clocks_no_stereo(), where the actual
comparison happens.)  This seemed like the least disruptive way to do
that comparison.

> 
> > +
> >  bool
> >  intel_dp_compute_config(struct intel_encoder *encoder,
> >  			struct intel_crtc_state *pipe_config,
> > @@ -1674,8 +1687,16 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >  	pipe_config->has_audio = intel_dp->has_audio && port != PORT_A;
> >  
> >  	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
> > -		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
> > -				       adjusted_mode);
> > +		struct drm_display_mode *panel_mode =
> > +			intel_connector->panel.alt_fixed_mode;
> > +		struct drm_display_mode *req_mode = &pipe_config->base.mode;
> > +
> > +		if (!intel_edp_compare_alt_mode(req_mode, panel_mode))
> > +			panel_mode = intel_connector->panel.fixed_mode;
> > +
> > +		drm_mode_debug_printmodeline(panel_mode);
> > +
> > +		intel_fixed_panel_mode(panel_mode, adjusted_mode);
> >  
> >  		if (INTEL_GEN(dev_priv) >= 9) {
> >  			int ret;
> > @@ -5829,6 +5850,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >  	struct drm_device *dev = intel_encoder->base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct drm_display_mode *fixed_mode = NULL;
> > +	struct drm_display_mode *alt_fixed_mode = NULL;
> >  	struct drm_display_mode *downclock_mode = NULL;
> >  	bool has_dpcd;
> >  	struct drm_display_mode *scan;
> > @@ -5884,13 +5906,14 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >  	}
> >  	intel_connector->edid = edid;
> >  
> > -	/* prefer fixed mode from EDID if available */
> > +	/* prefer fixed mode from EDID if available, save an alt mode also */
> >  	list_for_each_entry(scan, &connector->probed_modes, head) {
> >  		if ((scan->type & DRM_MODE_TYPE_PREFERRED)) {
> >  			fixed_mode = drm_mode_duplicate(dev, scan);
> >  			downclock_mode = intel_dp_drrs_init(
> >  						intel_connector, fixed_mode);
> > -			break;
> > +		} else {
> > +			alt_fixed_mode = drm_mode_duplicate(dev, scan);
> >  		}
> 
> This changes the fixed mode if there are more than one preferred
> mode. This will also leak all but the two modes.

I wasn't aware there can be more than one preferred mode.  I thought
the whole idea was to have a way of designating one mode of many
that would provide (at least in the opinion of the panel's makers)
the mode that will provide the best user experience.  FWIW, on the
panels I've tested with I haven't seen more than two modes supported
natively in any case.  If we follow your earlier suggestion about
just letting mode sets happen on all supported modes then all of this
stuff goes away.


Jim

> 
> >  	}
> >  
> > @@ -5927,7 +5950,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >  			      pipe_name(pipe));
> >  	}
> >  
> > -	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
> > +	intel_panel_init(&intel_connector->panel, fixed_mode, alt_fixed_mode,
> > +			 downclock_mode);
> >  	intel_connector->panel.backlight.power = intel_edp_backlight_power;
> >  	intel_panel_setup_backlight(connector, pipe);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 54f3ff8..19d0c8f 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -265,6 +265,7 @@ struct intel_encoder {
> >  
> >  struct intel_panel {
> >  	struct drm_display_mode *fixed_mode;
> > +	struct drm_display_mode *alt_fixed_mode;
> >  	struct drm_display_mode *downclock_mode;
> >  	int fitting_mode;
> >  
> > @@ -1676,6 +1677,7 @@ void intel_overlay_reset(struct drm_i915_private *dev_priv);
> >  /* intel_panel.c */
> >  int intel_panel_init(struct intel_panel *panel,
> >  		     struct drm_display_mode *fixed_mode,
> > +		     struct drm_display_mode *alt_fixed_mode,
> >  		     struct drm_display_mode *downclock_mode);
> >  void intel_panel_fini(struct intel_panel *panel);
> >  void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > index fc0ef49..a938f08 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -1852,7 +1852,7 @@ void intel_dsi_init(struct drm_i915_private *dev_priv)
> >  	connector->display_info.width_mm = fixed_mode->width_mm;
> >  	connector->display_info.height_mm = fixed_mode->height_mm;
> >  
> > -	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
> > +	intel_panel_init(&intel_connector->panel, fixed_mode, NULL, NULL);
> >  	intel_panel_setup_backlight(connector, INVALID_PIPE);
> >  
> >  	intel_dsi_add_properties(intel_connector);
> > diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> > index c1544a5..39fd4f3 100644
> > --- a/drivers/gpu/drm/i915/intel_dvo.c
> > +++ b/drivers/gpu/drm/i915/intel_dvo.c
> > @@ -554,7 +554,7 @@ void intel_dvo_init(struct drm_i915_private *dev_priv)
> >  			 */
> >  			intel_panel_init(&intel_connector->panel,
> >  					 intel_dvo_get_current_mode(connector),
> > -					 NULL);
> > +					 NULL, NULL);
> >  			intel_dvo->panel_wants_dither = true;
> >  		}
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 8b942ef..b1e6743 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -1181,7 +1181,8 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
> >  out:
> >  	mutex_unlock(&dev->mode_config.mutex);
> >  
> > -	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
> > +	intel_panel_init(&intel_connector->panel, fixed_mode, NULL,
> > +			 downclock_mode);
> >  	intel_panel_setup_backlight(connector, INVALID_PIPE);
> >  
> >  	lvds_encoder->is_dual_link = compute_is_dual_link_lvds(lvds_encoder);
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index cb50c52..41eb7b0 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -1801,11 +1801,13 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel)
> >  
> >  int intel_panel_init(struct intel_panel *panel,
> >  		     struct drm_display_mode *fixed_mode,
> > +		     struct drm_display_mode *alt_fixed_mode,
> >  		     struct drm_display_mode *downclock_mode)
> >  {
> >  	intel_panel_init_backlight_funcs(panel);
> >  
> >  	panel->fixed_mode = fixed_mode;
> > +	panel->alt_fixed_mode = alt_fixed_mode;
> >  	panel->downclock_mode = downclock_mode;
> >  
> >  	return 0;
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915/psr: Clean-up intel_enable_source_psr1()

[Jim Bride]

On SKL+ there is a bit in SRD_CTL that software is not supposed to
modify, but we currently clobber that bit when we enable PSR.  In
order to preserve the value of that bit, go ahead and read SRD_CTL and
do a field-wise setting of the various bits that we need to initialize
before writing the register back out.  Additionally, go ahead and
explicitly disable single-frame update since we aren't currently
supporting it.

v2: Do a field-wise init on EDP_PSR_MAX_SLEEP_TIME even though we
    always set it to the max value. (Rodrigo)

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Wayne Boyer <wayne.boyer@intel.com>
Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  4 ++++
 drivers/gpu/drm/i915/intel_psr.c | 21 +++++++++++++++++++--
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ee8170c..3a63555 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3584,18 +3584,22 @@ enum {
 #define   EDP_PSR_MIN_LINK_ENTRY_TIME_4_LINES	(1<<25)
 #define   EDP_PSR_MIN_LINK_ENTRY_TIME_2_LINES	(2<<25)
 #define   EDP_PSR_MIN_LINK_ENTRY_TIME_0_LINES	(3<<25)
+#define   EDP_PSR_MAX_SLEEP_TIME_MASK           (0x1f<<20)
 #define   EDP_PSR_MAX_SLEEP_TIME_SHIFT		20
 #define   EDP_PSR_SKIP_AUX_EXIT			(1<<12)
 #define   EDP_PSR_TP1_TP2_SEL			(0<<11)
 #define   EDP_PSR_TP1_TP3_SEL			(1<<11)
+#define   EDP_PSR_TP2_TP3_TIME_MASK             (3<<8)
 #define   EDP_PSR_TP2_TP3_TIME_500us		(0<<8)
 #define   EDP_PSR_TP2_TP3_TIME_100us		(1<<8)
 #define   EDP_PSR_TP2_TP3_TIME_2500us		(2<<8)
 #define   EDP_PSR_TP2_TP3_TIME_0us		(3<<8)
+#define   EDP_PSR_TP1_TIME_MASK                 (0x3<<4)
 #define   EDP_PSR_TP1_TIME_500us		(0<<4)
 #define   EDP_PSR_TP1_TIME_100us		(1<<4)
 #define   EDP_PSR_TP1_TIME_2500us		(2<<4)
 #define   EDP_PSR_TP1_TIME_0us			(3<<4)
+#define   EDP_PSR_IDLE_FRAME_MASK               (0xf<<0)
 #define   EDP_PSR_IDLE_FRAME_SHIFT		0
 
 #define EDP_PSR_AUX_CTL				_MMIO(dev_priv->psr_mmio_base + 0x10)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index c3780d0..068c382 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -280,17 +280,32 @@ static void intel_enable_source_psr1(struct intel_dp *intel_dp)
 	 * with the 5 or 6 idle patterns.
 	 */
 	uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
-	uint32_t val = EDP_PSR_ENABLE;
+	uint32_t val = I915_READ(EDP_PSR_CTL);
 
+	val |= EDP_PSR_ENABLE;
+
+	val &= ~EDP_PSR_MAX_SLEEP_TIME_MASK;
 	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
+
+	val &= ~EDP_PSR_IDLE_FRAME_MASK;
 	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
 
+	val &= ~EDP_PSR_MIN_LINK_ENTRY_TIME_MASK;
 	if (IS_HASWELL(dev_priv))
 		val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
 
-	if (dev_priv->psr.link_standby)
+	if (dev_priv->psr.link_standby) {
 		val |= EDP_PSR_LINK_STANDBY;
 
+		/* SFU should only be enabled with link standby, but for
+		 * now we do not support it. */
+		val &= ~BDW_PSR_SINGLE_FRAME;
+	} else {
+		val &= ~EDP_PSR_LINK_STANDBY;
+		val &= ~BDW_PSR_SINGLE_FRAME;
+	}
+
+	val &= ~EDP_PSR_TP1_TIME_MASK;
 	if (dev_priv->vbt.psr.tp1_wakeup_time > 5)
 		val |= EDP_PSR_TP1_TIME_2500us;
 	else if (dev_priv->vbt.psr.tp1_wakeup_time > 1)
@@ -300,6 +315,7 @@ static void intel_enable_source_psr1(struct intel_dp *intel_dp)
 	else
 		val |= EDP_PSR_TP1_TIME_0us;
 
+	val &= ~EDP_PSR_TP2_TP3_TIME_MASK;
 	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
 		val |= EDP_PSR_TP2_TP3_TIME_2500us;
 	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
@@ -309,6 +325,7 @@ static void intel_enable_source_psr1(struct intel_dp *intel_dp)
 	else
 		val |= EDP_PSR_TP2_TP3_TIME_0us;
 
+	val &= ~EDP_PSR_TP1_TP3_SEL;
 	if (intel_dp_source_supports_hbr2(intel_dp) &&
 	    drm_dp_tps3_supported(intel_dp->dpcd))
 		val |= EDP_PSR_TP1_TP3_SEL;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915/psr: Clean-up intel_enable_source_psr1()

[Rodrigo Vivi]

On Fri, May 5, 2017 at 2:02 PM, Jim Bride <jim.bride@linux.intel.com> wrote:
> On SKL+ there is a bit in SRD_CTL that software is not supposed to
> modify, but we currently clobber that bit when we enable PSR.  In
> order to preserve the value of that bit, go ahead and read SRD_CTL and
> do a field-wise setting of the various bits that we need to initialize
> before writing the register back out.  Additionally, go ahead and
> explicitly disable single-frame update since we aren't currently
> supporting it.
>
> v2: Do a field-wise init on EDP_PSR_MAX_SLEEP_TIME even though we
>     always set it to the max value. (Rodrigo)
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Wayne Boyer <wayne.boyer@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  4 ++++
>  drivers/gpu/drm/i915/intel_psr.c | 21 +++++++++++++++++++--
>  2 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index ee8170c..3a63555 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3584,18 +3584,22 @@ enum {
>  #define   EDP_PSR_MIN_LINK_ENTRY_TIME_4_LINES  (1<<25)
>  #define   EDP_PSR_MIN_LINK_ENTRY_TIME_2_LINES  (2<<25)
>  #define   EDP_PSR_MIN_LINK_ENTRY_TIME_0_LINES  (3<<25)
> +#define   EDP_PSR_MAX_SLEEP_TIME_MASK           (0x1f<<20)
>  #define   EDP_PSR_MAX_SLEEP_TIME_SHIFT         20
>  #define   EDP_PSR_SKIP_AUX_EXIT                        (1<<12)
>  #define   EDP_PSR_TP1_TP2_SEL                  (0<<11)
>  #define   EDP_PSR_TP1_TP3_SEL                  (1<<11)
> +#define   EDP_PSR_TP2_TP3_TIME_MASK             (3<<8)
>  #define   EDP_PSR_TP2_TP3_TIME_500us           (0<<8)
>  #define   EDP_PSR_TP2_TP3_TIME_100us           (1<<8)
>  #define   EDP_PSR_TP2_TP3_TIME_2500us          (2<<8)
>  #define   EDP_PSR_TP2_TP3_TIME_0us             (3<<8)
> +#define   EDP_PSR_TP1_TIME_MASK                 (0x3<<4)
>  #define   EDP_PSR_TP1_TIME_500us               (0<<4)
>  #define   EDP_PSR_TP1_TIME_100us               (1<<4)
>  #define   EDP_PSR_TP1_TIME_2500us              (2<<4)
>  #define   EDP_PSR_TP1_TIME_0us                 (3<<4)
> +#define   EDP_PSR_IDLE_FRAME_MASK               (0xf<<0)
>  #define   EDP_PSR_IDLE_FRAME_SHIFT             0
>
>  #define EDP_PSR_AUX_CTL                                _MMIO(dev_priv->psr_mmio_base + 0x10)
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index c3780d0..068c382 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -280,17 +280,32 @@ static void intel_enable_source_psr1(struct intel_dp *intel_dp)
>          * with the 5 or 6 idle patterns.
>          */
>         uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> -       uint32_t val = EDP_PSR_ENABLE;
> +       uint32_t val = I915_READ(EDP_PSR_CTL);
>
> +       val |= EDP_PSR_ENABLE;
> +
> +       val &= ~EDP_PSR_MAX_SLEEP_TIME_MASK;
>         val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
> +
> +       val &= ~EDP_PSR_IDLE_FRAME_MASK;
>         val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
>
> +       val &= ~EDP_PSR_MIN_LINK_ENTRY_TIME_MASK;
>         if (IS_HASWELL(dev_priv))
>                 val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
>
> -       if (dev_priv->psr.link_standby)
> +       if (dev_priv->psr.link_standby) {
>                 val |= EDP_PSR_LINK_STANDBY;
>
> +               /* SFU should only be enabled with link standby, but for
> +                * now we do not support it. */

/* goes to next line

> +               val &= ~BDW_PSR_SINGLE_FRAME;
> +       } else {
> +               val &= ~EDP_PSR_LINK_STANDBY;
> +               val &= ~BDW_PSR_SINGLE_FRAME;

probably better to do this out of the if to avoid duplication. Comment above
already explain it well.

with these feel free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +       }
> +
> +       val &= ~EDP_PSR_TP1_TIME_MASK;
>         if (dev_priv->vbt.psr.tp1_wakeup_time > 5)
>                 val |= EDP_PSR_TP1_TIME_2500us;
>         else if (dev_priv->vbt.psr.tp1_wakeup_time > 1)
> @@ -300,6 +315,7 @@ static void intel_enable_source_psr1(struct intel_dp *intel_dp)
>         else
>                 val |= EDP_PSR_TP1_TIME_0us;
>
> +       val &= ~EDP_PSR_TP2_TP3_TIME_MASK;
>         if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
>                 val |= EDP_PSR_TP2_TP3_TIME_2500us;
>         else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
> @@ -309,6 +325,7 @@ static void intel_enable_source_psr1(struct intel_dp *intel_dp)
>         else
>                 val |= EDP_PSR_TP2_TP3_TIME_0us;
>
> +       val &= ~EDP_PSR_TP1_TP3_SEL;
>         if (intel_dp_source_supports_hbr2(intel_dp) &&
>             drm_dp_tps3_supported(intel_dp->dpcd))
>                 val |= EDP_PSR_TP1_TP3_SEL;
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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 <mika.kahola@intel.com>
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 <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Manasi D Navare <manasi.d.navare@intel.com>
Cc: Mika Kahola <mika.kahola@intel.com>
Fixes: 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")
Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
---
 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, 270000, 540000 };
  * 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;
 		}
 
 		if (voltage_tries == 5) {
 			DRM_DEBUG_KMS("Same voltage tried 5 times\n");
+			intel_dp->train_set_valid = false;
 			return false;
 		}
 
 		if (max_vswing_tries == 1) {
 			DRM_DEBUG_KMS("Max Voltage Swing reached\n");
+			intel_dp->train_set_valid = false;
 			return false;
 		}
 
@@ -198,6 +204,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 		intel_get_adjust_train(intel_dp, link_status);
 		if (!intel_dp_update_link_train(intel_dp)) {
 			DRM_ERROR("failed to update link training\n");
+			intel_dp->train_set_valid = false;
 			return false;
 		}
 
@@ -256,6 +263,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 				     training_pattern |
 				     DP_LINK_SCRAMBLING_DISABLE)) {
 		DRM_ERROR("failed to start channel equalization\n");
+		intel_dp->train_set_valid = false;
 		return false;
 	}
 
@@ -296,6 +304,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 	/* Try 5 times, else fail and try at lower BW */
 	if (tries == 5) {
 		intel_dp_dump_link_status(link_status);
+		intel_dp->train_set_valid = false;
 		DRM_DEBUG_KMS("Channel equalization failed 5 times\n");
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 19d0c8f..149d4c9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -977,6 +977,7 @@ struct intel_dp {
 	struct drm_dp_aux aux;
 	enum intel_display_power_domain aux_power_domain;
 	uint8_t train_set[4];
+	bool train_set_valid;
 	int panel_power_up_delay;
 	int panel_power_down_delay;
 	int panel_power_cycle_delay;
@@ -1551,6 +1552,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
 bool __intel_dp_read_desc(struct intel_dp *intel_dp,
 			  struct intel_dp_desc *desc);
 bool intel_dp_read_desc(struct intel_dp *intel_dp);
+bool is_edp(struct intel_dp *intel_dp);
 int intel_dp_link_required(int pixel_clock, int bpp);
 int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
 bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Sat, 06 May 2017, Jim Bride <jim.bride@linux.intel.com> 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 <mika.kahola@intel.com>
> 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 <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Manasi D Navare <manasi.d.navare@intel.com>
> Cc: Mika Kahola <mika.kahola@intel.com>
> Fixes: 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  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, 270000, 540000 };
>   * 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;
>  		}
>  
>  		if (voltage_tries == 5) {
>  			DRM_DEBUG_KMS("Same voltage tried 5 times\n");
> +			intel_dp->train_set_valid = false;
>  			return false;
>  		}
>  
>  		if (max_vswing_tries == 1) {
>  			DRM_DEBUG_KMS("Max Voltage Swing reached\n");
> +			intel_dp->train_set_valid = false;
>  			return false;
>  		}
>  
> @@ -198,6 +204,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>  		intel_get_adjust_train(intel_dp, link_status);
>  		if (!intel_dp_update_link_train(intel_dp)) {
>  			DRM_ERROR("failed to update link training\n");
> +			intel_dp->train_set_valid = false;
>  			return false;
>  		}
>  
> @@ -256,6 +263,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>  				     training_pattern |
>  				     DP_LINK_SCRAMBLING_DISABLE)) {
>  		DRM_ERROR("failed to start channel equalization\n");
> +		intel_dp->train_set_valid = false;
>  		return false;
>  	}
>  
> @@ -296,6 +304,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>  	/* Try 5 times, else fail and try at lower BW */
>  	if (tries == 5) {
>  		intel_dp_dump_link_status(link_status);
> +		intel_dp->train_set_valid = false;
>  		DRM_DEBUG_KMS("Channel equalization failed 5 times\n");
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 19d0c8f..149d4c9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -977,6 +977,7 @@ struct intel_dp {
>  	struct drm_dp_aux aux;
>  	enum intel_display_power_domain aux_power_domain;
>  	uint8_t train_set[4];
> +	bool train_set_valid;
>  	int panel_power_up_delay;
>  	int panel_power_down_delay;
>  	int panel_power_cycle_delay;
> @@ -1551,6 +1552,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
>  bool __intel_dp_read_desc(struct intel_dp *intel_dp,
>  			  struct intel_dp_desc *desc);
>  bool intel_dp_read_desc(struct intel_dp *intel_dp);
> +bool is_edp(struct intel_dp *intel_dp);
>  int intel_dp_link_required(int pixel_clock, int bpp);
>  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
>  bool intel_digital_port_connected(struct drm_i915_private *dev_priv,

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jim Bride]

On Mon, May 08, 2017 at 11:41:25AM +0300, Jani Nikula wrote:
> On Sat, 06 May 2017, Jim Bride <jim.bride@linux.intel.com> 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 <mika.kahola@intel.com>
> > 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 <rodrigo.vivi@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Manasi D Navare <manasi.d.navare@intel.com>
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Fixes: 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")
> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> > ---
> >  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, 270000, 540000 };
> >   * 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;
> >  		}
> >  
> >  		if (voltage_tries == 5) {
> >  			DRM_DEBUG_KMS("Same voltage tried 5 times\n");
> > +			intel_dp->train_set_valid = false;
> >  			return false;
> >  		}
> >  
> >  		if (max_vswing_tries == 1) {
> >  			DRM_DEBUG_KMS("Max Voltage Swing reached\n");
> > +			intel_dp->train_set_valid = false;
> >  			return false;
> >  		}
> >  
> > @@ -198,6 +204,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> >  		intel_get_adjust_train(intel_dp, link_status);
> >  		if (!intel_dp_update_link_train(intel_dp)) {
> >  			DRM_ERROR("failed to update link training\n");
> > +			intel_dp->train_set_valid = false;
> >  			return false;
> >  		}
> >  
> > @@ -256,6 +263,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> >  				     training_pattern |
> >  				     DP_LINK_SCRAMBLING_DISABLE)) {
> >  		DRM_ERROR("failed to start channel equalization\n");
> > +		intel_dp->train_set_valid = false;
> >  		return false;
> >  	}
> >  
> > @@ -296,6 +304,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> >  	/* Try 5 times, else fail and try at lower BW */
> >  	if (tries == 5) {
> >  		intel_dp_dump_link_status(link_status);
> > +		intel_dp->train_set_valid = false;
> >  		DRM_DEBUG_KMS("Channel equalization failed 5 times\n");
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 19d0c8f..149d4c9 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -977,6 +977,7 @@ struct intel_dp {
> >  	struct drm_dp_aux aux;
> >  	enum intel_display_power_domain aux_power_domain;
> >  	uint8_t train_set[4];
> > +	bool train_set_valid;
> >  	int panel_power_up_delay;
> >  	int panel_power_down_delay;
> >  	int panel_power_cycle_delay;
> > @@ -1551,6 +1552,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> >  bool __intel_dp_read_desc(struct intel_dp *intel_dp,
> >  			  struct intel_dp_desc *desc);
> >  bool intel_dp_read_desc(struct intel_dp *intel_dp);
> > +bool is_edp(struct intel_dp *intel_dp);
> >  int intel_dp_link_required(int pixel_clock, int bpp);
> >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> >  bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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 <mika.kahola@intel.com>
> 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 <mika.kahola@intel.com>

> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Manasi D Navare <manasi.d.navare@intel.com>
> Cc: Mika Kahola <mika.kahola@intel.com>
> Fixes: 34511dce4 ("drm/i915: Revert DisplayPort fast link training
> feature")
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  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,
> 270000, 540000 };
>   * 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;
>  		}
>  
>  		if (voltage_tries == 5) {
>  			DRM_DEBUG_KMS("Same voltage tried 5
> times\n");
> +			intel_dp->train_set_valid = false;
>  			return false;
>  		}
>  
>  		if (max_vswing_tries == 1) {
>  			DRM_DEBUG_KMS("Max Voltage Swing
> reached\n");
> +			intel_dp->train_set_valid = false;
>  			return false;
>  		}
>  
> @@ -198,6 +204,7 @@ intel_dp_link_training_clock_recovery(struct
> intel_dp *intel_dp)
>  		intel_get_adjust_train(intel_dp, link_status);
>  		if (!intel_dp_update_link_train(intel_dp)) {
>  			DRM_ERROR("failed to update link
> training\n");
> +			intel_dp->train_set_valid = false;
>  			return false;
>  		}
>  
> @@ -256,6 +263,7 @@
> intel_dp_link_training_channel_equalization(struct intel_dp
> *intel_dp)
>  				     training_pattern |
>  				     DP_LINK_SCRAMBLING_DISABLE)) {
>  		DRM_ERROR("failed to start channel equalization\n");
> +		intel_dp->train_set_valid = false;
>  		return false;
>  	}
>  
> @@ -296,6 +304,7 @@
> intel_dp_link_training_channel_equalization(struct intel_dp
> *intel_dp)
>  	/* Try 5 times, else fail and try at lower BW */
>  	if (tries == 5) {
>  		intel_dp_dump_link_status(link_status);
> +		intel_dp->train_set_valid = false;
>  		DRM_DEBUG_KMS("Channel equalization failed 5
> times\n");
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 19d0c8f..149d4c9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -977,6 +977,7 @@ struct intel_dp {
>  	struct drm_dp_aux aux;
>  	enum intel_display_power_domain aux_power_domain;
>  	uint8_t train_set[4];
> +	bool train_set_valid;
>  	int panel_power_up_delay;
>  	int panel_power_down_delay;
>  	int panel_power_cycle_delay;
> @@ -1551,6 +1552,7 @@ bool intel_dp_read_dpcd(struct intel_dp
> *intel_dp);
>  bool __intel_dp_read_desc(struct intel_dp *intel_dp,
>  			  struct intel_dp_desc *desc);
>  bool intel_dp_read_desc(struct intel_dp *intel_dp);
> +bool is_edp(struct intel_dp *intel_dp);
>  int intel_dp_link_required(int pixel_clock, int bpp);
>  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
>  bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
-- 
Mika Kahola - Intel OTC

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915/psr: Account for sink CRC raciness on some panels

[Jim Bride]

According to the eDP spec, when the count field in TEST_SINK_MISC
increments then the six bytes of sink CRC information in the DPCD
should be valid.  Unfortunately, this doesn't seem to be the case
on some panels, and as a result we get some incorrect and inconsistent
values from the sink CRC DPCD locations at times.  This problem exhibits
itself more on faster processors (relative failure rates HSW < SKL < KBL.)
In order to try and account for this, we try a lot harder to read the sink
CRC until we get consistent values twice in a row before returning what we
read and delay for a time before trying to read.  We still see some
occasional failures, but reading the sink CRC is much more reliable,
particularly on SKL and KBL, with these changes than without.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++--
 drivers/gpu/drm/i915/intel_dp.c     | 57 ++++++++++++++++++++++++++++++++-----
 2 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 870c470..4902473 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2718,7 +2718,7 @@ static int i915_sink_crc(struct seq_file *m, void *data)
 	struct intel_connector *connector;
 	struct drm_connector_list_iter conn_iter;
 	struct intel_dp *intel_dp = NULL;
-	int ret;
+	int ret, tries = 6;
 	u8 crc[6];
 
 	drm_modeset_lock_all(dev);
@@ -2738,9 +2738,17 @@ static int i915_sink_crc(struct seq_file *m, void *data)
 
 		intel_dp = enc_to_intel_dp(connector->base.state->best_encoder);
 
-		ret = intel_dp_sink_crc(intel_dp, crc);
-		if (ret)
+		memset(crc, 0, 6);
+		do {
+			ret = intel_dp_sink_crc(intel_dp, crc);
+			if (ret == -ETIMEDOUT)
+				usleep_range(500, 700);
+		} while ((ret == -ETIMEDOUT) && --tries);
+
+		if (ret != 0) {
+			seq_printf(m, "000000000000\n");
 			goto out;
+		}
 
 		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
 			   crc[0], crc[1], crc[2],
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 06b8bd4..217bc06 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3877,13 +3877,15 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 
 	do {
 		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
-
+		usleep_range(16700, 17000);
 		if (drm_dp_dpcd_readb(&intel_dp->aux,
 				      DP_TEST_SINK_MISC, &buf) < 0) {
+			DRM_DEBUG_KMS("Could not read TEST_SINK_MISC\n");
 			ret = -EIO;
 			goto out;
 		}
 		count = buf & DP_TEST_COUNT_MASK;
+		DRM_DEBUG_KMS("PSR count is %d\n", count);
 	} while (--attempts && count);
 
 	if (attempts == 0) {
@@ -3928,6 +3930,8 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 	}
 
 	intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
+	usleep_range(16700, 17000);
+	DRM_DEBUG_KMS("PSR Successfully started sink CRC\n");
 	return 0;
 }
 
@@ -3939,21 +3943,30 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 	u8 buf;
 	int count, ret;
 	int attempts = 6;
+	u8 old_crc[6];
+
+	if (crc != NULL)
+		memset(crc, 0, 6);
+	else
+		return -ENOMEM;
 
 	ret = intel_dp_sink_crc_start(intel_dp);
-	if (ret)
+	if (ret) {
+		DRM_DEBUG_KMS("Could not start sink crc; ret %d\n", ret);
 		return ret;
+	}
 
 	do {
 		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
+		usleep_range(16700, 17000);
 
 		if (drm_dp_dpcd_readb(&intel_dp->aux,
 				      DP_TEST_SINK_MISC, &buf) < 0) {
+			DRM_DEBUG_KMS("Cound not read TEST_SINK_MISC\n");
 			ret = -EIO;
 			goto stop;
 		}
 		count = buf & DP_TEST_COUNT_MASK;
-
 	} while (--attempts && count == 0);
 
 	if (attempts == 0) {
@@ -3962,11 +3975,41 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 		goto stop;
 	}
 
-	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
-		ret = -EIO;
-		goto stop;
-	}
+	attempts = 6;
+	memset(old_crc, 0xFF, 6);
+	do {
+		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
+		usleep_range(16500, 17000);
+		if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR,
+				     crc, 6) < 0) {
+			DRM_DEBUG_KMS("Could not read sink crc\n");
+			ret = -EIO;
+			goto stop;
+		}
+
+		if (memcmp(old_crc, crc, 6) == 0) {
+			DRM_DEBUG_KMS("PSR Active: %3s Matching crc values found:"
+				      "%02x%02x%02x%02x%02x%02x\n",
+				      yesno(dev_priv->psr.active),
+				      crc[0], crc[1], crc[2],
+				      crc[3], crc[4], crc[5]);
+			ret = 0;
+			goto stop;
+		} else {
+			DRM_DEBUG_KMS("PSR Active: %3s crc %02x%02x%02x%02x%02x%02x "
+				      "doesn't match "
+				      "previous %02x%02x%02x%02x%02x%02x\n",
+				      yesno(dev_priv->psr.active),
+				      crc[0], crc[1], crc[2],
+				      crc[3], crc[4], crc[5],
+				      old_crc[0], old_crc[1], old_crc[2],
+				      old_crc[3], old_crc[4], old_crc[5]);
+			memcpy(old_crc, crc, 6);
+		}
+	} while (--attempts);
 
+	DRM_DEBUG_KMS("Failed to get CRC after 6 attempts.\n");
+	ret = -ETIMEDOUT;
 stop:
 	intel_dp_sink_crc_stop(intel_dp);
 	return ret;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/psr: Account for sink CRC raciness on some panels

[Jani Nikula]

On Sat, 06 May 2017, Jim Bride <jim.bride@linux.intel.com> wrote:
> According to the eDP spec, when the count field in TEST_SINK_MISC
> increments then the six bytes of sink CRC information in the DPCD
> should be valid.  Unfortunately, this doesn't seem to be the case
> on some panels, and as a result we get some incorrect and inconsistent
> values from the sink CRC DPCD locations at times.  This problem exhibits
> itself more on faster processors (relative failure rates HSW < SKL < KBL.)
> In order to try and account for this, we try a lot harder to read the sink
> CRC until we get consistent values twice in a row before returning what we
> read and delay for a time before trying to read.  We still see some
> occasional failures, but reading the sink CRC is much more reliable,
> particularly on SKL and KBL, with these changes than without.

Retrying seems like a good idea. But this patch retries up to 36 times
if the two consecutive reads don't match, which is excessive.

The sleeps should probably be a separate change. Also not happy about
the magic numbers there.

Also, there are plenty of what seem like unrelated changes. Whitespace,
debug logging, error checking, etc.


BR,
Jani.


>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++--
>  drivers/gpu/drm/i915/intel_dp.c     | 57 ++++++++++++++++++++++++++++++++-----
>  2 files changed, 61 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 870c470..4902473 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2718,7 +2718,7 @@ static int i915_sink_crc(struct seq_file *m, void *data)
>  	struct intel_connector *connector;
>  	struct drm_connector_list_iter conn_iter;
>  	struct intel_dp *intel_dp = NULL;
> -	int ret;
> +	int ret, tries = 6;
>  	u8 crc[6];
>  
>  	drm_modeset_lock_all(dev);
> @@ -2738,9 +2738,17 @@ static int i915_sink_crc(struct seq_file *m, void *data)
>  
>  		intel_dp = enc_to_intel_dp(connector->base.state->best_encoder);
>  
> -		ret = intel_dp_sink_crc(intel_dp, crc);
> -		if (ret)
> +		memset(crc, 0, 6);
> +		do {
> +			ret = intel_dp_sink_crc(intel_dp, crc);
> +			if (ret == -ETIMEDOUT)
> +				usleep_range(500, 700);
> +		} while ((ret == -ETIMEDOUT) && --tries);
> +
> +		if (ret != 0) {
> +			seq_printf(m, "000000000000\n");
>  			goto out;
> +		}
>  
>  		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
>  			   crc[0], crc[1], crc[2],
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 06b8bd4..217bc06 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3877,13 +3877,15 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>  
>  	do {
>  		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> -
> +		usleep_range(16700, 17000);
>  		if (drm_dp_dpcd_readb(&intel_dp->aux,
>  				      DP_TEST_SINK_MISC, &buf) < 0) {
> +			DRM_DEBUG_KMS("Could not read TEST_SINK_MISC\n");
>  			ret = -EIO;
>  			goto out;
>  		}
>  		count = buf & DP_TEST_COUNT_MASK;
> +		DRM_DEBUG_KMS("PSR count is %d\n", count);
>  	} while (--attempts && count);
>  
>  	if (attempts == 0) {
> @@ -3928,6 +3930,8 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>  	}
>  
>  	intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> +	usleep_range(16700, 17000);
> +	DRM_DEBUG_KMS("PSR Successfully started sink CRC\n");
>  	return 0;
>  }
>  
> @@ -3939,21 +3943,30 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  	u8 buf;
>  	int count, ret;
>  	int attempts = 6;
> +	u8 old_crc[6];
> +
> +	if (crc != NULL)
> +		memset(crc, 0, 6);
> +	else
> +		return -ENOMEM;
>  
>  	ret = intel_dp_sink_crc_start(intel_dp);
> -	if (ret)
> +	if (ret) {
> +		DRM_DEBUG_KMS("Could not start sink crc; ret %d\n", ret);
>  		return ret;
> +	}
>  
>  	do {
>  		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> +		usleep_range(16700, 17000);
>  
>  		if (drm_dp_dpcd_readb(&intel_dp->aux,
>  				      DP_TEST_SINK_MISC, &buf) < 0) {
> +			DRM_DEBUG_KMS("Cound not read TEST_SINK_MISC\n");
>  			ret = -EIO;
>  			goto stop;
>  		}
>  		count = buf & DP_TEST_COUNT_MASK;
> -
>  	} while (--attempts && count == 0);
>  
>  	if (attempts == 0) {
> @@ -3962,11 +3975,41 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  		goto stop;
>  	}
>  
> -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
> -		ret = -EIO;
> -		goto stop;
> -	}
> +	attempts = 6;
> +	memset(old_crc, 0xFF, 6);
> +	do {
> +		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> +		usleep_range(16500, 17000);
> +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR,
> +				     crc, 6) < 0) {
> +			DRM_DEBUG_KMS("Could not read sink crc\n");
> +			ret = -EIO;
> +			goto stop;
> +		}
> +
> +		if (memcmp(old_crc, crc, 6) == 0) {
> +			DRM_DEBUG_KMS("PSR Active: %3s Matching crc values found:"
> +				      "%02x%02x%02x%02x%02x%02x\n",
> +				      yesno(dev_priv->psr.active),
> +				      crc[0], crc[1], crc[2],
> +				      crc[3], crc[4], crc[5]);
> +			ret = 0;
> +			goto stop;
> +		} else {
> +			DRM_DEBUG_KMS("PSR Active: %3s crc %02x%02x%02x%02x%02x%02x "
> +				      "doesn't match "
> +				      "previous %02x%02x%02x%02x%02x%02x\n",
> +				      yesno(dev_priv->psr.active),
> +				      crc[0], crc[1], crc[2],
> +				      crc[3], crc[4], crc[5],
> +				      old_crc[0], old_crc[1], old_crc[2],
> +				      old_crc[3], old_crc[4], old_crc[5]);
> +			memcpy(old_crc, crc, 6);
> +		}
> +	} while (--attempts);
>  
> +	DRM_DEBUG_KMS("Failed to get CRC after 6 attempts.\n");
> +	ret = -ETIMEDOUT;
>  stop:
>  	intel_dp_sink_crc_stop(intel_dp);
>  	return ret;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/psr: Account for sink CRC raciness on some panels

[Jim Bride]

On Mon, May 08, 2017 at 12:12:47PM +0300, Jani Nikula wrote:
> On Sat, 06 May 2017, Jim Bride <jim.bride@linux.intel.com> wrote:
> > According to the eDP spec, when the count field in TEST_SINK_MISC
> > increments then the six bytes of sink CRC information in the DPCD
> > should be valid.  Unfortunately, this doesn't seem to be the case
> > on some panels, and as a result we get some incorrect and inconsistent
> > values from the sink CRC DPCD locations at times.  This problem exhibits
> > itself more on faster processors (relative failure rates HSW < SKL < KBL.)
> > In order to try and account for this, we try a lot harder to read the sink
> > CRC until we get consistent values twice in a row before returning what we
> > read and delay for a time before trying to read.  We still see some
> > occasional failures, but reading the sink CRC is much more reliable,
> > particularly on SKL and KBL, with these changes than without.
> 
> Retrying seems like a good idea. But this patch retries up to 36 times
> if the two consecutive reads don't match, which is excessive.

I don't see where you're getting 36 from; the loop runs through six times.

> The sleeps should probably be a separate change. Also not happy about
> the magic numbers there.

Looping w/o the sleeps doesn't make a ton of sense IMHO.  The numbers
were found experimentally on KBL, which seemed to more easily
exercise the race that this patch is meant to address.  Ideally the
panels would not increment the counter until they were sure that the
CRC data in the DPCD actually reflected reality.  Unfortunately, this
isn't what we're seeing.  I will create a couple of #defines for the
sleep values, at least, with some comments about them.  

> Also, there are plenty of what seem like unrelated changes. Whitespace,
> debug logging, error checking, etc.

I looked at this as refactoring the existing implementation, so I
went ahead and added some debugging info; the error checking is
part of the refactoring.  Given the history of problems with this
functionality that seemed warranted.  I did notice one line of
whitespace change that snuck in (where I missed that I removed a
blank line.)  I can take out the debug prints for the success
cases if they seem excessive (and I'll restore the deleted
blank line for the next version of the patch.)

Jim


> 
> BR,
> Jani.
> 
> 
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++--
> >  drivers/gpu/drm/i915/intel_dp.c     | 57 ++++++++++++++++++++++++++++++++-----
> >  2 files changed, 61 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 870c470..4902473 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2718,7 +2718,7 @@ static int i915_sink_crc(struct seq_file *m, void *data)
> >  	struct intel_connector *connector;
> >  	struct drm_connector_list_iter conn_iter;
> >  	struct intel_dp *intel_dp = NULL;
> > -	int ret;
> > +	int ret, tries = 6;
> >  	u8 crc[6];
> >  
> >  	drm_modeset_lock_all(dev);
> > @@ -2738,9 +2738,17 @@ static int i915_sink_crc(struct seq_file *m, void *data)
> >  
> >  		intel_dp = enc_to_intel_dp(connector->base.state->best_encoder);
> >  
> > -		ret = intel_dp_sink_crc(intel_dp, crc);
> > -		if (ret)
> > +		memset(crc, 0, 6);
> > +		do {
> > +			ret = intel_dp_sink_crc(intel_dp, crc);
> > +			if (ret == -ETIMEDOUT)
> > +				usleep_range(500, 700);
> > +		} while ((ret == -ETIMEDOUT) && --tries);
> > +
> > +		if (ret != 0) {
> > +			seq_printf(m, "000000000000\n");
> >  			goto out;
> > +		}
> >  
> >  		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
> >  			   crc[0], crc[1], crc[2],
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 06b8bd4..217bc06 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3877,13 +3877,15 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> >  
> >  	do {
> >  		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> > -
> > +		usleep_range(16700, 17000);
> >  		if (drm_dp_dpcd_readb(&intel_dp->aux,
> >  				      DP_TEST_SINK_MISC, &buf) < 0) {
> > +			DRM_DEBUG_KMS("Could not read TEST_SINK_MISC\n");
> >  			ret = -EIO;
> >  			goto out;
> >  		}
> >  		count = buf & DP_TEST_COUNT_MASK;
> > +		DRM_DEBUG_KMS("PSR count is %d\n", count);
> >  	} while (--attempts && count);
> >  
> >  	if (attempts == 0) {
> > @@ -3928,6 +3930,8 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
> >  	}
> >  
> >  	intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> > +	usleep_range(16700, 17000);
> > +	DRM_DEBUG_KMS("PSR Successfully started sink CRC\n");
> >  	return 0;
> >  }
> >  
> > @@ -3939,21 +3943,30 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> >  	u8 buf;
> >  	int count, ret;
> >  	int attempts = 6;
> > +	u8 old_crc[6];
> > +
> > +	if (crc != NULL)
> > +		memset(crc, 0, 6);
> > +	else
> > +		return -ENOMEM;
> >  
> >  	ret = intel_dp_sink_crc_start(intel_dp);
> > -	if (ret)
> > +	if (ret) {
> > +		DRM_DEBUG_KMS("Could not start sink crc; ret %d\n", ret);
> >  		return ret;
> > +	}
> >  
> >  	do {
> >  		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> > +		usleep_range(16700, 17000);
> >  
> >  		if (drm_dp_dpcd_readb(&intel_dp->aux,
> >  				      DP_TEST_SINK_MISC, &buf) < 0) {
> > +			DRM_DEBUG_KMS("Cound not read TEST_SINK_MISC\n");
> >  			ret = -EIO;
> >  			goto stop;
> >  		}
> >  		count = buf & DP_TEST_COUNT_MASK;
> > -
> >  	} while (--attempts && count == 0);
> >  
> >  	if (attempts == 0) {
> > @@ -3962,11 +3975,41 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> >  		goto stop;
> >  	}
> >  
> > -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
> > -		ret = -EIO;
> > -		goto stop;
> > -	}
> > +	attempts = 6;
> > +	memset(old_crc, 0xFF, 6);
> > +	do {
> > +		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> > +		usleep_range(16500, 17000);
> > +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR,
> > +				     crc, 6) < 0) {
> > +			DRM_DEBUG_KMS("Could not read sink crc\n");
> > +			ret = -EIO;
> > +			goto stop;
> > +		}
> > +
> > +		if (memcmp(old_crc, crc, 6) == 0) {
> > +			DRM_DEBUG_KMS("PSR Active: %3s Matching crc values found:"
> > +				      "%02x%02x%02x%02x%02x%02x\n",
> > +				      yesno(dev_priv->psr.active),
> > +				      crc[0], crc[1], crc[2],
> > +				      crc[3], crc[4], crc[5]);
> > +			ret = 0;
> > +			goto stop;
> > +		} else {
> > +			DRM_DEBUG_KMS("PSR Active: %3s crc %02x%02x%02x%02x%02x%02x "
> > +				      "doesn't match "
> > +				      "previous %02x%02x%02x%02x%02x%02x\n",
> > +				      yesno(dev_priv->psr.active),
> > +				      crc[0], crc[1], crc[2],
> > +				      crc[3], crc[4], crc[5],
> > +				      old_crc[0], old_crc[1], old_crc[2],
> > +				      old_crc[3], old_crc[4], old_crc[5]);
> > +			memcpy(old_crc, crc, 6);
> > +		}
> > +	} while (--attempts);
> >  
> > +	DRM_DEBUG_KMS("Failed to get CRC after 6 attempts.\n");
> > +	ret = -ETIMEDOUT;
> >  stop:
> >  	intel_dp_sink_crc_stop(intel_dp);
> >  	return ret;
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/psr: Account for sink CRC raciness on some panels

[Jani Nikula]

On Mon, 08 May 2017, Jim Bride <jim.bride@linux.intel.com> wrote:
> On Mon, May 08, 2017 at 12:12:47PM +0300, Jani Nikula wrote:
>> On Sat, 06 May 2017, Jim Bride <jim.bride@linux.intel.com> wrote:
>> > According to the eDP spec, when the count field in TEST_SINK_MISC
>> > increments then the six bytes of sink CRC information in the DPCD
>> > should be valid.  Unfortunately, this doesn't seem to be the case
>> > on some panels, and as a result we get some incorrect and inconsistent
>> > values from the sink CRC DPCD locations at times.  This problem exhibits
>> > itself more on faster processors (relative failure rates HSW < SKL < KBL.)
>> > In order to try and account for this, we try a lot harder to read the sink
>> > CRC until we get consistent values twice in a row before returning what we
>> > read and delay for a time before trying to read.  We still see some
>> > occasional failures, but reading the sink CRC is much more reliable,
>> > particularly on SKL and KBL, with these changes than without.
>> 
>> Retrying seems like a good idea. But this patch retries up to 36 times
>> if the two consecutive reads don't match, which is excessive.
>
> I don't see where you're getting 36 from; the loop runs through six times.

i915_sink_crc() calls intel_dp_sink_crc(), retrying on ETIMEDOUT. In
turn, intel_dp_sink_crc() reads CRC, retrying on mismatched CRC and
returning ETIMEDOUT to i915_sink_crc() on errors. Admittedly the loop in
intel_dp_sink_crc() will always run at least twice to get two results to
compare, so I guess it would be fair to say the combo tries CRC read up
to 6 * 5 = 30 times.

>> The sleeps should probably be a separate change. Also not happy about
>> the magic numbers there.
>
> Looping w/o the sleeps doesn't make a ton of sense IMHO.  The numbers
> were found experimentally on KBL, which seemed to more easily
> exercise the race that this patch is meant to address.  Ideally the
> panels would not increment the counter until they were sure that the
> CRC data in the DPCD actually reflected reality.  Unfortunately, this
> isn't what we're seeing.  I will create a couple of #defines for the
> sleep values, at least, with some comments about them.  

It's just that the magic sleeps are eerily close to a frame at 60
Hz. Did you try another vblank wait instead? Or maybe it's the sync to
vblank that is the issue.

>> Also, there are plenty of what seem like unrelated changes. Whitespace,
>> debug logging, error checking, etc.
>
> I looked at this as refactoring the existing implementation, so I
> went ahead and added some debugging info; the error checking is
> part of the refactoring.  Given the history of problems with this
> functionality that seemed warranted.  I did notice one line of
> whitespace change that snuck in (where I missed that I removed a
> blank line.)  I can take out the debug prints for the success
> cases if they seem excessive (and I'll restore the deleted
> blank line for the next version of the patch.)

The thing is, by now I'm looking at basically any sink visible
functional changes to eDP, and PSR in particular, with the thought that
this may have to be reverted due to regressions later. That has nothing
to do with these patches, but rather our track record with eDP and
PSR. (And that also says a lot about eDP sink quality in general.)

So I'd rather see the other changes split out so we have a smaller diff
to revert, in case we have to revert. The pessimist does not get
disappointed. And so far PSR hasn't given us any reasons for optimism.

BR,
Jani.



>
> Jim
>
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++--
>> >  drivers/gpu/drm/i915/intel_dp.c     | 57 ++++++++++++++++++++++++++++++++-----
>> >  2 files changed, 61 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> > index 870c470..4902473 100644
>> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> > @@ -2718,7 +2718,7 @@ static int i915_sink_crc(struct seq_file *m, void *data)
>> >  	struct intel_connector *connector;
>> >  	struct drm_connector_list_iter conn_iter;
>> >  	struct intel_dp *intel_dp = NULL;
>> > -	int ret;
>> > +	int ret, tries = 6;
>> >  	u8 crc[6];
>> >  
>> >  	drm_modeset_lock_all(dev);
>> > @@ -2738,9 +2738,17 @@ static int i915_sink_crc(struct seq_file *m, void *data)
>> >  
>> >  		intel_dp = enc_to_intel_dp(connector->base.state->best_encoder);
>> >  
>> > -		ret = intel_dp_sink_crc(intel_dp, crc);
>> > -		if (ret)
>> > +		memset(crc, 0, 6);
>> > +		do {
>> > +			ret = intel_dp_sink_crc(intel_dp, crc);
>> > +			if (ret == -ETIMEDOUT)
>> > +				usleep_range(500, 700);
>> > +		} while ((ret == -ETIMEDOUT) && --tries);
>> > +
>> > +		if (ret != 0) {
>> > +			seq_printf(m, "000000000000\n");
>> >  			goto out;
>> > +		}
>> >  
>> >  		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
>> >  			   crc[0], crc[1], crc[2],
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index 06b8bd4..217bc06 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -3877,13 +3877,15 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>> >  
>> >  	do {
>> >  		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
>> > -
>> > +		usleep_range(16700, 17000);
>> >  		if (drm_dp_dpcd_readb(&intel_dp->aux,
>> >  				      DP_TEST_SINK_MISC, &buf) < 0) {
>> > +			DRM_DEBUG_KMS("Could not read TEST_SINK_MISC\n");
>> >  			ret = -EIO;
>> >  			goto out;
>> >  		}
>> >  		count = buf & DP_TEST_COUNT_MASK;
>> > +		DRM_DEBUG_KMS("PSR count is %d\n", count);
>> >  	} while (--attempts && count);
>> >  
>> >  	if (attempts == 0) {
>> > @@ -3928,6 +3930,8 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>> >  	}
>> >  
>> >  	intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
>> > +	usleep_range(16700, 17000);
>> > +	DRM_DEBUG_KMS("PSR Successfully started sink CRC\n");
>> >  	return 0;
>> >  }
>> >  
>> > @@ -3939,21 +3943,30 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>> >  	u8 buf;
>> >  	int count, ret;
>> >  	int attempts = 6;
>> > +	u8 old_crc[6];
>> > +
>> > +	if (crc != NULL)
>> > +		memset(crc, 0, 6);
>> > +	else
>> > +		return -ENOMEM;
>> >  
>> >  	ret = intel_dp_sink_crc_start(intel_dp);
>> > -	if (ret)
>> > +	if (ret) {
>> > +		DRM_DEBUG_KMS("Could not start sink crc; ret %d\n", ret);
>> >  		return ret;
>> > +	}
>> >  
>> >  	do {
>> >  		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
>> > +		usleep_range(16700, 17000);
>> >  
>> >  		if (drm_dp_dpcd_readb(&intel_dp->aux,
>> >  				      DP_TEST_SINK_MISC, &buf) < 0) {
>> > +			DRM_DEBUG_KMS("Cound not read TEST_SINK_MISC\n");
>> >  			ret = -EIO;
>> >  			goto stop;
>> >  		}
>> >  		count = buf & DP_TEST_COUNT_MASK;
>> > -
>> >  	} while (--attempts && count == 0);
>> >  
>> >  	if (attempts == 0) {
>> > @@ -3962,11 +3975,41 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>> >  		goto stop;
>> >  	}
>> >  
>> > -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
>> > -		ret = -EIO;
>> > -		goto stop;
>> > -	}
>> > +	attempts = 6;
>> > +	memset(old_crc, 0xFF, 6);
>> > +	do {
>> > +		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
>> > +		usleep_range(16500, 17000);
>> > +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR,
>> > +				     crc, 6) < 0) {
>> > +			DRM_DEBUG_KMS("Could not read sink crc\n");
>> > +			ret = -EIO;
>> > +			goto stop;
>> > +		}
>> > +
>> > +		if (memcmp(old_crc, crc, 6) == 0) {
>> > +			DRM_DEBUG_KMS("PSR Active: %3s Matching crc values found:"
>> > +				      "%02x%02x%02x%02x%02x%02x\n",
>> > +				      yesno(dev_priv->psr.active),
>> > +				      crc[0], crc[1], crc[2],
>> > +				      crc[3], crc[4], crc[5]);
>> > +			ret = 0;
>> > +			goto stop;
>> > +		} else {
>> > +			DRM_DEBUG_KMS("PSR Active: %3s crc %02x%02x%02x%02x%02x%02x "
>> > +				      "doesn't match "
>> > +				      "previous %02x%02x%02x%02x%02x%02x\n",
>> > +				      yesno(dev_priv->psr.active),
>> > +				      crc[0], crc[1], crc[2],
>> > +				      crc[3], crc[4], crc[5],
>> > +				      old_crc[0], old_crc[1], old_crc[2],
>> > +				      old_crc[3], old_crc[4], old_crc[5]);
>> > +			memcpy(old_crc, crc, 6);
>> > +		}
>> > +	} while (--attempts);
>> >  
>> > +	DRM_DEBUG_KMS("Failed to get CRC after 6 attempts.\n");
>> > +	ret = -ETIMEDOUT;
>> >  stop:
>> >  	intel_dp_sink_crc_stop(intel_dp);
>> >  	return ret;
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/psr: Account for sink CRC raciness on some panels

[Jim Bride]

On Mon, May 08, 2017 at 09:05:08PM +0300, Jani Nikula wrote:
> On Mon, 08 May 2017, Jim Bride <jim.bride@linux.intel.com> wrote:
> > On Mon, May 08, 2017 at 12:12:47PM +0300, Jani Nikula wrote:
> >> On Sat, 06 May 2017, Jim Bride <jim.bride@linux.intel.com> wrote:
> >> > According to the eDP spec, when the count field in TEST_SINK_MISC
> >> > increments then the six bytes of sink CRC information in the DPCD
> >> > should be valid.  Unfortunately, this doesn't seem to be the case
> >> > on some panels, and as a result we get some incorrect and inconsistent
> >> > values from the sink CRC DPCD locations at times.  This problem exhibits
> >> > itself more on faster processors (relative failure rates HSW < SKL < KBL.)
> >> > In order to try and account for this, we try a lot harder to read the sink
> >> > CRC until we get consistent values twice in a row before returning what we
> >> > read and delay for a time before trying to read.  We still see some
> >> > occasional failures, but reading the sink CRC is much more reliable,
> >> > particularly on SKL and KBL, with these changes than without.
> >> 
> >> Retrying seems like a good idea. But this patch retries up to 36 times
> >> if the two consecutive reads don't match, which is excessive.
> >
> > I don't see where you're getting 36 from; the loop runs through six times.
> 
> i915_sink_crc() calls intel_dp_sink_crc(), retrying on ETIMEDOUT. In
> turn, intel_dp_sink_crc() reads CRC, retrying on mismatched CRC and
> returning ETIMEDOUT to i915_sink_crc() on errors. Admittedly the loop in
> intel_dp_sink_crc() will always run at least twice to get two results to
> compare, so I guess it would be fair to say the combo tries CRC read up
> to 6 * 5 = 30 times.

I'll play around with making i915_sink_crc() a little less
persistent and see what it does.  The loop in intel_dp_sink_crc()
frequently goes 3+ tries, so I'd prefer to keep that one as it is.


> >> The sleeps should probably be a separate change. Also not happy about
> >> the magic numbers there.
> >
> > Looping w/o the sleeps doesn't make a ton of sense IMHO.  The numbers
> > were found experimentally on KBL, which seemed to more easily
> > exercise the race that this patch is meant to address.  Ideally the
> > panels would not increment the counter until they were sure that the
> > CRC data in the DPCD actually reflected reality.  Unfortunately, this
> > isn't what we're seeing.  I will create a couple of #defines for the
> > sleep values, at least, with some comments about them.  
> 
> It's just that the magic sleeps are eerily close to a frame at 60
> Hz. Did you try another vblank wait instead? Or maybe it's the sync to
> vblank that is the issue.

This brings up an interesting point.  IMHO relying on vblanks for PSR
related stuff is a bad idea, at least while PSR is active.  The
reason I say this is that if PSR is active, then the link itself may
or may not be active.  We certainly shouldn't be sending frames of
data, vblanks,  or anything of that ilk.  There's a patch series that
Rodrigo wrote last August that I plan to write a IGT test case for (I
wanted to see IGT working right for PSR tests in general before doing
this) that basically makes sure that vblanks are disabled if PSR is
active, and that enabling vblanks causes PSR to exit.  Review
feedback on that series was that it needed an IGT test.  When thinking
about this series, I've been toying with the idea of doing a sleep
based on the time needed to render a frame + the appropriate blanking
intervals instead of doing all of the vblank waits anyhow, at least
for the sink CRC stuff since we need to get that data out while PSR
is active.  Now I'm wondering if we should go ahead and do that for
all but the PSR exit case now, perhaps.  Thoughts?

> >> Also, there are plenty of what seem like unrelated changes. Whitespace,
> >> debug logging, error checking, etc.
> >
> > I looked at this as refactoring the existing implementation, so I
> > went ahead and added some debugging info; the error checking is
> > part of the refactoring.  Given the history of problems with this
> > functionality that seemed warranted.  I did notice one line of
> > whitespace change that snuck in (where I missed that I removed a
> > blank line.)  I can take out the debug prints for the success
> > cases if they seem excessive (and I'll restore the deleted
> > blank line for the next version of the patch.)
> 
> The thing is, by now I'm looking at basically any sink visible
> functional changes to eDP, and PSR in particular, with the thought that
> this may have to be reverted due to regressions later. That has nothing
> to do with these patches, but rather our track record with eDP and
> PSR. (And that also says a lot about eDP sink quality in general.)
> 
> So I'd rather see the other changes split out so we have a smaller diff
> to revert, in case we have to revert. The pessimist does not get
> disappointed. And so far PSR hasn't given us any reasons for optimism.

Let me see what I can do.  I might be able to at least split out some
of the debug / error checking into a separate patch.

Jim

> BR,
> Jani.
> 
> 
> 
> >
> > Jim
> >
> >
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> >
> >> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++--
> >> >  drivers/gpu/drm/i915/intel_dp.c     | 57 ++++++++++++++++++++++++++++++++-----
> >> >  2 files changed, 61 insertions(+), 10 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >> > index 870c470..4902473 100644
> >> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> >> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >> > @@ -2718,7 +2718,7 @@ static int i915_sink_crc(struct seq_file *m, void *data)
> >> >  	struct intel_connector *connector;
> >> >  	struct drm_connector_list_iter conn_iter;
> >> >  	struct intel_dp *intel_dp = NULL;
> >> > -	int ret;
> >> > +	int ret, tries = 6;
> >> >  	u8 crc[6];
> >> >  
> >> >  	drm_modeset_lock_all(dev);
> >> > @@ -2738,9 +2738,17 @@ static int i915_sink_crc(struct seq_file *m, void *data)
> >> >  
> >> >  		intel_dp = enc_to_intel_dp(connector->base.state->best_encoder);
> >> >  
> >> > -		ret = intel_dp_sink_crc(intel_dp, crc);
> >> > -		if (ret)
> >> > +		memset(crc, 0, 6);
> >> > +		do {
> >> > +			ret = intel_dp_sink_crc(intel_dp, crc);
> >> > +			if (ret == -ETIMEDOUT)
> >> > +				usleep_range(500, 700);
> >> > +		} while ((ret == -ETIMEDOUT) && --tries);
> >> > +
> >> > +		if (ret != 0) {
> >> > +			seq_printf(m, "000000000000\n");
> >> >  			goto out;
> >> > +		}
> >> >  
> >> >  		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
> >> >  			   crc[0], crc[1], crc[2],
> >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> > index 06b8bd4..217bc06 100644
> >> > --- a/drivers/gpu/drm/i915/intel_dp.c
> >> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> > @@ -3877,13 +3877,15 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> >> >  
> >> >  	do {
> >> >  		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> >> > -
> >> > +		usleep_range(16700, 17000);
> >> >  		if (drm_dp_dpcd_readb(&intel_dp->aux,
> >> >  				      DP_TEST_SINK_MISC, &buf) < 0) {
> >> > +			DRM_DEBUG_KMS("Could not read TEST_SINK_MISC\n");
> >> >  			ret = -EIO;
> >> >  			goto out;
> >> >  		}
> >> >  		count = buf & DP_TEST_COUNT_MASK;
> >> > +		DRM_DEBUG_KMS("PSR count is %d\n", count);
> >> >  	} while (--attempts && count);
> >> >  
> >> >  	if (attempts == 0) {
> >> > @@ -3928,6 +3930,8 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
> >> >  	}
> >> >  
> >> >  	intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> >> > +	usleep_range(16700, 17000);
> >> > +	DRM_DEBUG_KMS("PSR Successfully started sink CRC\n");
> >> >  	return 0;
> >> >  }
> >> >  
> >> > @@ -3939,21 +3943,30 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> >> >  	u8 buf;
> >> >  	int count, ret;
> >> >  	int attempts = 6;
> >> > +	u8 old_crc[6];
> >> > +
> >> > +	if (crc != NULL)
> >> > +		memset(crc, 0, 6);
> >> > +	else
> >> > +		return -ENOMEM;
> >> >  
> >> >  	ret = intel_dp_sink_crc_start(intel_dp);
> >> > -	if (ret)
> >> > +	if (ret) {
> >> > +		DRM_DEBUG_KMS("Could not start sink crc; ret %d\n", ret);
> >> >  		return ret;
> >> > +	}
> >> >  
> >> >  	do {
> >> >  		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> >> > +		usleep_range(16700, 17000);
> >> >  
> >> >  		if (drm_dp_dpcd_readb(&intel_dp->aux,
> >> >  				      DP_TEST_SINK_MISC, &buf) < 0) {
> >> > +			DRM_DEBUG_KMS("Cound not read TEST_SINK_MISC\n");
> >> >  			ret = -EIO;
> >> >  			goto stop;
> >> >  		}
> >> >  		count = buf & DP_TEST_COUNT_MASK;
> >> > -
> >> >  	} while (--attempts && count == 0);
> >> >  
> >> >  	if (attempts == 0) {
> >> > @@ -3962,11 +3975,41 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> >> >  		goto stop;
> >> >  	}
> >> >  
> >> > -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
> >> > -		ret = -EIO;
> >> > -		goto stop;
> >> > -	}
> >> > +	attempts = 6;
> >> > +	memset(old_crc, 0xFF, 6);
> >> > +	do {
> >> > +		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> >> > +		usleep_range(16500, 17000);
> >> > +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR,
> >> > +				     crc, 6) < 0) {
> >> > +			DRM_DEBUG_KMS("Could not read sink crc\n");
> >> > +			ret = -EIO;
> >> > +			goto stop;
> >> > +		}
> >> > +
> >> > +		if (memcmp(old_crc, crc, 6) == 0) {
> >> > +			DRM_DEBUG_KMS("PSR Active: %3s Matching crc values found:"
> >> > +				      "%02x%02x%02x%02x%02x%02x\n",
> >> > +				      yesno(dev_priv->psr.active),
> >> > +				      crc[0], crc[1], crc[2],
> >> > +				      crc[3], crc[4], crc[5]);
> >> > +			ret = 0;
> >> > +			goto stop;
> >> > +		} else {
> >> > +			DRM_DEBUG_KMS("PSR Active: %3s crc %02x%02x%02x%02x%02x%02x "
> >> > +				      "doesn't match "
> >> > +				      "previous %02x%02x%02x%02x%02x%02x\n",
> >> > +				      yesno(dev_priv->psr.active),
> >> > +				      crc[0], crc[1], crc[2],
> >> > +				      crc[3], crc[4], crc[5],
> >> > +				      old_crc[0], old_crc[1], old_crc[2],
> >> > +				      old_crc[3], old_crc[4], old_crc[5]);
> >> > +			memcpy(old_crc, crc, 6);
> >> > +		}
> >> > +	} while (--attempts);
> >> >  
> >> > +	DRM_DEBUG_KMS("Failed to get CRC after 6 attempts.\n");
> >> > +	ret = -ETIMEDOUT;
> >> >  stop:
> >> >  	intel_dp_sink_crc_stop(intel_dp);
> >> >  	return ret;
> >> 
> >> -- 
> >> Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for Kernel PSR Fix-ups

[Patchwork]

== Series Details ==

Series: Kernel PSR Fix-ups
URL   : https://patchwork.freedesktop.org/series/24049/
State : success

== Summary ==

Series 24049v1 Kernel PSR Fix-ups
https://patchwork.freedesktop.org/api/1.0/series/24049/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:432s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:427s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:508s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:531s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:489s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:488s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:410s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:413s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:416s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:480s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:476s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:456s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time:577s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:456s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:456s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:489s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:428s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:527s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:402s
fi-bsw-n3050 failed to collect. IGT log at Patchwork_4633/fi-bsw-n3050/igt.log

fb550f86433515f36a0de161631541ec114581e3 drm-tip: 2017y-05m-05d-18h-33m-52s UTC integration manifest
46a8d3e drm/i915/psr: Account for sink CRC raciness on some panels
829bb88 drm/i915/edp: Be less aggressive about changing link config on eDP
a1d3407 drm/i915/psr: Clean-up intel_enable_source_psr1()
b74b07c drm/i915/edp: Allow alternate fixed mode for eDP if available.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4633/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx