All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle
@ 2018-06-26  5:57 Tarun Vyas
  2018-06-26  5:57 ` [PATCH v6 2/2] drm/i915: Wait for PSR exit before checking for vblank evasion Tarun Vyas
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Tarun Vyas @ 2018-06-26  5:57 UTC (permalink / raw)
  To: intel-gfx; +Cc: dhinakaran.pandiyan, rodrigo.vivi

This is a lockless version of the exisiting psr_wait_for_idle().
We want to wait for PSR to idle out inside intel_pipe_update_start.
At the time of a pipe update, we should never race with any psr
enable or disable code, which is a part of crtc enable/disable. So,
we can live w/o taking any psr locks at all.
The follow up patch will use this lockless wait inside pipe_update_
start to wait for PSR to idle out before checking for vblank evasion.

Even if psr is never enabled, psr2_enabled will be false and this
function will wait for PSR1 to idle out, which should just return
immediately, so a very short (~1-2 usec) wait for cases where PSR
is disabled.

v2: Add comment to explain the 25msec timeout (DK)

v3: Rename psr_wait_for_idle to __psr_wait_for_idle_locked to avoid
    naming conflicts and propagate err (if any) to the caller (Chris)

v5: Form a series with the next patch

Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 25 +++++++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 578346b8d7e2..9cb2b8afdd3e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 			      struct intel_crtc_state *crtc_state);
 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);
+int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
 
 /* 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 aea81ace854b..41e6962923ae 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -757,7 +757,28 @@ void intel_psr_disable(struct intel_dp *intel_dp,
 	cancel_work_sync(&dev_priv->psr.work);
 }
 
-static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
+int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
+{
+	i915_reg_t reg;
+	u32 mask;
+
+	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;
+	}
+
+	/*
+	 * The  25 msec timeout accounts for a frame @ 60Hz refresh rate,
+	 * exit training an aux handshake time.
+	 */
+	return intel_wait_for_register(dev_priv, reg, mask,
+				       EDP_PSR_STATUS_STATE_IDLE, 25);
+}
+
+static bool __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
 {
 	struct intel_dp *intel_dp;
 	i915_reg_t reg;
@@ -803,7 +824,7 @@ static void intel_psr_work(struct work_struct *work)
 	 * PSR might take some time to get fully disabled
 	 * and be ready for re-enable.
 	 */
-	if (!psr_wait_for_idle(dev_priv))
+	if (!__psr_wait_for_idle_locked(dev_priv))
 		goto unlock;
 
 	/*
-- 
2.13.5

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v6 2/2] drm/i915: Wait for PSR exit before checking for vblank evasion
  2018-06-26  5:57 [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle Tarun Vyas
@ 2018-06-26  5:57 ` Tarun Vyas
  2018-06-26  6:31 ` ✓ Fi.CI.BAT: success for series starting with [v6,1/2] drm/i915/psr: Lockless version of psr_wait_for_idle Patchwork
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Tarun Vyas @ 2018-06-26  5:57 UTC (permalink / raw)
  To: intel-gfx; +Cc: dhinakaran.pandiyan, rodrigo.vivi

The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then
the pipe_update_start call schedules itself out to check back later.

On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
lags w.r.t core kernel code, hot plugging an external display triggers
tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
closer analysis reveals that we try to read the scanline 3 times and
eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL
stuck @ 1599. This issue is not seen on upstream kernels, b/c for *some*
reason we loop inside intel_pipe_update start for ~2+ msec which in this
case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL
counter, hence no error. On the other hand, the ChromeOS kernel spends
~1.1 msec looping inside intel_pipe_update_start and hence errors out
b/c the source is still in PSR.

Regardless, we should wait for PSR exit (if PSR is disabled, we incur
a ~1-2 usec penalty) before reading the PIPEDSL, b/c if we haven't
fully exited PSR, then checking for vblank evasion isn't actually
applicable.

v4: Comment explaining psr_wait after enabling VBL interrupts (DK)

v5: CAN_PSR() to handle platforms that don't support PSR.

v6: Handle local_irq_disable on early return (Chris)

Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 344c0e709b19..4990d6e84ddf 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -107,13 +107,21 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 						      VBLANK_EVASION_TIME_US);
 	max = vblank_start - 1;
 
-	local_irq_disable();
-
 	if (min <= 0 || max <= 0)
-		return;
+		goto irq_disable;
 
 	if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
-		return;
+		goto irq_disable;
+
+	/*
+	 * Wait for psr to idle out after enabling the VBL interrupts
+	 * VBL interrupts will start the PSR exit and prevent a PSR
+	 * re-entry as well.
+	 */
+	if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv))
+		DRM_ERROR("PSR idle timed out, atomic update may fail\n");
+
+	local_irq_disable();
 
 	crtc->debug.min_vbl = min;
 	crtc->debug.max_vbl = max;
@@ -171,6 +179,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
 	crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
 
 	trace_i915_pipe_update_vblank_evaded(crtc);
+	return;
+
+irq_disable:
+	local_irq_disable();
 }
 
 /**
-- 
2.13.5

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* ✓ Fi.CI.BAT: success for series starting with [v6,1/2] drm/i915/psr: Lockless version of psr_wait_for_idle
  2018-06-26  5:57 [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle Tarun Vyas
  2018-06-26  5:57 ` [PATCH v6 2/2] drm/i915: Wait for PSR exit before checking for vblank evasion Tarun Vyas
@ 2018-06-26  6:31 ` Patchwork
  2018-06-26  7:38 ` ✓ Fi.CI.IGT: " Patchwork
  2018-06-26  8:26 ` [PATCH v6 1/2] " Daniel Vetter
  3 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-06-26  6:31 UTC (permalink / raw)
  To: Tarun Vyas; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v6,1/2] drm/i915/psr: Lockless version of psr_wait_for_idle
URL   : https://patchwork.freedesktop.org/series/45375/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4376 -> Patchwork_9419 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ctx_create@basic-files:
      fi-glk-dsi:         PASS -> DMESG-WARN (fdo#106954)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-skl-6600u:       DMESG-WARN -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
      fi-skl-guc:         FAIL (fdo#104724, fdo#103191) -> PASS

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#106954 https://bugs.freedesktop.org/show_bug.cgi?id=106954


== Participating hosts (43 -> 39) ==

  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4376 -> Patchwork_9419

  CI_DRM_4376: b64deb6f5bec9732aff68734c2e2382f044403f2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9419: c1936d91c6c57649e3c7d9342fc08908dad953c1 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

c1936d91c6c5 drm/i915: Wait for PSR exit before checking for vblank evasion
eb51eebf995d drm/i915/psr: Lockless version of psr_wait_for_idle

== Logs ==

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* ✓ Fi.CI.IGT: success for series starting with [v6,1/2] drm/i915/psr: Lockless version of psr_wait_for_idle
  2018-06-26  5:57 [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle Tarun Vyas
  2018-06-26  5:57 ` [PATCH v6 2/2] drm/i915: Wait for PSR exit before checking for vblank evasion Tarun Vyas
  2018-06-26  6:31 ` ✓ Fi.CI.BAT: success for series starting with [v6,1/2] drm/i915/psr: Lockless version of psr_wait_for_idle Patchwork
@ 2018-06-26  7:38 ` Patchwork
  2018-06-26  8:26 ` [PATCH v6 1/2] " Daniel Vetter
  3 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-06-26  7:38 UTC (permalink / raw)
  To: Tarun Vyas; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v6,1/2] drm/i915/psr: Lockless version of psr_wait_for_idle
URL   : https://patchwork.freedesktop.org/series/45375/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4376_full -> Patchwork_9419_full =

== Summary - WARNING ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@drv_selftest@live_guc:
      shard-kbl:          PASS -> SKIP +4

    igt@gem_exec_schedule@deep-vebox:
      shard-kbl:          SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_hangcheck:
      shard-kbl:          PASS -> DMESG-FAIL (fdo#106947)

    igt@kms_flip@2x-plain-flip-fb-recreate:
      shard-hsw:          PASS -> FAIL (fdo#100368) +1

    igt@kms_flip_tiling@flip-x-tiled:
      shard-glk:          PASS -> FAIL (fdo#104724) +2

    igt@perf_pmu@busy-accuracy-98-vcs1:
      shard-snb:          NOTRUN -> INCOMPLETE (fdo#105411)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      shard-apl:          DMESG-FAIL (fdo#106560, fdo#106947) -> PASS

    igt@drv_suspend@shrink:
      shard-apl:          FAIL (fdo#106886) -> PASS

    igt@gem_exec_schedule@preemptive-hang-render:
      shard-snb:          INCOMPLETE (fdo#105411) -> SKIP

    igt@gem_softpin@noreloc-s3:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@kms_flip@flip-vs-expired-vblank:
      shard-hsw:          FAIL (fdo#105363, fdo#102887) -> PASS
      shard-glk:          FAIL (fdo#105189) -> PASS

    igt@kms_flip_tiling@flip-to-y-tiled:
      shard-glk:          FAIL (fdo#104724, fdo#103822) -> PASS

    igt@kms_setmode@basic:
      shard-apl:          FAIL (fdo#99912) -> PASS
      shard-hsw:          FAIL (fdo#99912) -> PASS
      shard-kbl:          FAIL (fdo#99912) -> PASS

    
    ==== Warnings ====

    igt@drv_selftest@live_gtt:
      shard-glk:          FAIL (fdo#105347) -> INCOMPLETE (fdo#103359, k.org#198133)

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105189 https://bugs.freedesktop.org/show_bug.cgi?id=105189
  fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  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 (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4376 -> Patchwork_9419

  CI_DRM_4376: b64deb6f5bec9732aff68734c2e2382f044403f2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9419: c1936d91c6c57649e3c7d9342fc08908dad953c1 @ 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_9419/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle
  2018-06-26  5:57 [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle Tarun Vyas
                   ` (2 preceding siblings ...)
  2018-06-26  7:38 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-06-26  8:26 ` Daniel Vetter
  2018-06-26  8:42   ` Chris Wilson
  2018-06-26 19:43   ` Dhinakaran Pandiyan
  3 siblings, 2 replies; 10+ messages in thread
From: Daniel Vetter @ 2018-06-26  8:26 UTC (permalink / raw)
  To: Tarun Vyas; +Cc: intel-gfx, dhinakaran.pandiyan, rodrigo.vivi

On Mon, Jun 25, 2018 at 10:57:23PM -0700, Tarun Vyas wrote:
> This is a lockless version of the exisiting psr_wait_for_idle().
> We want to wait for PSR to idle out inside intel_pipe_update_start.
> At the time of a pipe update, we should never race with any psr
> enable or disable code, which is a part of crtc enable/disable. So,
> we can live w/o taking any psr locks at all.
> The follow up patch will use this lockless wait inside pipe_update_
> start to wait for PSR to idle out before checking for vblank evasion.

What's the upside of the lockless wait? The patch seems to be entirely
missing the motivation for the change. "Make it lockless" isn't a good
justification on itself, there needs to be data about overhead or stalls
included if that's the reason for doing this change.

> Even if psr is never enabled, psr2_enabled will be false and this
> function will wait for PSR1 to idle out, which should just return
> immediately, so a very short (~1-2 usec) wait for cases where PSR
> is disabled.
> 
> v2: Add comment to explain the 25msec timeout (DK)
> 
> v3: Rename psr_wait_for_idle to __psr_wait_for_idle_locked to avoid
>     naming conflicts and propagate err (if any) to the caller (Chris)
> 
> v5: Form a series with the next patch
> 
> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 25 +++++++++++++++++++++++--
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 578346b8d7e2..9cb2b8afdd3e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  			      struct intel_crtc_state *crtc_state);
>  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);
> +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
>  
>  /* 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 aea81ace854b..41e6962923ae 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -757,7 +757,28 @@ void intel_psr_disable(struct intel_dp *intel_dp,
>  	cancel_work_sync(&dev_priv->psr.work);
>  }
>  
> -static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
> +{
> +	i915_reg_t reg;
> +	u32 mask;
> +

I think a comment here explaining why the lockless access is correct is
justified here.

> +	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;
> +	}
> +
> +	/*
> +	 * The  25 msec timeout accounts for a frame @ 60Hz refresh rate,
> +	 * exit training an aux handshake time.
> +	 */

So this goes boom if the panel is running at e.g. 50Hz? Please either
calculate this from the current mode (but that's a bit tricky, due to
DRRS), or go with a more defensive timeout. Also small typo, s/an/and/.

Would also be good to have numbers for the exit training/aux handshake
time.
-Daniel

> +	return intel_wait_for_register(dev_priv, reg, mask,
> +				       EDP_PSR_STATUS_STATE_IDLE, 25);
> +}
> +
> +static bool __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_dp *intel_dp;
>  	i915_reg_t reg;
> @@ -803,7 +824,7 @@ static void intel_psr_work(struct work_struct *work)
>  	 * PSR might take some time to get fully disabled
>  	 * and be ready for re-enable.
>  	 */
> -	if (!psr_wait_for_idle(dev_priv))
> +	if (!__psr_wait_for_idle_locked(dev_priv))
>  		goto unlock;
>  
>  	/*
> -- 
> 2.13.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle
  2018-06-26  8:26 ` [PATCH v6 1/2] " Daniel Vetter
@ 2018-06-26  8:42   ` Chris Wilson
  2018-06-26 20:36     ` Tarun Vyas
  2018-06-26 19:43   ` Dhinakaran Pandiyan
  1 sibling, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2018-06-26  8:42 UTC (permalink / raw)
  To: Daniel Vetter, Tarun Vyas; +Cc: intel-gfx, dhinakaran.pandiyan, rodrigo.vivi

Quoting Daniel Vetter (2018-06-26 09:26:57)
> On Mon, Jun 25, 2018 at 10:57:23PM -0700, Tarun Vyas wrote:
> > This is a lockless version of the exisiting psr_wait_for_idle().
> > We want to wait for PSR to idle out inside intel_pipe_update_start.
> > At the time of a pipe update, we should never race with any psr
> > enable or disable code, which is a part of crtc enable/disable. So,
> > we can live w/o taking any psr locks at all.
> > The follow up patch will use this lockless wait inside pipe_update_
> > start to wait for PSR to idle out before checking for vblank evasion.
> 
> What's the upside of the lockless wait? The patch seems to be entirely
> missing the motivation for the change. "Make it lockless" isn't a good
> justification on itself, there needs to be data about overhead or stalls
> included if that's the reason for doing this change.
> 
> > Even if psr is never enabled, psr2_enabled will be false and this
> > function will wait for PSR1 to idle out, which should just return
> > immediately, so a very short (~1-2 usec) wait for cases where PSR
> > is disabled.
> > 
> > v2: Add comment to explain the 25msec timeout (DK)
> > 
> > v3: Rename psr_wait_for_idle to __psr_wait_for_idle_locked to avoid
> >     naming conflicts and propagate err (if any) to the caller (Chris)
> > 
> > v5: Form a series with the next patch
> > 
> > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 25 +++++++++++++++++++++++--
> >  2 files changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 578346b8d7e2..9cb2b8afdd3e 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
> >                             struct intel_crtc_state *crtc_state);
> >  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);
> > +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
> >  
> >  /* 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 aea81ace854b..41e6962923ae 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -757,7 +757,28 @@ void intel_psr_disable(struct intel_dp *intel_dp,
> >       cancel_work_sync(&dev_priv->psr.work);
> >  }
> >  
> > -static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > +{
> > +     i915_reg_t reg;
> > +     u32 mask;
> > +
> 
> I think a comment here explaining why the lockless access is correct is
> justified here.
> 
> > +     if (dev_priv->psr.psr2_enabled) {

In particular, it is this 'psr2_enabled' and which register we need that
is serialised in the locked version. The important question to answer is
why can we lift that here and not there.

In this case (and even in the other case), you could simply say "do both".
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle
  2018-06-26  8:26 ` [PATCH v6 1/2] " Daniel Vetter
  2018-06-26  8:42   ` Chris Wilson
@ 2018-06-26 19:43   ` Dhinakaran Pandiyan
  2018-06-26 20:20     ` Tarun Vyas
  1 sibling, 1 reply; 10+ messages in thread
From: Dhinakaran Pandiyan @ 2018-06-26 19:43 UTC (permalink / raw)
  To: Daniel Vetter, Tarun Vyas; +Cc: intel-gfx, rodrigo.vivi

On Tue, 2018-06-26 at 10:26 +0200, Daniel Vetter wrote:
> On Mon, Jun 25, 2018 at 10:57:23PM -0700, Tarun Vyas wrote:
> > 
> > This is a lockless version of the exisiting psr_wait_for_idle().
> > We want to wait for PSR to idle out inside intel_pipe_update_start.
> > At the time of a pipe update, we should never race with any psr
> > enable or disable code, which is a part of crtc enable/disable. So,
> > we can live w/o taking any psr locks at all.
> > The follow up patch will use this lockless wait inside pipe_update_
> > start to wait for PSR to idle out before checking for vblank
> > evasion.
> What's the upside of the lockless wait? The patch seems to be
> entirely
> missing the motivation for the change. "Make it lockless" isn't a
> good
> justification on itself, there needs to be data about overhead or
> stalls
> included if that's the reason for doing this change.
> 
Acquiring the PSR mutex means potential stalls due to PSR work having
already acquired it. The idea was to keep PSR changes in
pipe_update_start() less invasive latency wise.

But yeah, the commit has to add the explanation.



> > 
> > Even if psr is never enabled, psr2_enabled will be false and this
> > function will wait for PSR1 to idle out, which should just return
> > immediately, so a very short (~1-2 usec) wait for cases where PSR
> > is disabled.
> > 
> > v2: Add comment to explain the 25msec timeout (DK)
> > 
> > v3: Rename psr_wait_for_idle to __psr_wait_for_idle_locked to avoid
> >     naming conflicts and propagate err (if any) to the caller
> > (Chris)
> > 
> > v5: Form a series with the next patch
> > 
> > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 25 +++++++++++++++++++++++--
> >  2 files changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 578346b8d7e2..9cb2b8afdd3e 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp
> > *intel_dp,
> >  			      struct intel_crtc_state
> > *crtc_state);
> >  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);
> > +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
> >  
> >  /* 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 aea81ace854b..41e6962923ae 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -757,7 +757,28 @@ void intel_psr_disable(struct intel_dp
> > *intel_dp,
> >  	cancel_work_sync(&dev_priv->psr.work);
> >  }
> >  
> > -static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > +{
> > +	i915_reg_t reg;
> > +	u32 mask;
> > +
> I think a comment here explaining why the lockless access is correct
> is
> justified here.
> 
> > 
> > +	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;
> > +	}
> > +
> > +	/*
> > +	 * The  25 msec timeout accounts for a frame @ 60Hz
> > refresh rate,
> > +	 * exit training an aux handshake time.
> > +	 */
> So this goes boom if the panel is running at e.g. 50Hz? Please either
> calculate this from the current mode (but that's a bit tricky, due to
> DRRS), or go with a more defensive timeout. Also small typo,
> s/an/and/.
> 
> Would also be good to have numbers for the exit training/aux
> handshake
> time.

bspec says exit should be compelete in  "one full frame time (1/refresh
rate), plus SRD exit training time (max of 6ms), plus SRD aux channel
handshake (max of 1.5ms)."



> -Daniel
> 
> > 
> > +	return intel_wait_for_register(dev_priv, reg, mask,
> > +				       EDP_PSR_STATUS_STATE_IDLE,
> > 25);
> > +}
> > +
> > +static bool __psr_wait_for_idle_locked(struct drm_i915_private
> > *dev_priv)
> >  {
> >  	struct intel_dp *intel_dp;
> >  	i915_reg_t reg;
> > @@ -803,7 +824,7 @@ static void intel_psr_work(struct work_struct
> > *work)
> >  	 * PSR might take some time to get fully disabled
> >  	 * and be ready for re-enable.
> >  	 */
> > -	if (!psr_wait_for_idle(dev_priv))
> > +	if (!__psr_wait_for_idle_locked(dev_priv))
> >  		goto unlock;
> >  
> >  	/*
> > -- 
> > 2.13.5
> > 
> > _______________________________________________
> > 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle
  2018-06-26 19:43   ` Dhinakaran Pandiyan
@ 2018-06-26 20:20     ` Tarun Vyas
  2018-06-28  6:51       ` Daniel Vetter
  0 siblings, 1 reply; 10+ messages in thread
From: Tarun Vyas @ 2018-06-26 20:20 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: intel-gfx, rodrigo.vivi

On Tue, Jun 26, 2018 at 12:43:42PM -0700, Dhinakaran Pandiyan wrote:
> On Tue, 2018-06-26 at 10:26 +0200, Daniel Vetter wrote:
> > On Mon, Jun 25, 2018 at 10:57:23PM -0700, Tarun Vyas wrote:
> > > 
> > > This is a lockless version of the exisiting psr_wait_for_idle().
> > > We want to wait for PSR to idle out inside intel_pipe_update_start.
> > > At the time of a pipe update, we should never race with any psr
> > > enable or disable code, which is a part of crtc enable/disable. So,
> > > we can live w/o taking any psr locks at all.
> > > The follow up patch will use this lockless wait inside pipe_update_
> > > start to wait for PSR to idle out before checking for vblank
> > > evasion.
> > What's the upside of the lockless wait? The patch seems to be
> > entirely
> > missing the motivation for the change. "Make it lockless" isn't a
> > good
> > justification on itself, there needs to be data about overhead or
> > stalls
> > included if that's the reason for doing this change.
> > 
> Acquiring the PSR mutex means potential stalls due to PSR work having
> already acquired it. The idea was to keep PSR changes in
> pipe_update_start() less invasive latency wise.
> 
> But yeah, the commit has to add the explanation.
> 
> 
>
Yea, will explain it better in the commit message. 
> > > 
> > > Even if psr is never enabled, psr2_enabled will be false and this
> > > function will wait for PSR1 to idle out, which should just return
> > > immediately, so a very short (~1-2 usec) wait for cases where PSR
> > > is disabled.
> > > 
> > > v2: Add comment to explain the 25msec timeout (DK)
> > > 
> > > v3: Rename psr_wait_for_idle to __psr_wait_for_idle_locked to avoid
> > >     naming conflicts and propagate err (if any) to the caller
> > > (Chris)
> > > 
> > > v5: Form a series with the next patch
> > > 
> > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > >  drivers/gpu/drm/i915/intel_psr.c | 25 +++++++++++++++++++++++--
> > >  2 files changed, 24 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 578346b8d7e2..9cb2b8afdd3e 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp
> > > *intel_dp,
> > >  			      struct intel_crtc_state
> > > *crtc_state);
> > >  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);
> > > +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
> > >  
> > >  /* 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 aea81ace854b..41e6962923ae 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -757,7 +757,28 @@ void intel_psr_disable(struct intel_dp
> > > *intel_dp,
> > >  	cancel_work_sync(&dev_priv->psr.work);
> > >  }
> > >  
> > > -static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > > +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > > +{
> > > +	i915_reg_t reg;
> > > +	u32 mask;
> > > +
> > I think a comment here explaining why the lockless access is correct
> > is
> > justified here.
> > 
> > > 
> > > +	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;
> > > +	}
> > > +
> > > +	/*
> > > +	 * The  25 msec timeout accounts for a frame @ 60Hz
> > > refresh rate,
> > > +	 * exit training an aux handshake time.
> > > +	 */
> > So this goes boom if the panel is running at e.g. 50Hz? Please either
> > calculate this from the current mode (but that's a bit tricky, due to
> > DRRS), or go with a more defensive timeout. Also small typo,
> > s/an/and/.
> > 
> > Would also be good to have numbers for the exit training/aux
> > handshake
> > time.
> 
> bspec says exit should be compelete in  "one full frame time (1/refresh
> rate), plus SRD exit training time (max of 6ms), plus SRD aux channel
> handshake (max of 1.5ms)."
> 
> 
> 
So should we use 50 Hz as the lower limit for the refresh rate to calc our max timeout here. Can eDP go down to 30 Hz ?
> > -Daniel
> > 
> > > 
> > > +	return intel_wait_for_register(dev_priv, reg, mask,
> > > +				       EDP_PSR_STATUS_STATE_IDLE,
> > > 25);
> > > +}
> > > +
> > > +static bool __psr_wait_for_idle_locked(struct drm_i915_private
> > > *dev_priv)
> > >  {
> > >  	struct intel_dp *intel_dp;
> > >  	i915_reg_t reg;
> > > @@ -803,7 +824,7 @@ static void intel_psr_work(struct work_struct
> > > *work)
> > >  	 * PSR might take some time to get fully disabled
> > >  	 * and be ready for re-enable.
> > >  	 */
> > > -	if (!psr_wait_for_idle(dev_priv))
> > > +	if (!__psr_wait_for_idle_locked(dev_priv))
> > >  		goto unlock;
> > >  
> > >  	/*
> > > -- 
> > > 2.13.5
> > > 
> > > _______________________________________________
> > > 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle
  2018-06-26  8:42   ` Chris Wilson
@ 2018-06-26 20:36     ` Tarun Vyas
  0 siblings, 0 replies; 10+ messages in thread
From: Tarun Vyas @ 2018-06-26 20:36 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx, dhinakaran.pandiyan, rodrigo.vivi

On Tue, Jun 26, 2018 at 09:42:11AM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2018-06-26 09:26:57)
> > On Mon, Jun 25, 2018 at 10:57:23PM -0700, Tarun Vyas wrote:
> > > This is a lockless version of the exisiting psr_wait_for_idle().
> > > We want to wait for PSR to idle out inside intel_pipe_update_start.
> > > At the time of a pipe update, we should never race with any psr
> > > enable or disable code, which is a part of crtc enable/disable. So,
> > > we can live w/o taking any psr locks at all.
> > > The follow up patch will use this lockless wait inside pipe_update_
> > > start to wait for PSR to idle out before checking for vblank evasion.
> > 
> > What's the upside of the lockless wait? The patch seems to be entirely
> > missing the motivation for the change. "Make it lockless" isn't a good
> > justification on itself, there needs to be data about overhead or stalls
> > included if that's the reason for doing this change.
> > 
> > > Even if psr is never enabled, psr2_enabled will be false and this
> > > function will wait for PSR1 to idle out, which should just return
> > > immediately, so a very short (~1-2 usec) wait for cases where PSR
> > > is disabled.
> > > 
> > > v2: Add comment to explain the 25msec timeout (DK)
> > > 
> > > v3: Rename psr_wait_for_idle to __psr_wait_for_idle_locked to avoid
> > >     naming conflicts and propagate err (if any) to the caller (Chris)
> > > 
> > > v5: Form a series with the next patch
> > > 
> > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > >  drivers/gpu/drm/i915/intel_psr.c | 25 +++++++++++++++++++++++--
> > >  2 files changed, 24 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 578346b8d7e2..9cb2b8afdd3e 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
> > >                             struct intel_crtc_state *crtc_state);
> > >  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);
> > > +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
> > >  
> > >  /* 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 aea81ace854b..41e6962923ae 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -757,7 +757,28 @@ void intel_psr_disable(struct intel_dp *intel_dp,
> > >       cancel_work_sync(&dev_priv->psr.work);
> > >  }
> > >  
> > > -static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > > +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > > +{
> > > +     i915_reg_t reg;
> > > +     u32 mask;
> > > +
> > 
> > I think a comment here explaining why the lockless access is correct is
> > justified here.
> > 
> > > +     if (dev_priv->psr.psr2_enabled) {
> 
> In particular, it is this 'psr2_enabled' and which register we need that
> is serialised in the locked version. The important question to answer is
> why can we lift that here and not there.
> 
> In this case (and even in the other case), you could simply say "do both".
> -Chris
With the locked case, the concern was that intel_psr_work can race with psr_enable/disable paths but we have no such concern here, as we are already too far in the commit path. psr2_enabled should be stable by the time we do pipe_update(s), so we can do a fast check in pipe_update w/o grabbing and subsequently dropping locks.

Also, another difference here is that, "intel_dp = dev_priv->psr.enabled;" has been dropped in the lockless version as we don't really care if psr is enabled or disabled. If it is disabled, then the intel_wait_for_register should be close to a noop, ~1usec hit as per the preliminary measurements. 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle
  2018-06-26 20:20     ` Tarun Vyas
@ 2018-06-28  6:51       ` Daniel Vetter
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2018-06-28  6:51 UTC (permalink / raw)
  To: Tarun Vyas; +Cc: intel-gfx, Dhinakaran Pandiyan, rodrigo.vivi

On Tue, Jun 26, 2018 at 01:20:58PM -0700, Tarun Vyas wrote:
> On Tue, Jun 26, 2018 at 12:43:42PM -0700, Dhinakaran Pandiyan wrote:
> > On Tue, 2018-06-26 at 10:26 +0200, Daniel Vetter wrote:
> > > On Mon, Jun 25, 2018 at 10:57:23PM -0700, Tarun Vyas wrote:
> > > > 
> > > > This is a lockless version of the exisiting psr_wait_for_idle().
> > > > We want to wait for PSR to idle out inside intel_pipe_update_start.
> > > > At the time of a pipe update, we should never race with any psr
> > > > enable or disable code, which is a part of crtc enable/disable. So,
> > > > we can live w/o taking any psr locks at all.
> > > > The follow up patch will use this lockless wait inside pipe_update_
> > > > start to wait for PSR to idle out before checking for vblank
> > > > evasion.
> > > What's the upside of the lockless wait? The patch seems to be
> > > entirely
> > > missing the motivation for the change. "Make it lockless" isn't a
> > > good
> > > justification on itself, there needs to be data about overhead or
> > > stalls
> > > included if that's the reason for doing this change.
> > > 
> > Acquiring the PSR mutex means potential stalls due to PSR work having
> > already acquired it. The idea was to keep PSR changes in
> > pipe_update_start() less invasive latency wise.
> > 
> > But yeah, the commit has to add the explanation.
> > 
> > 
> >
> Yea, will explain it better in the commit message. 

Have we measured these stalls? Is it actually faster?

Directly poking hw registers because our own software tracking is a bit
funny still feels like a rather bad hack. And without gathering data I'd
assume that the mutex_lock is contended only when the there's a state
transition going on in psr, and in that case the register wait_for will
also take quite a while (equally long really, until the psr hw settles).
-Daniel

> > > > 
> > > > Even if psr is never enabled, psr2_enabled will be false and this
> > > > function will wait for PSR1 to idle out, which should just return
> > > > immediately, so a very short (~1-2 usec) wait for cases where PSR
> > > > is disabled.
> > > > 
> > > > v2: Add comment to explain the 25msec timeout (DK)
> > > > 
> > > > v3: Rename psr_wait_for_idle to __psr_wait_for_idle_locked to avoid
> > > >     naming conflicts and propagate err (if any) to the caller
> > > > (Chris)
> > > > 
> > > > v5: Form a series with the next patch
> > > > 
> > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > > >  drivers/gpu/drm/i915/intel_psr.c | 25 +++++++++++++++++++++++--
> > > >  2 files changed, 24 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 578346b8d7e2..9cb2b8afdd3e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp
> > > > *intel_dp,
> > > >  			      struct intel_crtc_state
> > > > *crtc_state);
> > > >  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);
> > > > +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
> > > >  
> > > >  /* 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 aea81ace854b..41e6962923ae 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -757,7 +757,28 @@ void intel_psr_disable(struct intel_dp
> > > > *intel_dp,
> > > >  	cancel_work_sync(&dev_priv->psr.work);
> > > >  }
> > > >  
> > > > -static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > > > +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > > > +{
> > > > +	i915_reg_t reg;
> > > > +	u32 mask;
> > > > +
> > > I think a comment here explaining why the lockless access is correct
> > > is
> > > justified here.
> > > 
> > > > 
> > > > +	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;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * The  25 msec timeout accounts for a frame @ 60Hz
> > > > refresh rate,
> > > > +	 * exit training an aux handshake time.
> > > > +	 */
> > > So this goes boom if the panel is running at e.g. 50Hz? Please either
> > > calculate this from the current mode (but that's a bit tricky, due to
> > > DRRS), or go with a more defensive timeout. Also small typo,
> > > s/an/and/.
> > > 
> > > Would also be good to have numbers for the exit training/aux
> > > handshake
> > > time.
> > 
> > bspec says exit should be compelete in  "one full frame time (1/refresh
> > rate), plus SRD exit training time (max of 6ms), plus SRD aux channel
> > handshake (max of 1.5ms)."
> > 
> > 
> > 
> So should we use 50 Hz as the lower limit for the refresh rate to calc our max timeout here. Can eDP go down to 30 Hz ?
> > > -Daniel
> > > 
> > > > 
> > > > +	return intel_wait_for_register(dev_priv, reg, mask,
> > > > +				       EDP_PSR_STATUS_STATE_IDLE,
> > > > 25);
> > > > +}
> > > > +
> > > > +static bool __psr_wait_for_idle_locked(struct drm_i915_private
> > > > *dev_priv)
> > > >  {
> > > >  	struct intel_dp *intel_dp;
> > > >  	i915_reg_t reg;
> > > > @@ -803,7 +824,7 @@ static void intel_psr_work(struct work_struct
> > > > *work)
> > > >  	 * PSR might take some time to get fully disabled
> > > >  	 * and be ready for re-enable.
> > > >  	 */
> > > > -	if (!psr_wait_for_idle(dev_priv))
> > > > +	if (!__psr_wait_for_idle_locked(dev_priv))
> > > >  		goto unlock;
> > > >  
> > > >  	/*
> > > > -- 
> > > > 2.13.5
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-06-28  6:51 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-26  5:57 [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle Tarun Vyas
2018-06-26  5:57 ` [PATCH v6 2/2] drm/i915: Wait for PSR exit before checking for vblank evasion Tarun Vyas
2018-06-26  6:31 ` ✓ Fi.CI.BAT: success for series starting with [v6,1/2] drm/i915/psr: Lockless version of psr_wait_for_idle Patchwork
2018-06-26  7:38 ` ✓ Fi.CI.IGT: " Patchwork
2018-06-26  8:26 ` [PATCH v6 1/2] " Daniel Vetter
2018-06-26  8:42   ` Chris Wilson
2018-06-26 20:36     ` Tarun Vyas
2018-06-26 19:43   ` Dhinakaran Pandiyan
2018-06-26 20:20     ` Tarun Vyas
2018-06-28  6:51       ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.