* [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()
@ 2018-01-12 21:57 Dhinakaran Pandiyan
2018-01-12 21:57 ` [PATCH 2/5] drm/vblank: Fix data type width for drm_crtc_arm_vblank_event() Dhinakaran Pandiyan
` (9 more replies)
0 siblings, 10 replies; 24+ messages in thread
From: Dhinakaran Pandiyan @ 2018-01-12 21:57 UTC (permalink / raw)
To: intel-gfx
Cc: Keith Packard, Michel Dänzer, dri-devel,
Dhinakaran Pandiyan, rodrigo.vivi
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];
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/5] drm/vblank: Fix data type width for drm_crtc_arm_vblank_event()
2018-01-12 21:57 [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count() Dhinakaran Pandiyan
@ 2018-01-12 21:57 ` Dhinakaran Pandiyan
2018-01-19 7:39 ` Rodrigo Vivi
2018-01-31 6:49 ` Keith Packard
2018-01-12 21:57 ` [PATCH 3/5] drm/vblank: Do not update vblank count if interrupts are already disabled Dhinakaran Pandiyan
` (8 subsequent siblings)
9 siblings, 2 replies; 24+ messages in thread
From: Dhinakaran Pandiyan @ 2018-01-12 21:57 UTC (permalink / raw)
To: intel-gfx
Cc: Keith Packard, Michel Dänzer, dri-devel,
Dhinakaran Pandiyan, rodrigo.vivi
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,
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 3/5] drm/vblank: Do not update vblank count if interrupts are already disabled.
2018-01-12 21:57 [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count() Dhinakaran Pandiyan
2018-01-12 21:57 ` [PATCH 2/5] drm/vblank: Fix data type width for drm_crtc_arm_vblank_event() Dhinakaran Pandiyan
@ 2018-01-12 21:57 ` Dhinakaran Pandiyan
2018-01-19 7:47 ` Rodrigo Vivi
2018-01-12 21:57 ` [PATCH 4/5] drm/vblank: Restoring vblank counts after device PM events Dhinakaran Pandiyan
` (7 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Dhinakaran Pandiyan @ 2018-01-12 21:57 UTC (permalink / raw)
To: intel-gfx
Cc: Daniel Vetter, Michel Dänzer, dri-devel,
Dhinakaran Pandiyan, rodrigo.vivi
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);
}
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 4/5] drm/vblank: Restoring vblank counts after device PM events.
2018-01-12 21:57 [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count() Dhinakaran Pandiyan
2018-01-12 21:57 ` [PATCH 2/5] drm/vblank: Fix data type width for drm_crtc_arm_vblank_event() Dhinakaran Pandiyan
2018-01-12 21:57 ` [PATCH 3/5] drm/vblank: Do not update vblank count if interrupts are already disabled Dhinakaran Pandiyan
@ 2018-01-12 21:57 ` Dhinakaran Pandiyan
2018-01-19 8:01 ` Rodrigo Vivi
2018-01-12 21:57 ` [PATCH 5/5] drm/i915: Estimate and update missed vblanks Dhinakaran Pandiyan
` (6 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Dhinakaran Pandiyan @ 2018-01-12 21:57 UTC (permalink / raw)
To: intel-gfx
Cc: Daniel Vetter, Michel Dänzer, dri-devel,
Dhinakaran Pandiyan, rodrigo.vivi
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,
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 5/5] drm/i915: Estimate and update missed vblanks.
2018-01-12 21:57 [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count() Dhinakaran Pandiyan
` (2 preceding siblings ...)
2018-01-12 21:57 ` [PATCH 4/5] drm/vblank: Restoring vblank counts after device PM events Dhinakaran Pandiyan
@ 2018-01-12 21:57 ` Dhinakaran Pandiyan
2018-01-15 9:45 ` Daniel Vetter
2018-01-19 7:26 ` Rodrigo Vivi
2018-01-12 22:26 ` ✓ Fi.CI.BAT: success for series starting with [1/5] drm/vblank: Fix return type for drm_vblank_count() Patchwork
` (5 subsequent siblings)
9 siblings, 2 replies; 24+ messages in thread
From: Dhinakaran Pandiyan @ 2018-01-12 21:57 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan, dri-devel, rodrigo.vivi
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;
}
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/5] drm/vblank: Fix return type for drm_vblank_count()
2018-01-12 21:57 [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count() Dhinakaran Pandiyan
` (3 preceding siblings ...)
2018-01-12 21:57 ` [PATCH 5/5] drm/i915: Estimate and update missed vblanks Dhinakaran Pandiyan
@ 2018-01-12 22:26 ` Patchwork
2018-01-12 23:16 ` ✗ Fi.CI.IGT: failure " Patchwork
` (4 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2018-01-12 22:26 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/5] drm/vblank: Fix return type for drm_vblank_count()
URL : https://patchwork.freedesktop.org/series/36435/
State : success
== Summary ==
Series 36435v1 series starting with [1/5] drm/vblank: Fix return type for drm_vblank_count()
https://patchwork.freedesktop.org/api/1.0/series/36435/revisions/1/mbox/
Test gem_exec_suspend:
Subgroup basic-s3:
pass -> INCOMPLETE (fi-skl-6700k2) fdo#104108
fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:424s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:428s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:370s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:487s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:282s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:482s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:488s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:467s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:461s
fi-elk-e7500 total:224 pass:168 dwarn:10 dfail:0 fail:0 skip:45
fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:273s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:511s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:392s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:403s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:411s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:461s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:416s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:467s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:498s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:457s
fi-kbl-r total:288 pass:260 dwarn:1 dfail:0 fail:0 skip:27 time:503s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:578s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:431s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:509s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:529s
fi-skl-6700k2 total:108 pass:96 dwarn:0 dfail:0 fail:0 skip:11
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:489s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:431s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:538s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:395s
Blacklisted hosts:
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:573s
fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:472s
fcf7fdfd2db6cfe8e529c79c5822f555a8b38fd4 drm-tip: 2018y-01m-12d-17h-40m-49s UTC integration manifest
ef67bfb35c68 drm/i915: Estimate and update missed vblanks.
b1edcc47812a drm/vblank: Restoring vblank counts after device PM events.
365390c83462 drm/vblank: Do not update vblank count if interrupts are already disabled.
18e5209cfde5 drm/vblank: Fix data type width for drm_crtc_arm_vblank_event()
60165b22a0be drm/vblank: Fix return type for drm_vblank_count()
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7660/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* ✗ Fi.CI.IGT: failure for series starting with [1/5] drm/vblank: Fix return type for drm_vblank_count()
2018-01-12 21:57 [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count() Dhinakaran Pandiyan
` (4 preceding siblings ...)
2018-01-12 22:26 ` ✓ Fi.CI.BAT: success for series starting with [1/5] drm/vblank: Fix return type for drm_vblank_count() Patchwork
@ 2018-01-12 23:16 ` Patchwork
2018-01-15 9:38 ` [PATCH 1/5] " Daniel Vetter
` (3 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2018-01-12 23:16 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/5] drm/vblank: Fix return type for drm_vblank_count()
URL : https://patchwork.freedesktop.org/series/36435/
State : failure
== Summary ==
Test gem_caching:
Subgroup writes:
pass -> INCOMPLETE (shard-snb)
Test kms_frontbuffer_tracking:
Subgroup fbc-1p-pri-indfb-multidraw:
pass -> FAIL (shard-snb) fdo#103167
Subgroup fbc-1p-offscren-pri-shrfb-draw-blt:
pass -> FAIL (shard-snb) fdo#101623 +1
Test kms_flip:
Subgroup vblank-vs-suspend-interruptible:
incomplete -> PASS (shard-hsw) fdo#100368 +1
Subgroup vblank-vs-suspend:
pass -> SKIP (shard-snb) fdo#102365
fdo#103167
fdo#101623
fdo#100368
fdo#102365
shard-hsw total:2713 pass:1537 dwarn:1 dfail:0 fail:10 skip:1165 time:9012s
shard-snb total:2683 pass:1294 dwarn:1 dfail:0 fail:12 skip:1375 time:7610s
Blacklisted hosts:
shard-apl total:2713 pass:1687 dwarn:1 dfail:0 fail:24 skip:1001 time:13691s
shard-kbl total:2708 pass:1804 dwarn:5 dfail:0 fail:24 skip:874 time:10328s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7660/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()
2018-01-12 21:57 [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count() Dhinakaran Pandiyan
` (5 preceding siblings ...)
2018-01-12 23:16 ` ✗ Fi.CI.IGT: failure " Patchwork
@ 2018-01-15 9:38 ` Daniel Vetter
2018-01-16 21:26 ` [Intel-gfx] " Pandiyan, Dhinakaran
2018-01-19 4:53 ` Pandiyan, Dhinakaran
` (2 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Daniel Vetter @ 2018-01-15 9:38 UTC (permalink / raw)
To: Dhinakaran Pandiyan
Cc: Michel Dänzer, dri-devel, rodrigo.vivi, 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] drm/i915: Estimate and update missed vblanks.
2018-01-12 21:57 ` [PATCH 5/5] drm/i915: Estimate and update missed vblanks Dhinakaran Pandiyan
@ 2018-01-15 9:45 ` Daniel Vetter
2018-01-19 7:26 ` Rodrigo Vivi
1 sibling, 0 replies; 24+ messages in thread
From: Daniel Vetter @ 2018-01-15 9:45 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: intel-gfx, dri-devel, rodrigo.vivi
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()
2018-01-15 9:38 ` [PATCH 1/5] " Daniel Vetter
@ 2018-01-16 21:26 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 24+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-01-16 21:26 UTC (permalink / raw)
To: daniel; +Cc: intel-gfx, michel, dri-devel, Vivi, Rodrigo
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
> >
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()
2018-01-12 21:57 [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count() Dhinakaran Pandiyan
` (6 preceding siblings ...)
2018-01-15 9:38 ` [PATCH 1/5] " Daniel Vetter
@ 2018-01-19 4:53 ` Pandiyan, Dhinakaran
2018-01-19 8:03 ` Rodrigo Vivi
2018-01-19 7:36 ` Rodrigo Vivi
2018-01-31 6:42 ` Keith Packard
9 siblings, 1 reply; 24+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-01-19 4:53 UTC (permalink / raw)
To: intel-gfx; +Cc: michel, dri-devel, Vivi, Rodrigo
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];
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] drm/i915: Estimate and update missed vblanks.
2018-01-12 21:57 ` [PATCH 5/5] drm/i915: Estimate and update missed vblanks Dhinakaran Pandiyan
2018-01-15 9:45 ` Daniel Vetter
@ 2018-01-19 7:26 ` Rodrigo Vivi
2018-01-19 21:42 ` [Intel-gfx] " Pandiyan, Dhinakaran
1 sibling, 1 reply; 24+ messages in thread
From: Rodrigo Vivi @ 2018-01-19 7:26 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: intel-gfx, dri-devel
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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()
2018-01-12 21:57 [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count() Dhinakaran Pandiyan
` (7 preceding siblings ...)
2018-01-19 4:53 ` Pandiyan, Dhinakaran
@ 2018-01-19 7:36 ` Rodrigo Vivi
2018-01-19 21:30 ` Pandiyan, Dhinakaran
2018-01-31 6:42 ` Keith Packard
9 siblings, 1 reply; 24+ messages in thread
From: Rodrigo Vivi @ 2018-01-19 7:36 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: Michel Dänzer, intel-gfx, dri-devel
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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/5] drm/vblank: Fix data type width for drm_crtc_arm_vblank_event()
2018-01-12 21:57 ` [PATCH 2/5] drm/vblank: Fix data type width for drm_crtc_arm_vblank_event() Dhinakaran Pandiyan
@ 2018-01-19 7:39 ` Rodrigo Vivi
2018-01-31 6:49 ` Keith Packard
1 sibling, 0 replies; 24+ messages in thread
From: Rodrigo Vivi @ 2018-01-19 7:39 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: Michel Dänzer, intel-gfx, dri-devel
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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/5] drm/vblank: Do not update vblank count if interrupts are already disabled.
2018-01-12 21:57 ` [PATCH 3/5] drm/vblank: Do not update vblank count if interrupts are already disabled Dhinakaran Pandiyan
@ 2018-01-19 7:47 ` Rodrigo Vivi
0 siblings, 0 replies; 24+ messages in thread
From: Rodrigo Vivi @ 2018-01-19 7:47 UTC (permalink / raw)
To: Dhinakaran Pandiyan
Cc: Daniel Vetter, intel-gfx, Michel Dänzer, dri-devel
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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/5] drm/vblank: Restoring vblank counts after device PM events.
2018-01-12 21:57 ` [PATCH 4/5] drm/vblank: Restoring vblank counts after device PM events Dhinakaran Pandiyan
@ 2018-01-19 8:01 ` Rodrigo Vivi
2018-01-19 22:02 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 24+ messages in thread
From: Rodrigo Vivi @ 2018-01-19 8:01 UTC (permalink / raw)
To: Dhinakaran Pandiyan
Cc: Daniel Vetter, intel-gfx, Michel Dänzer, dri-devel
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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()
2018-01-19 4:53 ` Pandiyan, Dhinakaran
@ 2018-01-19 8:03 ` Rodrigo Vivi
0 siblings, 0 replies; 24+ messages in thread
From: Rodrigo Vivi @ 2018-01-19 8:03 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx, dri-devel, michel
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];
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()
2018-01-19 7:36 ` Rodrigo Vivi
@ 2018-01-19 21:30 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 24+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-01-19 21:30 UTC (permalink / raw)
To: Vivi, Rodrigo; +Cc: intel-gfx, michel, dri-devel
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH 5/5] drm/i915: Estimate and update missed vblanks.
2018-01-19 7:26 ` Rodrigo Vivi
@ 2018-01-19 21:42 ` Pandiyan, Dhinakaran
2018-01-19 22:45 ` Rodrigo Vivi
0 siblings, 1 reply; 24+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-01-19 21:42 UTC (permalink / raw)
To: Vivi, Rodrigo; +Cc: intel-gfx, dri-devel
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/5] drm/vblank: Restoring vblank counts after device PM events.
2018-01-19 8:01 ` Rodrigo Vivi
@ 2018-01-19 22:02 ` Pandiyan, Dhinakaran
2018-01-19 22:44 ` [Intel-gfx] " Rodrigo Vivi
0 siblings, 1 reply; 24+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-01-19 22:02 UTC (permalink / raw)
To: Vivi, Rodrigo; +Cc: daniel.vetter, michel, dri-devel, intel-gfx
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH 4/5] drm/vblank: Restoring vblank counts after device PM events.
2018-01-19 22:02 ` Pandiyan, Dhinakaran
@ 2018-01-19 22:44 ` Rodrigo Vivi
0 siblings, 0 replies; 24+ messages in thread
From: Rodrigo Vivi @ 2018-01-19 22:44 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: daniel.vetter, michel, dri-devel, 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH 5/5] drm/i915: Estimate and update missed vblanks.
2018-01-19 21:42 ` [Intel-gfx] " Pandiyan, Dhinakaran
@ 2018-01-19 22:45 ` Rodrigo Vivi
0 siblings, 0 replies; 24+ messages in thread
From: Rodrigo Vivi @ 2018-01-19 22:45 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx, dri-devel
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count()
2018-01-12 21:57 [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count() Dhinakaran Pandiyan
` (8 preceding siblings ...)
2018-01-19 7:36 ` Rodrigo Vivi
@ 2018-01-31 6:42 ` Keith Packard
9 siblings, 0 replies; 24+ messages in thread
From: Keith Packard @ 2018-01-31 6:42 UTC (permalink / raw)
To: intel-gfx
Cc: Michel Dänzer, Dhinakaran Pandiyan, dri-devel, rodrigo.vivi
[-- Attachment #1.1: Type: text/plain, Size: 497 bytes --]
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.
--
-keith
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/5] drm/vblank: Fix data type width for drm_crtc_arm_vblank_event()
2018-01-12 21:57 ` [PATCH 2/5] drm/vblank: Fix data type width for drm_crtc_arm_vblank_event() Dhinakaran Pandiyan
2018-01-19 7:39 ` Rodrigo Vivi
@ 2018-01-31 6:49 ` Keith Packard
1 sibling, 0 replies; 24+ messages in thread
From: Keith Packard @ 2018-01-31 6:49 UTC (permalink / raw)
To: intel-gfx
Cc: Michel Dänzer, Dhinakaran Pandiyan, dri-devel, rodrigo.vivi
[-- Attachment #1.1: Type: text/plain, Size: 752 bytes --]
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!
--
-keith
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2018-01-31 6:49 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-12 21:57 [PATCH 1/5] drm/vblank: Fix return type for drm_vblank_count() Dhinakaran Pandiyan
2018-01-12 21:57 ` [PATCH 2/5] drm/vblank: Fix data type width for drm_crtc_arm_vblank_event() Dhinakaran Pandiyan
2018-01-19 7:39 ` Rodrigo Vivi
2018-01-31 6:49 ` Keith Packard
2018-01-12 21:57 ` [PATCH 3/5] drm/vblank: Do not update vblank count if interrupts are already disabled Dhinakaran Pandiyan
2018-01-19 7:47 ` Rodrigo Vivi
2018-01-12 21:57 ` [PATCH 4/5] drm/vblank: Restoring vblank counts after device PM events Dhinakaran Pandiyan
2018-01-19 8:01 ` Rodrigo Vivi
2018-01-19 22:02 ` Pandiyan, Dhinakaran
2018-01-19 22:44 ` [Intel-gfx] " Rodrigo Vivi
2018-01-12 21:57 ` [PATCH 5/5] drm/i915: Estimate and update missed vblanks Dhinakaran Pandiyan
2018-01-15 9:45 ` Daniel Vetter
2018-01-19 7:26 ` Rodrigo Vivi
2018-01-19 21:42 ` [Intel-gfx] " Pandiyan, Dhinakaran
2018-01-19 22:45 ` Rodrigo Vivi
2018-01-12 22:26 ` ✓ Fi.CI.BAT: success for series starting with [1/5] drm/vblank: Fix return type for drm_vblank_count() Patchwork
2018-01-12 23:16 ` ✗ Fi.CI.IGT: failure " Patchwork
2018-01-15 9:38 ` [PATCH 1/5] " Daniel Vetter
2018-01-16 21:26 ` [Intel-gfx] " Pandiyan, Dhinakaran
2018-01-19 4:53 ` Pandiyan, Dhinakaran
2018-01-19 8:03 ` Rodrigo Vivi
2018-01-19 7:36 ` Rodrigo Vivi
2018-01-19 21:30 ` Pandiyan, Dhinakaran
2018-01-31 6:42 ` Keith Packard
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.