[PATCH v15 0/1] drm/i915: Allow user to set cache at BO creation

[PATCH v15 0/1] drm/i915: Allow user to set cache at BO creation

[fei.yang]

From: Fei Yang <fei.yang@intel.com>

This series introduce a new extension for GEM_CREATE,
1. end support for set caching ioctl [PATCH 1/2]
2. add set_pat extension for gem_create [PATCH 2/2]

v2: drop one patch that was merged separately
    commit 341ad0e8e254 ("drm/i915/mtl: Add PTE encode function")
v3: rebased on https://patchwork.freedesktop.org/series/117082/
v4: fix missing unlock introduced in v3, and
    solve a rebase conflict
v5: replace obj->cache_level with pat_set_by_user,
    fix i915_cache_level_str() for legacy platforms.
v6: rebased on https://patchwork.freedesktop.org/series/117480/
v7: rebased on https://patchwork.freedesktop.org/series/117528/
v8: dropped the two dependent patches that has been merged
    separately. Add IGT link and Tested-by (MESA).
v9: addressing comments (Andi)
v10: acked-by and tested-by MESA
v11: drop "end support for set caching ioctl" (merged)
     remove tools/include/uapi/drm/i915_drm.h
v12: drop Bspec reference in comment. add to commit message instead
v13: sent to test with igt@gem_create@create-ext-set-pat
v14: sent to test with igt@gem_create@create-ext-set-pat
v15: update commit message with documentation note and t-b/a-b from
     Media driver folks.

Fei Yang (1):
  drm/i915: Allow user to set cache at BO creation

 drivers/gpu/drm/i915/gem/i915_gem_create.c | 36 +++++++++++++++++++
 drivers/gpu/drm/i915/gem/i915_gem_object.c |  6 ++++
 include/uapi/drm/i915_drm.h                | 41 ++++++++++++++++++++++
 3 files changed, 83 insertions(+)

-- 
2.25.1

[PATCH v15 1/1] drm/i915: Allow user to set cache at BO creation

[fei.yang]

From: Fei Yang <fei.yang@intel.com>

To comply with the design that buffer objects shall have immutable
cache setting through out their life cycle, {set, get}_caching ioctl's
are no longer supported from MTL onward. With that change caching
policy can only be set at object creation time. The current code
applies a default (platform dependent) cache setting for all objects.
However this is not optimal for performance tuning. The patch extends
the existing gem_create uAPI to let user set PAT index for the object
at creation time.
The new extension is platform independent, so UMD's can switch to using
this extension for older platforms as well, while {set, get}_caching are
still supported on these legacy paltforms for compatibility reason.

Note: The detailed description of PAT index is missing in current PRM
even for older hardware and will be added by the next PRM update under
chapter name "Memory Views".

BSpec: 45101

Mesa support has been submitted in this merge request:
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878

The media driver is supported by the following commits:
https://github.com/intel/media-driver/commit/92c00a857433ebb34ec575e9834f473c6fcb6341
https://github.com/intel/media-driver/commit/fd375cf2c5e1f6bf6b43258ff797b3134aadc9fd
https://github.com/intel/media-driver/commit/08dd244b22484770a33464c2c8ae85430e548000

The IGT test related to this change is
igt@gem_create@create-ext-set-pat

Signed-off-by: Fei Yang <fei.yang@intel.com>
Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Andi Shyti <andi.shyti@linux.intel.com>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
Acked-by: Jordan Justen <jordan.l.justen@intel.com>
Tested-by: Jordan Justen <jordan.l.justen@intel.com>
Acked-by: Carl Zhang <carl.zhang@intel.com>
Tested-by: Lihao Gu <lihao.gu@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_create.c | 36 +++++++++++++++++++
 drivers/gpu/drm/i915/gem/i915_gem_object.c |  6 ++++
 include/uapi/drm/i915_drm.h                | 41 ++++++++++++++++++++++
 3 files changed, 83 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
index bfe1dbda4cb7..644a936248ad 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
@@ -245,6 +245,7 @@ struct create_ext {
 	unsigned int n_placements;
 	unsigned int placement_mask;
 	unsigned long flags;
+	unsigned int pat_index;
 };
 
 static void repr_placements(char *buf, size_t size,
@@ -394,11 +395,39 @@ static int ext_set_protected(struct i915_user_extension __user *base, void *data
 	return 0;
 }
 
+static int ext_set_pat(struct i915_user_extension __user *base, void *data)
+{
+	struct create_ext *ext_data = data;
+	struct drm_i915_private *i915 = ext_data->i915;
+	struct drm_i915_gem_create_ext_set_pat ext;
+	unsigned int max_pat_index;
+
+	BUILD_BUG_ON(sizeof(struct drm_i915_gem_create_ext_set_pat) !=
+		     offsetofend(struct drm_i915_gem_create_ext_set_pat, rsvd));
+
+	if (copy_from_user(&ext, base, sizeof(ext)))
+		return -EFAULT;
+
+	max_pat_index = INTEL_INFO(i915)->max_pat_index;
+
+	if (ext.pat_index > max_pat_index) {
+		drm_dbg(&i915->drm, "PAT index is invalid: %u\n",
+			ext.pat_index);
+		return -EINVAL;
+	}
+
+	ext_data->pat_index = ext.pat_index;
+
+	return 0;
+}
+
 static const i915_user_extension_fn create_extensions[] = {
 	[I915_GEM_CREATE_EXT_MEMORY_REGIONS] = ext_set_placements,
 	[I915_GEM_CREATE_EXT_PROTECTED_CONTENT] = ext_set_protected,
+	[I915_GEM_CREATE_EXT_SET_PAT] = ext_set_pat,
 };
 
+#define PAT_INDEX_NOT_SET	0xffff
 /**
  * i915_gem_create_ext_ioctl - Creates a new mm object and returns a handle to it.
  * @dev: drm device pointer
@@ -418,6 +447,7 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
 	if (args->flags & ~I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS)
 		return -EINVAL;
 
+	ext_data.pat_index = PAT_INDEX_NOT_SET;
 	ret = i915_user_extensions(u64_to_user_ptr(args->extensions),
 				   create_extensions,
 				   ARRAY_SIZE(create_extensions),
@@ -454,5 +484,11 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
 	if (IS_ERR(obj))
 		return PTR_ERR(obj);
 
+	if (ext_data.pat_index != PAT_INDEX_NOT_SET) {
+		i915_gem_object_set_pat_index(obj, ext_data.pat_index);
+		/* Mark pat_index is set by UMD */
+		obj->pat_set_by_user = true;
+	}
+
 	return i915_gem_publish(obj, file, &args->size, &args->handle);
 }
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 46a19b099ec8..97ac6fb37958 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -208,6 +208,12 @@ bool i915_gem_object_can_bypass_llc(struct drm_i915_gem_object *obj)
 	if (!(obj->flags & I915_BO_ALLOC_USER))
 		return false;
 
+	/*
+	 * Always flush cache for UMD objects at creation time.
+	 */
+	if (obj->pat_set_by_user)
+		return true;
+
 	/*
 	 * EHL and JSL add the 'Bypass LLC' MOCS entry, which should make it
 	 * possible for userspace to bypass the GTT caching bits set by the
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index f31dfacde601..4083a23e0614 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -3679,9 +3679,13 @@ struct drm_i915_gem_create_ext {
 	 *
 	 * For I915_GEM_CREATE_EXT_PROTECTED_CONTENT usage see
 	 * struct drm_i915_gem_create_ext_protected_content.
+	 *
+	 * For I915_GEM_CREATE_EXT_SET_PAT usage see
+	 * struct drm_i915_gem_create_ext_set_pat.
 	 */
 #define I915_GEM_CREATE_EXT_MEMORY_REGIONS 0
 #define I915_GEM_CREATE_EXT_PROTECTED_CONTENT 1
+#define I915_GEM_CREATE_EXT_SET_PAT 2
 	__u64 extensions;
 };
 
@@ -3796,6 +3800,43 @@ struct drm_i915_gem_create_ext_protected_content {
 	__u32 flags;
 };
 
+/**
+ * struct drm_i915_gem_create_ext_set_pat - The
+ * I915_GEM_CREATE_EXT_SET_PAT extension.
+ *
+ * If this extension is provided, the specified caching policy (PAT index) is
+ * applied to the buffer object.
+ *
+ * Below is an example on how to create an object with specific caching policy:
+ *
+ * .. code-block:: C
+ *
+ *      struct drm_i915_gem_create_ext_set_pat set_pat_ext = {
+ *              .base = { .name = I915_GEM_CREATE_EXT_SET_PAT },
+ *              .pat_index = 0,
+ *      };
+ *      struct drm_i915_gem_create_ext create_ext = {
+ *              .size = PAGE_SIZE,
+ *              .extensions = (uintptr_t)&set_pat_ext,
+ *      };
+ *
+ *      int err = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE_EXT, &create_ext);
+ *      if (err) ...
+ */
+struct drm_i915_gem_create_ext_set_pat {
+	/** @base: Extension link. See struct i915_user_extension. */
+	struct i915_user_extension base;
+	/**
+	 * @pat_index: PAT index to be set
+	 * PAT index is a bit field in Page Table Entry to control caching
+	 * behaviors for GPU accesses. The definition of PAT index is
+	 * platform dependent and can be found in hardware specifications,
+	 */
+	__u32 pat_index;
+	/** @rsvd: reserved for future use */
+	__u32 rsvd;
+};
+
 /* ID of the protected content session managed by i915 when PXP is active */
 #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
 
-- 
2.25.1

Re: [PATCH v15 1/1] drm/i915: Allow user to set cache at BO creation

[Andi Shyti]

Hi Fei,

