Re: [igt-dev] [PATCH] tools/intel_gpu_top: Return the error to the caller of pmu_init

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Wed, Jun 07, 2023 at 10:10:51AM +0200, Kamil Konieczny wrote:
>On 2023-06-06 at 21:29:30 +0000, Umesh Nerlige Ramappa wrote:
>> When run as a regular user, we see an assert instead of a proper error
>> message.  Propagate errors correctly to caller.
>>
>> Link: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/issues/139
>- ^
>Maybe Closes: ?

right. I get confused often on this. Will change to Closes.
>
>> Fixes: (c67825ba40de: intel_gpu_top: Determine number of tiles)
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> ---
>>  tools/intel_gpu_top.c | 16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
>> index 7018499c..24fba88b 100644
>> --- a/tools/intel_gpu_top.c
>> +++ b/tools/intel_gpu_top.c
>> @@ -536,7 +536,7 @@ static void imc_reads_open(struct pmu_counter *pmu, struct engines *engines)
>>  	imc_open(pmu, "data_reads", engines);
>>  }
>>
>> -static int get_num_gts(uint64_t type)
>> +static int get_num_gts(uint64_t type, int *num_gts)
>>  {
>>  	int fd, cnt;
>>
>> @@ -548,11 +548,13 @@ static int get_num_gts(uint64_t type)
>>
>>  		close(fd);
>>  	}
>> -	assert(!errno || errno == ENOENT);
>> +	if (errno && errno != ENOENT)
>> +		return fd;
>
>What happens for ENOENT ?

KMD implements the frequency event only for available GTs and returns an 
ENOENT if user passes an event that is not available.

ENOENT is used to dynamically determine how many GTs there are.

This resolves the bug, but ideally, I think the original message that 
was printed by intel_gpu_top stating that CAP_PERFMON is required should 
check for EPERM, but I am looking for more comments on that.

Regards,
Umesh

>
>Regards,
>Kamil
>
>> +
>>  	assert(cnt > 0);
>> -	errno = 0;
>> +	*num_gts = cnt;
>>
>> -	return cnt;
>> +	return 0;
>>  }
>>
>>  static void init_aggregate_counters(struct engines *engines)
>> @@ -578,12 +580,14 @@ static void init_aggregate_counters(struct engines *engines)
>>  static int pmu_init(struct engines *engines)
>>  {
>>  	unsigned int i;
>> -	int fd;
>> +	int fd, ret;
>>  	uint64_t type = igt_perf_type_id(engines->device);
>>
>>  	engines->fd = -1;
>>  	engines->num_counters = 0;
>> -	engines->num_gts = get_num_gts(type);
>> +	ret = get_num_gts(type, &engines->num_gts);
>> +	if (ret)
>> +		return ret;
>>
>>  	engines->irq.config = I915_PMU_INTERRUPTS;
>>  	fd = _open_pmu(type, engines->num_counters, &engines->irq, engines->fd);
>> --
>> 2.34.1
>>