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

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

[Andi Shyti]

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.
v16: limiting the extension to GEN12+
v17: limiting the extension to Meteor Lake

Test-with: 20230606095123.476130-2-andi.shyti@linux.intel.com

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

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

-- 
2.40.1

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

[Andi Shyti]

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.
However, since PAT index was not clearly defined for platforms prior to
GEN12 (TGL), so we are limiting this externsion to GEN12+ platforms
only. See ext_set_pat() in for the implementation details.

Note: 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

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>
Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_create.c | 40 +++++++++++++++++++++
 drivers/gpu/drm/i915/gem/i915_gem_object.c |  6 ++++
 include/uapi/drm/i915_drm.h                | 41 ++++++++++++++++++++++
 3 files changed, 87 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
index bfe1dbda4cb75..d24c0ce8805c7 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,43 @@ 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));
+
+	/* Limiting the extension only to Meteor Lake */
+	if (!IS_METEORLAKE(i915))
+		return -ENODEV;
+
+	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 +451,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 +488,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 46a19b099ec88..97ac6fb37958f 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 a1848e8060590..7000e5910a1d7 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -3680,9 +3680,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;
 };
 
@@ -3797,6 +3801,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.40.1

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

[Tvrtko Ursulin]

On 06/06/2023 11:00, Andi Shyti 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.
> However, since PAT index was not clearly defined for platforms prior to
> GEN12 (TGL), so we are limiting this externsion to GEN12+ platforms
> only. See ext_set_pat() in for the implementation details.
> 
> Note: 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
> 
> 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>
> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>

Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

> ---
>   drivers/gpu/drm/i915/gem/i915_gem_create.c | 40 +++++++++++++++++++++
>   drivers/gpu/drm/i915/gem/i915_gem_object.c |  6 ++++
>   include/uapi/drm/i915_drm.h                | 41 ++++++++++++++++++++++
>   3 files changed, 87 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> index bfe1dbda4cb75..d24c0ce8805c7 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,43 @@ 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));
> +
> +	/* Limiting the extension only to Meteor Lake */
> +	if (!IS_METEORLAKE(i915))
> +		return -ENODEV;
> +
> +	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 +451,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 +488,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 46a19b099ec88..97ac6fb37958f 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 a1848e8060590..7000e5910a1d7 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -3680,9 +3680,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;
>   };
>   
> @@ -3797,6 +3801,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: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO creation

[Andi Shyti]

On Tue, Jun 06, 2023 at 11:10:04AM +0100, Tvrtko Ursulin wrote:
> 
> On 06/06/2023 11:00, Andi Shyti 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.
> > However, since PAT index was not clearly defined for platforms prior to
> > GEN12 (TGL), so we are limiting this externsion to GEN12+ platforms
> > only. See ext_set_pat() in for the implementation details.
> > 
> > Note: 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
> > 
> > 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>
> > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> 
> Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Fiuuu! Thanks a lot, Tvrtko!

As soon as CI gives green light, I will get this in.

Andi

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

[Joonas Lahtinen]

Quoting Andi Shyti (2023-06-06 13:18:06)
> On Tue, Jun 06, 2023 at 11:10:04AM +0100, Tvrtko Ursulin wrote:
> > 
> > On 06/06/2023 11:00, Andi Shyti 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.
> > > However, since PAT index was not clearly defined for platforms prior to
> > > GEN12 (TGL), so we are limiting this externsion to GEN12+ platforms
> > > only. See ext_set_pat() in for the implementation details.
> > > 
> > > Note: 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
> > > 
> > > 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

Andi, let's still get these corrected before merging once the media-driver
revert is completed.

Regards, Joonas

> > > 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>
> > > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> > 
> > Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Fiuuu! Thanks a lot, Tvrtko!
> 
> As soon as CI gives green light, I will get this in.
> 
> Andi

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

[Andi Shyti]

> > > > 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.
> > > > However, since PAT index was not clearly defined for platforms prior to
> > > > GEN12 (TGL), so we are limiting this externsion to GEN12+ platforms
> > > > only. See ext_set_pat() in for the implementation details.
> > > > 
> > > > Note: 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
> > > > 
> > > > 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
> 
> Andi, let's still get these corrected before merging once the media-driver
> revert is completed.

Sure!

At least this doesn't need a new version to be respinned.

Please, Carl, link the new pull request and I will update the
commit log.

Andi

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

[Zhang, Carl]

Media driver reverted previous patches, and file a new  PR
https://github.com/intel/media-driver/pull/1680
will hold this PR until the uapi changes appear in drm_next.

besides this, ask a dumb question. 
How we retrieve the pat_index from a shared resource though dma_buf fd?
maybe we need to know whether it could be CPU cached if we want map it.
Of course, looks there are no real usage to access it though CPU. 
Just use it directly without any pat related options ?

Thanks
Carl

> -----Original Message-----
> From: Andi Shyti <andi.shyti@linux.intel.com>
> Sent: Tuesday, June 6, 2023 7:15 PM
> To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>; Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com>; Yang, Fei <fei.yang@intel.com>; Chris
> Wilson <chris@chris-wilson.co.uk>; Roper, Matthew D
> <matthew.d.roper@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Zhang, Carl <carl.zhang@intel.com>; Gu, Lihao <lihao.gu@intel.com>; Intel
> GFX <intel-gfx@lists.freedesktop.org>; DRI Devel <dri-
> devel@lists.freedesktop.org>
> Subject: Re: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO creation
> 
> > > > > 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.
> > > > > However, since PAT index was not clearly defined for platforms
> > > > > prior to
> > > > > GEN12 (TGL), so we are limiting this externsion to GEN12+
> > > > > platforms only. See ext_set_pat() in for the implementation details.
> > > > >
> > > > > Note: 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
> > > > >
> > > > > 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/92c00a857433ebb34ec
> > > > > 575e9834f473c6fcb6341
> > > > > https://github.com/intel/media-driver/commit/fd375cf2c5e1f6bf6b4
> > > > > 3258ff797b3134aadc9fd
> > > > > https://github.com/intel/media-
> driver/commit/08dd244b22484770a33
> > > > > 464c2c8ae85430e548000
> >
> > Andi, let's still get these corrected before merging once the
> > media-driver revert is completed.
> 
> Sure!
> 
> At least this doesn't need a new version to be respinned.
> 
> Please, Carl, link the new pull request and I will update the commit log.
> 
> Andi

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

[Andi Shyti]

Hi Carl,

On Wed, Jun 07, 2023 at 03:40:20AM +0000, Zhang, Carl wrote:
> Media driver reverted previous patches, and file a new  PR
> https://github.com/intel/media-driver/pull/1680
> will hold this PR until the uapi changes appear in drm_next.

That's great, thanks a lot for the quick actions here.

Before pushing I am going to replace the Media part in the commit
log with the following sentence:

"
The media driver supprt has bin submitted in this merge request:
https://github.com/intel/media-driver/pull/1680
"

> besides this, ask a dumb question. 
> How we retrieve the pat_index from a shared resource though dma_buf fd?
> maybe we need to know whether it could be CPU cached if we want map it.
> Of course, looks there are no real usage to access it though CPU. 
> Just use it directly without any pat related options ?

I am not understanding. Do you want to ask the PAT table to the
driver? Are you referring to the CPU PAT index?

In any case, if I understood correctly, you don't necessarily
always need to set the PAT options and the cache options will
fall into the default values.

Please let me know if I haven't answered the question.

Andi

> Thanks
> Carl
> 
> > -----Original Message-----
> > From: Andi Shyti <andi.shyti@linux.intel.com>
> > Sent: Tuesday, June 6, 2023 7:15 PM
> > To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Andi Shyti <andi.shyti@linux.intel.com>; Tvrtko Ursulin
> > <tvrtko.ursulin@linux.intel.com>; Yang, Fei <fei.yang@intel.com>; Chris
> > Wilson <chris@chris-wilson.co.uk>; Roper, Matthew D
> > <matthew.d.roper@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> > Zhang, Carl <carl.zhang@intel.com>; Gu, Lihao <lihao.gu@intel.com>; Intel
> > GFX <intel-gfx@lists.freedesktop.org>; DRI Devel <dri-
> > devel@lists.freedesktop.org>
> > Subject: Re: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO creation
> > 
> > > > > > 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.
> > > > > > However, since PAT index was not clearly defined for platforms
> > > > > > prior to
> > > > > > GEN12 (TGL), so we are limiting this externsion to GEN12+
> > > > > > platforms only. See ext_set_pat() in for the implementation details.
> > > > > >
> > > > > > Note: 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
> > > > > >
> > > > > > 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/92c00a857433ebb34ec
> > > > > > 575e9834f473c6fcb6341
> > > > > > https://github.com/intel/media-driver/commit/fd375cf2c5e1f6bf6b4
> > > > > > 3258ff797b3134aadc9fd
> > > > > > https://github.com/intel/media-
> > driver/commit/08dd244b22484770a33
> > > > > > 464c2c8ae85430e548000
> > >
> > > Andi, let's still get these corrected before merging once the
> > > media-driver revert is completed.
> > 
> > Sure!
> > 
> > At least this doesn't need a new version to be respinned.
> > 
> > Please, Carl, link the new pull request and I will update the commit log.
> > 
> > Andi

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

[Zhang, Carl]

> -----Original Message-----
> From: Andi Shyti <andi.shyti@linux.intel.com>
> Sent: Wednesday, June 7, 2023 1:11 PM
> To: Zhang, Carl <carl.zhang@intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com>; Yang, Fei <fei.yang@intel.com>; Chris
> Wilson <chris@chris-wilson.co.uk>; Roper, Matthew D
> <matthew.d.roper@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Gu, Lihao <lihao.gu@intel.com>; Intel GFX <intel-gfx@lists.freedesktop.org>;
> DRI Devel <dri-devel@lists.freedesktop.org>
> Subject: Re: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO creation
> 
> Hi Carl,
> 
> On Wed, Jun 07, 2023 at 03:40:20AM +0000, Zhang, Carl wrote:
> > Media driver reverted previous patches, and file a new  PR
> > https://github.com/intel/media-driver/pull/1680
> > will hold this PR until the uapi changes appear in drm_next.
> 
> That's great, thanks a lot for the quick actions here.
> 
> Before pushing I am going to replace the Media part in the commit log with
> the following sentence:
> 
> "
> The media driver supprt has bin submitted in this merge request:
> https://github.com/intel/media-driver/pull/1680
> "
> 
> > besides this, ask a dumb question.
> > How we retrieve the pat_index from a shared resource though dma_buf fd?
> > maybe we need to know whether it could be CPU cached if we want map it.
> > Of course, looks there are no real usage to access it though CPU.
> > Just use it directly without any pat related options ?
> 
> I am not understanding. Do you want to ask the PAT table to the driver? Are
> you referring to the CPU PAT index?
> 
> In any case, if I understood correctly, you don't necessarily always need to
> set the PAT options and the cache options will fall into the default values.
> 
> Please let me know if I haven't answered the question.
> 

If mesa create a resource , then use DRM_IOCTL_PRIME_HANDLE_TO_FD convert it to a dma fd. 
Then share it to media, media use DRM_IOCTL_PRIME_FD_TO_HANDLE convert it to a gem bo. 
But media does not know the PAT index , because mesa create it and set it. 
So, if media want to call DRM_IOCTL_I915_GEM_MMAP_OFFSET, media does not know whether it could be WB.

> Andi
> 
> > Thanks
> > Carl
> >
> > > -----Original Message-----
> > > From: Andi Shyti <andi.shyti@linux.intel.com>
> > > Sent: Tuesday, June 6, 2023 7:15 PM
> > > To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Cc: Andi Shyti <andi.shyti@linux.intel.com>; Tvrtko Ursulin
> > > <tvrtko.ursulin@linux.intel.com>; Yang, Fei <fei.yang@intel.com>;
> > > Chris Wilson <chris@chris-wilson.co.uk>; Roper, Matthew D
> > > <matthew.d.roper@intel.com>; Justen, Jordan L
> > > <jordan.l.justen@intel.com>; Zhang, Carl <carl.zhang@intel.com>; Gu,
> > > Lihao <lihao.gu@intel.com>; Intel GFX
> > > <intel-gfx@lists.freedesktop.org>; DRI Devel <dri-
> > > devel@lists.freedesktop.org>
> > > Subject: Re: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO
> > > creation
> > >
> > > > > > > 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.
> > > > > > > However, since PAT index was not clearly defined for
> > > > > > > platforms prior to
> > > > > > > GEN12 (TGL), so we are limiting this externsion to GEN12+
> > > > > > > platforms only. See ext_set_pat() in for the implementation details.
> > > > > > >
> > > > > > > Note: 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-li
> > > > > > > nux/ developer-reference/1-0/tiger-lake.html
> > > > > > >
> > > > > > > BSpec: 45101
> > > > > > >
> > > > > > > Mesa support has been submitted in this merge request:
> > > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22
> > > > > > > 878
> > > > > > >
> > > > > > > The media driver is supported by the following commits:
> > > > > > > https://github.com/intel/media-
> > > driver/commit/92c00a857433ebb34ec
> > > > > > > 575e9834f473c6fcb6341
> > > > > > > https://github.com/intel/media-driver/commit/fd375cf2c5e1f6b
> > > > > > > f6b4
> > > > > > > 3258ff797b3134aadc9fd
> > > > > > > https://github.com/intel/media-
> > > driver/commit/08dd244b22484770a33
> > > > > > > 464c2c8ae85430e548000
> > > >
> > > > Andi, let's still get these corrected before merging once the
> > > > media-driver revert is completed.
> > >
> > > Sure!
> > >
> > > At least this doesn't need a new version to be respinned.
> > >
> > > Please, Carl, link the new pull request and I will update the commit log.
> > >
> > > Andi

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

[Andi Shyti]

Hi Carl,

> > > besides this, ask a dumb question.
> > > How we retrieve the pat_index from a shared resource though dma_buf fd?
> > > maybe we need to know whether it could be CPU cached if we want map it.
> > > Of course, looks there are no real usage to access it though CPU.
> > > Just use it directly without any pat related options ?
> > 
> > I am not understanding. Do you want to ask the PAT table to the driver? Are
> > you referring to the CPU PAT index?
> > 
> > In any case, if I understood correctly, you don't necessarily always need to
> > set the PAT options and the cache options will fall into the default values.
> > 
> > Please let me know if I haven't answered the question.
> > 
> 
> If mesa create a resource , then use DRM_IOCTL_PRIME_HANDLE_TO_FD convert it to a dma fd. 
> Then share it to media, media use DRM_IOCTL_PRIME_FD_TO_HANDLE convert it to a gem bo. 
> But media does not know the PAT index , because mesa create it and set it. 
> So, if media want to call DRM_IOCTL_I915_GEM_MMAP_OFFSET, media does not know whether it could be WB.

That's a good point. To be honest I am not really sure how this
is handled.

Fei, Jordan? Do you have suggestion here?

Andi

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

[Yang, Fei]

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

> Hi Carl,
>
>>>> besides this, ask a dumb question.
>>>> How we retrieve the pat_index from a shared resource though dma_buf fd?
>>>> maybe we need to know whether it could be CPU cached if we want map it.
>>>> Of course, looks there are no real usage to access it though CPU.
>>>> Just use it directly without any pat related options ?
>>>
>>> I am not understanding. Do you want to ask the PAT table to the driver? Are
>>> you referring to the CPU PAT index?
>>>
>>> In any case, if I understood correctly, you don't necessarily always need to
>>> set the PAT options and the cache options will fall into the default values.
>>>
>>> Please let me know if I haven't answered the question.
>>>
>>
>> If mesa create a resource , then use DRM_IOCTL_PRIME_HANDLE_TO_FD convert it to a dma fd.
>> Then share it to media, media use DRM_IOCTL_PRIME_FD_TO_HANDLE convert it to a gem bo.
>> But media does not know the PAT index , because mesa create it and set it.
>> So, if media want to call DRM_IOCTL_I915_GEM_MMAP_OFFSET, media does not know whether it could be WB.
>
> That's a good point. To be honest I am not really sure how this
> is handled.
>
> Fei, Jordan? Do you have suggestion here?

Is it possible to pass the PAT setting when sharing the fd?
Or perhaps we should have kept the get_caching ioctl functional?
Joonas, could you chime in here?

> Andi

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

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

[Zhang, Carl]

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

It is impossible to pass the PAT setting when sharing the fd.

  1.  All api , include vaapi, vulkan, gl... need such extension , the impact is huge.  Even these API level accept , some middle ware , such as ffmpeg... also will reject it.
  2.  PAT is intel specific implementation , and MTL , LNL has different value and meanings, you could not expect application developer understand the details, it should be transparent to application developer.
Thanks
Carl

From: Yang, Fei <fei.yang@intel.com>
Sent: Friday, June 9, 2023 11:13 PM
To: Andi Shyti <andi.shyti@linux.intel.com>; Zhang, Carl <carl.zhang@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>; Chris Wilson <chris@chris-wilson.co.uk>; Roper, Matthew D <matthew.d.roper@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Gu, Lihao <lihao.gu@intel.com>; Intel GFX <intel-gfx@lists.freedesktop.org>; DRI Devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO creation

> Hi Carl,
>
>>>> besides this, ask a dumb question.
>>>> How we retrieve the pat_index from a shared resource though dma_buf fd?
>>>> maybe we need to know whether it could be CPU cached if we want map it.
>>>> Of course, looks there are no real usage to access it though CPU.
>>>> Just use it directly without any pat related options ?
>>>
>>> I am not understanding. Do you want to ask the PAT table to the driver? Are
>>> you referring to the CPU PAT index?
>>>
>>> In any case, if I understood correctly, you don't necessarily always need to
>>> set the PAT options and the cache options will fall into the default values.
>>>
>>> Please let me know if I haven't answered the question.
>>>
>>
>> If mesa create a resource , then use DRM_IOCTL_PRIME_HANDLE_TO_FD convert it to a dma fd.
>> Then share it to media, media use DRM_IOCTL_PRIME_FD_TO_HANDLE convert it to a gem bo.
>> But media does not know the PAT index , because mesa create it and set it.
>> So, if media want to call DRM_IOCTL_I915_GEM_MMAP_OFFSET, media does not know whether it could be WB.
>
> That's a good point. To be honest I am not really sure how this
> is handled.
>
> Fei, Jordan? Do you have suggestion here?

Is it possible to pass the PAT setting when sharing the fd?
Or perhaps we should have kept the get_caching ioctl functional?
Joonas, could you chime in here?

> Andi

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

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

[Andi Shyti]

On Tue, Jun 06, 2023 at 12:00:42PM +0200, Andi Shyti 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.
> However, since PAT index was not clearly defined for platforms prior to
> GEN12 (TGL), so we are limiting this externsion to GEN12+ platforms
> only. See ext_set_pat() in for the implementation details.
> 
> Note: 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
> 
> 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>
> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>

I'm also adding:

Acked-by: Slawomir Milczarek <slawomir.milczarek@intel.com>

from the Compute UMD team.

Thanks, Slawek!
Andi

> ---
>  drivers/gpu/drm/i915/gem/i915_gem_create.c | 40 +++++++++++++++++++++
>  drivers/gpu/drm/i915/gem/i915_gem_object.c |  6 ++++
>  include/uapi/drm/i915_drm.h                | 41 ++++++++++++++++++++++
>  3 files changed, 87 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> index bfe1dbda4cb75..d24c0ce8805c7 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,43 @@ 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));
> +
> +	/* Limiting the extension only to Meteor Lake */
> +	if (!IS_METEORLAKE(i915))
> +		return -ENODEV;
> +
> +	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 +451,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 +488,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 46a19b099ec88..97ac6fb37958f 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 a1848e8060590..7000e5910a1d7 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -3680,9 +3680,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;
>  };
>  
> @@ -3797,6 +3801,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.40.1

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

[Andi Shyti]

Hi Fei,

On Tue, Jun 06, 2023 at 12:00:42PM +0200, Andi Shyti 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.
> However, since PAT index was not clearly defined for platforms prior to
> GEN12 (TGL), so we are limiting this externsion to GEN12+ platforms
> only. See ext_set_pat() in for the implementation details.
> 
> Note: 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
> 
> 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>
> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>

pushed to drm-intel-gt-next with:

 - Tvrtko's ack
 - Slawek's ack
 - the pull request link from media guys

Thank you!
Andi