[PATCH] drm: Aggressively disable vblanks

* [PATCH] drm: Aggressively disable vblanks
@ 2010-12-20 19:00 Andy Lutomirski
  2010-12-21  3:23 ` Keith Packard
  0 siblings, 1 reply; 7+ messages in thread
From: Andy Lutomirski @ 2010-12-20 19:00 UTC (permalink / raw)
  To: Jesse Barnes, Chris Wilson, David Airlie
  Cc: intel-gfx, Andy Lutomirski, dri-devel

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 */
 
-- 
1.7.3.3

^ permalink raw reply related	[flat|nested] 7+ messages in thread