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

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

On Wed, 2026-04-08 at 14:40 +0200, Michal Wajdeczko wrote:
> 
> 
> On 4/7/2026 8:55 PM, Summers, Stuart wrote:
> > On Tue, 2026-04-07 at 11:48 +0200, Michal Wajdeczko wrote:
> > > 
> > > 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
> > 
> > Ok makes sense I'll do that on the next upload.
> > 
> > > 
> > > and please not forget to use -v option while preparing new patch
> > > revisions
> > 
> > Thanks Michal, I'll make sure to include this going forward.
> > 
> > > 
> > > [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
> > 
> > Makes sense to me. I'll make the change.
> > 
> > > 
> > > > +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 ...)
> > 
> > Maybe I'll just include that in the next rev...
> 
> btw, as there might be more "GuC" specific attributes, maybe we
> should
> try to group them?
> 
> /sys/kernel/config/xe/
> └── 0000:4d:00.0
>     ├── ...
>     └── guc
>         ├── firmware_path       ""
>         ├── log_level           0..5
>         ├── log_npk             false|true
>         ├── log_target          0..3
>         └── ...

Yeah makes sense to me...

> 
> > 
> > > 
> > > 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);
> > 
> > So I do like having the ability to pick and chose how we configure
> > the
> > logging here. I think in most cases sending to both lmem and npk is
> > what we want, but there might be some corner case debugs where any
> > regular access to lmem (or even smem) like this causes problems and
> > we
> > really do just want to send everything to npk. And we could of
> > course
> > default to send to npk and lmem, but I think for the general case
> > we
> > will never actually use this - it's a debug only function.
> > 
> > But your point about ABI is fair here. We don't have any use case
> > beyond these two targets today, but that doesn't mean it always has
> > to
> > be the case... We could enforce these values for 0-2 and only add
> > additional destinations on top of that.
> > 
> > Although given this is really a debug feature, would it make more
> > sense
> > to split this out as part of a debug only configfs? That's really
> > the
> > case for almost all of what we have in configfs today, minus maybe
> > the
> > max VF configuration - it isn't about performance tuning or
> > anything,
> > just about reducing scope of the driver in one way or another or
> > enabling some debug capability otherwise. To me that isn't a true
> > ABI.
> 
> +1
> 
> but in the past I was told that configfs is ABI

At least from the documentation it seems like it is. The documentation
compares it directly to sysfs - a configuration alternative to the
sysfs runtime. But at least wrapping in DRM_XE_DEBUG we can prevent
obvious mis-use... For now I can at least float the patch and get
feedback. I'll add the pros and cons from what you described in the new
cover letter so we can discuss.

> 
> > 
> > What if I split this into xe_configfs.c and xe_configfs_debug.c,
> > moved
> > all of the debug specific entries to the latter with a little less
> > restriction and kept the max VF one in xe_configfs.c for now? Then
> > xe_configfs_debug.c can wrap in
> > CONFIG_DRM_XE_DEBUG/CONFIG_DRM_XE_DEBUG_GUC?
> 
> so now that's the question to the maintainers - and I guess the main
> question here is if already exposed attributes can be now hidden
> 
> (maybe we can just clarify that some entries are DEBUG only, and
> thus may be changed/dropped in the future)

Yeah I mean to me at least if we call it out as debug via config, it
means an end user should rely on it... it requires them to rebuild the
kernel if nothing else from the distro release.

Thanks,
Stuart

> 
> > 
> > I'll wait to hear back before making a change like that so we're on
> > the
> > same page.
> > 
> > > 
> > > 
> > > > +       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.
> > 
> > Ah makes sense thanks.
> > 
> > Thanks,
> > Stuart
> > 
> > > 
> > > [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
> > > > _DIS
> > > > PLAY)
> > > >  #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;
> > > >  }
> > > 
> > 
>