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

[Ville Syrjälä]

On Tue, Oct 06, 2020 at 09:58:07PM +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 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 at 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

-- 
Ville Syrjälä
Intel