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

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi Fei,

On Wed, May 24, 2023 at 08:56:10AM -0700, fei.yang@intel.com wrote:
> From: Fei Yang <fei.yang@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)/

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@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?

In any case:

Reviewed-by: Andi Shyti <andi.shyti@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