Enabling and disabling the vblank interrupt (on devices that support it) is cheap. So disable it quickly after each interrupt.

To avoid constantly enabling and disabling vblank when animations are running, after a predefined number (3) of consecutive enabled vblanks that someone cared about, just leave the interrupt on until an interrupt that no one cares about.

The full heuristic is:

There's a per-CRTC counter called vblank_consecutive. It starts at zero and counts consecutive useful vblanks. A vblank is useful if the refcount is nonzero when the interrupt comes in.

Whenever drm_vblank_get enables the interrupt, it stays on at least until the next vblank (*). If drm_vblank_put is called and vblank_consecutive is less than a threshold (currently 3), then the interrupt is disabled. If a vblank interrupt happens with refcount == 0, then the interrupt is disabled and vblank_consecutive is reset to 0. If vblank_get is called with the interrupt disabled and no interrupts were missed, then vblank_consecutive is incremented.

(*) I tried letting it turn off before the next interrupt, but compiz on my laptop seems to call drm_wait_vblank twice with relative waits of 0 (!) before actually waiting.

Signed-off-by: Andy Lutomirski luto@mit.edu ---

Jesse, you asked for the deletion of the timer to be separate from reducing the timeout, but that seemed silly because I'm ripping out the entire old mechanism. If you're worried about the added time spent in the interrupt handler, I could move it to a tasklet. That being said, disable_vblank should be very fast (it's at most a couple of register accesses in all in-tree drivers).

I've tested this on i915, where it works nicely and reduces my wakeups with a second hand showing on the clock but an otherwise idle system.

This changes the requirements on enable_vblank, disable_vblank and get_vblank_counter: they can now be called from an IRQ. They already had to work with IRQs off and a spinlock held, but now a driver has to watch out for recursive calls from drm_handle_vblank. The vbl_lock is still held.

I've audited the in-tree drivers:

mga, r128: get_vblank_counter just reads an atomic_t. enable_vblank just pokes registers without a lock. disable_vblank does nothing, so turning off vblanks is pointless.

via: get_vblank_counter just returns a counter. enable_vblank and disable_vblank just poke registers without locks. (This looks wrong: get_vblank_count does the wrong thing and will confuse my heuristic, but it should be any worse than it already is. I can comment out enable_vblank if anyone prefers that approach.)

vmwgfx: get_vblank_counter does nothing and the other hooks aren't implemented.

radeon: Everything looks safe.

i915: Looks good and tested!

nouveau: Not implemented at all. I'm not sure why either the old code or my code doesn't try to call a null pointer, but it doesn't. That being said, sync-to-vblank doesn't work on nouveau for me (glxgears gets over 600fps while claiming to be synced to vblank).

As a future improvement, all of the vblank-disabling code could be skipped if there is no disable_vblank callback.

drivers/gpu/drm/drm_irq.c | 103 +++++++++++++++++++++++++++++++++----------- include/drm/drmP.h | 5 ++- 2 files changed, 81 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 9d3a503..2f107c5 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -77,45 +77,59 @@ int drm_irq_by_busid(struct drm_device *dev, void *data, return 0; }

-static void vblank_disable_fn(unsigned long arg) +/* After VBLANK_CONSEC_THRESHOLD consecutive non-ignored vblank interrupts, + * vblanks will be left on. */ +#define VBLANK_CONSEC_THRESHOLD 3 + +static void __vblank_disable_now(struct drm_device *dev, int crtc, int force) +{ + if (!dev->vblank_disable_allowed) + return; + + if (atomic_read(&dev->vblank_refcount[crtc]) == 0 && dev->vblank_enabled[crtc] && + (dev->vblank_consecutive[crtc] < VBLANK_CONSEC_THRESHOLD || force)) + { + DRM_DEBUG("disabling vblank on crtc %d\n", crtc); + dev->last_vblank[crtc] = + dev->driver->get_vblank_counter(dev, crtc); + dev->driver->disable_vblank(dev, crtc); + dev->vblank_enabled[crtc] = 0; + if (force) + dev->vblank_consecutive[crtc] = 0; + } +} + +static void vblank_disable_now(struct drm_device *dev, int crtc, int force) { - struct drm_device *dev = (struct drm_device *)arg; unsigned long irqflags; - int i;

if (!dev->vblank_disable_allowed) return;

- for (i = 0; i < dev->num_crtcs; i++) { - spin_lock_irqsave(&dev->vbl_lock, irqflags); - if (atomic_read(&dev->vblank_refcount[i]) == 0 && - dev->vblank_enabled[i]) { - DRM_DEBUG("disabling vblank on crtc %d\n", i); - dev->last_vblank[i] = - dev->driver->get_vblank_counter(dev, i); - dev->driver->disable_vblank(dev, i); - dev->vblank_enabled[i] = 0; - } - spin_unlock_irqrestore(&dev->vbl_lock, irqflags); - } + spin_lock_irqsave(&dev->vbl_lock, irqflags); + __vblank_disable_now(dev, crtc, force); + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); }

void drm_vblank_cleanup(struct drm_device *dev) { + int i; + /* Bail if the driver didn't call drm_vblank_init() */ if (dev->num_crtcs == 0) return;

- del_timer(&dev->vblank_disable_timer); - - vblank_disable_fn((unsigned long)dev); + for (i = 0; i < dev->num_crtcs; i++) + vblank_disable_now(dev, i, 1);

kfree(dev->vbl_queue); kfree(dev->_vblank_count); kfree(dev->vblank_refcount); kfree(dev->vblank_enabled); kfree(dev->last_vblank); + kfree(dev->last_vblank_enable); kfree(dev->last_vblank_wait); + kfree(dev->vblank_consecutive); kfree(dev->vblank_inmodeset);

dev->num_crtcs = 0; @@ -126,8 +140,6 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs) { int i, ret = -ENOMEM;

- setup_timer(&dev->vblank_disable_timer, vblank_disable_fn, - (unsigned long)dev); spin_lock_init(&dev->vbl_lock); dev->num_crtcs = num_crtcs;

@@ -153,10 +165,18 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs) if (!dev->last_vblank) goto err;

+ dev->last_vblank_enable = kcalloc(num_crtcs, sizeof(u32), GFP_KERNEL); + if (!dev->last_vblank) + goto err; + dev->last_vblank_wait = kcalloc(num_crtcs, sizeof(u32), GFP_KERNEL); if (!dev->last_vblank_wait) goto err;

+ dev->vblank_consecutive = kcalloc(num_crtcs, sizeof(int), GFP_KERNEL); + if (!dev->vblank_consecutive) + goto err; + dev->vblank_inmodeset = kcalloc(num_crtcs, sizeof(int), GFP_KERNEL); if (!dev->vblank_inmodeset) goto err; @@ -387,10 +407,12 @@ EXPORT_SYMBOL(drm_vblank_count); * Only necessary when going from off->on, to account for frames we * didn't get an interrupt for. * + * Returns the number of vblanks missed. + * * Note: caller must hold dev->vbl_lock since this reads & writes * device vblank fields. */ -static void drm_update_vblank_count(struct drm_device *dev, int crtc) +static u32 drm_update_vblank_count(struct drm_device *dev, int crtc) { u32 cur_vblank, diff;

@@ -414,6 +436,17 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc) crtc, diff);

atomic_add(diff, &dev->_vblank_count[crtc]); + return diff; +} + +static void vblank_enabled_consecutively(struct drm_device *dev, int crtc) +{ + if (dev->vblank_consecutive[crtc] < VBLANK_CONSEC_THRESHOLD) { + dev->vblank_consecutive[crtc]++; + if (dev->vblank_consecutive[crtc] == VBLANK_CONSEC_THRESHOLD) + DRM_DEBUG("consecutive vblank usage on crtc %d\n", crtc); + } + }

/** @@ -433,7 +466,6 @@ int drm_vblank_get(struct drm_device *dev, int crtc) int ret = 0;

spin_lock_irqsave(&dev->vbl_lock, irqflags); - /* Going from 0->1 means we have to enable interrupts again */ if (atomic_add_return(1, &dev->vblank_refcount[crtc]) == 1) { if (!dev->vblank_enabled[crtc]) { ret = dev->driver->enable_vblank(dev, crtc); @@ -442,7 +474,12 @@ int drm_vblank_get(struct drm_device *dev, int crtc) atomic_dec(&dev->vblank_refcount[crtc]); else { dev->vblank_enabled[crtc] = 1; - drm_update_vblank_count(dev, crtc); + if (drm_update_vblank_count(dev, crtc) == 0) + vblank_enabled_consecutively(dev, crtc); + else + dev->vblank_consecutive[crtc] = 0; + dev->last_vblank_enable[crtc] = + drm_vblank_count(dev, crtc); } } } else { @@ -467,11 +504,18 @@ EXPORT_SYMBOL(drm_vblank_get); */ void drm_vblank_put(struct drm_device *dev, int crtc) { + unsigned long irqflags; + BUG_ON (atomic_read (&dev->vblank_refcount[crtc]) == 0);

- /* Last user schedules interrupt disable */ - if (atomic_dec_and_test(&dev->vblank_refcount[crtc])) - mod_timer(&dev->vblank_disable_timer, jiffies + 5*DRM_HZ); + if (atomic_dec_and_test(&dev->vblank_refcount[crtc])) { + spin_lock_irqsave(&dev->vbl_lock, irqflags); + if (atomic_read(&dev->_vblank_count[crtc]) != + dev->last_vblank_enable[crtc] + && dev->vblank_consecutive[crtc] < VBLANK_CONSEC_THRESHOLD) + __vblank_disable_now(dev, crtc, 0); + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); + } } EXPORT_SYMBOL(drm_vblank_put);

@@ -780,11 +824,18 @@ void drm_handle_vblank_events(struct drm_device *dev, int crtc) */ void drm_handle_vblank(struct drm_device *dev, int crtc) { + int vblank_refcount; if (!dev->num_crtcs) return;

+ vblank_refcount = atomic_read(&dev->vblank_refcount[crtc]); atomic_inc(&dev->_vblank_count[crtc]); DRM_WAKEUP(&dev->vbl_queue[crtc]); drm_handle_vblank_events(dev, crtc); + + if (vblank_refcount == 0) { + /* This interrupt was unnecessary. */ + vblank_disable_now(dev, crtc, 1); + } } EXPORT_SYMBOL(drm_handle_vblank); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 4c9461a..c15918b 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -997,11 +997,14 @@ struct drm_device { atomic_t *vblank_refcount; /* number of users of vblank interruptsper crtc */ u32 *last_vblank; /* protected by dev->vbl_lock, used */ /* for wraparound handling */ + u32 *last_vblank_enable; /* protected by dev->vbl_lock, used */ + /* for early disable path */ int *vblank_enabled; /* so we don't call enable more than once per disable */ int *vblank_inmodeset; /* Display driver is setting mode */ u32 *last_vblank_wait; /* Last vblank seqno waited per CRTC */ - struct timer_list vblank_disable_timer; + int *vblank_consecutive; /* protected by dev->vbl_lock, counts + consecutive interesting vblanks */

u32 max_vblank_count; /**< size of vblank counter register */