* [PATCH] drm/i915: Don't WARN about unexpected hpd interrupts on gmch platforms
@ 2014-04-12 17:31 Daniel Vetter
2014-04-12 18:12 ` Egbert Eich
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2014-04-12 17:31 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Egbert Eich, Daniel Vetter, stable, bitlord
The status bits are unconditionally set, the control bits only enable
the actual interrupt generation. Which means if we get some random
other interrupts we'll bogusly complain about them.
So restrict the WARN to platforms with a sane hotplug interrupt
handling scheme.
This WARN has been introduced in
commit b8f102e8bf71cacf33326360fdf9dcfd1a63925b
Author: Egbert Eich <eich@suse.de>
Date: Fri Jul 26 14:14:24 2013 +0200
drm/i915: Add messages useful for HPD storm detection debugging (v2)
Cc: Egbert Eich <eich@suse.de>
Cc: bitlord <bitlord0xff@gmail.com>
Reported-by: bitlord <bitlord0xff@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7753249b3a95..f98ba4e6e70b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1362,10 +1362,20 @@ static inline void intel_hpd_irq_handler(struct drm_device *dev,
spin_lock(&dev_priv->irq_lock);
for (i = 1; i < HPD_NUM_PINS; i++) {
- WARN_ONCE(hpd[i] & hotplug_trigger &&
- dev_priv->hpd_stats[i].hpd_mark == HPD_DISABLED,
- "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
- hotplug_trigger, i, hpd[i]);
+ if (hpd[i] & hotplug_trigger &&
+ dev_priv->hpd_stats[i].hpd_mark == HPD_DISABLED) {
+ /*
+ * On GMCH platforms the interrupt mask bits only
+ * prevent irq generation, not the setting of the
+ * hotplug bits itself. So only WARN about unexpected
+ * interrupts on saner platforms.
+ */
+ WARN_ONCE(INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev),
+ "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
+ hotplug_trigger, i, hpd[i]);
+
+ continue;
+ }
if (!(hpd[i] & hotplug_trigger) ||
dev_priv->hpd_stats[i].hpd_mark != HPD_ENABLED)
--
1.8.5.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't WARN about unexpected hpd interrupts on gmch platforms
2014-04-12 17:31 [PATCH] drm/i915: Don't WARN about unexpected hpd interrupts on gmch platforms Daniel Vetter
@ 2014-04-12 18:12 ` Egbert Eich
2014-04-12 18:38 ` [Intel-gfx] " Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Egbert Eich @ 2014-04-12 18:12 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Egbert Eich, Intel Graphics Development, stable, bitlord
Daniel Vetter writes:
> The status bits are unconditionally set, the control bits only enable
> the actual interrupt generation. Which means if we get some random
> other interrupts we'll bogusly complain about them.
>
> So restrict the WARN to platforms with a sane hotplug interrupt
> handling scheme.
>
> This WARN has been introduced in
>
> commit b8f102e8bf71cacf33326360fdf9dcfd1a63925b
> Author: Egbert Eich <eich@suse.de>
> Date: Fri Jul 26 14:14:24 2013 +0200
>
> drm/i915: Add messages useful for HPD storm detection debugging (v2)
>
> Cc: Egbert Eich <eich@suse.de>
> Cc: bitlord <bitlord0xff@gmail.com>
> Reported-by: bitlord <bitlord0xff@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 7753249b3a95..f98ba4e6e70b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1362,10 +1362,20 @@ static inline void intel_hpd_irq_handler(struct drm_device *dev,
> spin_lock(&dev_priv->irq_lock);
> for (i = 1; i < HPD_NUM_PINS; i++) {
>
> - WARN_ONCE(hpd[i] & hotplug_trigger &&
> - dev_priv->hpd_stats[i].hpd_mark == HPD_DISABLED,
> - "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
> - hotplug_trigger, i, hpd[i]);
> + if (hpd[i] & hotplug_trigger &&
> + dev_priv->hpd_stats[i].hpd_mark == HPD_DISABLED) {
> + /*
> + * On GMCH platforms the interrupt mask bits only
> + * prevent irq generation, not the setting of the
> + * hotplug bits itself. So only WARN about unexpected
> + * interrupts on saner platforms.
> + */
> + WARN_ONCE(INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev),
> + "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
> + hotplug_trigger, i, hpd[i]);
Personally I'd prefer the condition in the WARN..() macro to be the
unexpected condition you want to warn about. This makes it easier for
anybody not up to speed with the details of hotplug handling to understand
the code.
Of course the way you structure this avoids a lot of unnecessary tests.
But if this is a concern maybe the entire for loop should be restructured
with
if (!(hpd[i] & hotplug_trigger))
continue;
right at the beginning.
> +
> + continue;
> + }
>
> if (!(hpd[i] & hotplug_trigger) ||
> dev_priv->hpd_stats[i].hpd_mark != HPD_ENABLED)
> --
> 1.8.5.2
Cheers,
Egbert.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Don't WARN about unexpected hpd interrupts on gmch platforms
2014-04-12 18:12 ` Egbert Eich
@ 2014-04-12 18:38 ` Daniel Vetter
2014-04-24 8:11 ` Jani Nikula
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2014-04-12 18:38 UTC (permalink / raw)
To: Egbert Eich; +Cc: Egbert Eich, Intel Graphics Development, stable, bitlord
On Sat, Apr 12, 2014 at 8:12 PM, Egbert Eich <eich@freedesktop.org> wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 7753249b3a95..f98ba4e6e70b 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1362,10 +1362,20 @@ static inline void intel_hpd_irq_handler(struct drm_device *dev,
> > spin_lock(&dev_priv->irq_lock);
> > for (i = 1; i < HPD_NUM_PINS; i++) {
> >
> > - WARN_ONCE(hpd[i] & hotplug_trigger &&
> > - dev_priv->hpd_stats[i].hpd_mark == HPD_DISABLED,
> > - "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
> > - hotplug_trigger, i, hpd[i]);
> > + if (hpd[i] & hotplug_trigger &&
> > + dev_priv->hpd_stats[i].hpd_mark == HPD_DISABLED) {
> > + /*
> > + * On GMCH platforms the interrupt mask bits only
> > + * prevent irq generation, not the setting of the
> > + * hotplug bits itself. So only WARN about unexpected
> > + * interrupts on saner platforms.
> > + */
> > + WARN_ONCE(INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev),
> > + "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
> > + hotplug_trigger, i, hpd[i]);
>
> Personally I'd prefer the condition in the WARN..() macro to be the
> unexpected condition you want to warn about. This makes it easier for
> anybody not up to speed with the details of hotplug handling to understand
> the code.
> Of course the way you structure this avoids a lot of unnecessary tests.
> But if this is a concern maybe the entire for loop should be restructured
> with
>
> if (!(hpd[i] & hotplug_trigger))
> continue;
>
> right at the beginning.
>
> > +
> > + continue;
> > + }
We want to skip the hpd handling in any case if the interrupt is
blocked, otherwise every time we have some unrelated interrupt on gmch
platforms while a hpd storm is ongoing we'll fire off the hotplug
work. Which we obviously don't want since that defeats the point of
the storm handling code.
But we only want to WARN on platforms where we can reliably stop the
status bits too, hence why I've nested the checks like this.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't WARN about unexpected hpd interrupts on gmch platforms
2014-04-12 18:38 ` [Intel-gfx] " Daniel Vetter
@ 2014-04-24 8:11 ` Jani Nikula
2014-04-24 10:03 ` [PATCH] drm/i915: Don't WARN nor handle " Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2014-04-24 8:11 UTC (permalink / raw)
To: Daniel Vetter, Egbert Eich
Cc: Egbert Eich, Intel Graphics Development, stable, bitlord
On Sat, 12 Apr 2014, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Sat, Apr 12, 2014 at 8:12 PM, Egbert Eich <eich@freedesktop.org> wrote:
>> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> > index 7753249b3a95..f98ba4e6e70b 100644
>> > --- a/drivers/gpu/drm/i915/i915_irq.c
>> > +++ b/drivers/gpu/drm/i915/i915_irq.c
>> > @@ -1362,10 +1362,20 @@ static inline void intel_hpd_irq_handler(struct drm_device *dev,
>> > spin_lock(&dev_priv->irq_lock);
>> > for (i = 1; i < HPD_NUM_PINS; i++) {
>> >
>> > - WARN_ONCE(hpd[i] & hotplug_trigger &&
>> > - dev_priv->hpd_stats[i].hpd_mark == HPD_DISABLED,
>> > - "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
>> > - hotplug_trigger, i, hpd[i]);
>> > + if (hpd[i] & hotplug_trigger &&
>> > + dev_priv->hpd_stats[i].hpd_mark == HPD_DISABLED) {
>> > + /*
>> > + * On GMCH platforms the interrupt mask bits only
>> > + * prevent irq generation, not the setting of the
>> > + * hotplug bits itself. So only WARN about unexpected
>> > + * interrupts on saner platforms.
>> > + */
>> > + WARN_ONCE(INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev),
>> > + "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
>> > + hotplug_trigger, i, hpd[i]);
>>
>> Personally I'd prefer the condition in the WARN..() macro to be the
>> unexpected condition you want to warn about. This makes it easier for
>> anybody not up to speed with the details of hotplug handling to understand
>> the code.
>> Of course the way you structure this avoids a lot of unnecessary tests.
>> But if this is a concern maybe the entire for loop should be restructured
>> with
>>
>> if (!(hpd[i] & hotplug_trigger))
>> continue;
>>
>> right at the beginning.
>>
>> > +
>> > + continue;
>> > + }
>
>
> We want to skip the hpd handling in any case if the interrupt is
> blocked, otherwise every time we have some unrelated interrupt on gmch
> platforms while a hpd storm is ongoing we'll fire off the hotplug
> work. Which we obviously don't want since that defeats the point of
> the storm handling code.
IMO this skipping the rest of hpd handling is actually the bigger (and
an actual functional) change here, but the commit message only talks
about limiting the WARN. Please amend the commit message.
BR,
Jani.
>
> But we only want to WARN on platforms where we can reliably stop the
> status bits too, hence why I've nested the checks like this.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] drm/i915: Don't WARN nor handle unexpected hpd interrupts on gmch platforms
2014-04-24 8:11 ` Jani Nikula
@ 2014-04-24 10:03 ` Daniel Vetter
2014-04-24 10:43 ` Jani Nikula
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2014-04-24 10:03 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Egbert Eich, Daniel Vetter, stable, bitlord
The status bits are unconditionally set, the control bits only enable
the actual interrupt generation. Which means if we get some random
other interrupts we'll bogusly complain about them.
So restrict the WARN to platforms with a sane hotplug interrupt
handling scheme. And even more important also don't attempt to process
the hpd bit since we've detected a storm already. Instead just clear
the bit silently.
This WARN has been introduced in
commit b8f102e8bf71cacf33326360fdf9dcfd1a63925b
Author: Egbert Eich <eich@suse.de>
Date: Fri Jul 26 14:14:24 2013 +0200
drm/i915: Add messages useful for HPD storm detection debugging (v2)
before that we silently handled the hpd event and so partially
defeated the storm detection.
v2: Pimp commit message (Jani)
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Egbert Eich <eich@suse.de>
Cc: bitlord <bitlord0xff@gmail.com>
Reported-by: bitlord <bitlord0xff@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7753249b3a95..f98ba4e6e70b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1362,10 +1362,20 @@ static inline void intel_hpd_irq_handler(struct drm_device *dev,
spin_lock(&dev_priv->irq_lock);
for (i = 1; i < HPD_NUM_PINS; i++) {
- WARN_ONCE(hpd[i] & hotplug_trigger &&
- dev_priv->hpd_stats[i].hpd_mark == HPD_DISABLED,
- "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
- hotplug_trigger, i, hpd[i]);
+ if (hpd[i] & hotplug_trigger &&
+ dev_priv->hpd_stats[i].hpd_mark == HPD_DISABLED) {
+ /*
+ * On GMCH platforms the interrupt mask bits only
+ * prevent irq generation, not the setting of the
+ * hotplug bits itself. So only WARN about unexpected
+ * interrupts on saner platforms.
+ */
+ WARN_ONCE(INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev),
+ "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
+ hotplug_trigger, i, hpd[i]);
+
+ continue;
+ }
if (!(hpd[i] & hotplug_trigger) ||
dev_priv->hpd_stats[i].hpd_mark != HPD_ENABLED)
--
1.9.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Don't WARN nor handle unexpected hpd interrupts on gmch platforms
2014-04-24 10:03 ` [PATCH] drm/i915: Don't WARN nor handle " Daniel Vetter
@ 2014-04-24 10:43 ` Jani Nikula
0 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2014-04-24 10:43 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Egbert Eich, Daniel Vetter, stable, bitlord
On Thu, 24 Apr 2014, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The status bits are unconditionally set, the control bits only enable
> the actual interrupt generation. Which means if we get some random
> other interrupts we'll bogusly complain about them.
>
> So restrict the WARN to platforms with a sane hotplug interrupt
> handling scheme. And even more important also don't attempt to process
> the hpd bit since we've detected a storm already. Instead just clear
> the bit silently.
>
> This WARN has been introduced in
>
> commit b8f102e8bf71cacf33326360fdf9dcfd1a63925b
> Author: Egbert Eich <eich@suse.de>
> Date: Fri Jul 26 14:14:24 2013 +0200
>
> drm/i915: Add messages useful for HPD storm detection debugging (v2)
>
> before that we silently handled the hpd event and so partially
> defeated the storm detection.
>
> v2: Pimp commit message (Jani)
>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Egbert Eich <eich@suse.de>
> Cc: bitlord <bitlord0xff@gmail.com>
> Reported-by: bitlord <bitlord0xff@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Pushed to -fixes, thanks for the patch.
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-04-24 10:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-12 17:31 [PATCH] drm/i915: Don't WARN about unexpected hpd interrupts on gmch platforms Daniel Vetter
2014-04-12 18:12 ` Egbert Eich
2014-04-12 18:38 ` [Intel-gfx] " Daniel Vetter
2014-04-24 8:11 ` Jani Nikula
2014-04-24 10:03 ` [PATCH] drm/i915: Don't WARN nor handle " Daniel Vetter
2014-04-24 10:43 ` Jani Nikula
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.