| Message ID | 1462810123-28493-1-git-send-email-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/09/2016 08:11 PM, Daniel Vetter wrote: > On Mon, May 09, 2016 at 08:16:07PM +0300, Ville Syrjälä wrote: >> On Mon, May 09, 2016 at 05:08:43PM +0100, Matthew Auld wrote: >>> This patch aims to replace the roll-your-own seqlock implementation with >>> full-blown seqlock'. We also remove the timestamp ring-buffer in favour >>> of single timestamp/count pair protected by a seqlock. In turn this >>> means we can now increment the vblank freely without the need for >>> clamping. >> >> This will also change the behaviour to block new readers while the >> writer has the lock, whereas the old code would allow readers to >> proceed in parallel. We do the whole hw counter + scanout position >> query while holding the lock so it's not exactly zero amount of work, >> but I'm not sure that's a real problem. >> >> I guess we could reduce the scope of the seqlock, but then maybe we'd >> need to keep the vblank_time_lock spinlock as well. The details escape >> me now, so I'd have re-read the code again. >> >> Ccing Mario too. > > Yeah, my idea was to keep the spinlock, and only replace the stuff in > store_vblank and the few do {} while (cur_vblank != get_vblank_counter) > loops. Extending the seqlock stuff to everything seems indeed counter to > Mario's locking scheme. > > So goal would be to really just replace the half-baked seqlock that we > have already, and leave all other locking unchanged. > -Daniel +1 to that, for simplicity. I thought Ville already had a patch laying around somewhere which essentially does this? -mario > >> >>> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>> --- >>> drivers/gpu/drm/drm_irq.c | 111 +++++++++------------------------------------- >>> include/drm/drmP.h | 14 ++---- >>> 2 files changed, 25 insertions(+), 100 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c >>> index 3c1a6f1..bfc6a8d 100644 >>> --- a/drivers/gpu/drm/drm_irq.c >>> +++ b/drivers/gpu/drm/drm_irq.c >>> @@ -42,10 +42,6 @@ >>> #include <linux/vgaarb.h> >>> #include <linux/export.h> >>> >>> -/* Access macro for slots in vblank timestamp ringbuffer. */ >>> -#define vblanktimestamp(dev, pipe, count) \ >>> - ((dev)->vblank[pipe].time[(count) % DRM_VBLANKTIME_RBSIZE]) >>> - >>> /* Retry timestamp calculation up to 3 times to satisfy >>> * drm_timestamp_precision before giving up. >>> */ >>> @@ -82,29 +78,13 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe, >>> struct timeval *t_vblank, u32 last) >>> { >>> struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; >>> - u32 tslot; >>> >>> - assert_spin_locked(&dev->vblank_time_lock); >>> + assert_spin_locked(&dev->vblank_seqlock.lock); >>> >>> vblank->last = last; >>> >>> - /* All writers hold the spinlock, but readers are serialized by >>> - * the latching of vblank->count below. >>> - */ >>> - tslot = vblank->count + vblank_count_inc; >>> - vblanktimestamp(dev, pipe, tslot) = *t_vblank; >>> - >>> - /* >>> - * vblank timestamp updates are protected on the write side with >>> - * vblank_time_lock, but on the read side done locklessly using a >>> - * sequence-lock on the vblank counter. Ensure correct ordering using >>> - * memory barrriers. We need the barrier both before and also after the >>> - * counter update to synchronize with the next timestamp write. >>> - * The read-side barriers for this are in drm_vblank_count_and_time. >>> - */ >>> - smp_wmb(); >>> + vblank->time = *t_vblank; >>> vblank->count += vblank_count_inc; >>> - smp_wmb(); >>> } >>> >>> /** >>> @@ -127,7 +107,7 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe >>> struct timeval t_vblank; >>> int count = DRM_TIMESTAMP_MAXRETRIES; >>> >>> - spin_lock(&dev->vblank_time_lock); >>> + write_seqlock(&dev->vblank_seqlock); >>> >>> /* >>> * sample the current counter to avoid random jumps >>> @@ -152,7 +132,7 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe >>> */ >>> store_vblank(dev, pipe, 1, &t_vblank, cur_vblank); >>> >>> - spin_unlock(&dev->vblank_time_lock); >>> + write_sequnlock(&dev->vblank_seqlock); >>> } >>> >>> /** >>> @@ -205,7 +185,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, >>> const struct timeval *t_old; >>> u64 diff_ns; >>> >>> - t_old = &vblanktimestamp(dev, pipe, vblank->count); >>> + t_old = &vblank->time; >>> diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old); >>> >>> /* >>> @@ -239,49 +219,6 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, >>> diff = 1; >>> } >>> >>> - /* >>> - * FIMXE: Need to replace this hack with proper seqlocks. >>> - * >>> - * Restrict the bump of the software vblank counter to a safe maximum >>> - * value of +1 whenever there is the possibility that concurrent readers >>> - * of vblank timestamps could be active at the moment, as the current >>> - * implementation of the timestamp caching and updating is not safe >>> - * against concurrent readers for calls to store_vblank() with a bump >>> - * of anything but +1. A bump != 1 would very likely return corrupted >>> - * timestamps to userspace, because the same slot in the cache could >>> - * be concurrently written by store_vblank() and read by one of those >>> - * readers without the read-retry logic detecting the collision. >>> - * >>> - * Concurrent readers can exist when we are called from the >>> - * drm_vblank_off() or drm_vblank_on() functions and other non-vblank- >>> - * irq callers. However, all those calls to us are happening with the >>> - * vbl_lock locked to prevent drm_vblank_get(), so the vblank refcount >>> - * can't increase while we are executing. Therefore a zero refcount at >>> - * this point is safe for arbitrary counter bumps if we are called >>> - * outside vblank irq, a non-zero count is not 100% safe. Unfortunately >>> - * we must also accept a refcount of 1, as whenever we are called from >>> - * drm_vblank_get() -> drm_vblank_enable() the refcount will be 1 and >>> - * we must let that one pass through in order to not lose vblank counts >>> - * during vblank irq off - which would completely defeat the whole >>> - * point of this routine. >>> - * >>> - * Whenever we are called from vblank irq, we have to assume concurrent >>> - * readers exist or can show up any time during our execution, even if >>> - * the refcount is currently zero, as vblank irqs are usually only >>> - * enabled due to the presence of readers, and because when we are called >>> - * from vblank irq we can't hold the vbl_lock to protect us from sudden >>> - * bumps in vblank refcount. Therefore also restrict bumps to +1 when >>> - * called from vblank irq. >>> - */ >>> - if ((diff > 1) && (atomic_read(&vblank->refcount) > 1 || >>> - (flags & DRM_CALLED_FROM_VBLIRQ))) { >>> - DRM_DEBUG_VBL("clamping vblank bump to 1 on crtc %u: diffr=%u " >>> - "refcount %u, vblirq %u\n", pipe, diff, >>> - atomic_read(&vblank->refcount), >>> - (flags & DRM_CALLED_FROM_VBLIRQ) != 0); >>> - diff = 1; >>> - } >>> - >>> DRM_DEBUG_VBL("updating vblank count on crtc %u:" >>> " current=%u, diff=%u, hw=%u hw_last=%u\n", >>> pipe, vblank->count, diff, cur_vblank, vblank->last); >>> @@ -318,7 +255,7 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe) >>> * so no updates of timestamps or count can happen after we've >>> * disabled. Needed to prevent races in case of delayed irq's. >>> */ >>> - spin_lock_irqsave(&dev->vblank_time_lock, irqflags); >>> + write_seqlock_irqsave(&dev->vblank_seqlock, irqflags); >>> >>> /* >>> * Only disable vblank interrupts if they're enabled. This avoids >>> @@ -338,7 +275,7 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe) >>> */ >>> drm_update_vblank_count(dev, pipe, 0); >>> >>> - spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags); >>> + write_sequnlock_irqrestore(&dev->vblank_seqlock, irqflags); >>> } >>> >>> static void vblank_disable_fn(unsigned long arg) >>> @@ -404,7 +341,7 @@ int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs) >>> unsigned int i; >>> >>> spin_lock_init(&dev->vbl_lock); >>> - spin_lock_init(&dev->vblank_time_lock); >>> + seqlock_init(&dev->vblank_seqlock); >>> >>> dev->num_crtcs = num_crtcs; >>> >>> @@ -991,25 +928,19 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe, >>> struct timeval *vblanktime) >>> { >>> struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; >>> - int count = DRM_TIMESTAMP_MAXRETRIES; >>> - u32 cur_vblank; >>> + u32 vblank_count; >>> + unsigned int seq; >>> >>> if (WARN_ON(pipe >= dev->num_crtcs)) >>> return 0; >>> >>> - /* >>> - * Vblank timestamps are read lockless. To ensure consistency the vblank >>> - * counter is rechecked and ordering is ensured using memory barriers. >>> - * This works like a seqlock. The write-side barriers are in store_vblank. >>> - */ >>> do { >>> - cur_vblank = vblank->count; >>> - smp_rmb(); >>> - *vblanktime = vblanktimestamp(dev, pipe, cur_vblank); >>> - smp_rmb(); >>> - } while (cur_vblank != vblank->count && --count > 0); >>> + seq = read_seqbegin(&dev->vblank_seqlock); >>> + vblank_count = vblank->count; >>> + *vblanktime = vblank->time; >>> + } while (read_seqretry(&dev->vblank_seqlock, seq)); >>> >>> - return cur_vblank; >>> + return vblank_count; >>> } >>> EXPORT_SYMBOL(drm_vblank_count_and_time); >>> >>> @@ -1160,11 +1091,11 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe) >>> >>> assert_spin_locked(&dev->vbl_lock); >>> >>> - spin_lock(&dev->vblank_time_lock); >>> + write_seqlock(&dev->vblank_seqlock); >>> >>> if (!vblank->enabled) { >>> /* >>> - * Enable vblank irqs under vblank_time_lock protection. >>> + * Enable vblank irqs under vblank_seqlock protection. >>> * All vblank count & timestamp updates are held off >>> * until we are done reinitializing master counter and >>> * timestamps. Filtercode in drm_handle_vblank() will >>> @@ -1180,7 +1111,7 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe) >>> } >>> } >>> >>> - spin_unlock(&dev->vblank_time_lock); >>> + write_sequnlock(&dev->vblank_seqlock); >>> >>> return ret; >>> } >>> @@ -1880,18 +1811,18 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) >>> * vblank enable/disable, as this would cause inconsistent >>> * or corrupted timestamps and vblank counts. >>> */ >>> - spin_lock(&dev->vblank_time_lock); >>> + write_seqlock(&dev->vblank_seqlock); >>> >>> /* Vblank irq handling disabled. Nothing to do. */ >>> if (!vblank->enabled) { >>> - spin_unlock(&dev->vblank_time_lock); >>> + write_sequnlock(&dev->vblank_seqlock); >>> spin_unlock_irqrestore(&dev->event_lock, irqflags); >>> return false; >>> } >>> >>> drm_update_vblank_count(dev, pipe, DRM_CALLED_FROM_VBLIRQ); >>> >>> - spin_unlock(&dev->vblank_time_lock); >>> + write_sequnlock(&dev->vblank_seqlock); >>> >>> wake_up(&vblank->queue); >>> drm_handle_vblank_events(dev, pipe); >>> diff --git a/include/drm/drmP.h b/include/drm/drmP.h >>> index 360b2a7..8bee424 100644 >>> --- a/include/drm/drmP.h >>> +++ b/include/drm/drmP.h >>> @@ -52,6 +52,7 @@ >>> #include <linux/poll.h> >>> #include <linux/ratelimit.h> >>> #include <linux/sched.h> >>> +#include <linux/seqlock.h> >>> #include <linux/slab.h> >>> #include <linux/types.h> >>> #include <linux/vmalloc.h> >>> @@ -392,11 +393,6 @@ struct drm_master { >>> void *driver_priv; >>> }; >>> >>> -/* Size of ringbuffer for vblank timestamps. Just double-buffer >>> - * in initial implementation. >>> - */ >>> -#define DRM_VBLANKTIME_RBSIZE 2 >>> - >>> /* Flags and return codes for get_vblank_timestamp() driver function. */ >>> #define DRM_CALLED_FROM_VBLIRQ 1 >>> #define DRM_VBLANKTIME_SCANOUTPOS_METHOD (1 << 0) >>> @@ -725,10 +721,8 @@ struct drm_vblank_crtc { >>> wait_queue_head_t queue; /**< VBLANK wait queue */ >>> struct timer_list disable_timer; /* delayed disable timer */ >>> >>> - /* vblank counter, protected by dev->vblank_time_lock for writes */ >>> - u32 count; >>> - /* vblank timestamps, protected by dev->vblank_time_lock for writes */ >>> - struct timeval time[DRM_VBLANKTIME_RBSIZE]; >>> + u32 count; /* vblank counter, protected by dev->vblank_seqlock */ >>> + struct timeval time; /* vblank timestamp, protected by dev->vblank_seqlock */ >>> >>> atomic_t refcount; /* number of users of vblank interruptsper crtc */ >>> u32 last; /* protected by dev->vbl_lock, used */ >>> @@ -835,7 +829,7 @@ struct drm_device { >>> /* array of size num_crtcs */ >>> struct drm_vblank_crtc *vblank; >>> >>> - spinlock_t vblank_time_lock; /**< Protects vblank count and time updates during vblank enable/disable */ >>> + seqlock_t vblank_seqlock; /**< Protects vblank count and time updates during vblank enable/disable */ >>> spinlock_t vbl_lock; >>> >>> u32 max_vblank_count; /**< size of vblank counter register */ >>> -- >>> 2.4.11 >> >> -- >> Ville Syrjälä >> Intel OTC >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
There's an updated version of this patch already on the ml [1], which I Cc'd you in on. I take it that your @tuebingen.mpg.de is in fact an old email address? [1] https://patchwork.freedesktop.org/patch/86354/
On 05/18/2016 05:10 PM, Matthew Auld wrote: > There's an updated version of this patch already on the ml [1], which > I Cc'd you in on. I take it that your @tuebingen.mpg.de is in fact an > old email address? > > [1] https://patchwork.freedesktop.org/patch/86354/ > Your patch looks good to me. I'd only keep that one dropped comment line in drmP.h about the vblank counter and ts also needing to be protected by the vblank_timelock in addition to the seqlock, as this is still needed, especially to get _irqsave part of spin_lock_irqsave, as the write seqlocks in don't do the local irq disable. I'll give it a test later this week. Reviewed-by: Mario Kleiner <mario.kleiner.de@gmail.com> Indeed the old inactive @tuebingen.mpg.de is only a forward to the gmail address, probably with some botched mail filter rules, so they can go unnoticed quite a while. thanks, -mario
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 3c1a6f1..bfc6a8d 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -42,10 +42,6 @@ #include <linux/vgaarb.h> #include <linux/export.h> -/* Access macro for slots in vblank timestamp ringbuffer. */ -#define vblanktimestamp(dev, pipe, count) \ - ((dev)->vblank[pipe].time[(count) % DRM_VBLANKTIME_RBSIZE]) - /* Retry timestamp calculation up to 3 times to satisfy * drm_timestamp_precision before giving up. */ @@ -82,29 +78,13 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe, struct timeval *t_vblank, u32 last) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; - u32 tslot; - assert_spin_locked(&dev->vblank_time_lock); + assert_spin_locked(&dev->vblank_seqlock.lock); vblank->last = last; - /* All writers hold the spinlock, but readers are serialized by - * the latching of vblank->count below. - */ - tslot = vblank->count + vblank_count_inc; - vblanktimestamp(dev, pipe, tslot) = *t_vblank; - - /* - * vblank timestamp updates are protected on the write side with - * vblank_time_lock, but on the read side done locklessly using a - * sequence-lock on the vblank counter. Ensure correct ordering using - * memory barrriers. We need the barrier both before and also after the - * counter update to synchronize with the next timestamp write. - * The read-side barriers for this are in drm_vblank_count_and_time. - */ - smp_wmb(); + vblank->time = *t_vblank; vblank->count += vblank_count_inc; - smp_wmb(); } /** @@ -127,7 +107,7 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe struct timeval t_vblank; int count = DRM_TIMESTAMP_MAXRETRIES; - spin_lock(&dev->vblank_time_lock); + write_seqlock(&dev->vblank_seqlock); /* * sample the current counter to avoid random jumps @@ -152,7 +132,7 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe */ store_vblank(dev, pipe, 1, &t_vblank, cur_vblank); - spin_unlock(&dev->vblank_time_lock); + write_sequnlock(&dev->vblank_seqlock); } /** @@ -205,7 +185,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, const struct timeval *t_old; u64 diff_ns; - t_old = &vblanktimestamp(dev, pipe, vblank->count); + t_old = &vblank->time; diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old); /* @@ -239,49 +219,6 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, diff = 1; } - /* - * FIMXE: Need to replace this hack with proper seqlocks. - * - * Restrict the bump of the software vblank counter to a safe maximum - * value of +1 whenever there is the possibility that concurrent readers - * of vblank timestamps could be active at the moment, as the current - * implementation of the timestamp caching and updating is not safe - * against concurrent readers for calls to store_vblank() with a bump - * of anything but +1. A bump != 1 would very likely return corrupted - * timestamps to userspace, because the same slot in the cache could - * be concurrently written by store_vblank() and read by one of those - * readers without the read-retry logic detecting the collision. - * - * Concurrent readers can exist when we are called from the - * drm_vblank_off() or drm_vblank_on() functions and other non-vblank- - * irq callers. However, all those calls to us are happening with the - * vbl_lock locked to prevent drm_vblank_get(), so the vblank refcount - * can't increase while we are executing. Therefore a zero refcount at - * this point is safe for arbitrary counter bumps if we are called - * outside vblank irq, a non-zero count is not 100% safe. Unfortunately - * we must also accept a refcount of 1, as whenever we are called from - * drm_vblank_get() -> drm_vblank_enable() the refcount will be 1 and - * we must let that one pass through in order to not lose vblank counts - * during vblank irq off - which would completely defeat the whole - * point of this routine. - * - * Whenever we are called from vblank irq, we have to assume concurrent - * readers exist or can show up any time during our execution, even if - * the refcount is currently zero, as vblank irqs are usually only - * enabled due to the presence of readers, and because when we are called - * from vblank irq we can't hold the vbl_lock to protect us from sudden - * bumps in vblank refcount. Therefore also restrict bumps to +1 when - * called from vblank irq. - */ - if ((diff > 1) && (atomic_read(&vblank->refcount) > 1 || - (flags & DRM_CALLED_FROM_VBLIRQ))) { - DRM_DEBUG_VBL("clamping vblank bump to 1 on crtc %u: diffr=%u " - "refcount %u, vblirq %u\n", pipe, diff, - atomic_read(&vblank->refcount), - (flags & DRM_CALLED_FROM_VBLIRQ) != 0); - diff = 1; - } - DRM_DEBUG_VBL("updating vblank count on crtc %u:" " current=%u, diff=%u, hw=%u hw_last=%u\n", pipe, vblank->count, diff, cur_vblank, vblank->last); @@ -318,7 +255,7 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe) * so no updates of timestamps or count can happen after we've * disabled. Needed to prevent races in case of delayed irq's. */ - spin_lock_irqsave(&dev->vblank_time_lock, irqflags); + write_seqlock_irqsave(&dev->vblank_seqlock, irqflags); /* * Only disable vblank interrupts if they're enabled. This avoids @@ -338,7 +275,7 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe) */ drm_update_vblank_count(dev, pipe, 0); - spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags); + write_sequnlock_irqrestore(&dev->vblank_seqlock, irqflags); } static void vblank_disable_fn(unsigned long arg) @@ -404,7 +341,7 @@ int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs) unsigned int i; spin_lock_init(&dev->vbl_lock); - spin_lock_init(&dev->vblank_time_lock); + seqlock_init(&dev->vblank_seqlock); dev->num_crtcs = num_crtcs; @@ -991,25 +928,19 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe, struct timeval *vblanktime) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; - int count = DRM_TIMESTAMP_MAXRETRIES; - u32 cur_vblank; + u32 vblank_count; + unsigned int seq; if (WARN_ON(pipe >= dev->num_crtcs)) return 0; - /* - * Vblank timestamps are read lockless. To ensure consistency the vblank - * counter is rechecked and ordering is ensured using memory barriers. - * This works like a seqlock. The write-side barriers are in store_vblank. - */ do { - cur_vblank = vblank->count; - smp_rmb(); - *vblanktime = vblanktimestamp(dev, pipe, cur_vblank); - smp_rmb(); - } while (cur_vblank != vblank->count && --count > 0); + seq = read_seqbegin(&dev->vblank_seqlock); + vblank_count = vblank->count; + *vblanktime = vblank->time; + } while (read_seqretry(&dev->vblank_seqlock, seq)); - return cur_vblank; + return vblank_count; } EXPORT_SYMBOL(drm_vblank_count_and_time); @@ -1160,11 +1091,11 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe) assert_spin_locked(&dev->vbl_lock); - spin_lock(&dev->vblank_time_lock); + write_seqlock(&dev->vblank_seqlock); if (!vblank->enabled) { /* - * Enable vblank irqs under vblank_time_lock protection. + * Enable vblank irqs under vblank_seqlock protection. * All vblank count & timestamp updates are held off * until we are done reinitializing master counter and * timestamps. Filtercode in drm_handle_vblank() will @@ -1180,7 +1111,7 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe) } } - spin_unlock(&dev->vblank_time_lock); + write_sequnlock(&dev->vblank_seqlock); return ret; } @@ -1880,18 +1811,18 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) * vblank enable/disable, as this would cause inconsistent * or corrupted timestamps and vblank counts. */ - spin_lock(&dev->vblank_time_lock); + write_seqlock(&dev->vblank_seqlock); /* Vblank irq handling disabled. Nothing to do. */ if (!vblank->enabled) { - spin_unlock(&dev->vblank_time_lock); + write_sequnlock(&dev->vblank_seqlock); spin_unlock_irqrestore(&dev->event_lock, irqflags); return false; } drm_update_vblank_count(dev, pipe, DRM_CALLED_FROM_VBLIRQ); - spin_unlock(&dev->vblank_time_lock); + write_sequnlock(&dev->vblank_seqlock); wake_up(&vblank->queue); drm_handle_vblank_events(dev, pipe); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 360b2a7..8bee424 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -52,6 +52,7 @@ #include <linux/poll.h> #include <linux/ratelimit.h> #include <linux/sched.h> +#include <linux/seqlock.h> #include <linux/slab.h> #include <linux/types.h> #include <linux/vmalloc.h> @@ -392,11 +393,6 @@ struct drm_master { void *driver_priv; }; -/* Size of ringbuffer for vblank timestamps. Just double-buffer - * in initial implementation. - */ -#define DRM_VBLANKTIME_RBSIZE 2 - /* Flags and return codes for get_vblank_timestamp() driver function. */ #define DRM_CALLED_FROM_VBLIRQ 1 #define DRM_VBLANKTIME_SCANOUTPOS_METHOD (1 << 0) @@ -725,10 +721,8 @@ struct drm_vblank_crtc { wait_queue_head_t queue; /**< VBLANK wait queue */ struct timer_list disable_timer; /* delayed disable timer */ - /* vblank counter, protected by dev->vblank_time_lock for writes */ - u32 count; - /* vblank timestamps, protected by dev->vblank_time_lock for writes */ - struct timeval time[DRM_VBLANKTIME_RBSIZE]; + u32 count; /* vblank counter, protected by dev->vblank_seqlock */ + struct timeval time; /* vblank timestamp, protected by dev->vblank_seqlock */ atomic_t refcount; /* number of users of vblank interruptsper crtc */ u32 last; /* protected by dev->vbl_lock, used */ @@ -835,7 +829,7 @@ struct drm_device { /* array of size num_crtcs */ struct drm_vblank_crtc *vblank; - spinlock_t vblank_time_lock; /**< Protects vblank count and time updates during vblank enable/disable */ + seqlock_t vblank_seqlock; /**< Protects vblank count and time updates during vblank enable/disable */ spinlock_t vbl_lock; u32 max_vblank_count; /**< size of vblank counter register */
This patch aims to replace the roll-your-own seqlock implementation with full-blown seqlock'. We also remove the timestamp ring-buffer in favour of single timestamp/count pair protected by a seqlock. In turn this means we can now increment the vblank freely without the need for clamping. Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Matthew Auld <matthew.auld@intel.com> --- drivers/gpu/drm/drm_irq.c | 111 +++++++++------------------------------------- include/drm/drmP.h | 14 ++---- 2 files changed, 25 insertions(+), 100 deletions(-)