[PATCH i-g-t v1 1/2] tests/intel/xe_wedged: Ignore more dmesg warnings

[Kamil Konieczny]

Hi Jonathan,
On 2024-10-25 at 17:36:21 +0000, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Kamil Konieczny <kamil.konieczny at linux.intel.com> 
> Sent: Friday, October 25, 2024 10:13 AM
> To: igt-dev at lists.freedesktop.org
> Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>; Cavitt, Jonathan <jonathan.cavitt at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>; Kempczynski, Zbigniew <zbigniew.kempczynski at intel.com>
> Subject: [PATCH i-g-t v1 1/2] tests/intel/xe_wedged: Ignore more dmesg warnings
> > 
> > There are more warnings in dmesg which should be ignored,
> > one comes from driver, for example:
> > 
> > xe 0000:00:02.0: [drm] *ERROR* GT0: reset failed (-ECANCELED)
> > 
> > second from drm:
> > 
> > <4> [438.651441]  ? __pfx___drm_printfn_seq_file+0x10/0x10 [drm]
> > <4> [438.651476]  ? __pfx___drm_puts_seq_file+0x10/0x10 [drm]
> > 
> > so extend regex with them.

Maybe it should be also extended by one more I see on LunarLake without connected
display monitor:

xe 0000:00:02.0: [drm] *ERROR* Failed to write source OUI

so lets wait for CI results and also let me ask display/KMS devs about this.

> > 
> > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2919

I see that also old one was reopened...
https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1958

> > Cc: Jonathan-Cavitt <jonathan.cavitt at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Signed-off-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> 
> I don't see anything wrong with this, though I do have a few open, non-blocking
> questions:
> 
> 1. Could we have called igt_emit_ignore_dmesg_regex multiple times instead of
> needing to use a longform inline regex?  It might've been easier to parse that way.

You could but that will override last one, so it will not stack up,
only last one will 'stay' active during this subtest.
I could add '|' at begin of splitted string, for example:

	igt_emit_ignore_dmesg_regex("CRITICAL: Xe has declared device [0-9A-Fa-f:.]* as wedged"
				    "|GT[0-9A-Fa-f]*: reset failed .-ECANCELED."
				    "|__pfx___drm_[a-z]*seq_file.*drm");

Also spotted now that last dot '.' after '-ECANCELED' is not needed.

> 
> 2. I'm guessing that the igt_emit_ignore_dmesg_regex clears itself after the test
> completes?  I don't see where it clears otherwise, but we've been using this helper
> for a while with no issue, so I suppose it's been correct this entire time.

Strictly speaking it is interpreted by runner/resultsgen.c
during generating results, and yes it is dropped when new (dynamic)subtest
is starting or test exits.

> 
> 3. While I acknowledge that the drm reported errors are expected for this test, may
> I ask for some clarification on why they're expected and/or where they're coming
> from?  This would just be for aiding my understanding of the test/issue.

I do not have any broad knowledge about this, I see in dmesg that a kernel method
of injecting failure is used, so it naturally can lead to a few warnings.
Imho error injection could be intrusive and lead to various errors, also they
are hard to design and write. One simple method which come to mind  would be to
return error from function intended to return a pointer to allocated memory.

Regards,
Kamil

> 
> Thank you for the fix!
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> -Jonathan Cavitt
> 
> > ---
> >  tests/intel/xe_wedged.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/intel/xe_wedged.c b/tests/intel/xe_wedged.c
> > index 88e5d47f2..0e4c88ffe 100644
> > --- a/tests/intel/xe_wedged.c
> > +++ b/tests/intel/xe_wedged.c
> > @@ -29,7 +29,9 @@
> >  static void ignore_wedged_in_dmesg(void)
> >  {
> >  	/* this is needed for igt_runner so it will ignore it */
> > -	igt_emit_ignore_dmesg_regex("CRITICAL: Xe has declared device [0-9A-Fa-f:.]* as wedged");
> > +	igt_emit_ignore_dmesg_regex("CRITICAL: Xe has declared device [0-9A-Fa-f:.]* as wedged|"
> > +				    "GT[0-9A-Fa-f]*: reset failed .-ECANCELED.|"
> > +				    "__pfx___drm_[a-z]*seq_file.*drm");
> >  }
> >  
> >  static void force_wedged(int fd)
> > -- 
> > 2.47.0
> > 
> >