From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f68.google.com ([74.125.82.68]:34472 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751264AbdCZIqz (ORCPT ); Sun, 26 Mar 2017 04:46:55 -0400 Received: by mail-wm0-f68.google.com with SMTP id u132so5489614wmg.1 for ; Sun, 26 Mar 2017 01:46:40 -0700 (PDT) From: Chris Wilson To: intel-gfx@lists.freedesktop.org Cc: Chris Wilson , Tvrtko Ursulin , "# v4 . 10+" Subject: [PATCH] drm/i915: Keep all engine locks across scheduling Date: Sun, 26 Mar 2017 09:46:37 +0100 Message-Id: <20170326084637.13394-1-chris@chris-wilson.co.uk> In-Reply-To: <20170326084420.3231-1-chris@chris-wilson.co.uk> References: <20170326084420.3231-1-chris@chris-wilson.co.uk> Sender: stable-owner@vger.kernel.org List-ID: 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? */ } -- 2.11.0 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: [PATCH] drm/i915: Keep all engine locks across scheduling Date: Sun, 26 Mar 2017 09:46:37 +0100 Message-ID: <20170326084637.13394-1-chris@chris-wilson.co.uk> References: <20170326084420.3231-1-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: 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) In-Reply-To: <20170326084420.3231-1-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: intel-gfx@lists.freedesktop.org Cc: "# v4 . 10+" List-Id: intel-gfx@lists.freedesktop.org VW5sb2NraW5nIGlzIGRhbmdlcm91cy4gSW4gdGhpcyBjYXNlIHdlIGNvbWJpbmUgYW4gZWFybHkg dXBkYXRlIHRvIHRoZQpvdXQtb2YtcXVldWUgcmVxdWVzdCwgYmVjYXVzZSB3ZSBrbm93IHRoYXQg aXQgd2lsbCBiZSBpbnNlcnRlZCBpbnRvIHRoZQpjb3JyZWN0IEZJRk8gcHJpb3JpdHktb3JkZXJl ZCBzbG90IHdoZW4gaXQgYmVjb21lcyByZWFkeSBpbiB0aGUgZnV0dXJlLgpIb3dldmVyLCBnaXZl biBzdWZmaWNpZW50IGVudGh1c2lhc20sIGl0IG1heSBiZWNvbWUgcmVhZHkgYXMgd2UgYXJlCmNv bnRpbnVpbmcgdG8gcmVzY2hlZHVsZSwgYW5kIHNvIG1heSBnYXp1bXAgdGhlIEZJRk8gaWYgd2Ug aGF2ZSBzaW5jZQpkcm9wcGVkIGl0cyBzcGlubG9jay4gVGhlIHJlc3VsdCBpcyB0aGF0IGl0IG1h eSBiZSBleGVjdXRlZCB0b28gZWFybHksCmJlZm9yZSBpdHMgZGVwZW5kZWVzLgoKRml4ZXM6IDIw MzExYmQzNTA2MCAoImRybS9pOTE1L3NjaGVkdWxlcjogRXhlY3V0ZSByZXF1ZXN0cyBpbiBvcmRl ciBvZiBwcmlvcml0aWVzIikKVGVzdGNhc2U6IGlndC9nZW1fZXhlY193aGlzcGVyClNpZ25lZC1v ZmYtYnk6IENocmlzIFdpbHNvbiA8Y2hyaXNAY2hyaXMtd2lsc29uLmNvLnVrPgpDYzogVHZydGtv IFVyc3VsaW4gPHR2cnRrby51cnN1bGluQGludGVsLmNvbT4KQ2M6IDxzdGFibGVAdmdlci5rZXJu ZWwub3JnPiAjIHY0LjEwKwotLS0KIGRyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2xyYy5jIHwg NTQgKysrKysrKysrKysrKysrKysrKysrKysrKysrLS0tLS0tLS0tLS0tLQogMSBmaWxlIGNoYW5n ZWQsIDM3IGluc2VydGlvbnMoKyksIDE3IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL2RyaXZl cnMvZ3B1L2RybS9pOTE1L2ludGVsX2xyYy5jIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxf bHJjLmMKaW5kZXggZGQwZTlkNTg3ODUyLi4zZmRhYmJhMGEzMmQgMTAwNjQ0Ci0tLSBhL2RyaXZl cnMvZ3B1L2RybS9pOTE1L2ludGVsX2xyYy5jCisrKyBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2lu dGVsX2xyYy5jCkBAIC02NTgsMzAgKzY1OCw0NyBAQCBzdGF0aWMgdm9pZCBleGVjbGlzdHNfc3Vi bWl0X3JlcXVlc3Qoc3RydWN0IGRybV9pOTE1X2dlbV9yZXF1ZXN0ICpyZXF1ZXN0KQogCXNwaW5f dW5sb2NrX2lycXJlc3RvcmUoJmVuZ2luZS0+dGltZWxpbmUtPmxvY2ssIGZsYWdzKTsKIH0KIAot c3RhdGljIHN0cnVjdCBpbnRlbF9lbmdpbmVfY3MgKgotcHRfbG9ja19lbmdpbmUoc3RydWN0IGk5 MTVfcHJpb3RyZWUgKnB0LCBzdHJ1Y3QgaW50ZWxfZW5naW5lX2NzICpsb2NrZWQpCitzdGF0aWMg aW5saW5lIHN0cnVjdCBpbnRlbF9lbmdpbmVfY3MgKgorcHRfbG9ja19lbmdpbmUoc3RydWN0IGk5 MTVfcHJpb3RyZWUgKnB0LCB1bnNpZ25lZCBsb25nICpsb2NrZWQpCiB7Ci0Jc3RydWN0IGludGVs X2VuZ2luZV9jcyAqZW5naW5lOwotCi0JZW5naW5lID0gY29udGFpbmVyX29mKHB0LAotCQkJICAg ICAgc3RydWN0IGRybV9pOTE1X2dlbV9yZXF1ZXN0LAotCQkJICAgICAgcHJpb3RyZWUpLT5lbmdp bmU7Ci0JaWYgKGVuZ2luZSAhPSBsb2NrZWQpIHsKLQkJaWYgKGxvY2tlZCkKLQkJCXNwaW5fdW5s b2NrX2lycSgmbG9ja2VkLT50aW1lbGluZS0+bG9jayk7Ci0JCXNwaW5fbG9ja19pcnEoJmVuZ2lu ZS0+dGltZWxpbmUtPmxvY2spOwotCX0KKwlzdHJ1Y3QgaW50ZWxfZW5naW5lX2NzICplbmdpbmUg PQorCQljb250YWluZXJfb2YocHQsIHN0cnVjdCBkcm1faTkxNV9nZW1fcmVxdWVzdCwgcHJpb3Ry ZWUpLT5lbmdpbmU7CisKKwkvKiBMb2NraW5nIHRoZSBlbmdpbmVzIGluIGEgcmFuZG9tIG9yZGVy IHdpbGwgcmlnaHRmdWxseSB0cmlnZ2VyIGEKKwkgKiBzcGFzbSBpbiBsb2NrZGVwLiBIb3dldmVy LCB3ZSBjYW4gaWdub3JlIGxvY2tkZXAgKGJ5IG1hcmtpbmcgZWFjaAorCSAqIGFzIGEgc2VwZXJh dGUgbmVzdGluZykgc28gbG9uZyBhcyB3ZSBuZXZlciBuZXN0IHRoZQorCSAqIGVuZ2luZS0+dGlt ZWxpbmUtPmxvY2sgZWxzZXdoZXJlLiBBbHNvIHRoZSBudW1iZXIgb2YgbmVzdGluZworCSAqIHN1 YmNsYXNzZXMgaXMgc2V2ZXJlbHkgbGltaXRlZCAoNykgd2hpY2ggaXMgZ29pbmcgdG8gY2F1c2Ug YW4KKwkgKiBpc3N1ZSBhdCBzb21lIHBvaW50LgorCSAqIEJVSUxEX0JVR19PTihJOTE1X05VTV9F TkdJTkVTID49IE1BWF9MT0NLREVQX1NVQkNMQVNTRVMpOworCSAqLworCWlmICghX190ZXN0X2Fu ZF9zZXRfYml0KGVuZ2luZS0+aWQsIGxvY2tlZCkpCisJCXNwaW5fbG9ja19uZXN0ZWQoJmVuZ2lu ZS0+dGltZWxpbmUtPmxvY2ssCisJCQkJIGh3ZWlnaHRfbG9uZygqbG9ja2VkKSk7CiAKIAlyZXR1 cm4gZW5naW5lOwogfQogCitzdGF0aWMgdm9pZAordW5sb2NrX2VuZ2luZXMoc3RydWN0IGRybV9p OTE1X3ByaXZhdGUgKmk5MTUsIHVuc2lnbmVkIGxvbmcgbG9ja2VkKQoreworCXN0cnVjdCBpbnRl bF9lbmdpbmVfY3MgKmVuZ2luZTsKKwl1bnNpZ25lZCBsb25nIHRtcDsKKworCWZvcl9lYWNoX2Vu Z2luZV9tYXNrZWQoZW5naW5lLCBpOTE1LCBsb2NrZWQsIHRtcCkKKwkJc3Bpbl91bmxvY2soJmVu Z2luZS0+dGltZWxpbmUtPmxvY2spOworfQorCiBzdGF0aWMgdm9pZCBleGVjbGlzdHNfc2NoZWR1 bGUoc3RydWN0IGRybV9pOTE1X2dlbV9yZXF1ZXN0ICpyZXF1ZXN0LCBpbnQgcHJpbykKIHsKLQlz dHJ1Y3QgaW50ZWxfZW5naW5lX2NzICplbmdpbmUgPSBOVUxMOworCXN0cnVjdCBpbnRlbF9lbmdp bmVfY3MgKmVuZ2luZTsKIAlzdHJ1Y3QgaTkxNV9kZXBlbmRlbmN5ICpkZXAsICpwOwogCXN0cnVj dCBpOTE1X2RlcGVuZGVuY3kgc3RhY2s7CisJdW5zaWduZWQgbG9uZyBsb2NrZWQgPSAwOwogCUxJ U1RfSEVBRChkZnMpOwogCisJQlVJTERfQlVHX09OKEk5MTVfTlVNX0VOR0lORVMgPiBCSVRTX1BF Ul9MT05HKTsKKwogCWlmIChwcmlvIDw9IFJFQURfT05DRShyZXF1ZXN0LT5wcmlvdHJlZS5wcmlv cml0eSkpCiAJCXJldHVybjsKIApAQCAtNjkxLDYgKzcwOCw5IEBAIHN0YXRpYyB2b2lkIGV4ZWNs aXN0c19zY2hlZHVsZShzdHJ1Y3QgZHJtX2k5MTVfZ2VtX3JlcXVlc3QgKnJlcXVlc3QsIGludCBw cmlvKQogCXN0YWNrLnNpZ25hbGVyID0gJnJlcXVlc3QtPnByaW90cmVlOwogCWxpc3RfYWRkKCZz dGFjay5kZnNfbGluaywgJmRmcyk7CiAKKwlHRU1fQlVHX09OKGlycXNfZGlzYWJsZWQoKSk7CisJ bG9jYWxfaXJxX2Rpc2FibGUoKTsKKwogCS8qIFJlY3Vyc2l2ZWx5IGJ1bXAgYWxsIGRlcGVuZGVu dCBwcmlvcml0aWVzIHRvIG1hdGNoIHRoZSBuZXcgcmVxdWVzdC4KIAkgKgogCSAqIEEgbmFpdmUg YXBwcm9hY2ggd291bGQgYmUgdG8gdXNlIHJlY3Vyc2lvbjoKQEAgLTcxOSw3ICs3MzksNyBAQCBz dGF0aWMgdm9pZCBleGVjbGlzdHNfc2NoZWR1bGUoc3RydWN0IGRybV9pOTE1X2dlbV9yZXF1ZXN0 ICpyZXF1ZXN0LCBpbnQgcHJpbykKIAkJaWYgKCFSQl9FTVBUWV9OT0RFKCZwdC0+bm9kZSkpCiAJ CQljb250aW51ZTsKIAotCQllbmdpbmUgPSBwdF9sb2NrX2VuZ2luZShwdCwgZW5naW5lKTsKKwkJ ZW5naW5lID0gcHRfbG9ja19lbmdpbmUocHQsICZsb2NrZWQpOwogCiAJCS8qIElmIGl0IGlzIG5v dCBhbHJlYWR5IGluIHRoZSByYnRyZWUsIHdlIGNhbiB1cGRhdGUgdGhlCiAJCSAqIHByaW9yaXR5 IGlucGxhY2UgYW5kIHNraXAgb3ZlciBpdCAoYW5kIGl0cyBkZXBlbmRlbmNpZXMpCkBAIC03Mzcs NyArNzU3LDcgQEAgc3RhdGljIHZvaWQgZXhlY2xpc3RzX3NjaGVkdWxlKHN0cnVjdCBkcm1faTkx NV9nZW1fcmVxdWVzdCAqcmVxdWVzdCwgaW50IHByaW8pCiAKIAkJSU5JVF9MSVNUX0hFQUQoJmRl cC0+ZGZzX2xpbmspOwogCi0JCWVuZ2luZSA9IHB0X2xvY2tfZW5naW5lKHB0LCBlbmdpbmUpOwor CQllbmdpbmUgPSBwdF9sb2NrX2VuZ2luZShwdCwgJmxvY2tlZCk7CiAKIAkJaWYgKHByaW8gPD0g cHQtPnByaW9yaXR5KQogCQkJY29udGludWU7CkBAIC03NTAsOCArNzcwLDggQEAgc3RhdGljIHZv aWQgZXhlY2xpc3RzX3NjaGVkdWxlKHN0cnVjdCBkcm1faTkxNV9nZW1fcmVxdWVzdCAqcmVxdWVz dCwgaW50IHByaW8pCiAJCQllbmdpbmUtPmV4ZWNsaXN0X2ZpcnN0ID0gJnB0LT5ub2RlOwogCX0K IAotCWlmIChlbmdpbmUpCi0JCXNwaW5fdW5sb2NrX2lycSgmZW5naW5lLT50aW1lbGluZS0+bG9j ayk7CisJdW5sb2NrX2VuZ2luZXMocmVxdWVzdC0+aTkxNSwgbG9ja2VkKTsKKwlsb2NhbF9pcnFf ZW5hYmxlKCk7CiAKIAkvKiBYWFggRG8gd2UgbmVlZCB0byBwcmVlbXB0IHRvIG1ha2Ugcm9vbSBm b3IgdXMgYW5kIG91ciBkZXBzPyAqLwogfQotLSAKMi4xMS4wCgpfX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fXwpJbnRlbC1nZnggbWFpbGluZyBsaXN0CkludGVs LWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcv bWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK