Re: [PATCH v2 2/2] drm/xe: Remove H2G reads in CT send path in non-debug builds

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

On Mon, Feb 16, 2026 at 01:51:57PM +0100, Francois Dugast wrote:
> mOn Fri, Feb 13, 2026 at 01:16:25PM -0800, Matthew Brost wrote:
> > A single VRAM read on BMG can take over 1µs. While small, this is a
> > non-trivial amount of time in a hot path. Remove the descriptor H2G read
> > (potentially a VRAM access) from non-debug builds, as this
> > error-checking code is not needed outside of debug configurations.
> 
> About the change itself: I understand the performance benefit and this would
> make the code consistent with the 2 other reads.
> 

Yes.

> But the existing block under "if (IS_ENABLED(CONFIG_DRM_XE_DEBUG))" seems wrong
> because it is not just about optionally printing some debug statements, in case
> of error the function actually returns a different value if CONFIG_DRM_XE_DEBUG
> is enabled or not (goto), and this return value is used in __guc_ct_send_locked.
> 

I can't realy argue with this logic but I also see the benefits of
verbose checking of GuC state in CI configs. fwiw, this logic was added
here:

git format-patch -1 d2c5a5a926f43

The cost of reads are actually huge here - this patch by itself is
incomplete as a tracepoint below does a desc_read in the argument list
and this is uncoditionally executed even if ftrace is disabled. On BMG H2G
before this patch / fixing ftrace are 3-4us, after ~300ns. So I'd
suggest we move forward with with a couple of fixes patches first, then
in a follow up either:

The cost of reads is actually huge here — this patch by itself is
incomplete, as a tracepoint below performs a desc_read in the argument
list, and that is unconditionally executed even if ftrace is disabled.
On BMG, H2G reads before this patch / fixing ftrace are 3–4-µs; after,
they’re ~300ns. So I’d suggest we move forward with a couple of fixes
patches first, and then in a follow-up:

- Move all really expensive things in GuC CT layer under
  CONFIG_DRM_XE_DEBUG_GUC, enable this Kconfig some CI run
- Perhaps just assert if GuC state is corrupted in the path mentioned
  above

Matt

> Francois
> 
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_guc_ct.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> > index 6a96bea40720..f200d3ee9d22 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> > @@ -939,22 +939,22 @@ static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
> >  	u32 full_len;
> >  	struct iosys_map map = IOSYS_MAP_INIT_OFFSET(&h2g->cmds,
> >  							 tail * sizeof(u32));
> > -	u32 desc_status;
> >  
> >  	full_len = len + GUC_CTB_HDR_LEN;
> >  
> >  	lockdep_assert_held(&ct->lock);
> >  	xe_gt_assert(gt, full_len <= GUC_CTB_MSG_MAX_LEN);
> >  
> > -	desc_status = desc_read(xe, h2g, status);
> > -	if (desc_status) {
> > -		xe_gt_err(gt, "CT write: non-zero status: %u\n", desc_status);
> > -		goto corrupted;
> > -	}
> > -
> >  	if (IS_ENABLED(CONFIG_DRM_XE_DEBUG)) {
> >  		u32 desc_tail = desc_read(xe, h2g, tail);
> >  		u32 desc_head = desc_read(xe, h2g, head);
> > +		u32 desc_status;
> > +
> > +		desc_status = desc_read(xe, h2g, status);
> > +		if (desc_status) {
> > +			xe_gt_err(gt, "CT write: non-zero status: %u\n", desc_status);
> > +			goto corrupted;
> > +		}
> >  
> >  		if (tail != desc_tail) {
> >  			desc_write(xe, h2g, status, desc_status | GUC_CTB_STATUS_MISMATCH);
> > -- 
> > 2.34.1
> >