[PATCH v3 0/4] Kernel PSR Fix-ups

[PATCH v3 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 on HSW, BDW, and SKL.
The first change enables us to run the PSR tests on some 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.

v2 Highlights:

   * Rebased to current drm-tip
   * Greatly reduced looping around trying to read sink CRC (Jani)
   * Reduce amount of changes in the sink CRC patch (Jani)
   * Field-wise init of EDP_PSR_MAX_SLEEP_TIME (Rodrigo)
   * Minor commit message / cover letter tweaks

v3:
   * Re-ordered patches to put reviewed patches first.
   * Rebased to current drm-tip
   
Jim Bride (4):
  drm/i915/psr: Clean-up intel_enable_source_psr1()
  drm/i915/psr: Account for sink CRC raciness on some panels
  drm/i915/edp: Allow alternate fixed mode for eDP if available.
  drm/i915/edp: Be less aggressive about changing link config on eDP

 drivers/gpu/drm/i915/i915_reg.h               |  4 ++
 drivers/gpu/drm/i915/intel_dp.c               | 78 +++++++++++++++++++++++----
 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 +++++++-
 9 files changed, 111 insertions(+), 16 deletions(-)

-- 
2.7.4

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

[PATCH v3 1/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)
    * Rebase

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Wayne Boyer <wayne.boyer@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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 c712d01..3e62429 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3789,18 +3789,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 559f1ab..132987b 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

[PATCH v3 2/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.

v2: * Reduce number of retries when reading the sink CRC (Jani)
    * Refactor to minimize changes to the code (Jani)
    * Rebase

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2d42d09..69c8130c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3906,6 +3906,14 @@ 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);
+		memset(old_crc, 0xff, 6);
+	} else {
+		return -ENOMEM;
+	}
 
 	ret = intel_dp_sink_crc_start(intel_dp);
 	if (ret)
@@ -3929,11 +3937,35 @@ 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;
+
+	/*
+	 * Sometimes it takes a while for the "real" CRC values to land in
+	 * the DPCD, so try several times until we get two reads in a row
+	 * that are the same.  If we're an eDP panel, delay between reads
+	 * for a while since the values take a bit longer to propagate.
+	 */
+	do {
+		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
+		if (is_edp(intel_dp))
+			usleep_range(20000, 25000);
+
+		if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR,
+				     crc, 6) < 0) {
+			ret = -EIO;
+			goto stop;
+		}
+
+		if (memcmp(old_crc, crc, 6) == 0) {
+			ret = 0;
+			goto stop;
+		} else {
+			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

[PATCH v3 3/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.

v2: Rebase

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Jani Nikula <jani.nikula@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 69c8130c..ee2a3db 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1606,6 +1606,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,
@@ -1652,8 +1665,16 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 		pipe_config->has_audio = intel_conn_state->force_audio == HDMI_AUDIO_ON;
 
 	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;
@@ -5814,6 +5835,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;
@@ -5869,13 +5891,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);
 		}
 	}
 
@@ -5912,7 +5935,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 d17a324..0c14b05 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;
 
 	/* backlight */
@@ -1696,6 +1697,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 50ec836..d3a6608 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -1851,7 +1851,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 6fe5d7c..a995ccb 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -1140,7 +1140,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 96c2cbd..4e7ba93 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1919,11 +1919,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

[PATCH v3 4/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.

v2: Rebase

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>
Cc: Jani Nikula <jani.nikula@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 ee2a3db..b4cb5ba 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -106,7 +106,7 @@ static const int default_rates[] = { 162000, 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);
 
@@ -4764,6 +4764,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
 
 		intel_dp->reset_link_params = false;
+		intel_dp->train_set_valid = false;
 	}
 
 	intel_dp_print_rates(intel_dp);
@@ -6037,6 +6038,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	intel_dp_set_source_rates(intel_dp);
 
 	intel_dp->reset_link_params = true;
+	intel_dp->train_set_valid = false;
 	intel_dp->pps_pipe = INVALID_PIPE;
 	intel_dp->active_pipe = INVALID_PIPE;
 
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index b79c1c0..60233e2 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -94,7 +94,8 @@ static bool
 intel_dp_reset_link_train(struct intel_dp *intel_dp,
 			uint8_t dp_train_pat)
 {
-	memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
+	if (!intel_dp->train_set_valid)
+		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
 	intel_dp_set_signal_levels(intel_dp);
 	return intel_dp_set_link_train(intel_dp, dp_train_pat);
 }
@@ -162,6 +163,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 				       DP_TRAINING_PATTERN_1 |
 				       DP_LINK_SCRAMBLING_DISABLE)) {
 		DRM_ERROR("failed to enable link training\n");
+		intel_dp->train_set_valid = false;
 		return false;
 	}
 
@@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 
 		if (!intel_dp_get_link_status(intel_dp, link_status)) {
 			DRM_ERROR("failed to get link status\n");
+			intel_dp->train_set_valid = false;
 			return false;
 		}
 
 		if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
 			DRM_DEBUG_KMS("clock recovery OK\n");
+			intel_dp->train_set_valid = is_edp(intel_dp);
 			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 0c14b05..a1b4b5d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -996,6 +996,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;
@@ -1571,6 +1572,7 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
 }
 
 bool intel_dp_read_dpcd(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

✗ Fi.CI.BAT: failure for Kernel PSR Fix-ups

[Patchwork]

== Series Details ==

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

== Summary ==

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

Test core_auth:
        Subgroup basic-auth:
                pass       -> INCOMPLETE (fi-skl-6700hq)
                pass       -> INCOMPLETE (fi-kbl-r)
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> SKIP       (fi-skl-x1585l)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-pnv-d510) fdo#101597

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

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:440s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:426s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:355s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:524s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:508s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:490s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:482s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:436s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:412s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:420s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:497s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:465s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:463s
fi-kbl-7560u     total:279  pass:268  dwarn:1   dfail:0   fail:0   skip:10  time:582s
fi-kbl-r         total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:562s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:456s
fi-skl-6700hq    total:279  pass:0    dwarn:0   dfail:0   fail:0   skip:0  
fi-skl-6700k     total:279  pass:257  dwarn:4   dfail:0   fail:0   skip:18  time:471s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:483s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:433s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:470s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:542s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:400s
fi-glk-2a failed to connect after reboot

