Re: [PATCH v11 17/20] drm/xe/vsec: Support late bind fw information

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Sep 30, 2026 at 08:57:34AM -0400, Ruhl, Michael J wrote:
> >-----Original Message-----
> >From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> >Sent: Wednesday, September 30, 2026 12:09 AM
> >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; Brost, Matthew
> ><matthew.brost@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>; Poosa, Karthik
> ><karthik.poosa@intel.com>
> >Subject: Re: [PATCH v11 17/20] drm/xe/vsec: Support late bind fw information
> >
> >On Wed, Sep 23, 2026 at 11:11:32AM -0700, Michael J. Ruhl wrote:
> >> CRI FW is loaded on power on.  Because of this, access to the FW cannot
> >> be done until it is running.
> >>
> >> Update the XE PMT probe and access to check for late bind devices,
> >> verify, and wait for the appropriate FW state before probe or access.
> >>
> >> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> >
> >We do need this patch. One potential but unlikely corner case is the suspend
> >while this worker is waiting... might be worth investigating, but I'm not
> >holding/blocking on that:
> 
> I take the xe_runtime_get() before setting up the worker.  Once the worker completes successfully,
> I do the _put.
> 
> Can a suspend happened in this case?

it cannot runtime suspend (think d3hot/d3cold), but it can still system suspend (think s2idle/s3/s4)

> 
> >Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Thank you!
> 
> m
> >
> >> ---
> >>  drivers/gpu/drm/xe/xe_device.c       |   4 +-
> >>  drivers/gpu/drm/xe/xe_device_types.h |   5 ++
> >>  drivers/gpu/drm/xe/xe_vsec.c         | 129 ++++++++++++++++++++++++++-
> >>  drivers/gpu/drm/xe/xe_vsec.h         |   2 +-
> >>  4 files changed, 134 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_device.c
> >b/drivers/gpu/drm/xe/xe_device.c
> >> index 3ec950606e67..a2c8c7909688 100644
> >> --- a/drivers/gpu/drm/xe/xe_device.c
> >> +++ b/drivers/gpu/drm/xe/xe_device.c
> >> @@ -1053,7 +1053,9 @@ int xe_device_probe(struct xe_device *xe)
> >>  	if (err)
> >>  		goto err_unregister_display;
> >>
> >> -	xe_vsec_init(xe);
> >> +	err = xe_vsec_init(xe);
> >> +	if (err)
> >> +		goto err_unregister_display;
> >>
> >>  	xe_debugfs_register(xe);
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_device_types.h
> >b/drivers/gpu/drm/xe/xe_device_types.h
> >> index e08348a95b28..8f5cc5d65419 100644
> >> --- a/drivers/gpu/drm/xe/xe_device_types.h
> >> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> >> @@ -7,6 +7,7 @@
> >>  #define _XE_DEVICE_TYPES_H_
> >>
> >>  #include <linux/pci.h>
> >> +#include <linux/workqueue.h>
> >>
> >>  #include <drm/drm_device.h>
> >>  #include <drm/drm_file.h>
> >> @@ -488,6 +489,10 @@ struct xe_device {
> >>  		struct mutex lock;
> >>  		/** @pmt.base_offset: device specific base offset */
> >>  		u64 base_offset;
> >> +		/** @pmt.work: support late-bind probe */
> >> +		struct delayed_work work;
> >> +		/** @pmt.retry_count: late-bind probe retry */
> >> +		u32 retry_count;
> >>  	} pmt;
> >>
> >>  	/** @soc_remapper: SoC remapper object */
> >> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> >> index 0f6e3be520c0..5384fef5e5ac 100644
> >> --- a/drivers/gpu/drm/xe/xe_vsec.c
> >> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> >> @@ -3,6 +3,7 @@
> >>  #include <linux/bitfield.h>
> >>  #include <linux/bits.h>
> >>  #include <linux/cleanup.h>
> >> +#include <linux/delay.h>
> >>  #include <linux/errno.h>
> >>  #include <linux/intel_vsec.h>
> >>  #include <linux/module.h>
> >> @@ -18,6 +19,7 @@
> >>  #include "xe_mmio.h"
> >>  #include "xe_platform_types.h"
> >>  #include "xe_pm.h"
> >> +#include "xe_sysctrl.h"
> >>  #include "xe_vsec.h"
> >>
> >>  #include "regs/xe_pmt.h"
> >> @@ -173,6 +175,14 @@ enum capability {
> >>  	WATCHER,
> >>  };
> >>
> >> +/*
> >> + * Late bind will delay 100msec for up to 20 seconds
> >> + */
> >> +#define VSEC_LATE_BIND_DELAY_MSEC	100
> >> +#define VSEC_LATE_BIND_RETRY		200
> >> +
> >> +static void cri_late_bind_probe(struct xe_device *xe);
> >> +
> >>  static int bmg_guid_decode(u32 guid, int *index, u32 *offset)
> >>  {
> >>  	u32 record_id = FIELD_GET(GUID_RECORD_ID, guid);
> >> @@ -281,6 +291,48 @@ static int xe_guid_decode(u32 guid, int *index, u32
> >*offset)
> >>  	return -ENODEV;
> >>  }
> >>
> >> +static void cri_late_bind_probe_work(struct work_struct *work)
> >> +{
> >> +	struct xe_device *xe = container_of(work, struct xe_device,
> >pmt.work.work);
> >> +
> >> +	if (xe_sysctrl_is_oobmsm_fw_ready(xe)) {
> >> +		cri_late_bind_probe(xe);
> >> +		xe_pm_runtime_put(xe);
> >> +		return;
> >> +	}
> >> +
> >> +	xe->pmt.retry_count++;
> >> +
> >> +	/* wait up to 20 seconds */
> >> +	if (xe->pmt.retry_count == VSEC_LATE_BIND_RETRY) {
> >> +		drm_warn(&xe->drm, "PMT probe: Late Binding failed to
> >complete\n");
> >> +		xe_pm_runtime_put(xe);
> >> +		return;
> >> +	}
> >> +
> >> +	if (!schedule_delayed_work(&xe->pmt.work,
> >msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC)))
> >> +		xe_pm_runtime_put(xe);
> >> +}
> >> +
> >> +static bool wait_for_fw(struct xe_device *xe)
> >> +{
> >> +	int retries = VSEC_LATE_BIND_RETRY;  /* wait up to 20 secs */
> >> +
> >> +	if (xe->info.platform != XE_CRESCENTISLAND)
> >> +		return true;
> >> +
> >> +	while (retries--) {
> >> +		if (xe_sysctrl_is_oobmsm_fw_ready(xe))
> >> +			return true;
> >> +
> >> +		msleep(VSEC_LATE_BIND_DELAY_MSEC);
> >> +	}
> >> +
> >> +	drm_warn(&xe->drm, "Late Binding failed to complete\n");
> >> +
> >> +	return false;
> >> +}
> >> +
> >>  /**
> >>   * xe_pmt_telem_read - Given a device and a PMT GUID, read data into a
> >buffer
> >>   * @dev: valid Xe device
> >> @@ -343,6 +395,11 @@ int xe_pmt_telem_read(struct device *dev, u32
> >guid, u64 *data, loff_t user_offse
> >>  		goto dev_exit;
> >>  	}
> >>
> >> +	if (!wait_for_fw(xe)) {
> >> +		ret = -ENODATA;
> >> +		goto runtime_exit;
> >> +	}
> >> +
> >>  	scoped_guard(mutex, &xe->pmt.lock) {
> >>  		/* set SoC re-mapper index register based on GUID memory
> >region */
> >>  		xe->soc_remapper.set_telem_region(xe, mem_region);
> >> @@ -350,6 +407,7 @@ int xe_pmt_telem_read(struct device *dev, u32 guid,
> >u64 *data, loff_t user_offse
> >>  		memcpy_fromio(data, telem_addr, count);
> >>  	}
> >>
> >> +runtime_exit:
> >>  	xe_pm_runtime_put(xe);
> >>
> >>  dev_exit:
> >> @@ -397,6 +455,10 @@ static int xe_pmt_read_reg(struct device *dev, u32
> >guid, u32 *reg, u32 offset)
> >>  	disc_addr += inst + offset;
> >>
> >>  	xe_pm_runtime_get(xe);
> >> +	if (!wait_for_fw(xe)) {
> >> +		ret = -ENODATA;
> >> +		goto runtime_exit;
> >> +	}
> >>
> >>  	scoped_guard(mutex, &xe->pmt.lock) {
> >>  		xe->soc_remapper.set_telem_region(xe,
> >CRI_IDX_TELEM_DISCOVERY);
> >> @@ -404,6 +466,7 @@ static int xe_pmt_read_reg(struct device *dev, u32
> >guid, u32 *reg, u32 offset)
> >>  		*reg = readl(disc_addr);
> >>  	}
> >>
> >> +runtime_exit:
> >>  	xe_pm_runtime_put(xe);
> >>
> >>  dev_exit:
> >> @@ -434,6 +497,10 @@ static int xe_pmt_write_reg(struct device *dev, u32
> >guid, u32 reg, u32 offset)
> >>  	disc_addr += inst + offset;
> >>
> >>  	xe_pm_runtime_get(xe);
> >> +	if (!wait_for_fw(xe)) {
> >> +		ret = -ENODATA;
> >> +		goto runtime_exit;
> >> +	}
> >>
> >>  	scoped_guard(mutex, &xe->pmt.lock) {
> >>  		xe->soc_remapper.set_telem_region(xe,
> >CRI_IDX_TELEM_DISCOVERY);
> >> @@ -441,6 +508,7 @@ static int xe_pmt_write_reg(struct device *dev, u32
> >guid, u32 reg, u32 offset)
> >>  		writel(reg, disc_addr);
> >>  	}
> >>
> >> +runtime_exit:
> >>  	xe_pm_runtime_put(xe);
> >>
> >>  dev_exit:
> >> @@ -477,12 +545,46 @@ static enum xe_vsec get_platform_info(struct
> >xe_device *xe)
> >>  	return vsec_platforms[xe->info.platform];
> >>  }
> >>
> >> +static void cri_late_bind_probe(struct xe_device *xe)
> >> +{
> >> +	struct intel_vsec_platform_info *info;
> >> +	struct device *dev = xe->drm.dev;
> >> +	enum xe_vsec platform;
> >> +
> >> +	platform = get_platform_info(xe);
> >> +	if (platform != XE_VSEC_CRI)
> >> +		return;
> >> +
> >> +	info = &xe_vsec_info[platform];
> >> +	if (!info->headers)
> >> +		return;
> >> +
> >> +	guard(mutex)(&xe->pmt.lock);
> >> +
> >> +	info->priv_data = &xe_cri_pmt_cb;
> >> +	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> >> +
> >> +	intel_vsec_register(dev, info);
> >> +}
> >> +
> >> +static void vsec_disable_late_bind_work(void *arg)
> >> +{
> >> +	struct xe_device *xe = arg;
> >> +
> >> +	/*
> >> +	 * If the queued work is canceled, the runtime reference needs to be
> >> +	 * released here.
> >> +	 */
> >> +	if (disable_delayed_work_sync(&xe->pmt.work))
> >> +		xe_pm_runtime_put(xe);
> >> +}
> >> +
> >>  /**
> >>   * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
> >>   * interface
> >>   * @xe: valid xe instance
> >>   */
> >> -void xe_vsec_init(struct xe_device *xe)
> >> +int xe_vsec_init(struct xe_device *xe)
> >>  {
> >>  	struct intel_vsec_platform_info *info;
> >>  	struct device *dev = xe->drm.dev;
> >> @@ -490,11 +592,11 @@ void xe_vsec_init(struct xe_device *xe)
> >>
> >>  	platform = get_platform_info(xe);
> >>  	if (platform == XE_VSEC_UNKNOWN)
> >> -		return;
> >> +		return 0;
> >>
> >>  	info = &xe_vsec_info[platform];
> >>  	if (!info->headers)
> >> -		return;
> >> +		return 0;
> >>
> >>  	switch (platform) {
> >>  	case XE_VSEC_BMG:
> >> @@ -504,12 +606,26 @@ void xe_vsec_init(struct xe_device *xe)
> >>
> >>  	case XE_VSEC_CRI:
> >>  		xe->pmt.base_offset = CRI_PMT_OFFSET;
> >> +
> >> +		xe->pmt.retry_count = 0;
> >> +		INIT_DELAYED_WORK(&xe->pmt.work,
> >cri_late_bind_probe_work);
> >> +
> >> +		xe_pm_runtime_get_noresume(xe);
> >> +		if (!xe_sysctrl_is_oobmsm_fw_ready(xe)) {
> >> +			schedule_delayed_work(&xe->pmt.work,
> >> +
> >msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC));
> >> +			return devm_add_action_or_reset(xe->drm.dev,
> >> +
> >	vsec_disable_late_bind_work,
> >> +							xe);
> >> +		}
> >> +
> >>  		info->priv_data = &xe_cri_pmt_cb;
> >>  		xe->soc_remapper.set_telem_region(xe,
> >CRI_IDX_TELEM_DISCOVERY);
> >>  		break;
> >>
> >>  	default:
> >> -		break;
> >> +		drm_err(&xe->drm, "Unsupported platform: %u\n", platform);
> >> +		return 0;
> >>  	}
> >>
> >>  	/*
> >> @@ -517,5 +633,10 @@ void xe_vsec_init(struct xe_device *xe)
> >>  	 * resources.
> >>  	 */
> >>  	intel_vsec_register(dev, info);
> >> +
> >> +	if (platform == XE_VSEC_CRI)
> >> +		xe_pm_runtime_put(xe);
> >> +
> >> +	return 0;
> >>  }
> >>  MODULE_IMPORT_NS("INTEL_VSEC");
> >> diff --git a/drivers/gpu/drm/xe/xe_vsec.h b/drivers/gpu/drm/xe/xe_vsec.h
> >> index a25b4e6e681b..c4a1e2fc67d8 100644
> >> --- a/drivers/gpu/drm/xe/xe_vsec.h
> >> +++ b/drivers/gpu/drm/xe/xe_vsec.h
> >> @@ -9,7 +9,7 @@
> >>  struct device;
> >>  struct xe_device;
> >>
> >> -void xe_vsec_init(struct xe_device *xe);
> >> +int xe_vsec_init(struct xe_device *xe);
> >>  int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t
> >user_offset, u32 count);
> >>
> >>  #endif
> >> --
> >> 2.43.0
> >>