[PATCH] drm/i915: Use a spin lock to protect the pipe crc struct

[PATCH] drm/i915: Use a spin lock to protect the pipe crc struct

[Damien Lespiau]

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.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 53 ++++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/i915_drv.h     |  5 ++--
 drivers/gpu/drm/i915/i915_irq.c     | 11 +++++---
 3 files changed, 49 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 061182a..ae182af 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1771,14 +1771,20 @@ static int i915_pipe_crc_open(struct inode *inode, struct file *filep)
 	struct pipe_crc_info *info = inode->i_private;
 	struct drm_i915_private *dev_priv = info->dev->dev_private;
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
+	unsigned long irqflags;
 
-	if (!atomic_dec_and_test(&pipe_crc->available)) {
-		atomic_inc(&pipe_crc->available);
+	spin_lock_irqsave(&pipe_crc->lock, irqflags);
+
+	if (pipe_crc->opened) {
+		spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
 		return -EBUSY; /* already open */
 	}
 
+	pipe_crc->opened = true;
 	filep->private_data = inode->i_private;
 
+	spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
+
 	return 0;
 }
 
@@ -1787,8 +1793,11 @@ static int i915_pipe_crc_release(struct inode *inode, struct file *filep)
 	struct pipe_crc_info *info = inode->i_private;
 	struct drm_i915_private *dev_priv = info->dev->dev_private;
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
+	unsigned long irqflags;
 
-	atomic_inc(&pipe_crc->available); /* release the device */
+	spin_lock_irqsave(&pipe_crc->lock, irqflags);
+	pipe_crc->opened = false;
+	spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
 
 	return 0;
 }
@@ -1798,14 +1807,11 @@ static int i915_pipe_crc_release(struct inode *inode, struct file *filep)
 /* account for \'0' */
 #define PIPE_CRC_BUFFER_LEN	(PIPE_CRC_LINE_LEN + 1)
 
+/* needs pipe_crc->lock */
 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);
+	return CIRC_CNT(pipe_crc->head, pipe_crc->tail,
+			INTEL_PIPE_CRC_ENTRIES_NR);
 }
 
 static ssize_t
@@ -1819,6 +1825,7 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
 	char buf[PIPE_CRC_BUFFER_LEN];
 	int head, tail, n_entries, n;
 	ssize_t bytes_read;
+	unsigned long irqflags;
 
 	/*
 	 * Don't allow user space to provide buffers not big enough to hold
@@ -1830,21 +1837,29 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
 	if (pipe_crc->source == INTEL_PIPE_CRC_SOURCE_NONE)
 		return 0;
 
+
 	/* nothing to read */
+	spin_lock_irqsave(&pipe_crc->lock, irqflags);
 	while (pipe_crc_data_count(pipe_crc) == 0) {
+		spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
+
 		if (filep->f_flags & O_NONBLOCK)
 			return -EAGAIN;
 
 		if (wait_event_interruptible(pipe_crc->wq,
 					     pipe_crc_data_count(pipe_crc)))
 			 return -ERESTARTSYS;
+
+		spin_lock_irqsave(&pipe_crc->lock, irqflags);
 	}
 
 	/* 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_irqrestore(&pipe_crc->lock, irqflags);
+
 	bytes_read = 0;
 	n = 0;
 	do {
@@ -1864,10 +1879,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_irqsave(&pipe_crc->lock, irqflags);
+	pipe_crc->tail = tail;
+	spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
+
 	return bytes_read;
 }
 
@@ -2017,6 +2035,8 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 
 	/* none -> real source transition */
 	if (source) {
+		unsigned long irqflags;
+
 		DRM_DEBUG_DRIVER("collecting CRCs for pipe %c, %s\n",
 				 pipe_name(pipe), pipe_crc_source_name(source));
 
@@ -2026,8 +2046,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_irqsave(&pipe_crc->lock, irqflags);
+		pipe_crc->head = 0;
+		pipe_crc->tail = 0;
+		spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
 	}
 
 	pipe_crc->source = source;
@@ -2738,7 +2760,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);
 	}
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2ea33ee..f6b732f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1234,10 +1234,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;
+	int 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;
 };
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 156a1a4..a9ea8be 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1198,15 +1198,18 @@ static void display_pipe_crc_update(struct drm_device *dev, enum pipe pipe,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
 	struct intel_pipe_crc_entry *entry;
+	unsigned long irqflags;
 	int head, tail;
 
+	spin_lock_irqsave(&pipe_crc->lock, irqflags);
+
 	if (!pipe_crc->entries) {
 		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) {
 		DRM_ERROR("CRC buffer overflowing\n");
@@ -1223,7 +1226,9 @@ static void display_pipe_crc_update(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_irqrestore(&pipe_crc->lock, irqflags);
 
 	wake_up_interruptible(&pipe_crc->wq);
 }
-- 
1.8.3.1

[PATCH v2] drm/i915: Use a spin lock to protect the pipe crc struct

[Damien Lespiau]

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

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 53 ++++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/i915_drv.h     |  5 ++--
 drivers/gpu/drm/i915/i915_irq.c     | 11 +++++---
 3 files changed, 49 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 061182a..ae182af 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1771,14 +1771,20 @@ static int i915_pipe_crc_open(struct inode *inode, struct file *filep)
 	struct pipe_crc_info *info = inode->i_private;
 	struct drm_i915_private *dev_priv = info->dev->dev_private;
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
+	unsigned long irqflags;
 
-	if (!atomic_dec_and_test(&pipe_crc->available)) {
-		atomic_inc(&pipe_crc->available);
+	spin_lock_irqsave(&pipe_crc->lock, irqflags);
+
+	if (pipe_crc->opened) {
+		spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
 		return -EBUSY; /* already open */
 	}
 
+	pipe_crc->opened = true;
 	filep->private_data = inode->i_private;
 
+	spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
+
 	return 0;
 }
 
@@ -1787,8 +1793,11 @@ static int i915_pipe_crc_release(struct inode *inode, struct file *filep)
 	struct pipe_crc_info *info = inode->i_private;
 	struct drm_i915_private *dev_priv = info->dev->dev_private;
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
+	unsigned long irqflags;
 
-	atomic_inc(&pipe_crc->available); /* release the device */
+	spin_lock_irqsave(&pipe_crc->lock, irqflags);
+	pipe_crc->opened = false;
+	spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
 
 	return 0;
 }
@@ -1798,14 +1807,11 @@ static int i915_pipe_crc_release(struct inode *inode, struct file *filep)
 /* account for \'0' */
 #define PIPE_CRC_BUFFER_LEN	(PIPE_CRC_LINE_LEN + 1)
 
+/* needs pipe_crc->lock */
 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);
+	return CIRC_CNT(pipe_crc->head, pipe_crc->tail,
+			INTEL_PIPE_CRC_ENTRIES_NR);
 }
 
 static ssize_t
@@ -1819,6 +1825,7 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
 	char buf[PIPE_CRC_BUFFER_LEN];
 	int head, tail, n_entries, n;
 	ssize_t bytes_read;
+	unsigned long irqflags;
 
 	/*
 	 * Don't allow user space to provide buffers not big enough to hold
@@ -1830,21 +1837,29 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
 	if (pipe_crc->source == INTEL_PIPE_CRC_SOURCE_NONE)
 		return 0;
 
+
 	/* nothing to read */
+	spin_lock_irqsave(&pipe_crc->lock, irqflags);
 	while (pipe_crc_data_count(pipe_crc) == 0) {
+		spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
+
 		if (filep->f_flags & O_NONBLOCK)
 			return -EAGAIN;
 
 		if (wait_event_interruptible(pipe_crc->wq,
 					     pipe_crc_data_count(pipe_crc)))
 			 return -ERESTARTSYS;
+
+		spin_lock_irqsave(&pipe_crc->lock, irqflags);
 	}
 
 	/* 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_irqrestore(&pipe_crc->lock, irqflags);
+
 	bytes_read = 0;
 	n = 0;
 	do {
@@ -1864,10 +1879,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_irqsave(&pipe_crc->lock, irqflags);
+	pipe_crc->tail = tail;
+	spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
+
 	return bytes_read;
 }
 
@@ -2017,6 +2035,8 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 
 	/* none -> real source transition */
 	if (source) {
+		unsigned long irqflags;
+
 		DRM_DEBUG_DRIVER("collecting CRCs for pipe %c, %s\n",
 				 pipe_name(pipe), pipe_crc_source_name(source));
 
@@ -2026,8 +2046,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_irqsave(&pipe_crc->lock, irqflags);
+		pipe_crc->head = 0;
+		pipe_crc->tail = 0;
+		spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
 	}
 
 	pipe_crc->source = source;
@@ -2738,7 +2760,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);
 	}
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2ea33ee..0699b93 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1234,10 +1234,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;
 };
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 156a1a4..a9ea8be 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1198,15 +1198,18 @@ static void display_pipe_crc_update(struct drm_device *dev, enum pipe pipe,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
 	struct intel_pipe_crc_entry *entry;
+	unsigned long irqflags;
 	int head, tail;
 
+	spin_lock_irqsave(&pipe_crc->lock, irqflags);
+
 	if (!pipe_crc->entries) {
 		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) {
 		DRM_ERROR("CRC buffer overflowing\n");
@@ -1223,7 +1226,9 @@ static void display_pipe_crc_update(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_irqrestore(&pipe_crc->lock, irqflags);
 
 	wake_up_interruptible(&pipe_crc->wq);
 }
-- 
1.8.3.1

[PATCH v3] drm/i915: Use a spin lock to protect the pipe crc struct

[Damien Lespiau]

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)

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 53 ++++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/i915_drv.h     |  5 ++--
 drivers/gpu/drm/i915/i915_irq.c     | 11 +++++---
 3 files changed, 49 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 061182a..ebbb50e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1771,14 +1771,20 @@ static int i915_pipe_crc_open(struct inode *inode, struct file *filep)
 	struct pipe_crc_info *info = inode->i_private;
 	struct drm_i915_private *dev_priv = info->dev->dev_private;
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
+	unsigned long irqflags;
 
-	if (!atomic_dec_and_test(&pipe_crc->available)) {
-		atomic_inc(&pipe_crc->available);
+	spin_lock_irqsave(&pipe_crc->lock, irqflags);
+
+	if (pipe_crc->opened) {
+		spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
 		return -EBUSY; /* already open */
 	}
 
+	pipe_crc->opened = true;
 	filep->private_data = inode->i_private;
 
+	spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
+
 	return 0;
 }
 
@@ -1787,8 +1793,11 @@ static int i915_pipe_crc_release(struct inode *inode, struct file *filep)
 	struct pipe_crc_info *info = inode->i_private;
 	struct drm_i915_private *dev_priv = info->dev->dev_private;
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
+	unsigned long irqflags;
 
-	atomic_inc(&pipe_crc->available); /* release the device */
+	spin_lock_irqsave(&pipe_crc->lock, irqflags);
+	pipe_crc->opened = false;
+	spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
 
 	return 0;
 }
@@ -1800,12 +1809,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
@@ -1819,6 +1825,7 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
 	char buf[PIPE_CRC_BUFFER_LEN];
 	int head, tail, n_entries, n;
 	ssize_t bytes_read;
+	unsigned long irqflags;
 
 	/*
 	 * Don't allow user space to provide buffers not big enough to hold
@@ -1830,21 +1837,29 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
 	if (pipe_crc->source == INTEL_PIPE_CRC_SOURCE_NONE)
 		return 0;
 
+
 	/* nothing to read */
+	spin_lock_irqsave(&pipe_crc->lock, irqflags);
 	while (pipe_crc_data_count(pipe_crc) == 0) {
+		spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
+
 		if (filep->f_flags & O_NONBLOCK)
 			return -EAGAIN;
 
 		if (wait_event_interruptible(pipe_crc->wq,
 					     pipe_crc_data_count(pipe_crc)))
 			 return -ERESTARTSYS;
+
+		spin_lock_irqsave(&pipe_crc->lock, irqflags);
 	}
 
 	/* 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_irqrestore(&pipe_crc->lock, irqflags);
+
 	bytes_read = 0;
 	n = 0;
 	do {
@@ -1864,10 +1879,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_irqsave(&pipe_crc->lock, irqflags);
+	pipe_crc->tail = tail;
+	spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
+
 	return bytes_read;
 }
 
@@ -2017,6 +2035,8 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 
 	/* none -> real source transition */
 	if (source) {
+		unsigned long irqflags;
+
 		DRM_DEBUG_DRIVER("collecting CRCs for pipe %c, %s\n",
 				 pipe_name(pipe), pipe_crc_source_name(source));
 
@@ -2026,8 +2046,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_irqsave(&pipe_crc->lock, irqflags);
+		pipe_crc->head = 0;
+		pipe_crc->tail = 0;
+		spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
 	}
 
 	pipe_crc->source = source;
@@ -2738,7 +2760,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);
 	}
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2ea33ee..0699b93 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1234,10 +1234,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;
 };
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 156a1a4..a9ea8be 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1198,15 +1198,18 @@ static void display_pipe_crc_update(struct drm_device *dev, enum pipe pipe,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
 	struct intel_pipe_crc_entry *entry;
+	unsigned long irqflags;
 	int head, tail;
 
+	spin_lock_irqsave(&pipe_crc->lock, irqflags);
+
 	if (!pipe_crc->entries) {
 		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) {
 		DRM_ERROR("CRC buffer overflowing\n");
@@ -1223,7 +1226,9 @@ static void display_pipe_crc_update(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_irqrestore(&pipe_crc->lock, irqflags);
 
 	wake_up_interruptible(&pipe_crc->wq);
 }
-- 
1.8.3.1

Re: [PATCH v3] drm/i915: Use a spin lock to protect the pipe crc struct

[Daniel Vetter]

On Thu, Oct 17, 2013 at 05:32:28PM +0100, Damien Lespiau wrote:
> 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)

Ok, done a real review instead of just the quick comment bikeshed
yesterday on irc. See below for inline comments.
-Daniel

> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 53 ++++++++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/i915_drv.h     |  5 ++--
>  drivers/gpu/drm/i915/i915_irq.c     | 11 +++++---
>  3 files changed, 49 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 061182a..ebbb50e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1771,14 +1771,20 @@ static int i915_pipe_crc_open(struct inode *inode, struct file *filep)
>  	struct pipe_crc_info *info = inode->i_private;
>  	struct drm_i915_private *dev_priv = info->dev->dev_private;
>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
> +	unsigned long irqflags;
>  
> -	if (!atomic_dec_and_test(&pipe_crc->available)) {
> -		atomic_inc(&pipe_crc->available);
> +	spin_lock_irqsave(&pipe_crc->lock, irqflags);

Since this is process context and there's no irqsafe spinlock nesting
going on a spin_lock_irq is good enough. A while ago I've demoted all the
spin_lock_irqsave where possible, imo it helps a bit in making the call
context clear.

> +
> +	if (pipe_crc->opened) {
> +		spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
>  		return -EBUSY; /* already open */
>  	}
>  
> +	pipe_crc->opened = true;
>  	filep->private_data = inode->i_private;
>  
> +	spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
> +
>  	return 0;
>  }
>  
> @@ -1787,8 +1793,11 @@ static int i915_pipe_crc_release(struct inode *inode, struct file *filep)
>  	struct pipe_crc_info *info = inode->i_private;
>  	struct drm_i915_private *dev_priv = info->dev->dev_private;
>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[info->pipe];
> +	unsigned long irqflags;
>  
> -	atomic_inc(&pipe_crc->available); /* release the device */
> +	spin_lock_irqsave(&pipe_crc->lock, irqflags);
> +	pipe_crc->opened = false;
> +	spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
>  
>  	return 0;
>  }
> @@ -1800,12 +1809,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
> @@ -1819,6 +1825,7 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
>  	char buf[PIPE_CRC_BUFFER_LEN];
>  	int head, tail, n_entries, n;
>  	ssize_t bytes_read;
> +	unsigned long irqflags;
>  
>  	/*
>  	 * Don't allow user space to provide buffers not big enough to hold
> @@ -1830,21 +1837,29 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count,
>  	if (pipe_crc->source == INTEL_PIPE_CRC_SOURCE_NONE)
>  		return 0;
>  
> +
>  	/* nothing to read */
> +	spin_lock_irqsave(&pipe_crc->lock, irqflags);
>  	while (pipe_crc_data_count(pipe_crc) == 0) {
> +		spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
> +
>  		if (filep->f_flags & O_NONBLOCK)
>  			return -EAGAIN;
>  
>  		if (wait_event_interruptible(pipe_crc->wq,
>  					     pipe_crc_data_count(pipe_crc)))

I expect this to be unhappy about the assert_spin_locked. Could be that
you need to enable lockdep for the check to actually fire though. There's
a special wait_event_interruptible_lock_irq which dtrt.

>  			 return -ERESTARTSYS;
> +
> +		spin_lock_irqsave(&pipe_crc->lock, irqflags);
>  	}
>  
>  	/* 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_irqrestore(&pipe_crc->lock, irqflags);
> +
>  	bytes_read = 0;
>  	n = 0;
>  	do {
> @@ -1864,10 +1879,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_irqsave(&pipe_crc->lock, irqflags);
> +	pipe_crc->tail = tail;
> +	spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
> +
>  	return bytes_read;
>  }
>  
> @@ -2017,6 +2035,8 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  
>  	/* none -> real source transition */
>  	if (source) {
> +		unsigned long irqflags;
> +
>  		DRM_DEBUG_DRIVER("collecting CRCs for pipe %c, %s\n",
>  				 pipe_name(pipe), pipe_crc_source_name(source));
>  
> @@ -2026,8 +2046,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_irqsave(&pipe_crc->lock, irqflags);
> +		pipe_crc->head = 0;
> +		pipe_crc->tail = 0;
> +		spin_unlock_irqrestore(&pipe_crc->lock, irqflags);
>  	}

At the end of pipe_crc_set_source we also free the pipe_crc->entries
array, which is checked from the interrup handler. So that also needs to
be wrapped in the spinlock (and the kfree moved outside of the spinlock
after the pointer is cleared).

>  
>  	pipe_crc->source = source;
> @@ -2738,7 +2760,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);
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2ea33ee..0699b93 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1234,10 +1234,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;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 156a1a4..a9ea8be 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1198,15 +1198,18 @@ static void display_pipe_crc_update(struct drm_device *dev, enum pipe pipe,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
>  	struct intel_pipe_crc_entry *entry;
> +	unsigned long irqflags;
>  	int head, tail;
>  
> +	spin_lock_irqsave(&pipe_crc->lock, irqflags);
> +
>  	if (!pipe_crc->entries) {
>  		DRM_ERROR("spurious interrupt\n");

Leaked lock. Also since we know that this is only ever called from
interrupt context we can use the plain spin_lock function. See the various
spin locking going on in the *_irq_handler functions.

>  		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) {
>  		DRM_ERROR("CRC buffer overflowing\n");
> @@ -1223,7 +1226,9 @@ static void display_pipe_crc_update(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_irqrestore(&pipe_crc->lock, irqflags);
>  
>  	wake_up_interruptible(&pipe_crc->wq);
>  }
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v4] drm/i915: Use a spin lock to protect the pipe crc struct

[Damien Lespiau]

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>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 66 ++++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/i915_drv.h     |  5 +--
 drivers/gpu/drm/i915/i915_irq.c     | 12 +++++--
 3 files changed, 58 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 061182a..fd80064 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -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;
 }
 
@@ -2026,8 +2043,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;
@@ -2037,13 +2056,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;
@@ -2738,7 +2763,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);
 	}
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2ea33ee..0699b93 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1234,10 +1234,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;
 };
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 156a1a4..0e50dfd 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1200,15 +1200,19 @@ static void display_pipe_crc_update(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_update(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);
 }
-- 
1.8.3.1

Re: [PATCH v4] drm/i915: Use a spin lock to protect the pipe crc struct

[Daniel Vetter]

On Mon, Oct 21, 2013 at 02:29:30PM +0100, Damien Lespiau wrote:
> 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>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch