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

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

[Zhao Zhili]

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.
---
 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};
     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);
 
     config[0] = CPMU_CORE_CYCLE | CFGWORD_EL0A64EN_MASK;
     // config[3] = CPMU_INST_BRANCH | CFGWORD_EL0A64EN_MASK;
@@ -99,11 +102,11 @@ static void kperf_init(void)
 uint64_t ff_kperf_cycles(void)
 {
     static AVOnce init_static_once = AV_ONCE_INIT;
-    uint64_t counters[COUNTERS_COUNT];
+    uint64_t counters[KPC_MAX_COUNTERS];
 
     ff_thread_once(&init_static_once, kperf_init);
 
-    if (kpc_get_thread_counters(0, COUNTERS_COUNT, counters)) {
+    if (kpc_get_thread_counters(0, KPC_MAX_COUNTERS, counters)) {
         return -1;
     }
 
-- 
2.42.0

_______________________________________________
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".

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

[Martin Storsjö]

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?

> ---
> 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?

>     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".

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

[J. Dekker]

On Mon, Jun 17, 2024, at 13:10, Martin Storsjö 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?
>
>> ---
>> 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?
>
>>     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.

It’s a private kernel API there are no guarantees about it working at any point. I just know that it used to work. If it needs changes to work currently and this fixes it for you then that’s fine, you should probably also port this change to dav1d. I’m personally unable to test this as I am just running Linux on my M1 machine now.

-- 
jd
_______________________________________________
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".

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

[Zhao Zhili]

> 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".