Re: [Intel-gfx] [PATCH v7 6/8] drm/i915/uapi/pxp: Fix UAPI spec comments and add GET_PARAM for PXP

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>]

On 4/6/2023 10:44 AM, Alan Previn wrote:
> 1. UAPI update:
> Without actually changing backward compatible behavior, update
> i915's drm-uapi comments that describe the possible error values
> when creating a context with I915_CONTEXT_PARAM_PROTECTED_CONTENT.
> Since the first merge of PXP support on ADL, i915 returns
> -ENXIO if a dependency such as firmware or component driver
> was yet to be loaded or returns -EIO if the creation attempt
> failed when requested by the PXP firmware (specific firmware
> error responses are reported in dmesg).
>
> 2. GET_PARAM for PXP:
> Because of the additional firmware, component-driver and
> initialization depedencies required on MTL platform before a
> PXP context can be created, UMD calling for PXP creation as a
> way to get-caps can take a long time. An actual real world
> customer stack has seen this happen in the 4-to-8 second range
> after the kernel starts (which sees MESA's init appear in the
> middle of this range as the compositor comes up). To avoid
> unncessary delays experienced by the UMD for get-caps purposes,
> add a GET_PARAM for I915_PARAM_PXP_SUPPORT.
>
> However, some failures can still occur after all the depedencies
> are met (such as firmware init flow failure, bios configurations
> or SOC fusing not allowing PXP enablement). Those scenarios will
> only be known to user space when it attempts creating a PXP context.
>
> With this change, large delays are only met by user-space procsses
> that explicitly need to create a PXP context and boot very early.
> There is no way to avoid this today.
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_getparam.c |  5 +++++
>   include/uapi/drm/i915_drm.h          | 22 ++++++++++++++++++++++
>   2 files changed, 27 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c
> index 2238e096c957..9729384f033f 100644
> --- a/drivers/gpu/drm/i915/i915_getparam.c
> +++ b/drivers/gpu/drm/i915/i915_getparam.c
> @@ -5,6 +5,8 @@
>   #include "gem/i915_gem_mman.h"
>   #include "gt/intel_engine_user.h"
>   
> +#include "pxp/intel_pxp.h"
> +
>   #include "i915_cmd_parser.h"
>   #include "i915_drv.h"
>   #include "i915_getparam.h"
> @@ -102,6 +104,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
>   		if (value < 0)
>   			return value;
>   		break;
> +	case I915_PARAM_PXP_STATUS:
> +		value = intel_pxp_is_enabled(i915->pxp) ? 0 : -ENODEV;
> +		break;
>   	case I915_PARAM_MMAP_GTT_VERSION:
>   		/* Though we've started our numbering from 1, and so class all
>   		 * earlier versions as 0, in effect their value is undefined as
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index dba7c5a5b25e..0c1729bd911d 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -771,6 +771,20 @@ typedef struct drm_i915_irq_wait {
>    */
>   #define I915_PARAM_OA_TIMESTAMP_FREQUENCY 57
>   
> +/*
> + * Query the status of PXP support in i915.
> + *
> + * The query can fail in the following scenarios with the listed error codes:
> + *  -ENODEV = PXP support is not available on the GPU device or in the kernel
> + *            due to missing component drivers or kernel configs.
> + * If the IOCTL is successful, the returned parameter will be set to one of the
> + * following values:
> + *   0 = PXP support maybe available but underlying SOC fusing, BIOS or firmware
> + *       configuration is unknown and a PXP-context-creation would be required
> + *       for final verification of feature availibility.

Would it be useful to add:

1 = PXP support is available

And start returning that after we've successfully created our first 
session? Not sure if userspace would use this though, since they still 
need to handle the 0 case anyway.
I'm also ok with this patch as-is, as long as you get an ack from the 
userspace drivers for this interface behavior:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

> + */
> +#define I915_PARAM_PXP_STATUS		 58
> +
>   /* Must be kept compact -- no holes and well documented */
>   
>   /**
> @@ -2096,6 +2110,14 @@ struct drm_i915_gem_context_param {
>    *
>    * -ENODEV: feature not available
>    * -EPERM: trying to mark a recoverable or not bannable context as protected
> + * -ENXIO: A dependency such as a component driver or firmware is not yet
> + *         loaded and user space may attempt again. Depending on the device
> + *         this error may be reported if the protected context creation is
> + *         attempted very early from kernel start (numbers vary depending on
> + *         system and kernel config):
> + *            - ADL/RPL: up to 3 seconds
> + *            - MTL: up to 8 seconds
> + * -EIO: The firmware did not succeed in creating the protected context.
>    */
>   #define I915_CONTEXT_PARAM_PROTECTED_CONTENT    0xd
>   /* Must be kept compact -- no holes and well documented */