Re: [PATCH v12 1/1] drm/i915: Allow user to set cache at BO creation
Hi Carl,
On Wed, May 31, 2023 at 12:24:07PM +, Zhang, Carl wrote:
> Hi Andi & Fei,
> We verified your change by UMD change:
> 1. implement the uAPI by
>
> https://github.com/intel/media-driver/commit/92c00a857433ebb34ec575e9834f473c6fcb6341
> 2. old kernel may not support new uAPI, so UMD try the interface firstly, if
> it failed, will fallback to older interfaces
> https://github.com/intel/media-driver/commit/fd375cf2c5e1f6bf6b43258ff797b3134aadc9fd
> 3. removed some check for CPU cacheable and PAT conflict
>
> https://github.com/intel/media-driver/commit/08dd244b22484770a33464c2c8ae85430e548000
Thanks a lot... we can add these commits in the log, as well.
> after that, UMD works with your patches serious on MTL.
>
> Acked-by: Carl Zhang
> Tested-by: Lihao Gu
Thanks Carl and Lihao! Very appreciated :)
Andi
> Thanks
> Carl
> > -Original Message-
> > From: Andi Shyti
> > Sent: Wednesday, May 31, 2023 6:49 PM
> > To: Yang, Fei ; Zhang, Carl
> > Cc: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vivi,
> > Rodrigo ; andi.sh...@linux.intel.com; Chris Wilson
> > ; Roper, Matthew D
> > ; Justen, Jordan L
> > Subject: Re: [PATCH v12 1/1] drm/i915: Allow user to set cache at BO
> > creation
> >
> > Hi Carl,
> >
> > On Wed, May 24, 2023 at 01:02:55PM -0700, fei.y...@intel.com wrote:
> > > From: Fei Yang
> > >
> > > 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.
> > >
> > > BSpec: 45101
> > >
> > > Test igt@gem_create@create_ext_set_pat posted at
> > > https://patchwork.freedesktop.org/series/118314/
> > >
> > > Tested with
> > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
> > >
> > > Signed-off-by: Fei Yang
> > > Cc: Chris Wilson
> > > Cc: Matt Roper
> > > Cc: Andi Shyti
> > > Reviewed-by: Andi Shyti
> > > Acked-by: Jordan Justen
> > > Tested-by: Jordan Justen
> >
> > was it your intention to ack this patch?
> >
> > Thanks,
> > Andi
>
RE: [PATCH v12 1/1] drm/i915: Allow user to set cache at BO creation
Hi Andi & Fei,
We verified your change by UMD change:
1. implement the uAPI by
https://github.com/intel/media-driver/commit/92c00a857433ebb34ec575e9834f473c6fcb6341
2. old kernel may not support new uAPI, so UMD try the interface firstly, if it
failed, will fallback to older interfaces
https://github.com/intel/media-driver/commit/fd375cf2c5e1f6bf6b43258ff797b3134aadc9fd
3. removed some check for CPU cacheable and PAT conflict
https://github.com/intel/media-driver/commit/08dd244b22484770a33464c2c8ae85430e548000
after that, UMD works with your patches serious on MTL.
Acked-by: Carl Zhang
Tested-by: Lihao Gu
Thanks
Carl
> -Original Message-
> From: Andi Shyti
> Sent: Wednesday, May 31, 2023 6:49 PM
> To: Yang, Fei ; Zhang, Carl
> Cc: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vivi,
> Rodrigo ; andi.sh...@linux.intel.com; Chris Wilson
> ; Roper, Matthew D
> ; Justen, Jordan L
> Subject: Re: [PATCH v12 1/1] drm/i915: Allow user to set cache at BO creation
>
> Hi Carl,
>
> On Wed, May 24, 2023 at 01:02:55PM -0700, fei.y...@intel.com wrote:
> > From: Fei Yang
> >
> > 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.
> >
> > BSpec: 45101
> >
> > Test igt@gem_create@create_ext_set_pat posted at
> > https://patchwork.freedesktop.org/series/118314/
> >
> > Tested with
> > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
> >
> > Signed-off-by: Fei Yang
> > Cc: Chris Wilson
> > Cc: Matt Roper
> > Cc: Andi Shyti
> > Reviewed-by: Andi Shyti
> > Acked-by: Jordan Justen
> > Tested-by: Jordan Justen
>
> was it your intention to ack this patch?
>
> Thanks,
> Andi
Re: [PATCH v12 1/1] drm/i915: Allow user to set cache at BO creation
Hi Carl,
On Wed, May 24, 2023 at 01:02:55PM -0700, fei.y...@intel.com wrote:
> From: Fei Yang
>
> 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.
>
> BSpec: 45101
>
> Test igt@gem_create@create_ext_set_pat posted at
> https://patchwork.freedesktop.org/series/118314/
>
> Tested with https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
>
> Signed-off-by: Fei Yang
> Cc: Chris Wilson
> Cc: Matt Roper
> Cc: Andi Shyti
> Reviewed-by: Andi Shyti
> Acked-by: Jordan Justen
> Tested-by: Jordan Justen
was it your intention to ack this patch?
Thanks,
Andi
[PATCH v12 1/1] drm/i915: Allow user to set cache at BO creation
From: Fei Yang
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.
BSpec: 45101
Test igt@gem_create@create_ext_set_pat posted at
https://patchwork.freedesktop.org/series/118314/
Tested with https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
Signed-off-by: Fei Yang
Cc: Chris Wilson
Cc: Matt Roper
Cc: Andi Shyti
Reviewed-by: Andi Shyti
Acked-by: Jordan Justen
Tested-by: Jordan Justen
---
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(, base, sizeof(ext)))
+ return -EFAULT;
+
+ max_pat_index = INTEL_INFO(i915)->max_pat_index;
+
+ if (ext.pat_index > max_pat_index) {
+ drm_dbg(>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 0x
/**
* 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, >size, >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 {
*
*
