Re: [PATCH 2/4] drm/i915: Introduce DRM_I915_GEM_MMAP_OFFSET

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Nov 19, 2019 at 01:37:08PM +0200, Abdiel Janulgue wrote:
> This is really just an alias of mmap_gtt. The 'mmap offset' nomenclature
> comes from the value returned by this ioctl which is the offset into the
> device fd which userpace uses with mmap(2).
> 
> mmap_gtt was our initial mmap_offset implementation, this extends
> our CPU mmap support to allow additional fault handlers that depends on
> the object's backing pages.
> 
> Note that we multiplex mmap_gtt and mmap_offset through the same ioctl,
> and use the zero extending behaviour of drm to differentiate between
> them, when we inspect the flags.
> 
> v2:
> - Drop the alias, just rename the struct (Chris)
> - Don't bail out on no PAT when doing WB mmaps
> - Prepare uAPI for further extensions
> v3:
> - drop MMAP_OFFSET_FLAGS
> v4:
> - Tweaks, header re-org
> 
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_ioctls.h    |  4 +-
>  drivers/gpu/drm/i915/gem/i915_gem_mman.c      | 45 ++++++++++++++++---
>  drivers/gpu/drm/i915/gem/i915_gem_mman.h      |  1 +
>  .../gpu/drm/i915/gem/i915_gem_object_types.h  |  3 ++
>  drivers/gpu/drm/i915/i915_drv.c               |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h               |  1 -
>  include/uapi/drm/i915_drm.h                   | 27 +++++++++++
>  7 files changed, 72 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ioctls.h b/drivers/gpu/drm/i915/gem/i915_gem_ioctls.h
> index ddc7f2a52b3e..87d8b27f426d 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ioctls.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ioctls.h
> @@ -28,8 +28,8 @@ int i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
>  			   struct drm_file *file);
>  int i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
>  			struct drm_file *file);
> -int i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data,
> -			    struct drm_file *file);
> +int i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data,
> +			       struct drm_file *file);
>  int i915_gem_pread_ioctl(struct drm_device *dev, void *data,
>  			 struct drm_file *file);
>  int i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 36fffb671601..bb05c53c03c8 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -145,6 +145,9 @@ static unsigned int tile_row_pages(const struct drm_i915_gem_object *obj)
>   * 3 - Remove implicit set-domain(GTT) and synchronisation on initial
>   *     pagefault; swapin remains transparent.
>   *
> + * 4 - Support multiple fault handlers per object depending on object's
> + *     backing storage (a.k.a. MMAP_OFFSET).
> + *
>   * Restrictions:
>   *
>   *  * snoopable objects cannot be accessed via the GTT. It can cause machine
> @@ -172,7 +175,7 @@ static unsigned int tile_row_pages(const struct drm_i915_gem_object *obj)
>   */
>  int i915_gem_mmap_gtt_version(void)
>  {
> -	return 3;
> +	return 4;
>  }
>  
>  static inline struct i915_ggtt_view
> @@ -538,7 +541,7 @@ __assign_gem_object_mmap_data(struct drm_file *file,
>  }
>  
>  /**
> - * i915_gem_mmap_gtt_ioctl - prepare an object for GTT mmap'ing
> + * i915_gem_mmap_offset_ioctl - prepare an object for GTT mmap'ing
>   * @dev: DRM device
>   * @data: GTT mapping ioctl data
>   * @file: GEM object info
> @@ -553,13 +556,41 @@ __assign_gem_object_mmap_data(struct drm_file *file,
>   * userspace.
>   */
>  int
> -i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data,
> -			struct drm_file *file)
> +i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data,
> +			   struct drm_file *file)
>  {
> -	struct drm_i915_gem_mmap_gtt *args = data;
> +	struct drm_i915_private *i915 = to_i915(dev);
> +	struct drm_i915_gem_mmap_offset *args = data;
> +	enum i915_mmap_type type;
> +
> +	switch (args->flags) {
> +	case I915_MMAP_OFFSET_GTT:
> +		if (!i915_ggtt_has_aperture(&i915->ggtt))
> +			return -ENODEV;
> +		type = I915_MMAP_TYPE_GTT;
> +		break;
> +
> +	case I915_MMAP_OFFSET_WC:
> +		if (!boot_cpu_has(X86_FEATURE_PAT))
> +			return -ENODEV;
> +		type = I915_MMAP_TYPE_WC;
> +		break;
> +
> +	case I915_MMAP_OFFSET_WB:
> +		type = I915_MMAP_TYPE_WB;
> +		break;
> +
> +	case I915_MMAP_OFFSET_UC:
> +		if (!boot_cpu_has(X86_FEATURE_PAT))
> +			return -ENODEV;
> +		type = I915_MMAP_TYPE_UC;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
>  
> -	return __assign_gem_object_mmap_data(file, args->handle,
> -					     I915_MMAP_TYPE_GTT,
> +	return __assign_gem_object_mmap_data(file, args->handle, type,
>  					     &args->offset);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.h b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> index 25a3c4d6cd65..4d3b493e853a 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> @@ -24,5 +24,6 @@ void i915_mmap_offset_destroy(struct i915_mmap_offset *mmo, struct mutex *mutex)
>  void __i915_gem_object_release_mmap_gtt(struct drm_i915_gem_object *obj);
>  void i915_gem_object_release_mmap(struct drm_i915_gem_object *obj);
>  void i915_gem_object_release_mmap_offset(struct drm_i915_gem_object *obj);
> +int i915_gem_mmap_gtt_version(void);
>  
>  #endif
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> index 8ff0834a1d5c..c0beb97b56d1 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> @@ -65,6 +65,9 @@ struct drm_i915_gem_object_ops {
>  
>  enum i915_mmap_type {
>  	I915_MMAP_TYPE_GTT = 0,
> +	I915_MMAP_TYPE_WC,
> +	I915_MMAP_TYPE_WB,
> +	I915_MMAP_TYPE_UC,
>  };
>  
>  struct i915_mmap_offset {
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3e129bcf8a12..ac6d4470ce75 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2715,7 +2715,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(I915_GEM_PREAD, i915_gem_pread_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_PWRITE, i915_gem_pwrite_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_MMAP, i915_gem_mmap_ioctl, DRM_RENDER_ALLOW),
> -	DRM_IOCTL_DEF_DRV(I915_GEM_MMAP_GTT, i915_gem_mmap_gtt_ioctl, DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF_DRV(I915_GEM_MMAP_OFFSET, i915_gem_mmap_offset_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_SET_DOMAIN, i915_gem_set_domain_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_SW_FINISH, i915_gem_sw_finish_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_SET_TILING, i915_gem_set_tiling_ioctl, DRM_RENDER_ALLOW),
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a36c2778c09c..b2af6df2eae0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1853,7 +1853,6 @@ i915_mutex_lock_interruptible(struct drm_device *dev)
>  int i915_gem_dumb_create(struct drm_file *file_priv,
>  			 struct drm_device *dev,
>  			 struct drm_mode_create_dumb *args);
> -int i915_gem_mmap_gtt_version(void);
>  
>  int __must_check i915_gem_set_global_seqno(struct drm_device *dev, u32 seqno);
>  
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 5400d7e057f1..e844c3a8d197 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -395,6 +395,7 @@ typedef struct _drm_i915_sarea {
>  #define DRM_IOCTL_I915_GEM_PWRITE	DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_PWRITE, struct drm_i915_gem_pwrite)
>  #define DRM_IOCTL_I915_GEM_MMAP		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MMAP, struct drm_i915_gem_mmap)
>  #define DRM_IOCTL_I915_GEM_MMAP_GTT	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MMAP_GTT, struct drm_i915_gem_mmap_gtt)
> +#define DRM_IOCTL_I915_GEM_MMAP_OFFSET	DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MMAP_GTT, struct drm_i915_gem_mmap_offset)
>  #define DRM_IOCTL_I915_GEM_SET_DOMAIN	DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_SET_DOMAIN, struct drm_i915_gem_set_domain)
>  #define DRM_IOCTL_I915_GEM_SW_FINISH	DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_SW_FINISH, struct drm_i915_gem_sw_finish)
>  #define DRM_IOCTL_I915_GEM_SET_TILING	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_SET_TILING, struct drm_i915_gem_set_tiling)
> @@ -793,6 +794,32 @@ struct drm_i915_gem_mmap_gtt {
>  	__u64 offset;
>  };
>  
> +struct drm_i915_gem_mmap_offset {
> +	/** Handle for the object being mapped. */
> +	__u32 handle;
> +	__u32 pad;
> +	/**
> +	 * Fake offset to use for subsequent mmap call
> +	 *
> +	 * This is a fixed-size type for 32/64 compatibility.
> +	 */
> +	__u64 offset;
> +
> +	/**
> +	 * Flags for extended behaviour.
> +	 *
> +	 * It is mandatory that either one of the MMAP_OFFSET flags
> +	 * should be passed here.

What does "either one" mean? "at least one", or "one and only one"?

> +	 */
> +	__u64 flags;
> +#define I915_MMAP_OFFSET_GTT 0
> +#define I915_MMAP_OFFSET_WC  1
> +#define I915_MMAP_OFFSET_WB  2
> +#define I915_MMAP_OFFSET_UC  3

These are mean to be used as I915_MMAP_OFFSET_FOO << 0
or 1 << I915_MMAP_OFFSET_FOO ?

/me looks at the code. Seems like the <<0 version is correct,
and that also explains the earlier question since you can't
even pass more than one, nor can you pass none since
I915_MMAP_OFFSET_GTT is 0.

Maybe these defines should inclue <<0 in them to make the
usage 100% obvious?

> +
> +	__u64 extensions;
> +};
> +
>  struct drm_i915_gem_set_domain {
>  	/** Handle for the object */
>  	__u32 handle;
> -- 
> 2.23.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx