[PATCH 02/12] drm: Make the vblank disable timer per-crtc

[Daniel Vetter]

On Wed, May 21, 2014 at 01:17:49PM +0200, Thierry Reding wrote:
> On Wed, May 14, 2014 at 08:51:04PM +0200, Daniel Vetter wrote:
> > From: Ville Syrj?l? 
> > 
> > Currently there's one per-device vblank disable timer, and it gets
> > reset wheneven the vblank refcount for any crtc drops to zero. That
> 
> "whenever"
> 
> > 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.
> 
> Very pedantically: s/crtc/CRTC/ and maybe even s/vblank/VBLANK/. Feel
> free to ignore those, though. =)
> 
> Also, and I may have asked before, why do we even need this timer? Why
> not simply disable interrupts when the last vblank reference goes away?

Without intricate knowledge of where exactly the vblank interrupt fires
wrt the hw frame counter the enabling/disabling of the vblank machinery as
implemented in drm_irq.c is racy. Which means we shouldn't do it all the
time.

In i915 we are now solid enough with vblank handling in general and also
well-covered in tests that we'll attempt to kill the disabling timer as
the next step. Since keeping vblanks going when we don't need them if you
have a hw vblank counter seriously hampers deep sleep states residency.

But given how bug-riddled our vblank code was I want to move slowly. And
we need to keep the hack for all those drivers which haven't properly been
audited and tested (i.e. everyone else).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 02/12] drm: Make the vblank disable timer per-crtc

[Thierry Reding]

On Wed, May 14, 2014 at 08:51:04PM +0200, Daniel Vetter wrote:
> From: Ville Syrj?l? 
> 
> Currently there's one per-device vblank disable timer, and it gets
> reset wheneven the vblank refcount for any crtc drops to zero. That

"whenever"

> 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.

Very pedantically: s/crtc/CRTC/ and maybe even s/vblank/VBLANK/. Feel
free to ignore those, though. =)

Also, and I may have asked before, why do we even need this timer? Why
not simply disable interrupts when the last vblank reference goes away?

Generally, though:

Reviewed-by: Thierry Reding 
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

[PATCH 02/12] drm: Make the vblank disable timer per-crtc

[Daniel Vetter]

From: Ville Syrj?l? 

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? 
Signed-off-by: Daniel Vetter 
---
 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 ea20c4aa1b6b..90c59a8c820f 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -140,33 +140,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(>vbl_lock, irqflags);
-   if (atomic_read(>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(>vbl_lock, irqflags);
+   spin_lock_irqsave(>vbl_lock, irqflags);
+   if (atomic_read(>refcount) == 0 && vblank->enabled) {
+   DRM_DEBUG("disabling vblank on crtc %d\n", crtc);
+   vblank_disable_and_save(dev, crtc);
}
+   spin_unlock_irqrestore(>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(>vblank_disable_timer);
-
-   vblank_disable_fn((unsigned long)dev);
+   for (crtc = 0; crtc < dev->num_crtcs; crtc++) {
+   del_timer_sync(>vblank[crtc].disable_timer);
+   vblank_disable_fn((unsigned long)>vblank[crtc]);
+   }

kfree(dev->vblank);

@@ -178,8 +179,6 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs)
 {
int i, ret = -ENOMEM;

-   setup_timer(>vblank_disable_timer, vblank_disable_fn,
-   (unsigned long)dev);
spin_lock_init(>vbl_lock);
spin_lock_init(>vblank_time_lock);

@@ -189,8 +188,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(>vblank[i].queue);
+   setup_timer(>vblank[i].disable_timer, vblank_disable_fn,
+   (unsigned long)>vblank[i]);
+   }

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

@@ -900,7 +904,7 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
/* Last user schedules interrupt disable */
if (atomic_dec_and_test(>vblank[crtc].refcount) &&
(drm_vblank_offdelay > 0))
-   mod_timer(>vblank_disable_timer,
+   mod_timer(>vblank[crtc].disable_timer,
  jiffies + ((drm_vblank_offdelay * HZ)/1000));
 }
 EXPORT_SYMBOL(drm_vblank_put);
@@ -909,8 +913,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 12f10bc2395f..9d982d483f12 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1024,14 +1024,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