All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/psr: Kill delays when activating psr back.
@ 2018-06-13 19:26 Rodrigo Vivi
  2018-06-13 20:00 ` ✓ Fi.CI.BAT: success for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Rodrigo Vivi @ 2018-06-13 19:26 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dhinakaran Pandiyan, Rodrigo Vivi

The immediate enabling was actually not an issue for the
HW perspective for core platforms that have HW tracking.
HW will wait few identical idle frames before transitioning
to actual psr active anyways.

Now that we removed VLV/CHV out of the picture completely
we can safely remove any delays.

Note that this patch also remove the delayed activation
on HSW and BDW introduced by commit 'd0ac896a477d
("drm/i915: Delay first PSR activation.")'. This was
introduced to fix a blank screen on VLV/CHV and also
masked some frozen screens on other core platforms.
Probably the same that we are now properly hunting and fixing.

v2:(DK): Remove unnecessary WARN_ONs and make some other
         VLV | CHV more readable.
v3: Do it regardless the timer rework.
v4: (DK/CI): Add VLV || CHV check on cancel work at psr_disable.
v5: Kill remaining items and fully rework activation functions.
v6: Rebase on top of VLV/CHV clean-up and keep the reactivation
    on a regular non-delayed work to avoid extra delays on exit
    calls and allow us to add few more safety checks before
    real activation.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  2 --
 drivers/gpu/drm/i915/i915_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_psr.c    | 29 +++++++----------------------
 3 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 769ab9745834..948b973af067 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2660,8 +2660,6 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled));
 	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
 		   dev_priv->psr.busy_frontbuffer_bits);
-	seq_printf(m, "Re-enable work scheduled: %s\n",
-		   yesno(work_busy(&dev_priv->psr.work.work)));
 
 	if (dev_priv->psr.psr2_enabled)
 		enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index be8c2f0823c4..19defe73b156 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -613,7 +613,7 @@ struct i915_psr {
 	bool sink_support;
 	struct intel_dp *enabled;
 	bool active;
-	struct delayed_work work;
+	struct work_struct work;
 	unsigned busy_frontbuffer_bits;
 	bool sink_psr2_support;
 	bool link_standby;
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 71dfe541740f..ef0f4741a95d 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -671,21 +671,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 	dev_priv->psr.enable_source(intel_dp, crtc_state);
 	dev_priv->psr.enabled = intel_dp;
 
-	if (INTEL_GEN(dev_priv) >= 9) {
-		intel_psr_activate(intel_dp);
-	} else {
-		/*
-		 * FIXME: Activation should happen immediately since this
-		 * function is just called after pipe is fully trained and
-		 * enabled.
-		 * However on some platforms we face issues when first
-		 * activation follows a modeset so quickly.
-		 *     - On HSW/BDW we get a recoverable frozen screen until
-		 *       next exit-activate sequence.
-		 */
-		schedule_delayed_work(&dev_priv->psr.work,
-				      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
-	}
+	intel_psr_activate(intel_dp);
 
 unlock:
 	mutex_unlock(&dev_priv->psr.lock);
@@ -768,8 +754,6 @@ void intel_psr_disable(struct intel_dp *intel_dp,
 
 	dev_priv->psr.enabled = NULL;
 	mutex_unlock(&dev_priv->psr.lock);
-
-	cancel_delayed_work_sync(&dev_priv->psr.work);
 }
 
 static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
@@ -805,10 +789,13 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
 static void intel_psr_work(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
-		container_of(work, typeof(*dev_priv), psr.work.work);
+		container_of(work, typeof(*dev_priv), psr.work);
 
 	mutex_lock(&dev_priv->psr.lock);
 
+	if (!dev_priv->psr.enabled)
+		goto unlock;
+
 	/*
 	 * We have to make sure PSR is ready for re-enable
 	 * otherwise it keeps disabled until next full enable/disable cycle.
@@ -949,9 +936,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
 	}
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
-		if (!work_busy(&dev_priv->psr.work.work))
-			schedule_delayed_work(&dev_priv->psr.work,
-					      msecs_to_jiffies(100));
+		schedule_work(&dev_priv->psr.work);
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
@@ -998,7 +983,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
 		dev_priv->psr.link_standby = false;
 	}
 
-	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
+	INIT_WORK(&dev_priv->psr.work, intel_psr_work);
 	mutex_init(&dev_priv->psr.lock);
 
 	dev_priv->psr.enable_source = hsw_psr_enable_source;
-- 
2.17.1

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

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

* ✓ Fi.CI.BAT: success for drm/i915/psr: Kill delays when activating psr back.
  2018-06-13 19:26 [PATCH] drm/i915/psr: Kill delays when activating psr back Rodrigo Vivi
@ 2018-06-13 20:00 ` Patchwork
  2018-06-13 23:49 ` [PATCH] " Dhinakaran Pandiyan
  2018-06-14  1:49 ` ✓ Fi.CI.IGT: success for " Patchwork
  2 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2018-06-13 20:00 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/psr: Kill delays when activating psr back.
