drm/xe/pxp: Decouple queue addition from PXP start

Starting PXP and adding a queue to the PXP queue list are separate
actions. Given that a queue can only be added to the list if PXP is
active, the 2 actions were bundled together to avoid having to
re-lock and re-check the status to perform the queue addition after
having done so during the PXP start. However, we don't save a lot of
complexity by doing so and we lose in clarity of code, so overall it's
cleaner to just keep the 2 actions separate.

v2: remove leftover rpm_get (John), fix rpm_put in error case

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
Link: https://lore.kernel.org/r/20250522225401.3953243-8-daniele.ceraolospurio@intel.com

diff --git a/drivers/gpu/drm/xe/xe_pxp.c b/drivers/gpu/drm/xe/xe_pxp.c
index b5bc15f436fa2da64f101b20aaf8595b49ba2911..3d62008c99f15ab6d248f5370fa77a292864eb67 100644 (file)
--- a/drivers/gpu/drm/xe/xe_pxp.c
+++ b/drivers/gpu/drm/xe/xe_pxp.c
@@ -504,69 +504,62 @@ int xe_pxp_exec_queue_set_type(struct xe_pxp *pxp, struct xe_exec_queue *q, u8 t
        return 0;
 }
 
-static void __exec_queue_add(struct xe_pxp *pxp, struct xe_exec_queue *q)
+static int __exec_queue_add(struct xe_pxp *pxp, struct xe_exec_queue *q)
 {
-       spin_lock_irq(&pxp->queues.lock);
-       list_add_tail(&q->pxp.link, &pxp->queues.list);
-       spin_unlock_irq(&pxp->queues.lock);
+       int ret = 0;
+
+       /*
+        * A queue can be added to the list only if the PXP is in active status,
+        * otherwise the termination might not handle it correctly.
+        */
+       mutex_lock(&pxp->mutex);
+
+       if (pxp->status == XE_PXP_ACTIVE) {
+               spin_lock_irq(&pxp->queues.lock);
+               list_add_tail(&q->pxp.link, &pxp->queues.list);
+               spin_unlock_irq(&pxp->queues.lock);
+       } else if (pxp->status == XE_PXP_ERROR || pxp->status == XE_PXP_SUSPENDED) {
+               ret = -EIO;
+       } else {
+               ret = -EBUSY; /* try again later */
+       }
+
+       mutex_unlock(&pxp->mutex);
+
+       return ret;
 }
 
-/**
- * xe_pxp_exec_queue_add - add a queue to the PXP list
- * @pxp: the xe->pxp pointer (it will be NULL if PXP is disabled)
- * @q: the queue to add to the list
- *
- * If PXP is enabled and the prerequisites are done, start the PXP ARB
- * session (if not already running) and add the queue to the PXP list. Note
- * that the queue must have previously been marked as using PXP with
- * xe_pxp_exec_queue_set_type.
- *
- * Returns 0 if the PXP ARB session is running and the queue is in the list,
- * -ENODEV if PXP is disabled, -EBUSY if the PXP prerequisites are not done,
- * other errno value if something goes wrong during the session start.
- */
-int xe_pxp_exec_queue_add(struct xe_pxp *pxp, struct xe_exec_queue *q)
+static int pxp_start(struct xe_pxp *pxp, u8 type)
 {
        int ret = 0;
+       bool restart = false;
 
        if (!xe_pxp_is_enabled(pxp))
                return -ENODEV;
 
        /* we only support HWDRM sessions right now */
-       xe_assert(pxp->xe, q->pxp.type == DRM_XE_PXP_TYPE_HWDRM);
-
-       /*
-        * Runtime suspend kills PXP, so we take a reference to prevent it from
-        * happening while we have active queues that use PXP
-        */
-       xe_pm_runtime_get(pxp->xe);
+       xe_assert(pxp->xe, type == DRM_XE_PXP_TYPE_HWDRM);
 
        /* get_readiness_status() returns 0 for in-progress and 1 for done */
        ret = xe_pxp_get_readiness_status(pxp);
-       if (ret <= 0) {
-               if (!ret)
-                       ret = -EBUSY;
-               goto out;
-       }
+       if (ret <= 0)
+               return ret ?: -EBUSY;
+
        ret = 0;
 
 wait_for_idle:
        /*
         * if there is an action in progress, wait for it. We need to wait
         * outside the lock because the completion is done from within the lock.
-        * Note that the two action should never be pending at the same time.
+        * Note that the two actions should never be pending at the same time.
         */
        if (!wait_for_completion_timeout(&pxp->termination,
-                                        msecs_to_jiffies(PXP_TERMINATION_TIMEOUT_MS))) {
-               ret = -ETIMEDOUT;
-               goto out;
-       }
+                                        msecs_to_jiffies(PXP_TERMINATION_TIMEOUT_MS)))
+               return -ETIMEDOUT;
 
        if (!wait_for_completion_timeout(&pxp->activation,
-                                        msecs_to_jiffies(PXP_ACTIVATION_TIMEOUT_MS))) {
-               ret = -ETIMEDOUT;
-               goto out;
-       }
+                                        msecs_to_jiffies(PXP_ACTIVATION_TIMEOUT_MS)))
+               return -ETIMEDOUT;
 
        mutex_lock(&pxp->mutex);
 
@@ -574,11 +567,9 @@ wait_for_idle:
        switch (pxp->status) {
        case XE_PXP_ERROR:
                ret = -EIO;
-               break;
+               goto out_unlock;
        case XE_PXP_ACTIVE:
-               __exec_queue_add(pxp, q);
-               mutex_unlock(&pxp->mutex);
-               goto out;
+               goto out_unlock;
        case XE_PXP_READY_TO_START:
                pxp->status = XE_PXP_START_IN_PROGRESS;
                reinit_completion(&pxp->activation);
@@ -586,8 +577,8 @@ wait_for_idle:
        case XE_PXP_START_IN_PROGRESS:
                /* If a start is in progress then the completion must not be done */
                XE_WARN_ON(completion_done(&pxp->activation));
-               mutex_unlock(&pxp->mutex);
-               goto wait_for_idle;
+               restart = true;
+               goto out_unlock;
        case XE_PXP_NEEDS_TERMINATION:
                mark_termination_in_progress(pxp);
                break;
@@ -595,29 +586,25 @@ wait_for_idle:
        case XE_PXP_NEEDS_ADDITIONAL_TERMINATION:
                /* If a termination is in progress then the completion must not be done */
                XE_WARN_ON(completion_done(&pxp->termination));
-               mutex_unlock(&pxp->mutex);
-               goto wait_for_idle;
+               restart = true;
+               goto out_unlock;
        case XE_PXP_SUSPENDED:
        default:
                drm_err(&pxp->xe->drm, "unexpected state during PXP start: %u\n", pxp->status);
                ret = -EIO;
-               break;
+               goto out_unlock;
        }
 
        mutex_unlock(&pxp->mutex);
 
-       if (ret)
-               goto out;
-
        if (!completion_done(&pxp->termination)) {
                ret = pxp_terminate_hw(pxp);
                if (ret) {
                        drm_err(&pxp->xe->drm, "PXP termination failed before start\n");
                        mutex_lock(&pxp->mutex);
                        pxp->status = XE_PXP_ERROR;
-                       mutex_unlock(&pxp->mutex);
 
-                       goto out;
+                       goto out_unlock;
                }
 
                goto wait_for_idle;
@@ -639,21 +626,59 @@ wait_for_idle:
        if (pxp->status != XE_PXP_START_IN_PROGRESS) {
                drm_err(&pxp->xe->drm, "unexpected state after PXP start: %u\n", pxp->status);
                pxp->status = XE_PXP_NEEDS_TERMINATION;
-               mutex_unlock(&pxp->mutex);
-               goto wait_for_idle;
+               restart = true;
+               goto out_unlock;
        }
 
        /* If everything went ok, update the status and add the queue to the list */
-       if (!ret) {
+       if (!ret)
                pxp->status = XE_PXP_ACTIVE;
-               __exec_queue_add(pxp, q);
-       } else {
+       else
                pxp->status = XE_PXP_ERROR;
-       }
 
+out_unlock:
        mutex_unlock(&pxp->mutex);
 
-out:
+       if (restart)
+               goto wait_for_idle;
+
+       return ret;
+}
+
+/**
+ * xe_pxp_exec_queue_add - add a queue to the PXP list
+ * @pxp: the xe->pxp pointer (it will be NULL if PXP is disabled)
+ * @q: the queue to add to the list
+ *
+ * If PXP is enabled and the prerequisites are done, start the PXP default
+ * session (if not already running) and add the queue to the PXP list.
+ *
+ * Returns 0 if the PXP session is running and the queue is in the list,
+ * -ENODEV if PXP is disabled, -EBUSY if the PXP prerequisites are not done,
+ * other errno value if something goes wrong during the session start.
+ */
+int xe_pxp_exec_queue_add(struct xe_pxp *pxp, struct xe_exec_queue *q)
+{
+       int ret;
+
+       if (!xe_pxp_is_enabled(pxp))
+               return -ENODEV;
+
+       /*
+        * Runtime suspend kills PXP, so we take a reference to prevent it from
+        * happening while we have active queues that use PXP
+        */
+       xe_pm_runtime_get(pxp->xe);
+
+start:
+       ret = pxp_start(pxp, q->pxp.type);
+
+       if (!ret) {
+               ret = __exec_queue_add(pxp, q);
+               if (ret == -EBUSY)
+                       goto start;
+       }
+
        /*
         * in the successful case the PM ref is released from
         * xe_pxp_exec_queue_remove