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

[Imre Deak <imre.deak@intel.com>]

On Wed, Oct 07, 2020 at 10:22:41PM +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 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@redhat.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@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);

This call is the needed one (to re-probe the connectors) and the above
call is not.

>  	}
>  
>  	drm_atomic_state_put(state);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 7277e58b01f1..20ddc54298cb 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -1424,6 +1424,7 @@ static void vlv_display_power_well_init(struct drm_i915_private *dev_priv)
>  		return;
>  
>  	intel_hpd_init(dev_priv);
> +	intel_hpd_poll_disable(dev_priv);
>  
>  	/* Re-enable the ADPA, if we have one */
>  	for_each_intel_encoder(&dev_priv->drm, encoder) {
> @@ -1449,7 +1450,7 @@ static void vlv_display_power_well_deinit(struct drm_i915_private *dev_priv)
>  
>  	/* Prevent us from re-enabling polling on accident in late suspend */
>  	if (!dev_priv->drm.dev->power.is_suspended)
> -		intel_hpd_poll_init(dev_priv);
> +		intel_hpd_poll_enable(dev_priv);
>  }
>  
>  static void vlv_display_power_well_enable(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c
> index 5c58c1ed6493..30bd4c86d146 100644
> --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> @@ -584,7 +584,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
>   * This is a separate step from interrupt enabling to simplify the locking rules
>   * in the driver load and resume code.
>   *
> - * Also see: intel_hpd_poll_init(), which enables connector polling
> + * Also see: intel_hpd_poll_enable() and intel_hpd_poll_disable().
>   */
>  void intel_hpd_init(struct drm_i915_private *dev_priv)
>  {
> @@ -595,9 +595,6 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
>  		dev_priv->hotplug.stats[i].state = HPD_ENABLED;
>  	}
>  
> -	WRITE_ONCE(dev_priv->hotplug.poll_enabled, false);
> -	schedule_work(&dev_priv->hotplug.poll_init_work);
> -
>  	/*
>  	 * Interrupt setup is already guaranteed to be single-threaded, this is
>  	 * just to make the assert_spin_locked checks happy.
> @@ -654,12 +651,12 @@ static void i915_hpd_poll_init_work(struct work_struct *work)
>  }
>  
>  /**
> - * intel_hpd_poll_init - enables/disables polling for connectors with hpd
> + * intel_hpd_poll_enable - enable polling for connectors with hpd
>   * @dev_priv: i915 device instance
>   *
> - * This function enables polling for all connectors, regardless of whether or
> - * not they support hotplug detection. Under certain conditions HPD may not be
> - * functional. On most Intel GPUs, this happens when we enter runtime suspend.
> + * This function enables polling for all connectors which support HPD.
> + * Under certain conditions HPD may not be functional. On most Intel GPUs,
> + * this happens when we enter runtime suspend.
>   * On Valleyview and Cherryview systems, this also happens when we shut off all
>   * of the powerwells.
>   *
> @@ -667,9 +664,9 @@ static void i915_hpd_poll_init_work(struct work_struct *work)
>   * dev->mode_config.mutex, we do the actual hotplug enabling in a seperate
>   * worker.
>   *
> - * Also see: intel_hpd_init(), which restores hpd handling.
> + * Also see: intel_hpd_init() and intel_hpd_poll_disable().
>   */
> -void intel_hpd_poll_init(struct drm_i915_private *dev_priv)
> +void intel_hpd_poll_enable(struct drm_i915_private *dev_priv)
>  {
>  	WRITE_ONCE(dev_priv->hotplug.poll_enabled, true);
>  
> @@ -682,6 +679,31 @@ void intel_hpd_poll_init(struct drm_i915_private *dev_priv)
>  	schedule_work(&dev_priv->hotplug.poll_init_work);
>  }
>  
> +/**
> + * intel_hpd_poll_disable - disable polling for connectors with hpd
> + * @dev_priv: i915 device instance
> + *
> + * This function disables polling for all connectors which support HPD.
> + * Under certain conditions HPD may not be functional. On most Intel GPUs,
> + * this happens when we enter runtime suspend.
> + * On Valleyview and Cherryview systems, this also happens when we shut off all
> + * of the powerwells.
> + *
> + * Since this function can get called in contexts where we're already holding
> + * dev->mode_config.mutex, we do the actual hotplug enabling in a seperate
> + * worker.
> + *
> + * Also used during driver init to initialize connector->polled
> + * appropriately for all connectors.
> + *
> + * Also see: intel_hpd_init() and intel_hpd_poll_enable().
> + */
> +void intel_hpd_poll_disable(struct drm_i915_private *dev_priv)
> +{
> +	WRITE_ONCE(dev_priv->hotplug.poll_enabled, false);
> +	schedule_work(&dev_priv->hotplug.poll_init_work);
> +}
> +
>  void intel_hpd_init_work(struct drm_i915_private *dev_priv)
>  {
>  	INIT_DELAYED_WORK(&dev_priv->hotplug.hotplug_work,
> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.h b/drivers/gpu/drm/i915/display/intel_hotplug.h
> index a704d7c94d16..b87e95d606e6 100644
> --- a/drivers/gpu/drm/i915/display/intel_hotplug.h
> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.h
> @@ -14,7 +14,8 @@ struct intel_digital_port;
>  struct intel_encoder;
>  enum port;
>  
> -void intel_hpd_poll_init(struct drm_i915_private *dev_priv);
> +void intel_hpd_poll_enable(struct drm_i915_private *dev_priv);
> +void intel_hpd_poll_disable(struct drm_i915_private *dev_priv);
>  enum intel_hotplug_state intel_encoder_hotplug(struct intel_encoder *encoder,
>  					       struct intel_connector *connector);
>  void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index ebc15066d108..3fc7b996fc48 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1226,26 +1226,15 @@ 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 */

The above is a comment for intel_hpd_init().

intel_modeset_init() also calls intel_hpd_init(), looks like that
guarantees the explicit connector probing during driver loading. Do we
need to call intel_hpd_poll_disable() somewhere on that path too? (Maybe
only from i915_driver_register().)

>  	intel_dp_mst_resume(dev_priv);
> -
>  	intel_display_resume(dev);
>  
> +	intel_hpd_poll_disable(dev_priv);
>  	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);
> @@ -1557,7 +1546,7 @@ static int intel_runtime_suspend(struct device *kdev)
>  	assert_forcewakes_inactive(&dev_priv->uncore);
>  
>  	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
> -		intel_hpd_poll_init(dev_priv);
> +		intel_hpd_poll_enable(dev_priv);
>  
>  	drm_dbg_kms(&dev_priv->drm, "Device suspended\n");
>  	return 0;
> @@ -1602,8 +1591,10 @@ static int intel_runtime_resume(struct device *kdev)
>  	 * power well, so hpd is reinitialized from there. For
>  	 * everyone else do it here.
>  	 */
> -	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
> +	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
>  		intel_hpd_init(dev_priv);
> +		intel_hpd_poll_disable(dev_priv);
> +	}
>  
>  	intel_enable_ipc(dev_priv);
>  
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx