drm_vblank_count() has a u32 type returning what is a 64-bit vblank count. The effect of this is when drm_wait_vblank_ioctl() tries to widen the user space requested vblank sequence using this clipped 32-bit count(when the value is >= 2^32) as reference, the requested sequence remains a 32-bit value and gets queued like that. However, the code that checks if the requested sequence has passed compares this against the 64-bit vblank count.
Cc: Keith Packard keithp@keithp.com Cc: Michel Dänzer michel@daenzer.net Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com --- drivers/gpu/drm/drm_vblank.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 32d9bcf5be7f..768a8e44d99b 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, store_vblank(dev, pipe, diff, t_vblank, cur_vblank); }
-static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe) +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
Now that drm_vblank_count() returns all bits of the vblank count, update drm_crtc_arm_vblank_event() so that it queues the correct sequence. Otherwise, this leads to prolonged waits for a vblank sequence when the current count is >=2^32.
Cc: Keith Packard keithp@keithp.com Cc: Michel Dänzer michel@daenzer.net Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com --- drivers/gpu/drm/drm_vblank.c | 4 ++-- include/drm/drm_vblank.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 768a8e44d99b..f2bf1f5dbaa5 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -292,11 +292,11 @@ static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) * This is mostly useful for hardware that can obtain the scanout position, but * doesn't have a hardware frame counter. */ -u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc) +u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; unsigned int pipe = drm_crtc_index(crtc); - u32 vblank; + u64 vblank; unsigned long flags;
WARN_ONCE(drm_debug & DRM_UT_VBL && !dev->driver->get_vblank_timestamp, diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h index 848b463a0af5..a4c3b0a0a197 100644 --- a/include/drm/drm_vblank.h +++ b/include/drm/drm_vblank.h @@ -179,7 +179,7 @@ void drm_crtc_wait_one_vblank(struct drm_crtc *crtc); void drm_crtc_vblank_off(struct drm_crtc *crtc); void drm_crtc_vblank_reset(struct drm_crtc *crtc); void drm_crtc_vblank_on(struct drm_crtc *crtc); -u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc); +u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, unsigned int pipe, int *max_error,
On Fri, Jan 12, 2018 at 09:57:04PM +0000, Dhinakaran Pandiyan wrote:
Now that drm_vblank_count() returns all bits of the vblank count, update drm_crtc_arm_vblank_event() so that it queues the correct sequence. Otherwise, this leads to prolonged waits for a vblank sequence when the current count is >=2^32.
This could be probably squashed to the previous patch... Specially if you apply the Fixes tag.
Anyways, in case you decide to go with 2 patches:
Reviewed-by: Rodrigo Vivi rodrigo.vivi@intel.com
Cc: Keith Packard keithp@keithp.com Cc: Michel Dänzer michel@daenzer.net Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_vblank.c | 4 ++-- include/drm/drm_vblank.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 768a8e44d99b..f2bf1f5dbaa5 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -292,11 +292,11 @@ static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
- This is mostly useful for hardware that can obtain the scanout position, but
- doesn't have a hardware frame counter.
*/ -u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc) +u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; unsigned int pipe = drm_crtc_index(crtc);
- u32 vblank;
u64 vblank; unsigned long flags;
WARN_ONCE(drm_debug & DRM_UT_VBL && !dev->driver->get_vblank_timestamp,
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h index 848b463a0af5..a4c3b0a0a197 100644 --- a/include/drm/drm_vblank.h +++ b/include/drm/drm_vblank.h @@ -179,7 +179,7 @@ void drm_crtc_wait_one_vblank(struct drm_crtc *crtc); void drm_crtc_vblank_off(struct drm_crtc *crtc); void drm_crtc_vblank_reset(struct drm_crtc *crtc); void drm_crtc_vblank_on(struct drm_crtc *crtc); -u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc); +u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, unsigned int pipe, int *max_error, -- 2.11.0
Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com writes:
Now that drm_vblank_count() returns all bits of the vblank count, update drm_crtc_arm_vblank_event() so that it queues the correct sequence. Otherwise, this leads to prolonged waits for a vblank sequence when the current count is >=2^32.
The summary for this patch is incorrect; the function being fixed is drm_crtc_accurate_vblank_event.
And, I'm afraid you've uncovered a rabbit hole here -- there are a couple of users of this function outside of the core, including i915 in a couple of places and nouveau. We should at least review the whole call graph starting here and make sure it does what we think it should.
Thanks for finding these bugs!
Updating vblank counts requires register reads and these reads may not return meaningful values if the device was in a low power state after vblank interrupts were last disabled. So, update the count only if vblank interrupts are enabled. Secondly, this means the registers should be read before disabling vblank interrupts.
v2: Don't check vblank->enabled outside it's lock (Chris)
Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Michel Dänzer michel@daenzer.net Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com --- drivers/gpu/drm/drm_vblank.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index f2bf1f5dbaa5..2559d2d7b907 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -347,23 +347,25 @@ void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe) spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
/* - * Only disable vblank interrupts if they're enabled. This avoids - * calling the ->disable_vblank() operation in atomic context with the - * hardware potentially runtime suspended. + * Update vblank count and disable vblank interrupts only if the + * interrupts were enabled. This avoids calling the ->disable_vblank() + * operation in atomic context with the hardware potentially runtime + * suspended. */ - if (vblank->enabled) { - __disable_vblank(dev, pipe); - vblank->enabled = false; - } + if (!vblank->enabled) + goto out;
/* - * Always update the count and timestamp to maintain the + * Update the count and timestamp to maintain the * appearance that the counter has been ticking all along until * this time. This makes the count account for the entire time * between drm_crtc_vblank_on() and drm_crtc_vblank_off(). */ drm_update_vblank_count(dev, pipe, false); + __disable_vblank(dev, pipe); + vblank->enabled = false;
+out: spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags); }
On Fri, Jan 12, 2018 at 09:57:05PM +0000, Dhinakaran Pandiyan wrote:
Updating vblank counts requires register reads and these reads may not return meaningful values if the device was in a low power state after vblank interrupts were last disabled. So, update the count only if vblank interrupts are enabled. Secondly, this means the registers should be read before disabling vblank interrupts.
v2: Don't check vblank->enabled outside it's lock (Chris)
Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Michel Dänzer michel@daenzer.net Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_vblank.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index f2bf1f5dbaa5..2559d2d7b907 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -347,23 +347,25 @@ void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe) spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
/*
* Only disable vblank interrupts if they're enabled. This avoids* calling the ->disable_vblank() operation in atomic context with the* hardware potentially runtime suspended.
* Update vblank count and disable vblank interrupts only if the* interrupts were enabled. This avoids calling the ->disable_vblank()* operation in atomic context with the hardware potentially runtime* suspended.
- if (vblank->enabled) {
__disable_vblank(dev, pipe);vblank->enabled = false;- }
if (!vblank->enabled)
goto out;/*
* Always update the count and timestamp to maintain the
* Update the count and timestamp to maintain the*/
- appearance that the counter has been ticking all along until
- this time. This makes the count account for the entire time
- between drm_crtc_vblank_on() and drm_crtc_vblank_off().
I feel that this entire comment can be simply removed now... The approach looks good and right to me so you can use
Reviewed-by: Rodrigo Vivi rodrigo.vivi@intel.com
but please ping Ville to take a look here since he introduced this approach with 4dfd64862ff8 ("drm: Use vblank timestamps to guesstimate how many vblanks were missed")
drm_update_vblank_count(dev, pipe, false);
- __disable_vblank(dev, pipe);
- vblank->enabled = false;
+out: spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags); }
-- 2.11.0
The HW frame counter can get reset if device enters a low power state after vblank interrupts were disabled. This messes up any following vblank count update as a negative diff (huge unsigned diff) is calculated from the HW frame counter change. We cannot ignore negative diffs altogther as there could be legitimate wrap arounds. So, allow drivers to update vblank->count with missed vblanks for the time interrupts were disabled. This is similar to _crtc_vblank_on() except that vblanks interrupts are not enabled at the end as this function is expected to be called from the driver _enable_vblank() vfunc.
v2: drm_crtc_vblank_restore should take crtc as arg. (Chris) Add docs and sprinkle some asserts.
Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Michel Dänzer michel@daenzer.net Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com --- drivers/gpu/drm/drm_vblank.c | 59 ++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_vblank.h | 2 ++ 2 files changed, 61 insertions(+)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 2559d2d7b907..2690966694f0 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1237,6 +1237,65 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc) } EXPORT_SYMBOL(drm_crtc_vblank_on);
+/** + * drm_vblank_restore - estimated vblanks using timestamps and update it. + * + * Power manamement features can cause frame counter resets between vblank + * disable and enable. Drivers can then use this function in their + * &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since + * the last &drm_crtc_funcs.disable_vblank. + * + * This function is the legacy version of drm_crtc_vblank_restore(). + */ +void drm_vblank_restore(struct drm_device *dev, unsigned int pipe) +{ + ktime_t t_vblank; + struct drm_vblank_crtc *vblank; + int framedur_ns; + u64 diff_ns; + u32 cur_vblank, diff = 1; + int count = DRM_TIMESTAMP_MAXRETRIES; + + if (WARN_ON(pipe >= dev->num_crtcs)) + return; + + assert_spin_locked(&dev->vbl_lock); + assert_spin_locked(&dev->vblank_time_lock); + + vblank = &dev->vblank[pipe]; + WARN_ONCE((drm_debug & DRM_UT_VBL) && !vblank->framedur_ns, + "Cannot compute missed vblanks without frame duration\n"); + framedur_ns = vblank->framedur_ns; + + do { + cur_vblank = __get_vblank_counter(dev, pipe); + drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false); + } while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0); + + diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time)); + if (framedur_ns) + diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns); + + + DRM_DEBUG_VBL("missed %d vblanks in %lld ns, frame duration=%d ns, hw_diff=%d\n", + diff, diff_ns, framedur_ns, cur_vblank - vblank->last); + store_vblank(dev, pipe, diff, t_vblank, cur_vblank); +} +EXPORT_SYMBOL(drm_vblank_restore); + +/** + * drm_crtc_vblank_restore - estimate vblanks using timestamps and update it. + * Power manamement features can cause frame counter resets between vblank + * disable and enable. Drivers can then use this function in their + * &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since + * the last &drm_crtc_funcs.disable_vblank. + */ +void drm_crtc_vblank_restore(struct drm_crtc *crtc) +{ + drm_vblank_restore(crtc->dev, drm_crtc_index(crtc)); +} +EXPORT_SYMBOL(drm_crtc_vblank_restore); + static void drm_legacy_vblank_pre_modeset(struct drm_device *dev, unsigned int pipe) { diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h index a4c3b0a0a197..16d46e2a6854 100644 --- a/include/drm/drm_vblank.h +++ b/include/drm/drm_vblank.h @@ -180,6 +180,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc); void drm_crtc_vblank_reset(struct drm_crtc *crtc); void drm_crtc_vblank_on(struct drm_crtc *crtc); u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc); +void drm_vblank_restore(struct drm_device *dev, unsigned int pipe); +void drm_crtc_vblank_restore(struct drm_crtc *crtc);
bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, unsigned int pipe, int *max_error,
On Fri, Jan 12, 2018 at 09:57:06PM +0000, Dhinakaran Pandiyan wrote:
The HW frame counter can get reset if device enters a low power state after vblank interrupts were disabled. This messes up any following vblank count update as a negative diff (huge unsigned diff) is calculated from the HW frame counter change. We cannot ignore negative diffs altogther as there could be legitimate wrap arounds. So, allow drivers to update vblank->count with missed vblanks for the time interrupts were disabled. This is similar to _crtc_vblank_on() except that vblanks interrupts are not enabled at the end as this function is expected to be called from the driver _enable_vblank() vfunc.
v2: drm_crtc_vblank_restore should take crtc as arg. (Chris) Add docs and sprinkle some asserts.
Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Michel Dänzer michel@daenzer.net Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_vblank.c | 59 ++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_vblank.h | 2 ++ 2 files changed, 61 insertions(+)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 2559d2d7b907..2690966694f0 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1237,6 +1237,65 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc) } EXPORT_SYMBOL(drm_crtc_vblank_on);
+/**
- drm_vblank_restore - estimated vblanks using timestamps and update it.
- Power manamement features can cause frame counter resets between vblank
- disable and enable. Drivers can then use this function in their
- &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
- the last &drm_crtc_funcs.disable_vblank.
- This function is the legacy version of drm_crtc_vblank_restore().
- */
+void drm_vblank_restore(struct drm_device *dev, unsigned int pipe) +{
- ktime_t t_vblank;
- struct drm_vblank_crtc *vblank;
- int framedur_ns;
- u64 diff_ns;
- u32 cur_vblank, diff = 1;
- int count = DRM_TIMESTAMP_MAXRETRIES;
- if (WARN_ON(pipe >= dev->num_crtcs))
return;- assert_spin_locked(&dev->vbl_lock);
- assert_spin_locked(&dev->vblank_time_lock);
- vblank = &dev->vblank[pipe];
- WARN_ONCE((drm_debug & DRM_UT_VBL) && !vblank->framedur_ns,
do we really only need this warn on debug vlb?
"Cannot compute missed vblanks without frame duration\n");
The message seems hard... if we *cannot* why do we move fwd?
- framedur_ns = vblank->framedur_ns;
- do {
cur_vblank = __get_vblank_counter(dev, pipe);drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false);- } while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
Based on the commend of drm_update_vblank_count I don't feel that we have to do the loop here... And if we have maybe we should re-org things to avoid duplication?
- diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
- if (framedur_ns)
diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns);- DRM_DEBUG_VBL("missed %d vblanks in %lld ns, frame duration=%d ns, hw_diff=%d\n",
diff, diff_ns, framedur_ns, cur_vblank - vblank->last);- store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
hm... I wonder if the simple store_vblank(... __get_vblank_counter(dev, pipe)); wouldn't work here...
+} +EXPORT_SYMBOL(drm_vblank_restore);
+/**
- drm_crtc_vblank_restore - estimate vblanks using timestamps and update it.
- Power manamement features can cause frame counter resets between vblank
- disable and enable. Drivers can then use this function in their
- &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
- the last &drm_crtc_funcs.disable_vblank.
- */
+void drm_crtc_vblank_restore(struct drm_crtc *crtc) +{
- drm_vblank_restore(crtc->dev, drm_crtc_index(crtc));
+} +EXPORT_SYMBOL(drm_crtc_vblank_restore);
static void drm_legacy_vblank_pre_modeset(struct drm_device *dev, unsigned int pipe) { diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h index a4c3b0a0a197..16d46e2a6854 100644 --- a/include/drm/drm_vblank.h +++ b/include/drm/drm_vblank.h @@ -180,6 +180,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc); void drm_crtc_vblank_reset(struct drm_crtc *crtc); void drm_crtc_vblank_on(struct drm_crtc *crtc); u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc); +void drm_vblank_restore(struct drm_device *dev, unsigned int pipe); +void drm_crtc_vblank_restore(struct drm_crtc *crtc);
bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, unsigned int pipe, int *max_error, -- 2.11.0
On Fri, 2018-01-19 at 00:01 -0800, Rodrigo Vivi wrote:
On Fri, Jan 12, 2018 at 09:57:06PM +0000, Dhinakaran Pandiyan wrote:
The HW frame counter can get reset if device enters a low power state after vblank interrupts were disabled. This messes up any following vblank count update as a negative diff (huge unsigned diff) is calculated from the HW frame counter change. We cannot ignore negative diffs altogther as there could be legitimate wrap arounds. So, allow drivers to update vblank->count with missed vblanks for the time interrupts were disabled. This is similar to _crtc_vblank_on() except that vblanks interrupts are not enabled at the end as this function is expected to be called from the driver _enable_vblank() vfunc.
v2: drm_crtc_vblank_restore should take crtc as arg. (Chris) Add docs and sprinkle some asserts.
Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Michel Dänzer michel@daenzer.net Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_vblank.c | 59 ++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_vblank.h | 2 ++ 2 files changed, 61 insertions(+)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 2559d2d7b907..2690966694f0 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1237,6 +1237,65 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc) } EXPORT_SYMBOL(drm_crtc_vblank_on);
+/**
- drm_vblank_restore - estimated vblanks using timestamps and update it.
- Power manamement features can cause frame counter resets between vblank
- disable and enable. Drivers can then use this function in their
- &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
- the last &drm_crtc_funcs.disable_vblank.
- This function is the legacy version of drm_crtc_vblank_restore().
- */
+void drm_vblank_restore(struct drm_device *dev, unsigned int pipe) +{
- ktime_t t_vblank;
- struct drm_vblank_crtc *vblank;
- int framedur_ns;
- u64 diff_ns;
- u32 cur_vblank, diff = 1;
- int count = DRM_TIMESTAMP_MAXRETRIES;
- if (WARN_ON(pipe >= dev->num_crtcs))
return;- assert_spin_locked(&dev->vbl_lock);
- assert_spin_locked(&dev->vblank_time_lock);
- vblank = &dev->vblank[pipe];
- WARN_ONCE((drm_debug & DRM_UT_VBL) && !vblank->framedur_ns,
do we really only need this warn on debug vlb?
"Cannot compute missed vblanks without frame duration\n");The message seems hard... if we *cannot* why do we move fwd?
We assume the diff is 1 and make an update, some kind of a default similar to what is implemented in drm_update_vblank_count()
- framedur_ns = vblank->framedur_ns;
- do {
cur_vblank = __get_vblank_counter(dev, pipe);drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false);- } while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
Based on the commend of drm_update_vblank_count I don't feel that we have to
This one? - "* We repeat the hardware vblank counter & timestamp query until * we get consistent results. This to prevent races between gpu * updating its hardware counter while we are retrieving the * corresponding vblank timestamp."
I added the loop based on that comment. If the register if partially updated, we want to discard that and loop until it is stable.
do the loop here... And if we have maybe we should re-org things to avoid duplication?
I considered that, we need to pass at least four args for three lines of code. Felt it was too small to warrant a separate function.
- diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
- if (framedur_ns)
diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns);- DRM_DEBUG_VBL("missed %d vblanks in %lld ns, frame duration=%d ns, hw_diff=%d\n",
diff, diff_ns, framedur_ns, cur_vblank - vblank->last);- store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
hm... I wonder if the simple store_vblank(... __get_vblank_counter(dev, pipe)); wouldn't work here...
We have to store a stable count.
+} +EXPORT_SYMBOL(drm_vblank_restore);
+/**
- drm_crtc_vblank_restore - estimate vblanks using timestamps and update it.
- Power manamement features can cause frame counter resets between vblank
- disable and enable. Drivers can then use this function in their
- &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
- the last &drm_crtc_funcs.disable_vblank.
- */
+void drm_crtc_vblank_restore(struct drm_crtc *crtc) +{
- drm_vblank_restore(crtc->dev, drm_crtc_index(crtc));
+} +EXPORT_SYMBOL(drm_crtc_vblank_restore);
static void drm_legacy_vblank_pre_modeset(struct drm_device *dev, unsigned int pipe) { diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h index a4c3b0a0a197..16d46e2a6854 100644 --- a/include/drm/drm_vblank.h +++ b/include/drm/drm_vblank.h @@ -180,6 +180,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc); void drm_crtc_vblank_reset(struct drm_crtc *crtc); void drm_crtc_vblank_on(struct drm_crtc *crtc); u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc); +void drm_vblank_restore(struct drm_device *dev, unsigned int pipe); +void drm_crtc_vblank_restore(struct drm_crtc *crtc);
bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, unsigned int pipe, int *max_error, -- 2.11.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jan 19, 2018 at 10:02:12PM +0000, Pandiyan, Dhinakaran wrote:
On Fri, 2018-01-19 at 00:01 -0800, Rodrigo Vivi wrote:
On Fri, Jan 12, 2018 at 09:57:06PM +0000, Dhinakaran Pandiyan wrote:
The HW frame counter can get reset if device enters a low power state after vblank interrupts were disabled. This messes up any following vblank count update as a negative diff (huge unsigned diff) is calculated from the HW frame counter change. We cannot ignore negative diffs altogther as there could be legitimate wrap arounds. So, allow drivers to update vblank->count with missed vblanks for the time interrupts were disabled. This is similar to _crtc_vblank_on() except that vblanks interrupts are not enabled at the end as this function is expected to be called from the driver _enable_vblank() vfunc.
v2: drm_crtc_vblank_restore should take crtc as arg. (Chris) Add docs and sprinkle some asserts.
Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Michel Dänzer michel@daenzer.net Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_vblank.c | 59 ++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_vblank.h | 2 ++ 2 files changed, 61 insertions(+)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 2559d2d7b907..2690966694f0 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1237,6 +1237,65 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc) } EXPORT_SYMBOL(drm_crtc_vblank_on);
+/**
- drm_vblank_restore - estimated vblanks using timestamps and update it.
- Power manamement features can cause frame counter resets between vblank
- disable and enable. Drivers can then use this function in their
- &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
- the last &drm_crtc_funcs.disable_vblank.
- This function is the legacy version of drm_crtc_vblank_restore().
- */
+void drm_vblank_restore(struct drm_device *dev, unsigned int pipe) +{
- ktime_t t_vblank;
- struct drm_vblank_crtc *vblank;
- int framedur_ns;
- u64 diff_ns;
- u32 cur_vblank, diff = 1;
- int count = DRM_TIMESTAMP_MAXRETRIES;
- if (WARN_ON(pipe >= dev->num_crtcs))
return;- assert_spin_locked(&dev->vbl_lock);
- assert_spin_locked(&dev->vblank_time_lock);
- vblank = &dev->vblank[pipe];
- WARN_ONCE((drm_debug & DRM_UT_VBL) && !vblank->framedur_ns,
do we really only need this warn on debug vlb?
"Cannot compute missed vblanks without frame duration\n");The message seems hard... if we *cannot* why do we move fwd?
We assume the diff is 1 and make an update, some kind of a default similar to what is implemented in drm_update_vblank_count()
- framedur_ns = vblank->framedur_ns;
- do {
cur_vblank = __get_vblank_counter(dev, pipe);drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false);- } while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
Based on the commend of drm_update_vblank_count I don't feel that we have to
This one? - "* We repeat the hardware vblank counter & timestamp query until
- we get consistent results. This to prevent races between gpu
- updating its hardware counter while we are retrieving the
- corresponding vblank timestamp."
I added the loop based on that comment. If the register if partially updated, we want to discard that and loop until it is stable.
do the loop here... And if we have maybe we should re-org things to avoid duplication?
I considered that, we need to pass at least four args for three lines of code. Felt it was too small to warrant a separate function.
- diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
- if (framedur_ns)
diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns);- DRM_DEBUG_VBL("missed %d vblanks in %lld ns, frame duration=%d ns, hw_diff=%d\n",
diff, diff_ns, framedur_ns, cur_vblank - vblank->last);- store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
hm... I wonder if the simple store_vblank(... __get_vblank_counter(dev, pipe)); wouldn't work here...
We have to store a stable count.
I don't see why our counter wouldn't be stable at that point?
Our register is zero or the proper counter. Our frm_counter reg doesn't stop when vblank interrupts are disabled afaik...
But well, I see your point of making something more robust, using the well known and tested methods and preparing for the case where someone start using this call in other platforms as well.... So, whatever you decide..
If you go wit this:
Reviewed-by: Rodrigo Vivi rodrigo.vivi@intel.com
+} +EXPORT_SYMBOL(drm_vblank_restore);
+/**
- drm_crtc_vblank_restore - estimate vblanks using timestamps and update it.
- Power manamement features can cause frame counter resets between vblank
- disable and enable. Drivers can then use this function in their
- &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
- the last &drm_crtc_funcs.disable_vblank.
- */
+void drm_crtc_vblank_restore(struct drm_crtc *crtc) +{
- drm_vblank_restore(crtc->dev, drm_crtc_index(crtc));
+} +EXPORT_SYMBOL(drm_crtc_vblank_restore);
static void drm_legacy_vblank_pre_modeset(struct drm_device *dev, unsigned int pipe) { diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h index a4c3b0a0a197..16d46e2a6854 100644 --- a/include/drm/drm_vblank.h +++ b/include/drm/drm_vblank.h @@ -180,6 +180,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc); void drm_crtc_vblank_reset(struct drm_crtc *crtc); void drm_crtc_vblank_on(struct drm_crtc *crtc); u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc); +void drm_vblank_restore(struct drm_device *dev, unsigned int pipe); +void drm_crtc_vblank_restore(struct drm_crtc *crtc);
bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, unsigned int pipe, int *max_error, -- 2.11.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
The frame counter may have got reset between disabling and enabling vblank interrupts due to DMC putting the hardware to DC5/6 state if PSR was active. The frame counter also could have stalled if PSR is active in cases where there is no DMC. The frame counter resetting as a user visible impact of screen freezes. Use drm_vblank_restore() to compute missed vblanks in the duration for which vblank interrupts are disabled. There's no need particularly check if PSR was active in the interrupt disabled duration.
Enabling vblank interrupts wakes up the hardware from DC5/6 and prevents it from going back again as long as the there are pending interrupts. So, we don't have to explicity disallow DC5/6 after enabling vblank interrupts to keep the counter running.
Let's not apply this to CHV for now, as enabling interrupts does not prevent the hardware from activating PSR and thereby stalling the counter.
Cc: Rodrigo Vivi rodrigo.vivi@intel.com Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com --- drivers/gpu/drm/i915/i915_irq.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 3517c6548e2c..db3466ec6faa 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2956,6 +2956,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe) ilk_enable_display_irq(dev_priv, bit); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+ if (HAS_PSR(dev_priv)) + drm_vblank_restore(dev, pipe); + return 0; }
@@ -2968,6 +2971,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+ if (HAS_PSR(dev_priv)) + drm_vblank_restore(dev, pipe); + return 0; }
On Fri, Jan 12, 2018 at 01:57:07PM -0800, Dhinakaran Pandiyan wrote:
The frame counter may have got reset between disabling and enabling vblank interrupts due to DMC putting the hardware to DC5/6 state if PSR was active. The frame counter also could have stalled if PSR is active in cases where there is no DMC. The frame counter resetting as a user visible impact of screen freezes. Use drm_vblank_restore() to compute missed vblanks in the duration for which vblank interrupts are disabled. There's no need particularly check if PSR was active in the interrupt disabled duration.
Enabling vblank interrupts wakes up the hardware from DC5/6 and prevents it from going back again as long as the there are pending interrupts. So, we don't have to explicity disallow DC5/6 after enabling vblank interrupts to keep the counter running.
Let's not apply this to CHV for now, as enabling interrupts does not prevent the hardware from activating PSR and thereby stalling the counter.
Cc: Rodrigo Vivi rodrigo.vivi@intel.com Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
Read this series, and I think compared to all the previous attempts to fix this issue this one here seems the cleanest. Not an expert on the vblank code (Michel or Ville are best for that), but on the series:
Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
Cheers, Daniel
drivers/gpu/drm/i915/i915_irq.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 3517c6548e2c..db3466ec6faa 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2956,6 +2956,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe) ilk_enable_display_irq(dev_priv, bit); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
- if (HAS_PSR(dev_priv))
drm_vblank_restore(dev, pipe);- return 0;
}
@@ -2968,6 +2971,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
- if (HAS_PSR(dev_priv))
drm_vblank_restore(dev, pipe);- return 0;
}
-- 2.11.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jan 12, 2018 at 09:57:07PM +0000, Dhinakaran Pandiyan wrote:
The frame counter may have got reset between disabling and enabling vblank interrupts due to DMC putting the hardware to DC5/6 state if PSR was active. The frame counter also could have stalled if PSR is active in cases where there is no DMC. The frame counter resetting as a user visible impact of screen freezes. Use drm_vblank_restore() to compute missed vblanks in the duration for which vblank interrupts are disabled. There's no need particularly check if PSR was active in the interrupt disabled duration.
Enabling vblank interrupts wakes up the hardware from DC5/6 and prevents it from going back again as long as the there are pending interrupts. So, we don't have to explicity disallow DC5/6 after enabling vblank interrupts to keep the counter running.
Let's not apply this to CHV for now, as enabling interrupts does not prevent the hardware from activating PSR and thereby stalling the counter.
Cc: Rodrigo Vivi rodrigo.vivi@intel.com Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/i915/i915_irq.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 3517c6548e2c..db3466ec6faa 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2956,6 +2956,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe) ilk_enable_display_irq(dev_priv, bit); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
- if (HAS_PSR(dev_priv))
drm_vblank_restore(dev, pipe);
I don't believe we need this one here.
pre-gen9 platform has psr but not dmc, so the case where we need to restore the counter doesn't exist.
return 0; }
@@ -2968,6 +2971,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
- if (HAS_PSR(dev_priv))
HAS_PSR(dev_priv) && HAS_CSR(dev_priv) maybe? So it gets clear that it is not because PSR that we need to restore the counter, but also don't do it when not needed.
drm_vblank_restore(dev, pipe);- return 0;
}
-- 2.11.0
On Thu, 2018-01-18 at 23:26 -0800, Rodrigo Vivi wrote:
On Fri, Jan 12, 2018 at 09:57:07PM +0000, Dhinakaran Pandiyan wrote:
The frame counter may have got reset between disabling and enabling vblank interrupts due to DMC putting the hardware to DC5/6 state if PSR was active. The frame counter also could have stalled if PSR is active in cases where there is no DMC. The frame counter resetting as a user visible impact of screen freezes. Use drm_vblank_restore() to compute missed vblanks in the duration for which vblank interrupts are disabled. There's no need particularly check if PSR was active in the interrupt disabled duration.
Enabling vblank interrupts wakes up the hardware from DC5/6 and prevents it from going back again as long as the there are pending interrupts. So, we don't have to explicity disallow DC5/6 after enabling vblank interrupts to keep the counter running.
Let's not apply this to CHV for now, as enabling interrupts does not prevent the hardware from activating PSR and thereby stalling the counter.
Cc: Rodrigo Vivi rodrigo.vivi@intel.com Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/i915/i915_irq.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 3517c6548e2c..db3466ec6faa 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2956,6 +2956,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe) ilk_enable_display_irq(dev_priv, bit); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
- if (HAS_PSR(dev_priv))
drm_vblank_restore(dev, pipe);I don't believe we need this one here.
pre-gen9 platform has psr but not dmc, so the case where we need to restore the counter doesn't exist.
Even without DMC, counter should be stuck when PSR is active as no frames are generated by the pipe. I am using drm_vblank_restore_count() to take care of that.
return 0; }
@@ -2968,6 +2971,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
- if (HAS_PSR(dev_priv))
HAS_PSR(dev_priv) && HAS_CSR(dev_priv) maybe? So it gets clear that it is not because PSR that we need to restore the counter, but also don't do it when not needed.
Same reason as above, let me test this again by disabling DMC.
drm_vblank_restore(dev, pipe);- return 0;
}
-- 2.11.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jan 19, 2018 at 09:42:14PM +0000, Pandiyan, Dhinakaran wrote:
On Thu, 2018-01-18 at 23:26 -0800, Rodrigo Vivi wrote:
On Fri, Jan 12, 2018 at 09:57:07PM +0000, Dhinakaran Pandiyan wrote:
The frame counter may have got reset between disabling and enabling vblank interrupts due to DMC putting the hardware to DC5/6 state if PSR was active. The frame counter also could have stalled if PSR is active in cases where there is no DMC. The frame counter resetting as a user visible impact of screen freezes. Use drm_vblank_restore() to compute missed vblanks in the duration for which vblank interrupts are disabled. There's no need particularly check if PSR was active in the interrupt disabled duration.
Enabling vblank interrupts wakes up the hardware from DC5/6 and prevents it from going back again as long as the there are pending interrupts. So, we don't have to explicity disallow DC5/6 after enabling vblank interrupts to keep the counter running.
Let's not apply this to CHV for now, as enabling interrupts does not prevent the hardware from activating PSR and thereby stalling the counter.
Cc: Rodrigo Vivi rodrigo.vivi@intel.com Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/i915/i915_irq.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 3517c6548e2c..db3466ec6faa 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2956,6 +2956,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe) ilk_enable_display_irq(dev_priv, bit); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
- if (HAS_PSR(dev_priv))
drm_vblank_restore(dev, pipe);I don't believe we need this one here.
pre-gen9 platform has psr but not dmc, so the case where we need to restore the counter doesn't exist.
Even without DMC, counter should be stuck when PSR is active as no frames are generated by the pipe. I am using drm_vblank_restore_count() to take care of that.
Oh oh! Indeed. Now I remember you had told me this here. Can you please add a comment with this info somewhere so I don't ask the same question again ;)
anyways:
Reviewed-by: Rodrigo Vivi rodrigo.vivi@intel.com
return 0; }
@@ -2968,6 +2971,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
- if (HAS_PSR(dev_priv))
HAS_PSR(dev_priv) && HAS_CSR(dev_priv) maybe? So it gets clear that it is not because PSR that we need to restore the counter, but also don't do it when not needed.
Same reason as above, let me test this again by disabling DMC.
drm_vblank_restore(dev, pipe);- return 0;
}
-- 2.11.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jan 12, 2018 at 01:57:03PM -0800, Dhinakaran Pandiyan wrote:
drm_vblank_count() has a u32 type returning what is a 64-bit vblank count. The effect of this is when drm_wait_vblank_ioctl() tries to widen the user space requested vblank sequence using this clipped 32-bit count(when the value is >= 2^32) as reference, the requested sequence remains a 32-bit value and gets queued like that. However, the code that checks if the requested sequence has passed compares this against the 64-bit vblank count.
Cc: Keith Packard keithp@keithp.com Cc: Michel Dänzer michel@daenzer.net Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
Sounds like the 64bit widening wasn't all that well tested ... do we have an igt for this? Iirc the base igt was merged already. -Daniel
drivers/gpu/drm/drm_vblank.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 32d9bcf5be7f..768a8e44d99b 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, store_vblank(dev, pipe, diff, t_vblank, cur_vblank); }
-static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe) +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
-- 2.11.0
On Mon, 2018-01-15 at 10:38 +0100, Daniel Vetter wrote:
On Fri, Jan 12, 2018 at 01:57:03PM -0800, Dhinakaran Pandiyan wrote:
drm_vblank_count() has a u32 type returning what is a 64-bit vblank count. The effect of this is when drm_wait_vblank_ioctl() tries to widen the user space requested vblank sequence using this clipped 32-bit count(when the value is >= 2^32) as reference, the requested sequence remains a 32-bit value and gets queued like that. However, the code that checks if the requested sequence has passed compares this against the 64-bit vblank count.
Cc: Keith Packard keithp@keithp.com Cc: Michel Dänzer michel@daenzer.net Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
Sounds like the 64bit widening wasn't all that well tested ... do we have an igt for this? Iirc the base igt was merged already.
I don't see anything that would particularly trigger this condition i.e., vblank->count > 2^32 in the IGTs. We'll need to implement something to force set a very large vblank->count and then request a vblank sequence.
-Daniel
drivers/gpu/drm/drm_vblank.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 32d9bcf5be7f..768a8e44d99b 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, store_vblank(dev, pipe, diff, t_vblank, cur_vblank); }
-static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe) +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
-- 2.11.0
ping for review.
Let me know if there's anything that needs to be done, thanks!
On Fri, 2018-01-12 at 13:57 -0800, Dhinakaran Pandiyan wrote:
drm_vblank_count() has a u32 type returning what is a 64-bit vblank count. The effect of this is when drm_wait_vblank_ioctl() tries to widen the user space requested vblank sequence using this clipped 32-bit count(when the value is >= 2^32) as reference, the requested sequence remains a 32-bit value and gets queued like that. However, the code that checks if the requested sequence has passed compares this against the 64-bit vblank count.
Cc: Keith Packard keithp@keithp.com Cc: Michel Dänzer michel@daenzer.net Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_vblank.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 32d9bcf5be7f..768a8e44d99b 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, store_vblank(dev, pipe, diff, t_vblank, cur_vblank); }
-static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe) +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
On Fri, Jan 19, 2018 at 04:53:34AM +0000, Pandiyan, Dhinakaran wrote:
ping for review.
sorry for not getting back sooner here.
But yey \o/ I finally have dmc and psr working well on my own laptop! so far so good! :)
Let me know if there's anything that needs to be done, thanks!
On Fri, 2018-01-12 at 13:57 -0800, Dhinakaran Pandiyan wrote:
drm_vblank_count() has a u32 type returning what is a 64-bit vblank count. The effect of this is when drm_wait_vblank_ioctl() tries to widen the user space requested vblank sequence using this clipped 32-bit count(when the value is >= 2^32) as reference, the requested sequence remains a 32-bit value and gets queued like that. However, the code that checks if the requested sequence has passed compares this against the 64-bit vblank count.
Cc: Keith Packard keithp@keithp.com Cc: Michel Dänzer michel@daenzer.net Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_vblank.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 32d9bcf5be7f..768a8e44d99b 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, store_vblank(dev, pipe, diff, t_vblank, cur_vblank); }
-static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe) +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
On Fri, Jan 12, 2018 at 09:57:03PM +0000, Dhinakaran Pandiyan wrote:
drm_vblank_count() has a u32 type returning what is a 64-bit vblank count. The effect of this is when drm_wait_vblank_ioctl() tries to widen the user space requested vblank sequence using this clipped 32-bit count(when the value is >= 2^32) as reference, the requested sequence remains a 32-bit value and gets queued like that. However, the code that checks if the requested sequence has passed compares this against the 64-bit vblank count.
Worth to mention and probably Fixes: 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]")
btw, I spotted at least one more place even with the series applied. 32 current_vblank; at drm_mode_page_flip_ioctl...
so, probably worth to do a deeper check down to all paths...
anayway, for this patch: Reviewed-by: Rodrigo Vivi rodrigo.vivi@intel.com
Cc: Keith Packard keithp@keithp.com Cc: Michel Dänzer michel@daenzer.net Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_vblank.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 32d9bcf5be7f..768a8e44d99b 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, store_vblank(dev, pipe, diff, t_vblank, cur_vblank); }
-static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe) +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
-- 2.11.0
On Thu, 2018-01-18 at 23:36 -0800, Rodrigo Vivi wrote:
On Fri, Jan 12, 2018 at 09:57:03PM +0000, Dhinakaran Pandiyan wrote:
drm_vblank_count() has a u32 type returning what is a 64-bit vblank count. The effect of this is when drm_wait_vblank_ioctl() tries to widen the user space requested vblank sequence using this clipped 32-bit count(when the value is >= 2^32) as reference, the requested sequence remains a 32-bit value and gets queued like that. However, the code that checks if the requested sequence has passed compares this against the 64-bit vblank count.
Worth to mention and probably Fixes: 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]")
btw, I spotted at least one more place even with the series applied. 32 current_vblank; at drm_mode_page_flip_ioctl...
There seem to be several such callers for drm_crtc_vblank_count(). I can fix it up as a follow-up series.
so, probably worth to do a deeper check down to all paths...
anayway, for this patch: Reviewed-by: Rodrigo Vivi rodrigo.vivi@intel.com
Cc: Keith Packard keithp@keithp.com Cc: Michel Dänzer michel@daenzer.net Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
drivers/gpu/drm/drm_vblank.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 32d9bcf5be7f..768a8e44d99b 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -271,7 +271,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, store_vblank(dev, pipe, diff, t_vblank, cur_vblank); }
-static u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe) +static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
-- 2.11.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com writes:
drm_vblank_count() has a u32 type returning what is a 64-bit vblank count.
It looks like a general review of the 64-bit widening patch is needed.
* drm_crtc_accurate_vblank_count has a 32-bit return, and uses a 32-bit temporary * drm_wait_one_vblank uses a 32-bit temporary.
I looked at every 'u32' in drm_vblank.c; it would be good to have more eyes check this.
Thanks for finding the first one.
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Dhinakaran Pandiyan -
Keith Packard -
Pandiyan, Dhinakaran
-
Rodrigo Vivi