[Intel-gfx] [PATCH v3] drm/i915: Use a spin lock to protect the pipe crc struct
Daniel Vetter
daniel at ffwll.ch
Fri Oct 18 15:21:18 CEST 2013
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 at ffwll.ch>
> Signed-off-by: Damien Lespiau <damien.lespiau at 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 at 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
More information about the Intel-gfx
mailing list