[PATCH v5 1/9] drm/i915/psr: Remove PSR2 FIXME

[PATCH v5 1/9] drm/i915/psr: Remove PSR2 FIXME

[José Roberto de Souza]

Now we are checking sink capabilities when probing PSR DPCD register
and then dynamically checking in if new state is compatible with PSR
in, so this FIXME can be dropped.

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 75c1a5deebf5..8bed73914876 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -532,11 +532,6 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 	int crtc_vdisplay = crtc_state->base.adjusted_mode.crtc_vdisplay;
 	int psr_max_h = 0, psr_max_v = 0;
 
-	/*
-	 * FIXME psr2_support is messed up. It's both computed
-	 * dynamically during PSR enable, and extracted from sink
-	 * caps during eDP detection.
-	 */
 	if (!dev_priv->psr.sink_psr2_support)
 		return false;
 
-- 
2.21.0

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

[PATCH v5 2/9] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset

[José Roberto de Souza]

Forcing a specific CRTC to the eDP connector was causing the
intel_psr_fastset_force() to mark mode_chaged in the wrong and
disabled CRTC causing no update in the PSR state.

Looks like our internal state track do not clear output_types and
has_psr in the disabled CRTCs, not sure if this is the expected
behavior or not but in the mean time this fix the issue.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 8bed73914876..6175b1d2e0c8 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -981,7 +981,8 @@ static int intel_psr_fastset_force(struct drm_i915_private *dev_priv)
 
 		intel_crtc_state = to_intel_crtc_state(crtc_state);
 
-		if (intel_crtc_has_type(intel_crtc_state, INTEL_OUTPUT_EDP) &&
+		if (crtc_state->active &&
+		    intel_crtc_has_type(intel_crtc_state, INTEL_OUTPUT_EDP) &&
 		    intel_crtc_state->has_psr) {
 			/* Mark mode as changed to trigger a pipe->update() */
 			crtc_state->mode_changed = true;
-- 
2.21.0

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

[PATCH v5 3/9] drm/i915: Compute and commit color features in fastsets

[José Roberto de Souza]

In any commit, intel_modeset_pipe_config() will initialilly clear
and then recalculate most of the pipe states but it leave intel
specific color features states in reset state.

If after intel_pipe_config_compare() is detected that a fastset is
possible it will mark update_pipe as true and unsed mode_changed,
causing the color features state to be kept in reset state and then
latter being committed to hardware disabling the color features.

This issue can be reproduced by any code patch that duplicates the
actual(with color features already enabled) state and only mark
mode_changed as true.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d852cb282060..9312b3f35eb0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11235,7 +11235,8 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 			return ret;
 	}
 
-	if (mode_changed || crtc_state->color_mgmt_changed) {
+	if (mode_changed || pipe_config->update_pipe ||
+	    crtc_state->color_mgmt_changed) {
 		ret = intel_color_check(pipe_config);
 		if (ret)
 			return ret;
-- 
2.21.0

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

[PATCH v5 4/9] drm/i915/psr: Drop test for EDP in CRTC when forcing commit

[José Roberto de Souza]

If has_psr is set it means that CRTC has a EDP panel attached so it
can be dropped, also has_psr is better than check for EDP output
alone as it will avoid set mode_changed when PSR is not supported in
panel or with current modeset.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 6175b1d2e0c8..2d9f64c362e2 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -981,9 +981,7 @@ static int intel_psr_fastset_force(struct drm_i915_private *dev_priv)
 
 		intel_crtc_state = to_intel_crtc_state(crtc_state);
 
-		if (crtc_state->active &&
-		    intel_crtc_has_type(intel_crtc_state, INTEL_OUTPUT_EDP) &&
-		    intel_crtc_state->has_psr) {
+		if (crtc_state->active && intel_crtc_state->has_psr) {
 			/* Mark mode as changed to trigger a pipe->update() */
 			crtc_state->mode_changed = true;
 			break;
-- 
2.21.0

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

[PATCH v5 5/9] drm/i915/crc: Make IPS workaround generic

[José Roberto de Souza]

Other features like PSR2 also needs to be disabled while getting CRC
so lets rename ips_force_disable to crc_enabled, drop all this checks
for pipe A and HSW and BDW and make it generic and
hsw_compute_ips_config() will take care of all the checks removed
from here.

v2: Renaming and parameter changes to the functions that prepares the
commit (Ville)

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  | 10 ++++--
 drivers/gpu/drm/i915/intel_drv.h      |  3 +-
 drivers/gpu/drm/i915/intel_pipe_crc.c | 47 +++++++++++----------------
 3 files changed, 29 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9312b3f35eb0..b3a5d8462251 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6751,7 +6751,13 @@ static bool hsw_compute_ips_config(struct intel_crtc_state *crtc_state)
 	if (!hsw_crtc_state_ips_capable(crtc_state))
 		return false;
 
-	if (crtc_state->ips_force_disable)
+	/*
+	 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
+	 * enabled and disabled dynamically based on package C states,
+	 * user space can't make reliable use of the CRCs, so let's just
+	 * completely disable it.
+	 */
+	if (crtc_state->crc_enabled)
 		return false;
 
 	/* IPS should be fine as long as at least one plane is enabled. */
@@ -11687,7 +11693,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	saved_state->shared_dpll = crtc_state->shared_dpll;
 	saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
 	saved_state->pch_pfit.force_thru = crtc_state->pch_pfit.force_thru;
-	saved_state->ips_force_disable = crtc_state->ips_force_disable;
+	saved_state->crc_enabled = crtc_state->crc_enabled;
 	if (IS_G4X(dev_priv) ||
 	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		saved_state->wm = crtc_state->wm;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5412373e2f98..2be64529e4a2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -999,7 +999,8 @@ struct intel_crtc_state {
 	struct intel_link_m_n fdi_m_n;
 
 	bool ips_enabled;
-	bool ips_force_disable;
+
+	bool crc_enabled;
 
 	bool enable_fbc;
 
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 53d4ec68d3c4..af64597c5c6e 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -280,15 +280,15 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 	return 0;
 }
 
-static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
-			      bool enable)
+static void
+intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool enable)
 {
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	struct drm_device *dev = &dev_priv->drm;
-	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, PIPE_A);
 	struct intel_crtc_state *pipe_config;
 	struct drm_atomic_state *state;
 	struct drm_modeset_acquire_ctx ctx;
-	int ret = 0;
+	int ret;
 
 	drm_modeset_acquire_init(&ctx, 0);
 
@@ -307,17 +307,9 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
 		goto put_state;
 	}
 
-	if (HAS_IPS(dev_priv)) {
-		/*
-		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
-		 * enabled and disabled dynamically based on package C states,
-		 * user space can't make reliable use of the CRCs, so let's just
-		 * completely disable it.
-		 */
-		pipe_config->ips_force_disable = enable;
-	}
+	pipe_config->crc_enabled = enable;
 
-	if (IS_HASWELL(dev_priv)) {
+	if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) {
 		pipe_config->pch_pfit.force_thru = enable;
 		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
 		    pipe_config->pch_pfit.enabled != enable)
@@ -343,8 +335,7 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
 static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 				enum pipe pipe,
 				enum intel_pipe_crc_source *source,
-				u32 *val,
-				bool set_wa)
+				u32 *val)
 {
 	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
 		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
@@ -357,10 +348,6 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
 		break;
 	case INTEL_PIPE_CRC_SOURCE_PIPE:
-		if (set_wa && (IS_HASWELL(dev_priv) ||
-		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
-			hsw_pipe_A_crc_wa(dev_priv, true);
-
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
 		break;
 	case INTEL_PIPE_CRC_SOURCE_NONE:
@@ -418,8 +405,7 @@ static int skl_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
 
 static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
 			       enum pipe pipe,
-			       enum intel_pipe_crc_source *source, u32 *val,
-			       bool set_wa)
+			       enum intel_pipe_crc_source *source, u32 *val)
 {
 	if (IS_GEN(dev_priv, 2))
 		return i8xx_pipe_crc_ctl_reg(source, val);
@@ -430,7 +416,7 @@ static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
 	else if (IS_GEN_RANGE(dev_priv, 5, 6))
 		return ilk_pipe_crc_ctl_reg(source, val);
 	else if (INTEL_GEN(dev_priv) < 9)
-		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val, set_wa);
+		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
 	else
 		return skl_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
 }
@@ -605,6 +591,7 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
 	intel_wakeref_t wakeref;
 	u32 val = 0; /* shut up gcc */
 	int ret = 0;
+	bool enable;
 
 	if (display_crc_ctl_parse_source(source_name, &source) < 0) {
 		DRM_DEBUG_DRIVER("unknown source %s\n", source_name);
@@ -618,7 +605,11 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
 		return -EIO;
 	}
 
-	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val, true);
+	enable = source != INTEL_PIPE_CRC_SOURCE_NONE;
+	if (enable)
+		intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), enable);
+
+	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
 	if (ret != 0)
 		goto out;
 
@@ -629,14 +620,14 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
 	if (!source) {
 		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 			vlv_undo_pipe_scramble_reset(dev_priv, crtc->index);
-		else if ((IS_HASWELL(dev_priv) ||
-			  IS_BROADWELL(dev_priv)) && crtc->index == PIPE_A)
-			hsw_pipe_A_crc_wa(dev_priv, false);
 	}
 
 	pipe_crc->skipped = 0;
 
 out:
+	if (!enable)
+		intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), enable);
+
 	intel_display_power_put(dev_priv, power_domain, wakeref);
 
 	return ret;
@@ -652,7 +643,7 @@ void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc)
 	if (!crtc->crc.opened)
 		return;
 
-	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val, false) < 0)
+	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val) < 0)
 		return;
 
 	/* Don't need pipe_crc->lock here, IRQs are not generated. */
-- 
2.21.0

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

[PATCH v5 6/9] drm/i915: Disable PSR2 while getting pipe CRC

[José Roberto de Souza]

When PSR2 is active aka after the number of frames programmed in
PSR2_CTL 'Frames Before SU Entry' hardware stops to generate CRC
interruptions causing IGT tests to fail due timeout.

This same behavior don't happen with PSR1, as soon as pipe CRC is
enabled it blocks PSR1 activation so CRC calculation continues to
happens normaly.

This patch also set mode_changed as true when PSR is available to
force atomic check functions to compute new PSR state, otherwise PSR2
would not be disabled.

v4: Only setting mode_changed if has_psr is set(Dhinakaran)

v3: Reusing intel_crtc_crc_prepare() and crc_enabled, only setting
mode_changed if it can do PSR.

