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

[Francois Dugast <francois.dugast@intel.com>]

On Mon, Feb 16, 2026 at 09:40:19AM -0800, Matthew Brost wrote:
> 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

Sounds good.

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