On Wed, May 31, 2023 at 10:10:08AM -0700, fei.yang@intel.com wrote:
> From: Fei Yang <fei.yang@intel.com>
> 
> To comply with the design that buffer objects shall have immutable
> cache setting through out their life cycle, {set, get}_caching ioctl's
> are no longer supported from MTL onward. With that change caching
> policy can only be set at object creation time. The current code
> applies a default (platform dependent) cache setting for all objects.
> However this is not optimal for performance tuning. The patch extends
> the existing gem_create uAPI to let user set PAT index for the object
> at creation time.
> The new extension is platform independent, so UMD's can switch to using
> this extension for older platforms as well, while {set, get}_caching are
> still supported on these legacy paltforms for compatibility reason.
> 
> Note: The detailed description of PAT index is missing in current PRM
> even for older hardware and will be added by the next PRM update under
> chapter name "Memory Views".

Documentation has been updated:

https://www.intel.com/content/www/us/en/docs/graphics-for-linux/developer-reference/1-0/tiger-lake.html

If it's OK with you, before pushing I can replace this Note with:

"
The documentation related to the PAT/MOCS tables is currently
available for Tiger Lake here:

https://www.intel.com/content/www/us/en/docs/graphics-for-linux/developer-reference/1-0/tiger-lake.html
"

Thank you Tvrtko for the intution you had about the documentation
and for pushing for the update. It is greate to have this uAPI
well documented!

Andi

> BSpec: 45101
> 
> Mesa support has been submitted in this merge request:
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
> 
> The media driver is supported by the following commits:
> https://github.com/intel/media-driver/commit/92c00a857433ebb34ec575e9834f473c6fcb6341
> https://github.com/intel/media-driver/commit/fd375cf2c5e1f6bf6b43258ff797b3134aadc9fd
> https://github.com/intel/media-driver/commit/08dd244b22484770a33464c2c8ae85430e548000
> 
> The IGT test related to this change is
> igt@gem_create@create-ext-set-pat
> 
> Signed-off-by: Fei Yang <fei.yang@intel.com>
> Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
> Acked-by: Jordan Justen <jordan.l.justen@intel.com>
> Tested-by: Jordan Justen <jordan.l.justen@intel.com>
> Acked-by: Carl Zhang <carl.zhang@intel.com>
> Tested-by: Lihao Gu <lihao.gu@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_create.c | 36 +++++++++++++++++++
>  drivers/gpu/drm/i915/gem/i915_gem_object.c |  6 ++++
>  include/uapi/drm/i915_drm.h                | 41 ++++++++++++++++++++++
>  3 files changed, 83 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> index bfe1dbda4cb7..644a936248ad 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> @@ -245,6 +245,7 @@ struct create_ext {
>  	unsigned int n_placements;
>  	unsigned int placement_mask;
>  	unsigned long flags;
> +	unsigned int pat_index;
>  };
>  
>  static void repr_placements(char *buf, size_t size,
> @@ -394,11 +395,39 @@ static int ext_set_protected(struct i915_user_extension __user *base, void *data
>  	return 0;
>  }
>  
> +static int ext_set_pat(struct i915_user_extension __user *base, void *data)
> +{
> +	struct create_ext *ext_data = data;
> +	struct drm_i915_private *i915 = ext_data->i915;
> +	struct drm_i915_gem_create_ext_set_pat ext;
> +	unsigned int max_pat_index;
> +
> +	BUILD_BUG_ON(sizeof(struct drm_i915_gem_create_ext_set_pat) !=
> +		     offsetofend(struct drm_i915_gem_create_ext_set_pat, rsvd));
> +
> +	if (copy_from_user(&ext, base, sizeof(ext)))
> +		return -EFAULT;
> +
> +	max_pat_index = INTEL_INFO(i915)->max_pat_index;
> +
> +	if (ext.pat_index > max_pat_index) {
> +		drm_dbg(&i915->drm, "PAT index is invalid: %u\n",
> +			ext.pat_index);
> +		return -EINVAL;
> +	}
> +
> +	ext_data->pat_index = ext.pat_index;
> +
> +	return 0;
> +}
> +
>  static const i915_user_extension_fn create_extensions[] = {
>  	[I915_GEM_CREATE_EXT_MEMORY_REGIONS] = ext_set_placements,
>  	[I915_GEM_CREATE_EXT_PROTECTED_CONTENT] = ext_set_protected,
> +	[I915_GEM_CREATE_EXT_SET_PAT] = ext_set_pat,
>  };
>  
> +#define PAT_INDEX_NOT_SET	0xffff
>  /**
>   * i915_gem_create_ext_ioctl - Creates a new mm object and returns a handle to it.
>   * @dev: drm device pointer
> @@ -418,6 +447,7 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
>  	if (args->flags & ~I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS)
>  		return -EINVAL;
>  
> +	ext_data.pat_index = PAT_INDEX_NOT_SET;
>  	ret = i915_user_extensions(u64_to_user_ptr(args->extensions),
>  				   create_extensions,
>  				   ARRAY_SIZE(create_extensions),
> @@ -454,5 +484,11 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
>  	if (IS_ERR(obj))
>  		return PTR_ERR(obj);
>  
> +	if (ext_data.pat_index != PAT_INDEX_NOT_SET) {
> +		i915_gem_object_set_pat_index(obj, ext_data.pat_index);
> +		/* Mark pat_index is set by UMD */
> +		obj->pat_set_by_user = true;
> +	}
> +
>  	return i915_gem_publish(obj, file, &args->size, &args->handle);
>  }
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index 46a19b099ec8..97ac6fb37958 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -208,6 +208,12 @@ bool i915_gem_object_can_bypass_llc(struct drm_i915_gem_object *obj)
>  	if (!(obj->flags & I915_BO_ALLOC_USER))
>  		return false;
>  
> +	/*
> +	 * Always flush cache for UMD objects at creation time.
> +	 */
> +	if (obj->pat_set_by_user)
> +		return true;
> +
>  	/*
>  	 * EHL and JSL add the 'Bypass LLC' MOCS entry, which should make it
>  	 * possible for userspace to bypass the GTT caching bits set by the
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index f31dfacde601..4083a23e0614 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -3679,9 +3679,13 @@ struct drm_i915_gem_create_ext {
>  	 *
>  	 * For I915_GEM_CREATE_EXT_PROTECTED_CONTENT usage see
>  	 * struct drm_i915_gem_create_ext_protected_content.
> +	 *
> +	 * For I915_GEM_CREATE_EXT_SET_PAT usage see
> +	 * struct drm_i915_gem_create_ext_set_pat.
>  	 */
>  #define I915_GEM_CREATE_EXT_MEMORY_REGIONS 0
>  #define I915_GEM_CREATE_EXT_PROTECTED_CONTENT 1
> +#define I915_GEM_CREATE_EXT_SET_PAT 2
>  	__u64 extensions;
>  };
>  
> @@ -3796,6 +3800,43 @@ struct drm_i915_gem_create_ext_protected_content {
>  	__u32 flags;
>  };
>  
> +/**
> + * struct drm_i915_gem_create_ext_set_pat - The
> + * I915_GEM_CREATE_EXT_SET_PAT extension.
> + *
> + * If this extension is provided, the specified caching policy (PAT index) is
> + * applied to the buffer object.
> + *
> + * Below is an example on how to create an object with specific caching policy:
> + *
> + * .. code-block:: C
> + *
> + *      struct drm_i915_gem_create_ext_set_pat set_pat_ext = {
> + *              .base = { .name = I915_GEM_CREATE_EXT_SET_PAT },
> + *              .pat_index = 0,
> + *      };
> + *      struct drm_i915_gem_create_ext create_ext = {
> + *              .size = PAGE_SIZE,
> + *              .extensions = (uintptr_t)&set_pat_ext,
> + *      };
> + *
> + *      int err = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE_EXT, &create_ext);
> + *      if (err) ...
> + */
> +struct drm_i915_gem_create_ext_set_pat {
> +	/** @base: Extension link. See struct i915_user_extension. */
> +	struct i915_user_extension base;
> +	/**
> +	 * @pat_index: PAT index to be set
> +	 * PAT index is a bit field in Page Table Entry to control caching
> +	 * behaviors for GPU accesses. The definition of PAT index is
> +	 * platform dependent and can be found in hardware specifications,
> +	 */
> +	__u32 pat_index;
> +	/** @rsvd: reserved for future use */
> +	__u32 rsvd;
> +};
> +
>  /* ID of the protected content session managed by i915 when PXP is active */
>  #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
>  
> -- 
> 2.25.1

Re: [PATCH v15 1/1] drm/i915: Allow user to set cache at BO creation

[Yang, Fei]

