[PATCH v2 4/8] drm/xe/exec: adding msix infra to exec queue

[Niranjana Vishwanathapura]

On Thu, Jun 27, 2024 at 03:40:39PM +0300, Dani Liberman wrote:
>On MSIX platform each exec queue will allocate MSIX.
>
>Signed-off-by: Dani Liberman <dliberman at habana.ai>
>---
> drivers/gpu/drm/xe/xe_exec_queue.c       | 39 ++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_exec_queue_types.h |  2 ++
> drivers/gpu/drm/xe/xe_irq.c              | 11 +++++--
> 3 files changed, 49 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
>index 0ba37835849b..e40c5380e292 100644
>--- a/drivers/gpu/drm/xe/xe_exec_queue.c
>+++ b/drivers/gpu/drm/xe/xe_exec_queue.c
>@@ -22,6 +22,7 @@
> #include "xe_ring_ops_types.h"
> #include "xe_trace.h"
> #include "xe_vm.h"
>+#include "xe_irq.h"
>
> enum xe_exec_queue_sched_prop {
> 	XE_EXEC_QUEUE_JOB_TIMEOUT = 0,
>@@ -33,8 +34,40 @@ 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)
>+{
>+	u32 msix;
>+	int ret = 0;
>+
>+	if (!xe->irq.msix_enabled)
>+		return 0;
>+
>+	ret = xe_request_irq(xe, xe_irq_hwe_handler, q->hwe, q->hwe->name, true, &msix);
>+	if (ret < 0) {
>+		drm_dbg(&xe->drm, "Can't allocate unique MSIX to exec queue (%d)\n", ret);
>+		return ret;
>+	}
>+
>+	q->msix_number = 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->irq.msix_enabled)
>+		return;
>+
>+	if (q->msix_number)
>+		xe_free_irq(xe, q->msix_number);
>+}
>+
> 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);
> 	kfree(q);
>@@ -81,6 +114,12 @@ static struct xe_exec_queue *__xe_exec_queue_alloc(struct xe_device *xe,
> 	else
> 		q->sched_props.priority = XE_EXEC_QUEUE_PRIORITY_NORMAL;
>
>+	err = xe_exec_queue_msix_init(xe, q);
>+	if (err) {
>+		kfree(q);
>+		return ERR_PTR(err);
>+	}
>+
> 	if (vm)
> 		q->vm = xe_vm_get(vm);
>
>diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h b/drivers/gpu/drm/xe/xe_exec_queue_types.h
>index 201588ec33c3..7da157b0e676 100644
>--- a/drivers/gpu/drm/xe/xe_exec_queue_types.h
>+++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h
>@@ -56,6 +56,8 @@ struct xe_exec_queue {
> 	 * @logical_mask: logical mask of where job submitted to exec queue can run
> 	 */
> 	u32 logical_mask;
>+	/** @msix_number: msix number */
>+	u32 msix_number;
> 	/** @name: name of this exec queue */
> 	char name[MAX_FENCE_NAME_LEN];
> 	/** @width: width (number BB submitted per exec) of this exec queue */
>diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
>index 28173e946e77..cf4f523b7fae 100644
>--- a/drivers/gpu/drm/xe/xe_irq.c
>+++ b/drivers/gpu/drm/xe/xe_irq.c
>@@ -895,11 +895,16 @@ void xe_free_irq(struct xe_device *xe, u32 msix)
> 	void *irq_buf;
> 	int irq;
>
>+	/* When removing the driver this function can be called twice for each msix that was
>+	 * allocated for exec queue. First from irq_uninstall() and then from exec queue free.
>+	 * Hence, it is possible that the xarray was already destroyed or the member was removed.
>+	 */

I think the comment standard followed in the driver is to leave a empty line at the top fo
multi-line comment. Perhaps, it is more readable if we follow same standard throughout the driver.

>+	if (xa_empty(&xe->irq.msix_indexes))
>+		return;
>+
> 	irq_buf = xa_load(&xe->irq.msix_indexes, msix);
>-	if (!irq_buf) {
>-		drm_err(&xe->drm, "MSIX %u can't be released since it wasn't allocated\n", msix);
>+	if (!irq_buf)
> 		return;
>-	}

I think we need some lokcing here to avoid such race.
Though xarray apis themselves have locking, I am worried our usage might have races.
If the exec queue freeing removes the allocated msix from xarray, then irq_ininstall()
shouldn't see that. Also, xa_empty() check above might be redundent.
I am also fine with leaving it as is as we have it documented in the comment above.

Niranjana

>
> 	irq = pci_irq_vector(pdev, msix);
> 	if (irq < 0) {
>-- 
>2.34.1
>