[igt-dev] [RFC PATCH 1/4] lib/i915/gem_mman: add mmap_offset support

[Joonas Lahtinen]

Quoting Lukasz Kalamarz (2019-08-14 13:21:37)
> With introduction of LMEM concept new IOCTL call were implemented
> - gem_mmap_offset. This patch add support in IGT for it.
> 
> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz at intel.com>
> Signed-off-by: Antonio Argenziano <antonio.argenziano at intel.com>
> Cc: Matthew Auld <matthew.auld at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>

<SNIP>

> @@ -106,35 +127,80 @@ bool gem_mmap__has_wc(int fd)
>         static int has_wc = -1;
>  
>         if (has_wc == -1) {
> -               struct drm_i915_getparam gp;
> -               int mmap_version = -1;
> -               int gtt_version = -1;
>  
>                 has_wc = 0;
>  
> -               memset(&gp, 0, sizeof(gp));
> -               gp.param = I915_PARAM_MMAP_GTT_VERSION;
> -               gp.value = &gtt_version;
> -               ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> -
> -               memset(&gp, 0, sizeof(gp));
> -               gp.param = I915_PARAM_MMAP_VERSION;
> -               gp.value = &mmap_version;
> -               ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> -
> -               /* Do we have the new mmap_ioctl with DOMAIN_WC? */
> -               if (mmap_version >= 1 && gtt_version >= 2) {
> -                       struct drm_i915_gem_mmap arg;
> +               /* Do we have the new mmap_offset ioctl? */
> +               if (gem_has_mmap_offset(fd)) {
> +                       struct local_i915_gem_mmap_offset arg;
>  
>                         /* Does this device support wc-mmaps ? */
>                         memset(&arg, 0, sizeof(arg));
>                         arg.handle = gem_create(fd, 4096);
>                         arg.offset = 0;
> -                       arg.size = 4096;
> -                       arg.flags = I915_MMAP_WC;
> -                       has_wc = igt_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP, &arg) == 0;
> +                       arg.flags = LOCAL_I915_MMAP_OFFSET_WC;
> +                       has_wc = igt_ioctl(fd, LOCAL_IOCTL_I915_GEM_MMAP_OFFSET,
> +                                          &arg) == 0;
> +                       gem_close(fd, arg.handle);

This is duplicated at gem_mmap_offset__has_wc()

This logic shouldn't be needed either, if we have mmap_offset, we'll
have WC support. What this logic checks is if default placed object will
support WC mmap, which is very different from what this function
originally did.

<SNIP>

> +bool gem_mmap_offset__has_wc(int fd)
> +{

Ditto, shouldn't be needed ever.

<SNIP>

> @@ -194,7 +297,10 @@ static void
>   */
>  void *__gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot)
>  {
> -       return __gem_mmap(fd, handle, offset, size, prot, I915_MMAP_WC);
> +       if (gem_has_mmap_offset(fd))
> +               return __gem_mmap_offset(fd, handle, offset, size, prot, LOCAL_I915_MMAP_OFFSET_WC);

Again, there is code duplication below (__gem_mmap_offset__wc).

> +       else
> +               return __gem_mmap(fd, handle, offset, size, prot, I915_MMAP_WC);

The naming also gets confusing when we have nesting multiplexers.

I would assume us to want to flatten the code:

	if (has_mmap_offset())
		return __gem_mmap_offset__wc()
	else
		return __gem_mmap_gtt__wc()

That probably requires some renames.

Regards, Joonas