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

[John Harrison <john.c.harrison@intel.com>]

On 6/11/2025 2:49 PM, Lucas De Marchi wrote:
> On Wed, Jun 11, 2025 at 02:05:53PM -0700, John.C.Harrison@Intel.com 
> wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> The GuC has an option to write log data via NPK. This is basically a
>> magic IO address that GuC writes arbitrary data to and which can be
>> logged by a suitable hardware logger. This can allow retrieval of the
>> GuC log in hardware debug environments even when the system as a whole
>> dies horribly.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_guc.c    | 4 ++++
>> drivers/gpu/drm/xe/xe_module.c | 4 ++++
>> drivers/gpu/drm/xe/xe_module.h | 1 +
>> 3 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>> index e16d19b44bcc..9c0e3113f7d5 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>> @@ -35,6 +35,7 @@
>> #include "xe_guc_submit.h"
>> #include "xe_memirq.h"
>> #include "xe_mmio.h"
>> +#include "xe_module.h"
>> #include "xe_platform_types.h"
>> #include "xe_sriov.h"
>> #include "xe_uc.h"
>> @@ -74,6 +75,9 @@ static u32 guc_ctl_debug_flags(struct xe_guc *guc)
>>     else
>>         flags |= FIELD_PREP(GUC_LOG_VERBOSITY, 
>> GUC_LOG_LEVEL_TO_VERBOSITY(level));
>>
>> +    if (xe_modparam.guc_log_target)
>> +        flags |= FIELD_PREP(GUC_LOG_DESTINATION, 
>> xe_modparam.guc_log_target);
>
> is this supported across the board for all platforms and firmware
> versions? Otherwise, what'd happen if you have and old igfx and you are
> debugging a new dgfx? or vice-versa.
All platforms that have a GuC and all firmware builds that have been 
supported under Linux. North PeaK is part of the host chipset not the 
GuC or GT.

>
> We should probably need to sanitize the user input here?
Doesn't FIELD_PREP do that already? It will ensure the value written 
does not overflow the field. And if the user is setting a dumb value 
then that's their problem. It will have no effect beyond where the GuC 
log goes.

John.

>
> Lucas De Marchi
>
>> +
>>     return flags;
>> }
>>
>> diff --git a/drivers/gpu/drm/xe/xe_module.c 
>> b/drivers/gpu/drm/xe/xe_module.c
>> index 1c4dfafbcd0b..fc8c681819b9 100644
>> --- a/drivers/gpu/drm/xe/xe_module.c
>> +++ b/drivers/gpu/drm/xe/xe_module.c
>> @@ -21,6 +21,7 @@
>> struct xe_modparam xe_modparam = {
>>     .probe_display = true,
>>     .guc_log_level = 3,
>> +    .guc_log_target = 0,
>>     .force_probe = CONFIG_DRM_XE_FORCE_PROBE,
>> #ifdef CONFIG_PCI_IOV
>>     .max_vfs = IS_ENABLED(CONFIG_DRM_XE_DEBUG) ? ~0 : 0,
>> @@ -45,6 +46,9 @@ MODULE_PARM_DESC(vram_bar_size, "Set the vram bar 
>> size (in MiB) - <0=disable-res
>> module_param_named(guc_log_level, xe_modparam.guc_log_level, int, 0600);
>> MODULE_PARM_DESC(guc_log_level, "GuC firmware logging level 
>> (0=disable, 1..5=enable with verbosity min..max)");
>>
>> +module_param_named(guc_log_target, xe_modparam.guc_log_target, int, 
>> 0600);
>> +MODULE_PARM_DESC(guc_log_target, "GuC firmware logging target 
>> (0=memory [default], 1 = NPK, 2 = memory + NPK)");
>> +
>> module_param_named_unsafe(guc_firmware_path, 
>> xe_modparam.guc_firmware_path, charp, 0400);
>> MODULE_PARM_DESC(guc_firmware_path,
>>          "GuC firmware path to use instead of the default one");
>> diff --git a/drivers/gpu/drm/xe/xe_module.h 
>> b/drivers/gpu/drm/xe/xe_module.h
>> index 5a3bfea8b7b4..4d978f6f26b6 100644
>> --- a/drivers/gpu/drm/xe/xe_module.h
>> +++ b/drivers/gpu/drm/xe/xe_module.h
>> @@ -14,6 +14,7 @@ struct xe_modparam {
>>     bool probe_display;
>>     u32 force_vram_bar_size;
>>     int guc_log_level;
>> +    int guc_log_target;
>>     char *guc_firmware_path;
>>     char *huc_firmware_path;
>>     char *gsc_firmware_path;
>> -- 
>> 2.49.0
>>