All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/psr: Kill scheduled work for Core platforms.
@ 2018-03-09  1:17 Rodrigo Vivi
  2018-03-09  1:42 ` ✗ Fi.CI.BAT: failure for " Patchwork
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Rodrigo Vivi @ 2018-03-09  1:17 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dhinakaran Pandiyan, Rodrigo Vivi

The immediate enabling is 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.

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.

Furthermore, if we stop using delayed activation on core
platforms we will be able, on following up patches,
to use available workarounds to make HW tracking properly
exit PSR instead of the big nuke of disabling psr and
re-enable on exit and activate respectively.
At least on few reliable cases.

v2:(DK): Remove unnecessary WARN_ONs and make some other
	 VLV | CHV more readable.
v3: Do it regardless the timer rework.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 17 +++++++++--------
 drivers/gpu/drm/i915/intel_psr.c    | 22 ++++++++++++++--------
 2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e838c765b251..2fd6f98364d3 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2572,15 +2572,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
 	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 (HAS_DDI(dev_priv)) {
-		if (dev_priv->psr.psr2_support)
-			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
-		else
-			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
-	} else {
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+		seq_printf(m, "Re-enable work scheduled: %s\n",
+			   yesno(work_busy(&dev_priv->psr.work.work)));
+
 		for_each_pipe(dev_priv, pipe) {
 			enum transcoder cpu_transcoder =
 				intel_pipe_to_cpu_transcoder(dev_priv, pipe);
@@ -2599,6 +2595,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 
 			intel_display_power_put(dev_priv, power_domain);
 		}
+	} else {
+		if (dev_priv->psr.psr2_support)
+			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
+		else
+			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
 	}
 
 	seq_printf(m, "Main link in standby mode: %s\n",
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index b0286722a72f..9705d89c5ebf 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -652,9 +652,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 {
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		/*
 		 * FIXME: Activation should happen immediately since this
 		 * function is just called after pipe is fully trained and
@@ -667,6 +665,8 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 		 */
 		schedule_delayed_work(&dev_priv->psr.work,
 				      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
+	} else {
+		intel_psr_activate(intel_dp);
 	}
 
 unlock:
@@ -1045,10 +1045,15 @@ 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));
+	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) {
+		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+			if (!work_busy(&dev_priv->psr.work.work))
+				schedule_delayed_work(&dev_priv->psr.work,
+						      msecs_to_jiffies(100));
+		} else {
+			intel_psr_activate(dev_priv->psr.enabled);
+		}
+	}
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
@@ -1095,7 +1100,8 @@ 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);
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
 	mutex_init(&dev_priv->psr.lock);
 
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
-- 
2.13.6

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

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

* ✗ Fi.CI.BAT: failure for drm/i915/psr: Kill scheduled work for Core platforms.
  2018-03-09  1:17 [PATCH] drm/i915/psr: Kill scheduled work for Core platforms Rodrigo Vivi
@ 2018-03-09  1:42 ` Patchwork
  2018-03-12 18:15   ` Pandiyan, Dhinakaran
  2018-03-12 19:49 ` ✗ Fi.CI.BAT: warning " Patchwork
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Patchwork @ 2018-03-09  1:42 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/psr: Kill scheduled work for Core platforms.
URL   : https://patchwork.freedesktop.org/series/39650/
State : failure

== Summary ==

Applying: drm/i915/psr: Kill scheduled work for Core platforms.
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_psr.c).
error: could not build fake ancestor
Patch failed at 0001 drm/i915/psr: Kill scheduled work for Core platforms.
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

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

* Re: ✗ Fi.CI.BAT: failure for drm/i915/psr: Kill scheduled work for Core platforms.
  2018-03-09  1:42 ` ✗ Fi.CI.BAT: failure for " Patchwork
@ 2018-03-12 18:15   ` Pandiyan, Dhinakaran
  2018-03-12 18:54     ` Rodrigo Vivi
  0 siblings, 1 reply; 13+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-12 18:15 UTC (permalink / raw)
  To: intel-gfx; +Cc: Vivi, Rodrigo

On Fri, 2018-03-09 at 01:42 +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915/psr: Kill scheduled work for Core platforms.
> URL   : https://patchwork.freedesktop.org/series/39650/
> State : failure
> 
> == Summary ==
> 
> Applying: drm/i915/psr: Kill scheduled work for Core platforms.
> error: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_psr.c).
> error: could not build fake ancestor
> Patch failed at 0001 drm/i915/psr: Kill scheduled work for Core platforms.

Was this supposed to be applied on another patch?


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

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

* Re: ✗ Fi.CI.BAT: failure for drm/i915/psr: Kill scheduled work for Core platforms.
  2018-03-12 18:15   ` Pandiyan, Dhinakaran
@ 2018-03-12 18:54     ` Rodrigo Vivi
  0 siblings, 0 replies; 13+ messages in thread
From: Rodrigo Vivi @ 2018-03-12 18:54 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran; +Cc: intel-gfx

On Mon, Mar 12, 2018 at 11:15:12AM -0700, Pandiyan, Dhinakaran wrote:
> On Fri, 2018-03-09 at 01:42 +0000, Patchwork wrote:
> > == Series Details ==
> > 
> > Series: drm/i915/psr: Kill scheduled work for Core platforms.
> > URL   : https://patchwork.freedesktop.org/series/39650/
> > State : failure
> > 
> > == Summary ==
> > 
> > Applying: drm/i915/psr: Kill scheduled work for Core platforms.
> > error: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_psr.c).
> > error: could not build fake ancestor
> > Patch failed at 0001 drm/i915/psr: Kill scheduled work for Core platforms.
> 
> Was this supposed to be applied on another patch?

the only patch I had here was the one I just merged.
So triggering a retest here. If that doesn't help I will
send a local v2 that I already have here but
that is identical to this anyways :/

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

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

* ✗ Fi.CI.BAT: warning for drm/i915/psr: Kill scheduled work for Core platforms.
  2018-03-09  1:17 [PATCH] drm/i915/psr: Kill scheduled work for Core platforms Rodrigo Vivi
  2018-03-09  1:42 ` ✗ Fi.CI.BAT: failure for " Patchwork
@ 2018-03-12 19:49 ` Patchwork
  2018-03-12 21:54 ` [PATCH] " Pandiyan, Dhinakaran
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-03-12 19:49 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/psr: Kill scheduled work for Core platforms.
URL   : https://patchwork.freedesktop.org/series/39650/
State : warning

== Summary ==

Series 39650v1 drm/i915/psr: Kill scheduled work for Core platforms.
https://patchwork.freedesktop.org/api/1.0/series/39650/revisions/1/mbox/

---- Possible new issues:

Test kms_psr_sink_crc:
        Subgroup psr_basic:
                pass       -> DMESG-WARN (fi-cfl-s2)
                pass       -> DMESG-WARN (fi-glk-1)
                pass       -> DMESG-WARN (fi-kbl-r)
                pass       -> DMESG-WARN (fi-skl-6600u)
                pass       -> DMESG-WARN (fi-skl-6700hq)

---- Known issues:

Test drv_module_reload:
        Subgroup basic-reload-inject:
                incomplete -> PASS       (fi-bwr-2160) fdo#105268
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                incomplete -> PASS       (fi-bdw-5557u) fdo#104162
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575

fdo#105268 https://bugs.freedesktop.org/show_bug.cgi?id=105268
fdo#104162 https://bugs.freedesktop.org/show_bug.cgi?id=104162
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:438s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:434s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:383s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:532s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:298s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:507s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:508s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:507s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:496s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:409s
fi-cfl-s2        total:288  pass:261  dwarn:1   dfail:0   fail:0   skip:26  time:579s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:426s
fi-gdg-551       total:288  pass:180  dwarn:0   dfail:0   fail:0   skip:108 time:315s
fi-glk-1         total:288  pass:259  dwarn:1   dfail:0   fail:0   skip:28  time:532s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:403s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:419s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:472s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:426s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:473s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:471s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:512s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:642s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:444s
fi-skl-6600u     total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:530s
fi-skl-6700hq    total:288  pass:261  dwarn:1   dfail:0   fail:0   skip:26  time:542s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:497s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:490s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:429s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:433s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:536s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:400s
Blacklisted hosts:
fi-cfl-u         total:288  pass:261  dwarn:1   dfail:0   fail:0   skip:26  time:509s
fi-cnl-drrs      total:288  pass:257  dwarn:3   dfail:0   fail:0   skip:28  time:514s
fi-glk-j5005 failed to collect. IGT log at Patchwork_8312/fi-glk-j5005/run0.log

59d9b08494470514939c3db146db674d79c5d572 drm-tip: 2018y-03m-12d-18h-44m-09s UTC integration manifest
ea6a8409770b drm/i915/psr: Kill scheduled work for Core platforms.

== Logs ==

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

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

* Re: [PATCH] drm/i915/psr: Kill scheduled work for Core platforms.
  2018-03-09  1:17 [PATCH] drm/i915/psr: Kill scheduled work for Core platforms Rodrigo Vivi
  2018-03-09  1:42 ` ✗ Fi.CI.BAT: failure for " Patchwork
  2018-03-12 19:49 ` ✗ Fi.CI.BAT: warning " Patchwork
@ 2018-03-12 21:54 ` Pandiyan, Dhinakaran
  2018-03-13 22:23   ` [RFC v5] " Rodrigo Vivi
  2018-03-13 23:51 ` ✓ Fi.CI.BAT: success for drm/i915/psr: Kill scheduled work for Core platforms. (rev2) Patchwork
  2018-03-14  2:37 ` ✓ Fi.CI.IGT: " Patchwork
  4 siblings, 1 reply; 13+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-12 21:54 UTC (permalink / raw)
  To: Vivi, Rodrigo; +Cc: intel-gfx




On Thu, 2018-03-08 at 17:17 -0800, Rodrigo Vivi wrote:
> The immediate enabling is 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.
> 
> 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.
> 
> Furthermore, if we stop using delayed activation on core
> platforms we will be able, on following up patches,
> to use available workarounds to make HW tracking properly
> exit PSR instead of the big nuke of disabling psr and
> re-enable on exit and activate respectively.
> At least on few reliable cases.
> 
> v2:(DK): Remove unnecessary WARN_ONs and make some other
> 	 VLV | CHV more readable.
> v3: Do it regardless the timer rework.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 17 +++++++++--------
>  drivers/gpu/drm/i915/intel_psr.c    | 22 ++++++++++++++--------
>  2 files changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index e838c765b251..2fd6f98364d3 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2572,15 +2572,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
>  	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 (HAS_DDI(dev_priv)) {
> -		if (dev_priv->psr.psr2_support)
> -			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> -		else
> -			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> -	} else {
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +		seq_printf(m, "Re-enable work scheduled: %s\n",
> +			   yesno(work_busy(&dev_priv->psr.work.work)));
> +
>  		for_each_pipe(dev_priv, pipe) {
>  			enum transcoder cpu_transcoder =
>  				intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> @@ -2599,6 +2595,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  
>  			intel_display_power_put(dev_priv, power_domain);
>  		}
> +	} else {
> +		if (dev_priv->psr.psr2_support)
> +			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> +		else
> +			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
>  	}
>  
>  	seq_printf(m, "Main link in standby mode: %s\n",
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index b0286722a72f..9705d89c5ebf 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -652,9 +652,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 {
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>  		/*
>  		 * FIXME: Activation should happen immediately since this
>  		 * function is just called after pipe is fully trained and
> @@ -667,6 +665,8 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  		 */
>  		schedule_delayed_work(&dev_priv->psr.work,
>  				      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
> +	} else {
> +		intel_psr_activate(intel_dp);
>  	}
>  

I think we should split this into a separate patch.

>  unlock:
> @@ -1045,10 +1045,15 @@ 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));
> +	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) {
> +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +			if (!work_busy(&dev_priv->psr.work.work))


These conditions have become extremely hard to read, we need to rework
them into functions and/or re-evaluate some of the conditions. I don't
follow the logic every time I read it after a couple of days.


For instance, if frontbuffer_bits == 0, why do we need to
activate/schedule psr. Isn't that a nop for this function?


> +				schedule_delayed_work(&dev_priv->psr.work,
> +						      msecs_to_jiffies(100));
> +		} else {
> +			intel_psr_activate(dev_priv->psr.enabled);
> +		}
> +	}
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> @@ -1095,7 +1100,8 @@ 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);
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +		INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
>  	mutex_init(&dev_priv->psr.lock);
>  
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [RFC v5] drm/i915/psr: Kill scheduled work for Core platforms.
  2018-03-12 21:54 ` [PATCH] " Pandiyan, Dhinakaran
@ 2018-03-13 22:23   ` Rodrigo Vivi
  2018-03-14 20:24     ` Pandiyan, Dhinakaran
  0 siblings, 1 reply; 13+ messages in thread
From: Rodrigo Vivi @ 2018-03-13 22:23 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dhinakaran Pandiyan, Rodrigo Vivi

The immediate enabling is 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.

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.

Furthermore, if we stop using delayed activation on core
platforms we will be able, on following up patches,
to use available workarounds to make HW tracking properly
exit PSR instead of the big nuke of disabling psr and
re-enable on exit and activate respectively.
At least on few reliable cases.

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.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 972014b2497d..7dfada863f9c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2567,15 +2567,14 @@ 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 (HAS_DDI(dev_priv)) {
-		if (dev_priv->psr.psr2_support)
-			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
-		else
-			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
-	} else {
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+		bool scheduled;
+
+		scheduled = work_busy(&dev_priv->psr.vlv_activate_work.work);
+		seq_printf(m, "Re-enable work scheduled: %s\n",
+			   yesno(scheduled));
+
 		for_each_pipe(dev_priv, pipe) {
 			enum transcoder cpu_transcoder =
 				intel_pipe_to_cpu_transcoder(dev_priv, pipe);
@@ -2594,6 +2593,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 
 			intel_display_power_put(dev_priv, power_domain);
 		}
+	} else {
+		if (dev_priv->psr.psr2_support)
+			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
+		else
+			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
 	}
 
 	seq_printf(m, "Main link in standby mode: %s\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 74b0e9d8ff62..409997f29e07 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -598,7 +598,7 @@ struct i915_psr {
 	bool sink_support;
 	struct intel_dp *enabled;
 	bool active;
-	struct delayed_work work;
+	struct delayed_work vlv_activate_work;
 	unsigned busy_frontbuffer_bits;
 	bool psr2_support;
 	bool aux_frame_sync;
@@ -613,7 +613,6 @@ struct i915_psr {
 	void (*disable_source)(struct intel_dp *,
 			       const struct intel_crtc_state *);
 	void (*enable_sink)(struct intel_dp *);
-	void (*activate)(struct intel_dp *);
 	void (*setup_vsc)(struct intel_dp *, const struct intel_crtc_state *);
 };
 
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 317cb4a12693..bc7b94a06417 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -311,24 +311,6 @@ static void vlv_psr_enable_source(struct intel_dp *intel_dp,
 		   VLV_EDP_PSR_ENABLE);
 }
 
-static void vlv_psr_activate(struct intel_dp *intel_dp)
-{
-	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = dig_port->base.base.dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct drm_crtc *crtc = dig_port->base.base.crtc;
-	enum pipe pipe = to_intel_crtc(crtc)->pipe;
-
-	/*
-	 * Let's do the transition from PSR_state 1 (inactive) to
-	 * PSR_state 2 (transition to active - static frame transmission).
-	 * Then Hardware is responsible for the transition to
-	 * PSR_state 3 (active - no Remote Frame Buffer (RFB) update).
-	 */
-	I915_WRITE(VLV_PSRCTL(pipe), I915_READ(VLV_PSRCTL(pipe)) |
-		   VLV_EDP_PSR_ACTIVE_ENTRY);
-}
-
 static void hsw_activate_psr1(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
@@ -434,16 +416,25 @@ static void hsw_psr_activate(struct intel_dp *intel_dp)
 	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
+	WARN_ON(dev_priv->psr.active);
+	lockdep_assert_held(&dev_priv->psr.lock);
+
 	/* On HSW+ after we enable PSR on source it will activate it
 	 * as soon as it match configure idle_frame count. So
 	 * we just actually enable it here on activation time.
 	 */
+	if (dev_priv->psr.psr2_support)
+		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
+	else
+		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
 
 	/* psr1 and psr2 are mutually exclusive.*/
 	if (dev_priv->psr.psr2_support)
 		hsw_activate_psr2(intel_dp);
 	else
 		hsw_activate_psr1(intel_dp);
+
+	dev_priv->psr.active = true;
 }
 
 static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
@@ -568,15 +559,20 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	if (dev_priv->psr.psr2_support)
-		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
-	else
-		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
-	WARN_ON(dev_priv->psr.active);
-	lockdep_assert_held(&dev_priv->psr.lock);
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+		/*
+		 * FIXME: VLV/CHV has many problems, but worst being blank
+		 * screens on first activation. Let's pick the worst case
+		 * delay for now.
+		 */
+		int delay = 1500;
 
-	dev_priv->psr.activate(intel_dp);
-	dev_priv->psr.active = true;
+		if (!work_busy(&dev_priv->psr.vlv_activate_work.work))
+			schedule_delayed_work(&dev_priv->psr.vlv_activate_work,
+					      msecs_to_jiffies(delay));
+	} else {
+		hsw_psr_activate(intel_dp);
+	}
 }
 
 static void hsw_psr_enable_source(struct intel_dp *intel_dp,
@@ -652,22 +648,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 VLV/CHV we get bank screen on first activation
-		 *     - 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);
@@ -786,13 +767,14 @@ 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);
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		cancel_delayed_work_sync(&dev_priv->psr.vlv_activate_work);
 }
 
-static void intel_psr_work(struct work_struct *work)
+static void vlv_psr_activate_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.vlv_activate_work.work);
 	struct intel_dp *intel_dp = dev_priv->psr.enabled;
 	struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc;
 	enum pipe pipe = to_intel_crtc(crtc)->pipe;
@@ -802,35 +784,13 @@ static void intel_psr_work(struct work_struct *work)
 	 * PSR might take some time to get fully disabled
 	 * and be ready for re-enable.
 	 */
-	if (HAS_DDI(dev_priv)) {
-		if (dev_priv->psr.psr2_support) {
-			if (intel_wait_for_register(dev_priv,
-						    EDP_PSR2_STATUS,
-						    EDP_PSR2_STATUS_STATE_MASK,
-						    0,
-						    50)) {
-				DRM_ERROR("Timed out waiting for PSR2 Idle for re-enable\n");
-				return;
-			}
-		} else {
-			if (intel_wait_for_register(dev_priv,
-						    EDP_PSR_STATUS,
-						    EDP_PSR_STATUS_STATE_MASK,
-						    0,
-						    50)) {
-				DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
-				return;
-			}
-		}
-	} else {
-		if (intel_wait_for_register(dev_priv,
-					    VLV_PSRSTAT(pipe),
-					    VLV_EDP_PSR_IN_TRANS,
-					    0,
-					    1)) {
-			DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
-			return;
-		}
+	if (intel_wait_for_register(dev_priv,
+				    VLV_PSRSTAT(pipe),
+				    VLV_EDP_PSR_IN_TRANS,
+				    0,
+				    1)) {
+		DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
+		return;
 	}
 	mutex_lock(&dev_priv->psr.lock);
 	intel_dp = dev_priv->psr.enabled;
@@ -846,8 +806,17 @@ static void intel_psr_work(struct work_struct *work)
 	if (dev_priv->psr.busy_frontbuffer_bits)
 		goto unlock;
 
-	intel_psr_activate(intel_dp);
-unlock:
+	/*
+	 * Let's do the transition from PSR_state 1 (inactive) to
+	 * PSR_state 2 (transition to active - static frame transmission).
+	 * Then Hardware is responsible for the transition to
+	 * PSR_state 3 (active - no Remote Frame Buffer (RFB) update).
+	 */
+	I915_WRITE(VLV_PSRCTL(pipe), I915_READ(VLV_PSRCTL(pipe)) |
+		   VLV_EDP_PSR_ACTIVE_ENTRY);
+	dev_priv->psr.active = true;
+
+ unlock:
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
@@ -1021,6 +990,9 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
 	if (dev_priv->psr.has_hw_tracking && origin == ORIGIN_FLIP)
 		return;
 
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		cancel_delayed_work_sync(&dev_priv->psr.vlv_activate_work);
+
 	mutex_lock(&dev_priv->psr.lock);
 	if (!dev_priv->psr.enabled) {
 		mutex_unlock(&dev_priv->psr.lock);
@@ -1053,9 +1025,8 @@ 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));
+		intel_psr_activate(dev_priv->psr.enabled);
+
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
@@ -1102,21 +1073,20 @@ 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);
 	mutex_init(&dev_priv->psr.lock);
 
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+		INIT_DELAYED_WORK(&dev_priv->psr.vlv_activate_work,
+				  vlv_psr_activate_work);
 		dev_priv->psr.enable_source = vlv_psr_enable_source;
 		dev_priv->psr.disable_source = vlv_psr_disable;
 		dev_priv->psr.enable_sink = vlv_psr_enable_sink;
-		dev_priv->psr.activate = vlv_psr_activate;
 		dev_priv->psr.setup_vsc = vlv_psr_setup_vsc;
 	} else {
 		dev_priv->psr.has_hw_tracking = true;
 		dev_priv->psr.enable_source = hsw_psr_enable_source;
 		dev_priv->psr.disable_source = hsw_psr_disable;
 		dev_priv->psr.enable_sink = hsw_psr_enable_sink;
-		dev_priv->psr.activate = hsw_psr_activate;
 		dev_priv->psr.setup_vsc = hsw_psr_setup_vsc;
 	}
 }
-- 
2.13.6

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

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

* ✓ Fi.CI.BAT: success for drm/i915/psr: Kill scheduled work for Core platforms. (rev2)
  2018-03-09  1:17 [PATCH] drm/i915/psr: Kill scheduled work for Core platforms Rodrigo Vivi
                   ` (2 preceding siblings ...)
  2018-03-12 21:54 ` [PATCH] " Pandiyan, Dhinakaran
@ 2018-03-13 23:51 ` Patchwork
  2018-03-14  2:37 ` ✓ Fi.CI.IGT: " Patchwork
  4 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-03-13 23:51 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/psr: Kill scheduled work for Core platforms. (rev2)
URL   : https://patchwork.freedesktop.org/series/39650/
State : success

== Summary ==

Series 39650v2 drm/i915/psr: Kill scheduled work for Core platforms.
https://patchwork.freedesktop.org/api/1.0/series/39650/revisions/2/mbox/

---- Known issues:

Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> FAIL       (fi-ilk-650) fdo#104008

fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:430s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:435s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:384s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:539s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:300s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:509s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:512s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:510s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:492s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:409s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:577s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:590s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:421s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:315s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:530s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:404s
fi-ilk-650       total:288  pass:227  dwarn:0   dfail:0   fail:1   skip:60  time:422s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:470s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:431s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:472s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:469s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:520s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:452s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:533s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:536s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:500s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:485s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:428s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:438s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:527s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:402s
Blacklisted hosts:
fi-cfl-u         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:513s
fi-cnl-drrs      total:288  pass:257  dwarn:3   dfail:0   fail:0   skip:28  time:547s

c30d96215a171b633dae0098d1e62a2556c8bf90 drm-tip: 2018y-03m-13d-22h-35m-59s UTC integration manifest
eb8fe170731f drm/i915/psr: Kill scheduled work for Core platforms.

== Logs ==

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

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

* ✓ Fi.CI.IGT: success for drm/i915/psr: Kill scheduled work for Core platforms. (rev2)
  2018-03-09  1:17 [PATCH] drm/i915/psr: Kill scheduled work for Core platforms Rodrigo Vivi
                   ` (3 preceding siblings ...)
  2018-03-13 23:51 ` ✓ Fi.CI.BAT: success for drm/i915/psr: Kill scheduled work for Core platforms. (rev2) Patchwork
@ 2018-03-14  2:37 ` Patchwork
  4 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-03-14  2:37 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/psr: Kill scheduled work for Core platforms. (rev2)
URL   : https://patchwork.freedesktop.org/series/39650/
State : success

== Summary ==

---- Known issues:

Test kms_atomic_transition:
        Subgroup 1x-modeset-transitions:
                pass       -> FAIL       (shard-apl) fdo#103207
Test kms_cursor_crc:
        Subgroup cursor-64x64-suspend:
                pass       -> SKIP       (shard-hsw) fdo#103540
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                fail       -> PASS       (shard-apl) fdo#103375 +1
Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-apl) fdo#99912
Test kms_vblank:
        Subgroup pipe-b-ts-continuation-suspend:
                skip       -> PASS       (shard-snb) fdo#105411

fdo#103207 https://bugs.freedesktop.org/show_bug.cgi?id=103207
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411

shard-apl        total:3444 pass:1815 dwarn:1   dfail:0   fail:7   skip:1620 time:12996s
shard-hsw        total:3444 pass:1769 dwarn:1   dfail:0   fail:1   skip:1672 time:11706s
shard-snb        total:3444 pass:1360 dwarn:1   dfail:0   fail:2   skip:2081 time:7182s
Blacklisted hosts:
shard-kbl        total:3360 pass:1898 dwarn:1   dfail:0   fail:9   skip:1451 time:9595s

== Logs ==

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

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

* Re: [RFC v5] drm/i915/psr: Kill scheduled work for Core platforms.
  2018-03-13 22:23   ` [RFC v5] " Rodrigo Vivi
@ 2018-03-14 20:24     ` Pandiyan, Dhinakaran
  2018-03-14 20:47       ` Rodrigo Vivi
  0 siblings, 1 reply; 13+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-14 20:24 UTC (permalink / raw)
  To: Vivi, Rodrigo; +Cc: intel-gfx

On Tue, 2018-03-13 at 15:23 -0700, Rodrigo Vivi wrote:
> The immediate enabling is 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.
> 
> 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.
> 
> Furthermore, if we stop using delayed activation on core
> platforms we will be able, on following up patches,
> to use available workarounds to make HW tracking properly
> exit PSR instead of the big nuke of disabling psr and
> re-enable on exit and activate respectively.
> At least on few reliable cases.
> 
> 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.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  20 +++---
>  drivers/gpu/drm/i915/i915_drv.h     |   3 +-
>  drivers/gpu/drm/i915/intel_psr.c    | 134 ++++++++++++++----------------------
>  3 files changed, 65 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 972014b2497d..7dfada863f9c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2567,15 +2567,14 @@ 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 (HAS_DDI(dev_priv)) {
> -		if (dev_priv->psr.psr2_support)
> -			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> -		else
> -			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> -	} else {
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +		bool scheduled;
> +
> +		scheduled = work_busy(&dev_priv->psr.vlv_activate_work.work);
> +		seq_printf(m, "Re-enable work scheduled: %s\n",
> +			   yesno(scheduled));
> +
>  		for_each_pipe(dev_priv, pipe) {
>  			enum transcoder cpu_transcoder =
>  				intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> @@ -2594,6 +2593,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  
>  			intel_display_power_put(dev_priv, power_domain);
>  		}
> +	} else {
> +		if (dev_priv->psr.psr2_support)
> +			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> +		else
> +			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
>  	}
>  
>  	seq_printf(m, "Main link in standby mode: %s\n",
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 74b0e9d8ff62..409997f29e07 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -598,7 +598,7 @@ struct i915_psr {
>  	bool sink_support;
>  	struct intel_dp *enabled;
>  	bool active;
> -	struct delayed_work work;
> +	struct delayed_work vlv_activate_work;
>  	unsigned busy_frontbuffer_bits;
>  	bool psr2_support;
>  	bool aux_frame_sync;
> @@ -613,7 +613,6 @@ struct i915_psr {
>  	void (*disable_source)(struct intel_dp *,
>  			       const struct intel_crtc_state *);
>  	void (*enable_sink)(struct intel_dp *);
> -	void (*activate)(struct intel_dp *);
>  	void (*setup_vsc)(struct intel_dp *, const struct intel_crtc_state *);
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 317cb4a12693..bc7b94a06417 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -311,24 +311,6 @@ static void vlv_psr_enable_source(struct intel_dp *intel_dp,
>  		   VLV_EDP_PSR_ENABLE);
>  }
>  
> -static void vlv_psr_activate(struct intel_dp *intel_dp)
> -{
> -	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> -	struct drm_device *dev = dig_port->base.base.dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct drm_crtc *crtc = dig_port->base.base.crtc;
> -	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> -
> -	/*
> -	 * Let's do the transition from PSR_state 1 (inactive) to
> -	 * PSR_state 2 (transition to active - static frame transmission).
> -	 * Then Hardware is responsible for the transition to
> -	 * PSR_state 3 (active - no Remote Frame Buffer (RFB) update).
> -	 */
> -	I915_WRITE(VLV_PSRCTL(pipe), I915_READ(VLV_PSRCTL(pipe)) |
> -		   VLV_EDP_PSR_ACTIVE_ENTRY);
> -}
> -
>  static void hsw_activate_psr1(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> @@ -434,16 +416,25 @@ static void hsw_psr_activate(struct intel_dp *intel_dp)
>  	struct drm_device *dev = dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
> +	WARN_ON(dev_priv->psr.active);
> +	lockdep_assert_held(&dev_priv->psr.lock);
> +
>  	/* On HSW+ after we enable PSR on source it will activate it
>  	 * as soon as it match configure idle_frame count. So
>  	 * we just actually enable it here on activation time.
>  	 */
> +	if (dev_priv->psr.psr2_support)
> +		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> +	else
> +		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>  
>  	/* psr1 and psr2 are mutually exclusive.*/
>  	if (dev_priv->psr.psr2_support)
>  		hsw_activate_psr2(intel_dp);
>  	else
>  		hsw_activate_psr1(intel_dp);

nit: The branches can be grouped together.

> +
> +	dev_priv->psr.active = true;
>  }
>  
>  static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> @@ -568,15 +559,20 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
> -	if (dev_priv->psr.psr2_support)
> -		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> -	else
> -		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> -	WARN_ON(dev_priv->psr.active);
> -	lockdep_assert_held(&dev_priv->psr.lock);
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +		/*
> +		 * FIXME: VLV/CHV has many problems, but worst being blank
> +		 * screens on first activation. Let's pick the worst case
> +		 * delay for now.
> +		 */
> +		int delay = 1500;
>  
> -	dev_priv->psr.activate(intel_dp);
> -	dev_priv->psr.active = true;
> +		if (!work_busy(&dev_priv->psr.vlv_activate_work.work))

Since the work is already cancelled, do we need this?

The patch looks good to me, fewer indirect calls and easier to read.


> +			schedule_delayed_work(&dev_priv->psr.vlv_activate_work,
> +					      msecs_to_jiffies(delay));
> +	} else {
> +		hsw_psr_activate(intel_dp);
> +	}
>  }
>  
>  static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> @@ -652,22 +648,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 VLV/CHV we get bank screen on first activation
> -		 *     - 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);
> @@ -786,13 +767,14 @@ 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);
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +		cancel_delayed_work_sync(&dev_priv->psr.vlv_activate_work);
>  }
>  
> -static void intel_psr_work(struct work_struct *work)
> +static void vlv_psr_activate_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.vlv_activate_work.work);
>  	struct intel_dp *intel_dp = dev_priv->psr.enabled;
>  	struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc;
>  	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> @@ -802,35 +784,13 @@ static void intel_psr_work(struct work_struct *work)
>  	 * PSR might take some time to get fully disabled
>  	 * and be ready for re-enable.
>  	 */
> -	if (HAS_DDI(dev_priv)) {
> -		if (dev_priv->psr.psr2_support) {
> -			if (intel_wait_for_register(dev_priv,
> -						    EDP_PSR2_STATUS,
> -						    EDP_PSR2_STATUS_STATE_MASK,
> -						    0,
> -						    50)) {
> -				DRM_ERROR("Timed out waiting for PSR2 Idle for re-enable\n");
> -				return;
> -			}
> -		} else {
> -			if (intel_wait_for_register(dev_priv,
> -						    EDP_PSR_STATUS,
> -						    EDP_PSR_STATUS_STATE_MASK,
> -						    0,
> -						    50)) {
> -				DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> -				return;
> -			}
> -		}
> -	} else {
> -		if (intel_wait_for_register(dev_priv,
> -					    VLV_PSRSTAT(pipe),
> -					    VLV_EDP_PSR_IN_TRANS,
> -					    0,
> -					    1)) {
> -			DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> -			return;
> -		}
> +	if (intel_wait_for_register(dev_priv,
> +				    VLV_PSRSTAT(pipe),
> +				    VLV_EDP_PSR_IN_TRANS,
> +				    0,
> +				    1)) {
> +		DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> +		return;
>  	}
>  	mutex_lock(&dev_priv->psr.lock);
>  	intel_dp = dev_priv->psr.enabled;
> @@ -846,8 +806,17 @@ static void intel_psr_work(struct work_struct *work)
>  	if (dev_priv->psr.busy_frontbuffer_bits)
>  		goto unlock;
>  
> -	intel_psr_activate(intel_dp);
> -unlock:
> +	/*
> +	 * Let's do the transition from PSR_state 1 (inactive) to
> +	 * PSR_state 2 (transition to active - static frame transmission).
> +	 * Then Hardware is responsible for the transition to
> +	 * PSR_state 3 (active - no Remote Frame Buffer (RFB) update).
> +	 */
> +	I915_WRITE(VLV_PSRCTL(pipe), I915_READ(VLV_PSRCTL(pipe)) |
> +		   VLV_EDP_PSR_ACTIVE_ENTRY);
> +	dev_priv->psr.active = true;
> +
> + unlock:
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> @@ -1021,6 +990,9 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
>  	if (dev_priv->psr.has_hw_tracking && origin == ORIGIN_FLIP)
>  		return;
>  
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +		cancel_delayed_work_sync(&dev_priv->psr.vlv_activate_work);

psr_invalidate() will also need a cancel_delayed_work_sync() I wonder if
this should move to intel_psr_exit(), right before writing to
VLV_PSRCTL.


> +
>  	mutex_lock(&dev_priv->psr.lock);
>  	if (!dev_priv->psr.enabled) {
>  		mutex_unlock(&dev_priv->psr.lock);
> @@ -1053,9 +1025,8 @@ 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));
> +		intel_psr_activate(dev_priv->psr.enabled);
> +
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> @@ -1102,21 +1073,20 @@ 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);
>  	mutex_init(&dev_priv->psr.lock);
>  
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +		INIT_DELAYED_WORK(&dev_priv->psr.vlv_activate_work,
> +				  vlv_psr_activate_work);
>  		dev_priv->psr.enable_source = vlv_psr_enable_source;
>  		dev_priv->psr.disable_source = vlv_psr_disable;
>  		dev_priv->psr.enable_sink = vlv_psr_enable_sink;
> -		dev_priv->psr.activate = vlv_psr_activate;
>  		dev_priv->psr.setup_vsc = vlv_psr_setup_vsc;
>  	} else {
>  		dev_priv->psr.has_hw_tracking = true;
>  		dev_priv->psr.enable_source = hsw_psr_enable_source;
>  		dev_priv->psr.disable_source = hsw_psr_disable;
>  		dev_priv->psr.enable_sink = hsw_psr_enable_sink;

We might be able to remove enable_sink also if my aux patch goes
through. enable_*sink* required platform specific hooks is a bit odd
IMO.


> -		dev_priv->psr.activate = hsw_psr_activate;
>  		dev_priv->psr.setup_vsc = hsw_psr_setup_vsc;
>  	}
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [RFC v5] drm/i915/psr: Kill scheduled work for Core platforms.
  2018-03-14 20:24     ` Pandiyan, Dhinakaran
@ 2018-03-14 20:47       ` Rodrigo Vivi
  2018-03-14 22:38         ` Pandiyan, Dhinakaran
  0 siblings, 1 reply; 13+ messages in thread
From: Rodrigo Vivi @ 2018-03-14 20:47 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran; +Cc: intel-gfx

On Wed, Mar 14, 2018 at 01:24:13PM -0700, Pandiyan, Dhinakaran wrote:
> On Tue, 2018-03-13 at 15:23 -0700, Rodrigo Vivi wrote:
> > The immediate enabling is 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.
> > 
> > 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.
> > 
> > Furthermore, if we stop using delayed activation on core
> > platforms we will be able, on following up patches,
> > to use available workarounds to make HW tracking properly
> > exit PSR instead of the big nuke of disabling psr and
> > re-enable on exit and activate respectively.
> > At least on few reliable cases.
> > 
> > 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.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c |  20 +++---
> >  drivers/gpu/drm/i915/i915_drv.h     |   3 +-
> >  drivers/gpu/drm/i915/intel_psr.c    | 134 ++++++++++++++----------------------
> >  3 files changed, 65 insertions(+), 92 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 972014b2497d..7dfada863f9c 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2567,15 +2567,14 @@ 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 (HAS_DDI(dev_priv)) {
> > -		if (dev_priv->psr.psr2_support)
> > -			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> > -		else
> > -			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> > -	} else {
> > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > +		bool scheduled;
> > +
> > +		scheduled = work_busy(&dev_priv->psr.vlv_activate_work.work);
> > +		seq_printf(m, "Re-enable work scheduled: %s\n",
> > +			   yesno(scheduled));
> > +
> >  		for_each_pipe(dev_priv, pipe) {
> >  			enum transcoder cpu_transcoder =
> >  				intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > @@ -2594,6 +2593,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> >  
> >  			intel_display_power_put(dev_priv, power_domain);
> >  		}
> > +	} else {
> > +		if (dev_priv->psr.psr2_support)
> > +			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> > +		else
> > +			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> >  	}
> >  
> >  	seq_printf(m, "Main link in standby mode: %s\n",
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 74b0e9d8ff62..409997f29e07 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -598,7 +598,7 @@ struct i915_psr {
> >  	bool sink_support;
> >  	struct intel_dp *enabled;
> >  	bool active;
> > -	struct delayed_work work;
> > +	struct delayed_work vlv_activate_work;
> >  	unsigned busy_frontbuffer_bits;
> >  	bool psr2_support;
> >  	bool aux_frame_sync;
> > @@ -613,7 +613,6 @@ struct i915_psr {
> >  	void (*disable_source)(struct intel_dp *,
> >  			       const struct intel_crtc_state *);
> >  	void (*enable_sink)(struct intel_dp *);
> > -	void (*activate)(struct intel_dp *);
> >  	void (*setup_vsc)(struct intel_dp *, const struct intel_crtc_state *);
> >  };
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 317cb4a12693..bc7b94a06417 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -311,24 +311,6 @@ static void vlv_psr_enable_source(struct intel_dp *intel_dp,
> >  		   VLV_EDP_PSR_ENABLE);
> >  }
> >  
> > -static void vlv_psr_activate(struct intel_dp *intel_dp)
> > -{
> > -	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > -	struct drm_device *dev = dig_port->base.base.dev;
> > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > -	struct drm_crtc *crtc = dig_port->base.base.crtc;
> > -	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> > -
> > -	/*
> > -	 * Let's do the transition from PSR_state 1 (inactive) to
> > -	 * PSR_state 2 (transition to active - static frame transmission).
> > -	 * Then Hardware is responsible for the transition to
> > -	 * PSR_state 3 (active - no Remote Frame Buffer (RFB) update).
> > -	 */
> > -	I915_WRITE(VLV_PSRCTL(pipe), I915_READ(VLV_PSRCTL(pipe)) |
> > -		   VLV_EDP_PSR_ACTIVE_ENTRY);
> > -}
> > -
> >  static void hsw_activate_psr1(struct intel_dp *intel_dp)
> >  {
> >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > @@ -434,16 +416,25 @@ static void hsw_psr_activate(struct intel_dp *intel_dp)
> >  	struct drm_device *dev = dig_port->base.base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> > +	WARN_ON(dev_priv->psr.active);
> > +	lockdep_assert_held(&dev_priv->psr.lock);
> > +
> >  	/* On HSW+ after we enable PSR on source it will activate it
> >  	 * as soon as it match configure idle_frame count. So
> >  	 * we just actually enable it here on activation time.
> >  	 */
> > +	if (dev_priv->psr.psr2_support)
> > +		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> > +	else
> > +		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> >  
> >  	/* psr1 and psr2 are mutually exclusive.*/
> >  	if (dev_priv->psr.psr2_support)
> >  		hsw_activate_psr2(intel_dp);
> >  	else
> >  		hsw_activate_psr1(intel_dp);
> 
> nit: The branches can be grouped together.
> 
> > +
> > +	dev_priv->psr.active = true;
> >  }
> >  
> >  static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> > @@ -568,15 +559,20 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
> >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> > -	if (dev_priv->psr.psr2_support)
> > -		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> > -	else
> > -		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > -	WARN_ON(dev_priv->psr.active);
> > -	lockdep_assert_held(&dev_priv->psr.lock);
> > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > +		/*
> > +		 * FIXME: VLV/CHV has many problems, but worst being blank
> > +		 * screens on first activation. Let's pick the worst case
> > +		 * delay for now.
> > +		 */
> > +		int delay = 1500;
> >  
> > -	dev_priv->psr.activate(intel_dp);
> > -	dev_priv->psr.active = true;
> > +		if (!work_busy(&dev_priv->psr.vlv_activate_work.work))
> 
> Since the work is already cancelled, do we need this?

if we are for sure canceling everywhere you are right. we could remove this.
> 
> The patch looks good to me, fewer indirect calls and easier to read.
> 
> 
> > +			schedule_delayed_work(&dev_priv->psr.vlv_activate_work,
> > +					      msecs_to_jiffies(delay));
> > +	} else {
> > +		hsw_psr_activate(intel_dp);
> > +	}
> >  }
> >  
> >  static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > @@ -652,22 +648,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 VLV/CHV we get bank screen on first activation
> > -		 *     - 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);
> > @@ -786,13 +767,14 @@ 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);
> > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > +		cancel_delayed_work_sync(&dev_priv->psr.vlv_activate_work);
> >  }
> >  
> > -static void intel_psr_work(struct work_struct *work)
> > +static void vlv_psr_activate_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.vlv_activate_work.work);
> >  	struct intel_dp *intel_dp = dev_priv->psr.enabled;
> >  	struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc;
> >  	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> > @@ -802,35 +784,13 @@ static void intel_psr_work(struct work_struct *work)
> >  	 * PSR might take some time to get fully disabled
> >  	 * and be ready for re-enable.
> >  	 */
> > -	if (HAS_DDI(dev_priv)) {
> > -		if (dev_priv->psr.psr2_support) {
> > -			if (intel_wait_for_register(dev_priv,
> > -						    EDP_PSR2_STATUS,
> > -						    EDP_PSR2_STATUS_STATE_MASK,
> > -						    0,
> > -						    50)) {
> > -				DRM_ERROR("Timed out waiting for PSR2 Idle for re-enable\n");
> > -				return;
> > -			}
> > -		} else {
> > -			if (intel_wait_for_register(dev_priv,
> > -						    EDP_PSR_STATUS,
> > -						    EDP_PSR_STATUS_STATE_MASK,
> > -						    0,
> > -						    50)) {
> > -				DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> > -				return;
> > -			}
> > -		}
> > -	} else {
> > -		if (intel_wait_for_register(dev_priv,
> > -					    VLV_PSRSTAT(pipe),
> > -					    VLV_EDP_PSR_IN_TRANS,
> > -					    0,
> > -					    1)) {
> > -			DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> > -			return;
> > -		}
> > +	if (intel_wait_for_register(dev_priv,
> > +				    VLV_PSRSTAT(pipe),
> > +				    VLV_EDP_PSR_IN_TRANS,
> > +				    0,
> > +				    1)) {
> > +		DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> > +		return;
> >  	}
> >  	mutex_lock(&dev_priv->psr.lock);
> >  	intel_dp = dev_priv->psr.enabled;
> > @@ -846,8 +806,17 @@ static void intel_psr_work(struct work_struct *work)
> >  	if (dev_priv->psr.busy_frontbuffer_bits)
> >  		goto unlock;
> >  
> > -	intel_psr_activate(intel_dp);
> > -unlock:
> > +	/*
> > +	 * Let's do the transition from PSR_state 1 (inactive) to
> > +	 * PSR_state 2 (transition to active - static frame transmission).
> > +	 * Then Hardware is responsible for the transition to
> > +	 * PSR_state 3 (active - no Remote Frame Buffer (RFB) update).
> > +	 */
> > +	I915_WRITE(VLV_PSRCTL(pipe), I915_READ(VLV_PSRCTL(pipe)) |
> > +		   VLV_EDP_PSR_ACTIVE_ENTRY);
> > +	dev_priv->psr.active = true;
> > +
> > + unlock:
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
> >  
> > @@ -1021,6 +990,9 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
> >  	if (dev_priv->psr.has_hw_tracking && origin == ORIGIN_FLIP)
> >  		return;
> >  
> > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > +		cancel_delayed_work_sync(&dev_priv->psr.vlv_activate_work);
> 
> psr_invalidate() will also need a cancel_delayed_work_sync()

indeed...

> I wonder if
> this should move to intel_psr_exit(), right before writing to
> VLV_PSRCTL.

I'm afraid of calling it from inside the mutex area since it is
cancel and wait and wait might be on the mutex_lock inside the work...

thoughts?

> 
> 
> > +
> >  	mutex_lock(&dev_priv->psr.lock);
> >  	if (!dev_priv->psr.enabled) {
> >  		mutex_unlock(&dev_priv->psr.lock);
> > @@ -1053,9 +1025,8 @@ 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));
> > +		intel_psr_activate(dev_priv->psr.enabled);
> > +
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
> >  
> > @@ -1102,21 +1073,20 @@ 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);
> >  	mutex_init(&dev_priv->psr.lock);
> >  
> >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > +		INIT_DELAYED_WORK(&dev_priv->psr.vlv_activate_work,
> > +				  vlv_psr_activate_work);
> >  		dev_priv->psr.enable_source = vlv_psr_enable_source;
> >  		dev_priv->psr.disable_source = vlv_psr_disable;
> >  		dev_priv->psr.enable_sink = vlv_psr_enable_sink;
> > -		dev_priv->psr.activate = vlv_psr_activate;
> >  		dev_priv->psr.setup_vsc = vlv_psr_setup_vsc;
> >  	} else {
> >  		dev_priv->psr.has_hw_tracking = true;
> >  		dev_priv->psr.enable_source = hsw_psr_enable_source;
> >  		dev_priv->psr.disable_source = hsw_psr_disable;
> >  		dev_priv->psr.enable_sink = hsw_psr_enable_sink;
> 
> We might be able to remove enable_sink also if my aux patch goes
> through. enable_*sink* required platform specific hooks is a bit odd
> IMO.

cool!

> 
> 
> > -		dev_priv->psr.activate = hsw_psr_activate;
> >  		dev_priv->psr.setup_vsc = hsw_psr_setup_vsc;
> >  	}
> >  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [RFC v5] drm/i915/psr: Kill scheduled work for Core platforms.
  2018-03-14 20:47       ` Rodrigo Vivi
@ 2018-03-14 22:38         ` Pandiyan, Dhinakaran
  0 siblings, 0 replies; 13+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-14 22:38 UTC (permalink / raw)
  To: Vivi, Rodrigo; +Cc: intel-gfx




On Wed, 2018-03-14 at 13:47 -0700, Rodrigo Vivi wrote:
> On Wed, Mar 14, 2018 at 01:24:13PM -0700, Pandiyan, Dhinakaran wrote:
> > On Tue, 2018-03-13 at 15:23 -0700, Rodrigo Vivi wrote:
> > > The immediate enabling is 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.
> > > 
> > > 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.
> > > 
> > > Furthermore, if we stop using delayed activation on core
> > > platforms we will be able, on following up patches,
> > > to use available workarounds to make HW tracking properly
> > > exit PSR instead of the big nuke of disabling psr and
> > > re-enable on exit and activate respectively.
> > > At least on few reliable cases.
> > > 
> > > 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.
> > > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c |  20 +++---
> > >  drivers/gpu/drm/i915/i915_drv.h     |   3 +-
> > >  drivers/gpu/drm/i915/intel_psr.c    | 134 ++++++++++++++----------------------
> > >  3 files changed, 65 insertions(+), 92 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 972014b2497d..7dfada863f9c 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2567,15 +2567,14 @@ 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 (HAS_DDI(dev_priv)) {
> > > -		if (dev_priv->psr.psr2_support)
> > > -			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> > > -		else
> > > -			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> > > -	} else {
> > > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > +		bool scheduled;
> > > +
> > > +		scheduled = work_busy(&dev_priv->psr.vlv_activate_work.work);
> > > +		seq_printf(m, "Re-enable work scheduled: %s\n",
> > > +			   yesno(scheduled));
> > > +
> > >  		for_each_pipe(dev_priv, pipe) {
> > >  			enum transcoder cpu_transcoder =
> > >  				intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > > @@ -2594,6 +2593,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> > >  
> > >  			intel_display_power_put(dev_priv, power_domain);
> > >  		}
> > > +	} else {
> > > +		if (dev_priv->psr.psr2_support)
> > > +			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> > > +		else
> > > +			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> > >  	}
> > >  
> > >  	seq_printf(m, "Main link in standby mode: %s\n",
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 74b0e9d8ff62..409997f29e07 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -598,7 +598,7 @@ struct i915_psr {
> > >  	bool sink_support;
> > >  	struct intel_dp *enabled;
> > >  	bool active;
> > > -	struct delayed_work work;
> > > +	struct delayed_work vlv_activate_work;
> > >  	unsigned busy_frontbuffer_bits;
> > >  	bool psr2_support;
> > >  	bool aux_frame_sync;
> > > @@ -613,7 +613,6 @@ struct i915_psr {
> > >  	void (*disable_source)(struct intel_dp *,
> > >  			       const struct intel_crtc_state *);
> > >  	void (*enable_sink)(struct intel_dp *);
> > > -	void (*activate)(struct intel_dp *);
> > >  	void (*setup_vsc)(struct intel_dp *, const struct intel_crtc_state *);
> > >  };
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > index 317cb4a12693..bc7b94a06417 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -311,24 +311,6 @@ static void vlv_psr_enable_source(struct intel_dp *intel_dp,
> > >  		   VLV_EDP_PSR_ENABLE);
> > >  }
> > >  
> > > -static void vlv_psr_activate(struct intel_dp *intel_dp)
> > > -{
> > > -	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > -	struct drm_device *dev = dig_port->base.base.dev;
> > > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > > -	struct drm_crtc *crtc = dig_port->base.base.crtc;
> > > -	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> > > -
> > > -	/*
> > > -	 * Let's do the transition from PSR_state 1 (inactive) to
> > > -	 * PSR_state 2 (transition to active - static frame transmission).
> > > -	 * Then Hardware is responsible for the transition to
> > > -	 * PSR_state 3 (active - no Remote Frame Buffer (RFB) update).
> > > -	 */
> > > -	I915_WRITE(VLV_PSRCTL(pipe), I915_READ(VLV_PSRCTL(pipe)) |
> > > -		   VLV_EDP_PSR_ACTIVE_ENTRY);
> > > -}
> > > -
> > >  static void hsw_activate_psr1(struct intel_dp *intel_dp)
> > >  {
> > >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > @@ -434,16 +416,25 @@ static void hsw_psr_activate(struct intel_dp *intel_dp)
> > >  	struct drm_device *dev = dig_port->base.base.dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  
> > > +	WARN_ON(dev_priv->psr.active);
> > > +	lockdep_assert_held(&dev_priv->psr.lock);
> > > +
> > >  	/* On HSW+ after we enable PSR on source it will activate it
> > >  	 * as soon as it match configure idle_frame count. So
> > >  	 * we just actually enable it here on activation time.
> > >  	 */
> > > +	if (dev_priv->psr.psr2_support)
> > > +		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> > > +	else
> > > +		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > >  
> > >  	/* psr1 and psr2 are mutually exclusive.*/
> > >  	if (dev_priv->psr.psr2_support)
> > >  		hsw_activate_psr2(intel_dp);
> > >  	else
> > >  		hsw_activate_psr1(intel_dp);
> > 
> > nit: The branches can be grouped together.
> > 
> > > +
> > > +	dev_priv->psr.active = true;
> > >  }
> > >  
> > >  static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> > > @@ -568,15 +559,20 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
> > >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  
> > > -	if (dev_priv->psr.psr2_support)
> > > -		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> > > -	else
> > > -		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > > -	WARN_ON(dev_priv->psr.active);
> > > -	lockdep_assert_held(&dev_priv->psr.lock);
> > > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > +		/*
> > > +		 * FIXME: VLV/CHV has many problems, but worst being blank
> > > +		 * screens on first activation. Let's pick the worst case
> > > +		 * delay for now.
> > > +		 */
> > > +		int delay = 1500;
> > >  
> > > -	dev_priv->psr.activate(intel_dp);
> > > -	dev_priv->psr.active = true;
> > > +		if (!work_busy(&dev_priv->psr.vlv_activate_work.work))
> > 
> > Since the work is already cancelled, do we need this?
> 
> if we are for sure canceling everywhere you are right. we could remove this.
> > 
> > The patch looks good to me, fewer indirect calls and easier to read.
> > 
> > 
> > > +			schedule_delayed_work(&dev_priv->psr.vlv_activate_work,
> > > +					      msecs_to_jiffies(delay));
> > > +	} else {
> > > +		hsw_psr_activate(intel_dp);
> > > +	}
> > >  }
> > >  
> > >  static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > > @@ -652,22 +648,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 VLV/CHV we get bank screen on first activation
> > > -		 *     - 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);
> > > @@ -786,13 +767,14 @@ 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);
> > > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > +		cancel_delayed_work_sync(&dev_priv->psr.vlv_activate_work);
> > >  }
> > >  
> > > -static void intel_psr_work(struct work_struct *work)
> > > +static void vlv_psr_activate_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.vlv_activate_work.work);
> > >  	struct intel_dp *intel_dp = dev_priv->psr.enabled;
> > >  	struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc;
> > >  	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> > > @@ -802,35 +784,13 @@ static void intel_psr_work(struct work_struct *work)
> > >  	 * PSR might take some time to get fully disabled
> > >  	 * and be ready for re-enable.
> > >  	 */
> > > -	if (HAS_DDI(dev_priv)) {
> > > -		if (dev_priv->psr.psr2_support) {
> > > -			if (intel_wait_for_register(dev_priv,
> > > -						    EDP_PSR2_STATUS,
> > > -						    EDP_PSR2_STATUS_STATE_MASK,
> > > -						    0,
> > > -						    50)) {
> > > -				DRM_ERROR("Timed out waiting for PSR2 Idle for re-enable\n");
> > > -				return;
> > > -			}
> > > -		} else {
> > > -			if (intel_wait_for_register(dev_priv,
> > > -						    EDP_PSR_STATUS,
> > > -						    EDP_PSR_STATUS_STATE_MASK,
> > > -						    0,
> > > -						    50)) {
> > > -				DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> > > -				return;
> > > -			}
> > > -		}
> > > -	} else {
> > > -		if (intel_wait_for_register(dev_priv,
> > > -					    VLV_PSRSTAT(pipe),
> > > -					    VLV_EDP_PSR_IN_TRANS,
> > > -					    0,
> > > -					    1)) {
> > > -			DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> > > -			return;
> > > -		}
> > > +	if (intel_wait_for_register(dev_priv,
> > > +				    VLV_PSRSTAT(pipe),
> > > +				    VLV_EDP_PSR_IN_TRANS,
> > > +				    0,
> > > +				    1)) {
> > > +		DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> > > +		return;
> > >  	}
> > >  	mutex_lock(&dev_priv->psr.lock);
> > >  	intel_dp = dev_priv->psr.enabled;
> > > @@ -846,8 +806,17 @@ static void intel_psr_work(struct work_struct *work)
> > >  	if (dev_priv->psr.busy_frontbuffer_bits)
> > >  		goto unlock;
> > >  
> > > -	intel_psr_activate(intel_dp);
> > > -unlock:
> > > +	/*
> > > +	 * Let's do the transition from PSR_state 1 (inactive) to
> > > +	 * PSR_state 2 (transition to active - static frame transmission).
> > > +	 * Then Hardware is responsible for the transition to
> > > +	 * PSR_state 3 (active - no Remote Frame Buffer (RFB) update).
> > > +	 */
> > > +	I915_WRITE(VLV_PSRCTL(pipe), I915_READ(VLV_PSRCTL(pipe)) |
> > > +		   VLV_EDP_PSR_ACTIVE_ENTRY);
> > > +	dev_priv->psr.active = true;
> > > +
> > > + unlock:
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > >  }
> > >  
> > > @@ -1021,6 +990,9 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
> > >  	if (dev_priv->psr.has_hw_tracking && origin == ORIGIN_FLIP)
> > >  		return;
> > >  
> > > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > +		cancel_delayed_work_sync(&dev_priv->psr.vlv_activate_work);
> > 
> > psr_invalidate() will also need a cancel_delayed_work_sync()
> 
> indeed...
> 
> > I wonder if
> > this should move to intel_psr_exit(), right before writing to
> > VLV_PSRCTL.
> 
> I'm afraid of calling it from inside the mutex area since it is
> cancel and wait and wait might be on the mutex_lock inside the work...
> 
> thoughts?

Waiting should be okay. We'd be waiting for the mutex if the work
function is already executing and has acquired the mutex. So cancel_work
will not involve any waiting.

But, if the mutex is available for flush and the work function has
already started executing but hasn't acquired the mutex yet, I think we
might deadlock in cancel_work_sync. We can check what lockdep says by
making that change.

The way it is currently (and after you made the change to include
cancel_work in invaldate ) seems racy. If invalidate follows a flush, we
can end up in two situations

1)
	 	Flush
Invalidate	cancel_work()
cancel_work()
		exit()
		schedule_work()
exit()


2)
		Flush
Invalidate	cancel_work
		exit()
		schedule_work()
cancel_work()
exit()



> 
> > 
> > 
> > > +
> > >  	mutex_lock(&dev_priv->psr.lock);
> > >  	if (!dev_priv->psr.enabled) {
> > >  		mutex_unlock(&dev_priv->psr.lock);
> > > @@ -1053,9 +1025,8 @@ 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));
> > > +		intel_psr_activate(dev_priv->psr.enabled);
> > > +
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > >  }
> > >  
> > > @@ -1102,21 +1073,20 @@ 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);
> > >  	mutex_init(&dev_priv->psr.lock);
> > >  
> > >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > +		INIT_DELAYED_WORK(&dev_priv->psr.vlv_activate_work,
> > > +				  vlv_psr_activate_work);
> > >  		dev_priv->psr.enable_source = vlv_psr_enable_source;
> > >  		dev_priv->psr.disable_source = vlv_psr_disable;
> > >  		dev_priv->psr.enable_sink = vlv_psr_enable_sink;
> > > -		dev_priv->psr.activate = vlv_psr_activate;
> > >  		dev_priv->psr.setup_vsc = vlv_psr_setup_vsc;
> > >  	} else {
> > >  		dev_priv->psr.has_hw_tracking = true;
> > >  		dev_priv->psr.enable_source = hsw_psr_enable_source;
> > >  		dev_priv->psr.disable_source = hsw_psr_disable;
> > >  		dev_priv->psr.enable_sink = hsw_psr_enable_sink;
> > 
> > We might be able to remove enable_sink also if my aux patch goes
> > through. enable_*sink* required platform specific hooks is a bit odd
> > IMO.
> 
> cool!
> 
> > 
> > 
> > > -		dev_priv->psr.activate = hsw_psr_activate;
> > >  		dev_priv->psr.setup_vsc = hsw_psr_setup_vsc;
> > >  	}
> > >  }
> _______________________________________________
> 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] 13+ messages in thread

* [PATCH] drm/i915/psr: Kill scheduled work for Core platforms.
       [not found] <Message-ID: <20180312194929.24800.36550@emeril.freedesktop.org>
@ 2018-03-12 20:20 ` Rodrigo Vivi
  0 siblings, 0 replies; 13+ messages in thread
From: Rodrigo Vivi @ 2018-03-12 20:20 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dhinakaran Pandiyan, Rodrigo Vivi

The immediate enabling is 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.

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.

Furthermore, if we stop using delayed activation on core
platforms we will be able, on following up patches,
to use available workarounds to make HW tracking properly
exit PSR instead of the big nuke of disabling psr and
re-enable on exit and activate respectively.
At least on few reliable cases.

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.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c4cc8fef11a0..f803b40848e8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2568,15 +2568,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
 	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 (HAS_DDI(dev_priv)) {
-		if (dev_priv->psr.psr2_support)
-			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
-		else
-			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
-	} else {
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+		seq_printf(m, "Re-enable work scheduled: %s\n",
+			   yesno(work_busy(&dev_priv->psr.work.work)));
+
 		for_each_pipe(dev_priv, pipe) {
 			enum transcoder cpu_transcoder =
 				intel_pipe_to_cpu_transcoder(dev_priv, pipe);
@@ -2595,6 +2591,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 
 			intel_display_power_put(dev_priv, power_domain);
 		}
+	} else {
+		if (dev_priv->psr.psr2_support)
+			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
+		else
+			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
 	}
 
 	seq_printf(m, "Main link in standby mode: %s\n",
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 975ebb51c7af..d93ecfc5d739 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -652,9 +652,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 {
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		/*
 		 * FIXME: Activation should happen immediately since this
 		 * function is just called after pipe is fully trained and
@@ -667,6 +665,8 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 		 */
 		schedule_delayed_work(&dev_priv->psr.work,
 				      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
+	} else {
+		intel_psr_activate(intel_dp);
 	}
 
 unlock:
@@ -786,7 +786,8 @@ 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);
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		cancel_delayed_work_sync(&dev_priv->psr.work);
 }
 
 static void intel_psr_work(struct work_struct *work)
@@ -1045,10 +1046,15 @@ 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));
+	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) {
+		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+			if (!work_busy(&dev_priv->psr.work.work))
+				schedule_delayed_work(&dev_priv->psr.work,
+						      msecs_to_jiffies(100));
+		} else {
+			intel_psr_activate(dev_priv->psr.enabled);
+		}
+	}
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
@@ -1095,7 +1101,8 @@ 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);
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
 	mutex_init(&dev_priv->psr.lock);
 
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
-- 
2.13.6

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

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

end of thread, other threads:[~2018-03-14 22:38 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-09  1:17 [PATCH] drm/i915/psr: Kill scheduled work for Core platforms Rodrigo Vivi
2018-03-09  1:42 ` ✗ Fi.CI.BAT: failure for " Patchwork
2018-03-12 18:15   ` Pandiyan, Dhinakaran
2018-03-12 18:54     ` Rodrigo Vivi
2018-03-12 19:49 ` ✗ Fi.CI.BAT: warning " Patchwork
2018-03-12 21:54 ` [PATCH] " Pandiyan, Dhinakaran
2018-03-13 22:23   ` [RFC v5] " Rodrigo Vivi
2018-03-14 20:24     ` Pandiyan, Dhinakaran
2018-03-14 20:47       ` Rodrigo Vivi
2018-03-14 22:38         ` Pandiyan, Dhinakaran
2018-03-13 23:51 ` ✓ Fi.CI.BAT: success for drm/i915/psr: Kill scheduled work for Core platforms. (rev2) Patchwork
2018-03-14  2:37 ` ✓ Fi.CI.IGT: " Patchwork
     [not found] <Message-ID: <20180312194929.24800.36550@emeril.freedesktop.org>
2018-03-12 20:20 ` [PATCH] drm/i915/psr: Kill scheduled work for Core platforms Rodrigo Vivi

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.