v2: Changed commit description to describe that PSR2 inhibit CRC
calculations.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_pipe_crc.c | 1 +
 drivers/gpu/drm/i915/intel_psr.c      | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index af64597c5c6e..c17f02b88453 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -307,6 +307,7 @@ intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool enable)
 		goto put_state;
 	}
 
+	pipe_config->base.mode_changed = pipe_config->has_psr;
 	pipe_config->crc_enabled = enable;
 
 	if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) {
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2d9f64c362e2..73453d89a841 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -572,6 +572,9 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 		return false;
 	}
 
+	if (crtc_state->crc_enabled)
+		return false;
+
 	return true;
 }
 
-- 
2.21.0

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

[PATCH v5 7/9] drm/i915: Drop redundant checks to update PSR state

[José Roberto de Souza]

All of this checks are redudant and can be removed as the if bellow
already takes care when there is no changes in the state.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 73453d89a841..d3e3996551c6 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -878,15 +878,11 @@ void intel_psr_update(struct intel_dp *intel_dp,
 	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled)
 		goto unlock;
 
-	if (psr->enabled) {
-		if (!enable || psr2_enable != psr->psr2_enabled)
-			intel_psr_disable_locked(intel_dp);
-	}
+	if (psr->enabled)
+		intel_psr_disable_locked(intel_dp);
 
-	if (enable) {
-		if (!psr->enabled || psr2_enable != psr->psr2_enabled)
-			intel_psr_enable_locked(dev_priv, crtc_state);
-	}
+	if (enable)
+		intel_psr_enable_locked(dev_priv, crtc_state);
 
 unlock:
 	mutex_unlock(&dev_priv->psr.lock);
-- 
2.21.0

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

[PATCH v5 8/9] drm/i915: Force PSR exit when getting pipe CRC

[José Roberto de Souza]

If PSR is active when pipe CRC is enabled the CRC calculations will
be inhibit by the transition to low power states that PSR brings.
So lets for a PSR exit and as soon as pipe CRC is enabled it will
block PSR activation avoid CRC timeouts when running IGT tests.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 36 ++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index d3e3996551c6..5d66e7313c75 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -452,6 +452,7 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
 	 * frames, we'll go with 9 frames for now
 	 */
 	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1);
+
 	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
 
 	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
@@ -851,6 +852,20 @@ void intel_psr_disable(struct intel_dp *intel_dp,
 	cancel_work_sync(&dev_priv->psr.work);
 }
 
+static void psr_force_hw_tracking_exit(struct drm_i915_private *dev_priv)
+{
+	/*
+	 * Display WA #0884: all
+	 * This documented WA for bxt can be safely applied
+	 * broadly so we can force HW tracking to exit PSR
+	 * instead of disabling and re-enabling.
+	 * Workaround tells us to write 0 to CUR_SURFLIVE_A,
+	 * but it makes more sense write to the current active
+	 * pipe.
+	 */
+	I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0);
+}
+
 /**
  * intel_psr_update - Update PSR state
  * @intel_dp: Intel DP
@@ -875,8 +890,13 @@ void intel_psr_update(struct intel_dp *intel_dp,
 	enable = crtc_state->has_psr && psr_global_enabled(psr->debug);
 	psr2_enable = intel_psr2_enabled(dev_priv, crtc_state);
 
-	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled)
+	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled) {
+		/* Force a PSR exit when enabling CRC to avoid CRC timeouts */
+		if (crtc_state->crc_enabled && psr->enabled)
+			psr_force_hw_tracking_exit(dev_priv);
+
 		goto unlock;
+	}
 
 	if (psr->enabled)
 		intel_psr_disable_locked(intel_dp);
@@ -1146,18 +1166,8 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
 	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
 
 	/* By definition flush = invalidate + flush */
-	if (frontbuffer_bits) {
-		/*
-		 * Display WA #0884: all
-		 * This documented WA for bxt can be safely applied
-		 * broadly so we can force HW tracking to exit PSR
-		 * instead of disabling and re-enabling.
-		 * Workaround tells us to write 0 to CUR_SURFLIVE_A,
-		 * but it makes more sense write to the current active
-		 * pipe.
-		 */
-		I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0);
-	}
+	if (frontbuffer_bits)
+		psr_force_hw_tracking_exit(dev_priv);
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
 		schedule_work(&dev_priv->psr.work);
-- 
2.21.0

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

[PATCH v5 9/9] drm/i915: Enable PSR2 by default

[José Roberto de Souza]

The support for PSR2 was polished, IGT tests for PSR2 was added and
it was tested performing regular user workloads like browsing,
editing documents and compiling Linux, so it is time to enable it by
default and enjoy even more power-savings.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 5d66e7313c75..bae266869c20 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -80,9 +80,6 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
 	case I915_PSR_DEBUG_DISABLE:
 	case I915_PSR_DEBUG_FORCE_PSR1:
 		return false;
-	case I915_PSR_DEBUG_DEFAULT:
-		if (i915_modparams.enable_psr <= 0)
-			return false;
 	default:
 		return crtc_state->has_psr2;
 	}
-- 
2.21.0

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

✗ Fi.CI.SPARSE: warning for series starting with [v5,1/9] drm/i915/psr: Remove PSR2 FIXME

[Patchwork]

== Series Details ==

Series: series starting with [v5,1/9] drm/i915/psr: Remove PSR2 FIXME
URL   : https://patchwork.freedesktop.org/series/57628/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/psr: Remove PSR2 FIXME
Okay!

Commit: drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset
Okay!

Commit: drm/i915: Compute and commit color features in fastsets
Okay!

Commit: drm/i915/psr: Drop test for EDP in CRTC when forcing commit
Okay!

Commit: drm/i915/crc: Make IPS workaround generic
Okay!

Commit: drm/i915: Disable PSR2 while getting pipe CRC
Okay!

Commit: drm/i915: Drop redundant checks to update PSR state
Okay!

Commit: drm/i915: Force PSR exit when getting pipe CRC
-O:drivers/gpu/drm/i915/intel_psr.c:454:23: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_psr.c:454:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_psr.c:454:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_psr.c:454:23: warning: expression using sizeof(void)

Commit: drm/i915: Enable PSR2 by default
Okay!

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

✓ Fi.CI.BAT: success for series starting with [v5,1/9] drm/i915/psr: Remove PSR2 FIXME

[Patchwork]

== Series Details ==

Series: series starting with [v5,1/9] drm/i915/psr: Remove PSR2 FIXME
URL   : https://patchwork.freedesktop.org/series/57628/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5708 -> Patchwork_12388
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/57628/revisions/1/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12388 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103167]

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362]

  * igt@kms_psr@cursor_plane_move:
    - fi-whl-u:           PASS -> FAIL [fdo#107383] +3

  * igt@kms_psr@primary_mmap_gtt:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +27

  * igt@prime_vgem@basic-fence-flip:
    - fi-gdg-551:         PASS -> FAIL [fdo#103182]

  
#### Possible fixes ####

  * igt@gem_mmap@basic-small-bo:
    - {fi-icl-y}:         DMESG-WARN -> PASS

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS +1

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107383]: https://bugs.freedesktop.org/show_bug.cgi?id=107383
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271


Participating hosts (47 -> 41)
------------------------------

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-pnv-d510 fi-bdw-samus 


Build changes
-------------

    * Linux: CI_DRM_5708 -> Patchwork_12388

  CI_DRM_5708: afd34c5dec857362de91fb3044f09d90e83ad6a5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4871: 8feb147562ba1b364615ddacd44c3846f0250d37 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12388: b47c5668b993c7f46fedd5d2f4185633e4bf48cf @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

b47c5668b993 drm/i915: Enable PSR2 by default
0351b3626e46 drm/i915: Force PSR exit when getting pipe CRC
1098eda1010e drm/i915: Drop redundant checks to update PSR state
107f2d29e13c drm/i915: Disable PSR2 while getting pipe CRC
ca535036359a drm/i915/crc: Make IPS workaround generic
b44ee7a5d070 drm/i915/psr: Drop test for EDP in CRTC when forcing commit
d04169730a46 drm/i915: Compute and commit color features in fastsets
8cc93ba346d5 drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset
3fad93a4257c drm/i915/psr: Remove PSR2 FIXME

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12388/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [v5,1/9] drm/i915/psr: Remove PSR2 FIXME

[Patchwork]

== Series Details ==

Series: series starting with [v5,1/9] drm/i915/psr: Remove PSR2 FIXME
URL   : https://patchwork.freedesktop.org/series/57628/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5708_full -> Patchwork_12388_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12388_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12388_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12388_full:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_eio@wait-10ms:
    - shard-iclb:         NOTRUN -> FAIL +2

  * igt@gem_exec_fence@basic-wait-default:
    - shard-iclb:         NOTRUN -> WARN

  * igt@gem_exec_schedule@preempt-queue-bsd:
    - shard-iclb:         NOTRUN -> INCOMPLETE

  * igt@i915_selftest@live_gem:
    - shard-iclb:         NOTRUN -> DMESG-FAIL +1

  * igt@runner@aborted:
    - shard-iclb:         NOTRUN -> ( 6 FAIL ) [fdo#109467]

  
Known issues
------------

  Here are the changes found in Patchwork_12388_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109281] +19

  * igt@gem_ctx_isolation@vcs1-s3:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109276] / [fdo#109281] +1

  * igt@gem_ctx_param@invalid-param-set:
    - shard-iclb:         NOTRUN -> FAIL [fdo#109674]

  * igt@gem_ctx_param@set-priority-not-supported:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109314]

  * igt@gem_exec_flush@basic-batch-kernel-default-cmd:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109313]

  * igt@gem_exec_params@no-vebox:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109283] +2

  * igt@gem_exec_parse@basic-rejected:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109289] +19

  * igt@gem_exec_schedule@preempt-other-bsd1:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109276] +110

  * igt@gem_mmap_gtt@coherency:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109292] +1

  * igt@gem_mocs_settings@mocs-reset-ctx-render:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109287] +12

  * igt@gem_mocs_settings@mocs-settings-bsd1:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109276] / [fdo#109287] +4

  * igt@gem_pwrite@huge-gtt-backwards:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109290] +9

  * igt@gem_softpin@evict-snoop:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109312] +1

  * igt@gem_stolen@stolen-clear:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109277] +12

  * igt@gem_workarounds@suspend-resume-context:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108] / [fdo#107773] +1

  * igt@i915_missed_irq:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109503]

  * igt@i915_pm_backlight@fade_with_suspend:
    - shard-iclb:         NOTRUN -> FAIL [fdo#107847]

  * igt@i915_pm_lpsp@non-edp:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109301] +2

  * igt@i915_pm_rpm@dpms-mode-unset-non-lpsp:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109308] +4

  * igt@i915_pm_rpm@gem-execbuf-stress-pc8:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109506]

  * igt@i915_pm_rpm@pc8-residency:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109293]

  * igt@i915_pm_rps@min-max-config-loaded:
    - shard-iclb:         NOTRUN -> FAIL [fdo#102250]

  * igt@i915_pm_rps@reset:
    - shard-iclb:         NOTRUN -> FAIL [fdo#102250] / [fdo#108059]

  * igt@i915_pm_sseu@full-enable:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109288]

  * igt@i915_query@query-topology-known-pci-ids:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109303]

  * igt@i915_query@query-topology-unsupported:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109302]

  * igt@i915_selftest@live_workarounds:
    - shard-iclb:         NOTRUN -> DMESG-FAIL [fdo#108954]

  * igt@kms_available_modes_crc@available_mode_test_crc:
    - shard-iclb:         NOTRUN -> FAIL [fdo#106641]

  * igt@kms_busy@extended-modeset-hang-newfb-render-d:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109278] +41

  * igt@kms_busy@extended-modeset-hang-oldfb-with-reset-render-d:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_busy@extended-pageflip-hang-newfb-render-f:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +4

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
    - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#107956] +3

  * igt@kms_ccs@pipe-a-crc-primary-rotation-180:
    - shard-iclb:         NOTRUN -> FAIL [fdo#107725] +5

  * igt@kms_chamelium@dp-frame-dump:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109284] +38

  * igt@kms_color@pipe-a-legacy-gamma:
    - shard-glk:          PASS -> FAIL [fdo#104782] / [fdo#108145]

  * igt@kms_color@pipe-b-gamma:
    - shard-iclb:         NOTRUN -> FAIL [fdo#104782] +7

  * igt@kms_color@pipe-c-ctm-max:
    - shard-iclb:         NOTRUN -> FAIL [fdo#108147] +1

  * igt@kms_content_protection@atomic:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109300] / [fdo#109527]

  * igt@kms_content_protection@atomic-dpms:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109527]

  * igt@kms_cursor_crc@cursor-128x128-random:
    - shard-apl:          PASS -> FAIL [fdo#103232] +1

  * igt@kms_cursor_crc@cursor-128x128-suspend:
    - shard-skl:          NOTRUN -> INCOMPLETE [fdo#104108]

  * igt@kms_cursor_crc@cursor-256x256-random:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103232] +19

  * igt@kms_cursor_crc@cursor-256x85-onscreen:
    - shard-skl:          NOTRUN -> FAIL [fdo#103232]

  * igt@kms_cursor_crc@cursor-512x512-rapid-movement:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109279] +10

  * igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
    - shard-skl:          PASS -> FAIL [fdo#106081]

  * igt@kms_cursor_legacy@flip-vs-cursor-busy-crc-atomic:
    - shard-skl:          PASS -> FAIL [fdo#102670]

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109349] +1

  * igt@kms_fbcon_fbt@psr-suspend:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103833] +1

  * igt@kms_flip@2x-flip-vs-rmfb-interruptible:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109274] +61

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          PASS -> FAIL [fdo#105363]

  * igt@kms_flip_tiling@flip-changes-tiling-y:
    - shard-skl:          PASS -> FAIL [fdo#107931] / [fdo#108303]

  * igt@kms_force_connector_basic@force-edid:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109285] +3

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff:
    - shard-apl:          PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-render:
    - shard-glk:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-indfb-draw-blt:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] +42

  * igt@kms_frontbuffer_tracking@fbc-stridechange:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103167] +12

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw:
    - shard-skl:          PASS -> FAIL [fdo#105682]

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] +15

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-shrfb-fliptrack:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109280] +176

  * igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-blt:
    - shard-skl:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@psr-2p-primscrn-cur-indfb-move:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271]

  * igt@kms_hdmi_inject@inject-audio:
    - shard-iclb:         NOTRUN -> FAIL [fdo#102370]

  * igt@kms_invalid_dotclock:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109310]

  * igt@kms_panel_fitting@legacy:
    - shard-skl:          NOTRUN -> FAIL [fdo#105456]

  * igt@kms_plane@pixel-format-pipe-a-planes:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103166] +2

  * igt@kms_plane@pixel-format-pipe-b-planes-source-clamping:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#106885]
    - shard-iclb:         NOTRUN -> FAIL [fdo#108948] +2

  * igt@kms_plane_alpha_blend@pipe-b-alpha-7efc:
    - shard-skl:          NOTRUN -> FAIL [fdo#107815] / [fdo#108145]

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
    - shard-apl:          PASS -> FAIL [fdo#103166] +1

  * igt@kms_plane_scaling@pipe-b-scaler-with-pixel-format:
    - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#107724] +6

  * igt@kms_psr2_su@frontbuffer:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109642]

  * igt@kms_psr@psr2_cursor_plane_onoff:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109441] +15

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-kbl:          PASS -> FAIL [fdo#109016]

  * igt@kms_setmode@basic:
    - shard-iclb:         NOTRUN -> FAIL [fdo#99912]
    - shard-kbl:          PASS -> FAIL [fdo#99912]

  * igt@kms_tv_load_detect@load-detect:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109309]

  * igt@kms_vblank@pipe-a-ts-continuation-modeset-rpm:
    - shard-apl:          NOTRUN -> FAIL [fdo#104894]

  * igt@kms_vrr@flip-basic:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109502] +2

  * igt@prime_nv_pcopy@test2:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109291] +23

  * igt@prime_vgem@basic-fence-flip:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109294]

  * igt@prime_vgem@fence-write-hang:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109295] +1

  * igt@tools_test@sysfs_l3_parity:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109307]

  * igt@v3d_get_param@get-bad-flags:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109315] +3

  
