Re: [PATCH v4 22/23] drm/xe/configfs: Add attribute to disable GT types

[Matt Roper <matthew.d.roper@intel.com>]

On Wed, Oct 08, 2025 at 12:12:37PM +0200, Michal Wajdeczko wrote:
> 
> 
> On 10/7/2025 10:48 PM, Matt Roper wrote:
> > Preventing the driver from initializing GTs of specific type(s) can be
> > useful for debugging and early hardware bringup.  Add a configfs
> > attribute to allow this kind of control for debugging.
> > 
> > With today's platforms and software design, this configuration setting
> > is only effective for disabling the media GT since the driver currently
> > requires that there always be a primary GT to probe the device.  However
> > this might change in the future ---  in theory it should be possible
> > (with some additional driver work) to allow an igpu device to come up
> > with only the media GT and no primary GT.  Or to allow an igpu device to
> > come up with no GTs at all (for display-only usage).  A primary GT will
> > likely always be required on dgpu platforms because we rely on the BCS
> > engines inside the primary GT for various vram operations.
> > 
> > v2:
> >  - Expand/clarify kerneldoc for configfs attribute.  (Gustavo)
> >  - Tighten type usage in gt_types[] structure.  (Gustavo)
> >  - Adjust string parsing/name matching to match exact GT names and not
> >    accept partial names.  (Gustavo)
> > 
> > v3:
> >  - Switch to scope-based cleanup in gt_types_allowed_store() to fix a
> >    leak if the device is already bound.  (Gustavo)
> >  - Switch configfs lookup interface to two boolean functions that
> >    specify whether primary/media are supported rather than one function
> >    that returns a mask.  This is simpler to use and understand.
> > 
> > Cc: Gustavo Sousa <gustavo.sousa@intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_configfs.c | 145 +++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/xe/xe_configfs.h |   4 +
> >  drivers/gpu/drm/xe/xe_pci.c      |  22 +++++
> >  3 files changed, 171 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
> > index 139663423185..e36cc5e1bc8f 100644
> > --- a/drivers/gpu/drm/xe/xe_configfs.c
> > +++ b/drivers/gpu/drm/xe/xe_configfs.c
> > @@ -15,6 +15,7 @@
> >  
> >  #include "instructions/xe_mi_commands.h"
> >  #include "xe_configfs.h"
> > +#include "xe_gt_types.h"
> >  #include "xe_hw_engine_types.h"
> >  #include "xe_module.h"
> >  #include "xe_pci_types.h"
> > @@ -56,6 +57,7 @@
> >   *	:
> >   *	└── 0000:03:00.0
> >   *	    ├── survivability_mode
> > + *	    ├── gt_types_allowed
> 
> I'm wondering if we want to keep such advance knobs at the same level as others?
> maybe create sub-group for them, like I did for sriov?
> 
> 	    └── tweaks
> 	        ├── gt_types_allowed
> 	        ├── engines_allowed

That makes sense to me, although I'm not sure if it's too late to make
this change now since I think 'engines_allowed' is already part of
v6.17.  Are these configfs settings considered ABI that we can't change
now, even though they're explicitly intended for driver developers?

+Cc Lucas to see what he thinks.

> 
> 
> >   *	    ├── engines_allowed
> >   *	    └── enable_psmi
> 
> oops, and it looks that I missed to update this part of the doc when adding max_vfs with:
> 
> 	    └── sriov
> 	        ├── max_vfs
> 
> >   *
> > @@ -79,6 +81,44 @@
> >   *
> >   * This attribute can only be set before binding to the device.
> >   *
> > + * Allowed GT types:
> > + * -----------------
> > + *
> > + * Allow only specific types of GTs to be detected and initialized by the
> > + * driver.  Any combination of GT types can be enabled/disabled, although
> > + * some settings will cause the device to fail to probe.
> > + *
> > + * Writes support both comma- and newline-separated input format. Reads
> > + * will always return one GT type per line. "primary" and "media" are the
> > + * GT type names supported by this interface.
> > + *
> > + * This attribute can only be set before binding to the device.
> > + *
> > + * Examples:
> > + *
> > + * Allow both primary and media GTs to be initialized and used.  This matches
> > + * the driver's default behavior::
> > + *
> > + *	# echo 'primary,media' > /sys/kernel/config/xe/0000:03:00.0/gt_types_allowed
> 
> maybe "all" as an alias?
> 
> > + *
> > + * Allow only the primary GT of each tile to be initialized and used,
> > + * effectively disabling the media GT if it exists on the platform::
> > + *
> > + *	# echo 'primary' > /sys/kernel/config/xe/0000:03:00.0/gt_types_allowed
> > + *
> > + * Allow only the media GT of each tile to be initialized and used,
> > + * effectively disabling the primary GT.  **This configuration will cause
> > + * device probe failure on all current platforms, but may be allowed on
> > + * igpu platforms in the future**::
> > + *
> > + *	# echo 'media' > /sys/kernel/config/xe/0000:03:00.0/gt_types_allowed
> > + *
> > + * Disable all GTs.  Only other GPU IP (such as display) is potentially usable.
> > + * **This configuration will cause device probe failure on all current
> > + * platforms, but may be allowed on igpu platforms in the future**::
> > + *
> > + *	# echo '' > /sys/kernel/config/xe/0000:03:00.0/gt_types_allowed
> 
> maybe "none" as an alias?

While we could add the all/none aliases, I'm not sure it's worth the
extra parsing hassle, since then we also need to check for nonsense
settings like "primary,none" and such.  Since only a small subset of
driver developers are expected to ever use these, we don't need to worry
too much about convenience aliases.

> 
> > + *
> >   * Allowed engines:
> >   * ----------------
> >   *
> > @@ -187,6 +227,7 @@ struct xe_config_group_device {
> >  	struct config_group group;
> >  
> >  	struct xe_config_device {
> > +		u64 gt_types_allowed;
> >  		u64 engines_allowed;
> >  		struct wa_bb ctx_restore_post_bb[XE_ENGINE_CLASS_MAX];
> >  		struct wa_bb ctx_restore_mid_bb[XE_ENGINE_CLASS_MAX];
> > @@ -201,6 +242,7 @@ struct xe_config_group_device {
> >  };
> >  
> >  static const struct xe_config_device device_defaults = {
> > +	.gt_types_allowed = U64_MAX,
> >  	.engines_allowed = U64_MAX,
> >  	.survivability_mode = false,
> >  	.enable_psmi = false,
> > @@ -220,6 +262,7 @@ struct engine_info {
> >  /* Some helpful macros to aid on the sizing of buffer allocation when parsing */
> >  #define MAX_ENGINE_CLASS_CHARS 5
> >  #define MAX_ENGINE_INSTANCE_CHARS 2
> > +#define MAX_GT_TYPE_CHARS 7
> >  
> >  static const struct engine_info engine_info[] = {
> >  	{ .cls = "rcs", .mask = XE_HW_ENGINE_RCS_MASK, .engine_class = XE_ENGINE_CLASS_RENDER },
> > @@ -230,6 +273,14 @@ static const struct engine_info engine_info[] = {
> >  	{ .cls = "gsccs", .mask = XE_HW_ENGINE_GSCCS_MASK, .engine_class = XE_ENGINE_CLASS_OTHER },
> >  };
> >  
> > +static const struct {
> > +	const char name[MAX_GT_TYPE_CHARS + 1];
> > +	enum xe_gt_type type;
> > +} gt_types[] = {
> > +	{ .name = "primary", .type = XE_GT_TYPE_MAIN },
> > +	{ .name = "media", .type = XE_GT_TYPE_MEDIA },
> > +};
> > +
> >  static struct xe_config_group_device *to_xe_config_group_device(struct config_item *item)
> >  {
> >  	return container_of(to_config_group(item), struct xe_config_group_device, group);
> > @@ -292,6 +343,58 @@ static ssize_t survivability_mode_store(struct config_item *item, const char *pa
> >  	return len;
> >  }
> >  
> > +static ssize_t gt_types_allowed_show(struct config_item *item, char *page)
> > +{
> > +	struct xe_config_device *dev = to_xe_config_device(item);
> > +	char *p = page;
> > +
> > +	for (size_t i = 0; i < ARRAY_SIZE(gt_types); i++)
> > +		if (dev->gt_types_allowed & BIT_ULL(gt_types[i].type))
> > +			p += sprintf(p, "%s\n", gt_types[i].name);
> > +
> > +	return p - page;
> > +}
> > +
> > +static ssize_t gt_types_allowed_store(struct config_item *item, const char *page,
> > +				      size_t len)
> > +{
> > +	struct xe_config_group_device *dev = to_xe_config_group_device(item);
> > +	char *buf __free(kfree) = kstrdup(page, GFP_KERNEL);
> > +	char *p = buf;
> > +	u64 typemask = 0;
> > +
> > +	if (!buf)
> > +		return -ENOMEM;
> > +
> > +	while (p) {
> > +		char *typename = strsep(&p, ",\n");
> > +		bool matched = false;
> > +
> > +		if (typename[0] == '\0')
> > +			continue;
> > +
> > +		for (size_t i = 0; i < ARRAY_SIZE(gt_types); i++) {
> > +			if (strcmp(typename, gt_types[i].name) == 0) {
> > +				typemask |= BIT(gt_types[i].type);
> > +				matched = true;
> > +				break;
> > +			}
> > +		}
> > +
> > +		if (!matched)
> > +			return -EINVAL;
> > +	}
> > +
> > +	scoped_guard(mutex, &dev->lock) {
> 
> probably plain guard(mutex) will work here too
> 
> > +		if (is_bound(dev))
> > +			return -EBUSY;
> 
> then we can take a lock and return earlier, before parsing input
> 
> > +
> > +		dev->config.gt_types_allowed = typemask;
> > +	}
> > +
> > +	return len;
> > +}
> > +
> >  static ssize_t engines_allowed_show(struct config_item *item, char *page)
> >  {
> >  	struct xe_config_device *dev = to_xe_config_device(item);
> > @@ -672,6 +775,7 @@ CONFIGFS_ATTR(, ctx_restore_mid_bb);
> >  CONFIGFS_ATTR(, ctx_restore_post_bb);
> >  CONFIGFS_ATTR(, enable_psmi);
> >  CONFIGFS_ATTR(, engines_allowed);
> > +CONFIGFS_ATTR(, gt_types_allowed);
> >  CONFIGFS_ATTR(, survivability_mode);
> >  
> >  static struct configfs_attribute *xe_config_device_attrs[] = {
> > @@ -679,6 +783,7 @@ static struct configfs_attribute *xe_config_device_attrs[] = {
> >  	&attr_ctx_restore_post_bb,
> >  	&attr_enable_psmi,
> >  	&attr_engines_allowed,
> > +	&attr_gt_types_allowed,
> >  	&attr_survivability_mode,
> >  	NULL,
> >  };
> > @@ -846,6 +951,7 @@ static void dump_custom_dev_config(struct pci_dev *pdev,
> >  				 dev->config.attr_); \
> >  	} while (0)
> >  
> > +	PRI_CUSTOM_ATTR("%llx", gt_types_allowed);
> >  	PRI_CUSTOM_ATTR("%llx", engines_allowed);
> >  	PRI_CUSTOM_ATTR("%d", enable_psmi);
> >  	PRI_CUSTOM_ATTR("%d", survivability_mode);
> > @@ -896,6 +1002,45 @@ bool xe_configfs_get_survivability_mode(struct pci_dev *pdev)
> >  	return mode;
> >  }
> >  
> > +static u64 get_gt_types_allowed(struct xe_device *xe)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> > +	struct xe_config_group_device *dev = find_xe_config_group_device(pdev);
> > +	u64 mask;
> > +
> > +	if (!dev)
> > +		return device_defaults.gt_types_allowed;
> > +
> > +	mask = dev->config.gt_types_allowed;
> 
> btw, as we are using guard during write, shouldn't we also guard during read?

I'm not sure; is there something racy you see on the read path that we
need to avoid?  An individual get_gt_types_allowed() call racing with a
configfs write doesn't seem like it would matter; rather the problems of
a racing configfs write would be if it happened while a device was in
the middle of getting probed (so some calls to get_gt_types_allowed()
see the old value and others see the new one, leading to inconsistent
device intialization).  But given this is a low-level kernel hacking
interface intended for driver developers, I'm not sure how much we need
to worry about them shooting themselves in the foot.  Maybe I'm
overlooking something?


Matt

> 
> > +	config_group_put(&dev->group);
> > +
> > +	return mask;
> > +}
> > +
> > +/**
> > + * xe_configfs_primary_gt_supported - determine whether primary GTs are supported
> > + * @xe: xe device
> > + *
> > + * Return: True if primary GTs are enabled, false if they have been disabled via
> > + *     configfs.
> > + */
> > +bool xe_configfs_primary_gt_supported(struct xe_device *xe)
> > +{
> > +	return (get_gt_types_allowed(xe) & BIT_ULL(XE_GT_TYPE_MAIN)) != 0;
> 
> can't we just rely on the promotion to bool?
> 
> 	return get_gt_types_allowed(xe) & BIT_ULL(XE_GT_TYPE_MAIN);
> 
> > +}
> > +
> > +/**
> > + * xe_configfs_media_gt_supported - determine whether media GTs are supported
> > + * @xe: xe device
> > + *
> > + * Return: True if the media GTs are enabled, false if they have been disabled
> > + *     via configfs.
> > + */
> > +bool xe_configfs_media_gt_supported(struct xe_device *xe)
> > +{
> > +	return (get_gt_types_allowed(xe) & BIT_ULL(XE_GT_TYPE_MEDIA)) != 0;
> > +}
> > +
> >  /**
> >   * xe_configfs_get_engines_allowed - get engine allowed mask from configfs
> >   * @pdev: pci device
> > diff --git a/drivers/gpu/drm/xe/xe_configfs.h b/drivers/gpu/drm/xe/xe_configfs.h
> > index c61e0e47ed94..5624e965b911 100644
> > --- a/drivers/gpu/drm/xe/xe_configfs.h
> > +++ b/drivers/gpu/drm/xe/xe_configfs.h
> > @@ -17,6 +17,8 @@ int xe_configfs_init(void);
> >  void xe_configfs_exit(void);
> >  void xe_configfs_check_device(struct pci_dev *pdev);
> >  bool xe_configfs_get_survivability_mode(struct pci_dev *pdev);
> > +bool xe_configfs_primary_gt_supported(struct xe_device *xe);
> > +bool xe_configfs_media_gt_supported(struct xe_device *xe);
> 
> I guess we need decide now whether we want to continue to pass pdev or switch to xe as argument for all xe_configfs functions
> 
> >  u64 xe_configfs_get_engines_allowed(struct pci_dev *pdev);
> >  bool xe_configfs_get_psmi_enabled(struct pci_dev *pdev);
> >  u32 xe_configfs_get_ctx_restore_mid_bb(struct pci_dev *pdev, enum xe_engine_class,
> > @@ -28,6 +30,8 @@ static inline int xe_configfs_init(void) { return 0; }
> >  static inline void xe_configfs_exit(void) { }
> >  static inline void xe_configfs_check_device(struct pci_dev *pdev) { }
> >  static inline bool xe_configfs_get_survivability_mode(struct pci_dev *pdev) { return false; }
> > +static inline bool xe_configfs_primary_gt_supported(struct xe_device *xe) { return true; }
> > +static inline bool xe_configfs_media_gt_supported(struct xe_device *xe) { return true; }
> >  static inline u64 xe_configfs_get_engines_allowed(struct pci_dev *pdev) { return U64_MAX; }
> >  static inline bool xe_configfs_get_psmi_enabled(struct pci_dev *pdev) { return false; }
> >  static inline u32 xe_configfs_get_ctx_restore_mid_bb(struct pci_dev *pdev, enum xe_engine_class,
> > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > index a5932e4f4a23..9c8ab2b41737 100644
> > --- a/drivers/gpu/drm/xe/xe_pci.c
> > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > @@ -695,6 +695,11 @@ static struct xe_gt *alloc_primary_gt(struct xe_tile *tile,
> >  	struct xe_device *xe = tile_to_xe(tile);
> >  	struct xe_gt *gt;
> >  
> > +	if (!xe_configfs_primary_gt_supported(xe)) {
> > +		drm_info(&xe->drm, "Primary GT disabled via configfs\n");
> 
> nit: you can use xe_info(xe, "...") now
> 
> > +		return NULL;
> > +	}
> > +
> >  	gt = xe_gt_alloc(tile);
> >  	if (IS_ERR(gt))
> >  		return gt;
> > @@ -720,6 +725,11 @@ static struct xe_gt *alloc_media_gt(struct xe_tile *tile,
> >  	struct xe_device *xe = tile_to_xe(tile);
> >  	struct xe_gt *gt;
> >  
> > +	if (!xe_configfs_media_gt_supported(xe)) {
> > +		drm_info(&xe->drm, "Media GT disabled via configfs\n");
> > +		return NULL;
> > +	}
> > +
> >  	if (MEDIA_VER(xe) < 13 || !media_desc)
> >  		return NULL;
> >  
> > @@ -829,6 +839,18 @@ static int xe_info_init(struct xe_device *xe,
> >  		if (IS_ERR(tile->primary_gt))
> >  			return PTR_ERR(tile->primary_gt);
> >  
> > +		/*
> > +		 * It's not currently possible to probe a device with the
> > +		 * primary GT disabled.  With some work, this may be future in
> > +		 * the possible for igpu platforms (although probably not for
> > +		 * dgpu's since access to the primary GT's BCS engines is
> > +		 * required for VRAM management).
> > +		 */
> > +		if (!tile->primary_gt) {
> > +			drm_err(&xe->drm, "Cannot probe device with without a primary GT\n");
> > +			return -ENODEV;
> > +		}
> > +
> >  		tile->media_gt = alloc_media_gt(tile, media_desc);
> >  		if (IS_ERR(tile->media_gt))
> >  			return PTR_ERR(tile->media_gt);
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation