On Tue, 09 Apr 2013, Egbert Eich <eich at freedesktop.org> wrote:
> From: Egbert Eich <eich at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
