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);
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);
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); }
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);@@ -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
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
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.
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Ville Syrjälä -
ville.syrjala＠linux.intel.com