[PATCH 0/3] Fixes for HPD

[PATCH 0/3] Fixes for HPD

[Lyude]

These are a couple of patches intended to fix one of the big problems we have
with a lot of chipsets on i915 right now: in the various forms of suspend we
use in the driver, many of them break HPD while active and can lead to some
seriously confusing situations where they can't get their monitors to turn on.

The patches here for fixing Valleyview need to be used with Ville Syrjälä's
patchset for (partially?) preventing valleyview from getting in an infinite hpd
detect loop when doing polling:

https://patchwork.freedesktop.org/series/5884/

It should also be noted some of these are resends, since the original patches
never got picked up by patchwork

Lyude (3):
  drm/i915/vlv: Make intel_crt_reset() per-encoder
  drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init()
  drm/i915: Enable polling when we don't have hpd

Cc: stable@vger.kernel.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>

 drivers/gpu/drm/i915/i915_drv.c         |  7 +++-
 drivers/gpu/drm/i915/i915_drv.h         |  3 ++
 drivers/gpu/drm/i915/intel_crt.c        | 10 ++---
 drivers/gpu/drm/i915/intel_drv.h        |  4 +-
 drivers/gpu/drm/i915/intel_hotplug.c    | 69 ++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_runtime_pm.c | 10 +++++
 6 files changed, 86 insertions(+), 17 deletions(-)

-- 
2.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH RESEND 1/3] drm/i915/vlv: Make intel_crt_reset() per-encoder

[Lyude]

This lets call intel_crt_reset() in contexts where IRQs are disabled and
as such, can't hold the locks required to work with the connectors.

Cc: stable@vger.kernel.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/intel_crt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 3fbb6fc..e4dc33e 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -713,11 +713,11 @@ static int intel_crt_set_property(struct drm_connector *connector,
 	return 0;
 }
 
-static void intel_crt_reset(struct drm_connector *connector)
+static void intel_crt_reset(struct drm_encoder *encoder)
 {
-	struct drm_device *dev = connector->dev;
+	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crt *crt = intel_attached_crt(connector);
+	struct intel_crt *crt = intel_encoder_to_crt(to_intel_encoder(encoder));
 
 	if (INTEL_INFO(dev)->gen >= 5) {
 		u32 adpa;
@@ -739,7 +739,6 @@ static void intel_crt_reset(struct drm_connector *connector)
  */
 
 static const struct drm_connector_funcs intel_crt_connector_funcs = {
-	.reset = intel_crt_reset,
 	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_crt_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
@@ -757,6 +756,7 @@ static const struct drm_connector_helper_funcs intel_crt_connector_helper_funcs
 };
 
 static const struct drm_encoder_funcs intel_crt_enc_funcs = {
+	.reset = intel_crt_reset,
 	.destroy = intel_encoder_destroy,
 };
 
@@ -902,5 +902,5 @@ void intel_crt_init(struct drm_device *dev)
 		dev_priv->fdi_rx_config = I915_READ(FDI_RX_CTL(PIPE_A)) & fdi_config;
 	}
 
-	intel_crt_reset(connector);
+	intel_crt_reset(&crt->base.base);
 }
-- 
2.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH RESEND 2/3] drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init()

[Lyude]

While VGA hotplugging worked(ish) before, it looks like that was mainly
because we'd unintentionally enable it in
valleyview_crt_detect_hotplug() when we did a force trigger. This
doesn't work reliably enough because whenever the display powerwell on
vlv gets disabled, the values set in VLV_ADPA get cleared and
consequently VGA hotplugging gets disabled. This causes bugs such as one
we found on an Intel NUC, where doing the following sequence of
hotplugs:

      - Disconnect all monitors
      - Connect VGA
      - Disconnect VGA
      - Connect HDMI

Would result in VGA hotplugging becoming disabled, due to the powerwells
getting toggled in the process of connecting HDMI.

Changes since v3:
 - Expose intel_crt_reset() through intel_drv.h and call that in
   vlv_display_power_well_init() instead of
   encoder->base.funcs->reset(&encoder->base);

Changes since v2:
 - Use intel_encoder structs instead of drm_encoder structs

Changes since v1:
 - Instead of handling the register writes ourself, we just reuse
   intel_crt_detect()
 - Instead of resetting the ADPA during display IRQ installation, we now
   reset them in vlv_display_power_well_init()

Cc: stable@vger.kernel.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Lyude <cpaul@redhat.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c        | 2 +-
 drivers/gpu/drm/i915/intel_drv.h        | 2 +-
 drivers/gpu/drm/i915/intel_runtime_pm.c | 7 +++++++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index e4dc33e..d0fb961 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -713,7 +713,7 @@ static int intel_crt_set_property(struct drm_connector *connector,
 	return 0;
 }
 
