[PATCH v4] drm/xe: Add helper function to inject fault into ct_dead_capture()

[K V P, Satyanarayana]

Hi.
> -----Original Message-----
> From: Harrison, John C <john.c.harrison at intel.com>
> Sent: Thursday, May 22, 2025 3:01 AM
> To: K V P, Satyanarayana <satyanarayana.k.v.p at intel.com>; intel-
> xe at lists.freedesktop.org
> Cc: Wajdeczko, Michal <Michal.Wajdeczko at intel.com>; Nikula, Jani
> <jani.nikula at intel.com>
> Subject: Re: [PATCH v4] drm/xe: Add helper function to inject fault into
> ct_dead_capture()
> 
> On 5/15/2025 3:19 AM, Satyanarayana K V P wrote:
> > When injecting fault to xe_guc_ct_send_recv() & xe_guc_mmio_send_recv()
> > functions, the CI test systems are going out of space and crashing. To
> > avoid this issue, a new helper function is created and when fault is
> > injected into this xe_is_injection_active() helper function,
> > ct dead capture is avoided which suppresses ct dumps in the log.
> >
> > Signed-off-by: Satyanarayana K V P <satyanarayana.k.v.p at intel.com>
> > Suggested-by: John Harrison <John.C.Harrison at Intel.com>
> > ---
> > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > Cc: Jani Nikula <jani.nikula at intel.com>
> >
> > V3 -> V4:
> > - Updated the name of helper function and moved to xe_device.h file.
> >
> > V2 -> V3:
> > - Added inline function to avoid compilation error in the absence of
> > CONFIG_FUNCTION_ERROR_INJECTION.
> >
> > V1 -> V2:
> > - Fixed review comments.
> > ---
> >   drivers/gpu/drm/xe/xe_device.h | 18 ++++++++++++++++++
> >   drivers/gpu/drm/xe/xe_guc_ct.c |  6 ++++++
> >   2 files changed, 24 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_device.h
> b/drivers/gpu/drm/xe/xe_device.h
> > index 0bc3bc8e6803..b937d585083b 100644
> > --- a/drivers/gpu/drm/xe/xe_device.h
> > +++ b/drivers/gpu/drm/xe/xe_device.h
> > @@ -195,6 +195,24 @@ void xe_device_declare_wedged(struct xe_device
> *xe);
> >   struct xe_file *xe_file_get(struct xe_file *xef);
> >   void xe_file_put(struct xe_file *xef);
> >
> > +/**
> > + * is_injection_active() - Helper function to inject fault.
> Technically, this is not injecting the fault. It is a place holder to
> have a fault injected in to so that it can report to other code that a
> fault injection test is running.
> 
Defined a new function to inject fault and this function returns the fault status.
> > + *
> > + * This is a helper function to inject fault into ct_dead_capture().
> Given that this is now a generic function in a a generic location, it
> should not really talk about specific instances of usage.
> 
> I would say something like:
> 
> +/**
> + * is_injection_active() - Helper function to test whether a fault
> inject test is running.
> + *
> + * This is a helper function that the driver can use to detect whether
> a fault injection
> + * test is running in order to suppress excessive debug output. By
> default, the return
> + * value is fixed as zero but it can be modified by the fault injection
> framework to
> + * return an error.
> + *
> + * Returns:
> + * 0 if fault injection is not
> + *-EINVAL if fault injection is active
> 
Updated in the new revision.
> Or whatever error was picked by the IGT side?
> 
> > + * As fault is injected using this function, need to make sure that
> > + * the compiler does not optimize and make it as a inline function.
> > + * To prevent compile optimization, "noinline" is added.
> > + */
> > +#ifdef CONFIG_FUNCTION_ERROR_INJECTION
> > +static noinline int xe_is_injection_active(void)
> > +{
> > +	        return 0;
> Is the indentation correct here? It is showing as double intended in the
> email reader.
> 
> > +}
> > +ALLOW_ERROR_INJECTION(xe_is_injection_active, ERRNO);
> > +#else
> > +static inline int xe_is_injection_active(void) { return 0; }
> Why have the first version spread over three lines but this version
> compressed to a single line? The code is identical except for 'noinline'
> vs 'inline' isn't it?
> 
> You could even do it as:
> +#ifdef CONFIG_FUNCTION_ERROR_INJECTION
> +ALLOW_ERROR_INJECTION(xe_is_injection_active, ERRNO);
> +static noinline
> +#else
> +static inline
> +#endif
> +int xe_is_injection_active(void)
> +{
> +            return 0;
> +}
> 
> Although I guess the ALLOW_ macro needs to be after the function definition?
> 
> John.
> 
ALLOW_ERROR_INJECTION() needs to be after function definition.
> > +#endif
> > +
> >   /*
> >    * Occasionally it is seen that the G2H worker starts running after a delay of
> more than
> >    * a second even after being queued and activated by the Linux workqueue
> subsystem. This
> > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c
> b/drivers/gpu/drm/xe/xe_guc_ct.c
> > index ff6edf1b14f4..a6964d13f3a3 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> > @@ -1963,6 +1963,12 @@ static void ct_dead_capture(struct xe_guc_ct
> *ct, struct guc_ctb *ctb, u32 reaso
> >
> >   	if (ctb)
> >   		ctb->info.broken = true;
> > +	/*
> > +	 * Huge dump is getting generated when injecting error for guc
> CT/MMIO
> > +	 * functions. So, let us suppress the dump when fault is injected.
> > +	 */
> > +	if (xe_is_injection_active())
> > +		return;
> >
> >   	/* Ignore further errors after the first dump until a reset */
> >   	if (ct->dead.reported)