From patchwork Sun Mar 26 08:46:37 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chris Wilson X-Patchwork-Id: 9644977 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id ED5E7601E9 for ; Sun, 26 Mar 2017 08:46:42 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E4BD52522B for ; Sun, 26 Mar 2017 08:46:42 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D99F9262FF; Sun, 26 Mar 2017 08:46:42 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 7CF432522B for ; Sun, 26 Mar 2017 08:46:42 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id CA8456E064; Sun, 26 Mar 2017 08:46:41 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-wm0-x241.google.com (mail-wm0-x241.google.com [IPv6:2a00:1450:400c:c09::241]) by gabe.freedesktop.org (Postfix) with ESMTPS id D53406E064 for ; Sun, 26 Mar 2017 08:46:40 +0000 (UTC) Received: by mail-wm0-x241.google.com with SMTP id n11so5493270wma.0 for ; Sun, 26 Mar 2017 01:46:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:from:to:cc:subject:date:message-id:in-reply-to:references; bh=QHMsEeiW9iAZbSZ/pNKNDt90CO8T597O59AjCWoWtBk=; b=cPWJIdLBLFAyr1WPI3B1qvJ4qAdIr1GfsqKz5DW9gjgQnqrGxmM0Z2jR9O4QUZCKqE t2Uw7VnWQ7IuLfcrvaldc1kgkjVpEiI4ZuS09vC1t9nISbwthidS4JwbzuM6HazTaEUS RihqSGeLQSVD1kRP/TG/nAWelfHGZZ18i9keMmY3aGuQ60y+jF7vL1vOQt1pN1wlqj3c 1/ULNEDTHxFZQbYuo5m1PjczGx0eOScXC7IoDKwTJzJjVN50k6oj8XAJLIRn74d2z3sE NzBs2F73cRhDssRIcfy8lgYmxIV3ktI3SmuzZbU4kmQkkCCNtO52r4XLRaXo9wQzLRYv aQ/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:from:to:cc:subject:date:message-id :in-reply-to:references; bh=QHMsEeiW9iAZbSZ/pNKNDt90CO8T597O59AjCWoWtBk=; b=sM39DXV6VGbwqMw3+ngjQLVQKWCDQMrmF3ety6Bke276B33NcmLXF85SvmEhpI6kR2 IR/nDDW2203KFN7ndRpCgxV5gyOCGHx2BAk3U11NdYKkZwlwpK47G1ZrVUTo/XX2F/bQ KMoT/uefRE6JiS33Z6+J2tpJZrPEeG2VGPHEOg/FpI2B8pgTXD2NEs+dieyiQWrYbEgO zB91lW6QRfq+BwfGHF3M9tA/uvoZDmZ5iNS/LuVYQ7el8SEQUX2gwOJRYZK2al0vpoID 6XSfp9zrh6J95q5UJcj2e3pdTkkmSmoj7pltnlQo8bVe6uX0FbfDH99FGDRk25tcd6Yq aqcQ== X-Gm-Message-State: AFeK/H36htnay3jwzpUNdxB7mKsG+/0oovaFTTBKODkNOs0j6jStfuokcOa/XmFlmGDtCg== X-Received: by 10.28.197.133 with SMTP id v127mr4464301wmf.120.1490517999501; Sun, 26 Mar 2017 01:46:39 -0700 (PDT) Received: from haswell.alporthouse.com ([78.156.65.138]) by smtp.gmail.com with ESMTPSA id 140sm9577593wmg.18.2017.03.26.01.46.38 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 26 Mar 2017 01:46:38 -0700 (PDT) From: Chris Wilson To: intel-gfx@lists.freedesktop.org Date: Sun, 26 Mar 2017 09:46:37 +0100 Message-Id: <20170326084637.13394-1-chris@chris-wilson.co.uk> X-Mailer: git-send-email 2.11.0 In-Reply-To: <20170326084420.3231-1-chris@chris-wilson.co.uk> References: <20170326084420.3231-1-chris@chris-wilson.co.uk> Cc: "# v4 . 10+" Subject: [Intel-gfx] [PATCH] drm/i915: Keep all engine locks across scheduling X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP Unlocking is dangerous. In this case we combine an early update to the out-of-queue request, because we know that it will be inserted into the correct FIFO priority-ordered slot when it becomes ready in the future. However, given sufficient enthusiasm, it may become ready as we are continuing to reschedule, and so may gazump the FIFO if we have since dropped its spinlock. The result is that it may be executed too early, before its dependees. Fixes: 20311bd35060 ("drm/i915/scheduler: Execute requests in order of priorities") Testcase: igt/gem_exec_whisper Signed-off-by: Chris Wilson Cc: Tvrtko Ursulin Cc: # v4.10+ --- drivers/gpu/drm/i915/intel_lrc.c | 54 +++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index dd0e9d587852..3fdabba0a32d 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -658,30 +658,47 @@ static void execlists_submit_request(struct drm_i915_gem_request *request) spin_unlock_irqrestore(&engine->timeline->lock, flags); } -static struct intel_engine_cs * -pt_lock_engine(struct i915_priotree *pt, struct intel_engine_cs *locked) +static inline struct intel_engine_cs * +pt_lock_engine(struct i915_priotree *pt, unsigned long *locked) { - struct intel_engine_cs *engine; - - engine = container_of(pt, - struct drm_i915_gem_request, - priotree)->engine; - if (engine != locked) { - if (locked) - spin_unlock_irq(&locked->timeline->lock); - spin_lock_irq(&engine->timeline->lock); - } + struct intel_engine_cs *engine = + container_of(pt, struct drm_i915_gem_request, priotree)->engine; + + /* Locking the engines in a random order will rightfully trigger a + * spasm in lockdep. However, we can ignore lockdep (by marking each + * as a seperate nesting) so long as we never nest the + * engine->timeline->lock elsewhere. Also the number of nesting + * subclasses is severely limited (7) which is going to cause an + * issue at some point. + * BUILD_BUG_ON(I915_NUM_ENGINES >= MAX_LOCKDEP_SUBCLASSES); + */ + if (!__test_and_set_bit(engine->id, locked)) + spin_lock_nested(&engine->timeline->lock, + hweight_long(*locked)); return engine; } +static void +unlock_engines(struct drm_i915_private *i915, unsigned long locked) +{ + struct intel_engine_cs *engine; + unsigned long tmp; + + for_each_engine_masked(engine, i915, locked, tmp) + spin_unlock(&engine->timeline->lock); +} + static void execlists_schedule(struct drm_i915_gem_request *request, int prio) { - struct intel_engine_cs *engine = NULL; + struct intel_engine_cs *engine; struct i915_dependency *dep, *p; struct i915_dependency stack; + unsigned long locked = 0; LIST_HEAD(dfs); + BUILD_BUG_ON(I915_NUM_ENGINES > BITS_PER_LONG); + if (prio <= READ_ONCE(request->priotree.priority)) return; @@ -691,6 +708,9 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio) stack.signaler = &request->priotree; list_add(&stack.dfs_link, &dfs); + GEM_BUG_ON(irqs_disabled()); + local_irq_disable(); + /* Recursively bump all dependent priorities to match the new request. * * A naive approach would be to use recursion: @@ -719,7 +739,7 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio) if (!RB_EMPTY_NODE(&pt->node)) continue; - engine = pt_lock_engine(pt, engine); + engine = pt_lock_engine(pt, &locked); /* If it is not already in the rbtree, we can update the * priority inplace and skip over it (and its dependencies) @@ -737,7 +757,7 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio) INIT_LIST_HEAD(&dep->dfs_link); - engine = pt_lock_engine(pt, engine); + engine = pt_lock_engine(pt, &locked); if (prio <= pt->priority) continue; @@ -750,8 +770,8 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio) engine->execlist_first = &pt->node; } - if (engine) - spin_unlock_irq(&engine->timeline->lock); + unlock_engines(request->i915, locked); + local_irq_enable(); /* XXX Do we need to preempt to make room for us and our deps? */ }