Re: [PATCH v3 2/2] drm/i915/pxp: Do not support PXP if CSME is not available

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 25 Nov 2025, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote:
> On 11/25/2025 2:28 AM, Jani Nikula wrote:
>> On Fri, 14 Nov 2025, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> 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 <valentine.burley@collabora.com>
>>> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14516
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: Alexander Usyskin <alexander.usyskin@intel.com>
>>> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
>>> ---
>>>   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