8ad9e19aafea47c272163c2cbf554e06ff7f9857 drm-tip: 2017y-07m-11d-19h-08m-20s UTC integration manifest
472f04a drm/i915/edp: Be less aggressive about changing link config on eDP
3a25950 drm/i915/edp: Allow alternate fixed mode for eDP if available.
6e3212d drm/i915/psr: Account for sink CRC raciness on some panels
6798070 drm/i915/psr: Clean-up intel_enable_source_psr1()

== Logs ==

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

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

[Chris Wilson]

Quoting Jim Bride (2017-07-11 23:19:56)
> @@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>  
>                 if (!intel_dp_get_link_status(intel_dp, link_status)) {
>                         DRM_ERROR("failed to get link status\n");
> +                       intel_dp->train_set_valid = false;
>                         return false;
>                 }
>  
>                 if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
>                         DRM_DEBUG_KMS("clock recovery OK\n");
> +                       intel_dp->train_set_valid = is_edp(intel_dp);

Ouch, that was hard to spot amongst the decoys. How about setting
intel_dp->train_set_valid = false at the very start of training, and
only on success set it to true, something like

@@ -316,6 +316,8 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 {
        struct intel_connector *intel_connector = intel_dp->attached_connector;
 
+       intel_dp->train_set_valid = false;
+
        if (!intel_dp_link_training_clock_recovery(intel_dp))
                goto failure_handling;
        if (!intel_dp_link_training_channel_equalization(intel_dp))
@@ -323,6 +325,11 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 
        DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d",
                      intel_dp->link_rate, intel_dp->lane_count);
+       /*
+        * Record the last known working parameters for quick retraining
+        * next time. We only trust eDP for now.
+        */
+       intel_dp->train_set_valid = is_edp(intel_dp);
        return;
 
  failure_handling:
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Jim Bride (2017-07-11 23:19:55)
> @@ -5869,13 +5891,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);
>                 }
>         }

Save one, leak the rest?

> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 96c2cbd..4e7ba93 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1919,11 +1919,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;

Ah, at least it is consistent; they all get leaked. :)

In general though, it looks like for edp you want to scan
connector->probed_modes at config time looking for an exact match in
preference to using the scaled mode.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Vivi, Rodrigo]

If you had sent these 2 in a separated series I believe it would had
passed CI so I could merge today.

Also it would be better if you want to speed up things with a bit of
sense of progress since the other 2 patches in this series will probably
require some rework.

On Tue, 2017-07-11 at 15:19 -0700, Jim Bride 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.
> 
> v2: * Reduce number of retries when reading the sink CRC (Jani)
>     * Refactor to minimize changes to the code (Jani)
>     * Rebase
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 40 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 2d42d09..69c8130c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3906,6 +3906,14 @@ 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);
> +		memset(old_crc, 0xff, 6);
> +	} else {
> +		return -ENOMEM;
> +	}
>  
>  	ret = intel_dp_sink_crc_start(intel_dp);
>  	if (ret)
> @@ -3929,11 +3937,35 @@ 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;
> +
> +	/*
> +	 * Sometimes it takes a while for the "real" CRC values to land in
> +	 * the DPCD, so try several times until we get two reads in a row
> +	 * that are the same.  If we're an eDP panel, delay between reads
> +	 * for a while since the values take a bit longer to propagate.
> +	 */
> +	do {
> +		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> +		if (is_edp(intel_dp))
> +			usleep_range(20000, 25000);
> +
> +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR,
> +				     crc, 6) < 0) {
> +			ret = -EIO;
> +			goto stop;
> +		}
> +
> +		if (memcmp(old_crc, crc, 6) == 0) {
> +			ret = 0;
> +			goto stop;
> +		} else {
> +			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;

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

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

[Dhinakaran Pandiyan]

On Tuesday, July 11, 2017 3:19:53 PM PDT Jim Bride 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 which bit is that?

> 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.

I see a caller to intel_psr_single_frame_update() 

> 
> v2: * Do a field-wise init on EDP_PSR_MAX_SLEEP_TIME even though we
>       always set it to the max value. (Rodrigo)
>     * Rebase
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Wayne Boyer <wayne.boyer@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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 c712d01..3e62429 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3789,18 +3789,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 559f1ab..132987b 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;

EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES is  (0<<25) 
We can just remove the two lines above, I think it's just misleading to do a 
nop after checking for Haswell.

Curious how the entry time is determined, the rest seem to come from VBT.

> 
> -	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;


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

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

[Dhinakaran Pandiyan]

On Tuesday, July 11, 2017 3:19:54 PM PDT Jim Bride 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.

What's the goal here? Is this retry loop being added to deal with an IGT 
failure? A bit of context as to how this is related to PSR will be helpful.

> 
> v2: * Reduce number of retries when reading the sink CRC (Jani)
>     * Refactor to minimize changes to the code (Jani)
>     * Rebase
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 40
> ++++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+),
> 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c index 2d42d09..69c8130c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3906,6 +3906,14 @@ 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) {

Why is this needed?

> +		memset(crc, 0, 6);
> +		memset(old_crc, 0xff, 6);

I don't know much about crc values, is 0xff a known invalid value?

> +	} else {
> +		return -ENOMEM;
> +	}
> 
>  	ret = intel_dp_sink_crc_start(intel_dp);
>  	if (ret)
> @@ -3929,11 +3937,35 @@ 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;
> +
> +	/*
> +	 * Sometimes it takes a while for the "real" CRC values to land in
> +	 * the DPCD, so try several times until we get two reads in a row
> +	 * that are the same.  If we're an eDP panel, delay between reads
> +	 * for a while since the values take a bit longer to propagate.
> +	 */
> +	do {
> +		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);

Why wait for a vblank if the idea is to check for consistent crc values for 
the same frame (I'm assuming that is the idea) ? 

> +		if (is_edp(intel_dp))
> +			usleep_range(20000, 25000);
> +
> +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR,
> +				     crc, 6) < 0) {
> +			ret = -EIO;
> +			goto stop;
> +		}
> +
> +		if (memcmp(old_crc, crc, 6) == 0) {
> +			ret = 0;
> +			goto stop;
> +		} else {
> +			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;


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

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

[Chris Wilson]

Quoting Dhinakaran Pandiyan (2017-07-12 09:47:25)
> On Tuesday, July 11, 2017 3:19:53 PM PDT Jim Bride 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 which bit is that?

I think we would all be happier with keeping the explicit construction
(and a smaller patch) if we used

	val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_CTL_RSVD_MASK;

Better name than reserved most welcome, since reserved often means "do
not set; reserved for past/future use" rather than must not modify but
keep. So EDP_PSR_CTL_KEEP_MASK ?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jim Bride]

On Wed, Jul 12, 2017 at 12:27:33AM +0100, Chris Wilson wrote:
> Quoting Jim Bride (2017-07-11 23:19:55)
> > @@ -5869,13 +5891,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);
> >                 }
> >         }
> 
> Save one, leak the rest?

Good catch, I'll tweak the code so that we only duplicate once,
although in practice (on the panels I have to test with) fixed
panels support either one or two modes, with only the frame
rate being different if two are supported.

> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index 96c2cbd..4e7ba93 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -1919,11 +1919,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;
> 
> Ah, at least it is consistent; they all get leaked. :)

Yeah, I missed adding a destroy call in intel_panel_fini().  I'll
fix that.

Thanks for the review!

Jim


> In general though, it looks like for edp you want to scan
> connector->probed_modes at config time looking for an exact match in
> preference to using the scaled mode.
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Manasi Navare]

On Tue, Jul 11, 2017 at 03:19:56PM -0700, Jim Bride wrote:
> This set of changes has some history to them.  There were several attempts
> to add what was called "fast link training" to i915, which actually wasn't
> fast link training as per the DP spec.  These changes were
> 
> 5fa836a9d859 ("drm/i915: DP link training optimization")
> 4e96c97742f4 ("drm/i915: eDP link training optimization")
> 
> which were eventually hand-reverted by
> 
> 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")
> 
> in kernel 4.7-rc4.  The eDP pieces of the above revert, however, had some
> very bad side-effects on PSR functionality on Skylake. The issue at
> hand is that when PSR exits i915 briefly emits TP1 followed by TP2/3
> (depending on the original link configuration) in order to quickly get
> the source and sink back in synchronization across the link before handing
> control back to the i915.  There's an assumption that none of the link
> configuration information has changed (and thus it's still valid) since the
> last full link training operation.  The revert above was identified via a
> bisect as the cause of some of Skylake's PSR woes.  This patch, largely
> based on
> 
> commit 4e96c97742f4201edf1b0f8e1b1b6b2ac6ff33e7
> Author: Mika Kahola <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.
> 
> v2: Rebase
> 
> 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>
> Cc: Jani Nikula <jani.nikula@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 ee2a3db..b4cb5ba 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -106,7 +106,7 @@ static const int default_rates[] = { 162000, 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);
>  
> @@ -4764,6 +4764,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  		intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
>  
>  		intel_dp->reset_link_params = false;
> +		intel_dp->train_set_valid = false;
>  	}
>  
>  	intel_dp_print_rates(intel_dp);
> @@ -6037,6 +6038,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	intel_dp_set_source_rates(intel_dp);
>  
>  	intel_dp->reset_link_params = true;
> +	intel_dp->train_set_valid = false;
>  	intel_dp->pps_pipe = INVALID_PIPE;
>  	intel_dp->active_pipe = INVALID_PIPE;
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index b79c1c0..60233e2 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -94,7 +94,8 @@ static bool
>  intel_dp_reset_link_train(struct intel_dp *intel_dp,
>  			uint8_t dp_train_pat)
>  {
> -	memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
> +	if (!intel_dp->train_set_valid)
> +		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
>  	intel_dp_set_signal_levels(intel_dp);
>  	return intel_dp_set_link_train(intel_dp, dp_train_pat);
>  }
> @@ -162,6 +163,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>  				       DP_TRAINING_PATTERN_1 |
>  				       DP_LINK_SCRAMBLING_DISABLE)) {
>  		DRM_ERROR("failed to enable link training\n");
> +		intel_dp->train_set_valid = false;
>  		return false;
>  	}
>  
> @@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>  
>  		if (!intel_dp_get_link_status(intel_dp, link_status)) {
>  			DRM_ERROR("failed to get link status\n");
> +			intel_dp->train_set_valid = false;
>  			return false;
>  		}
>  
>  		if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
>  			DRM_DEBUG_KMS("clock recovery OK\n");
> +			intel_dp->train_set_valid = is_edp(intel_dp);
>  			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");
>  	}
>

I see that train_set_valid is set to false in case any kind of failure
and when the function is return a false.
If you see this false is handled in failure_handling: inside intel_dp_start_link_train. So instead of setting train_set_valid  as false each time when we return false, it makes more sense to set it in failure_handling label.

Also, in intel_dp_link_training_channel_equalization(), when drm_dp_channel_eq_ok(), dont we need to set train_set_valid to is_edp() or true?

Manasi


> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0c14b05..a1b4b5d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -996,6 +996,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;
> @@ -1571,6 +1572,7 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
>  }
>  
>  bool intel_dp_read_dpcd(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 v3 4/4] drm/i915/edp: Be less aggressive about changing link config on eDP

[Manasi Navare]

On Wed, Jul 12, 2017 at 12:16:13AM +0100, Chris Wilson wrote:
> Quoting Jim Bride (2017-07-11 23:19:56)
> > @@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> >  
> >                 if (!intel_dp_get_link_status(intel_dp, link_status)) {
> >                         DRM_ERROR("failed to get link status\n");
> > +                       intel_dp->train_set_valid = false;
> >                         return false;
> >                 }
> >  
> >                 if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
> >                         DRM_DEBUG_KMS("clock recovery OK\n");
> > +                       intel_dp->train_set_valid = is_edp(intel_dp);
> 
> Ouch, that was hard to spot amongst the decoys. How about setting
> intel_dp->train_set_valid = false at the very start of training, and
> only on success set it to true, something like
>

Or like I suggested, just set train_set_valid to false in the
failure_handling and set it to true only on success.

Manasi


