* [Intel-gfx] [PATCH] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS
@ 2023-06-01 17:45 Alan Previn
2023-06-01 23:10 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Alan Previn @ 2023-06-01 17:45 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel, 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.
This patch adds this 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.
- 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').
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 11 +++++++++-
drivers/gpu/drm/i915/i915_getparam.c | 2 +-
drivers/gpu/drm/i915/pxp/intel_pxp.c | 25 ++++++++++++++++++----
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 ++++++++
7 files changed, 50 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
index fb0984f875f9..4dd744c96a37 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
@@ -42,8 +42,17 @@ static void gsc_work(struct work_struct *work)
}
ret = intel_gsc_proxy_request_handler(gsc);
- if (ret)
+ if (ret) {
+ if (actions & GSC_ACTION_FW_LOAD) {
+ /*
+ * a proxy request failure that came together with the
+ * firmware load action means the last part of init has
+ * failed so GSC fw won't be usable after this
+ */
+ intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_LOAD_FAIL);
+ }
goto out_put;
+ }
/* mark the GSC FW init as done the first time we run this */
if (actions & GSC_ACTION_FW_LOAD) {
diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c
index 6f11d7eaa91a..1b2ee98a158a 100644
--- a/drivers/gpu/drm/i915/i915_getparam.c
+++ b/drivers/gpu/drm/i915/i915_getparam.c
@@ -105,7 +105,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, 1);
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 bb2e15329f34..1478bb9b4e26 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -359,21 +359,38 @@ 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;
+}
+
/*
* 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)
{
if (!intel_pxp_is_enabled(pxp))
return -ENODEV;
+ if (pxp_required_fw_failed(pxp))
+ return -ENODEV;
+
+ if (pxp->platform_cfg_is_bad)
+ return -ENODEV;
+
if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) {
- if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 250))
+ if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), timeout))
return 2;
} else {
- if (wait_for(pxp_component_bound(pxp), 250))
+ if (wait_for(pxp_component_bound(pxp), timeout))
return 2;
}
return 1;
@@ -387,7 +404,7 @@ 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, 250);
if (ret < 0)
return ret;
else if (ret > 1)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 17254c3f1267..46d65d641e2b 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -26,7 +26,7 @@ void intel_pxp_fini_hw(struct intel_pxp *pxp);
void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id);
-int intel_pxp_get_readiness_status(struct intel_pxp *pxp);
+int intel_pxp_get_readiness_status(struct intel_pxp *pxp, int timeout);
int intel_pxp_get_backend_timeout_ms(struct intel_pxp *pxp);
int intel_pxp_start(struct intel_pxp *pxp);
void intel_pxp_end(struct intel_pxp *pxp);
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
index 8dc41de3f6f7..d891dd1d051e 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
@@ -17,12 +17,13 @@
#include "intel_pxp_types.h"
static bool
-is_fw_err_platform_config(u32 type)
+is_fw_err_platform_config(u32 type, struct intel_pxp *pxp)
{
switch (type) {
case PXP_STATUS_ERROR_API_VERSION:
case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF:
case PXP_STATUS_PLATFCONFIG_KF1_BAD:
+ pxp->platform_cfg_is_bad = true;
return true;
default:
break;
@@ -225,7 +226,7 @@ int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
if (ret) {
drm_err(&i915->drm, "Failed to init session %d, ret=[%d]\n", arb_session_id, ret);
} else if (msg_out.header.status != 0) {
- if (is_fw_err_platform_config(msg_out.header.status)) {
+ if (is_fw_err_platform_config(msg_out.header.status, pxp)) {
drm_info_once(&i915->drm,
"PXP init-session-%d failed due to BIOS/SOC:0x%08x:%s\n",
arb_session_id, msg_out.header.status,
@@ -268,7 +269,7 @@ void intel_pxp_gsccs_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id)
drm_err(&i915->drm, "Failed to inv-stream-key-%u, ret=[%d]\n",
session_id, ret);
} else if (msg_out.header.status != 0) {
- if (is_fw_err_platform_config(msg_out.header.status)) {
+ if (is_fw_err_platform_config(msg_out.header.status, pxp)) {
drm_info_once(&i915->drm,
"PXP inv-stream-key-%u failed due to BIOS/SOC :0x%08x:%s\n",
session_id, msg_out.header.status,
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
index 1ce07d7e8769..535f4ff824b8 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
@@ -20,12 +20,13 @@
#include "intel_pxp_types.h"
static bool
-is_fw_err_platform_config(u32 type)
+is_fw_err_platform_config(u32 type, struct intel_pxp *pxp)
{
switch (type) {
case PXP_STATUS_ERROR_API_VERSION:
case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF:
case PXP_STATUS_PLATFCONFIG_KF1_BAD:
+ pxp->platform_cfg_is_bad = true;
return true;
default:
break;
@@ -339,7 +340,7 @@ int intel_pxp_tee_cmd_create_arb_session(struct intel_pxp *pxp,
if (ret) {
drm_err(&i915->drm, "Failed to send tee msg init arb session, ret=[%d]\n", ret);
} else if (msg_out.header.status != 0) {
- if (is_fw_err_platform_config(msg_out.header.status)) {
+ if (is_fw_err_platform_config(msg_out.header.status, pxp)) {
drm_info_once(&i915->drm,
"PXP init-arb-session-%d failed due to BIOS/SOC:0x%08x:%s\n",
arb_session_id, msg_out.header.status,
@@ -387,7 +388,7 @@ void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id)
drm_err(&i915->drm, "Failed to send tee msg for inv-stream-key-%u, ret=[%d]\n",
session_id, ret);
} else if (msg_out.header.status != 0) {
- if (is_fw_err_platform_config(msg_out.header.status)) {
+ if (is_fw_err_platform_config(msg_out.header.status, pxp)) {
drm_info_once(&i915->drm,
"PXP inv-stream-key-%u failed due to BIOS/SOC :0x%08x:%s\n",
session_id, msg_out.header.status,
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
index 1a8765866b8b..7e11fa8034b2 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
@@ -26,6 +26,15 @@ struct intel_pxp {
*/
struct intel_gt *ctrl_gt;
+ /**
+ * @platform_cfg_is_bad: used to track if any prior arb session creation resulted
+ * in a failure that was caused by a platform configuration issue, meaning that
+ * failure will not get resolved without a change to the platform (not kernel)
+ * such as BIOS configuration, firwmware update, etc. This bool gets reflected when
+ * GET_PARAM:I915_PARAM_PXP_STATUS is called.
+ */
+ bool platform_cfg_is_bad;
+
/**
* @kcr_base: base mmio offset for the KCR engine which is different on legacy platforms
* vs newer platforms where the KCR is inside the media-tile.
base-commit: a66da4c33d8ede541aea9ba6d0d73b556a072d54
--
2.39.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS 2023-06-01 17:45 [Intel-gfx] [PATCH] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS Alan Previn @ 2023-06-01 23:10 ` Patchwork 2023-06-02 13:03 ` [Intel-gfx] [PATCH] " Jani Nikula 2023-06-20 14:30 ` Balasubrawmanian, Vivaik 2 siblings, 0 replies; 6+ messages in thread From: Patchwork @ 2023-06-01 23:10 UTC (permalink / raw) To: Teres Alexis, Alan Previn; +Cc: intel-gfx [-- Attachment #1: Type: text/plain, Size: 8123 bytes --] == Series Details == Series: drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS URL : https://patchwork.freedesktop.org/series/118723/ State : success == Summary == CI Bug Log - changes from CI_DRM_13217 -> Patchwork_118723v1 ==================================================== Summary ------- **SUCCESS** No regressions found. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/index.html Participating hosts (38 -> 37) ------------------------------ Missing (1): fi-kbl-soraka Known issues ------------ Here are the changes found in Patchwork_118723v1 that come from known issues: ### CI changes ### #### Possible fixes #### * boot: - fi-kbl-8809g: [FAIL][1] ([i915#8293] / [i915#8298]) -> [PASS][2] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13217/fi-kbl-8809g/boot.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/fi-kbl-8809g/boot.html ### IGT changes ### #### Issues hit #### * igt@core_hotunplug@unbind-rebind: - fi-kbl-8809g: NOTRUN -> [ABORT][3] ([i915#8298] / [i915#8299] / [i915#8397]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/fi-kbl-8809g/igt@core_hotunplug@unbind-rebind.html * igt@gem_huc_copy@huc-copy: - fi-kbl-8809g: NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#2190]) [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/fi-kbl-8809g/igt@gem_huc_copy@huc-copy.html * igt@i915_selftest@live@reset: - bat-rpls-2: [PASS][5] -> [ABORT][6] ([i915#4983] / [i915#7461] / [i915#7913] / [i915#8347]) [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13217/bat-rpls-2/igt@i915_selftest@live@reset.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/bat-rpls-2/igt@i915_selftest@live@reset.html * igt@kms_addfb_basic@too-high: - fi-kbl-8809g: NOTRUN -> [FAIL][7] ([i915#8296]) +2 similar issues [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/fi-kbl-8809g/igt@kms_addfb_basic@too-high.html * igt@kms_chamelium_hpd@common-hpd-after-suspend: - bat-dg2-11: NOTRUN -> [SKIP][8] ([i915#7828]) [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/bat-dg2-11/igt@kms_chamelium_hpd@common-hpd-after-suspend.html * igt@kms_force_connector_basic@force-connector-state: - fi-kbl-8809g: NOTRUN -> [DMESG-FAIL][9] ([i915#8299]) [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/fi-kbl-8809g/igt@kms_force_connector_basic@force-connector-state.html * igt@kms_force_connector_basic@force-edid: - fi-kbl-8809g: NOTRUN -> [CRASH][10] ([i915#8299]) [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/fi-kbl-8809g/igt@kms_force_connector_basic@force-edid.html * igt@kms_pipe_crc_basic@read-crc: - bat-adlp-9: NOTRUN -> [SKIP][11] ([i915#3546]) +1 similar issue [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/bat-adlp-9/igt@kms_pipe_crc_basic@read-crc.html - bat-dg2-11: NOTRUN -> [SKIP][12] ([i915#1845] / [i915#5354]) +1 similar issue [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/bat-dg2-11/igt@kms_pipe_crc_basic@read-crc.html * igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-dp-3: - bat-dg2-9: [PASS][13] -> [FAIL][14] ([fdo#103375]) +1 similar issue [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13217/bat-dg2-9/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-dp-3.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/bat-dg2-9/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-dp-3.html * igt@kms_pipe_crc_basic@suspend-read-crc@pipe-c-dp-3: - bat-dg2-9: [PASS][15] -> [FAIL][16] ([fdo#103375] / [i915#7932]) [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13217/bat-dg2-9/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-c-dp-3.html [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/bat-dg2-9/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-c-dp-3.html * igt@kms_psr@cursor_plane_move: - fi-kbl-8809g: NOTRUN -> [SKIP][17] ([fdo#109271]) +59 similar issues [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/fi-kbl-8809g/igt@kms_psr@cursor_plane_move.html * igt@kms_setmode@basic-clone-single-crtc: - fi-kbl-8809g: NOTRUN -> [SKIP][18] ([fdo#109271] / [i915#4579]) [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/fi-kbl-8809g/igt@kms_setmode@basic-clone-single-crtc.html #### Possible fixes #### * igt@i915_selftest@live@gt_mocs: - {bat-mtlp-8}: [DMESG-FAIL][19] ([i915#7059]) -> [PASS][20] [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13217/bat-mtlp-8/igt@i915_selftest@live@gt_mocs.html [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/bat-mtlp-8/igt@i915_selftest@live@gt_mocs.html * igt@i915_selftest@live@gt_pm: - bat-rpls-2: [DMESG-FAIL][21] ([i915#4258] / [i915#7913]) -> [PASS][22] [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13217/bat-rpls-2/igt@i915_selftest@live@gt_pm.html [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/bat-rpls-2/igt@i915_selftest@live@gt_pm.html * igt@i915_selftest@live@reset: - bat-dg2-11: [INCOMPLETE][23] ([i915#7913]) -> [PASS][24] [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13217/bat-dg2-11/igt@i915_selftest@live@reset.html [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/bat-dg2-11/igt@i915_selftest@live@reset.html * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1: - bat-dg2-8: [FAIL][25] ([i915#7932]) -> [PASS][26] [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13217/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375 [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845 [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190 [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546 [i915#4258]: https://gitlab.freedesktop.org/drm/intel/issues/4258 [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579 [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983 [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354 [i915#7059]: https://gitlab.freedesktop.org/drm/intel/issues/7059 [i915#7461]: https://gitlab.freedesktop.org/drm/intel/issues/7461 [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828 [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913 [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932 [i915#8293]: https://gitlab.freedesktop.org/drm/intel/issues/8293 [i915#8296]: https://gitlab.freedesktop.org/drm/intel/issues/8296 [i915#8298]: https://gitlab.freedesktop.org/drm/intel/issues/8298 [i915#8299]: https://gitlab.freedesktop.org/drm/intel/issues/8299 [i915#8347]: https://gitlab.freedesktop.org/drm/intel/issues/8347 [i915#8397]: https://gitlab.freedesktop.org/drm/intel/issues/8397 [i915#8497]: https://gitlab.freedesktop.org/drm/intel/issues/8497 Build changes ------------- * Linux: CI_DRM_13217 -> Patchwork_118723v1 CI-20190529: 20190529 CI_DRM_13217: 37b9b6d05f2421323eb83d005d8863f59855b003 @ git://anongit.freedesktop.org/gfx-ci/linux IGT_7317: c902b72df45aa49faa38205bc5be3c748d33a3e0 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git Patchwork_118723v1: 37b9b6d05f2421323eb83d005d8863f59855b003 @ git://anongit.freedesktop.org/gfx-ci/linux ### Linux commits 1d9a18187d13 drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118723v1/index.html [-- Attachment #2: Type: text/html, Size: 9688 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS 2023-06-01 17:45 [Intel-gfx] [PATCH] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS Alan Previn 2023-06-01 23:10 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork @ 2023-06-02 13:03 ` Jani Nikula 2023-06-02 15:32 ` Teres Alexis, Alan Previn 2023-06-20 14:30 ` Balasubrawmanian, Vivaik 2 siblings, 1 reply; 6+ messages in thread From: Jani Nikula @ 2023-06-02 13:03 UTC (permalink / raw) To: Alan Previn, intel-gfx; +Cc: Alan Previn, dri-devel On Thu, 01 Jun 2023, Alan Previn <alan.previn.teres.alexis@intel.com> 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. > > This patch adds this additional optimizations: Nitpick, please avoid "This patch". It's redundant, and after the patch gets applied it becomes a commit, not a patch. Instead, use the imperative mood, e.g. "Add these additional optimizations". See https://docs.kernel.org/process/submitting-patches.html#describe-your-changes > - 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. > - 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'). > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 11 +++++++++- > drivers/gpu/drm/i915/i915_getparam.c | 2 +- > drivers/gpu/drm/i915/pxp/intel_pxp.c | 25 ++++++++++++++++++---- > 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 ++++++++ > 7 files changed, 50 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c > index fb0984f875f9..4dd744c96a37 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c > @@ -42,8 +42,17 @@ static void gsc_work(struct work_struct *work) > } > > ret = intel_gsc_proxy_request_handler(gsc); > - if (ret) > + if (ret) { > + if (actions & GSC_ACTION_FW_LOAD) { > + /* > + * a proxy request failure that came together with the > + * firmware load action means the last part of init has > + * failed so GSC fw won't be usable after this > + */ > + intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_LOAD_FAIL); > + } > goto out_put; > + } > > /* mark the GSC FW init as done the first time we run this */ > if (actions & GSC_ACTION_FW_LOAD) { > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c > index 6f11d7eaa91a..1b2ee98a158a 100644 > --- a/drivers/gpu/drm/i915/i915_getparam.c > +++ b/drivers/gpu/drm/i915/i915_getparam.c > @@ -105,7 +105,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, 1); > 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 bb2e15329f34..1478bb9b4e26 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c > @@ -359,21 +359,38 @@ 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; > +} > + > /* > * 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) It would help the reader if you named the parameter timeout_ms. Assuming the unit is ms. > { > if (!intel_pxp_is_enabled(pxp)) > return -ENODEV; > > + if (pxp_required_fw_failed(pxp)) > + return -ENODEV; > + > + if (pxp->platform_cfg_is_bad) > + return -ENODEV; > + > if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) { > - if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 250)) > + if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), timeout)) > return 2; > } else { > - if (wait_for(pxp_component_bound(pxp), 250)) > + if (wait_for(pxp_component_bound(pxp), timeout)) > return 2; > } > return 1; > @@ -387,7 +404,7 @@ 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, 250); > if (ret < 0) > return ret; > else if (ret > 1) > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h > index 17254c3f1267..46d65d641e2b 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h > @@ -26,7 +26,7 @@ void intel_pxp_fini_hw(struct intel_pxp *pxp); > void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp); > void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id); > > -int intel_pxp_get_readiness_status(struct intel_pxp *pxp); > +int intel_pxp_get_readiness_status(struct intel_pxp *pxp, int timeout); > int intel_pxp_get_backend_timeout_ms(struct intel_pxp *pxp); > int intel_pxp_start(struct intel_pxp *pxp); > void intel_pxp_end(struct intel_pxp *pxp); > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > index 8dc41de3f6f7..d891dd1d051e 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > @@ -17,12 +17,13 @@ > #include "intel_pxp_types.h" > > static bool > -is_fw_err_platform_config(u32 type) > +is_fw_err_platform_config(u32 type, struct intel_pxp *pxp) It's customary to have the parameters ordered from higher context to lower. > { > switch (type) { > case PXP_STATUS_ERROR_API_VERSION: > case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: > case PXP_STATUS_PLATFCONFIG_KF1_BAD: > + pxp->platform_cfg_is_bad = true; > return true; > default: > break; > @@ -225,7 +226,7 @@ int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, > if (ret) { > drm_err(&i915->drm, "Failed to init session %d, ret=[%d]\n", arb_session_id, ret); > } else if (msg_out.header.status != 0) { > - if (is_fw_err_platform_config(msg_out.header.status)) { > + if (is_fw_err_platform_config(msg_out.header.status, pxp)) { > drm_info_once(&i915->drm, > "PXP init-session-%d failed due to BIOS/SOC:0x%08x:%s\n", > arb_session_id, msg_out.header.status, > @@ -268,7 +269,7 @@ void intel_pxp_gsccs_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id) > drm_err(&i915->drm, "Failed to inv-stream-key-%u, ret=[%d]\n", > session_id, ret); > } else if (msg_out.header.status != 0) { > - if (is_fw_err_platform_config(msg_out.header.status)) { > + if (is_fw_err_platform_config(msg_out.header.status, pxp)) { > drm_info_once(&i915->drm, > "PXP inv-stream-key-%u failed due to BIOS/SOC :0x%08x:%s\n", > session_id, msg_out.header.status, > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c > index 1ce07d7e8769..535f4ff824b8 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c > @@ -20,12 +20,13 @@ > #include "intel_pxp_types.h" > > static bool > -is_fw_err_platform_config(u32 type) > +is_fw_err_platform_config(u32 type, struct intel_pxp *pxp) Ditto. > { > switch (type) { > case PXP_STATUS_ERROR_API_VERSION: > case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: > case PXP_STATUS_PLATFCONFIG_KF1_BAD: > + pxp->platform_cfg_is_bad = true; > return true; > default: > break; > @@ -339,7 +340,7 @@ int intel_pxp_tee_cmd_create_arb_session(struct intel_pxp *pxp, > if (ret) { > drm_err(&i915->drm, "Failed to send tee msg init arb session, ret=[%d]\n", ret); > } else if (msg_out.header.status != 0) { > - if (is_fw_err_platform_config(msg_out.header.status)) { > + if (is_fw_err_platform_config(msg_out.header.status, pxp)) { > drm_info_once(&i915->drm, > "PXP init-arb-session-%d failed due to BIOS/SOC:0x%08x:%s\n", > arb_session_id, msg_out.header.status, > @@ -387,7 +388,7 @@ void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id) > drm_err(&i915->drm, "Failed to send tee msg for inv-stream-key-%u, ret=[%d]\n", > session_id, ret); > } else if (msg_out.header.status != 0) { > - if (is_fw_err_platform_config(msg_out.header.status)) { > + if (is_fw_err_platform_config(msg_out.header.status, pxp)) { > drm_info_once(&i915->drm, > "PXP inv-stream-key-%u failed due to BIOS/SOC :0x%08x:%s\n", > session_id, msg_out.header.status, > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > index 1a8765866b8b..7e11fa8034b2 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > @@ -26,6 +26,15 @@ struct intel_pxp { > */ > struct intel_gt *ctrl_gt; > > + /** > + * @platform_cfg_is_bad: used to track if any prior arb session creation resulted > + * in a failure that was caused by a platform configuration issue, meaning that > + * failure will not get resolved without a change to the platform (not kernel) > + * such as BIOS configuration, firwmware update, etc. This bool gets reflected when > + * GET_PARAM:I915_PARAM_PXP_STATUS is called. > + */ > + bool platform_cfg_is_bad; > + > /** > * @kcr_base: base mmio offset for the KCR engine which is different on legacy platforms > * vs newer platforms where the KCR is inside the media-tile. > > base-commit: a66da4c33d8ede541aea9ba6d0d73b556a072d54 -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS 2023-06-02 13:03 ` [Intel-gfx] [PATCH] " Jani Nikula @ 2023-06-02 15:32 ` Teres Alexis, Alan Previn 0 siblings, 0 replies; 6+ messages in thread From: Teres Alexis, Alan Previn @ 2023-06-02 15:32 UTC (permalink / raw) To: intel-gfx@lists.freedesktop.org, jani.nikula@linux.intel.com Cc: dri-devel@lists.freedesktop.org Thanks Jani - will rev this up and fix these. On Fri, 2023-06-02 at 16:03 +0300, Jani Nikula wrote: > On Thu, 01 Jun 2023, Alan Previn <alan.previn.teres.alexis@intel.com> 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. > > > > This patch adds this additional optimizations: > > Nitpick, please avoid "This patch". It's redundant, and after the patch > gets applied it becomes a commit, not a patch. > > Instead, use the imperative mood, e.g. "Add these additional > optimizations". > > See https://docs.kernel.org/process/submitting-patches.html#describe-your-changes alan:snip > > > -int intel_pxp_get_readiness_status(struct intel_pxp *pxp) > > +int intel_pxp_get_readiness_status(struct intel_pxp *pxp, int timeout) > > It would help the reader if you named the parameter timeout_ms. Assuming > the unit is ms. alan:snip > > -is_fw_err_platform_config(u32 type) > > +is_fw_err_platform_config(u32 type, struct intel_pxp *pxp) > > It's customary to have the parameters ordered from higher context to > lower. > alan:snip ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS 2023-06-01 17:45 [Intel-gfx] [PATCH] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS Alan Previn 2023-06-01 23:10 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork 2023-06-02 13:03 ` [Intel-gfx] [PATCH] " Jani Nikula @ 2023-06-20 14:30 ` Balasubrawmanian, Vivaik 2023-06-29 22:39 ` Teres Alexis, Alan Previn 2 siblings, 1 reply; 6+ messages in thread From: Balasubrawmanian, Vivaik @ 2023-06-20 14:30 UTC (permalink / raw) To: Alan Previn, intel-gfx; +Cc: dri-devel On 6/1/2023 12:45 PM, 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. > > This patch adds this 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. > - 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'). > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 11 +++++++++- > drivers/gpu/drm/i915/i915_getparam.c | 2 +- > drivers/gpu/drm/i915/pxp/intel_pxp.c | 25 ++++++++++++++++++---- > 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 ++++++++ > 7 files changed, 50 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c > index fb0984f875f9..4dd744c96a37 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c > @@ -42,8 +42,17 @@ static void gsc_work(struct work_struct *work) > } > > ret = intel_gsc_proxy_request_handler(gsc); > - if (ret) > + if (ret) { > + if (actions & GSC_ACTION_FW_LOAD) { > + /* > + * a proxy request failure that came together with the > + * firmware load action means the last part of init has > + * failed so GSC fw won't be usable after this > + */ > + intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_LOAD_FAIL); > + } > goto out_put; > + } > > /* mark the GSC FW init as done the first time we run this */ > if (actions & GSC_ACTION_FW_LOAD) { On the huc authentication comment block above this snippet, the last statement: "Note that we can only do the GSC auth if the GuC auth was" is confusing as the code below is only dealing with HuC Authentication. This function seems to have a section to deal with FW load action and another to deal with SW Proxy requests, but we seem to be mixing both actions in the SW proxy section. instead, can we move this call to intel_gsc_proxy_request_handler to the FW load section itself instead of handling it as an additional check in the SW_proxy section? In the same vein, we should also move the intel_uc_fw_change_status() call into the above FW Load action section. i think that way the code reads better. > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c > index 6f11d7eaa91a..1b2ee98a158a 100644 > --- a/drivers/gpu/drm/i915/i915_getparam.c > +++ b/drivers/gpu/drm/i915/i915_getparam.c > @@ -105,7 +105,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, 1); > 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 bb2e15329f34..1478bb9b4e26 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c > @@ -359,21 +359,38 @@ 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; > +} > + > /* > * 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) > { > if (!intel_pxp_is_enabled(pxp)) > return -ENODEV; > > + if (pxp_required_fw_failed(pxp)) > + return -ENODEV; > + > + if (pxp->platform_cfg_is_bad) > + return -ENODEV; > + > if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) { > - if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 250)) > + if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), timeout)) > return 2; > } else { > - if (wait_for(pxp_component_bound(pxp), 250)) > + if (wait_for(pxp_component_bound(pxp), timeout)) > return 2; > } > return 1; > @@ -387,7 +404,7 @@ 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, 250); > if (ret < 0) > return ret; > else if (ret > 1) In intel_pxp_start(), shouldn't the 250ms be defined in the struct as a define with a comment that explains why it is 250 vs some other number? Also in the i915_getparam_ioctl, shouldn't the timeout value be 0 instead of 1 as this is a simple status check? Also, the return value of 2 if the timeout expires seems counter-intuitive. I think EBUSY will be more appropriate especially since the IOCTL call seems to be a quick status check. > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h > index 17254c3f1267..46d65d641e2b 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h > @@ -26,7 +26,7 @@ void intel_pxp_fini_hw(struct intel_pxp *pxp); > void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp); > void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id); > > -int intel_pxp_get_readiness_status(struct intel_pxp *pxp); > +int intel_pxp_get_readiness_status(struct intel_pxp *pxp, int timeout); > int intel_pxp_get_backend_timeout_ms(struct intel_pxp *pxp); > int intel_pxp_start(struct intel_pxp *pxp); > void intel_pxp_end(struct intel_pxp *pxp); > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > index 8dc41de3f6f7..d891dd1d051e 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > @@ -17,12 +17,13 @@ > #include "intel_pxp_types.h" > > static bool > -is_fw_err_platform_config(u32 type) > +is_fw_err_platform_config(u32 type, struct intel_pxp *pxp) > { > switch (type) { > case PXP_STATUS_ERROR_API_VERSION: > case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: > case PXP_STATUS_PLATFCONFIG_KF1_BAD: > + pxp->platform_cfg_is_bad = true; > return true; > default: > break; > @@ -225,7 +226,7 @@ int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, > if (ret) { > drm_err(&i915->drm, "Failed to init session %d, ret=[%d]\n", arb_session_id, ret); > } else if (msg_out.header.status != 0) { > - if (is_fw_err_platform_config(msg_out.header.status)) { > + if (is_fw_err_platform_config(msg_out.header.status, pxp)) { > drm_info_once(&i915->drm, > "PXP init-session-%d failed due to BIOS/SOC:0x%08x:%s\n", > arb_session_id, msg_out.header.status, > @@ -268,7 +269,7 @@ void intel_pxp_gsccs_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id) > drm_err(&i915->drm, "Failed to inv-stream-key-%u, ret=[%d]\n", > session_id, ret); > } else if (msg_out.header.status != 0) { > - if (is_fw_err_platform_config(msg_out.header.status)) { > + if (is_fw_err_platform_config(msg_out.header.status, pxp)) { > drm_info_once(&i915->drm, > "PXP inv-stream-key-%u failed due to BIOS/SOC :0x%08x:%s\n", > session_id, msg_out.header.status, > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c > index 1ce07d7e8769..535f4ff824b8 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c > @@ -20,12 +20,13 @@ > #include "intel_pxp_types.h" > > static bool > -is_fw_err_platform_config(u32 type) > +is_fw_err_platform_config(u32 type, struct intel_pxp *pxp) > { > switch (type) { > case PXP_STATUS_ERROR_API_VERSION: > case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: > case PXP_STATUS_PLATFCONFIG_KF1_BAD: > + pxp->platform_cfg_is_bad = true; > return true; > default: > break; > @@ -339,7 +340,7 @@ int intel_pxp_tee_cmd_create_arb_session(struct intel_pxp *pxp, > if (ret) { > drm_err(&i915->drm, "Failed to send tee msg init arb session, ret=[%d]\n", ret); > } else if (msg_out.header.status != 0) { > - if (is_fw_err_platform_config(msg_out.header.status)) { > + if (is_fw_err_platform_config(msg_out.header.status, pxp)) { > drm_info_once(&i915->drm, > "PXP init-arb-session-%d failed due to BIOS/SOC:0x%08x:%s\n", > arb_session_id, msg_out.header.status, > @@ -387,7 +388,7 @@ void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id) > drm_err(&i915->drm, "Failed to send tee msg for inv-stream-key-%u, ret=[%d]\n", > session_id, ret); > } else if (msg_out.header.status != 0) { > - if (is_fw_err_platform_config(msg_out.header.status)) { > + if (is_fw_err_platform_config(msg_out.header.status, pxp)) { > drm_info_once(&i915->drm, > "PXP inv-stream-key-%u failed due to BIOS/SOC :0x%08x:%s\n", > session_id, msg_out.header.status, > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > index 1a8765866b8b..7e11fa8034b2 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > @@ -26,6 +26,15 @@ struct intel_pxp { > */ > struct intel_gt *ctrl_gt; > > + /** > + * @platform_cfg_is_bad: used to track if any prior arb session creation resulted > + * in a failure that was caused by a platform configuration issue, meaning that > + * failure will not get resolved without a change to the platform (not kernel) > + * such as BIOS configuration, firwmware update, etc. This bool gets reflected when > + * GET_PARAM:I915_PARAM_PXP_STATUS is called. > + */ > + bool platform_cfg_is_bad; > + > /** > * @kcr_base: base mmio offset for the KCR engine which is different on legacy platforms > * vs newer platforms where the KCR is inside the media-tile. > > base-commit: a66da4c33d8ede541aea9ba6d0d73b556a072d54 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS 2023-06-20 14:30 ` Balasubrawmanian, Vivaik @ 2023-06-29 22:39 ` Teres Alexis, Alan Previn 0 siblings, 0 replies; 6+ messages in thread From: Teres Alexis, Alan Previn @ 2023-06-29 22:39 UTC (permalink / raw) To: Balasubrawmanian, Vivaik, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org On Tue, 2023-06-20 at 09:30 -0500, Balasubrawmanian, Vivaik wrote: > On 6/1/2023 12:45 PM, 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. > > > > This patch adds this 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. > > - 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'). > > > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > > --- > > drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 11 +++++++++- > > drivers/gpu/drm/i915/i915_getparam.c | 2 +- > > drivers/gpu/drm/i915/pxp/intel_pxp.c | 25 ++++++++++++++++++---- > > 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 ++++++++ > > 7 files changed, 50 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c > > index fb0984f875f9..4dd744c96a37 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c > > @@ -42,8 +42,17 @@ static void gsc_work(struct work_struct *work) > > } > > > > ret = intel_gsc_proxy_request_handler(gsc); > > - if (ret) > > + if (ret) { > > + if (actions & GSC_ACTION_FW_LOAD) { > > + /* > > + * a proxy request failure that came together with the > > + * firmware load action means the last part of init has > > + * failed so GSC fw won't be usable after this > > + */ > > + intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_LOAD_FAIL); > > + } > > goto out_put; > > + } > > > > /* mark the GSC FW init as done the first time we run this */ > > if (actions & GSC_ACTION_FW_LOAD) { > > On the huc authentication comment block above this snippet, the last > statement: "Note that we can only do the GSC auth if the GuC auth was" > is confusing as the code below is only dealing with HuC Authentication. alan: i believe what he meant was "can only do the GSC-based auth if the GuC-based auth"... but I can't change that code as part of this patch - I believe the rules for kernel patch is to ensure each single patch is modular (not mixing unrelated changes) and focuses just on what its described to do. IIRC, we would need to create a separate patch review for that change. > > This function seems to have a section to deal with FW load action and > another to deal with SW Proxy requests, but we seem to be mixing both > actions in the SW proxy section. instead, can we move this call to > intel_gsc_proxy_request_handler to the FW load section itself instead of > handling it as an additional check in the SW_proxy section? In the same > vein, we should also move the intel_uc_fw_change_status() call into the > above FW Load action section. i think that way the code reads better. alan: GSC_ACTION_FW_LOAD is used for loading the GSC firmware which is a one-time thing per i915 load. However, GSC_ACTION_SW_PROXY events can happen any time the GSC fw needs to communicate with CSE firmware (or vice versa) due to platform events that may have not been triggered by i915 long after init. However, the rule is after GSC FW is loaded, i915 is required to do a 1-time proxy-init step to prime both GSC and CSE fws that proxy comms is avail. without this step, we can't use the gsc-fw for other ops. So to recap the rules: 1. we launch the worker to do the one-time the GSC firmware load. 2. after the GSC firmware load is successful, we have to do a one-time SW-proxy init. -> this is why we add the GSC_ACTION_SW_PROXY flag successful load completion. 3. If we are doing proxy-handling for the very first time, we ensure -> FW status is only set to RUNNING if proxy int was good (since GSC FW cant be accessed to do anything (such as hdcp, pxp, etc) without proxy init completion. -> print a message to signal is proxy init failed. This is the only reason why we have the additional "if (actions & GSC_ACTION_FW_LOAD)" check inside the SW Proxy block - we are not mixing fw loading steps all, but just to distinguish between the first-ever-sw-proxy vs the regular runtime sw-proxy where the latter can fail gracefully without blocking future GSC-fw operation or future sw-proxy handling. That said, to ensure we can properly honor all 3 steps above, we can either call intel_gsc_proxy_request_handler twice (once right after fw-load and every other time runtime proxy events occur) ... or ... we can set GSC_ACTION_SW_PROXY twice... basically its the same thing - so wont make much difference. More importantly, this patch is not changing how and where we call intel_gsc_proxy_request_handler but only optimize how we handle the GET_PARAM:PXP_STATUS. In this specific code block we are reviewing, the only change being done is to ensure that we treat the GSC FW status as failed if the first-time-proxy-init step fails. So once again, if we want to change how we call intel_gsc_proxy_request_handler, that would have to be another patch, but in light of above recap of the rules this worker is attempting to honor, i dont agree that we need to change when we call intel_gsc_proxy_request_handler. ...alan > > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c > > index 6f11d7eaa91a..1b2ee98a158a 100644 > > --- a/drivers/gpu/drm/i915/i915_getparam.c > > +++ b/drivers/gpu/drm/i915/i915_getparam.c > > @@ -105,7 +105,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, 1); > > 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 bb2e15329f34..1478bb9b4e26 100644 > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c > > @@ -359,21 +359,38 @@ 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; > > +} > > + > > /* > > * 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) > > { > > if (!intel_pxp_is_enabled(pxp)) > > return -ENODEV; > > > > + if (pxp_required_fw_failed(pxp)) > > + return -ENODEV; > > + > > + if (pxp->platform_cfg_is_bad) > > + return -ENODEV; > > + > > if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) { > > - if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 250)) > > + if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), timeout)) > > return 2; > > } else { > > - if (wait_for(pxp_component_bound(pxp), 250)) > > + if (wait_for(pxp_component_bound(pxp), timeout)) > > return 2; > > } > > return 1; > > @@ -387,7 +404,7 @@ 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, 250); > > if (ret < 0) > > return ret; > > else if (ret > 1) > > In intel_pxp_start(), shouldn't the 250ms be defined in the struct as a > define with a comment that explains why it is 250 vs some other number? alan: the value 250 is a carry forward from previous ADL implementation but this number is not being used for fw interaction but only to check for readiness. That said there is not other location we use this value for this purpose. Thus, i can add a #define, but would only be used in the same function call and no other. I can add that if u insist. Side note: When intel_pxp_start is called a part of GEM_CONTEXT_CREATE call, i915 UAPI already specs that I915_CONTEXT_PARAM_PROTECTED_CONTENT can fail with -ENXIO when dependencies are not ready and user space can retry so the 250 was chosen based on historical ADL reviews but won't mean anything since the user space will have to retry anyway. > Also in the i915_getparam_ioctl, shouldn't the timeout value be 0 > instead of 1 as this is a simple status check? alan: yes, you are right .. I'll fix that -> get_param shouldnt wait for any timeout. > Also, the return value of 2 if the timeout expires seems > counter-intuitive. I think EBUSY will be more appropriate especially > since the IOCTL call seems to be a quick status check. The IOCTL UAPI behavior was spec'd in the past with the UMD folks and was mirror-ing the only other GET_PARAM type that has a runtime status change where negative value means no support and positive values mean support is available. But we use different positive values to represent different stages of support readiness where 1 is fully ready and 2,3,4... for not yet ready for reason b, c, d... (this model scales for future hw/fw/sw readiness states that doesnt exist yet without breaking backwards compatibility of the UAPI spec. Ofc, today we only use '2' for adl/mtl-pxp. that said, we can't change the UAPI spec now els we'd break backware compatibility with existing SW since the UMD has already implemented the change to follow this spec meaning of negative vs 1 vs 2. alan:snip ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-06-29 22:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-06-01 17:45 [Intel-gfx] [PATCH] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS Alan Previn 2023-06-01 23:10 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork 2023-06-02 13:03 ` [Intel-gfx] [PATCH] " Jani Nikula 2023-06-02 15:32 ` Teres Alexis, Alan Previn 2023-06-20 14:30 ` Balasubrawmanian, Vivaik 2023-06-29 22:39 ` Teres Alexis, Alan Previn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox