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