Re: [PATCH v2 1/1] drm/xe: Report GT TDR and VM rebind faults using SIGID

["Yadav, Arvind" <arvind.yadav@intel.com>]

On 17-08-2026 15:50, Michal Wajdeczko wrote:
>
> On 8/4/2026 11:36 AM, Arvind Yadav wrote:
>> Route a few existing GT TDR and VM rebind failure logs through the
>> structured SIGID logging helpers.
> above is a good candidate for the cover-letter
> then IMO there should be separate patches per each new SIGID usage (see
> examples in SIGID introduction series)


Agree. I will move the high-level text to the cover letter and and split 
this into smaller patches.

>> Use the GT component for GuC job-timeout checks and engine reset-request
>> failure, which reports them with XE_SIGID_GT_TDR. Use XE_SIGID_MEM_FAULT
>> for the terminal preempt rebind worker failure, since the VM is killed
>> immediately afterwards.
>>
>> v2:
>>   - Rebased on the latest structured SIGID logging series.
>>   - Switched from the old xe_ras_log_() helpers to the new xe_log_()
>>     helpers.
>>   - Dropped paths already covered by the base SIGID series.
> you can keep change log under ---


Noted,

>
>> Cc: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
>> Cc: Badal Nilawar <badal.nilawar@intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Arvind Yadav <arvind.yadav@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guc_submit.c | 12 ++++++++----
>>   drivers/gpu/drm/xe/xe_vm.c         |  5 ++++-
>>   2 files changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>> index 8aaed4fd13ea..79bd0e46640f 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>> @@ -34,6 +34,7 @@
>>   #include "xe_guc_klv_helpers.h"
>>   #include "xe_guc_submit_types.h"
>>   #include "xe_hw_engine.h"
>> +#include "xe_log.h"
>>   #include "xe_lrc.h"
>>   #include "xe_macros.h"
>>   #include "xe_map.h"
>> @@ -1375,7 +1376,8 @@ static bool check_timeout(struct xe_exec_queue *q, struct xe_sched_job *job)
>>   	u64 running_time_ms;
>>   
>>   	if (!xe_sched_job_started(job)) {
>> -		xe_gt_warn(gt, "Check job timeout: seqno=%u, lrc_seqno=%u, guc_id=%d, not started",
>> +		xe_log_err(gt, GT, -ETIMEDOUT,
>> +			   "Check job timeout: seqno=%u, lrc_seqno=%u, guc_id=%d, not started\n",
>>   			   xe_sched_job_seqno(job), xe_sched_job_lrc_seqno(job),
>>   			   q->guc->id);
>>   
>> @@ -1390,7 +1392,8 @@ static bool check_timeout(struct xe_exec_queue *q, struct xe_sched_job *job)
>>   				     xe_sched_job_seqno(job),
>>   				     xe_sched_job_lrc_seqno(job), q->guc->id);
>>   		else
>> -			xe_gt_warn(gt, "Check job timeout: seqno=%u, lrc_seqno=%u, guc_id=%d, timestamp stuck",
>> +			xe_log_err(gt, GT, -ETIMEDOUT,
>> +				   "Check job timeout: seqno=%u, lrc_seqno=%u, guc_id=%d, timestamp stuck\n",
>>   				   xe_sched_job_seqno(job),
>>   				   xe_sched_job_lrc_seqno(job), q->guc->id);
> in both above cases you are promoting from warn to err level
> is this expected/required ?
>
> if we want to log them with SIGID but not with an error level,
> we can use xe_log_err_info() instead
>
> @Matthew, @Rodrigo ?


Good point. I did not intend to silently change the log level. Since 
these two paths were xe_gt_warn(), I will drop these conversions from 
the next revision.

>
> also, early documentation was suggesting that each SIGID should
> be selected based on the source file; now since we have components
> are are more relaxed, but still we have to follow some guidelines
>
> @Aravind, @Rodrigo :
>
> is it ok to use GT in the GuC file?
> or maybe we should introduce GUCSUBMIT/SUBMISSION component with
> associated GT_TDR instead of RUNTIME_FW ?


My intent was to classify the fault, not just the source file. Since 
these are timeout and reset recovery paths, GT_TDR seemed more suitable.
I agree we should follow the existing guidelines, so I will avoid adding 
a new component in this patch.

>>   
>> @@ -3354,8 +3357,9 @@ int xe_guc_exec_queue_reset_failure_handler(struct xe_guc *guc, u32 *msg, u32 le
>>   	reason = msg[2];
>>   
>>   	/* Unexpected failure of a hardware feature, log an actual error */
>> -	xe_gt_err(gt, "GuC engine reset request failed on %d:%d because 0x%08X",
>> -		  guc_class, instance, reason);
>> +	xe_log_err(gt, GT, -EIO,
>> +		   "GuC engine reset request failed on %d:%d because 0x%08X\n",
>> +		   guc_class, instance, reason);
> separate patch ?


Noted,

>>   
>>   	xe_gt_reset_async(gt);
>>   
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index 9e0176861cb6..7c70ea23045a 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -28,6 +28,7 @@
>>   #include "xe_drm_client.h"
>>   #include "xe_exec_queue.h"
>>   #include "xe_gt.h"
>> +#include "xe_log.h"
>>   #include "xe_migrate.h"
>>   #include "xe_pat.h"
>>   #include "xe_pm.h"
>> @@ -591,7 +592,9 @@ static void preempt_rebind_work_func(struct work_struct *w)
>>   	}
>>   
>>   	if (err) {
>> -		drm_warn(&vm->xe->drm, "VM worker error: %d\n", err);
>> +		xe_log_from_recoverable(vm->xe, XE_SIGID_MEM_FAULT,
>> +					XE_LOG_COMPONENT_NONE, ERR_PTR(err), 0,
>> +					"Preempt rebind worker failed\n");
> separate patch ?
>
> maybe we should just introduce new Xe component named VM *)
> and assign it the SIGID MEM_FAULT ?


Agree. I will split the preempt rebind worker conversion into a 
separate patch.
A VM/VMA component mapped to MEM_FAULT would be cleaner than using 
XE_LOG_COMPONENT_NONE, but that looks like an infrastructure change.
I will do the changes accordingly.

>
> *) or maybe better VMA to avoid clash with Virtual Machine?
>
> btw, there is also other existing warn that we may want
> promote to SIGID:


Yes, that looks like a good follow-up candidate. But I will follow the 
current guideline.

>
> 	"Could not allocate memory for fault!\n"
>
>>   		xe_vm_kill(vm, true);
>>   	}
>>   	up_write(&vm->lock);