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

["Summers, Stuart" <stuart.summers@intel.com>]

On Mon, 2026-04-06 at 14:28 -0700, Daniele Ceraolo Spurio wrote:
> 
> 
> On 2/26/2026 3:26 PM, 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)
> > 
> > Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> > Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_configfs.c | 60
> > ++++++++++++++++++++++++++++++++
> >   drivers/gpu/drm/xe/xe_configfs.h |  5 +++
> >   drivers/gpu/drm/xe/xe_defaults.h |  1 +
> >   drivers/gpu/drm/xe/xe_guc.c      |  7 +++-
> >   4 files changed, 72 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_configfs.c
> > b/drivers/gpu/drm/xe/xe_configfs.c
> > index 078dee985d24..e0c21328af75 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.
> > + *
> > + *     # 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,37 @@ static bool is_bound(struct
> > xe_config_group_device *dev)
> >         return ret;
> >   }
> >   
> > +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;
> > +
> > +#define GUC_LOG_TARGET_MAX     2
> 
> IMO better to define this with XE_DEFAULT_GUC_LOG_TARGET. Maybe also
> add 
> a comment to explain that the modes are set directly by the user and 
> therefore we don't need individual defines.

Hm.. it feels weird to add the max define in the defaults header which
is one reason I kept that here. I could put both of them in
xe_configfs.h or xe_guc_log_types.h?

Comment you suggested makes sense. I'll add that in the next rev.

> 
> > +       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);
> > @@ -814,6 +857,7 @@ CONFIGFS_ATTR(, ctx_restore_mid_bb);
> >   CONFIGFS_ATTR(, ctx_restore_post_bb);
> >   CONFIGFS_ATTR(, enable_psmi);
> >   CONFIGFS_ATTR(, engines_allowed);
> > +CONFIGFS_ATTR(, guc_log_target);
> 
> This should go after gt_types if we want to keep the alphabetical
> order.

Makes sense.

> 
> >   CONFIGFS_ATTR(, gt_types_allowed);
> >   CONFIGFS_ATTR(, survivability_mode);
> >   
> > @@ -822,6 +866,7 @@ static struct configfs_attribute
> > *xe_config_device_attrs[] = {
> >         &attr_ctx_restore_post_bb,
> >         &attr_enable_psmi,
> >         &attr_engines_allowed,
> > +       &attr_guc_log_target,
> 
> here as well
> 
> >         &attr_gt_types_allowed,
> >         &attr_survivability_mode,
> >         NULL,
> > @@ -1094,6 +1139,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);
> > @@ -1146,6 +1192,20 @@ bool
> > xe_configfs_get_survivability_mode(struct pci_dev *pdev)
> >         return mode;
> >   }
> >   
> > +u8 xe_configfs_get_guc_log_target(struct pci_dev *pdev)
> 
> This function needs documentation

Ok.

> 
> > +{
> > +       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_DISP
> > LAY)
> >   #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 54d2fc780127..d98d5d051f1b 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))
> >                 flags |= GUC_LOG_DISABLED;
> >         else
> > -               flags |= FIELD_PREP(GUC_LOG_VERBOSITY,
> > GUC_LOG_LEVEL_TO_VERBOSITY(level));
> > +               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));
> 
> nit: Should this be part of the else case above? the specs don't say 
> that it is an error to set this with the logs disabled but it does
> seem 
> weird to do so. not a blocker, so I'm ok it you want to keep it here.

Yeah makes sense to me and I agree with your reasoning. I'll make the
change and repost.

Thanks!
Stuart

> 
> Daniele
> 
> >   
> >         return flags;
> >   }
>