[PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused.

[PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused.

[Tarun Vyas]

intel_pipe_update_start also needs to wait for PSR to idle
out. Need some minor modifications in psr_wait_for_idle in
order to reuse it.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index db27f2faa1de..40aafc0f4513 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -889,11 +889,15 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
 	i915_reg_t reg;
 	u32 mask;
 	int err;
+	bool wait = false;
+
+	mutex_lock(&dev_priv->psr.lock);
 
 	intel_dp = dev_priv->psr.enabled;
 	if (!intel_dp)
-		return false;
+		goto unlock;
 
+	wait = true;
 	if (HAS_DDI(dev_priv)) {
 		if (dev_priv->psr.psr2_enabled) {
 			reg = EDP_PSR2_STATUS;
@@ -911,15 +915,18 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
 		mask = VLV_EDP_PSR_IN_TRANS;
 	}
 
+unlock:
 	mutex_unlock(&dev_priv->psr.lock);
 
-	err = intel_wait_for_register(dev_priv, reg, mask, 0, 50);
-	if (err)
-		DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
+	if(wait) {
+		err = intel_wait_for_register(dev_priv, reg, mask, 0, 50);
+		if (err) {
+			DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
+			wait = false;
+		}
+	}
 
-	/* After the unlocked wait, verify that PSR is still wanted! */
-	mutex_lock(&dev_priv->psr.lock);
-	return err == 0 && dev_priv->psr.enabled;
+	return wait;
 }
 
 static void intel_psr_work(struct work_struct *work)
@@ -927,7 +934,6 @@ static void intel_psr_work(struct work_struct *work)
 	struct drm_i915_private *dev_priv =
 		container_of(work, typeof(*dev_priv), psr.work.work);
 
-	mutex_lock(&dev_priv->psr.lock);
 
 	/*
 	 * We have to make sure PSR is ready for re-enable
@@ -936,14 +942,15 @@ static void intel_psr_work(struct work_struct *work)
 	 * and be ready for re-enable.
 	 */
 	if (!psr_wait_for_idle(dev_priv))
-		goto unlock;
+		return;
 
-	/*
+	/* After the unlocked wait, verify that PSR is still wanted!
 	 * The delayed work can race with an invalidate hence we need to
 	 * recheck. Since psr_flush first clears this and then reschedules we
 	 * won't ever miss a flush when bailing out here.
 	 */
-	if (dev_priv->psr.busy_frontbuffer_bits)
+	mutex_lock(&dev_priv->psr.lock);
+	if (dev_priv->psr.enabled && dev_priv->psr.busy_frontbuffer_bits)
 		goto unlock;
 
 	intel_psr_activate(dev_priv->psr.enabled);
-- 
2.13.5

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

[PATCH 2/2] drm/i915: Un-statify psr_wait_for_idle

[Tarun Vyas]

We have new users (follow up patch). So, un-statify it

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 1 +
 drivers/gpu/drm/i915/intel_psr.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d7dbca1aabff..d8f8282f1fad 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1919,6 +1919,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);
+bool 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 40aafc0f4513..e69859fa782f 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -883,7 +883,7 @@ void intel_psr_disable(struct intel_dp *intel_dp,
 	cancel_delayed_work_sync(&dev_priv->psr.work);
 }
 
-static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
+bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
 {
 	struct intel_dp *intel_dp;
 	i915_reg_t reg;
-- 
2.13.5

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

[PATCH v2] 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 supported and active
on the current pipe) before reading the PIPEDSL, b/c if we haven't
fully exited PSR, then checking for vblank evasion isn't actually
applicable.

This scenario applies to a configuration with an additional pipe,
as of now

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

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index ee23613f9fd4..481d310e5c3b 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -107,14 +107,17 @@ 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;
 
+	if(new_crtc_state->has_psr && dev_priv->psr.active)
+		psr_wait_for_idle(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

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

[Chris Wilson]

Quoting Tarun Vyas (2018-05-14 21:49:22)
> 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 supported and active
> on the current pipe) before reading the PIPEDSL, b/c if we haven't
> fully exited PSR, then checking for vblank evasion isn't actually
> applicable.
> 
> This scenario applies to a configuration with an additional pipe,
> as of now
> 
> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ee23613f9fd4..481d310e5c3b 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -107,14 +107,17 @@ 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;
>  
> +       if(new_crtc_state->has_psr && dev_priv->psr.active)
> +               psr_wait_for_idle(dev_priv);
> +
> +       local_irq_disable();

Pop quiz, does intel_pipe_update_finish() unconditionally assume it is
called with irqs disabled?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tarun Vyas]

On Mon, May 14, 2018 at 10:16:38PM +0100, Chris Wilson wrote:
> Quoting Tarun Vyas (2018-05-14 21:49:22)
> > 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 supported and active
> > on the current pipe) before reading the PIPEDSL, b/c if we haven't
> > fully exited PSR, then checking for vblank evasion isn't actually
> > applicable.
> > 
> > This scenario applies to a configuration with an additional pipe,
> > as of now
> > 
> > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index ee23613f9fd4..481d310e5c3b 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -107,14 +107,17 @@ 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;
> >  
> > +       if(new_crtc_state->has_psr && dev_priv->psr.active)
> > +               psr_wait_for_idle(dev_priv);
> > +
> > +       local_irq_disable();
> 
> Pop quiz, does intel_pipe_update_finish() unconditionally assume it is
> called with irqs disabled?
> -Chris
Unless local_irq_disable() fails, intel_pipe_update_end() should always get called with irqs disabled, from what it looks like to me.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5370 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.17-rc5 next-20180514]
[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/20180515-103355
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x000-201819 (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=i386 

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:117:3: error: implicit declaration of function 'psr_wait_for_idle'; did you mean 'apic_wait_icr_idle'? [-Werror=implicit-function-declaration]
      psr_wait_for_idle(dev_priv);
      ^~~~~~~~~~~~~~~~~
      apic_wait_icr_idle
   cc1: some warnings being treated as errors

vim +117 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		if(new_crtc_state->has_psr && dev_priv->psr.active)
 > 117			psr_wait_for_idle(dev_priv);
   118	
   119		local_irq_disable();
   120	
   121		crtc->debug.min_vbl = min;
   122		crtc->debug.max_vbl = max;
   123		trace_i915_pipe_update_start(crtc);
   124	
   125		for (;;) {
   126			/*
   127			 * prepare_to_wait() has a memory barrier, which guarantees
   128			 * other CPUs can see the task state update by the time we
   129			 * read the scanline.
   130			 */
   131			prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
   132	
   133			scanline = intel_get_crtc_scanline(crtc);
   134			if (scanline < min || scanline > max)
   135				break;
   136	
   137			if (!timeout) {
   138				DRM_ERROR("Potential atomic update failure on pipe %c\n",
   139					  pipe_name(crtc->pipe));
   140				break;
   141			}
   142	
   143			local_irq_enable();
   144	
   145			timeout = schedule_timeout(timeout);
   146	
   147			local_irq_disable();
   148		}
   149	
   150		finish_wait(wq, &wait);
   151	
   152		drm_crtc_vblank_put(&crtc->base);
   153	
   154		/*
   155		 * On VLV/CHV DSI the scanline counter would appear to
   156		 * increment approx. 1/3 of a scanline before start of vblank.
   157		 * The registers still get latched at start of vblank however.
   158		 * This means we must not write any registers on the first
   159		 * line of vblank (since not the whole line is actually in
   160		 * vblank). And unfortunately we can't use the interrupt to
   161		 * wait here since it will fire too soon. We could use the
   162		 * frame start interrupt instead since it will fire after the
   163		 * critical scanline, but that would require more changes
   164		 * in the interrupt code. So for now we'll just do the nasty
   165		 * thing and poll for the bad scanline to pass us by.
   166		 *
   167		 * FIXME figure out if BXT+ DSI suffers from this as well
   168		 */
   169		while (need_vlv_dsi_wa && scanline == vblank_start)
   170			scanline = intel_get_crtc_scanline(crtc);
   171	
   172		crtc->debug.scanline_start = scanline;
   173		crtc->debug.start_vbl_time = ktime_get();
   174		crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
   175	
   176		trace_i915_pipe_update_vblank_evaded(crtc);
   177	}
   178	

---
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: 32668 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

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

[Dhinakaran Pandiyan]

On Mon, 2018-05-14 at 13:49 -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 supported and
> active
> on the current pipe) before reading the PIPEDSL, b/c if we haven't
> fully exited PSR, then checking for vblank evasion isn't actually
> applicable.
> 
> This scenario applies to a configuration with an additional pipe,
> as of now
> 
> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index ee23613f9fd4..481d310e5c3b 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -107,14 +107,17 @@ 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;
>  
> +	if(new_crtc_state->has_psr && dev_priv->psr.active)
> +		psr_wait_for_idle(dev_priv);

How about just waiting for PSR_STATUS to idle without grabbing any
locks or checking whether PSR is active?

Status should be idle if PSR was disabled or on it's way to becoming
idle if it was enabled. Even if PSR did get enabled while we are in
pipe_update_start(), it will not be active as long as VBIs are enabled.



> +
> +	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 v2] drm/i915: Wait for PSR exit before checking for vblank evasion

[Dhinakaran Pandiyan]

On Tue, 2018-06-19 at 14:27 -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-05-14 at 13:49 -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 supported and
> > active
> > on the current pipe) before reading the PIPEDSL, b/c if we haven't
> > fully exited PSR, then checking for vblank evasion isn't actually
> > applicable.
> > 
> > This scenario applies to a configuration with an additional pipe,
> > as of now
> > 
> > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index ee23613f9fd4..481d310e5c3b 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct
> > intel_crtc_state *new_crtc_state)
> >  						      VBLANK_EVASI
> > ON
> > _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;
> >  
> > +	if(new_crtc_state->has_psr && dev_priv->psr.active)
> > +		psr_wait_for_idle(dev_priv);
> How about just waiting for PSR_STATUS to idle without grabbing any
> locks or checking whether PSR is active?
> 
> Status should be idle if PSR was disabled or on it's way to becoming
> idle if it was enabled. Even if PSR did get enabled while we are in
> pipe_update_start(), it will not be active as long as VBIs are
> enabled.
> 
Correct me if this was already considered, why not wait until the
scanline counter starts moving? I see we have a 
	intel_wait_for_pipe_scanline_moving(crtc) that's used when the
pipe is enabled.

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

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

[Tarun Vyas]

On Tue, Jun 19, 2018 at 02:54:07PM -0700, Dhinakaran Pandiyan wrote:
> On Tue, 2018-06-19 at 14:27 -0700, Dhinakaran Pandiyan wrote:
> > On Mon, 2018-05-14 at 13:49 -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 supported and
> > > active
> > > on the current pipe) before reading the PIPEDSL, b/c if we haven't
> > > fully exited PSR, then checking for vblank evasion isn't actually
> > > applicable.
> > > 
> > > This scenario applies to a configuration with an additional pipe,
> > > as of now
> > > 
> > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_sprite.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > index ee23613f9fd4..481d310e5c3b 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct
> > > intel_crtc_state *new_crtc_state)
> > >  						      VBLANK_EVASI
> > > ON
> > > _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;
> > >  
> > > +	if(new_crtc_state->has_psr && dev_priv->psr.active)
> > > +		psr_wait_for_idle(dev_priv);
> > How about just waiting for PSR_STATUS to idle without grabbing any
> > locks or checking whether PSR is active?
> > 
> > Status should be idle if PSR was disabled or on it's way to becoming
> > idle if it was enabled. Even if PSR did get enabled while we are in
> > pipe_update_start(), it will not be active as long as VBIs are
> > enabled.
> > 
Right, if we are OK with some duplication (of psr_wait_for_idle) inside intel_psr.c, then we can duplicate the PSR2 vs. PSR check that's being done in psr_wait_for_idle and then just wait without grabbing any locks, so essentially a lockless version of psr_wait_for_idle()
> Correct me if this was already considered, why not wait until the
> scanline counter starts moving? I see we have a 
> 	intel_wait_for_pipe_scanline_moving(crtc) that's used when the
> pipe is enabled.
> 
> -DK

Didn't consider this before, but, pipe_scanline_is_moving waits for a minimum of 5 msec. Are we OK with a min wait of 5 msec inside pipe_update_start ? Heuristically, waiting for PSR idle has almost always returned within < 2 msec. Occasionally it takes upto 1 full frame. 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dhinakaran Pandiyan]

On Tue, 2018-06-19 at 14:59 -0700, Tarun Vyas wrote:
> On Tue, Jun 19, 2018 at 02:54:07PM -0700, Dhinakaran Pandiyan wrote:
> > 
> > On Tue, 2018-06-19 at 14:27 -0700, Dhinakaran Pandiyan wrote:
> > > 
> > > On Mon, 2018-05-14 at 13:49 -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 supported
> > > > and
> > > > active
> > > > on the current pipe) before reading the PIPEDSL, b/c if we
> > > > haven't
> > > > fully exited PSR, then checking for vblank evasion isn't
> > > > actually
> > > > applicable.
> > > > 
> > > > This scenario applies to a configuration with an additional
> > > > pipe,
> > > > as of now
> > > > 
> > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_sprite.c | 7 +++++--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > > index ee23613f9fd4..481d310e5c3b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct
> > > > intel_crtc_state *new_crtc_state)
> > > >  						      VBLANK_E
> > > > VASI
> > > > ON
> > > > _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;
> > > >  
> > > > +	if(new_crtc_state->has_psr && dev_priv->psr.active)
> > > > +		psr_wait_for_idle(dev_priv);
> > > How about just waiting for PSR_STATUS to idle without grabbing
> > > any
> > > locks or checking whether PSR is active?
> > > 
> > > Status should be idle if PSR was disabled or on it's way to
> > > becoming
> > > idle if it was enabled. Even if PSR did get enabled while we are
> > > in
> > > pipe_update_start(), it will not be active as long as VBIs are
> > > enabled.
> > > 
> Right, if we are OK with some duplication (of psr_wait_for_idle)
> inside intel_psr.c, then we can duplicate the PSR2 vs. PSR check
> that's being done in psr_wait_for_idle and then just wait without
> grabbing any locks, so essentially a lockless version of
> psr_wait_for_idle()

Yeah, you can extract the wait into psr_wait_for_idle_locked() 

> > 
> > Correct me if this was already considered, why not wait until the
> > scanline counter starts moving? I see we have a 
> > 	intel_wait_for_pipe_scanline_moving(crtc) that's used when the
> > pipe is enabled.
> > 
> > -DK
> Didn't consider this before, but, pipe_scanline_is_moving waits for a
> minimum of 5 msec. Are we OK with a min wait of 5 msec inside
> pipe_update_start ? Heuristically, waiting for PSR idle has almost
> always returned within < 2 msec. Occasionally it takes upto 1 full
> frame.

We should be able to change that function.
 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tarun Vyas]

On Tue, Jun 19, 2018 at 02:59:54PM -0700, Tarun Vyas wrote:
> On Tue, Jun 19, 2018 at 02:54:07PM -0700, Dhinakaran Pandiyan wrote:
> > On Tue, 2018-06-19 at 14:27 -0700, Dhinakaran Pandiyan wrote:
> > > On Mon, 2018-05-14 at 13:49 -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 supported and
> > > > active
> > > > on the current pipe) before reading the PIPEDSL, b/c if we haven't
> > > > fully exited PSR, then checking for vblank evasion isn't actually
> > > > applicable.
> > > > 
> > > > This scenario applies to a configuration with an additional pipe,
> > > > as of now
> > > > 
> > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_sprite.c | 7 +++++--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > > index ee23613f9fd4..481d310e5c3b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct
> > > > intel_crtc_state *new_crtc_state)
> > > >  						      VBLANK_EVASI
> > > > ON
> > > > _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;
> > > >  
> > > > +	if(new_crtc_state->has_psr && dev_priv->psr.active)
> > > > +		psr_wait_for_idle(dev_priv);
> > > How about just waiting for PSR_STATUS to idle without grabbing any
> > > locks or checking whether PSR is active?
> > > 
> > > Status should be idle if PSR was disabled or on it's way to becoming
> > > idle if it was enabled. Even if PSR did get enabled while we are in
> > > pipe_update_start(), it will not be active as long as VBIs are
> > > enabled.
> > > 
> Right, if we are OK with some duplication (of psr_wait_for_idle) inside intel_psr.c, then we can duplicate the PSR2 vs. PSR check that's being done in psr_wait_for_idle and then just wait without grabbing any locks, so essentially a lockless version of psr_wait_for_idle()
> > Correct me if this was already considered, why not wait until the
> > scanline counter starts moving? I see we have a 
> > 	intel_wait_for_pipe_scanline_moving(crtc) that's used when the
> > pipe is enabled.
> > 
> > -DK
> 
> Didn't consider this before, but, pipe_scanline_is_moving waits for a minimum of 5 msec. Are we OK with a min wait of 5 msec inside pipe_update_start ? Heuristically, waiting for PSR idle has almost always returned within < 2 msec. Occasionally it takes upto 1 full frame.
As per some preliminary measurements
Approach 1:
	Wait *unconditionally* (so no need to check for PSR enabled/disabled and hence no locks) for PSR_STATUS to IDLE out. 
This takes ~7msec when PSR is active and ~2 usec when PSR is inactive/disabled.

Approach 2:
	Use intel_wait_for_pipe_scanline_moving to wait for PIPEDSL to start moving after PSR exit. Currently, this ends up waiting for a minimum of 5 msec but I changed this to accept a caller defined value for the delay.
After the above changes, this approach takes ~7msec when PSR is active and ~25-40 usec with PSR disabled, b/c we still need to check for at least 10+ usec and see if PIPEDSL moved, if it did, we wait for longer, otherwise we move on.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused.

[Chris Wilson]

Quoting Tarun Vyas (2018-05-14 21:49:20)
> intel_pipe_update_start also needs to wait for PSR to idle
> out. Need some minor modifications in psr_wait_for_idle in
> order to reuse it.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index db27f2faa1de..40aafc0f4513 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -889,11 +889,15 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
>         i915_reg_t reg;
>         u32 mask;
>         int err;
> +       bool wait = false;
> +
> +       mutex_lock(&dev_priv->psr.lock);
>  
>         intel_dp = dev_priv->psr.enabled;
>         if (!intel_dp)
> -               return false;
> +               goto unlock;
>  
> +       wait = true;
>         if (HAS_DDI(dev_priv)) {
>                 if (dev_priv->psr.psr2_enabled) {
>                         reg = EDP_PSR2_STATUS;
> @@ -911,15 +915,18 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
>                 mask = VLV_EDP_PSR_IN_TRANS;
>         }
>  
> +unlock:
>         mutex_unlock(&dev_priv->psr.lock);
>  
> -       err = intel_wait_for_register(dev_priv, reg, mask, 0, 50);
> -       if (err)
> -               DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> +       if(wait) {
> +               err = intel_wait_for_register(dev_priv, reg, mask, 0, 50);
> +               if (err) {
> +                       DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> +                       wait = false;
> +               }
> +       }
>  
> -       /* After the unlocked wait, verify that PSR is still wanted! */
> -       mutex_lock(&dev_priv->psr.lock);
> -       return err == 0 && dev_priv->psr.enabled;
> +       return wait;
>  }
>  
>  static void intel_psr_work(struct work_struct *work)
> @@ -927,7 +934,6 @@ static void intel_psr_work(struct work_struct *work)
>         struct drm_i915_private *dev_priv =
>                 container_of(work, typeof(*dev_priv), psr.work.work);
>  
> -       mutex_lock(&dev_priv->psr.lock);
>  
>         /*
>          * We have to make sure PSR is ready for re-enable
> @@ -936,14 +942,15 @@ static void intel_psr_work(struct work_struct *work)
>          * and be ready for re-enable.
>          */
>         if (!psr_wait_for_idle(dev_priv))
> -               goto unlock;
> +               return;
>  
> -       /*
> +       /* After the unlocked wait, verify that PSR is still wanted!
>          * The delayed work can race with an invalidate hence we need to
>          * recheck. Since psr_flush first clears this and then reschedules we
>          * won't ever miss a flush when bailing out here.
>          */
> -       if (dev_priv->psr.busy_frontbuffer_bits)
> +       mutex_lock(&dev_priv->psr.lock);
> +       if (dev_priv->psr.enabled && dev_priv->psr.busy_frontbuffer_bits)
>                 goto unlock;

I'm not sold on the locking dropping here, doing so inside the wait is
bad enough. (And do we need to there anyway?)

Since you need to introduce intel_psr_wait_for_idle() anyway, how about

void intel_psr_wait_for_idle(...)
{
	mutex_lock(&i915->psr.lock);
	psr_wait_for_idle();
	mutex_unlock(&i915->psr.lock);
}
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused.

[Tarun Vyas]

On Mon, May 14, 2018 at 10:15:19PM +0100, Chris Wilson wrote:
> Quoting Tarun Vyas (2018-05-14 21:49:20)
> > intel_pipe_update_start also needs to wait for PSR to idle
> > out. Need some minor modifications in psr_wait_for_idle in
> > order to reuse it.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 29 ++++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index db27f2faa1de..40aafc0f4513 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -889,11 +889,15 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> >         i915_reg_t reg;
> >         u32 mask;
> >         int err;
> > +       bool wait = false;
> > +
> > +       mutex_lock(&dev_priv->psr.lock);
> >  
> >         intel_dp = dev_priv->psr.enabled;
> >         if (!intel_dp)
> > -               return false;
> > +               goto unlock;
> >  
> > +       wait = true;
> >         if (HAS_DDI(dev_priv)) {
> >                 if (dev_priv->psr.psr2_enabled) {
> >                         reg = EDP_PSR2_STATUS;
> > @@ -911,15 +915,18 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> >                 mask = VLV_EDP_PSR_IN_TRANS;
> >         }
> >  
> > +unlock:
> >         mutex_unlock(&dev_priv->psr.lock);
> >  
> > -       err = intel_wait_for_register(dev_priv, reg, mask, 0, 50);
> > -       if (err)
> > -               DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> > +       if(wait) {
> > +               err = intel_wait_for_register(dev_priv, reg, mask, 0, 50);
> > +               if (err) {
> > +                       DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> > +                       wait = false;
> > +               }
> > +       }
> >  
> > -       /* After the unlocked wait, verify that PSR is still wanted! */
> > -       mutex_lock(&dev_priv->psr.lock);
> > -       return err == 0 && dev_priv->psr.enabled;
> > +       return wait;
I wanted to avoid taking this additional lock b/c all we need inside intel_pipe_update_start is for PSR to go idle. So can we retain moving it to intel_psr_work ?
> >  }
> >  
> >  static void intel_psr_work(struct work_struct *work)
> > @@ -927,7 +934,6 @@ static void intel_psr_work(struct work_struct *work)
> >         struct drm_i915_private *dev_priv =
> >                 container_of(work, typeof(*dev_priv), psr.work.work);
> >  
> > -       mutex_lock(&dev_priv->psr.lock);
> >  
> >         /*
> >          * We have to make sure PSR is ready for re-enable
> > @@ -936,14 +942,15 @@ static void intel_psr_work(struct work_struct *work)
> >          * and be ready for re-enable.
> >          */
> >         if (!psr_wait_for_idle(dev_priv))
> > -               goto unlock;
> > +               return;
> >  
> > -       /*
> > +       /* After the unlocked wait, verify that PSR is still wanted!
> >          * The delayed work can race with an invalidate hence we need to
> >          * recheck. Since psr_flush first clears this and then reschedules we
> >          * won't ever miss a flush when bailing out here.
> >          */
> > -       if (dev_priv->psr.busy_frontbuffer_bits)
> > +       mutex_lock(&dev_priv->psr.lock);
> > +       if (dev_priv->psr.enabled && dev_priv->psr.busy_frontbuffer_bits)
> >                 goto unlock;
> 
> I'm not sold on the locking dropping here, doing so inside the wait is
> bad enough. (And do we need to there anyway?)
> 
Thanks for the comments Chris.
In that case, as suggested by Rodrigo, can we assert that the lock is held, inside psr_wait_for_idle() ?
> Since you need to introduce intel_psr_wait_for_idle() anyway, how about
> 
> void intel_psr_wait_for_idle(...)
> {
> 	mutex_lock(&i915->psr.lock);
> 	psr_wait_for_idle();
> 	mutex_unlock(&i915->psr.lock);
> }
> -Chris
>>        /* After the unlocked wait, verify that PSR is still wanted! */
>>	mutex_lock(&dev_priv->psr.lock); 
>>	return err == 0 && dev_priv->psr.enabled; 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused.

[Tarun Vyas]

On Mon, May 14, 2018 at 03:00:15PM -0700, Tarun Vyas wrote:
> On Mon, May 14, 2018 at 10:15:19PM +0100, Chris Wilson wrote:
> > Quoting Tarun Vyas (2018-05-14 21:49:20)
> > > intel_pipe_update_start also needs to wait for PSR to idle
> > > out. Need some minor modifications in psr_wait_for_idle in
> > > order to reuse it.
> > > 
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 29 ++++++++++++++++++-----------
> > >  1 file changed, 18 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > index db27f2faa1de..40aafc0f4513 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -889,11 +889,15 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > >         i915_reg_t reg;
> > >         u32 mask;
> > >         int err;
> > > +       bool wait = false;
> > > +
> > > +       mutex_lock(&dev_priv->psr.lock);
> > >  
> > >         intel_dp = dev_priv->psr.enabled;
> > >         if (!intel_dp)
> > > -               return false;
> > > +               goto unlock;
> > >  
> > > +       wait = true;
> > >         if (HAS_DDI(dev_priv)) {
> > >                 if (dev_priv->psr.psr2_enabled) {
> > >                         reg = EDP_PSR2_STATUS;
> > > @@ -911,15 +915,18 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > >                 mask = VLV_EDP_PSR_IN_TRANS;
> > >         }
> > >  
> > > +unlock:
> > >         mutex_unlock(&dev_priv->psr.lock);
> > >  
> > > -       err = intel_wait_for_register(dev_priv, reg, mask, 0, 50);
> > > -       if (err)
> > > -               DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> > > +       if(wait) {
> > > +               err = intel_wait_for_register(dev_priv, reg, mask, 0, 50);
> > > +               if (err) {
> > > +                       DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> > > +                       wait = false;
> > > +               }
> > > +       }
> > >  
> > > -       /* After the unlocked wait, verify that PSR is still wanted! */
> > > -       mutex_lock(&dev_priv->psr.lock);
> > > -       return err == 0 && dev_priv->psr.enabled;
> > > +       return wait;
> I wanted to avoid taking this additional lock b/c all we need inside intel_pipe_update_start is for PSR to go idle. So can we retain moving it to intel_psr_work ?
> > >  }
> > >  
> > >  static void intel_psr_work(struct work_struct *work)
> > > @@ -927,7 +934,6 @@ static void intel_psr_work(struct work_struct *work)
> > >         struct drm_i915_private *dev_priv =
> > >                 container_of(work, typeof(*dev_priv), psr.work.work);
> > >  
> > > -       mutex_lock(&dev_priv->psr.lock);
> > >  
> > >         /*
> > >          * We have to make sure PSR is ready for re-enable
> > > @@ -936,14 +942,15 @@ static void intel_psr_work(struct work_struct *work)
> > >          * and be ready for re-enable.
> > >          */
> > >         if (!psr_wait_for_idle(dev_priv))
> > > -               goto unlock;
> > > +               return;
> > >  
> > > -       /*
> > > +       /* After the unlocked wait, verify that PSR is still wanted!
> > >          * The delayed work can race with an invalidate hence we need to
> > >          * recheck. Since psr_flush first clears this and then reschedules we
> > >          * won't ever miss a flush when bailing out here.
> > >          */
> > > -       if (dev_priv->psr.busy_frontbuffer_bits)
> > > +       mutex_lock(&dev_priv->psr.lock);
> > > +       if (dev_priv->psr.enabled && dev_priv->psr.busy_frontbuffer_bits)
> > >                 goto unlock;
> > 
> > I'm not sold on the locking dropping here, doing so inside the wait is
> > bad enough. (And do we need to there anyway?)
> > 
Per the commit message in "daeb725e drm/i915/psr: Chase psr.enabled only under the psr.lock",
the wait_for_register is done after dropping the locks so that we don't block the modeset path.
> > Since you need to introduce intel_psr_wait_for_idle() anyway, how about
> > 
> > void intel_psr_wait_for_idle(...)
> > {
> > 	mutex_lock(&i915->psr.lock);
> > 	psr_wait_for_idle();
> > 	mutex_unlock(&i915->psr.lock);
On a second thought, I need to wait for PSR idle inside intel_pipe_update_start. Now, intel_psr_disable/intel_psr_enable will
*not race* inside intel_pipe_update_start(), so we do not need any psr.locks, at all.
The locking dropping inside psr_wait_for_idle makes it difficult to move the common wait code, so I was thinking if I can do
something like:
void intel_psr_wait_for_idle(...)
{
	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;
	}
	
	intel_wait_for_register(dev_priv, reg, mask, 0, 25);
}
which is essentially a lockless version of psr_wait_for_idle(). So, there is code duplication here, but I can't help it.

Any thoughts ?
> > }
> > -Chris
> >>        /* After the unlocked wait, verify that PSR is still wanted! */
> >>	mutex_lock(&dev_priv->psr.lock); 
> >>	return err == 0 && dev_priv->psr.enabled; 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [v2] drm/i915: Wait for PSR exit before checking for vblank evasion (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2] drm/i915: Wait for PSR exit before checking for vblank evasion (rev2)
URL   : https://patchwork.freedesktop.org/series/43155/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
2e41c5eabfce drm/i915: Wait for PSR exit before checking for vblank evasion
-:48: ERROR:SPACING: space required before the open parenthesis '('
#48: FILE: drivers/gpu/drm/i915/intel_sprite.c:116:
+	if(new_crtc_state->has_psr && dev_priv->psr.active)

total: 1 errors, 0 warnings, 0 checks, 19 lines checked
199b120ec46b drm/i915: Un-statify psr_wait_for_idle

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

✗ Fi.CI.SPARSE: warning for series starting with [v2] drm/i915: Wait for PSR exit before checking for vblank evasion (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2] drm/i915: Wait for PSR exit before checking for vblank evasion (rev2)
URL   : https://patchwork.freedesktop.org/series/43155/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915: Wait for PSR exit before checking for vblank evasion
+   ^~~~~~~~~~~~~~~~~
+   apic_wait_icr_idle
+cc1: all warnings being treated as errors
+drivers/gpu/drm/i915/intel_sprite.c:117:17: error: undefined identifier 'psr_wait_for_idle'
+drivers/gpu/drm/i915/intel_sprite.c:117:34: warning: call with no type!
+drivers/gpu/drm/i915/intel_sprite.c:117:3: error: implicit declaration of function ‘psr_wait_for_idle’; did you mean ‘apic_wait_icr_idle’? [-Werror=implicit-function-declaration]
+drivers/gpu/drm/i915/intel_sprite.c: In function ‘intel_pipe_update_start’:
+make[1]: *** [drivers/gpu/drm/i915] Error 2
+make[2]: *** [drivers/gpu/drm/i915/intel_sprite.o] Error 1
+make: *** [drivers/gpu/drm/] Error 2
+   psr_wait_for_idle(dev_priv);

Commit: drm/i915: Un-statify psr_wait_for_idle
-   ^~~~~~~~~~~~~~~~~
-   apic_wait_icr_idle
-cc1: all warnings being treated as errors
-drivers/gpu/drm/i915/intel_sprite.c:117:17: error: undefined identifier 'psr_wait_for_idle'
-drivers/gpu/drm/i915/intel_sprite.c:117:34: warning: call with no type!
-drivers/gpu/drm/i915/intel_sprite.c:117:3: error: implicit declaration of function ‘psr_wait_for_idle’; did you mean ‘apic_wait_icr_idle’? [-Werror=implicit-function-declaration]
-drivers/gpu/drm/i915/intel_sprite.c: In function ‘intel_pipe_update_start’:
-make[1]: *** [drivers/gpu/drm/i915] Error 2
-make[2]: *** [drivers/gpu/drm/i915/intel_sprite.o] Error 1
-make[2]: *** Waiting for unfinished jobs....
-make: *** [drivers/gpu/drm/] Error 2
-   psr_wait_for_idle(dev_priv);

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

✗ Fi.CI.BAT: failure for series starting with [v2] drm/i915: Wait for PSR exit before checking for vblank evasion (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2] drm/i915: Wait for PSR exit before checking for vblank evasion (rev2)
URL   : https://patchwork.freedesktop.org/series/43155/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4179 -> Patchwork_8998 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_8998 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8998, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/43155/revisions/2/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_psr_sink_crc@psr_basic:
      fi-kbl-r:           PASS -> INCOMPLETE
      fi-skl-6600u:       PASS -> INCOMPLETE
      fi-kbl-7560u:       PASS -> INCOMPLETE
      fi-cfl-s3:          PASS -> INCOMPLETE
      fi-cfl-u:           PASS -> INCOMPLETE

    
    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_psr_sink_crc@psr_basic:
      fi-cnl-y3:          PASS -> INCOMPLETE (fdo#105086)
      fi-cnl-psr:         PASS -> INCOMPLETE (fdo#105086)

    
    ==== Possible fixes ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       DMESG-WARN (fdo#105128) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2600:        DMESG-WARN (fdo#102365) -> PASS

    
  fdo#102365 https://bugs.freedesktop.org/show_bug.cgi?id=102365
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#105086 https://bugs.freedesktop.org/show_bug.cgi?id=105086
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128


== Participating hosts (41 -> 37) ==

  Additional (1): fi-byt-j1900 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4179 -> Patchwork_8998

  CI_DRM_4179: be6d36ea8d6130f54ab5ec816555f1a46bd95f7b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4479: 89ae332745e31a075747a63ac5acc5baccf75769 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8998: 199b120ec46b363ca9d0d44db972a0b1ad3779f7 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4479: 3ba0657bff4216d1ec7179935590261855f1651e @ git://anongit.freedesktop.org/piglit


== Linux commits ==

199b120ec46b drm/i915: Un-statify psr_wait_for_idle
2e41c5eabfce drm/i915: Wait for PSR exit before checking for vblank evasion

== Logs ==

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