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 21:59 +0000, Lin, Shuicheng wrote:
> On Thu, Feb 26, 2026 3:27 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>
> 
> Some nits below.
> The code itself looks good to me. But I don't have NPK environment to
> verify it.
> Acked-by: Shuicheng Lin <shuicheng.lin@intel.com>

Thanks Shuicheng! I'll make your suggested changes in the next rev.

-Stuart

> 
> > ---
> >  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:
> > + * -------------
> 
> Nit: The underline must be at least as long as the header. So there
> should be 15 dashes (2 missed).
> 
> > + *
> > + * Set the destination for the GuC log. 0 - memory only (default),
> > + * 1 - NPK only, 2 - memory + NPK.
> 
> Nit: This line should end with "::" to introduce the code example. We
> could add " Example to enable it:: " as other cases. 
> 
> > + *
> > + *     # echo 2 >
> > /sys/kernel/config/xe/0000:03:00.0/guc_log_target
> 
> Nit: There is space + tab between "*    #", and that space should be
> removed.
> 
> Shuicheng
> 
> > + *
> > + * 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
> > +       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);
> >  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,
> >         &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) {
> > +       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
> > 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));
> > 
> >         return flags;
> >  }
> > --
> > 2.34.1
>