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

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

Gustavo Sousa <gustavo.sousa@intel.com> writes:

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

Since that's arguably a separate logical change, I did this as a new
patch that will be placed after this one in v2,

--
Gustavo Sousa

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