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

[PATCH v4 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 v4 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 v4 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 7c5e84ef5171..816e8f124b3b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11232,7 +11232,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 v4 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>
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 v4 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>
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 816e8f124b3b..328967c642b3 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. */
@@ -11684,7 +11690,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 v4 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>
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 v4 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>
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 v4 8/9] drm/i915/psr: Set idle frames to maximum while getting pipe CRC

[José Roberto de Souza]

Increase the idle frames to activate PSR1 to avoid CRC timeouts, as
soon as pipe CRC is enabled it will avoid PSR1 to activate but if
PSR1 is activate before that, hardware goes to lower power states
that inhibits CRC calculations causing CRC timeout errors in 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/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 453af7438e67..e336f758e481 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -521,6 +521,7 @@ struct i915_psr {
 	bool sink_not_reliable;
 	bool irq_aux_error;
 	u16 su_x_granularity;
+	bool crc_enabled;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index d3e3996551c6..b237d96db277 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -452,6 +452,16 @@ 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);
+
+	/*
+	 * Increase the idle frames to active PSR1 to avoid CRC timeouts, as
+	 * soon as pipe CRC is enabled it will avoid PSR1 to activate but if
+	 * PSR1 is activate before that, hardware goes to lower power states
+	 * that inhibits CRC calculations.
+	 */
+	if (dev_priv->psr.crc_enabled)
+		idle_frames = 0xf;
+
 	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
 
 	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
@@ -723,6 +733,7 @@ static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
 	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 	dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)->pipe;
+	dev_priv->psr.crc_enabled = crtc_state->crc_enabled;
 
 	DRM_DEBUG_KMS("Enabling PSR%s\n",
 		      dev_priv->psr.psr2_enabled ? "2" : "1");
@@ -865,7 +876,7 @@ void intel_psr_update(struct intel_dp *intel_dp,
 {
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
 	struct i915_psr *psr = &dev_priv->psr;
-	bool enable, psr2_enable;
+	bool enable, psr2_enable, pipe_crc_changed;
 
 	if (!CAN_PSR(dev_priv) || READ_ONCE(psr->dp) != intel_dp)
 		return;
@@ -874,8 +885,10 @@ 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);
+	pipe_crc_changed = crtc_state->crc_enabled != psr->crc_enabled;
 
-	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled)
+	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled &&
+	    !pipe_crc_changed)
 		goto unlock;
 
 	if (psr->enabled)
-- 
2.21.0

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

[PATCH v4 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>
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 b237d96db277..116c8b50ee78 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 [v4,1/9] drm/i915/psr: Remove PSR2 FIXME

[Patchwork]

== Series Details ==

Series: series starting with [v4,1/9] drm/i915/psr: Remove PSR2 FIXME
URL   : https://patchwork.freedesktop.org/series/57455/
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/psr: Set idle frames to maximum while 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)
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3566:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3567:16: 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 [v4,1/9] drm/i915/psr: Remove PSR2 FIXME

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_5683 -> Patchwork_12353
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@cs-compute:
    - fi-kbl-8809g:       NOTRUN -> FAIL [fdo#108094]

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
    - fi-apl-guc:         PASS -> DMESG-WARN [fdo#108566]

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

  
#### Possible fixes ####

  * igt@amdgpu/amd_basic@userptr:
    - fi-kbl-8809g:       DMESG-WARN [fdo#108965] -> PASS

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

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#107383]: https://bugs.freedesktop.org/show_bug.cgi?id=107383
  [fdo#108094]: https://bugs.freedesktop.org/show_bug.cgi?id=108094
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965


Participating hosts (45 -> 38)
------------------------------

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


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

    * Linux: CI_DRM_5683 -> Patchwork_12353

  CI_DRM_5683: 40251405bb454b06259738bcebf4529c888f7fe0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4866: 189956af183c245eb237b3be4fa22953ec93bbe0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12353: ff4c7ff7b3f047ed9bd682d56a5050eb9a2b64e4 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

ff4c7ff7b3f0 drm/i915: Enable PSR2 by default
adab6c2064da drm/i915/psr: Set idle frames to maximum while getting pipe CRC
9e7167e444cf drm/i915: Drop redundant checks to update PSR state
0aef9406df81 drm/i915: Disable PSR2 while getting pipe CRC
dd9ba679c107 drm/i915/crc: Make IPS workaround generic
376f040f9469 drm/i915/psr: Drop test for EDP in CRTC when forcing commit
6dcd509f0c19 drm/i915: Compute and commit color features in fastsets
d41c4a7f5c1d drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset
b965245818e9 drm/i915/psr: Remove PSR2 FIXME

== Logs ==

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

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

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_5683_full -> Patchwork_12353_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_create@stolen-invalid-flag:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109277]

  * igt@gem_ctx_isolation@rcs0-dirty-create:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109281] +1

  * igt@gem_exec_schedule@preempt-other-chain-bsd2:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109276] +2

  * igt@gem_mocs_settings@mocs-settings-vebox:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109287] +1

  * igt@gem_userptr_blits@process-exit-gtt:
    - shard-glk:          NOTRUN -> SKIP [fdo#109271] +20

  * igt@i915_pm_rpm@system-suspend-devices:
    - shard-iclb:         PASS -> INCOMPLETE [fdo#107713] / [fdo#108840]

  * igt@i915_pm_rpm@system-suspend-modeset:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#107724] +1

  * igt@kms_atomic_transition@2x-modeset-transitions:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109280] +4

  * igt@kms_atomic_transition@4x-modeset-transitions-nonblocking-fencing:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +5

  * igt@kms_busy@extended-modeset-hang-newfb-render-f:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#107956]

  * igt@kms_busy@extended-modeset-hang-oldfb-render-f:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109278]

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

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

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
    - shard-glk:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
    - shard-glk:          PASS -> FAIL [fdo#108145]

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

  * igt@kms_cursor_crc@cursor-128x42-sliding:
    - shard-apl:          PASS -> FAIL [fdo#103232] +2

  * igt@kms_cursor_crc@cursor-256x256-dpms:
    - shard-glk:          NOTRUN -> FAIL [fdo#103232] +1

  * igt@kms_cursor_crc@cursor-256x256-suspend:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108]

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

  * igt@kms_cursor_crc@cursor-512x512-dpms:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109279]

  * igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
    - shard-hsw:          PASS -> FAIL [fdo#105767]

  * igt@kms_flip@2x-nonexisting-fb:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109274] +3

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

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

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-shrfb-plflip-blt:
    - shard-glk:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-badstride:
    - shard-skl:          NOTRUN -> FAIL [fdo#105682]

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-pwrite:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +5

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

  * igt@kms_frontbuffer_tracking@fbcpsr-rgb565-draw-blt:
    - shard-skl:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-indfb-draw-mmap-wc:
    - shard-skl:          NOTRUN -> FAIL [fdo#103167] +4

  * igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
    - shard-skl:          NOTRUN -> FAIL [fdo#103191] / [fdo#107362]

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

  * igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
    - shard-skl:          NOTRUN -> FAIL [fdo#108145] +3

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

  * igt@kms_plane_alpha_blend@pipe-b-alpha-7efc:
    - shard-apl:          PASS -> FAIL [fdo#108145]

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

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-yf:
    - shard-iclb:         PASS -> FAIL [fdo#103166]

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

  * igt@kms_sysfs_edid_timing:
    - shard-iclb:         PASS -> FAIL [fdo#100047]

  * igt@perf_pmu@busy-accuracy-50-vcs1:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] +96

  * igt@prime_busy@wait-after-bsd:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] +30

  * igt@prime_nv_api@i915_nv_double_export:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] +1

  * igt@prime_nv_test@i915_nv_sharing:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109291]

  
#### Possible fixes ####

  * igt@gem_eio@reset-stress:
    - shard-snb:          INCOMPLETE [fdo#105411] -> PASS

  * igt@gem_mmap_wc@set-cache-level:
    - shard-snb:          SKIP [fdo#109271] -> PASS +3

  * igt@i915_pm_rpm@fences:
    - shard-iclb:         DMESG-WARN [fdo#107724] -> PASS +5

  * igt@i915_pm_rpm@legacy-planes:
    - shard-iclb:         DMESG-WARN [fdo#108654] -> PASS

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
    - shard-iclb:         DMESG-WARN [fdo#107956] -> PASS

  * igt@kms_color@pipe-a-ctm-max:
    - shard-apl:          FAIL [fdo#108147] -> PASS

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

  * igt@kms_cursor_crc@cursor-64x64-suspend:
    - shard-apl:          FAIL [fdo#103191] / [fdo#103232] -> PASS

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-hsw:          FAIL [fdo#102887] -> PASS

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

  * igt@kms_flip@flip-vs-panning-vs-hang:
    - shard-apl:          INCOMPLETE [fdo#103927] -> PASS

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

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

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-draw-blt:
    - shard-iclb:         FAIL [fdo#103167] -> PASS +1

  * igt@kms_plane@pixel-format-pipe-b-planes:
    - shard-apl:          FAIL [fdo#103166] -> PASS

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

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

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

  * igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
    - shard-kbl:          DMESG-FAIL [fdo#105763] -> PASS

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

  * igt@kms_vblank@pipe-b-ts-continuation-suspend:
    - shard-kbl:          INCOMPLETE [fdo#103665] -> PASS

  
#### Warnings ####

  * igt@i915_pm_rpm@pc8-residency:
    - shard-skl:          INCOMPLETE [fdo#107807] -> SKIP [fdo#109271] +1

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

  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#105767]: https://bugs.freedesktop.org/show_bug.cgi?id=105767
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [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#108654]: https://bugs.freedesktop.org/show_bug.cgi?id=108654
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [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#109287]: https://bugs.freedesktop.org/show_bug.cgi?id=109287
  [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts


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

    * Linux: CI_DRM_5683 -> Patchwork_12353

  CI_DRM_5683: 40251405bb454b06259738bcebf4529c888f7fe0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4866: 189956af183c245eb237b3be4fa22953ec93bbe0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12353: ff4c7ff7b3f047ed9bd682d56a5050eb9a2b64e4 @ 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_12353/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

On Fri, Mar 01, 2019 at 05:34:51PM -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.
> 
> 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>

indeed protected by sink_support only getting set on edp init
connector flow..


Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 8/9] drm/i915/psr: Set idle frames to maximum while getting pipe CRC

[Dhinakaran Pandiyan]

On Fri, 2019-03-01 at 17:34 -0800, José Roberto de Souza wrote:
> Increase the idle frames to activate PSR1 to avoid CRC timeouts, as
> soon as pipe CRC is enabled it will avoid PSR1 to activate but if
> PSR1 is activate before that, hardware goes to lower power states
> that inhibits CRC calculations causing CRC timeout errors in 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/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 17 +++++++++++++++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 453af7438e67..e336f758e481 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -521,6 +521,7 @@ struct i915_psr {
>  	bool sink_not_reliable;
>  	bool irq_aux_error;
>  	u16 su_x_granularity;
> +	bool crc_enabled;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index d3e3996551c6..b237d96db277 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -452,6 +452,16 @@ 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);
> +
> +	/*
> +	 * Increase the idle frames to active PSR1 to avoid CRC
> timeouts, as
> +	 * soon as pipe CRC is enabled it will avoid PSR1 to activate
> but if
> +	 * PSR1 is activate before that, hardware goes to lower power
> states
> +	 * that inhibits CRC calculations.
> +	 */
> +	if (dev_priv->psr.crc_enabled)
> +		idle_frames = 0xf;

My understanding was only the enable bit can be changed when PSR is
enabled. Does this work?

> +
>  	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;

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

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

[Rodrigo Vivi]

On Fri, Mar 01, 2019 at 05:34:52PM -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>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

nice clean-up.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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 816e8f124b3b..328967c642b3 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. */
> @@ -11684,7 +11690,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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

On Fri, Mar 01, 2019 at 05:34:53PM -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.
> 
> 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.

Oh, this part was confusing... I was about to ask has_psr2 below,
and then I read the commit message again to understand it.

it probably deserved a separated patch...
or at least a comment on the code...

but, up to you, makes sense now

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


> 
> 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>
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 8/9] drm/i915/psr: Set idle frames to maximum while getting pipe CRC

[Souza, Jose]

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

On Mon, 2019-03-04 at 10:31 -0800, Dhinakaran Pandiyan wrote:
> On Fri, 2019-03-01 at 17:34 -0800, José Roberto de Souza wrote:
> > Increase the idle frames to activate PSR1 to avoid CRC timeouts, as
> > soon as pipe CRC is enabled it will avoid PSR1 to activate but if
> > PSR1 is activate before that, hardware goes to lower power states
> > that inhibits CRC calculations causing CRC timeout errors in 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/i915_drv.h  |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 17 +++++++++++++++--
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 453af7438e67..e336f758e481 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -521,6 +521,7 @@ struct i915_psr {
> >  	bool sink_not_reliable;
> >  	bool irq_aux_error;
> >  	u16 su_x_granularity;
> > +	bool crc_enabled;
> >  };
> >  
> >  enum intel_pch {
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index d3e3996551c6..b237d96db277 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -452,6 +452,16 @@ 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);
> > +
> > +	/*
> > +	 * Increase the idle frames to active PSR1 to avoid CRC
> > timeouts, as
> > +	 * soon as pipe CRC is enabled it will avoid PSR1 to activate
> > but if
> > +	 * PSR1 is activate before that, hardware goes to lower power
> > states
> > +	 * that inhibits CRC calculations.
> > +	 */
> > +	if (dev_priv->psr.crc_enabled)
> > +		idle_frames = 0xf;
> 
> My understanding was only the enable bit can be changed when PSR is
> enabled. Does this work?

That is true, the additional changes in intel_psr_update() will disable
and then enable PSR with this new idle_frames.

> 
> > +
> >  	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;

[-- 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 v4 7/9] drm/i915: Drop redundant checks to update PSR state

[Rodrigo Vivi]

On Fri, Mar 01, 2019 at 05:34:54PM -0800, José Roberto de Souza wrote:
> 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.

is it just redundant or does it really change the behaviour for PSR2
as needed?

code seems right, explanation here not so sure...
but if this is really right and I am missing something feel
free to use:


Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

otherwise please change the msg.

Thanks,
Rodrigo.

> 
> 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 | 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

On Fri, Mar 01, 2019 at 05:34:56PM -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.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

\o/ !

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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 b237d96db277..116c8b50ee78 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dhinakaran Pandiyan]

On Mon, 2019-03-04 at 10:42 -0800, Rodrigo Vivi wrote:
> On Fri, Mar 01, 2019 at 05:34:54PM -0800, José Roberto de Souza
> wrote:
> > 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.
> 
> is it just redundant or does it really change the behaviour for PSR2
> as needed?

I have the same question now that I read José's response to patch 8/9.
At first, I ignored reading this patch because it included the word
"redundant" in the commit message :)

> 
> code seems right, explanation here not so sure...
> but if this is really right and I am missing something feel
> free to use:
> 
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> otherwise please change the msg.
> 
> Thanks,
> Rodrigo.
> 
> > 
> > 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 | 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

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

Re: [PATCH v4 8/9] drm/i915/psr: Set idle frames to maximum while getting pipe CRC

[Dhinakaran Pandiyan]

On Mon, 2019-03-04 at 10:40 -0800, Souza, Jose wrote:
> On Mon, 2019-03-04 at 10:31 -0800, Dhinakaran Pandiyan wrote:
> > On Fri, 2019-03-01 at 17:34 -0800, José Roberto de Souza wrote:
> > > Increase the idle frames to activate PSR1 to avoid CRC timeouts,
> > > as
> > > soon as pipe CRC is enabled it will avoid PSR1 to activate but if
> > > PSR1 is activate before that, hardware goes to lower power states
> > > that inhibits CRC calculations causing CRC timeout errors in IGT
> > > tests.

Okay, so you are solving the case where PSR is already active when we
want CRCs. How does modifying the idle frame count help if PSR is
already active? 

The commit also says PSR does not become active if CRCs were already
enabled (idle count should not matter here). In that case, just
disabling and re-enabling should be good, isn't it? Or perhaps, just
doing a psr_flush() would do the same job?

> > > 
> > > 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/i915_drv.h  |  1 +
> > >  drivers/gpu/drm/i915/intel_psr.c | 17 +++++++++++++++--
> > >  2 files changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 453af7438e67..e336f758e481 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -521,6 +521,7 @@ struct i915_psr {
> > >  	bool sink_not_reliable;
> > >  	bool irq_aux_error;
> > >  	u16 su_x_granularity;
> > > +	bool crc_enabled;
> > >  };
> > >  
> > >  enum intel_pch {
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index d3e3996551c6..b237d96db277 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -452,6 +452,16 @@ 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);
> > > +
> > > +	/*
> > > +	 * Increase the idle frames to active PSR1 to avoid CRC
> > > timeouts, as
> > > +	 * soon as pipe CRC is enabled it will avoid PSR1 to activate
> > > but if
> > > +	 * PSR1 is activate before that, hardware goes to lower power
> > > states
> > > +	 * that inhibits CRC calculations.
> > > +	 */
> > > +	if (dev_priv->psr.crc_enabled)
> > > +		idle_frames = 0xf;
> > 
> > My understanding was only the enable bit can be changed when PSR is
> > enabled. Does this work?
> 
> That is true, the additional changes in intel_psr_update() will
> disable
> and then enable PSR with this new idle_frames.
> 
> > 
> > > +
> > >  	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;

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

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

[Souza, Jose]

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

On Mon, 2019-03-04 at 10:54 -0800, Dhinakaran Pandiyan wrote:
> On Mon, 2019-03-04 at 10:42 -0800, Rodrigo Vivi wrote:
> > On Fri, Mar 01, 2019 at 05:34:54PM -0800, José Roberto de Souza
> > wrote:
> > > 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.
> > 
> > is it just redundant or does it really change the behaviour for
> > PSR2
> > as needed?
> 
> I have the same question now that I read José's response to patch
> 8/9.
> At first, I ignored reading this patch because it included the word
> "redundant" in the commit message :)


It is just redudant, the 'if (enable == psr->enabled && psr2_enable ==
psr->psr2_enabled)' takes care of going from PSR1 -> PSR1, PSR2 -> PSR2
and disabled -> disabled, all other combinations requires one the calls
bellow that can be simplified.


> 
> > code seems right, explanation here not so sure...
> > but if this is really right and I am missing something feel
> > free to use:
> > 
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > otherwise please change the msg.
> > 
> > Thanks,
> > Rodrigo.
> > 
> > > 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 | 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

[-- 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 v4 8/9] drm/i915/psr: Set idle frames to maximum while getting pipe CRC

[Souza, Jose]

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

On Mon, 2019-03-04 at 11:00 -0800, Dhinakaran Pandiyan wrote:
> On Mon, 2019-03-04 at 10:40 -0800, Souza, Jose wrote:
> > On Mon, 2019-03-04 at 10:31 -0800, Dhinakaran Pandiyan wrote:
> > > On Fri, 2019-03-01 at 17:34 -0800, José Roberto de Souza wrote:
> > > > Increase the idle frames to activate PSR1 to avoid CRC
> > > > timeouts,
> > > > as
> > > > soon as pipe CRC is enabled it will avoid PSR1 to activate but
> > > > if
> > > > PSR1 is activate before that, hardware goes to lower power
> > > > states
> > > > that inhibits CRC calculations causing CRC timeout errors in
> > > > IGT
> > > > tests.
> 
> Okay, so you are solving the case where PSR is already active when we
> want CRCs. How does modifying the idle frame count help if PSR is
> already active?

Exactly this sporadicts timeout should be caused by this:

- IGT test modify frontbuffer or do a flip
- IGT execution is preempted and it takes more than X idle frames
- PSR is activated
- IGT test start CRC capture
- CRC timeout

So increasing the idle frames should reduce the scenario above.

> 
> The commit also says PSR does not become active if CRCs were already
> enabled (idle count should not matter here). In that case, just
> disabling and re-enabling should be good, isn't it? Or perhaps, just
> doing a psr_flush() would do the same job?

Good suggestion, psr_flush() should do the same job I will test and
change to that.

> 
> > > > 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/i915_drv.h  |  1 +
> > > >  drivers/gpu/drm/i915/intel_psr.c | 17 +++++++++++++++--
> > > >  2 files changed, 16 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 453af7438e67..e336f758e481 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -521,6 +521,7 @@ struct i915_psr {
> > > >  	bool sink_not_reliable;
> > > >  	bool irq_aux_error;
> > > >  	u16 su_x_granularity;
> > > > +	bool crc_enabled;
> > > >  };
> > > >  
> > > >  enum intel_pch {
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index d3e3996551c6..b237d96db277 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -452,6 +452,16 @@ 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);
> > > > +
> > > > +	/*
> > > > +	 * Increase the idle frames to active PSR1 to avoid CRC
> > > > timeouts, as
> > > > +	 * soon as pipe CRC is enabled it will avoid PSR1 to
> > > > activate
> > > > but if
> > > > +	 * PSR1 is activate before that, hardware goes to lower
> > > > power
> > > > states
> > > > +	 * that inhibits CRC calculations.
> > > > +	 */
> > > > +	if (dev_priv->psr.crc_enabled)
> > > > +		idle_frames = 0xf;
> > > 
> > > My understanding was only the enable bit can be changed when PSR
> > > is
> > > enabled. Does this work?
> > 
> > That is true, the additional changes in intel_psr_update() will
> > disable
> > and then enable PSR with this new idle_frames.
> > 
> > > > +
> > > >  	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;

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