[igt-dev] [PATCH i-g-t v2] lib/i915/perf: Fix non-card0 processing

[Lionel Landwerlin]

On 05/05/2021 15:41, Janusz Krzysztofik wrote:
> IGT i915/perf library functions now always operate on sysfs perf
> attributes of card0 device node, no matter which DRM device fd a user
> passes.  The intention was to always switch to primary device node if
> a user passes a render device node fd, but that breaks handling of
> non-card0 devices.
>
> If a user passed a render device node fd, find a primary device node of
> the same device and use it instead of forcibly using the primary device
> with minor number 0 when opening the device sysfs area.
>
> v2: Don't assume primary minor matches render minor with masked type.
>
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> ---
>   lib/i915/perf.c | 31 ++++++++++++++++++++++++++++---
>   1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/lib/i915/perf.c b/lib/i915/perf.c
> index 56d5c0b3a..d7768468e 100644
> --- a/lib/i915/perf.c
> +++ b/lib/i915/perf.c
> @@ -372,14 +372,39 @@ open_master_sysfs_dir(int drm_fd)
>   {
>   	char path[128];
>   	struct stat st;
> +	int sysfs;
>   
>   	if (fstat(drm_fd, &st) || !S_ISCHR(st.st_mode))
>                   return -1;
>   
> -        snprintf(path, sizeof(path), "/sys/dev/char/%d:0",
> -                 major(st.st_rdev));
> +	snprintf(path, sizeof(path), "/sys/dev/char/%d:%d", major(st.st_rdev), minor(st.st_rdev));
> +	sysfs = open(path, O_DIRECTORY);

Just to spell out the error paths :


if (sysfs < 0)

     return sysfs;


>   
> -	return open(path, O_DIRECTORY);
> +	if (sysfs >= 0 && minor(st.st_rdev) >= 128) {


Then just if (minor(st.st_rdev) >= 128) { ...

Maybe add a comment above this is : /* If we were given a renderD* 
drm_fd, find it's associated cardX node. */


> +		char device[100], cmp[100];
> +		int device_len, cmp_len, i;
> +
> +		device_len = readlinkat(sysfs, "device", device, sizeof(device));
> +		close(sysfs);
> +		if (device_len < 0)
> +			return device_len;
> +
> +		for (i = 0; i < 128; i++) {
> +
> +			snprintf(path, sizeof(path), "/sys/dev/char/%d:%d", major(st.st_rdev), i);
> +			sysfs = open(path, O_DIRECTORY);
> +			if (sysfs < 0)
> +				continue;
> +
> +			cmp_len = readlinkat(sysfs, "device", cmp, sizeof(cmp));
> +			if (cmp_len == device_len && !memcmp(cmp, device, cmp_len))
> +				break;
> +
> +			close(sysfs);
You might want to set sysfs = -1 here just in the unlikely case this is 
never found.
> +		}
> +	}
> +
> +	return sysfs;
>   }
>   
>   struct intel_perf *


With the proposed changes :

Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>