Re: [PATCH v3 4/7] drm/i915: Disable HPD interrupt on pin when irq storm is detected (v2)

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 09 Apr 2013, Egbert Eich <eich@freedesktop.org> wrote:
> From: Egbert Eich <eich@suse.de>
>
> This patch disables hotplug interrupts if an 'interrupt storm'
> has been detected.
> Noise on the interrupt line renders the hotplug interrupt useless:
> each hotplug event causes the devices to be rescanned which will
> will only increase the system load.
> Thus disable the hotplug interrupts and fall back to periodic
> device polling.
>
> v2: Fixed cleanup typo.
>
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 71 +++++++++++++++++++++++++++++++----------
>  1 file changed, 54 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a3f1ac4..d0e6f6d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -88,7 +88,8 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>  	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
>  };
>  
> -
> +static void ibx_hpd_irq_setup(struct drm_device *dev);
> +static void i915_hpd_irq_setup(struct drm_device *dev);
>  
>  /* For display hotplug interrupt */
>  static void
> @@ -342,7 +343,11 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  						    hotplug_work);
>  	struct drm_device *dev = dev_priv->dev;
>  	struct drm_mode_config *mode_config = &dev->mode_config;
> -	struct intel_encoder *encoder;
> +	struct intel_connector *intel_connector;
> +	struct intel_encoder *intel_encoder;
> +	struct drm_connector *connector;
> +	unsigned long irqflags;
> +	bool connector_disabled = false;

Isn't it really hpd_disabled, not connector?

>  
>  	/* HPD irq before everything is fully set up. */
>  	if (!dev_priv->enable_hotplug_processing)
> @@ -351,9 +356,29 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  	mutex_lock(&mode_config->mutex);
>  	DRM_DEBUG_KMS("running encoder hotplug functions\n");
>  
> -	list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
> -		if (encoder->hot_plug)
> -			encoder->hot_plug(encoder);
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +	list_for_each_entry(connector, &mode_config->connector_list, head) {
> +		intel_connector = to_intel_connector(connector);
> +		intel_encoder = intel_connector->encoder;
> +		if (intel_encoder->hpd_pin > HPD_NONE &&
> +		    dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_MARK_DISABLED &&
> +		    connector->polled == DRM_CONNECTOR_POLL_HPD) {
> +			pr_warn("HPD interrupt storm detected on connector %s disabling\n",
> +				drm_get_connector_name(connector));

Please use some DRM message, like DRM_INFO. And perhaps make the message
something like, "Switching from hotplug detection to polling on
connector %s due to interrupt storm\n".

> +			dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark = HPD_DISABLED;
> +			connector->polled = DRM_CONNECTOR_POLL_CONNECT
> +				| DRM_CONNECTOR_POLL_DISCONNECT;
> +			connector_disabled = true;
> +		}
> +	}
> +	if (connector_disabled)
> +		drm_kms_helper_poll_enable(dev); /* if there were no outputs to poll, poll is disabled */

Please move the comment to a line of it's own. (It's okay to add braces
for clarity, since you'll add them later anyway.)

> +
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +
> +	list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> +		if (intel_encoder->hot_plug)
> +			intel_encoder->hot_plug(intel_encoder);
>  
>  	mutex_unlock(&mode_config->mutex);
>  
> @@ -584,13 +609,14 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
>  
>  #define HPD_STORM_DETECT_PERIOD 1000
>  
> -static inline void hotplug_irq_storm_detect(struct drm_device *dev,
> +static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
>  					    u32 hotplug_trigger,
>  					    const u32 *hpd)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	unsigned long irqflags;
>  	int i;
> +	bool ret = false;
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  
> @@ -605,12 +631,15 @@ static inline void hotplug_irq_storm_detect(struct drm_device *dev,
>  			} else if (dev_priv->hpd_stats[i].hpd_cnt > 5) {
>  				dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
>  				DRM_DEBUG_KMS("HPD interrupt storm detected on  PIN %d\n", i);
> +				ret = true;
>  			} else
>  				dev_priv->hpd_stats[i].hpd_cnt++;
>  		}
>  	}
>  
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +
> +	return ret;

As mentioned earlier, these hunks could be moved to the patch
introducing hotplug_irq_storm_detect() in the first place.

>  }
>  
>  static void gmbus_irq_handler(struct drm_device *dev)
> @@ -686,7 +715,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
>  					 hotplug_status);
>  			if (hotplug_trigger) {
> -				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
> +				if( hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915))

Misplaced space after (.


BR,
Jani.

> +					i915_hpd_irq_setup(dev);
>  				queue_work(dev_priv->wq,
>  					   &dev_priv->hotplug_work);
>  			}
> @@ -716,7 +746,8 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
>  	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK;
>  
>  	if (hotplug_trigger) {
> -		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_ibx);
> +		if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_ibx))
> +			ibx_hpd_irq_setup(dev);
>  		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
>  	}
>  	if (pch_iir & SDE_AUDIO_POWER_MASK)
> @@ -764,7 +795,8 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>  	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
>  
>  	if (hotplug_trigger) {
> -		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_cpt);
> +		if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_cpt))
> +			ibx_hpd_irq_setup(dev);
>  		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
>  	}
>  	if (pch_iir & SDE_AUDIO_POWER_MASK_CPT)
> @@ -2119,11 +2151,13 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
>  	if (HAS_PCH_IBX(dev)) {
>  		mask &= ~SDE_HOTPLUG_MASK;
>  		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> -			mask |= hpd_ibx[intel_encoder->hpd_pin];
> +			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
> +				mask |= hpd_ibx[intel_encoder->hpd_pin];
>  	} else {
>  		mask &= ~SDE_HOTPLUG_MASK_CPT;
>  		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> -			mask |= hpd_cpt[intel_encoder->hpd_pin];
> +			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
> +				mask |= hpd_cpt[intel_encoder->hpd_pin];
>  	}
>  
>  	I915_WRITE(SDEIMR, ~mask);
> @@ -2651,7 +2685,8 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
>  			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
>  				  hotplug_status);
>  			if (hotplug_trigger) {
> -				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
> +				if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915))
> +					i915_hpd_irq_setup(dev);
>  				queue_work(dev_priv->wq,
>  					   &dev_priv->hotplug_work);
>  			}
> @@ -2801,7 +2836,7 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	struct drm_mode_config *mode_config = &dev->mode_config;
> -	struct intel_encoder *encoder;
> +	struct intel_encoder *intel_encoder;
>  	u32 hotplug_en;
>  
>  	if (I915_HAS_HOTPLUG(dev)) {
> @@ -2809,8 +2844,9 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
>  		hotplug_en &= ~HOTPLUG_INT_EN_MASK;
>  		/* Note HDMI and DP share hotplug bits */
>  		/* enable bits are the same for all generations */
> -		list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
> -			hotplug_en |= hpd_mask_i915[encoder->hpd_pin];
> +		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> +			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
> +				hotplug_en |= hpd_mask_i915[intel_encoder->hpd_pin];
>  		/* Programming the CRT detection parameters tends
>  		   to generate a spurious hotplug event about three
>  		   seconds later.  So just do it once.
> @@ -2888,8 +2924,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
>  			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
>  				  hotplug_status);
>  			if (hotplug_trigger) {
> -				hotplug_irq_storm_detect(dev, hotplug_trigger,
> -							 IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i965);
> +				if (hotplug_irq_storm_detect(dev, hotplug_trigger,
> +							    IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i965))
> +					i915_hpd_irq_setup(dev);
>  				queue_work(dev_priv->wq,
>  					   &dev_priv->hotplug_work);
>  			}
> -- 
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx