public inbox for igt-dev@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v2 i-g-t] lib/intel_pat: Cache PAT config for unprivileged processes
@ 2026-03-05 12:25 himanshu.girotra
  2026-03-05 13:23 ` Zbigniew Kempczyński
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: himanshu.girotra @ 2026-03-05 12:25 UTC (permalink / raw)
  To: matthew.d.roper, x.wang, igt-dev, nishit.sharma, ashutosh.dixit

From: Himanshu Girotra <himanshu.girotra@intel.com>

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 — xe_device_get() already
    populates the cache, so call it directly in xe_oa before
    igt_drop_root() (Ashutosh Dixit)

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Xin Wang <x.wang@intel.com>
Cc: Nishit Sharma <nishit.sharma@intel.com>
Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>

Signed-off-by: Himanshu Girotra <himanshu.girotra@intel.com>
---
 lib/intel_pat.c     | 18 ++++++++++--------
 lib/xe/xe_query.c   | 10 ++++++++++
 lib/xe/xe_query.h   |  8 ++++++++
 tests/intel/xe_oa.c |  8 ++++++++
 4 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/lib/intel_pat.c b/lib/intel_pat.c
index 8660a2515..1dcb84cc6 100644
--- a/lib/intel_pat.c
+++ b/lib/intel_pat.c
@@ -6,6 +6,7 @@
 #include <fcntl.h>
 #include "igt.h"
 #include "intel_pat.h"
+#include "xe/xe_query.h"
 
 /**
  * xe_get_pat_sw_config - Helper to read PAT (Page Attribute Table) software configuration
@@ -99,17 +100,18 @@ static void intel_get_pat_idx(int fd, struct intel_pat_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 = xe_get_pat_sw_config(fd, pat);
+		struct xe_device *xe_dev = xe_device_get(fd);
 
-		igt_assert_f(parsed > 0,
-			     "Failed to get PAT sw_config from debugfs (parsed=%d)\n",
-			     parsed);
+		igt_assert_f(xe_dev->pat_cached,
+			     "PAT not cached in xe_device for fd %d -- "
+			     "was xe_device_get() called before igt_drop_root()?\n",
+			     fd);
+		*pat = xe_dev->pat_cache;
 		return;
 	}
 
diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
index 981d76948..256ec1d4d 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"
 #include "xe_query.h"
 #include "xe_ioctl.h"
 
@@ -299,6 +300,15 @@ struct xe_device *xe_device_get(int fd)
 	xe_dev->default_alignment = __mem_default_alignment(xe_dev->mem_regions);
 	xe_dev->has_vram = __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().
+	 */
+	if (xe_get_pat_sw_config(xe_dev->fd, &xe_dev->pat_cache) > 0)
+		xe_dev->pat_cached = true;
+
 	/* We may get here from multiple threads, use first cached xe_dev */
 	pthread_mutex_lock(&cache.cache_mutex);
 	prev = find_in_cache_unlocked(fd);
diff --git a/lib/xe/xe_query.h b/lib/xe/xe_query.h
index d7a9f95f9..97bcf0d32 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"
+
 #include <stdint.h>
 #include <xe_drm.h>
 
@@ -74,6 +76,12 @@ struct xe_device {
 
 	/** @dev_id: Device id of xe device */
 	uint16_t dev_id;
+
+	/** @pat_cache: cached PAT index configuration */
+	struct intel_pat_cache pat_cache;
+
+	/** @pat_cached: true once @pat_cache has been populated */
+	bool pat_cached;
 };
 
 #define xe_for_each_engine(__fd, __hwe) \
diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
index 927f3f4f2..9ce828619 100644
--- a/tests/intel/xe_oa.c
+++ b/tests/intel/xe_oa.c
@@ -3453,6 +3453,14 @@ test_single_ctx_render_target_writes_a_counter(const struct drm_xe_oa_unit *oau)
 			/* A local device for local resources. */
 			drm_fd = drm_reopen_driver(drm_fd);
 
+			/*
+			 * Populate the xe_device cache (including PAT) for
+			 * the new fd while we still have root privileges.
+			 * intel_get_pat_idx_*() reads PAT from xe_device,
+			 * so this must happen before igt_drop_root().
+			 */
+			xe_device_get(drm_fd);
+
 			igt_drop_root();
 
 			single_ctx_helper(oau);
-- 
2.50.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2 i-g-t] lib/intel_pat: Cache PAT config for unprivileged processes
  2026-03-05 12:25 [PATCH v2 i-g-t] lib/intel_pat: Cache PAT config for unprivileged processes himanshu.girotra
@ 2026-03-05 13:23 ` Zbigniew Kempczyński
  2026-03-05 13:48 ` Dixit, Ashutosh
  2026-03-05 13:57 ` Dixit, Ashutosh
  2 siblings, 0 replies; 4+ messages in thread
From: Zbigniew Kempczyński @ 2026-03-05 13:23 UTC (permalink / raw)
  To: himanshu.girotra
  Cc: matthew.d.roper, x.wang, igt-dev, nishit.sharma, ashutosh.dixit

On Thu, Mar 05, 2026 at 05:55:48PM +0530, himanshu.girotra@intel.com wrote:
> From: Himanshu Girotra <himanshu.girotra@intel.com>
> 
> 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 — xe_device_get() already
>     populates the cache, so call it directly in xe_oa before
>     igt_drop_root() (Ashutosh Dixit)
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Xin Wang <x.wang@intel.com>
> Cc: Nishit Sharma <nishit.sharma@intel.com>
> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> 
> Signed-off-by: Himanshu Girotra <himanshu.girotra@intel.com>
> ---
>  lib/intel_pat.c     | 18 ++++++++++--------
>  lib/xe/xe_query.c   | 10 ++++++++++
>  lib/xe/xe_query.h   |  8 ++++++++
>  tests/intel/xe_oa.c |  8 ++++++++
>  4 files changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/intel_pat.c b/lib/intel_pat.c
> index 8660a2515..1dcb84cc6 100644
> --- a/lib/intel_pat.c
> +++ b/lib/intel_pat.c
> @@ -6,6 +6,7 @@
>  #include <fcntl.h>
>  #include "igt.h"
>  #include "intel_pat.h"
> +#include "xe/xe_query.h"
>  
>  /**
>   * xe_get_pat_sw_config - Helper to read PAT (Page Attribute Table) software configuration
> @@ -99,17 +100,18 @@ static void intel_get_pat_idx(int fd, struct intel_pat_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 = xe_get_pat_sw_config(fd, pat);
> +		struct xe_device *xe_dev = xe_device_get(fd);
>  
> -		igt_assert_f(parsed > 0,
> -			     "Failed to get PAT sw_config from debugfs (parsed=%d)\n",
> -			     parsed);
> +		igt_assert_f(xe_dev->pat_cached,
> +			     "PAT not cached in xe_device for fd %d -- "
> +			     "was xe_device_get() called before igt_drop_root()?\n",
> +			     fd);
> +		*pat = xe_dev->pat_cache;
>  		return;
>  	}
>  
> diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> index 981d76948..256ec1d4d 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"
>  #include "xe_query.h"
>  #include "xe_ioctl.h"
>  
> @@ -299,6 +300,15 @@ struct xe_device *xe_device_get(int fd)
>  	xe_dev->default_alignment = __mem_default_alignment(xe_dev->mem_regions);
>  	xe_dev->has_vram = __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().
> +	 */
> +	if (xe_get_pat_sw_config(xe_dev->fd, &xe_dev->pat_cache) > 0)
> +		xe_dev->pat_cached = true;

I think you should add some 'fixme' note above because it will work
only for single gpu. This is acceptable atm, but it should be fixed
in case of more gpus (by adding list which caches pat entries for
ver/rel).

So for this fix:

Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>

--
Zbigniew

> +
>  	/* We may get here from multiple threads, use first cached xe_dev */
>  	pthread_mutex_lock(&cache.cache_mutex);
>  	prev = find_in_cache_unlocked(fd);
> diff --git a/lib/xe/xe_query.h b/lib/xe/xe_query.h
> index d7a9f95f9..97bcf0d32 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"
> +
>  #include <stdint.h>
>  #include <xe_drm.h>
>  
> @@ -74,6 +76,12 @@ struct xe_device {
>  
>  	/** @dev_id: Device id of xe device */
>  	uint16_t dev_id;
> +
> +	/** @pat_cache: cached PAT index configuration */
> +	struct intel_pat_cache pat_cache;
> +
> +	/** @pat_cached: true once @pat_cache has been populated */
> +	bool pat_cached;
>  };
>  
>  #define xe_for_each_engine(__fd, __hwe) \
> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> index 927f3f4f2..9ce828619 100644
> --- a/tests/intel/xe_oa.c
> +++ b/tests/intel/xe_oa.c
> @@ -3453,6 +3453,14 @@ test_single_ctx_render_target_writes_a_counter(const struct drm_xe_oa_unit *oau)
>  			/* A local device for local resources. */
>  			drm_fd = drm_reopen_driver(drm_fd);
>  
> +			/*
> +			 * Populate the xe_device cache (including PAT) for
> +			 * the new fd while we still have root privileges.
> +			 * intel_get_pat_idx_*() reads PAT from xe_device,
> +			 * so this must happen before igt_drop_root().
> +			 */
> +			xe_device_get(drm_fd);
> +
>  			igt_drop_root();
>  
>  			single_ctx_helper(oau);
> -- 
> 2.50.1
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2 i-g-t] lib/intel_pat: Cache PAT config for unprivileged processes
  2026-03-05 12:25 [PATCH v2 i-g-t] lib/intel_pat: Cache PAT config for unprivileged processes himanshu.girotra
  2026-03-05 13:23 ` Zbigniew Kempczyński
@ 2026-03-05 13:48 ` Dixit, Ashutosh
  2026-03-05 13:57 ` Dixit, Ashutosh
  2 siblings, 0 replies; 4+ messages in thread
From: Dixit, Ashutosh @ 2026-03-05 13:48 UTC (permalink / raw)
  To: himanshu.girotra
  Cc: matthew.d.roper, x.wang, igt-dev, nishit.sharma,
	Zbigniew Kempczynski

On Thu, 05 Mar 2026 04:25:48 -0800, <himanshu.girotra@intel.com> wrote:
>
> diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> index 981d76948..256ec1d4d 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"
>  #include "xe_query.h"
>  #include "xe_ioctl.h"
>
> @@ -299,6 +300,15 @@ struct xe_device *xe_device_get(int fd)
>	xe_dev->default_alignment = __mem_default_alignment(xe_dev->mem_regions);
>	xe_dev->has_vram = __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().
> +	 */
> +	if (xe_get_pat_sw_config(xe_dev->fd, &xe_dev->pat_cache) > 0)
> +		xe_dev->pat_cached = true;
> +
x>	/* We may get here from multiple threads, use first cached xe_dev */
>	pthread_mutex_lock(&cache.cache_mutex);
>	prev = find_in_cache_unlocked(fd);
> diff --git a/lib/xe/xe_query.h b/lib/xe/xe_query.h
> index d7a9f95f9..97bcf0d32 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"
> +
>  #include <stdint.h>
>  #include <xe_drm.h>
>
> @@ -74,6 +76,12 @@ struct xe_device {
>
>	/** @dev_id: Device id of xe device */
>	uint16_t dev_id;
> +
> +	/** @pat_cache: cached PAT index configuration */
> +	struct intel_pat_cache pat_cache;

This should just be "struct intel_pat_cache *pat_cache";

> +
> +	/** @pat_cached: true once @pat_cache has been populated */
> +	bool pat_cached;

And then we don't need this, we can just check for NULL pointer, like has
been done for other members of 'struct xe_device'.

>  };
>
>  #define xe_for_each_engine(__fd, __hwe) \
> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> index 927f3f4f2..9ce828619 100644
> --- a/tests/intel/xe_oa.c
> +++ b/tests/intel/xe_oa.c
> @@ -3453,6 +3453,14 @@ test_single_ctx_render_target_writes_a_counter(const struct drm_xe_oa_unit *oau)
>			/* A local device for local resources. */
>			drm_fd = drm_reopen_driver(drm_fd);
>
> +			/*
> +			 * Populate the xe_device cache (including PAT) for
> +			 * the new fd while we still have root privileges.
> +			 * intel_get_pat_idx_*() reads PAT from xe_device,
> +			 * so this must happen before igt_drop_root().
> +			 */
> +			xe_device_get(drm_fd);

This is not needed, this is done in igt_main() in xe_oa.c. Also you have
done a xe_device_get() in lib/intel_pat.c in this patch itself.

> +
>			igt_drop_root();
>
>			single_ctx_helper(oau);
> --
> 2.50.1
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2 i-g-t] lib/intel_pat: Cache PAT config for unprivileged processes
  2026-03-05 12:25 [PATCH v2 i-g-t] lib/intel_pat: Cache PAT config for unprivileged processes himanshu.girotra
  2026-03-05 13:23 ` Zbigniew Kempczyński
  2026-03-05 13:48 ` Dixit, Ashutosh
@ 2026-03-05 13:57 ` Dixit, Ashutosh
  2 siblings, 0 replies; 4+ messages in thread
From: Dixit, Ashutosh @ 2026-03-05 13:57 UTC (permalink / raw)
  To: himanshu.girotra; +Cc: matthew.d.roper, x.wang, igt-dev, nishit.sharma

On Thu, 05 Mar 2026 04:25:48 -0800, <himanshu.girotra@intel.com> wrote:
>
> diff --git a/lib/intel_pat.c b/lib/intel_pat.c
> index 8660a2515..1dcb84cc6 100644
> --- a/lib/intel_pat.c
> +++ b/lib/intel_pat.c
> @@ -6,6 +6,7 @@
>  #include <fcntl.h>
>  #include "igt.h"
>  #include "intel_pat.h"
> +#include "xe/xe_query.h"
>
>  /**
>   * xe_get_pat_sw_config - Helper to read PAT (Page Attribute Table) software configuration
> @@ -99,17 +100,18 @@ static void intel_get_pat_idx(int fd, struct intel_pat_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 = xe_get_pat_sw_config(fd, pat);
> +		struct xe_device *xe_dev = xe_device_get(fd);
>
> -		igt_assert_f(parsed > 0,
> -			     "Failed to get PAT sw_config from debugfs (parsed=%d)\n",
> -			     parsed);
> +		igt_assert_f(xe_dev->pat_cached,
> +			     "PAT not cached in xe_device for fd %d -- "
> +			     "was xe_device_get() called before igt_drop_root()?\n",
> +			     fd);

Maybe change this print too since we are doing xe_device_get() right above.

> +		*pat = xe_dev->pat_cache;
>		return;
>	}
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-03-05 13:57 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-05 12:25 [PATCH v2 i-g-t] lib/intel_pat: Cache PAT config for unprivileged processes himanshu.girotra
2026-03-05 13:23 ` Zbigniew Kempczyński
2026-03-05 13:48 ` Dixit, Ashutosh
2026-03-05 13:57 ` Dixit, Ashutosh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox