[igt-dev] [PATCH v5 1/1] test/gem_create: exercise gem_create_ext_set_pat

[Yang, Fei]

> Hi Fei,
>
> On Wed, May 24, 2023 at 08:56:10AM -0700, fei.yang at intel.com wrote:
>> From: Fei Yang <fei.yang at intel.com>
>>
>> Gem_create uAPI has been extended with capability of setting PAT index.
>> Add a test case to exercise it.
>
> As we discussed offline, can we:
>
> /PAT/PAT (Page Attribute Table)/

I did explain "PAT is a page attribute" below. If that's not good enough I
have no objection to add what you proposed above.

> But I think Kamil can fix this before pushing it.
>
>> PAT index is a page attribute that gets programmed into Page Table
>> Entry (PTE) for caching policy control. Please refer to kernel patch
>> for implementation details.
>>
>> https://patchwork.freedesktop.org/series/116870/
>>
>> Signed-off-by: Fei Yang <fei.yang at intel.com>
>> ---
>>  lib/i915/i915_drm_local.h | 34 +++++++++++++++++++++++
>>  tests/i915/gem_create.c   | 58 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 92 insertions(+)
>>
>> diff --git a/lib/i915/i915_drm_local.h b/lib/i915/i915_drm_local.h
>> index bb2ebef3..0f47578c 100644
>> --- a/lib/i915/i915_drm_local.h
>> +++ b/lib/i915/i915_drm_local.h
>> @@ -41,6 +41,40 @@ extern "C" {
>>  #define __I915_PMU_RC6_RESIDENCY(gt)         ___I915_PMU_OTHER(gt, 3)
>>  #define __I915_PMU_SOFTWARE_GT_AWAKE_TIME(gt)        ___I915_PMU_OTHER(gt, 4)
>>
>> +#define I915_GEM_CREATE_EXT_SET_PAT 2
>> +
>> +/**
>> + * 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 */
>> +     __u32 pat_index;
>> +     /** @rsvd: reserved for future use */
>> +     __u32 rsvd;
>> +};
>> +
>>  #if defined(__cplusplus)
>>  }
>>  #endif
>> diff --git a/tests/i915/gem_create.c b/tests/i915/gem_create.c
>> index fc991894..2e0c0ab9 100644
>> --- a/tests/i915/gem_create.c
>> +++ b/tests/i915/gem_create.c
>> @@ -587,6 +587,60 @@ static void create_ext_placement_each(int fd)
>>        free(regions);
>>  }
>>
>> +/**
>> +  * TEST: GEM create uAPI
>> +  * Category: Infrastructure
>> +  * Description: Tests the DRM_IOCTL_I915_GEM_CREATE_EXT ioctl.
>> +  * Feature: core
>> +  * Functionality: Create GEM object with specific PAT index
>> +  * Run type: BAT
>> +  * Sub-category: i915
>> +  * Test category: GEM_Legacy
>> +  *
>> +  * SUBTEST: create-ext-set-pat
>> +  */
>> +static void create_ext_set_pat(int fd)
>> +{
>> +     struct drm_i915_gem_create_ext_set_pat setparam_pat = {
>> +             .base = { .name = I915_GEM_CREATE_EXT_SET_PAT },
>> +             .pat_index = 0,
>> +     };
>> +     struct drm_i915_gem_create_ext_set_pat setparam_inv_pat = {
>> +             .base = { .name = I915_GEM_CREATE_EXT_SET_PAT },
>> +             .pat_index = 65,
>> +     };
>> +     struct drm_i915_gem_caching arg;
>> +     uint64_t size;
>> +     uint32_t handle;
>> +     int ret;
>> +
>> +     size = PAGE_SIZE;
>> +
>> +     ret = __gem_create_ext(fd, &size, 0, &handle, &setparam_pat.base);
>
> Tvrtko was talking about a -EFAULT path that can come only from
> the use extention, unless I missed any other path.
>
> I'm OK if we skip it as it would triggered by setting:
>
>        "name = <something_random>"
>
> right?

The point is that we want specifically check if a failure here is due to
kernel not up-to-date. Otherwise any none-zero return should be
treated as failure.

> In any case:
>
> Reviewed-by: Andi Shyti <andi.shyti at linux.intel.com>
>
> Andi
>
>> +     /*
>> +      * With a valid PAT index specified, returning -EINVAL here
>> +      * indicates set_pat extension is not supported
>> +      */
>> +     if (ret == -EINVAL)
>> +             igt_skip("I915_GEM_CREATE_EXT_SET_PAT is not supported\n");
>> +     igt_assert(ret == 0);
>> +
>> +     /* {set|get}_caching ioctl should fail for objects created with set_pat */
>> +     igt_assert_eq(__gem_set_caching(fd, handle, 1), -EOPNOTSUPP);
>> +
>> +     memset(&arg, 0, sizeof(arg));
>> +     arg.handle = handle;
>> +     igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_GET_CACHING, &arg) < 0 &&
>> +                errno == EOPNOTSUPP);
>> +
>> +     /* gem_create should fail with -EINVAL if invalid pat index specified */
>> +     igt_assert_eq(__gem_create_ext(fd, &size, 0, &handle, &setparam_inv_pat.base),
>> +                   -EINVAL);
>> +
>> +     gem_close(fd, handle);
>> +}
>> +
>>  static bool supports_needs_cpu_access(int fd)
>>  {
>>        struct drm_i915_gem_memory_class_instance regions[] = {
>> @@ -947,6 +1001,10 @@ igt_main
>>        igt_subtest("create-ext-placement-all")
>>                create_ext_placement_all(fd);
>>
>> +     igt_describe("Validate basic creation of objects with PAT cache setting.");
>> +     igt_subtest("create-ext-set-pat")
>> +             create_ext_set_pat(fd);
>> +
>>        igt_describe("Verify the basic functionally and expected ABI contract around I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS");
>>        igt_subtest("create-ext-cpu-access-sanity-check") {
>>                igt_require(supports_needs_cpu_access(fd));
>> --
>> 2.25.1

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20230524/e5ec918f/attachment-0001.htm>