| Message ID | 20240111125306.77553-1-martin@martin.st |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] checkasm: Test whether direct cycle counter access works | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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();
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
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.
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();