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

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

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
        └── ...

> 
>>
>> 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

> 
> 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)

> 
> 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;
>>>  }
>>
>