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 237FAC7115B for ; Mon, 23 Jun 2025 09:33:24 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id CF33A10E2BA; Mon, 23 Jun 2025 09:33:23 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="D//vTX+w"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.12]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2B05F10E2BA for ; Mon, 23 Jun 2025 09:33:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1750671202; x=1782207202; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=KWWmxF1JHGmW6mX/5Z5TfYMyUPVWQh09rALjSOXdo68=; b=D//vTX+wtKHV4PBuZZuIP24OrIa6H7/lt+orxRDJybnCbBi4TYAZJF05 bsej1XTUv9Te8cCuYr4eHzpd5N2AIeXMOCRkXeRXFe8IqOFfB3Eh2eLCE iQTAUZGrJHCm0bNYzTTrVIEmpKw/gutU7y4EnLC2RxYnr+TQ3sxJC9VXI 2neV/2Gl0MKPwhV0hLeMOloU0h5pfcjAMUPzliai/8yJgi9wR/fZsqEQf lGRZ8DFjoNyTi55bNafetBiS8HvNHq+3cqTlI9Xp/N4qMyA2vw/wdwih5 Zcb4ElIxIVj6E+2qu4/2ZFXlz/cdi0npCgHN2JU4UibGJxKvP+5IXbtVr A==; X-CSE-ConnectionGUID: rb82PU5PRLKmZw8pJpkHDg== X-CSE-MsgGUID: KX+8QSCzRTq+D+XMNaFqeA== X-IronPort-AV: E=McAfee;i="6800,10657,11472"; a="64301822" X-IronPort-AV: E=Sophos;i="6.16,258,1744095600"; d="scan'208";a="64301822" Received: from orviesa008.jf.intel.com ([10.64.159.148]) by orvoesa104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Jun 2025 02:33:17 -0700 X-CSE-ConnectionGUID: KP0JEizpQwuk0KiKrhRPsw== X-CSE-MsgGUID: xkh3SnC3S8KgcLht0tzniQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.16,258,1744095600"; d="scan'208";a="152068044" Received: from savramon-mobl1 (HELO [10.245.244.83]) ([10.245.244.83]) by orviesa008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Jun 2025 02:33:16 -0700 Message-ID: <530d502e-165d-41fb-9ce8-b6bbbbb8107a@intel.com> Date: Mon, 23 Jun 2025 10:33:13 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] drm/xe/migrate: make MI_TLB_INVALIDATE conditional To: Matthew Brost Cc: intel-xe@lists.freedesktop.org, Himal Prasad Ghimiray , =?UTF-8?Q?Thomas_Hellstr=C3=B6m?= References: <20250620152446.239699-2-matthew.auld@intel.com> Content-Language: en-GB From: Matthew Auld In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 20/06/2025 17:11, Matthew Brost wrote: > On Fri, Jun 20, 2025 at 04:24:47PM +0100, Matthew Auld wrote: >> When clearing VRAM we should be able to skip invalidating the TLBs if we > > For copies, we always program SRAM PTEs, and need a invalidate? > Maybe mention this in the commit message. Yup exactly that. I don't think we currently do vram -> vram in practice so I didn't bother with the copy path. Will tweak the commit message. > >> are only using the identity map to access VRAM (which is the common >> case), since no modifications are made to PTEs on the fly. Also since we >> use huge 1G entries within the identity map, there should be a pretty >> decent chance that the next packet(s) (if also clears) can avoid a tree >> walk if we don't shoot down the TLBs, like if we have to process a long >> stream of clears. >> >> Signed-off-by: Matthew Auld >> Cc: Himal Prasad Ghimiray >> Cc: Thomas Hellström >> Cc: Matthew Brost >> --- >> drivers/gpu/drm/xe/xe_migrate.c | 18 +++++++++++------- >> drivers/gpu/drm/xe/xe_ring_ops.c | 10 +++++----- >> 2 files changed, 16 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c >> index 8f8e9fdfb2a8..a76363740a12 100644 >> --- a/drivers/gpu/drm/xe/xe_migrate.c >> +++ b/drivers/gpu/drm/xe/xe_migrate.c >> @@ -896,7 +896,7 @@ struct dma_fence *xe_migrate_copy(struct xe_migrate *m, >> goto err; >> } >> >> - xe_sched_job_add_migrate_flush(job, flush_flags); >> + xe_sched_job_add_migrate_flush(job, flush_flags | MI_INVALIDATE_TLB); >> if (!fence) { >> err = xe_sched_job_add_deps(job, src_bo->ttm.base.resv, >> DMA_RESV_USAGE_BOOKKEEP); >> @@ -1119,11 +1119,13 @@ struct dma_fence *xe_migrate_clear(struct xe_migrate *m, >> >> size -= clear_L0; >> /* Preemption is enabled again by the ring ops. */ >> - if (clear_vram && xe_migrate_allow_identity(clear_L0, &src_it)) >> + if (clear_vram && xe_migrate_allow_identity(clear_L0, &src_it)) { >> xe_res_next(&src_it, clear_L0); >> - else >> - emit_pte(m, bb, clear_L0_pt, clear_vram, clear_only_system_ccs, >> - &src_it, clear_L0, dst); >> + } else { >> + emit_pte(m, bb, clear_L0_pt, clear_vram, >> + clear_only_system_ccs, &src_it, clear_L0, dst); >> + flush_flags |= MI_INVALIDATE_TLB; >> + } > > What about the if statements for dst_it / copy_system_ccs? Do we not > need to set MI_INVALIDATE_TLB there? You mean for the copy path? We unconditionally apply MI_INVALIDATE_TLB when doing any copy. > > Matt > >> >> bb->cs[bb->len++] = MI_BATCH_BUFFER_END; >> update_idx = bb->len; >> @@ -1134,7 +1136,7 @@ struct dma_fence *xe_migrate_clear(struct xe_migrate *m, >> if (xe_migrate_needs_ccs_emit(xe)) { >> emit_copy_ccs(gt, bb, clear_L0_ofs, true, >> m->cleared_mem_ofs, false, clear_L0); >> - flush_flags = MI_FLUSH_DW_CCS; >> + flush_flags |= MI_FLUSH_DW_CCS; >> } >> >> job = xe_bb_create_migration_job(m->q, bb, >> @@ -1469,6 +1471,8 @@ __xe_migrate_update_pgtables(struct xe_migrate *m, >> goto err_sa; >> } >> >> + xe_sched_job_add_migrate_flush(job, MI_INVALIDATE_TLB); >> + >> if (ops->pre_commit) { >> pt_update->job = job; >> err = ops->pre_commit(pt_update); >> @@ -1667,7 +1671,7 @@ static struct dma_fence *xe_migrate_vram(struct xe_migrate *m, >> goto err; >> } >> >> - xe_sched_job_add_migrate_flush(job, 0); >> + xe_sched_job_add_migrate_flush(job, MI_INVALIDATE_TLB); >> >> mutex_lock(&m->job_mutex); >> xe_sched_job_arm(job); >> diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c b/drivers/gpu/drm/xe/xe_ring_ops.c >> index bc1689db4cd7..b5548e0769f4 100644 >> --- a/drivers/gpu/drm/xe/xe_ring_ops.c >> +++ b/drivers/gpu/drm/xe/xe_ring_ops.c >> @@ -110,10 +110,10 @@ static int emit_bb_start(u64 batch_addr, u32 ppgtt_flag, u32 *dw, int i) >> return i; >> } >> >> -static int emit_flush_invalidate(u32 *dw, int i) >> +static int emit_flush_invalidate(u32 *dw, int i, u32 flush_flags) >> { >> - dw[i++] = MI_FLUSH_DW | MI_INVALIDATE_TLB | MI_FLUSH_DW_OP_STOREDW | >> - MI_FLUSH_IMM_DW | MI_FLUSH_DW_STORE_INDEX; >> + dw[i++] = MI_FLUSH_DW | MI_FLUSH_DW_OP_STOREDW | MI_FLUSH_IMM_DW | >> + MI_FLUSH_DW_STORE_INDEX | (flush_flags & MI_INVALIDATE_TLB) ?: 0; >> dw[i++] = LRC_PPHWSP_FLUSH_INVAL_SCRATCH_ADDR; >> dw[i++] = 0; >> dw[i++] = 0; >> @@ -411,13 +411,13 @@ static void emit_migration_job_gen12(struct xe_sched_job *job, >> if (!IS_SRIOV_VF(gt_to_xe(job->q->gt))) { >> /* XXX: Do we need this? Leaving for now. */ >> dw[i++] = preparser_disable(true); >> - i = emit_flush_invalidate(dw, i); >> + i = emit_flush_invalidate(dw, i, job->migrate_flush_flags); >> dw[i++] = preparser_disable(false); >> } >> >> i = emit_bb_start(job->ptrs[1].batch_addr, BIT(8), dw, i); >> >> - dw[i++] = MI_FLUSH_DW | MI_INVALIDATE_TLB | job->migrate_flush_flags | >> + dw[i++] = MI_FLUSH_DW | job->migrate_flush_flags | >> MI_FLUSH_DW_OP_STOREDW | MI_FLUSH_IMM_DW; >> dw[i++] = xe_lrc_seqno_ggtt_addr(lrc) | MI_FLUSH_DW_USE_GTT; >> dw[i++] = 0; >> -- >> 2.49.0 >>