Re: [Intel-gfx] [PATCH v3] drm/i915/pxp: limit drm-errors or warning on firmware API failures

[Eero Tamminen <eero.t.tamminen@intel.com>]

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(&gt->i915->drm, "arb session failed to go in play\n");
>>> +        drm_dbg(&gt->i915->drm, "arb session failed to go in play\n");
>>>           return ret;
>>>       }
>>>       drm_dbg(&gt->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