Re: [Intel-xe] [PATCH] drm/xe/uapi: Remove MMIO ioctl

["Souza, Jose" <jose.souza@intel.com>]

On Thu, 2023-09-07 at 19:35 +0000, Francois Dugast wrote:
> This was previously used in UMD for timestamp correlation, which can now
> be done with DRM_XE_QUERY_CS_CYCLES.

Please just make sure it lands together of after https://patchwork.freedesktop.org/series/122054/

`Reviewed-by: José Roberto de Souza <jose.souza@intel.com>`

> 
> Link: https://lore.kernel.org/all/20230706042044.GR6953@mdroper-desk1.amr.corp.intel.com/
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/636
> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c |   1 -
>  drivers/gpu/drm/xe/xe_mmio.c   | 102 ---------------------------------
>  include/uapi/drm/xe_drm.h      |  31 ++--------
>  3 files changed, 4 insertions(+), 130 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 109aeb25d19c..10fa1b55578a 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -107,7 +107,6 @@ static const struct drm_ioctl_desc xe_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(XE_EXEC_QUEUE_DESTROY, xe_exec_queue_destroy_ioctl,
>  			  DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(XE_EXEC, xe_exec_ioctl, DRM_RENDER_ALLOW),
> -	DRM_IOCTL_DEF_DRV(XE_MMIO, xe_mmio_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(XE_EXEC_QUEUE_SET_PROPERTY, xe_exec_queue_set_property_ioctl,
>  			  DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(XE_WAIT_USER_FENCE, xe_wait_user_fence_ioctl,
> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> index 3ccc0af4430b..e636e3f3456d 100644
> --- a/drivers/gpu/drm/xe/xe_mmio.c
> +++ b/drivers/gpu/drm/xe/xe_mmio.c
> @@ -422,108 +422,6 @@ int xe_mmio_init(struct xe_device *xe)
>  	return 0;
>  }
>  
> -#define VALID_MMIO_FLAGS (\
> -	DRM_XE_MMIO_BITS_MASK |\
> -	DRM_XE_MMIO_READ |\
> -	DRM_XE_MMIO_WRITE)
> -
> -static const struct xe_reg mmio_read_whitelist[] = {
> -	RING_TIMESTAMP(RENDER_RING_BASE),
> -};
> -
> -int xe_mmio_ioctl(struct drm_device *dev, void *data,
> -		  struct drm_file *file)
> -{
> -	struct xe_device *xe = to_xe_device(dev);
> -	struct xe_gt *gt = xe_root_mmio_gt(xe);
> -	struct drm_xe_mmio *args = data;
> -	unsigned int bits_flag, bytes;
> -	struct xe_reg reg;
> -	bool allowed;
> -	int ret = 0;
> -
> -	if (XE_IOCTL_DBG(xe, args->extensions) ||
> -	    XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
> -		return -EINVAL;
> -
> -	if (XE_IOCTL_DBG(xe, args->flags & ~VALID_MMIO_FLAGS))
> -		return -EINVAL;
> -
> -	if (XE_IOCTL_DBG(xe, !(args->flags & DRM_XE_MMIO_WRITE) && args->value))
> -		return -EINVAL;
> -
> -	allowed = capable(CAP_SYS_ADMIN);
> -	if (!allowed && ((args->flags & ~DRM_XE_MMIO_BITS_MASK) == DRM_XE_MMIO_READ)) {
> -		unsigned int i;
> -
> -		for (i = 0; i < ARRAY_SIZE(mmio_read_whitelist); i++) {
> -			if (mmio_read_whitelist[i].addr == args->addr) {
> -				allowed = true;
> -				break;
> -			}
> -		}
> -	}
> -
> -	if (XE_IOCTL_DBG(xe, !allowed))
> -		return -EPERM;
> -
> -	bits_flag = args->flags & DRM_XE_MMIO_BITS_MASK;
> -	bytes = 1 << bits_flag;
> -	if (XE_IOCTL_DBG(xe, args->addr + bytes > xe->mmio.size))
> -		return -EINVAL;
> -
> -	/*
> -	 * TODO: migrate to xe_gt_mcr to lookup the mmio range and handle
> -	 * multicast registers. Steering would need uapi extension.
> -	 */
> -	reg = XE_REG(args->addr);
> -
> -	xe_device_mem_access_get(xe);
> -	xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> -
> -	if (args->flags & DRM_XE_MMIO_WRITE) {
> -		switch (bits_flag) {
> -		case DRM_XE_MMIO_32BIT:
> -			if (XE_IOCTL_DBG(xe, args->value > U32_MAX)) {
> -				ret = -EINVAL;
> -				goto exit;
> -			}
> -			xe_mmio_write32(gt, reg, args->value);
> -			break;
> -		default:
> -			drm_dbg(&xe->drm, "Invalid MMIO bit size");
> -			fallthrough;
> -		case DRM_XE_MMIO_8BIT: /* TODO */
> -		case DRM_XE_MMIO_16BIT: /* TODO */
> -			ret = -EOPNOTSUPP;
> -			goto exit;
> -		}
> -	}
> -
> -	if (args->flags & DRM_XE_MMIO_READ) {
> -		switch (bits_flag) {
> -		case DRM_XE_MMIO_32BIT:
> -			args->value = xe_mmio_read32(gt, reg);
> -			break;
> -		case DRM_XE_MMIO_64BIT:
> -			args->value = xe_mmio_read64_2x32(gt, reg);
> -			break;
> -		default:
> -			drm_dbg(&xe->drm, "Invalid MMIO bit size");
> -			fallthrough;
> -		case DRM_XE_MMIO_8BIT: /* TODO */
> -		case DRM_XE_MMIO_16BIT: /* TODO */
> -			ret = -EOPNOTSUPP;
> -		}
> -	}
> -
> -exit:
> -	xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> -	xe_device_mem_access_put(xe);
> -
> -	return ret;
> -}
> -
>  /**
>   * xe_mmio_read64_2x32() - Read a 64-bit register as two 32-bit reads
>   * @gt: MMIO target GT
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 86f16d50e9cc..6c6d1cfa415a 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -106,11 +106,10 @@ struct xe_user_extension {
>  #define DRM_XE_EXEC_QUEUE_CREATE		0x06
>  #define DRM_XE_EXEC_QUEUE_DESTROY		0x07
>  #define DRM_XE_EXEC			0x08
> -#define DRM_XE_MMIO			0x09
> -#define DRM_XE_EXEC_QUEUE_SET_PROPERTY	0x0a
> -#define DRM_XE_WAIT_USER_FENCE		0x0b
> -#define DRM_XE_VM_MADVISE		0x0c
> -#define DRM_XE_EXEC_QUEUE_GET_PROPERTY	0x0d
> +#define DRM_XE_EXEC_QUEUE_SET_PROPERTY	0x09
> +#define DRM_XE_WAIT_USER_FENCE		0x0a
> +#define DRM_XE_VM_MADVISE		0x0b
> +#define DRM_XE_EXEC_QUEUE_GET_PROPERTY	0x0c
>  
>  /* Must be kept compact -- no holes */
>  #define DRM_IOCTL_XE_DEVICE_QUERY		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_DEVICE_QUERY, struct drm_xe_device_query)
> @@ -123,7 +122,6 @@ struct xe_user_extension {
>  #define DRM_IOCTL_XE_EXEC_QUEUE_GET_PROPERTY	DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_EXEC_QUEUE_GET_PROPERTY, struct drm_xe_exec_queue_get_property)
>  #define DRM_IOCTL_XE_EXEC_QUEUE_DESTROY		 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_EXEC_QUEUE_DESTROY, struct drm_xe_exec_queue_destroy)
>  #define DRM_IOCTL_XE_EXEC			 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_EXEC, struct drm_xe_exec)
> -#define DRM_IOCTL_XE_MMIO			DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_MMIO, struct drm_xe_mmio)
>  #define DRM_IOCTL_XE_EXEC_QUEUE_SET_PROPERTY	 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_EXEC_QUEUE_SET_PROPERTY, struct drm_xe_exec_queue_set_property)
>  #define DRM_IOCTL_XE_WAIT_USER_FENCE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE, struct drm_xe_wait_user_fence)
>  #define DRM_IOCTL_XE_VM_MADVISE			 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_MADVISE, struct drm_xe_vm_madvise)
> @@ -883,27 +881,6 @@ struct drm_xe_exec {
>  	__u64 reserved[2];
>  };
>  
> -struct drm_xe_mmio {
> -	/** @extensions: Pointer to the first extension struct, if any */
> -	__u64 extensions;
> -
> -	__u32 addr;
> -
> -#define DRM_XE_MMIO_8BIT	0x0
> -#define DRM_XE_MMIO_16BIT	0x1
> -#define DRM_XE_MMIO_32BIT	0x2
> -#define DRM_XE_MMIO_64BIT	0x3
> -#define DRM_XE_MMIO_BITS_MASK	0x3
> -#define DRM_XE_MMIO_READ	0x4
> -#define DRM_XE_MMIO_WRITE	0x8
> -	__u32 flags;
> -
> -	__u64 value;
> -
> -	/** @reserved: Reserved */
> -	__u64 reserved[2];
> -};
> -
>  /**
>   * struct drm_xe_wait_user_fence - wait user fence
>   *