From patchwork Thu Sep 17 09:44:09 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Thomas_Hellstr=C3=B6m_=28Intel=29?= X-Patchwork-Id: 11782071 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 24CD659D for ; Thu, 17 Sep 2020 09:44:36 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D3EBC20770 for ; Thu, 17 Sep 2020 09:44:35 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=shipmail.org header.i=@shipmail.org header.b="Tw9Bsrsz" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D3EBC20770 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=shipmail.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 22C336EB91; Thu, 17 Sep 2020 09:44:35 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from ste-pvt-msa1.bahnhof.se (ste-pvt-msa1.bahnhof.se [213.80.101.70]) by gabe.freedesktop.org (Postfix) with ESMTPS id BB71F6EB91 for ; Thu, 17 Sep 2020 09:44:33 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by ste-pvt-msa1.bahnhof.se (Postfix) with ESMTP id 6AFC33F3B2; Thu, 17 Sep 2020 11:44:32 +0200 (CEST) Authentication-Results: ste-pvt-msa1.bahnhof.se; dkim=pass (1024-bit key; unprotected) header.d=shipmail.org header.i=@shipmail.org header.b=Tw9Bsrsz; dkim-atps=neutral X-Virus-Scanned: Debian amavisd-new at bahnhof.se X-Spam-Flag: NO X-Spam-Score: -2.099 X-Spam-Level: X-Spam-Status: No, score=-2.099 tagged_above=-999 required=6.31 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no Received: from ste-pvt-msa1.bahnhof.se ([127.0.0.1]) by localhost (ste-pvt-msa1.bahnhof.se [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id fdc-UO2TD5jX; Thu, 17 Sep 2020 11:44:31 +0200 (CEST) Received: by ste-pvt-msa1.bahnhof.se (Postfix) with ESMTPA id 39C143F386; Thu, 17 Sep 2020 11:44:31 +0200 (CEST) Received: from thellstr-mobl2.intel.com (unknown [134.134.139.83]) by mail1.shipmail.org (Postfix) with ESMTPSA id 51F8736010B; Thu, 17 Sep 2020 11:44:27 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=shipmail.org; s=mail; t=1600335871; bh=XsNqtWNq0YzcoCpEzfGvW11ZA3J40u8ZczrYjgp6Bm4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Tw9BsrszrjPphrYgYORXfnfEjMgttj2Q788laskISv9nDVEOacAtzdR5vc6LiRwzW EPoHfRZpc2P1T6zstkD5kH865ges6nNNn5hTfYGMGLlj360wl/GNeXk54kTcUj9Lk5 N8L1ttUNEW/RH7mIDoIQBE3YgWCvENh6n3MMEULE= From: =?utf-8?q?Thomas_Hellstr=C3=B6m_=28Intel=29?= To: intel-gfx@lists.freedesktop.org Date: Thu, 17 Sep 2020 11:44:09 +0200 Message-Id: <20200917094409.193697-3-thomas_os@shipmail.org> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20200917094409.193697-1-thomas_os@shipmail.org> References: <20200917094409.193697-1-thomas_os@shipmail.org> MIME-Version: 1.0 Subject: [Intel-gfx] [RFC PATCH 2/2] drm/i915: Introduce a i915_gem_do_ww(){} utility X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: maarten.lankhorst@intel.com, chris@chris-wilson.co.uk Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" From: Thomas Hellström With the huge number of sites where multiple-object locking is needed in the driver, it becomes difficult to avoid recursive ww_acquire_ctx initialization, and the function prototypes become bloated passing the ww_acquire_ctx around. Furthermore it's not always easy to get the -EDEADLK handling correct and to follow it. Introduce a i915_gem_do_ww utility that tries to remedy all these problems by enclosing parts of a ww transaction in the following way: my_function() { struct i915_gem_ww_ctx *ww, template; int err; bool interruptible = true; i915_do_ww(ww, &template, err, interruptible) { err = ww_transaction_part(ww); } return err; } The utility will automatically look up an active ww_acquire_ctx if one is initialized previously in the call chain, and if one found will forward the -EDEADLK instead of handling it, which takes care of the recursive initalization. Using the utility also discourages nested ww unlocking / relocking that is both very fragile and hard to follow. To look up and register an active ww_acquire_ctx, use a driver-wide hash table for now. But noting that a task could only have a single active ww_acqurie_ctx per ww_class, the active CTX is really task state and a generic version of this utility in the ww_mutex code could thus probably use a quick lookup from a list in the struct task_struct. Signed-off-by: Thomas Hellström --- drivers/gpu/drm/i915/i915_gem_ww.c | 73 +++++++++++++++++++++++++++++- drivers/gpu/drm/i915/i915_gem_ww.h | 55 +++++++++++++++++++++- 2 files changed, 126 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_ww.c b/drivers/gpu/drm/i915/i915_gem_ww.c index 3490b72cf613..9b51cb535b65 100644 --- a/drivers/gpu/drm/i915/i915_gem_ww.c +++ b/drivers/gpu/drm/i915/i915_gem_ww.c @@ -1,10 +1,12 @@ // SPDX-License-Identifier: MIT /* - * Copyright © 2020 Intel Corporation + * Copyright © 2019 Intel Corporation */ +#include #include #include #include "i915_gem_ww.h" +#include "i915_globals.h" #include "gem/i915_gem_object.h" void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ww, bool intr) @@ -70,3 +72,72 @@ int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ww) return ret; } + +static struct rhashtable ww_ht; +static const struct rhashtable_params ww_params = { + .key_len = sizeof(struct task_struct *), + .key_offset = offsetof(struct i915_gem_ww_ctx, ctx.task), + .head_offset = offsetof(struct i915_gem_ww_ctx, head), + .min_size = 128, +}; + +static void i915_ww_item_free(void *ptr, void *arg) +{ + WARN_ON_ONCE(1); +} + +static void i915_global_ww_exit(void) +{ + rhashtable_free_and_destroy(&ww_ht, i915_ww_item_free, NULL); +} + +static void i915_global_ww_shrink(void) +{ +} + +static struct i915_global global = { + .shrink = i915_global_ww_shrink, + .exit = i915_global_ww_exit, +}; + +int __init i915_global_ww_init(void) +{ + int ret = rhashtable_init(&ww_ht, &ww_params); + + if (ret) + return ret; + + i915_global_register(&global); + + return 0; +} + +void __i915_gem_ww_mark_unused(struct i915_gem_ww_ctx *ww) +{ + GEM_WARN_ON(rhashtable_remove_fast(&ww_ht, &ww->head, ww_params)); +} + +/** + * __i915_gem_ww_locate_or_find - return the task's i915_gem_ww_ctx context to use. + * + * @template: The context to use if there was none initialized previously + * in the call chain. + * + * RETURN: The task's i915_gem_ww_ctx context. + */ +struct i915_gem_ww_ctx * +__i915_gem_ww_locate_or_use(struct i915_gem_ww_ctx *template) +{ + struct i915_gem_ww_ctx *tmp; + + /* ctx.task is the hash key, so set it first. */ + template->ctx.task = current; + + /* + * Ideally we'd just hook the active context to the + * struct task_struct. But for now use a hash table. + */ + tmp = rhashtable_lookup_get_insert_fast(&ww_ht, &template->head, + ww_params); + return tmp; +} diff --git a/drivers/gpu/drm/i915/i915_gem_ww.h b/drivers/gpu/drm/i915/i915_gem_ww.h index 94fdf8c5f89b..1a874e2d9f13 100644 --- a/drivers/gpu/drm/i915/i915_gem_ww.h +++ b/drivers/gpu/drm/i915/i915_gem_ww.h @@ -6,18 +6,71 @@ #define __I915_GEM_WW_H__ #include +#include #include struct i915_gem_ww_ctx { struct ww_acquire_ctx ctx; + struct rhash_head head; struct list_head obj_list; struct drm_i915_gem_object *contended; depot_stack_handle_t contended_bt; - bool intr; + u32 call_depth; + unsigned short intr; + unsigned short loop; }; void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ctx, bool intr); void i915_gem_ww_ctx_fini(struct i915_gem_ww_ctx *ctx); int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ctx); void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj); + +/* Internal functions used by the inlines! Don't use. */ +void __i915_gem_ww_mark_unused(struct i915_gem_ww_ctx *ww); +struct i915_gem_ww_ctx * +__i915_gem_ww_locate_or_use(struct i915_gem_ww_ctx *template); + +static inline int __i915_gem_ww_fini(struct i915_gem_ww_ctx *ww, int err) +{ + if (ww->call_depth) { + ww->call_depth--; + return err; + } + + if (err == -EDEADLK) { + err = i915_gem_ww_ctx_backoff(ww); + if (!err) + ww->loop = 1; + } + + if (!ww->loop) { + i915_gem_ww_ctx_fini(ww); + __i915_gem_ww_mark_unused(ww); + } + + return err; +} + +static inline struct i915_gem_ww_ctx * +__ww_i915_gem_ww_init(struct i915_gem_ww_ctx *template, bool intr) +{ + struct i915_gem_ww_ctx *ww = __i915_gem_ww_locate_or_use(template); + + if (!ww) { + ww = template; + ww->call_depth = 0; + i915_gem_ww_ctx_init(ww, intr); + } else { + ww->call_depth++; + } + + ww->loop = 1; + + return ww; +} + +#define i915_gem_do_ww(_ww, _template, _err, _intr) \ + for ((_ww) = __i915_gem_ww_init(&(_template), _intr); (_ww)->loop--; \ + _err = __i915_ww_ww_fini(_ww, _err)) + #endif