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

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