| Message ID | 20230524200255.443021-2-fei.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Allow user to set cache at BO creation | expand |
Hi Carl, On Wed, May 24, 2023 at 01:02:55PM -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. > > 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 <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> was it your intention to ack this patch? Thanks, Andi
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 <carl.zhang@intel.com> Tested-by: Lihao Gu <lihao.gu@intel.com> Thanks Carl > -----Original Message----- > From: Andi Shyti <andi.shyti@linux.intel.com> > Sent: Wednesday, May 31, 2023 6:49 PM > To: Yang, Fei <fei.yang@intel.com>; Zhang, Carl <carl.zhang@intel.com> > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vivi, > Rodrigo <rodrigo.vivi@intel.com>; andi.shyti@linux.intel.com; Chris Wilson > <chris.p.wilson@linux.intel.com>; Roper, Matthew D > <matthew.d.roper@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com> > 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.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. > > > > 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 <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> > > was it your intention to ack this patch? > > Thanks, > Andi
Hi Carl, On Wed, May 31, 2023 at 12:24:07PM +0000, 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 <carl.zhang@intel.com> > Tested-by: Lihao Gu <lihao.gu@intel.com> Thanks Carl and Lihao! Very appreciated :) Andi > Thanks > Carl > > -----Original Message----- > > From: Andi Shyti <andi.shyti@linux.intel.com> > > Sent: Wednesday, May 31, 2023 6:49 PM > > To: Yang, Fei <fei.yang@intel.com>; Zhang, Carl <carl.zhang@intel.com> > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vivi, > > Rodrigo <rodrigo.vivi@intel.com>; andi.shyti@linux.intel.com; Chris Wilson > > <chris.p.wilson@linux.intel.com>; Roper, Matthew D > > <matthew.d.roper@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com> > > 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.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. > > > > > > 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 <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> > > > > was it your intention to ack this patch? > > > > Thanks, > > Andi >
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