[FFmpeg-devel] [PATCH] checkasm: Test whether direct cycle counter access works

[FFmpeg-devel] [PATCH] checkasm: Test whether direct cycle counter access works

[Martin Storsjö]

This should print a nicer error message than crashing due to
an illegal instruction, if direct cycle counter access isn't
allowed.

This matches the dav1d checkasm commit
95a192549a448b70d9542e840c4e34b60d09b093.
---
 tests/checkasm/checkasm.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c
index 994d64e96b..9c5abb53dc 100644
--- a/tests/checkasm/checkasm.c
+++ b/tests/checkasm/checkasm.c
@@ -754,6 +754,14 @@ static int bench_init_kperf(void)
 static int bench_init_ffmpeg(void)
 {
 #ifdef AV_READ_TIME
+    if (!checkasm_save_context()) {
+        checkasm_set_signal_handler_state(1);
+        AV_READ_TIME();
+        checkasm_set_signal_handler_state(0);
+    } else {
+        fprintf(stderr, "checkasm: unable to access cycle counter\n");
+        return -1;
+    }
     printf("benchmarking with native FFmpeg timers\n");
     return 0;
 #else
@@ -927,7 +935,9 @@ int checkasm_bench_func(void)
 /* Indicate that the current test has failed */
 void checkasm_fail_func(const char *msg, ...)
 {
-    if (state.current_func_ver->cpu && state.current_func_ver->ok) {
+    if (state.current_func_ver && state.current_func_ver->cpu &&
+        state.current_func_ver->ok)
+    {
         va_list arg;
 
         print_cpu_name();
-- 
2.39.3 (Apple Git-145)

_______________________________________________
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] checkasm: Test whether direct cycle counter access works

[Rémi Denis-Courmont]

Le torstaina 11. tammikuuta 2024, 14.53.05 EET Martin Storsjö a écrit :
> This should print a nicer error message than crashing due to
> an illegal instruction, if direct cycle counter access isn't
> allowed.
> 
> This matches the dav1d checkasm commit
> 95a192549a448b70d9542e840c4e34b60d09b093.
> ---
>  tests/checkasm/checkasm.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c
> index 994d64e96b..9c5abb53dc 100644
> --- a/tests/checkasm/checkasm.c
> +++ b/tests/checkasm/checkasm.c
> @@ -754,6 +754,14 @@ static int bench_init_kperf(void)
>  static int bench_init_ffmpeg(void)
>  {
>  #ifdef AV_READ_TIME
> +    if (!checkasm_save_context()) {
> +        checkasm_set_signal_handler_state(1);
> +        AV_READ_TIME();
> +        checkasm_set_signal_handler_state(0);
> +    } else {
> +        fprintf(stderr, "checkasm: unable to access cycle counter\n");

AV_READ_TIME() reads time, not cycles. If we want cycle count, then we should 
add a separate macro, as the two are different performance counters at least on 
RISC-V. As things stand, this code won't do anything on RISC-V, sinec 
AV_READ_TIME() actually reads, well, time, not cycles.

> +        return -1;
> +    }
>      printf("benchmarking with native FFmpeg timers\n");
>      return 0;
>  #else
> @@ -927,7 +935,9 @@ int checkasm_bench_func(void)
>  /* Indicate that the current test has failed */
>  void checkasm_fail_func(const char *msg, ...)
>  {
> -    if (state.current_func_ver->cpu && state.current_func_ver->ok) {
> +    if (state.current_func_ver && state.current_func_ver->cpu &&
> +        state.current_func_ver->ok)
> +    {
>          va_list arg;
> 
>          print_cpu_name();


-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/



_______________________________________________
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] checkasm: Test whether direct cycle counter access works

[Martin Storsjö]

On Thu, 11 Jan 2024, Rémi Denis-Courmont wrote:

> Le torstaina 11. tammikuuta 2024, 14.53.05 EET Martin Storsjö a écrit :
>> This should print a nicer error message than crashing due to
>> an illegal instruction, if direct cycle counter access isn't
>> allowed.
>> 
>> This matches the dav1d checkasm commit
>> 95a192549a448b70d9542e840c4e34b60d09b093.
>> ---
>>  tests/checkasm/checkasm.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c
>> index 994d64e96b..9c5abb53dc 100644
>> --- a/tests/checkasm/checkasm.c
>> +++ b/tests/checkasm/checkasm.c
>> @@ -754,6 +754,14 @@ static int bench_init_kperf(void)
>>  static int bench_init_ffmpeg(void)
>>  {
>>  #ifdef AV_READ_TIME
>> +    if (!checkasm_save_context()) {
>> +        checkasm_set_signal_handler_state(1);
>> +        AV_READ_TIME();
>> +        checkasm_set_signal_handler_state(0);
>> +    } else {
>> +        fprintf(stderr, "checkasm: unable to access cycle counter\n");
>
> AV_READ_TIME() reads time, not cycles.

Right, I can adjust the wording. Exactly what kind of measurement 
AV_READ_TIME returns varies between architectures and environments indeed.

What about:

     checkasm: unable to execute platform specific timer

> If we want cycle count, then we should add a separate macro, as the two 
> are different performance counters at least on RISC-V.

That's not what I try to do here, I just want to test whether the timer, 
whatever we have in AV_READ_TIME, is usable.

> As things stand, this code won't do anything on RISC-V, sinec 
> AV_READ_TIME() actually reads, well, time, not cycles.

Should I interpret this, as, the current AV_READ_TIME implementation on 
RISC-V always succeeds, contrary to the previous implementation (with 
rdcycle) which is unavailable on some systems, referencing 
05115a77e012331b6ff5e24bab40e75848447c62?

In that case - sure, this would be mostly a no-op for RISC-V, just like it 
is for x86, but for ARM/AArch64 it would provide a nicer error message if 
access to the relevant registers hasn't been configured.

// 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] checkasm: Test whether direct cycle counter access works

[Rémi Denis-Courmont]

Le torstaina 11. tammikuuta 2024, 16.15.29 EET Martin Storsjö a écrit :
> > AV_READ_TIME() reads time, not cycles.
> 
> Right, I can adjust the wording. Exactly what kind of measurement
> AV_READ_TIME returns varies between architectures and environments indeed.

In practice, yes, but I would argue that it's a bug if it does not measure 
time. At the very least because, the name is extremely misleading.

> What about:
> 
>      checkasm: unable to execute platform specific timer
> 
> > If we want cycle count, then we should add a separate macro, as the two
> > are different performance counters at least on RISC-V.
> 
> That's not what I try to do here, I just want to test whether the timer,
> whatever we have in AV_READ_TIME, is usable.

Sure, I can live with that, but I thought that checkasm actually prefered to 
measure cycles than time periods.

> > As things stand, this code won't do anything on RISC-V, sinec
> > AV_READ_TIME() actually reads, well, time, not cycles.
> 
> Should I interpret this, as, the current AV_READ_TIME implementation on
> RISC-V always succeeds, contrary to the previous implementation (with
> rdcycle) which is unavailable on some systems, referencing
> 05115a77e012331b6ff5e24bab40e75848447c62?

Yes.

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/



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