Re: [PATCH] drm/xe/guc: Add support for NPK as a GuC log target

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 4/7/2026 12:53 AM, Stuart Summers wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> GuC provides the ability to gather logs through a hardware interface
> called NPK. For certain debugging scenarios this can be advantageous
> over getting logs from memory (or in addition to).
> 
> Add a hook for this alternate debugging mode via a configfs. This
> translates into a parameter passed to GuC during load time.
> 
> v2: Convert to configfs from modparam (Matt)
> v3: Configfs documentation formatting (Shuicheng)
>     Kerneldoc/comment add + configfs entry ordering
>     Only set the guc_log_target when GuC log is enabled (Daniele)

nit: we may keep change log under --- so it stays on the ML only

and please not forget to use -v option while preparing new patch revisions

[1] https://git-scm.com/docs/git-format-patch#Documentation/git-format-patch.txt--vn

> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> Acked-by: Shuicheng Lin <shuicheng.lin@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_configfs.c | 70 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_configfs.h |  5 +++
>  drivers/gpu/drm/xe/xe_defaults.h |  1 +
>  drivers/gpu/drm/xe/xe_guc.c      | 11 +++--
>  4 files changed, 84 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
> index 32102600a148..e4511ee4e135 100644
> --- a/drivers/gpu/drm/xe/xe_configfs.c
> +++ b/drivers/gpu/drm/xe/xe_configfs.c
> @@ -83,6 +83,16 @@
>   *
>   * This attribute can only be set before binding to the device.
>   *
> + * GuC log target:
> + * ---------------
> + *
> + * Set the destination for the GuC log. 0 - memory only (default),
> + * 1 - NPK only, 2 - memory + NPK. Example::
> + *
> + *	# echo 2 > /sys/kernel/config/xe/0000:03:00.0/guc_log_target
> + *
> + * This attribute can only be set before binding to the device.
> + *
>   * Allowed GT types:
>   * -----------------
>   *
> @@ -256,6 +266,7 @@ struct xe_config_group_device {
>  	struct config_group sriov;
>  
>  	struct xe_config_device {
> +		u8 guc_log_target;
>  		u64 gt_types_allowed;
>  		u64 engines_allowed;
>  		struct wa_bb ctx_restore_post_bb[XE_ENGINE_CLASS_MAX];
> @@ -277,6 +288,7 @@ struct xe_config_group_device {
>  };
>  
>  static const struct xe_config_device device_defaults = {
> +	.guc_log_target = XE_DEFAULT_GUC_LOG_TARGET,
>  	.gt_types_allowed = U64_MAX,
>  	.engines_allowed = U64_MAX,
>  	.survivability_mode = false,
> @@ -357,6 +369,41 @@ static bool is_bound(struct xe_config_group_device *dev)
>  	return ret;
>  }
>  

can we expose this new attribute only for XE_DEBUG_GUC config?
we can easily use xe_config_device_is_visible for this

> +static ssize_t guc_log_target_show(struct config_item *item, char *page)
> +{
> +	struct xe_config_device *dev = to_xe_config_device(item);
> +
> +	return sprintf(page, "%d\n", dev->guc_log_target);
> +}
> +
> +static ssize_t guc_log_target_store(struct config_item *item, const char *page, size_t len)
> +{
> +	struct xe_config_group_device *dev = to_xe_config_group_device(item);
> +	u8 guc_log_target;
> +	int ret;
> +
> +	ret = kstrtou8(page, 0, &guc_log_target);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * No need to define full enumeration set since this is directly
> +	 * applied from the user here to GuC.
> +	 */
> +#define GUC_LOG_TARGET_MAX	2

late to the party but agree with Daniele that this should be part of the
GuC ABI file, and unlike Daniele I would prefer to enforce that as otherwise
we will start again mixing local defines that we can change with GuC
definitions that we don't own and can't change

(and yes, GUC_LOG_LEVEL min/max definitions should also follow that rule
so they should be fixed too - someday ...)

for me it looks that this MAX could be based on existing define:

#define GUC_LOG_TARGET_MAX	FIELD_MAX(GUC_LOG_DESTINATION)

but I'm wondering whether actually we should expose in stable configfs
the low-level GuC ABI definitions

maybe more safe solution would be to add just higher-level attribute

	bool guc_npk; /* use NPK? default: false */

and just convert that into GuC ABI bits in guc_ctl_debug_flags() ?

	flags |= FIELD_PREP(GUC_LOG_DESTINATION,
			xe_configfs_guc_npk(pdev) ?
			GUC_LOG_TARGET_MEM_AND_NPK :
			GUC_LOG_TARGET_MEM_ONLY);


> +	if (guc_log_target > GUC_LOG_TARGET_MAX)
> +		return -EINVAL;
> +#undef GUC_LOG_TARGET_MAX
> +> +	guard(mutex)(&dev->lock);
> +	if (is_bound(dev))
> +		return -EBUSY;
> +
> +	dev->config.guc_log_target = guc_log_target;
> +
> +	return len;
> +}
> +
>  static ssize_t survivability_mode_show(struct config_item *item, char *page)
>  {
>  	struct xe_config_device *dev = to_xe_config_device(item);
> @@ -815,6 +862,7 @@ CONFIGFS_ATTR(, ctx_restore_post_bb);
>  CONFIGFS_ATTR(, enable_psmi);
>  CONFIGFS_ATTR(, engines_allowed);
>  CONFIGFS_ATTR(, gt_types_allowed);
> +CONFIGFS_ATTR(, guc_log_target);
>  CONFIGFS_ATTR(, survivability_mode);
>  
>  static struct configfs_attribute *xe_config_device_attrs[] = {
> @@ -823,6 +871,7 @@ static struct configfs_attribute *xe_config_device_attrs[] = {
>  	&attr_enable_psmi,
>  	&attr_engines_allowed,
>  	&attr_gt_types_allowed,
> +	&attr_guc_log_target,
>  	&attr_survivability_mode,
>  	NULL,
>  };
> @@ -1095,6 +1144,7 @@ static void dump_custom_dev_config(struct pci_dev *pdev,
>  				 dev->config.attr_); \
>  	} while (0)
>  
> +	PRI_CUSTOM_ATTR("%d", guc_log_target);
>  	PRI_CUSTOM_ATTR("%llx", gt_types_allowed);
>  	PRI_CUSTOM_ATTR("%llx", engines_allowed);
>  	PRI_CUSTOM_ATTR("%d", enable_psmi);
> @@ -1147,6 +1197,26 @@ bool xe_configfs_get_survivability_mode(struct pci_dev *pdev)
>  	return mode;
>  }
>  
> +/**
> + * xe_configfs_get_guc_log_target - get configfs GuC log target attribute

nit: we try to append () to the function name in doc, like:

    * xe_configfs_get_guc_log_target() - Get GuC log target attribute.

[2] https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation

> + * @pdev: pci device
> + *
> + * Return: guc_log_target attribute in configfs
> + */
> +u8 xe_configfs_get_guc_log_target(struct pci_dev *pdev)
> +{
> +	struct xe_config_group_device *dev = find_xe_config_group_device(pdev);
> +	u8 target;
> +
> +	if (!dev)
> +		return device_defaults.guc_log_target;
> +
> +	target = dev->config.guc_log_target;
> +	config_group_put(&dev->group);
> +
> +	return target;
> +}
> +
>  static u64 get_gt_types_allowed(struct pci_dev *pdev)
>  {
>  	struct xe_config_group_device *dev = find_xe_config_group_device(pdev);
> diff --git a/drivers/gpu/drm/xe/xe_configfs.h b/drivers/gpu/drm/xe/xe_configfs.h
> index 07d62bf0c152..fb5cb7c57e75 100644
> --- a/drivers/gpu/drm/xe/xe_configfs.h
> +++ b/drivers/gpu/drm/xe/xe_configfs.h
> @@ -19,6 +19,7 @@ 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);
> +u8 xe_configfs_get_guc_log_target(struct pci_dev *pdev);
>  bool xe_configfs_primary_gt_allowed(struct pci_dev *pdev);
>  bool xe_configfs_media_gt_allowed(struct pci_dev *pdev);
>  u64 xe_configfs_get_engines_allowed(struct pci_dev *pdev);
> @@ -38,6 +39,10 @@ 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 u8 xe_configfs_get_guc_log_target(struct pci_dev *pdev)
> +{
> +	return XE_DEFAULT_GUC_LOG_TARGET;
> +}
>  static inline bool xe_configfs_primary_gt_allowed(struct pci_dev *pdev) { return true; }
>  static inline bool xe_configfs_media_gt_allowed(struct pci_dev *pdev) { return true; }
>  static inline u64 xe_configfs_get_engines_allowed(struct pci_dev *pdev) { return U64_MAX; }
> diff --git a/drivers/gpu/drm/xe/xe_defaults.h b/drivers/gpu/drm/xe/xe_defaults.h
> index c8ae1d5f3d60..fbe670668a04 100644
> --- a/drivers/gpu/drm/xe/xe_defaults.h
> +++ b/drivers/gpu/drm/xe/xe_defaults.h
> @@ -12,6 +12,7 @@
>  #else
>  #define XE_DEFAULT_GUC_LOG_LEVEL		1
>  #endif
> +#define XE_DEFAULT_GUC_LOG_TARGET		0
>  
>  #define XE_DEFAULT_PROBE_DISPLAY		IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>  #define XE_DEFAULT_VRAM_BAR_SIZE		0
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index e762eada21db..c40bd4aec2ce 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -73,13 +73,18 @@ static u32 guc_bo_ggtt_addr(struct xe_guc *guc,
>  
>  static u32 guc_ctl_debug_flags(struct xe_guc *guc)
>  {
> +	struct pci_dev *pdev = to_pci_dev(guc_to_xe(guc)->drm.dev);
>  	u32 level = xe_guc_log_get_level(&guc->log);
>  	u32 flags = 0;
>  
> -	if (!GUC_LOG_LEVEL_IS_VERBOSE(level))
> +	if (!GUC_LOG_LEVEL_IS_VERBOSE(level)) {
>  		flags |= GUC_LOG_DISABLED;
> -	else
> -		flags |= FIELD_PREP(GUC_LOG_VERBOSITY, GUC_LOG_LEVEL_TO_VERBOSITY(level));
> +	} else {
> +		flags |= FIELD_PREP(GUC_LOG_VERBOSITY,
> +				    GUC_LOG_LEVEL_TO_VERBOSITY(level));
> +		flags |= FIELD_PREP(GUC_LOG_DESTINATION,
> +				    xe_configfs_get_guc_log_target(pdev));
> +	}
>  
>  	return flags;
>  }