-static void intel_crt_reset(struct drm_encoder *encoder)
+void intel_crt_reset(struct drm_encoder *encoder)
 {
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4a24b00..fdec45d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1054,7 +1054,7 @@ void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
 
 /* intel_crt.c */
 void intel_crt_init(struct drm_device *dev);
-
+void intel_crt_reset(struct drm_encoder *encoder);
 
 /* intel_ddi.c */
 void intel_ddi_clk_select(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 7fb1da4..4a3fd3a 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -952,6 +952,7 @@ static void vlv_init_display_clock_gating(struct drm_i915_private *dev_priv)
 
 static void vlv_display_power_well_init(struct drm_i915_private *dev_priv)
 {
+	struct intel_encoder *encoder;
 	enum pipe pipe;
 
 	/*
@@ -987,6 +988,12 @@ static void vlv_display_power_well_init(struct drm_i915_private *dev_priv)
 
 	intel_hpd_init(dev_priv);
 
+	/* Re-enable the ADPA, if we have one */
+	for_each_intel_encoder(dev_priv->dev, encoder) {
+		if (encoder->type == INTEL_OUTPUT_ANALOG)
+			intel_crt_reset(&encoder->base);
+	}
+
 	i915_redisable_vga_power_on(dev_priv->dev);
 }
 
-- 
2.5.5

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

[Lyude]

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.

Cc: stable@vger.kernel.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Lyude <cpaul@redhat.com>
---
 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    | 69 ++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_runtime_pm.c |  3 ++
 5 files changed, 73 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f313b4d..a593889 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1574,6 +1574,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;
 }
@@ -1629,8 +1632,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);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7c334e9..3062d55 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -281,6 +281,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 fdec45d..80cd626 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1348,6 +1348,8 @@ void intel_dsi_init(struct drm_device *dev);
 
 /* 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 bee6730..0fc2fe0 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -465,20 +465,25 @@ 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;
+	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;
 
-		/* MST has a dynamic intel_connector->encoder and it's reprobing
-		 * is all handled by the MST helpers. */
-		if (intel_connector->mst_port)
-			continue;
+			/* 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;
+			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.
@@ -489,10 +494,54 @@ 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;
+
+	/* Don't let this run before we setup polling in the driver */
+	if (!dev->mode_config.poll_enabled)
+		return;
+
+	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);
+}
+
+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);
 }
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 4a3fd3a..0090e88 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -986,6 +986,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 */
@@ -1007,6 +1008,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,
-- 
2.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Enable polling when we don't have hpd

[Daniel Vetter]

On Fri, Jun 17, 2016 at 05:58:24PM -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.
> 
> Cc: stable@vger.kernel.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Lyude <cpaul@redhat.com>
> ---
>  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    | 69 ++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 ++
>  5 files changed, 73 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f313b4d..a593889 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1574,6 +1574,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;
>  }
> @@ -1629,8 +1632,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);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7c334e9..3062d55 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -281,6 +281,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 fdec45d..80cd626 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1348,6 +1348,8 @@ void intel_dsi_init(struct drm_device *dev);
>  
>  /* 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 bee6730..0fc2fe0 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -465,20 +465,25 @@ 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;
> +	if (!mode_config->poll_running) {

Why do we need this new condition?

> +		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;
> +			/* 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;
> +			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.
> @@ -489,10 +494,54 @@ 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;
> +
> +	/* Don't let this run before we setup polling in the driver */
> +	if (!dev->mode_config.poll_enabled)
> +		return;

Same here, why this check? rpm/power-well handling only starts once the
driver is fully loaded. We unconditionally hold a ref to every power well
while loading the driver, to avoid such complications.

> +
> +	mutex_lock(&dev->mode_config.mutex);

One issue Ville noticed: When we go from polling->hpd we might have missed
a hotplug between the last poll round and before hpd was working again. So
for that case I think we need to manually probe everything once more.
Calling drm_helper_hpd_irq_event() should get the job done.

> +
> +	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);
> +}
> +
> +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);

Need a cancel_work_sync in system suspend and driver unload paths to make
sure we don't leak this one here accidentlly.

Beside these nitpicks looks good, and if it works would be surprisingly
simple indeed. I'd say so better safe than sorry, and we should soak this
for at least 1 week in -next before cherry-picking over to -fixes. It is
rather risky, and the bug has been around since a long time so not worth
it much to rush too much.

Also, I'll be going on vacation. With the above issues addresss,
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> on the entire series.
-Daniel

> +}
> +
>  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);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 4a3fd3a..0090e88 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -986,6 +986,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 */
> @@ -1007,6 +1008,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,
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Fixes for HPD

[Lyude]

Forgot to mention, Ville: if you could get me an example of how to get
vlv into an infinite loop with these patches I'd appreciate that. I
haven't been able to reproduce this at all with the Valleyview machine
I've got here.

On Fri, 2016-06-17 at 17:58 -0400, Lyude wrote:
> These are a couple of patches intended to fix one of the big problems
> we have
> with a lot of chipsets on i915 right now: in the various forms of
> suspend we
> use in the driver, many of them break HPD while active and can lead
> to some
> seriously confusing situations where they can't get their monitors to
> turn on.
> 
> The patches here for fixing Valleyview need to be used with Ville
> Syrjälä's
> patchset for (partially?) preventing valleyview from getting in an
> infinite hpd
> detect loop when doing polling:
> 
> https://patchwork.freedesktop.org/series/5884/
> 
> It should also be noted some of these are resends, since the original
> patches
> never got picked up by patchwork
> 
> Lyude (3):
>   drm/i915/vlv: Make intel_crt_reset() per-encoder
>   drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init()
>   drm/i915: Enable polling when we don't have hpd
> 
> Cc: stable@vger.kernel.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> 
>  drivers/gpu/drm/i915/i915_drv.c         |  7 +++-
>  drivers/gpu/drm/i915/i915_drv.h         |  3 ++
>  drivers/gpu/drm/i915/intel_crt.c        | 10 ++---
>  drivers/gpu/drm/i915/intel_drv.h        |  4 +-
>  drivers/gpu/drm/i915/intel_hotplug.c    | 69
> ++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 10 +++++
>  6 files changed, 86 insertions(+), 17 deletions(-)
> 
-- 
Cheers,
	Lyude

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/3] Fixes for HPD

[Ville Syrjälä]

On Fri, Jun 17, 2016 at 06:04:11PM -0400, Lyude wrote:
> Forgot to mention, Ville: if you could get me an example of how to get
> vlv into an infinite loop with these patches I'd appreciate that. I
> haven't been able to reproduce this at all with the Valleyview machine
> I've got here.

Whether it goes totally wild or not might depend on amount of
logging/debug features/etc. But it's not terribly hard to see
that there is a problem by just reading the code.

> 
> On Fri, 2016-06-17 at 17:58 -0400, Lyude wrote:
> > These are a couple of patches intended to fix one of the big problems
> > we have
> > with a lot of chipsets on i915 right now: in the various forms of
> > suspend we
> > use in the driver, many of them break HPD while active and can lead
> > to some
> > seriously confusing situations where they can't get their monitors to
> > turn on.
> > 
> > The patches here for fixing Valleyview need to be used with Ville
> > Syrjälä's
> > patchset for (partially?) preventing valleyview from getting in an
> > infinite hpd
> > detect loop when doing polling:
> > 
> > https://patchwork.freedesktop.org/series/5884/
> > 
> > It should also be noted some of these are resends, since the original
> > patches
> > never got picked up by patchwork
> > 
> > Lyude (3):
> >   drm/i915/vlv: Make intel_crt_reset() per-encoder
> >   drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init()
> >   drm/i915: Enable polling when we don't have hpd
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > 
> >  drivers/gpu/drm/i915/i915_drv.c         |  7 +++-
> >  drivers/gpu/drm/i915/i915_drv.h         |  3 ++
> >  drivers/gpu/drm/i915/intel_crt.c        | 10 ++---
> >  drivers/gpu/drm/i915/intel_drv.h        |  4 +-
> >  drivers/gpu/drm/i915/intel_hotplug.c    | 69
> > ++++++++++++++++++++++++++++-----
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 10 +++++
> >  6 files changed, 86 insertions(+), 17 deletions(-)
> > 
> -- 
> Cheers,
> 	Lyude

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for Fixes for HPD

[Patchwork]

== Series Details ==

Series: Fixes for HPD
URL   : https://patchwork.freedesktop.org/series/8870/
State : failure

== Summary ==

Applying: drm/i915/vlv: Make intel_crt_reset() per-encoder
Applying: drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init()
Applying: drm/i915: Enable polling when we don't have hpd
fatal: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_drv.h).
error: could not build fake ancestor
Patch failed at 0003 drm/i915: Enable polling when we don't have hpd
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Ro.CI.BAT: failure for Fixes for HPD

[Daniel Vetter]

On Sat, Jun 18, 2016 at 06:09:48AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: Fixes for HPD
> URL   : https://patchwork.freedesktop.org/series/8870/
> State : failure
> 
> == Summary ==
> 
> Applying: drm/i915/vlv: Make intel_crt_reset() per-encoder
> Applying: drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init()
> Applying: drm/i915: Enable polling when we don't have hpd
> fatal: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_drv.h).
> error: could not build fake ancestor
> Patch failed at 0003 drm/i915: Enable polling when we don't have hpd
> The copy of the patch that failed is found in: .git/rebase-apply/patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".

For the next round, please make sure your series applies cleanly on top of
drm-intel-nightly. We'll cherry-pick to -fixes later on anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx