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

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)
> 
> 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 ---

> 
> Cc: Mallesh Koujalagi <[email protected]>
> Cc: Badal Nilawar <[email protected]>
> Cc: Matthew Brost <[email protected]>
> Cc: Himal Prasad Ghimiray <[email protected]>
> Cc: Michal Wajdeczko <[email protected]>
> Cc: Rodrigo Vivi <[email protected]>
> Signed-off-by: Arvind Yadav <[email protected]>
> ---
>  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 ?

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 ?

>  
> @@ -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 ?
>  
>  	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 ?

*) or maybe better VMA to avoid clash with Virtual Machine?

btw, there is also other existing warn that we may want
promote to SIGID:

	"Could not allocate memory for fault!\n"

>  		xe_vm_kill(vm, true);
>  	}
>  	up_write(&vm->lock);