Re: [PATCH] drm/xe: Do not preempt fence signaling CS instructions

["Summers, Stuart" <stuart.summers@intel.com>]

On Fri, 2026-01-16 at 08:43 -0800, Daniele Ceraolo Spurio wrote:
> 
> 
> On 1/16/2026 2:12 AM, Francois Dugast wrote:
> > On Fri, Jan 16, 2026 at 10:45:39AM +0100, Zbigniew Kempczyński
> > wrote:
> > > On Wed, Jan 14, 2026 at 04:45:46PM -0800, Matthew Brost wrote:
> > > > If a batch buffer is complete, it makes little sense to preempt
> > > > the
> > > > fence signaling instructions in the ring, as the largest
> > > > portion of the
> > > > work (the batch buffer) is already done and fence signaling
> > > > consists of
> > > > only a few instructions. If these instructions are preempted,
> > > > the GuC
> > > > would need to perform a context switch just to signal the
> > > > fence, which
> > > > is costly and delays fence signaling. Avoid this scenario by
> > > > disabling
> > > > preemption immediately after the BB start instruction and re-
> > > > enabling it
> > > > after executing the fence signaling instructions.
> > > > 
> > > > Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for
> > > > Intel GPUs")
> > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > > Cc: Carlos Santa <carlos.santa@intel.com>
> > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/xe/xe_ring_ops.c | 9 +++++++++
> > > >   1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c
> > > > b/drivers/gpu/drm/xe/xe_ring_ops.c
> > > > index a1fd99f2d539..cd645ee400b9 100644
> > > > --- a/drivers/gpu/drm/xe/xe_ring_ops.c
> > > > +++ b/drivers/gpu/drm/xe/xe_ring_ops.c
> > > > @@ -282,6 +282,9 @@ static void __emit_job_gen12_simple(struct
> > > > xe_sched_job *job, struct xe_lrc *lrc
> > > >   
> > > >         i = emit_bb_start(batch_addr, ppgtt_flag, dw, i);
> > > >   
> > > > +       /* Don't preempt fence signaling */
> > > > +       dw[i++] = MI_ARB_ON_OFF | MI_ARB_DISABLE;
> > > > +
> > > >         if (job->user_fence.used) {
> > > >                 i = emit_flush_dw(dw, i);
> > > >                 i = emit_store_imm_ppgtt_posted(job-
> > > > >user_fence.addr,
> > > > @@ -347,6 +350,9 @@ static void __emit_job_gen12_video(struct
> > > > xe_sched_job *job, struct xe_lrc *lrc,
> > > >   
> > > >         i = emit_bb_start(batch_addr, ppgtt_flag, dw, i);
> > > >   
> > > > +       /* Don't preempt fence signaling */
> > > > +       dw[i++] = MI_ARB_ON_OFF | MI_ARB_DISABLE;
> > > > +
> > > >         if (job->user_fence.used) {
> > > >                 i = emit_flush_dw(dw, i);
> > > >                 i = emit_store_imm_ppgtt_posted(job-
> > > > >user_fence.addr,
> > > > @@ -399,6 +405,9 @@ static void
> > > > __emit_job_gen12_render_compute(struct xe_sched_job *job,
> > > >   
> > > >         i = emit_bb_start(batch_addr, ppgtt_flag, dw, i);
> > > >   
> > > > +       /* Don't preempt fence signaling */
> > > > +       dw[i++] = MI_ARB_ON_OFF | MI_ARB_DISABLE;
> > > > +
> > > IGT tests which calls compute-walker, then bbe are asynchronous
> > > (don't
> > > wait for completion, pipe-control is necessary to wait on
> > > compute-walker).
> > > 
> > > May you try to put arb disable after emit_render_cache_flush?
> > Thanks Zbigniew, xe_compute_preempt tests do pass with this change:
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c
> > b/drivers/gpu/drm/xe/xe_ring_ops.c
> > index cd645ee400b9..d8cceab97fa8 100644
> > --- a/drivers/gpu/drm/xe/xe_ring_ops.c
> > +++ b/drivers/gpu/drm/xe/xe_ring_ops.c
> > @@ -405,11 +405,11 @@ static void
> > __emit_job_gen12_render_compute(struct xe_sched_job *job,
> >   
> >          i = emit_bb_start(batch_addr, ppgtt_flag, dw, i);
> >   
> > +       i = emit_render_cache_flush(job, dw, i);
> > +
> 
> The pipe control in emit_render_cache_flush is preemptable, so having
> that before the arb off switch invalidates what the patch is trying
> to 
> do (i.e., no preemption points after the bb completes until we signal
> the fence).
> 
> Why does disabling arbitration cause this specific pipe control to
> hang?

Are we enabling/disabling preemption from the batch too? It seems like
the batch preemption control should be owned by the user and not rely
on the ring configuration here (which might have other intention as
seen here).

Also would be interesting to know if the compute/render UMD compliance
tests are passing with this change.

Thanks,
Stuart

> 
> Daniele
> 
> >          /* Don't preempt fence signaling */
> >          dw[i++] = MI_ARB_ON_OFF | MI_ARB_DISABLE;
> >   
> > -       i = emit_render_cache_flush(job, dw, i);
> > -
> >          if (job->user_fence.used)
> >                  i = emit_store_imm_ppgtt_posted(job-
> > >user_fence.addr,
> >                                                  job-
> > >user_fence.value,
> > 
> > 
> > Francois
> > 
> > > --
> > > Zbigniew
> > > 
> > > >         i = emit_render_cache_flush(job, dw, i);
> > > >   
> > > >         if (job->user_fence.used)
> > > > -- 
> > > > 2.34.1
> > > > 
>