#### Possible fixes ####

  * igt@i915_pm_rpm@gem-execbuf-stress-extra-wait:
    - shard-skl:          INCOMPLETE [fdo#107803] / [fdo#107807] -> PASS

  * igt@kms_cursor_crc@cursor-256x256-onscreen:
    - shard-apl:          FAIL [fdo#103232] -> PASS

  * igt@kms_cursor_crc@cursor-alpha-opaque:
    - shard-glk:          FAIL [fdo#109350] -> PASS

  * igt@kms_cursor_crc@cursor-size-change:
    - shard-glk:          FAIL [fdo#103232] -> PASS

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-skl:          FAIL [fdo#105363] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-apl:          FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
    - shard-glk:          FAIL [fdo#103167] -> PASS

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max:
    - shard-glk:          FAIL [fdo#108145] -> PASS +1

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          FAIL [fdo#107815] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-none:
    - shard-apl:          FAIL [fdo#103166] -> PASS

  * igt@kms_rotation_crc@multiplane-rotation:
    - shard-kbl:          INCOMPLETE [fdo#103665] -> PASS

  
#### Warnings ####

  * igt@i915_pm_rpm@modeset-non-lpsp:
    - shard-skl:          SKIP [fdo#109271] -> INCOMPLETE [fdo#107807]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102250]: https://bugs.freedesktop.org/show_bug.cgi?id=102250
  [fdo#102370]: https://bugs.freedesktop.org/show_bug.cgi?id=102370
  [fdo#102670]: https://bugs.freedesktop.org/show_bug.cgi?id=102670
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103833]: https://bugs.freedesktop.org/show_bug.cgi?id=103833
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105456]: https://bugs.freedesktop.org/show_bug.cgi?id=105456
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#106081]: https://bugs.freedesktop.org/show_bug.cgi?id=106081
  [fdo#106641]: https://bugs.freedesktop.org/show_bug.cgi?id=106641
  [fdo#106885]: https://bugs.freedesktop.org/show_bug.cgi?id=106885
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#107725]: https://bugs.freedesktop.org/show_bug.cgi?id=107725
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#107803]: https://bugs.freedesktop.org/show_bug.cgi?id=107803
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
  [fdo#107847]: https://bugs.freedesktop.org/show_bug.cgi?id=107847
  [fdo#107931]: https://bugs.freedesktop.org/show_bug.cgi?id=107931
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108059]: https://bugs.freedesktop.org/show_bug.cgi?id=108059
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
  [fdo#108303]: https://bugs.freedesktop.org/show_bug.cgi?id=108303
  [fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
  [fdo#108954]: https://bugs.freedesktop.org/show_bug.cgi?id=108954
  [fdo#109016]: https://bugs.freedesktop.org/show_bug.cgi?id=109016
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109277]: https://bugs.freedesktop.org/show_bug.cgi?id=109277
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109279]: https://bugs.freedesktop.org/show_bug.cgi?id=109279
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109281]: https://bugs.freedesktop.org/show_bug.cgi?id=109281
  [fdo#109283]: https://bugs.freedesktop.org/show_bug.cgi?id=109283
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109287]: https://bugs.freedesktop.org/show_bug.cgi?id=109287
  [fdo#109288]: https://bugs.freedesktop.org/show_bug.cgi?id=109288
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109290]: https://bugs.freedesktop.org/show_bug.cgi?id=109290
  [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
  [fdo#109292]: https://bugs.freedesktop.org/show_bug.cgi?id=109292
  [fdo#109293]: https://bugs.freedesktop.org/show_bug.cgi?id=109293
  [fdo#109294]: https://bugs.freedesktop.org/show_bug.cgi?id=109294
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#109300]: https://bugs.freedesktop.org/show_bug.cgi?id=109300
  [fdo#109301]: https://bugs.freedesktop.org/show_bug.cgi?id=109301
  [fdo#109302]: https://bugs.freedesktop.org/show_bug.cgi?id=109302
  [fdo#109303]: https://bugs.freedesktop.org/show_bug.cgi?id=109303
  [fdo#109307]: https://bugs.freedesktop.org/show_bug.cgi?id=109307
  [fdo#109308]: https://bugs.freedesktop.org/show_bug.cgi?id=109308
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#109310]: https://bugs.freedesktop.org/show_bug.cgi?id=109310
  [fdo#109312]: https://bugs.freedesktop.org/show_bug.cgi?id=109312
  [fdo#109313]: https://bugs.freedesktop.org/show_bug.cgi?id=109313
  [fdo#109314]: https://bugs.freedesktop.org/show_bug.cgi?id=109314
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109350]: https://bugs.freedesktop.org/show_bug.cgi?id=109350
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109467]: https://bugs.freedesktop.org/show_bug.cgi?id=109467
  [fdo#109502]: https://bugs.freedesktop.org/show_bug.cgi?id=109502
  [fdo#109503]: https://bugs.freedesktop.org/show_bug.cgi?id=109503
  [fdo#109506]: https://bugs.freedesktop.org/show_bug.cgi?id=109506
  [fdo#109527]: https://bugs.freedesktop.org/show_bug.cgi?id=109527
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#109674]: https://bugs.freedesktop.org/show_bug.cgi?id=109674
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (6 -> 6)
------------------------------

  Additional (1): shard-iclb 
  Missing    (1): shard-snb 


Build changes
-------------

    * Linux: CI_DRM_5708 -> Patchwork_12388

  CI_DRM_5708: afd34c5dec857362de91fb3044f09d90e83ad6a5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4871: 8feb147562ba1b364615ddacd44c3846f0250d37 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12388: b47c5668b993c7f46fedd5d2f4185633e4bf48cf @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12388/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 5/9] drm/i915/crc: Make IPS workaround generic

[Dhinakaran Pandiyan]

On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> Other features like PSR2 also needs to be disabled while getting CRC
> so lets rename ips_force_disable to crc_enabled, drop all this checks
> for pipe A and HSW and BDW and make it generic and
> hsw_compute_ips_config() will take care of all the checks removed
> from here.
> 
> v2: Renaming and parameter changes to the functions that prepares the
> commit (Ville)
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  | 10 ++++--
>  drivers/gpu/drm/i915/intel_drv.h      |  3 +-
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 47 +++++++++++------------
> ----
>  3 files changed, 29 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 9312b3f35eb0..b3a5d8462251 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6751,7 +6751,13 @@ static bool hsw_compute_ips_config(struct
> intel_crtc_state *crtc_state)
>  	if (!hsw_crtc_state_ips_capable(crtc_state))
>  		return false;
>  
> -	if (crtc_state->ips_force_disable)
> +	/*
> +	 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> +	 * enabled and disabled dynamically based on package C states,
> +	 * user space can't make reliable use of the CRCs, so let's
> just
> +	 * completely disable it.
> +	 */
> +	if (crtc_state->crc_enabled)
>  		return false;
>  
>  	/* IPS should be fine as long as at least one plane is enabled.
> */
> @@ -11687,7 +11693,7 @@ clear_intel_crtc_state(struct
> intel_crtc_state *crtc_state)
>  	saved_state->shared_dpll = crtc_state->shared_dpll;
>  	saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
>  	saved_state->pch_pfit.force_thru = crtc_state-
> >pch_pfit.force_thru;
> -	saved_state->ips_force_disable = crtc_state->ips_force_disable;
> +	saved_state->crc_enabled = crtc_state->crc_enabled;
>  	if (IS_G4X(dev_priv) ||
>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		saved_state->wm = crtc_state->wm;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 5412373e2f98..2be64529e4a2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -999,7 +999,8 @@ struct intel_crtc_state {
>  	struct intel_link_m_n fdi_m_n;
>  
>  	bool ips_enabled;
> -	bool ips_force_disable;
> +
> +	bool crc_enabled;
>  
>  	bool enable_fbc;
>  
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 53d4ec68d3c4..af64597c5c6e 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -280,15 +280,15 @@ static int ilk_pipe_crc_ctl_reg(enum
> intel_pipe_crc_source *source,
>  	return 0;
>  }
>  
> -static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> -			      bool enable)
> +static void
> +intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool
> enable)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	struct drm_device *dev = &dev_priv->drm;
We could eliminate this local.

> -	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> PIPE_A);
>  	struct intel_crtc_state *pipe_config;
>  	struct drm_atomic_state *state;
>  	struct drm_modeset_acquire_ctx ctx;
> -	int ret = 0;
> +	int ret;
>  
>  	drm_modeset_acquire_init(&ctx, 0);
>  
> @@ -307,17 +307,9 @@ static void hsw_pipe_A_crc_wa(struct
> drm_i915_private *dev_priv,
>  		goto put_state;
>  	}
>  
> -	if (HAS_IPS(dev_priv)) {
> -		/*
> -		 * When IPS gets enabled, the pipe CRC changes. Since
> IPS gets
> -		 * enabled and disabled dynamically based on package C
> states,
> -		 * user space can't make reliable use of the CRCs, so
> let's just
> -		 * completely disable it.
> -		 */
> -		pipe_config->ips_force_disable = enable;
> -	}
> +	pipe_config->crc_enabled = enable;
>  
> -	if (IS_HASWELL(dev_priv)) {
> +	if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) {
>  		pipe_config->pch_pfit.force_thru = enable;
>  		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
>  		    pipe_config->pch_pfit.enabled != enable)
> @@ -343,8 +335,7 @@ static void hsw_pipe_A_crc_wa(struct
> drm_i915_private *dev_priv,
>  static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  				enum pipe pipe,
>  				enum intel_pipe_crc_source *source,
> -				u32 *val,
> -				bool set_wa)
> +				u32 *val)
>  {
>  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
>  		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
> @@ -357,10 +348,6 @@ static int ivb_pipe_crc_ctl_reg(struct
> drm_i915_private *dev_priv,
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
>  		break;
>  	case INTEL_PIPE_CRC_SOURCE_PIPE:
> -		if (set_wa && (IS_HASWELL(dev_priv) ||
> -		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> -			hsw_pipe_A_crc_wa(dev_priv, true);
> -
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
>  		break;
>  	case INTEL_PIPE_CRC_SOURCE_NONE:
> @@ -418,8 +405,7 @@ static int skl_pipe_crc_ctl_reg(struct
> drm_i915_private *dev_priv,
>  
>  static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
>  			       enum pipe pipe,
> -			       enum intel_pipe_crc_source *source, u32
> *val,
> -			       bool set_wa)
> +			       enum intel_pipe_crc_source *source, u32
> *val)
>  {
>  	if (IS_GEN(dev_priv, 2))
>  		return i8xx_pipe_crc_ctl_reg(source, val);
> @@ -430,7 +416,7 @@ static int get_new_crc_ctl_reg(struct
> drm_i915_private *dev_priv,
>  	else if (IS_GEN_RANGE(dev_priv, 5, 6))
>  		return ilk_pipe_crc_ctl_reg(source, val);
>  	else if (INTEL_GEN(dev_priv) < 9)
> -		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source,
> val, set_wa);
> +		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source,
> val);
>  	else
>  		return skl_pipe_crc_ctl_reg(dev_priv, pipe, source,
> val);
>  }
> @@ -605,6 +591,7 @@ int intel_crtc_set_crc_source(struct drm_crtc
> *crtc, const char *source_name)
>  	intel_wakeref_t wakeref;
>  	u32 val = 0; /* shut up gcc */
>  	int ret = 0;
> +	bool enable;
>  
>  	if (display_crc_ctl_parse_source(source_name, &source) < 0) {
>  		DRM_DEBUG_DRIVER("unknown source %s\n", source_name);
> @@ -618,7 +605,11 @@ int intel_crtc_set_crc_source(struct drm_crtc
> *crtc, const char *source_name)
>  		return -EIO;
>  	}
>  
> -	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val,
> true);
> +	enable = source != INTEL_PIPE_CRC_SOURCE_NONE;
> +	if (enable)
> +		intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc),
> enable);
I personally, find it easier to read when we pass true/false, it's just
clear that we have already worked out what do when enable is true.
	intel_crtc_crc_setup_workaround(to_intel_crtc(crtc), true);


> +
> +	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source,
> &val);
>  	if (ret != 0)
>  		goto out;
>  
> @@ -629,14 +620,14 @@ int intel_crtc_set_crc_source(struct drm_crtc
> *crtc, const char *source_name)
>  	if (!source) {
>  		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  			vlv_undo_pipe_scramble_reset(dev_priv, crtc-
> >index);
> -		else if ((IS_HASWELL(dev_priv) ||
> -			  IS_BROADWELL(dev_priv)) && crtc->index ==
> PIPE_A)
> -			hsw_pipe_A_crc_wa(dev_priv, false);
>  	}
>  
>  	pipe_crc->skipped = 0;
>  
>  out:
> +	if (!enable)
> +		intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc),
> enable);
> +
>  	intel_display_power_put(dev_priv, power_domain, wakeref);
>  
>  	return ret;
> @@ -652,7 +643,7 @@ void intel_crtc_enable_pipe_crc(struct intel_crtc
> *intel_crtc)
>  	if (!crtc->crc.opened)
>  		return;
>  
> -	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc-
> >source, &val, false) < 0)
> +	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc-
> >source, &val) < 0)

Now that 'ips_force_disable' is 'crc_enabled', I guess we should set it
here for the sake of completeness. Anyway, doing that doesn't have any
functional benefit since we weren't doing it earlier.

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>  		return;
>  
>  	/* Don't need pipe_crc->lock here, IRQs are not generated. */

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

Re: [PATCH v5 4/9] drm/i915/psr: Drop test for EDP in CRTC when forcing commit

[Dhinakaran Pandiyan]

On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> If has_psr is set it means that CRTC has a EDP panel attached so it
> can be dropped, also has_psr is better than check for EDP output
> alone as it will avoid set mode_changed when PSR is not supported in
> panel or with current modeset.
Since we were already checking has_psr, I don't think it is relevant to
say checking just that is "better than check for EDP output alone". Can
you please reword the commit message?

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 6175b1d2e0c8..2d9f64c362e2 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -981,9 +981,7 @@ static int intel_psr_fastset_force(struct
> drm_i915_private *dev_priv)
>  
>  		intel_crtc_state = to_intel_crtc_state(crtc_state);
>  
> -		if (crtc_state->active &&
> -		    intel_crtc_has_type(intel_crtc_state,
> INTEL_OUTPUT_EDP) &&
> -		    intel_crtc_state->has_psr) {
> +		if (crtc_state->active && intel_crtc_state->has_psr) {
>  			/* Mark mode as changed to trigger a pipe-
> >update() */
>  			crtc_state->mode_changed = true;
>  			break;

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

Re: [PATCH v5 6/9] drm/i915: Disable PSR2 while getting pipe CRC

[Dhinakaran Pandiyan]

On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> When PSR2 is active aka after the number of frames programmed in
> PSR2_CTL 'Frames Before SU Entry' hardware stops to generate CRC
> interruptions causing IGT tests to fail due timeout.
s/interruptions/interrupts

I suppose there are no bugzilla references, since we do not check CRCs
in kms_psr and PSR2 is not enabled in kms_frontbuffer_tracking 
> 
> This same behavior don't happen with PSR1, as soon as pipe CRC is
> enabled it blocks PSR1 activation
The hardware also deactivates PSR1, not just block future PSR1
activation. Right?

>  so CRC calculation continues to
> happens normaly.
> 
> This patch also set mode_changed as true when PSR is available to
> force atomic check functions to compute new PSR state, otherwise PSR2
> would not be disabled.
> 
> v4: Only setting mode_changed if has_psr is set(Dhinakaran)
> 
> v3: Reusing intel_crtc_crc_prepare() and crc_enabled, only setting
> mode_changed if it can do PSR.
> 
> v2: Changed commit description to describe that PSR2 inhibit CRC
> calculations.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 1 +
>  drivers/gpu/drm/i915/intel_psr.c      | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index af64597c5c6e..c17f02b88453 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -307,6 +307,7 @@ intel_crtc_crc_setup_workarounds(struct
> intel_crtc *crtc, bool enable)
>  		goto put_state;
>  	}
>  
> +	pipe_config->base.mode_changed = pipe_config->has_psr;
>  	pipe_config->crc_enabled = enable;
>  
>  	if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) {
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 2d9f64c362e2..73453d89a841 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -572,6 +572,9 @@ static bool intel_psr2_config_valid(struct
> intel_dp *intel_dp,
>  		return false;
>  	}
>  
> +	if (crtc_state->crc_enabled)
> +		return false;
No DRM_DEBUG_KMS()? 

with that added
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> +
>  	return true;
>  }
>  

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

Re: [PATCH v5 7/9] drm/i915: Drop redundant checks to update PSR state

[Dhinakaran Pandiyan]

On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> All of this checks are redudant and can be removed as the if bellow
below*

> already takes care when there is no changes in the state.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 73453d89a841..d3e3996551c6 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -878,15 +878,11 @@ void intel_psr_update(struct intel_dp
> *intel_dp,
>  	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled)
>  		goto unlock;
>  
> -	if (psr->enabled) {
> -		if (!enable || psr2_enable != psr->psr2_enabled)
> -			intel_psr_disable_locked(intel_dp);
> -	}
> +	if (psr->enabled)
> +		intel_psr_disable_locked(intel_dp);
>  
> -	if (enable) {
> -		if (!psr->enabled || psr2_enable != psr->psr2_enabled)
> -			intel_psr_enable_locked(dev_priv, crtc_state);
> -	}
> +	if (enable)
> +		intel_psr_enable_locked(dev_priv, crtc_state);
>  
Looks like all cases are handled
PSR1 -> PSR2, disable, PSR1
PSR2 -> PSR1, disable, PSR2
disable -> PSR1, disable, PSR2


Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>  unlock:
>  	mutex_unlock(&dev_priv->psr.lock);

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

Re: [PATCH v5 8/9] drm/i915: Force PSR exit when getting pipe CRC

[Dhinakaran Pandiyan]

On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> If PSR is active when pipe CRC is enabled the CRC calculations will
> be inhibit by the transition to low power states that PSR brings.
The MMIO write to enable CRCs should bring the hardware out from PSR,
but I can imagine some initial CRCs  are going to be corrupt or
unavailable.

> So lets for a PSR exit and as soon as pipe CRC is enabled it will
There is a missing word.

> block PSR activation avoid CRC timeouts when running IGT tests.

This surely has fdo bugs, please include them in the commit message.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 36 ++++++++++++++++++++--------
> ----
>  1 file changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index d3e3996551c6..5d66e7313c75 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -452,6 +452,7 @@ static void hsw_activate_psr1(struct intel_dp
> *intel_dp)
>  	 * frames, we'll go with 9 frames for now
>  	 */
>  	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency
> + 1);
> +
>  	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
>  
>  	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
> @@ -851,6 +852,20 @@ void intel_psr_disable(struct intel_dp
> *intel_dp,
>  	cancel_work_sync(&dev_priv->psr.work);
>  }
>  
> +static void psr_force_hw_tracking_exit(struct drm_i915_private
> *dev_priv)
> +{
> +	/*
> +	 * Display WA #0884: all
> +	 * This documented WA for bxt can be safely applied
> +	 * broadly so we can force HW tracking to exit PSR
> +	 * instead of disabling and re-enabling.
> +	 * Workaround tells us to write 0 to CUR_SURFLIVE_A,
> +	 * but it makes more sense write to the current active
> +	 * pipe.
> +	 */
> +	I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0);
> +}
> +
>  /**
>   * intel_psr_update - Update PSR state
>   * @intel_dp: Intel DP
> @@ -875,8 +890,13 @@ void intel_psr_update(struct intel_dp *intel_dp,
>  	enable = crtc_state->has_psr && psr_global_enabled(psr->debug);
>  	psr2_enable = intel_psr2_enabled(dev_priv, crtc_state);
>  
> -	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled)
> +	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled) 
> {

PSR2 is enabled, then user requests CRCs, the mode_changed commit
switches to PSR1. The above condition isn't true in that case.

Also, since the CRC workaround is done before enabling pipe CRC, isn't
there a possibility that the idle frame counter times out and activates
PSR1 before CRC is enabled?

> +		/* Force a PSR exit when enabling CRC to avoid CRC
> timeouts */
> +		if (crtc_state->crc_enabled && psr->enabled)
> +			psr_force_hw_tracking_exit(dev_priv);
The patch fixes a PSR1 issue, why isn't there any reference to PSR1
anywhere?


> +
>  		goto unlock;
> +	}
>  
>  	if (psr->enabled)
>  		intel_psr_disable_locked(intel_dp);
> @@ -1146,18 +1166,8 @@ void intel_psr_flush(struct drm_i915_private
> *dev_priv,
>  	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
>  
>  	/* By definition flush = invalidate + flush */
> -	if (frontbuffer_bits) {
> -		/*
> -		 * Display WA #0884: all
> -		 * This documented WA for bxt can be safely applied
> -		 * broadly so we can force HW tracking to exit PSR
> -		 * instead of disabling and re-enabling.
> -		 * Workaround tells us to write 0 to CUR_SURFLIVE_A,
> -		 * but it makes more sense write to the current active
> -		 * pipe.
> -		 */
> -		I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0);
> -	}
> +	if (frontbuffer_bits)
> +		psr_force_hw_tracking_exit(dev_priv);
>  
>  	if (!dev_priv->psr.active && !dev_priv-
> >psr.busy_frontbuffer_bits)
>  		schedule_work(&dev_priv->psr.work);

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

Re: [PATCH v5 9/9] drm/i915: Enable PSR2 by default

[Dhinakaran Pandiyan]

On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> The support for PSR2 was polished, IGT tests for PSR2 was added and
> it was tested performing regular user workloads like browsing,
> editing documents and compiling Linux, so it is time to enable it by
> default and enjoy even more power-savings.

Great job :)
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Is there a reason to include the PSR1 CRC fix in this series, that is
an orthogonal issue IMO. And I think we should merge the PSR2 patches
independently.

> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 5d66e7313c75..bae266869c20 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -80,9 +80,6 @@ static bool intel_psr2_enabled(struct
> drm_i915_private *dev_priv,
>  	case I915_PSR_DEBUG_DISABLE:
>  	case I915_PSR_DEBUG_FORCE_PSR1:
>  		return false;
> -	case I915_PSR_DEBUG_DEFAULT:
> -		if (i915_modparams.enable_psr <= 0)
> -			return false;
>  	default:
>  		return crtc_state->has_psr2;
>  	}

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

Re: [PATCH v5 8/9] drm/i915: Force PSR exit when getting pipe CRC

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 5419 bytes --]

On Thu, 2019-03-07 at 13:25 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> > If PSR is active when pipe CRC is enabled the CRC calculations will
> > be inhibit by the transition to low power states that PSR brings.
> The MMIO write to enable CRCs should bring the hardware out from PSR,
> but I can imagine some initial CRCs  are going to be corrupt or
> unavailable.

That do not happen, if PSR is active and CRC is configured PSR will
remain active and no CRC will be calculated.

> 
> > So lets for a PSR exit and as soon as pipe CRC is enabled it will
> There is a missing word.

Thanks

"So lets force a PSR exit and as soon as pipe CRC is enabled it will
block PSR activation and avoid CRC timeouts when running IGT tests."


> 
> > block PSR activation avoid CRC timeouts when running IGT tests.
> 
> This surely has fdo bugs, please include them in the commit message.

I did this patch because of the regressions that I got from CI in my
testings, the only bug that I found related is 
https://bugs.freedesktop.org/show_bug.cgi?id=107814 but we don't have
PSR enabled by default on HSW.

> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 36 ++++++++++++++++++++--------
> > ----
> >  1 file changed, 23 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index d3e3996551c6..5d66e7313c75 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -452,6 +452,7 @@ static void hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> >  	 * frames, we'll go with 9 frames for now
> >  	 */
> >  	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency
> > + 1);
> > +
> >  	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
> >  
> >  	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
> > @@ -851,6 +852,20 @@ void intel_psr_disable(struct intel_dp
> > *intel_dp,
> >  	cancel_work_sync(&dev_priv->psr.work);
> >  }
> >  
> > +static void psr_force_hw_tracking_exit(struct drm_i915_private
> > *dev_priv)
> > +{
> > +	/*
> > +	 * Display WA #0884: all
> > +	 * This documented WA for bxt can be safely applied
> > +	 * broadly so we can force HW tracking to exit PSR
> > +	 * instead of disabling and re-enabling.
> > +	 * Workaround tells us to write 0 to CUR_SURFLIVE_A,
> > +	 * but it makes more sense write to the current active
> > +	 * pipe.
> > +	 */
> > +	I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0);
> > +}
> > +
> >  /**
> >   * intel_psr_update - Update PSR state
> >   * @intel_dp: Intel DP
> > @@ -875,8 +890,13 @@ void intel_psr_update(struct intel_dp
> > *intel_dp,
> >  	enable = crtc_state->has_psr && psr_global_enabled(psr->debug);
> >  	psr2_enable = intel_psr2_enabled(dev_priv, crtc_state);
> >  
> > -	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled)
> > +	if (enable == psr->enabled && psr2_enable == psr-
> > >psr2_enabled) 
> > {
> 
> PSR2 is enabled, then user requests CRCs, the mode_changed commit
> switches to PSR1. The above condition isn't true in that case.

Not sure if I understood the question but if PSR2 is enabled and user
request CRC it will switch to PSR1 already forcing a PSR exit so we
don't need to call psr_force_hw_tracking_exit() in this case.

> 
> Also, since the CRC workaround is done before enabling pipe CRC,
> isn't
> there a possibility that the idle frame counter times out and
> activates
> PSR1 before CRC is enabled?

There still the possibility since syscalls can be preempted too but
this is a huge improvement over current scenario, now it will give at
least the time of 6 idle frames between
intel_crtc_crc_setup_workarounds() and the I915_WRITE() to the PIPE CRC
registers and it happens right after intel_crtc_crc_setup_workarounds()
returns.

> 
> > +		/* Force a PSR exit when enabling CRC to avoid CRC
> > timeouts */
> > +		if (crtc_state->crc_enabled && psr->enabled)
> > +			psr_force_hw_tracking_exit(dev_priv);
> The patch fixes a PSR1 issue, why isn't there any reference to PSR1
> anywhere?

fair, going to update the commit message.

> 
> 
> > +
> >  		goto unlock;
> > +	}
> >  
> >  	if (psr->enabled)
> >  		intel_psr_disable_locked(intel_dp);
> > @@ -1146,18 +1166,8 @@ void intel_psr_flush(struct drm_i915_private
> > *dev_priv,
> >  	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
> >  
> >  	/* By definition flush = invalidate + flush */
> > -	if (frontbuffer_bits) {
> > -		/*
> > -		 * Display WA #0884: all
> > -		 * This documented WA for bxt can be safely applied
> > -		 * broadly so we can force HW tracking to exit PSR
> > -		 * instead of disabling and re-enabling.
> > -		 * Workaround tells us to write 0 to CUR_SURFLIVE_A,
> > -		 * but it makes more sense write to the current active
> > -		 * pipe.
> > -		 */
> > -		I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0);
> > -	}
> > +	if (frontbuffer_bits)
> > +		psr_force_hw_tracking_exit(dev_priv);
> >  
> >  	if (!dev_priv->psr.active && !dev_priv-
> > > psr.busy_frontbuffer_bits)
> >  		schedule_work(&dev_priv->psr.work);

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v5 9/9] drm/i915: Enable PSR2 by default

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 1801 bytes --]

On Thu, 2019-03-07 at 13:33 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> > The support for PSR2 was polished, IGT tests for PSR2 was added and
> > it was tested performing regular user workloads like browsing,
> > editing documents and compiling Linux, so it is time to enable it
> > by
> > default and enjoy even more power-savings.
> 
> Great job :)
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> Is there a reason to include the PSR1 CRC fix in this series, that is
> an orthogonal issue IMO. And I think we should merge the PSR2 patches
> independently.

I added those patches to get all green CI runs but I can merge all
other patches than 'drm/i915: Force PSR exit when getting pipe CRC', I
just answered your questions on that patch, lets see what you think
about the answers.

> 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 5d66e7313c75..bae266869c20 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -80,9 +80,6 @@ static bool intel_psr2_enabled(struct
> > drm_i915_private *dev_priv,
> >  	case I915_PSR_DEBUG_DISABLE:
> >  	case I915_PSR_DEBUG_FORCE_PSR1:
> >  		return false;
> > -	case I915_PSR_DEBUG_DEFAULT:
> > -		if (i915_modparams.enable_psr <= 0)
> > -			return false;
> >  	default:
> >  		return crtc_state->has_psr2;
> >  	}

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v5 8/9] drm/i915: Force PSR exit when getting pipe CRC

[Dhinakaran Pandiyan]

On Thu, 2019-03-07 at 13:57 -0800, Souza, Jose wrote:
> On Thu, 2019-03-07 at 13:25 -0800, Dhinakaran Pandiyan wrote:
> > On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> > > If PSR is active when pipe CRC is enabled the CRC calculations
> > > will
> > > be inhibit by the transition to low power states that PSR brings.
> > 
> > The MMIO write to enable CRCs should bring the hardware out from
> > PSR,
> > but I can imagine some initial CRCs  are going to be corrupt or
> > unavailable.
> 
> That do not happen, if PSR is active and CRC is configured PSR will
> remain active and no CRC will be calculated.

Odd, MMIO writes should trigger DC6 exit, which always results in PSR
exit. If the hardware isnt't in DC6, then I can imagine CRC enabling
not causing a PSR exit. 
> 
> > 
> > > So lets for a PSR exit and as soon as pipe CRC is enabled it will
> > 
> > There is a missing word.
> 
> Thanks
> 
> "So lets force a PSR exit and as soon as pipe CRC is enabled it will
> block PSR activation and avoid CRC timeouts when running IGT tests."
> 
> 
> > 
> > > block PSR activation avoid CRC timeouts when running IGT tests.
> > 
> > This surely has fdo bugs, please include them in the commit
> > message.
> 
> I did this patch because of the regressions that I got from CI in my
> testings, 
Can you include the test name and platform in the commit message?

> the only bug that I found related is 
> https://bugs.freedesktop.org/show_bug.cgi?id=107814 but we don't have
> PSR enabled by default on HSW.

Jani S had reported the issue, Cc'ing him.

> 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 36 ++++++++++++++++++++----
> > > ----
> > > ----
> > >  1 file changed, 23 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index d3e3996551c6..5d66e7313c75 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -452,6 +452,7 @@ static void hsw_activate_psr1(struct intel_dp
> > > *intel_dp)
> > >  	 * frames, we'll go with 9 frames for now
> > >  	 */
> > >  	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency
> > > + 1);
> > > +
> > >  	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
> > >  
> > >  	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
> > > @@ -851,6 +852,20 @@ void intel_psr_disable(struct intel_dp
> > > *intel_dp,
> > >  	cancel_work_sync(&dev_priv->psr.work);
> > >  }
> > >  
> > > +static void psr_force_hw_tracking_exit(struct drm_i915_private
> > > *dev_priv)
> > > +{
> > > +	/*
> > > +	 * Display WA #0884: all
> > > +	 * This documented WA for bxt can be safely applied
> > > +	 * broadly so we can force HW tracking to exit PSR
> > > +	 * instead of disabling and re-enabling.
> > > +	 * Workaround tells us to write 0 to CUR_SURFLIVE_A,
> > > +	 * but it makes more sense write to the current active
> > > +	 * pipe.
> > > +	 */
> > > +	I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0);
> > > +}
> > > +
> > >  /**
> > >   * intel_psr_update - Update PSR state
> > >   * @intel_dp: Intel DP
> > > @@ -875,8 +890,13 @@ void intel_psr_update(struct intel_dp
> > > *intel_dp,
> > >  	enable = crtc_state->has_psr && psr_global_enabled(psr->debug);
> > >  	psr2_enable = intel_psr2_enabled(dev_priv, crtc_state);
> > >  
> > > -	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled)
> > > +	if (enable == psr->enabled && psr2_enable == psr-
> > > > psr2_enabled) 
> > > 
> > > {
> > 
> > PSR2 is enabled, then user requests CRCs, the mode_changed commit
> > switches to PSR1. The above condition isn't true in that case.
> 
> Not sure if I understood the question but if PSR2 is enabled and user
> request CRC it will switch to PSR1 already forcing a PSR exit so we
> don't need to call psr_force_hw_tracking_exit() in this case.
The PSR2->PSR1 switching gives us the same window before CRC enabling
as force_hw_tracking_exit(). My question was about if that window is
sufficient.
> 
> > 
> > Also, since the CRC workaround is done before enabling pipe CRC,
> > isn't
> > there a possibility that the idle frame counter times out and
> > activates
> > PSR1 before CRC is enabled?
> 
> There still the possibility since syscalls can be preempted too but
> this is a huge improvement over current scenario, now it will give at
> least the time of 6 idle frames between
> intel_crtc_crc_setup_workarounds() and the I915_WRITE() to the PIPE
> CRC
> registers and it happens right after
> intel_crtc_crc_setup_workarounds()
> returns.

Yeah, it should be okay. Please document in the commit message the idea
is that we rely on the idle frame counter to have not expired before
CRC is enabled and we are only kicking the hardware out of PSR1. Since
what this patch does is different from what psr_exit() implements, I
think it makes sense to clarify what exit means.

With the commit message fixed, this looks okay to me but let's see if
the timeouts go away.

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> 
> > 
> > > +		/* Force a PSR exit when enabling CRC to avoid CRC
> > > timeouts */
> > > +		if (crtc_state->crc_enabled && psr->enabled)
> > > +			psr_force_hw_tracking_exit(dev_priv);
> > 
> > The patch fixes a PSR1 issue, why isn't there any reference to PSR1
> > anywhere?
> 
> fair, going to update the commit message.
> 
> > 
> > 
> > > +
> > >  		goto unlock;
> > > +	}
> > >  
> > >  	if (psr->enabled)
> > >  		intel_psr_disable_locked(intel_dp);
> > > @@ -1146,18 +1166,8 @@ void intel_psr_flush(struct
> > > drm_i915_private
> > > *dev_priv,
> > >  	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
> > >  
> > >  	/* By definition flush = invalidate + flush */
> > > -	if (frontbuffer_bits) {
> > > -		/*
> > > -		 * Display WA #0884: all
> > > -		 * This documented WA for bxt can be safely applied
> > > -		 * broadly so we can force HW tracking to exit PSR
> > > -		 * instead of disabling and re-enabling.
> > > -		 * Workaround tells us to write 0 to CUR_SURFLIVE_A,
> > > -		 * but it makes more sense write to the current active
> > > -		 * pipe.
> > > -		 */
> > > -		I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0);
> > > -	}
> > > +	if (frontbuffer_bits)
> > > +		psr_force_hw_tracking_exit(dev_priv);
> > >  
> > >  	if (!dev_priv->psr.active && !dev_priv-
> > > > psr.busy_frontbuffer_bits)
> > > 
> > >  		schedule_work(&dev_priv->psr.work);

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

Re: [PATCH v5 4/9] drm/i915/psr: Drop test for EDP in CRTC when forcing commit

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 1910 bytes --]

On Thu, 2019-03-07 at 12:26 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> > If has_psr is set it means that CRTC has a EDP panel attached so it
> > can be dropped, also has_psr is better than check for EDP output
> > alone as it will avoid set mode_changed when PSR is not supported
> > in
> > panel or with current modeset.
> Since we were already checking has_psr, I don't think it is relevant
> to
> say checking just that is "better than check for EDP output alone".
> Can
> you please reword the commit message?

What about replace everything to:

If has_psr is set it means that CRTC has a EDP panel attached so the
EDP check is redundant and can be dropped.

> 
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 6175b1d2e0c8..2d9f64c362e2 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -981,9 +981,7 @@ static int intel_psr_fastset_force(struct
> > drm_i915_private *dev_priv)
> >  
> >  		intel_crtc_state = to_intel_crtc_state(crtc_state);
> >  
> > -		if (crtc_state->active &&
> > -		    intel_crtc_has_type(intel_crtc_state,
> > INTEL_OUTPUT_EDP) &&
> > -		    intel_crtc_state->has_psr) {
> > +		if (crtc_state->active && intel_crtc_state->has_psr) {
> >  			/* Mark mode as changed to trigger a pipe-
> > > update() */
> >  			crtc_state->mode_changed = true;
> >  			break;

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v5 4/9] drm/i915/psr: Drop test for EDP in CRTC when forcing commit

[Pandiyan, Dhinakaran]

On Thu, 2019-03-07 at 14:53 -0800, Souza, Jose wrote:
> On Thu, 2019-03-07 at 12:26 -0800, Dhinakaran Pandiyan wrote:
> > On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> > > If has_psr is set it means that CRTC has a EDP panel attached so
> > > it
> > > can be dropped, also has_psr is better than check for EDP output
> > > alone as it will avoid set mode_changed when PSR is not supported
> > > in
> > > panel or with current modeset.
> > 
> > Since we were already checking has_psr, I don't think it is
> > relevant
> > to
> > say checking just that is "better than check for EDP output alone".
> > Can
> > you please reword the commit message?
> 
> What about replace everything to:
> 
> If has_psr is set it means that CRTC has a EDP panel attached so the
> EDP check is redundant and can be dropped.
Yes, reads better.

> 
> > 
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 6175b1d2e0c8..2d9f64c362e2 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -981,9 +981,7 @@ static int intel_psr_fastset_force(struct
> > > drm_i915_private *dev_priv)
> > >  
> > >  		intel_crtc_state = to_intel_crtc_state(crtc_state);
> > >  
> > > -		if (crtc_state->active &&
> > > -		    intel_crtc_has_type(intel_crtc_state,
> > > INTEL_OUTPUT_EDP) &&
> > > -		    intel_crtc_state->has_psr) {
> > > +		if (crtc_state->active && intel_crtc_state->has_psr) {
> > >  			/* Mark mode as changed to trigger a pipe-
> > > > update() */
> > > 
> > >  			crtc_state->mode_changed = true;
> > >  			break;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 5/9] drm/i915/crc: Make IPS workaround generic

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 9320 bytes --]

On Thu, 2019-03-07 at 12:18 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> > Other features like PSR2 also needs to be disabled while getting
> > CRC
> > so lets rename ips_force_disable to crc_enabled, drop all this
> > checks
> > for pipe A and HSW and BDW and make it generic and
> > hsw_compute_ips_config() will take care of all the checks removed
> > from here.
> > 
> > v2: Renaming and parameter changes to the functions that prepares
> > the
> > commit (Ville)
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c  | 10 ++++--
> >  drivers/gpu/drm/i915/intel_drv.h      |  3 +-
> >  drivers/gpu/drm/i915/intel_pipe_crc.c | 47 +++++++++++------------
> > ----
> >  3 files changed, 29 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 9312b3f35eb0..b3a5d8462251 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6751,7 +6751,13 @@ static bool hsw_compute_ips_config(struct
> > intel_crtc_state *crtc_state)
> >  	if (!hsw_crtc_state_ips_capable(crtc_state))
> >  		return false;
> >  
> > -	if (crtc_state->ips_force_disable)
> > +	/*
> > +	 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> > +	 * enabled and disabled dynamically based on package C states,
> > +	 * user space can't make reliable use of the CRCs, so let's
> > just
> > +	 * completely disable it.
> > +	 */
> > +	if (crtc_state->crc_enabled)
> >  		return false;
> >  
> >  	/* IPS should be fine as long as at least one plane is enabled.
> > */
> > @@ -11687,7 +11693,7 @@ clear_intel_crtc_state(struct
> > intel_crtc_state *crtc_state)
> >  	saved_state->shared_dpll = crtc_state->shared_dpll;
> >  	saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> >  	saved_state->pch_pfit.force_thru = crtc_state-
> > > pch_pfit.force_thru;
> > -	saved_state->ips_force_disable = crtc_state->ips_force_disable;
> > +	saved_state->crc_enabled = crtc_state->crc_enabled;
> >  	if (IS_G4X(dev_priv) ||
> >  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >  		saved_state->wm = crtc_state->wm;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 5412373e2f98..2be64529e4a2 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -999,7 +999,8 @@ struct intel_crtc_state {
> >  	struct intel_link_m_n fdi_m_n;
> >  
> >  	bool ips_enabled;
> > -	bool ips_force_disable;
> > +
> > +	bool crc_enabled;
> >  
> >  	bool enable_fbc;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index 53d4ec68d3c4..af64597c5c6e 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -280,15 +280,15 @@ static int ilk_pipe_crc_ctl_reg(enum
> > intel_pipe_crc_source *source,
> >  	return 0;
> >  }
> >  
> > -static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> > -			      bool enable)
> > +static void
> > +intel_crtc_crc_setup_workarounds(struct intel_crtc *crtc, bool
> > enable)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  	struct drm_device *dev = &dev_priv->drm;
> We could eliminate this local.

Done

> 
> > -	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> > PIPE_A);
> >  	struct intel_crtc_state *pipe_config;
> >  	struct drm_atomic_state *state;
> >  	struct drm_modeset_acquire_ctx ctx;
> > -	int ret = 0;
> > +	int ret;
> >  
> >  	drm_modeset_acquire_init(&ctx, 0);
> >  
> > @@ -307,17 +307,9 @@ static void hsw_pipe_A_crc_wa(struct
> > drm_i915_private *dev_priv,
> >  		goto put_state;
> >  	}
> >  
> > -	if (HAS_IPS(dev_priv)) {
> > -		/*
> > -		 * When IPS gets enabled, the pipe CRC changes. Since
> > IPS gets
> > -		 * enabled and disabled dynamically based on package C
> > states,
> > -		 * user space can't make reliable use of the CRCs, so
> > let's just
> > -		 * completely disable it.
> > -		 */
> > -		pipe_config->ips_force_disable = enable;
> > -	}
> > +	pipe_config->crc_enabled = enable;
> >  
> > -	if (IS_HASWELL(dev_priv)) {
> > +	if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) {
> >  		pipe_config->pch_pfit.force_thru = enable;
> >  		if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> >  		    pipe_config->pch_pfit.enabled != enable)
> > @@ -343,8 +335,7 @@ static void hsw_pipe_A_crc_wa(struct
> > drm_i915_private *dev_priv,
> >  static int ivb_pipe_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >  				enum pipe pipe,
> >  				enum intel_pipe_crc_source *source,
> > -				u32 *val,
> > -				bool set_wa)
> > +				u32 *val)
> >  {
> >  	if (*source == INTEL_PIPE_CRC_SOURCE_AUTO)
> >  		*source = INTEL_PIPE_CRC_SOURCE_PIPE;
> > @@ -357,10 +348,6 @@ static int ivb_pipe_crc_ctl_reg(struct
> > drm_i915_private *dev_priv,
> >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_SPRITE_IVB;
> >  		break;
> >  	case INTEL_PIPE_CRC_SOURCE_PIPE:
> > -		if (set_wa && (IS_HASWELL(dev_priv) ||
> > -		     IS_BROADWELL(dev_priv)) && pipe == PIPE_A)
> > -			hsw_pipe_A_crc_wa(dev_priv, true);
> > -
> >  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
> >  		break;
> >  	case INTEL_PIPE_CRC_SOURCE_NONE:
> > @@ -418,8 +405,7 @@ static int skl_pipe_crc_ctl_reg(struct
> > drm_i915_private *dev_priv,
> >  
> >  static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
> >  			       enum pipe pipe,
> > -			       enum intel_pipe_crc_source *source, u32
> > *val,
> > -			       bool set_wa)
> > +			       enum intel_pipe_crc_source *source, u32
> > *val)
> >  {
> >  	if (IS_GEN(dev_priv, 2))
> >  		return i8xx_pipe_crc_ctl_reg(source, val);
> > @@ -430,7 +416,7 @@ static int get_new_crc_ctl_reg(struct
> > drm_i915_private *dev_priv,
> >  	else if (IS_GEN_RANGE(dev_priv, 5, 6))
> >  		return ilk_pipe_crc_ctl_reg(source, val);
> >  	else if (INTEL_GEN(dev_priv) < 9)
> > -		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source,
> > val, set_wa);
> > +		return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source,
> > val);
> >  	else
> >  		return skl_pipe_crc_ctl_reg(dev_priv, pipe, source,
> > val);
> >  }
> > @@ -605,6 +591,7 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > *crtc, const char *source_name)
> >  	intel_wakeref_t wakeref;
> >  	u32 val = 0; /* shut up gcc */
> >  	int ret = 0;
> > +	bool enable;
> >  
> >  	if (display_crc_ctl_parse_source(source_name, &source) < 0) {
> >  		DRM_DEBUG_DRIVER("unknown source %s\n", source_name);
> > @@ -618,7 +605,11 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > *crtc, const char *source_name)
> >  		return -EIO;
> >  	}
> >  
> > -	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val,
> > true);
> > +	enable = source != INTEL_PIPE_CRC_SOURCE_NONE;
> > +	if (enable)
> > +		intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc),
> > enable);
> I personally, find it easier to read when we pass true/false, it's
> just
> clear that we have already worked out what do when enable is true.
> 	intel_crtc_crc_setup_workaround(to_intel_crtc(crtc), true);

