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

Daniel Vetter daniel at ffwll.ch
Mon May 26 15:31:38 CEST 2014


On Mon, May 26, 2014 at 02:46:31PM +0300, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at 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 at linux.intel.com>

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

Reviewed-by: Daniel Vetter <daniel.vetter at 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 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