Re: [PATCH i-g-t 05/10] lib/xe: Return boolean from xe_wa()

[Gustavo Sousa <gustavo.sousa@intel.com>]

Matt Roper <matthew.d.roper@intel.com> writes:

> On Tue, Aug 18, 2026 at 03:08:34PM -0300, Gustavo Sousa wrote:
>> Matt Roper <matthew.d.roper@intel.com> writes:
>> 
>> > On Tue, Jul 21, 2026 at 03:59:58PM -0300, Gustavo Sousa wrote:
>> >> The function xe_wa() will cause a warning to be printed when an error
>> >> condition is found.  Since the current users of xe_wa() do not check
>> >> for errors, let's just convert the function to return a boolean.
>> >> 
>> >> Checking for xe_wa(...) instead of xe_wa(...) > 0 feels more natural.
>> >> 
>> >> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> >> ---
>> >>  lib/xe/xe_wa.c      | 11 ++++++-----
>> >>  lib/xe/xe_wa.h      |  4 +++-
>> >>  tests/intel/xe_oa.c |  2 +-
>> >>  3 files changed, 10 insertions(+), 7 deletions(-)
>> >> 
>> >> diff --git a/lib/xe/xe_wa.c b/lib/xe/xe_wa.c
>> >> index 0f4d2edc0795..8c8f7156c21f 100644
>> >> --- a/lib/xe/xe_wa.c
>> >> +++ b/lib/xe/xe_wa.c
>> >> @@ -108,20 +108,21 @@ static bool debugfs_dump_has_wa(char *dump, const char *wa)
>> >>   * @fd: A drm file descriptor.
>> >>   * @wa: Name of the workaround to be checked.
>> >>   *
>> >> - * Returns 1 if enabled, 0 if disabled, -1 on error.
>> >> + * Return a boolean indicating whether the workaround is enabled.
>> >> + * On error, returns false and a warning is printed.
>> >>   */
>> >> -int xe_wa(int fd, const char *wa)
>> >> +bool xe_wa(int fd, const char *wa)
>> >>  {
>> >>  	char **dumps = xe_device_get(fd)->wa_cache;
>> >>  
>> >>  	if (igt_warn_on(!dumps))
>> >> -		return -1;
>> >> +		return false;
>> >
>> > Should we use an igt_assert() so that failures here get propagated up as
>> > test failures?
>> 
>> I'm not sure.  In the past I advocated for stuff under lib/ to avoid
>> causing asserts and leave that for real test code...
>> 
>> Cc'ing Ashutosh here, since he was involved in that discussion.
>> 
>> Do we have strong reasons to do an igt_assert() here?
>> 
>> A workaround like Wa_14026539277 will be checked in many tests and the
>> assert failure will cause disruptions on many unrelated platforms.
>
> If we're not able to accurately determine the workaround list, then any
> test which is calling xe_wa() to check workarounds has effectively
> become unreliable and we can't trust that it's giving accurate results
> anymore.  That would mean we have general IGT infrastructure breakage
> that needs to be resolved before we can trust the results of those
> tests.

Fair.  I'll change this to use igt_assert() in the next
iteration.

>
> It would be nice if there was a cleaner way to differentiate IGT's
> library-internal assertions from regular test result failures, but from
> grep'ing the current codebase, the use of igt_assert in lib/ seems to be
> the common way to handle this.

What differentiation did you have in mind? Making library-internal
assertions non-fatal? Or just how we report them?

--
Gustavo Sousa

>
>
> Matt
>
>> 
>> --
>> Gustavo Sousa
>> 
>> >
>> >
>> > Matt
>> >
>> >>  
>> >>  	for (char **dump = dumps; *dump; dump++)
>> >>  		if (debugfs_dump_has_wa(*dump, wa))
>> >> -			return 1;
>> >> +			return true;
>> >>  
>> >> -	return 0;
>> >> +	return false;
>> >>  }
>> >>  
>> >>  /**
>> >> diff --git a/lib/xe/xe_wa.h b/lib/xe/xe_wa.h
>> >> index f0a826553df8..aa1c50c0f9c9 100644
>> >> --- a/lib/xe/xe_wa.h
>> >> +++ b/lib/xe/xe_wa.h
>> >> @@ -6,9 +6,11 @@
>> >>  #ifndef XE_WA_H
>> >>  #define XE_WA_H
>> >>  
>> >> +#include <stdbool.h>
>> >> +
>> >>  struct xe_device;
>> >>  
>> >> -int xe_wa(int fd, const char *wa);
>> >> +bool xe_wa(int fd, const char *wa);
>> >>  
>> >>  void xe_wa_build_cache(struct xe_device *xe_dev);
>> >>  void xe_wa_free_cache(struct xe_device *xe_dev);
>> >> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
>> >> index fcf1d71a167f..bdaa7141004b 100644
>> >> --- a/tests/intel/xe_oa.c
>> >> +++ b/tests/intel/xe_oa.c
>> >> @@ -2678,7 +2678,7 @@ test_non_zero_reason(const struct drm_xe_oa_unit *oau, size_t oa_buffer_size)
>> >>  	 * can result in buffer overflows.
>> >>  	 */
>> >>  	if (oau->oa_unit_type == DRM_XE_OA_UNIT_TYPE_MERT &&
>> >> -	    xe_wa(drm_fd, "14026633728") > 0) {
>> >> +	    xe_wa(drm_fd, "14026633728")) {
>> >>  		oa_exponent = max(oa_exponent, 8);
>> >>  		properties[9] = oa_exponent;
>> >>  	}
>> >> 
>> >> -- 
>> >> 2.55.0
>> >> 
>> >
>> > -- 
>> > Matt Roper
>> > Graphics Software Engineer
>> > Linux GPU Platform Enablement
>> > Intel Corporation
>
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation