[igt-dev] [PATCH] tests/i915/gem_mmap_wc: Update test to use mmap_wc IOCTL

[Dixit, Ashutosh]

On Wed, 22 Jan 2020 15:33:26 -0800, Vinay Belgaumkar wrote:
>
> Purpose of this test is to validate the mmap_wc ioctl. After recent
> additions to library wrappers, the local wrapper ended up calling
> the unintended mmap. Also check for presence of mappable GGTT before
> testing it.
>
> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
> Cc: Antonio Argenziano <antonio.argenziano at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  tests/i915/gem_mmap_wc.c | 59 ++++++++++++++++++++++++----------------
>  1 file changed, 35 insertions(+), 24 deletions(-)
>
> diff --git a/tests/i915/gem_mmap_wc.c b/tests/i915/gem_mmap_wc.c
> index 375a9b50..27334b6d 100644
> --- a/tests/i915/gem_mmap_wc.c
> +++ b/tests/i915/gem_mmap_wc.c
> @@ -51,19 +51,36 @@ struct local_i915_gem_mmap_v2 {
>
>  static int OBJECT_SIZE = 16*1024*1024;
>
> +static int mmap_ioctl(int i915, struct drm_i915_gem_mmap *arg)
> +{
> +	int err = 0;
> +
> +	if (igt_ioctl(i915, DRM_IOCTL_I915_GEM_MMAP, arg))
> +		err = -errno;
> +
> +	errno = 0;
> +	return err;
> +}
> +
>  /*
>   * Local WC mmap wrapper. This is used to make sure we go through
>   * the GEM_MMAP IOCTL.
>   * */
>  static void *
> -local_gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot)
> +local_gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, uint64_t size)
>  {
> -	void *ptr;
>
> -	ptr = __gem_mmap__wc(fd, handle, 0, OBJECT_SIZE, PROT_READ | PROT_WRITE);
> -	igt_assert(ptr);
> +	struct drm_i915_gem_mmap arg = {
> +		.handle = handle,
> +		.offset = offset,
> +		.size = size,
> +		.flags = I915_MMAP_WC,
> +	};
>
> -	return ptr;
> +	igt_assert_eq(mmap_ioctl(fd, &arg), 0);

Sorry maybe I am missing something but could you please explain which
unintended mmap is being called, at least at this point it seems
__gem_mmap__wc() is also calling the DRM_IOCTL_I915_GEM_MMAP ioctl? I agree
the protection flags are unused and can be removed.

> @@ -593,14 +600,18 @@ igt_main
>		run_without_prefault(fd, test_read);
>	igt_subtest("write-no-prefault")
>		run_without_prefault(fd, test_write);
> -	igt_subtest("write-gtt-no-prefault")
> +	igt_subtest("write-gtt-no-prefault") {
> +		gem_require_mappable_ggtt(fd);
>		run_without_prefault(fd, test_write_gtt);

test_write_gtt() is actually named incorrectly, it is actually setting up a
WC mapping so no need to skip?

> +	}
>	igt_subtest("write-cpu-read-wc")
>		test_write_cpu_read_wc(fd, 1);
>	igt_subtest("write-cpu-read-wc-unflushed")
>		test_write_cpu_read_wc(fd, 0);
> -	igt_subtest("write-gtt-read-wc")
> +	igt_subtest("write-gtt-read-wc") {
> +		gem_require_mappable_ggtt(fd);
>		test_write_gtt_read_wc(fd);

Here it is ok to skip I think.