Re: [Intel-gfx] [PATCH 8/9] drm: Reduce the amount of dev->vblank[crtc] in the code

On Mon, May 26, 2014 at 02:46:31PM +0300, ville.syrjala@linux.intel.com wrote:

From: Ville Syrjälä ville.syrjala@linux.intel.com

Declare a local struct drm_vblank_crtc * and use that instead of having to do dig it out via 'dev->vblank[crtc]' everywhere.

Performed with the following coccinelle incantation, and a few manual whitespace cleanups:

@@ identifier func,member; expression num_crtcs; struct drm_device *dev; unsigned int crtc; @@ func (...) {

• struct drm_vblank_crtc *vblank;

... if (crtc >= num_crtcs) return ...;

• vblank = &dev->vblank[crtc];

<+... (

• dev->vblank[crtc].member

• vblank->member

|

• &(dev->vblank[crtc])

• vblank

) ...+> }

@@ struct drm_device *dev; int crtc; identifier member; expression num_crtcs; @@ for (crtc = 0; crtc < num_crtcs; crtc++) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc];

<+... (

• dev->vblank[crtc].member

• vblank->member

|

• &(dev->vblank[crtc])

• vblank

) ...+> }

@@ identifier func,member; @@ func (struct drm_device *dev, int crtc, ...) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc];

<+... (

• dev->vblank[crtc].member

• vblank->member

|

• &(dev->vblank[crtc])

• vblank

) ...+> }

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

Reviewing coccinelle scripts is a bit easier than reviewing the actual patch ;-)

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

Btw one cleanup on top I've onticed is that drm_update_vblank_count and vblank_disable_and_save duplicate the same do {} while (vblank_not_stable) loop. But not quite since one has a timeout. I think one more patch on top to extract that would be pretty. -Daniel

---

drivers/gpu/drm/drm_irq.c | 136 +++++++++++++++++++++++++++------------------- 1 file changed, 80 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 54a56b2..20a4855 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -73,6 +73,7 @@ */ static void drm_update_vblank_count(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; u32 cur_vblank, diff, tslot, rc; struct timeval t_vblank;

@@ -94,12 +95,12 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc) } while (cur_vblank != dev->driver->get_vblank_counter(dev, crtc));

/* Deal with counter wrap */

• diff = cur_vblank - dev->vblank[crtc].last;
• if (cur_vblank < dev->vblank[crtc].last) {

• diff = cur_vblank - vblank->last;

• if (cur_vblank < vblank->last) { diff += dev->max_vblank_count;

  DRM_DEBUG("last_vblank[%d]=0x%x, cur_vblank=0x%x => diff=0x%x\n",

• 	  crtc, dev->vblank[crtc].last, cur_vblank, diff);
  

• 	  crtc, vblank->last, cur_vblank, diff);
  

  }

  DRM_DEBUG("enabling vblank interrupts on crtc %d, missed %d\n",

@@ -110,12 +111,12 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc) * reinitialize delayed at next vblank interrupt in that case. */ if (rc) {

• tslot = atomic_read(&dev->vblank[crtc].count) + diff;
  

• tslot = atomic_read(&vblank->count) + diff;
  

  vblanktimestamp(dev, crtc, tslot) = t_vblank; }

  smp_mb__before_atomic_inc();

• atomic_add(diff, &dev->vblank[crtc].count);

• atomic_add(diff, &vblank->count); smp_mb__after_atomic_inc();

}

@@ -127,6 +128,7 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc) */ static void vblank_disable_and_save(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; unsigned long irqflags; u32 vblcount; s64 diff_ns;

@@ -147,14 +149,14 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc) * count account for the entire time between drm_vblank_on() and * drm_vblank_off(). */

• if (!dev->vblank[crtc].enabled) {

• if (!vblank->enabled) { drm_update_vblank_count(dev, crtc); spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags); return; }

  dev->driver->disable_vblank(dev, crtc);

• dev->vblank[crtc].enabled = false;

• vblank->enabled = false;

  /* No further vblank irq's will be processed after

  • this point. Get current hardware vblank count and

@@ -169,9 +171,9 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc) * delayed gpu counter increment. */ do {

• dev->vblank[crtc].last = dev->driver->get_vblank_counter(dev, crtc);
  

• vblank->last = dev->driver->get_vblank_counter(dev, crtc);
  

  vblrc = drm_get_last_vbltimestamp(dev, crtc, &tvblank, 0);

• } while (dev->vblank[crtc].last != dev->driver->get_vblank_counter(dev, crtc) && (--count) && vblrc);

• } while (vblank->last != dev->driver->get_vblank_counter(dev, crtc) && (--count) && vblrc);

  if (!count) vblrc = 0;

@@ -179,7 +181,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc) /* Compute time difference to stored timestamp of last vblank * as updated by last invocation of drm_handle_vblank() in vblank irq. */

• vblcount = atomic_read(&dev->vblank[crtc].count);

• vblcount = atomic_read(&vblank->count); diff_ns = timeval_to_ns(&tvblank) - timeval_to_ns(&vblanktimestamp(dev, crtc, vblcount));

@@ -196,7 +198,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc) * hope for the best. */ if ((vblrc > 0) && (abs64(diff_ns) > 1000000)) {

• atomic_inc(&dev->vblank[crtc].count);
  

• atomic_inc(&vblank->count);
  

  smp_mb__after_atomic_inc(); }

@@ -236,8 +238,10 @@ void drm_vblank_cleanup(struct drm_device *dev) return;

for (crtc = 0; crtc < dev->num_crtcs; crtc++) {

• del_timer_sync(&dev->vblank[crtc].disable_timer);
  

• vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
  

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
  

• del_timer_sync(&vblank->disable_timer);
  

• vblank_disable_fn((unsigned long)vblank);
  

  }

  kfree(dev->vblank);

@@ -270,11 +274,13 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs) goto err;

for (i = 0; i < num_crtcs; i++) {

• dev->vblank[i].dev = dev;
  

• dev->vblank[i].crtc = i;
  

• init_waitqueue_head(&dev->vblank[i].queue);
  

• setup_timer(&dev->vblank[i].disable_timer, vblank_disable_fn,
  

• 	    (unsigned long)&dev->vblank[i]);
  

• struct drm_vblank_crtc *vblank = &dev->vblank[i];
  

• vblank->dev = dev;
  

• vblank->crtc = i;
  

• init_waitqueue_head(&vblank->queue);
  

• setup_timer(&vblank->disable_timer, vblank_disable_fn,
  

• 	    (unsigned long)vblank);
  

  }

  DRM_INFO("Supports vblank timestamp caching Rev 2 (21.10.2013).\n");

@@ -426,9 +432,11 @@ int drm_irq_uninstall(struct drm_device *dev) if (dev->num_crtcs) { spin_lock_irqsave(&dev->vbl_lock, irqflags); for (i = 0; i < dev->num_crtcs; i++) {

• 	wake_up(&dev->vblank[i].queue);
  

• 	dev->vblank[i].enabled = false;
  

• 	dev->vblank[i].last =
  

• 	struct drm_vblank_crtc *vblank = &dev->vblank[i];
  

• 	wake_up(&vblank->queue);
  

• 	vblank->enabled = false;
  

• 	vblank->last =
  	dev->driver->get_vblank_counter(dev, i);
  

  } spin_unlock_irqrestore(&dev->vbl_lock, irqflags);

@@ -796,7 +804,9 @@ EXPORT_SYMBOL(drm_get_last_vbltimestamp); */ u32 drm_vblank_count(struct drm_device *dev, int crtc) {

• return atomic_read(&dev->vblank[crtc].count);

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
• return atomic_read(&vblank->count);

} EXPORT_SYMBOL(drm_vblank_count);

@@ -816,6 +826,7 @@ EXPORT_SYMBOL(drm_vblank_count); u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc, struct timeval *vblanktime) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; u32 cur_vblank;

  /* Read timestamp from slot of _vblank_time ringbuffer

@@ -824,10 +835,10 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc, * a seqlock. */ do {

• cur_vblank = atomic_read(&dev->vblank[crtc].count);
  

• cur_vblank = atomic_read(&vblank->count);
  

  *vblanktime = vblanktimestamp(dev, crtc, cur_vblank); smp_rmb();

• } while (cur_vblank != atomic_read(&dev->vblank[crtc].count));