URL   : https://patchwork.freedesktop.org/series/44715/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4311 -> Patchwork_9294 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9294 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9294, 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/44715/revisions/1/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

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

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload-inject:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106725, fdo#106248)

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

    igt@kms_flip@basic-flip-vs-modeset:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000)

    
    ==== Possible fixes ====

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

    igt@gem_exec_gttfill@basic:
      fi-byt-n2820:       FAIL (fdo#106744) -> PASS

    igt@gem_exec_suspend@basic-s4-devices:
      fi-glk-j4005:       DMESG-WARN (fdo#106097) -> PASS

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-cnl-psr:         FAIL (fdo#100368) -> PASS

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

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#105719 https://bugs.freedesktop.org/show_bug.cgi?id=105719
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#106744 https://bugs.freedesktop.org/show_bug.cgi?id=106744


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

  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan 


== Build changes ==

    * Linux: CI_DRM_4311 -> Patchwork_9294

  CI_DRM_4311: 875eebee4a444f5b7ba146754e91118ff3c11ad5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4518: e4908004547b63131352fbc0ddcdb1d3d55480e0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9294: 5191095d6bf24d10318f10ed6e2a01287730ae32 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

5191095d6bf2 drm/i915/psr: Kill delays when activating psr back.

== Logs ==

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

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

* Re: [PATCH] drm/i915/psr: Kill delays when activating psr back.
  2018-06-13 19:26 [PATCH] drm/i915/psr: Kill delays when activating psr back Rodrigo Vivi
  2018-06-13 20:00 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-06-13 23:49 ` Dhinakaran Pandiyan
  2018-06-13 23:59   ` Souza, Jose
  2018-06-18 20:21   ` Dhinakaran Pandiyan
  2018-06-14  1:49 ` ✓ Fi.CI.IGT: success for " Patchwork
  2 siblings, 2 replies; 8+ messages in thread
From: Dhinakaran Pandiyan @ 2018-06-13 23:49 UTC (permalink / raw)
  To: Rodrigo Vivi, intel-gfx

On Wed, 2018-06-13 at 12:26 -0700, Rodrigo Vivi wrote:
> The immediate enabling was actually not an issue for the
> HW perspective for core platforms that have HW tracking.
> HW will wait few identical idle frames before transitioning
> to actual psr active anyways.
> 
> Now that we removed VLV/CHV out of the picture completely
> we can safely remove any delays.
> 
> Note that this patch also remove the delayed activation
> on HSW and BDW introduced by commit 'd0ac896a477d
> ("drm/i915: Delay first PSR activation.")'. This was
> introduced to fix a blank screen on VLV/CHV and also
> masked some frozen screens on other core platforms.
> Probably the same that we are now properly hunting and fixing.
> 
> v2:(DK): Remove unnecessary WARN_ONs and make some other
>          VLV | CHV more readable.
> v3: Do it regardless the timer rework.
> v4: (DK/CI): Add VLV || CHV check on cancel work at psr_disable.
> v5: Kill remaining items and fully rework activation functions.
> v6: Rebase on top of VLV/CHV clean-up and keep the reactivation
>     on a regular non-delayed work to avoid extra delays on exit
>     calls and allow us to add few more safety checks before
>     real activation.

We have to implement this bspec step in the disable sequence now that
you are removing the delay - 
"Wait for SRD_STATUS to show SRD is Idle. This will take up to one full
frame time (1/refresh rate), plus SRD exit training time (max of 6ms),
plus SRD aux channel handshake (max of 1.5ms)."

Otherwise, we can end up re-enabling right after a disable in
psr_exit()


> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  2 --
>  drivers/gpu/drm/i915/i915_drv.h     |  2 +-
>  drivers/gpu/drm/i915/intel_psr.c    | 29 +++++++------------------
> ----
>  3 files changed, 8 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 769ab9745834..948b973af067 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2660,8 +2660,6 @@ static int i915_edp_psr_status(struct seq_file
> *m, void *data)
>  	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv-
> >psr.enabled));
>  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
>  		   dev_priv->psr.busy_frontbuffer_bits);
> -	seq_printf(m, "Re-enable work scheduled: %s\n",
> -		   yesno(work_busy(&dev_priv->psr.work.work)));
>  
>  	if (dev_priv->psr.psr2_enabled)
>  		enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index be8c2f0823c4..19defe73b156 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -613,7 +613,7 @@ struct i915_psr {
>  	bool sink_support;
>  	struct intel_dp *enabled;
>  	bool active;
> -	struct delayed_work work;
> +	struct work_struct work;
>  	unsigned busy_frontbuffer_bits;
>  	bool sink_psr2_support;
>  	bool link_standby;
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 71dfe541740f..ef0f4741a95d 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -671,21 +671,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  	dev_priv->psr.enable_source(intel_dp, crtc_state);
>  	dev_priv->psr.enabled = intel_dp;
>  
> -	if (INTEL_GEN(dev_priv) >= 9) {
> -		intel_psr_activate(intel_dp);
> -	} else {
> -		/*
> -		 * FIXME: Activation should happen immediately since
> this
> -		 * function is just called after pipe is fully
> trained and
> -		 * enabled.
> -		 * However on some platforms we face issues when
> first
> -		 * activation follows a modeset so quickly.
> -		 *     - On HSW/BDW we get a recoverable frozen
> screen until
> -		 *       next exit-activate sequence.
> -		 */
> -		schedule_delayed_work(&dev_priv->psr.work,
> -				      msecs_to_jiffies(intel_dp-
> >panel_power_cycle_delay * 5));
> -	}
> +	intel_psr_activate(intel_dp);
>  
>  unlock:
>  	mutex_unlock(&dev_priv->psr.lock);
> @@ -768,8 +754,6 @@ void intel_psr_disable(struct intel_dp *intel_dp,
>  
>  	dev_priv->psr.enabled = NULL;
>  	mutex_unlock(&dev_priv->psr.lock);
> -
> -	cancel_delayed_work_sync(&dev_priv->psr.work);
>  }
>  
>  static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> @@ -805,10 +789,13 @@ static bool psr_wait_for_idle(struct
> drm_i915_private *dev_priv)
>  static void intel_psr_work(struct work_struct *work)
>  {
>  	struct drm_i915_private *dev_priv =
> -		container_of(work, typeof(*dev_priv),
> psr.work.work);
> +		container_of(work, typeof(*dev_priv), psr.work);
>  
>  	mutex_lock(&dev_priv->psr.lock);
>  
> +	if (!dev_priv->psr.enabled)
> +		goto unlock;
> +

I thought flush_work() was missing in psr_disable(), but this check
should take care of not enabling PSR after psr_disable()


>  	/*
>  	 * We have to make sure PSR is ready for re-enable
>  	 * otherwise it keeps disabled until next full
> enable/disable cycle.
> @@ -949,9 +936,7 @@ void intel_psr_flush(struct drm_i915_private
> *dev_priv,
>  	}
>  
>  	if (!dev_priv->psr.active && !dev_priv-
> >psr.busy_frontbuffer_bits)
> -		if (!work_busy(&dev_priv->psr.work.work))
> -			schedule_delayed_work(&dev_priv->psr.work,
> -					      msecs_to_jiffies(100))
> ;
> +		schedule_work(&dev_priv->psr.work);
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> @@ -998,7 +983,7 @@ void intel_psr_init(struct drm_i915_private
> *dev_priv)
>  		dev_priv->psr.link_standby = false;
>  	}
>  
> -	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
> +	INIT_WORK(&dev_priv->psr.work, intel_psr_work);
>  	mutex_init(&dev_priv->psr.lock);
>  
>  	dev_priv->psr.enable_source = hsw_psr_enable_source;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915/psr: Kill delays when activating psr back.
  2018-06-13 23:49 ` [PATCH] " Dhinakaran Pandiyan
@ 2018-06-13 23:59   ` Souza, Jose
  2018-06-14  0:40     ` Dhinakaran Pandiyan
  2018-06-18 20:21   ` Dhinakaran Pandiyan
  1 sibling, 1 reply; 8+ messages in thread
From: Souza, Jose @ 2018-06-13 23:59 UTC (permalink / raw)
  To: intel-gfx, Vivi, Rodrigo, Pandiyan, Dhinakaran

On Wed, 2018-06-13 at 16:49 -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2018-06-13 at 12:26 -0700, Rodrigo Vivi wrote:
> > The immediate enabling was actually not an issue for the
> > HW perspective for core platforms that have HW tracking.
> > HW will wait few identical idle frames before transitioning
> > to actual psr active anyways.
> > 
> > Now that we removed VLV/CHV out of the picture completely
> > we can safely remove any delays.
> > 
> > Note that this patch also remove the delayed activation
> > on HSW and BDW introduced by commit 'd0ac896a477d
> > ("drm/i915: Delay first PSR activation.")'. This was
> > introduced to fix a blank screen on VLV/CHV and also
> > masked some frozen screens on other core platforms.
> > Probably the same that we are now properly hunting and fixing.
> > 
> > v2:(DK): Remove unnecessary WARN_ONs and make some other
> >          VLV | CHV more readable.
> > v3: Do it regardless the timer rework.
> > v4: (DK/CI): Add VLV || CHV check on cancel work at psr_disable.
> > v5: Kill remaining items and fully rework activation functions.
> > v6: Rebase on top of VLV/CHV clean-up and keep the reactivation
> >     on a regular non-delayed work to avoid extra delays on exit
> >     calls and allow us to add few more safety checks before
> >     real activation.
> 
> We have to implement this bspec step in the disable sequence now that
> you are removing the delay - 
> "Wait for SRD_STATUS to show SRD is Idle. This will take up to one
> full
> frame time (1/refresh rate), plus SRD exit training time (max of
> 6ms),
> plus SRD aux channel handshake (max of 1.5ms)."
> 
> Otherwise, we can end up re-enabling right after a disable in
> psr_exit()

It is still waiting PSR idle before renable.

> > 


> 
> 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c |  2 --
> >  drivers/gpu/drm/i915/i915_drv.h     |  2 +-
> >  drivers/gpu/drm/i915/intel_psr.c    | 29 +++++++------------------
> > ----
> >  3 files changed, 8 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 769ab9745834..948b973af067 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2660,8 +2660,6 @@ static int i915_edp_psr_status(struct
> > seq_file
> > *m, void *data)
> >  	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv-
> > > psr.enabled));
> > 
> >  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> >  		   dev_priv->psr.busy_frontbuffer_bits);
> > -	seq_printf(m, "Re-enable work scheduled: %s\n",
> > -		   yesno(work_busy(&dev_priv->psr.work.work)));
> >  
> >  	if (dev_priv->psr.psr2_enabled)
> >  		enabled = I915_READ(EDP_PSR2_CTL) &
> > EDP_PSR2_ENABLE;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index be8c2f0823c4..19defe73b156 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -613,7 +613,7 @@ struct i915_psr {
> >  	bool sink_support;
> >  	struct intel_dp *enabled;
> >  	bool active;
> > -	struct delayed_work work;
> > +	struct work_struct work;
> >  	unsigned busy_frontbuffer_bits;
> >  	bool sink_psr2_support;
> >  	bool link_standby;
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 71dfe541740f..ef0f4741a95d 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -671,21 +671,7 @@ void intel_psr_enable(struct intel_dp
> > *intel_dp,
> >  	dev_priv->psr.enable_source(intel_dp, crtc_state);
> >  	dev_priv->psr.enabled = intel_dp;
> >  
> > -	if (INTEL_GEN(dev_priv) >= 9) {
> > -		intel_psr_activate(intel_dp);
> > -	} else {
> > -		/*
> > -		 * FIXME: Activation should happen immediately
> > since
> > this
> > -		 * function is just called after pipe is fully
> > trained and
> > -		 * enabled.
> > -		 * However on some platforms we face issues when
> > first
> > -		 * activation follows a modeset so quickly.
> > -		 *     - On HSW/BDW we get a recoverable frozen
> > screen until
> > -		 *       next exit-activate sequence.
> > -		 */
> > -		schedule_delayed_work(&dev_priv->psr.work,
> > -				      msecs_to_jiffies(intel_dp-
> > > panel_power_cycle_delay * 5));
> > 
> > -	}
> > +	intel_psr_activate(intel_dp);
> >  
> >  unlock:
> >  	mutex_unlock(&dev_priv->psr.lock);
> > @@ -768,8 +754,6 @@ void intel_psr_disable(struct intel_dp
> > *intel_dp,
> >  
> >  	dev_priv->psr.enabled = NULL;
> >  	mutex_unlock(&dev_priv->psr.lock);
> > -
> > -	cancel_delayed_work_sync(&dev_priv->psr.work);
> >  }
> >  
> >  static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > @@ -805,10 +789,13 @@ static bool psr_wait_for_idle(struct
> > drm_i915_private *dev_priv)
> >  static void intel_psr_work(struct work_struct *work)
> >  {
> >  	struct drm_i915_private *dev_priv =
> > -		container_of(work, typeof(*dev_priv),
> > psr.work.work);
> > +		container_of(work, typeof(*dev_priv), psr.work);
> >  
> >  	mutex_lock(&dev_priv->psr.lock);
> >  
> > +	if (!dev_priv->psr.enabled)
> > +		goto unlock;
> > +
> 
> I thought flush_work() was missing in psr_disable(), but this check
> should take care of not enabling PSR after psr_disable()
> 
> 
> >  	/*
> >  	 * We have to make sure PSR is ready for re-enable
> >  	 * otherwise it keeps disabled until next full
> > enable/disable cycle.
> > @@ -949,9 +936,7 @@ void intel_psr_flush(struct drm_i915_private
> > *dev_priv,
> >  	}
> >  
> >  	if (!dev_priv->psr.active && !dev_priv-
> > > psr.busy_frontbuffer_bits)
> > 
> > -		if (!work_busy(&dev_priv->psr.work.work))
> > -			schedule_delayed_work(&dev_priv->psr.work,
> > -					      msecs_to_jiffies(100
> > ))
> > ;
> > +		schedule_work(&dev_priv->psr.work);
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
> >  
> > @@ -998,7 +983,7 @@ void intel_psr_init(struct drm_i915_private
> > *dev_priv)
> >  		dev_priv->psr.link_standby = false;
> >  	}
> >  
> > -	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
> > +	INIT_WORK(&dev_priv->psr.work, intel_psr_work);
> >  	mutex_init(&dev_priv->psr.lock);
> >  
> >  	dev_priv->psr.enable_source = hsw_psr_enable_source;
> 
> _______________________________________________
> 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] 8+ messages in thread

* Re: [PATCH] drm/i915/psr: Kill delays when activating psr back.
  2018-06-13 23:59   ` Souza, Jose
@ 2018-06-14  0:40     ` Dhinakaran Pandiyan
  2018-06-14 16:13       ` Rodrigo Vivi
  0 siblings, 1 reply; 8+ messages in thread
From: Dhinakaran Pandiyan @ 2018-06-14  0:40 UTC (permalink / raw)
  To: Souza, Jose, intel-gfx, Vivi, Rodrigo

On Wed, 2018-06-13 at 23:59 +0000, Souza, Jose wrote:
> On Wed, 2018-06-13 at 16:49 -0700, Dhinakaran Pandiyan wrote:
> > 
> > On Wed, 2018-06-13 at 12:26 -0700, Rodrigo Vivi wrote:
> > > 
> > > The immediate enabling was actually not an issue for the
> > > HW perspective for core platforms that have HW tracking.
> > > HW will wait few identical idle frames before transitioning
> > > to actual psr active anyways.
> > > 
> > > Now that we removed VLV/CHV out of the picture completely
> > > we can safely remove any delays.
> > > 
> > > Note that this patch also remove the delayed activation
> > > on HSW and BDW introduced by commit 'd0ac896a477d
> > > ("drm/i915: Delay first PSR activation.")'. This was
> > > introduced to fix a blank screen on VLV/CHV and also
> > > masked some frozen screens on other core platforms.
> > > Probably the same that we are now properly hunting and fixing.
> > > 
> > > v2:(DK): Remove unnecessary WARN_ONs and make some other
> > >          VLV | CHV more readable.
> > > v3: Do it regardless the timer rework.
> > > v4: (DK/CI): Add VLV || CHV check on cancel work at psr_disable.
> > > v5: Kill remaining items and fully rework activation functions.
> > > v6: Rebase on top of VLV/CHV clean-up and keep the reactivation
> > >     on a regular non-delayed work to avoid extra delays on exit
> > >     calls and allow us to add few more safety checks before
> > >     real activation.
> > We have to implement this bspec step in the disable sequence now
> > that
> > you are removing the delay - 
> > "Wait for SRD_STATUS to show SRD is Idle. This will take up to one
> > full
> > frame time (1/refresh rate), plus SRD exit training time (max of
> > 6ms),
> > plus SRD aux channel handshake (max of 1.5ms)."
> > 
> > Otherwise, we can end up re-enabling right after a disable in
> > psr_exit()
> It is still waiting PSR idle before renable.
Oh yeah, we do wait in psr_activate.
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>


> > 
> > > 
> > > 
> 
> > 
> > 
> > 
> > > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> 
> > 
> > > 
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c |  2 --
> > >  drivers/gpu/drm/i915/i915_drv.h     |  2 +-
> > >  drivers/gpu/drm/i915/intel_psr.c    | 29 +++++++--------------
> > > ----
> > > ----
> > >  3 files changed, 8 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 769ab9745834..948b973af067 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2660,8 +2660,6 @@ static int i915_edp_psr_status(struct
> > > seq_file
> > > *m, void *data)
> > >  	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv-
> > > > 
> > > > psr.enabled));
> > >  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> > >  		   dev_priv->psr.busy_frontbuffer_bits);
> > > -	seq_printf(m, "Re-enable work scheduled: %s\n",
> > > -		   yesno(work_busy(&dev_priv->psr.work.work)));
> > >  
> > >  	if (dev_priv->psr.psr2_enabled)
> > >  		enabled = I915_READ(EDP_PSR2_CTL) &
> > > EDP_PSR2_ENABLE;
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index be8c2f0823c4..19defe73b156 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -613,7 +613,7 @@ struct i915_psr {
> > >  	bool sink_support;
> > >  	struct intel_dp *enabled;
> > >  	bool active;
> > > -	struct delayed_work work;
> > > +	struct work_struct work;
> > >  	unsigned busy_frontbuffer_bits;
> > >  	bool sink_psr2_support;
> > >  	bool link_standby;
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 71dfe541740f..ef0f4741a95d 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -671,21 +671,7 @@ void intel_psr_enable(struct intel_dp
> > > *intel_dp,
> > >  	dev_priv->psr.enable_source(intel_dp, crtc_state);
> > >  	dev_priv->psr.enabled = intel_dp;
> > >  
> > > -	if (INTEL_GEN(dev_priv) >= 9) {
> > > -		intel_psr_activate(intel_dp);
> > > -	} else {
> > > -		/*
> > > -		 * FIXME: Activation should happen immediately
> > > since
> > > this
> > > -		 * function is just called after pipe is fully
> > > trained and
> > > -		 * enabled.
> > > -		 * However on some platforms we face issues when
> > > first
> > > -		 * activation follows a modeset so quickly.
> > > -		 *     - On HSW/BDW we get a recoverable frozen
> > > screen until
> > > -		 *       next exit-activate sequence.
> > > -		 */
> > > -		schedule_delayed_work(&dev_priv->psr.work,
> > > -				      msecs_to_jiffies(intel_dp-
> > > > 
> > > > panel_power_cycle_delay * 5));
> > > -	}
> > > +	intel_psr_activate(intel_dp);
> > >  
> > >  unlock:
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > > @@ -768,8 +754,6 @@ void intel_psr_disable(struct intel_dp
> > > *intel_dp,
> > >  
> > >  	dev_priv->psr.enabled = NULL;
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > > -
> > > -	cancel_delayed_work_sync(&dev_priv->psr.work);
> > >  }
> > >  
> > >  static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > > @@ -805,10 +789,13 @@ static bool psr_wait_for_idle(struct
> > > drm_i915_private *dev_priv)
> > >  static void intel_psr_work(struct work_struct *work)
> > >  {
> > >  	struct drm_i915_private *dev_priv =
> > > -		container_of(work, typeof(*dev_priv),
> > > psr.work.work);
> > > +		container_of(work, typeof(*dev_priv), psr.work);
> > >  
> > >  	mutex_lock(&dev_priv->psr.lock);
> > >  
> > > +	if (!dev_priv->psr.enabled)
> > > +		goto unlock;
> > > +
> > I thought flush_work() was missing in psr_disable(), but this check
> > should take care of not enabling PSR after psr_disable()
> > 
> > 
> > > 
> > >  	/*
> > >  	 * We have to make sure PSR is ready for re-enable
> > >  	 * otherwise it keeps disabled until next full
> > > enable/disable cycle.
> > > @@ -949,9 +936,7 @@ void intel_psr_flush(struct drm_i915_private
> > > *dev_priv,
> > >  	}
> > >  
> > >  	if (!dev_priv->psr.active && !dev_priv-
> > > > 
> > > > psr.busy_frontbuffer_bits)
> > > -		if (!work_busy(&dev_priv->psr.work.work))
> > > -			schedule_delayed_work(&dev_priv-
> > > >psr.work,
> > > -					      msecs_to_jiffies(1
> > > 00
> > > ))
> > > ;
> > > +		schedule_work(&dev_priv->psr.work);
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > >  }
> > >  
> > > @@ -998,7 +983,7 @@ void intel_psr_init(struct drm_i915_private
> > > *dev_priv)
> > >  		dev_priv->psr.link_standby = false;
> > >  	}
> > >  
> > > -	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
> > > +	INIT_WORK(&dev_priv->psr.work, intel_psr_work);
> > >  	mutex_init(&dev_priv->psr.lock);
> > >  
> > >  	dev_priv->psr.enable_source = hsw_psr_enable_source;
> > _______________________________________________
> > 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] 8+ messages in thread

* ✓ Fi.CI.IGT: success for drm/i915/psr: Kill delays when activating psr back.
  2018-06-13 19:26 [PATCH] drm/i915/psr: Kill delays when activating psr back Rodrigo Vivi
  2018-06-13 20:00 ` ✓ Fi.CI.BAT: success for " Patchwork
  2018-06-13 23:49 ` [PATCH] " Dhinakaran Pandiyan
@ 2018-06-14  1:49 ` Patchwork
  2 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2018-06-14  1:49 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/psr: Kill delays when activating psr back.
URL   : https://patchwork.freedesktop.org/series/44715/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4311_full -> Patchwork_9294_full =

== Summary - WARNING ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-bsd2:
      shard-kbl:          PASS -> SKIP

    igt@gem_mocs_settings@mocs-rc6-ctx-dirty-render:
      shard-kbl:          SKIP -> PASS

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          PASS -> INCOMPLETE (fdo#106023, fdo#103665)

    igt@gem_wait@await-bsd2:
      shard-snb:          SKIP -> INCOMPLETE (fdo#105411)

    igt@kms_vblank@pipe-c-ts-continuation-idle:
      shard-glk:          PASS -> DMESG-WARN (fdo#106247)

    
    ==== Possible fixes ====

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

    igt@kms_flip@2x-plain-flip-ts-check:
      shard-hsw:          FAIL (fdo#100368) -> PASS

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

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

    
    ==== Warnings ====

    igt@drv_selftest@live_gtt:
      shard-kbl:          FAIL (fdo#105347) -> INCOMPLETE (fdo#103665)

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  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#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106247 https://bugs.freedesktop.org/show_bug.cgi?id=106247
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


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

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4311 -> Patchwork_9294

  CI_DRM_4311: 875eebee4a444f5b7ba146754e91118ff3c11ad5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4518: e4908004547b63131352fbc0ddcdb1d3d55480e0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9294: 5191095d6bf24d10318f10ed6e2a01287730ae32 @ 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_9294/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915/psr: Kill delays when activating psr back.
  2018-06-14  0:40     ` Dhinakaran Pandiyan
@ 2018-06-14 16:13       ` Rodrigo Vivi
  0 siblings, 0 replies; 8+ messages in thread
From: Rodrigo Vivi @ 2018-06-14 16:13 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: intel-gfx

On Wed, Jun 13, 2018 at 05:40:43PM -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2018-06-13 at 23:59 +0000, Souza, Jose wrote:
> > On Wed, 2018-06-13 at 16:49 -0700, Dhinakaran Pandiyan wrote:
> > > 
> > > On Wed, 2018-06-13 at 12:26 -0700, Rodrigo Vivi wrote:
> > > > 
> > > > The immediate enabling was actually not an issue for the
> > > > HW perspective for core platforms that have HW tracking.
> > > > HW will wait few identical idle frames before transitioning
> > > > to actual psr active anyways.
> > > > 
> > > > Now that we removed VLV/CHV out of the picture completely
> > > > we can safely remove any delays.
> > > > 
> > > > Note that this patch also remove the delayed activation
> > > > on HSW and BDW introduced by commit 'd0ac896a477d
> > > > ("drm/i915: Delay first PSR activation.")'. This was
> > > > introduced to fix a blank screen on VLV/CHV and also
> > > > masked some frozen screens on other core platforms.
> > > > Probably the same that we are now properly hunting and fixing.
> > > > 
> > > > v2:(DK): Remove unnecessary WARN_ONs and make some other
> > > >          VLV | CHV more readable.
> > > > v3: Do it regardless the timer rework.
> > > > v4: (DK/CI): Add VLV || CHV check on cancel work at psr_disable.
> > > > v5: Kill remaining items and fully rework activation functions.
> > > > v6: Rebase on top of VLV/CHV clean-up and keep the reactivation
> > > >     on a regular non-delayed work to avoid extra delays on exit
> > > >     calls and allow us to add few more safety checks before
> > > >     real activation.
> > > We have to implement this bspec step in the disable sequence now
> > > that
> > > you are removing the delay - 
> > > "Wait for SRD_STATUS to show SRD is Idle. This will take up to one
> > > full
> > > frame time (1/refresh rate), plus SRD exit training time (max of
> > > 6ms),
> > > plus SRD aux channel handshake (max of 1.5ms)."
> > > 
> > > Otherwise, we can end up re-enabling right after a disable in
> > > psr_exit()
> > It is still waiting PSR idle before renable.
> Oh yeah, we do wait in psr_activate.
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> 
> 
> > > 
> > > > 
> > > > 
> > 
> > > 
> > > 
> > > 
> > > > 
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

pushed to dinq, thank you both for reviewing.

> > 
> > > 
> > > > 
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_debugfs.c |  2 --
> > > >  drivers/gpu/drm/i915/i915_drv.h     |  2 +-
> > > >  drivers/gpu/drm/i915/intel_psr.c    | 29 +++++++--------------
> > > > ----
> > > > ----
> > > >  3 files changed, 8 insertions(+), 25 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index 769ab9745834..948b973af067 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -2660,8 +2660,6 @@ static int i915_edp_psr_status(struct
> > > > seq_file
> > > > *m, void *data)
> > > >  	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv-
> > > > > 
> > > > > psr.enabled));
> > > >  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> > > >  		   dev_priv->psr.busy_frontbuffer_bits);
> > > > -	seq_printf(m, "Re-enable work scheduled: %s\n",
> > > > -		   yesno(work_busy(&dev_priv->psr.work.work)));
> > > >  
> > > >  	if (dev_priv->psr.psr2_enabled)
> > > >  		enabled = I915_READ(EDP_PSR2_CTL) &
> > > > EDP_PSR2_ENABLE;
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index be8c2f0823c4..19defe73b156 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -613,7 +613,7 @@ struct i915_psr {
> > > >  	bool sink_support;
> > > >  	struct intel_dp *enabled;
> > > >  	bool active;
> > > > -	struct delayed_work work;
> > > > +	struct work_struct work;
> > > >  	unsigned busy_frontbuffer_bits;
> > > >  	bool sink_psr2_support;
> > > >  	bool link_standby;
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 71dfe541740f..ef0f4741a95d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -671,21 +671,7 @@ void intel_psr_enable(struct intel_dp
> > > > *intel_dp,
> > > >  	dev_priv->psr.enable_source(intel_dp, crtc_state);
> > > >  	dev_priv->psr.enabled = intel_dp;
> > > >  
> > > > -	if (INTEL_GEN(dev_priv) >= 9) {
> > > > -		intel_psr_activate(intel_dp);
> > > > -	} else {
> > > > -		/*
> > > > -		 * FIXME: Activation should happen immediately
> > > > since
> > > > this
> > > > -		 * function is just called after pipe is fully
> > > > trained and
> > > > -		 * enabled.
> > > > -		 * However on some platforms we face issues when
> > > > first
> > > > -		 * activation follows a modeset so quickly.
> > > > -		 *     - On HSW/BDW we get a recoverable frozen
> > > > screen until
> > > > -		 *       next exit-activate sequence.
> > > > -		 */
> > > > -		schedule_delayed_work(&dev_priv->psr.work,
> > > > -				      msecs_to_jiffies(intel_dp-
> > > > > 
> > > > > panel_power_cycle_delay * 5));
> > > > -	}
> > > > +	intel_psr_activate(intel_dp);
> > > >  
> > > >  unlock:
> > > >  	mutex_unlock(&dev_priv->psr.lock);
> > > > @@ -768,8 +754,6 @@ void intel_psr_disable(struct intel_dp
> > > > *intel_dp,
> > > >  
> > > >  	dev_priv->psr.enabled = NULL;
> > > >  	mutex_unlock(&dev_priv->psr.lock);
> > > > -
> > > > -	cancel_delayed_work_sync(&dev_priv->psr.work);
> > > >  }
> > > >  
> > > >  static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > > > @@ -805,10 +789,13 @@ static bool psr_wait_for_idle(struct
> > > > drm_i915_private *dev_priv)
> > > >  static void intel_psr_work(struct work_struct *work)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv =
> > > > -		container_of(work, typeof(*dev_priv),
> > > > psr.work.work);
> > > > +		container_of(work, typeof(*dev_priv), psr.work);
> > > >  
> > > >  	mutex_lock(&dev_priv->psr.lock);
> > > >  
> > > > +	if (!dev_priv->psr.enabled)
> > > > +		goto unlock;
> > > > +
> > > I thought flush_work() was missing in psr_disable(), but this check
> > > should take care of not enabling PSR after psr_disable()
> > > 
> > > 
> > > > 
> > > >  	/*
> > > >  	 * We have to make sure PSR is ready for re-enable
> > > >  	 * otherwise it keeps disabled until next full
> > > > enable/disable cycle.
> > > > @@ -949,9 +936,7 @@ void intel_psr_flush(struct drm_i915_private
> > > > *dev_priv,
> > > >  	}
> > > >  
> > > >  	if (!dev_priv->psr.active && !dev_priv-
> > > > > 
> > > > > psr.busy_frontbuffer_bits)
> > > > -		if (!work_busy(&dev_priv->psr.work.work))
> > > > -			schedule_delayed_work(&dev_priv-
> > > > >psr.work,
> > > > -					      msecs_to_jiffies(1
> > > > 00
> > > > ))
> > > > ;
> > > > +		schedule_work(&dev_priv->psr.work);
> > > >  	mutex_unlock(&dev_priv->psr.lock);
> > > >  }
> > > >  
> > > > @@ -998,7 +983,7 @@ void intel_psr_init(struct drm_i915_private
> > > > *dev_priv)
> > > >  		dev_priv->psr.link_standby = false;
> > > >  	}
> > > >  
> > > > -	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
> > > > +	INIT_WORK(&dev_priv->psr.work, intel_psr_work);
> > > >  	mutex_init(&dev_priv->psr.lock);
> > > >  
> > > >  	dev_priv->psr.enable_source = hsw_psr_enable_source;
> > > _______________________________________________
> > > 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] 8+ messages in thread

* Re: [PATCH] drm/i915/psr: Kill delays when activating psr back.
  2018-06-13 23:49 ` [PATCH] " Dhinakaran Pandiyan
  2018-06-13 23:59   ` Souza, Jose
@ 2018-06-18 20:21   ` Dhinakaran Pandiyan
  1 sibling, 0 replies; 8+ messages in thread
From: Dhinakaran Pandiyan @ 2018-06-18 20:21 UTC (permalink / raw)
  To: Rodrigo Vivi, intel-gfx, José Roberto de Souza

On Wed, 2018-06-13 at 16:49 -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2018-06-13 at 12:26 -0700, Rodrigo Vivi wrote:
> > 
> > The immediate enabling was actually not an issue for the
> > HW perspective for core platforms that have HW tracking.
> > HW will wait few identical idle frames before transitioning
> > to actual psr active anyways.
> > 
> > Now that we removed VLV/CHV out of the picture completely
> > we can safely remove any delays.
> > 
> > Note that this patch also remove the delayed activation
> > on HSW and BDW introduced by commit 'd0ac896a477d
> > ("drm/i915: Delay first PSR activation.")'. This was
> > introduced to fix a blank screen on VLV/CHV and also
> > masked some frozen screens on other core platforms.
> > Probably the same that we are now properly hunting and fixing.
> > 
> > v2:(DK): Remove unnecessary WARN_ONs and make some other
> >          VLV | CHV more readable.
> > v3: Do it regardless the timer rework.
> > v4: (DK/CI): Add VLV || CHV check on cancel work at psr_disable.
> > v5: Kill remaining items and fully rework activation functions.
> > v6: Rebase on top of VLV/CHV clean-up and keep the reactivation
> >     on a regular non-delayed work to avoid extra delays on exit
> >     calls and allow us to add few more safety checks before
> >     real activation.
> We have to implement this bspec step in the disable sequence now that
> you are removing the delay - 
> "Wait for SRD_STATUS to show SRD is Idle. This will take up to one
> full
> frame time (1/refresh rate), plus SRD exit training time (max of
> 6ms),
> plus SRD aux channel handshake (max of 1.5ms)."
> 
> Otherwise, we can end up re-enabling right after a disable in
> psr_exit()
> 
> 
> > 
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c |  2 --
> >  drivers/gpu/drm/i915/i915_drv.h     |  2 +-
> >  drivers/gpu/drm/i915/intel_psr.c    | 29 +++++++------------------
> > ----
> >  3 files changed, 8 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 769ab9745834..948b973af067 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2660,8 +2660,6 @@ static int i915_edp_psr_status(struct
> > seq_file
> > *m, void *data)
> >  	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv-
> > > 
> > > psr.enabled));
> >  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> >  		   dev_priv->psr.busy_frontbuffer_bits);
> > -	seq_printf(m, "Re-enable work scheduled: %s\n",
> > -		   yesno(work_busy(&dev_priv->psr.work.work)));
> >  
> >  	if (dev_priv->psr.psr2_enabled)
> >  		enabled = I915_READ(EDP_PSR2_CTL) &
> > EDP_PSR2_ENABLE;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index be8c2f0823c4..19defe73b156 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -613,7 +613,7 @@ struct i915_psr {
> >  	bool sink_support;
> >  	struct intel_dp *enabled;
> >  	bool active;
> > -	struct delayed_work work;
> > +	struct work_struct work;
> >  	unsigned busy_frontbuffer_bits;
> >  	bool sink_psr2_support;
> >  	bool link_standby;
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 71dfe541740f..ef0f4741a95d 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -671,21 +671,7 @@ void intel_psr_enable(struct intel_dp
> > *intel_dp,
> >  	dev_priv->psr.enable_source(intel_dp, crtc_state);
> >  	dev_priv->psr.enabled = intel_dp;
> >  
> > -	if (INTEL_GEN(dev_priv) >= 9) {
> > -		intel_psr_activate(intel_dp);
> > -	} else {
> > -		/*
> > -		 * FIXME: Activation should happen immediately
> > since
> > this
> > -		 * function is just called after pipe is fully
> > trained and
> > -		 * enabled.
> > -		 * However on some platforms we face issues when
> > first
> > -		 * activation follows a modeset so quickly.
> > -		 *     - On HSW/BDW we get a recoverable frozen
> > screen until
> > -		 *       next exit-activate sequence.
> > -		 */
> > -		schedule_delayed_work(&dev_priv->psr.work,
> > -				      msecs_to_jiffies(intel_dp-
> > > 
> > > panel_power_cycle_delay * 5));
> > -	}
> > +	intel_psr_activate(intel_dp);
> >  
> >  unlock:
> >  	mutex_unlock(&dev_priv->psr.lock);
> > @@ -768,8 +754,6 @@ void intel_psr_disable(struct intel_dp
> > *intel_dp,
> >  
> >  	dev_priv->psr.enabled = NULL;
> >  	mutex_unlock(&dev_priv->psr.lock);
> > -
> > -	cancel_delayed_work_sync(&dev_priv->psr.work);
> >  }
> >  
> >  static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > @@ -805,10 +789,13 @@ static bool psr_wait_for_idle(struct
> > drm_i915_private *dev_priv)
> >  static void intel_psr_work(struct work_struct *work)
> >  {
> >  	struct drm_i915_private *dev_priv =
> > -		container_of(work, typeof(*dev_priv),
> > psr.work.work);
> > +		container_of(work, typeof(*dev_priv), psr.work);
> >  
> >  	mutex_lock(&dev_priv->psr.lock);
> >  
> > +	if (!dev_priv->psr.enabled)
> > +		goto unlock;
> > +
> I thought flush_work() was missing in psr_disable(), but this check
> should take care of not enabling PSR after psr_disable()
> 
Looks like a cancel_work_sync() is still needed,
https://bugs.freedesktop.org/show_bug.cgi?id=106948

If the work item was already scheduled and is followed by psr_disable
-> psr_enable, we'll end up hitting the psr.active warning.



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

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

end of thread, other threads:[~2018-06-18 19:56 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-13 19:26 [PATCH] drm/i915/psr: Kill delays when activating psr back Rodrigo Vivi
2018-06-13 20:00 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-06-13 23:49 ` [PATCH] " Dhinakaran Pandiyan
2018-06-13 23:59   ` Souza, Jose
2018-06-14  0:40     ` Dhinakaran Pandiyan
2018-06-14 16:13       ` Rodrigo Vivi
2018-06-18 20:21   ` Dhinakaran Pandiyan
2018-06-14  1:49 ` ✓ Fi.CI.IGT: success for " Patchwork

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.