Re: [RFC PATCH 30/42] drm/i915: Introduce DRM_I915_GEM_MMAP_OFFSET

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 14/02/2019 14:57, Matthew Auld wrote:
> From: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> 
> CPU mmap implementation depending on the object's backing pages.

depends?

> At the moment we introduce shmem and local-memory BAR fault handlers
> Note that the mmap type is done one at a time to circumvent the DRM
> offset manager limitation. Note that we multiplex mmap_gtt and

Perhaps it is time to sort out the offset manager? I have a feeling that 
would make things much easier/cleaner for us.

And I at least find mmap_origin a confusing term. It is nor a origin of 
a mapping, but location of object backing store what matters, right?

> mmap_offset through the same ioctl, and use the zero extending behaviour
> of drm to differentiate between them, when we inspect the flags.
> 
> 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/i915_drv.c        |  5 +-
>   drivers/gpu/drm/i915/i915_drv.h        |  3 +
>   drivers/gpu/drm/i915/i915_gem.c        | 94 ++++++++++++++++++++++----
>   drivers/gpu/drm/i915/i915_gem_object.h | 10 +++
>   include/uapi/drm/i915_drm.h            | 30 ++++++++
>   5 files changed, 126 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b1200d7ebd13..90785030a0dd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -423,6 +423,7 @@ static int i915_getparam_ioctl(struct drm_device *dev, void *data,
>   	case I915_PARAM_HAS_EXEC_CAPTURE:
>   	case I915_PARAM_HAS_EXEC_BATCH_FIRST:
>   	case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
> +	case I915_PARAM_MMAP_OFFSET_VERSION:
>   		/* For the time being all of these are always true;
>   		 * if some supported hardware does not have one of these
>   		 * features this value needs to be provided from
> @@ -2936,7 +2937,7 @@ const struct dev_pm_ops i915_pm_ops = {
>   static const struct vm_operations_struct i915_gem_vm_ops = {
>   	.fault = i915_gem_fault,
>   	.open = drm_gem_vm_open,
> -	.close = drm_gem_vm_close,
> +	.close = i915_gem_close,
>   };
>   
>   static const struct file_operations i915_driver_fops = {
> @@ -2991,7 +2992,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_gtt_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 065953a9264f..c6ae157d0ede 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2770,6 +2770,8 @@ int i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
>   			struct drm_file *file_priv);
>   int i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data,
>   			struct drm_file *file_priv);
> +int i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data,
> +			       struct drm_file *file_priv);
>   int i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>   			      struct drm_file *file_priv);
>   int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
> @@ -3073,6 +3075,7 @@ void i915_gem_suspend_late(struct drm_i915_private *dev_priv);
>   void i915_gem_resume(struct drm_i915_private *dev_priv);
>   int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma);
>   vm_fault_t i915_gem_fault(struct vm_fault *vmf);
> +void i915_gem_close(struct vm_area_struct *vma);
>   int i915_gem_object_wait(struct drm_i915_gem_object *obj,
>   			 unsigned int flags,
>   			 long timeout);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 48dbb57fbc6d..cc6c88ec749d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2123,11 +2123,12 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
>   	drm_gem_free_mmap_offset(&obj->base);
>   }
>   
> -int
> -i915_gem_mmap_gtt(struct drm_file *file,
> -		  struct drm_device *dev,
> -		  u32 handle,
> -		  u64 *offset)
> +static int
> +__assign_gem_object_mmap_data(struct drm_file *file,
> +			      u32 handle,
> +			      enum i915_cpu_mmap_origin_type mmap_type,
> +			      u64 mmap_flags,
> +			      u64 *offset)
>   {
>   	struct drm_i915_gem_object *obj;
>   	int ret;
> @@ -2136,14 +2137,35 @@ i915_gem_mmap_gtt(struct drm_file *file,
>   	if (!obj)
>   		return -ENOENT;
>   
> +	if (atomic_read(&obj->mmap_count) &&
> +	    obj->mmap_origin != mmap_type) {

What is the locking for mmap_count? Can it change state between first 
and second part of the conditional? If not, does it need to be atomic?

> +	        /* Re-map object with existing different map-type */
> +		ret = -EINVAL;

Would -EBUSY be a better fit?

> +		goto err;
> +	}
> +
>   	ret = i915_gem_object_create_mmap_offset(obj);

