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=-16.5 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,HK_RANDOM_FROM,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 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 6A94EC4320E for ; Thu, 2 Sep 2021 14:54:43 +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 06A85610CF for ; Thu, 2 Sep 2021 14:54:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 06A85610CF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com 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 04FD06E59D; Thu, 2 Sep 2021 14:54:42 +0000 (UTC) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTPS id 60BC96E598; Thu, 2 Sep 2021 14:54:41 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10094"; a="218847652" X-IronPort-AV: E=Sophos;i="5.85,262,1624345200"; d="scan'208";a="218847652" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Sep 2021 07:54:40 -0700 X-IronPort-AV: E=Sophos;i="5.85,262,1624345200"; d="scan'208";a="461695549" Received: from rlsmith2-mobl1.amr.corp.intel.com (HELO [10.213.229.210]) ([10.213.229.210]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Sep 2021 07:54:37 -0700 Subject: Re: [Intel-gfx] [PATCH 07/11] drm/i915: Add i915_gem_context_is_full_ppgtt To: Daniel Vetter , DRI Development Cc: Intel Graphics Development , Maarten Lankhorst , Daniel Vetter , Jon Bloomfield , Chris Wilson , Joonas Lahtinen , =?UTF-8?Q?Thomas_Hellstr=c3=b6m?= , Matthew Auld , Lionel Landwerlin , Dave Airlie , Jason Ekstrand References: <20210902142057.929669-1-daniel.vetter@ffwll.ch> <20210902142057.929669-7-daniel.vetter@ffwll.ch> From: Tvrtko Ursulin Organization: Intel Corporation UK Plc Message-ID: <1cb4b910-ad02-ff02-46ef-7b3b4f393eb3@linux.intel.com> Date: Thu, 2 Sep 2021 15:54:36 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210902142057.929669-7-daniel.vetter@ffwll.ch> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On 02/09/2021 15:20, Daniel Vetter wrote: > And use it anywhere we have open-coded checks for ctx->vm that really > only check for full ppgtt. > > Plus for paranoia add a GEM_BUG_ON that checks it's really only set > when we have full ppgtt, just in case. gem_context->vm is different > since it's NULL in ggtt mode, unlike intel_context->vm or gt->vm, > which is always set. > > v2: 0day found a testcase that I missed. > > Reviewed-by: Maarten Lankhorst > Signed-off-by: Daniel Vetter > Cc: Jon Bloomfield > Cc: Chris Wilson > Cc: Maarten Lankhorst > Cc: Joonas Lahtinen > Cc: Daniel Vetter > Cc: "Thomas Hellström" > Cc: Matthew Auld > Cc: Lionel Landwerlin > Cc: Dave Airlie > Cc: Jason Ekstrand > --- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +- > drivers/gpu/drm/i915/gem/i915_gem_context.h | 7 +++++++ > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- > drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 6 +++--- > 4 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > index 7a566fb7cca4..1eec85944c1f 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -1566,7 +1566,7 @@ static int get_ppgtt(struct drm_i915_file_private *file_priv, > int err; > u32 id; > > - if (!rcu_access_pointer(ctx->vm)) > + if (!i915_gem_context_is_full_ppgtt(ctx)) It reads a bit wrong because GEM context cannot *be* full ppggt. It can be associated with a VM which is or isn't full ppgtt. So a test on a VM retrieved from a context is semnntically more correct. Perhaps you want to consider adding a helper to that effect instead? It could mean splitting into two helpers (getter + test) or maybe just renaming would work. Like i915_gem_context_has_full_ppgtt_vm(ctx)? Regards, Tvrtko > return -ENODEV; > > rcu_read_lock(); > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h b/drivers/gpu/drm/i915/gem/i915_gem_context.h > index da6e8b506d96..37536a260e6e 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h > @@ -154,6 +154,13 @@ i915_gem_context_vm(struct i915_gem_context *ctx) > return rcu_dereference_protected(ctx->vm, lockdep_is_held(&ctx->mutex)); > } > > +static inline bool i915_gem_context_is_full_ppgtt(struct i915_gem_context *ctx) > +{ > + GEM_BUG_ON(!!rcu_access_pointer(ctx->vm) != HAS_FULL_PPGTT(ctx->i915)); > + > + return !!rcu_access_pointer(ctx->vm); > +} > + > static inline struct i915_address_space * > i915_gem_context_get_eb_vm(struct i915_gem_context *ctx) > { > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index 905b1cbd22d5..40f08948f0b2 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -749,7 +749,7 @@ static int eb_select_context(struct i915_execbuffer *eb) > return PTR_ERR(ctx); > > eb->gem_context = ctx; > - if (rcu_access_pointer(ctx->vm)) > + if (i915_gem_context_is_full_ppgtt(ctx)) > eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT; > > return 0; > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c > index fc7fb33a3a52..947154e445a7 100644 > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c > @@ -704,7 +704,7 @@ static int igt_ctx_exec(void *arg) > pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) [full-ppgtt? %s], err=%d\n", > ndwords, dw, max_dwords(obj), > engine->name, > - yesno(!!rcu_access_pointer(ctx->vm)), > + yesno(i915_gem_context_is_full_ppgtt(ctx)), > err); > intel_context_put(ce); > kernel_context_close(ctx); > @@ -838,7 +838,7 @@ static int igt_shared_ctx_exec(void *arg) > pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) [full-ppgtt? %s], err=%d\n", > ndwords, dw, max_dwords(obj), > engine->name, > - yesno(!!rcu_access_pointer(ctx->vm)), > + yesno(i915_gem_context_is_full_ppgtt(ctx)), > err); > intel_context_put(ce); > kernel_context_close(ctx); > @@ -1417,7 +1417,7 @@ static int igt_ctx_readonly(void *arg) > pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) [full-ppgtt? %s], err=%d\n", > ndwords, dw, max_dwords(obj), > ce->engine->name, > - yesno(!!ctx_vm(ctx)), > + yesno(i915_gem_context_is_full_ppgtt(ctx)), > err); > i915_gem_context_unlock_engines(ctx); > goto out_file; >