| Message ID | 20180313222319.2721-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > } > }
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; > > } > > }
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
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; } }
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(-)