• } while (cur_vblank != atomic_read(&vblank->count));

  return cur_vblank;

} @@ -882,13 +893,14 @@ EXPORT_SYMBOL(drm_send_vblank_event); */ static int drm_vblank_enable(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; int ret = 0;

  assert_spin_locked(&dev->vbl_lock);

  spin_lock(&dev->vblank_time_lock);

• if (!dev->vblank[crtc].enabled) {

• if (!vblank->enabled) { /*
  • Enable vblank irqs under vblank_time_lock protection.
  • All vblank count & timestamp updates are held off

@@ -899,9 +911,9 @@ static int drm_vblank_enable(struct drm_device *dev, int crtc) ret = dev->driver->enable_vblank(dev, crtc); DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n", crtc, ret); if (ret)

• 	atomic_dec(&dev->vblank[crtc].refcount);
  

• 	atomic_dec(&vblank->refcount);
  

  else {

• 	dev->vblank[crtc].enabled = true;
  

• 	vblank->enabled = true;
  drm_update_vblank_count(dev, crtc);
  

  } }

@@ -926,16 +938,17 @@ static int drm_vblank_enable(struct drm_device *dev, int crtc) */ int drm_vblank_get(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; unsigned long irqflags; 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[crtc].refcount) == 1) {

• if (atomic_add_return(1, &vblank->refcount) == 1) { ret = drm_vblank_enable(dev, crtc); } else {

• if (!dev->vblank[crtc].enabled) {
  

• 	atomic_dec(&dev->vblank[crtc].refcount);
  

• if (!vblank->enabled) {
  

• 	atomic_dec(&vblank->refcount);
  ret = -EINVAL;
  

  } }

@@ -975,15 +988,17 @@ EXPORT_SYMBOL(drm_crtc_vblank_get); */ void drm_vblank_put(struct drm_device *dev, int crtc) {

• BUG_ON(atomic_read(&dev->vblank[crtc].refcount) == 0);

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc];

• BUG_ON(atomic_read(&vblank->refcount) == 0);

  /* Last user schedules interrupt disable */

• if (atomic_dec_and_test(&dev->vblank[crtc].refcount)) {

• if (atomic_dec_and_test(&vblank->refcount)) { if (drm_vblank_offdelay > 0)

• 	mod_timer(&dev->vblank[crtc].disable_timer,
  

• 	mod_timer(&vblank->disable_timer,
  	  jiffies + ((drm_vblank_offdelay * HZ)/1000));
  

  else if (drm_vblank_offdelay == 0)

• 	vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
  

• 	vblank_disable_fn((unsigned long)vblank);
  

  }

} EXPORT_SYMBOL(drm_vblank_put); @@ -1019,6 +1034,7 @@ EXPORT_SYMBOL(drm_crtc_vblank_put); */ void drm_vblank_off(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; struct drm_pending_vblank_event *e, *t; struct timeval now; unsigned long irqflags;

@@ -1026,7 +1042,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc)

spin_lock_irqsave(&dev->vbl_lock, irqflags); vblank_disable_and_save(dev, crtc);

• wake_up(&dev->vblank[crtc].queue);

• wake_up(&vblank->queue);

  /* Send any queued vblank events, lest the natives grow disquiet */ seq = drm_vblank_count_and_time(dev, crtc, &now);

@@ -1048,9 +1064,9 @@ void drm_vblank_off(struct drm_device *dev, int crtc) * Prevent subsequent drm_vblank_get() from re-enabling * the vblank interrupt by bumping the refcount. */

• if (!dev->vblank[crtc].inmodeset) {

• atomic_inc(&dev->vblank[crtc].refcount);
  

• dev->vblank[crtc].inmodeset = 1;
  

• if (!vblank->inmodeset) {

• atomic_inc(&vblank->refcount);
  

• vblank->inmodeset = 1;
  

  }

  spin_unlock_irqrestore(&dev->vbl_lock, irqflags);

@@ -1090,13 +1106,14 @@ EXPORT_SYMBOL(drm_crtc_vblank_off); */ void drm_vblank_on(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; unsigned long irqflags;

  spin_lock_irqsave(&dev->vbl_lock, irqflags); /* Drop our private "prevent drm_vblank_get" refcount */

• if (dev->vblank[crtc].inmodeset) {

• atomic_dec(&dev->vblank[crtc].refcount);
  

• dev->vblank[crtc].inmodeset = 0;
  

• if (vblank->inmodeset) {

• atomic_dec(&vblank->refcount);
  

• vblank->inmodeset = 0;
  

  }

  /*

@@ -1106,12 +1123,12 @@ void drm_vblank_on(struct drm_device *dev, int crtc) * -1 to make sure user will never see the same * vblank counter value before and after a modeset */

• dev->vblank[crtc].last =

• vblank->last = (dev->driver->get_vblank_counter(dev, crtc) - 1) & dev->max_vblank_count;

  /* re-enable interrupts if there's are users left */

• if (atomic_read(&dev->vblank[crtc].refcount) != 0)

• if (atomic_read(&vblank->refcount) != 0) WARN_ON(drm_vblank_enable(dev, crtc)); spin_unlock_irqrestore(&dev->vbl_lock, irqflags);

} @@ -1159,6 +1176,8 @@ EXPORT_SYMBOL(drm_crtc_vblank_on); */ void drm_vblank_pre_modeset(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
• /* vblank is not initialized (IRQ not installed ?), or has been freed */ if (!dev->num_crtcs) return;

@@ -1169,10 +1188,10 @@ void drm_vblank_pre_modeset(struct drm_device *dev, int crtc) * to avoid corrupting the count if multiple, mismatch calls occur), * so that interrupts remain enabled in the interim. */

• if (!dev->vblank[crtc].inmodeset) {

• dev->vblank[crtc].inmodeset = 0x1;
  

• if (!vblank->inmodeset) {

• vblank->inmodeset = 0x1;
  

  if (drm_vblank_get(dev, crtc) == 0)

• 	dev->vblank[crtc].inmodeset |= 0x2;
  

• 	vblank->inmodeset |= 0x2;
  

  }

} EXPORT_SYMBOL(drm_vblank_pre_modeset); @@ -1187,21 +1206,22 @@ EXPORT_SYMBOL(drm_vblank_pre_modeset); */ void drm_vblank_post_modeset(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; unsigned long irqflags;

  /* vblank is not initialized (IRQ not installed ?), or has been freed */ if (!dev->num_crtcs) return;

• if (dev->vblank[crtc].inmodeset) {

• if (vblank->inmodeset) { spin_lock_irqsave(&dev->vbl_lock, irqflags); dev->vblank_disable_allowed = true; spin_unlock_irqrestore(&dev->vbl_lock, irqflags);

• if (dev->vblank[crtc].inmodeset & 0x2)
  

• if (vblank->inmodeset & 0x2)
  drm_vblank_put(dev, crtc);
  
  

• dev->vblank[crtc].inmodeset = 0;
  

• vblank->inmodeset = 0;
  

  }

} EXPORT_SYMBOL(drm_vblank_post_modeset); @@ -1336,6 +1356,7 @@ err_put: int drm_wait_vblank(struct drm_device *dev, void *data, struct drm_file *file_priv) {

• struct drm_vblank_crtc *vblank; union drm_wait_vblank *vblwait = data; int ret; unsigned int flags, seq, crtc, high_crtc;

@@ -1365,6 +1386,8 @@ int drm_wait_vblank(struct drm_device *dev, void *data, if (crtc >= dev->num_crtcs) return -EINVAL;

• vblank = &dev->vblank[crtc];
• ret = drm_vblank_get(dev, crtc); if (ret) { DRM_DEBUG("failed to acquire vblank counter, %d\n", ret);

@@ -1397,11 +1420,11 @@ int drm_wait_vblank(struct drm_device *dev, void *data,

DRM_DEBUG("waiting on vblank count %d, crtc %d\n", vblwait->request.sequence, crtc);

• dev->vblank[crtc].last_wait = vblwait->request.sequence;
• DRM_WAIT_ON(ret, dev->vblank[crtc].queue, 3 * HZ,

• vblank->last_wait = vblwait->request.sequence;
• DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, (((drm_vblank_count(dev, crtc) - vblwait->request.sequence) <= (1 << 23)) ||

•      !dev->vblank[crtc].enabled ||
  

•      !vblank->enabled ||
  !dev->irq_enabled));
  
  

  if (ret != -EINTR) {

@@ -1462,6 +1485,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc) */ bool drm_handle_vblank(struct drm_device *dev, int crtc) {

• struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; u32 vblcount; s64 diff_ns; struct timeval tvblank;

@@ -1477,7 +1501,7 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc) spin_lock_irqsave(&dev->vblank_time_lock, irqflags);

/* Vblank irq handling disabled. Nothing to do. */

• if (!dev->vblank[crtc].enabled) {

• if (!vblank->enabled) { spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags); return false; }

@@ -1487,7 +1511,7 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc) */

/* Get current timestamp and count. */

• vblcount = atomic_read(&dev->vblank[crtc].count);

• vblcount = atomic_read(&vblank->count); drm_get_last_vbltimestamp(dev, crtc, &tvblank, DRM_CALLED_FROM_VBLIRQ);

  /* Compute time difference to timestamp of last vblank */

@@ -1511,14 +1535,14 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc) * the timestamp computed above. */ smp_mb__before_atomic_inc();

• atomic_inc(&dev->vblank[crtc].count);
  

• atomic_inc(&vblank->count);
  

  smp_mb__after_atomic_inc(); } else { DRM_DEBUG("crtc %d: Redundant vblirq ignored. diff_ns = %d\n", crtc, (int) diff_ns); }

• wake_up(&dev->vblank[crtc].queue);

• wake_up(&vblank->queue); drm_handle_vblank_events(dev, crtc);

  spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);

-- 1.8.5.5

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx