[PATCH 1/4] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset

[PATCH 1/4] 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>
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 75c1a5deebf5..08967836b48e 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -986,7 +986,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->enable &&
+		    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.20.1

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

[PATCH 2/4] drm/i915: Disable PSR2 while getting pipe CRC

[José Roberto de Souza]

As stated in CRC_CTL spec, after PSR entry state CRC will not be
calculated anymore what is not a problem as IGT tests do some screen
change and then request the pipe CRC right after the change so PSR
will go to idle state and only will entry again after at least 6
idles frames.

But for PSR2 it is more problematic as any change to the screen could
trigger a selective/partial update causing the CRC value not to be
calculated over the full frame.

So here it disables PSR2 and keep it disabled while user is
requesting pipe CRC.

BSpec: 7536

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_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_pipe_crc.c | 10 ++++++++++
 drivers/gpu/drm/i915/intel_psr.c      | 23 +++++++++++++++++++++++
 4 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 17fe942eaafa..609e9c5bd453 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -520,6 +520,7 @@ struct i915_psr {
 	bool sink_not_reliable;
 	bool irq_aux_error;
 	u16 su_x_granularity;
+	bool pipe_crc_enabled;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3398b28c053b..40ce7a600585 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2103,6 +2103,7 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp);
 int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
 			    u32 *out_value);
 bool intel_psr_enabled(struct intel_dp *intel_dp);
+void intel_psr_crc_prepare_or_finish(struct drm_i915_private *dev_priv, enum pipe pipe, bool prepare);
 
 /* intel_quirks.c */
 void intel_init_quirks(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index a8554dc4f196..5d8772399f60 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -583,6 +583,14 @@ int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 	return -EINVAL;
 }
 
+static inline void intel_crtc_crc_prepare_or_finish(struct drm_crtc *crtc, bool prepare)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	intel_psr_crc_prepare_or_finish(dev_priv, intel_crtc->pipe, prepare);
+}
+
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
@@ -609,6 +617,8 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
 	if (ret != 0)
 		goto out;
 
+	intel_crtc_crc_prepare_or_finish(crtc, source != INTEL_PIPE_CRC_SOURCE_NONE);
+
 	pipe_crc->source = source;
 	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
 	POSTING_READ(PIPE_CRC_CTL(crtc->index));
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 08967836b48e..9c93138988aa 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -577,6 +577,9 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 		return false;
 	}
 
+	if (dev_priv->psr.pipe_crc_enabled)
+		return false;
+
 	return true;
 }
 
@@ -1291,3 +1294,23 @@ bool intel_psr_enabled(struct intel_dp *intel_dp)
 
 	return ret;
 }
+
+void intel_psr_crc_prepare_or_finish(struct drm_i915_private *dev_priv, enum pipe pipe, bool prepare)
+{
+	bool fastset = false;
+
+	if (!CAN_PSR(dev_priv))
+		return;
+
+	mutex_lock(&dev_priv->psr.lock);
+
+	if (dev_priv->psr.pipe == pipe) {
+		dev_priv->psr.pipe_crc_enabled = prepare;
+		fastset = !prepare || dev_priv->psr.psr2_enabled;
+	}
+
+	mutex_unlock(&dev_priv->psr.lock);
+
+	if (fastset)
+		intel_psr_fastset_force(dev_priv);
+}
-- 
2.20.1

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

[PATCH 3/4] 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 9c93138988aa..7b12b888061c 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.20.1

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

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

We can hold this patch a little longer, I'm mainly sending it to
show that 'drm/i915: Disable PSR2 while getting pipe CRC' fixed
the CRC tests when PSR2 is enabled.

 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 7b12b888061c..3d8a050407f6 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.20.1

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset
URL   : https://patchwork.freedesktop.org/series/56647/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
e53ec9ed5dc6 drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset
441f8b87c393 drm/i915: Disable PSR2 while getting pipe CRC
-:48: WARNING:LONG_LINE: line over 100 characters
#48: FILE: drivers/gpu/drm/i915/intel_drv.h:2106:
+void intel_psr_crc_prepare_or_finish(struct drm_i915_private *dev_priv, enum pipe pipe, bool prepare);

-:99: WARNING:LONG_LINE: line over 100 characters
#99: FILE: drivers/gpu/drm/i915/intel_psr.c:1298:
+void intel_psr_crc_prepare_or_finish(struct drm_i915_private *dev_priv, enum pipe pipe, bool prepare)

total: 0 errors, 2 warnings, 0 checks, 68 lines checked
727cc543561b drm/i915/psr: Remove PSR2 FIXME
b912f44f1bf4 drm/i915: Enable PSR2 by default

_______________________________________________
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 [1/4] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset
URL   : https://patchwork.freedesktop.org/series/56647/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset
Okay!

Commit: drm/i915: Disable PSR2 while getting pipe CRC
-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/psr: Remove PSR2 FIXME
Okay!

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 [1/4] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset
URL   : https://patchwork.freedesktop.org/series/56647/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5601 -> Patchwork_12220
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

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

  * igt@i915_selftest@live_evict:
    - fi-bsw-kefka:       PASS -> DMESG-WARN [fdo#107709]

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

  
#### Possible fixes ####

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

  * igt@gem_exec_suspend@basic-s3:
    - {fi-icl-u3}:        FAIL [fdo#103375] -> PASS

  * igt@gem_mmap_gtt@basic-small-bo:
    - {fi-icl-u3}:        DMESG-WARN [fdo#107724] -> PASS

  * igt@i915_module_load@reload:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  * igt@kms_flip@basic-flip-vs-dpms:
    - fi-skl-6700hq:      DMESG-WARN [fdo#105998] -> PASS

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

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-ivb-3520m:       FAIL [fdo#103375] -> PASS

  
#### Warnings ####

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7567u:       DMESG-WARN [fdo#103558] / [fdo#105079] / [fdo#105602] -> DMESG-FAIL [fdo#105079]

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

  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#105079]: https://bugs.freedesktop.org/show_bug.cgi?id=105079
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#105998]: https://bugs.freedesktop.org/show_bug.cgi?id=105998
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107709]: https://bugs.freedesktop.org/show_bug.cgi?id=107709
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108094]: https://bugs.freedesktop.org/show_bug.cgi?id=108094
  [fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
  [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271


Participating hosts (50 -> 42)
------------------------------

  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-glk-j4005 fi-gdg-551 fi-icl-y fi-bdw-samus 


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

    * Linux: CI_DRM_5601 -> Patchwork_12220

  CI_DRM_5601: 7977cc73f17770b9b1ed8baff66a8c9fd681d6a8 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4827: 395eaffd7e1390c9d6043c2980dc14ce3e08b154 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12220: b912f44f1bf4d36de451e198a419b937f0637cde @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

b912f44f1bf4 drm/i915: Enable PSR2 by default
727cc543561b drm/i915/psr: Remove PSR2 FIXME
441f8b87c393 drm/i915: Disable PSR2 while getting pipe CRC
e53ec9ed5dc6 drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset

== Logs ==

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

Re: [PATCH 2/4] drm/i915: Disable PSR2 while getting pipe CRC

[Dhinakaran Pandiyan]

On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
> As stated in CRC_CTL spec, after PSR entry state CRC will not be
> calculated anymore what is not a problem as IGT tests do some screen
> change and then request the pipe CRC right after the change so PSR
> will go to idle state and only will entry again after at least 6
> idles frames.
> 
> But for PSR2 it is more problematic as any change to the screen could
> trigger a selective/partial update causing the CRC value not to be
> calculated over the full frame.
Okay, that reasoning runs counter to my understanding. My understanding
is that the whole frame is fetched and processed at the pipe level but
the DDI sends selective blocks of pixels. So, if the CRC's are
calculated at the pipe level, the CRC should be for the full frame with
PSR2 having no effect? Checking bspec, I see there are DDI CRCs as
well, which should reflect the partial frame that PSR2 sends.

To get a better understanding, I'd like to know what the source for
mismatching CRCs is?


> So here it disables PSR2 and keep it disabled while user is
> requesting pipe CRC.
> 
> BSpec: 7536
> 
> 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_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_psr.c      | 23 +++++++++++++++++++++++
>  4 files changed, 35 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 17fe942eaafa..609e9c5bd453 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -520,6 +520,7 @@ struct i915_psr {
>  	bool sink_not_reliable;
>  	bool irq_aux_error;
>  	u16 su_x_granularity;
> +	bool pipe_crc_enabled;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 3398b28c053b..40ce7a600585 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2103,6 +2103,7 @@ void intel_psr_short_pulse(struct intel_dp
> *intel_dp);
>  int intel_psr_wait_for_idle(const struct intel_crtc_state
> *new_crtc_state,
>  			    u32 *out_value);
>  bool intel_psr_enabled(struct intel_dp *intel_dp);
> +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> *dev_priv, enum pipe pipe, bool prepare);
>  
>  /* intel_quirks.c */
>  void intel_init_quirks(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index a8554dc4f196..5d8772399f60 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -583,6 +583,14 @@ int intel_crtc_verify_crc_source(struct drm_crtc
> *crtc, const char *source_name,
>  	return -EINVAL;
>  }
>  
> +static inline void intel_crtc_crc_prepare_or_finish(struct drm_crtc
> *crtc, bool prepare)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +	intel_psr_crc_prepare_or_finish(dev_priv, intel_crtc->pipe,
> prepare);
> +}
> +
>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *source_name)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> @@ -609,6 +617,8 @@ int intel_crtc_set_crc_source(struct drm_crtc
> *crtc, const char *source_name)
>  	if (ret != 0)
>  		goto out;
>  
> +	intel_crtc_crc_prepare_or_finish(crtc, source !=
> INTEL_PIPE_CRC_SOURCE_NONE);
> +
>  	pipe_crc->source = source;
>  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
>  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 08967836b48e..9c93138988aa 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -577,6 +577,9 @@ static bool intel_psr2_config_valid(struct
> intel_dp *intel_dp,
>  		return false;
>  	}
>  
> +	if (dev_priv->psr.pipe_crc_enabled)
> +		return false;
> +
>  	return true;
>  }
>  
> @@ -1291,3 +1294,23 @@ bool intel_psr_enabled(struct intel_dp
> *intel_dp)
>  
>  	return ret;
>  }
> +
> +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> *dev_priv, enum pipe pipe, bool prepare)
> +{
> +	bool fastset = false;
> +
> +	if (!CAN_PSR(dev_priv))
> +		return;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +
> +	if (dev_priv->psr.pipe == pipe) {
> +		dev_priv->psr.pipe_crc_enabled = prepare;
> +		fastset = !prepare || dev_priv->psr.psr2_enabled;
> +	}
> +
> +	mutex_unlock(&dev_priv->psr.lock);
> +
> +	if (fastset)
> +		intel_psr_fastset_force(dev_priv);
> +}

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

Re: [PATCH 2/4] drm/i915: Disable PSR2 while getting pipe CRC

[Souza, Jose]

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

On Thu, 2019-02-14 at 11:00 -0800, Dhinakaran Pandiyan wrote:
> On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
> > As stated in CRC_CTL spec, after PSR entry state CRC will not be
> > calculated anymore what is not a problem as IGT tests do some
> > screen
> > change and then request the pipe CRC right after the change so PSR
> > will go to idle state and only will entry again after at least 6
> > idles frames.
> > 
> > But for PSR2 it is more problematic as any change to the screen
> > could
> > trigger a selective/partial update causing the CRC value not to be
> > calculated over the full frame.
> Okay, that reasoning runs counter to my understanding. My
> understanding
> is that the whole frame is fetched and processed at the pipe level
> but
> the DDI sends selective blocks of pixels. So, if the CRC's are
> calculated at the pipe level, the CRC should be for the full frame
> with
> PSR2 having no effect? Checking bspec, I see there are DDI CRCs as
> well, which should reflect the partial frame that PSR2 sends.
> 
> To get a better understanding, I'd like to know what the source for
> mismatching CRCs is?

Well the naming is misleading for newer gens, before BDW this was a
pipe register(8067) but now it is a DDI register(7536) but the register
offset was kept the same.

In my testing hardware generate 4 interruptions with wrong CRC values
then stops forever probably because vblank interruptions are off.

> 
> 
> > So here it disables PSR2 and keep it disabled while user is
> > requesting pipe CRC.
> > 
> > BSpec: 7536
> > 
> > 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_drv.h      |  1 +
> >  drivers/gpu/drm/i915/intel_pipe_crc.c | 10 ++++++++++
> >  drivers/gpu/drm/i915/intel_psr.c      | 23 +++++++++++++++++++++++
> >  4 files changed, 35 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 17fe942eaafa..609e9c5bd453 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -520,6 +520,7 @@ struct i915_psr {
> >  	bool sink_not_reliable;
> >  	bool irq_aux_error;
> >  	u16 su_x_granularity;
> > +	bool pipe_crc_enabled;
> >  };
> >  
> >  enum intel_pch {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 3398b28c053b..40ce7a600585 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -2103,6 +2103,7 @@ void intel_psr_short_pulse(struct intel_dp
> > *intel_dp);
> >  int intel_psr_wait_for_idle(const struct intel_crtc_state
> > *new_crtc_state,
> >  			    u32 *out_value);
> >  bool intel_psr_enabled(struct intel_dp *intel_dp);
> > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > *dev_priv, enum pipe pipe, bool prepare);
> >  
> >  /* intel_quirks.c */
> >  void intel_init_quirks(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index a8554dc4f196..5d8772399f60 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -583,6 +583,14 @@ int intel_crtc_verify_crc_source(struct
> > drm_crtc
> > *crtc, const char *source_name,
> >  	return -EINVAL;
> >  }
> >  
> > +static inline void intel_crtc_crc_prepare_or_finish(struct
> > drm_crtc
> > *crtc, bool prepare)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +
> > +	intel_psr_crc_prepare_or_finish(dev_priv, intel_crtc->pipe,
> > prepare);
> > +}
> > +
> >  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char
> > *source_name)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > @@ -609,6 +617,8 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > *crtc, const char *source_name)
> >  	if (ret != 0)
> >  		goto out;
> >  
> > +	intel_crtc_crc_prepare_or_finish(crtc, source !=
> > INTEL_PIPE_CRC_SOURCE_NONE);
> > +
> >  	pipe_crc->source = source;
> >  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 08967836b48e..9c93138988aa 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -577,6 +577,9 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> >  		return false;
> >  	}
> >  
> > +	if (dev_priv->psr.pipe_crc_enabled)
> > +		return false;
> > +
> >  	return true;
> >  }
> >  
> > @@ -1291,3 +1294,23 @@ bool intel_psr_enabled(struct intel_dp
> > *intel_dp)
> >  
> >  	return ret;
> >  }
> > +
> > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > *dev_priv, enum pipe pipe, bool prepare)
> > +{
> > +	bool fastset = false;
> > +
> > +	if (!CAN_PSR(dev_priv))
> > +		return;
> > +
> > +	mutex_lock(&dev_priv->psr.lock);
> > +
> > +	if (dev_priv->psr.pipe == pipe) {
> > +		dev_priv->psr.pipe_crc_enabled = prepare;
> > +		fastset = !prepare || dev_priv->psr.psr2_enabled;
> > +	}
> > +
> > +	mutex_unlock(&dev_priv->psr.lock);
> > +
> > +	if (fastset)
> > +		intel_psr_fastset_force(dev_priv);
> > +}

[-- 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 2/4] drm/i915: Disable PSR2 while getting pipe CRC

[Pandiyan, Dhinakaran]

On Thu, 2019-02-14 at 15:19 -0800, Souza, Jose wrote:
> On Thu, 2019-02-14 at 11:00 -0800, Dhinakaran Pandiyan wrote:
> > On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
> > > As stated in CRC_CTL spec, after PSR entry state CRC will not be
> > > calculated anymore what is not a problem as IGT tests do some
> > > screen
> > > change and then request the pipe CRC right after the change so
> > > PSR
> > > will go to idle state and only will entry again after at least 6
> > > idles frames.
> > > 
> > > But for PSR2 it is more problematic as any change to the screen
> > > could
> > > trigger a selective/partial update causing the CRC value not to
> > > be
> > > calculated over the full frame.
> > 
> > Okay, that reasoning runs counter to my understanding. My
> > understanding
> > is that the whole frame is fetched and processed at the pipe level
> > but
> > the DDI sends selective blocks of pixels. So, if the CRC's are
> > calculated at the pipe level, the CRC should be for the full frame
> > with
> > PSR2 having no effect? Checking bspec, I see there are DDI CRCs as
> > well, which should reflect the partial frame that PSR2 sends.
> > 
> > To get a better understanding, I'd like to know what the source for
> > mismatching CRCs is?
> 
> Well the naming is misleading for newer gens, before BDW this was a
> pipe register(8067) but now it is a DDI register(7536) but the
> register
> offset was kept the same.
Do we even read this register DDI_CRC_CTL? Don't see it defined in
i915_reg.h or referenced in intel_pipe_crc.c.
> 
> In my testing hardware generate 4 interruptions with wrong CRC values
> then stops forever probably because vblank interruptions are off.
Yeah, my guess is that power management is affecting the CRC generation
rather than pipe CRC's being calculated only on the SU update blocks.


> > 
> > 
> > > So here it disables PSR2 and keep it disabled while user is
> > > requesting pipe CRC.
> > > 
> > > BSpec: 7536
> > > 
> > > 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_drv.h      |  1 +
> > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 10 ++++++++++
> > >  drivers/gpu/drm/i915/intel_psr.c      | 23
> > > +++++++++++++++++++++++
> > >  4 files changed, 35 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 17fe942eaafa..609e9c5bd453 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -520,6 +520,7 @@ struct i915_psr {
> > >  	bool sink_not_reliable;
> > >  	bool irq_aux_error;
> > >  	u16 su_x_granularity;
> > > +	bool pipe_crc_enabled;
> > >  };
> > >  
> > >  enum intel_pch {
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 3398b28c053b..40ce7a600585 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -2103,6 +2103,7 @@ void intel_psr_short_pulse(struct intel_dp
> > > *intel_dp);
> > >  int intel_psr_wait_for_idle(const struct intel_crtc_state
> > > *new_crtc_state,
> > >  			    u32 *out_value);
> > >  bool intel_psr_enabled(struct intel_dp *intel_dp);
> > > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > > *dev_priv, enum pipe pipe, bool prepare);
> > >  
> > >  /* intel_quirks.c */
> > >  void intel_init_quirks(struct drm_i915_private *dev_priv);
> > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > index a8554dc4f196..5d8772399f60 100644
> > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > @@ -583,6 +583,14 @@ int intel_crtc_verify_crc_source(struct
> > > drm_crtc
> > > *crtc, const char *source_name,
> > >  	return -EINVAL;
> > >  }
> > >  
> > > +static inline void intel_crtc_crc_prepare_or_finish(struct
> > > drm_crtc
> > > *crtc, bool prepare)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +
> > > +	intel_psr_crc_prepare_or_finish(dev_priv, intel_crtc->pipe,
> > > prepare);
> > > +}
> > > +
> > >  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char
> > > *source_name)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > > @@ -609,6 +617,8 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > > *crtc, const char *source_name)
> > >  	if (ret != 0)
> > >  		goto out;
> > >  
> > > +	intel_crtc_crc_prepare_or_finish(crtc, source !=
> > > INTEL_PIPE_CRC_SOURCE_NONE);
> > > +
> > >  	pipe_crc->source = source;
> > >  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> > >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 08967836b48e..9c93138988aa 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -577,6 +577,9 @@ static bool intel_psr2_config_valid(struct
> > > intel_dp *intel_dp,
> > >  		return false;
> > >  	}
> > >  
> > > +	if (dev_priv->psr.pipe_crc_enabled)
> > > +		return false;
> > > +
> > >  	return true;
> > >  }
> > >  
> > > @@ -1291,3 +1294,23 @@ bool intel_psr_enabled(struct intel_dp
> > > *intel_dp)
> > >  
> > >  	return ret;
> > >  }
> > > +
> > > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > > *dev_priv, enum pipe pipe, bool prepare)
> > > +{
> > > +	bool fastset = false;
> > > +
> > > +	if (!CAN_PSR(dev_priv))
> > > +		return;
> > > +
> > > +	mutex_lock(&dev_priv->psr.lock);
> > > +
> > > +	if (dev_priv->psr.pipe == pipe) {
> > > +		dev_priv->psr.pipe_crc_enabled = prepare;
> > > +		fastset = !prepare || dev_priv->psr.psr2_enabled;
> > > +	}
> > > +
> > > +	mutex_unlock(&dev_priv->psr.lock);
> > > +
> > > +	if (fastset)
> > > +		intel_psr_fastset_force(dev_priv);
> > > +}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Disable PSR2 while getting pipe CRC

[Souza, Jose]

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

On Thu, 2019-02-14 at 16:10 -0800, Pandiyan, Dhinakaran wrote:
> On Thu, 2019-02-14 at 15:19 -0800, Souza, Jose wrote:
> > On Thu, 2019-02-14 at 11:00 -0800, Dhinakaran Pandiyan wrote:
> > > On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
> > > > As stated in CRC_CTL spec, after PSR entry state CRC will not
> > > > be
> > > > calculated anymore what is not a problem as IGT tests do some
> > > > screen
> > > > change and then request the pipe CRC right after the change so
> > > > PSR
> > > > will go to idle state and only will entry again after at least
> > > > 6
> > > > idles frames.
> > > > 
> > > > But for PSR2 it is more problematic as any change to the screen
> > > > could
> > > > trigger a selective/partial update causing the CRC value not to
> > > > be
> > > > calculated over the full frame.
> > > 
> > > Okay, that reasoning runs counter to my understanding. My
> > > understanding
> > > is that the whole frame is fetched and processed at the pipe
> > > level
> > > but
> > > the DDI sends selective blocks of pixels. So, if the CRC's are
> > > calculated at the pipe level, the CRC should be for the full
> > > frame
> > > with
> > > PSR2 having no effect? Checking bspec, I see there are DDI CRCs
> > > as
> > > well, which should reflect the partial frame that PSR2 sends.
> > > 
> > > To get a better understanding, I'd like to know what the source
> > > for
> > > mismatching CRCs is?
> > 
> > Well the naming is misleading for newer gens, before BDW this was a
> > pipe register(8067) but now it is a DDI register(7536) but the
> > register
> > offset was kept the same.
> Do we even read this register DDI_CRC_CTL? Don't see it defined in
> i915_reg.h or referenced in intel_pipe_crc.c.

We do: PIPE_CRC_CTL().
#define _PIPE_CRC_CTL_A			0x60050

> > In my testing hardware generate 4 interruptions with wrong CRC
> > values
> > then stops forever probably because vblank interruptions are off.
> Yeah, my guess is that power management is affecting the CRC
> generation
> rather than pipe CRC's being calculated only on the SU update blocks.
> 
> 
> > > 
> > > > So here it disables PSR2 and keep it disabled while user is
> > > > requesting pipe CRC.
> > > > 
> > > > BSpec: 7536
> > > > 
> > > > 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_drv.h      |  1 +
> > > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 10 ++++++++++
> > > >  drivers/gpu/drm/i915/intel_psr.c      | 23
> > > > +++++++++++++++++++++++
> > > >  4 files changed, 35 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 17fe942eaafa..609e9c5bd453 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -520,6 +520,7 @@ struct i915_psr {
> > > >  	bool sink_not_reliable;
> > > >  	bool irq_aux_error;
> > > >  	u16 su_x_granularity;
> > > > +	bool pipe_crc_enabled;
> > > >  };
> > > >  
> > > >  enum intel_pch {
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 3398b28c053b..40ce7a600585 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -2103,6 +2103,7 @@ void intel_psr_short_pulse(struct
> > > > intel_dp
> > > > *intel_dp);
> > > >  int intel_psr_wait_for_idle(const struct intel_crtc_state
> > > > *new_crtc_state,
> > > >  			    u32 *out_value);
> > > >  bool intel_psr_enabled(struct intel_dp *intel_dp);
> > > > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > > > *dev_priv, enum pipe pipe, bool prepare);
> > > >  
> > > >  /* intel_quirks.c */
> > > >  void intel_init_quirks(struct drm_i915_private *dev_priv);
> > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > index a8554dc4f196..5d8772399f60 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > @@ -583,6 +583,14 @@ int intel_crtc_verify_crc_source(struct
> > > > drm_crtc
> > > > *crtc, const char *source_name,
> > > >  	return -EINVAL;
> > > >  }
> > > >  
> > > > +static inline void intel_crtc_crc_prepare_or_finish(struct
> > > > drm_crtc
> > > > *crtc, bool prepare)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > +
> > > > +	intel_psr_crc_prepare_or_finish(dev_priv, intel_crtc-
> > > > >pipe,
> > > > prepare);
> > > > +}
> > > > +
> > > >  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const
> > > > char
> > > > *source_name)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > > > @@ -609,6 +617,8 @@ int intel_crtc_set_crc_source(struct
> > > > drm_crtc
> > > > *crtc, const char *source_name)
> > > >  	if (ret != 0)
> > > >  		goto out;
> > > >  
> > > > +	intel_crtc_crc_prepare_or_finish(crtc, source !=
> > > > INTEL_PIPE_CRC_SOURCE_NONE);
> > > > +
> > > >  	pipe_crc->source = source;
> > > >  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> > > >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 08967836b48e..9c93138988aa 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -577,6 +577,9 @@ static bool intel_psr2_config_valid(struct
> > > > intel_dp *intel_dp,
> > > >  		return false;
> > > >  	}
> > > >  
> > > > +	if (dev_priv->psr.pipe_crc_enabled)
> > > > +		return false;
> > > > +
> > > >  	return true;
> > > >  }
> > > >  
> > > > @@ -1291,3 +1294,23 @@ bool intel_psr_enabled(struct intel_dp
> > > > *intel_dp)
> > > >  
> > > >  	return ret;
> > > >  }
> > > > +
> > > > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > > > *dev_priv, enum pipe pipe, bool prepare)
> > > > +{
> > > > +	bool fastset = false;
> > > > +
> > > > +	if (!CAN_PSR(dev_priv))
> > > > +		return;
> > > > +
> > > > +	mutex_lock(&dev_priv->psr.lock);
> > > > +
> > > > +	if (dev_priv->psr.pipe == pipe) {
> > > > +		dev_priv->psr.pipe_crc_enabled = prepare;
> > > > +		fastset = !prepare || dev_priv-
> > > > >psr.psr2_enabled;
> > > > +	}
> > > > +
> > > > +	mutex_unlock(&dev_priv->psr.lock);
> > > > +
> > > > +	if (fastset)
> > > > +		intel_psr_fastset_force(dev_priv);
> > > > +}

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

✗ Fi.CI.IGT: failure for series starting with [1/4] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset
URL   : https://patchwork.freedesktop.org/series/56647/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5601_full -> Patchwork_12220_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12220_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12220_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_12220_full:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_vblank@pipe-b-ts-continuation-dpms-rpm:
    - shard-apl:          PASS -> FAIL

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * {igt@kms_psr2_su@frontbuffer}:
    - shard-iclb:         NOTRUN -> {SKIP}

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

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

### IGT changes ###

#### Issues hit ####

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

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

  * igt@kms_atomic_transition@plane-all-modeset-transition-fencing:
    - shard-apl:          PASS -> INCOMPLETE [fdo#103927] / [fdo#109225]

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

  * igt@kms_ccs@pipe-b-crc-primary-basic:
    - shard-iclb:         NOTRUN -> FAIL [fdo#107725]

  * igt@kms_color@pipe-b-ctm-0-75:
    - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#109624]

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

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

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

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

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

  * igt@kms_plane@pixel-format-pipe-b-planes-source-clamping:
    - shard-glk:          PASS -> FAIL [fdo#108948]

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

  * igt@kms_plane_alpha_blend@pipe-c-alpha-7efc:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145] / [fdo#108590]

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

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

  * igt@kms_psr@no_drrs:
    - shard-iclb:         PASS -> FAIL [fdo#108341]

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

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

  * igt@kms_sysfs_edid_timing:
    - shard-kbl:          NOTRUN -> FAIL [fdo#100047]

  * igt@kms_vblank@pipe-b-ts-continuation-suspend:
    - shard-apl:          PASS -> DMESG-WARN [fdo#108566]

  * igt@pm_rpm@cursor:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#107724] +1

  * igt@pm_rpm@legacy-planes:
    - shard-iclb:         PASS -> INCOMPLETE [fdo#108840] / [fdo#109369]

  
#### Possible fixes ####

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

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

  * igt@kms_cursor_crc@cursor-128x128-suspend:
    - shard-kbl:          DMESG-WARN [fdo#108566] -> PASS

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

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

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

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

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-fullscreen:
    - shard-iclb:         FAIL [fdo#103167] -> PASS

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

  * igt@kms_universal_plane@universal-plane-pipe-a-functional:
    - shard-apl:          FAIL [fdo#103166] -> PASS

  * igt@pm_backlight@basic-brightness:
    - shard-iclb:         INCOMPLETE [fdo#107820] -> PASS

  * igt@pm_rpm@debugfs-read:
    - shard-iclb:         DMESG-WARN [fdo#107724] -> PASS +4

  * igt@pm_rpm@universal-planes:
    - shard-iclb:         DMESG-WARN [fdo#108654] / [fdo#108756] -> PASS

  
#### Warnings ####

  * igt@kms_cursor_crc@cursor-256x256-suspend:
    - shard-glk:          FAIL [fdo#103232] -> INCOMPLETE [fdo#103359] / [k.org#198133]

  * igt@pm_backlight@fade_with_suspend:
    - shard-iclb:         DMESG-FAIL [fdo#107724] / [fdo#107847] -> FAIL [fdo#107847]

  
  {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#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#107725]: https://bugs.freedesktop.org/show_bug.cgi?id=107725
  [fdo#107799]: https://bugs.freedesktop.org/show_bug.cgi?id=107799
  [fdo#107820]: https://bugs.freedesktop.org/show_bug.cgi?id=107820
  [fdo#107847]: https://bugs.freedesktop.org/show_bug.cgi?id=107847
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
  [fdo#108654]: https://bugs.freedesktop.org/show_bug.cgi?id=108654
  [fdo#108756]: https://bugs.freedesktop.org/show_bug.cgi?id=108756
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [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#109225]: https://bugs.freedesktop.org/show_bug.cgi?id=109225
  [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#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109281]: https://bugs.freedesktop.org/show_bug.cgi?id=109281
  [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#109307]: https://bugs.freedesktop.org/show_bug.cgi?id=109307
  [fdo#109308]: https://bugs.freedesktop.org/show_bug.cgi?id=109308
  [fdo#109369]: https://bugs.freedesktop.org/show_bug.cgi?id=109369
  [fdo#109624]: https://bugs.freedesktop.org/show_bug.cgi?id=109624
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


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

  Missing    (1): shard-skl 


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

    * Linux: CI_DRM_5601 -> Patchwork_12220

  CI_DRM_5601: 7977cc73f17770b9b1ed8baff66a8c9fd681d6a8 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4827: 395eaffd7e1390c9d6043c2980dc14ce3e08b154 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12220: b912f44f1bf4d36de451e198a419b937f0637cde @ 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_12220/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Disable PSR2 while getting pipe CRC

[Souza, Jose]

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

On Fri, 2019-02-15 at 00:17 +0000, Souza, Jose wrote:
> On Thu, 2019-02-14 at 16:10 -0800, Pandiyan, Dhinakaran wrote:
> > On Thu, 2019-02-14 at 15:19 -0800, Souza, Jose wrote:
> > > On Thu, 2019-02-14 at 11:00 -0800, Dhinakaran Pandiyan wrote:
> > > > On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
> > > > > As stated in CRC_CTL spec, after PSR entry state CRC will not
> > > > > be
> > > > > calculated anymore what is not a problem as IGT tests do some
> > > > > screen
> > > > > change and then request the pipe CRC right after the change
> > > > > so
> > > > > PSR
> > > > > will go to idle state and only will entry again after at
> > > > > least
> > > > > 6
> > > > > idles frames.
> > > > > 
> > > > > But for PSR2 it is more problematic as any change to the
> > > > > screen
> > > > > could
> > > > > trigger a selective/partial update causing the CRC value not
> > > > > to
> > > > > be
> > > > > calculated over the full frame.
> > > > 
> > > > Okay, that reasoning runs counter to my understanding. My
> > > > understanding
> > > > is that the whole frame is fetched and processed at the pipe
> > > > level
> > > > but
> > > > the DDI sends selective blocks of pixels. So, if the CRC's are
> > > > calculated at the pipe level, the CRC should be for the full
> > > > frame
> > > > with
> > > > PSR2 having no effect? Checking bspec, I see there are DDI CRCs
> > > > as
> > > > well, which should reflect the partial frame that PSR2 sends.
> > > > 
> > > > To get a better understanding, I'd like to know what the source
> > > > for
> > > > mismatching CRCs is?
> > > 
> > > Well the naming is misleading for newer gens, before BDW this was
> > > a
> > > pipe register(8067) but now it is a DDI register(7536) but the
> > > register
> > > offset was kept the same.
> > Do we even read this register DDI_CRC_CTL? Don't see it defined in
> > i915_reg.h or referenced in intel_pipe_crc.c.
> 
> We do: PIPE_CRC_CTL().
> #define _PIPE_CRC_CTL_A			0x60050

I was wrong, the first CRC generated is wrong but it is not read by
userspace as we skip it using pipe_crc->skipped it is docummented in
the code too.

The problem here is that after PSR2 is activated(after 'Frames Before
SU Entry') there is no more CRC interruptions so no calls to
drm_crtc_add_crc_entry(), what don't happen with PSR1 even setting a
'Idle Frames' to 1.

Other odd thing that I discovered it that we have CRC_CTL register in
DDI(Bspec 7536) and other in PIPE(Bspec 7646) with diferent bits and we
write in the transcoder offset using the pipe bits. I tried to use the
PIPE offset but it do not generate any interruption, anyone know more
about that?

> 
> > > In my testing hardware generate 4 interruptions with wrong CRC
> > > values
> > > then stops forever probably because vblank interruptions are off.
> > Yeah, my guess is that power management is affecting the CRC
> > generation
> > rather than pipe CRC's being calculated only on the SU update
> > blocks.
> > 
> > 
> > > > > So here it disables PSR2 and keep it disabled while user is
> > > > > requesting pipe CRC.
> > > > > 
> > > > > BSpec: 7536
> > > > > 
> > > > > 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_drv.h      |  1 +
> > > > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 10 ++++++++++
> > > > >  drivers/gpu/drm/i915/intel_psr.c      | 23
> > > > > +++++++++++++++++++++++
> > > > >  4 files changed, 35 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 17fe942eaafa..609e9c5bd453 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -520,6 +520,7 @@ struct i915_psr {
> > > > >  	bool sink_not_reliable;
> > > > >  	bool irq_aux_error;
> > > > >  	u16 su_x_granularity;
> > > > > +	bool pipe_crc_enabled;
> > > > >  };
> > > > >  
> > > > >  enum intel_pch {
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 3398b28c053b..40ce7a600585 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -2103,6 +2103,7 @@ void intel_psr_short_pulse(struct
> > > > > intel_dp
> > > > > *intel_dp);
> > > > >  int intel_psr_wait_for_idle(const struct intel_crtc_state
> > > > > *new_crtc_state,
> > > > >  			    u32 *out_value);
> > > > >  bool intel_psr_enabled(struct intel_dp *intel_dp);
> > > > > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > > > > *dev_priv, enum pipe pipe, bool prepare);
> > > > >  
> > > > >  /* intel_quirks.c */
> > > > >  void intel_init_quirks(struct drm_i915_private *dev_priv);
> > > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > index a8554dc4f196..5d8772399f60 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > @@ -583,6 +583,14 @@ int intel_crtc_verify_crc_source(struct
> > > > > drm_crtc
> > > > > *crtc, const char *source_name,
> > > > >  	return -EINVAL;
> > > > >  }
> > > > >  
> > > > > +static inline void intel_crtc_crc_prepare_or_finish(struct
> > > > > drm_crtc
> > > > > *crtc, bool prepare)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > > > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > > +
> > > > > +	intel_psr_crc_prepare_or_finish(dev_priv, intel_crtc-
> > > > > > pipe,
> > > > > prepare);
> > > > > +}
> > > > > +
> > > > >  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const
> > > > > char
> > > > > *source_name)
> > > > >  {
> > > > >  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > > > > @@ -609,6 +617,8 @@ int intel_crtc_set_crc_source(struct
> > > > > drm_crtc
> > > > > *crtc, const char *source_name)
> > > > >  	if (ret != 0)
> > > > >  		goto out;
> > > > >  
> > > > > +	intel_crtc_crc_prepare_or_finish(crtc, source !=
> > > > > INTEL_PIPE_CRC_SOURCE_NONE);
> > > > > +
> > > > >  	pipe_crc->source = source;
> > > > >  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> > > > >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > index 08967836b48e..9c93138988aa 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -577,6 +577,9 @@ static bool
> > > > > intel_psr2_config_valid(struct
> > > > > intel_dp *intel_dp,
> > > > >  		return false;
> > > > >  	}
> > > > >  
> > > > > +	if (dev_priv->psr.pipe_crc_enabled)
> > > > > +		return false;
> > > > > +
> > > > >  	return true;
> > > > >  }
> > > > >  
> > > > > @@ -1291,3 +1294,23 @@ bool intel_psr_enabled(struct intel_dp
> > > > > *intel_dp)
> > > > >  
> > > > >  	return ret;
> > > > >  }
> > > > > +
> > > > > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > > > > *dev_priv, enum pipe pipe, bool prepare)
> > > > > +{
> > > > > +	bool fastset = false;
> > > > > +
> > > > > +	if (!CAN_PSR(dev_priv))
> > > > > +		return;
> > > > > +
> > > > > +	mutex_lock(&dev_priv->psr.lock);
> > > > > +
> > > > > +	if (dev_priv->psr.pipe == pipe) {
> > > > > +		dev_priv->psr.pipe_crc_enabled = prepare;
> > > > > +		fastset = !prepare || dev_priv-
> > > > > > psr.psr2_enabled;
> > > > > +	}
> > > > > +
> > > > > +	mutex_unlock(&dev_priv->psr.lock);
> > > > > +
> > > > > +	if (fastset)
> > > > > +		intel_psr_fastset_force(dev_priv);
> > > > > +}
> 
> _______________________________________________
> 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 2/4] drm/i915: Disable PSR2 while getting pipe CRC

[Souza, Jose]

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

On Fri, 2019-02-15 at 16:20 -0800, José Roberto de Souza wrote:
> On Fri, 2019-02-15 at 00:17 +0000, Souza, Jose wrote:
> > On Thu, 2019-02-14 at 16:10 -0800, Pandiyan, Dhinakaran wrote:
> > > On Thu, 2019-02-14 at 15:19 -0800, Souza, Jose wrote:
> > > > On Thu, 2019-02-14 at 11:00 -0800, Dhinakaran Pandiyan wrote:
> > > > > On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza
> > > > > wrote:
> > > > > > As stated in CRC_CTL spec, after PSR entry state CRC will
> > > > > > not
> > > > > > be
> > > > > > calculated anymore what is not a problem as IGT tests do
> > > > > > some
> > > > > > screen
> > > > > > change and then request the pipe CRC right after the change
> > > > > > so
> > > > > > PSR
> > > > > > will go to idle state and only will entry again after at
> > > > > > least
> > > > > > 6
> > > > > > idles frames.
> > > > > > 
> > > > > > But for PSR2 it is more problematic as any change to the
> > > > > > screen
> > > > > > could
> > > > > > trigger a selective/partial update causing the CRC value
> > > > > > not
> > > > > > to
> > > > > > be
> > > > > > calculated over the full frame.
> > > > > 
> > > > > Okay, that reasoning runs counter to my understanding. My
> > > > > understanding
> > > > > is that the whole frame is fetched and processed at the pipe
> > > > > level
> > > > > but
> > > > > the DDI sends selective blocks of pixels. So, if the CRC's
> > > > > are
> > > > > calculated at the pipe level, the CRC should be for the full
> > > > > frame
> > > > > with
> > > > > PSR2 having no effect? Checking bspec, I see there are DDI
> > > > > CRCs
> > > > > as
> > > > > well, which should reflect the partial frame that PSR2 sends.
> > > > > 
> > > > > To get a better understanding, I'd like to know what the
> > > > > source
> > > > > for
> > > > > mismatching CRCs is?
> > > > 
> > > > Well the naming is misleading for newer gens, before BDW this
> > > > was
> > > > a
> > > > pipe register(8067) but now it is a DDI register(7536) but the
> > > > register
> > > > offset was kept the same.
> > > Do we even read this register DDI_CRC_CTL? Don't see it defined
> > > in
> > > i915_reg.h or referenced in intel_pipe_crc.c.
> > 
> > We do: PIPE_CRC_CTL().
> > #define _PIPE_CRC_CTL_A			0x60050
> 
> I was wrong, the first CRC generated is wrong but it is not read by
> userspace as we skip it using pipe_crc->skipped it is docummented in
> the code too.
> 
> The problem here is that after PSR2 is activated(after 'Frames Before
> SU Entry') there is no more CRC interruptions so no calls to
> drm_crtc_add_crc_entry(), what don't happen with PSR1 even setting a
> 'Idle Frames' to 1.

So I will update the commit description to:

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.

Oddly that don't happen when PSR1 active, so here it switches from
PSR2 to PSR1 while user is requesting pipe CRC.

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


> 
> Other odd thing that I discovered it that we have CRC_CTL register in
> DDI(Bspec 7536) and other in PIPE(Bspec 7646) with diferent bits and
> we
> write in the transcoder offset using the pipe bits. I tried to use
> the
> PIPE offset but it do not generate any interruption, anyone know more
> about that?
> 
> > > > In my testing hardware generate 4 interruptions with wrong CRC
> > > > values
> > > > then stops forever probably because vblank interruptions are
> > > > off.
> > > Yeah, my guess is that power management is affecting the CRC
> > > generation
> > > rather than pipe CRC's being calculated only on the SU update
> > > blocks.
> > > 
> > > 
> > > > > > So here it disables PSR2 and keep it disabled while user is
> > > > > > requesting pipe CRC.
> > > > > > 
> > > > > > BSpec: 7536
> > > > > > 
> > > > > > 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_drv.h      |  1 +
> > > > > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 10 ++++++++++
> > > > > >  drivers/gpu/drm/i915/intel_psr.c      | 23
> > > > > > +++++++++++++++++++++++
> > > > > >  4 files changed, 35 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > index 17fe942eaafa..609e9c5bd453 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > @@ -520,6 +520,7 @@ struct i915_psr {
> > > > > >  	bool sink_not_reliable;
> > > > > >  	bool irq_aux_error;
> > > > > >  	u16 su_x_granularity;
> > > > > > +	bool pipe_crc_enabled;
> > > > > >  };
> > > > > >  
> > > > > >  enum intel_pch {
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > index 3398b28c053b..40ce7a600585 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > @@ -2103,6 +2103,7 @@ void intel_psr_short_pulse(struct
> > > > > > intel_dp
> > > > > > *intel_dp);
> > > > > >  int intel_psr_wait_for_idle(const struct intel_crtc_state
> > > > > > *new_crtc_state,
> > > > > >  			    u32 *out_value);
> > > > > >  bool intel_psr_enabled(struct intel_dp *intel_dp);
> > > > > > +void intel_psr_crc_prepare_or_finish(struct
> > > > > > drm_i915_private
> > > > > > *dev_priv, enum pipe pipe, bool prepare);
> > > > > >  
> > > > > >  /* intel_quirks.c */
> > > > > >  void intel_init_quirks(struct drm_i915_private *dev_priv);
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > index a8554dc4f196..5d8772399f60 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > @@ -583,6 +583,14 @@ int
> > > > > > intel_crtc_verify_crc_source(struct
> > > > > > drm_crtc
> > > > > > *crtc, const char *source_name,
> > > > > >  	return -EINVAL;
> > > > > >  }
> > > > > >  
> > > > > > +static inline void intel_crtc_crc_prepare_or_finish(struct
> > > > > > drm_crtc
> > > > > > *crtc, bool prepare)
> > > > > > +{
> > > > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > > > > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > > > +
> > > > > > +	intel_psr_crc_prepare_or_finish(dev_priv, intel_crtc-
> > > > > > > pipe,
> > > > > > prepare);
> > > > > > +}
> > > > > > +
> > > > > >  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const
> > > > > > char
> > > > > > *source_name)
> > > > > >  {
> > > > > >  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > > > > > @@ -609,6 +617,8 @@ int intel_crtc_set_crc_source(struct
> > > > > > drm_crtc
> > > > > > *crtc, const char *source_name)
> > > > > >  	if (ret != 0)
> > > > > >  		goto out;
> > > > > >  
> > > > > > +	intel_crtc_crc_prepare_or_finish(crtc, source !=
> > > > > > INTEL_PIPE_CRC_SOURCE_NONE);
> > > > > > +
> > > > > >  	pipe_crc->source = source;
> > > > > >  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> > > > > >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > index 08967836b48e..9c93138988aa 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > @@ -577,6 +577,9 @@ static bool
> > > > > > intel_psr2_config_valid(struct
> > > > > > intel_dp *intel_dp,
> > > > > >  		return false;
> > > > > >  	}
> > > > > >  
> > > > > > +	if (dev_priv->psr.pipe_crc_enabled)
> > > > > > +		return false;
> > > > > > +
> > > > > >  	return true;
> > > > > >  }
> > > > > >  
> > > > > > @@ -1291,3 +1294,23 @@ bool intel_psr_enabled(struct
> > > > > > intel_dp
> > > > > > *intel_dp)
> > > > > >  
> > > > > >  	return ret;
> > > > > >  }
> > > > > > +
> > > > > > +void intel_psr_crc_prepare_or_finish(struct
> > > > > > drm_i915_private
> > > > > > *dev_priv, enum pipe pipe, bool prepare)
> > > > > > +{
> > > > > > +	bool fastset = false;
> > > > > > +
> > > > > > +	if (!CAN_PSR(dev_priv))
> > > > > > +		return;
> > > > > > +
> > > > > > +	mutex_lock(&dev_priv->psr.lock);
> > > > > > +
> > > > > > +	if (dev_priv->psr.pipe == pipe) {
> > > > > > +		dev_priv->psr.pipe_crc_enabled = prepare;
> > > > > > +		fastset = !prepare || dev_priv-
> > > > > > > psr.psr2_enabled;
> > > > > > +	}
> > > > > > +
> > > > > > +	mutex_unlock(&dev_priv->psr.lock);
> > > > > > +
> > > > > > +	if (fastset)
> > > > > > +		intel_psr_fastset_force(dev_priv);
> > > > > > +}
> > 
> > _______________________________________________
> > 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 1/4] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset

[Pandiyan, Dhinakaran]

On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
> 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.

Is there an IGT that's failing because of this? Looks like the state
would have got cleared only if the crtc was enabled to drive another
encoder.

> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.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, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 75c1a5deebf5..08967836b48e 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -986,7 +986,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->enable &&
> +		    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;
I was wondering if we should remove the 'break;' that follows instead
and let ->update_pipe take care of the rest. However, checking
crtc_state->enable makes sense as there is not much point in triggering
a fastset on a disabled crtc.

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

There might be a better way to do this, so please double check with
someone :)

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

Re: [PATCH 1/4] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset

[Souza, Jose]

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

On Fri, 2019-02-22 at 16:27 -0800, Pandiyan, Dhinakaran wrote:
> On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
> > 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.
> 
> Is there an IGT that's failing because of this? Looks like the state
> would have got cleared only if the crtc was enabled to drive another
> encoder.

When PSR2 is enabled by default tests like 
kms_pipe_crc_basic@read-crc-pipe-b are failling even with the patch
that disable PSR2 when getting CRC.

> 
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.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, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 75c1a5deebf5..08967836b48e 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -986,7 +986,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->enable &&
> > +		    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;
> I was wondering if we should remove the 'break;' that follows instead
> and let ->update_pipe take care of the rest. However, checking
> crtc_state->enable makes sense as there is not much point in
> triggering
> a fastset on a disabled crtc.
> 
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> 
> 
> There might be a better way to do this, so please double check with
> someone :)
> 

[-- 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 1/4] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset

[Dhinakaran Pandiyan]

On Fri, 2019-02-22 at 16:33 -0800, Souza, Jose wrote:
> On Fri, 2019-02-22 at 16:27 -0800, Pandiyan, Dhinakaran wrote:
> > On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
> > > 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.
> > 
> > Is there an IGT that's failing because of this? Looks like the
> > state
> > would have got cleared only if the crtc was enabled to drive
> > another
> > encoder.
> 
> When PSR2 is enabled by default tests like 
> kms_pipe_crc_basic@read-crc-pipe-b are failling even with the patch
> that disable PSR2 when getting CRC.

Thanks!

> 
> > 
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.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, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 75c1a5deebf5..08967836b48e 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -986,7 +986,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->enable &&
> > > +		    intel_crtc_has_type(intel_crtc_state,
> > > INTEL_OUTPUT_EDP) &&
> > >  		    intel_crtc_state->has_psr) {


if (crtc_state->enable && intel_crtc_state->has_psr)
	should cover all the cases, no?

And also add a WARN() in case we somehow end up with more than once
crtc with the above condition being true.

> > >  			/* Mark mode as changed to trigger a pipe-
> > > > update() */
> > > 
> > >  			crtc_state->mode_changed = true;
> > 
> > I was wondering if we should remove the 'break;' that follows
> > instead
> > and let ->update_pipe take care of the rest. However, checking
> > crtc_state->enable makes sense as there is not much point in
> > triggering
> > a fastset on a disabled crtc.
> > 
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> 
> > 
> > There might be a better way to do this, so please double check with
> > someone :)
> > 

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

Re: [PATCH 2/4] drm/i915: Disable PSR2 while getting pipe CRC

[Dhinakaran Pandiyan]

On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
> As stated in CRC_CTL spec, after PSR entry state CRC will not be
> calculated anymore what is not a problem as IGT tests do some screen
> change and then request the pipe CRC right after the change so PSR
> will go to idle state and only will entry again after at least 6
> idles frames.
> 
> But for PSR2 it is more problematic as any change to the screen could
> trigger a selective/partial update causing the CRC value not to be
> calculated over the full frame.
> 
> So here it disables PSR2 and keep it disabled while user is
> requesting pipe CRC.
> 
> BSpec: 7536
> 
> 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_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_psr.c      | 23 +++++++++++++++++++++++
>  4 files changed, 35 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 17fe942eaafa..609e9c5bd453 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -520,6 +520,7 @@ struct i915_psr {
>  	bool sink_not_reliable;
>  	bool irq_aux_error;
>  	u16 su_x_granularity;
> +	bool pipe_crc_enabled;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 3398b28c053b..40ce7a600585 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2103,6 +2103,7 @@ void intel_psr_short_pulse(struct intel_dp
> *intel_dp);
>  int intel_psr_wait_for_idle(const struct intel_crtc_state
> *new_crtc_state,
>  			    u32 *out_value);
>  bool intel_psr_enabled(struct intel_dp *intel_dp);
> +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> *dev_priv, enum pipe pipe, bool prepare);
>  
>  /* intel_quirks.c */
>  void intel_init_quirks(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index a8554dc4f196..5d8772399f60 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -583,6 +583,14 @@ int intel_crtc_verify_crc_source(struct drm_crtc
> *crtc, const char *source_name,
>  	return -EINVAL;
>  }
>  
> +static inline void intel_crtc_crc_prepare_or_finish(struct drm_crtc
> *crtc, bool prepare)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +	intel_psr_crc_prepare_or_finish(dev_priv, intel_crtc->pipe,
> prepare);
> +}
> +
>  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *source_name)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> @@ -609,6 +617,8 @@ int intel_crtc_set_crc_source(struct drm_crtc
> *crtc, const char *source_name)
>  	if (ret != 0)
>  		goto out;
>  
> +	intel_crtc_crc_prepare_or_finish(crtc, source !=
> INTEL_PIPE_CRC_SOURCE_NONE);
> +
>  	pipe_crc->source = source;
>  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
>  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 08967836b48e..9c93138988aa 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -577,6 +577,9 @@ static bool intel_psr2_config_valid(struct
> intel_dp *intel_dp,
>  		return false;
>  	}
>  
> +	if (dev_priv->psr.pipe_crc_enabled)
> +		return false;
> +

Disabling PSR instead of switching to PSR1 is safer considering the
past bug reports with PSR1.
 
>  	return true;
>  }
>  
> @@ -1291,3 +1294,23 @@ bool intel_psr_enabled(struct intel_dp
> *intel_dp)
>  
>  	return ret;
>  }
> +
> +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> *dev_priv, enum pipe pipe, bool prepare)
> +{
> +	bool fastset = false;
> +
> +	if (!CAN_PSR(dev_priv))
> +		return;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +
> +	if (dev_priv->psr.pipe == pipe) {
> +		dev_priv->psr.pipe_crc_enabled = prepare;

.crc_enabled seems like it belongs in crtc_state rather than in the
global atomic state.

Looks like we could rename and re-purpose crtc_state.ips_force_disable
for this. I don't see that flag being used for anything other working
around CRC issues.


> +		fastset = !prepare || dev_priv->psr.psr2_enabled;
> +	}
> +
> +	mutex_unlock(&dev_priv->psr.lock);
> +
> +	if (fastset)
> +		intel_psr_fastset_force(dev_priv);
> +}

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

Re: [PATCH 2/4] drm/i915: Disable PSR2 while getting pipe CRC

[Souza, Jose]

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

On Fri, 2019-02-22 at 18:13 -0800, Dhinakaran Pandiyan wrote:
> On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
> > As stated in CRC_CTL spec, after PSR entry state CRC will not be
> > calculated anymore what is not a problem as IGT tests do some
> > screen
> > change and then request the pipe CRC right after the change so PSR
> > will go to idle state and only will entry again after at least 6
> > idles frames.
> > 
> > But for PSR2 it is more problematic as any change to the screen
> > could
> > trigger a selective/partial update causing the CRC value not to be
> > calculated over the full frame.
> > 
> > So here it disables PSR2 and keep it disabled while user is
> > requesting pipe CRC.
> > 
> > BSpec: 7536
> > 
> > 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_drv.h      |  1 +
> >  drivers/gpu/drm/i915/intel_pipe_crc.c | 10 ++++++++++
> >  drivers/gpu/drm/i915/intel_psr.c      | 23 +++++++++++++++++++++++
> >  4 files changed, 35 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 17fe942eaafa..609e9c5bd453 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -520,6 +520,7 @@ struct i915_psr {
> >  	bool sink_not_reliable;
> >  	bool irq_aux_error;
> >  	u16 su_x_granularity;
> > +	bool pipe_crc_enabled;
> >  };
> >  
> >  enum intel_pch {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 3398b28c053b..40ce7a600585 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -2103,6 +2103,7 @@ void intel_psr_short_pulse(struct intel_dp
> > *intel_dp);
> >  int intel_psr_wait_for_idle(const struct intel_crtc_state
> > *new_crtc_state,
> >  			    u32 *out_value);
> >  bool intel_psr_enabled(struct intel_dp *intel_dp);
> > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > *dev_priv, enum pipe pipe, bool prepare);
> >  
> >  /* intel_quirks.c */
> >  void intel_init_quirks(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index a8554dc4f196..5d8772399f60 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -583,6 +583,14 @@ int intel_crtc_verify_crc_source(struct
> > drm_crtc
> > *crtc, const char *source_name,
> >  	return -EINVAL;
> >  }
> >  
> > +static inline void intel_crtc_crc_prepare_or_finish(struct
> > drm_crtc
> > *crtc, bool prepare)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +
> > +	intel_psr_crc_prepare_or_finish(dev_priv, intel_crtc->pipe,
> > prepare);
> > +}
> > +
> >  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char
> > *source_name)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > @@ -609,6 +617,8 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > *crtc, const char *source_name)
> >  	if (ret != 0)
> >  		goto out;
> >  
> > +	intel_crtc_crc_prepare_or_finish(crtc, source !=
> > INTEL_PIPE_CRC_SOURCE_NONE);
> > +
> >  	pipe_crc->source = source;
> >  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 08967836b48e..9c93138988aa 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -577,6 +577,9 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> >  		return false;
> >  	}
> >  
> > +	if (dev_priv->psr.pipe_crc_enabled)
> > +		return false;
> > +
> 
> Disabling PSR instead of switching to PSR1 is safer considering the
> past bug reports with PSR1.

I thought about that but it would break every PSR subtest in
kms_frontbuffer_tracking, I guess is better start by disabling PSR2 and
then discuss about PSR1 if decided to disable, we need to remove PSR1
from kms_frontbuffer_tracking first.

>  
> >  	return true;
> >  }
> >  
> > @@ -1291,3 +1294,23 @@ bool intel_psr_enabled(struct intel_dp
> > *intel_dp)
> >  
> >  	return ret;
> >  }
> > +
> > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > *dev_priv, enum pipe pipe, bool prepare)
> > +{
> > +	bool fastset = false;
> > +
> > +	if (!CAN_PSR(dev_priv))
> > +		return;
> > +
> > +	mutex_lock(&dev_priv->psr.lock);
> > +
> > +	if (dev_priv->psr.pipe == pipe) {
> > +		dev_priv->psr.pipe_crc_enabled = prepare;
> 
> .crc_enabled seems like it belongs in crtc_state rather than in the
> global atomic state.
> 
> Looks like we could rename and re-purpose
> crtc_state.ips_force_disable
> for this. I don't see that flag being used for anything other working
> around CRC issues.
> 

My understanging is that we should not update crtc_state or any other
state outside of atomic_check() call chain, if that is not true I will
rename and reuse ips_force_disable.

> 
> > +		fastset = !prepare || dev_priv->psr.psr2_enabled;
> > +	}
> > +
> > +	mutex_unlock(&dev_priv->psr.lock);
> > +
> > +	if (fastset)
> > +		intel_psr_fastset_force(dev_priv);
> > +}

[-- 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 2/4] drm/i915: Disable PSR2 while getting pipe CRC

[Dhinakaran Pandiyan]

On Sat, 2019-02-23 at 02:48 +0000, Souza, Jose wrote:
> On Fri, 2019-02-22 at 18:13 -0800, Dhinakaran Pandiyan wrote:
> > On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
> > > As stated in CRC_CTL spec, after PSR entry state CRC will not be
> > > calculated anymore what is not a problem as IGT tests do some
> > > screen
> > > change and then request the pipe CRC right after the change so
> > > PSR
> > > will go to idle state and only will entry again after at least 6
> > > idles frames.
> > > 
> > > But for PSR2 it is more problematic as any change to the screen
> > > could
> > > trigger a selective/partial update causing the CRC value not to
> > > be
> > > calculated over the full frame.
> > > 
> > > So here it disables PSR2 and keep it disabled while user is
> > > requesting pipe CRC.
> > > 
> > > BSpec: 7536
> > > 
> > > 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_drv.h      |  1 +
> > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 10 ++++++++++
> > >  drivers/gpu/drm/i915/intel_psr.c      | 23
> > > +++++++++++++++++++++++
> > >  4 files changed, 35 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 17fe942eaafa..609e9c5bd453 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -520,6 +520,7 @@ struct i915_psr {
> > >  	bool sink_not_reliable;
> > >  	bool irq_aux_error;
> > >  	u16 su_x_granularity;
> > > +	bool pipe_crc_enabled;
> > >  };
> > >  
> > >  enum intel_pch {
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 3398b28c053b..40ce7a600585 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -2103,6 +2103,7 @@ void intel_psr_short_pulse(struct intel_dp
> > > *intel_dp);
> > >  int intel_psr_wait_for_idle(const struct intel_crtc_state
> > > *new_crtc_state,
> > >  			    u32 *out_value);
> > >  bool intel_psr_enabled(struct intel_dp *intel_dp);
> > > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > > *dev_priv, enum pipe pipe, bool prepare);
> > >  
> > >  /* intel_quirks.c */
> > >  void intel_init_quirks(struct drm_i915_private *dev_priv);
> > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > index a8554dc4f196..5d8772399f60 100644
> > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > @@ -583,6 +583,14 @@ int intel_crtc_verify_crc_source(struct
> > > drm_crtc
> > > *crtc, const char *source_name,
> > >  	return -EINVAL;
> > >  }
> > >  
> > > +static inline void intel_crtc_crc_prepare_or_finish(struct
> > > drm_crtc
> > > *crtc, bool prepare)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +
> > > +	intel_psr_crc_prepare_or_finish(dev_priv, intel_crtc->pipe,
> > > prepare);
> > > +}
> > > +
> > >  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char
> > > *source_name)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > > @@ -609,6 +617,8 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > > *crtc, const char *source_name)
> > >  	if (ret != 0)
> > >  		goto out;
> > >  
> > > +	intel_crtc_crc_prepare_or_finish(crtc, source !=
> > > INTEL_PIPE_CRC_SOURCE_NONE);
> > > +
> > >  	pipe_crc->source = source;
> > >  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> > >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 08967836b48e..9c93138988aa 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -577,6 +577,9 @@ static bool intel_psr2_config_valid(struct
> > > intel_dp *intel_dp,
> > >  		return false;
> > >  	}
> > >  
> > > +	if (dev_priv->psr.pipe_crc_enabled)
> > > +		return false;
> > > +
> > 
> > Disabling PSR instead of switching to PSR1 is safer considering the
> > past bug reports with PSR1.
> 
> I thought about that but it would break every PSR subtest in
> kms_frontbuffer_tracking, I guess is better start by disabling PSR2
> and
> then discuss about PSR1 if decided to disable, we need to remove PSR1
> from kms_frontbuffer_tracking first.

I see. 
> 
> >  
> > >  	return true;
> > >  }
> > >  
> > > @@ -1291,3 +1294,23 @@ bool intel_psr_enabled(struct intel_dp
> > > *intel_dp)
> > >  
> > >  	return ret;
> > >  }
> > > +
> > > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > > *dev_priv, enum pipe pipe, bool prepare)
> > > +{
> > > +	bool fastset = false;
> > > +
> > > +	if (!CAN_PSR(dev_priv))
> > > +		return;
> > > +
> > > +	mutex_lock(&dev_priv->psr.lock);
> > > +
> > > +	if (dev_priv->psr.pipe == pipe) {
> > > +		dev_priv->psr.pipe_crc_enabled = prepare;
> > 
> > .crc_enabled seems like it belongs in crtc_state rather than in the
> > global atomic state.
> > 
> > Looks like we could rename and re-purpose
> > crtc_state.ips_force_disable
> > for this. I don't see that flag being used for anything other
> > working
> > around CRC issues.
> > 
> 
> My understanging is that we should not update crtc_state or any other
> state outside of atomic_check() call chain, if that is not true I
> will
> rename and reuse ips_force_disable.

I don't see it being any different from modifying 
crtc_state->mode_changed to do a fastset. Besides that, all that needs
to be done is rename the flag and shuffle the code a bit.

> 
> > 
> > > +		fastset = !prepare || dev_priv->psr.psr2_enabled;
> > > +	}
> > > +
> > > +	mutex_unlock(&dev_priv->psr.lock);
> > > +
> > > +	if (fastset)
> > > +		intel_psr_fastset_force(dev_priv);
> > > +}
> 
> _______________________________________________
> 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 1/4] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset

[Maarten Lankhorst]

Op 23-02-2019 om 03:02 schreef Dhinakaran Pandiyan:
> On Fri, 2019-02-22 at 16:33 -0800, Souza, Jose wrote:
>> On Fri, 2019-02-22 at 16:27 -0800, Pandiyan, Dhinakaran wrote:
>>> On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
>>>> 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.
>>> Is there an IGT that's failing because of this? Looks like the
>>> state
>>> would have got cleared only if the crtc was enabled to drive
>>> another
>>> encoder.
>> When PSR2 is enabled by default tests like 
>> kms_pipe_crc_basic@read-crc-pipe-b are failling even with the patch
>> that disable PSR2 when getting CRC.
> Thanks!
>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.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, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_psr.c
>>>> b/drivers/gpu/drm/i915/intel_psr.c
>>>> index 75c1a5deebf5..08967836b48e 100644
>>>> --- a/drivers/gpu/drm/i915/intel_psr.c
>>>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>>>> @@ -986,7 +986,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->enable &&
>>>> +		    intel_crtc_has_type(intel_crtc_state,
>>>> INTEL_OUTPUT_EDP) &&
>>>>  		    intel_crtc_state->has_psr) {
>
> if (crtc_state->enable && intel_crtc_state->has_psr)
> 	should cover all the cases, no?
>
> And also add a WARN() in case we somehow end up with more than once
> crtc with the above condition being true.


Please use crtc_state->active

crtc_state->enable only tells if the crtc is configured. It doesn't mean thatthe pipe is running.

~Maarten

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

Re: [PATCH 2/4] drm/i915: Disable PSR2 while getting pipe CRC

[Souza, Jose]

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

On Fri, 2019-02-22 at 22:14 -0800, Dhinakaran Pandiyan wrote:
> On Sat, 2019-02-23 at 02:48 +0000, Souza, Jose wrote:
> > On Fri, 2019-02-22 at 18:13 -0800, Dhinakaran Pandiyan wrote:
> > > On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
> > > > As stated in CRC_CTL spec, after PSR entry state CRC will not
> > > > be
> > > > calculated anymore what is not a problem as IGT tests do some
> > > > screen
> > > > change and then request the pipe CRC right after the change so
> > > > PSR
> > > > will go to idle state and only will entry again after at least
> > > > 6
> > > > idles frames.
> > > > 
> > > > But for PSR2 it is more problematic as any change to the screen
> > > > could
> > > > trigger a selective/partial update causing the CRC value not to
> > > > be
> > > > calculated over the full frame.
> > > > 
> > > > So here it disables PSR2 and keep it disabled while user is
> > > > requesting pipe CRC.
> > > > 
> > > > BSpec: 7536
> > > > 
> > > > 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_drv.h      |  1 +
> > > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 10 ++++++++++
> > > >  drivers/gpu/drm/i915/intel_psr.c      | 23
> > > > +++++++++++++++++++++++
> > > >  4 files changed, 35 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 17fe942eaafa..609e9c5bd453 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -520,6 +520,7 @@ struct i915_psr {
> > > >  	bool sink_not_reliable;
> > > >  	bool irq_aux_error;
> > > >  	u16 su_x_granularity;
> > > > +	bool pipe_crc_enabled;
> > > >  };
> > > >  
> > > >  enum intel_pch {
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 3398b28c053b..40ce7a600585 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -2103,6 +2103,7 @@ void intel_psr_short_pulse(struct
> > > > intel_dp
> > > > *intel_dp);
> > > >  int intel_psr_wait_for_idle(const struct intel_crtc_state
> > > > *new_crtc_state,
> > > >  			    u32 *out_value);
> > > >  bool intel_psr_enabled(struct intel_dp *intel_dp);
> > > > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > > > *dev_priv, enum pipe pipe, bool prepare);
> > > >  
> > > >  /* intel_quirks.c */
> > > >  void intel_init_quirks(struct drm_i915_private *dev_priv);
> > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > index a8554dc4f196..5d8772399f60 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > @@ -583,6 +583,14 @@ int intel_crtc_verify_crc_source(struct
> > > > drm_crtc
> > > > *crtc, const char *source_name,
> > > >  	return -EINVAL;
> > > >  }
> > > >  
> > > > +static inline void intel_crtc_crc_prepare_or_finish(struct
> > > > drm_crtc
> > > > *crtc, bool prepare)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > +
> > > > +	intel_psr_crc_prepare_or_finish(dev_priv, intel_crtc-
> > > > >pipe,
> > > > prepare);
> > > > +}
> > > > +
> > > >  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const
> > > > char
> > > > *source_name)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > > > @@ -609,6 +617,8 @@ int intel_crtc_set_crc_source(struct
> > > > drm_crtc
> > > > *crtc, const char *source_name)
> > > >  	if (ret != 0)
> > > >  		goto out;
> > > >  
> > > > +	intel_crtc_crc_prepare_or_finish(crtc, source !=
> > > > INTEL_PIPE_CRC_SOURCE_NONE);
> > > > +
> > > >  	pipe_crc->source = source;
> > > >  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> > > >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 08967836b48e..9c93138988aa 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -577,6 +577,9 @@ static bool intel_psr2_config_valid(struct
> > > > intel_dp *intel_dp,
> > > >  		return false;
> > > >  	}
> > > >  
> > > > +	if (dev_priv->psr.pipe_crc_enabled)
> > > > +		return false;
> > > > +
> > > 
> > > Disabling PSR instead of switching to PSR1 is safer considering
> > > the
> > > past bug reports with PSR1.
> > 
> > I thought about that but it would break every PSR subtest in
> > kms_frontbuffer_tracking, I guess is better start by disabling PSR2
> > and
> > then discuss about PSR1 if decided to disable, we need to remove
> > PSR1
> > from kms_frontbuffer_tracking first.
> 
> I see. 
> > >  
> > > >  	return true;
> > > >  }
> > > >  
> > > > @@ -1291,3 +1294,23 @@ bool intel_psr_enabled(struct intel_dp
> > > > *intel_dp)
> > > >  
> > > >  	return ret;
> > > >  }
> > > > +
> > > > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > > > *dev_priv, enum pipe pipe, bool prepare)
> > > > +{
> > > > +	bool fastset = false;
> > > > +
> > > > +	if (!CAN_PSR(dev_priv))
> > > > +		return;
> > > > +
> > > > +	mutex_lock(&dev_priv->psr.lock);
> > > > +
> > > > +	if (dev_priv->psr.pipe == pipe) {
> > > > +		dev_priv->psr.pipe_crc_enabled = prepare;
> > > 
> > > .crc_enabled seems like it belongs in crtc_state rather than in
> > > the
> > > global atomic state.
> > > 
> > > Looks like we could rename and re-purpose
> > > crtc_state.ips_force_disable
> > > for this. I don't see that flag being used for anything other
> > > working
> > > around CRC issues.
> > > 
> > 
> > My understanging is that we should not update crtc_state or any
> > other
> > state outside of atomic_check() call chain, if that is not true I
> > will
> > rename and reuse ips_force_disable.
> 
> I don't see it being any different from modifying 
> crtc_state->mode_changed to do a fastset. Besides that, all that
> needs
> to be done is rename the flag and shuffle the code a bit.

Ooh okay, I was thiking that you wanted to set it as
psr.pipe_crc_enabled is set in this patch, okay I will do the shuffling
and send other version.

Thanks for the review.

> 
> > > > +		fastset = !prepare || dev_priv-
> > > > >psr.psr2_enabled;
> > > > +	}
> > > > +
> > > > +	mutex_unlock(&dev_priv->psr.lock);
> > > > +
> > > > +	if (fastset)
> > > > +		intel_psr_fastset_force(dev_priv);
> > > > +}
> > 
> > _______________________________________________
> > 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