Re: [PATCH v4 1/2] lib/intel_wa: Add workaround check for a wa within debugfs file

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

Shekhar Chauhan <shekhar.chauhan@intel.com> writes:

> Add a check to see if a workaround exists in the debugfs file.

The commit subject and body are a bit vague and doesn't quite reflect
the purpose of this change.  The subject should try to summarize what is
being done; the most important thing in the body is the "why", although
some details of what is being done doesn't hurt as well.

I would go with something like this:

  lib/intel_wa: Also check for device workarounds

  Currently igt_has_intel_wa() only checks for GT workarounds, but in an
  upcoming change we will also need to check for device workarounds.  As
  such, update igt_has_intel_wa() to also check for device workarounds.

  While at it, make sure that we do not duplicate the logic for reading
  from the debugfs file by extracting it into a separate function called
  debugfs_file_has_wa().


I think there is room for improvement on how those functions are
implemented: the use of strstr() is not very robust, since it can return
partial matches; and this function is currently xe-specific (i.e. can't
be used for i915), so we need some assert to make sure that it is being
called only for xe.  However, those are unrelated to this patch and
could be fixed as a follow-up change.

So, with the commit subject and message fixed as suggested above,

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

--
Gustavo Sousa
  
>
> Signed-off-by: Shekhar Chauhan <shekhar.chauhan@intel.com>
> ---
>  lib/intel_wa.c | 40 ++++++++++++++++++++++++++--------------
>  1 file changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/lib/intel_wa.c b/lib/intel_wa.c
> index 685df106f..727dd6c98 100644
> --- a/lib/intel_wa.c
> +++ b/lib/intel_wa.c
> @@ -12,6 +12,27 @@
>  #include "intel_wa.h"
>  #include "xe/xe_query.h"
>  
> +static int debugfs_file_has_wa(int drm_fd, int debugfs_fd,
> +			       const char *debugfs_name, const char *wa)
> +{
> +	char *debugfs_dump;
> +
> +	if (!igt_debugfs_exists(drm_fd, debugfs_name, O_RDONLY))
> +		return -1;
> +
> +	debugfs_dump = igt_sysfs_get(debugfs_fd, debugfs_name);
> +	if (debugfs_dump) {
> +		char *has_wa = strstr(debugfs_dump, wa);
> +
> +		free(debugfs_dump);
> +
> +		if (has_wa)
> +			return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * igt_has_intel_wa:
>   * @drm_fd:	A drm file descriptor
> @@ -25,7 +46,6 @@ int igt_has_intel_wa(int drm_fd, const char *check_wa)
>  	int debugfs_fd;
>  	unsigned int xe;
>  	char name[256];
> -	char *debugfs_dump, *has_wa;
>  
>  	debugfs_fd = igt_debugfs_dir(drm_fd);
>  	if (debugfs_fd == -1)
> @@ -33,22 +53,14 @@ int igt_has_intel_wa(int drm_fd, const char *check_wa)
>  
>  	xe_for_each_gt(drm_fd, xe) {
>  		sprintf(name, "gt%d/workarounds", xe);
> -		if (!igt_debugfs_exists(drm_fd, name, O_RDONLY)) {
> -			ret = -1;
> +		ret = debugfs_file_has_wa(drm_fd, debugfs_fd, name, check_wa);
> +		if (ret)
>  			break;
> -		}
> -
> -		debugfs_dump = igt_sysfs_get(debugfs_fd, name);
> -		if (debugfs_dump) {
> -			has_wa = strstr(debugfs_dump, check_wa);
> -			free(debugfs_dump);
> -			if (has_wa) {
> -				ret = 1;
> -				break;
> -			}
> -		}
>  	}
>  
> +	if (!ret)
> +		ret = debugfs_file_has_wa(drm_fd, debugfs_fd, "workarounds", check_wa);
> +
>  	close(debugfs_fd);
>  	return ret;
>  }
> -- 
> 2.53.0