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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> On 16/05/2023 19:11, fei.yang@xxxxxxxxx wrote:
>> From: Fei Yang <fei.yang@xxxxxxxxx>
>>
>> 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.
>>
>> Test igt@gem_create@create_ext_set_pat posted at
>> https://patchwork.freedesktop.org/series/117695/
>>
>> Tested with https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
>>
>> Signed-off-by: Fei Yang <fei.yang@xxxxxxxxx>
>> Cc: Chris Wilson <chris.p.wilson@xxxxxxxxxxxxxxx>
>> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
>> Cc: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
>> Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
>> Tested-by: Jordan Justen <jordan.l.justen@xxxxxxxxx>
>> ---
>>   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                | 42 ++++++++++++++++++++++
>>   tools/include/uapi/drm/i915_drm.h          | 42 ++++++++++++++++++++++
>>   4 files changed, 126 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 ba40855dbc93..4f3fd650e5e1 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -3664,9 +3664,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;
>>   };
>>  
>> @@ -3781,6 +3785,44 @@ 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,
>> +      * e.g. BSpec 45101.
>
> AFAIU BSpec numbers do not work from the outside so please put an url to
> the place where PRMs are hosted instead.

Yeah, I was thinking of https://www.intel.com/content/www/us/en/docs/graphics-for-linux/developer-reference/1-0/hardware-specs.html
But from these documents I could only find the definition of PTE which is
referring the concept of to PAT: Page Attribute indirectly. There isn't any
further detail and apparently the PAT tables are not included there either.

Any suggestion?

>> +      */
>> +     __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
>>  
>> diff --git a/tools/include/uapi/drm/i915_drm.h b/tools/include/uapi/drm/i915_drm.h
>> index 8df261c5ab9b..4fbfa472b9fc 100644
>> --- a/tools/include/uapi/drm/i915_drm.h
>> +++ b/tools/include/uapi/drm/i915_drm.h
>
> Changes to this file I think you should drop. Looking at the log, it is
> mostly other folks who synchronise it.

I don't really know how this is maintained, just grep'ed for i915 uAPIs and found
this copy. But okay I can drop it if a different group of people is supposed to
maintain it.

> Regards,
>
> Tvrtko
>
>> @@ -3607,9 +3607,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;
>>   };
>>  
>> @@ -3724,6 +3728,44 @@ 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,
>> +      * e.g. BSpec 45101.
>> +      */
>> +     __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
>>  

[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux