[PATCH 3/3] drm/xe/pxp: Decouple queue addition from PXP start
John Harrison
john.c.harrison at intel.com
Wed May 21 20:41:07 UTC 2025
On 5/19/2025 4:32 PM, Daniele Ceraolo Spurio wrote:
> 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.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: John Harrison <John.C.Harrison at Intel.com>
Reviewed-by: John Harrison <John.C.Harrison at Intel.com>
> ---
> drivers/gpu/drm/xe/xe_pxp.c | 143 ++++++++++++++++++++++--------------
> 1 file changed, 87 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_pxp.c b/drivers/gpu/drm/xe/xe_pxp.c
> index b5bc15f436fa..32ee38e11811 100644
> --- a/drivers/gpu/drm/xe/xe_pxp.c
> +++ b/drivers/gpu/drm/xe/xe_pxp.c
> @@ -504,36 +504,41 @@ 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);
> + xe_assert(pxp->xe, type == DRM_XE_PXP_TYPE_HWDRM);
>
> /*
> * Runtime suspend kills PXP, so we take a reference to prevent it from
> @@ -543,30 +548,24 @@ int xe_pxp_exec_queue_add(struct xe_pxp *pxp, struct xe_exec_queue *q)
>
> /* 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 +573,9 @@ int xe_pxp_exec_queue_add(struct xe_pxp *pxp, struct xe_exec_queue *q)
> 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 +583,8 @@ int xe_pxp_exec_queue_add(struct xe_pxp *pxp, struct xe_exec_queue *q)
> 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 +592,25 @@ int xe_pxp_exec_queue_add(struct xe_pxp *pxp, struct xe_exec_queue *q)
> 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,27 +632,65 @@ int xe_pxp_exec_queue_add(struct xe_pxp *pxp, struct xe_exec_queue *q)
> 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);
> +
> /*
> * in the successful case the PM ref is released from
> * xe_pxp_exec_queue_remove
> */
> - if (ret)
> + if (ret) {
> xe_pm_runtime_put(pxp->xe);
> + return ret;
> + }
> +
> + ret = __exec_queue_add(pxp, q);
> + if (ret == -EBUSY)
> + goto start;
>
> return ret;
> }
More information about the Intel-xe
mailing list