Re: [FFmpeg-devel] [PATCH 2/2] avutil/macos_kperf: Fix assert which makes kperf failed to run

From: Zhao Zhili <quinkblack@foxmail.com>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Cc: "J. Dekker" <jdek@itanimul.li>
Subject: Re: [FFmpeg-devel] [PATCH 2/2] avutil/macos_kperf: Fix assert which makes kperf failed to run
Date: Mon, 17 Jun 2024 20:20:53 +0800
Message-ID: <tencent_4B4B80E602509326C167D12A8D931C5C5E06@qq.com> (raw)
In-Reply-To: <d54da2f7-4028-d7f-abf1-eb50923e32e8@martin.st>

> On Jun 17, 2024, at 19:10, Martin Storsjö <martin@martin.st> wrote:
> 
> On Wed, 12 Jun 2024, Zhao Zhili wrote:
> 
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> 
>> On m1, kpc_get_counter_count(KPC_MASK) return 8. The exact value
>> doesn't matter in our case.
> 
> This is somewhat unexpected, I had expected that this API was originally tested on an m1. I guess it might depend on what OS version you're using as well?

On arm64 [1]

#define KPC_ARM64_FIXED_COUNT        (2)
#define KPC_ARM64_CONFIGURABLE_COUNT (CORE_NCTRS - KPC_ARM64_FIXED_COUNT)

#define KPC_MAX_COUNTERS (KPC_ARM64_FIXED_COUNT + KPC_ARM64_CONFIGURABLE_COUNT + 1)

On x86_64:
#define KPC_MAX_COUNTERS 32

[1] https://github.com/apple/darwin-xnu/blob/2ff845c2e033bd0ff64b5b6aa6063a1f8f65aa32/osfmk/arm64/machine_kpc.h#L36

> 
>> ---
>> libavutil/macos_kperf.c | 15 +++++++++------
>> 1 file changed, 9 insertions(+), 6 deletions(-)
>> 
>> diff --git a/libavutil/macos_kperf.c b/libavutil/macos_kperf.c
>> index a0bc845fd3..906b276a34 100644
>> --- a/libavutil/macos_kperf.c
>> +++ b/libavutil/macos_kperf.c
>> @@ -67,14 +67,15 @@ KPERF_LIST
>> #define KPC_CLASS_POWER_MASK        (1 << 2)
>> #define KPC_CLASS_RAWPMU_MASK       (1 << 3)
>> 
>> -#define COUNTERS_COUNT 10
>> +#define KPC_MAX_COUNTERS 32
>> #define CONFIG_COUNT 8
>> #define KPC_MASK (KPC_CLASS_CONFIGURABLE_MASK | KPC_CLASS_FIXED_MASK)
>> 
>> static void kperf_init(void)
>> {
>> -    uint64_t config[COUNTERS_COUNT] = {0};
>> +    uint64_t config[CONFIG_COUNT] = {0};
> 
> Hmm, this changes the array from 10 to 8 elements. While the change looks reasonable based on the variable names, I just wanted to doublecheck that we have some clues that this is right?

The change is base on the check

av_assert0(kpc_get_config_count(KPC_MASK) == CONFIG_COUNT

> 
>>    void *kperf = NULL;
>> +    uint32_t n;
>> 
>>    av_assert0(kperf = dlopen("/System/Library/PrivateFrameworks/kperf.framework/Versions/A/kperf", RTLD_LAZY));
>> 
>> @@ -82,8 +83,10 @@ static void kperf_init(void)
>>    KPERF_LIST
>> #undef F
>> 
>> -    av_assert0(kpc_get_counter_count(KPC_MASK) == COUNTERS_COUNT);
>> -    av_assert0(kpc_get_config_count(KPC_MASK) == CONFIG_COUNT);
>> +    n = kpc_get_counter_count(KPC_MASK);
>> +    av_assert0(n <= KPC_MAX_COUNTERS);
>> +    n = kpc_get_config_count(KPC_MASK);
>> +    av_assert0(n <= CONFIG_COUNT);
> 
> I guess this is the actual functional change here, I think this seems right.
> 
> I CC's Josh on this change too, in case he has something to add here, but it looks mostly reasonable to me.

> 
> // Martin
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".