| Message ID | 20230314175832.1506427-1-alan.previn.teres.alexis@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/i915/pxp: limit drm-errors or warning on firmware API failures | expand |
Hi, Tested the patch with Ubuntu 22.04 desktop + Linux 6.2-rc3 (drm-tip) kernel, on TGL-H HW. With it, this log spam has disappeared: ---------------------------------------------------- [ 8691.608933] i915 0000:00:02.0: [drm] PXP firmware failed arb session init request ret=[0x0000101f] [ 8691.709442] i915 0000:00:02.0: [drm] *ERROR* arb session failed to go in play ---------------------------------------------------- There's now just a single message on boot: ---------------------------------------------------- [ 11.674177] ------------[ cut here ]------------ [ 11.674183] i915 0000:00:02.0: PXP init-arb-session-15 failed due to BIOS/SOC:0x0000101a:ERR_PLATFORM_CONFIG [ 11.674222] WARNING: CPU: 12 PID: 123 at drivers/gpu/drm/i915/pxp/intel_pxp_tee.c:345 intel_pxp_tee_cmd_create_arb_session+0x19a/0x1e0 [i915] ... [ 11.674961] Call Trace: [ 11.674966] <TASK> [ 11.674970] ? pxp_session_work+0x123/0x1d0 [i915] [ 11.675130] pxp_session_work+0x123/0x1d0 [i915] [ 11.675286] process_one_work+0x1d9/0x3e0 [ 11.675295] worker_thread+0x21b/0x3d0 [ 11.675301] ? __pfx_worker_thread+0x10/0x10 [ 11.675307] kthread+0xe9/0x110 [ 11.675311] ? __pfx_kthread+0x10/0x10 [ 11.675315] ret_from_fork+0x29/0x50 [ 11.675323] </TASK> [ 11.675325] ---[ end trace 0000000000000000 ]--- ---------------------------------------------------- - Eero On 14.3.2023 19.58, Alan Previn wrote: > MESA driver is creating protected context on every driver handle > creation to query caps bits for app. So when running CI tests, > they are observing hundreds of drm_errors when enabling PXP > in .config but using SOC fusing or BIOS configuration that cannot > support PXP sessions. > > The fixes tag referenced below was to resolve a related issue > where we wanted to silence error messages, but that case was due > to outdated IFWI (firmware) that definitely needed an upgrade and > was, at that point, considered a one-off case as opposed to today's > realization that default CI was enabling PXP in kernel config for > all testing. > > So with this patch, let's strike a balance between issues that is > critical but are root-caused from HW/platform gaps (louder drm-warn > but just ONCE) vs other cases where it could also come from session > state machine (which cannot be a WARN_ONCE since it can be triggered > due to runtime operation events). > > Let's use helpers for these so as more functions are added in future > features / HW (or as FW designers continue to bless upstreaming of > the error codes and meanings), we only need to update the helpers. > > NOTE: Don't completely remove FW errors (via drm_debug) or else cusomer > apps that really needs to know that content protection failed won't > be aware of it. > > v2: - Add fixes tag (Trvtko) > v3: - Break multi-line drm_dbg strings into separate drm_dbg (Daniele) > - Fix couple of typecasting nits (Daniele) > > Fixes: b762787bf767 ("drm/i915/pxp: Use drm_dbg if arb session failed due to fw version") > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > --- > .../i915/pxp/intel_pxp_cmd_interface_cmn.h | 3 + > drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 2 +- > drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 77 +++++++++++++++---- > 3 files changed, 67 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h > index ae9b151b7cb7..6f6541d5e49a 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h > @@ -18,6 +18,9 @@ > enum pxp_status { > PXP_STATUS_SUCCESS = 0x0, > PXP_STATUS_ERROR_API_VERSION = 0x1002, > + PXP_STATUS_NOT_READY = 0x100e, > + PXP_STATUS_PLATFCONFIG_KF1_NOVERIF = 0x101a, > + PXP_STATUS_PLATFCONFIG_KF1_BAD = 0x101f, > PXP_STATUS_OP_NOT_PERMITTED = 0x4013 > }; > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > index 448cacb0465d..7de849cb6c47 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > @@ -74,7 +74,7 @@ static int pxp_create_arb_session(struct intel_pxp *pxp) > > ret = pxp_wait_for_session_state(pxp, ARB_SESSION, true); > if (ret) { > - drm_err(>->i915->drm, "arb session failed to go in play\n"); > + drm_dbg(>->i915->drm, "arb session failed to go in play\n"); > return ret; > } > drm_dbg(>->i915->drm, "PXP ARB session is alive\n"); > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c > index d9d248b48093..6f89cd850251 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c > @@ -19,6 +19,37 @@ > #include "intel_pxp_tee.h" > #include "intel_pxp_types.h" > > +static bool > +is_fw_err_platform_config(u32 type) > +{ > + switch (type) { > + case PXP_STATUS_ERROR_API_VERSION: > + case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: > + case PXP_STATUS_PLATFCONFIG_KF1_BAD: > + return true; > + default: > + break; > + } > + return false; > +} > + > +static const char * > +fw_err_to_string(u32 type) > +{ > + switch (type) { > + case PXP_STATUS_ERROR_API_VERSION: > + return "ERR_API_VERSION"; > + case PXP_STATUS_NOT_READY: > + return "ERR_NOT_READY"; > + case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: > + case PXP_STATUS_PLATFCONFIG_KF1_BAD: > + return "ERR_PLATFORM_CONFIG"; > + default: > + break; > + } > + return NULL; > +} > + > static int intel_pxp_tee_io_message(struct intel_pxp *pxp, > void *msg_in, u32 msg_in_size, > void *msg_out, u32 msg_out_max_size, > @@ -307,15 +338,22 @@ int intel_pxp_tee_cmd_create_arb_session(struct intel_pxp *pxp, > &msg_out, sizeof(msg_out), > NULL); > > - if (ret) > - drm_err(&i915->drm, "Failed to send tee msg ret=[%d]\n", ret); > - else if (msg_out.header.status == PXP_STATUS_ERROR_API_VERSION) > - drm_dbg(&i915->drm, "PXP firmware version unsupported, requested: " > - "CMD-ID-[0x%08x] on API-Ver-[0x%08x]\n", > - msg_in.header.command_id, msg_in.header.api_version); > - else if (msg_out.header.status != 0x0) > - drm_warn(&i915->drm, "PXP firmware failed arb session init request ret=[0x%08x]\n", > - msg_out.header.status); > + 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)) { > + drm_WARN_ONCE(&i915->drm, true, > + "PXP init-arb-session-%d failed due to BIOS/SOC:0x%08x:%s\n", > + arb_session_id, msg_out.header.status, > + fw_err_to_string(msg_out.header.status)); > + } else { > + drm_dbg(&i915->drm, "PXP init-arb-session--%d failed 0x%08x:%st:\n", > + arb_session_id, msg_out.header.status, > + fw_err_to_string(msg_out.header.status)); > + drm_dbg(&i915->drm, " cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n", > + msg_in.header.command_id, msg_in.header.api_version); > + } > + } > > return ret; > } > @@ -347,10 +385,21 @@ void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id) > if ((ret || msg_out.header.status != 0x0) && ++trials < 3) > goto try_again; > > - if (ret) > - drm_err(&i915->drm, "Failed to send tee msg for inv-stream-key-%d, ret=[%d]\n", > + if (ret) { > + 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 != 0x0) > - drm_warn(&i915->drm, "PXP firmware failed inv-stream-key-%d with status 0x%08x\n", > - session_id, msg_out.header.status); > + } else if (msg_out.header.status != 0) { > + if (is_fw_err_platform_config(msg_out.header.status)) { > + drm_WARN_ONCE(&i915->drm, true, > + "PXP inv-stream-key-%u failed due to BIOS/SOC :0x%08x:%s\n", > + session_id, msg_out.header.status, > + fw_err_to_string(msg_out.header.status)); > + } else { > + drm_dbg(&i915->drm, "PXP inv-stream-key-%u failed 0x%08x:%s:\n", > + session_id, msg_out.header.status, > + fw_err_to_string(msg_out.header.status)); > + drm_dbg(&i915->drm, " cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n", > + msg_in.header.command_id, msg_in.header.api_version); > + } > + } > } > > base-commit: 07f982ec08c9bfc4aa82d98022111126cd638a0b
On 15/03/2023 09:16, Eero Tamminen wrote: > Hi, > > Tested the patch with Ubuntu 22.04 desktop + Linux 6.2-rc3 (drm-tip) > kernel, on TGL-H HW. > > With it, this log spam has disappeared: > ---------------------------------------------------- > [ 8691.608933] i915 0000:00:02.0: [drm] PXP firmware failed arb session > init request ret=[0x0000101f] > [ 8691.709442] i915 0000:00:02.0: [drm] *ERROR* arb session failed to go > in play > ---------------------------------------------------- > > There's now just a single message on boot: > ---------------------------------------------------- > [ 11.674177] ------------[ cut here ]------------ > [ 11.674183] i915 0000:00:02.0: PXP init-arb-session-15 failed due to > BIOS/SOC:0x0000101a:ERR_PLATFORM_CONFIG > [ 11.674222] WARNING: CPU: 12 PID: 123 at > drivers/gpu/drm/i915/pxp/intel_pxp_tee.c:345 > intel_pxp_tee_cmd_create_arb_session+0x19a/0x1e0 [i915] > ... > [ 11.674961] Call Trace: > [ 11.674966] <TASK> > [ 11.674970] ? pxp_session_work+0x123/0x1d0 [i915] > [ 11.675130] pxp_session_work+0x123/0x1d0 [i915] > [ 11.675286] process_one_work+0x1d9/0x3e0 > [ 11.675295] worker_thread+0x21b/0x3d0 > [ 11.675301] ? __pfx_worker_thread+0x10/0x10 > [ 11.675307] kthread+0xe9/0x110 > [ 11.675311] ? __pfx_kthread+0x10/0x10 > [ 11.675315] ret_from_fork+0x29/0x50 > [ 11.675323] </TASK> > [ 11.675325] ---[ end trace 0000000000000000 ]--- > ---------------------------------------------------- Alan - is this expected during normal operation on some parts, or it's something truly unexpected/unexplained? If the former then I think it would be good to downgrade away from drm_WARN so it is less scary. Commit message talks about "HW/platform gaps" - if it is like a missing BIOS support or so then I think WARN_ON is too much. Regards, Tvrtko > > > - Eero > > On 14.3.2023 19.58, Alan Previn wrote: >> MESA driver is creating protected context on every driver handle >> creation to query caps bits for app. So when running CI tests, >> they are observing hundreds of drm_errors when enabling PXP >> in .config but using SOC fusing or BIOS configuration that cannot >> support PXP sessions. >> >> The fixes tag referenced below was to resolve a related issue >> where we wanted to silence error messages, but that case was due >> to outdated IFWI (firmware) that definitely needed an upgrade and >> was, at that point, considered a one-off case as opposed to today's >> realization that default CI was enabling PXP in kernel config for >> all testing. >> >> So with this patch, let's strike a balance between issues that is >> critical but are root-caused from HW/platform gaps (louder drm-warn >> but just ONCE) vs other cases where it could also come from session >> state machine (which cannot be a WARN_ONCE since it can be triggered >> due to runtime operation events). >> >> Let's use helpers for these so as more functions are added in future >> features / HW (or as FW designers continue to bless upstreaming of >> the error codes and meanings), we only need to update the helpers. >> >> NOTE: Don't completely remove FW errors (via drm_debug) or else cusomer >> apps that really needs to know that content protection failed won't >> be aware of it. >> >> v2: - Add fixes tag (Trvtko) >> v3: - Break multi-line drm_dbg strings into separate drm_dbg (Daniele) >> - Fix couple of typecasting nits (Daniele) >> >> Fixes: b762787bf767 ("drm/i915/pxp: Use drm_dbg if arb session failed >> due to fw version") >> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> >> --- >> .../i915/pxp/intel_pxp_cmd_interface_cmn.h | 3 + >> drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 2 +- >> drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 77 +++++++++++++++---- >> 3 files changed, 67 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h >> b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h >> index ae9b151b7cb7..6f6541d5e49a 100644 >> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h >> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h >> @@ -18,6 +18,9 @@ >> enum pxp_status { >> PXP_STATUS_SUCCESS = 0x0, >> PXP_STATUS_ERROR_API_VERSION = 0x1002, >> + PXP_STATUS_NOT_READY = 0x100e, >> + PXP_STATUS_PLATFCONFIG_KF1_NOVERIF = 0x101a, >> + PXP_STATUS_PLATFCONFIG_KF1_BAD = 0x101f, >> PXP_STATUS_OP_NOT_PERMITTED = 0x4013 >> }; >> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c >> b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c >> index 448cacb0465d..7de849cb6c47 100644 >> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c >> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c >> @@ -74,7 +74,7 @@ static int pxp_create_arb_session(struct intel_pxp >> *pxp) >> ret = pxp_wait_for_session_state(pxp, ARB_SESSION, true); >> if (ret) { >> - drm_err(>->i915->drm, "arb session failed to go in play\n"); >> + drm_dbg(>->i915->drm, "arb session failed to go in play\n"); >> return ret; >> } >> drm_dbg(>->i915->drm, "PXP ARB session is alive\n"); >> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c >> b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c >> index d9d248b48093..6f89cd850251 100644 >> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c >> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c >> @@ -19,6 +19,37 @@ >> #include "intel_pxp_tee.h" >> #include "intel_pxp_types.h" >> +static bool >> +is_fw_err_platform_config(u32 type) >> +{ >> + switch (type) { >> + case PXP_STATUS_ERROR_API_VERSION: >> + case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: >> + case PXP_STATUS_PLATFCONFIG_KF1_BAD: >> + return true; >> + default: >> + break; >> + } >> + return false; >> +} >> + >> +static const char * >> +fw_err_to_string(u32 type) >> +{ >> + switch (type) { >> + case PXP_STATUS_ERROR_API_VERSION: >> + return "ERR_API_VERSION"; >> + case PXP_STATUS_NOT_READY: >> + return "ERR_NOT_READY"; >> + case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: >> + case PXP_STATUS_PLATFCONFIG_KF1_BAD: >> + return "ERR_PLATFORM_CONFIG"; >> + default: >> + break; >> + } >> + return NULL; >> +} >> + >> static int intel_pxp_tee_io_message(struct intel_pxp *pxp, >> void *msg_in, u32 msg_in_size, >> void *msg_out, u32 msg_out_max_size, >> @@ -307,15 +338,22 @@ int intel_pxp_tee_cmd_create_arb_session(struct >> intel_pxp *pxp, >> &msg_out, sizeof(msg_out), >> NULL); >> - if (ret) >> - drm_err(&i915->drm, "Failed to send tee msg ret=[%d]\n", ret); >> - else if (msg_out.header.status == PXP_STATUS_ERROR_API_VERSION) >> - drm_dbg(&i915->drm, "PXP firmware version unsupported, >> requested: " >> - "CMD-ID-[0x%08x] on API-Ver-[0x%08x]\n", >> - msg_in.header.command_id, msg_in.header.api_version); >> - else if (msg_out.header.status != 0x0) >> - drm_warn(&i915->drm, "PXP firmware failed arb session init >> request ret=[0x%08x]\n", >> - msg_out.header.status); >> + 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)) { >> + drm_WARN_ONCE(&i915->drm, true, >> + "PXP init-arb-session-%d failed due to >> BIOS/SOC:0x%08x:%s\n", >> + arb_session_id, msg_out.header.status, >> + fw_err_to_string(msg_out.header.status)); >> + } else { >> + drm_dbg(&i915->drm, "PXP init-arb-session--%d failed >> 0x%08x:%st:\n", >> + arb_session_id, msg_out.header.status, >> + fw_err_to_string(msg_out.header.status)); >> + drm_dbg(&i915->drm, " cmd-detail: >> ID=[0x%08x],API-Ver-[0x%08x]\n", >> + msg_in.header.command_id, msg_in.header.api_version); >> + } >> + } >> return ret; >> } >> @@ -347,10 +385,21 @@ void intel_pxp_tee_end_arb_fw_session(struct >> intel_pxp *pxp, u32 session_id) >> if ((ret || msg_out.header.status != 0x0) && ++trials < 3) >> goto try_again; >> - if (ret) >> - drm_err(&i915->drm, "Failed to send tee msg for >> inv-stream-key-%d, ret=[%d]\n", >> + if (ret) { >> + 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 != 0x0) >> - drm_warn(&i915->drm, "PXP firmware failed inv-stream-key-%d >> with status 0x%08x\n", >> - session_id, msg_out.header.status); >> + } else if (msg_out.header.status != 0) { >> + if (is_fw_err_platform_config(msg_out.header.status)) { >> + drm_WARN_ONCE(&i915->drm, true, >> + "PXP inv-stream-key-%u failed due to BIOS/SOC >> :0x%08x:%s\n", >> + session_id, msg_out.header.status, >> + fw_err_to_string(msg_out.header.status)); >> + } else { >> + drm_dbg(&i915->drm, "PXP inv-stream-key-%u failed >> 0x%08x:%s:\n", >> + session_id, msg_out.header.status, >> + fw_err_to_string(msg_out.header.status)); >> + drm_dbg(&i915->drm, " cmd-detail: >> ID=[0x%08x],API-Ver-[0x%08x]\n", >> + msg_in.header.command_id, msg_in.header.api_version); >> + } >> + } >> } >> >> base-commit: 07f982ec08c9bfc4aa82d98022111126cd638a0b
Hi, On 16.3.2023 10.50, Tvrtko Ursulin wrote: >> [ 11.674183] i915 0000:00:02.0: PXP init-arb-session-15 failed due >> to BIOS/SOC:0x0000101a:ERR_PLATFORM_CONFIG ... > Alan - is this expected during normal operation on some parts, or it's > something truly unexpected/unexplained? If the former then I think it > would be good to downgrade away from drm_WARN so it is less scary. > > Commit message talks about "HW/platform gaps" - if it is like a missing > BIOS support or so then I think WARN_ON is too much. Note that this was on pre-production TGL-H HW with BIOS from April 2021. (I don't know where to get update, nor interested to update it.) - Eero >> On 14.3.2023 19.58, Alan Previn wrote: >>> MESA driver is creating protected context on every driver handle >>> creation to query caps bits for app. So when running CI tests, >>> they are observing hundreds of drm_errors when enabling PXP >>> in .config but using SOC fusing or BIOS configuration that cannot >>> support PXP sessions. >>> >>> The fixes tag referenced below was to resolve a related issue >>> where we wanted to silence error messages, but that case was due >>> to outdated IFWI (firmware) that definitely needed an upgrade and >>> was, at that point, considered a one-off case as opposed to today's >>> realization that default CI was enabling PXP in kernel config for >>> all testing. >>> >>> So with this patch, let's strike a balance between issues that is >>> critical but are root-caused from HW/platform gaps (louder drm-warn >>> but just ONCE) vs other cases where it could also come from session >>> state machine (which cannot be a WARN_ONCE since it can be triggered >>> due to runtime operation events). >>> >>> Let's use helpers for these so as more functions are added in future >>> features / HW (or as FW designers continue to bless upstreaming of >>> the error codes and meanings), we only need to update the helpers. >>> >>> NOTE: Don't completely remove FW errors (via drm_debug) or else cusomer >>> apps that really needs to know that content protection failed won't >>> be aware of it. >>> >>> v2: - Add fixes tag (Trvtko) >>> v3: - Break multi-line drm_dbg strings into separate drm_dbg (Daniele) >>> - Fix couple of typecasting nits (Daniele) >>> >>> Fixes: b762787bf767 ("drm/i915/pxp: Use drm_dbg if arb session failed >>> due to fw version") >>> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> >>> --- >>> .../i915/pxp/intel_pxp_cmd_interface_cmn.h | 3 + >>> drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 2 +- >>> drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 77 +++++++++++++++---- >>> 3 files changed, 67 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h >>> b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h >>> index ae9b151b7cb7..6f6541d5e49a 100644 >>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h >>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h >>> @@ -18,6 +18,9 @@ >>> enum pxp_status { >>> PXP_STATUS_SUCCESS = 0x0, >>> PXP_STATUS_ERROR_API_VERSION = 0x1002, >>> + PXP_STATUS_NOT_READY = 0x100e, >>> + PXP_STATUS_PLATFCONFIG_KF1_NOVERIF = 0x101a, >>> + PXP_STATUS_PLATFCONFIG_KF1_BAD = 0x101f, >>> PXP_STATUS_OP_NOT_PERMITTED = 0x4013 >>> }; >>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c >>> b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c >>> index 448cacb0465d..7de849cb6c47 100644 >>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c >>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c >>> @@ -74,7 +74,7 @@ static int pxp_create_arb_session(struct intel_pxp >>> *pxp) >>> ret = pxp_wait_for_session_state(pxp, ARB_SESSION, true); >>> if (ret) { >>> - drm_err(>->i915->drm, "arb session failed to go in play\n"); >>> + drm_dbg(>->i915->drm, "arb session failed to go in play\n"); >>> return ret; >>> } >>> drm_dbg(>->i915->drm, "PXP ARB session is alive\n"); >>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c >>> b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c >>> index d9d248b48093..6f89cd850251 100644 >>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c >>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c >>> @@ -19,6 +19,37 @@ >>> #include "intel_pxp_tee.h" >>> #include "intel_pxp_types.h" >>> +static bool >>> +is_fw_err_platform_config(u32 type) >>> +{ >>> + switch (type) { >>> + case PXP_STATUS_ERROR_API_VERSION: >>> + case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: >>> + case PXP_STATUS_PLATFCONFIG_KF1_BAD: >>> + return true; >>> + default: >>> + break; >>> + } >>> + return false; >>> +} >>> + >>> +static const char * >>> +fw_err_to_string(u32 type) >>> +{ >>> + switch (type) { >>> + case PXP_STATUS_ERROR_API_VERSION: >>> + return "ERR_API_VERSION"; >>> + case PXP_STATUS_NOT_READY: >>> + return "ERR_NOT_READY"; >>> + case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: >>> + case PXP_STATUS_PLATFCONFIG_KF1_BAD: >>> + return "ERR_PLATFORM_CONFIG"; >>> + default: >>> + break; >>> + } >>> + return NULL; >>> +} >>> + >>> static int intel_pxp_tee_io_message(struct intel_pxp *pxp, >>> void *msg_in, u32 msg_in_size, >>> void *msg_out, u32 msg_out_max_size, >>> @@ -307,15 +338,22 @@ int intel_pxp_tee_cmd_create_arb_session(struct >>> intel_pxp *pxp, >>> &msg_out, sizeof(msg_out), >>> NULL); >>> - if (ret) >>> - drm_err(&i915->drm, "Failed to send tee msg ret=[%d]\n", ret); >>> - else if (msg_out.header.status == PXP_STATUS_ERROR_API_VERSION) >>> - drm_dbg(&i915->drm, "PXP firmware version unsupported, >>> requested: " >>> - "CMD-ID-[0x%08x] on API-Ver-[0x%08x]\n", >>> - msg_in.header.command_id, msg_in.header.api_version); >>> - else if (msg_out.header.status != 0x0) >>> - drm_warn(&i915->drm, "PXP firmware failed arb session init >>> request ret=[0x%08x]\n", >>> - msg_out.header.status); >>> + 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)) { >>> + drm_WARN_ONCE(&i915->drm, true, >>> + "PXP init-arb-session-%d failed due to >>> BIOS/SOC:0x%08x:%s\n", >>> + arb_session_id, msg_out.header.status, >>> + fw_err_to_string(msg_out.header.status)); >>> + } else { >>> + drm_dbg(&i915->drm, "PXP init-arb-session--%d failed >>> 0x%08x:%st:\n", >>> + arb_session_id, msg_out.header.status, >>> + fw_err_to_string(msg_out.header.status)); >>> + drm_dbg(&i915->drm, " cmd-detail: >>> ID=[0x%08x],API-Ver-[0x%08x]\n", >>> + msg_in.header.command_id, msg_in.header.api_version); >>> + } >>> + } >>> return ret; >>> } >>> @@ -347,10 +385,21 @@ void intel_pxp_tee_end_arb_fw_session(struct >>> intel_pxp *pxp, u32 session_id) >>> if ((ret || msg_out.header.status != 0x0) && ++trials < 3) >>> goto try_again; >>> - if (ret) >>> - drm_err(&i915->drm, "Failed to send tee msg for >>> inv-stream-key-%d, ret=[%d]\n", >>> + if (ret) { >>> + 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 != 0x0) >>> - drm_warn(&i915->drm, "PXP firmware failed inv-stream-key-%d >>> with status 0x%08x\n", >>> - session_id, msg_out.header.status); >>> + } else if (msg_out.header.status != 0) { >>> + if (is_fw_err_platform_config(msg_out.header.status)) { >>> + drm_WARN_ONCE(&i915->drm, true, >>> + "PXP inv-stream-key-%u failed due to BIOS/SOC >>> :0x%08x:%s\n", >>> + session_id, msg_out.header.status, >>> + fw_err_to_string(msg_out.header.status)); >>> + } else { >>> + drm_dbg(&i915->drm, "PXP inv-stream-key-%u failed >>> 0x%08x:%s:\n", >>> + session_id, msg_out.header.status, >>> + fw_err_to_string(msg_out.header.status)); >>> + drm_dbg(&i915->drm, " cmd-detail: >>> ID=[0x%08x],API-Ver-[0x%08x]\n", >>> + msg_in.header.command_id, msg_in.header.api_version); >>> + } >>> + } >>> } >>> >>> base-commit: 07f982ec08c9bfc4aa82d98022111126cd638a0b
On Fri, 2023-03-17 at 13:37 +0200, Tamminen, Eero T wrote: > Hi, > > On 16.3.2023 10.50, Tvrtko Ursulin wrote: > > > [ 11.674183] i915 0000:00:02.0: PXP init-arb-session-15 failed due > > > to BIOS/SOC:0x0000101a:ERR_PLATFORM_CONFIG > ... > > Alan - is this expected during normal operation on some parts, or it's > > something truly unexpected/unexplained? If the former then I think it > > would be good to downgrade away from drm_WARN so it is less scary. > > > > Commit message talks about "HW/platform gaps" - if it is like a missing > > BIOS support or so then I think WARN_ON is too much. > > Note that this was on pre-production TGL-H HW with BIOS from April 2021. > > (I don't know where to get update, nor interested to update it.) > > > - Eero Alan: Hi Tvrtko, thanks for the feedback -i shall change from WARN_ONCE to drm_info_once. Alan: snip
On 23/03/2023 00:27, Teres Alexis, Alan Previn wrote: > On Fri, 2023-03-17 at 13:37 +0200, Tamminen, Eero T wrote: >> Hi, >> >> On 16.3.2023 10.50, Tvrtko Ursulin wrote: >>>> [ 11.674183] i915 0000:00:02.0: PXP init-arb-session-15 failed due >>>> to BIOS/SOC:0x0000101a:ERR_PLATFORM_CONFIG >> ... >>> Alan - is this expected during normal operation on some parts, or it's >>> something truly unexpected/unexplained? If the former then I think it >>> would be good to downgrade away from drm_WARN so it is less scary. >>> >>> Commit message talks about "HW/platform gaps" - if it is like a missing >>> BIOS support or so then I think WARN_ON is too much. >> >> Note that this was on pre-production TGL-H HW with BIOS from April 2021. >> >> (I don't know where to get update, nor interested to update it.) >> >> >> - Eero > > Alan: Hi Tvrtko, thanks for the feedback -i shall change from WARN_ONCE to drm_info_once. Maybe it deserves to be a warning? Or a notice? I was just thinking it does not need a call trace and all since it is not a driver issue. Your call on the level and whether or not there is any chance for it to happen in the field to make the discussion relevant or not. Regards, Tvrtko
On Thu, 2023-03-23 at 08:35 +0000, Tvrtko Ursulin wrote:
On 23/03/2023 00:27, Teres Alexis, Alan Previn wrote:
On Fri, 2023-03-17 at 13:37 +0200, Tamminen, Eero T wrote:
Hi,
On 16.3.2023 10.50, Tvrtko Ursulin wrote:
[ 11.674183] i915 0000:00:02.0: PXP init-arb-session-15 failed due
to BIOS/SOC:0x0000101a:ERR_PLATFORM_CONFIG
...
Alan - is this expected during normal operation on some parts, or it's
something truly unexpected/unexplained? If the former then I think it
would be good to downgrade away from drm_WARN so it is less scary.
Commit message talks about "HW/platform gaps" - if it is like a missing
BIOS support or so then I think WARN_ON is too much.
Note that this was on pre-production TGL-H HW with BIOS from April 2021.
(I don't know where to get update, nor interested to update it.)
- Eero
Alan: Hi Tvrtko, thanks for the feedback -i shall change from WARN_ONCE to drm_info_once.
Maybe it deserves to be a warning? Or a notice? I was just thinking it
does not need a call trace and all since it is not a driver issue. Your
call on the level and whether or not there is any chance for it to
happen in the field to make the discussion relevant or not.
I agree we don't need a call trace. I can't say how often this can happen in the field as it would depend on
when the platform was purchased, whether the owner had been updating the firwmware releases and whether
the bios settings were configured correctly. I'm okay with the drm_info_once - it could be ignored which is fine.
If customers seeing protected rendering/decoding fail when they actually DO need it, I am expecting they will
provide the dmesg that shows above new message at compositor startup (mesa attempting to establish caps)
along with more serious complaints such as their application getting a failure when attempting to create a
protected context via I915 ioctl (which aligns with existing PXP UAPI behavior).
Any chance for an r-b? :)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h index ae9b151b7cb7..6f6541d5e49a 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h @@ -18,6 +18,9 @@ enum pxp_status { PXP_STATUS_SUCCESS = 0x0, PXP_STATUS_ERROR_API_VERSION = 0x1002, + PXP_STATUS_NOT_READY = 0x100e, + PXP_STATUS_PLATFCONFIG_KF1_NOVERIF = 0x101a, + PXP_STATUS_PLATFCONFIG_KF1_BAD = 0x101f, PXP_STATUS_OP_NOT_PERMITTED = 0x4013 }; diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c index 448cacb0465d..7de849cb6c47 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c @@ -74,7 +74,7 @@ static int pxp_create_arb_session(struct intel_pxp *pxp) ret = pxp_wait_for_session_state(pxp, ARB_SESSION, true); if (ret) { - drm_err(>->i915->drm, "arb session failed to go in play\n"); + drm_dbg(>->i915->drm, "arb session failed to go in play\n"); return ret; } drm_dbg(>->i915->drm, "PXP ARB session is alive\n"); diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c index d9d248b48093..6f89cd850251 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c @@ -19,6 +19,37 @@ #include "intel_pxp_tee.h" #include "intel_pxp_types.h" +static bool +is_fw_err_platform_config(u32 type) +{ + switch (type) { + case PXP_STATUS_ERROR_API_VERSION: + case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: + case PXP_STATUS_PLATFCONFIG_KF1_BAD: + return true; + default: + break; + } + return false; +} + +static const char * +fw_err_to_string(u32 type) +{ + switch (type) { + case PXP_STATUS_ERROR_API_VERSION: + return "ERR_API_VERSION"; + case PXP_STATUS_NOT_READY: + return "ERR_NOT_READY"; + case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: + case PXP_STATUS_PLATFCONFIG_KF1_BAD: + return "ERR_PLATFORM_CONFIG"; + default: + break; + } + return NULL; +} + static int intel_pxp_tee_io_message(struct intel_pxp *pxp, void *msg_in, u32 msg_in_size, void *msg_out, u32 msg_out_max_size, @@ -307,15 +338,22 @@ int intel_pxp_tee_cmd_create_arb_session(struct intel_pxp *pxp, &msg_out, sizeof(msg_out), NULL); - if (ret) - drm_err(&i915->drm, "Failed to send tee msg ret=[%d]\n", ret); - else if (msg_out.header.status == PXP_STATUS_ERROR_API_VERSION) - drm_dbg(&i915->drm, "PXP firmware version unsupported, requested: " - "CMD-ID-[0x%08x] on API-Ver-[0x%08x]\n", - msg_in.header.command_id, msg_in.header.api_version); - else if (msg_out.header.status != 0x0) - drm_warn(&i915->drm, "PXP firmware failed arb session init request ret=[0x%08x]\n", - msg_out.header.status); + 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)) { + drm_WARN_ONCE(&i915->drm, true, + "PXP init-arb-session-%d failed due to BIOS/SOC:0x%08x:%s\n", + arb_session_id, msg_out.header.status, + fw_err_to_string(msg_out.header.status)); + } else { + drm_dbg(&i915->drm, "PXP init-arb-session--%d failed 0x%08x:%st:\n", + arb_session_id, msg_out.header.status, + fw_err_to_string(msg_out.header.status)); + drm_dbg(&i915->drm, " cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n", + msg_in.header.command_id, msg_in.header.api_version); + } + } return ret; } @@ -347,10 +385,21 @@ void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id) if ((ret || msg_out.header.status != 0x0) && ++trials < 3) goto try_again; - if (ret) - drm_err(&i915->drm, "Failed to send tee msg for inv-stream-key-%d, ret=[%d]\n", + if (ret) { + 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 != 0x0) - drm_warn(&i915->drm, "PXP firmware failed inv-stream-key-%d with status 0x%08x\n", - session_id, msg_out.header.status); + } else if (msg_out.header.status != 0) { + if (is_fw_err_platform_config(msg_out.header.status)) { + drm_WARN_ONCE(&i915->drm, true, + "PXP inv-stream-key-%u failed due to BIOS/SOC :0x%08x:%s\n", + session_id, msg_out.header.status, + fw_err_to_string(msg_out.header.status)); + } else { + drm_dbg(&i915->drm, "PXP inv-stream-key-%u failed 0x%08x:%s:\n", + session_id, msg_out.header.status, + fw_err_to_string(msg_out.header.status)); + drm_dbg(&i915->drm, " cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n", + msg_in.header.command_id, msg_in.header.api_version); + } + } }
MESA driver is creating protected context on every driver handle creation to query caps bits for app. So when running CI tests, they are observing hundreds of drm_errors when enabling PXP in .config but using SOC fusing or BIOS configuration that cannot support PXP sessions. The fixes tag referenced below was to resolve a related issue where we wanted to silence error messages, but that case was due to outdated IFWI (firmware) that definitely needed an upgrade and was, at that point, considered a one-off case as opposed to today's realization that default CI was enabling PXP in kernel config for all testing. So with this patch, let's strike a balance between issues that is critical but are root-caused from HW/platform gaps (louder drm-warn but just ONCE) vs other cases where it could also come from session state machine (which cannot be a WARN_ONCE since it can be triggered due to runtime operation events). Let's use helpers for these so as more functions are added in future features / HW (or as FW designers continue to bless upstreaming of the error codes and meanings), we only need to update the helpers. NOTE: Don't completely remove FW errors (via drm_debug) or else cusomer apps that really needs to know that content protection failed won't be aware of it. v2: - Add fixes tag (Trvtko) v3: - Break multi-line drm_dbg strings into separate drm_dbg (Daniele) - Fix couple of typecasting nits (Daniele) Fixes: b762787bf767 ("drm/i915/pxp: Use drm_dbg if arb session failed due to fw version") Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> --- .../i915/pxp/intel_pxp_cmd_interface_cmn.h | 3 + drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 2 +- drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 77 +++++++++++++++---- 3 files changed, 67 insertions(+), 15 deletions(-) base-commit: 07f982ec08c9bfc4aa82d98022111126cd638a0b