diff mbox series

[FFmpeg-devel] lavu/riscv: Revert d808070, removing AV_READ_TIME

Message ID 20240728171051.29078-1-unlord@xiph.org
State New
Headers show
Series [FFmpeg-devel] lavu/riscv: Revert d808070, removing AV_READ_TIME | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Nathan E. Egge July 28, 2024, 5:10 p.m. UTC 
The implementation of ff_read_time() for RISC-V uses rdtime which has
 precision on existing hardware too low (!) for benchmarking purposes.
Deleting this implementation falls back on clock_gettime() which was
 added as the default ff_read_time() implementation in 33e4cc9.
Below are metrics gathered on SpacemiT K1, before and after this commit:

Before:

$ tests/checkasm/checkasm --bench
benchmarking with native FFmpeg timers
nop: 0.0
checkasm: using random seed 3473665261
checkasm: bench runs 1024 (1 << 10)
RVI:
 - pixblockdsp.get_pixels                [OK]
 - vc1dsp.mspel_pixels                   [OK]
RVF:
 - audiodsp.audiodsp                     [OK]
checkasm: all 4 tests passed
audiodsp.vector_clipf_c: 1388.7
audiodsp.vector_clipf_rvf: 261.5
get_pixels_c: 2.0
get_pixels_rvi: 1.5
vc1dsp.put_vc1_mspel_pixels_tab[0][0]_c: 8.0
vc1dsp.put_vc1_mspel_pixels_tab[0][0]_rvi: 1.0
vc1dsp.put_vc1_mspel_pixels_tab[1][0]_c: 2.0
vc1dsp.put_vc1_mspel_pixels_tab[1][0]_rvi: 0.5

After:

$ tests/checkasm/checkasm --bench
benchmarking with native FFmpeg timers
nop: 56.4
checkasm: using random seed 1021411603
checkasm: bench runs 1024 (1 << 10)
RVI:
 - pixblockdsp.get_pixels                [OK]
 - vc1dsp.mspel_pixels                   [OK]
RVF:
 - audiodsp.audiodsp                     [OK]
checkasm: all 4 tests passed
audiodsp.vector_clipf_c: 23236.4
audiodsp.vector_clipf_rvf: 11038.4
get_pixels_c: 79.6
get_pixels_rvi: 48.4
vc1dsp.put_vc1_mspel_pixels_tab[0][0]_c: 329.6
vc1dsp.put_vc1_mspel_pixels_tab[0][0]_rvi: 38.1
vc1dsp.put_vc1_mspel_pixels_tab[1][0]_c: 89.9
vc1dsp.put_vc1_mspel_pixels_tab[1][0]_rvi: 17.1
---
 libavutil/riscv/timer.h | 54 -----------------------------------------
 libavutil/timer.h       |  2 --
 2 files changed, 56 deletions(-)
 delete mode 100644 libavutil/riscv/timer.h

Comments

Rémi Denis-Courmont July 29, 2024, 6:44 p.m. UTC | #1 
Le sunnuntaina 28. heinäkuuta 2024, 20.10.51 EEST Nathan E. Egge a écrit :
> The implementation of ff_read_time() for RISC-V uses rdtime which has
>  precision on existing hardware too low (!) for benchmarking purposes.
> Deleting this implementation falls back on clock_gettime() which was
>  added as the default ff_read_time() implementation in 33e4cc9.

So the results seem fairly stable, unlike the Linux kperf cycle count, so I am 
fine with this.

Except that if we are switching to nanoseconds, the new FF_TIMER_UNITS value 
should be "ns", not "UNITS".
diff mbox series

Patch

diff --git a/libavutil/riscv/timer.h b/libavutil/riscv/timer.h
deleted file mode 100644
index 174b469cbe..0000000000
--- a/libavutil/riscv/timer.h
+++ /dev/null
@@ -1,54 +0,0 @@ 
-/*
- * This file is part of FFmpeg.
- *
- * FFmpeg is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2.1 of the License, or (at your option) any later version.
- *
- * FFmpeg is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with FFmpeg; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
- */
-
-#ifndef AVUTIL_RISCV_TIMER_H
-#define AVUTIL_RISCV_TIMER_H
-
-#include "config.h"
-
-#if HAVE_INLINE_ASM
-#include <stdint.h>
-
-static inline uint64_t ff_read_time(void)
-{
-#if (__riscv_xlen >= 64)
-    uintptr_t cycles;
-
-    __asm__ volatile ("rdtime  %0" : "=r" (cycles));
-
-#else
-    uint64_t cycles;
-    uint32_t hi, lo, check;
-
-    __asm__ volatile (
-        "1: rdtimeh %0\n"
-        "   rdtime  %1\n"
-        "   rdtimeh %2\n"
-        "   bne %0, %2, 1b\n" : "=r" (hi), "=r" (lo), "=r" (check));
-
-    cycles = (((uint64_t)hi) << 32) | lo;
-
-#endif
-    return cycles;
-}
-
-#define AV_READ_TIME ff_read_time
-#define FF_TIMER_UNITS "ticks"
-
-#endif
-#endif /* AVUTIL_RISCV_TIMER_H */
diff --git a/libavutil/timer.h b/libavutil/timer.h
index 9fe6aa1385..663daf81c6 100644
--- a/libavutil/timer.h
+++ b/libavutil/timer.h
@@ -61,8 +61,6 @@ 
 #   include "arm/timer.h"
 #elif ARCH_PPC
 #   include "ppc/timer.h"
-#elif ARCH_RISCV
-#   include "riscv/timer.h"
 #elif ARCH_X86
 #   include "x86/timer.h"
 #elif ARCH_LOONGARCH