[PATCH v5 5/5] drm/xe/uapi: Support requesting unique MSI-X for exec queue
Piotr Piórkowski
piotr.piorkowski at intel.com
Thu Dec 5 09:00:13 UTC 2024
Ilia Levi <ilia.levi at intel.com> wrote on czw [2024-lis-28 14:53:45 +0200]:
> From: Dani Liberman <dliberman at habana.ai>
>
> Unique MSI-X per exec queue will improve the performance of the
> IRQ handler. In case no MSI-X is available, the uAPI will return
> -EBUSY error and the user would be able to execute the uAPI again
> without the flag (fallback to default MSI-X).
>
> Co-developed-by: Ilia Levi <ilia.levi at intel.com>
> Signed-off-by: Ilia Levi <ilia.levi at intel.com>
> Signed-off-by: Dani Liberman <dliberman at habana.ai>
> ---
> drivers/gpu/drm/xe/xe_exec_queue.c | 57 +++++++++++++++++++++---
> drivers/gpu/drm/xe/xe_exec_queue_types.h | 2 +
> drivers/gpu/drm/xe/xe_irq.c | 18 ++++++++
> drivers/gpu/drm/xe/xe_irq.h | 1 +
> include/uapi/drm/xe_drm.h | 8 +++-
> 5 files changed, 80 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> index 9c94be571900..1478fdfbf3f7 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> @@ -12,6 +12,7 @@
> #include <uapi/drm/xe_drm.h>
>
> #include "xe_device.h"
> +#include "xe_drv.h"
> #include "xe_gt.h"
> #include "xe_hw_engine_class_sysfs.h"
> #include "xe_hw_engine_group.h"
> @@ -35,8 +36,46 @@ enum xe_exec_queue_sched_prop {
> static int exec_queue_user_extensions(struct xe_device *xe, struct xe_exec_queue *q,
> u64 extensions, int ext_number);
>
> +static int xe_exec_queue_msix_init(struct xe_device *xe, struct xe_exec_queue *q, bool unique_msix)
> +{
> + u16 msix;
> + int ret = 0;
NIT: It seems to me that in case you are expecting success or error in case of a function
it is better to use err instead of ret
> +
> + if (!xe_device_has_msix(xe))
> + return 0;
> +
> + if (!unique_msix) {
> + q->msix_vec = XE_IRQ_DEFAULT_MSIX;
> + return 0;
> + }
> +
> + ret = xe_irq_msix_request_irq(xe, xe_irq_msix_hwe_handler, q,
> + DRIVER_NAME "-exec-queue", true, &msix);
> + if (ret < 0) {
> + drm_dbg(&xe->drm, "Can't allocate unique MSI-X to exec queue (%d)\n", ret);
> + return ret;
> + }
> +
> + q->msix_vec = msix;
> +
> + return 0;
> +}
> +
> +static void xe_exec_queue_msix_fini(struct xe_exec_queue *q)
> +{
> + struct xe_device *xe = gt_to_xe(q->gt);
> +
> + if (!xe_device_has_msix(xe))
> + return;
> +
> + if (q->msix_vec && q->msix_vec != XE_IRQ_DEFAULT_MSIX)
> + xe_irq_msix_free_irq(xe, q->msix_vec);
> +}
> +
> static void __xe_exec_queue_free(struct xe_exec_queue *q)
> {
> + xe_exec_queue_msix_fini(q);
> +
> if (q->vm)
> xe_vm_put(q->vm);
>
> @@ -69,7 +108,12 @@ static struct xe_exec_queue *__xe_exec_queue_alloc(struct xe_device *xe,
> q->gt = gt;
> q->class = hwe->class;
> q->width = width;
> - q->msix_vec = XE_IRQ_DEFAULT_MSIX;
> + err = xe_exec_queue_msix_init(xe, q, flags & EXEC_QUEUE_FLAG_UNIQUE_MSIX);
NIT: You could use !! to highlight that you are casting an u32 to bool
err = xe_exec_queue_msix_init(xe, q, !!(flags & EXEC_QUEUE_FLAG_UNIQUE_MSIX));
> + if (err) {
> + kfree(q);
> + return ERR_PTR(err);
> + }
> +
> q->logical_mask = logical_mask;
> q->fence_irq = >->fence_irq[hwe->class];
> q->ring_ops = gt->ring_ops[hwe->class];
> @@ -547,13 +591,13 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
> struct xe_gt *gt;
> struct xe_tile *tile;
> struct xe_exec_queue *q = NULL;
> + u32 flags = 0;
> u32 logical_mask;
> u32 id;
> u32 len;
> int err;
>
> - if (XE_IOCTL_DBG(xe, args->flags) ||
> - XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
> + if (XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
> return -EINVAL;
>
> len = args->width * args->num_placements;
> @@ -569,6 +613,9 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
> if (XE_IOCTL_DBG(xe, eci[0].gt_id >= xe->info.gt_count))
> return -EINVAL;
>
> + if (args->flags & DRM_XE_EXEC_QUEUE_CREATE_FLAG_UNIQUE_INTERRUPT_HINT)
> + flags |= EXEC_QUEUE_FLAG_UNIQUE_MSIX;
> +
> if (eci[0].engine_class == DRM_XE_ENGINE_CLASS_VM_BIND) {
> if (XE_IOCTL_DBG(xe, args->width != 1) ||
> XE_IOCTL_DBG(xe, args->num_placements != 1) ||
> @@ -577,8 +624,8 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
>
> for_each_tile(tile, xe, id) {
> struct xe_exec_queue *new;
> - u32 flags = EXEC_QUEUE_FLAG_VM;
>
> + flags |= EXEC_QUEUE_FLAG_VM;
> if (id)
> flags |= EXEC_QUEUE_FLAG_BIND_ENGINE_CHILD;
>
> @@ -625,7 +672,7 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
> }
>
> q = xe_exec_queue_create(xe, vm, logical_mask,
> - args->width, hwe, 0,
> + args->width, hwe, flags,
> args->extensions);
> up_read(&vm->lock);
> xe_vm_put(vm);
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> index eec8f9935a58..6e419b572000 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue_types.h
> +++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> @@ -85,6 +85,8 @@ struct xe_exec_queue {
> #define EXEC_QUEUE_FLAG_BIND_ENGINE_CHILD BIT(3)
> /* kernel exec_queue only, set priority to highest level */
> #define EXEC_QUEUE_FLAG_HIGH_PRIORITY BIT(4)
> +/* queue with unique msix interrupt */
> +#define EXEC_QUEUE_FLAG_UNIQUE_MSIX BIT(5)
>
> /**
> * @flags: flags for this exec queue, should statically setup aside from ban
> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
> index 32f5a67a917b..52829771c89b 100644
> --- a/drivers/gpu/drm/xe/xe_irq.c
> +++ b/drivers/gpu/drm/xe/xe_irq.c
> @@ -14,6 +14,7 @@
> #include "regs/xe_irq_regs.h"
> #include "xe_device.h"
> #include "xe_drv.h"
> +#include "xe_exec_queue_types.h"
> #include "xe_gsc_proxy.h"
> #include "xe_gt.h"
> #include "xe_guc.h"
> @@ -880,6 +881,23 @@ static irqreturn_t xe_irq_msix_default_hwe_handler(int irq, void *arg)
> return IRQ_HANDLED;
> }
>
> +/*
> + * In MSI-X mode command streamers raise an interrupt only as a result of
> + * MI_USER_INTERRUPT and MI_FLUSH_DW_NOTIFY commands.
> + */
> +irqreturn_t xe_irq_msix_hwe_handler(int irq, void *arg)
> +{
> + struct xe_exec_queue *q = arg;
> + struct xe_tile *tile = gt_to_tile(q->hwe->gt);
> +
> + if (!atomic_read(&tile->xe->irq.enabled))
> + return IRQ_NONE;
> +
> + xe_memirq_hwe_handler(&tile->memirq, q->hwe);
> +
> + return IRQ_HANDLED;
> +}
> +
> static int xe_irq_msix_alloc_vector(struct xe_device *xe, void *irq_buf,
> bool dynamic_msix, u16 *msix)
> {
> diff --git a/drivers/gpu/drm/xe/xe_irq.h b/drivers/gpu/drm/xe/xe_irq.h
> index a28bd577ba52..47058e2d9d36 100644
> --- a/drivers/gpu/drm/xe/xe_irq.h
> +++ b/drivers/gpu/drm/xe/xe_irq.h
> @@ -19,6 +19,7 @@ int xe_irq_install(struct xe_device *xe);
> void xe_irq_suspend(struct xe_device *xe);
> void xe_irq_resume(struct xe_device *xe);
> void xe_irq_enable_hwe(struct xe_gt *gt);
> +irqreturn_t xe_irq_msix_hwe_handler(int irq, void *arg);
> int xe_irq_msix_request_irq(struct xe_device *xe, irq_handler_t handler, void *irq_buf,
> const char *name, bool dynamic_msix, u16 *msix);
> void xe_irq_msix_free_irq(struct xe_device *xe, u16 msix);
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 4a8a4a63e99c..5f32553b14fb 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -1123,7 +1123,13 @@ struct drm_xe_exec_queue_create {
> /** @vm_id: VM to use for this exec queue */
> __u32 vm_id;
>
> - /** @flags: MBZ */
> + /*
> + * When creating exec queue in MSIX platforms, the user can request a unique MSIX interrupt
> + * for the irq handler. If there is no available MSIX, -EBUSY will be returned.
> + */
> +#define DRM_XE_EXEC_QUEUE_CREATE_FLAG_UNIQUE_INTERRUPT_HINT (0x1 << 0)
> +
> + /** @flags: create queue flags */
> __u32 flags;
>
> /** @exec_queue_id: Returned exec queue ID */
> --
> 2.43.2
Two minor comments above, but LGTM:
Reviewed-by: Piotr Piórkowski <piotr.piorkowski at intel.com>
>
--
More information about the Intel-xe
mailing list