RE: [PATCH 6/7] drm/xe/vsec: Crescent Island PMT callbacks

>-----Original Message-----
>From: Brost, Matthew <[email protected]>
>Sent: Thursday, August 6, 2026 5:50 PM
>To: Ruhl, Michael J <[email protected]>
>Cc: [email protected]; [email protected];
>[email protected]; [email protected]; Vivi, Rodrigo
><[email protected]>; [email protected];
>[email protected]; [email protected]; [email protected]; Vijay,
>Anoop C <[email protected]>; Nilawar, Badal
><[email protected]>; Roper, Matthew D <[email protected]>;
>Ausmus, James <[email protected]>
>Subject: Re: [PATCH 6/7] drm/xe/vsec: Crescent Island PMT callbacks
>
>On Thu, Aug 06, 2026 at 06:58:26AM -0700, Michael J. Ruhl wrote:
>> CRI PMT support requires callbacks to access the discovery status
>> and control areas.  Access is a common MMIO area that requires an
>> index to be set before access is allowed.
>>
>> Introduce the necessary callbacks to get the status and control
>> information for CRI PMT usage.
>>
>> Add the glue logic to register the CRI PMT functionality.
>>
>> Signed-off-by: Michael J. Ruhl <[email protected]>
>> ---
>>  drivers/gpu/drm/xe/xe_vsec.c | 85
>++++++++++++++++++++++++++++++++++--
>>  1 file changed, 82 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
>> index 3849f99c1c91..b84ec9088de7 100644
>> --- a/drivers/gpu/drm/xe/xe_vsec.c
>> +++ b/drivers/gpu/drm/xe/xe_vsec.c
>> @@ -320,17 +320,90 @@ int xe_pmt_telem_read(struct device *dev, u32
>guid, u64 *data, loff_t user_offse
>>  	return count;
>>  }
>>
>> -static struct pmt_callbacks xe_pmt_cb = {
>> +/**
>> + * xe_pmt_read_reg() - read a crashlog register
>> + * @dev: the xe device that registered the callback
>> + * @guid: PMT guid of the crashlog instance
>> + * @reg: data read from the PMT data structure
>> + * @offset: which data to read from the PMT data structure
>> + *
>> + * Read the requested PMT register based on the pcie device and guid.  The
>> + * supported struct is the Crashlog Type1 Version2.
>> + *
>> + * Currently this is for CRI only.
>> + */
>> +static int xe_pmt_read_reg(struct device *dev, u32 guid, u32 *reg, u32
>offset)
>> +{
>> +	struct xe_device *xe = kdev_to_xe_device(dev);
>> +	void __iomem *disc_addr = xe->mmio.regs;
>> +	u32 inst;
>> +
>> +	if (FIELD_GET(GUID_DEVICE_ID, guid) != CRI_DEVICE_ID ||
>> +	    FIELD_GET(GUID_CAP_TYPE, guid) != CRASHLOG)
>> +		return -EINVAL;
>> +
>> +	inst = FIELD_GET(GUID_RECORD_ID, guid) == PUNIT ?
>> +		CRI_CRASHLOG_PUNIT_DISC_OFFSET :
>CRI_CRASHLOG_OOBMSM_DISC_OFFSET;
>> +	disc_addr += CRI_DISCOVERY_OFFSET + inst + offset;
>> +
>> +	guard(mutex)(&xe->pmt.lock);
>> +
>> +	xe_pm_runtime_get(xe);
>
>We have guard(xe_pm_runtime)(xe), and this should also be the
>outermost construct (i.e., do not use xe_pm_runtime_get(), as it can
>wake the device, while other locks are held).

Hi Matt,

I am using the _get() routine because the device MUST be on for me to access the
registers.

Does the guard(xe_pm_runtime)(xe) turn the device on?


>So this should either be:
>
>guard(xe_pm_runtime)(xe);
>guard(mutex)(&xe->pmt.lock);

Ok, this makes sense.

>Or, like the other in-tree usage with xe->pmt.lock in
>xe_pmt_telem_read(), which calls xe_pm_runtime_get_if_active() while
>holding the lock. This is fine because xe_pm_runtime_get_if_active()
>cannot wake the device. This is most likely the correct choice, given
>that this is a vfunc called by a different driver, and we have no way of
>knowing whether that driver is holding locks that could create
>problematic lock dependency chains if we wake the Xe device here.

For telemetry, the data read should NOT happen if the device is not active (so the get_if_active usage)

For Crashlog, I have to get the data and have to make sure the device is enabled....(see patch 3).

Do you have some thoughts on the correct sequencing here?

Thanks,

M

>Matt
>
>> +
>> +	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
>> +
>> +	memcpy_fromio(reg, disc_addr, sizeof(*reg));
>> +
>> +	xe_pm_runtime_put(xe);
>> +
>> +	return 0;
>> +}
>> +
>> +static int xe_pmt_write_reg(struct device *dev, u32 guid, u32 reg, u32
>offset)
>> +{
>> +	struct xe_device *xe = kdev_to_xe_device(dev);
>> +	void __iomem *disc_addr = xe->mmio.regs;
>> +	u32 inst;
>> +
>> +	if (FIELD_GET(GUID_DEVICE_ID, guid) != CRI_DEVICE_ID ||
>> +	    FIELD_GET(GUID_CAP_TYPE, guid) != CRASHLOG)
>> +		return -EINVAL;
>> +
>> +	inst = FIELD_GET(GUID_RECORD_ID, guid) == PUNIT ?
>> +		CRI_CRASHLOG_PUNIT_DISC_OFFSET :
>CRI_CRASHLOG_OOBMSM_DISC_OFFSET;
>> +	disc_addr += CRI_DISCOVERY_OFFSET + inst + offset;
>> +
>> +	guard(mutex)(&xe->pmt.lock);
>> +
>> +	xe_pm_runtime_get(xe);
>> +
>> +	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
>> +
>> +	memcpy_toio(disc_addr, &reg, sizeof(reg));
>> +
>> +	xe_pm_runtime_put(xe);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct pmt_callbacks xe_bmg_pmt_cb = {
>> +	.read_telem = xe_pmt_telem_read,
>> +};
>> +
>> +static struct pmt_callbacks xe_cri_pmt_cb = {
>>  	.read_telem = xe_pmt_telem_read,
>> +	.read_reg = xe_pmt_read_reg,
>> +	.write_reg = xe_pmt_write_reg,
>>  };
>>
>>  static const int vsec_platforms[] = {
>>  	[XE_BATTLEMAGE] = XE_VSEC_BMG,
>> +	[XE_CRESCENTISLAND] = XE_VSEC_CRI,
>>  };
>>
>>  static enum xe_vsec get_platform_info(struct xe_device *xe)
>>  {
>> -	if (xe->info.platform > XE_BATTLEMAGE)
>> +	if (xe->info.platform > XE_CRESCENTISLAND)
>>  		return XE_VSEC_UNKNOWN;
>>
>>  	return vsec_platforms[xe->info.platform];
>> @@ -357,8 +430,14 @@ void xe_vsec_init(struct xe_device *xe)
>>
>>  	switch (platform) {
>>  	case XE_VSEC_BMG:
>> -		info->priv_data = &xe_pmt_cb;
>> +		info->priv_data = &xe_bmg_pmt_cb;
>>  		break;
>> +
>> +	case XE_VSEC_CRI:
>> +		info->priv_data = &xe_cri_pmt_cb;
>> +		xe->soc_remapper.set_telem_region(xe,
>CRI_IDX_TELEM_DISCOVERY);
>> +		break;
>> +
>>  	default:
>>  		break;
>>  	}
>> --
>> 2.43.0
>>