From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3895210E087 for ; Wed, 24 May 2023 18:06:46 +0000 (UTC) Date: Wed, 24 May 2023 20:06:40 +0200 From: Andi Shyti To: fei.yang@intel.com Message-ID: References: <20230524155610.360748-1-fei.yang@intel.com> <20230524155610.360748-2-fei.yang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230524155610.360748-2-fei.yang@intel.com> Subject: Re: [igt-dev] [PATCH v5 1/1] test/gem_create: exercise gem_create_ext_set_pat List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: igt-dev@lists.freedesktop.org Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: Hi Fei, On Wed, May 24, 2023 at 08:56:10AM -0700, fei.yang@intel.com wrote: > From: Fei Yang > > 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)/ 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 > --- > 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 = " right? In any case: Reviewed-by: Andi Shyti 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