From b4d5e7d1dbdd6a758a4f7717beef7bd6b007bd66 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Tue, 11 Dec 2012 16:59:31 +0100 Subject: [PATCH] drm: only grab the crtc lock for pageflips The pagelip ioctl itself is rather simply, so the hard work for this patch is auditing all the drivers: - exynos: Pageflip is protect with dev->struct_mutex and ... synchronous. But nothing fancy going on, besides a check whether the crtc is enabled, which should probably be somewhere in the drm core so that we have unified behaviour across all drivers. - i915: hw-state is protected with dev->struct_mutex, the delayed unpin work together with the other stuff the pageflip complete irq handler needs is protected by the event_lock spinlock. - nouveau: With the pin/unpin functions fixed, everything looks safe: A bit of ttm wrestling and refcounting, and a few channel accesses. The later are either already proteced sufficiently, or are now safe with the channel locking introduced to make cursor updates safe. - radeon: The irq_get/put functions look a bit race, since the atomic_inc/dec isn't protect with locks. Otoh they're all per-crtc, so we should be safe with per-crtc locking from the drm core. Then there's tons of per-crtc register access, which could potentially go through the indirect reg acces. But that's fixed to make cursor updates concurrent. Bookeeping for the drm even is also protected with the even_lock, which also protects against the pageflip irq handler since radeon hw seems to have no way to queue these up asynchronously. Otherwise just a bit of ttm-based buffer handling and fencing, which is now safe with the previous patch to hold bdev->fence_lock while grabbing the ttm fence. - shmob: Only one crtc. That's an easy one ... - vmwgfx: As usual a bit special with tons different things: - Flippable check using is_implicit and num_implicit. Changes to those seem to be nicely covered with the global modeset lock, so we should be fine. - Some dirty cliprect handling stuff, or at least that is my guess. Looks like it's fine since either it's per-crtc, invariant or (like the execbuf stuff launched) protected otherwise. - Adding the actual flip to the fence_event list. On a quick look this seems to have solid locking in place, too. ... but generally this is all way over my head. - imx: Impressive display of races between the page_flip implementation and the irq handler. Also, ipu_drm_set_base which gets eventually called from the irq handler to update the display base isn't really protected against concurrent set_config calls from process context. In any case, going for per-crtc locking won't make this worse, so nothing to do. - omap: The new async callback code merged into 3.8 seems to have solid locking in place, and there doesn't seem to be any shared state at risk. Especially since the callbacks still use modeset_lock_all and are so not converted. v2: Update omapdrm analysis to 3.8 code per the discussion with Rob Clark. Reviewed-by: Rob Clark Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_crtc.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 9492e7ec9d9d..fd3e9a13b04f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3699,12 +3699,12 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, page_flip->reserved != 0) return -EINVAL; - drm_modeset_lock_all(dev); obj = drm_mode_object_find(dev, page_flip->crtc_id, DRM_MODE_OBJECT_CRTC); if (!obj) - goto out; + return -EINVAL; crtc = obj_to_crtc(obj); + mutex_lock(&crtc->mutex); if (crtc->fb == NULL) { /* The framebuffer is currently unbound, presumably * due to a hotplug event, that userspace has not @@ -3786,7 +3786,8 @@ out: drm_framebuffer_unreference(fb); if (old_fb) drm_framebuffer_unreference(old_fb); - drm_modeset_unlock_all(dev); + mutex_unlock(&crtc->mutex); + return ret; } -- 2.20.1