Re: [PATCH v5 4/5] lib/igt_sysfs: add helpers to access engine sysfs directory

[Andrzej Hajda <andrzej.hajda@intel.com>]

On 29.05.2024 11:21, Kamil Konieczny wrote:
> Hi Andrzej,
> On 2024-05-28 at 16:04:50 +0200, Andrzej Hajda wrote:
>> Helpers follow convention of xe_sysfs_gt_(path|open).
>>
>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
>> ---
>>   lib/igt_sysfs.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   lib/igt_sysfs.h |  3 +++
>>   2 files changed, 74 insertions(+)
>>
>> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
>> index 0c5817eb1580..e11eecc25aa1 100644
>> --- a/lib/igt_sysfs.c
>> +++ b/lib/igt_sysfs.c
>> @@ -40,7 +40,9 @@
>>   #include <dirent.h>
>>   #include <unistd.h>
>>   #include <fcntl.h>
>> +#include <xe_drm.h>
>>   
>> +#include "drmtest.h"
>>   #include "igt_core.h"
>>   #include "igt_sysfs.h"
>>   #include "igt_device.h"
>> @@ -263,6 +265,75 @@ int xe_sysfs_gt_open(int xe_device, int gt)
>>   	return open(path, O_RDONLY);
>>   }
>>   
>> +static const char *engine_class_to_str(__u16 class)
>> +{
>> +	const char *str[] = {
>> +		[DRM_XE_ENGINE_CLASS_RENDER] = "rcs",
>> +		[DRM_XE_ENGINE_CLASS_COPY] = "bcs",
>> +		[DRM_XE_ENGINE_CLASS_VIDEO_DECODE] = "vcs",
>> +		[DRM_XE_ENGINE_CLASS_VIDEO_ENHANCE] = "vecs",
>> +		[DRM_XE_ENGINE_CLASS_COMPUTE] = "ccs",
>> +	};
> These looks like intel-specific global helper.

It is just static helper used only by xe_sysfs_engine_path.
Do you want to make it global? If yes, where it should be put?

>
>> +
>> +	if (class < ARRAY_SIZE(str))
>> +		return str[class];
>> +
>> +	return "unk";
>> +}
>> +
>> +/**
>> + * xe_sysfs_engine_path:
>> + * @xe_device: fd of the device
>> + * @gt: gt number
>> + * @class: engine class
>> + * @path: buffer to fill with the sysfs gt path to the device
>> + * @pathlen: length of @path buffer
>> + *
>> + * Returns:
>> + * The directory path, or NULL on failure.
>> + */
>> +char *
>> +xe_sysfs_engine_path(int xe_device, int gt, int class, char *path, int pathlen)
>> +{
>> +	struct stat st;
>> +	int tile = IS_PONTEVECCHIO(intel_get_drm_devid(xe_device)) ? gt : 0;
>> +
>> +	if (xe_device < 0)
>> +		return NULL;
>> +
>> +	if (igt_debug_on(fstat(xe_device, &st)) || igt_debug_on(!S_ISCHR(st.st_mode)))
>> +		return NULL;
>> +
>> +	snprintf(path, pathlen, "/sys/dev/char/%d:%d/device/tile%d/gt%d/engines/%s",
>> +		 major(st.st_rdev), minor(st.st_rdev), tile, gt, engine_class_to_str(class));
>> +
>> +	if (!access(path, F_OK))
>> +		return path;
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * xe_sysfs_engine_open:
>> + * @xe_device: fd of the device
>> + * @gt: gt number
>> + * @class: engine class
>> + *
>> + * This opens the sysfs gt directory corresponding to device and tile for use
>> + *
>> + * Returns:
>> + * The directory fd, or -1 on failure.
>> + */
>> +int xe_sysfs_engine_open(int xe_device, int gt, int class)
>> +{
>> +	char path[96];
> ------------- ^^
> Is it enough?

Format string has 50 chars, so I guess yes, it should be enough.

Regards
Andrzej

>
> Regards,
> Kamil
>
>> +
>> +	if (!xe_sysfs_engine_path(xe_device, gt, class, path, sizeof(path)))
>> +		return -1;
>> +
>> +	return open(path, O_RDONLY);
>> +}
>> +
>>   /**
>>    * igt_sysfs_gt_path:
>>    * @device: fd of the device
>> diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
>> index f37d80ec130e..6c604d939013 100644
>> --- a/lib/igt_sysfs.h
>> +++ b/lib/igt_sysfs.h
>> @@ -166,4 +166,7 @@ int xe_sysfs_gt_open(int xe_device, int gt);
>>   char *xe_sysfs_tile_path(int xe_device, int tile, char *path, int pathlen);
>>   int xe_sysfs_tile_open(int xe_device, int tile);
>>   int xe_sysfs_get_num_tiles(int xe_device);
>> +char *xe_sysfs_engine_path(int xe_device, int gt, int class, char *path, int pathlen);
>> +int xe_sysfs_engine_open(int xe_device, int gt, int class);
>> +
>>   #endif /* __IGT_SYSFS_H__ */
>>
>> -- 
>> 2.34.1
>>