From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7EF9E10E465 for ; Thu, 19 Oct 2023 04:45:54 +0000 (UTC) Date: Wed, 18 Oct 2023 21:45:47 -0700 From: Niranjana Vishwanathapura To: Matthew Auld Message-ID: References: <20231016141450.55936-1-matthew.auld@intel.com> <20231016141450.55936-7-matthew.auld@intel.com> <07cc536e-a5a7-b273-fd27-0abeb02e4b9c@intel.com> Content-Type: text/plain; charset="iso-8859-1"; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <07cc536e-a5a7-b273-fd27-0abeb02e4b9c@intel.com> MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t v3 06/12] lib/intel_pat: add helpers for common pat_index modes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: igt-dev@lists.freedesktop.org Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On Tue, Oct 17, 2023 at 11:59:28AM +0100, Matthew Auld wrote: >On 16/10/2023 23:07, Niranjana Vishwanathapura wrote: >>On Mon, Oct 16, 2023 at 03:14:44PM +0100, Matthew Auld wrote: >>>For now just add uc, wt and wb for every platform. The wb mode should >>>always be at least 1way coherent, if messing around with system memory. >>>Also make non-matching platforms throw an error rather than trying to >>>inherit the modes from previous platforms since they will likely be >>>different. >>> >>>Signed-off-by: Matthew Auld >>>Cc: José Roberto de Souza >>>Cc: Pallavi Mishra >>>--- >>>lib/intel_pat.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++ >>>lib/intel_pat.h | 19 ++++++++++++ >>>lib/meson.build |  1 + >>>3 files changed, 97 insertions(+) >>>create mode 100644 lib/intel_pat.c >>>create mode 100644 lib/intel_pat.h >>> >>>diff --git a/lib/intel_pat.c b/lib/intel_pat.c >>>new file mode 100644 >>>index 000000000..2b892ee52 >>>--- /dev/null >>>+++ b/lib/intel_pat.c >>>@@ -0,0 +1,77 @@ >>>+// SPDX-License-Identifier: MIT >>>+/* >>>+ * Copyright © 2023 Intel Corporation >>>+ */ >>>+ >>>+#include "intel_pat.h" >>>+ >>>+#include "igt.h" >>>+ >>>+struct intel_pat_cache { >>>+    uint8_t uc; /* UC + COH_NONE */ >>>+    uint8_t wt; /* WT + COH_NONE */ >>>+    uint8_t wb; /* WB + COH_AT_LEAST_1WAY */ >>>+ >>>+    uint8_t max_index; >>>+}; >>>+ >>>+static void intel_get_pat_idx(int fd, struct intel_pat_cache *pat) >>>+{ >>>+    uint16_t dev_id = intel_get_drm_devid(fd); >>>+ >>>+    if (intel_get_device_info(dev_id)->graphics_ver == 20) { >>>+        pat->uc = 3; >>>+        pat->wt = 15; /* Compressed + WB-transient */ >>>+        pat->wb = 2; >>>+        pat->max_index = 31; >>>+    } else if (IS_METEORLAKE(dev_id)) { >>>+        pat->uc = 2; >>>+        pat->wt = 1; >>>+        pat->wb = 3; >>>+        pat->max_index = 3; >>>+    } else if (IS_PONTEVECCHIO(dev_id)) { >>>+        pat->uc = 0; >>>+        pat->wt = 2; >>>+        pat->wb = 3; >>>+        pat->max_index = 7; >>>+    } else if (intel_graphics_ver(dev_id) <= IP_VER(12, 60)) { >>>+        pat->uc = 3; >>>+        pat->wt = 2; >>>+        pat->wb = 0; >>>+        pat->max_index = 3; >>>+    } else { >>>+        igt_critical("Platform is missing PAT settings for uc/wt/wb\n"); >>>+    } >>>+} >>>+ >>>+uint8_t intel_get_max_pat_index(int fd) >>>+{ >>>+    struct intel_pat_cache pat = {}; >>>+ >>>+    intel_get_pat_idx(fd, &pat); >>>+    return pat.max_index; >>>+} >>>+ >>>+uint8_t intel_get_pat_idx_uc(int fd) >>>+{ >>>+    struct intel_pat_cache pat = {}; >>>+ >>>+    intel_get_pat_idx(fd, &pat); >>>+    return pat.uc; >>>+} >>>+ >>>+uint8_t intel_get_pat_idx_wt(int fd) >>>+{ >>>+    struct intel_pat_cache pat = {}; >>>+ >>>+    intel_get_pat_idx(fd, &pat); >>>+    return pat.wt; >>>+} >>>+ >>>+uint8_t intel_get_pat_idx_wb(int fd) >>>+{ >>>+    struct intel_pat_cache pat = {}; >>>+ >>>+    intel_get_pat_idx(fd, &pat); >>>+    return pat.wb; >>>+} >>>diff --git a/lib/intel_pat.h b/lib/intel_pat.h >>>new file mode 100644 >>>index 000000000..c24dbc275 >>>--- /dev/null >>>+++ b/lib/intel_pat.h >>>@@ -0,0 +1,19 @@ >>>+/* SPDX-License-Identifier: MIT */ >>>+/* >>>+ * Copyright © 2023 Intel Corporation >>>+ */ >>>+ >>>+#ifndef INTEL_PAT_H >>>+#define INTEL_PAT_H >>>+ >>>+#include >>>+ >>>+#define DEFAULT_PAT_INDEX ((uint8_t)-1) /* igt-core can pick 1way >>>or better */ >> >>In the uapi pat_index is u16. >>But here and elsewhere in tests, we are using uint8_t. >>Why not use uint16_t? > >Yeah, uint16 is likely overkill, expectation is that uint8 should be >enough. However there is non-zero chance that we could get more than >256 so figured to make the uapi part completely future proof. > >Also due to some IGT limitations it currently needs to fit in the >lower 12 bits of rsvd1. > Sounds ok to me as this is IGT specific (doesn't need uapi change if we decide to use uint16_t later on here). >> >>Given pat_index 0 is valid, it forces user to always specify pat_index in >>vm_bind call (hence this DEFAULT_PAT_INDEX). Not sure if it is worth >>considering making pat_index a extension so that by not including this >>extension, XeKMD will use the default value? I am fine either way >>and I understand uapi is already reviewed, but just thought of >>bringint it up. > >Yeah, that is one possibility. Based on the cpu_caching and coh_mode >we could potentially select some sane default. No strong opinion. >Although I think real userspace would maybe just always select exactly >what they wanted, so likely only IGT would be making use of it. > Ok, as uapi is already reviewed, I am fine with it. Only worry is what issues we can expect if user doesn't poplulate pat_index and leave it as 0. But yah, UMDs are expected to set it anyway. LGTM. Reviewed-by: Niranjana Vishwanathapura >> >>Niranjana >> >>>+ >>>+uint8_t intel_get_max_pat_index(int fd); >>>+ >>>+uint8_t intel_get_pat_idx_uc(int fd); >>>+uint8_t intel_get_pat_idx_wt(int fd); >>>+uint8_t intel_get_pat_idx_wb(int fd); >>>+ >>>+#endif /* INTEL_PAT_H */ >>>diff --git a/lib/meson.build b/lib/meson.build >>>index a7bccafc3..48466a2e9 100644 >>>--- a/lib/meson.build >>>+++ b/lib/meson.build >>>@@ -64,6 +64,7 @@ lib_sources = [ >>>    'intel_device_info.c', >>>    'intel_mmio.c', >>>    'intel_mocs.c', >>>+    'intel_pat.c', >>>    'ioctl_wrappers.c', >>>    'media_spin.c', >>>    'media_fill.c', >>>-- >>>2.41.0 >>>