From patchwork Wed Dec 12 13:07:16 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 1866341 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by patchwork2.kernel.org (Postfix) with ESMTP id 0159EDF2EE for ; Wed, 12 Dec 2012 13:49:51 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id D7D55E65BB for ; Wed, 12 Dec 2012 05:49:50 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-ee0-f49.google.com (mail-ee0-f49.google.com [74.125.83.49]) by gabe.freedesktop.org (Postfix) with ESMTP id 6623DE6141 for ; Wed, 12 Dec 2012 05:07:59 -0800 (PST) Received: by mail-ee0-f49.google.com with SMTP id c4so424582eek.36 for ; Wed, 12 Dec 2012 05:07:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=from:to:cc:subject:date:message-id:x-mailer:in-reply-to:references; bh=ijZex+dI0qPWcTdRbxtrdoOqoXzv6tBhL9EHFHtJOuc=; b=BLToOXF7mfwhvoBTYeU1c/+HQh4VoXdstObq0VfKPj/YDXneJiVbyN3Yi3g7S4P6ar RSLoHjvNFHOzhuigOv6GD5ukq6RHyJ2YKoOTUV/WCqxWSgNONYp+4R62sCoU1FBa7feC mdMH3zMpKdUM8VOFBdQue+IM56B+Jvz09Pjmk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=from:to:cc:subject:date:message-id:x-mailer:in-reply-to:references :x-gm-message-state; bh=ijZex+dI0qPWcTdRbxtrdoOqoXzv6tBhL9EHFHtJOuc=; b=SjMX/5zJwRdBtHdhnc5fandcyPNJ4CXTld4eHofoeW/6Jztc34/+rAzOUi9v+aalzK fO9vNnOEhW1fWYZS1IrPHU4AM83X1Eby0xmVyoArfE9UlChC8Nv7TwjC0ESOI9stNwrR OCj2oZBra35n9JM5KSTj15jFIFqS16npmaniuSY7txxmOQYAv5TfaY/oCykPA1qb2Qiz z9qyCoTBEZQkb0c/32zjZUzGU/tw0v9/ojV8Wsdntw51jUMsljyznXsGZHMcs1eYulj3 s7KjBNopY62WmQhUoffuD73gvJDILAHkMxvofoz0gW05t80aRNeeHZQ09AmFeLxM+2mc EXuw== Received: by 10.14.216.70 with SMTP id f46mr2714715eep.12.1355317678533; Wed, 12 Dec 2012 05:07:58 -0800 (PST) Received: from biers.ffwll.local (178-83-130-250.dynamic.hispeed.ch. [178.83.130.250]) by mx.google.com with ESMTPS id r1sm55868541eeo.2.2012.12.12.05.07.57 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 12 Dec 2012 05:07:57 -0800 (PST) From: Daniel Vetter To: DRI Development Subject: [PATCH 36/37] drm: only grab the crtc lock for pageflips Date: Wed, 12 Dec 2012 14:07:16 +0100 Message-Id: <1355317637-16742-37-git-send-email-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 1.7.10.4 In-Reply-To: <1355317637-16742-1-git-send-email-daniel.vetter@ffwll.ch> References: <1355317637-16742-1-git-send-email-daniel.vetter@ffwll.ch> X-Gm-Message-State: ALoCoQlHP011pXCuagKoS48GhaJJNc8v0bkUzqr9u/q5MFF13HtEd4TjqP718hKvgFp7TLpaDACe Cc: Nouveau Dev , Intel Graphics Development , Radeon Dev , Daniel Vetter X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org 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 --- 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; }