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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 57FECE77188 for ; Fri, 27 Dec 2024 03:31:41 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id E4AD410E1AD; Fri, 27 Dec 2024 03:31:40 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="i/A9nvMm"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.18]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1411B10E1AD for ; Fri, 27 Dec 2024 03:31:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1735270300; x=1766806300; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version:content-transfer-encoding; bh=qa4XUmGnnlxTeUQKkk9ig8Gg8iEVgqhGquLw6EFLxX8=; b=i/A9nvMmTlFbmRkX0km2lB7Fa3I9eLsp9XOHKqukjjga5myzXy1k/MEK mJToyK44HVVSK3Sop8DT99VZyvS6r3jVVa0VqlesZHp5RbfcfoHwq4It2 O04hex+aqgKcz/vIyeybpo36nFjELuWmPG9S9oDoMwZw8S39efszlikRX FXlZ0wg2n2GLu0Ipi0pg5fV1MAs44XrhwJMx6NN1Dotpl4agjcilA9Nat /DMrlE7L33FPlJY1dPiFpOmg6KalbZ5Ws79vqWRdAEfJk3GiCi4x1J0BF juOAPJnUzlCO996sSrMeKvAuf8VkIXtmDGn2xhnWqq+LL6YK9Rz3nu/Nb w==; X-CSE-ConnectionGUID: dTjDM0IARXu+Y0h2pK1SXQ== X-CSE-MsgGUID: +Qhgg5NdS7qild+Q6yW+XA== X-IronPort-AV: E=McAfee;i="6700,10204,11297"; a="34990315" X-IronPort-AV: E=Sophos;i="6.12,268,1728975600"; d="scan'208";a="34990315" Received: from fmviesa008.fm.intel.com ([10.60.135.148]) by fmvoesa112.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Dec 2024 19:31:40 -0800 X-CSE-ConnectionGUID: pylNxL9kTWCY9EC6hslcyg== X-CSE-MsgGUID: 04PYaIYvTQmyGfYcSsc/1Q== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.12,268,1728975600"; d="scan'208";a="100257088" Received: from orsosgc001.jf.intel.com (HELO orsosgc001.intel.com) ([10.165.21.142]) by fmviesa008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Dec 2024 19:31:39 -0800 Date: Thu, 26 Dec 2024 19:31:39 -0800 Message-ID: <85a5ch244k.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: =?ISO-8859-1?Q?Jos=E9?= Roberto de Souza Cc: , Thomas =?ISO-8859-1?Q?Hellstr=F6m?= Subject: Re: [PATCH] Revert "drm/xe: Force write completion of MI_STORE_DATA_IMM" In-Reply-To: <20241226162310.47489-1-jose.souza@intel.com> References: <20241226162310.47489-1-jose.souza@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2 (x86_64-redhat-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-BeenThere: intel-xe@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Xe graphics driver List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On Thu, 26 Dec 2024 08:23:10 -0800, Jos=E9 Roberto de Souza wrote: > > This reverts commit 1460bb1fef9ccf7390af0d74a15252442fd6effd. > > In all places the MI_STORE_DATA_IMM are not followed by a read of > the same memory address in the same batch buffer and the posted writes > are flushed with PIPE_CONTROL or MI_FLUSH_DW in xe_ring_ops.c functions > so there is no need to set this register. As I was saying this commit message should just be: In all places where MI_STORE_DATA_IMM is used, posted writes are flushed with PIPE_CONTROL or MI_FLUSH_DW in xe_ring_ops.c functions, so there is no need to set this register. So according to me the "are not followed by a read of the same memory address in the same batch buffer" is incorrect, it will work even without setting the flag. Anyway apart from that nit, this is: Reviewed-by: Ashutosh Dixit > > Cc: Thomas Hellstr=F6m > Cc: Ashutosh Dixit > Fixes: 1460bb1fef9c ("drm/xe: Force write completion of MI_STORE_DATA_IMM= ") > Signed-off-by: Jos=E9 Roberto de Souza > --- > drivers/gpu/drm/xe/instructions/xe_mi_commands.h | 13 ++++++------- > drivers/gpu/drm/xe/xe_migrate.c | 11 +++-------- > drivers/gpu/drm/xe/xe_ring_ops.c | 6 ++---- > 3 files changed, 11 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/xe/instructions/xe_mi_commands.h b/drivers/g= pu/drm/xe/instructions/xe_mi_commands.h > index f4ee910f09432..10ec2920d31b3 100644 > --- a/drivers/gpu/drm/xe/instructions/xe_mi_commands.h > +++ b/drivers/gpu/drm/xe/instructions/xe_mi_commands.h > @@ -33,13 +33,12 @@ > #define MI_TOPOLOGY_FILTER __MI_INSTR(0xD) > #define MI_FORCE_WAKEUP __MI_INSTR(0x1D) > > -#define MI_STORE_DATA_IMM __MI_INSTR(0x20) > -#define MI_SDI_GGTT REG_BIT(22) > -#define MI_FORCE_WRITE_COMPLETION_CHECK REG_BIT(10) > -#define MI_SDI_LEN_DW GENMASK(9, 0) > -#define MI_SDI_NUM_DW(x) REG_FIELD_PREP(MI_SDI_LEN_DW, (x) + 3 - 2) > -#define MI_SDI_NUM_QW(x) (REG_FIELD_PREP(MI_SDI_LEN_DW, 2 * (x) + 3 = - 2) | \ > - REG_BIT(21)) > +#define MI_STORE_DATA_IMM __MI_INSTR(0x20) > +#define MI_SDI_GGTT REG_BIT(22) > +#define MI_SDI_LEN_DW GENMASK(9, 0) > +#define MI_SDI_NUM_DW(x) REG_FIELD_PREP(MI_SDI_LEN_DW, (x) + 3 - 2) > +#define MI_SDI_NUM_QW(x) (REG_FIELD_PREP(MI_SDI_LEN_DW, 2 * (x) + 3 -= 2) | \ > + REG_BIT(21)) > > #define MI_LOAD_REGISTER_IMM __MI_INSTR(0x22) > #define MI_LRI_LRM_CS_MMIO REG_BIT(19) > diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migr= ate.c > index 8b32fad678782..1b97d90aaddaf 100644 > --- a/drivers/gpu/drm/xe/xe_migrate.c > +++ b/drivers/gpu/drm/xe/xe_migrate.c > @@ -581,9 +581,7 @@ static void emit_pte(struct xe_migrate *m, > while (ptes) { > u32 chunk =3D min(MAX_PTE_PER_SDI, ptes); > > - bb->cs[bb->len++] =3D MI_STORE_DATA_IMM | > - MI_FORCE_WRITE_COMPLETION_CHECK | > - MI_SDI_NUM_QW(chunk); > + bb->cs[bb->len++] =3D MI_STORE_DATA_IMM | MI_SDI_NUM_QW(chunk); > bb->cs[bb->len++] =3D ofs; > bb->cs[bb->len++] =3D 0; > > @@ -1225,9 +1223,7 @@ static void write_pgtable(struct xe_tile *tile, str= uct xe_bb *bb, u64 ppgtt_ofs, > if (!(bb->len & 1)) > bb->cs[bb->len++] =3D MI_NOOP; > > - bb->cs[bb->len++] =3D MI_STORE_DATA_IMM | > - MI_FORCE_WRITE_COMPLETION_CHECK | > - MI_SDI_NUM_QW(chunk); > + bb->cs[bb->len++] =3D MI_STORE_DATA_IMM | MI_SDI_NUM_QW(chunk); > bb->cs[bb->len++] =3D lower_32_bits(addr); > bb->cs[bb->len++] =3D upper_32_bits(addr); > if (pt_op->bind) > @@ -1392,8 +1388,7 @@ __xe_migrate_update_pgtables(struct xe_migrate *m, > u32 idx =3D 0; > > bb->cs[bb->len++] =3D MI_STORE_DATA_IMM | > - MI_FORCE_WRITE_COMPLETION_CHECK | > - MI_SDI_NUM_QW(chunk); > + MI_SDI_NUM_QW(chunk); > bb->cs[bb->len++] =3D ofs; > bb->cs[bb->len++] =3D 0; /* upper_32_bits */ > > diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c b/drivers/gpu/drm/xe/xe_rin= g_ops.c > index c8ab37fa0d198..9f327f27c0726 100644 > --- a/drivers/gpu/drm/xe/xe_ring_ops.c > +++ b/drivers/gpu/drm/xe/xe_ring_ops.c > @@ -72,8 +72,7 @@ static int emit_user_interrupt(u32 *dw, int i) > > static int emit_store_imm_ggtt(u32 addr, u32 value, u32 *dw, int i) > { > - dw[i++] =3D MI_STORE_DATA_IMM | MI_SDI_GGTT | > - MI_FORCE_WRITE_COMPLETION_CHECK | MI_SDI_NUM_DW(1); > + dw[i++] =3D MI_STORE_DATA_IMM | MI_SDI_GGTT | MI_SDI_NUM_DW(1); > dw[i++] =3D addr; > dw[i++] =3D 0; > dw[i++] =3D value; > @@ -163,8 +162,7 @@ static int emit_pipe_invalidate(u32 mask_flags, bool = invalidate_tlb, u32 *dw, > static int emit_store_imm_ppgtt_posted(u64 addr, u64 value, > u32 *dw, int i) > { > - dw[i++] =3D MI_STORE_DATA_IMM | MI_FORCE_WRITE_COMPLETION_CHECK | > - MI_SDI_NUM_QW(1); > + dw[i++] =3D MI_STORE_DATA_IMM | MI_SDI_NUM_QW(1); > dw[i++] =3D lower_32_bits(addr); > dw[i++] =3D upper_32_bits(addr); > dw[i++] =3D lower_32_bits(value); > -- > 2.47.1 >