Re: [PATCH 2/3] drm/xe/pxp: Clarify PXP queue creation behavior if PXP is not ready

[John Harrison <john.c.harrison@intel.com>]

On 5/19/2025 4:32 PM, Daniele Ceraolo Spurio wrote:
> The expected flow of operations when using PXP is to query the PXP
> status and wait for it to transition to "ready" before attempting to
> create an exec_queue. This flow is followed by the Mesa driver, but
> there is no guarantee that an incorrectly coded (or malicious) app
> will not attempt to create the queue first without querying the status.
> Therefore, we need to clarify what the expected behavior of the queue
> creation ioctl is in this scenario.
>
> Currently, the ioctl always fails with an -EBUSY code no matter the
> error, but for consistency it is better to distinguish between "failed
> to init" (-EIO) and "not ready" (-EBUSY), the same way the query ioctl
> does. Note that, while this is a change in the return code of an ioctl,
> the behavior of the ioctl in this particular corner case was not clearly
> spec'd, so no one should have been relying on it (and we know that Mesa,
> which is the only known userspace for this, didn't).
>
> Fixes: 72d479601d67 ("drm/xe/pxp/uapi: Add userspace and LRC support for PXP-using queues")
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
Reviewed-by: John Harrison <John.C.Harrison@Intel.com>

> ---
>   drivers/gpu/drm/xe/xe_pxp.c | 8 ++++++--
>   include/uapi/drm/xe_drm.h   | 5 +++++
>   2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_pxp.c b/drivers/gpu/drm/xe/xe_pxp.c
> index 454ea7dc08ac..b5bc15f436fa 100644
> --- a/drivers/gpu/drm/xe/xe_pxp.c
> +++ b/drivers/gpu/drm/xe/xe_pxp.c
> @@ -541,10 +541,14 @@ int xe_pxp_exec_queue_add(struct xe_pxp *pxp, struct xe_exec_queue *q)
>   	 */
>   	xe_pm_runtime_get(pxp->xe);
>   
> -	if (!pxp_prerequisites_done(pxp)) {
> -		ret = -EBUSY;
> +	/* 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;
>   	}
> +	ret = 0;
>   
>   wait_for_idle:
>   	/*
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 9c08738c3b91..2708eb73c590 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -1210,6 +1210,11 @@ struct drm_xe_vm_bind {
>    *    there is no need to explicitly set that. When a queue of type
>    *    %DRM_XE_PXP_TYPE_HWDRM is created, the PXP default HWDRM session
>    *    (%XE_PXP_HWDRM_DEFAULT_SESSION) will be started, if isn't already running.
> + *    The user is expected to query the PXP status via the query ioctl (see
> + *    %DRM_XE_DEVICE_QUERY_PXP_STATUS) and to wait for PXP to be ready before
> + *    attempting to create a queue with this property. If a queue is created
> + *    before PXP is ready, the ioctl will return -EBUSY if init is still in
> + *    progress or -EIO if init failed.
>    *    Given that going into a power-saving state kills PXP HWDRM sessions,
>    *    runtime PM will be blocked while queues of this type are alive.
>    *    All PXP queues will be killed if a PXP invalidation event occurs.