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