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

>-----Original Message-----
>From: Brost, Matthew <[email protected]>
>Sent: Friday, August 7, 2026 3:38 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 Fri, Aug 07, 2026 at 12:22:19PM -0700, Matthew Brost wrote:
>> On Fri, Aug 07, 2026 at 08:00:40AM -0600, Ruhl, Michael J wrote:
>> > >-----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?
>> >
>>
>> Yes, it would be same as:
>>
>> xe_pm_runtime_get(xe);
>> mutux_lock(&xe->pmt.lock);
>> /* Do something *.
>> mutux_unlock(&xe->pmt.lock);
>> xe_pm_runtime_put(xe);

Got it.  I will update.

>> >
>> > >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).
>> >
>>
>> If Xe gets hotplugged nothing is going to save you here either, you to
>> some extent you must deal errors at the caller, more below.
>>
>> > Do you have some thoughts on the correct sequencing here?
>>
>> I'd at least swap the order as suggested so that the Xe code isn't
>> internally waking the device while holding locks, which goes against our
>> PM rules.
>>
>> I'd also consider adding internal hotplug protection, unless the caller
>> already provides it. I don't really know what the PMT code is doing
>> here, so it's possible that this path is already protected against
>> hotplug events.
>>
>> So:
>>
>> bound = drm_dev_enter(&xe->drm, idx);
>> if (!bound)
>
>Sorry typo with polarity inverted: s/if (!bound)/if (bound) {
>
>> 	xe_pm_runtime_get(xe);
>> 	mutux_lock(&xe->pmt.lock);
>>
>> 	/* Do something */
>>
>> 	mutux_unlock(&xe->pmt.lock);
>> 	xe_pm_runtime_put(xe);
>>
>> 	drm_dev_exit(idx);
>> } else {
>> 	/* Device is already unplugged, caller has to deal with this */
>> 	return some_error;
>> }
>>
>> If hotplug protection is needed here, then xe_pmt_telem_read should have
>> this too.

Hmm, this is "new" stuff for me, so I will explore this some more and then
probably follow what you have done here. 😊

Thanks!

Mike

>> Matt
>>
>> >
>> > 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
>> > >>