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

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 4/6/2026 3:37 PM, Summers, Stuart wrote:
> 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?

Not sure why but I thought XE_DEFAULT_GUC_LOG_TARGET was in one of the 
GuC ABI files. We usually try to keep all the defines related to the GuC 
interface in one of the abi files (in this case it would be 
abi/guc_log_abi.h). Might be worth defining all 3 modes in that file and 
having something like:

in guc_log_abi.h:

enum guc_log_target {
     GUC_LOG_TARGET_MEM = 0,
     GUC_LOG_TARGET_NPK,
     GUC_LOG_TARGET_MEM_AND_NPK,
}

in the configfs files:

#define XE_DEFAULT_GUC_LOG_TARGET    GUC_LOG_TARGET_MEM
#define GUC_LOG_TARGET_MAX    GUC_LOG_TARGET_MEM_AND_NPK

Although this might be overkill, so I won't block the current approach.

Daniele

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