[Intel-xe] [PATCH] drm/xe/uapi: Remove MMIO ioctl
Ofir Bitton
obitton at habana.ai
Mon Sep 11 04:21:37 UTC 2023
On 11/09/2023 6:45, Lucas De Marchi wrote:
> 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
Some register read/write requires driver involvement, for example
disabling clock gating or going through a register gateway.
In addition, if a user would like to access directly through the bar he
will need to be aware of the exact bar mapping. It is preferable that
the user will be able to ask for read/write from/to a specific MMIO
address and the driver will map it to the proper offset in bar.
More information about the Intel-xe
mailing list