From mboxrd@z Thu Jan 1 00:00:00 1970 From: Egbert Eich Subject: Re: [PATCH v3 1/7] drm/i915: Add HPD IRQ storm detection (v4) Date: Tue, 16 Apr 2013 13:34:58 +0200 Message-ID: <20130416113458.GD17947@debian> References: <1365499470-28646-1-git-send-email-eich@freedesktop.org> <1365499470-28646-2-git-send-email-eich@freedesktop.org> <87hajdpg42.fsf@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from moutng.kundenserver.de (moutng.kundenserver.de [212.227.126.187]) by gabe.freedesktop.org (Postfix) with ESMTP id 14867E5C21 for ; Tue, 16 Apr 2013 04:35:05 -0700 (PDT) Content-Disposition: inline In-Reply-To: <87hajdpg42.fsf@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Jani Nikula Cc: Egbert Eich , Daniel Vetter , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org Hi Jani, I've rebased and regenerated all the patches now, as there were some other bikesheds i had not adressed. I've also included all Reviewed-By: This should make it easier to integrate the patches. Some comments below: On Thu, Apr 11, 2013 at 12:32:29PM +0300, Jani Nikula wrote: > > + struct { > > + unsigned long hpd_last_jiffies; > > + int hpd_cnt; > > + enum { > > + HPD_ENABLED = 0, > > + HPD_DISABLED = 1, > > + HPD_MARK_DISABLED = 2 > > + } hpd_mark; > > + } hpd_stats[HPD_NUM_PINS]; > > With all the hotplug stuff being added, I think it's getting time to > group all the hotplug stuff under a struct. I will postpone this until I address the issues that I have on my TODO. > > > 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 4c5bdd0..32b5527 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -582,6 +582,37 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv, > > queue_work(dev_priv->wq, &dev_priv->rps.work); > > } > > > > +#define HPD_STORM_DETECT_PERIOD 1000 > > + > > +static inline void hotplug_irq_storm_detect(struct drm_device *dev, > > + u32 hotplug_trigger, > > + const u32 *hpd) > > I think you should just add the bool return status right here instead of > postponing until the later patch that needs it. I thought about this and left it as it is: Returning a bool status now will raise other questions as the logic in this patch doesn't require it. I'd rather have a bigger patch later which will however clearly explains the reason for the change to the function. > > > +{ > > + drm_i915_private_t *dev_priv = dev->dev_private; > > + unsigned long irqflags; > > + int i; > > + > > + spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > > + > > + for (i = 1; i < HPD_NUM_PINS; i++) { > > + if ((hpd[i] & hotplug_trigger) && > > + dev_priv->hpd_stats[i].hpd_mark == HPD_ENABLED) { > > Bikeshed: You might get a nicer layout below by negating that if and > adding continue. Fixed. > > > + if (!time_in_range(jiffies, dev_priv->hpd_stats[i].hpd_last_jiffies, > > + dev_priv->hpd_stats[i].hpd_last_jiffies > > + + msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) { > > + dev_priv->hpd_stats[i].hpd_last_jiffies = jiffies; > > + dev_priv->hpd_stats[i].hpd_cnt = 0; > > + } 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); > > Extra space before "PIN". Fixed. > > > + } else > > + dev_priv->hpd_stats[i].hpd_cnt++; > > If one branch requires braces, then all do. Fixed. Cheers, Egbert.