From 9625604cebcb3f5be2e692408274734e8ae63979 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Fri, 13 Feb 2015 21:03:42 +0100 Subject: drm/irq: Add drm_crtc_vblank_reset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At driver load we need to tell the vblank code about the state of the pipes, so that the logic around reject vblank_get when the pipe is off works correctly. Thus far i915 used drm_vblank_off, but one of the side-effects of it is that it also saves the vblank counter. And for that it calls down into the ->get_vblank_counter hook. Which isn't really a good idea when the pipe is off for a few reasons: - With runtime pm the register might not respond. - If the pipe is off some datastructures might not be around or unitialized. The later is what blew up on gen3: We look at intel_crtc->config to compute the vblank counter, and for a disabled pipe at boot-up that's just not there. Thus far this was papered over by a check for intel_crtc->active, but I want to get rid of that (since it's fairly race, vblank hooks are called from all kinds of places). So prep for that by adding a _reset functions which only does what we really need to be done at driver load: Mark the vblank pipe as off, but don't do any vblank counter saving or event flushing - neither of that is required. v2: Clarify the code flow slightly as suggested by Ville. v3: Fix kerneldoc spelling, spotted by Laurent. Cc: Ville Syrjälä Cc: Laurent Pinchart Cc: Imre Deak Reviewed-by: Imre Deak (v2) Acked-by: Dave Airlie Acked-by: Laurent Pinchart Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_irq.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) (limited to 'drivers/gpu/drm/drm_irq.c') diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 75647e7f012b..e78a1f5cad9c 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1225,6 +1225,38 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc) } EXPORT_SYMBOL(drm_crtc_vblank_off); +/** + * drm_crtc_vblank_reset - reset vblank state to off on a CRTC + * @crtc: CRTC in question + * + * Drivers can use this function to reset the vblank state to off at load time. + * Drivers should use this together with the drm_crtc_vblank_off() and + * drm_crtc_vblank_on() functions. The difference compared to + * drm_crtc_vblank_off() is that this function doesn't save the vblank counter + * and hence doesn't need to call any driver hooks. + */ +void drm_crtc_vblank_reset(struct drm_crtc *drm_crtc) +{ + struct drm_device *dev = drm_crtc->dev; + unsigned long irqflags; + int crtc = drm_crtc_index(drm_crtc); + struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; + + spin_lock_irqsave(&dev->vbl_lock, irqflags); + /* + * Prevent subsequent drm_vblank_get() from enabling the vblank + * interrupt by bumping the refcount. + */ + if (!vblank->inmodeset) { + atomic_inc(&vblank->refcount); + vblank->inmodeset = 1; + } + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); + + WARN_ON(!list_empty(&dev->vblank_event_list)); +} +EXPORT_SYMBOL(drm_crtc_vblank_reset); + /** * drm_vblank_on - enable vblank events on a CRTC * @dev: DRM device -- cgit v1.2.3 From 3bff93d64cf59f0e4547d5893adccc3925a14380 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Sun, 22 Feb 2015 15:11:19 +0100 Subject: drm/irq: Don't call ->get_vblank_counter directly from irq_uninstall/cleanup The pipe might already have been shut down, and then it's not a good idea to call hw accessor functions. Instead use the same logic as drm_vblank_off which has all the necessary checks to avoid troubles or inconsistency. Noticed by Imre while reviewing my patches to remove some sanity checks from ->get_vblank_counter. v2: Try harder. disable_and_save can still access the vblank stuff when vblank->enabled isn't set. It has to, since vlbank irq could be disable but the pipe is still on when being called from drm_vblank_off. But we still want to use that code for more code sharing. So add a check for vblank->enabled on top - if that's not set we shouldn't have anyone waiting for the vblank. If we have that's a pretty serious bug. The other issue that Imre spotted is drm_vblank_cleanup. That code again calls disable_and_save and so suffers from the same issues. But really drm_irq_uninstall should have cleaned that all up, so replace the code with WARN_ON. Note that we can't delete the timer cleanup since drivers aren't required to use drm_irq_install/uninstall, but can do their own irq handling. v3: Make it clear that all that gunk in drm_irq_uninstall is really just bandaids for UMS races between the irq/vblank code. In UMS userspace is in control of enabling/disabling interrupts in general and vblanks specifically. v4: Imre observed that KMS drivers all call drm_vblank_cleanup before drm_irq_uninstall (as they should), so again the code in there is dead for KMS (due to dev->num_crtcs == 0 after drm_vblank_cleanup). Or should be, so only WARN for KMS - with UMS userspace could try to do evil things. v5: After more discussion on irc we've gone back to v3: the del_timer_sync is required in all cases in drm_vblank_cleanup, but let's restrict the WARN_ON to kms drivers only. Imre was also concerned that bad things could happen without the disable_and_save call. But we immediately free vblank structures afterwards which makes the save useless. And drm_handle_vblank has a check for dev->num_crtcs to avoid surprises with ums. Cc: Imre Deak Reviewed-by: Imre Deak Acked-by: Dave Airlie Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_irq.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) (limited to 'drivers/gpu/drm/drm_irq.c') diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index e78a1f5cad9c..4bb7d34ff79e 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -269,7 +269,6 @@ static void vblank_disable_fn(unsigned long arg) void drm_vblank_cleanup(struct drm_device *dev) { int crtc; - unsigned long irqflags; /* Bail if the driver didn't call drm_vblank_init() */ if (dev->num_crtcs == 0) @@ -278,11 +277,10 @@ void drm_vblank_cleanup(struct drm_device *dev) for (crtc = 0; crtc < dev->num_crtcs; crtc++) { struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; - del_timer_sync(&vblank->disable_timer); + WARN_ON(vblank->enabled && + drm_core_check_feature(dev, DRIVER_MODESET)); - spin_lock_irqsave(&dev->vbl_lock, irqflags); - vblank_disable_and_save(dev, crtc); - spin_unlock_irqrestore(&dev->vbl_lock, irqflags); + del_timer_sync(&vblank->disable_timer); } kfree(dev->vblank); @@ -468,17 +466,23 @@ int drm_irq_uninstall(struct drm_device *dev) dev->irq_enabled = false; /* - * Wake up any waiters so they don't hang. + * Wake up any waiters so they don't hang. This is just to paper over + * isssues for UMS drivers which aren't in full control of their + * vblank/irq handling. KMS drivers must ensure that vblanks are all + * disabled when uninstalling the irq handler. */ if (dev->num_crtcs) { spin_lock_irqsave(&dev->vbl_lock, irqflags); for (i = 0; i < dev->num_crtcs; i++) { struct drm_vblank_crtc *vblank = &dev->vblank[i]; + if (!vblank->enabled) + continue; + + WARN_ON(drm_core_check_feature(dev, DRIVER_MODESET)); + + vblank_disable_and_save(dev, i); wake_up(&vblank->queue); - vblank->enabled = false; - vblank->last = - dev->driver->get_vblank_counter(dev, i); } spin_unlock_irqrestore(&dev->vbl_lock, irqflags); } -- cgit v1.2.3 From ee3c7795e54f405fbd278b17d2e0cd8ca1c92b69 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Sun, 22 Feb 2015 15:11:20 +0100 Subject: drm: WARN if drm_handle_vblank is called errornously KMS drivers are in full control of their irq and vblank handling, if they get a vblank interrupt before drm_vblank_init or after drm_vblank_cleanup that's just a driver bug. For ums driver there's only r128 and radeon which support vblank, and they call drm_vblank_init in their driver load functions. Which again means that userspace can do whatever it wants with interrupt, vblank structures will always be there. So this should never happen, let's catch driver issues with a WARN_ON. Motivated by some discussions with Imre. v2: Use WARN_ON_ONCE as suggested by Imre. Cc: Imre Deak Reviewed-by: Imre Deak Acked-by: Dave Airlie Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_irq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/gpu/drm/drm_irq.c') diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 4bb7d34ff79e..65a45ce70e2a 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1682,7 +1682,7 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc) struct timeval tvblank; unsigned long irqflags; - if (!dev->num_crtcs) + if (WARN_ON_ONCE(!dev->num_crtcs)) return false; if (WARN_ON(crtc >= dev->num_crtcs)) -- cgit v1.2.3 From 30b79f062ea0ab5df3afdfeb503794a5b4beb8f1 Mon Sep 17 00:00:00 2001 From: Damien Lespiau Date: Fri, 13 Feb 2015 15:53:31 +0000 Subject: drm: Fix drm_crtc_vblank_get() documentation drm_crtc_vblank_get() is the new drm_vblank_get(), not the new drm_vblank_off(). Signed-off-by: Damien Lespiau Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_irq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/gpu/drm/drm_irq.c') diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 10574a0c3a55..c9f5453f20e7 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1052,7 +1052,7 @@ EXPORT_SYMBOL(drm_vblank_get); * Acquire a reference count on vblank events to avoid having them disabled * while in use. * - * This is the native kms version of drm_vblank_off(). + * This is the native kms version of drm_vblank_get(). * * Returns: * Zero on success, nonzero on failure. -- cgit v1.2.3 From fdb68e09bbb1c981f24608d7022c7d93cc47b326 Mon Sep 17 00:00:00 2001 From: Mario Kleiner Date: Tue, 7 Apr 2015 06:31:09 +0200 Subject: drm: Zero out invalid vblank timestamp in drm_update_vblank_count. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since commit 844b03f27739135fe1fed2fef06da0ffc4c7a081 we make sure that after vblank irq off, we return the last valid (vblank count, vblank timestamp) pair to clients, e.g., during modesets, which is good. An overlooked side effect of that commit for kms drivers without support for precise vblank timestamping is that at vblank irq enable, when we update the vblank counter from the hw counter, we can't update the corresponding vblank timestamp, so now we have a totally mismatched timestamp for the new count to confuse clients. Restore old client visible behaviour from before Linux 3.17, but zero out the timestamp at vblank counter update (instead of disable as in original implementation) if we can't generate a meaningful timestamp immediately for the new vblank counter. This will fix this regression, so callers know they need to retry again later if they need a valid timestamp, but at the same time preserves the improvements made in the commit mentioned above. Signed-off-by: Mario Kleiner Cc: #v3.17+ Cc: Ville Syrjälä Cc: Daniel Vetter Signed-off-by: Dave Airlie --- drivers/gpu/drm/drm_irq.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'drivers/gpu/drm/drm_irq.c') diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index c8a34476570a..af9662e58272 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -131,12 +131,11 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc) /* Reinitialize corresponding vblank timestamp if high-precision query * available. Skip this step if query unsupported or failed. Will - * reinitialize delayed at next vblank interrupt in that case. + * reinitialize delayed at next vblank interrupt in that case and + * assign 0 for now, to mark the vblanktimestamp as invalid. */ - if (rc) { - tslot = atomic_read(&vblank->count) + diff; - vblanktimestamp(dev, crtc, tslot) = t_vblank; - } + tslot = atomic_read(&vblank->count) + diff; + vblanktimestamp(dev, crtc, tslot) = rc ? t_vblank : (struct timeval) {0, 0}; smp_mb__before_atomic(); atomic_add(diff, &vblank->count); -- cgit v1.2.3