[PATCH] drm/i915/psr: Lockless version of psr_wait_for_idle

[PATCH] drm/i915/psr: Lockless version of psr_wait_for_idle

[Tarun Vyas]

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.

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

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 578346b8d7e2..a48aad0f99bf 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);
+void psr_wait_for_idle_lockless(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..425147444f69 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -757,6 +757,25 @@ void intel_psr_disable(struct intel_dp *intel_dp,
 	cancel_work_sync(&dev_priv->psr.work);
 }
 
+void psr_wait_for_idle_lockless(struct drm_i915_private *dev_priv)
+{
+	i915_reg_t reg;
+	u32 mask;
+	int err;
+
+	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;
+	}
+
+	err = intel_wait_for_register(dev_priv, reg, mask, EDP_PSR_STATUS_STATE_IDLE, 25);
+	if (err)
+		DRM_ERROR("Timed out waiting for PSR Idle for pipe update\n");
+}
+
 static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
 {
 	struct intel_dp *intel_dp;
-- 
2.13.5

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

[PATCH v3] drm/i915: Wait for PSR exit before checking for vblank evasion

[Tarun Vyas]

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.

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

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 344c0e709b19..34754771d7a7 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -107,14 +107,16 @@ 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;
 
 	if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
 		return;
 
+	psr_wait_for_idle_lockless(dev_priv);
+
+	local_irq_disable();
+
 	crtc->debug.min_vbl = min;
 	crtc->debug.max_vbl = max;
 	trace_i915_pipe_update_start(crtc);
-- 
2.13.5

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

✗ Fi.CI.BAT: failure for drm/i915/psr: Lockless version of psr_wait_for_idle (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/psr: Lockless version of psr_wait_for_idle (rev2)
URL   : https://patchwork.freedesktop.org/series/45209/
State : failure

== Summary ==

CALL    scripts/checksyscalls.sh
  DESCEND  objtool
  CHK     include/generated/compile.h
  CC [M]  drivers/gpu/drm/i915/intel_sprite.o
drivers/gpu/drm/i915/intel_sprite.c: In function ‘intel_pipe_update_start’:
drivers/gpu/drm/i915/intel_sprite.c:116:2: error: implicit declaration of function ‘psr_wait_for_idle_lockless’; did you mean ‘wait_on_page_locked’? [-Werror=implicit-function-declaration]
  psr_wait_for_idle_lockless(dev_priv);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~
  wait_on_page_locked
cc1: all warnings being treated as errors
scripts/Makefile.build:317: recipe for target 'drivers/gpu/drm/i915/intel_sprite.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_sprite.o] Error 1
scripts/Makefile.build:558: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:558: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:558: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1034: recipe for target 'drivers' failed
make: *** [drivers] Error 2

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

Re: [PATCH] drm/i915/psr: Lockless version of psr_wait_for_idle

[Dhinakaran Pandiyan]

On Thu, 2018-06-21 at 18:03 -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.
> 
> 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.
> 
> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 578346b8d7e2..a48aad0f99bf 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);
> +void psr_wait_for_idle_lockless(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..425147444f69 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -757,6 +757,25 @@ void intel_psr_disable(struct intel_dp
> *intel_dp,
>  	cancel_work_sync(&dev_priv->psr.work);
>  }
>  
> +void psr_wait_for_idle_lockless(struct drm_i915_private *dev_priv)
> +{
> +	i915_reg_t reg;
> +	u32 mask;
> +	int err;
> +
> +	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;
> +	}
> +
> +	err = intel_wait_for_register(dev_priv, reg, mask,
> EDP_PSR_STATUS_STATE_IDLE, 25);
A comment explaining the rationale for 25 ms is necessary here. 

> +	if (err)
> +		DRM_ERROR("Timed out waiting for PSR Idle for pipe
> update\n");
> +}
> +
>  static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_dp *intel_dp;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Wait for PSR exit before checking for vblank evasion

[Dhinakaran Pandiyan]

On Thu, 2018-06-21 at 18:03 -0700, Tarun Vyas wrote:
> 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.
> 
> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 344c0e709b19..34754771d7a7 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -107,14 +107,16 @@ 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;
>  
>  	if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>  		return;
>  
> +	psr_wait_for_idle_lockless(dev_priv);

Document the implicit requirement that we enable vblank interrupts
before waiting for PSR idle. Looks good to me, I'll wait for others to
chime in.

The hope is https://bugs.freedesktop.org/show_bug.cgi?id=106678 gets
fixed with this patch.

> +
> +	local_irq_disable();
> +
>  	crtc->debug.min_vbl = min;
>  	crtc->debug.max_vbl = max;
>  	trace_i915_pipe_update_start(crtc);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Wait for PSR exit before checking for vblank evasion

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5348 bytes --]

Hi Tarun,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.18-rc1 next-20180621]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Tarun-Vyas/drm-i915-Wait-for-PSR-exit-before-checking-for-vblank-evasion/20180622-090643
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x011-201824 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/intel_sprite.c: In function 'intel_pipe_update_start':
>> drivers/gpu/drm/i915/intel_sprite.c:116:2: error: implicit declaration of function 'psr_wait_for_idle_lockless'; did you mean 'wait_on_page_locked'? [-Werror=implicit-function-declaration]
     psr_wait_for_idle_lockless(dev_priv);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~
     wait_on_page_locked
   cc1: some warnings being treated as errors

vim +116 drivers/gpu/drm/i915/intel_sprite.c

    76	
    77	/**
    78	 * intel_pipe_update_start() - start update of a set of display registers
    79	 * @new_crtc_state: the new crtc state
    80	 *
    81	 * Mark the start of an update to pipe registers that should be updated
    82	 * atomically regarding vblank. If the next vblank will happens within
    83	 * the next 100 us, this function waits until the vblank passes.
    84	 *
    85	 * After a successful call to this function, interrupts will be disabled
    86	 * until a subsequent call to intel_pipe_update_end(). That is done to
    87	 * avoid random delays.
    88	 */
    89	void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
    90	{
    91		struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
    92		struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
    93		const struct drm_display_mode *adjusted_mode = &new_crtc_state->base.adjusted_mode;
    94		long timeout = msecs_to_jiffies_timeout(1);
    95		int scanline, min, max, vblank_start;
    96		wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
    97		bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
    98			intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI);
    99		DEFINE_WAIT(wait);
   100	
   101		vblank_start = adjusted_mode->crtc_vblank_start;
   102		if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
   103			vblank_start = DIV_ROUND_UP(vblank_start, 2);
   104	
   105		/* FIXME needs to be calibrated sensibly */
   106		min = vblank_start - intel_usecs_to_scanlines(adjusted_mode,
   107							      VBLANK_EVASION_TIME_US);
   108		max = vblank_start - 1;
   109	
   110		if (min <= 0 || max <= 0)
   111			return;
   112	
   113		if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
   114			return;
   115	
 > 116		psr_wait_for_idle_lockless(dev_priv);
   117	
   118		local_irq_disable();
   119	
   120		crtc->debug.min_vbl = min;
   121		crtc->debug.max_vbl = max;
   122		trace_i915_pipe_update_start(crtc);
   123	
   124		for (;;) {
   125			/*
   126			 * prepare_to_wait() has a memory barrier, which guarantees
   127			 * other CPUs can see the task state update by the time we
   128			 * read the scanline.
   129			 */
   130			prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
   131	
   132			scanline = intel_get_crtc_scanline(crtc);
   133			if (scanline < min || scanline > max)
   134				break;
   135	
   136			if (!timeout) {
   137				DRM_ERROR("Potential atomic update failure on pipe %c\n",
   138					  pipe_name(crtc->pipe));
   139				break;
   140			}
   141	
   142			local_irq_enable();
   143	
   144			timeout = schedule_timeout(timeout);
   145	
   146			local_irq_disable();
   147		}
   148	
   149		finish_wait(wq, &wait);
   150	
   151		drm_crtc_vblank_put(&crtc->base);
   152	
   153		/*
   154		 * On VLV/CHV DSI the scanline counter would appear to
   155		 * increment approx. 1/3 of a scanline before start of vblank.
   156		 * The registers still get latched at start of vblank however.
   157		 * This means we must not write any registers on the first
   158		 * line of vblank (since not the whole line is actually in
   159		 * vblank). And unfortunately we can't use the interrupt to
   160		 * wait here since it will fire too soon. We could use the
   161		 * frame start interrupt instead since it will fire after the
   162		 * critical scanline, but that would require more changes
   163		 * in the interrupt code. So for now we'll just do the nasty
   164		 * thing and poll for the bad scanline to pass us by.
   165		 *
   166		 * FIXME figure out if BXT+ DSI suffers from this as well
   167		 */
   168		while (need_vlv_dsi_wa && scanline == vblank_start)
   169			scanline = intel_get_crtc_scanline(crtc);
   170	
   171		crtc->debug.scanline_start = scanline;
   172		crtc->debug.start_vbl_time = ktime_get();
   173		crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
   174	
   175		trace_i915_pipe_update_vblank_evaded(crtc);
   176	}
   177	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34875 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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