[PATCH 1/2] drm/i915/psr: Print PSR_STATUS when PSR idle wait times out.

[PATCH 1/2] drm/i915/psr: Print PSR_STATUS when PSR idle wait times out.

[Dhinakaran Pandiyan]

Knowing the status of the PSR HW state machine is useful for debug,
especially since we are seeing errors with PSR2 in CI.

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

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0601abb8c71f..6ee0d056229d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1944,7 +1944,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
 void intel_psr_short_pulse(struct intel_dp *intel_dp);
-int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state);
+int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
+			    u32 *out_value);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 4bd5768731ee..5686ddaa6a72 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -717,7 +717,8 @@ void intel_psr_disable(struct intel_dp *intel_dp,
 	cancel_work_sync(&dev_priv->psr.work);
 }
 
-int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state)
+int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
+			    u32 *out_value)
 {
 	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -750,8 +751,10 @@ int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state)
 	 * 6 ms of exit training time + 1.5 ms of aux channel
 	 * handshake. 50 msec is defesive enough to cover everything.
 	 */
-	return intel_wait_for_register(dev_priv, reg, mask,
-				       EDP_PSR_STATUS_STATE_IDLE, 50);
+
+	return __intel_wait_for_register(dev_priv, reg, mask,
+					 EDP_PSR_STATUS_STATE_IDLE, 2, 50,
+					 out_value);
 }
 
 static bool __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index f7026e887fa9..774bfb03c5d9 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -83,6 +83,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 	bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
 		intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI);
 	DEFINE_WAIT(wait);
+	u32 psr_status;
 
 	vblank_start = adjusted_mode->crtc_vblank_start;
 	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
@@ -104,8 +105,9 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 	 * VBL interrupts will start the PSR exit and prevent a PSR
 	 * re-entry as well.
 	 */
-	if (intel_psr_wait_for_idle(new_crtc_state))
-		DRM_ERROR("PSR idle timed out, atomic update may fail\n");
+	if (intel_psr_wait_for_idle(new_crtc_state, &psr_status))
+		DRM_ERROR("PSR idle timed out 0x%x, atomic update may fail\n",
+			  psr_status);
 
 	local_irq_disable();
 
-- 
2.17.1

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

[PATCH 2/2] drm/i915/psr: Remove wait_for_idle() for PSR2

[Dhinakaran Pandiyan]

CI runs show PSR2 does not go to IDLE with selective update enabled on
all PSR exit triggers. Specifically, logs indicate the hardware enters
"SLEEP Selective Update" and not "IDLE Reset state' like the kernel
expects. This check was added for PSR1 but incorrectly extended to PSR2,
remove this check for PSR2 as there is a plan to test only PSR1 on PSR2
panels.

Also add bspec reference to the comment about idle timeout.

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

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 5686ddaa6a72..09be9bfee2be 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -722,37 +722,26 @@ int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
 {
 	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	i915_reg_t reg;
-	u32 mask;
-
-	if (!new_crtc_state->has_psr)
-		return 0;
 
 	/*
-	 * The sole user right now is intel_pipe_update_start(),
-	 * which won't race with psr_enable/disable, which is
-	 * where psr2_enabled is written to. So, we don't need
-	 * to acquire the psr.lock. More importantly, we want the
-	 * latency inside intel_pipe_update_start() to be as low
-	 * as possible, so no need to acquire psr.lock when it is
-	 * not needed and will induce latencies in the atomic
-	 * update path.
+	 * The sole user right now is intel_pipe_update_start(), which won't
+	 * race with psr_enable/disable where psr2_enabled is written to. So, we
+	 * don't need to acquire the psr.lock. More importantly, we want the
+	 * latency inside intel_pipe_update_start() to be as low as possible, so
+	 * no need to acquire psr.lock when it is not needed and will induce
+	 * latencies in the atomic update path.
 	 */
-	if (dev_priv->psr.psr2_enabled) {
-		reg = EDP_PSR2_STATUS;
-		mask = EDP_PSR2_STATUS_STATE_MASK;
-	} else {
-		reg = EDP_PSR_STATUS;
-		mask = EDP_PSR_STATUS_STATE_MASK;
-	}
+	if (!new_crtc_state->has_psr || READ_ONCE(dev_priv->psr.psr2_enabled))
+		return 0;
 
 	/*
-	 * Max time for PSR to idle = Inverse of the refresh rate +
-	 * 6 ms of exit training time + 1.5 ms of aux channel
-	 * handshake. 50 msec is defesive enough to cover everything.
+	 * From Bspec Panel Self Refresh (BDW+):
+	 * Max. time for PSR to idle = inverse of the refresh rate + 6 ms of
+	 * exit training time + 1.5 ms of aux channel handshake. 50 ms is
+	 * defensive enough to cover everything.
 	 */
-
-	return __intel_wait_for_register(dev_priv, reg, mask,
+	return __intel_wait_for_register(dev_priv, EDP_PSR_STATUS,
+					 EDP_PSR_STATUS_STATE_MASK,
 					 EDP_PSR_STATUS_STATE_IDLE, 2, 50,
 					 out_value);
 }
-- 
2.17.1

_______________________________________________
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/2] drm/i915/psr: Print PSR_STATUS when PSR idle wait times out.

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/psr: Print PSR_STATUS when PSR idle wait times out.
URL   : https://patchwork.freedesktop.org/series/47978/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4639 -> Patchwork_9917 =

== Summary - SUCCESS ==

  No regressions found.

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

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    {igt@kms_psr@cursor_plane_move}:
      fi-cnl-psr:         DMESG-FAIL (fdo#107372) -> FAIL

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload-inject:
      fi-hsw-4770r:       PASS -> DMESG-WARN (fdo#107425)

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         NOTRUN -> DMESG-FAIL (fdo#107164)

    igt@drv_selftest@live_hangcheck:
      fi-kbl-7560u:       PASS -> DMESG-FAIL (fdo#106947, fdo#106560)

    igt@drv_selftest@live_workarounds:
      fi-cnl-psr:         PASS -> DMESG-FAIL (fdo#107292)

    igt@kms_frontbuffer_tracking@basic:
      {fi-byt-clapper}:   PASS -> FAIL (fdo#103167)

    
    ==== Possible fixes ====

    {igt@amdgpu/amd_prime@amd-to-i915}:
      fi-bxt-j4205:       INCOMPLETE (fdo#103927) -> SKIP

    {igt@amdgpu/amd_prime@i915-to-amd}:
      fi-bxt-j4205:       DMESG-FAIL -> SKIP

    igt@drv_selftest@live_workarounds:
      {fi-cfl-8109u}:     DMESG-FAIL (fdo#107292) -> PASS
      {fi-bsw-kefka}:     DMESG-FAIL (fdo#107292) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-a:
      {fi-byt-clapper}:   FAIL (fdo#107362) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

    {igt@kms_psr@primary_page_flip}:
      fi-cnl-psr:         DMESG-FAIL (fdo#107372) -> PASS

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

  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372
  fdo#107425 https://bugs.freedesktop.org/show_bug.cgi?id=107425


== Participating hosts (53 -> 47) ==

  Additional (1): fi-gdg-551 
  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-j1900 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-u 


== Build changes ==

    * Linux: CI_DRM_4639 -> Patchwork_9917

  CI_DRM_4639: da34c4841d4bd5098cef0bd3ddaeed1ee3eb3103 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4591: 6cb3d7dbe5831a7b2b5b7a4638d8a8b7ac624f5f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9917: 8373ea76fc7c3089fc6725873e501a44de28b278 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

8373ea76fc7c drm/i915/psr: Remove wait_for_idle() for PSR2
b79c05c167d6 drm/i915/psr: Print PSR_STATUS when PSR idle wait times out.

== Logs ==

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

✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915/psr: Print PSR_STATUS when PSR idle wait times out.

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/psr: Print PSR_STATUS when PSR idle wait times out.
URL   : https://patchwork.freedesktop.org/series/47978/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4639_full -> Patchwork_9917_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9917_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9917_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_9917_full:

  === IGT changes ===

    ==== Warnings ====

    igt@kms_cursor_legacy@cursorb-vs-flipa-varying-size:
      shard-hsw:          PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Possible fixes ====

    igt@kms_flip@modeset-vs-vblank-race-interruptible:
      shard-hsw:          DMESG-WARN (fdo#102614) -> PASS

    igt@perf@buffer-fill:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@perf@polling:
      shard-hsw:          FAIL (fdo#102252) -> PASS

    
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4639 -> Patchwork_9917

  CI_DRM_4639: da34c4841d4bd5098cef0bd3ddaeed1ee3eb3103 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4591: 6cb3d7dbe5831a7b2b5b7a4638d8a8b7ac624f5f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9917: 8373ea76fc7c3089fc6725873e501a44de28b278 @ 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_9917/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/psr: Remove wait_for_idle() for PSR2

[Rodrigo Vivi]

On Thu, Aug 09, 2018 at 05:41:35PM -0700, Dhinakaran Pandiyan wrote:
> CI runs show PSR2 does not go to IDLE with selective update enabled on
> all PSR exit triggers. Specifically, logs indicate the hardware enters
> "SLEEP Selective Update" and not "IDLE Reset state' like the kernel
> expects. This check was added for PSR1 but incorrectly extended to PSR2,
> remove this check for PSR2 as there is a plan to test only PSR1 on PSR2
> panels.
> 
> Also add bspec reference to the comment about idle timeout.
> 
> Cc: Tarun Vyas <tarun.vyas@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 39 ++++++++++++--------------------
>  1 file changed, 14 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 5686ddaa6a72..09be9bfee2be 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -722,37 +722,26 @@ int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	i915_reg_t reg;
> -	u32 mask;
> -
> -	if (!new_crtc_state->has_psr)
> -		return 0;
>  
>  	/*
> -	 * The sole user right now is intel_pipe_update_start(),
> -	 * which won't race with psr_enable/disable, which is
> -	 * where psr2_enabled is written to. So, we don't need
> -	 * to acquire the psr.lock. More importantly, we want the
> -	 * latency inside intel_pipe_update_start() to be as low
> -	 * as possible, so no need to acquire psr.lock when it is
> -	 * not needed and will induce latencies in the atomic
> -	 * update path.
> +	 * The sole user right now is intel_pipe_update_start(), which won't
> +	 * race with psr_enable/disable where psr2_enabled is written to. So, we
> +	 * don't need to acquire the psr.lock. More importantly, we want the
> +	 * latency inside intel_pipe_update_start() to be as low as possible, so
> +	 * no need to acquire psr.lock when it is not needed and will induce
> +	 * latencies in the atomic update path.
>  	 */

I think we shouldn't change this format here to keep patch cleaner...
if there is any change here I couldn't see because it is changing all
lines and if there is no change I think it is better not to touch because
it removes the focus of the real changes.

> -	if (dev_priv->psr.psr2_enabled) {
> -		reg = EDP_PSR2_STATUS;
> -		mask = EDP_PSR2_STATUS_STATE_MASK;
> -	} else {
> -		reg = EDP_PSR_STATUS;
> -		mask = EDP_PSR_STATUS_STATE_MASK;
> -	}
> +	if (!new_crtc_state->has_psr || READ_ONCE(dev_priv->psr.psr2_enabled))

I now see that we are removing psr2 of the picture, but I don't see how we are
improving psr2 situation here.
what am I missing?

> +		return 0;
>  
>  	/*
> -	 * Max time for PSR to idle = Inverse of the refresh rate +
> -	 * 6 ms of exit training time + 1.5 ms of aux channel
> -	 * handshake. 50 msec is defesive enough to cover everything.
> +	 * From Bspec Panel Self Refresh (BDW+):

This is another case, if we didn't change the format only this line ^
would be in the patch and it would be cleaner and easier to review the
changes.

but my biggest concern with this patch is how do we check now wait_psr2 idle

> +	 * Max. time for PSR to idle = inverse of the refresh rate + 6 ms of
> +	 * exit training time + 1.5 ms of aux channel handshake. 50 ms is
> +	 * defensive enough to cover everything.
>  	 */
> -
> -	return __intel_wait_for_register(dev_priv, reg, mask,
> +	return __intel_wait_for_register(dev_priv, EDP_PSR_STATUS,
> +					 EDP_PSR_STATUS_STATE_MASK,
>  					 EDP_PSR_STATUS_STATE_IDLE, 2, 50,
>  					 out_value);
>  }
> -- 
> 2.17.1
> 
> _______________________________________________
> 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 2/2] drm/i915/psr: Remove wait_for_idle() for PSR2

[Pandiyan, Dhinakaran]

On Mon, 2018-08-13 at 09:57 -0700, Rodrigo Vivi wrote:
> On Thu, Aug 09, 2018 at 05:41:35PM -0700, Dhinakaran Pandiyan wrote:
> > CI runs show PSR2 does not go to IDLE with selective update enabled
> > on
> > all PSR exit triggers. Specifically, logs indicate the hardware
> > enters
> > "SLEEP Selective Update" and not "IDLE Reset state' like the kernel
> > expects. This check was added for PSR1 but incorrectly extended to
> > PSR2,
> > remove this check for PSR2 as there is a plan to test only PSR1 on
> > PSR2
> > panels.
> > 
> > Also add bspec reference to the comment about idle timeout.
> > 
> > Cc: Tarun Vyas <tarun.vyas@intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 39 ++++++++++++--------------
> > ------
> >  1 file changed, 14 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 5686ddaa6a72..09be9bfee2be 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -722,37 +722,26 @@ int intel_psr_wait_for_idle(const struct
> > intel_crtc_state *new_crtc_state,
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state-
> > >base.crtc);
> >  	struct drm_i915_private *dev_priv = to_i915(crtc-
> > >base.dev);
> > -	i915_reg_t reg;
> > -	u32 mask;
> > -
> > -	if (!new_crtc_state->has_psr)
> > -		return 0;
> >  
> >  	/*
> > -	 * The sole user right now is intel_pipe_update_start(),
> > -	 * which won't race with psr_enable/disable, which is
> > -	 * where psr2_enabled is written to. So, we don't need
> > -	 * to acquire the psr.lock. More importantly, we want the
> > -	 * latency inside intel_pipe_update_start() to be as low
> > -	 * as possible, so no need to acquire psr.lock when it is
> > -	 * not needed and will induce latencies in the atomic
> > -	 * update path.
> > +	 * The sole user right now is intel_pipe_update_start(),
> > which won't
> > +	 * race with psr_enable/disable where psr2_enabled is
> > written to. So, we
> > +	 * don't need to acquire the psr.lock. More importantly,
> > we want the
> > +	 * latency inside intel_pipe_update_start() to be as low
> > as possible, so
> > +	 * no need to acquire psr.lock when it is not needed and
> > will induce
> > +	 * latencies in the atomic update path.
> >  	 */
> 
> I think we shouldn't change this format here to keep patch cleaner...
> if there is any change here I couldn't see because it is changing all
> lines and if there is no change I think it is better not to touch
> because
> it removes the focus of the real changes.

Okay.
> 
> > -	if (dev_priv->psr.psr2_enabled) {
> > -		reg = EDP_PSR2_STATUS;
> > -		mask = EDP_PSR2_STATUS_STATE_MASK;
> > -	} else {
> > -		reg = EDP_PSR_STATUS;
> > -		mask = EDP_PSR_STATUS_STATE_MASK;
> > -	}
> > +	if (!new_crtc_state->has_psr || READ_ONCE(dev_priv-
> > >psr.psr2_enabled))
> 
> I now see that we are removing psr2 of the picture, but I don't see
> how we are
> improving psr2 situation here.
> what am I missing?
> 
When the patch was written, we did not have sufficient tests to tell us
the wait_for_idle condition was wrong for PSR2. It was not known
whether the wait was *necessary* for PSR2, think of this as a partial
revert. Now that CI has pointed out, (and I checked with a PSR2 panel)
that the condition is wrong, we should be removing it for PSR2. If you
think about it, it does improve PSR2 my removing irrelevant code.


> > +		return 0;
> >  
> >  	/*
> > -	 * Max time for PSR to idle = Inverse of the refresh rate
> > +
> > -	 * 6 ms of exit training time + 1.5 ms of aux channel
> > -	 * handshake. 50 msec is defesive enough to cover
> > everything.
> > +	 * From Bspec Panel Self Refresh (BDW+):
> 
> This is another case, if we didn't change the format only this line ^
> would be in the patch and it would be cleaner and easier to review
> the
> changes.
> 
> but my biggest concern with this patch is how do we check now
> wait_psr2 idle
> 
> > +	 * Max. time for PSR to idle = inverse of the refresh rate
> > + 6 ms of
> > +	 * exit training time + 1.5 ms of aux channel handshake.
> > 50 ms is
> > +	 * defensive enough to cover everything.
> >  	 */
> > -
> > -	return __intel_wait_for_register(dev_priv, reg, mask,
> > +	return __intel_wait_for_register(dev_priv, EDP_PSR_STATUS,
> > +					 EDP_PSR_STATUS_STATE_MASK
> > ,
> >  					 EDP_PSR_STATUS_STATE_IDLE
> > , 2, 50,
> >  					 out_value);
> >  }
> > -- 
> > 2.17.1
> > 
> > _______________________________________________
> > 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 2/2] drm/i915/psr: Remove wait_for_idle() for PSR2

[Tarun Vyas]

On Mon, Aug 13, 2018 at 11:10:00AM -0700, Pandiyan, Dhinakaran wrote:
> On Mon, 2018-08-13 at 09:57 -0700, Rodrigo Vivi wrote:
> > On Thu, Aug 09, 2018 at 05:41:35PM -0700, Dhinakaran Pandiyan wrote:
> > > CI runs show PSR2 does not go to IDLE with selective update enabled
> > > on
> > > all PSR exit triggers. Specifically, logs indicate the hardware
> > > enters
> > > "SLEEP Selective Update" and not "IDLE Reset state' like the kernel
> > > expects. This check was added for PSR1 but incorrectly extended to
> > > PSR2,
> > > remove this check for PSR2 as there is a plan to test only PSR1 on
> > > PSR2
> > > panels.
> > > 
> > > Also add bspec reference to the comment about idle timeout.
> > > 
> > > Cc: Tarun Vyas <tarun.vyas@intel.com>
> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 39 ++++++++++++--------------
> > > ------
> > >  1 file changed, 14 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 5686ddaa6a72..09be9bfee2be 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -722,37 +722,26 @@ int intel_psr_wait_for_idle(const struct
> > > intel_crtc_state *new_crtc_state,
> > >  {
> > >  	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state-
> > > >base.crtc);
> > >  	struct drm_i915_private *dev_priv = to_i915(crtc-
> > > >base.dev);
> > > -	i915_reg_t reg;
> > > -	u32 mask;
> > > -
> > > -	if (!new_crtc_state->has_psr)
> > > -		return 0;
> > >  
> > >  	/*
> > > -	 * The sole user right now is intel_pipe_update_start(),
> > > -	 * which won't race with psr_enable/disable, which is
> > > -	 * where psr2_enabled is written to. So, we don't need
> > > -	 * to acquire the psr.lock. More importantly, we want the
> > > -	 * latency inside intel_pipe_update_start() to be as low
> > > -	 * as possible, so no need to acquire psr.lock when it is
> > > -	 * not needed and will induce latencies in the atomic
> > > -	 * update path.
> > > +	 * The sole user right now is intel_pipe_update_start(),
> > > which won't
> > > +	 * race with psr_enable/disable where psr2_enabled is
> > > written to. So, we
> > > +	 * don't need to acquire the psr.lock. More importantly,
> > > we want the
> > > +	 * latency inside intel_pipe_update_start() to be as low
> > > as possible, so
> > > +	 * no need to acquire psr.lock when it is not needed and
> > > will induce
> > > +	 * latencies in the atomic update path.
> > >  	 */
> > 
> > I think we shouldn't change this format here to keep patch cleaner...
> > if there is any change here I couldn't see because it is changing all
> > lines and if there is no change I think it is better not to touch
> > because
> > it removes the focus of the real changes.
> 
> Okay.
> > 
> > > -	if (dev_priv->psr.psr2_enabled) {
> > > -		reg = EDP_PSR2_STATUS;
> > > -		mask = EDP_PSR2_STATUS_STATE_MASK;
> > > -	} else {
> > > -		reg = EDP_PSR_STATUS;
> > > -		mask = EDP_PSR_STATUS_STATE_MASK;
> > > -	}
> > > +	if (!new_crtc_state->has_psr || READ_ONCE(dev_priv-
> > > >psr.psr2_enabled))
> > 
> > I now see that we are removing psr2 of the picture, but I don't see
> > how we are
> > improving psr2 situation here.
> > what am I missing?
> > 
> When the patch was written, we did not have sufficient tests to tell us
> the wait_for_idle condition was wrong for PSR2. It was not known
> whether the wait was *necessary* for PSR2, think of this as a partial
> revert. Now that CI has pointed out, (and I checked with a PSR2 panel)
> that the condition is wrong, we should be removing it for PSR2. If you
> think about it, it does improve PSR2 my removing irrelevant code.
Do we have another way to ensure that we dont try to do a pipe update or rather
check for the PIPE DSL when still in a PSR2 sleep state ?
> 
> 
> > > +		return 0;
> > >  
> > >  	/*
> > > -	 * Max time for PSR to idle = Inverse of the refresh rate
> > > +
> > > -	 * 6 ms of exit training time + 1.5 ms of aux channel
> > > -	 * handshake. 50 msec is defesive enough to cover
> > > everything.
> > > +	 * From Bspec Panel Self Refresh (BDW+):
> > 
> > This is another case, if we didn't change the format only this line ^
> > would be in the patch and it would be cleaner and easier to review
> > the
> > changes.
> > 
> > but my biggest concern with this patch is how do we check now
> > wait_psr2 idle
> > 
> > > +	 * Max. time for PSR to idle = inverse of the refresh rate
> > > + 6 ms of
> > > +	 * exit training time + 1.5 ms of aux channel handshake.
> > > 50 ms is
> > > +	 * defensive enough to cover everything.
> > >  	 */
> > > -
> > > -	return __intel_wait_for_register(dev_priv, reg, mask,
> > > +	return __intel_wait_for_register(dev_priv, EDP_PSR_STATUS,
> > > +					 EDP_PSR_STATUS_STATE_MASK
> > > ,
> > >  					 EDP_PSR_STATUS_STATE_IDLE
> > > , 2, 50,
> > >  					 out_value);
> > >  }
> > > -- 
> > > 2.17.1
> > > 
> > > _______________________________________________
> > > 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 2/2] drm/i915/psr: Remove wait_for_idle() for PSR2

[Pandiyan, Dhinakaran]

On Mon, 2018-08-13 at 11:17 -0700, Tarun Vyas wrote:
> On Mon, Aug 13, 2018 at 11:10:00AM -0700, Pandiyan, Dhinakaran wrote:
> > On Mon, 2018-08-13 at 09:57 -0700, Rodrigo Vivi wrote:
> > > On Thu, Aug 09, 2018 at 05:41:35PM -0700, Dhinakaran Pandiyan
> > > wrote:
> > > > CI runs show PSR2 does not go to IDLE with selective update
> > > > enabled
> > > > on
> > > > all PSR exit triggers. Specifically, logs indicate the hardware
> > > > enters
> > > > "SLEEP Selective Update" and not "IDLE Reset state' like the
> > > > kernel
> > > > expects. This check was added for PSR1 but incorrectly extended
> > > > to
> > > > PSR2,
> > > > remove this check for PSR2 as there is a plan to test only PSR1
> > > > on
> > > > PSR2
> > > > panels.
> > > > 
> > > > Also add bspec reference to the comment about idle timeout.
> > > > 
> > > > Cc: Tarun Vyas <tarun.vyas@intel.com>
> > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.c
> > > > om>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_psr.c | 39 ++++++++++++----------
> > > > ----
> > > > ------
> > > >  1 file changed, 14 insertions(+), 25 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 5686ddaa6a72..09be9bfee2be 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -722,37 +722,26 @@ int intel_psr_wait_for_idle(const struct
> > > > intel_crtc_state *new_crtc_state,
> > > >  {
> > > >  	struct intel_crtc *crtc =
> > > > to_intel_crtc(new_crtc_state-
> > > > > base.crtc);
> > > > 
> > > >  	struct drm_i915_private *dev_priv = to_i915(crtc-
> > > > > base.dev);
> > > > 
> > > > -	i915_reg_t reg;
> > > > -	u32 mask;
> > > > -
> > > > -	if (!new_crtc_state->has_psr)
> > > > -		return 0;
> > > >  
> > > >  	/*
> > > > -	 * The sole user right now is
> > > > intel_pipe_update_start(),
> > > > -	 * which won't race with psr_enable/disable, which is
> > > > -	 * where psr2_enabled is written to. So, we don't need
> > > > -	 * to acquire the psr.lock. More importantly, we want
> > > > the
> > > > -	 * latency inside intel_pipe_update_start() to be as
> > > > low
> > > > -	 * as possible, so no need to acquire psr.lock when it
> > > > is
> > > > -	 * not needed and will induce latencies in the atomic
> > > > -	 * update path.
> > > > +	 * The sole user right now is
> > > > intel_pipe_update_start(),
> > > > which won't
> > > > +	 * race with psr_enable/disable where psr2_enabled is
> > > > written to. So, we
> > > > +	 * don't need to acquire the psr.lock. More
> > > > importantly,
> > > > we want the
> > > > +	 * latency inside intel_pipe_update_start() to be as
> > > > low
> > > > as possible, so
> > > > +	 * no need to acquire psr.lock when it is not needed
> > > > and
> > > > will induce
> > > > +	 * latencies in the atomic update path.
> > > >  	 */
> > > 
> > > I think we shouldn't change this format here to keep patch
> > > cleaner...
> > > if there is any change here I couldn't see because it is changing
> > > all
> > > lines and if there is no change I think it is better not to touch
> > > because
> > > it removes the focus of the real changes.
> > 
> > Okay.
> > > 
> > > > -	if (dev_priv->psr.psr2_enabled) {
> > > > -		reg = EDP_PSR2_STATUS;
> > > > -		mask = EDP_PSR2_STATUS_STATE_MASK;
> > > > -	} else {
> > > > -		reg = EDP_PSR_STATUS;
> > > > -		mask = EDP_PSR_STATUS_STATE_MASK;
> > > > -	}
> > > > +	if (!new_crtc_state->has_psr || READ_ONCE(dev_priv-
> > > > > psr.psr2_enabled))
> > > 
> > > I now see that we are removing psr2 of the picture, but I don't
> > > see
> > > how we are
> > > improving psr2 situation here.
> > > what am I missing?
> > > 
> > 
> > When the patch was written, we did not have sufficient tests to
> > tell us
> > the wait_for_idle condition was wrong for PSR2. It was not known
> > whether the wait was *necessary* for PSR2, think of this as a
> > partial
> > revert. Now that CI has pointed out, (and I checked with a PSR2
> > panel)
> > that the condition is wrong, we should be removing it for PSR2. If
> > you
> > think about it, it does improve PSR2 my removing irrelevant code.
> 
> Do we have another way to ensure that we dont try to do a pipe update
> or rather
> check for the PIPE DSL when still in a PSR2 sleep state ?
I don't know, the PSR2 source states aren't documented well enough for
us to implement this change. The current check is clearly wrong, I
think we should remove and fix it correctly when we know it is needed.


> > 
> > 
> > > > +		return 0;
> > > >  
> > > >  	/*
> > > > -	 * Max time for PSR to idle = Inverse of the refresh
> > > > rate
> > > > +
> > > > -	 * 6 ms of exit training time + 1.5 ms of aux channel
> > > > -	 * handshake. 50 msec is defesive enough to cover
> > > > everything.
> > > > +	 * From Bspec Panel Self Refresh (BDW+):
> > > 
> > > This is another case, if we didn't change the format only this
> > > line ^
> > > would be in the patch and it would be cleaner and easier to
> > > review
> > > the
> > > changes.
> > > 
> > > but my biggest concern with this patch is how do we check now
> > > wait_psr2 idle
> > > 
> > > > +	 * Max. time for PSR to idle = inverse of the refresh
> > > > rate
> > > > + 6 ms of
> > > > +	 * exit training time + 1.5 ms of aux channel
> > > > handshake.
> > > > 50 ms is
> > > > +	 * defensive enough to cover everything.
> > > >  	 */
> > > > -
> > > > -	return __intel_wait_for_register(dev_priv, reg, mask,
> > > > +	return __intel_wait_for_register(dev_priv,
> > > > EDP_PSR_STATUS,
> > > > +					 EDP_PSR_STATUS_STATE_
> > > > MASK
> > > > ,
> > > >  					 EDP_PSR_STATUS_STATE_
> > > > IDLE
> > > > , 2, 50,
> > > >  					 out_value);
> > > >  }
> > > > -- 
> > > > 2.17.1
> > > > 
> > > > _______________________________________________
> > > > 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 2/2] drm/i915/psr: Remove wait_for_idle() for PSR2

[Dhinakaran Pandiyan]

On Mon, 2018-08-13 at 18:10 +0000, Pandiyan, Dhinakaran wrote:
> On Mon, 2018-08-13 at 09:57 -0700, Rodrigo Vivi wrote:
> > On Thu, Aug 09, 2018 at 05:41:35PM -0700, Dhinakaran Pandiyan
> > wrote:
> > > CI runs show PSR2 does not go to IDLE with selective update
> > > enabled
> > > on
> > > all PSR exit triggers. Specifically, logs indicate the hardware
> > > enters
> > > "SLEEP Selective Update" and not "IDLE Reset state' like the
> > > kernel
> > > expects. This check was added for PSR1 but incorrectly extended
> > > to
> > > PSR2,
> > > remove this check for PSR2 as there is a plan to test only PSR1
> > > on
> > > PSR2
> > > panels.
> > > 
> > > Also add bspec reference to the comment about idle timeout.
> > > 
> > > Cc: Tarun Vyas <tarun.vyas@intel.com>
> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > >
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 39 ++++++++++++--------------
> > > ------
> > >  1 file changed, 14 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 5686ddaa6a72..09be9bfee2be 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -722,37 +722,26 @@ int intel_psr_wait_for_idle(const struct
> > > intel_crtc_state *new_crtc_state,
> > >  {
> > >  	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state-
> > > > base.crtc);
> > > 
> > >  	struct drm_i915_private *dev_priv = to_i915(crtc-
> > > > base.dev);
> > > 
> > > -	i915_reg_t reg;
> > > -	u32 mask;
> > > -
> > > -	if (!new_crtc_state->has_psr)
> > > -		return 0;
> > >  
> > >  	/*
> > > -	 * The sole user right now is intel_pipe_update_start(),
> > > -	 * which won't race with psr_enable/disable, which is
> > > -	 * where psr2_enabled is written to. So, we don't need
> > > -	 * to acquire the psr.lock. More importantly, we want
> > > the
> > > -	 * latency inside intel_pipe_update_start() to be as low
> > > -	 * as possible, so no need to acquire psr.lock when it
> > > is
> > > -	 * not needed and will induce latencies in the atomic
> > > -	 * update path.
> > > +	 * The sole user right now is intel_pipe_update_start(),
> > > which won't
> > > +	 * race with psr_enable/disable where psr2_enabled is
> > > written to. So, we
> > > +	 * don't need to acquire the psr.lock. More importantly,
> > > we want the
> > > +	 * latency inside intel_pipe_update_start() to be as low
> > > as possible, so
> > > +	 * no need to acquire psr.lock when it is not needed and
> > > will induce
> > > +	 * latencies in the atomic update path.
> > >  	 */
> > 
> > I think we shouldn't change this format here to keep patch
> > cleaner...
> > if there is any change here I couldn't see because it is changing
> > all
> > lines and if there is no change I think it is better not to touch
> > because
> > it removes the focus of the real changes.
> 
> Okay.
> > 
> > > -	if (dev_priv->psr.psr2_enabled) {
> > > -		reg = EDP_PSR2_STATUS;
> > > -		mask = EDP_PSR2_STATUS_STATE_MASK;
> > > -	} else {
> > > -		reg = EDP_PSR_STATUS;
> > > -		mask = EDP_PSR_STATUS_STATE_MASK;
> > > -	}
> > > +	if (!new_crtc_state->has_psr || READ_ONCE(dev_priv-
> > > > psr.psr2_enabled))
> > 
> > I now see that we are removing psr2 of the picture, but I don't see
> > how we are
> > improving psr2 situation here.
> > what am I missing?
> > 
> 
> When the patch was written, we did not have sufficient tests to tell
> us
> the wait_for_idle condition was wrong for PSR2. It was not known
> whether the wait was *necessary* for PSR2, think of this as a partial
> revert. Now that CI has pointed out, (and I checked with a PSR2
> panel)
> that the condition is wrong, we should be removing it for PSR2. If
> you
> think about it, it does improve PSR2 my removing irrelevant code.
> 
I'll submit a new version without the comment diff if you are satisfied
with the above reasoning.

> 
> > > +		return 0;
> > >  
> > >  	/*
> > > -	 * Max time for PSR to idle = Inverse of the refresh
> > > rate
> > > +
> > > -	 * 6 ms of exit training time + 1.5 ms of aux channel
> > > -	 * handshake. 50 msec is defesive enough to cover
> > > everything.
> > > +	 * From Bspec Panel Self Refresh (BDW+):
> > 
> > This is another case, if we didn't change the format only this line
> > ^
> > would be in the patch and it would be cleaner and easier to review
> > the
> > changes.
> > 
> > but my biggest concern with this patch is how do we check now
> > wait_psr2 idle
> > 
> > > +	 * Max. time for PSR to idle = inverse of the refresh
> > > rate
> > > + 6 ms of
> > > +	 * exit training time + 1.5 ms of aux channel handshake.
> > > 50 ms is
> > > +	 * defensive enough to cover everything.
> > >  	 */
> > > -
> > > -	return __intel_wait_for_register(dev_priv, reg, mask,
> > > +	return __intel_wait_for_register(dev_priv,
> > > EDP_PSR_STATUS,
> > > +					 EDP_PSR_STATUS_STATE_MA
> > > SK
> > > ,
> > >  					 EDP_PSR_STATUS_STATE_ID
> > > LE
> > > , 2, 50,
> > >  					 out_value);
> > >  }
> > > -- 
> > > 2.17.1
> > > 
> > > _______________________________________________
> > > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/psr: Remove wait_for_idle() for PSR2

[Rodrigo Vivi]

On Wed, Aug 15, 2018 at 06:35:15PM -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-08-13 at 18:10 +0000, Pandiyan, Dhinakaran wrote:
> > On Mon, 2018-08-13 at 09:57 -0700, Rodrigo Vivi wrote:
> > > On Thu, Aug 09, 2018 at 05:41:35PM -0700, Dhinakaran Pandiyan
> > > wrote:
> > > > CI runs show PSR2 does not go to IDLE with selective update
> > > > enabled
> > > > on
> > > > all PSR exit triggers. Specifically, logs indicate the hardware
> > > > enters
> > > > "SLEEP Selective Update" and not "IDLE Reset state' like the
> > > > kernel
> > > > expects. This check was added for PSR1 but incorrectly extended
> > > > to
> > > > PSR2,
> > > > remove this check for PSR2 as there is a plan to test only PSR1
> > > > on
> > > > PSR2
> > > > panels.
> > > > 
> > > > Also add bspec reference to the comment about idle timeout.
> > > > 
> > > > Cc: Tarun Vyas <tarun.vyas@intel.com>
> > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > > >
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_psr.c | 39 ++++++++++++--------------
> > > > ------
> > > >  1 file changed, 14 insertions(+), 25 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 5686ddaa6a72..09be9bfee2be 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -722,37 +722,26 @@ int intel_psr_wait_for_idle(const struct
> > > > intel_crtc_state *new_crtc_state,
> > > >  {
> > > >  	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state-
> > > > > base.crtc);
> > > > 
> > > >  	struct drm_i915_private *dev_priv = to_i915(crtc-
> > > > > base.dev);
> > > > 
> > > > -	i915_reg_t reg;
> > > > -	u32 mask;
> > > > -
> > > > -	if (!new_crtc_state->has_psr)
> > > > -		return 0;
> > > >  
> > > >  	/*
> > > > -	 * The sole user right now is intel_pipe_update_start(),
> > > > -	 * which won't race with psr_enable/disable, which is
> > > > -	 * where psr2_enabled is written to. So, we don't need
> > > > -	 * to acquire the psr.lock. More importantly, we want
> > > > the
> > > > -	 * latency inside intel_pipe_update_start() to be as low
> > > > -	 * as possible, so no need to acquire psr.lock when it
> > > > is
> > > > -	 * not needed and will induce latencies in the atomic
> > > > -	 * update path.
> > > > +	 * The sole user right now is intel_pipe_update_start(),
> > > > which won't
> > > > +	 * race with psr_enable/disable where psr2_enabled is
> > > > written to. So, we
> > > > +	 * don't need to acquire the psr.lock. More importantly,
> > > > we want the
> > > > +	 * latency inside intel_pipe_update_start() to be as low
> > > > as possible, so
> > > > +	 * no need to acquire psr.lock when it is not needed and
> > > > will induce
> > > > +	 * latencies in the atomic update path.
> > > >  	 */
> > > 
> > > I think we shouldn't change this format here to keep patch
> > > cleaner...
> > > if there is any change here I couldn't see because it is changing
> > > all
> > > lines and if there is no change I think it is better not to touch
> > > because
> > > it removes the focus of the real changes.
> > 
> > Okay.
> > > 
> > > > -	if (dev_priv->psr.psr2_enabled) {
> > > > -		reg = EDP_PSR2_STATUS;
> > > > -		mask = EDP_PSR2_STATUS_STATE_MASK;
> > > > -	} else {
> > > > -		reg = EDP_PSR_STATUS;
> > > > -		mask = EDP_PSR_STATUS_STATE_MASK;
> > > > -	}
> > > > +	if (!new_crtc_state->has_psr || READ_ONCE(dev_priv-
> > > > > psr.psr2_enabled))
> > > 
> > > I now see that we are removing psr2 of the picture, but I don't see
> > > how we are
> > > improving psr2 situation here.
> > > what am I missing?
> > > 
> > 
> > When the patch was written, we did not have sufficient tests to tell
> > us
> > the wait_for_idle condition was wrong for PSR2. It was not known
> > whether the wait was *necessary* for PSR2, think of this as a partial
> > revert. Now that CI has pointed out, (and I checked with a PSR2
> > panel)
> > that the condition is wrong, we should be removing it for PSR2. If
> > you
> > think about it, it does improve PSR2 my removing irrelevant code.
> > 
> I'll submit a new version without the comment diff if you are satisfied
> with the above reasoning.

Please add a "FIXME:" here and we can fix psr2 situation later when we understand
it better...
so we at least unblock PSR for now...

> 
> > 
> > > > +		return 0;
> > > >  
> > > >  	/*
> > > > -	 * Max time for PSR to idle = Inverse of the refresh
> > > > rate
> > > > +
> > > > -	 * 6 ms of exit training time + 1.5 ms of aux channel
> > > > -	 * handshake. 50 msec is defesive enough to cover
> > > > everything.
> > > > +	 * From Bspec Panel Self Refresh (BDW+):
> > > 
> > > This is another case, if we didn't change the format only this line
> > > ^
> > > would be in the patch and it would be cleaner and easier to review
> > > the
> > > changes.
> > > 
> > > but my biggest concern with this patch is how do we check now
> > > wait_psr2 idle
> > > 
> > > > +	 * Max. time for PSR to idle = inverse of the refresh
> > > > rate
> > > > + 6 ms of
> > > > +	 * exit training time + 1.5 ms of aux channel handshake.
> > > > 50 ms is
> > > > +	 * defensive enough to cover everything.
> > > >  	 */
> > > > -
> > > > -	return __intel_wait_for_register(dev_priv, reg, mask,
> > > > +	return __intel_wait_for_register(dev_priv,
> > > > EDP_PSR_STATUS,
> > > > +					 EDP_PSR_STATUS_STATE_MA
> > > > SK
> > > > ,
> > > >  					 EDP_PSR_STATUS_STATE_ID
> > > > LE
> > > > , 2, 50,
> > > >  					 out_value);
> > > >  }
> > > > -- 
> > > > 2.17.1
> > > > 
> > > > _______________________________________________
> > > > 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
> _______________________________________________
> 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