diff mbox

[v3,5/7] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v3)

Message ID 1365499470-28646-6-git-send-email-eich@freedesktop.org (mailing list archive)
State New, archived
Headers show

Commit Message

Egbert Eich April 9, 2013, 9:24 a.m. UTC
From: Egbert Eich <eich@suse.de>

We disable hoptplug detection when we encounter a hotplug event
storm. Still hotplug detection is required on some outputs (like
Display Port). The interrupt storm may be only temporary (on certain
Dell Laptops for instance it happens at certain charging states of
the system). Thus we enable it after a certain grace period (2 minutes).
Should the interrupt storm persist it will be detected immediately
and it will be disabled again.

v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state tracker.
v3: Clarified loop start value,
    Removed superfluous test for Ivybridge and Haswell,
    Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/i915_drv.h |  2 ++
 drivers/gpu/drm/i915/i915_irq.c | 49 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

Comments

Jani Nikula April 11, 2013, 10:44 a.m. UTC | #1
On Tue, 09 Apr 2013, Egbert Eich <eich@freedesktop.org> wrote:
> From: Egbert Eich <eich@suse.de>
>
> We disable hoptplug detection when we encounter a hotplug event
> storm. Still hotplug detection is required on some outputs (like
> Display Port). The interrupt storm may be only temporary (on certain
> Dell Laptops for instance it happens at certain charging states of
> the system). Thus we enable it after a certain grace period (2 minutes).
> Should the interrupt storm persist it will be detected immediately
> and it will be disabled again.
>
> v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state tracker.
> v3: Clarified loop start value,
>     Removed superfluous test for Ivybridge and Haswell,
>     Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)
>
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  2 ++
>  drivers/gpu/drm/i915/i915_irq.c | 49 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 83fc1a6..a3ed2e3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -938,6 +938,8 @@ typedef struct drm_i915_private {
>  			HPD_MARK_DISABLED = 2
>  		} hpd_mark;
>  	} hpd_stats[HPD_NUM_PINS];
> +#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)

It's already too crowded here, please move this #define to i915_irq.c.

> +	struct timer_list hotplug_reenable_timer;
>  
>  	int num_pch_pll;
>  	int num_plane;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d0e6f6d..1a00533 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -371,8 +371,11 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  			connector_disabled = true;
>  		}
>  	}
> -	if (connector_disabled)
> +	if (connector_disabled) {
>  		drm_kms_helper_poll_enable(dev); /* if there were no outputs to poll, poll is disabled */
> +		mod_timer(&dev_priv->hotplug_reenable_timer,
> +			  jiffies + msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
> +	}
>  
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
> @@ -2348,6 +2351,8 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
>  	if (!dev_priv)
>  		return;
>  
> +	del_timer_sync(&dev_priv->hotplug_reenable_timer);
> +
>  	for_each_pipe(pipe)
>  		I915_WRITE(PIPESTAT(pipe), 0xffff);
>  
> @@ -2369,6 +2374,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>  	if (!dev_priv)
>  		return;
>  
> +	del_timer_sync(&dev_priv->hotplug_reenable_timer);
> +
>  	I915_WRITE(HWSTAM, 0xffffffff);
>  
>  	I915_WRITE(DEIMR, 0xffffffff);
> @@ -2745,6 +2752,8 @@ static void i915_irq_uninstall(struct drm_device * dev)
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	int pipe;
>  
> +	del_timer_sync(&dev_priv->hotplug_reenable_timer);
> +
>  	if (I915_HAS_HOTPLUG(dev)) {
>  		I915_WRITE(PORT_HOTPLUG_EN, 0);
>  		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> @@ -2989,6 +2998,8 @@ static void i965_irq_uninstall(struct drm_device * dev)
>  	if (!dev_priv)
>  		return;
>  
> +	del_timer_sync(&dev_priv->hotplug_reenable_timer);
> +
>  	I915_WRITE(PORT_HOTPLUG_EN, 0);
>  	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>  
> @@ -3004,6 +3015,40 @@ static void i965_irq_uninstall(struct drm_device * dev)
>  	I915_WRITE(IIR, I915_READ(IIR));
>  }
>  
> +static void i915_reenable_hotplug_timer_func(unsigned long data)
> +{
> +	drm_i915_private_t *dev_priv = (drm_i915_private_t *)data;
> +	struct drm_device *dev = dev_priv->dev;
> +	struct drm_mode_config *mode_config = &dev->mode_config;
> +	unsigned long irqflags;
> +	int i;
> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +	for (i = (HPD_NONE + 1); i < HPD_NUM_PINS; i++) {
> +		struct drm_connector *connector;
> +
> +		if (dev_priv->hpd_stats[i].hpd_mark != HPD_MARK_DISABLED)

I think this is wrong, skipping HPD_DISABLED.

> +			continue;
> +
> +		dev_priv->hpd_stats[i].hpd_mark = HPD_ENABLED;
> +
> +		list_for_each_entry(connector, &mode_config->connector_list, head) {
> +			struct intel_connector *intel_connector = to_intel_connector(connector);
> +
> +			if (intel_connector->encoder->hpd_pin == i) {
> +				if (connector->polled != intel_connector->polled)
> +					DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
> +							 drm_get_connector_name(connector));
> +				connector->polled = intel_connector->polled;
> +				if (!connector->polled)
> +					connector->polled = DRM_CONNECTOR_POLL_HPD;
> +			}
> +		}
> +		dev_priv->display.hpd_irq_setup(dev);

You don't need to call this at each iteration, do you?

> +	}

In fact, couldn't you just call intel_hpd_init(), or modify it to do
what you want? Keep it simple.

> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +}
> +
>  void intel_irq_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3016,6 +3061,8 @@ void intel_irq_init(struct drm_device *dev)
>  	setup_timer(&dev_priv->gpu_error.hangcheck_timer,
>  		    i915_hangcheck_elapsed,
>  		    (unsigned long) dev);
> +	setup_timer(&dev_priv->hotplug_reenable_timer, i915_reenable_hotplug_timer_func,
> +		    (unsigned long) dev_priv);
>  
>  	pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
>  
> -- 
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Egbert Eich April 11, 2013, 1:10 p.m. UTC | #2
On Thu, Apr 11, 2013 at 01:44:51PM +0300, Jani Nikula wrote:
> > +
> > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > +	for (i = (HPD_NONE + 1); i < HPD_NUM_PINS; i++) {
> > +		struct drm_connector *connector;
> > +
> > +		if (dev_priv->hpd_stats[i].hpd_mark != HPD_MARK_DISABLED)
> 
> I think this is wrong, skipping HPD_DISABLED.

Right, this was indeed wrong.

> 
> > +			continue;
> > +
> > +		dev_priv->hpd_stats[i].hpd_mark = HPD_ENABLED;
> > +
> > +		list_for_each_entry(connector, &mode_config->connector_list, head) {
> > +			struct intel_connector *intel_connector = to_intel_connector(connector);
> > +
> > +			if (intel_connector->encoder->hpd_pin == i) {
> > +				if (connector->polled != intel_connector->polled)
> > +					DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
> > +							 drm_get_connector_name(connector));
> > +				connector->polled = intel_connector->polled;
> > +				if (!connector->polled)
> > +					connector->polled = DRM_CONNECTOR_POLL_HPD;
> > +			}
> > +		}
> > +		dev_priv->display.hpd_irq_setup(dev);
> 
> You don't need to call this at each iteration, do you?

Right, you are right here as well.
> 
> > +	}
> 
> In fact, couldn't you just call intel_hpd_init(), or modify it to do
> what you want? Keep it simple.

Well, intel_hpd_init() is initializing every to dev_priv->hpd_stats[i].hpd_mark
to HPD_ENABLED. Can we rule out that the timer runs between the interrupt
handler and the worker - as in this state this variable might me set to
HPD_MARK_DISABLED?
In fact it would probably not even hurt if we changed the HPD_MARK_DISABLED
to HPD_ENABLED as in a storm condition this condition will soon reappear but
I'd rather avoid it.
Of course we could pass an argument to the function to distinguish both
conditions. This is a simplification which can still be introduced - when
I'm in fact able to do some testing.

Thanks a lot for the review!

Cheers,
	Egbert.
Jani Nikula April 11, 2013, 2:48 p.m. UTC | #3
On Thu, 11 Apr 2013, Egbert Eich <eich@freedesktop.org> wrote:
> On Thu, Apr 11, 2013 at 01:44:51PM +0300, Jani Nikula wrote:
>> > +
>> > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>> > +	for (i = (HPD_NONE + 1); i < HPD_NUM_PINS; i++) {
>> > +		struct drm_connector *connector;
>> > +
>> > +		if (dev_priv->hpd_stats[i].hpd_mark != HPD_MARK_DISABLED)
>> 
>> I think this is wrong, skipping HPD_DISABLED.
>
> Right, this was indeed wrong.
>
>> 
>> > +			continue;
>> > +
>> > +		dev_priv->hpd_stats[i].hpd_mark = HPD_ENABLED;
>> > +
>> > +		list_for_each_entry(connector, &mode_config->connector_list, head) {
>> > +			struct intel_connector *intel_connector = to_intel_connector(connector);
>> > +
>> > +			if (intel_connector->encoder->hpd_pin == i) {
>> > +				if (connector->polled != intel_connector->polled)
>> > +					DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
>> > +							 drm_get_connector_name(connector));
>> > +				connector->polled = intel_connector->polled;
>> > +				if (!connector->polled)
>> > +					connector->polled = DRM_CONNECTOR_POLL_HPD;
>> > +			}
>> > +		}
>> > +		dev_priv->display.hpd_irq_setup(dev);
>> 
>> You don't need to call this at each iteration, do you?
>
> Right, you are right here as well.
>> 
>> > +	}
>> 
>> In fact, couldn't you just call intel_hpd_init(), or modify it to do
>> what you want? Keep it simple.
>
> Well, intel_hpd_init() is initializing every to dev_priv->hpd_stats[i].hpd_mark
> to HPD_ENABLED. Can we rule out that the timer runs between the interrupt
> handler and the worker - as in this state this variable might me set to
> HPD_MARK_DISABLED?
> In fact it would probably not even hurt if we changed the HPD_MARK_DISABLED
> to HPD_ENABLED as in a storm condition this condition will soon reappear but
> I'd rather avoid it.
> Of course we could pass an argument to the function to distinguish both
> conditions. This is a simplification which can still be introduced - when
> I'm in fact able to do some testing.

Yeah, let's go with this for now. R-b added.

One idea is to reuse the per-pin hpd_last_jiffies to check if enough
time has passed from the last storm on each pin. That could be done
here, or we could even throw out the timer, and check hpd_last_jiffies
on polling detect callbacks. Or maybe the latter is too spread out all
over the place. Perhaps you can think of something nice based on
this. ;)

> Thanks a lot for the review!

Thanks for doing this!


BR,
Jani.


>
> Cheers,
> 	Egbert.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 83fc1a6..a3ed2e3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -938,6 +938,8 @@  typedef struct drm_i915_private {
 			HPD_MARK_DISABLED = 2
 		} hpd_mark;
 	} hpd_stats[HPD_NUM_PINS];
+#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
+	struct timer_list hotplug_reenable_timer;
 
 	int num_pch_pll;
 	int num_plane;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d0e6f6d..1a00533 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -371,8 +371,11 @@  static void i915_hotplug_work_func(struct work_struct *work)
 			connector_disabled = true;
 		}
 	}
-	if (connector_disabled)
+	if (connector_disabled) {
 		drm_kms_helper_poll_enable(dev); /* if there were no outputs to poll, poll is disabled */
+		mod_timer(&dev_priv->hotplug_reenable_timer,
+			  jiffies + msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
+	}
 
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
@@ -2348,6 +2351,8 @@  static void valleyview_irq_uninstall(struct drm_device *dev)
 	if (!dev_priv)
 		return;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
 
@@ -2369,6 +2374,8 @@  static void ironlake_irq_uninstall(struct drm_device *dev)
 	if (!dev_priv)
 		return;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	I915_WRITE(HWSTAM, 0xffffffff);
 
 	I915_WRITE(DEIMR, 0xffffffff);
@@ -2745,6 +2752,8 @@  static void i915_irq_uninstall(struct drm_device * dev)
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	int pipe;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	if (I915_HAS_HOTPLUG(dev)) {
 		I915_WRITE(PORT_HOTPLUG_EN, 0);
 		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
@@ -2989,6 +2998,8 @@  static void i965_irq_uninstall(struct drm_device * dev)
 	if (!dev_priv)
 		return;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
@@ -3004,6 +3015,40 @@  static void i965_irq_uninstall(struct drm_device * dev)
 	I915_WRITE(IIR, I915_READ(IIR));
 }
 
+static void i915_reenable_hotplug_timer_func(unsigned long data)
+{
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *)data;
+	struct drm_device *dev = dev_priv->dev;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	unsigned long irqflags;
+	int i;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+	for (i = (HPD_NONE + 1); i < HPD_NUM_PINS; i++) {
+		struct drm_connector *connector;
+
+		if (dev_priv->hpd_stats[i].hpd_mark != HPD_MARK_DISABLED)
+			continue;
+
+		dev_priv->hpd_stats[i].hpd_mark = HPD_ENABLED;
+
+		list_for_each_entry(connector, &mode_config->connector_list, head) {
+			struct intel_connector *intel_connector = to_intel_connector(connector);
+
+			if (intel_connector->encoder->hpd_pin == i) {
+				if (connector->polled != intel_connector->polled)
+					DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
+							 drm_get_connector_name(connector));
+				connector->polled = intel_connector->polled;
+				if (!connector->polled)
+					connector->polled = DRM_CONNECTOR_POLL_HPD;
+			}
+		}
+		dev_priv->display.hpd_irq_setup(dev);
+	}
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
+
 void intel_irq_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3016,6 +3061,8 @@  void intel_irq_init(struct drm_device *dev)
 	setup_timer(&dev_priv->gpu_error.hangcheck_timer,
 		    i915_hangcheck_elapsed,
 		    (unsigned long) dev);
+	setup_timer(&dev_priv->hotplug_reenable_timer, i915_reenable_hotplug_timer_func,
+		    (unsigned long) dev_priv);
 
 	pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);