Re: [PATCH 2/5] drm: Make the vblank disable timer per-crtc

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Fri, 21 Feb 2014 21:03:32 +0200
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently there's one per-device vblank disable timer, and it gets
> reset wheneven the vblank refcount for any crtc drops to zero. That
> means that one crtc could accidentally be keeping the vblank interrupts
> for other crtcs enabled even if there are no users for them. Make the
> disable timer per-crtc to avoid this issue.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_irq.c | 40 +++++++++++++++++++++-------------------
>  include/drm/drmP.h        |  4 +++-
>  2 files changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index baa12e7..3211158 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -167,33 +167,34 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
>  
>  static void vblank_disable_fn(unsigned long arg)
>  {
> -	struct drm_device *dev = (struct drm_device *)arg;
> +	struct drm_vblank_crtc *vblank = (void *)arg;
> +	struct drm_device *dev = vblank->dev;
>  	unsigned long irqflags;
> -	int i;
> +	int crtc = vblank->crtc;
>  
>  	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[i].refcount) == 0 &&
> -		    dev->vblank[i].enabled) {
> -			DRM_DEBUG("disabling vblank on crtc %d\n", i);
> -			vblank_disable_and_save(dev, i);
> -		}
> -		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> +	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> +	if (atomic_read(&vblank->refcount) == 0 && vblank->enabled) {
> +		DRM_DEBUG("disabling vblank on crtc %d\n", crtc);
> +		vblank_disable_and_save(dev, crtc);
>  	}
> +	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>  }
>  
>  void drm_vblank_cleanup(struct drm_device *dev)
>  {
> +	int crtc;
> +
>  	/* Bail if the driver didn't call drm_vblank_init() */
>  	if (dev->num_crtcs == 0)
>  		return;
>  
> -	del_timer_sync(&dev->vblank_disable_timer);
> -
> -	vblank_disable_fn((unsigned long)dev);
> +	for (crtc = 0; crtc < dev->num_crtcs; crtc++) {
> +		del_timer_sync(&dev->vblank[crtc].disable_timer);
> +		vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
> +	}
>  
>  	kfree(dev->vblank);
>  
> @@ -205,8 +206,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);
>  	spin_lock_init(&dev->vblank_time_lock);
>  
> @@ -216,8 +215,13 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs)
>  	if (!dev->vblank)
>  		goto err;
>  
> -	for (i = 0; i < num_crtcs; i++)
> +	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]);
> +	}
>  
>  	DRM_INFO("Supports vblank timestamp caching Rev 2 (21.10.2013).\n");
>  
> @@ -934,7 +938,7 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
>  	/* Last user schedules interrupt disable */
>  	if (atomic_dec_and_test(&dev->vblank[crtc].refcount) &&
>  	    (drm_vblank_offdelay > 0))
> -		mod_timer(&dev->vblank_disable_timer,
> +		mod_timer(&dev->vblank[crtc].disable_timer,
>  			  jiffies + ((drm_vblank_offdelay * HZ)/1000));
>  }
>  EXPORT_SYMBOL(drm_vblank_put);
> @@ -943,8 +947,6 @@ EXPORT_SYMBOL(drm_vblank_put);
>   * drm_vblank_off - disable vblank events on a CRTC
>   * @dev: DRM device
>   * @crtc: CRTC in question
> - *
> - * Caller must hold event lock.
>   */
>  void drm_vblank_off(struct drm_device *dev, int crtc)
>  {
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 04a7f31..f974da9 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1077,14 +1077,17 @@ struct drm_pending_vblank_event {
>  };
>  
>  struct drm_vblank_crtc {
> +	struct drm_device *dev;		/* pointer to the drm_device */
>  	wait_queue_head_t queue;	/**< VBLANK wait queue */
>  	struct timeval time[DRM_VBLANKTIME_RBSIZE];	/**< timestamp of current count */
> +	struct timer_list disable_timer;		/* delayed disable timer */
>  	atomic_t count;			/**< number of VBLANK interrupts */
>  	atomic_t refcount;		/* number of users of vblank interruptsper crtc */
>  	u32 last;			/* protected by dev->vbl_lock, used */
>  					/* for wraparound handling */
>  	u32 last_wait;			/* Last vblank seqno waited per CRTC */
>  	unsigned int inmodeset;		/* Display driver is setting mode */
> +	int crtc;			/* crtc index */
>  	bool enabled;			/* so we don't call enable more than
>  					   once per disable */
>  };
> @@ -1157,7 +1160,6 @@ struct drm_device {
>  
>  	spinlock_t vblank_time_lock;    /**< Protects vblank count and time updates during vblank enable/disable */
>  	spinlock_t vbl_lock;
> -	struct timer_list vblank_disable_timer;
>  
>  	u32 max_vblank_count;           /**< size of vblank counter register */
>  

Yeah this looks like a good fix.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx