Re: [Intel-gfx] [PATCH] drm/i915/psr: Kill scheduled work for Core platforms.
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
> Signed-off-by: Rodrigo Vivi
> ---
> 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 co
[Intel-gfx] [PATCH] drm/i915/psr: Kill scheduled work for Core platforms.
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
Signed-off-by: Rodrigo Vivi
---
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)) {
+
[Intel-gfx] [PATCH] drm/i915/psr: Kill scheduled work for Core platforms.
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
Signed-off-by: Rodrigo Vivi
---
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_
