[Intel-xe] [PATCH] drm/xe/uapi: Remove MMIO ioctl
Lucas De Marchi
lucas.demarchi at intel.com
Mon Sep 11 03:45:54 UTC 2023
On Sun, Sep 10, 2023 at 04:34:24PM +0000, Ofir Bitton wrote:
>On 07/09/2023 22:35, Francois Dugast wrote:
>> This was previously used in UMD for timestamp correlation, which can now
>> be done with DRM_XE_QUERY_CS_CYCLES.
>>
>> 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 at 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
>> *
>
>I would prefer to keep this ioctl for debug, or an equivavlent
>alternative. We can remove the timestmap part but keep the general mmio
>ioctl interface for 'CAP_SYS_ADMIN' access only.
>Any objections?
CAP_SYS_ADMIN can already map the bar and do the reads and
writes. What's the benefit of going through the kernel?
Lucas De Marchi
>
>Ofir
More information about the Intel-xe
mailing list