From patchwork Wed Dec 12 13:07:12 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 1867501 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 31DC2DF215 for ; Wed, 12 Dec 2012 14:45:37 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 061BBE6614 for ; Wed, 12 Dec 2012 06:45:37 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-ea0-f177.google.com (mail-ea0-f177.google.com [209.85.215.177]) by gabe.freedesktop.org (Postfix) with ESMTP id 683A3E653C for ; Wed, 12 Dec 2012 05:16:10 -0800 (PST) Received: by mail-ea0-f177.google.com with SMTP id c10so222421eaa.36 for ; Wed, 12 Dec 2012 05:16:09 -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=48jGGgUDKDx+CHS8QAe8pDIlNTuOBHIbumPsySEfX9E=; b=hZxWjruCRpKm8iyKlK0zgenogI4zwVLnfFuHNuDHnknBfey42DyaM5CWRbcJ0ZUuoD SE/myCIvjdXoWo632Gqyfwu23B5mZEyqwrHIDPGDYpzl7cRzlk4+8Qek4ueU9Qoxe9Ui DBL39/UtE+0Iibcfp3ebfisPmu7JvJEUy/4VI= 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=48jGGgUDKDx+CHS8QAe8pDIlNTuOBHIbumPsySEfX9E=; b=X71fKwsU0tNmBD3CDDDFNlwYvCt0bs0noCZhinCXNc6kh2v15DjZr1t1cvq6pzzyT2 /Z/s1Vf6tff93Jx8GzxSfnqNrw5FjrwymMz/aoAEfQMW2vY11MJyUTj0MovRa218pV3G sSDwipLEBEZaZE/NwvHQL4WrL2B/HLxXSBjF+cGS6gwxMAoOoZGzoieXU1rpkd54SLzU poE8EodqS8xCEZGlFupqWqgPtiREYV4UVWfHZpAiNuLf6KvSj/GZ1MYr8fU++khgW7W+ PeV/phVg+gQBbZd1S1e21fif6+9Z99GptFsXd+aMYSj9G9kpIU0JTyUixujtcTJzK9wL 2wbw== Received: by 10.14.174.198 with SMTP id x46mr2713083eel.23.1355317674500; Wed, 12 Dec 2012 05:07:54 -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.53 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 12 Dec 2012 05:07:54 -0800 (PST) From: Daniel Vetter To: DRI Development Subject: [PATCH 32/37] drm: optimize drm_framebuffer_remove Date: Wed, 12 Dec 2012 14:07:12 +0100 Message-Id: <1355317637-16742-33-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: ALoCoQkVtkHPUjqqFivRjEg82zRUp8R88zb0lT1Wiyoy4nRKYFC9vd1xD5rwyOAQzeNHLugEhO4P 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 Now that all framebuffer usage is properly refcounted, we are no longer required to hold the modeset locks while dropping the last reference. Hence implemented a fastpath which avoids the potential stalls associated with grabbing mode_config.lock for the case where there's no other reference around. Explain in a big comment why it is safe. Also update kerneldocs with the new locking rules around drm_framebuffer_remove. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_crtc.c | 72 +++++++++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 6562eba..6dd441c 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -516,7 +516,11 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup); * * Scans all the CRTCs and planes in @dev's mode_config. If they're * using @fb, removes it, setting it to NULL. Then drops the reference to the - * passed-in framebuffer. + * passed-in framebuffer. Might take the modeset locks. + * + * Note that this function optimizes the cleanup away if the caller holds the + * last reference to the framebuffer. It is also guaranteed to not take the + * modeset locks in this case. */ void drm_framebuffer_remove(struct drm_framebuffer *fb) { @@ -526,33 +530,51 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) struct drm_mode_set set; int ret; - WARN_ON(!drm_modeset_is_locked(dev)); WARN_ON(!list_empty(&fb->filp_head)); - /* remove from any CRTC */ - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { - if (crtc->fb == fb) { - /* should turn off the crtc */ - memset(&set, 0, sizeof(struct drm_mode_set)); - set.crtc = crtc; - set.fb = NULL; - ret = drm_mode_set_config_internal(&set); - if (ret) - DRM_ERROR("failed to reset crtc %p when fb was deleted\n", crtc); + /* + * drm ABI mandates that we remove any deleted framebuffers from active + * useage. But since most sane clients only remove framebuffers they no + * longer need, try to optimize this away. + * + * Since we're holding a reference ourselves, observing a refcount of 1 + * means that we're the last holder and can skip it. Also, the refcount + * can never increase from 1 again, so we don't need any barriers or + * locks. + * + * Note that userspace could try to race with use and instate a new + * usage _after_ we've cleared all current ones. End result will be an + * in-use fb with fb-id == 0. Userspace is allowed to shoot its own foot + * in this manner. + */ + if (atomic_read(&fb->refcount.refcount) > 1) { + drm_modeset_lock_all(dev); + /* remove from any CRTC */ + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + if (crtc->fb == fb) { + /* should turn off the crtc */ + memset(&set, 0, sizeof(struct drm_mode_set)); + set.crtc = crtc; + set.fb = NULL; + ret = drm_mode_set_config_internal(&set); + if (ret) + DRM_ERROR("failed to reset crtc %p when fb was deleted\n", crtc); + } } - } - list_for_each_entry(plane, &dev->mode_config.plane_list, head) { - if (plane->fb == fb) { - /* should turn off the crtc */ - ret = plane->funcs->disable_plane(plane); - if (ret) - DRM_ERROR("failed to disable plane with busy fb\n"); - /* disconnect the plane from the fb and crtc: */ - __drm_framebuffer_unreference(plane->fb); - plane->fb = NULL; - plane->crtc = NULL; + list_for_each_entry(plane, &dev->mode_config.plane_list, head) { + if (plane->fb == fb) { + /* should turn off the crtc */ + ret = plane->funcs->disable_plane(plane); + if (ret) + DRM_ERROR("failed to disable plane with busy fb\n"); + /* disconnect the plane from the fb and crtc: */ + __drm_framebuffer_unreference(plane->fb); + plane->fb = NULL; + plane->crtc = NULL; + } } + drm_modeset_unlock_all(dev); } drm_framebuffer_unreference(fb); @@ -2537,9 +2559,7 @@ int drm_mode_rmfb(struct drm_device *dev, mutex_unlock(&dev->mode_config.fb_lock); mutex_unlock(&file_priv->fbs_lock); - drm_modeset_lock_all(dev); drm_framebuffer_remove(fb); - drm_modeset_unlock_all(dev); return 0; @@ -2687,9 +2707,7 @@ void drm_fb_release(struct drm_file *priv) list_del_init(&fb->filp_head); /* This will also drop the fpriv->fbs reference. */ - drm_modeset_lock_all(dev); drm_framebuffer_remove(fb); - drm_modeset_unlock_all(dev); } mutex_unlock(&priv->fbs_lock); }