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: Does just some prep work on per-crtc data and grabs a ref on the backing storage, then calls down into omap_gem_op_async which does some nicely-protected async callback stuff, or directly calls the passed-in page_flip_cb. That seems to lock most of the stuff it touches properly, safe for the eventually called omap_plane_dpms, which updates modeset state. Which will be a problem if this is called asynchronously, since the sync_op waiter callback code in omap_gem.c does not seem to take the right modeset locks. So looks a bit racy already with the old locking, and no worse off with the new per-crtc locks.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- 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 6dd441c..36c75e6 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3716,12 +3716,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 @@ -3803,7 +3803,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; }