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@xxxxxxxxx>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
