[PATCH v2 3/3] drm/xe/pxp: Decouple queue addition from PXP start
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Thu May 22 22:02:54 UTC 2025
On 5/22/2025 2:26 PM, John Harrison wrote:
> On 5/22/2025 1:46 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
> Just spotted that this block of code needs to be removed. It is now
> done at the level above in xe_pxp_exec_queue_add() and therefore
> should not do a second pm get at this level.
Good catch. Looks like CI had caught this (PXP RPM tests went from pass
to skip) but weirdly the email report didn't include this change. Will fix.
Daniele
>
> John.
>
>> @@ -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