Re: [PATCH 16/20] drm/xe/eudebug: Mark guc contexts as debuggable

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Matthew Brost <matthew.brost@intel.com> writes:

> On Mon, Oct 06, 2025 at 02:17:06PM +0300, Mika Kuoppala wrote:
>> We need to inform to guc which contexts are debuggable
>> as their handling is different from ordinary contexts.
>> 
>> Co-developed-by: Dominik Grzegorzek <dominik.grzegorzek@intel.com>
>> Co-developed-by: Maciej Patelczyk <maciej.patelczyk@intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/xe/abi/guc_actions_abi.h |  5 +++
>>  drivers/gpu/drm/xe/xe_eudebug_hw.c       | 55 ++++++++++++++++++++++++
>>  drivers/gpu/drm/xe/xe_eudebug_hw.h       |  4 ++
>>  drivers/gpu/drm/xe/xe_guc_submit.c       |  4 ++
>>  4 files changed, 68 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/xe/abi/guc_actions_abi.h b/drivers/gpu/drm/xe/abi/guc_actions_abi.h
>> index 47756e4674a1..32a5f680a6d2 100644
>> --- a/drivers/gpu/drm/xe/abi/guc_actions_abi.h
>> +++ b/drivers/gpu/drm/xe/abi/guc_actions_abi.h
>> @@ -155,6 +155,7 @@ enum xe_guc_action {
>>  	XE_GUC_ACTION_NOTIFY_FLUSH_LOG_BUFFER_TO_FILE = 0x8003,
>>  	XE_GUC_ACTION_NOTIFY_CRASH_DUMP_POSTED = 0x8004,
>>  	XE_GUC_ACTION_NOTIFY_EXCEPTION = 0x8005,
>> +	XE_GUC_ACTION_EU_KERNEL_DEBUG = 0x8006,
>>  	XE_GUC_ACTION_TEST_G2G_SEND = 0xF001,
>>  	XE_GUC_ACTION_TEST_G2G_RECV = 0xF002,
>>  	XE_GUC_ACTION_LIMIT
>> @@ -278,4 +279,8 @@ enum xe_guc_g2g_type {
>>  /* invalid type for XE_GUC_ACTION_NOTIFY_MEMORY_CAT_ERROR */
>>  #define XE_GUC_CAT_ERR_TYPE_INVALID 0xdeadbeef
>>  
>> +enum  xe_guc_eu_kernel_debug_request_type {
>> +	XE_GUC_EU_KERNEL_DEBUG_ENABLE = 0x3,
>> +};
>> +
>>  #endif
>> diff --git a/drivers/gpu/drm/xe/xe_eudebug_hw.c b/drivers/gpu/drm/xe/xe_eudebug_hw.c
>> index a62c4b439888..cd4627705b56 100644
>> --- a/drivers/gpu/drm/xe/xe_eudebug_hw.c
>> +++ b/drivers/gpu/drm/xe/xe_eudebug_hw.c
>> @@ -12,6 +12,7 @@
>>  #include "regs/xe_gt_regs.h"
>>  #include "regs/xe_engine_regs.h"
>>  
>> +#include "abi/guc_actions_abi.h"
>>  #include "xe_eudebug.h"
>>  #include "xe_eudebug_types.h"
>>  #include "xe_exec_queue.h"
>> @@ -20,6 +21,9 @@
>>  #include "xe_gt.h"
>>  #include "xe_gt_debug.h"
>>  #include "xe_gt_mcr.h"
>> +#include "xe_guc.h"
>> +#include "xe_guc_ct.h"
>> +#include "xe_guc_exec_queue_types.h"
>>  #include "xe_hw_engine.h"
>>  #include "xe_lrc.h"
>>  #include "xe_macros.h"
>> @@ -675,6 +679,57 @@ static int xe_eu_control_stopped(struct xe_eudebug *d,
>>  	return xe_gt_eu_attention_bitmap(q->gt, bits, bitmask_size);
>>  }
>>  
>> +static int xe_guc_action_eu_kernel_debug(struct xe_device *xe,
>> +					 struct xe_exec_queue *q,
>> +					 struct xe_lrc *lrc, u32 cmd)
>> +{
>> +	u32 action[] = {
>> +		XE_GUC_ACTION_EU_KERNEL_DEBUG,
>> +		q->guc->id,
>> +		cmd,
>> +		0, /* reserved */
>> +	};
>> +	int ret, i;
>> +
>> +	if (cmd != XE_GUC_EU_KERNEL_DEBUG_ENABLE)
>
> Maybe an xe_gt_assert here instead.
>
>> +		return -EINVAL;
>> +
>> +	ret = -EINVAL;
>> +	for (i = 0; i < q->width; i++) {
>
> I would double check with the GuC team if you have have enable EU
> debugging on each guc_id in multi-lrc contexts. I suspect not given
> register, scheduling toggles, and deregister H2G operate only on the
> main GuC ID. I'm am however unsure as H2G 8006 is not in GuC spec I'm
> looking at.
>

I have not yet had definite answer if main GuC id is enough.

>> +		if (lrc && q->lrc[i] != lrc)
>> +			continue;
>> +
>
> The above code looks unused as LRC is always NULL.
>
>> +		action[1] = q->guc->id + i;
>> +		drm_dbg(&xe->drm, "Guc action[%u] for ctx=%d",
>> +			cmd, action[1]);
>
> Prefer xe_gt_dbg.
>
>> +
>> +		ret = xe_guc_ct_send(&q->gt->uc.guc.ct,
>> +				     action, ARRAY_SIZE(action), 0, 0);
>> +
>> +		if (ret)
>> +			drm_dbg(&xe->drm, "eudebug guc cmd %u failed with %d\n",
>> +				cmd, ret);
>
> Prefer xe_gt_dbg.
>
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static bool xe_guc_has_debug_contexts(struct xe_gt *gt)
>> +{
>> +	return GUC_FIRMWARE_VER(&gt->uc.guc) >=	MAKE_GUC_VER(70, 49, 0);
>> +}
>> +
>> +int xe_eudebug_exec_queue_enable(struct xe_exec_queue *q)
>> +{
>
> The return value is not checked at the caller, so NULL return would be
> better choice.
>
>> +	struct xe_device *xe = gt_to_xe(q->gt);
>> +
>> +	if (!xe_guc_has_debug_contexts(q->gt))
>> +		return 0;
>> +
>> +	return xe_guc_action_eu_kernel_debug(xe, q, NULL,
>> +					     XE_GUC_EU_KERNEL_DEBUG_ENABLE);
>> +}
>> +
>>  static struct xe_eudebug_eu_control_ops eu_control = {
>>  	.interrupt_all = xe_eu_control_interrupt_all,
>>  	.stopped = xe_eu_control_stopped,
>> diff --git a/drivers/gpu/drm/xe/xe_eudebug_hw.h b/drivers/gpu/drm/xe/xe_eudebug_hw.h
>> index 8f59ec574e4e..5d1df5d7dc46 100644
>> --- a/drivers/gpu/drm/xe/xe_eudebug_hw.h
>> +++ b/drivers/gpu/drm/xe/xe_eudebug_hw.h
>> @@ -23,10 +23,14 @@ long xe_eudebug_eu_control(struct xe_eudebug *d, const u64 arg);
>>  
>>  struct xe_exec_queue *xe_gt_runalone_active_queue_get(struct xe_gt *gt, int *lrc_idx);
>>  
>> +int xe_eudebug_exec_queue_enable(struct xe_exec_queue *q);
>> +
>
> I'd probably stick this implementation xe_guc_submit.c as even though
> this EU debug specific, IMO you don't really need to compile this part
> out as surely if EU kconfig is unset EXEC_QUEUE_EUDEBUG_FLAG_ENABLE
> would be clear. Maybe ask others about where to stick this, as my
> opinion on this isn't strong either way.

I tucked it in xe_guc_submit.c without config switch. Much more
straightforward. Thanks.
-Mika

>
> Matt
>
>>  #else /* CONFIG_DRM_XE_EUDEBUG */
>>  
>>  static inline void xe_eudebug_init_hw_engine(struct xe_hw_engine *hwe, bool enable) { }
>>  
>> +static inline int xe_eudebug_exec_queue_enable(struct xe_exec_queue *q) { return 0; }
>> +
>>  #endif /* CONFIG_DRM_XE_EUDEBUG */
>>  
>>  #endif /* _XE_EUDEBUG_HW_H_ */
>> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>> index 16f78376f196..da264c1cfe76 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>> @@ -21,6 +21,7 @@
>>  #include "xe_assert.h"
>>  #include "xe_devcoredump.h"
>>  #include "xe_device.h"
>> +#include "xe_eudebug_hw.h"
>>  #include "xe_exec_queue.h"
>>  #include "xe_force_wake.h"
>>  #include "xe_gpu_scheduler.h"
>> @@ -655,6 +656,9 @@ static void register_exec_queue(struct xe_exec_queue *q, int ctx_type)
>>  	if (xe_exec_queue_is_lr(q))
>>  		xe_exec_queue_get(q);
>>  
>> +	if (q->eudebug_flags & EXEC_QUEUE_EUDEBUG_FLAG_ENABLE)
>> +		xe_eudebug_exec_queue_enable(q);
>> +
>>  	set_exec_queue_registered(q);
>>  	trace_xe_exec_queue_register(q);
>>  	if (xe_exec_queue_is_parallel(q))
>> -- 
>> 2.43.0
>>