Drivers without a hardware vblank counter simply can't account for the vblanks that happened while the vblank interrupt was off. To check this grab a vblank timestamp and if the result is dubious follow the normal save-and-disable logic.

Drivers should prevent this by setting vblank_disable_allowed = false, but since running vblank interrupts constantly is not good for power consumption most drivers lie. Testing for precise vblank timestamps is the next best thing we can check for.

Suggested-by: Mario Kleiner mario.kleiner.de@gmail.com Cc: Mario Kleiner mario.kleiner.de@gmail.com Cc: Matt Roper matthew.d.roper@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_irq.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 6eb015020af2..922721ead29a 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -163,8 +163,15 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc) * has been ticking all along until this time. This makes the * count account for the entire time between drm_vblank_on() and * drm_vblank_off(). + * + * But only do this if precise vblank timestamps are available. + * Otherwise we might read a totally bogus timestamp since drivers + * lacking precise timestamp support rely upon sampling the system clock + * at vblank interrupt time. Which obviously won't work out well if the + * vblank interrupt is disabled. */ - if (!vblank->enabled) { + if (!vblank->enabled && + drm_get_last_vbltimestamp(dev, crtc, &tvblank, 0) > 0) { drm_update_vblank_count(dev, crtc); spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags); return;

Imo u32 hints at a register value, but in reality all callers only care whether the sampled timestamp is precise or not. So give them just a bool.

Also move the declaration out of drmP.h, it's only used in drm_irq.c.

Cc: Mario Kleiner mario.kleiner.de@gmail.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_irq.c | 24 +++++++++++++++--------- include/drm/drmP.h | 2 -- 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 922721ead29a..b16f0bcef959 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -70,6 +70,10 @@ module_param_named(vblankoffdelay, drm_vblank_offdelay, int, 0600); module_param_named(timestamp_precision_usec, drm_timestamp_precision, int, 0600); module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);

+static bool +drm_get_last_vbltimestamp(struct drm_device *dev, int crtc, + struct timeval *tvblank, unsigned flags); + /** * drm_update_vblank_count - update the master vblank counter * @dev: DRM device @@ -89,7 +93,8 @@ module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600); 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; + u32 cur_vblank, diff, tslot; + bool rc; struct timeval t_vblank;

/* @@ -147,7 +152,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc) unsigned long irqflags; u32 vblcount; s64 diff_ns; - int vblrc; + bool vblrc; struct timeval tvblank; int count = DRM_TIMESTAMP_MAXRETRIES;

@@ -171,7 +176,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc) * vblank interrupt is disabled. */ if (!vblank->enabled && - drm_get_last_vbltimestamp(dev, crtc, &tvblank, 0) > 0) { + drm_get_last_vbltimestamp(dev, crtc, &tvblank, 0)) { drm_update_vblank_count(dev, crtc); spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags); return; @@ -219,7 +224,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc) * available. In that case we can't account for this and just * hope for the best. */ - if ((vblrc > 0) && (abs64(diff_ns) > 1000000)) { + if (vblrc && (abs64(diff_ns) > 1000000)) { /* Store new timestamp in ringbuffer. */ vblanktimestamp(dev, crtc, vblcount + 1) = tvblank;

@@ -786,10 +791,11 @@ static struct timeval get_drm_timestamp(void) * call, i.e., it isn't very precisely locked to the true vblank. * * Returns: - * Non-zero if timestamp is considered to be very precise, zero otherwise. + * True if timestamp is considered to be very precise, false otherwise. */ -u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc, - struct timeval *tvblank, unsigned flags) +static bool +drm_get_last_vbltimestamp(struct drm_device *dev, int crtc, + struct timeval *tvblank, unsigned flags) { int ret;

@@ -801,7 +807,7 @@ u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc, ret = dev->driver->get_vblank_timestamp(dev, crtc, &max_error, tvblank, flags); if (ret > 0) - return (u32) ret; + return true; }

/* GPU high precision timestamp query unsupported or failed. @@ -809,7 +815,7 @@ u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc, */ *tvblank = get_drm_timestamp();

- return 0; + return false; } EXPORT_SYMBOL(drm_get_last_vbltimestamp);

diff --git a/include/drm/drmP.h b/include/drm/drmP.h index ad952b08711e..2ccb0e715569 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1004,8 +1004,6 @@ extern void drm_crtc_vblank_off(struct drm_crtc *crtc); extern void drm_crtc_vblank_on(struct drm_crtc *crtc); extern void drm_vblank_cleanup(struct drm_device *dev);

-extern u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc, - struct timeval *tvblank, unsigned flags); extern int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc, int *max_error, struct timeval *vblank_time,

On Wed, Sep 10, 2014 at 05:36:08PM +0200, Daniel Vetter wrote:

With the new support for immediate vblank disabling we always disabled the vblank interrupt right away, irrespective of the vblank offdelay setting.

But being able to let vblanks run forever is fairly useful for debugging, so restore that behaviour.

Suggested-by: Mario Kleiner mario.kleiner.de@gmail.com Cc: Mario Kleiner mario.kleiner.de@gmail.com Cc: Matt Roper matthew.d.roper@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Reviewed-by: Matt Roper matthew.d.roper@intel.com

---

drivers/gpu/drm/drm_irq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a75da075927c..6eb015020af2 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1024,9 +1024,11 @@ void drm_vblank_put(struct drm_device *dev, int crtc)

/* Last user schedules interrupt disable */ if (atomic_dec_and_test(&vblank->refcount)) {

• if (dev->vblank_disable_immediate || drm_vblank_offdelay < 0)
  

• if (drm_vblank_offdelay == 0)
  

• 	return;
  

• else if (dev->vblank_disable_immediate || drm_vblank_offdelay < 0)
  vblank_disable_fn((unsigned long)vblank);
  

• else if (drm_vblank_offdelay > 0)
  

• else
  mod_timer(&vblank->disable_timer,
  	  jiffies + ((drm_vblank_offdelay * HZ)/1000));
  

  }

-- 1.9.3