If mmap_count is greater than zero, and type/origin match, why a new 
offset is needed?

> -	if (ret == 0)
> +	if (ret == 0) {
> +		obj->mmap_origin = mmap_type;

Right, so why not obj->mmap_type as well?

> +		obj->mmap_flags = mmap_flags;
>   		*offset = drm_vma_node_offset_addr(&obj->base.vma_node);
> +	}
>   
> + err:
>   	i915_gem_object_put(obj);
>   	return ret;
>   }
>   
> +int
> +i915_gem_mmap_gtt(struct drm_file *file,
> +		  struct drm_device *dev,
> +		  u32 handle,
> +		  u64 *offset)
> +{
> +	return __assign_gem_object_mmap_data(file, handle, I915_MMAP_ORIGIN_GTT,
> +					     0, offset);
> +}

Is there a caller for this function at this point?

> +
>   /**
>    * i915_gem_mmap_gtt_ioctl - prepare an object for GTT mmap'ing
>    * @dev: DRM device
> @@ -2163,9 +2185,45 @@ int
>   i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data,
>   			struct drm_file *file)
>   {
> -	struct drm_i915_gem_mmap_gtt *args = data;
> +	struct drm_i915_gem_mmap_offset *args = data;
> +	struct drm_i915_private *i915 = to_i915(dev);
> +
> +	if (args->flags & I915_MMAP_OFFSET_FLAGS)
> +		return i915_gem_mmap_offset_ioctl(dev, data, file);
> +
> +	if (!HAS_MAPPABLE_APERTURE(i915)) {
> +		DRM_ERROR("No aperture, cannot mmap via legacy GTT\n");

Maybe best to lose the DRM_ERROR since userspace can hammer on it and it 
is not really an error in the driver.

> +		return -ENODEV;
> +	}
> +
> +	return __assign_gem_object_mmap_data(file, args->handle,
> +					     I915_MMAP_ORIGIN_GTT,
> +					     0, &args->offset);
> +}
> +
> +int i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data,
> +			       struct drm_file *file)
> +{
> +	struct drm_i915_gem_mmap_offset *args = data;
>   
> -	return i915_gem_mmap_gtt(file, dev, args->handle, &args->offset);
> +	if ((args->flags & (I915_MMAP_OFFSET_WC | I915_MMAP_OFFSET_WB)) &&
> +	    !boot_cpu_has(X86_FEATURE_PAT))
> +		return -ENODEV;
> +
> +        return __assign_gem_object_mmap_data(file, args->handle,
> +					     I915_MMAP_ORIGIN_OFFSET,
> +					     args->flags,
> +					     &args->offset);
> +}
> +
> +void i915_gem_close(struct vm_area_struct *vma)
> +{
> +	struct drm_gem_object *gem = vma->vm_private_data;
> +	struct drm_i915_gem_object *obj = to_intel_bo(gem);
> +
> +	atomic_dec(&obj->mmap_count);
> +
> +	drm_gem_vm_close(vma);
>   }
>   
>   int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> @@ -2178,12 +2236,19 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>   		return ret;
>   
>   	obj = to_intel_bo(vma->vm_private_data);
> -	if (obj->memory_region) {
> -		if (obj->mmap_origin == I915_MMAP_ORIGIN_OFFSET) {
> -			vma->vm_flags &= ~VM_PFNMAP;
> -			vma->vm_flags |= VM_MIXEDMAP;
> -		}
> +	if (obj->mmap_origin == I915_MMAP_ORIGIN_OFFSET) {
> +		vma->vm_flags &= ~VM_PFNMAP;
> +		vma->vm_flags |= VM_MIXEDMAP;
> +		if (obj->mmap_flags & I915_MMAP_OFFSET_WC)
> +			vma->vm_page_prot =
> +				pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> +		else if (obj->mmap_flags & I915_MMAP_OFFSET_WB)
> +			vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> +		else if (obj->mmap_flags & I915_MMAP_OFFSET_UC)
> +			vma->vm_page_prot =
> +				pgprot_noncached(vm_get_page_prot(vma->vm_flags));
>   	}
> +	atomic_inc(&obj->mmap_count);
>   
>   	return ret;
>   }
> @@ -4228,7 +4293,8 @@ int i915_gem_vmf_fill_pages_cpu(struct drm_i915_gem_object *obj,
>   	vm_fault_t vmf_ret;
>   	pgoff_t pg_off = (vmf->address - area->vm_start) >> PAGE_SHIFT;
>   
> -	if (HAS_MAPPABLE_APERTURE(dev_priv))
> +	if (HAS_MAPPABLE_APERTURE(dev_priv) &&
> +	    obj->mmap_origin == I915_MMAP_ORIGIN_GTT)
>   		return __vmf_fill_pages_gtt(obj, vmf, page_offset);
>   
>   	page = i915_gem_object_get_page(obj, pg_off);
> diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
> index 5c6bbe6f5e84..b37ffe2e17b6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/i915_gem_object.h
> @@ -86,6 +86,12 @@ struct drm_i915_gem_object_ops {
>   			      pgoff_t);
>   };
>   
> +enum i915_cpu_mmap_origin_type {

i915_mmap_type ?

> +	I915_MMAP_ORIGIN_NONE = 0,
> +	I915_MMAP_ORIGIN_GTT,
> +	I915_MMAP_ORIGIN_OFFSET,
> +};
> +
>   struct drm_i915_gem_object {
>   	struct drm_gem_object base;
>   
> @@ -157,6 +163,10 @@ struct drm_i915_gem_object {
>   	unsigned int userfault_count;
>   	struct list_head userfault_link;
>   
> +	enum i915_cpu_mmap_origin_type mmap_origin;
> +	atomic_t mmap_count;
> +	u64 mmap_flags;

Does mmap_flags need to be stored in the object? Is it not only used 
when setting up the mmap?

> +
>   	struct list_head batch_pool_link;
>   	I915_SELFTEST_DECLARE(struct list_head st_link);
>   
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 397810fa2d33..26d2274b5d2b 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -319,6 +319,7 @@ typedef struct _drm_i915_sarea {
>   #define DRM_I915_PERF_ADD_CONFIG	0x37
>   #define DRM_I915_PERF_REMOVE_CONFIG	0x38
>   #define DRM_I915_QUERY			0x39
> +#define DRM_I915_GEM_MMAP_OFFSET   	DRM_I915_GEM_MMAP_GTT
>   
>   #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
>   #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
> @@ -377,6 +378,7 @@ typedef struct _drm_i915_sarea {
>   #define DRM_IOCTL_I915_PERF_ADD_CONFIG	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_ADD_CONFIG, struct drm_i915_perf_oa_config)
>   #define DRM_IOCTL_I915_PERF_REMOVE_CONFIG	DRM_IOW(DRM_COMMAND_BASE + DRM_I915_PERF_REMOVE_CONFIG, __u64)
>   #define DRM_IOCTL_I915_QUERY			DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_QUERY, struct drm_i915_query)
> +#define DRM_IOCTL_I915_GEM_MMAP_OFFSET		DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_MMAP_OFFSET, struct drm_i915_gem_mmap_offset)
>   
>   /* Allow drivers to submit batchbuffers directly to hardware, relying
>    * on the security mechanisms provided by hardware.
> @@ -559,6 +561,9 @@ typedef struct drm_i915_irq_wait {
>    */
>   #define I915_PARAM_MMAP_GTT_COHERENT	52
>   
> +/* Mmap offset ioctl */
> +#define I915_PARAM_MMAP_OFFSET_VERSION	55
> +
>   typedef struct drm_i915_getparam {
>   	__s32 param;
>   	/*
> @@ -731,6 +736,31 @@ 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 _WC/_WB flags
> +	 * should be passed here.
> +	 */
> +	__u64 flags;
> +#define I915_MMAP_OFFSET_WC (1 << 0)
> +#define I915_MMAP_OFFSET_WB (1 << 1)
> +#define I915_MMAP_OFFSET_UC (1 << 2)

Add explicit GTT as well so userspace can use a single ioctl in all cases?

> +#define I915_MMAP_OFFSET_FLAGS \
> +	(I915_MMAP_OFFSET_WC | I915_MMAP_OFFSET_WB | I915_MMAP_OFFSET_UC)
> +};
> +
>   struct drm_i915_gem_set_domain {
>   	/** Handle for the object */
>   	__u32 handle;
> 
Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx