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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxx>
---
  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@xxxxxxxxx>

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 */




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux