[Intel-gfx] [PATCH 08/24] drm/i915: fix hpd interrupt register locking

2013-06-12 Thread Daniel Vetter 
Our interrupt handler (in hardird context) could race with the timer
(in softirq context), hence we need to hold the spinlock around the
call to -hdp_irq_setup in intel_hpd_irq_handler, too.

But as an optimization (and more so to clarify things) we don't need
to do the irqsave/restore dance in the hardirq context.

Note also that on ilk+ the race isn't just against the hotplug
reenable timer, but also against the fifo underrun reporting. That one
also modifies the SDEIMR register (again protected by the same
dev_priv-irq_lock).

To lock things down again sprinkle a assert_spin_locked. But exclude
the functions touching SDEIMR for now, I want to extract them all into
a new helper function (like we do already for pipestate, display
interrupts and all the various gt interrupts).

Cc: Egbert Eich e...@suse.de
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
 drivers/gpu/drm/i915/i915_irq.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1815891..95e15cd 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -880,15 +880,13 @@ static inline void intel_hpd_irq_handler(struct 
drm_device *dev,
 const u32 *hpd)
 {
drm_i915_private_t *dev_priv = dev-dev_private;
-   unsigned long irqflags;
int i;
bool storm_detected = false;
 
if (!hotplug_trigger)
return;
 
-   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
-
+   spin_lock(dev_priv-irq_lock);
for (i = 1; i  HPD_NUM_PINS; i++) {
 
if (!(hpd[i]  hotplug_trigger) ||
@@ -911,10 +909,9 @@ static inline void intel_hpd_irq_handler(struct drm_device 
*dev,
}
}
 
-   spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
-
if (storm_detected)
dev_priv-display.hpd_irq_setup(dev);
+   spin_unlock(dev_priv-irq_lock);
 
queue_work(dev_priv-wq,
   dev_priv-hotplug_work);
@@ -3327,6 +3324,8 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
struct intel_encoder *intel_encoder;
u32 hotplug_en;
 
+   assert_spin_locked(dev_priv-irq_lock);
+
if (I915_HAS_HOTPLUG(dev)) {
hotplug_en = I915_READ(PORT_HOTPLUG_EN);
hotplug_en = ~HOTPLUG_INT_EN_MASK;
@@ -3610,6 +3609,7 @@ void intel_hpd_init(struct drm_device *dev)
struct drm_i915_private *dev_priv = dev-dev_private;
struct drm_mode_config *mode_config = dev-mode_config;
struct drm_connector *connector;
+   unsigned long irqflags;
int i;
 
for (i = 1; i  HPD_NUM_PINS; i++) {
@@ -3622,6 +3622,11 @@ void intel_hpd_init(struct drm_device *dev)
if (!connector-polled  I915_HAS_HOTPLUG(dev)  
intel_connector-encoder-hpd_pin  HPD_NONE)
connector-polled = DRM_CONNECTOR_POLL_HPD;
}
+
+   /* Interrup setup is already guaranteed to be single-threaded, this is
+* just to make the assert_spin_locked checks happy. */
+   spin_lock_irqsave(dev_priv-irq_lock, irqflags);
if (dev_priv-display.hpd_irq_setup)
dev_priv-display.hpd_irq_setup(dev);
+   spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
 }
-- 
1.8.1.4

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

Re: [Intel-gfx] [PATCH 08/24] drm/i915: fix hpd interrupt register locking

2013-06-12 Thread Egbert Eich 
Daniel Vetter writes:
  Our interrupt handler (in hardird context) could race with the timer
  Nitpick: s/d/q/

  (in softirq context), hence we need to hold the spinlock around the
  call to -hdp_irq_setup in intel_hpd_irq_handler, too.
  
  But as an optimization (and more so to clarify things) we don't need
  to do the irqsave/restore dance in the hardirq context.
  
  Note also that on ilk+ the race isn't just against the hotplug
  reenable timer, but also against the fifo underrun reporting. That one
  also modifies the SDEIMR register (again protected by the same
  dev_priv-irq_lock).
  
  To lock things down again sprinkle a assert_spin_locked. But exclude
  the functions touching SDEIMR for now, I want to extract them all into
  a new helper function (like we do already for pipestate, display
  interrupts and all the various gt interrupts).
  
  Cc: Egbert Eich e...@suse.de
  Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
  ---
   drivers/gpu/drm/i915/i915_irq.c | 15 ++-
   1 file changed, 10 insertions(+), 5 deletions(-)
  
  diff --git a/drivers/gpu/drm/i915/i915_irq.c 
  b/drivers/gpu/drm/i915/i915_irq.c
  index 1815891..95e15cd 100644
  --- a/drivers/gpu/drm/i915/i915_irq.c
  +++ b/drivers/gpu/drm/i915/i915_irq.c
  @@ -880,15 +880,13 @@ static inline void intel_hpd_irq_handler(struct 
  drm_device *dev,
const u32 *hpd)
   {
   drm_i915_private_t *dev_priv = dev-dev_private;
  -unsigned long irqflags;
   int i;
   bool storm_detected = false;
   
   if (!hotplug_trigger)
   return;
   
  -spin_lock_irqsave(dev_priv-irq_lock, irqflags);
  -
  +spin_lock(dev_priv-irq_lock);
   for (i = 1; i  HPD_NUM_PINS; i++) {
   
   if (!(hpd[i]  hotplug_trigger) ||
  @@ -911,10 +909,9 @@ static inline void intel_hpd_irq_handler(struct 
  drm_device *dev,
   }
   }
   
  -spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
  -
   if (storm_detected)
   dev_priv-display.hpd_irq_setup(dev);
  +spin_unlock(dev_priv-irq_lock);
   
   queue_work(dev_priv-wq,
  dev_priv-hotplug_work);
  @@ -3327,6 +3324,8 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
   struct intel_encoder *intel_encoder;
   u32 hotplug_en;
   
  +assert_spin_locked(dev_priv-irq_lock);
  +
   if (I915_HAS_HOTPLUG(dev)) {
   hotplug_en = I915_READ(PORT_HOTPLUG_EN);
   hotplug_en = ~HOTPLUG_INT_EN_MASK;


Didn't you want to do the same for ibx_hpd_irq_setup() ?

  @@ -3610,6 +3609,7 @@ void intel_hpd_init(struct drm_device *dev)
   struct drm_i915_private *dev_priv = dev-dev_private;
   struct drm_mode_config *mode_config = dev-mode_config;
   struct drm_connector *connector;
  +unsigned long irqflags;
   int i;
   
   for (i = 1; i  HPD_NUM_PINS; i++) {
  @@ -3622,6 +3622,11 @@ void intel_hpd_init(struct drm_device *dev)
   if (!connector-polled  I915_HAS_HOTPLUG(dev)  
  intel_connector-encoder-hpd_pin  HPD_NONE)
   connector-polled = DRM_CONNECTOR_POLL_HPD;
   }
  +
  +/* Interrup setup is already guaranteed to be single-threaded, this is

Nitpick -  missed a 't'.

  + * just to make the assert_spin_locked checks happy. */
  +spin_lock_irqsave(dev_priv-irq_lock, irqflags);
   if (dev_priv-display.hpd_irq_setup)
   dev_priv-display.hpd_irq_setup(dev);
  +spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
   }
  -- 
  1.8.1.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 08/24] drm/i915: fix hpd interrupt register locking

2013-06-12 Thread Daniel Vetter 
On Wed, Jun 12, 2013 at 4:59 PM, Egbert Eich e...@suse.com wrote:
   +assert_spin_locked(dev_priv-irq_lock);
   +
if (I915_HAS_HOTPLUG(dev)) {
hotplug_en = I915_READ(PORT_HOTPLUG_EN);
hotplug_en = ~HOTPLUG_INT_EN_MASK;


 Didn't you want to do the same for ibx_hpd_irq_setup() ?

Will happen in a follow-up patch, since I first consolidate all access
to SDEIMR into one single helper function. I've tried to explain that
a bit in the commit message but failed to make the connection to
ibx_hpd_irq_setup - once we have the helper function the
assert_spin_locked is in there and so ibx_hpd_irq_setup doesn't need
its own check (since it won't touch SDEIMR directly any more).
-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