[Intel-xe] [PATCH] drm/xe/uapi: Remove MMIO ioctl
Francois Dugast
francois.dugast at intel.com
Wed Sep 13 13:56:10 UTC 2023
On Tue, Sep 12, 2023 at 09:42:41AM -0500, Lucas De Marchi wrote:
> On Tue, Sep 12, 2023 at 03:43:06AM -0500, Ofir Bitton wrote:
> > On 12/09/2023 3:25, Matt Roper wrote:
> > > On Mon, Sep 11, 2023 at 04:21:37AM +0000, Ofir Bitton wrote:
> > > > 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?
> > >
> > > Well, one general roadblock is that the DRM subsystem rules don't allow
> > > adding ABI without a real, open-source userspace consumer. Not only do
> > > we not have a "real" consumer for this ioctl itself, but we also don't
> > > even want to give anyone the impression that we have a backdoor
> > > interface to allow non-opensource userspace to go behind the driver's
> > > back and start controlling the hardware. Userspace can still obviously
> > > do that by running as root and mapping the BAR directly, but they're not
> > > using official driver uapi at that point, and there are potentially
> > > other ways that a sysadmin can close those holes if necessary.
> >
> > Hey Matt, I totally undesrstand your concern, I might have another
> > suggestion. We can create another FD in debugfs and move this ioctl
> > there (I can take ownership on this), This way ABI is not an issue.
>
> an interface in debugfs would be better than keeping the ioctl.
> We seem to have some consensus now to go ahead and remove this.
>
> Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
>
> Please record the Acked-by from UMD when you get them and before
> merging. And we will also need to change IGT ahead of merging this.
Sure, do we rely on intel_reg only or do we also keep xe_reg?
Francois
>
>
> thanks
> Lucas De Marchi
>
>
> >
> > >
> > > > >
> > > > > 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.
> > >
> > > Are you using the term "user" to describe the userspace _software_ or
> > > the person sitting at the computer? If the latter, then we don't need
> > > this ioctl at all; the "intel_reg" tool from IGT has provided this kind
> > > of functionality on i915 for years; it's a simple command-line tool that
> > > can be used with commands like:
> > >
> > > intel_reg read 0x1234
> > > intel_reg write 0x5678 0xabcd
> > >
> > > The tool uses libpciaccess under the hood to map the BAR, and can
> > > perform other necessary pre/post operations (like grabbing releasing
> > > forcewake to make sure the register is accessible).
> >
> > By 'user' I do mean userspace process. Intel_reg tool can work but it
> > has its own limitations.
> >
> > >
> > > If you're using "user" to refer to the userspace _software_ (e.g., IGT's
> > > xe_reg debug tool which is using the ioctl right now), then there's even
> > > more incentive to move away from the ioctl --- the ioctl interface we
> > > have today already has a bunch of limitations that make it less useful
> > > than it should be for debugging:
> > >
> > > * There's no way to specify/access registers on non-tile0.
> >
> > I am aware, I will add support for multi-tile devices.
> >
> > > * There's no way to specify that you do/don't want it to do the "extra"
> > > steps like you describe (e.g., automatically grabbing forcewake is
> > > convenient a lot of the time, but if you're actually trying to debug
> > > forcewake itself, then the ioctl's automatic behavior just gets in
> > > your way).
> > >
> >
> > Understood, for these kind of debugs the mmio ioctl is irrelevant.
> >
> > > While the ioctl could theoretically be extended to overcome these
> > > limitations, we still shouldn't really be building up ABI like that for
> > > debug purposes without real userspace consumers, especially if we have a
> > > viable alternative that's been used for a long time. It's relatively
> > > simple for tools like intel_reg and xe_reg to just use libpciaccess to
> > > map the BAR, figure out the appropriate offset, and then optionally
> > > perform other operations (like forcewake) according to whatever options
> > > were passed on the command line. With debug tools it's especially
> > > likely that we'll come up with new behaviors that we want to add (e.g.,
> > > handle display register accesses via msgbus transactions on MTL/LNL?)
> > > and it would be better if we didn't need to keep extending the ioctl
> > > uapi and ABI for these debug-only purposes).
> >
> > I agree we need an interface that will not be obligated to any ABI as
> > this is for pure debug, so we are left with 2 options:
> > 1. My suggestion to create a debugfs FD and use the ioctl there.
> > 2. Move code to innersource
> >
> > I prefer option #1 as the latter will not be available for customers who
> > uses the opensource driver.
> >
> > Ofir.
> >
> > >
> > >
> > > Matt
> > >
> >
More information about the Intel-xe
mailing list