Re: [PATCH v4 1/1] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS

2023-08-03 Thread Teres Alexis, Alan Previn
On Wed, 2023-08-02 at 11:25 -0700, Teres Alexis, Alan Previn wrote:
> After recent discussions with Mesa folks, it was requested
> that we optimize i915's GET_PARAM for the PXP_STATUS without
> changing the UAPI spec.
> 
> Add these additional optimizations:
>- If any PXP initializatoin flow failed, then ensure that
>  we catch it so that we can change the returned PXP_STATUS
>  from "2" (i.e. 'PXP is supported but not yet ready')
>  to "-ENODEV". This typically should not happen and if it
>  does, we have a platform configuration issue.
>- If a PXP arbitration session creation event failed
>  due to incorrect firmware version or blocking SOC fusing
>  or blocking BIOS configuration (platform reasons that won't
>  change if we retry), then reflect that blockage by also
>  returning -ENODEV in the GET_PARAM:PXP_STATUS.
>- GET_PARAM:PXP_STATUS should not wait at all if PXP is
>  supported but non-i915 dependencies (component-driver /
>  firmware) we are still pending to complete the init flows.
>  In this case, just return "2" immediately (i.e. 'PXP is
>  supported but not yet ready').
> 
> Difference from prio revs:
>   v3: - Rebase with latest tip that has pulled in setting the
> gsc fw load to fail if proxy init fails.
>   v2: - Use a #define for the default readiness timeout (Vivaik).
>   - Improve comments around the failing of proxy-init.
>   v1: - Change the commit msg style to be imperative. (Jani)
>   - Rename timeout to timeout_ms. (Jani)
>   - Fix is_fw_err_platform_config to use higher order
> param (pxp) first. (Jani)
> 
> Signed-off-by: Alan Previn 

alan: Daniele pointed out that i removed Vivaik's RB from rev-3.
The difference from this rev vs rev is a hunk of code got removed
and went into a different patch (that reused the same code, is
higher priority - CI related, and had to go first). So this rev
is identical to rev3 except a hunk that has been removed.
I've checked with Vivaik and he is good with keeping his R-b
since the rest of the patch is the same. Thus repasting his
R-b from rev3 (Thanks Daniele and Vivaik):

Reviewed-by: Balasubrawmanian, Vivaik 


[PATCH v4 1/1] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS

2023-08-02 Thread Alan Previn
After recent discussions with Mesa folks, it was requested
that we optimize i915's GET_PARAM for the PXP_STATUS without
changing the UAPI spec.

Add these additional optimizations:
   - If any PXP initializatoin flow failed, then ensure that
 we catch it so that we can change the returned PXP_STATUS
 from "2" (i.e. 'PXP is supported but not yet ready')
 to "-ENODEV". This typically should not happen and if it
 does, we have a platform configuration issue.
   - If a PXP arbitration session creation event failed
 due to incorrect firmware version or blocking SOC fusing
 or blocking BIOS configuration (platform reasons that won't
 change if we retry), then reflect that blockage by also
 returning -ENODEV in the GET_PARAM:PXP_STATUS.
   - GET_PARAM:PXP_STATUS should not wait at all if PXP is
 supported but non-i915 dependencies (component-driver /
 firmware) we are still pending to complete the init flows.
 In this case, just return "2" immediately (i.e. 'PXP is
 supported but not yet ready').

Difference from prio revs:
  v3: - Rebase with latest tip that has pulled in setting the
gsc fw load to fail if proxy init fails.
  v2: - Use a #define for the default readiness timeout (Vivaik).
  - Improve comments around the failing of proxy-init.
  v1: - Change the commit msg style to be imperative. (Jani)
  - Rename timeout to timeout_ms. (Jani)
  - Fix is_fw_err_platform_config to use higher order
param (pxp) first. (Jani)

Signed-off-by: Alan Previn 
---
 drivers/gpu/drm/i915/i915_getparam.c   |  2 +-
 drivers/gpu/drm/i915/pxp/intel_pxp.c   | 40 ++
 drivers/gpu/drm/i915/pxp/intel_pxp.h   |  2 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c |  7 ++--
 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c   |  7 ++--
 drivers/gpu/drm/i915/pxp/intel_pxp_types.h |  9 +
 6 files changed, 52 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_getparam.c 
b/drivers/gpu/drm/i915/i915_getparam.c
index 890f2b382bee..5c3fec63cb4c 100644
--- a/drivers/gpu/drm/i915/i915_getparam.c
+++ b/drivers/gpu/drm/i915/i915_getparam.c
@@ -109,7 +109,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
return value;
break;
case I915_PARAM_PXP_STATUS:
-   value = intel_pxp_get_readiness_status(i915->pxp);
+   value = intel_pxp_get_readiness_status(i915->pxp, 0);
if (value < 0)
return value;
break;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c 
b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 38ec754d0ec8..dc327cf40b5a 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -359,22 +359,46 @@ void intel_pxp_end(struct intel_pxp *pxp)
intel_runtime_pm_put(&i915->runtime_pm, wakeref);
 }
 
+static bool pxp_required_fw_failed(struct intel_pxp *pxp)
+{
+   if (__intel_uc_fw_status(&pxp->ctrl_gt->uc.huc.fw) == 
INTEL_UC_FIRMWARE_LOAD_FAIL)
+   return true;
+   if (HAS_ENGINE(pxp->ctrl_gt, GSC0) &&
+   __intel_uc_fw_status(&pxp->ctrl_gt->uc.gsc.fw) == 
INTEL_UC_FIRMWARE_LOAD_FAIL)
+   return true;
+
+   return false;
+}
+
+static bool pxp_fw_dependencies_completed(struct intel_pxp *pxp)
+{
+   if (HAS_ENGINE(pxp->ctrl_gt, GSC0))
+   return intel_pxp_gsccs_is_ready_for_sessions(pxp);
+
+   return pxp_component_bound(pxp);
+}
+
 /*
  * this helper is used by both intel_pxp_start and by
  * the GET_PARAM IOCTL that user space calls. Thus, the
  * return values here should match the UAPI spec.
  */
-int intel_pxp_get_readiness_status(struct intel_pxp *pxp)
+int intel_pxp_get_readiness_status(struct intel_pxp *pxp, int timeout_ms)
 {
if (!intel_pxp_is_enabled(pxp))
return -ENODEV;
 
-   if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) {
-   if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 250))
-   return 2;
-   } else {
-   if (wait_for(pxp_component_bound(pxp), 250))
+   if (pxp_required_fw_failed(pxp))
+   return -ENODEV;
+
+   if (pxp->platform_cfg_is_bad)
+   return -ENODEV;
+
+   if (timeout_ms) {
+   if (wait_for(pxp_fw_dependencies_completed(pxp), timeout_ms))
return 2;
+   } else if (!pxp_fw_dependencies_completed(pxp)) {
+   return 2;
}
return 1;
 }
@@ -383,11 +407,13 @@ int intel_pxp_get_readiness_status(struct intel_pxp *pxp)
  * the arb session is restarted from the irq work when we receive the
  * termination completion interrupt
  */
+#define PXP_READINESS_TIMEOUT 250
+
 int intel_pxp_start(struct intel_pxp *pxp)
 {
int ret = 0;
 
-   ret = intel_pxp_get_readiness_status(pxp);
+   ret = intel_pxp_get_readiness_status(pxp, PXP_READINESS_TIMEOUT);
if (ret < 0)
retur