drm/i915: Use a spin lock to protect the pipe crc struct
authorDamien Lespiau <damien.lespiau@intel.com>
Mon, 21 Oct 2013 13:29:30 +0000 (14:29 +0100)
committerDaniel Vetter <daniel.vetter@ffwll.ch>
Mon, 21 Oct 2013 22:27:49 +0000 (00:27 +0200)
Daniel pointed out that it was hard to get anything lockless to work
correctly, so don't even try for this non critical piece of code and
just use a spin lock.

v2: Make intel_pipe_crc->opened a bool
v3: Use assert_spin_locked() instead of a comment (Daniel Vetter)
v4: Use spin_lock_irq() in the debugfs functions (they can only be
    called from process context),
    Use spin_lock() in the pipe_crc_update() function that can only be
    called from an interrupt handler,
    Use wait_event_interruptible_lock_irq() when waiting for data in the
    cicular buffer to ensure proper locking around the condition we are
    waiting for. (Daniel Vetter)

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
drivers/gpu/drm/i915/i915_debugfs.c
drivers/gpu/drm/i915/i915_drv.h
drivers/gpu/drm/i915/i915_irq.c

index 25fc3841a2b1c3dbf67a7ac32f8adc43da60a790..5c45e9e598deb0ef88b376e378ea2628ea4a6b0a 100644 (file)
@@ -1772,13 +1772,18 @@ static int i915_pipe_crc_open(struct inode *inode, struct file *filep)
        struct drm_i915_private *dev_priv = info->dev->dev_private;
        struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
 
-       if (!atomic_dec_and_test(&pipe_crc->available)) {
-               atomic_inc(&pipe_crc->available);
+       spin_lock_irq(&pipe_crc->lock);
+
+       if (pipe_crc->opened) {
+               spin_unlock_irq(&pipe_crc->lock);
                return -EBUSY; /* already open */
        }
 
+       pipe_crc->opened = true;
        filep->private_data = inode->i_private;
 
+       spin_unlock_irq(&pipe_crc->lock);
+
        return 0;
 }
 
@@ -1788,7 +1793,9 @@ static int i915_pipe_crc_release(struct inode *inode, struct file *filep)
        struct drm_i915_private *dev_priv = info->dev->dev_private;
        struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
 
-       atomic_inc(&pipe_crc->available); /* release the device */
+       spin_lock_irq(&pipe_crc->lock);
+       pipe_crc->opened = false;
+       spin_unlock_irq(&pipe_crc->lock);
 
        return 0;
 }
@@ -1800,12 +1807,9 @@ static int i915_pipe_crc_release(struct inode *inode, struct file *filep)
 
 static int pipe_crc_data_count(struct intel_pipe_crc *pipe_crc)
 {
-       int head, tail;
-
-       head = atomic_read(&pipe_crc->head);
-       tail = atomic_read(&pipe_crc->tail);
-
-       return CIRC_CNT(head, tail, INTEL_PIPE_CRC_ENTRIES_NR);
+       assert_spin_locked(&pipe_crc->lock);
+       return CIRC_CNT(pipe_crc->head, pipe_crc->tail,
+                       INTEL_PIPE_CRC_ENTRIES_NR);
 }
 
 static ssize_t
@@ -1831,20 +1835,30 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
                return 0;
 
        /* nothing to read */
+       spin_lock_irq(&pipe_crc->lock);
        while (pipe_crc_data_count(pipe_crc) == 0) {
-               if (filep->f_flags & O_NONBLOCK)
+               int ret;
+
+               if (filep->f_flags & O_NONBLOCK) {
+                       spin_unlock_irq(&pipe_crc->lock);
                        return -EAGAIN;
+               }
 
-               if (wait_event_interruptible(pipe_crc->wq,
-                                            pipe_crc_data_count(pipe_crc)))
-                        return -ERESTARTSYS;
+               ret = wait_event_interruptible_lock_irq(pipe_crc->wq,
+                               pipe_crc_data_count(pipe_crc), pipe_crc->lock);
+               if (ret) {
+                       spin_unlock_irq(&pipe_crc->lock);
+                       return ret;
+               }
        }
 
        /* We now have one or more entries to read */
-       head = atomic_read(&pipe_crc->head);
-       tail = atomic_read(&pipe_crc->tail);
+       head = pipe_crc->head;
+       tail = pipe_crc->tail;
        n_entries = min((size_t)CIRC_CNT(head, tail, INTEL_PIPE_CRC_ENTRIES_NR),
                        count / PIPE_CRC_LINE_LEN);
+       spin_unlock_irq(&pipe_crc->lock);
+
        bytes_read = 0;
        n = 0;
        do {
@@ -1864,10 +1878,13 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
 
                BUILD_BUG_ON_NOT_POWER_OF_2(INTEL_PIPE_CRC_ENTRIES_NR);
                tail = (tail + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
-               atomic_set(&pipe_crc->tail, tail);
                n++;
        } while (--n_entries);
 
+       spin_lock_irq(&pipe_crc->lock);
+       pipe_crc->tail = tail;
+       spin_unlock_irq(&pipe_crc->lock);
+
        return bytes_read;
 }
 
@@ -2111,8 +2128,10 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
                if (!pipe_crc->entries)
                        return -ENOMEM;
 
-               atomic_set(&pipe_crc->head, 0);
-               atomic_set(&pipe_crc->tail, 0);
+               spin_lock_irq(&pipe_crc->lock);
+               pipe_crc->head = 0;
+               pipe_crc->tail = 0;
+               spin_unlock_irq(&pipe_crc->lock);
        }
 
        pipe_crc->source = source;
@@ -2122,13 +2141,19 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 
        /* real source -> none transition */
        if (source == INTEL_PIPE_CRC_SOURCE_NONE) {
+               struct intel_pipe_crc_entry *entries;
+
                DRM_DEBUG_DRIVER("stopping CRCs for pipe %c\n",
                                 pipe_name(pipe));
 
                intel_wait_for_vblank(dev, pipe);
 
-               kfree(pipe_crc->entries);
+               spin_lock_irq(&pipe_crc->lock);
+               entries = pipe_crc->entries;
                pipe_crc->entries = NULL;
+               spin_unlock_irq(&pipe_crc->lock);
+
+               kfree(entries);
        }
 
        return 0;
@@ -2823,7 +2848,8 @@ void intel_display_crc_init(struct drm_device *dev)
        for (i = 0; i < INTEL_INFO(dev)->num_pipes; i++) {
                struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[i];
 
-               atomic_set(&pipe_crc->available, 1);
+               pipe_crc->opened = false;
+               spin_lock_init(&pipe_crc->lock);
                init_waitqueue_head(&pipe_crc->wq);
        }
 }
index 2e1e884ac86c03aa1ac19a96e44a712a530cd12c..5bfcf0f91a51723c67b2ab260a3fa6eea30b1cf7 100644 (file)
@@ -1249,10 +1249,11 @@ struct intel_pipe_crc_entry {
 
 #define INTEL_PIPE_CRC_ENTRIES_NR      128
 struct intel_pipe_crc {
-       atomic_t available;             /* exclusive access to the device */
+       spinlock_t lock;
+       bool opened;            /* exclusive access to the result file */
        struct intel_pipe_crc_entry *entries;
        enum intel_pipe_crc_source source;
-       atomic_t head, tail;
+       int head, tail;
        wait_queue_head_t wq;
 };
 
index 8f7baad7231649d482d81c8956d77a40f6e00cab..1a7dc7754e2f8511d43b810ce038997aebb6c015 100644 (file)
@@ -1200,15 +1200,19 @@ static void display_pipe_crc_irq_handler(struct drm_device *dev, enum pipe pipe,
        struct intel_pipe_crc_entry *entry;
        int head, tail;
 
+       spin_lock(&pipe_crc->lock);
+
        if (!pipe_crc->entries) {
+               spin_unlock(&pipe_crc->lock);
                DRM_ERROR("spurious interrupt\n");
                return;
        }
 
-       head = atomic_read(&pipe_crc->head);
-       tail = atomic_read(&pipe_crc->tail);
+       head = pipe_crc->head;
+       tail = pipe_crc->tail;
 
        if (CIRC_SPACE(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) < 1) {
+               spin_unlock(&pipe_crc->lock);
                DRM_ERROR("CRC buffer overflowing\n");
                return;
        }
@@ -1223,7 +1227,9 @@ static void display_pipe_crc_irq_handler(struct drm_device *dev, enum pipe pipe,
        entry->crc[4] = crc4;
 
        head = (head + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
-       atomic_set(&pipe_crc->head, head);
+       pipe_crc->head = head;
+
+       spin_unlock(&pipe_crc->lock);
 
        wake_up_interruptible(&pipe_crc->wq);
 }