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 <imre.deak@intel.com>
Reviewed-by: Imre Deak <imre.deak@intel.com>
Acked-by: Dave Airlie <airlied@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index e78a1f5cad9cdb189d597f35e64aedc1d0f80261..4bb7d34ff79eb4c8ae5fe949a1bf1d860bb4de68 100644 (file)
--- 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);
        }