[PATCH v9] drm/xe: add mmio debugfs file & restore xe_mmio_ioctl as its ioctl handler
Lucas De Marchi
lucas.demarchi at intel.com
Wed May 15 20:04:16 UTC 2024
On Wed, May 15, 2024 at 07:52:44PM GMT, Koby Elbaz wrote:
>The drm mmio ioctl handler (xe_mmio_ioctl) wasn't needed anymore,
>and therefore, recently removed.
>Nevertheless, it is now restored for debugging purposes as an
>ioctl handler for a newly added 'mmio' debugfs file.
>
>Notes:
>1. The non-admin user's limited mmio access (aka, whitelist),
>was removed being irrelevant anymore, now that a user using debugfs
>must anyway have root permissions to use it.
>2. To support multi-tile access, the tile index is now dynamic and
>will be retrieved from the given mmio address itself.
>
>Cc: Daniel Vetter <daniel at ffwll.ch>
>Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>Cc: Ofir Bitton <obitton at habana.ai>
>Cc: José Roberto de Souza <jose.souza at intel.com>
>Signed-off-by: Koby Elbaz <kelbaz at habana.ai>
>---
>Changes in v9:
> - Fix checkpatch warnings
>
> drivers/gpu/drm/xe/xe_debugfs.c | 169 ++++++++++++++++++++++++++-
> drivers/gpu/drm/xe/xe_debugfs_mmio.h | 43 +++++++
> 2 files changed, 211 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/xe/xe_debugfs_mmio.h
>
>diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
>index 1011e5d281fa..ff99f5466931 100644
>--- a/drivers/gpu/drm/xe/xe_debugfs.c
>+++ b/drivers/gpu/drm/xe/xe_debugfs.c
>@@ -7,7 +7,7 @@
>
> #include <linux/debugfs.h>
> #include <linux/string_helpers.h>
>-
>+#include <linux/security.h>
> #include <drm/drm_debugfs.h>
>
> #include "xe_bo.h"
>@@ -19,6 +19,8 @@
> #include "xe_pm.h"
> #include "xe_sriov.h"
> #include "xe_step.h"
>+#include "xe_debugfs_mmio.h"
>+#include "xe_mmio.h"
>
> #ifdef CONFIG_DRM_XE_DEBUG
> #include "xe_bo_evict.h"
>@@ -115,6 +117,164 @@ static int forcewake_release(struct inode *inode, struct file *file)
> return 0;
> }
>
>+static int get_tile_id_of_mmio_addr(struct xe_device *xe, u32 mmio_addr, u8 *tile_id)
>+{
>+ /* MMIO space size is identical for all tiles so arbitrarily choose root tile */
>+ struct xe_tile *tile = xe_device_get_root_tile(xe);
>+ u32 tile_mmio_base_addr = 0;
>+ u8 id;
>+
>+ if (mmio_addr >= (tile->mmio.size * xe->info.tile_count))
>+ return -EINVAL;
>+
>+ for_each_tile(tile, xe, id) {
>+ tile_mmio_base_addr = id * tile->mmio.size;
>+ /* Each tile has a 16M MMIO space, but the contained CFG space is only 4M */
>+ if (mmio_addr >= tile_mmio_base_addr && mmio_addr < (tile_mmio_base_addr + SZ_4M)) {
>+ *tile_id = id;
>+ return 0;
>+ }
>+ }
>+
>+ return -EINVAL;
>+}
>+
>+static int assign_mmio_args_to_xe_reg(struct xe_device *xe, struct xe_debugfs_mmio_ioctl_data *args,
>+ struct xe_reg *reg, u8 *tile_id)
>+{
>+ unsigned int mmio_bits_flag, bytes;
>+ u32 mmio_offset_in_tile;
>+ struct xe_gt *gt;
>+ u8 local_tile_id;
>+ int ret;
>+
>+ mmio_bits_flag = args->flags & XE_DEBUGFS_MMIO_BITS_MASK;
>+ bytes = 1 << mmio_bits_flag;
looks like you are using the value of the flag to calculate the number
of bytes, is that right?
>+
>+ ret = get_tile_id_of_mmio_addr(xe, args->addr, &local_tile_id);
>+ if (ret)
>+ return ret;
>+
>+ gt = xe_device_get_gt(xe, local_tile_id);
is there some confusion going on here between tile and gt?
xe_device_get_gt() is supposed to get the gt id, not the tile id. this
would fail for e.g. when trying to read something from media gt in MTL.
>+ mmio_offset_in_tile = args->addr - local_tile_id * SZ_16M;
>+
>+ /*
>+ * TODO: migrate to xe_gt_mcr to lookup the mmio range and handle
>+ * multicast registers. Steering would need uapi extension.
>+ */
>+ *reg = XE_REG(mmio_offset_in_tile);
This basically kills any usefulness of this entire patch. It's already a
stretch to require usable driver to read a register, which can already
be done by mapping the BAR like done by intel_reg.
The only meaninful thing here is that it uses the driver's steering
knowledge to steer register accesses, whic is not supported here.
>+
>+ if (XE_IOCTL_DBG(xe, reg->addr + bytes > gt_to_tile(gt)->mmio.size))
>+ return -EINVAL;
>+
>+ *tile_id = local_tile_id;
>+
>+ return 0;
>+}
>+
>+static int _xe_debugfs_mmio_ioctl(struct xe_device *xe, struct xe_debugfs_mmio_ioctl_data *args)
>+{
>+ struct xe_reg reg;
>+ struct xe_gt *gt;
>+ u8 tile_id, id;
>+ int ret;
>+
>+ 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 & ~XE_DEBUGFS_MMIO_VALID_FLAGS))
>+ return -EINVAL;
>+
>+ if (XE_IOCTL_DBG(xe, !(args->flags & XE_DEBUGFS_MMIO_WRITE) && args->value))
>+ return -EINVAL;
>+
>+ xe_pm_runtime_get_noresume(xe);
>+ for_each_gt(gt, xe, id)
>+ XE_WARN_ON(xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL));
we are trying to get rid of the XE_WARN_ON. Please use the gt variant.
>+
>+ ret = assign_mmio_args_to_xe_reg(xe, args, ®, &tile_id);
>+ if (ret)
>+ goto exit;
>+
>+ gt = xe_device_get_gt(xe, tile_id);
>+
>+ if (args->flags & XE_DEBUGFS_MMIO_WRITE) {
>+ switch (args->flags & XE_DEBUGFS_MMIO_BITS_MASK) {
>+ case XE_DEBUGFS_MMIO_32BIT:
>+ if (XE_IOCTL_DBG(xe, args->value > U32_MAX)) {
>+ ret = -EINVAL;
>+ goto exit;
>+ }
>+ xe_mmio_write32(gt, reg, args->value);
I'm not sure how this is going to work. xe_mmio_write32() adds the GTI
offset, but it looks like the desire here would be to pass with the GTI
offset already encoded (since assign_mmio_args_to_xe_reg() tries to
convert the raw address back to tile / gt).
>+ break;
>+ default:
>+ drm_dbg(&xe->drm, "Invalid MMIO bit size");
>+ fallthrough;
>+ case XE_DEBUGFS_MMIO_8BIT: /* TODO */
>+ case XE_DEBUGFS_MMIO_16BIT: /* TODO */
it's not suppored for neither read or write so it would be better just
to not support it.
>+ ret = -EOPNOTSUPP;
>+ goto exit;
>+ }
>+ }
>+
>+ if (args->flags & XE_DEBUGFS_MMIO_READ) {
>+ switch (args->flags & XE_DEBUGFS_MMIO_BITS_MASK) {
>+ case XE_DEBUGFS_MMIO_32BIT:
>+ args->value = xe_mmio_read32(gt, reg);
>+ break;
>+ case XE_DEBUGFS_MMIO_64BIT:
>+ args->value = xe_mmio_read64_2x32(gt, reg);
>+ break;
>+ default:
>+ drm_dbg(&xe->drm, "Invalid MMIO bit size");
>+ fallthrough;
>+ case XE_DEBUGFS_MMIO_8BIT: /* TODO */
>+ case XE_DEBUGFS_MMIO_16BIT: /* TODO */
>+ ret = -EOPNOTSUPP;
>+ }
>+ }
>+
>+exit:
>+ for_each_gt(gt, xe, id)
>+ XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL));
>+
>+ xe_pm_runtime_put(xe);
>+
>+ return ret;
>+}
>+
>+static long xe_debugfs_mmio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>+{
>+ struct xe_device *xe = file_inode(filp)->i_private;
>+ struct xe_debugfs_mmio_ioctl_data mmio_data;
>+ int ret;
>+
>+ ret = security_locked_down(LOCKDOWN_PCI_ACCESS);
>+ if (ret)
>+ return ret;
>+
>+ if (cmd == XE_DEBUGFS_MMIO_IOCTL) {
>+ ret = copy_from_user(&mmio_data, (struct xe_debugfs_mmio_ioctl_data __user *)arg,
>+ sizeof(mmio_data));
>+ if (ret)
>+ return -EFAULT;
>+
>+ ret = _xe_debugfs_mmio_ioctl(xe, &mmio_data);
>+ if (ret)
>+ return ret;
>+
>+ ret = copy_to_user((struct xe_debugfs_mmio_ioctl_data __user *)arg,
>+ &mmio_data, sizeof(mmio_data));
>+ if (ret)
>+ return -EFAULT;
>+ } else {
>+ return -EINVAL;
>+ }
>+
>+ return 0;
>+}
>+
> static const struct file_operations forcewake_all_fops = {
> .owner = THIS_MODULE,
> .open = forcewake_open,
>@@ -173,6 +333,11 @@ static const struct file_operations wedged_mode_fops = {
> .write = wedged_mode_set,
> };
>
>+static const struct file_operations mmio_fops = {
>+ .owner = THIS_MODULE,
>+ .unlocked_ioctl = xe_debugfs_mmio_ioctl,
>+};
>+
> void xe_debugfs_register(struct xe_device *xe)
> {
> struct ttm_device *bdev = &xe->ttm;
>@@ -214,6 +379,8 @@ void xe_debugfs_register(struct xe_device *xe)
> for_each_gt(gt, xe, id)
> xe_gt_debugfs_register(gt);
>
>+ debugfs_create_file("mmio", 0644, root, xe, &mmio_fops);
>+
> #ifdef CONFIG_FAULT_INJECTION
> fault_create_debugfs_attr("fail_gt_reset", root, >_reset_failure);
> #endif
>diff --git a/drivers/gpu/drm/xe/xe_debugfs_mmio.h b/drivers/gpu/drm/xe/xe_debugfs_mmio.h
>new file mode 100644
>index 000000000000..1dfb8fa2ce9a
>--- /dev/null
>+++ b/drivers/gpu/drm/xe/xe_debugfs_mmio.h
How is userspace going to know about the values?
Even if it's debugfs, we need some way to communicate the values the
kernel expects. Not sure if it fits include/uapi/ though. Are there
other use cases in the kernel to follow along?
>@@ -0,0 +1,43 @@
>+/* SPDX-License-Identifier: MIT */
>+/*
>+ * Copyright © 2023 Intel Corporation
>+ */
>+
>+#ifndef _XE_DEBUGFS_MMIO_H_
>+#define _XE_DEBUGFS_MMIO_H_
>+
>+#include <drm/drm.h>
what is this for? Apparently you want asm/ioctl.h rather than this.
>+
>+#define XE_DEBUGFS_MMIO_8BIT 0x0
>+#define XE_DEBUGFS_MMIO_16BIT 0x1
as said above, just don't expose things you don't support.
>+#define XE_DEBUGFS_MMIO_32BIT 0x2
>+#define XE_DEBUGFS_MMIO_64BIT 0x3
>+#define XE_DEBUGFS_MMIO_BITS_MASK 0x3
>+#define XE_DEBUGFS_MMIO_READ 0x4
>+#define XE_DEBUGFS_MMIO_WRITE 0x8
>+
>+#define XE_DEBUGFS_MMIO_VALID_FLAGS (\
^ this can't be in the header if it goes to uapi
>+ XE_DEBUGFS_MMIO_BITS_MASK |\
>+ XE_DEBUGFS_MMIO_READ |\
>+ XE_DEBUGFS_MMIO_WRITE)
>+
>+struct xe_debugfs_mmio_ioctl_data {
>+ /** @extensions: Pointer to the first extension struct, if any */
>+ __u64 extensions;
>+
>+ __u32 addr;
>+
>+ __u32 flags;
>+
>+ __u64 value;
>+
>+ /** @reserved: Reserved */
>+ __u64 reserved[2];
>+};
>+
>+#define XE_DEBUGFS_MMIO_IOCTL_NR 0x00
>+
>+#define XE_DEBUGFS_MMIO_IOCTL \
>+ _IOWR(0x20, XE_DEBUGFS_MMIO_IOCTL_NR, struct xe_debugfs_mmio_ioctl_data)
>+
>+#endif /* _XE_DEBUGFS_MMIO_H_ */
^ no need for this as it often gets out of sync with the
guard above
I'm not very confident this works as is. I think one possibility is to
implement the MCR missing above and implement this backend in intel_reg.
Then you can prove it works and show it's worth it.
Currently intel_reg has a few backends like mapping the BAR, or
submitting a batch and reading/writing from the GPU side. Adding another
one to be "via driver's debugfs" would probably be ok. +Jani as
intel_reg's original author.
Lucas De Marchi
>--
>2.34.1
>
More information about the Intel-xe
mailing list