On Tue, 25 Nov 2025, Daniele Ceraolo Spurio <[email protected]> wrote: > On 11/25/2025 2:28 AM, Jani Nikula wrote: >> On Fri, 14 Nov 2025, Daniele Ceraolo Spurio >> <[email protected]> wrote: >>> The PXP flow requires us to communicate with CSME, which we do via a >>> mei component. Since the mei component binding is async and can take >>> a bit to complete, we don't wait for it during i915 load. If userspace >>> queries the state before the async binding is complete, we return an >>> "init in progress" state, with the expectation that it will eventually >>> transition to "init complete" if the CSME device is functional. >>> >>> Mesa CI is flashing a custom coreboot on their Chromebooks that hides >>> the CSME device, which means that we never transition to the "init >>> complete" state. While from an interface POV it is not incorrect to not >>> end up in "init complete" if the CSME is missing, we can mitigate the >>> impact of this by simply checking if the CSME device is available at >>> all before attempting to initialize PXP. >>> >>> v2: rebase on updated mei patch. >>> v3: rebase on exported pci id list. >>> >>> Reported-by: Valentine Burley <[email protected]> >>> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14516 >>> Signed-off-by: Daniele Ceraolo Spurio <[email protected]> >>> Cc: Rodrigo Vivi <[email protected]> >>> Cc: Alexander Usyskin <[email protected]> >>> Cc: Alan Previn <[email protected]> >>> --- >>> drivers/gpu/drm/i915/pxp/intel_pxp.c | 25 +++++++++++++++++++++++++ >>> 1 file changed, 25 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c >>> b/drivers/gpu/drm/i915/pxp/intel_pxp.c >>> index 2860474ad2d0..26e7c5c26bac 100644 >>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c >>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c >>> @@ -3,6 +3,7 @@ >>> * Copyright(c) 2020 Intel Corporation. >>> */ >>> >>> +#include <linux/mei_me.h> >>> #include <linux/workqueue.h> >>> >>> #include <drm/drm_print.h> >>> @@ -197,6 +198,15 @@ static struct intel_gt >>> *find_gt_for_required_protected_content(struct drm_i915_p >>> return NULL; >>> } >>> >>> +static bool mei_device_available(void) >>> +{ >>> +#if IS_ENABLED(CONFIG_INTEL_MEI_ME) >>> + return pci_dev_present(mei_me_pci_tbl); >>> +#else >>> + return false; >>> +#endif >>> +} >>> + >> I think it's just ugly to have this in i915. IMO the function belongs in >> mei. > > That is what I had in v1 [1], but there were concerns from Greg about > the state changing after we check it and the relevant required locking > to avoid that. In i915 we don't care if the state changes after we check > it and therefore we don't need locking, so I moved the actual check to > i915 with an explanation (see comment in the code below).
I'm not sure how it makes a difference where the check is. I just think the whole mei_device_available() function above is a bunch of mei implementation details that i915 should not have to know. i915 shouldn't have to do any CONFIG_INTEL_MEI_ME stuff, i915 shouldn't have to know mei_me_pci_tbl or what it means. BR, Jani. > > [1] > https://patchwork.freedesktop.org/patch/664208/?series=151677&rev=1 > > Daniele > >> >> BR, >> Jani. >> >>> int intel_pxp_init(struct drm_i915_private *i915) >>> { >>> struct intel_gt *gt; >>> @@ -205,6 +215,21 @@ int intel_pxp_init(struct drm_i915_private *i915) >>> if (intel_gt_is_wedged(to_gt(i915))) >>> return -ENOTCONN; >>> >>> + /* >>> + * iGPUs require CSME to be available to use PXP. Note that the >>> + * availability of CSME might change after we check, but we only support >>> + * PXP in the case where the CSME device is available from boot and >>> + * stays that way. If CSME was not initially available and appears later >>> + * we'll just continue without PXP, while if it was available and is >>> + * then removed we'll catch it via the component unbind callback and >>> + * handle it gracefully. Therefore, we don't require any locking around >>> + * the state checking. >>> + */ >>> + if (!IS_DGFX(i915) && !mei_device_available()) { >>> + drm_dbg(&i915->drm, "skipping PXP init due to missing ME >>> device\n"); >>> + return -ENODEV; >>> + } >>> + >>> /* >>> * NOTE: Get the ctrl_gt before checking intel_pxp_is_supported since >>> * we still need it if PXP's backend tee transport is needed. > -- Jani Nikula, Intel
