From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.5 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 05917C4338F for ; Thu, 12 Aug 2021 15:10:08 +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 C0E6C610A5 for ; Thu, 12 Aug 2021 15:10:07 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org C0E6C610A5 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=jlekstrand.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id AB5436E426; Thu, 12 Aug 2021 15:10:01 +0000 (UTC) Received: from mail-yb1-xb35.google.com (mail-yb1-xb35.google.com [IPv6:2607:f8b0:4864:20::b35]) by gabe.freedesktop.org (Postfix) with ESMTPS id 557356E420 for ; Thu, 12 Aug 2021 15:10:00 +0000 (UTC) Received: by mail-yb1-xb35.google.com with SMTP id n15so11631991ybm.4 for ; Thu, 12 Aug 2021 08:10:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jlekstrand-net.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=3RObpsYa3oVTvHKYxaZZ0VmBfjjX+QSLwEhjq3E7b14=; b=UFOhg2pIql72mkDHPixHSQHFrff9UCLGsq0NzrIPupCfzGerCKXWq7SFLetFZS4BBL Z2cxQQJfIgkaSmgED9ViTm6TlGUFyBH8NiXI2zCb3vRMyP65QwesJWMI0lhZh6ThLqqL xWM8rd2CWpfWCNhNz8mG08ETPkMD96x0p9ZXMFdZo1ynMpWZqlYI4J957aw9XF3UUFtJ 4Y4J8eeySlUghoN2TV9Lp2jTQ+LW6oBbDQHkct+8JP9+FiwuzSyYTY1TLrmSPfIJMSBX Gh1jm8WGqBqROm45616JBYCl7xqOKXTAZguN3VdAY4yp4hgkE1Vc3sdNa6QgTKdKJbry dUQQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=3RObpsYa3oVTvHKYxaZZ0VmBfjjX+QSLwEhjq3E7b14=; b=Vzr3QlvyaEwKMP2ahPPrKw27wLv1ig2r2sciAEBm5NsD7b6S+qhfOGwqQcpOweRiWA P70FbyiUtiS/SoDR6pZhWx08X1S1sCac1gsLkB1R64kASsuziFyJliWufHkXpus2TWB9 JcFIp283DYggBKF9Mj/isGsCzNlkRZ30u+gLkHr+Y9MTWgHM2qdPEqKulG9Q9Wp4BoUU kh1NTyPWZkHT9gtiInrLQ/IQTD4ZGbbcR1wsiRULKEUhUP5K7Gzq8XcYhA9Gyj49STD/ xUQ3L/a2UUIoOEe1SkoUYtcugyhiZ2OkrJ7NHzgOx9Vdx9iNq4j+85+ahdc55ecJujVR uGVA== X-Gm-Message-State: AOAM532nYX34zC7mGTDVXKFhqUtCz/v/YuMV5y9gc78W63qmHksFZUz4 AA5RFlpviGy+B8ZhfiEZ6UVU2eakPvH9WCzRpNc8oQ== X-Google-Smtp-Source: ABdhPJxtCuiBUVBRm0fzrChQsr6f09/RJVBYo6zuDgnWiRk2zGfYO9IBi8lRP1GKAMbtN2UiSroHecyl4WJwiRtJPiU= X-Received: by 2002:a25:208b:: with SMTP id g133mr4728159ybg.211.1628780999248; Thu, 12 Aug 2021 08:09:59 -0700 (PDT) MIME-Version: 1.0 References: <20210810130523.1972031-1-daniel.vetter@ffwll.ch> In-Reply-To: <20210810130523.1972031-1-daniel.vetter@ffwll.ch> From: Jason Ekstrand Date: Thu, 12 Aug 2021 10:09:47 -0500 Message-ID: To: Daniel Vetter Cc: Intel Graphics Development , DRI Development , Daniel Vetter , Chris Wilson , Mika Kuoppala , Tvrtko Ursulin , Joonas Lahtinen , Matthew Brost Content-Type: text/plain; charset="UTF-8" Subject: Re: [Intel-gfx] [PATCH] drm/i915: Use locked access to ctx->engines in set_priority 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: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Tue, Aug 10, 2021 at 8:05 AM Daniel Vetter wrote: > > This essentially reverts > > commit 89ff76bf9b3b0b86e6bbe344bd6378d8661303fc > Author: Chris Wilson > Date: Thu Apr 2 13:42:18 2020 +0100 > > drm/i915/gem: Utilize rcu iteration of context engines > > Note that the other use of __context_engines_await have disappeard in > the following commits: > > ccbc1b97948a ("drm/i915/gem: Don't allow changing the VM on running contexts (v4)") > c7a71fc8ee04 ("drm/i915: Drop getparam support for I915_CONTEXT_PARAM_ENGINES") > 4a766ae40ec8 ("drm/i915: Drop the CONTEXT_CLONE API (v2)") > > None of these have any business to optimize their engine lookup with > rcu, unless extremely convincing benchmark data and a solid analysis > why we can't make that workload (whatever it is that does) faster with > a proper design fix. > > Also since there's only one caller of context_apply_all left and it's > really just a loop, inline it and then inline the lopp body too. This > is how all other callers that take the engine lock loop over engines, > it's much simpler. > > Signed-off-by: Daniel Vetter > Cc: Chris Wilson > Cc: Mika Kuoppala > Cc: Daniel Vetter > Cc: Jason Ekstrand > Cc: Tvrtko Ursulin > Cc: Joonas Lahtinen > Cc: Matthew Brost > --- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 72 ++++----------------- > 1 file changed, 14 insertions(+), 58 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > index dbaeb924a437..fd169cf2f75a 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -1284,49 +1284,6 @@ static int __context_set_persistence(struct i915_gem_context *ctx, bool state) > return 0; > } > > -static inline struct i915_gem_engines * > -__context_engines_await(const struct i915_gem_context *ctx, > - bool *user_engines) > -{ > - struct i915_gem_engines *engines; > - > - rcu_read_lock(); > - do { > - engines = rcu_dereference(ctx->engines); > - GEM_BUG_ON(!engines); > - > - if (user_engines) > - *user_engines = i915_gem_context_user_engines(ctx); > - > - /* successful await => strong mb */ > - if (unlikely(!i915_sw_fence_await(&engines->fence))) Ugh... The first time I looked at this I thought the SW fence meant it was actually waiting on something. But, no, it's just making sure the engines object still exists. *sigh* Burn it! Reviewed-by: Jason Ekstrand > - continue; > - > - if (likely(engines == rcu_access_pointer(ctx->engines))) > - break; > - > - i915_sw_fence_complete(&engines->fence); > - } while (1); > - rcu_read_unlock(); > - > - return engines; > -} > - > -static void > -context_apply_all(struct i915_gem_context *ctx, > - void (*fn)(struct intel_context *ce, void *data), > - void *data) > -{ > - struct i915_gem_engines_iter it; > - struct i915_gem_engines *e; > - struct intel_context *ce; > - > - e = __context_engines_await(ctx, NULL); > - for_each_gem_engine(ce, e, it) > - fn(ce, data); > - i915_sw_fence_complete(&e->fence); > -} > - > static struct i915_gem_context * > i915_gem_create_context(struct drm_i915_private *i915, > const struct i915_gem_proto_context *pc) > @@ -1776,23 +1733,11 @@ set_persistence(struct i915_gem_context *ctx, > return __context_set_persistence(ctx, args->value); > } > > -static void __apply_priority(struct intel_context *ce, void *arg) > -{ > - struct i915_gem_context *ctx = arg; > - > - if (!intel_engine_has_timeslices(ce->engine)) > - return; > - > - if (ctx->sched.priority >= I915_PRIORITY_NORMAL && > - intel_engine_has_semaphores(ce->engine)) > - intel_context_set_use_semaphores(ce); > - else > - intel_context_clear_use_semaphores(ce); > -} > - > static int set_priority(struct i915_gem_context *ctx, > const struct drm_i915_gem_context_param *args) > { > + struct i915_gem_engines_iter it; > + struct intel_context *ce; > int err; > > err = validate_priority(ctx->i915, args); > @@ -1800,7 +1745,18 @@ static int set_priority(struct i915_gem_context *ctx, > return err; > > ctx->sched.priority = args->value; > - context_apply_all(ctx, __apply_priority, ctx); > + > + for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) { > + if (!intel_engine_has_timeslices(ce->engine)) > + continue; > + > + if (ctx->sched.priority >= I915_PRIORITY_NORMAL && > + intel_engine_has_semaphores(ce->engine)) > + intel_context_set_use_semaphores(ce); > + else > + intel_context_clear_use_semaphores(ce); > + } > + i915_gem_context_unlock_engines(ctx); > > return 0; > } > -- > 2.32.0 >