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

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

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).

[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.