On Fri, 21 Feb 2014 21:03:32 +0200
ville.syrjala at linux.intel.com wrote:
> From: Ville Syrj?l? <ville.syrjala at 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 at 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 at virtuousgeek.org>
--
Jesse Barnes, Intel Open Source Technology Center