> @@ -316,6 +316,8 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>  {
>         struct intel_connector *intel_connector = intel_dp->attached_connector;
>  
> +       intel_dp->train_set_valid = false;
> +
>         if (!intel_dp_link_training_clock_recovery(intel_dp))
>                 goto failure_handling;
>         if (!intel_dp_link_training_channel_equalization(intel_dp))
> @@ -323,6 +325,11 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>  
>         DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d",
>                       intel_dp->link_rate, intel_dp->lane_count);
> +       /*
> +        * Record the last known working parameters for quick retraining
> +        * next time. We only trust eDP for now.
> +        */
> +       intel_dp->train_set_valid = is_edp(intel_dp);
>         return;
>  
>   failure_handling:
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Manasi Navare (2017-07-12 22:36:49)
> On Wed, Jul 12, 2017 at 12:16:13AM +0100, Chris Wilson wrote:
> > Quoting Jim Bride (2017-07-11 23:19:56)
> > > @@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > >  
> > >                 if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > >                         DRM_ERROR("failed to get link status\n");
> > > +                       intel_dp->train_set_valid = false;
> > >                         return false;
> > >                 }
> > >  
> > >                 if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
> > >                         DRM_DEBUG_KMS("clock recovery OK\n");
> > > +                       intel_dp->train_set_valid = is_edp(intel_dp);
> > 
> > Ouch, that was hard to spot amongst the decoys. How about setting
> > intel_dp->train_set_valid = false at the very start of training, and
> > only on success set it to true, something like
> >
> 
> Or like I suggested, just set train_set_valid to false in the
> failure_handling and set it to true only on success.

It just looked a little crowded in the failure_handling: whereas at the
start of the function, there was plenty of whitespace for it to stand
out. That was all I was thinking.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Manasi Navare]

On Wed, Jul 12, 2017 at 10:38:03PM +0100, Chris Wilson wrote:
> Quoting Manasi Navare (2017-07-12 22:36:49)
> > On Wed, Jul 12, 2017 at 12:16:13AM +0100, Chris Wilson wrote:
> > > Quoting Jim Bride (2017-07-11 23:19:56)
> > > > @@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > > >  
> > > >                 if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > > >                         DRM_ERROR("failed to get link status\n");
> > > > +                       intel_dp->train_set_valid = false;
> > > >                         return false;
> > > >                 }
> > > >  
> > > >                 if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
> > > >                         DRM_DEBUG_KMS("clock recovery OK\n");
> > > > +                       intel_dp->train_set_valid = is_edp(intel_dp);
> > > 
> > > Ouch, that was hard to spot amongst the decoys. How about setting
> > > intel_dp->train_set_valid = false at the very start of training, and
> > > only on success set it to true, something like
> > >
> > 
> > Or like I suggested, just set train_set_valid to false in the
> > failure_handling and set it to true only on success.
> 
> It just looked a little crowded in the failure_handling: whereas at the
> start of the function, there was plenty of whitespace for it to stand
> out. That was all I was thinking.
> -Chris

But at the beginning of the function, it is anyway set to False since
intel_dp_init_connector sets it to false.
So we dont need to again set it to false at the beginning of the function right?
Then we only set it to true when it succeeds so after 1 successful link training
it will be set to true.
And now since this code will also be used fro DP case, we need to make sure
we set this to false in failure handling so that it resets link train during
link train fallback.
Makes sense?

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

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

[Jim Bride]

On Wed, Jul 12, 2017 at 02:53:36PM -0700, Manasi Navare wrote:
> On Wed, Jul 12, 2017 at 10:38:03PM +0100, Chris Wilson wrote:
> > Quoting Manasi Navare (2017-07-12 22:36:49)
> > > On Wed, Jul 12, 2017 at 12:16:13AM +0100, Chris Wilson wrote:
> > > > Quoting Jim Bride (2017-07-11 23:19:56)
> > > > > @@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > > > >  
> > > > >                 if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > > > >                         DRM_ERROR("failed to get link status\n");
> > > > > +                       intel_dp->train_set_valid = false;
> > > > >                         return false;
> > > > >                 }
> > > > >  
> > > > >                 if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
> > > > >                         DRM_DEBUG_KMS("clock recovery OK\n");
> > > > > +                       intel_dp->train_set_valid = is_edp(intel_dp);
> > > > 
> > > > Ouch, that was hard to spot amongst the decoys. How about setting
> > > > intel_dp->train_set_valid = false at the very start of training, and
> > > > only on success set it to true, something like
> > > >
> > > 
> > > Or like I suggested, just set train_set_valid to false in the
> > > failure_handling and set it to true only on success.
> > 
> > It just looked a little crowded in the failure_handling: whereas at the
> > start of the function, there was plenty of whitespace for it to stand
> > out. That was all I was thinking.
> > -Chris
> 
> But at the beginning of the function, it is anyway set to False since
> intel_dp_init_connector sets it to false.
> So we dont need to again set it to false at the beginning of the function right?
> Then we only set it to true when it succeeds so after 1 successful link training
> it will be set to true.
> And now since this code will also be used fro DP case, we need to make sure
> we set this to false in failure handling so that it resets link train during
> link train fallback.
> Makes sense?

What seems the easiest to me is to set intel_dp->train_set_valid
to false right before the main clock recovery loop (but after calling
intel_dp_reset_link_train(), which is what uses the value) in
intel_dp_link_training_clock_recovery(), and remove the
sets to false for the failing cases.  If clock recovery is ok,
then we set it to true and return.  This makes it much easier
to sort out when we succeed by reading the code without changing
any of the existing functionality.  New patch coming as soon as I
get done testing it.

Jim


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

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

[Jani Nikula]

On Wed, 12 Jul 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Dhinakaran Pandiyan (2017-07-12 09:47:25)
>> On Tuesday, July 11, 2017 3:19:53 PM PDT Jim Bride 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 which bit is that?
>
> I think we would all be happier with keeping the explicit construction
> (and a smaller patch) if we used
>
> 	val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_CTL_RSVD_MASK;

Agreed. Avoid read-modify-write as much as possible.

BR,
Jani.


-- 
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 v3 2/4] drm/i915/psr: Account for sink CRC raciness on some panels

[Jani Nikula]

On Tue, 11 Jul 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.
>
> v2: * Reduce number of retries when reading the sink CRC (Jani)
>     * Refactor to minimize changes to the code (Jani)
>     * Rebase
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 40 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 2d42d09..69c8130c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3906,6 +3906,14 @@ 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) {

As DK said, please drop the check.

> +		memset(crc, 0, 6);
> +		memset(old_crc, 0xff, 6);

Both unnecessary, see below.

> +	} else {
> +		return -ENOMEM;
> +	}
>  
>  	ret = intel_dp_sink_crc_start(intel_dp);
>  	if (ret)
> @@ -3929,11 +3937,35 @@ 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;
> +
> +	/*
> +	 * Sometimes it takes a while for the "real" CRC values to land in
> +	 * the DPCD, so try several times until we get two reads in a row
> +	 * that are the same.  If we're an eDP panel, delay between reads
> +	 * for a while since the values take a bit longer to propagate.
> +	 */
> +	do {

Never use a do-while when a for loop will do. for (i = 0; i < 6; i++)
gets interpreted in the spine, no need for further processing.

> +		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> +		if (is_edp(intel_dp))
> +			usleep_range(20000, 25000);

Is the intention to do these *between* reads? If yes, then move this
*after* the memcmp to only do this between reads.

> +
> +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR,
> +				     crc, 6) < 0) {
> +			ret = -EIO;
> +			goto stop;

break;

> +		}
> +
> +		if (memcmp(old_crc, crc, 6) == 0) {
> +			ret = 0;
> +			goto stop;
> +		} else {
> +			memcpy(old_crc, crc, 6);
> +		}

After you've switched this to the for loop, you can do:

	if (i && memcmp(old_crc, crc, sizeof(old_crc)) == 0)
        	break;
	memcpy(old_crc, crc, sizeof(old_crc));

> +	} while (--attempts);
>  

	if (i == 6) {

> +	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 v3 2/4] drm/i915/psr: Account for sink CRC raciness on some panels

[Jim Bride]

On Fri, Jul 14, 2017 at 12:46:08PM +0300, Jani Nikula wrote:
> On Tue, 11 Jul 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.
> >
> > v2: * Reduce number of retries when reading the sink CRC (Jani)
> >     * Refactor to minimize changes to the code (Jani)
> >     * Rebase
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 40 ++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 36 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 2d42d09..69c8130c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3906,6 +3906,14 @@ 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) {
> 
> As DK said, please drop the check.
> 
> > +		memset(crc, 0, 6);
> > +		memset(old_crc, 0xff, 6);
> 
> Both unnecessary, see below.
> 
> > +	} else {
> > +		return -ENOMEM;
> > +	}
> >  
> >  	ret = intel_dp_sink_crc_start(intel_dp);
> >  	if (ret)
> > @@ -3929,11 +3937,35 @@ 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;
> > +
> > +	/*
> > +	 * Sometimes it takes a while for the "real" CRC values to land in
> > +	 * the DPCD, so try several times until we get two reads in a row
> > +	 * that are the same.  If we're an eDP panel, delay between reads
> > +	 * for a while since the values take a bit longer to propagate.
> > +	 */
> > +	do {
> 
> Never use a do-while when a for loop will do. for (i = 0; i < 6; i++)
> gets interpreted in the spine, no need for further processing.

I had used do-while because it was used earlier to read the test
count (code I didn't touch.)  In any event, I can convert to a
for loop easy enough.

> > +		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> > +		if (is_edp(intel_dp))
> > +			usleep_range(20000, 25000);
> 
> Is the intention to do these *between* reads? If yes, then move this
> *after* the memcmp to only do this between reads.

Moved.

> > +
> > +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR,
> > +				     crc, 6) < 0) {
> > +			ret = -EIO;
> > +			goto stop;
> 
> break;

Changed.

> > +		}
> > +
> > +		if (memcmp(old_crc, crc, 6) == 0) {
> > +			ret = 0;
> > +			goto stop;
> > +		} else {
> > +			memcpy(old_crc, crc, 6);
> > +		}
> 
> After you've switched this to the for loop, you can do:
> 
> 	if (i && memcmp(old_crc, crc, sizeof(old_crc)) == 0)
>         	break;
> 	memcpy(old_crc, crc, sizeof(old_crc));

Changed.  I'll submit a new version of the patch after I retest.

Jim

> > +	} while (--attempts);
> >  
> 
> 	if (i == 6) {
> 
> > +	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

[PATCH v4 1/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)
    * Rebase
v3-v4: * Rebase

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Wayne Boyer <wayne.boyer@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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 c712d01..3e62429 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3789,18 +3789,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 559f1ab..132987b 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

[PATCH v4 2/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.

v2: * Reduce number of retries when reading the sink CRC (Jani)
    * Refactor to minimize changes to the code (Jani)
    * Rebase
v3: * Rebase
v4: * Switch from do-while to for loop when reading CRC values (Jani)
    * Rebase
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2d42d09..c90ca1c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3906,6 +3906,11 @@ 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) {
+		return -ENOMEM;
+	}
 
 	ret = intel_dp_sink_crc_start(intel_dp);
 	if (ret)
@@ -3929,11 +3934,33 @@ 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;
+	/*
+	 * Sometimes it takes a while for the "real" CRC values to land in
+	 * the DPCD, so try several times until we get two reads in a row
+	 * that are the same.  If we're an eDP panel, delay between reads
+	 * for a while since the values take a bit longer to propagate.
+	 */
+	for (attempts = 0; attempts < 6; attempts++) {
+		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
+
+		if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR,
+				     crc, 6) < 0) {
+			ret = -EIO;
+			break;
+		}
+
+		if (attempts && memcmp(old_crc, crc, 6) == 0)
+			break;
+		memcpy(old_crc, crc, 6);
+
+		if (is_edp(intel_dp))
+			usleep_range(20000, 25000);
 	}
 
+	if (attempts == 6) {
+		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

[PATCH v4 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.

v2 and v3: Rebase
v4: * Clean up accesses to train_set_valid a bit for easier reading. (Chris)
    * Rebase

Cc: Chris Wilson <chris@chris-wilson.co.uk>
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>
Cc: Jani Nikula <jani.nikula@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 | 15 ++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h              |  2 ++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c90ca1c..7c0e530 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);
 
@@ -4738,6 +4738,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);
@@ -6008,6 +6009,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..d12200d 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -94,7 +94,8 @@ static bool
 intel_dp_reset_link_train(struct intel_dp *intel_dp,
 			uint8_t dp_train_pat)
 {
-	memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
+	if (!intel_dp->train_set_valid)
+		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
 	intel_dp_set_signal_levels(intel_dp);
 	return intel_dp_set_link_train(intel_dp, dp_train_pat);
 }
@@ -162,9 +163,18 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 				       DP_TRAINING_PATTERN_1 |
 				       DP_LINK_SCRAMBLING_DISABLE)) {
 		DRM_ERROR("failed to enable link training\n");
+		intel_dp->train_set_valid = false;
 		return false;
 	}
 
+	/*
+	 * The initial set of link parameters are set by this point, so go
+	 * ahead and set intel_dp->train_set_valid to false in case any of
+	 * the succeeding steps fail.  It will be set back to true if we were
+	 * able to achieve clock recovery in the specified configuration.
+	 */
+	intel_dp->train_set_valid = false;
+
 	voltage_tries = 1;
 	max_vswing_tries = 0;
 	for (;;) {
@@ -179,6 +189,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 
 		if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
 			DRM_DEBUG_KMS("clock recovery OK\n");
+			intel_dp->train_set_valid = is_edp(intel_dp);
 			return true;
 		}
 
@@ -256,6 +267,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 +308,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 0902d7c..e45163a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -995,6 +995,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;
@@ -1570,6 +1571,7 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
 }
 
 bool intel_dp_read_dpcd(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

[PATCH v4 4/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.

v2 and v3: Rebase
v4: * Fix up some leaky mode stuff (Chris)
    * Rebase

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
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 |  6 ++++++
 6 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7c0e530..c9db0e6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1606,6 +1606,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,
@@ -1652,8 +1665,16 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 		pipe_config->has_audio = intel_conn_state->force_audio == HDMI_AUDIO_ON;
 
 	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;
@@ -5810,6 +5831,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;
@@ -5865,13 +5887,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 if (!alt_fixed_mode) {
+			alt_fixed_mode = drm_mode_duplicate(dev, scan);
 		}
 	}
 
@@ -5908,7 +5931,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 e45163a..3bd11e2 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;
 
 	/* backlight */
@@ -1698,6 +1699,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 50ec836..d3a6608 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -1851,7 +1851,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 6fe5d7c..a995ccb 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -1140,7 +1140,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 96c2cbd..aa59b12 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1919,11 +1919,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;
@@ -1937,6 +1939,10 @@ void intel_panel_fini(struct intel_panel *panel)
 	if (panel->fixed_mode)
 		drm_mode_destroy(intel_connector->base.dev, panel->fixed_mode);
 
+	if (panel->alt_fixed_mode)
+		drm_mode_destroy(intel_connector->base.dev,
+				panel->alt_fixed_mode);
+
 	if (panel->downclock_mode)
 		drm_mode_destroy(intel_connector->base.dev,
 				panel->downclock_mode);
-- 
2.7.4

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

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

[Rodrigo Vivi]

On Tue, Jul 18, 2017 at 2:34 PM, 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.

Is DK now ok with this description?
I believe he requested more info here.

>
> v2: * Reduce number of retries when reading the sink CRC (Jani)
>     * Refactor to minimize changes to the code (Jani)
>     * Rebase
> v3: * Rebase
> v4: * Switch from do-while to for loop when reading CRC values (Jani)
>     * Rebase
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 2d42d09..c90ca1c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3906,6 +3906,11 @@ 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) {
> +               return -ENOMEM;
> +       }

wouldn't we drop this check per DK and Jani request?
I believe we don't need it, but even if there are cases that we need
we could remove the braces..

>
>         ret = intel_dp_sink_crc_start(intel_dp);
>         if (ret)
> @@ -3929,11 +3934,33 @@ 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;
> +       /*
> +        * Sometimes it takes a while for the "real" CRC values to land in
> +        * the DPCD, so try several times until we get two reads in a row
> +        * that are the same.  If we're an eDP panel, delay between reads
> +        * for a while since the values take a bit longer to propagate.
> +        */
> +       for (attempts = 0; attempts < 6; attempts++) {
> +               intel_wait_for_vblank(dev_priv, intel_crtc->pipe);

DK, we need vblank wait because the crc calculation also may take one vblank.
usually 2 actually... one to make sure you have the full screen
updated and one for the calculation.
In the past when we didn't used the count we were waiting 2 vblanks...

> +
> +               if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR,
> +                                    crc, 6) < 0) {
> +                       ret = -EIO;
> +                       break;
> +               }
> +
> +               if (attempts && memcmp(old_crc, crc, 6) == 0)
> +                       break;
> +               memcpy(old_crc, crc, 6);

little bikeshed: too many hardcoded "6" around... a sizeof would be better...
but whatever...

> +
> +               if (is_edp(intel_dp))
> +                       usleep_range(20000, 25000);
>         }
>
> +       if (attempts == 6) {
> +               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



-- 
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

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

[Jim Bride]

On Fri, Jul 14, 2017 at 12:34:28PM +0300, Jani Nikula wrote:
> On Wed, 12 Jul 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Quoting Dhinakaran Pandiyan (2017-07-12 09:47:25)
> >> On Tuesday, July 11, 2017 3:19:53 PM PDT Jim Bride 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 which bit is that?

Bit 29 (Context restore to PSR Active) in SRD_CTL.  I'll add it to the
commit message.  It's worth noting that the bit is not technically
reserved, but rather that SW is not allowed to change it.

> >
> > I think we would all be happier with keeping the explicit construction
> > (and a smaller patch) if we used
> >
> > 	val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_CTL_RSVD_MASK;
> 
> Agreed. Avoid read-modify-write as much as possible.

I can do this if everyone thinks it's the thing to do, but it
does open us up to a similar class of bug (B-Spec restricting mods
to a bit / bit range after initial support for a platform was added)
in the future.  IMHO the code as written is safer.

Jim


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

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

[Jani Nikula]

On Thu, 03 Aug 2017, Jim Bride <jim.bride@linux.intel.com> wrote:
> On Fri, Jul 14, 2017 at 12:34:28PM +0300, Jani Nikula wrote:
>> On Wed, 12 Jul 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > Quoting Dhinakaran Pandiyan (2017-07-12 09:47:25)
>> >> On Tuesday, July 11, 2017 3:19:53 PM PDT Jim Bride 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 which bit is that?
>
> Bit 29 (Context restore to PSR Active) in SRD_CTL.  I'll add it to the
> commit message.  It's worth noting that the bit is not technically
> reserved, but rather that SW is not allowed to change it.
>
>> >
>> > I think we would all be happier with keeping the explicit construction
>> > (and a smaller patch) if we used
>> >
>> > 	val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_CTL_RSVD_MASK;
>> 
>> Agreed. Avoid read-modify-write as much as possible.
>
> I can do this if everyone thinks it's the thing to do, but it
> does open us up to a similar class of bug (B-Spec restricting mods
> to a bit / bit range after initial support for a platform was added)
> in the future.  IMHO the code as written is safer.

Chris' suggestion preserves the restricted bits that must remain the
same, while initializing everything else. Instead of only changing the
bits we must change, only preserve the bits we must not change. Sorry if
I wasn't clear with the "as much as possible" part there.

Preserving the restricted bits is a functional change, and the subject
of this patch does not reflect that. When I look at the logs, I pretty
much expect clean up commits to be non-functional. There are some areas
where I'd look the other way, but PSR is something where we must
carefully split up the patches and write the commit messages diligently,
because I know we will be spending time debugging this code and reading
the logs.

BR,
Jani.



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

-- 
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 v4 2/4] drm/i915/psr: Account for sink CRC raciness on some panels

[Pandiyan, Dhinakaran]

On Thu, 2017-08-03 at 11:07 -0700, Rodrigo Vivi wrote:
> On Tue, Jul 18, 2017 at 2:34 PM, 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.

I'm curious if we get the correct crc if we waited a full second.

> 
> Is DK now ok with this description?
> I believe he requested more info here.
> 
> >
> > v2: * Reduce number of retries when reading the sink CRC (Jani)
> >     * Refactor to minimize changes to the code (Jani)
> >     * Rebase
> > v3: * Rebase
> > v4: * Switch from do-while to for loop when reading CRC values (Jani)
> >     * Rebase
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 33 ++++++++++++++++++++++++++++++---
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 2d42d09..c90ca1c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3906,6 +3906,11 @@ 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) {
> > +               return -ENOMEM;
> > +       }
> 
> wouldn't we drop this check per DK and Jani request?
> I believe we don't need it, but even if there are cases that we need
> we could remove the braces..
> 

Yeah, crc is allocated on the stack. If that is null, we'll have bigger
problems to deal with. And I think it's reasonable to assume the caller
is sending a valid array to fill data.

> >
> >         ret = intel_dp_sink_crc_start(intel_dp);
> >         if (ret)
> > @@ -3929,11 +3934,33 @@ 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;
> > +       /*
> > +        * Sometimes it takes a while for the "real" CRC values to land in
> > +        * the DPCD, so try several times until we get two reads in a row
> > +        * that are the same.  If we're an eDP panel, delay between reads
> > +        * for a while since the values take a bit longer to propagate.
> > +        */
> > +       for (attempts = 0; attempts < 6; attempts++) {
> > +               intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> 
> DK, we need vblank wait because the crc calculation also may take one vblank.
> usually 2 actually... one to make sure you have the full screen
> updated and one for the calculation.
> In the past when we didn't used the count we were waiting 2 vblanks...
> 


Thanks for the clarification. My reasoning was, after the first two
vblank_waits for the sink to calculate crc, the ones in the retry path
were unnecessary. We just need some delay before reading the dpcd again
without having to enable vblank interrupts. Anyway, the number of
retries is low enough that it shouldn't matter.

On the other hand, since the only consumers of dp sink crc are tests,
why can't the kernel just dump what it reads to debugfs and let the test
deal with erroneous results?

> > +
> > +               if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR,
> > +                                    crc, 6) < 0) {
> > +                       ret = -EIO;
> > +                       break;
> > +               }
> > +
> > +               if (attempts && memcmp(old_crc, crc, 6) == 0)
> > +                       break;
> > +               memcpy(old_crc, crc, 6);
> 
> little bikeshed: too many hardcoded "6" around... a sizeof would be better...
> but whatever...
> 
> > +
> > +               if (is_edp(intel_dp))
> > +                       usleep_range(20000, 25000);
> >         }
> >
> > +       if (attempts == 6) {
> > +               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
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jim Bride]

On Fri, Aug 04, 2017 at 10:29:33AM +0300, Jani Nikula wrote:
> On Thu, 03 Aug 2017, Jim Bride <jim.bride@linux.intel.com> wrote:
> > On Fri, Jul 14, 2017 at 12:34:28PM +0300, Jani Nikula wrote:
> >> On Wed, 12 Jul 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> > Quoting Dhinakaran Pandiyan (2017-07-12 09:47:25)
> >> >> On Tuesday, July 11, 2017 3:19:53 PM PDT Jim Bride 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 which bit is that?
> >
> > Bit 29 (Context restore to PSR Active) in SRD_CTL.  I'll add it to the
> > commit message.  It's worth noting that the bit is not technically
> > reserved, but rather that SW is not allowed to change it.
> >
> >> >
> >> > I think we would all be happier with keeping the explicit construction
> >> > (and a smaller patch) if we used
> >> >
> >> > 	val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_CTL_RSVD_MASK;
> >> 
> >> Agreed. Avoid read-modify-write as much as possible.
> >
> > I can do this if everyone thinks it's the thing to do, but it
> > does open us up to a similar class of bug (B-Spec restricting mods
> > to a bit / bit range after initial support for a platform was added)
> > in the future.  IMHO the code as written is safer.
> 
> Chris' suggestion preserves the restricted bits that must remain the
> same, while initializing everything else. Instead of only changing the
> bits we must change, only preserve the bits we must not change. Sorry if
> I wasn't clear with the "as much as possible" part there.

I think I followed you.  What I was trying to highlight is that the
patch as written doesn't touch anything other than what we explicitly
need to initialize.  While Chris' suggestion is much more terse, it
leaves us open to another bit being flagged out as 'software
shouldn't change' and we'd have a similar bug again.  The patch as
written doesn't expose us to that situation.  I'm happy to go with
Chris' suggestion if everyone still thinks it's the right thing, but
I wanted to highlight that it's not entirely equivalent to what was
in the original patch and in my opinion it's less safe than the
original patch.

> Preserving the restricted bits is a functional change, and the subject
> of this patch does not reflect that. When I look at the logs, I pretty
> much expect clean up commits to be non-functional. There are some areas
> where I'd look the other way, but PSR is something where we must
> carefully split up the patches and write the commit messages diligently,
> because I know we will be spending time debugging this code and reading
> the logs.

I will remove the word 'clean-up' and reword the subject, independent
of what we decide relative to the two approaches described above.
The body of the commit message (IMHO) does a good job (and I'll add
the specific bit in SRD_CTL to the body also) of describing the
functional changes that the patch makes.

Jim

> BR,
> Jani.
> 
> 
> 
> >
> > Jim
> >
> >
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> -- 
> >> Jani Nikula, Intel Open Source Technology Center
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> 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 v4 2/4] drm/i915/psr: Account for sink CRC raciness on some panels

[Jim Bride]

On Fri, Aug 04, 2017 at 06:38:02PM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Thu, 2017-08-03 at 11:07 -0700, Rodrigo Vivi wrote:
> > On Tue, Jul 18, 2017 at 2:34 PM, 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.
> 
> I'm curious if we get the correct crc if we waited a full second.

On SKL, times less than a second work fine generally.  On KBL, the
sink CRC is *way* less reliable, and I've seen runs where I set the
retry counts ridiculously high (> 30) and still not received valid
values.