Done

> 
> 
> > +
> > +	ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source,
> > &val);
> >  	if (ret != 0)
> >  		goto out;
> >  
> > @@ -629,14 +620,14 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > *crtc, const char *source_name)
> >  	if (!source) {
> >  		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >  			vlv_undo_pipe_scramble_reset(dev_priv, crtc-
> > > index);
> > -		else if ((IS_HASWELL(dev_priv) ||
> > -			  IS_BROADWELL(dev_priv)) && crtc->index ==
> > PIPE_A)
> > -			hsw_pipe_A_crc_wa(dev_priv, false);
> >  	}
> >  
> >  	pipe_crc->skipped = 0;
> >  
> >  out:
> > +	if (!enable)
> > +		intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc),
> > enable);
> > +
> >  	intel_display_power_put(dev_priv, power_domain, wakeref);
> >  
> >  	return ret;
> > @@ -652,7 +643,7 @@ void intel_crtc_enable_pipe_crc(struct
> > intel_crtc
> > *intel_crtc)
> >  	if (!crtc->crc.opened)
> >  		return;
> >  
> > -	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc-
> > > source, &val, false) < 0)
> > +	if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc-
> > > source, &val) < 0)
> 
> Now that 'ips_force_disable' is 'crc_enabled', I guess we should set
> it
> here for the sake of completeness. Anyway, doing that doesn't have
> any
> functional benefit since we weren't doing it earlier.

This is only executed when doing a full modeset with CRC alread
enabled, so crc_enabled will be kept set and the workarounds will be
applied again, that is why we don't need to call it here.

> 
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >  		return;
> >  
> >  	/* Don't need pipe_crc->lock here, IRQs are not generated. */

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v5 6/9] drm/i915: Disable PSR2 while getting pipe CRC

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 3150 bytes --]

On Thu, 2019-03-07 at 12:47 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> > When PSR2 is active aka after the number of frames programmed in
> > PSR2_CTL 'Frames Before SU Entry' hardware stops to generate CRC
> > interruptions causing IGT tests to fail due timeout.
> s/interruptions/interrupts
> 
> I suppose there are no bugzilla references, since we do not check
> CRCs
> in kms_psr and PSR2 is not enabled in kms_frontbuffer_tracking 

Exacly, I have added simple CRC tests in kms_psr to test the expected
behavior with PSR1 and PSR2 enabled, will send it after get PSR2
enabled by default.

> > This same behavior don't happen with PSR1, as soon as pipe CRC is
> > enabled it blocks PSR1 activation
> The hardware also deactivates PSR1, not just block future PSR1
> activation. Right?

It do not exit PSR1 when writing PIPE CRC registers that is why
'drm/i915: Force PSR exit when getting pipe CRC' is needed.

> 
> >  so CRC calculation continues to
> > happens normaly.
> > 
> > This patch also set mode_changed as true when PSR is available to
> > force atomic check functions to compute new PSR state, otherwise
> > PSR2
> > would not be disabled.
> > 
> > v4: Only setting mode_changed if has_psr is set(Dhinakaran)
> > 
> > v3: Reusing intel_crtc_crc_prepare() and crc_enabled, only setting
> > mode_changed if it can do PSR.
> > 
> > v2: Changed commit description to describe that PSR2 inhibit CRC
> > calculations.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pipe_crc.c | 1 +
> >  drivers/gpu/drm/i915/intel_psr.c      | 3 +++
> >  2 files changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index af64597c5c6e..c17f02b88453 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -307,6 +307,7 @@ intel_crtc_crc_setup_workarounds(struct
> > intel_crtc *crtc, bool enable)
> >  		goto put_state;
> >  	}
> >  
> > +	pipe_config->base.mode_changed = pipe_config->has_psr;
> >  	pipe_config->crc_enabled = enable;
> >  
> >  	if (IS_HASWELL(dev_priv) && crtc->pipe == PIPE_A) {
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 2d9f64c362e2..73453d89a841 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -572,6 +572,9 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> >  		return false;
> >  	}
> >  
> > +	if (crtc_state->crc_enabled)
> > +		return false;
> No DRM_DEBUG_KMS()? 

Done:

DRM_DEBUG_KMS("PSR2 not enabled because it would inhibit pipe CRC calculation\n");

> 
> with that added
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > +
> >  	return true;
> >  }
> >  

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v5 8/9] drm/i915: Force PSR exit when getting pipe CRC

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 7399 bytes --]

On Thu, 2019-03-07 at 14:30 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2019-03-07 at 13:57 -0800, Souza, Jose wrote:
> > On Thu, 2019-03-07 at 13:25 -0800, Dhinakaran Pandiyan wrote:
> > > On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> > > > If PSR is active when pipe CRC is enabled the CRC calculations
> > > > will
> > > > be inhibit by the transition to low power states that PSR
> > > > brings.
> > > 
> > > The MMIO write to enable CRCs should bring the hardware out from
> > > PSR,
> > > but I can imagine some initial CRCs  are going to be corrupt or
> > > unavailable.
> > 
> > That do not happen, if PSR is active and CRC is configured PSR will
> > remain active and no CRC will be calculated.
> 
> Odd, MMIO writes should trigger DC6 exit, which always results in PSR
> exit. If the hardware isnt't in DC6, then I can imagine CRC enabling
> not causing a PSR exit. 
> > > > So lets for a PSR exit and as soon as pipe CRC is enabled it
> > > > will
> > > 
> > > There is a missing word.
> > 
> > Thanks
> > 
> > "So lets force a PSR exit and as soon as pipe CRC is enabled it
> > will
> > block PSR activation and avoid CRC timeouts when running IGT
> > tests."
> > 
> > 
> > > > block PSR activation avoid CRC timeouts when running IGT tests.
> > > 
> > > This surely has fdo bugs, please include them in the commit
> > > message.
> > 
> > I did this patch because of the regressions that I got from CI in
> > my
> > testings, 
> Can you include the test name and platform in the commit message?

Done

> 
> > the only bug that I found related is 
> > https://bugs.freedesktop.org/show_bug.cgi?id=107814 but we don't
> > have
> > PSR enabled by default on HSW.
> 
> Jani S had reported the issue, Cc'ing him.

As said the issue don't look related to this.

> 
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_psr.c | 36 ++++++++++++++++++++----
> > > > ----
> > > > ----
> > > >  1 file changed, 23 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index d3e3996551c6..5d66e7313c75 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -452,6 +452,7 @@ static void hsw_activate_psr1(struct
> > > > intel_dp
> > > > *intel_dp)
> > > >  	 * frames, we'll go with 9 frames for now
> > > >  	 */
> > > >  	idle_frames = max(idle_frames, dev_priv-
> > > > >psr.sink_sync_latency
> > > > + 1);
> > > > +
> > > >  	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
> > > >  
> > > >  	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
> > > > @@ -851,6 +852,20 @@ void intel_psr_disable(struct intel_dp
> > > > *intel_dp,
> > > >  	cancel_work_sync(&dev_priv->psr.work);
> > > >  }
> > > >  
> > > > +static void psr_force_hw_tracking_exit(struct drm_i915_private
> > > > *dev_priv)
> > > > +{
> > > > +	/*
> > > > +	 * Display WA #0884: all
> > > > +	 * This documented WA for bxt can be safely applied
> > > > +	 * broadly so we can force HW tracking to exit PSR
> > > > +	 * instead of disabling and re-enabling.
> > > > +	 * Workaround tells us to write 0 to CUR_SURFLIVE_A,
> > > > +	 * but it makes more sense write to the current active
> > > > +	 * pipe.
> > > > +	 */
> > > > +	I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0);
> > > > +}
> > > > +
> > > >  /**
> > > >   * intel_psr_update - Update PSR state
> > > >   * @intel_dp: Intel DP
> > > > @@ -875,8 +890,13 @@ void intel_psr_update(struct intel_dp
> > > > *intel_dp,
> > > >  	enable = crtc_state->has_psr && psr_global_enabled(psr-
> > > > >debug);
> > > >  	psr2_enable = intel_psr2_enabled(dev_priv, crtc_state);
> > > >  
> > > > -	if (enable == psr->enabled && psr2_enable == psr-
> > > > >psr2_enabled)
> > > > +	if (enable == psr->enabled && psr2_enable == psr-
> > > > > psr2_enabled) 
> > > > 
> > > > {
> > > 
> > > PSR2 is enabled, then user requests CRCs, the mode_changed commit
> > > switches to PSR1. The above condition isn't true in that case.
> > 
> > Not sure if I understood the question but if PSR2 is enabled and
> > user
> > request CRC it will switch to PSR1 already forcing a PSR exit so we
> > don't need to call psr_force_hw_tracking_exit() in this case.
> The PSR2->PSR1 switching gives us the same window before CRC enabling
> as force_hw_tracking_exit(). My question was about if that window is
> sufficient.
> > > Also, since the CRC workaround is done before enabling pipe CRC,
> > > isn't
> > > there a possibility that the idle frame counter times out and
> > > activates
> > > PSR1 before CRC is enabled?
> > 
> > There still the possibility since syscalls can be preempted too but
> > this is a huge improvement over current scenario, now it will give
> > at
> > least the time of 6 idle frames between
> > intel_crtc_crc_setup_workarounds() and the I915_WRITE() to the PIPE
> > CRC
> > registers and it happens right after
> > intel_crtc_crc_setup_workarounds()
> > returns.
> 
> Yeah, it should be okay. Please document in the commit message the
> idea
> is that we rely on the idle frame counter to have not expired before
> CRC is enabled and we are only kicking the hardware out of PSR1.
> Since
> what this patch does is different from what psr_exit() implements, I
> think it makes sense to clarify what exit means.
> 
> With the commit message fixed, this looks okay to me but let's see if
> the timeouts go away.

Done, thanks.

> 
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> > > > +		/* Force a PSR exit when enabling CRC to avoid
> > > > CRC
> > > > timeouts */
> > > > +		if (crtc_state->crc_enabled && psr->enabled)
> > > > +			psr_force_hw_tracking_exit(dev_priv);
> > > 
> > > The patch fixes a PSR1 issue, why isn't there any reference to
> > > PSR1
> > > anywhere?
> > 
> > fair, going to update the commit message.
> > 
> > > 
> > > > +
> > > >  		goto unlock;
> > > > +	}
> > > >  
> > > >  	if (psr->enabled)
> > > >  		intel_psr_disable_locked(intel_dp);
> > > > @@ -1146,18 +1166,8 @@ void intel_psr_flush(struct
> > > > drm_i915_private
> > > > *dev_priv,
> > > >  	dev_priv->psr.busy_frontbuffer_bits &=
> > > > ~frontbuffer_bits;
> > > >  
> > > >  	/* By definition flush = invalidate + flush */
> > > > -	if (frontbuffer_bits) {
> > > > -		/*
> > > > -		 * Display WA #0884: all
> > > > -		 * This documented WA for bxt can be safely
> > > > applied
> > > > -		 * broadly so we can force HW tracking to exit
> > > > PSR
> > > > -		 * instead of disabling and re-enabling.
> > > > -		 * Workaround tells us to write 0 to
> > > > CUR_SURFLIVE_A,
> > > > -		 * but it makes more sense write to the current
> > > > active
> > > > -		 * pipe.
> > > > -		 */
> > > > -		I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0);
> > > > -	}
> > > > +	if (frontbuffer_bits)
> > > > +		psr_force_hw_tracking_exit(dev_priv);
> > > >  
> > > >  	if (!dev_priv->psr.active && !dev_priv-
> > > > > psr.busy_frontbuffer_bits)
> > > > 
> > > >  		schedule_work(&dev_priv->psr.work);

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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