[v2,3/3] drm/i915: Enable polling when we don't have hpd

Commit Message

[e-mail] June 20, 2016, 8:57 p.m. UTC

Unfortunately, there's two situations where we lose hpd right now:
- Runtime suspend
- When we've shut off all of the power wells on Valleyview/Cherryview

While it would be nice if this didn't cause issues, this has the
ability to get us in some awkward states where a user won't be able to
get their display to turn on. For instance; if we boot a Valleyview
system without any monitors connected, it won't need any of it's power
wells and thus shut them off. Since this causes us to lose HPD, this
means that unless the user knows how to ssh into their machine and do a
manual reprobe for monitors, none of the monitors they connect after
booting will actually work.

Eventually we should come up with a better fix then having to enable
polling for this, since this makes rpm a lot less useful, but for now
the infrastructure in i915 just isn't there yet to get hpd in these
situations.

Changes since v1:
 - Add comment explaining the addition of the if
   (!mode_config->poll_running) in intel_hpd_init()
 - Remove unneeded if (!dev->mode_config.poll_enabled) in
   i915_hpd_poll_init_work()
 - Call to drm_helper_hpd_irq_event() after we disable polling
 - Add cancel_work_sync() call to intel_hpd_cancel_work()

Cc: [e-mail]
Cc: Ville Syrjälä <[e-mail]>
Acked-by: Daniel Vetter <[e-mail]>
Signed-off-by: Lyude <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.c         |  7 ++-
 drivers/gpu/drm/i915/i915_drv.h         |  3 ++
 drivers/gpu/drm/i915/intel_drv.h        |  2 +
 drivers/gpu/drm/i915/intel_hotplug.c    | 82 +++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_runtime_pm.c |  3 ++
 5 files changed, 86 insertions(+), 11 deletions(-)

Comments

[e-mail] June 20, 2016, 8:59 p.m. UTC | #1

As a forewarning: I have confirmed the CRT hpd loops Vsyrjala has mentioned, and
they are triggered by this patch. I'm working on a patch for this atm.

On Mon, 2016-06-20 at 16:57 -0400, Lyude wrote:
> Unfortunately, there's two situations where we lose hpd right now:
> - Runtime suspend
> - When we've shut off all of the power wells on Valleyview/Cherryview
> 
> While it would be nice if this didn't cause issues, this has the
> ability to get us in some awkward states where a user won't be able to
> get their display to turn on. For instance; if we boot a Valleyview
> system without any monitors connected, it won't need any of it's power
> wells and thus shut them off. Since this causes us to lose HPD, this
> means that unless the user knows how to ssh into their machine and do a
> manual reprobe for monitors, none of the monitors they connect after
> booting will actually work.
> 
> Eventually we should come up with a better fix then having to enable
> polling for this, since this makes rpm a lot less useful, but for now
> the infrastructure in i915 just isn't there yet to get hpd in these
> situations.
> 
> Changes since v1:
>  - Add comment explaining the addition of the if
>    (!mode_config->poll_running) in intel_hpd_init()
>  - Remove unneeded if (!dev->mode_config.poll_enabled) in
>    i915_hpd_poll_init_work()
>  - Call to drm_helper_hpd_irq_event() after we disable polling
>  - Add cancel_work_sync() call to intel_hpd_cancel_work()
> 
> Cc: [e-mail]
> Cc: Ville Syrjälä <[e-mail]>
> Acked-by: Daniel Vetter <[e-mail]>
> Signed-off-by: Lyude <[e-mail]>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         |  7 ++-
>  drivers/gpu/drm/i915/i915_drv.h         |  3 ++
>  drivers/gpu/drm/i915/intel_drv.h        |  2 +
>  drivers/gpu/drm/i915/intel_hotplug.c    | 82 +++++++++++++++++++++++++++++---
> -
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 ++
>  5 files changed, 86 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3eb47fb..5381d9e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1612,6 +1612,9 @@ static int intel_runtime_suspend(struct device *device)
>  
>  	assert_forcewakes_inactive(dev_priv);
>  
> +	if (!IS_VALLEYVIEW(dev_priv) || !IS_CHERRYVIEW(dev_priv))
> +		intel_hpd_poll_enable(dev_priv, true);
> +
>  	DRM_DEBUG_KMS("Device suspended\n");
>  	return 0;
>  }
> @@ -1667,8 +1670,10 @@ static int intel_runtime_resume(struct device *device)
>  	 * 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_enable(dev_priv, false);
> +	}
>  
>  	intel_enable_gt_powersave(dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1eef5ef..6f7075d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -283,6 +283,9 @@ struct i915_hotplug {
>  	u32 short_port_mask;
>  	struct work_struct dig_port_work;
>  
> +	struct work_struct poll_enable_work;
> +	bool poll_enabled;
> +
>  	/*
>  	 * if we get a HPD irq from DP and a HPD irq from non-DP
>  	 * the non-DP HPD could block the workqueue on a mode config
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index cd14751..87c5143 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1384,6 +1384,8 @@ int intel_dsi_dcs_init_backlight_funcs(struct
> intel_connector *intel_connector);
>  
>  /* intel_dvo.c */
>  void intel_dvo_init(struct drm_device *dev);
> +/* intel_hotplug.c */
> +void intel_hpd_poll_enable(struct drm_i915_private *dev_priv, bool enabled);
>  
>  
>  /* legacy fbdev emulation in intel_fbdev.c */
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c
> b/drivers/gpu/drm/i915/intel_hotplug.c
> index ec3285f..3ef5096 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -464,20 +464,34 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
>  		dev_priv->hotplug.stats[i].count = 0;
>  		dev_priv->hotplug.stats[i].state = HPD_ENABLED;
>  	}
> -	list_for_each_entry(connector, &mode_config->connector_list, head) {
> -		struct intel_connector *intel_connector =
> to_intel_connector(connector);
> -		connector->polled = intel_connector->polled;
>  
> -		/* MST has a dynamic intel_connector->encoder and it's
> reprobing
> -		 * is all handled by the MST helpers. */
> -		if (intel_connector->mst_port)
> -			continue;
> +	/*
> +	 * When we're in the midst of doing connector polling in situations
> +	 * where we don't have working hpd, we need to make sure we don't try
> +	 * changing the polling settings of any connectors, since this can
> lead
> +	 * to us setting connector->polled to DRM_CONNECTOR_POLL_HPD,
> resulting
> +	 * in DRM's polling helpers automatically skipping the rest of the
> +	 * connectors that still need polling
> +	 */
> +	if (!mode_config->poll_running) {
> +		list_for_each_entry(connector, &mode_config->connector_list,
> head) {
> +			struct intel_connector *intel_connector =
> +				to_intel_connector(connector);
> +			connector->polled = intel_connector->polled;
>  
> -		if (!connector->polled && I915_HAS_HOTPLUG(dev) &&
> -		    intel_connector->encoder->hpd_pin > HPD_NONE)
> -			connector->polled = DRM_CONNECTOR_POLL_HPD;
> +			/* MST has a dynamic intel_connector->encoder and
> it's
> +			 * reprobing is all handled by the MST helpers. */
> +			if (intel_connector->mst_port)
> +				continue;
> +
> +			if (!connector->polled && I915_HAS_HOTPLUG(dev) &&
> +			    intel_connector->encoder->hpd_pin > HPD_NONE)
> +				connector->polled = DRM_CONNECTOR_POLL_HPD;
> +		}
>  	}
>  
> +	intel_hpd_poll_enable(dev_priv, false);
> +
>  	/*
>  	 * Interrupt setup is already guaranteed to be single-threaded, this
> is
>  	 * just to make the assert_spin_locked checks happy.
> @@ -488,10 +502,57 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
>  
> +void i915_hpd_poll_init_work(struct work_struct *work) {
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, struct drm_i915_private,
> +			     hotplug.poll_enable_work);
> +	struct drm_device *dev = dev_priv->dev;
> +	struct intel_connector *intel_connector;
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +
> +	for_each_intel_connector(dev, intel_connector) {
> +		struct drm_connector *connector = &intel_connector->base;
> +
> +		if (dev_priv->hotplug.poll_enabled) {
> +			connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> +				DRM_CONNECTOR_POLL_DISCONNECT;
> +
> +		} else if (!intel_connector->polled && I915_HAS_HOTPLUG(dev)
> &&
> +			   intel_connector->encoder->hpd_pin > HPD_NONE) {
> +			connector->polled = DRM_CONNECTOR_POLL_HPD;
> +		}
> +	}
> +
> +	if (dev_priv->hotplug.poll_enabled)
> +		drm_kms_helper_poll_enable_locked(dev);
> +
> +	mutex_unlock(&dev->mode_config.mutex);
> +
> +	/*
> +	 * We might have missed any hotplugs that happened while we were
> +	 * in the middle of disabling polling
> +	 */
> +	if (!dev_priv->hotplug.poll_enabled)
> +		drm_helper_hpd_irq_event(dev);
> +}
> +
> +void intel_hpd_poll_enable(struct drm_i915_private *dev_priv, bool enabled)
> +{
> +	dev_priv->hotplug.poll_enabled = enabled;
> +
> +	/*
> +	 * We might already be holding dev->mode_config.mutex, so do this in
> a
> +	 * seperate worker
> +	 */
> +	schedule_work(&dev_priv->hotplug.poll_enable_work);
> +}
> +
>  void intel_hpd_init_work(struct drm_i915_private *dev_priv)
>  {
>  	INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
>  	INIT_WORK(&dev_priv->hotplug.dig_port_work, i915_digport_work_func);
> +	INIT_WORK(&dev_priv->hotplug.poll_enable_work,
> i915_hpd_poll_init_work);
>  	INIT_DELAYED_WORK(&dev_priv->hotplug.reenable_work,
>  			  intel_hpd_irq_storm_reenable_work);
>  }
> @@ -508,6 +569,7 @@ void intel_hpd_cancel_work(struct drm_i915_private
> *dev_priv)
>  
>  	cancel_work_sync(&dev_priv->hotplug.dig_port_work);
>  	cancel_work_sync(&dev_priv->hotplug.hotplug_work);
> +	cancel_work_sync(&dev_priv->hotplug.poll_enable_work);
>  	cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 6f3d019..592cb7d 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1099,6 +1099,7 @@ static void vlv_display_power_well_init(struct
> drm_i915_private *dev_priv)
>  	if (dev_priv->power_domains.initializing)
>  		return;
>  
> +	intel_hpd_poll_enable(dev_priv, false);
>  	intel_hpd_init(dev_priv);
>  
>  	/* Re-enable the ADPA, if we have one */
> @@ -1120,6 +1121,8 @@ static void vlv_display_power_well_deinit(struct
> drm_i915_private *dev_priv)
>  	synchronize_irq(dev_priv->dev->irq);
>  
>  	vlv_power_sequencer_reset(dev_priv);
> +
> +	intel_hpd_poll_enable(dev_priv, true);
>  }
>  
>  static void vlv_display_power_well_enable(struct drm_i915_private *dev_priv,

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3eb47fb..5381d9e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1612,6 +1612,9 @@  static int intel_runtime_suspend(struct device *device)
 
 	assert_forcewakes_inactive(dev_priv);
 
+	if (!IS_VALLEYVIEW(dev_priv) || !IS_CHERRYVIEW(dev_priv))
+		intel_hpd_poll_enable(dev_priv, true);
+
 	DRM_DEBUG_KMS("Device suspended\n");
 	return 0;
 }
@@ -1667,8 +1670,10 @@  static int intel_runtime_resume(struct device *device)
 	 * 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_enable(dev_priv, false);
+	}
 
 	intel_enable_gt_powersave(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1eef5ef..6f7075d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -283,6 +283,9 @@  struct i915_hotplug {
 	u32 short_port_mask;
 	struct work_struct dig_port_work;
 
+	struct work_struct poll_enable_work;
+	bool poll_enabled;
+
 	/*
 	 * if we get a HPD irq from DP and a HPD irq from non-DP
 	 * the non-DP HPD could block the workqueue on a mode config
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cd14751..87c5143 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1384,6 +1384,8 @@  int intel_dsi_dcs_init_backlight_funcs(struct intel_connector *intel_connector);
 
 /* intel_dvo.c */
 void intel_dvo_init(struct drm_device *dev);
+/* intel_hotplug.c */
+void intel_hpd_poll_enable(struct drm_i915_private *dev_priv, bool enabled);
 
 
 /* legacy fbdev emulation in intel_fbdev.c */
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index ec3285f..3ef5096 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -464,20 +464,34 @@  void intel_hpd_init(struct drm_i915_private *dev_priv)
 		dev_priv->hotplug.stats[i].count = 0;
 		dev_priv->hotplug.stats[i].state = HPD_ENABLED;
 	}
-	list_for_each_entry(connector, &mode_config->connector_list, head) {
-		struct intel_connector *intel_connector = to_intel_connector(connector);
-		connector->polled = intel_connector->polled;
 
-		/* MST has a dynamic intel_connector->encoder and it's reprobing
-		 * is all handled by the MST helpers. */
-		if (intel_connector->mst_port)
-			continue;
+	/*
+	 * When we're in the midst of doing connector polling in situations
+	 * where we don't have working hpd, we need to make sure we don't try
+	 * changing the polling settings of any connectors, since this can lead
+	 * to us setting connector->polled to DRM_CONNECTOR_POLL_HPD, resulting
+	 * in DRM's polling helpers automatically skipping the rest of the
+	 * connectors that still need polling
+	 */
+	if (!mode_config->poll_running) {
+		list_for_each_entry(connector, &mode_config->connector_list, head) {
+			struct intel_connector *intel_connector =
+				to_intel_connector(connector);
+			connector->polled = intel_connector->polled;
 
-		if (!connector->polled && I915_HAS_HOTPLUG(dev) &&
-		    intel_connector->encoder->hpd_pin > HPD_NONE)
-			connector->polled = DRM_CONNECTOR_POLL_HPD;
+			/* MST has a dynamic intel_connector->encoder and it's
+			 * reprobing is all handled by the MST helpers. */
+			if (intel_connector->mst_port)
+				continue;
+
+			if (!connector->polled && I915_HAS_HOTPLUG(dev) &&
+			    intel_connector->encoder->hpd_pin > HPD_NONE)
+				connector->polled = DRM_CONNECTOR_POLL_HPD;
+		}
 	}
 
+	intel_hpd_poll_enable(dev_priv, false);
+
 	/*
 	 * Interrupt setup is already guaranteed to be single-threaded, this is
 	 * just to make the assert_spin_locked checks happy.
@@ -488,10 +502,57 @@  void intel_hpd_init(struct drm_i915_private *dev_priv)
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
 
+void i915_hpd_poll_init_work(struct work_struct *work) {
+	struct drm_i915_private *dev_priv =
+		container_of(work, struct drm_i915_private,
+			     hotplug.poll_enable_work);
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_connector *intel_connector;
+
+	mutex_lock(&dev->mode_config.mutex);
+
+	for_each_intel_connector(dev, intel_connector) {
+		struct drm_connector *connector = &intel_connector->base;
+
+		if (dev_priv->hotplug.poll_enabled) {
+			connector->polled = DRM_CONNECTOR_POLL_CONNECT |
+				DRM_CONNECTOR_POLL_DISCONNECT;
+
+		} else if (!intel_connector->polled && I915_HAS_HOTPLUG(dev) &&
+			   intel_connector->encoder->hpd_pin > HPD_NONE) {
+			connector->polled = DRM_CONNECTOR_POLL_HPD;
+		}
+	}
+
+	if (dev_priv->hotplug.poll_enabled)
+		drm_kms_helper_poll_enable_locked(dev);
+
+	mutex_unlock(&dev->mode_config.mutex);
+
+	/*
+	 * We might have missed any hotplugs that happened while we were
+	 * in the middle of disabling polling
+	 */
+	if (!dev_priv->hotplug.poll_enabled)
+		drm_helper_hpd_irq_event(dev);
+}
+
+void intel_hpd_poll_enable(struct drm_i915_private *dev_priv, bool enabled)
+{
+	dev_priv->hotplug.poll_enabled = enabled;
+
+	/*
+	 * We might already be holding dev->mode_config.mutex, so do this in a
+	 * seperate worker
+	 */
+	schedule_work(&dev_priv->hotplug.poll_enable_work);
+}
+
 void intel_hpd_init_work(struct drm_i915_private *dev_priv)
 {
 	INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
 	INIT_WORK(&dev_priv->hotplug.dig_port_work, i915_digport_work_func);
+	INIT_WORK(&dev_priv->hotplug.poll_enable_work, i915_hpd_poll_init_work);
 	INIT_DELAYED_WORK(&dev_priv->hotplug.reenable_work,
 			  intel_hpd_irq_storm_reenable_work);
 }
@@ -508,6 +569,7 @@  void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
 
 	cancel_work_sync(&dev_priv->hotplug.dig_port_work);
 	cancel_work_sync(&dev_priv->hotplug.hotplug_work);
+	cancel_work_sync(&dev_priv->hotplug.poll_enable_work);
 	cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6f3d019..592cb7d 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1099,6 +1099,7 @@  static void vlv_display_power_well_init(struct drm_i915_private *dev_priv)
 	if (dev_priv->power_domains.initializing)
 		return;
 
+	intel_hpd_poll_enable(dev_priv, false);
 	intel_hpd_init(dev_priv);
 
 	/* Re-enable the ADPA, if we have one */
@@ -1120,6 +1121,8 @@  static void vlv_display_power_well_deinit(struct drm_i915_private *dev_priv)
 	synchronize_irq(dev_priv->dev->irq);
 
 	vlv_power_sequencer_reset(dev_priv);
+
+	intel_hpd_poll_enable(dev_priv, true);
 }
 
 static void vlv_display_power_well_enable(struct drm_i915_private *dev_priv,