Jim


> > 
> > Is DK now ok with this description?
> > I believe he requested more info here.
> > 
> > >
> > > v2: * Reduce number of retries when reading the sink CRC (Jani)
> > >     * Refactor to minimize changes to the code (Jani)
> > >     * Rebase
> > > v3: * Rebase
> > > v4: * Switch from do-while to for loop when reading CRC values (Jani)
> > >     * Rebase
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 33 ++++++++++++++++++++++++++++++---
> > >  1 file changed, 30 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 2d42d09..c90ca1c 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3906,6 +3906,11 @@ 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) {
> > > +               return -ENOMEM;
> > > +       }
> > 
> > wouldn't we drop this check per DK and Jani request?
> > I believe we don't need it, but even if there are cases that we need
> > we could remove the braces..
> > 
> 
> Yeah, crc is allocated on the stack. If that is null, we'll have bigger
> problems to deal with. And I think it's reasonable to assume the caller
> is sending a valid array to fill data.
> 
> > >
> > >         ret = intel_dp_sink_crc_start(intel_dp);
> > >         if (ret)
> > > @@ -3929,11 +3934,33 @@ 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;
> > > +       /*
> > > +        * Sometimes it takes a while for the "real" CRC values to land in
> > > +        * the DPCD, so try several times until we get two reads in a row
> > > +        * that are the same.  If we're an eDP panel, delay between reads
> > > +        * for a while since the values take a bit longer to propagate.
> > > +        */
> > > +       for (attempts = 0; attempts < 6; attempts++) {
> > > +               intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> > 
> > DK, we need vblank wait because the crc calculation also may take one vblank.
> > usually 2 actually... one to make sure you have the full screen
> > updated and one for the calculation.
> > In the past when we didn't used the count we were waiting 2 vblanks...
> > 
> 
> 
> Thanks for the clarification. My reasoning was, after the first two
> vblank_waits for the sink to calculate crc, the ones in the retry path
> were unnecessary. We just need some delay before reading the dpcd again
> without having to enable vblank interrupts. Anyway, the number of
> retries is low enough that it shouldn't matter.
> 
> On the other hand, since the only consumers of dp sink crc are tests,
> why can't the kernel just dump what it reads to debugfs and let the test
> deal with erroneous results?
> 
> > > +
> > > +               if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR,
> > > +                                    crc, 6) < 0) {
> > > +                       ret = -EIO;
> > > +                       break;
> > > +               }
> > > +
> > > +               if (attempts && memcmp(old_crc, crc, 6) == 0)
> > > +                       break;
> > > +               memcpy(old_crc, crc, 6);
> > 
> > little bikeshed: too many hardcoded "6" around... a sizeof would be better...
> > but whatever...
> > 
> > > +
> > > +               if (is_edp(intel_dp))
> > > +                       usleep_range(20000, 25000);
> > >         }
> > >
> > > +       if (attempts == 6) {
> > > +               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
> > 
> > 
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jim Bride]

On Mon, Aug 07, 2017 at 08:55:00AM -0700, Jim Bride wrote:
> On Fri, Aug 04, 2017 at 10:29:33AM +0300, Jani Nikula wrote:
> > On Thu, 03 Aug 2017, Jim Bride <jim.bride@linux.intel.com> wrote:
> > > On Fri, Jul 14, 2017 at 12:34:28PM +0300, Jani Nikula wrote:
> > >> On Wed, 12 Jul 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > >> > Quoting Dhinakaran Pandiyan (2017-07-12 09:47:25)
> > >> >> On Tuesday, July 11, 2017 3:19:53 PM PDT Jim Bride 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 which bit is that?
> > >
> > > Bit 29 (Context restore to PSR Active) in SRD_CTL.  I'll add it to the
> > > commit message.  It's worth noting that the bit is not technically
> > > reserved, but rather that SW is not allowed to change it.
> > >
> > >> >
> > >> > I think we would all be happier with keeping the explicit construction
> > >> > (and a smaller patch) if we used
> > >> >
> > >> > 	val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_CTL_RSVD_MASK;
> > >> 
> > >> Agreed. Avoid read-modify-write as much as possible.
> > >
> > > I can do this if everyone thinks it's the thing to do, but it
> > > does open us up to a similar class of bug (B-Spec restricting mods
> > > to a bit / bit range after initial support for a platform was added)
> > > in the future.  IMHO the code as written is safer.
> > 
> > Chris' suggestion preserves the restricted bits that must remain the
> > same, while initializing everything else. Instead of only changing the
> > bits we must change, only preserve the bits we must not change. Sorry if
> > I wasn't clear with the "as much as possible" part there.
> 
> I think I followed you.  What I was trying to highlight is that the
> patch as written doesn't touch anything other than what we explicitly
> need to initialize.  While Chris' suggestion is much more terse, it
> leaves us open to another bit being flagged out as 'software
> shouldn't change' and we'd have a similar bug again.  The patch as
> written doesn't expose us to that situation.  I'm happy to go with
> Chris' suggestion if everyone still thinks it's the right thing, but
> I wanted to highlight that it's not entirely equivalent to what was
> in the original patch and in my opinion it's less safe than the
> original patch.


Ok, folks think brevity wins out here, so I'm going to go ahead and
spin a different, stand-alone  patch following Chris' suggestion.
Please disregard this one.

Jim


> > Preserving the restricted bits is a functional change, and the subject
> > of this patch does not reflect that. When I look at the logs, I pretty
> > much expect clean up commits to be non-functional. There are some areas
> > where I'd look the other way, but PSR is something where we must
> > carefully split up the patches and write the commit messages diligently,
> > because I know we will be spending time debugging this code and reading
> > the logs.
> 
> I will remove the word 'clean-up' and reword the subject, independent
> of what we decide relative to the two approaches described above.
> The body of the commit message (IMHO) does a good job (and I'll add
> the specific bit in SRD_CTL to the body also) of describing the
> functional changes that the patch makes.
> 
> Jim
> 
> > BR,
> > Jani.
> > 
> > 
> > 
> > >
> > > Jim
> > >
> > >
> > >> 
> > >> BR,
> > >> Jani.
> > >> 
> > >> 
> > >> -- 
> > >> Jani Nikula, Intel Open Source Technology Center
> > >> _______________________________________________
> > >> Intel-gfx mailing list
> > >> Intel-gfx@lists.freedesktop.org
> > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx