Re: [PATCH 1/1] drm/xe: Restore engine registers before restarting schedulers after GT reset

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

On Wed, Dec 10, 2025 at 11:25:43AM -0700, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Jan Maslak
> Sent: Wednesday, December 10, 2025 6:56 AM
> To: intel-xe@lists.freedesktop.org
> Cc: Patelczyk, Maciej <maciej.patelczyk@intel.com>; joonas.lahtinen@linux.intel.com; Brost, Matthew <matthew.brost@intel.com>; Maslak, Jan <jan.maslak@intel.com>
> Subject: [PATCH 1/1] drm/xe: Restore engine registers before restarting schedulers after GT reset
> > 
> > During GT reset recovery in do_gt_restart(), xe_uc_start() was called
> > before xe_reg_sr_apply_mmio() restored engine-specific registers. This
> > created a race window where the scheduler could run jobs before hardware
> > state was fully restored.
> > 
> > This caused failures in eudebug tests (xe_exec_sip_eudebug@breakpoint-
> > waitsip-*) where TD_CTL register (containing TD_CTL_GLOBAL_DEBUG_ENABLE)
> > wasn't restored before jobs started executing. Breakpoints would fail to
> > trigger SIP entry because the debug enable bit wasn't set yet.
> > 
> > Fix by moving xe_uc_start() after all MMIO register restoration,
> > including engine registers and CCS mode configuration, ensuring all
> > hardware state is fully restored before any jobs can be scheduled.
> > 
> > Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> > Signed-off-by: Jan Maslak <jan.maslak@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_gt.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> > index 7caf781ba9e8..313ce83ab0e5 100644
> > --- a/drivers/gpu/drm/xe/xe_gt.c
> > +++ b/drivers/gpu/drm/xe/xe_gt.c
> > @@ -771,9 +771,6 @@ static int do_gt_restart(struct xe_gt *gt)
> >  		xe_gt_sriov_pf_init_hw(gt);
> >  
> >  	xe_mocs_init(gt);
> > -	err = xe_uc_start(&gt->uc);
> > -	if (err)
> > -		return err;
> >  
> >  	for_each_hw_engine(hwe, gt, id)
> >  		xe_reg_sr_apply_mmio(&hwe->reg_sr, gt);
> > @@ -781,6 +778,10 @@ static int do_gt_restart(struct xe_gt *gt)
> >  	/* Get CCS mode in sync between sw/hw */
> >  	xe_gt_apply_ccs_mode(gt);
> >  
> > +	err = xe_uc_start(&gt->uc);
> > +	if (err)
> > +		return err;
> > +
> 
> This probably doesn't need to go after xe_gt_sanitize_freq (or, in other
> words, this is probably the correct place to move this to), so:
> 

Yes, xe_gt_sanitize_freq relies on the GuC/CTs being live, so this is
the correct place.

With that:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> 
> -Jonathan Cavitt
> 
> >  	/* Restore GT freq to expected values */
> >  	xe_gt_sanitize_freq(gt);
> >  
> > -- 
> > 2.34.1
> > 
> >