* [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()
@ 2017-06-30 14:18 ville.syrjala
2017-06-30 14:18 ` [PATCH 2/3] drm/i915: Fix race between pipe update and vblank irq ville.syrjala
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: ville.syrjala @ 2017-06-30 14:18 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Add drm_crtc_vblank_get_accurate() which is the same as
drm_crtc_vblank_get() except that it will forcefully update the
the current vblank counter/timestamp value even if the interrupt
is already enabled.
And we'll switch drm_wait_one_vblank() over to using it to make sure it
will really wait at least one vblank even if it races with the irq
handler.
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/drm_vblank.c | 37 ++++++++++++++++++++++++++++++++-----
include/drm/drm_vblank.h | 1 +
2 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 823c853de052..c57383b8753b 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -955,7 +955,8 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
return ret;
}
-static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
+static int drm_vblank_get(struct drm_device *dev, unsigned int pipe,
+ bool force_update)
{
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
unsigned long irqflags;
@@ -975,6 +976,10 @@ static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
if (!vblank->enabled) {
atomic_dec(&vblank->refcount);
ret = -EINVAL;
+ } else if (force_update) {
+ spin_lock(&dev->vblank_time_lock);
+ drm_update_vblank_count(dev, pipe, false);
+ spin_unlock(&dev->vblank_time_lock);
}
}
spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
@@ -994,10 +999,32 @@ static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
*/
int drm_crtc_vblank_get(struct drm_crtc *crtc)
{
- return drm_vblank_get(crtc->dev, drm_crtc_index(crtc));
+ return drm_vblank_get(crtc->dev, drm_crtc_index(crtc), false);
}
EXPORT_SYMBOL(drm_crtc_vblank_get);
+/**
+ * drm_crtc_vblank_get_accurate - get a reference count on vblank events and
+ * make sure the counter is uptodate
+ * @crtc: which CRTC to own
+ *
+ * Acquire a reference count on vblank events to avoid having them disabled
+ * while in use.
+ *
+ * Also make sure the current vblank counter is value is fully up to date
+ * even if we're already past the start of vblank but the irq hasn't fired
+ * yet, which may be the case with some hardware that raises the interrupt
+ * only some time after the start of vblank.
+ *
+ * Returns:
+ * Zero on success or a negative error code on failure.
+ */
+int drm_crtc_vblank_get_accurate(struct drm_crtc *crtc)
+{
+ return drm_vblank_get(crtc->dev, drm_crtc_index(crtc), true);
+}
+EXPORT_SYMBOL(drm_crtc_vblank_get_accurate);
+
static void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
{
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
@@ -1053,7 +1080,7 @@ void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe)
if (WARN_ON(pipe >= dev->num_crtcs))
return;
- ret = drm_vblank_get(dev, pipe);
+ ret = drm_vblank_get(dev, pipe, true);
if (WARN(ret, "vblank not available on crtc %i, ret=%i\n", pipe, ret))
return;
@@ -1248,7 +1275,7 @@ static void drm_legacy_vblank_pre_modeset(struct drm_device *dev,
*/
if (!vblank->inmodeset) {
vblank->inmodeset = 0x1;
- if (drm_vblank_get(dev, pipe) == 0)
+ if (drm_vblank_get(dev, pipe, false) == 0)
vblank->inmodeset |= 0x2;
}
}
@@ -1448,7 +1475,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
return 0;
}
- ret = drm_vblank_get(dev, pipe);
+ ret = drm_vblank_get(dev, pipe, false);
if (ret) {
DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
return ret;
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 7fba9efe4951..5629e7841318 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -162,6 +162,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
int drm_crtc_vblank_get(struct drm_crtc *crtc);
+int drm_crtc_vblank_get_accurate(struct drm_crtc *crtc);
void drm_crtc_vblank_put(struct drm_crtc *crtc);
void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe);
void drm_crtc_wait_one_vblank(struct drm_crtc *crtc);
--
2.13.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] drm/i915: Fix race between pipe update and vblank irq
2017-06-30 14:18 [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate() ville.syrjala
@ 2017-06-30 14:18 ` ville.syrjala
2017-06-30 14:18 ` [PATCH 3/3] drm/i915: Use drm_crtc_vblank_get_accurate() in intel_atomic_wait_for_vblanks() ville.syrjala
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: ville.syrjala @ 2017-06-30 14:18 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Make sure that we have an up to date vblank counter value for sending
out the completion event. On gen2/3 the vblank irq fires at frame start
rather than at start of vblank, so if we perform an update between
vblank start and frame start we would potentially sample a stale vblank
counter value, and thus send out the event one frame too soon.
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_sprite.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0c650c2cbca8..61681a11086a 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -197,7 +197,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
* event outside of the critical section - the spinlock might spin for a
* while ... */
if (crtc->base.state->event) {
- WARN_ON(drm_crtc_vblank_get(&crtc->base) != 0);
+ WARN_ON(drm_crtc_vblank_get_accurate(&crtc->base) != 0);
spin_lock(&crtc->base.dev->event_lock);
drm_crtc_arm_vblank_event(&crtc->base, crtc->base.state->event);
--
2.13.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] drm/i915: Use drm_crtc_vblank_get_accurate() in intel_atomic_wait_for_vblanks()
2017-06-30 14:18 [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate() ville.syrjala
2017-06-30 14:18 ` [PATCH 2/3] drm/i915: Fix race between pipe update and vblank irq ville.syrjala
@ 2017-06-30 14:18 ` ville.syrjala
2017-06-30 14:39 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate() Patchwork
2017-06-30 15:26 ` [PATCH 1/3] " Daniel Vetter
3 siblings, 0 replies; 8+ messages in thread
From: ville.syrjala @ 2017-06-30 14:18 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
To make sure intel_atomic_wait_for_vblanks() really waits for at least
one vblank let's switch to using drm_crtc_vblank_get_accurate().
Also toss in a FIXME that we should really be sampling the vblank
counter when we did the plane/wm update instead of resampling it
potentially much later when we call intel_atomic_wait_for_vblanks().
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4e03ca6c946f..12fc4fcf78c5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12826,12 +12826,16 @@ static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
if (!((1 << pipe) & crtc_mask))
continue;
- ret = drm_crtc_vblank_get(&crtc->base);
+ ret = drm_crtc_vblank_get_accurate(&crtc->base);
if (WARN_ON(ret != 0)) {
crtc_mask &= ~(1 << pipe);
continue;
}
+ /*
+ * FIXME we should have sampled this
+ * when we did the actual update.
+ */
last_vblank_count[pipe] = drm_crtc_vblank_count(&crtc->base);
}
--
2.13.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()
2017-06-30 14:18 [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate() ville.syrjala
2017-06-30 14:18 ` [PATCH 2/3] drm/i915: Fix race between pipe update and vblank irq ville.syrjala
2017-06-30 14:18 ` [PATCH 3/3] drm/i915: Use drm_crtc_vblank_get_accurate() in intel_atomic_wait_for_vblanks() ville.syrjala
@ 2017-06-30 14:39 ` Patchwork
2017-06-30 15:26 ` [PATCH 1/3] " Daniel Vetter
3 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2017-06-30 14:39 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()
URL : https://patchwork.freedesktop.org/series/26633/
State : success
== Summary ==
Series 26633v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/26633/revisions/1/mbox/
Test gem_exec_suspend:
Subgroup basic-s4-devices:
pass -> DMESG-WARN (fi-kbl-r) fdo#100125
Test kms_busy:
Subgroup basic-flip-default-b:
dmesg-warn -> FAIL (fi-skl-6700hq) fdo#101144
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
dmesg-warn -> PASS (fi-pnv-d510) fdo#101597 +1
Subgroup suspend-read-crc-pipe-a:
fail -> PASS (fi-skl-6700k)
Subgroup suspend-read-crc-pipe-b:
pass -> DMESG-WARN (fi-byt-j1900) fdo#101516 +1
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597
fdo#101516 https://bugs.freedesktop.org/show_bug.cgi?id=101516
fi-bdw-5557u total:279 pass:264 dwarn:0 dfail:0 fail:3 skip:11 time:440s
fi-bdw-gvtdvm total:279 pass:257 dwarn:8 dfail:0 fail:0 skip:14 time:434s
fi-blb-e6850 total:279 pass:224 dwarn:1 dfail:0 fail:0 skip:54 time:352s
fi-bsw-n3050 total:279 pass:239 dwarn:0 dfail:0 fail:3 skip:36 time:515s
fi-bxt-j4205 total:279 pass:256 dwarn:0 dfail:0 fail:3 skip:19 time:509s
fi-byt-j1900 total:279 pass:250 dwarn:1 dfail:0 fail:3 skip:24 time:477s
fi-byt-n2820 total:279 pass:247 dwarn:0 dfail:0 fail:3 skip:28 time:474s
fi-glk-2a total:279 pass:256 dwarn:0 dfail:0 fail:3 skip:19 time:584s
fi-hsw-4770 total:279 pass:259 dwarn:0 dfail:0 fail:3 skip:16 time:429s
fi-hsw-4770r total:279 pass:259 dwarn:0 dfail:0 fail:3 skip:16 time:414s
fi-ilk-650 total:279 pass:225 dwarn:0 dfail:0 fail:3 skip:50 time:414s
fi-ivb-3520m total:279 pass:257 dwarn:0 dfail:0 fail:3 skip:18 time:495s
fi-ivb-3770 total:279 pass:257 dwarn:0 dfail:0 fail:3 skip:18 time:463s
fi-kbl-7500u total:279 pass:257 dwarn:0 dfail:0 fail:3 skip:18 time:457s
fi-kbl-7560u total:279 pass:265 dwarn:0 dfail:0 fail:3 skip:10 time:561s
fi-kbl-r total:279 pass:256 dwarn:1 dfail:0 fail:3 skip:18 time:571s
fi-pnv-d510 total:279 pass:223 dwarn:1 dfail:0 fail:0 skip:55 time:555s
fi-skl-6260u total:279 pass:265 dwarn:0 dfail:0 fail:3 skip:10 time:452s
fi-skl-6700hq total:279 pass:219 dwarn:1 dfail:0 fail:33 skip:24 time:333s
fi-skl-6700k total:279 pass:257 dwarn:0 dfail:0 fail:3 skip:18 time:460s
fi-skl-6770hq total:279 pass:265 dwarn:0 dfail:0 fail:3 skip:10 time:477s
fi-skl-gvtdvm total:279 pass:266 dwarn:0 dfail:0 fail:0 skip:13 time:435s
fi-snb-2520m total:279 pass:247 dwarn:0 dfail:0 fail:3 skip:28 time:539s
fi-snb-2600 total:279 pass:246 dwarn:0 dfail:0 fail:3 skip:29 time:405s
bcea90f867d52f57e0919ae8eaad5fba3c53735e drm-tip: 2017y-06m-30d-13h-38m-04s UTC integration manifest
58679573 drm/i915: Use drm_crtc_vblank_get_accurate() in intel_atomic_wait_for_vblanks()
47a42e9 drm/i915: Fix race between pipe update and vblank irq
139e473 drm/vblank: Introduce drm_crtc_vblank_get_accurate()
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5079/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()
2017-06-30 14:18 [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate() ville.syrjala
` (2 preceding siblings ...)
2017-06-30 14:39 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate() Patchwork
@ 2017-06-30 15:26 ` Daniel Vetter
2017-06-30 18:18 ` Daniel Vetter
3 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2017-06-30 15:26 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx, dri-devel
On Fri, Jun 30, 2017 at 05:18:41PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Add drm_crtc_vblank_get_accurate() which is the same as
> drm_crtc_vblank_get() except that it will forcefully update the
> the current vblank counter/timestamp value even if the interrupt
> is already enabled.
>
> And we'll switch drm_wait_one_vblank() over to using it to make sure it
> will really wait at least one vblank even if it races with the irq
> handler.
>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Hm, I just thought of doing an
s/drm_vblank_count/drm_crtc_accurate_vblank_count/ in
drm_crtc_arm_vblank_event.
What does this buy us above&beyond the other patch? And why isn't
vblank_get accurate always?
/me confused
Cheers, Daniel
> ---
> drivers/gpu/drm/drm_vblank.c | 37 ++++++++++++++++++++++++++++++++-----
> include/drm/drm_vblank.h | 1 +
> 2 files changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 823c853de052..c57383b8753b 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -955,7 +955,8 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
> return ret;
> }
>
> -static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
> +static int drm_vblank_get(struct drm_device *dev, unsigned int pipe,
> + bool force_update)
> {
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> unsigned long irqflags;
> @@ -975,6 +976,10 @@ static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
> if (!vblank->enabled) {
> atomic_dec(&vblank->refcount);
> ret = -EINVAL;
> + } else if (force_update) {
> + spin_lock(&dev->vblank_time_lock);
> + drm_update_vblank_count(dev, pipe, false);
> + spin_unlock(&dev->vblank_time_lock);
> }
> }
> spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> @@ -994,10 +999,32 @@ static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
> */
> int drm_crtc_vblank_get(struct drm_crtc *crtc)
> {
> - return drm_vblank_get(crtc->dev, drm_crtc_index(crtc));
> + return drm_vblank_get(crtc->dev, drm_crtc_index(crtc), false);
> }
> EXPORT_SYMBOL(drm_crtc_vblank_get);
>
> +/**
> + * drm_crtc_vblank_get_accurate - get a reference count on vblank events and
> + * make sure the counter is uptodate
> + * @crtc: which CRTC to own
> + *
> + * Acquire a reference count on vblank events to avoid having them disabled
> + * while in use.
> + *
> + * Also make sure the current vblank counter is value is fully up to date
> + * even if we're already past the start of vblank but the irq hasn't fired
> + * yet, which may be the case with some hardware that raises the interrupt
> + * only some time after the start of vblank.
> + *
> + * Returns:
> + * Zero on success or a negative error code on failure.
> + */
> +int drm_crtc_vblank_get_accurate(struct drm_crtc *crtc)
> +{
> + return drm_vblank_get(crtc->dev, drm_crtc_index(crtc), true);
> +}
> +EXPORT_SYMBOL(drm_crtc_vblank_get_accurate);
> +
> static void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
> {
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> @@ -1053,7 +1080,7 @@ void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe)
> if (WARN_ON(pipe >= dev->num_crtcs))
> return;
>
> - ret = drm_vblank_get(dev, pipe);
> + ret = drm_vblank_get(dev, pipe, true);
> if (WARN(ret, "vblank not available on crtc %i, ret=%i\n", pipe, ret))
> return;
>
> @@ -1248,7 +1275,7 @@ static void drm_legacy_vblank_pre_modeset(struct drm_device *dev,
> */
> if (!vblank->inmodeset) {
> vblank->inmodeset = 0x1;
> - if (drm_vblank_get(dev, pipe) == 0)
> + if (drm_vblank_get(dev, pipe, false) == 0)
> vblank->inmodeset |= 0x2;
> }
> }
> @@ -1448,7 +1475,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
> return 0;
> }
>
> - ret = drm_vblank_get(dev, pipe);
> + ret = drm_vblank_get(dev, pipe, false);
> if (ret) {
> DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
> return ret;
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 7fba9efe4951..5629e7841318 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -162,6 +162,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
> bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
> bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
> int drm_crtc_vblank_get(struct drm_crtc *crtc);
> +int drm_crtc_vblank_get_accurate(struct drm_crtc *crtc);
> void drm_crtc_vblank_put(struct drm_crtc *crtc);
> void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe);
> void drm_crtc_wait_one_vblank(struct drm_crtc *crtc);
> --
> 2.13.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()
2017-06-30 15:26 ` [PATCH 1/3] " Daniel Vetter
@ 2017-06-30 18:18 ` Daniel Vetter
2017-07-03 14:26 ` Ville Syrjälä
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2017-06-30 18:18 UTC (permalink / raw)
To: Syrjala, Ville; +Cc: intel-gfx, dri-devel
On Fri, Jun 30, 2017 at 5:26 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Jun 30, 2017 at 05:18:41PM +0300, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Add drm_crtc_vblank_get_accurate() which is the same as
>> drm_crtc_vblank_get() except that it will forcefully update the
>> the current vblank counter/timestamp value even if the interrupt
>> is already enabled.
>>
>> And we'll switch drm_wait_one_vblank() over to using it to make sure it
>> will really wait at least one vblank even if it races with the irq
>> handler.
>>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Hm, I just thought of doing an
> s/drm_vblank_count/drm_crtc_accurate_vblank_count/ in
> drm_crtc_arm_vblank_event.
>
> What does this buy us above&beyond the other patch? And why isn't
> vblank_get accurate always?
This also seems to have the risk of not fixing the arm_vblank_event
race if someone puts the vblank_get_accurate outside of the critical
section. Then we're back to the same old race. And since that means
you need to have a vblank_get_accurate right before the
arm_vblank_event in all cases, we might as well just put it into the
helper. You wrote in the other thread putting it into arm_vblank_event
might be wasteful, but I don't see any scenario where that could be
the case ...
Or do I miss something?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - 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] 8+ messages in thread
* Re: [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()
2017-06-30 18:18 ` Daniel Vetter
@ 2017-07-03 14:26 ` Ville Syrjälä
2017-07-03 16:46 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Ville Syrjälä @ 2017-07-03 14:26 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, dri-devel
On Fri, Jun 30, 2017 at 08:18:19PM +0200, Daniel Vetter wrote:
> On Fri, Jun 30, 2017 at 5:26 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Jun 30, 2017 at 05:18:41PM +0300, ville.syrjala@linux.intel.com wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> Add drm_crtc_vblank_get_accurate() which is the same as
> >> drm_crtc_vblank_get() except that it will forcefully update the
> >> the current vblank counter/timestamp value even if the interrupt
> >> is already enabled.
> >>
> >> And we'll switch drm_wait_one_vblank() over to using it to make sure it
> >> will really wait at least one vblank even if it races with the irq
> >> handler.
> >>
> >> Cc: Daniel Vetter <daniel@ffwll.ch>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Hm, I just thought of doing an
> > s/drm_vblank_count/drm_crtc_accurate_vblank_count/ in
> > drm_crtc_arm_vblank_event.
> >
> > What does this buy us above&beyond the other patch? And why isn't
> > vblank_get accurate always?
>
> This also seems to have the risk of not fixing the arm_vblank_event
> race if someone puts the vblank_get_accurate outside of the critical
> section. Then we're back to the same old race. And since that means
> you need to have a vblank_get_accurate right before the
> arm_vblank_event in all cases, we might as well just put it into the
> helper. You wrote in the other thread putting it into arm_vblank_event
> might be wasteful, but I don't see any scenario where that could be
> the case ...
>
> Or do I miss something?
The interrupt most likely will be on already when pipe_update_end() gets
called since pipe_update_start() will enable it already, and Chris's
disable_immediate optimization will keep it on until the next interrupt.
Prior to that optimization the drm_vblank_get() in pipe_update_end()
would have had to turn on the interrupt and perform the vblank counter
update, and thus the second update from drm_crtc_accurate_vblank_count()
would have been wasteful.
I'm not sure I like relying on the fact that pipe_update_start() already
turned the interrupt on and it's going to be kept on magically. One
solution for that would be to hang on to the reference from
pipe_update_start() until we've armed the event. Then we know the
vblank_get() won't have to enable the interrupt.
However, if we start to sample the counter for some other purpose
(watermarks, fb unpinning etc.) during pipe_update_end() then we'll
again be faced with another potentially pointless update if we
decide to use drm_crtc_accurate_vblank_count() again.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate()
2017-07-03 14:26 ` Ville Syrjälä
@ 2017-07-03 16:46 ` Daniel Vetter
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2017-07-03 16:46 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, dri-devel
On Mon, Jul 03, 2017 at 05:26:24PM +0300, Ville Syrjälä wrote:
> On Fri, Jun 30, 2017 at 08:18:19PM +0200, Daniel Vetter wrote:
> > On Fri, Jun 30, 2017 at 5:26 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Fri, Jun 30, 2017 at 05:18:41PM +0300, ville.syrjala@linux.intel.com wrote:
> > >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>
> > >> Add drm_crtc_vblank_get_accurate() which is the same as
> > >> drm_crtc_vblank_get() except that it will forcefully update the
> > >> the current vblank counter/timestamp value even if the interrupt
> > >> is already enabled.
> > >>
> > >> And we'll switch drm_wait_one_vblank() over to using it to make sure it
> > >> will really wait at least one vblank even if it races with the irq
> > >> handler.
> > >>
> > >> Cc: Daniel Vetter <daniel@ffwll.ch>
> > >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Hm, I just thought of doing an
> > > s/drm_vblank_count/drm_crtc_accurate_vblank_count/ in
> > > drm_crtc_arm_vblank_event.
> > >
> > > What does this buy us above&beyond the other patch? And why isn't
> > > vblank_get accurate always?
> >
> > This also seems to have the risk of not fixing the arm_vblank_event
> > race if someone puts the vblank_get_accurate outside of the critical
> > section. Then we're back to the same old race. And since that means
> > you need to have a vblank_get_accurate right before the
> > arm_vblank_event in all cases, we might as well just put it into the
> > helper. You wrote in the other thread putting it into arm_vblank_event
> > might be wasteful, but I don't see any scenario where that could be
> > the case ...
> >
> > Or do I miss something?
>
> The interrupt most likely will be on already when pipe_update_end() gets
> called since pipe_update_start() will enable it already, and Chris's
> disable_immediate optimization will keep it on until the next interrupt.
> Prior to that optimization the drm_vblank_get() in pipe_update_end()
> would have had to turn on the interrupt and perform the vblank counter
> update, and thus the second update from drm_crtc_accurate_vblank_count()
> would have been wasteful.
>
> I'm not sure I like relying on the fact that pipe_update_start() already
> turned the interrupt on and it's going to be kept on magically. One
> solution for that would be to hang on to the reference from
> pipe_update_start() until we've armed the event. Then we know the
> vblank_get() won't have to enable the interrupt.
Yeah, relying on some implict vblank_get happening in the right way, far
away from the arm_vblank_event, is the part I don't like. Makes review
harder, and I don't see the gain we'll get in return.
> However, if we start to sample the counter for some other purpose
> (watermarks, fb unpinning etc.) during pipe_update_end() then we'll
> again be faced with another potentially pointless update if we
> decide to use drm_crtc_accurate_vblank_count() again.
I think at that point we should write our own arm_vblank_event and cache
the accurate vblank ts within i915 code. My point is that for a generic
helper, we should aim for safe over fastest option. Having a special
vblank_get that you have to call before you call arm_vblank_event, and
within the critical section, just adds to the list of caveats of that
function. Simply making arm_vblank_event more robust seems like the much
better plan (and we can always fix things in i915 if the overhead is too
much).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-07-03 16:47 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-30 14:18 [PATCH 1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate() ville.syrjala
2017-06-30 14:18 ` [PATCH 2/3] drm/i915: Fix race between pipe update and vblank irq ville.syrjala
2017-06-30 14:18 ` [PATCH 3/3] drm/i915: Use drm_crtc_vblank_get_accurate() in intel_atomic_wait_for_vblanks() ville.syrjala
2017-06-30 14:39 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/vblank: Introduce drm_crtc_vblank_get_accurate() Patchwork
2017-06-30 15:26 ` [PATCH 1/3] " Daniel Vetter
2017-06-30 18:18 ` Daniel Vetter
2017-07-03 14:26 ` Ville Syrjälä
2017-07-03 16:46 ` Daniel Vetter
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.