[-- Attachment #1: Type: text/plain, Size: 9180 bytes --]

> Hi Fei,
>
> On Wed, May 31, 2023 at 10:10:08AM -0700, fei.yang@intel.com wrote:
>> From: Fei Yang <fei.yang@intel.com>
>>
>> To comply with the design that buffer objects shall have immutable
>> cache setting through out their life cycle, {set, get}_caching ioctl's
>> are no longer supported from MTL onward. With that change caching
>> policy can only be set at object creation time. The current code
>> applies a default (platform dependent) cache setting for all objects.
>> However this is not optimal for performance tuning. The patch extends
>> the existing gem_create uAPI to let user set PAT index for the object
>> at creation time.
>> The new extension is platform independent, so UMD's can switch to using
>> this extension for older platforms as well, while {set, get}_caching are
>> still supported on these legacy paltforms for compatibility reason.
>>
>> Note: The detailed description of PAT index is missing in current PRM
>> even for older hardware and will be added by the next PRM update under
>> chapter name "Memory Views".
>
> Documentation has been updated:
>
> https://www.intel.com/content/www/us/en/docs/graphics-for-linux/developer-reference/1-0/tiger-lake.html
>
> If it's OK with you, before pushing I can replace this Note with:
>
>"
>The documentation related to the PAT/MOCS tables is currently
>available for Tiger Lake here:
>
>https://www.intel.com/content/www/us/en/docs/graphics-for-linux/developer-reference/1-0/tiger-lake.html
>"

Looks good to me. Thank you Andi and Tvrtko for all your help.

-Fei

>Thank you Tvrtko for the intution you had about the documentation
>and for pushing for the update. It is greate to have this uAPI
>well documented!
>
>Andi
>
>> BSpec: 45101
>>
>> Mesa support has been submitted in this merge request:
>> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
>>
>> The media driver is supported by the following commits:
>> https://github.com/intel/media-driver/commit/92c00a857433ebb34ec575e9834f473c6fcb6341
>> https://github.com/intel/media-driver/commit/fd375cf2c5e1f6bf6b43258ff797b3134aadc9fd
>> https://github.com/intel/media-driver/commit/08dd244b22484770a33464c2c8ae85430e548000
>>
>> The IGT test related to this change is
>> igt@gem_create@create-ext-set-pat
>>
>> Signed-off-by: Fei Yang <fei.yang@intel.com>
>> Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
>> Acked-by: Jordan Justen <jordan.l.justen@intel.com>
>> Tested-by: Jordan Justen <jordan.l.justen@intel.com>
>> Acked-by: Carl Zhang <carl.zhang@intel.com>
>> Tested-by: Lihao Gu <lihao.gu@intel.com>
>> ---
>>  drivers/gpu/drm/i915/gem/i915_gem_create.c | 36 +++++++++++++++++++
>>  drivers/gpu/drm/i915/gem/i915_gem_object.c |  6 ++++
>>  include/uapi/drm/i915_drm.h                | 41 ++++++++++++++++++++++
>>  3 files changed, 83 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
>> index bfe1dbda4cb7..644a936248ad 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
>> @@ -245,6 +245,7 @@ struct create_ext {
>>        unsigned int n_placements;
>>        unsigned int placement_mask;
>>        unsigned long flags;
>> +     unsigned int pat_index;
>>  };
>>
>>  static void repr_placements(char *buf, size_t size,
>> @@ -394,11 +395,39 @@ static int ext_set_protected(struct i915_user_extension __user *base, void *data
>>        return 0;
>>  }
>>
>> +static int ext_set_pat(struct i915_user_extension __user *base, void *data)
>> +{
>> +     struct create_ext *ext_data = data;
>> +     struct drm_i915_private *i915 = ext_data->i915;
>> +     struct drm_i915_gem_create_ext_set_pat ext;
>> +     unsigned int max_pat_index;
>> +
>> +     BUILD_BUG_ON(sizeof(struct drm_i915_gem_create_ext_set_pat) !=
>> +                  offsetofend(struct drm_i915_gem_create_ext_set_pat, rsvd));
>> +
>> +     if (copy_from_user(&ext, base, sizeof(ext)))
>> +             return -EFAULT;
>> +
>> +     max_pat_index = INTEL_INFO(i915)->max_pat_index;
>> +
>> +     if (ext.pat_index > max_pat_index) {
>> +             drm_dbg(&i915->drm, "PAT index is invalid: %u\n",
>> +                     ext.pat_index);
>> +             return -EINVAL;
>> +     }
>> +
>> +     ext_data->pat_index = ext.pat_index;
>> +
>> +     return 0;
>> +}
>> +
>>  static const i915_user_extension_fn create_extensions[] = {
>>        [I915_GEM_CREATE_EXT_MEMORY_REGIONS] = ext_set_placements,
>>        [I915_GEM_CREATE_EXT_PROTECTED_CONTENT] = ext_set_protected,
>> +     [I915_GEM_CREATE_EXT_SET_PAT] = ext_set_pat,
>>  };
>>
>> +#define PAT_INDEX_NOT_SET    0xffff
>>  /**
>>   * i915_gem_create_ext_ioctl - Creates a new mm object and returns a handle to it.
>>   * @dev: drm device pointer
>> @@ -418,6 +447,7 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
>>        if (args->flags & ~I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS)
>>                return -EINVAL;
>>
>> +     ext_data.pat_index = PAT_INDEX_NOT_SET;
>>        ret = i915_user_extensions(u64_to_user_ptr(args->extensions),
>>                                   create_extensions,
>>                                   ARRAY_SIZE(create_extensions),
>> @@ -454,5 +484,11 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
>>        if (IS_ERR(obj))
>>                return PTR_ERR(obj);
>>
>> +     if (ext_data.pat_index != PAT_INDEX_NOT_SET) {
>> +             i915_gem_object_set_pat_index(obj, ext_data.pat_index);
>> +             /* Mark pat_index is set by UMD */
>> +             obj->pat_set_by_user = true;
>> +     }
>> +
>>        return i915_gem_publish(obj, file, &args->size, &args->handle);
>>  }
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> index 46a19b099ec8..97ac6fb37958 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> @@ -208,6 +208,12 @@ bool i915_gem_object_can_bypass_llc(struct drm_i915_gem_object *obj)
>>        if (!(obj->flags & I915_BO_ALLOC_USER))
>>                return false;
>>
>> +     /*
>> +      * Always flush cache for UMD objects at creation time.
>> +      */
>> +     if (obj->pat_set_by_user)
>> +             return true;
>> +
>>        /*
>>         * EHL and JSL add the 'Bypass LLC' MOCS entry, which should make it
>>         * possible for userspace to bypass the GTT caching bits set by the
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index f31dfacde601..4083a23e0614 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -3679,9 +3679,13 @@ struct drm_i915_gem_create_ext {
>>         *
>>         * For I915_GEM_CREATE_EXT_PROTECTED_CONTENT usage see
>>         * struct drm_i915_gem_create_ext_protected_content.
>> +      *
>> +      * For I915_GEM_CREATE_EXT_SET_PAT usage see
>> +      * struct drm_i915_gem_create_ext_set_pat.
>>         */
>>  #define I915_GEM_CREATE_EXT_MEMORY_REGIONS 0
>>  #define I915_GEM_CREATE_EXT_PROTECTED_CONTENT 1
>> +#define I915_GEM_CREATE_EXT_SET_PAT 2
>>        __u64 extensions;
>>  };
>>
>> @@ -3796,6 +3800,43 @@ struct drm_i915_gem_create_ext_protected_content {
>>        __u32 flags;
>>  };
>>
>> +/**
>> + * struct drm_i915_gem_create_ext_set_pat - The
>> + * I915_GEM_CREATE_EXT_SET_PAT extension.
>> + *
>> + * If this extension is provided, the specified caching policy (PAT index) is
>> + * applied to the buffer object.
>> + *
>> + * Below is an example on how to create an object with specific caching policy:
>> + *
>> + * .. code-block:: C
>> + *
>> + *      struct drm_i915_gem_create_ext_set_pat set_pat_ext = {
>> + *              .base = { .name = I915_GEM_CREATE_EXT_SET_PAT },
>> + *              .pat_index = 0,
>> + *      };
>> + *      struct drm_i915_gem_create_ext create_ext = {
>> + *              .size = PAGE_SIZE,
>> + *              .extensions = (uintptr_t)&set_pat_ext,
>> + *      };
>> + *
>> + *      int err = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE_EXT, &create_ext);
>> + *      if (err) ...
>> + */
>> +struct drm_i915_gem_create_ext_set_pat {
>> +     /** @base: Extension link. See struct i915_user_extension. */
>> +     struct i915_user_extension base;
>> +     /**
>> +      * @pat_index: PAT index to be set
>> +      * PAT index is a bit field in Page Table Entry to control caching
>> +      * behaviors for GPU accesses. The definition of PAT index is
>> +      * platform dependent and can be found in hardware specifications,
>> +      */
>> +     __u32 pat_index;
>> +     /** @rsvd: reserved for future use */
>> +     __u32 rsvd;
>> +};
>> +
>>  /* ID of the protected content session managed by i915 when PXP is active */
>>  #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
>>
>> --
>> 2.25.1

[-- Attachment #2: Type: text/html, Size: 20272 bytes --]

Re: [Intel-gfx] [PATCH v15 1/1] drm/i915: Allow user to set cache at BO creation

[Tvrtko Ursulin]

On 31/05/2023 18:10, fei.yang@intel.com wrote:
> From: Fei Yang <fei.yang@intel.com>
> 
> To comply with the design that buffer objects shall have immutable
> cache setting through out their life cycle, {set, get}_caching ioctl's
> are no longer supported from MTL onward. With that change caching
> policy can only be set at object creation time. The current code
> applies a default (platform dependent) cache setting for all objects.
> However this is not optimal for performance tuning. The patch extends
> the existing gem_create uAPI to let user set PAT index for the object
> at creation time.
> The new extension is platform independent, so UMD's can switch to using
> this extension for older platforms as well, while {set, get}_caching are
> still supported on these legacy paltforms for compatibility reason.
> 
> Note: The detailed description of PAT index is missing in current PRM
> even for older hardware and will be added by the next PRM update under
> chapter name "Memory Views".
> 
> BSpec: 45101
> 
> Mesa support has been submitted in this merge request:
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
> 
> The media driver is supported by the following commits:
> https://github.com/intel/media-driver/commit/92c00a857433ebb34ec575e9834f473c6fcb6341
> https://github.com/intel/media-driver/commit/fd375cf2c5e1f6bf6b43258ff797b3134aadc9fd
> https://github.com/intel/media-driver/commit/08dd244b22484770a33464c2c8ae85430e548000

On which platforms will media-driver use the uapi? I couldn't easily 
figure out myself from the links above and also in the master branch I 
couldn't find the implementation of CachePolicyGetPATIndex.

Now that PRMs for Tigerlake have been published and Meteorlake situation 
is documented indirectly in Mesa code, my only remaining concern is with 
the older platforms. So if there is no particular reason to have the 
extension working on those, I would strongly suggest we disable there.

For a precedent see I915_CONTEXT_PARAM_SSEU and how it allows the 
extension only on Gen11 and only for a very specific usecase (see 
restrictions in set_sseu() and i915_gem_user_to_context_sseu()).

Regards,

Tvrtko

> 
> The IGT test related to this change is
> igt@gem_create@create-ext-set-pat
> 
> Signed-off-by: Fei Yang <fei.yang@intel.com>
> Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
> Acked-by: Jordan Justen <jordan.l.justen@intel.com>
> Tested-by: Jordan Justen <jordan.l.justen@intel.com>
> Acked-by: Carl Zhang <carl.zhang@intel.com>
> Tested-by: Lihao Gu <lihao.gu@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_create.c | 36 +++++++++++++++++++
>   drivers/gpu/drm/i915/gem/i915_gem_object.c |  6 ++++
>   include/uapi/drm/i915_drm.h                | 41 ++++++++++++++++++++++
>   3 files changed, 83 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> index bfe1dbda4cb7..644a936248ad 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> @@ -245,6 +245,7 @@ struct create_ext {
>   	unsigned int n_placements;
>   	unsigned int placement_mask;
>   	unsigned long flags;
> +	unsigned int pat_index;
>   };
>   
>   static void repr_placements(char *buf, size_t size,
> @@ -394,11 +395,39 @@ static int ext_set_protected(struct i915_user_extension __user *base, void *data
>   	return 0;
>   }
>   
> +static int ext_set_pat(struct i915_user_extension __user *base, void *data)
> +{
> +	struct create_ext *ext_data = data;
> +	struct drm_i915_private *i915 = ext_data->i915;
> +	struct drm_i915_gem_create_ext_set_pat ext;
> +	unsigned int max_pat_index;
> +
> +	BUILD_BUG_ON(sizeof(struct drm_i915_gem_create_ext_set_pat) !=
> +		     offsetofend(struct drm_i915_gem_create_ext_set_pat, rsvd));
> +
> +	if (copy_from_user(&ext, base, sizeof(ext)))
> +		return -EFAULT;
> +
> +	max_pat_index = INTEL_INFO(i915)->max_pat_index;
> +
> +	if (ext.pat_index > max_pat_index) {
> +		drm_dbg(&i915->drm, "PAT index is invalid: %u\n",
> +			ext.pat_index);
> +		return -EINVAL;
> +	}
> +
> +	ext_data->pat_index = ext.pat_index;
> +
> +	return 0;
> +}
> +
>   static const i915_user_extension_fn create_extensions[] = {
>   	[I915_GEM_CREATE_EXT_MEMORY_REGIONS] = ext_set_placements,
>   	[I915_GEM_CREATE_EXT_PROTECTED_CONTENT] = ext_set_protected,
> +	[I915_GEM_CREATE_EXT_SET_PAT] = ext_set_pat,
>   };
>   
> +#define PAT_INDEX_NOT_SET	0xffff
>   /**
>    * i915_gem_create_ext_ioctl - Creates a new mm object and returns a handle to it.
>    * @dev: drm device pointer
> @@ -418,6 +447,7 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
>   	if (args->flags & ~I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS)
>   		return -EINVAL;
>   
> +	ext_data.pat_index = PAT_INDEX_NOT_SET;
>   	ret = i915_user_extensions(u64_to_user_ptr(args->extensions),
>   				   create_extensions,
>   				   ARRAY_SIZE(create_extensions),
> @@ -454,5 +484,11 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
>   	if (IS_ERR(obj))
>   		return PTR_ERR(obj);
>   
> +	if (ext_data.pat_index != PAT_INDEX_NOT_SET) {
> +		i915_gem_object_set_pat_index(obj, ext_data.pat_index);
> +		/* Mark pat_index is set by UMD */
> +		obj->pat_set_by_user = true;
> +	}
> +
>   	return i915_gem_publish(obj, file, &args->size, &args->handle);
>   }
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index 46a19b099ec8..97ac6fb37958 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -208,6 +208,12 @@ bool i915_gem_object_can_bypass_llc(struct drm_i915_gem_object *obj)
>   	if (!(obj->flags & I915_BO_ALLOC_USER))
>   		return false;
>   
> +	/*
> +	 * Always flush cache for UMD objects at creation time.
> +	 */
> +	if (obj->pat_set_by_user)
> +		return true;
> +
>   	/*
>   	 * EHL and JSL add the 'Bypass LLC' MOCS entry, which should make it
>   	 * possible for userspace to bypass the GTT caching bits set by the
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index f31dfacde601..4083a23e0614 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -3679,9 +3679,13 @@ struct drm_i915_gem_create_ext {
>   	 *
>   	 * For I915_GEM_CREATE_EXT_PROTECTED_CONTENT usage see
>   	 * struct drm_i915_gem_create_ext_protected_content.
> +	 *
> +	 * For I915_GEM_CREATE_EXT_SET_PAT usage see
> +	 * struct drm_i915_gem_create_ext_set_pat.
>   	 */
>   #define I915_GEM_CREATE_EXT_MEMORY_REGIONS 0
>   #define I915_GEM_CREATE_EXT_PROTECTED_CONTENT 1
> +#define I915_GEM_CREATE_EXT_SET_PAT 2
>   	__u64 extensions;
>   };
>   
> @@ -3796,6 +3800,43 @@ struct drm_i915_gem_create_ext_protected_content {
>   	__u32 flags;
>   };
>   
> +/**
> + * struct drm_i915_gem_create_ext_set_pat - The
> + * I915_GEM_CREATE_EXT_SET_PAT extension.
> + *
> + * If this extension is provided, the specified caching policy (PAT index) is
> + * applied to the buffer object.
> + *
> + * Below is an example on how to create an object with specific caching policy:
> + *
> + * .. code-block:: C
> + *
> + *      struct drm_i915_gem_create_ext_set_pat set_pat_ext = {
> + *              .base = { .name = I915_GEM_CREATE_EXT_SET_PAT },
> + *              .pat_index = 0,
> + *      };
> + *      struct drm_i915_gem_create_ext create_ext = {
> + *              .size = PAGE_SIZE,
> + *              .extensions = (uintptr_t)&set_pat_ext,
> + *      };
> + *
> + *      int err = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE_EXT, &create_ext);
> + *      if (err) ...
> + */
> +struct drm_i915_gem_create_ext_set_pat {
> +	/** @base: Extension link. See struct i915_user_extension. */
> +	struct i915_user_extension base;
> +	/**
> +	 * @pat_index: PAT index to be set
> +	 * PAT index is a bit field in Page Table Entry to control caching
> +	 * behaviors for GPU accesses. The definition of PAT index is
> +	 * platform dependent and can be found in hardware specifications,
> +	 */
> +	__u32 pat_index;
> +	/** @rsvd: reserved for future use */
> +	__u32 rsvd;
> +};
> +
>   /* ID of the protected content session managed by i915 when PXP is active */
>   #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
>   

Re: [Intel-gfx] [PATCH v15 1/1] drm/i915: Allow user to set cache at BO creation

[Yang, Fei]

[-- Attachment #1: Type: text/plain, Size: 9590 bytes --]

> On 31/05/2023 18:10, fei.yang@intel.com wrote:
>> From: Fei Yang <fei.yang@intel.com>
>>
>> To comply with the design that buffer objects shall have immutable
>> cache setting through out their life cycle, {set, get}_caching ioctl's
>> are no longer supported from MTL onward. With that change caching
>> policy can only be set at object creation time. The current code
>> applies a default (platform dependent) cache setting for all objects.
>> However this is not optimal for performance tuning. The patch extends
>> the existing gem_create uAPI to let user set PAT index for the object
>> at creation time.
>> The new extension is platform independent, so UMD's can switch to using
>> this extension for older platforms as well, while {set, get}_caching are
>> still supported on these legacy paltforms for compatibility reason.
>>
>> Note: The detailed description of PAT index is missing in current PRM
>> even for older hardware and will be added by the next PRM update under
>> chapter name "Memory Views".
>>
>> BSpec: 45101
>>
>> Mesa support has been submitted in this merge request:
>> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
>>
>> The media driver is supported by the following commits:
>> https://github.com/intel/media-driver/commit/92c00a857433ebb34ec575e9834f473c6fcb6341
>> https://github.com/intel/media-driver/commit/fd375cf2c5e1f6bf6b43258ff797b3134aadc9fd
>> https://github.com/intel/media-driver/commit/08dd244b22484770a33464c2c8ae85430e548000
>
> On which platforms will media-driver use the uapi? I couldn't easily
> figure out myself from the links above and also in the master branch I
> couldn't find the implementation of CachePolicyGetPATIndex.

These commits look like platform independent. Carl, could you chime in here?

> Now that PRMs for Tigerlake have been published and Meteorlake situation
> is documented indirectly in Mesa code, my only remaining concern is with
> the older platforms. So if there is no particular reason to have the
> extension working on those, I would strongly suggest we disable there.

What's the concern? There is no change required for older platforms, existing
user space code should continue to work. And this extension should be made
available for any new development because the cache settings for BO's need
to be immutable. And that is platform independent.

> For a precedent see I915_CONTEXT_PARAM_SSEU and how it allows the
> extension only on Gen11 and only for a very specific usecase (see
> restrictions in set_sseu() and i915_gem_user_to_context_sseu()).
>
> Regards,
>
> Tvrtko
>
>>
>> The IGT test related to this change is
>> igt@gem_create@create-ext-set-pat
>>
>> Signed-off-by: Fei Yang <fei.yang@intel.com>
>> Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
>> Acked-by: Jordan Justen <jordan.l.justen@intel.com>
>> Tested-by: Jordan Justen <jordan.l.justen@intel.com>
>> Acked-by: Carl Zhang <carl.zhang@intel.com>
>> Tested-by: Lihao Gu <lihao.gu@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_create.c | 36 +++++++++++++++++++
>>   drivers/gpu/drm/i915/gem/i915_gem_object.c |  6 ++++
>>   include/uapi/drm/i915_drm.h                | 41 ++++++++++++++++++++++
>>   3 files changed, 83 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
>> index bfe1dbda4cb7..644a936248ad 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
>> @@ -245,6 +245,7 @@ struct create_ext {
>>        unsigned int n_placements;
>>        unsigned int placement_mask;
>>        unsigned long flags;
>> +     unsigned int pat_index;
>>   };
>>
>>   static void repr_placements(char *buf, size_t size,
>> @@ -394,11 +395,39 @@ static int ext_set_protected(struct i915_user_extension __user *base, void *data
>>        return 0;
>>   }
>>
>> +static int ext_set_pat(struct i915_user_extension __user *base, void *data)
>> +{
>> +     struct create_ext *ext_data = data;
>> +     struct drm_i915_private *i915 = ext_data->i915;
>> +     struct drm_i915_gem_create_ext_set_pat ext;
>> +     unsigned int max_pat_index;
>> +
>> +     BUILD_BUG_ON(sizeof(struct drm_i915_gem_create_ext_set_pat) !=
>> +                  offsetofend(struct drm_i915_gem_create_ext_set_pat, rsvd));
>> +
>> +     if (copy_from_user(&ext, base, sizeof(ext)))
>> +             return -EFAULT;
>> +
>> +     max_pat_index = INTEL_INFO(i915)->max_pat_index;
>> +
>> +     if (ext.pat_index > max_pat_index) {
>> +             drm_dbg(&i915->drm, "PAT index is invalid: %u\n",
>> +                     ext.pat_index);
>> +             return -EINVAL;
>> +     }
>> +
>> +     ext_data->pat_index = ext.pat_index;
>> +
>> +     return 0;
>> +}
>> +
>>   static const i915_user_extension_fn create_extensions[] = {
>>        [I915_GEM_CREATE_EXT_MEMORY_REGIONS] = ext_set_placements,
>>        [I915_GEM_CREATE_EXT_PROTECTED_CONTENT] = ext_set_protected,
>> +     [I915_GEM_CREATE_EXT_SET_PAT] = ext_set_pat,
>>   };
>>
>> +#define PAT_INDEX_NOT_SET    0xffff
>>   /**
>>    * i915_gem_create_ext_ioctl - Creates a new mm object and returns a handle to it.
>>    * @dev: drm device pointer
>> @@ -418,6 +447,7 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
>>        if (args->flags & ~I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS)
>>                return -EINVAL;
>>
>> +     ext_data.pat_index = PAT_INDEX_NOT_SET;
>>        ret = i915_user_extensions(u64_to_user_ptr(args->extensions),
>>                                   create_extensions,
>>                                   ARRAY_SIZE(create_extensions),
>> @@ -454,5 +484,11 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
>>        if (IS_ERR(obj))
>>                return PTR_ERR(obj);
>>
>> +     if (ext_data.pat_index != PAT_INDEX_NOT_SET) {
>> +             i915_gem_object_set_pat_index(obj, ext_data.pat_index);
>> +             /* Mark pat_index is set by UMD */
>> +             obj->pat_set_by_user = true;
>> +     }
>> +
>>        return i915_gem_publish(obj, file, &args->size, &args->handle);
>>   }
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> index 46a19b099ec8..97ac6fb37958 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> @@ -208,6 +208,12 @@ bool i915_gem_object_can_bypass_llc(struct drm_i915_gem_object *obj)
>>        if (!(obj->flags & I915_BO_ALLOC_USER))
>>                return false;
>>
>> +     /*
>> +      * Always flush cache for UMD objects at creation time.
>> +      */
>> +     if (obj->pat_set_by_user)
>> +             return true;
>> +
>>        /*
>>         * EHL and JSL add the 'Bypass LLC' MOCS entry, which should make it
>>         * possible for userspace to bypass the GTT caching bits set by the
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index f31dfacde601..4083a23e0614 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -3679,9 +3679,13 @@ struct drm_i915_gem_create_ext {
>>         *
>>         * For I915_GEM_CREATE_EXT_PROTECTED_CONTENT usage see
>>         * struct drm_i915_gem_create_ext_protected_content.
>> +      *
>> +      * For I915_GEM_CREATE_EXT_SET_PAT usage see
>> +      * struct drm_i915_gem_create_ext_set_pat.
>>         */
>>   #define I915_GEM_CREATE_EXT_MEMORY_REGIONS 0
>>   #define I915_GEM_CREATE_EXT_PROTECTED_CONTENT 1
>> +#define I915_GEM_CREATE_EXT_SET_PAT 2
>>        __u64 extensions;
>>   };
>>
>> @@ -3796,6 +3800,43 @@ struct drm_i915_gem_create_ext_protected_content {
>>        __u32 flags;
>>   };
>>
>> +/**
>> + * struct drm_i915_gem_create_ext_set_pat - The
>> + * I915_GEM_CREATE_EXT_SET_PAT extension.
>> + *
>> + * If this extension is provided, the specified caching policy (PAT index) is
>> + * applied to the buffer object.
>> + *
>> + * Below is an example on how to create an object with specific caching policy:
>> + *
>> + * .. code-block:: C
>> + *
>> + *      struct drm_i915_gem_create_ext_set_pat set_pat_ext = {
>> + *              .base = { .name = I915_GEM_CREATE_EXT_SET_PAT },
>> + *              .pat_index = 0,
>> + *      };
>> + *      struct drm_i915_gem_create_ext create_ext = {
>> + *              .size = PAGE_SIZE,
>> + *              .extensions = (uintptr_t)&set_pat_ext,
>> + *      };
>> + *
>> + *      int err = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE_EXT, &create_ext);
>> + *      if (err) ...
>> + */
>> +struct drm_i915_gem_create_ext_set_pat {
>> +     /** @base: Extension link. See struct i915_user_extension. */
>> +     struct i915_user_extension base;
>> +     /**
>> +      * @pat_index: PAT index to be set
>> +      * PAT index is a bit field in Page Table Entry to control caching
>> +      * behaviors for GPU accesses. The definition of PAT index is
>> +      * platform dependent and can be found in hardware specifications,
>> +      */
>> +     __u32 pat_index;
>> +     /** @rsvd: reserved for future use */
>> +     __u32 rsvd;
>> +};
>> +
>>   /* ID of the protected content session managed by i915 when PXP is active */
>>   #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
>>


[-- Attachment #2: Type: text/html, Size: 21547 bytes --]

Re: [Intel-gfx] [PATCH v15 1/1] drm/i915: Allow user to set cache at BO creation

[Yang, Fei]

[-- Attachment #1: Type: text/plain, Size: 10417 bytes --]

>> On 31/05/2023 18:10, fei.yang@intel.com wrote:
>>> From: Fei Yang <fei.yang@intel.com>
>>>
>>> To comply with the design that buffer objects shall have immutable
>>> cache setting through out their life cycle, {set, get}_caching ioctl's
>>> are no longer supported from MTL onward. With that change caching
>>> policy can only be set at object creation time. The current code
>>> applies a default (platform dependent) cache setting for all objects.
>>> However this is not optimal for performance tuning. The patch extends
>>> the existing gem_create uAPI to let user set PAT index for the object
>>> at creation time.
>>> The new extension is platform independent, so UMD's can switch to using
>>> this extension for older platforms as well, while {set, get}_caching are
>>> still supported on these legacy paltforms for compatibility reason.
>>>
>>> Note: The detailed description of PAT index is missing in current PRM
>>> even for older hardware and will be added by the next PRM update under
>>> chapter name "Memory Views".
>>>
>>> BSpec: 45101
>>>
>>> Mesa support has been submitted in this merge request:
>>> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
>>>
>>> The media driver is supported by the following commits:
>>> https://github.com/intel/media-driver/commit/92c00a857433ebb34ec575e9834f473c6fcb6341
>>> https://github.com/intel/media-driver/commit/fd375cf2c5e1f6bf6b43258ff797b3134aadc9fd
>>> https://github.com/intel/media-driver/commit/08dd244b22484770a33464c2c8ae85430e548000
>>
>> On which platforms will media-driver use the uapi? I couldn't easily
>> figure out myself from the links above and also in the master branch I
>> couldn't find the implementation of CachePolicyGetPATIndex.
>
> These commits look like platform independent. Carl, could you chime in here?

Confirmed with Carl and Lihao offline that the media driver is calling set_pat
extension in common code path, so the use of set_pat extension is platform
independent. The only problem right now is that the gmm library is not returning
correct PAT index for all hardware platforms, so on some platforms the call would
be bypassed and fall back to the old way.
I think this is the correct implementation. It should be platform independent as
long as the application knows what PAT index to set. Updating the gmm library to
understand PAT index for each hardware platform is a separate issue.

>> Now that PRMs for Tigerlake have been published and Meteorlake situation
>> is documented indirectly in Mesa code, my only remaining concern is with
>> the older platforms. So if there is no particular reason to have the
>> extension working on those, I would strongly suggest we disable there.
>
> What's the concern? There is no change required for older platforms, existing
> user space code should continue to work. And this extension should be made
> available for any new development because the cache settings for BO's need
> to be immutable. And that is platform independent.
>
>> For a precedent see I915_CONTEXT_PARAM_SSEU and how it allows the
>> extension only on Gen11 and only for a very specific usecase (see
>> restrictions in set_sseu() and i915_gem_user_to_context_sseu()).
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> The IGT test related to this change is
>>> igt@gem_create@create-ext-set-pat
>>>
>>> Signed-off-by: Fei Yang <fei.yang@intel.com>
>>> Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>>> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
>>> Acked-by: Jordan Justen <jordan.l.justen@intel.com>
>>> Tested-by: Jordan Justen <jordan.l.justen@intel.com>
>>> Acked-by: Carl Zhang <carl.zhang@intel.com>
>>> Tested-by: Lihao Gu <lihao.gu@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gem/i915_gem_create.c | 36 +++++++++++++++++++
>>>   drivers/gpu/drm/i915/gem/i915_gem_object.c |  6 ++++
>>>   include/uapi/drm/i915_drm.h                | 41 ++++++++++++++++++++++
>>>   3 files changed, 83 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
>>> index bfe1dbda4cb7..644a936248ad 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
>>> @@ -245,6 +245,7 @@ struct create_ext {
>>>        unsigned int n_placements;
>>>        unsigned int placement_mask;
>>>        unsigned long flags;
>>> +     unsigned int pat_index;
>>>   };
>>>
>>>   static void repr_placements(char *buf, size_t size,
>>> @@ -394,11 +395,39 @@ static int ext_set_protected(struct i915_user_extension __user *base, void *data
>>>        return 0;
>>>   }
>>>
>>> +static int ext_set_pat(struct i915_user_extension __user *base, void *data)
>>> +{
>>> +     struct create_ext *ext_data = data;
>>> +     struct drm_i915_private *i915 = ext_data->i915;
>>> +     struct drm_i915_gem_create_ext_set_pat ext;
>>> +     unsigned int max_pat_index;
>>> +
>>> +     BUILD_BUG_ON(sizeof(struct drm_i915_gem_create_ext_set_pat) !=
>>> +                  offsetofend(struct drm_i915_gem_create_ext_set_pat, rsvd));
>>> +
>>> +     if (copy_from_user(&ext, base, sizeof(ext)))
>>> +             return -EFAULT;
>>> +
>>> +     max_pat_index = INTEL_INFO(i915)->max_pat_index;
>>> +
>>> +     if (ext.pat_index > max_pat_index) {
>>> +             drm_dbg(&i915->drm, "PAT index is invalid: %u\n",
>>> +                     ext.pat_index);
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     ext_data->pat_index = ext.pat_index;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>>   static const i915_user_extension_fn create_extensions[] = {
>>>        [I915_GEM_CREATE_EXT_MEMORY_REGIONS] = ext_set_placements,
>>>        [I915_GEM_CREATE_EXT_PROTECTED_CONTENT] = ext_set_protected,
>>> +     [I915_GEM_CREATE_EXT_SET_PAT] = ext_set_pat,
>>>   };
>>>
>>> +#define PAT_INDEX_NOT_SET    0xffff
>>>   /**
>>>    * i915_gem_create_ext_ioctl - Creates a new mm object and returns a handle to it.
>>>    * @dev: drm device pointer
>>> @@ -418,6 +447,7 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
>>>        if (args->flags & ~I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS)
>>>                return -EINVAL;
>>>
>>> +     ext_data.pat_index = PAT_INDEX_NOT_SET;
>>>        ret = i915_user_extensions(u64_to_user_ptr(args->extensions),
>>>                                   create_extensions,
>>>                                   ARRAY_SIZE(create_extensions),
>>> @@ -454,5 +484,11 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
>>>        if (IS_ERR(obj))
>>>                return PTR_ERR(obj);
>>>
>>> +     if (ext_data.pat_index != PAT_INDEX_NOT_SET) {
>>> +             i915_gem_object_set_pat_index(obj, ext_data.pat_index);
>>> +             /* Mark pat_index is set by UMD */
>>> +             obj->pat_set_by_user = true;
>>> +     }
>>> +
>>>        return i915_gem_publish(obj, file, &args->size, &args->handle);
>>>   }
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>>> index 46a19b099ec8..97ac6fb37958 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>>> @@ -208,6 +208,12 @@ bool i915_gem_object_can_bypass_llc(struct drm_i915_gem_object *obj)
>>>        if (!(obj->flags & I915_BO_ALLOC_USER))
>>>                return false;
>>>
>>> +     /*
>>> +      * Always flush cache for UMD objects at creation time.
>>> +      */
>>> +     if (obj->pat_set_by_user)
>>> +             return true;
>>> +
>>>        /*
>>>         * EHL and JSL add the 'Bypass LLC' MOCS entry, which should make it
>>>         * possible for userspace to bypass the GTT caching bits set by the
>>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>>> index f31dfacde601..4083a23e0614 100644
>>> --- a/include/uapi/drm/i915_drm.h
>>> +++ b/include/uapi/drm/i915_drm.h
>>> @@ -3679,9 +3679,13 @@ struct drm_i915_gem_create_ext {
>>>         *
>>>         * For I915_GEM_CREATE_EXT_PROTECTED_CONTENT usage see
>>>         * struct drm_i915_gem_create_ext_protected_content.
>>> +      *
>>> +      * For I915_GEM_CREATE_EXT_SET_PAT usage see
>>> +      * struct drm_i915_gem_create_ext_set_pat.
>>>         */
>>>   #define I915_GEM_CREATE_EXT_MEMORY_REGIONS 0
>>>   #define I915_GEM_CREATE_EXT_PROTECTED_CONTENT 1
>>> +#define I915_GEM_CREATE_EXT_SET_PAT 2
>>>        __u64 extensions;
>>>   };
>>>
>>> @@ -3796,6 +3800,43 @@ struct drm_i915_gem_create_ext_protected_content {
>>>        __u32 flags;
>>>   };
>>>
>>> +/**
>>> + * struct drm_i915_gem_create_ext_set_pat - The
>>> + * I915_GEM_CREATE_EXT_SET_PAT extension.
>>> + *
>>> + * If this extension is provided, the specified caching policy (PAT index) is
>>> + * applied to the buffer object.
>>> + *
>>> + * Below is an example on how to create an object with specific caching policy:
>>> + *
>>> + * .. code-block:: C
>>> + *
>>> + *      struct drm_i915_gem_create_ext_set_pat set_pat_ext = {
>>> + *              .base = { .name = I915_GEM_CREATE_EXT_SET_PAT },
>>> + *              .pat_index = 0,
>>> + *      };
>>> + *      struct drm_i915_gem_create_ext create_ext = {
>>> + *              .size = PAGE_SIZE,
>>> + *              .extensions = (uintptr_t)&set_pat_ext,
>>> + *      };
>>> + *
>>> + *      int err = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE_EXT, &create_ext);
>>> + *      if (err) ...
>>> + */
>>> +struct drm_i915_gem_create_ext_set_pat {
>>> +     /** @base: Extension link. See struct i915_user_extension. */
>>> +     struct i915_user_extension base;
>>> +     /**
>>> +      * @pat_index: PAT index to be set
>>> +      * PAT index is a bit field in Page Table Entry to control caching
>>> +      * behaviors for GPU accesses. The definition of PAT index is
>>> +      * platform dependent and can be found in hardware specifications,
>>> +      */
>>> +     __u32 pat_index;
>>> +     /** @rsvd: reserved for future use */
>>> +     __u32 rsvd;
>>> +};
>>> +
>>>   /* ID of the protected content session managed by i915 when PXP is active */
>>>   #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
>>>


[-- Attachment #2: Type: text/html, Size: 22410 bytes --]

Re: [Intel-gfx] [PATCH v15 1/1] drm/i915: Allow user to set cache at BO creation

[Joonas Lahtinen]

Quoting Yang, Fei (2023-06-06 09:51:06)
> >> On 31/05/2023 18:10, fei.yang@intel.com wrote:
> >>> From: Fei Yang <fei.yang@intel.com>
> >>>
> >>> To comply with the design that buffer objects shall have immutable
> >>> cache setting through out their life cycle, {set, get}_caching ioctl's
> >>> are no longer supported from MTL onward. With that change caching
> >>> policy can only be set at object creation time. The current code
> >>> applies a default (platform dependent) cache setting for all objects.
> >>> However this is not optimal for performance tuning. The patch extends
> >>> the existing gem_create uAPI to let user set PAT index for the object
> >>> at creation time.
> >>> The new extension is platform independent, so UMD's can switch to using
> >>> this extension for older platforms as well, while {set, get}_caching are
> >>> still supported on these legacy paltforms for compatibility reason.
> >>>
> >>> Note: The detailed description of PAT index is missing in current PRM
> >>> even for older hardware and will be added by the next PRM update under
> >>> chapter name "Memory Views".
> >>>
> >>> BSpec: 45101
> >>>
> >>> Mesa support has been submitted in this merge request:
> >>> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
> >>>
> >>> The media driver is supported by the following commits:
> >>> https://github.com/intel/media-driver/commit/
> 92c00a857433ebb34ec575e9834f473c6fcb6341
> >>> https://github.com/intel/media-driver/commit/
> fd375cf2c5e1f6bf6b43258ff797b3134aadc9fd
> >>> https://github.com/intel/media-driver/commit/
> 08dd244b22484770a33464c2c8ae85430e548000

We absolutely should not have merged this code to master branch yet.

These should be reverted immediately and any releases that include the code
must be pulled back.

This is clearly explained in:

https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements

"The kernel patch can only be merged after all the above requirements
are met, but it *must* be merged to either drm-next or drm-misc-next
*before* the userspace patches land. uAPI always flows from the kernel,
doing things the other way round risks divergence of the uAPI
definitions and header files."

> >> On which platforms will media-driver use the uapi? I couldn't easily
> >> figure out myself from the links above and also in the master branch I
> >> couldn't find the implementation of CachePolicyGetPATIndex.
> >
> > These commits look like platform independent. Carl, could you chime in here?
> 
> Confirmed with Carl and Lihao offline that the media driver is calling set_pat
> extension in common code path, so the use of set_pat extension is platform
> independent. The only problem right now is that the gmm library is not
> returning
> correct PAT index for all hardware platforms, so on some platforms the call
> would
> be bypassed and fall back to the old way.

That means the code is unused for older platforms. The fact that there
is potential to be used is not alone a reason for merging it.

So I agree with Tvrtko that we should only limit this to the newer
platforms where we have actual use that is ready and reviewed.

We can extend to older platforms later, but in order not to block the
progress please move the code for older platform to later series and
only apply to platforms where this is needed.

> I think this is the correct implementation. It should be platform independent
> as
> long as the application knows what PAT index to set. Updating the gmm library
> to
> understand PAT index for each hardware platform is a separate issue.

If we don't have userspace ready, we don't merge the code.

Regards, Joonas

> >> Now that PRMs for Tigerlake have been published and Meteorlake situation
> >> is documented indirectly in Mesa code, my only remaining concern is with
> >> the older platforms. So if there is no particular reason to have the
> >> extension working on those, I would strongly suggest we disable there.
> >
> > What's the concern? There is no change required for older platforms, existing
> > user space code should continue to work. And this extension should be made
> > available for any new development because the cache settings for BO's need
> > to be immutable. And that is platform independent.
> >
> >> For a precedent see I915_CONTEXT_PARAM_SSEU and how it allows the
> >> extension only on Gen11 and only for a very specific usecase (see
> >> restrictions in set_sseu() and i915_gem_user_to_context_sseu()).
> >>
> >> Regards,
> >>
> >> Tvrtko
> >>
> >>>
> >>> The IGT test related to this change is
> >>> igt@gem_create@create-ext-set-pat
> >>>
> >>> Signed-off-by: Fei Yang <fei.yang@intel.com>
> >>> Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
> >>> Cc: Matt Roper <matthew.d.roper@intel.com>
> >>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> >>> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
> >>> Acked-by: Jordan Justen <jordan.l.justen@intel.com>
> >>> Tested-by: Jordan Justen <jordan.l.justen@intel.com>
> >>> Acked-by: Carl Zhang <carl.zhang@intel.com>
> >>> Tested-by: Lihao Gu <lihao.gu@intel.com>
> >>> ---
> >>>   drivers/gpu/drm/i915/gem/i915_gem_create.c | 36 +++++++++++++++++++
> >>>   drivers/gpu/drm/i915/gem/i915_gem_object.c |  6 ++++
> >>>   include/uapi/drm/i915_drm.h                | 41 ++++++++++++++++++++++
> >>>   3 files changed, 83 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/
> i915/gem/i915_gem_create.c
> >>> index bfe1dbda4cb7..644a936248ad 100644
> >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
> >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> >>> @@ -245,6 +245,7 @@ struct create_ext {
> >>>        unsigned int n_placements;
> >>>        unsigned int placement_mask;
> >>>        unsigned long flags;
> >>> +     unsigned int pat_index;
> >>>   };
> >>>
> >>>   static void repr_placements(char *buf, size_t size,
> >>> @@ -394,11 +395,39 @@ static int ext_set_protected(struct
> i915_user_extension __user *base, void *data
> >>>        return 0;
> >>>   }
> >>>
> >>> +static int ext_set_pat(struct i915_user_extension __user *base, void
> *data)
> >>> +{
> >>> +     struct create_ext *ext_data = data;
> >>> +     struct drm_i915_private *i915 = ext_data->i915;
> >>> +     struct drm_i915_gem_create_ext_set_pat ext;
> >>> +     unsigned int max_pat_index;
> >>> +
> >>> +     BUILD_BUG_ON(sizeof(struct drm_i915_gem_create_ext_set_pat) !=
> >>> +                  offsetofend(struct drm_i915_gem_create_ext_set_pat,
> rsvd));
> >>> +
> >>> +     if (copy_from_user(&ext, base, sizeof(ext)))
> >>> +             return -EFAULT;
> >>> +
> >>> +     max_pat_index = INTEL_INFO(i915)->max_pat_index;
> >>> +
> >>> +     if (ext.pat_index > max_pat_index) {
> >>> +             drm_dbg(&i915->drm, "PAT index is invalid: %u\n",
> >>> +                     ext.pat_index);
> >>> +             return -EINVAL;
> >>> +     }
> >>> +
> >>> +     ext_data->pat_index = ext.pat_index;
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>>   static const i915_user_extension_fn create_extensions[] = {
> >>>        [I915_GEM_CREATE_EXT_MEMORY_REGIONS] = ext_set_placements,
> >>>        [I915_GEM_CREATE_EXT_PROTECTED_CONTENT] = ext_set_protected,
> >>> +     [I915_GEM_CREATE_EXT_SET_PAT] = ext_set_pat,
> >>>   };
> >>>
> >>> +#define PAT_INDEX_NOT_SET    0xffff
> >>>   /**
> >>>    * i915_gem_create_ext_ioctl - Creates a new mm object and returns a
> handle to it.
> >>>    * @dev: drm device pointer
> >>> @@ -418,6 +447,7 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void
> *data,
> >>>        if (args->flags & ~I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS)
> >>>                return -EINVAL;
> >>>
> >>> +     ext_data.pat_index = PAT_INDEX_NOT_SET;
> >>>        ret = i915_user_extensions(u64_to_user_ptr(args->extensions),
> >>>                                   create_extensions,
> >>>                                   ARRAY_SIZE(create_extensions),
> >>> @@ -454,5 +484,11 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void
> *data,
> >>>        if (IS_ERR(obj))
> >>>                return PTR_ERR(obj);
> >>>
> >>> +     if (ext_data.pat_index != PAT_INDEX_NOT_SET) {
> >>> +             i915_gem_object_set_pat_index(obj, ext_data.pat_index);
> >>> +             /* Mark pat_index is set by UMD */
> >>> +             obj->pat_set_by_user = true;
> >>> +     }
> >>> +
> >>>        return i915_gem_publish(obj, file, &args->size, &args->handle);
> >>>   }
> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/
> i915/gem/i915_gem_object.c
> >>> index 46a19b099ec8..97ac6fb37958 100644
> >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> >>> @@ -208,6 +208,12 @@ bool i915_gem_object_can_bypass_llc(struct
> drm_i915_gem_object *obj)
> >>>        if (!(obj->flags & I915_BO_ALLOC_USER))
> >>>                return false;
> >>>
> >>> +     /*
> >>> +      * Always flush cache for UMD objects at creation time.
> >>> +      */
> >>> +     if (obj->pat_set_by_user)
> >>> +             return true;
> >>> +
> >>>        /*
> >>>         * EHL and JSL add the 'Bypass LLC' MOCS entry, which should make it
> >>>         * possible for userspace to bypass the GTT caching bits set by the
> >>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> >>> index f31dfacde601..4083a23e0614 100644
> >>> --- a/include/uapi/drm/i915_drm.h
> >>> +++ b/include/uapi/drm/i915_drm.h
> >>> @@ -3679,9 +3679,13 @@ struct drm_i915_gem_create_ext {
> >>>         *
> >>>         * For I915_GEM_CREATE_EXT_PROTECTED_CONTENT usage see
> >>>         * struct drm_i915_gem_create_ext_protected_content.
> >>> +      *
> >>> +      * For I915_GEM_CREATE_EXT_SET_PAT usage see
> >>> +      * struct drm_i915_gem_create_ext_set_pat.
> >>>         */
> >>>   #define I915_GEM_CREATE_EXT_MEMORY_REGIONS 0
> >>>   #define I915_GEM_CREATE_EXT_PROTECTED_CONTENT 1
> >>> +#define I915_GEM_CREATE_EXT_SET_PAT 2
> >>>        __u64 extensions;
> >>>   };
> >>>
> >>> @@ -3796,6 +3800,43 @@ struct drm_i915_gem_create_ext_protected_content {
> >>>        __u32 flags;
> >>>   };
> >>>
> >>> +/**
> >>> + * struct drm_i915_gem_create_ext_set_pat - The
> >>> + * I915_GEM_CREATE_EXT_SET_PAT extension.
> >>> + *
> >>> + * If this extension is provided, the specified caching policy (PAT index)
> is
> >>> + * applied to the buffer object.
> >>> + *
> >>> + * Below is an example on how to create an object with specific caching
> policy:
> >>> + *
> >>> + * .. code-block:: C
> >>> + *
> >>> + *      struct drm_i915_gem_create_ext_set_pat set_pat_ext = {
> >>> + *              .base = { .name = I915_GEM_CREATE_EXT_SET_PAT },
> >>> + *              .pat_index = 0,
> >>> + *      };
> >>> + *      struct drm_i915_gem_create_ext create_ext = {
> >>> + *              .size = PAGE_SIZE,
> >>> + *              .extensions = (uintptr_t)&set_pat_ext,
> >>> + *      };
> >>> + *
> >>> + *      int err = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE_EXT, &create_ext);
> >>> + *      if (err) ...
> >>> + */
> >>> +struct drm_i915_gem_create_ext_set_pat {
> >>> +     /** @base: Extension link. See struct i915_user_extension. */
> >>> +     struct i915_user_extension base;
> >>> +     /**
> >>> +      * @pat_index: PAT index to be set
> >>> +      * PAT index is a bit field in Page Table Entry to control caching
> >>> +      * behaviors for GPU accesses. The definition of PAT index is
> >>> +      * platform dependent and can be found in hardware specifications,
> >>> +      */
> >>> +     __u32 pat_index;
> >>> +     /** @rsvd: reserved for future use */
> >>> +     __u32 rsvd;
> >>> +};
> >>> +
> >>>   /* ID of the protected content session managed by i915 when PXP is active
> */
> >>>   #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
> >>>
> 

Re: [Intel-gfx] [PATCH v15 0/1] drm/i915: Allow user to set cache at BO creation

[Tvrtko Ursulin]

On 31/05/2023 18:10, fei.yang@intel.com wrote:
> From: Fei Yang <fei.yang@intel.com>
> 
> This series introduce a new extension for GEM_CREATE,
> 1. end support for set caching ioctl [PATCH 1/2]
> 2. add set_pat extension for gem_create [PATCH 2/2]
> 
> v2: drop one patch that was merged separately
>      commit 341ad0e8e254 ("drm/i915/mtl: Add PTE encode function")
> v3: rebased on https://patchwork.freedesktop.org/series/117082/
> v4: fix missing unlock introduced in v3, and
>      solve a rebase conflict
> v5: replace obj->cache_level with pat_set_by_user,
>      fix i915_cache_level_str() for legacy platforms.
> v6: rebased on https://patchwork.freedesktop.org/series/117480/
> v7: rebased on https://patchwork.freedesktop.org/series/117528/
> v8: dropped the two dependent patches that has been merged
>      separately. Add IGT link and Tested-by (MESA).
> v9: addressing comments (Andi)
> v10: acked-by and tested-by MESA
> v11: drop "end support for set caching ioctl" (merged)
>       remove tools/include/uapi/drm/i915_drm.h
> v12: drop Bspec reference in comment. add to commit message instead
> v13: sent to test with igt@gem_create@create-ext-set-pat
> v14: sent to test with igt@gem_create@create-ext-set-pat
> v15: update commit message with documentation note and t-b/a-b from
>       Media driver folks.
> 
> Fei Yang (1):
>    drm/i915: Allow user to set cache at BO creation
> 
>   drivers/gpu/drm/i915/gem/i915_gem_create.c | 36 +++++++++++++++++++
>   drivers/gpu/drm/i915/gem/i915_gem_object.c |  6 ++++
>   include/uapi/drm/i915_drm.h                | 41 ++++++++++++++++++++++
>   3 files changed, 83 insertions(+)
> 

Try with:

Test-with: 20230526172221.1438998-1-fei.yang@intel.com

That is how it is supposed to be done, to do a CI run against a test 
case not yet merged that is.

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH v15 0/1] drm/i915: Allow user to set cache at BO creation

[Tvrtko Ursulin]

On 05/06/2023 09:53, Tvrtko Ursulin wrote:
> 
> On 31/05/2023 18:10, fei.yang@intel.com wrote:
>> From: Fei Yang <fei.yang@intel.com>
>>
>> This series introduce a new extension for GEM_CREATE,
>> 1. end support for set caching ioctl [PATCH 1/2]
>> 2. add set_pat extension for gem_create [PATCH 2/2]
>>
>> v2: drop one patch that was merged separately
>>      commit 341ad0e8e254 ("drm/i915/mtl: Add PTE encode function")
>> v3: rebased on https://patchwork.freedesktop.org/series/117082/
>> v4: fix missing unlock introduced in v3, and
>>      solve a rebase conflict
>> v5: replace obj->cache_level with pat_set_by_user,
>>      fix i915_cache_level_str() for legacy platforms.
>> v6: rebased on https://patchwork.freedesktop.org/series/117480/
>> v7: rebased on https://patchwork.freedesktop.org/series/117528/
>> v8: dropped the two dependent patches that has been merged
>>      separately. Add IGT link and Tested-by (MESA).
>> v9: addressing comments (Andi)
>> v10: acked-by and tested-by MESA
>> v11: drop "end support for set caching ioctl" (merged)
>>       remove tools/include/uapi/drm/i915_drm.h
>> v12: drop Bspec reference in comment. add to commit message instead
>> v13: sent to test with igt@gem_create@create-ext-set-pat
>> v14: sent to test with igt@gem_create@create-ext-set-pat
>> v15: update commit message with documentation note and t-b/a-b from
>>       Media driver folks.
>>
>> Fei Yang (1):
>>    drm/i915: Allow user to set cache at BO creation
>>
>>   drivers/gpu/drm/i915/gem/i915_gem_create.c | 36 +++++++++++++++++++
>>   drivers/gpu/drm/i915/gem/i915_gem_object.c |  6 ++++
>>   include/uapi/drm/i915_drm.h                | 41 ++++++++++++++++++++++
>>   3 files changed, 83 insertions(+)
>>
> 
> Try with:
> 
> Test-with: 20230526172221.1438998-1-fei.yang@intel.com
> 
> That is how it is supposed to be done, to do a CI run against a test 
> case not yet merged that is.

Or I see that IGT has been since merged so you probably have results 
already?

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH v15 0/1] drm/i915: Allow user to set cache at BO creation

