Re: [PATCH 11/16] drm/xe/multi_queue: Handle CGP context error

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Nov 03, 2025 at 08:44:16AM -0800, Niranjana Vishwanathapura wrote:
> On Sun, Nov 02, 2025 at 10:29:32AM -0800, Matthew Brost wrote:
> > On Fri, Oct 31, 2025 at 11:29:31AM -0700, Niranjana Vishwanathapura wrote:
> > > Trigger multi-queue context cleanup upon CGP context error
> > > notification from GuC.
> > > 
> > > Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/abi/guc_actions_abi.h |  1 +
> > >  drivers/gpu/drm/xe/xe_guc_ct.c           |  4 +++
> > >  drivers/gpu/drm/xe/xe_guc_submit.c       | 33 ++++++++++++++++++++++++
> > >  drivers/gpu/drm/xe/xe_guc_submit.h       |  2 ++
> > >  drivers/gpu/drm/xe/xe_trace.h            |  5 ++++
> > >  5 files changed, 45 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/abi/guc_actions_abi.h b/drivers/gpu/drm/xe/abi/guc_actions_abi.h
> > > index 3e9fbed9cda6..8af3691626bf 100644
> > > --- a/drivers/gpu/drm/xe/abi/guc_actions_abi.h
> > > +++ b/drivers/gpu/drm/xe/abi/guc_actions_abi.h
> > > @@ -142,6 +142,7 @@ enum xe_guc_action {
> > >  	XE_GUC_ACTION_REGISTER_CONTEXT_MULTI_QUEUE = 0x4602,
> > >  	XE_GUC_ACTION_MULTI_QUEUE_CONTEXT_CGP_SYNC = 0x4603,
> > >  	XE_GUC_ACTION_NOTIFY_MULTI_QUEUE_CONTEXT_CGP_SYNC_DONE = 0x4604,
> > > +	XE_GUC_ACTION_NOTIFY_MULTI_QUEUE_CGP_CONTEXT_ERROR = 0x4605,
> > >  	XE_GUC_ACTION_CLIENT_SOFT_RESET = 0x5507,
> > >  	XE_GUC_ACTION_SET_ENG_UTIL_BUFF = 0x550A,
> > >  	XE_GUC_ACTION_SET_DEVICE_ENGINE_ACTIVITY_BUFFER = 0x550C,
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> > > index 48b5006eb080..d0e19af0b4d2 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> > > @@ -1574,6 +1574,10 @@ static int process_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
> > >  	case XE_GUC_ACTION_NOTIFY_MULTI_QUEUE_CONTEXT_CGP_SYNC_DONE:
> > >  		ret = xe_guc_exec_queue_cgp_sync_done_handler(guc, payload, adj_len);
> > >  		break;
> > > +	case XE_GUC_ACTION_NOTIFY_MULTI_QUEUE_CGP_CONTEXT_ERROR:
> > > +		ret = xe_guc_exec_queue_cgp_context_error_handler(guc, payload,
> > > +								  adj_len);
> > > +		break;
> > >  	default:
> > >  		xe_gt_err(gt, "unexpected G2H action 0x%04x\n", action);
> > >  	}
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > index 87c13feb2cef..605352145d76 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > @@ -48,6 +48,8 @@
> > >  #include "xe_vm.h"
> > >  #include "xe_bo.h"
> > > 
> > > +#define XE_GUC_EXEC_QUEUE_CGP_CONTEXT_ERROR_LEN		6
> > > +
> > >  static struct xe_guc *
> > >  exec_queue_to_guc(struct xe_exec_queue *q)
> > >  {
> > > @@ -3001,6 +3003,37 @@ int xe_guc_exec_queue_reset_failure_handler(struct xe_guc *guc, u32 *msg, u32 le
> > >  	return 0;
> > >  }
> > > 
> > > +int xe_guc_exec_queue_cgp_context_error_handler(struct xe_guc *guc, u32 *msg,
> > > +						u32 len)
> > > +{
> > > +	struct xe_gt *gt = guc_to_gt(guc);
> > > +	struct xe_device *xe = guc_to_xe(guc);
> > > +	struct xe_exec_queue *q;
> > > +	u32 guc_id = msg[2];
> > > +
> > > +	if (unlikely(len != XE_GUC_EXEC_QUEUE_CGP_CONTEXT_ERROR_LEN)) {
> > > +		drm_err(&xe->drm, "Invalid length %u", len);
> > > +		return -EPROTO;
> > > +	}
> > > +
> > > +	q = g2h_exec_queue_lookup(guc, guc_id);
> > > +	if (unlikely(!q))
> > > +		return -EPROTO;
> > > +
> > > +	xe_gt_dbg(gt,
> > > +		  "CGP context error: region=%s err=0x%x, context=0x%x LRCA=0x%x:0x%x SgId=0x%x",
> > > +		  msg[0] & 1 ? "uc" : "kmd", msg[1], msg[2], msg[4], msg[3], msg[5]);
> > > +
> > > +	trace_xe_exec_queue_cgp_context_error(q);
> > > +
> > > +	/* Treat the same as engine reset */
> > > +	set_exec_queue_reset(q);
> > > +	if (!exec_queue_banned(q) && !exec_queue_check_timeout(q))
> > 
> > I don't think you need the exec_queue_check_timeout check.
> > 
> 
> The check here is same as in other guc error handlers like
> xe_guc_exec_queue_reset_handler() and xe_guc_exec_queue_memory_cat_error_handler().

Ah, it is but it doesn't seem right in those cases either. Maybe I'm
forgeting something, will try page this information back in.

> Hence the reason to keep it here also. Doesn't exec_queue_check_timeout()
> mean TDR is already underway?

Yes, it does.

So with the current state of the code, this is correct, so:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

If I reason this can be dropped, let's drop it everywhere in a single
shot.

Matt

> 
> Niranjana
> 
> > Otherwise LGTM.
> > 
> > Matt
> > 
> > > +		xe_guc_exec_queue_trigger_cleanup(q);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /**
> > >   * xe_guc_exec_queue_cgp_sync_done_handler - CGP synchronization done handler
> > >   * @guc: guc
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.h b/drivers/gpu/drm/xe/xe_guc_submit.h
> > > index abfa94bce391..01b013a90b1b 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_submit.h
> > > +++ b/drivers/gpu/drm/xe/xe_guc_submit.h
> > > @@ -35,6 +35,8 @@ int xe_guc_exec_queue_memory_cat_error_handler(struct xe_guc *guc, u32 *msg,
> > >  int xe_guc_exec_queue_reset_failure_handler(struct xe_guc *guc, u32 *msg, u32 len);
> > >  int xe_guc_error_capture_handler(struct xe_guc *guc, u32 *msg, u32 len);
> > >  int xe_guc_exec_queue_cgp_sync_done_handler(struct xe_guc *guc, u32 *msg, u32 len);
> > > +int xe_guc_exec_queue_cgp_context_error_handler(struct xe_guc *guc, u32 *msg,
> > > +						u32 len);
> > > 
> > >  struct xe_guc_submit_exec_queue_snapshot *
> > >  xe_guc_exec_queue_snapshot_capture(struct xe_exec_queue *q);
> > > diff --git a/drivers/gpu/drm/xe/xe_trace.h b/drivers/gpu/drm/xe/xe_trace.h
> > > index 79a97b086cb2..c9d0748dae9d 100644
> > > --- a/drivers/gpu/drm/xe/xe_trace.h
> > > +++ b/drivers/gpu/drm/xe/xe_trace.h
> > > @@ -172,6 +172,11 @@ DEFINE_EVENT(xe_exec_queue, xe_exec_queue_memory_cat_error,
> > >  	     TP_ARGS(q)
> > >  );
> > > 
> > > +DEFINE_EVENT(xe_exec_queue, xe_exec_queue_cgp_context_error,
> > > +	     TP_PROTO(struct xe_exec_queue *q),
> > > +	     TP_ARGS(q)
> > > +);
> > > +
> > >  DEFINE_EVENT(xe_exec_queue, xe_exec_queue_stop,
> > >  	     TP_PROTO(struct xe_exec_queue *q),
> > >  	     TP_ARGS(q)
> > > --
> > > 2.43.0
> > >