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/12/2025 3:05 PM, Lucas De Marchi wrote:
> On Wed, Jun 11, 2025 at 04:51:24PM -0700, John Harrison wrote:
>> 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.
>
> so I imagine this only works in igfx? I'm mostly concerned about this
AFAIK, all platforms means all platforms. TGL, DG2, PVC, LNL, BMG, PTL, 
everything.

> being a module param and then you try to debug one card and get the
> other to do what you don't want (or crash, or refuse to load). For
> similar reason we've been adding these debug aids via configfs rather
> than module param.  If it's harmless, then.... well guc_log_level is
> already there, doesn't hurt much to have the other.
Yeah, Stuart mentioned the configfs thing. I'm going to take a look at 
doing it that way. My only concern with that is the learning curve to 
use configfs seems quite high. We can't just add a comment to an end 
user bug report saying "please set the module parameter". It would take 
quite a bit of explaining to get someone to set an option via configfs.

For this particular setting, it's not so much of a concern. As 
mentioned, this is not something any random end user can actually make 
use of. It requires some serious hardware to be attached to get the logs 
out. So someone using this is going to be more hack savvy than an 
average user.

John.

>
> Lucas De Marchi