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

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Aug 07, 2026 at 08:00:40AM -0600, Ruhl, Michael J wrote:
> >-----Original Message-----
> >From: Brost, Matthew <matthew.brost@intel.com>
> >Sent: Thursday, August 6, 2026 5:50 PM
> >To: Ruhl, Michael J <michael.j.ruhl@intel.com>
> >Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org;
> >hansg@kernel.org; ilpo.jarvinen@linux.intel.com; Vivi, Rodrigo
> ><rodrigo.vivi@intel.com>; thomas.hellstrom@linux.intel.com;
> >airlied@gmail.com; simona@ffwll.ch; david.e.box@linux.intel.com; Vijay,
> >Anoop C <anoop.c.vijay@intel.com>; Nilawar, Badal
> ><badal.nilawar@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>;
> >Ausmus, James <james.ausmus@intel.com>
> >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 <michael.j.ruhl@intel.com>
> >> ---
> >>  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);

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

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