Re: [PATCH v4 20/34] drm/xe/vf: Use GUC_HXG_TYPE_EVENT for GuC context register

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

On Fri, Oct 03, 2025 at 04:26:03PM +0200, Lis, Tomasz wrote:
> 
> On 10/2/2025 7:53 AM, Matthew Brost wrote:
> > The only case where the GuC submission backend cannot reason 100%
> > correctly is when a GuC context is registered during VF post-migration
> > recovery. In this scenario, it's possible that the GuC context register
> > H2G is processed, but the immediately following schedule-enable H2G gets
> > lost.
> Shouldn't the solution then be to treat the schedule-enable H2G as separate
> state, and be able to revert to it?
> > 
> > A double register is harmless when using `GUC_HXG_TYPE_EVENT`, as GuC
> > simply drops the duplicate H2G. To keep things simple, use
> > `GUC_HXG_TYPE_EVENT` for all context registrations on VFs.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_guc_ct.c | 32 ++++++++++++++++++++++++--------
> >   1 file changed, 24 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> > index d0fde371fae3..d84de8544532 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> > @@ -736,6 +736,26 @@ static u16 next_ct_seqno(struct xe_guc_ct *ct, bool is_g2h_fence)
> >   	return seqno;
> >   }
> > +#define MAKE_ACTION(type, __action)				\
> > +({								\
> > +	FIELD_PREP(GUC_HXG_MSG_0_TYPE, type) |			\
> > +	FIELD_PREP(GUC_HXG_EVENT_MSG_0_ACTION |			\
> > +		   GUC_HXG_EVENT_MSG_0_DATA0, __action);	\
> > +})
> > +
> > +static bool vf_action_can_safely_fail(struct xe_device *xe, u32 action)
> Something is very wrong with that name. Context registration can't "safely
> fail".

What? This name pefectly describes what this doing - these VF actions
can safely fail in the GuC. 

> > +{
> > +	/*
> > +	 * If we are VF resuming, we can't exactly track if a context
> > +	 * registration has been completed in the GuC state machine, it is
> > +	 * harmless to resend as it will just fail silently if
> > +	 * GUC_HXG_TYPE_EVENT is used.
> > +	 */
> > +	return IS_SRIOV_VF(xe) &&
> > +		(action == XE_GUC_ACTION_REGISTER_CONTEXT_MULTI_LRC ||
> > +		 action == XE_GUC_ACTION_REGISTER_CONTEXT);
> 
> Shouldn't we at the very least limit that to vf migration enabled?
> 

Sure.

> Looks to me like we're completely ripping out context registration errors
> propagation. From the kalloc() fixup patches, I was under impression that
> error support and propagation is important for Xe. And here, we're willingly
> damaging it?
> 

This is fundamentally different than omitting a bug check which crashes
the kernel.

> Can we really ignore them? If we really must, shouldn't we mimic GuC checks
> here on the KMD side, to at least decrease probability of problems?
> 
> This will cause possible error messages in GuC log, which do not indicate
> real errors. We're willingly sending a message which may result in an error.
> 
> Having error messages in GuC log which are considered normal flow on KMD
> side would have to be properly documented, to avoid people debugging
> non-existing issues. Best in several places.
> 
> 
> I don't know about this, we're creating a bad situation by choice.
> 

I've already explained this. Not ideal but I don't see any better
options. If we remove this a fast req can randomly fail with the right
race condition trashing the VF as that results in a GT reset.

This error check is protocol level check in Xe. If we had protocol
problem with H2G it would show up all platforms in our CI. We are not
losing any coverage as this check is enabled everywhere but a VF.

Matt

> -Tomasz
> 
> > +}
> > +
> >   #define H2G_CT_HEADERS (GUC_CTB_HDR_LEN + 1) /* one DW CTB header and one DW HxG header */
> >   static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
> > @@ -807,18 +827,14 @@ static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
> >   		FIELD_PREP(GUC_CTB_MSG_0_NUM_DWORDS, len) |
> >   		FIELD_PREP(GUC_CTB_MSG_0_FENCE, ct_fence_value);
> >   	if (want_response) {
> > -		cmd[1] =
> > -			FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |
> > -			FIELD_PREP(GUC_HXG_EVENT_MSG_0_ACTION |
> > -				   GUC_HXG_EVENT_MSG_0_DATA0, action[0]);
> > +		cmd[1] = MAKE_ACTION(GUC_HXG_TYPE_REQUEST, action[0]);
> > +	} else if (vf_action_can_safely_fail(xe, action[0])) {
> > +		cmd[1] = MAKE_ACTION(GUC_HXG_TYPE_EVENT, action[0]);
> >   	} else {
> >   		fast_req_track(ct, ct_fence_value,
> >   			       FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, action[0]));
> > -		cmd[1] =
> > -			FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_FAST_REQUEST) |
> > -			FIELD_PREP(GUC_HXG_EVENT_MSG_0_ACTION |
> > -				   GUC_HXG_EVENT_MSG_0_DATA0, action[0]);
> > +		cmd[1] = MAKE_ACTION(GUC_HXG_TYPE_FAST_REQUEST, action[0]);
> >   	}
> >   	/* H2G header in cmd[1] replaces action[0] so: */