[Andi Shyti]

On Mon, Jun 05, 2023 at 10:16:22AM +0100, Tvrtko Ursulin wrote:
> 
> On 05/06/2023 09:53, Tvrtko Ursulin wrote:
> > 
> > On 31/05/2023 18:10, fei.yang@intel.com wrote:
> > > From: Fei Yang <fei.yang@intel.com>
> > > 
> > > This series introduce a new extension for GEM_CREATE,
> > > 1. end support for set caching ioctl [PATCH 1/2]
> > > 2. add set_pat extension for gem_create [PATCH 2/2]
> > > 
> > > v2: drop one patch that was merged separately
> > >      commit 341ad0e8e254 ("drm/i915/mtl: Add PTE encode function")
> > > v3: rebased on https://patchwork.freedesktop.org/series/117082/
> > > v4: fix missing unlock introduced in v3, and
> > >      solve a rebase conflict
> > > v5: replace obj->cache_level with pat_set_by_user,
> > >      fix i915_cache_level_str() for legacy platforms.
> > > v6: rebased on https://patchwork.freedesktop.org/series/117480/
> > > v7: rebased on https://patchwork.freedesktop.org/series/117528/
> > > v8: dropped the two dependent patches that has been merged
> > >      separately. Add IGT link and Tested-by (MESA).
> > > v9: addressing comments (Andi)
> > > v10: acked-by and tested-by MESA
> > > v11: drop "end support for set caching ioctl" (merged)
> > >       remove tools/include/uapi/drm/i915_drm.h
> > > v12: drop Bspec reference in comment. add to commit message instead
> > > v13: sent to test with igt@gem_create@create-ext-set-pat
> > > v14: sent to test with igt@gem_create@create-ext-set-pat
> > > v15: update commit message with documentation note and t-b/a-b from
> > >       Media driver folks.
> > > 
> > > Fei Yang (1):
> > >    drm/i915: Allow user to set cache at BO creation
> > > 
> > >   drivers/gpu/drm/i915/gem/i915_gem_create.c | 36 +++++++++++++++++++
> > >   drivers/gpu/drm/i915/gem/i915_gem_object.c |  6 ++++
> > >   include/uapi/drm/i915_drm.h                | 41 ++++++++++++++++++++++
> > >   3 files changed, 83 insertions(+)
> > > 
> > 
> > Try with:
> > 
> > Test-with: 20230526172221.1438998-1-fei.yang@intel.com
> > 
> > That is how it is supposed to be done, to do a CI run against a test
> > case not yet merged that is.
> 
> Or I see that IGT has been since merged so you probably have results
> already?

CI has stopped somewhere in the BAT tests. Can anyone hit the
"Test revision 1 again" button? Fei, would you?

We had it tested some revisions back, though.

Andi

Re: [Intel-gfx] [PATCH v15 0/1] drm/i915: Allow user to set cache at BO creation

[Yang, Fei]

[-- Attachment #1: Type: text/plain, Size: 2446 bytes --]

> On 05/06/2023 09:53, Tvrtko Ursulin wrote:
>> On 31/05/2023 18:10, fei.yang@intel.com wrote:
>>> From: Fei Yang <fei.yang@intel.com>
>>>
>>> This series introduce a new extension for GEM_CREATE,
>>> 1. end support for set caching ioctl [PATCH 1/2]
>>> 2. add set_pat extension for gem_create [PATCH 2/2]
>>>
>>> v2: drop one patch that was merged separately
>>>      commit 341ad0e8e254 ("drm/i915/mtl: Add PTE encode function")
>>> v3: rebased on https://patchwork.freedesktop.org/series/117082/
>>> v4: fix missing unlock introduced in v3, and
>>>      solve a rebase conflict
>>> v5: replace obj->cache_level with pat_set_by_user,
>>>      fix i915_cache_level_str() for legacy platforms.
>>> v6: rebased on https://patchwork.freedesktop.org/series/117480/
>>> v7: rebased on https://patchwork.freedesktop.org/series/117528/
>>> v8: dropped the two dependent patches that has been merged
>>>      separately. Add IGT link and Tested-by (MESA).
>>> v9: addressing comments (Andi)
>>> v10: acked-by and tested-by MESA
>>> v11: drop "end support for set caching ioctl" (merged)
>>>       remove tools/include/uapi/drm/i915_drm.h
>>> v12: drop Bspec reference in comment. add to commit message instead
>>> v13: sent to test with igt@gem_create@create-ext-set-pat
>>> v14: sent to test with igt@gem_create@create-ext-set-pat
>>> v15: update commit message with documentation note and t-b/a-b from
>>>       Media driver folks.
>>>
>>> Fei Yang (1):
>>>    drm/i915: Allow user to set cache at BO creation
>>>
>>>   drivers/gpu/drm/i915/gem/i915_gem_create.c | 36 +++++++++++++++++++
>>>   drivers/gpu/drm/i915/gem/i915_gem_object.c |  6 ++++
>>>   include/uapi/drm/i915_drm.h                | 41 ++++++++++++++++++++++
>>>   3 files changed, 83 insertions(+)
>>>
>>
>> Try with:
>>
>> Test-with: 20230526172221.1438998-1-fei.yang@intel.com
>>
>> That is how it is supposed to be done, to do a CI run against a test
>> case not yet merged that is.

Yes, the result can be found at https://patchwork.freedesktop.org/series/116870/
, under rev14, expand Fi.CI.IGT, you would see,

New IGT tests (1)
        igt@gem_create@create-ext-set-pat:
        Statuses : 6 pass(s)
        Exec time: [0.0] s

> Or I see that IGT has been since merged so you probably have results
> already?

Seems like the last update ran into some random failure which caused CI to stop.

> Regards,
>
> Tvrtko


[-- Attachment #2: Type: text/html, Size: 5512 bytes --]