[Intel-gfx] [PATCH v2 1/3] drm/i915: Reorder hpd init vs. display resume

[Ville Syrjälä]

On Wed, Oct 07, 2020 at 06:15:47PM -0400, Lyude Paul wrote:
> On Wed, 2020-10-07 at 22:22 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala at 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 nuke the open-coded call and move the intel_hpd_init()
> > call earlier. However we need to leave the poll_init_work stuff
> > behind after the display resume as that will trigger display
> > detection while we're resuming. We don't want that trampling over
> > the display resume process. To make this a bit more symmetric
> > we turn this into a intel_hpd_poll_{enable,disable}() pair.
> > So we end up with the following transformation:
> > intel_hpd_poll_init() -> intel_hpd_poll_enable()
> > lone intel_hpd_init() -> intel_hpd_init()+intel_hpd_poll_disable()
> > .hpd_irq_setup()+resume+intel_hpd_init() ->
> > intel_hpd_init()+resume+intel_hpd_poll_disable()
> > 
> > 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.
> > 
> > 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 the 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 hpd while resuming.
> > 
> > v2: Leave the poll_init_work behind
> > 
> > Cc: Lyude Paul <lyude at redhat.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c  |  9 ++--
> >  .../drm/i915/display/intel_display_power.c    |  3 +-
> >  drivers/gpu/drm/i915/display/intel_hotplug.c  | 42 ++++++++++++++-----
> >  drivers/gpu/drm/i915/display/intel_hotplug.h  |  3 +-
> >  drivers/gpu/drm/i915/i915_drv.c               | 23 ++++------
> >  5 files changed, 46 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 907e1d155443..0d5607ae97c4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -5036,18 +5036,15 @@ 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);
> > +		intel_hpd_poll_disable(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);
> > +		intel_hpd_poll_disable(dev_priv);
> 
> Looks like you're calling intel_hpd_poll_disable() twice here, is this
> intentional?

No, just a failure to follow my own rules. Thanks for spotting it.

-- 
Ville Syrjälä
Intel