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 8BAE0F47CA0 for ; Thu, 5 Mar 2026 17:40:41 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3828610E2D2; Thu, 5 Mar 2026 17:40:41 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="jon5M4Ou"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.7]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1A2DC10E2D2 for ; Thu, 5 Mar 2026 17:40: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=1772732440; x=1804268440; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version:content-transfer-encoding; bh=P9Mlhg+KgKsT8nfxaTqIPXujjeds0PNb3rST0Ctn9LI=; b=jon5M4OuZO8iLZtiiVoGv1fbcuNSUWpmOdk2EjG8niifi12C+1pXzjMg q/1GhN9lcJgaKiHjhwitBBMx4sm/enCFK0KqBuDd3OZ0jHH18OkwIhfPW 3JsReVjjQsoF4Huqf6p2d/QHQMjRmOdAVDm9tdAdfZg0bmaiS5mRVbmYw mVmenEEqvPn4RJEgDAVyvgR7/MQEM5+6X/JKXguejbudpHaDnGVpru0cn wBn0NE8CBwfRMWm5b+zmoN0Awu8/VCVuh5fWxk35NL0RziMZSQDXix6sc z9Ya0zNCMoFQdJpF9pxOGkMzotKK0TUHKQehyl5EyYQ6y5srO9rALOCD8 w==; X-CSE-ConnectionGUID: 57KWxsphRG+xmvDRcKF9ng== X-CSE-MsgGUID: mlASZT6ERHCxP2RDijtK6g== X-IronPort-AV: E=McAfee;i="6800,10657,11720"; a="99298008" X-IronPort-AV: E=Sophos;i="6.23,103,1770624000"; d="scan'208";a="99298008" Received: from orviesa010.jf.intel.com ([10.64.159.150]) by fmvoesa101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Mar 2026 09:40:39 -0800 X-CSE-ConnectionGUID: WawuUEBbSf2L0J3fExsFxw== X-CSE-MsgGUID: kFdC3iqTSTyCDFNWo7X9uA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.23,103,1770624000"; d="scan'208";a="217991191" Received: from rmigue1x-mobl.amr.corp.intel.com (HELO adixit-MOBL3.intel.com) ([10.125.38.160]) by orviesa010-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Mar 2026 09:40:40 -0800 Date: Thu, 05 Mar 2026 09:40:38 -0800 Message-ID: <87seaerx7t.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Cc: , , , , Subject: Re: [PATCH v3 i-g-t] lib/intel_pat: Cache PAT config for unprivileged processes In-Reply-To: <20260305160246.12557-1-himanshu.girotra@intel.com> References: <20260305160246.12557-1-himanshu.girotra@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/30.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-BeenThere: igt-dev@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development mailing list for IGT GPU Tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" On Thu, 05 Mar 2026 08:02:46 -0800, wrote: > > From: Himanshu Girotra > > Commit 4e59b8e7779b ("lib/intel_pat: use kernel debugfs as > authoritative PAT source for Xe") changed the Xe PAT lookup to > read from debugfs, which requires root access. Some xe_oa subtests > fork a child process that drops root privileges before performing > GPU operations that internally need the PAT write-back index. > After dropping root, the debugfs read fails. > > Fix this by caching the PAT configuration in struct xe_device at > xe_device_get() time. Since xe_device_get() is called while still > root, the cache is available to forked children even after > igt_drop_root(). Tests that open a new fd after forking must call > xe_device_get() explicitly before dropping root to populate the > cache for that fd. > > v2: Cache PAT in struct xe_device instead of a global static; > drop the intel_pat_precache() helper =E2=80=94 xe_device_get() already > populates the cache, so call it directly in xe_oa before > igt_drop_root() (Ashutosh Dixit) > > v3: Use pat_cache pointer, NULL indicates unpopulated cache; > remove redundant xe_oa.c change (Ashutosh Dixit) > > Add FIXME for multi-GPU (Zbigniew Kempczy=C5=84ski) > > Cc: Matt Roper > Cc: Xin Wang > Cc: Nishit Sharma > Cc: Ashutosh Dixit > Cc: Zbigniew Kempczy=C5=84ski > > Reviewed-by: Zbigniew Kempczy=C5=84ski > Signed-off-by: Himanshu Girotra > --- > lib/intel_pat.c | 18 ++++++++++-------- > lib/xe/xe_query.c | 20 ++++++++++++++++++++ > lib/xe/xe_query.h | 5 +++++ > 3 files changed, 35 insertions(+), 8 deletions(-) > > diff --git a/lib/intel_pat.c b/lib/intel_pat.c > index 8660a2515..e758ce9be 100644 > --- a/lib/intel_pat.c > +++ b/lib/intel_pat.c > @@ -6,6 +6,7 @@ > #include > #include "igt.h" > #include "intel_pat.h" > +#include "xe/xe_query.h" > > /** > * xe_get_pat_sw_config - Helper to read PAT (Page Attribute Table) soft= ware configuration > @@ -99,17 +100,18 @@ static void intel_get_pat_idx(int fd, struct intel_p= at_cache *pat) > uint16_t dev_id; > > /* > - * For Xe driver, query the kernel's PAT software configuration > - * via debugfs. The kernel is the authoritative source for PAT > - * indices, accounting for platform-specific workarounds > - * (e.g. Wa_16023588340) at runtime. > + * For Xe, use the PAT cache stored in struct xe_device. > + * xe_device_get() populates the cache while still root; forked > + * children that inherit the xe_device can use it post-drop_root(). > */ > if (is_xe_device(fd)) { > - int32_t parsed =3D xe_get_pat_sw_config(fd, pat); > + struct xe_device *xe_dev =3D xe_device_get(fd); > > - igt_assert_f(parsed > 0, > - "Failed to get PAT sw_config from debugfs (parsed=3D%d)\n", > - parsed); > + igt_assert_f(xe_dev->pat_cache, > + "PAT sw_config not available for fd %d -- " > + "debugfs not accessible (missing root or not mounted?)\n", > + fd); Why print fd here? What information is it providing? Do we need it? > + *pat =3D *xe_dev->pat_cache; > return; > } > > diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c > index 981d76948..ea8889f3b 100644 > --- a/lib/xe/xe_query.c > +++ b/lib/xe/xe_query.c > @@ -22,6 +22,7 @@ > #include "ioctl_wrappers.h" > #include "igt_map.h" > > +#include "intel_pat.h" nit but let's move this just below igt_map.h (i.e. move up by one line without a blank line) so that the xe includes below stay separate. > #include "xe_query.h" > #include "xe_ioctl.h" > > @@ -235,6 +236,7 @@ static void xe_device_free(struct xe_device *xe_dev) > free(xe_dev->mem_regions); > free(xe_dev->vram_size); > free(xe_dev->eu_stall); > + free(xe_dev->pat_cache); > free(xe_dev); > } > > @@ -299,6 +301,24 @@ struct xe_device *xe_device_get(int fd) > xe_dev->default_alignment =3D __mem_default_alignment(xe_dev->mem_regions= ); > xe_dev->has_vram =3D __mem_has_vram(xe_dev->mem_regions); > > + /* > + * Populate the PAT cache while we still have sufficient privileges > + * to read debugfs. Forked children that inherit this xe_device > + * (via fork()) will be able to use the cached values even after > + * dropping root with igt_drop_root(). pat_cache is left NULL if > + * debugfs is not accessible. > + * > + * FIXME: the cache is keyed by fd; for multi-GPU support this > + * should be extended to cache PAT entries by platform version/ > + * revision instead. > + */ > + xe_dev->pat_cache =3D calloc(1, sizeof(*xe_dev->pat_cache)); > + igt_assert(xe_dev->pat_cache); > + if (xe_get_pat_sw_config(xe_dev->fd, xe_dev->pat_cache) <=3D 0) { > + free(xe_dev->pat_cache); > + xe_dev->pat_cache =3D NULL; > + } > + > /* We may get here from multiple threads, use first cached xe_dev */ > pthread_mutex_lock(&cache.cache_mutex); > prev =3D find_in_cache_unlocked(fd); > diff --git a/lib/xe/xe_query.h b/lib/xe/xe_query.h > index d7a9f95f9..53a32e4b1 100644 > --- a/lib/xe/xe_query.h > +++ b/lib/xe/xe_query.h > @@ -9,6 +9,8 @@ > #ifndef XE_QUERY_H > #define XE_QUERY_H > > +#include "intel_pat.h" > + Why include this before the <> includes below. Please include this if it is needed below with the "" includes in alphabetical order. You likely don't even need the include, you can just use the following forward struct declaration: struct intel_pat_cache; That's it. Because we have a struct pointer. Basically the idea for a forward declaration is to not create a potential circular dependency in the include files. Put the forward declaration below all the includes. > #include > #include > > @@ -74,6 +76,9 @@ struct xe_device { > > /** @dev_id: Device id of xe device */ > uint16_t dev_id; > + > + /** @pat_cache: cached PAT index configuration, NULL if not yet populat= ed */ > + struct intel_pat_cache *pat_cache; > }; > > #define xe_for_each_engine(__fd, __hwe) \ > -- > 2.50.1 > I'll take another look at the next version and R-b it everything is ok. Thanks. -- Ashutosh