Re: [Intel-gfx] [PATCH RESEND] drm/tests: Suballocator test
Hi Michal, Thanks for the review, see comments inline, On 3/26/23 11:42, Michał Winiarski wrote: On Thu, Mar 02, 2023 at 09:34:22AM +0100, Thomas Hellström wrote: Add a suballocator test to get some test coverage for the new drm suballocator, and perform some basic timing (elapsed time). Signed-off-by: Thomas Hellström --- drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/tests/Makefile| 3 +- drivers/gpu/drm/tests/drm_suballoc_test.c | 356 ++ 3 files changed, 359 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/tests/drm_suballoc_test.c diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 8fbe57407c60..dced53723721 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -78,6 +78,7 @@ config DRM_KUNIT_TEST select DRM_LIB_RANDOM select DRM_KMS_HELPER select DRM_BUDDY + select DRM_SUBALLOC_HELPER select DRM_EXPORT_FOR_TESTS if m select DRM_KUNIT_TEST_HELPERS default KUNIT_ALL_TESTS diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile index bca726a8f483..c664944a48ab 100644 --- a/drivers/gpu/drm/tests/Makefile +++ b/drivers/gpu/drm/tests/Makefile @@ -17,6 +17,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \ drm_modes_test.o \ drm_plane_helper_test.o \ drm_probe_helper_test.o \ - drm_rect_test.o + drm_rect_test.o \ + drm_suballoc_test.o CFLAGS_drm_mm_test.o := $(DISABLE_STRUCTLEAK_PLUGIN) diff --git a/drivers/gpu/drm/tests/drm_suballoc_test.c b/drivers/gpu/drm/tests/drm_suballoc_test.c new file mode 100644 index ..e7303a5505a0 --- /dev/null +++ b/drivers/gpu/drm/tests/drm_suballoc_test.c @@ -0,0 +1,356 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT +/* + * Test case for the drm_suballoc suballocator manager + * Copyright 2023 Intel Corporation. + */ + +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#define SA_ITERATIONS 1 +#define SA_SIZE SZ_1M +#define SA_DEFAULT_ALIGN SZ_4K + +static bool intr = true; +static bool from_reclaim; +static bool pre_throttle; +static unsigned int num_rings = 4; +static unsigned int iterations = SA_ITERATIONS; + +static atomic64_t free_space; + +static atomic_t other_id; + +struct suballoc_fence; + +/** + * struct suballoc_ring - fake gpu engine. + * @list: List of fences to signal. + * @signal_time: Accumulated fence signal execution time. + * @lock: Protects the suballoc ring members. hardirq safe. + * @hrtimer: Fake execution time timer. + * @active: The currently active fence for which we have pending work or a + * timer running. + * @seqno: Fence submissin seqno. + * @idx: Index for calculation of fake execution time. + * @work: Work struct used solely to move the timer start to a different + *processor than that used for submission. + */ +struct suballoc_ring { + ktime_t signal_time; + struct list_head list; + /* Protect the ring processing. */ + spinlock_t lock; + struct hrtimer hrtimer; + struct suballoc_fence *active; + atomic64_t seqno; + u32 idx; + struct work_struct work; +}; + +/** + * struct suballoc_fence - Hrtimer-driven fence. + * @fence: The base class fence struct. + * @link: Link for the ring's fence list. + * @size: The size of the suballocator range associated with this fence. + * @id: Cpu id likely used by the submission thread for suballoc allocation. + */ +struct suballoc_fence { + struct dma_fence fence; + struct list_head link; + size_t size; + unsigned int id; +}; + +/* A varying but repeatable fake execution time */ +static ktime_t ring_next_delay(struct suballoc_ring *ring) +{ + return ns_to_ktime((u64)(++ring->idx % 8) * 200 * NSEC_PER_USEC); +} Is there any way we can avoid using time (and large number of iterations) here, while keeping the coverage? drm_suballoc have longest runtime out of all tests in DRM (taking ~60% of the whole DRM kunit execution, drm_mm being the second and taking ~35%, without those two suites DRM tests execute in milliseconds rather than tens of seconds), Building test cases in a way that operate on time basis makes it tricky to optimize the runtime. If we extract various parameters from modparams to separate test cases, it's going to get even worse. This is intended to mimic the behaviour of different rings / engines using the same suballocator but with different typical batch-buffer execution time, causing suballocator fragmentation. TBH I haven't thought much about test execution time here so I can take a look at improving that. Also if time-based becomes an issue what's important to maintain coverage is the order in which the fences are signaled, and also that we are able to drain the suballocator completely. However, I'm a bit afraid that trying to achieve that in other ways may complicate the test even more.
Re: [Intel-gfx] [PATCH RESEND] drm/tests: Suballocator test
On Thu, Mar 02, 2023 at 09:34:22AM +0100, Thomas Hellström wrote: > Add a suballocator test to get some test coverage for the new drm > suballocator, and perform some basic timing (elapsed time). > > Signed-off-by: Thomas Hellström > --- > drivers/gpu/drm/Kconfig | 1 + > drivers/gpu/drm/tests/Makefile| 3 +- > drivers/gpu/drm/tests/drm_suballoc_test.c | 356 ++ > 3 files changed, 359 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/tests/drm_suballoc_test.c > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 8fbe57407c60..dced53723721 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -78,6 +78,7 @@ config DRM_KUNIT_TEST > select DRM_LIB_RANDOM > select DRM_KMS_HELPER > select DRM_BUDDY > + select DRM_SUBALLOC_HELPER > select DRM_EXPORT_FOR_TESTS if m > select DRM_KUNIT_TEST_HELPERS > default KUNIT_ALL_TESTS > diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile > index bca726a8f483..c664944a48ab 100644 > --- a/drivers/gpu/drm/tests/Makefile > +++ b/drivers/gpu/drm/tests/Makefile > @@ -17,6 +17,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \ > drm_modes_test.o \ > drm_plane_helper_test.o \ > drm_probe_helper_test.o \ > - drm_rect_test.o > + drm_rect_test.o \ > + drm_suballoc_test.o > > CFLAGS_drm_mm_test.o := $(DISABLE_STRUCTLEAK_PLUGIN) > diff --git a/drivers/gpu/drm/tests/drm_suballoc_test.c > b/drivers/gpu/drm/tests/drm_suballoc_test.c > new file mode 100644 > index ..e7303a5505a0 > --- /dev/null > +++ b/drivers/gpu/drm/tests/drm_suballoc_test.c > @@ -0,0 +1,356 @@ > +// SPDX-License-Identifier: GPL-2.0 OR MIT > +/* > + * Test case for the drm_suballoc suballocator manager > + * Copyright 2023 Intel Corporation. > + */ > + > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define SA_ITERATIONS 1 > +#define SA_SIZE SZ_1M > +#define SA_DEFAULT_ALIGN SZ_4K > + > +static bool intr = true; > +static bool from_reclaim; > +static bool pre_throttle; > +static unsigned int num_rings = 4; > +static unsigned int iterations = SA_ITERATIONS; > + > +static atomic64_t free_space; > + > +static atomic_t other_id; > + > +struct suballoc_fence; > + > +/** > + * struct suballoc_ring - fake gpu engine. > + * @list: List of fences to signal. > + * @signal_time: Accumulated fence signal execution time. > + * @lock: Protects the suballoc ring members. hardirq safe. > + * @hrtimer: Fake execution time timer. > + * @active: The currently active fence for which we have pending work or a > + * timer running. > + * @seqno: Fence submissin seqno. > + * @idx: Index for calculation of fake execution time. > + * @work: Work struct used solely to move the timer start to a different > + *processor than that used for submission. > + */ > +struct suballoc_ring { > + ktime_t signal_time; > + struct list_head list; > + /* Protect the ring processing. */ > + spinlock_t lock; > + struct hrtimer hrtimer; > + struct suballoc_fence *active; > + atomic64_t seqno; > + u32 idx; > + struct work_struct work; > +}; > + > +/** > + * struct suballoc_fence - Hrtimer-driven fence. > + * @fence: The base class fence struct. > + * @link: Link for the ring's fence list. > + * @size: The size of the suballocator range associated with this fence. > + * @id: Cpu id likely used by the submission thread for suballoc allocation. > + */ > +struct suballoc_fence { > + struct dma_fence fence; > + struct list_head link; > + size_t size; > + unsigned int id; > +}; > + > +/* A varying but repeatable fake execution time */ > +static ktime_t ring_next_delay(struct suballoc_ring *ring) > +{ > + return ns_to_ktime((u64)(++ring->idx % 8) * 200 * NSEC_PER_USEC); > +} Is there any way we can avoid using time (and large number of iterations) here, while keeping the coverage? drm_suballoc have longest runtime out of all tests in DRM (taking ~60% of the whole DRM kunit execution, drm_mm being the second and taking ~35%, without those two suites DRM tests execute in milliseconds rather than tens of seconds), Building test cases in a way that operate on time basis makes it tricky to optimize the runtime. If we extract various parameters from modparams to separate test cases, it's going to get even worse. > + > +/* > + * Launch from a work item to decrease the likelyhood of the timer expiry > + * callback getting called from the allocating cpu. > + * We want to trigger cache-line bouncing between allocating and signalling > + * cpus. > + */ > +static void ring_launch_timer_work(struct work_struct *work) > +{ > + struct suballoc_ring *ring = > + container_of(work, typeof(*ring), work); > + > + spin_lock_irq(&ring->lock); > + if (ring->active) > + hrtimer_start_range_ns(&ri
Re: [Intel-gfx] [PATCH RESEND] drm/tests: Suballocator test
Hi Thomas, Thank you for the patch! Yet something to improve: [auto build test ERROR on drm-tip/drm-tip] url: https://github.com/intel-lab-lkp/linux/commits/Thomas-Hellstr-m/drm-tests-Suballocator-test/20230302-163704 base: git://anongit.freedesktop.org/drm/drm-tip drm-tip patch link: https://lore.kernel.org/r/20230302083422.76608-1-thomas.hellstrom%40linux.intel.com patch subject: [Intel-gfx] [PATCH RESEND] drm/tests: Suballocator test config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20230303/202303030052.ybiikxrn-...@intel.com/config) compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/e970911bccf3145b76cd755e2d78c0c0f7f22ca1 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Thomas-Hellstr-m/drm-tests-Suballocator-test/20230302-163704 git checkout e970911bccf3145b76cd755e2d78c0c0f7f22ca1 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot | Link: https://lore.kernel.org/oe-kbuild-all/202303030052.ybiikxrn-...@intel.com/ All errors (new ones prefixed by >>, old ones prefixed by <<): >> ERROR: modpost: "__aeabi_uldivmod" >> [drivers/gpu/drm/tests/drm_suballoc_test.ko] undefined! -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests
Re: [Intel-gfx] [PATCH RESEND] drm/tests: Suballocator test
Hi Thomas, Thank you for the patch! Yet something to improve: [auto build test ERROR on drm-tip/drm-tip] url: https://github.com/intel-lab-lkp/linux/commits/Thomas-Hellstr-m/drm-tests-Suballocator-test/20230302-163704 base: git://anongit.freedesktop.org/drm/drm-tip drm-tip patch link: https://lore.kernel.org/r/20230302083422.76608-1-thomas.hellstrom%40linux.intel.com patch subject: [Intel-gfx] [PATCH RESEND] drm/tests: Suballocator test config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20230303/202303030056.cnezgrqr-...@intel.com/config) compiler: mips-linux-gcc (GCC) 12.1.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/e970911bccf3145b76cd755e2d78c0c0f7f22ca1 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Thomas-Hellstr-m/drm-tests-Suballocator-test/20230302-163704 git checkout e970911bccf3145b76cd755e2d78c0c0f7f22ca1 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot | Link: https://lore.kernel.org/oe-kbuild-all/202303030056.cnezgrqr-...@intel.com/ All errors (new ones prefixed by >>): arch/mips/kernel/head.o: in function `kernel_entry': (.ref.text+0xac): relocation truncated to fit: R_MIPS_26 against `start_kernel' init/main.o: in function `set_reset_devices': main.c:(.init.text+0x20): relocation truncated to fit: R_MIPS_26 against `_mcount' main.c:(.init.text+0x30): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc' init/main.o: in function `debug_kernel': main.c:(.init.text+0xa4): relocation truncated to fit: R_MIPS_26 against `_mcount' main.c:(.init.text+0xb4): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc' init/main.o: in function `quiet_kernel': main.c:(.init.text+0x128): relocation truncated to fit: R_MIPS_26 against `_mcount' main.c:(.init.text+0x138): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc' init/main.o: in function `warn_bootconfig': main.c:(.init.text+0x1ac): relocation truncated to fit: R_MIPS_26 against `_mcount' main.c:(.init.text+0x1bc): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc' init/main.o: in function `init_setup': main.c:(.init.text+0x234): relocation truncated to fit: R_MIPS_26 against `_mcount' main.c:(.init.text+0x254): additional relocation overflows omitted from the output mips-linux-ld: drivers/gpu/drm/tests/drm_suballoc_test.o: in function `drm_test_suballoc': >> drm_suballoc_test.c:(.text.drm_test_suballoc+0xcbc): undefined reference to >> `__udivdi3' >> mips-linux-ld: drm_suballoc_test.c:(.text.drm_test_suballoc+0xd20): >> undefined reference to `__udivdi3' mips-linux-ld: drm_suballoc_test.c:(.text.drm_test_suballoc+0xd84): undefined reference to `__udivdi3' mips-linux-ld: drm_suballoc_test.c:(.text.drm_test_suballoc+0xde8): undefined reference to `__udivdi3' mips-linux-ld: drm_suballoc_test.c:(.text.drm_test_suballoc+0xe40): undefined reference to `__udivdi3' mips-linux-ld: drivers/gpu/drm/tests/drm_suballoc_test.o:drm_suballoc_test.c:(.text.drm_test_suballoc+0xfb4): more undefined references to `__udivdi3' follow -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests
Re: [Intel-gfx] [PATCH RESEND] drm/tests: Suballocator test
Am 02.03.23 um 09:34 schrieb Thomas Hellström: Add a suballocator test to get some test coverage for the new drm suballocator, and perform some basic timing (elapsed time). Signed-off-by: Thomas Hellström Nice, I haven't had time to go over it in all detail but it looks pretty sophisticated. Feel free to add an Acked-by: Christian König . Regards, Christian. --- drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/tests/Makefile| 3 +- drivers/gpu/drm/tests/drm_suballoc_test.c | 356 ++ 3 files changed, 359 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/tests/drm_suballoc_test.c diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 8fbe57407c60..dced53723721 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -78,6 +78,7 @@ config DRM_KUNIT_TEST select DRM_LIB_RANDOM select DRM_KMS_HELPER select DRM_BUDDY + select DRM_SUBALLOC_HELPER select DRM_EXPORT_FOR_TESTS if m select DRM_KUNIT_TEST_HELPERS default KUNIT_ALL_TESTS diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile index bca726a8f483..c664944a48ab 100644 --- a/drivers/gpu/drm/tests/Makefile +++ b/drivers/gpu/drm/tests/Makefile @@ -17,6 +17,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \ drm_modes_test.o \ drm_plane_helper_test.o \ drm_probe_helper_test.o \ - drm_rect_test.o + drm_rect_test.o \ + drm_suballoc_test.o CFLAGS_drm_mm_test.o := $(DISABLE_STRUCTLEAK_PLUGIN) diff --git a/drivers/gpu/drm/tests/drm_suballoc_test.c b/drivers/gpu/drm/tests/drm_suballoc_test.c new file mode 100644 index ..e7303a5505a0 --- /dev/null +++ b/drivers/gpu/drm/tests/drm_suballoc_test.c @@ -0,0 +1,356 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT +/* + * Test case for the drm_suballoc suballocator manager + * Copyright 2023 Intel Corporation. + */ + +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#define SA_ITERATIONS 1 +#define SA_SIZE SZ_1M +#define SA_DEFAULT_ALIGN SZ_4K + +static bool intr = true; +static bool from_reclaim; +static bool pre_throttle; +static unsigned int num_rings = 4; +static unsigned int iterations = SA_ITERATIONS; + +static atomic64_t free_space; + +static atomic_t other_id; + +struct suballoc_fence; + +/** + * struct suballoc_ring - fake gpu engine. + * @list: List of fences to signal. + * @signal_time: Accumulated fence signal execution time. + * @lock: Protects the suballoc ring members. hardirq safe. + * @hrtimer: Fake execution time timer. + * @active: The currently active fence for which we have pending work or a + * timer running. + * @seqno: Fence submissin seqno. + * @idx: Index for calculation of fake execution time. + * @work: Work struct used solely to move the timer start to a different + *processor than that used for submission. + */ +struct suballoc_ring { + ktime_t signal_time; + struct list_head list; + /* Protect the ring processing. */ + spinlock_t lock; + struct hrtimer hrtimer; + struct suballoc_fence *active; + atomic64_t seqno; + u32 idx; + struct work_struct work; +}; + +/** + * struct suballoc_fence - Hrtimer-driven fence. + * @fence: The base class fence struct. + * @link: Link for the ring's fence list. + * @size: The size of the suballocator range associated with this fence. + * @id: Cpu id likely used by the submission thread for suballoc allocation. + */ +struct suballoc_fence { + struct dma_fence fence; + struct list_head link; + size_t size; + unsigned int id; +}; + +/* A varying but repeatable fake execution time */ +static ktime_t ring_next_delay(struct suballoc_ring *ring) +{ + return ns_to_ktime((u64)(++ring->idx % 8) * 200 * NSEC_PER_USEC); +} + +/* + * Launch from a work item to decrease the likelyhood of the timer expiry + * callback getting called from the allocating cpu. + * We want to trigger cache-line bouncing between allocating and signalling + * cpus. + */ +static void ring_launch_timer_work(struct work_struct *work) +{ + struct suballoc_ring *ring = + container_of(work, typeof(*ring), work); + + spin_lock_irq(&ring->lock); + if (ring->active) + hrtimer_start_range_ns(&ring->hrtimer, ring_next_delay(ring), + 100ULL * NSEC_PER_USEC, + HRTIMER_MODE_REL_PINNED); + + spin_unlock_irq(&ring->lock); +} + +/* + * Signal an active fence and pull the next off the list if any and make it + * active. + */ +static enum hrtimer_restart ring_hrtimer_expired(struct hrtimer *hrtimer) +{ + struct suballoc_ring *ring = + container_of(hrtimer, typeof(*ring), hrtimer); + struct suballoc_fence *sfence; + ktime_t now, then; + unsigned long irqflags; + + sp