| Message ID | 20201006185809.4655-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Reorder hpd init vs. display resume | expand |
On Tue, Oct 06, 2020 at 09:58:07PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Currently we call .hpd_irq_setup() directly just before display > resume, and follow it with another call via intel_hpd_init() > just afterwards. Assuming the hpd pins are marked as enabled > during the open-coded call these two things do exactly the > same thing (ie. enable HPD interrupts). Which even makes sense > since we definitely need working HPD interrupts for MST sideband > during the display resume. > > So let's just nuke the open-coded call and move the intel_hpd_init() > call earlier. > > If we really would like to prevent all *long* HPD processing during > display resume we'd need some kind of software mechanism to simply > ignore all long HPDs. Currently we appear to have that just for > fbdev via ifbdev->hpd_suspended. Since we aren't exploding left and > right all the time I guess that's mostly sufficient. Daniel suggested including the archaeological record here. This is what I was planning to amend here: "For a bit of history on this, we first got a mechanism to block hotplug processing during suspend in commit 15239099d7a7 ("drm/i915: enable irqs earlier when resuming") on account of moving the irq enable earlier. This then got removed in commit 50c3dc970a09 ("drm/fb-helper: Fix hpd vs. initial config races") because the fdev initial config got pushed to a later point. The second ad-hoc hpd_irq_setup() for resume was added in commit 0e32b39ceed6 ("drm/i915: add DP 1.2 MST support (v0.7)") to be able to do MST sideband during resume. And finally we got a partial resurrection of the hpd blocking mechanism in commit e8a8fedd57fd ("drm/i915: Block fbdev HPD processing during suspend"), but this time it only prevent fbdev from handling the hpd while resuming." But looks like I was far too optimistic and this did in fact blow up in CI. So I'll need to do some actual work to figure out what's going on... > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 8 +------- > drivers/gpu/drm/i915/i915_drv.c | 16 ++-------------- > 2 files changed, 3 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 907e1d155443..87df7167433f 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -5036,18 +5036,12 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) > intel_pps_unlock_regs_wa(dev_priv); > intel_modeset_init_hw(dev_priv); > intel_init_clock_gating(dev_priv); > - > - spin_lock_irq(&dev_priv->irq_lock); > - if (dev_priv->display.hpd_irq_setup) > - dev_priv->display.hpd_irq_setup(dev_priv); > - spin_unlock_irq(&dev_priv->irq_lock); > + intel_hpd_init(dev_priv); > > ret = __intel_display_resume(dev, state, ctx); > if (ret) > drm_err(&dev_priv->drm, > "Restoring old state failed with %i\n", ret); > - > - intel_hpd_init(dev_priv); > } > > drm_atomic_state_put(state); > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index ebc15066d108..b2a050d916e3 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1226,26 +1226,14 @@ static int i915_drm_resume(struct drm_device *dev) > > intel_modeset_init_hw(dev_priv); > intel_init_clock_gating(dev_priv); > + intel_hpd_init(dev_priv); > > - spin_lock_irq(&dev_priv->irq_lock); > - if (dev_priv->display.hpd_irq_setup) > - dev_priv->display.hpd_irq_setup(dev_priv); > - spin_unlock_irq(&dev_priv->irq_lock); > - > + /* MST sideband requires HPD interrupts enabled */ > intel_dp_mst_resume(dev_priv); > - > intel_display_resume(dev); > > drm_kms_helper_poll_enable(dev); > > - /* > - * ... but also need to make sure that hotplug processing > - * doesn't cause havoc. Like in the driver load code we don't > - * bother with the tiny race here where we might lose hotplug > - * notifications. > - * */ > - intel_hpd_init(dev_priv); > - > intel_opregion_resume(dev_priv); > > intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false); > -- > 2.26.2
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 907e1d155443..87df7167433f 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -5036,18 +5036,12 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) intel_pps_unlock_regs_wa(dev_priv); intel_modeset_init_hw(dev_priv); intel_init_clock_gating(dev_priv); - - spin_lock_irq(&dev_priv->irq_lock); - if (dev_priv->display.hpd_irq_setup) - dev_priv->display.hpd_irq_setup(dev_priv); - spin_unlock_irq(&dev_priv->irq_lock); + intel_hpd_init(dev_priv); ret = __intel_display_resume(dev, state, ctx); if (ret) drm_err(&dev_priv->drm, "Restoring old state failed with %i\n", ret); - - intel_hpd_init(dev_priv); } drm_atomic_state_put(state); diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index ebc15066d108..b2a050d916e3 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1226,26 +1226,14 @@ static int i915_drm_resume(struct drm_device *dev) intel_modeset_init_hw(dev_priv); intel_init_clock_gating(dev_priv); + intel_hpd_init(dev_priv); - spin_lock_irq(&dev_priv->irq_lock); - if (dev_priv->display.hpd_irq_setup) - dev_priv->display.hpd_irq_setup(dev_priv); - spin_unlock_irq(&dev_priv->irq_lock); - + /* MST sideband requires HPD interrupts enabled */ intel_dp_mst_resume(dev_priv); - intel_display_resume(dev); drm_kms_helper_poll_enable(dev); - /* - * ... but also need to make sure that hotplug processing - * doesn't cause havoc. Like in the driver load code we don't - * bother with the tiny race here where we might lose hotplug - * notifications. - * */ - intel_hpd_init(dev_priv); - intel_opregion_resume(dev_priv); intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false);