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

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