Re: [Intel-gfx] [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Egbert Eich]

Daniel Vetter writes:
 > On Wed, Sep 02, 2015 at 04:19:00PM +0200, Egbert Eich wrote:
 > 
 > Hm I missed that this same register is also accessed by the irq handler
 > code, and it's not just that touching these bits can cause interrupts. So
 > yeah we need your patch, but it needs to be clearer in the commit message
 > that there's also trouble with concurrent register access to
 > PORT_HOTPLUG_EN.
 > 
 > Also I think a commen in the code why we grab that spinlock would be good.
 > For that extracting a small helper to manipulate the register (like we do
 > with other irq mask registers with functions like ilk_update_gt_irq) would
 > be good - then we have just one place to put that commment.

OK, I will come up with a suggestion.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Daniel Vetter]

On Tue, Sep 01, 2015 at 10:21:35PM +0200, Egbert Eich wrote:
> A HPD interrupt may fire during intel_crt_detect_hotplug() - especially
> when HPD interrupt storms occur.
> Since the interrupt handler changes the enabled interrupt lines when it
> detects a storm this races with intel_crt_detect_hotplug().
> To avoid this, shiled the rmw cycles with IRQ save spinlocks.
> 
> Signed-off-by: Egbert Eich 

I think this only reduces one source of such races, but fundamentally we
can't avoid them. E.g. if you're _very_ unlucky you might cause a real hpd
right when the strom code frobs around. Plugging the race with this known
interrupt source here doesn't fix the fundamental issue.

Also I think the actual interrupt deliver is fairly asynchronous, both in
the hardware and in the sw handling. E.g. spin_lock_irq does not
synchronize with the interrupt handler on SMP systems, it only guarantees
that it's not running concurrently on the same cpu (which would deadlock).

I think fundamentally this race is unfixable.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_crt.c | 17 -
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c 
> b/drivers/gpu/drm/i915/intel_crt.c
> index af5e43b..685f3de 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -376,9 +376,10 @@ static bool intel_crt_detect_hotplug(struct 
> drm_connector *connector)
>  {
>   struct drm_device *dev = connector->dev;
>   struct drm_i915_private *dev_priv = dev->dev_private;
> - u32 hotplug_en, orig, stat;
> + u32 hotplug_en, stat;
>   bool ret = false;
>   int i, tries = 0;
> + unsigned long irqflags;
>  
>   if (HAS_PCH_SPLIT(dev))
>   return intel_ironlake_crt_detect_hotplug(connector);
> @@ -395,12 +396,14 @@ static bool intel_crt_detect_hotplug(struct 
> drm_connector *connector)
>   tries = 2;
>   else
>   tries = 1;
> - hotplug_en = orig = I915_READ(PORT_HOTPLUG_EN);
> - hotplug_en |= CRT_HOTPLUG_FORCE_DETECT;
>  
>   for (i = 0; i < tries ; i++) {
>   /* turn on the FORCE_DETECT */
> - I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
> + spin_lock_irqsave(_priv->irq_lock, irqflags);
> + hotplug_en = I915_READ(PORT_HOTPLUG_EN);
> + I915_WRITE(PORT_HOTPLUG_EN, hotplug_en |
> +CRT_HOTPLUG_FORCE_DETECT);
> + spin_unlock_irqrestore(_priv->irq_lock, irqflags);
>   /* wait for FORCE_DETECT to go off */
>   if (wait_for((I915_READ(PORT_HOTPLUG_EN) &
> CRT_HOTPLUG_FORCE_DETECT) == 0,
> @@ -416,7 +419,11 @@ static bool intel_crt_detect_hotplug(struct 
> drm_connector *connector)
>   I915_WRITE(PORT_HOTPLUG_STAT, CRT_HOTPLUG_INT_STATUS);
>  
>   /* and put the bits back */
> - I915_WRITE(PORT_HOTPLUG_EN, orig);
> + spin_lock_irqsave(_priv->irq_lock, irqflags);
> + hotplug_en = I915_READ(PORT_HOTPLUG_EN);
> + hotplug_en &= ~CRT_HOTPLUG_FORCE_DETECT;
> + I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
> + spin_unlock_irqrestore(_priv->irq_lock, irqflags);
>  
>   return ret;
>  }
> -- 
> 1.8.4.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Daniel Vetter]

On Wed, Sep 02, 2015 at 04:19:00PM +0200, Egbert Eich wrote:
> Daniel Vetter writes:
>  > On Tue, Sep 01, 2015 at 10:21:35PM +0200, Egbert Eich wrote:
>  > > A HPD interrupt may fire during intel_crt_detect_hotplug() - especially
>  > > when HPD interrupt storms occur.
>  > > Since the interrupt handler changes the enabled interrupt lines when it
>  > > detects a storm this races with intel_crt_detect_hotplug().
>  > > To avoid this, shiled the rmw cycles with IRQ save spinlocks.
>  > > 
>  > > Signed-off-by: Egbert Eich 
>  > 
>  > I think this only reduces one source of such races, but fundamentally we
>  > can't avoid them. E.g. if you're _very_ unlucky you might cause a real hpd
>  > right when the strom code frobs around. Plugging the race with this known
> 
> This is exactly the scenatio I'm getting here. I get HPD interrupts at an 
> order of 10^4 / sec.
> 
>  > interrupt source here doesn't fix the fundamental issue.
>  > 
>  > Also I think the actual interrupt deliver is fairly asynchronous, both in
>  > the hardware and in the sw handling. E.g. spin_lock_irq does not
>  > synchronize with the interrupt handler on SMP systems, it only guarantees
>  > that it's not running concurrently on the same cpu (which would deadlock).
>  > 
>  > I think fundamentally this race is unfixable.
> 
> 
> There is one important race we avoid with this patch: It is that
> we change the PORT_HOTPLUG_EN concurrently in the interrupt handler
> (thru xxx_hpd_irq_setup() and in intel_crt_detect_hotplug()).
> 
> With the test system that I have here the old version of the code
> easily runs into this as the time spent inside intel_crt_detect_hotplug() 
> is quite long - especially when no CRT is connected.
> 
> What happens intel_crt_detect_hotplug() reads the value of PORT_HOTPLUG_EN
> at entry, then frobs around for ages, during this time several HPD interrupts
> occur, the storm is detected, the bit related to the stormy line is unset
> then after ages intel_crt_detect_hotplug() decides to give up and restores
> the value it read on entry.
> 
> To remedy this this patch reads the value of PORT_HOTPLUG_EN whenever 
> it is going to change it and adds the spin locks to make sure the 
> read-modify-write cycles don't happen concurrently.
> 
> PORT_HOTPLUG_EN is only touched in two places: in xxx_hpd_irq_setup()
> and in intel_crt_detect_hotplug(), the former can be called from an
> interrupt handler.
> 
> Not sure why you see a problem here.

Hm I missed that this same register is also accessed by the irq handler
code, and it's not just that touching these bits can cause interrupts. So
yeah we need your patch, but it needs to be clearer in the commit message
that there's also trouble with concurrent register access to
PORT_HOTPLUG_EN.

Also I think a commen in the code why we grab that spinlock would be good.
For that extracting a small helper to manipulate the register (like we do
with other irq mask registers with functions like ilk_update_gt_irq) would
be good - then we have just one place to put that commment.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Egbert Eich]

Jani Nikula writes:
 > On Wed, 02 Sep 2015, Egbert Eich  wrote:
 > > This is exactly the scenatio I'm getting here. I get HPD interrupts at an 
 > > order of 10^4 / sec.
 > 
 > Makes you wonder if either you have faulty hardware or we are
 > configuring the hardware wrong (we overlook some configuration about
 > some voltage/duration threshold maybe, or get irqs from a line that's
 > floating, or something).

It is faulty hardware. But it is not a single machine that broke.
It is an entire series. IMHO due to bad signal routing and poor shilding
there is crosstalk on the SDVO lines signaling the plug status.
Since SDVO uses PCIe lines it is AC coupled, if I recall correctly
from reading the specs long time ago, one status is signalled by a
10MHz signal, the other by 20MHz.

At the time when I implemented this I've seen other reports from systems 
which showed similar problems under certain conditions(*) - although not 
quite as bad, therefore I thought of a general solution to get rid of 
this once and for all. If this had only been one system with this problem, 
I would just have blacklisted it.

(*) It seems that this somewhat depends on the video mode set (supports
the crosstalk theory) but I also had a report where this occurred at
certain charging levels and whether a power supply was connected or
not.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Egbert Eich]

Daniel Vetter writes:
 > On Tue, Sep 01, 2015 at 10:21:35PM +0200, Egbert Eich wrote:
 > > A HPD interrupt may fire during intel_crt_detect_hotplug() - especially
 > > when HPD interrupt storms occur.
 > > Since the interrupt handler changes the enabled interrupt lines when it
 > > detects a storm this races with intel_crt_detect_hotplug().
 > > To avoid this, shiled the rmw cycles with IRQ save spinlocks.
 > > 
 > > Signed-off-by: Egbert Eich 
 > 
 > I think this only reduces one source of such races, but fundamentally we
 > can't avoid them. E.g. if you're _very_ unlucky you might cause a real hpd
 > right when the strom code frobs around. Plugging the race with this known

This is exactly the scenatio I'm getting here. I get HPD interrupts at an 
order of 10^4 / sec.

 > interrupt source here doesn't fix the fundamental issue.
 > 
 > Also I think the actual interrupt deliver is fairly asynchronous, both in
 > the hardware and in the sw handling. E.g. spin_lock_irq does not
 > synchronize with the interrupt handler on SMP systems, it only guarantees
 > that it's not running concurrently on the same cpu (which would deadlock).
 > 
 > I think fundamentally this race is unfixable.


There is one important race we avoid with this patch: It is that
we change the PORT_HOTPLUG_EN concurrently in the interrupt handler
(thru xxx_hpd_irq_setup() and in intel_crt_detect_hotplug()).

With the test system that I have here the old version of the code
easily runs into this as the time spent inside intel_crt_detect_hotplug() 
is quite long - especially when no CRT is connected.

What happens intel_crt_detect_hotplug() reads the value of PORT_HOTPLUG_EN
at entry, then frobs around for ages, during this time several HPD interrupts
occur, the storm is detected, the bit related to the stormy line is unset
then after ages intel_crt_detect_hotplug() decides to give up and restores
the value it read on entry.

To remedy this this patch reads the value of PORT_HOTPLUG_EN whenever 
it is going to change it and adds the spin locks to make sure the 
read-modify-write cycles don't happen concurrently.

PORT_HOTPLUG_EN is only touched in two places: in xxx_hpd_irq_setup()
and in intel_crt_detect_hotplug(), the former can be called from an
interrupt handler.

Not sure why you see a problem here.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Jani Nikula]

On Wed, 02 Sep 2015, Egbert Eich  wrote:
> This is exactly the scenatio I'm getting here. I get HPD interrupts at an 
> order of 10^4 / sec.

Makes you wonder if either you have faulty hardware or we are
configuring the hardware wrong (we overlook some configuration about
some voltage/duration threshold maybe, or get irqs from a line that's
floating, or something).

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt

[Egbert Eich]

A HPD interrupt may fire during intel_crt_detect_hotplug() - especially
when HPD interrupt storms occur.
Since the interrupt handler changes the enabled interrupt lines when it
detects a storm this races with intel_crt_detect_hotplug().
To avoid this, shiled the rmw cycles with IRQ save spinlocks.

Signed-off-by: Egbert Eich 
---
 drivers/gpu/drm/i915/intel_crt.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index af5e43b..685f3de 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -376,9 +376,10 @@ static bool intel_crt_detect_hotplug(struct drm_connector 
*connector)
 {
struct drm_device *dev = connector->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
-   u32 hotplug_en, orig, stat;
+   u32 hotplug_en, stat;
bool ret = false;
int i, tries = 0;
+   unsigned long irqflags;
 
if (HAS_PCH_SPLIT(dev))
return intel_ironlake_crt_detect_hotplug(connector);
@@ -395,12 +396,14 @@ static bool intel_crt_detect_hotplug(struct drm_connector 
*connector)
tries = 2;
else
tries = 1;
-   hotplug_en = orig = I915_READ(PORT_HOTPLUG_EN);
-   hotplug_en |= CRT_HOTPLUG_FORCE_DETECT;
 
for (i = 0; i < tries ; i++) {
/* turn on the FORCE_DETECT */
-   I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
+   spin_lock_irqsave(_priv->irq_lock, irqflags);
+   hotplug_en = I915_READ(PORT_HOTPLUG_EN);
+   I915_WRITE(PORT_HOTPLUG_EN, hotplug_en |
+  CRT_HOTPLUG_FORCE_DETECT);
+   spin_unlock_irqrestore(_priv->irq_lock, irqflags);
/* wait for FORCE_DETECT to go off */
if (wait_for((I915_READ(PORT_HOTPLUG_EN) &
  CRT_HOTPLUG_FORCE_DETECT) == 0,
@@ -416,7 +419,11 @@ static bool intel_crt_detect_hotplug(struct drm_connector 
*connector)
I915_WRITE(PORT_HOTPLUG_STAT, CRT_HOTPLUG_INT_STATUS);
 
/* and put the bits back */
-   I915_WRITE(PORT_HOTPLUG_EN, orig);
+   spin_lock_irqsave(_priv->irq_lock, irqflags);
+   hotplug_en = I915_READ(PORT_HOTPLUG_EN);
+   hotplug_en &= ~CRT_HOTPLUG_FORCE_DETECT;
+   I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
+   spin_unlock_irqrestore(_priv->irq_lock, irqflags);
 
return ret;
 }
-- 
1.8.4.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx