[2/5] drm/i915/irq: abstract irq storm hotplug disabling
diff mbox

Message ID c3e22fbaed7b3fbdd54846f7e492bfee31d20149.1434621691.git.jani.nikula@intel.com
State New
Headers show

Commit Message

Jani Nikula June 18, 2015, 10:06 a.m. UTC
Continue abstracting hotplug storm related functions to clarify the
code. This time, abstract hotplug irq storm related hotplug
disabling. While at it, clean up the loop iterating over connectors for
readability.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 77 ++++++++++++++++++++++++++---------------
 1 file changed, 50 insertions(+), 27 deletions(-)

Comments

Daniel Vetter June 22, 2015, 1:02 p.m. UTC | #1
On Thu, Jun 18, 2015 at 01:06:14PM +0300, Jani Nikula wrote:
> Continue abstracting hotplug storm related functions to clarify the
> code. This time, abstract hotplug irq storm related hotplug
> disabling. While at it, clean up the loop iterating over connectors for
> readability.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 77 ++++++++++++++++++++++++++---------------
>  1 file changed, 50 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d64d6895a2e5..bf4c15d0ea2b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -879,7 +879,7 @@ static void i915_digport_work_func(struct work_struct *work)
>  /*
>   * Handle hotplug events outside the interrupt handler proper.
>   */
> -#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
> +static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv);
>  
>  static void i915_hotplug_work_func(struct work_struct *work)
>  {
> @@ -890,7 +890,6 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  	struct intel_connector *intel_connector;
>  	struct intel_encoder *intel_encoder;
>  	struct drm_connector *connector;
> -	bool hpd_disabled = false;
>  	bool changed = false;
>  	u32 hpd_event_bits;
>  
> @@ -901,31 +900,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  
>  	hpd_event_bits = dev_priv->hotplug.event_bits;
>  	dev_priv->hotplug.event_bits = 0;
> -	list_for_each_entry(connector, &mode_config->connector_list, head) {

Random comment: We have piles of connector_list walking in probe codde,
and DP MST adds/removes them without much thought really users of these.
Dave? Do we need a connector_list spinlock?

Just grabbing one of the modeset locks won't cut it I think since it'll
serialize way too much.
-Daniel
Dave Airlie June 23, 2015, 12:29 a.m. UTC | #2
On 22 June 2015 at 23:02, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Jun 18, 2015 at 01:06:14PM +0300, Jani Nikula wrote:
>> Continue abstracting hotplug storm related functions to clarify the
>> code. This time, abstract hotplug irq storm related hotplug
>> disabling. While at it, clean up the loop iterating over connectors for
>> readability.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 77 ++++++++++++++++++++++++++---------------
>>  1 file changed, 50 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index d64d6895a2e5..bf4c15d0ea2b 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -879,7 +879,7 @@ static void i915_digport_work_func(struct work_struct *work)
>>  /*
>>   * Handle hotplug events outside the interrupt handler proper.
>>   */
>> -#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
>> +static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv);
>>
>>  static void i915_hotplug_work_func(struct work_struct *work)
>>  {
>> @@ -890,7 +890,6 @@ static void i915_hotplug_work_func(struct work_struct *work)
>>       struct intel_connector *intel_connector;
>>       struct intel_encoder *intel_encoder;
>>       struct drm_connector *connector;
>> -     bool hpd_disabled = false;
>>       bool changed = false;
>>       u32 hpd_event_bits;
>>
>> @@ -901,31 +900,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
>>
>>       hpd_event_bits = dev_priv->hotplug.event_bits;
>>       dev_priv->hotplug.event_bits = 0;
>> -     list_for_each_entry(connector, &mode_config->connector_list, head) {
>
> Random comment: We have piles of connector_list walking in probe codde,
> and DP MST adds/removes them without much thought really users of these.
> Dave? Do we need a connector_list spinlock?
>
> Just grabbing one of the modeset locks won't cut it I think since it'll
> serialize way too much.

In my tree currently this code grabs mode_config->mutex, so it should
be fine wrt
the connector list disappearing, as removing connectors is protected
my mode_config->mutex
as well.

if we do add a spinlock, we need to be careful about dropping it and restarting
the loops etc, as I would guess there are a fair few paths we don't
want to descend
holding the spin lock, due to possible sleeping/scheduling etc.

I think the intel_hpd_irq_reenable_work is the one that stands out to
me, the short
hotplug paths don't traverse connector lists deliberately to avoid the problem.

Dave.
Daniel Vetter June 23, 2015, 6:36 a.m. UTC | #3
On Tue, Jun 23, 2015 at 10:29:27AM +1000, Dave Airlie wrote:
> On 22 June 2015 at 23:02, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Jun 18, 2015 at 01:06:14PM +0300, Jani Nikula wrote:
> >> Continue abstracting hotplug storm related functions to clarify the
> >> code. This time, abstract hotplug irq storm related hotplug
> >> disabling. While at it, clean up the loop iterating over connectors for
> >> readability.
> >>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_irq.c | 77 ++++++++++++++++++++++++++---------------
> >>  1 file changed, 50 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> index d64d6895a2e5..bf4c15d0ea2b 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -879,7 +879,7 @@ static void i915_digport_work_func(struct work_struct *work)
> >>  /*
> >>   * Handle hotplug events outside the interrupt handler proper.
> >>   */
> >> -#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
> >> +static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv);
> >>
> >>  static void i915_hotplug_work_func(struct work_struct *work)
> >>  {
> >> @@ -890,7 +890,6 @@ static void i915_hotplug_work_func(struct work_struct *work)
> >>       struct intel_connector *intel_connector;
> >>       struct intel_encoder *intel_encoder;
> >>       struct drm_connector *connector;
> >> -     bool hpd_disabled = false;
> >>       bool changed = false;
> >>       u32 hpd_event_bits;
> >>
> >> @@ -901,31 +900,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
> >>
> >>       hpd_event_bits = dev_priv->hotplug.event_bits;
> >>       dev_priv->hotplug.event_bits = 0;
> >> -     list_for_each_entry(connector, &mode_config->connector_list, head) {
> >
> > Random comment: We have piles of connector_list walking in probe codde,
> > and DP MST adds/removes them without much thought really users of these.
> > Dave? Do we need a connector_list spinlock?
> >
> > Just grabbing one of the modeset locks won't cut it I think since it'll
> > serialize way too much.
> 
> In my tree currently this code grabs mode_config->mutex, so it should
> be fine wrt
> the connector list disappearing, as removing connectors is protected
> my mode_config->mutex
> as well.

Hm right I mixed things up and didn't realize this is protected by the
mode_config->mutex.

> if we do add a spinlock, we need to be careful about dropping it and restarting
> the loops etc, as I would guess there are a fair few paths we don't
> want to descend
> holding the spin lock, due to possible sleeping/scheduling etc.
> 
> I think the intel_hpd_irq_reenable_work is the one that stands out to
> me, the short
> hotplug paths don't traverse connector lists deliberately to avoid the problem.

I think grabbing the mode_config->mutex in there should be fine. Looks
like we can stretch just grabbing mode_config->mutex for a bit longer ;-)
Jani, can you perhaps throw this in with your next round of hpd cleanup?
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d64d6895a2e5..bf4c15d0ea2b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -879,7 +879,7 @@  static void i915_digport_work_func(struct work_struct *work)
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
-#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
+static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv);
 
 static void i915_hotplug_work_func(struct work_struct *work)
 {
@@ -890,7 +890,6 @@  static void i915_hotplug_work_func(struct work_struct *work)
 	struct intel_connector *intel_connector;
 	struct intel_encoder *intel_encoder;
 	struct drm_connector *connector;
-	bool hpd_disabled = false;
 	bool changed = false;
 	u32 hpd_event_bits;
 
@@ -901,31 +900,9 @@  static void i915_hotplug_work_func(struct work_struct *work)
 
 	hpd_event_bits = dev_priv->hotplug.event_bits;
 	dev_priv->hotplug.event_bits = 0;
-	list_for_each_entry(connector, &mode_config->connector_list, head) {
-		intel_connector = to_intel_connector(connector);
-		if (!intel_connector->encoder)
-			continue;
-		intel_encoder = intel_connector->encoder;
-		if (intel_encoder->hpd_pin > HPD_NONE &&
-		    dev_priv->hotplug.stats[intel_encoder->hpd_pin].state == HPD_MARK_DISABLED &&
-		    connector->polled == DRM_CONNECTOR_POLL_HPD) {
-			DRM_INFO("HPD interrupt storm detected on connector %s: "
-				 "switching from hotplug detection to polling\n",
-				connector->name);
-			dev_priv->hotplug.stats[intel_encoder->hpd_pin].state = HPD_DISABLED;
-			connector->polled = DRM_CONNECTOR_POLL_CONNECT
-				| DRM_CONNECTOR_POLL_DISCONNECT;
-			hpd_disabled = true;
-		}
-	}
-	 /* if there were no outputs to poll, poll was disabled,
-	  * therefore make sure it's enabled when disabling HPD on
-	  * some connectors */
-	if (hpd_disabled) {
-		drm_kms_helper_poll_enable(dev);
-		mod_delayed_work(system_wq, &dev_priv->hotplug.reenable_work,
-				 msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
-	}
+
+	/* Disable hotplug on connectors that hit an irq storm. */
+	intel_hpd_irq_storm_disable(dev_priv);
 
 	spin_unlock_irq(&dev_priv->irq_lock);
 
@@ -1411,6 +1388,52 @@  static bool intel_hpd_irq_storm(struct drm_i915_private *dev_priv,
 	return storm;
 }
 
+#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
+
+static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	struct intel_connector *intel_connector;
+	struct intel_encoder *intel_encoder;
+	struct drm_connector *connector;
+	enum hpd_pin pin;
+	bool hpd_disabled = false;
+
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	list_for_each_entry(connector, &mode_config->connector_list, head) {
+		if (connector->polled != DRM_CONNECTOR_POLL_HPD)
+			continue;
+
+		intel_connector = to_intel_connector(connector);
+		intel_encoder = intel_connector->encoder;
+		if (!intel_encoder)
+			continue;
+
+		pin = intel_encoder->hpd_pin;
+		if (pin == HPD_NONE ||
+		    dev_priv->hotplug.stats[pin].state != HPD_MARK_DISABLED)
+			continue;
+
+		DRM_INFO("HPD interrupt storm detected on connector %s: "
+			 "switching from hotplug detection to polling\n",
+			 connector->name);
+
+		dev_priv->hotplug.stats[pin].state = HPD_DISABLED;
+		connector->polled = DRM_CONNECTOR_POLL_CONNECT
+			| DRM_CONNECTOR_POLL_DISCONNECT;
+		hpd_disabled = true;
+	}
+
+	/* Enable polling and queue hotplug re-enabling. */
+	if (hpd_disabled) {
+		drm_kms_helper_poll_enable(dev);
+		mod_delayed_work(system_wq, &dev_priv->hotplug.reenable_work,
+				 msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
+	}
+}
+
 static bool pch_port_hotplug_long_detect(enum port port, u32 val)
 {
 	switch (port) {