| Message ID | 20250306170555.7244-2-tvrtko.ursulin@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DRM scheduler kunit tests | expand |
Hi, You forgot to put folks in CC as recipents for the cover letter :( On Thu, 2025-03-06 at 17:05 +0000, Tvrtko Ursulin wrote: > Move some options out into a new debug specific kconfig file in order > to > make things a bit cleaner. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > Cc: Christian König <christian.koenig@amd.com> > Cc: Danilo Krummrich <dakr@kernel.org> > Cc: Matthew Brost <matthew.brost@intel.com> > Cc: Philipp Stanner <phasta@kernel.org> We all have our individual work flows, so don't take this as lecturing or anything – I just suspect that I was forgotten in the cover letter because you Cc people by hand in the individual patches. What I do is that I run get_maintainer and then put the individuals listed there into the --to= field. That sends the entire series to all of them. Only sometimes, when there's a huge list of recipents or when the patches of a series are very independent, I deviate from that rule. JFYI ---- Anyways, we have a bigger problem about the entire series. I now tested again with the same setup as yesterday and the faults are indeed gone, so that's good. But to be sure I then did run kmemleak and got a list of leaks that is more than 2000 lines long. Excerpt: unreferenced object 0xffff88810ade5600 (size 168): comm "kunit_try_catch", pid 300, jiffies 4294670705 hex dump (first 32 bytes): 98 56 de 0a 81 88 ff ff e0 f2 48 b3 ff ff ff ff .V........H..... d9 de 75 22 01 00 00 00 20 7c 45 01 81 88 ff ff ..u".... |E..... backtrace (crc 4f2b379c): kmem_cache_alloc_noprof+0x299/0x320 drm_sched_fence_alloc+0x1d/0xa0 drm_sched_job_init+0xbc/0x2d0 drm_mock_sched_job_new+0x105/0x400 drm_sched_basic_entity_cleanup+0x182/0x510 kunit_try_run_case+0x1ae/0x490 kunit_generic_run_threadfn_adapter+0x7b/0xe0 kthread+0x30f/0x620 ret_from_fork+0x2f/0x70 ret_from_fork_asm+0x1a/0x30 unreferenced object 0xffff88810ade5700 (size 168): comm "kunit_try_catch", pid 300, jiffies 4294670705 hex dump (first 32 bytes): 98 57 de 0a 81 88 ff ff e0 f2 48 b3 ff ff ff ff .W........H..... ff 2d 85 22 01 00 00 00 20 7c 45 01 81 88 ff ff .-.".... |E..... backtrace (crc b5f15c1c): kmem_cache_alloc_noprof+0x299/0x320 drm_sched_fence_alloc+0x1d/0xa0 drm_sched_job_init+0xbc/0x2d0 drm_mock_sched_job_new+0x105/0x400 drm_sched_basic_entity_cleanup+0x182/0x510 kunit_try_run_case+0x1ae/0x490 kunit_generic_run_threadfn_adapter+0x7b/0xe0 kthread+0x30f/0x620 ret_from_fork+0x2f/0x70 ret_from_fork_asm+0x1a/0x30 unreferenced object 0xffff88810ade5800 (size 168): comm "kunit_try_catch", pid 300, jiffies 4294670705 hex dump (first 32 bytes): 98 58 de 0a 81 88 ff ff e0 f2 48 b3 ff ff ff ff .X........H..... fb a0 82 1f 01 00 00 00 20 7c 45 01 81 88 ff ff ........ |E..... backtrace (crc 9fe7a1c9): kmem_cache_alloc_noprof+0x299/0x320 drm_sched_fence_alloc+0x1d/0xa0 drm_sched_job_init+0xbc/0x2d0 drm_mock_sched_job_new+0x105/0x400 drm_sched_basic_entity_cleanup+0x182/0x510 kunit_try_run_case+0x1ae/0x490 kunit_generic_run_threadfn_adapter+0x7b/0xe0 kthread+0x30f/0x620 ret_from_fork+0x2f/0x70 ret_from_fork_asm+0x1a/0x30 unreferenced object 0xffff88810ade5900 (size 168): comm "kunit_try_catch", pid 300, jiffies 4294670705 hex dump (first 32 bytes): 98 59 de 0a 81 88 ff ff e0 f2 48 b3 ff ff ff ff .Y........H..... 03 cb 82 1f 01 00 00 00 20 7c 45 01 81 88 ff ff ........ |E..... backtrace (crc 524ba9e4): kmem_cache_alloc_noprof+0x299/0x320 drm_sched_fence_alloc+0x1d/0xa0 drm_sched_job_init+0xbc/0x2d0 drm_mock_sched_job_new+0x105/0x400 drm_sched_basic_entity_cleanup+0x182/0x510 kunit_try_run_case+0x1ae/0x490 kunit_generic_run_threadfn_adapter+0x7b/0xe0 kthread+0x30f/0x620 ret_from_fork+0x2f/0x70 ret_from_fork_asm+0x1a/0x30 I'm no expert with kunit tests, but I'm being told that there shouldn't be leaks from test leftovers. Please do a test with kmemleak, too, i.e. build and boot a kernel in QEMU and then do some scans. Thanks, Philipp > --- > drivers/gpu/drm/Kconfig | 109 ++------------------------------ > -- > drivers/gpu/drm/Kconfig.debug | 103 ++++++++++++++++++++++++++++++++ > 2 files changed, 108 insertions(+), 104 deletions(-) > create mode 100644 drivers/gpu/drm/Kconfig.debug > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index d9986fd52194..46ba24592553 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -26,6 +26,11 @@ menuconfig DRM > details. You should also select and configure AGP > (/dev/agpgart) support if it is available for your > platform. > > +menu "DRM debugging options" > +depends on DRM > +source "drivers/gpu/drm/Kconfig.debug" > +endmenu > + > if DRM > > config DRM_MIPI_DBI > @@ -37,65 +42,6 @@ config DRM_MIPI_DSI > bool > depends on DRM > > -config DRM_DEBUG_MM > - bool "Insert extra checks and debug info into the DRM range > managers" > - default n > - depends on DRM > - depends on STACKTRACE_SUPPORT > - select STACKDEPOT > - help > - Enable allocation tracking of memory manager and leak > detection on > - shutdown. > - > - Recommended for driver developers only. > - > - If in doubt, say "N". > - > -config DRM_USE_DYNAMIC_DEBUG > - bool "use dynamic debug to implement drm.debug" > - default n > - depends on BROKEN > - depends on DRM > - depends on DYNAMIC_DEBUG || DYNAMIC_DEBUG_CORE > - depends on JUMP_LABEL > - help > - Use dynamic-debug to avoid drm_debug_enabled() runtime > overheads. > - Due to callsite counts in DRM drivers (~4k in amdgpu) and > 56 > - bytes per callsite, the .data costs can be substantial, > and > - are therefore configurable. > - > -config DRM_KUNIT_TEST_HELPERS > - tristate > - depends on DRM && KUNIT > - select DRM_KMS_HELPER > - help > - KUnit Helpers for KMS drivers. > - > -config DRM_KUNIT_TEST > - tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS > - depends on DRM && KUNIT && MMU > - select DRM_BUDDY > - select DRM_DISPLAY_DP_HELPER > - select DRM_DISPLAY_HDMI_STATE_HELPER > - select DRM_DISPLAY_HELPER > - select DRM_EXEC > - select DRM_EXPORT_FOR_TESTS if m > - select DRM_GEM_SHMEM_HELPER > - select DRM_KUNIT_TEST_HELPERS > - select DRM_LIB_RANDOM > - select PRIME_NUMBERS > - default KUNIT_ALL_TESTS > - help > - This builds unit tests for DRM. This option is not useful > for > - distributions or general kernels, but only for kernel > - developers working on DRM and associated drivers. > - > - For more information on KUnit and unit tests in general, > - please refer to the KUnit documentation in > - Documentation/dev-tools/kunit/. > - > - If in doubt, say "N". > - > config DRM_KMS_HELPER > tristate > depends on DRM > @@ -247,23 +193,6 @@ config DRM_TTM > GPU memory types. Will be enabled automatically if a > device driver > uses it. > > -config DRM_TTM_KUNIT_TEST > - tristate "KUnit tests for TTM" if !KUNIT_ALL_TESTS > - default n > - depends on DRM && KUNIT && MMU && (UML || COMPILE_TEST) > - select DRM_TTM > - select DRM_BUDDY > - select DRM_EXPORT_FOR_TESTS if m > - select DRM_KUNIT_TEST_HELPERS > - default KUNIT_ALL_TESTS > - help > - Enables unit tests for TTM, a GPU memory manager subsystem > used > - to manage memory buffers. This option is mostly useful for > kernel > - developers. It depends on (UML || COMPILE_TEST) since no > other driver > - which uses TTM can be loaded while running the tests. > - > - If in doubt, say "N". > - > config DRM_EXEC > tristate > depends on DRM > @@ -463,9 +392,6 @@ config DRM_HYPERV > > If M is selected the module will be called hyperv_drm. > > -config DRM_EXPORT_FOR_TESTS > - bool > - > # Separate option as not all DRM drivers use it > config DRM_PANEL_BACKLIGHT_QUIRKS > tristate > @@ -478,31 +404,6 @@ config DRM_PRIVACY_SCREEN > bool > default n > > -config DRM_WERROR > - bool "Compile the drm subsystem with warnings as errors" > - depends on DRM && EXPERT > - depends on !WERROR > - default n > - help > - A kernel build should not cause any compiler warnings, and > this > - enables the '-Werror' flag to enforce that rule in the drm > subsystem. > - > - The drm subsystem enables more warnings than the kernel > default, so > - this config option is disabled by default. > - > - If in doubt, say N. > - > -config DRM_HEADER_TEST > - bool "Ensure DRM headers are self-contained and pass kernel- > doc" > - depends on DRM && EXPERT > - default n > - help > - Ensure the DRM subsystem headers both under > drivers/gpu/drm and > - include/drm compile, are self-contained, have header > guards, and have > - no kernel-doc warnings. > - > - If in doubt, say N. > - > endif > > # Separate option because drm_panel_orientation_quirks.c is shared > with fbdev > diff --git a/drivers/gpu/drm/Kconfig.debug > b/drivers/gpu/drm/Kconfig.debug > new file mode 100644 > index 000000000000..601d7e07d421 > --- /dev/null > +++ b/drivers/gpu/drm/Kconfig.debug > @@ -0,0 +1,103 @@ > +config DRM_USE_DYNAMIC_DEBUG > + bool "use dynamic debug to implement drm.debug" > + default n > + depends on BROKEN > + depends on DRM > + depends on DYNAMIC_DEBUG || DYNAMIC_DEBUG_CORE > + depends on JUMP_LABEL > + help > + Use dynamic-debug to avoid drm_debug_enabled() runtime > overheads. > + Due to callsite counts in DRM drivers (~4k in amdgpu) and > 56 > + bytes per callsite, the .data costs can be substantial, and > + are therefore configurable. > + > +config DRM_WERROR > + bool "Compile the drm subsystem with warnings as errors" > + depends on DRM && EXPERT > + depends on !WERROR > + default n > + help > + A kernel build should not cause any compiler warnings, and > this > + enables the '-Werror' flag to enforce that rule in the drm > subsystem. > + > + The drm subsystem enables more warnings than the kernel > default, so > + this config option is disabled by default. > + > + If in doubt, say N. > + > +config DRM_HEADER_TEST > + bool "Ensure DRM headers are self-contained and pass kernel- > doc" > + depends on DRM && EXPERT > + default n > + help > + Ensure the DRM subsystem headers both under > drivers/gpu/drm and > + include/drm compile, are self-contained, have header > guards, and have > + no kernel-doc warnings. > + > + If in doubt, say N. > + > +config DRM_DEBUG_MM > + bool "Insert extra checks and debug info into the DRM range > managers" > + default n > + depends on DRM > + depends on STACKTRACE_SUPPORT > + select STACKDEPOT > + help > + Enable allocation tracking of memory manager and leak > detection on > + shutdown. > + > + Recommended for driver developers only. > + > + If in doubt, say "N". > + > +config DRM_KUNIT_TEST_HELPERS > + tristate > + depends on DRM && KUNIT > + select DRM_KMS_HELPER > + help > + KUnit Helpers for KMS drivers. > + > +config DRM_KUNIT_TEST > + tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS > + depends on DRM && KUNIT && MMU > + select DRM_BUDDY > + select DRM_DISPLAY_DP_HELPER > + select DRM_DISPLAY_HDMI_STATE_HELPER > + select DRM_DISPLAY_HELPER > + select DRM_EXEC > + select DRM_EXPORT_FOR_TESTS if m > + select DRM_GEM_SHMEM_HELPER > + select DRM_KUNIT_TEST_HELPERS > + select DRM_LIB_RANDOM > + select PRIME_NUMBERS > + default KUNIT_ALL_TESTS > + help > + This builds unit tests for DRM. This option is not useful > for > + distributions or general kernels, but only for kernel > + developers working on DRM and associated drivers. > + > + For more information on KUnit and unit tests in general, > + please refer to the KUnit documentation in > + Documentation/dev-tools/kunit/. > + > + If in doubt, say "N". > + > +config DRM_TTM_KUNIT_TEST > + tristate "KUnit tests for TTM" if !KUNIT_ALL_TESTS > + default n > + depends on DRM && KUNIT && MMU && (UML || COMPILE_TEST) > + select DRM_TTM > + select DRM_BUDDY > + select DRM_EXPORT_FOR_TESTS if m > + select DRM_KUNIT_TEST_HELPERS > + default KUNIT_ALL_TESTS > + help > + Enables unit tests for TTM, a GPU memory manager subsystem > used > + to manage memory buffers. This option is mostly useful for > kernel > + developers. It depends on (UML || COMPILE_TEST) since no > other driver > + which uses TTM can be loaded while running the tests. > + > + If in doubt, say "N". > + > +config DRM_EXPORT_FOR_TESTS > + bool
On 07/03/2025 13:41, Philipp Stanner wrote: > Hi, > > You forgot to put folks in CC as recipents for the cover letter :( > > > On Thu, 2025-03-06 at 17:05 +0000, Tvrtko Ursulin wrote: >> Move some options out into a new debug specific kconfig file in order >> to >> make things a bit cleaner. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> >> Cc: Christian König <christian.koenig@amd.com> >> Cc: Danilo Krummrich <dakr@kernel.org> >> Cc: Matthew Brost <matthew.brost@intel.com> >> Cc: Philipp Stanner <phasta@kernel.org> > > We all have our individual work flows, so don't take this as lecturing > or anything – I just suspect that I was forgotten in the cover letter > because you Cc people by hand in the individual patches. > > What I do is that I run get_maintainer and then put the individuals > listed there into the --to= field. That sends the entire series to all > of them. > > Only sometimes, when there's a huge list of recipents or when the > patches of a series are very independent, I deviate from that rule. > > JFYI Notice it was there in v3, I just omitted to paste it this time. > Anyways, we have a bigger problem about the entire series. I now tested > again with the same setup as yesterday and the faults are indeed gone, > so that's good. > > But to be sure I then did run kmemleak and got a list of leaks that is > more than 2000 lines long. There is this comment for drm_sched_fini which ends with: """ ... * This stops submission of new jobs to the hardware through * drm_sched_backend_ops.run_job(). Consequently, drm_sched_backend_ops.free_job() * will not be called for all jobs still in drm_gpu_scheduler.pending_list. * There is no solution for this currently. Thus, it is up to the driver to make * sure that: * * a) drm_sched_fini() is only called after for all submitted jobs * drm_sched_backend_ops.free_job() has been called or that * b) the jobs for which drm_sched_backend_ops.free_job() has not been called * after drm_sched_fini() ran are freed manually. * * FIXME: Take care of the above problem and prevent this function from leaking * the jobs in drm_gpu_scheduler.pending_list under any circumstances. """ I got bitten by that. Keep forgetting how fragile the thing is.. :( Regards, Tvrtko
On Fri, 2025-03-07 at 16:59 +0000, Tvrtko Ursulin wrote: > > On 07/03/2025 13:41, Philipp Stanner wrote: > > Hi, > > > > You forgot to put folks in CC as recipents for the cover letter :( > > > > > > On Thu, 2025-03-06 at 17:05 +0000, Tvrtko Ursulin wrote: > > > Move some options out into a new debug specific kconfig file in > > > order > > > to > > > make things a bit cleaner. > > > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > > > Cc: Christian König <christian.koenig@amd.com> > > > Cc: Danilo Krummrich <dakr@kernel.org> > > > Cc: Matthew Brost <matthew.brost@intel.com> > > > Cc: Philipp Stanner <phasta@kernel.org> > > > > We all have our individual work flows, so don't take this as > > lecturing > > or anything – I just suspect that I was forgotten in the cover > > letter > > because you Cc people by hand in the individual patches. > > > > What I do is that I run get_maintainer and then put the individuals > > listed there into the --to= field. That sends the entire series to > > all > > of them. > > > > Only sometimes, when there's a huge list of recipents or when the > > patches of a series are very independent, I deviate from that rule. > > > > JFYI > > Notice it was there in v3, I just omitted to paste it this time. > > > Anyways, we have a bigger problem about the entire series. I now > > tested > > again with the same setup as yesterday and the faults are indeed > > gone, > > so that's good. > > > > But to be sure I then did run kmemleak and got a list of leaks that > > is > > more than 2000 lines long. > > There is this comment for drm_sched_fini which ends with: > > """ > ... > * This stops submission of new jobs to the hardware through > * drm_sched_backend_ops.run_job(). Consequently, > drm_sched_backend_ops.free_job() > * will not be called for all jobs still in > drm_gpu_scheduler.pending_list. > * There is no solution for this currently. Thus, it is up to the > driver to make > * sure that: > * > * a) drm_sched_fini() is only called after for all submitted jobs > * drm_sched_backend_ops.free_job() has been called or that > * b) the jobs for which drm_sched_backend_ops.free_job() has not > been > called > * after drm_sched_fini() ran are freed manually. > * > > * FIXME: Take care of the above problem and prevent this function > from > leaking > * the jobs in drm_gpu_scheduler.pending_list under any > circumstances. > """ > > I got bitten by that. Keep forgetting how fragile the thing is.. :( argh damn, those are *all* from the pending_list?! OK. Well. Now we've got a philosophical problem: We still have to fix those leaks (I'm still working on it, but my current attempt has failed and I probably fall back to a refcount solution). And to see whether the fix actually fixes the leaks, directly using the kunit tests would be handy. After all, this is what the kunit tests are there for: show what is broken within the scheduler. And those leaks definitely qualify. Or should kunit tests follow the same rules we demand from drivers? I'd like to hear more opinions about that. @Danilo, @Dave, @Sima would it be OK if we add kunit tests for the scheduler to DRM that cause leaks until we can fix them? P. > > Regards, > > Tvrtko >
On 07/03/2025 18:06, Philipp Stanner wrote: > On Fri, 2025-03-07 at 16:59 +0000, Tvrtko Ursulin wrote: >> >> On 07/03/2025 13:41, Philipp Stanner wrote: >>> Hi, >>> >>> You forgot to put folks in CC as recipents for the cover letter :( >>> >>> >>> On Thu, 2025-03-06 at 17:05 +0000, Tvrtko Ursulin wrote: >>>> Move some options out into a new debug specific kconfig file in >>>> order >>>> to >>>> make things a bit cleaner. >>>> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> >>>> Cc: Christian König <christian.koenig@amd.com> >>>> Cc: Danilo Krummrich <dakr@kernel.org> >>>> Cc: Matthew Brost <matthew.brost@intel.com> >>>> Cc: Philipp Stanner <phasta@kernel.org> >>> >>> We all have our individual work flows, so don't take this as >>> lecturing >>> or anything – I just suspect that I was forgotten in the cover >>> letter >>> because you Cc people by hand in the individual patches. >>> >>> What I do is that I run get_maintainer and then put the individuals >>> listed there into the --to= field. That sends the entire series to >>> all >>> of them. >>> >>> Only sometimes, when there's a huge list of recipents or when the >>> patches of a series are very independent, I deviate from that rule. >>> >>> JFYI >> >> Notice it was there in v3, I just omitted to paste it this time. >> >>> Anyways, we have a bigger problem about the entire series. I now >>> tested >>> again with the same setup as yesterday and the faults are indeed >>> gone, >>> so that's good. >>> >>> But to be sure I then did run kmemleak and got a list of leaks that >>> is >>> more than 2000 lines long. >> >> There is this comment for drm_sched_fini which ends with: >> >> """ >> ... >> * This stops submission of new jobs to the hardware through >> * drm_sched_backend_ops.run_job(). Consequently, >> drm_sched_backend_ops.free_job() >> * will not be called for all jobs still in >> drm_gpu_scheduler.pending_list. >> * There is no solution for this currently. Thus, it is up to the >> driver to make >> * sure that: >> * >> * a) drm_sched_fini() is only called after for all submitted jobs >> * drm_sched_backend_ops.free_job() has been called or that >> * b) the jobs for which drm_sched_backend_ops.free_job() has not >> been >> called >> * after drm_sched_fini() ran are freed manually. >> * >> >> * FIXME: Take care of the above problem and prevent this function >> from >> leaking >> * the jobs in drm_gpu_scheduler.pending_list under any >> circumstances. >> """ >> >> I got bitten by that. Keep forgetting how fragile the thing is.. :( > > argh damn, those are *all* from the pending_list?! Right, all leaks I saw were from the drm_sched_basic_entity_cleanup test. All other tests actually wait for jobs to finish so can't hit that. Fix was simply to add a drm_sched_job_cleanup call when unwinding unfinished mock scheduler jobs from drm_mock_sched_fini, which happens before calling drm_sched_fini. That's pretty much how things are expected to be handled AFAIU. > OK. Well. > > Now we've got a philosophical problem: > > We still have to fix those leaks (I'm still working on it, but my > current attempt has failed and I probably fall back to a refcount > solution). You propose to move the responsibility of cleaning up in-flight jobs to the scheduler core? > And to see whether the fix actually fixes the leaks, directly using the > kunit tests would be handy. > > After all, this is what the kunit tests are there for: show what is > broken within the scheduler. And those leaks definitely qualify. Or > should kunit tests follow the same rules we demand from drivers? > > I'd like to hear more opinions about that. > > @Danilo, @Dave, @Sima > would it be OK if we add kunit tests for the scheduler to DRM that > cause leaks until we can fix them? It is indeed a bit philosophical. I'd say only if there is a 100% agreement that drm_sched_fini should be able to clean up, or drive cleaning up, all driver state. And if we are prepared to handle a permanently failing test from now to some future date when this would be implemented. I have a similar conundrum with set priority, where I was contemplating to add a permanently failing test showing how that does not fully work, and then get improved with my deadline scheduling series. On the other side of the argument is the past experience of CI systems generally not coping well with permanently failing test. Eventually they succumb to the pressure to remove them due noisy results. Therefore other option is to have the mock scheduler adhere to the current implementation and only change it once the DRM scheduler rules change. Regards, Tvrtko
On Mon, 2025-03-10 at 09:55 +0000, Tvrtko Ursulin wrote: > > On 07/03/2025 18:06, Philipp Stanner wrote: > > On Fri, 2025-03-07 at 16:59 +0000, Tvrtko Ursulin wrote: > > > > > > On 07/03/2025 13:41, Philipp Stanner wrote: > > > > Hi, > > > > > > > > You forgot to put folks in CC as recipents for the cover letter > > > > :( > > > > > > > > > > > > On Thu, 2025-03-06 at 17:05 +0000, Tvrtko Ursulin wrote: > > > > > Move some options out into a new debug specific kconfig file > > > > > in > > > > > order > > > > > to > > > > > make things a bit cleaner. > > > > > > > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > > > > > Cc: Christian König <christian.koenig@amd.com> > > > > > Cc: Danilo Krummrich <dakr@kernel.org> > > > > > Cc: Matthew Brost <matthew.brost@intel.com> > > > > > Cc: Philipp Stanner <phasta@kernel.org> > > > > > > > > We all have our individual work flows, so don't take this as > > > > lecturing > > > > or anything – I just suspect that I was forgotten in the cover > > > > letter > > > > because you Cc people by hand in the individual patches. > > > > > > > > What I do is that I run get_maintainer and then put the > > > > individuals > > > > listed there into the --to= field. That sends the entire series > > > > to > > > > all > > > > of them. > > > > > > > > Only sometimes, when there's a huge list of recipents or when > > > > the > > > > patches of a series are very independent, I deviate from that > > > > rule. > > > > > > > > JFYI > > > > > > Notice it was there in v3, I just omitted to paste it this time. > > > > > > > Anyways, we have a bigger problem about the entire series. I > > > > now > > > > tested > > > > again with the same setup as yesterday and the faults are > > > > indeed > > > > gone, > > > > so that's good. > > > > > > > > But to be sure I then did run kmemleak and got a list of leaks > > > > that > > > > is > > > > more than 2000 lines long. > > > > > > There is this comment for drm_sched_fini which ends with: > > > > > > """ > > > ... > > > * This stops submission of new jobs to the hardware through > > > * drm_sched_backend_ops.run_job(). Consequently, > > > drm_sched_backend_ops.free_job() > > > * will not be called for all jobs still in > > > drm_gpu_scheduler.pending_list. > > > * There is no solution for this currently. Thus, it is up to > > > the > > > driver to make > > > * sure that: > > > * > > > * a) drm_sched_fini() is only called after for all submitted > > > jobs > > > * drm_sched_backend_ops.free_job() has been called or that > > > * b) the jobs for which drm_sched_backend_ops.free_job() has > > > not > > > been > > > called > > > * after drm_sched_fini() ran are freed manually. > > > * > > > > > > * FIXME: Take care of the above problem and prevent this > > > function > > > from > > > leaking > > > * the jobs in drm_gpu_scheduler.pending_list under any > > > circumstances. > > > """ > > > > > > I got bitten by that. Keep forgetting how fragile the thing is.. > > > :( > > > > argh damn, those are *all* from the pending_list?! > > Right, all leaks I saw were from the drm_sched_basic_entity_cleanup > test. All other tests actually wait for jobs to finish so can't hit > that. > > Fix was simply to add a drm_sched_job_cleanup call when unwinding > unfinished mock scheduler jobs from drm_mock_sched_fini, which > happens > before calling drm_sched_fini. > > That's pretty much how things are expected to be handled AFAIU. > > > OK. Well. > > > > Now we've got a philosophical problem: > > > > We still have to fix those leaks (I'm still working on it, but my > > current attempt has failed and I probably fall back to a refcount > > solution). > > You propose to move the responsibility of cleaning up in-flight jobs > to > the scheduler core? The scheduler core is already and has always been responsible for cleaning up "in-flight jobs". It does so through backend_ops.free_job(). And we prevent it from cleaning up all jobs by cancelling the work items in drm_sched_fini(). Semantically, the scheduler is the one in charge of the job life times. As of right now, every single driver is effectively forced to implement the same logic, but they have implemented it in different ways (Xe refcounts the scheduler and only calls drm_sched_fini() once refcnt == 0, Nouveau maintains a copy of the pending_list, blocking for it to become empty before calling drm_sched_fini()) > > > And to see whether the fix actually fixes the leaks, directly using > > the > > kunit tests would be handy. > > > > After all, this is what the kunit tests are there for: show what is > > broken within the scheduler. And those leaks definitely qualify. Or > > should kunit tests follow the same rules we demand from drivers? > > > > I'd like to hear more opinions about that. > > > > @Danilo, @Dave, @Sima > > would it be OK if we add kunit tests for the scheduler to DRM that > > cause leaks until we can fix them? > > It is indeed a bit philosophical. I'd say only if there is a 100% > agreement that drm_sched_fini should be able to clean up, or drive > cleaning up, all driver state. And if we are prepared to handle a > permanently failing test from now to some future date when this would > be > implemented. > > I have a similar conundrum with set priority, where I was > contemplating > to add a permanently failing test showing how that does not fully > work, > and then get improved with my deadline scheduling series. > > On the other side of the argument is the past experience of CI > systems > generally not coping well with permanently failing test. Eventually > they > succumb to the pressure to remove them due noisy results. Therefore > other option is to have the mock scheduler adhere to the current > implementation and only change it once the DRM scheduler rules > change. Can you think of a way, like flags or kconfig options, with which developers such as you and I could "switch the bugs on" for working on those issues? P. > > Regards, > > Tvrtko >
On 10/03/2025 11:11, Philipp Stanner wrote: > On Mon, 2025-03-10 at 09:55 +0000, Tvrtko Ursulin wrote: >> >> On 07/03/2025 18:06, Philipp Stanner wrote: >>> On Fri, 2025-03-07 at 16:59 +0000, Tvrtko Ursulin wrote: >>>> >>>> On 07/03/2025 13:41, Philipp Stanner wrote: >>>>> Hi, >>>>> >>>>> You forgot to put folks in CC as recipents for the cover letter >>>>> :( >>>>> >>>>> >>>>> On Thu, 2025-03-06 at 17:05 +0000, Tvrtko Ursulin wrote: >>>>>> Move some options out into a new debug specific kconfig file >>>>>> in >>>>>> order >>>>>> to >>>>>> make things a bit cleaner. >>>>>> >>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> >>>>>> Cc: Christian König <christian.koenig@amd.com> >>>>>> Cc: Danilo Krummrich <dakr@kernel.org> >>>>>> Cc: Matthew Brost <matthew.brost@intel.com> >>>>>> Cc: Philipp Stanner <phasta@kernel.org> >>>>> >>>>> We all have our individual work flows, so don't take this as >>>>> lecturing >>>>> or anything – I just suspect that I was forgotten in the cover >>>>> letter >>>>> because you Cc people by hand in the individual patches. >>>>> >>>>> What I do is that I run get_maintainer and then put the >>>>> individuals >>>>> listed there into the --to= field. That sends the entire series >>>>> to >>>>> all >>>>> of them. >>>>> >>>>> Only sometimes, when there's a huge list of recipents or when >>>>> the >>>>> patches of a series are very independent, I deviate from that >>>>> rule. >>>>> >>>>> JFYI >>>> >>>> Notice it was there in v3, I just omitted to paste it this time. >>>> >>>>> Anyways, we have a bigger problem about the entire series. I >>>>> now >>>>> tested >>>>> again with the same setup as yesterday and the faults are >>>>> indeed >>>>> gone, >>>>> so that's good. >>>>> >>>>> But to be sure I then did run kmemleak and got a list of leaks >>>>> that >>>>> is >>>>> more than 2000 lines long. >>>> >>>> There is this comment for drm_sched_fini which ends with: >>>> >>>> """ >>>> ... >>>> * This stops submission of new jobs to the hardware through >>>> * drm_sched_backend_ops.run_job(). Consequently, >>>> drm_sched_backend_ops.free_job() >>>> * will not be called for all jobs still in >>>> drm_gpu_scheduler.pending_list. >>>> * There is no solution for this currently. Thus, it is up to >>>> the >>>> driver to make >>>> * sure that: >>>> * >>>> * a) drm_sched_fini() is only called after for all submitted >>>> jobs >>>> * drm_sched_backend_ops.free_job() has been called or that >>>> * b) the jobs for which drm_sched_backend_ops.free_job() has >>>> not >>>> been >>>> called >>>> * after drm_sched_fini() ran are freed manually. >>>> * >>>> >>>> * FIXME: Take care of the above problem and prevent this >>>> function >>>> from >>>> leaking >>>> * the jobs in drm_gpu_scheduler.pending_list under any >>>> circumstances. >>>> """ >>>> >>>> I got bitten by that. Keep forgetting how fragile the thing is.. >>>> :( >>> >>> argh damn, those are *all* from the pending_list?! >> >> Right, all leaks I saw were from the drm_sched_basic_entity_cleanup >> test. All other tests actually wait for jobs to finish so can't hit >> that. >> >> Fix was simply to add a drm_sched_job_cleanup call when unwinding >> unfinished mock scheduler jobs from drm_mock_sched_fini, which >> happens >> before calling drm_sched_fini. >> >> That's pretty much how things are expected to be handled AFAIU. >> >>> OK. Well. >>> >>> Now we've got a philosophical problem: >>> >>> We still have to fix those leaks (I'm still working on it, but my >>> current attempt has failed and I probably fall back to a refcount >>> solution). >> >> You propose to move the responsibility of cleaning up in-flight jobs >> to >> the scheduler core? > > The scheduler core is already and has always been responsible for > cleaning up "in-flight jobs". It does so through > backend_ops.free_job(). And we prevent it from cleaning up all jobs by > cancelling the work items in drm_sched_fini(). > > Semantically, the scheduler is the one in charge of the job life times. > > As of right now, every single driver is effectively forced to implement > the same logic, but they have implemented it in different ways (Xe > refcounts the scheduler and only calls drm_sched_fini() once refcnt == > 0, Nouveau maintains a copy of the pending_list, blocking for it to > become empty before calling drm_sched_fini()) Right. And to change it means making ->free_job() for all drivers handle different potential job states, while today it only needs to handle finished jobs. Or adding a new vfunc. Or something. It sounds doable but a lot of work, not least because there is a lot of drivers. >>> And to see whether the fix actually fixes the leaks, directly using >>> the >>> kunit tests would be handy. >>> >>> After all, this is what the kunit tests are there for: show what is >>> broken within the scheduler. And those leaks definitely qualify. Or >>> should kunit tests follow the same rules we demand from drivers? >>> >>> I'd like to hear more opinions about that. >>> >>> @Danilo, @Dave, @Sima >>> would it be OK if we add kunit tests for the scheduler to DRM that >>> cause leaks until we can fix them? >> >> It is indeed a bit philosophical. I'd say only if there is a 100% >> agreement that drm_sched_fini should be able to clean up, or drive >> cleaning up, all driver state. And if we are prepared to handle a >> permanently failing test from now to some future date when this would >> be >> implemented. >> >> I have a similar conundrum with set priority, where I was >> contemplating >> to add a permanently failing test showing how that does not fully >> work, >> and then get improved with my deadline scheduling series. >> >> On the other side of the argument is the past experience of CI >> systems >> generally not coping well with permanently failing test. Eventually >> they >> succumb to the pressure to remove them due noisy results. Therefore >> other option is to have the mock scheduler adhere to the current >> implementation and only change it once the DRM scheduler rules >> change. > > Can you think of a way, like flags or kconfig options, with which > developers such as you and I could "switch the bugs on" for working on > those issues? We could do that easily I think. Something like: config DRM_SCHED_KUNIT_TEST_ASPIRATIONAL bool "Turn on the aspirational mode for DRM scheduler unit tests" if !KUNIT_ALL_TESTS select DRM_SCHED depends on DRM && KUNIT && DRM_SCHED_KUNIT_TEST default KUNIT_ALL_TESTS help Choose this option to make the DRM scheduler unit tests test for behaviour which was agreed as a design goal, even if the current implementation can make specific tests fail. Recommended for driver developers only. If in doubt, say "N". I can skip the job cleanup based on it and also add some validation that the pending list is empty after drm_sched_fini if on. Regards, Tvrtko
On Mon, 2025-03-10 at 11:54 +0000, Tvrtko Ursulin wrote: > > On 10/03/2025 11:11, Philipp Stanner wrote: > > On Mon, 2025-03-10 at 09:55 +0000, Tvrtko Ursulin wrote: > > > > > > On 07/03/2025 18:06, Philipp Stanner wrote: > > > > On Fri, 2025-03-07 at 16:59 +0000, Tvrtko Ursulin wrote: > > > > > > > > > > On 07/03/2025 13:41, Philipp Stanner wrote: > > > > > > Hi, > > > > > > > > > > > > You forgot to put folks in CC as recipents for the cover > > > > > > letter > > > > > > :( > > > > > > > > > > > > > > > > > > On Thu, 2025-03-06 at 17:05 +0000, Tvrtko Ursulin wrote: > > > > > > > Move some options out into a new debug specific kconfig > > > > > > > file > > > > > > > in > > > > > > > order > > > > > > > to > > > > > > > make things a bit cleaner. > > > > > > > > > > > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > > > > > > > Cc: Christian König <christian.koenig@amd.com> > > > > > > > Cc: Danilo Krummrich <dakr@kernel.org> > > > > > > > Cc: Matthew Brost <matthew.brost@intel.com> > > > > > > > Cc: Philipp Stanner <phasta@kernel.org> > > > > > > > > > > > > We all have our individual work flows, so don't take this > > > > > > as > > > > > > lecturing > > > > > > or anything – I just suspect that I was forgotten in the > > > > > > cover > > > > > > letter > > > > > > because you Cc people by hand in the individual patches. > > > > > > > > > > > > What I do is that I run get_maintainer and then put the > > > > > > individuals > > > > > > listed there into the --to= field. That sends the entire > > > > > > series > > > > > > to > > > > > > all > > > > > > of them. > > > > > > > > > > > > Only sometimes, when there's a huge list of recipents or > > > > > > when > > > > > > the > > > > > > patches of a series are very independent, I deviate from > > > > > > that > > > > > > rule. > > > > > > > > > > > > JFYI > > > > > > > > > > Notice it was there in v3, I just omitted to paste it this > > > > > time. > > > > > > > > > > > Anyways, we have a bigger problem about the entire series. > > > > > > I > > > > > > now > > > > > > tested > > > > > > again with the same setup as yesterday and the faults are > > > > > > indeed > > > > > > gone, > > > > > > so that's good. > > > > > > > > > > > > But to be sure I then did run kmemleak and got a list of > > > > > > leaks > > > > > > that > > > > > > is > > > > > > more than 2000 lines long. > > > > > > > > > > There is this comment for drm_sched_fini which ends with: > > > > > > > > > > """ > > > > > ... > > > > > * This stops submission of new jobs to the hardware > > > > > through > > > > > * drm_sched_backend_ops.run_job(). Consequently, > > > > > drm_sched_backend_ops.free_job() > > > > > * will not be called for all jobs still in > > > > > drm_gpu_scheduler.pending_list. > > > > > * There is no solution for this currently. Thus, it is up > > > > > to > > > > > the > > > > > driver to make > > > > > * sure that: > > > > > * > > > > > * a) drm_sched_fini() is only called after for all > > > > > submitted > > > > > jobs > > > > > * drm_sched_backend_ops.free_job() has been called or > > > > > that > > > > > * b) the jobs for which drm_sched_backend_ops.free_job() > > > > > has > > > > > not > > > > > been > > > > > called > > > > > * after drm_sched_fini() ran are freed manually. > > > > > * > > > > > > > > > > * FIXME: Take care of the above problem and prevent this > > > > > function > > > > > from > > > > > leaking > > > > > * the jobs in drm_gpu_scheduler.pending_list under any > > > > > circumstances. > > > > > """ > > > > > > > > > > I got bitten by that. Keep forgetting how fragile the thing > > > > > is.. > > > > > :( > > > > > > > > argh damn, those are *all* from the pending_list?! > > > > > > Right, all leaks I saw were from the > > > drm_sched_basic_entity_cleanup > > > test. All other tests actually wait for jobs to finish so can't > > > hit > > > that. > > > > > > Fix was simply to add a drm_sched_job_cleanup call when unwinding > > > unfinished mock scheduler jobs from drm_mock_sched_fini, which > > > happens > > > before calling drm_sched_fini. > > > > > > That's pretty much how things are expected to be handled AFAIU. > > > > > > > OK. Well. > > > > > > > > Now we've got a philosophical problem: > > > > > > > > We still have to fix those leaks (I'm still working on it, but > > > > my > > > > current attempt has failed and I probably fall back to a > > > > refcount > > > > solution). > > > > > > You propose to move the responsibility of cleaning up in-flight > > > jobs > > > to > > > the scheduler core? > > > > The scheduler core is already and has always been responsible for > > cleaning up "in-flight jobs". It does so through > > backend_ops.free_job(). And we prevent it from cleaning up all jobs > > by > > cancelling the work items in drm_sched_fini(). > > > > Semantically, the scheduler is the one in charge of the job life > > times. > > > > As of right now, every single driver is effectively forced to > > implement > > the same logic, but they have implemented it in different ways (Xe > > refcounts the scheduler and only calls drm_sched_fini() once refcnt > > == > > 0, Nouveau maintains a copy of the pending_list, blocking for it to > > become empty before calling drm_sched_fini()) > > Right. And to change it means making ->free_job() for all drivers > handle > different potential job states, while today it only needs to handle > finished jobs. Or adding a new vfunc. Or something. It sounds doable > but > a lot of work, not least because there is a lot of drivers. I know the pain, I'm working on that since ~November. I plan to propose a new solution "soon"(tm) > > > > > And to see whether the fix actually fixes the leaks, directly > > > > using > > > > the > > > > kunit tests would be handy. > > > > > > > > After all, this is what the kunit tests are there for: show > > > > what is > > > > broken within the scheduler. And those leaks definitely > > > > qualify. Or > > > > should kunit tests follow the same rules we demand from > > > > drivers? > > > > > > > > I'd like to hear more opinions about that. > > > > > > > > @Danilo, @Dave, @Sima > > > > would it be OK if we add kunit tests for the scheduler to DRM > > > > that > > > > cause leaks until we can fix them? > > > > > > It is indeed a bit philosophical. I'd say only if there is a 100% > > > agreement that drm_sched_fini should be able to clean up, or > > > drive > > > cleaning up, all driver state. And if we are prepared to handle a > > > permanently failing test from now to some future date when this > > > would > > > be > > > implemented. > > > > > > I have a similar conundrum with set priority, where I was > > > contemplating > > > to add a permanently failing test showing how that does not fully > > > work, > > > and then get improved with my deadline scheduling series. > > > > > > On the other side of the argument is the past experience of CI > > > systems > > > generally not coping well with permanently failing test. > > > Eventually > > > they > > > succumb to the pressure to remove them due noisy results. > > > Therefore > > > other option is to have the mock scheduler adhere to the current > > > implementation and only change it once the DRM scheduler rules > > > change. > > > > Can you think of a way, like flags or kconfig options, with which > > developers such as you and I could "switch the bugs on" for working > > on > > those issues? > > We could do that easily I think. Something like: > > config DRM_SCHED_KUNIT_TEST_ASPIRATIONAL > bool "Turn on the aspirational mode for DRM scheduler unit > tests" if !KUNIT_ALL_TESTS > select DRM_SCHED > depends on DRM && KUNIT && DRM_SCHED_KUNIT_TEST > default KUNIT_ALL_TESTS > help > Choose this option to make the DRM scheduler unit tests > test > for behaviour which was agreed as a design goal, even if the current > implementation can make specific tests fail. > > Recommended for driver developers only. > > If in doubt, say "N". If you can work out something like this, that would be fantastic and solve all the aforementioned problems > > I can skip the job cleanup based on it and also add some validation > that > the pending list is empty after drm_sched_fini if on. Sounds handy. That way the developer doesn't even have to use kmemleak. P. > > Regards, > > Tvrtko >
On 10/03/2025 12:00, Philipp Stanner wrote: > On Mon, 2025-03-10 at 11:54 +0000, Tvrtko Ursulin wrote: >> >> On 10/03/2025 11:11, Philipp Stanner wrote: >>> On Mon, 2025-03-10 at 09:55 +0000, Tvrtko Ursulin wrote: >>>> >>>> On 07/03/2025 18:06, Philipp Stanner wrote: >>>>> On Fri, 2025-03-07 at 16:59 +0000, Tvrtko Ursulin wrote: >>>>>> >>>>>> On 07/03/2025 13:41, Philipp Stanner wrote: >>>>>>> Hi, >>>>>>> >>>>>>> You forgot to put folks in CC as recipents for the cover >>>>>>> letter >>>>>>> :( >>>>>>> >>>>>>> >>>>>>> On Thu, 2025-03-06 at 17:05 +0000, Tvrtko Ursulin wrote: >>>>>>>> Move some options out into a new debug specific kconfig >>>>>>>> file >>>>>>>> in >>>>>>>> order >>>>>>>> to >>>>>>>> make things a bit cleaner. >>>>>>>> >>>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> >>>>>>>> Cc: Christian König <christian.koenig@amd.com> >>>>>>>> Cc: Danilo Krummrich <dakr@kernel.org> >>>>>>>> Cc: Matthew Brost <matthew.brost@intel.com> >>>>>>>> Cc: Philipp Stanner <phasta@kernel.org> >>>>>>> >>>>>>> We all have our individual work flows, so don't take this >>>>>>> as >>>>>>> lecturing >>>>>>> or anything – I just suspect that I was forgotten in the >>>>>>> cover >>>>>>> letter >>>>>>> because you Cc people by hand in the individual patches. >>>>>>> >>>>>>> What I do is that I run get_maintainer and then put the >>>>>>> individuals >>>>>>> listed there into the --to= field. That sends the entire >>>>>>> series >>>>>>> to >>>>>>> all >>>>>>> of them. >>>>>>> >>>>>>> Only sometimes, when there's a huge list of recipents or >>>>>>> when >>>>>>> the >>>>>>> patches of a series are very independent, I deviate from >>>>>>> that >>>>>>> rule. >>>>>>> >>>>>>> JFYI >>>>>> >>>>>> Notice it was there in v3, I just omitted to paste it this >>>>>> time. >>>>>> >>>>>>> Anyways, we have a bigger problem about the entire series. >>>>>>> I >>>>>>> now >>>>>>> tested >>>>>>> again with the same setup as yesterday and the faults are >>>>>>> indeed >>>>>>> gone, >>>>>>> so that's good. >>>>>>> >>>>>>> But to be sure I then did run kmemleak and got a list of >>>>>>> leaks >>>>>>> that >>>>>>> is >>>>>>> more than 2000 lines long. >>>>>> >>>>>> There is this comment for drm_sched_fini which ends with: >>>>>> >>>>>> """ >>>>>> ... >>>>>> * This stops submission of new jobs to the hardware >>>>>> through >>>>>> * drm_sched_backend_ops.run_job(). Consequently, >>>>>> drm_sched_backend_ops.free_job() >>>>>> * will not be called for all jobs still in >>>>>> drm_gpu_scheduler.pending_list. >>>>>> * There is no solution for this currently. Thus, it is up >>>>>> to >>>>>> the >>>>>> driver to make >>>>>> * sure that: >>>>>> * >>>>>> * a) drm_sched_fini() is only called after for all >>>>>> submitted >>>>>> jobs >>>>>> * drm_sched_backend_ops.free_job() has been called or >>>>>> that >>>>>> * b) the jobs for which drm_sched_backend_ops.free_job() >>>>>> has >>>>>> not >>>>>> been >>>>>> called >>>>>> * after drm_sched_fini() ran are freed manually. >>>>>> * >>>>>> >>>>>> * FIXME: Take care of the above problem and prevent this >>>>>> function >>>>>> from >>>>>> leaking >>>>>> * the jobs in drm_gpu_scheduler.pending_list under any >>>>>> circumstances. >>>>>> """ >>>>>> >>>>>> I got bitten by that. Keep forgetting how fragile the thing >>>>>> is.. >>>>>> :( >>>>> >>>>> argh damn, those are *all* from the pending_list?! >>>> >>>> Right, all leaks I saw were from the >>>> drm_sched_basic_entity_cleanup >>>> test. All other tests actually wait for jobs to finish so can't >>>> hit >>>> that. >>>> >>>> Fix was simply to add a drm_sched_job_cleanup call when unwinding >>>> unfinished mock scheduler jobs from drm_mock_sched_fini, which >>>> happens >>>> before calling drm_sched_fini. >>>> >>>> That's pretty much how things are expected to be handled AFAIU. >>>> >>>>> OK. Well. >>>>> >>>>> Now we've got a philosophical problem: >>>>> >>>>> We still have to fix those leaks (I'm still working on it, but >>>>> my >>>>> current attempt has failed and I probably fall back to a >>>>> refcount >>>>> solution). >>>> >>>> You propose to move the responsibility of cleaning up in-flight >>>> jobs >>>> to >>>> the scheduler core? >>> >>> The scheduler core is already and has always been responsible for >>> cleaning up "in-flight jobs". It does so through >>> backend_ops.free_job(). And we prevent it from cleaning up all jobs >>> by >>> cancelling the work items in drm_sched_fini(). >>> >>> Semantically, the scheduler is the one in charge of the job life >>> times. >>> >>> As of right now, every single driver is effectively forced to >>> implement >>> the same logic, but they have implemented it in different ways (Xe >>> refcounts the scheduler and only calls drm_sched_fini() once refcnt >>> == >>> 0, Nouveau maintains a copy of the pending_list, blocking for it to >>> become empty before calling drm_sched_fini()) >> >> Right. And to change it means making ->free_job() for all drivers >> handle >> different potential job states, while today it only needs to handle >> finished jobs. Or adding a new vfunc. Or something. It sounds doable >> but >> a lot of work, not least because there is a lot of drivers. > > I know the pain, I'm working on that since ~November. > > I plan to propose a new solution "soon"(tm) Ack. >>>>> And to see whether the fix actually fixes the leaks, directly >>>>> using >>>>> the >>>>> kunit tests would be handy. >>>>> >>>>> After all, this is what the kunit tests are there for: show >>>>> what is >>>>> broken within the scheduler. And those leaks definitely >>>>> qualify. Or >>>>> should kunit tests follow the same rules we demand from >>>>> drivers? >>>>> >>>>> I'd like to hear more opinions about that. >>>>> >>>>> @Danilo, @Dave, @Sima >>>>> would it be OK if we add kunit tests for the scheduler to DRM >>>>> that >>>>> cause leaks until we can fix them? >>>> >>>> It is indeed a bit philosophical. I'd say only if there is a 100% >>>> agreement that drm_sched_fini should be able to clean up, or >>>> drive >>>> cleaning up, all driver state. And if we are prepared to handle a >>>> permanently failing test from now to some future date when this >>>> would >>>> be >>>> implemented. >>>> >>>> I have a similar conundrum with set priority, where I was >>>> contemplating >>>> to add a permanently failing test showing how that does not fully >>>> work, >>>> and then get improved with my deadline scheduling series. >>>> >>>> On the other side of the argument is the past experience of CI >>>> systems >>>> generally not coping well with permanently failing test. >>>> Eventually >>>> they >>>> succumb to the pressure to remove them due noisy results. >>>> Therefore >>>> other option is to have the mock scheduler adhere to the current >>>> implementation and only change it once the DRM scheduler rules >>>> change. >>> >>> Can you think of a way, like flags or kconfig options, with which >>> developers such as you and I could "switch the bugs on" for working >>> on >>> those issues? >> >> We could do that easily I think. Something like: >> >> config DRM_SCHED_KUNIT_TEST_ASPIRATIONAL >> bool "Turn on the aspirational mode for DRM scheduler unit >> tests" if !KUNIT_ALL_TESTS >> select DRM_SCHED >> depends on DRM && KUNIT && DRM_SCHED_KUNIT_TEST >> default KUNIT_ALL_TESTS >> help >> Choose this option to make the DRM scheduler unit tests >> test >> for behaviour which was agreed as a design goal, even if the current >> implementation can make specific tests fail. >> >> Recommended for driver developers only. >> >> If in doubt, say "N". > > If you can work out something like this, that would be fantastic and > solve all the aforementioned problems > >> >> I can skip the job cleanup based on it and also add some validation >> that >> the pending list is empty after drm_sched_fini if on. > > Sounds handy. That way the developer doesn't even have to use kmemleak. Okay I'll add that and respin. Maybe add an unit test for credits while at it. Regards, Tvrtko
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index d9986fd52194..46ba24592553 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -26,6 +26,11 @@ menuconfig DRM details. You should also select and configure AGP (/dev/agpgart) support if it is available for your platform. +menu "DRM debugging options" +depends on DRM +source "drivers/gpu/drm/Kconfig.debug" +endmenu + if DRM config DRM_MIPI_DBI @@ -37,65 +42,6 @@ config DRM_MIPI_DSI bool depends on DRM -config DRM_DEBUG_MM - bool "Insert extra checks and debug info into the DRM range managers" - default n - depends on DRM - depends on STACKTRACE_SUPPORT - select STACKDEPOT - help - Enable allocation tracking of memory manager and leak detection on - shutdown. - - Recommended for driver developers only. - - If in doubt, say "N". - -config DRM_USE_DYNAMIC_DEBUG - bool "use dynamic debug to implement drm.debug" - default n - depends on BROKEN - depends on DRM - depends on DYNAMIC_DEBUG || DYNAMIC_DEBUG_CORE - depends on JUMP_LABEL - help - Use dynamic-debug to avoid drm_debug_enabled() runtime overheads. - Due to callsite counts in DRM drivers (~4k in amdgpu) and 56 - bytes per callsite, the .data costs can be substantial, and - are therefore configurable. - -config DRM_KUNIT_TEST_HELPERS - tristate - depends on DRM && KUNIT - select DRM_KMS_HELPER - help - KUnit Helpers for KMS drivers. - -config DRM_KUNIT_TEST - tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS - depends on DRM && KUNIT && MMU - select DRM_BUDDY - select DRM_DISPLAY_DP_HELPER - select DRM_DISPLAY_HDMI_STATE_HELPER - select DRM_DISPLAY_HELPER - select DRM_EXEC - select DRM_EXPORT_FOR_TESTS if m - select DRM_GEM_SHMEM_HELPER - select DRM_KUNIT_TEST_HELPERS - select DRM_LIB_RANDOM - select PRIME_NUMBERS - default KUNIT_ALL_TESTS - help - This builds unit tests for DRM. This option is not useful for - distributions or general kernels, but only for kernel - developers working on DRM and associated drivers. - - For more information on KUnit and unit tests in general, - please refer to the KUnit documentation in - Documentation/dev-tools/kunit/. - - If in doubt, say "N". - config DRM_KMS_HELPER tristate depends on DRM @@ -247,23 +193,6 @@ config DRM_TTM GPU memory types. Will be enabled automatically if a device driver uses it. -config DRM_TTM_KUNIT_TEST - tristate "KUnit tests for TTM" if !KUNIT_ALL_TESTS - default n - depends on DRM && KUNIT && MMU && (UML || COMPILE_TEST) - select DRM_TTM - select DRM_BUDDY - select DRM_EXPORT_FOR_TESTS if m - select DRM_KUNIT_TEST_HELPERS - default KUNIT_ALL_TESTS - help - Enables unit tests for TTM, a GPU memory manager subsystem used - to manage memory buffers. This option is mostly useful for kernel - developers. It depends on (UML || COMPILE_TEST) since no other driver - which uses TTM can be loaded while running the tests. - - If in doubt, say "N". - config DRM_EXEC tristate depends on DRM @@ -463,9 +392,6 @@ config DRM_HYPERV If M is selected the module will be called hyperv_drm. -config DRM_EXPORT_FOR_TESTS - bool - # Separate option as not all DRM drivers use it config DRM_PANEL_BACKLIGHT_QUIRKS tristate @@ -478,31 +404,6 @@ config DRM_PRIVACY_SCREEN bool default n -config DRM_WERROR - bool "Compile the drm subsystem with warnings as errors" - depends on DRM && EXPERT - depends on !WERROR - default n - help - A kernel build should not cause any compiler warnings, and this - enables the '-Werror' flag to enforce that rule in the drm subsystem. - - The drm subsystem enables more warnings than the kernel default, so - this config option is disabled by default. - - If in doubt, say N. - -config DRM_HEADER_TEST - bool "Ensure DRM headers are self-contained and pass kernel-doc" - depends on DRM && EXPERT - default n - help - Ensure the DRM subsystem headers both under drivers/gpu/drm and - include/drm compile, are self-contained, have header guards, and have - no kernel-doc warnings. - - If in doubt, say N. - endif # Separate option because drm_panel_orientation_quirks.c is shared with fbdev diff --git a/drivers/gpu/drm/Kconfig.debug b/drivers/gpu/drm/Kconfig.debug new file mode 100644 index 000000000000..601d7e07d421 --- /dev/null +++ b/drivers/gpu/drm/Kconfig.debug @@ -0,0 +1,103 @@ +config DRM_USE_DYNAMIC_DEBUG + bool "use dynamic debug to implement drm.debug" + default n + depends on BROKEN + depends on DRM + depends on DYNAMIC_DEBUG || DYNAMIC_DEBUG_CORE + depends on JUMP_LABEL + help + Use dynamic-debug to avoid drm_debug_enabled() runtime overheads. + Due to callsite counts in DRM drivers (~4k in amdgpu) and 56 + bytes per callsite, the .data costs can be substantial, and + are therefore configurable. + +config DRM_WERROR + bool "Compile the drm subsystem with warnings as errors" + depends on DRM && EXPERT + depends on !WERROR + default n + help + A kernel build should not cause any compiler warnings, and this + enables the '-Werror' flag to enforce that rule in the drm subsystem. + + The drm subsystem enables more warnings than the kernel default, so + this config option is disabled by default. + + If in doubt, say N. + +config DRM_HEADER_TEST + bool "Ensure DRM headers are self-contained and pass kernel-doc" + depends on DRM && EXPERT + default n + help + Ensure the DRM subsystem headers both under drivers/gpu/drm and + include/drm compile, are self-contained, have header guards, and have + no kernel-doc warnings. + + If in doubt, say N. + +config DRM_DEBUG_MM + bool "Insert extra checks and debug info into the DRM range managers" + default n + depends on DRM + depends on STACKTRACE_SUPPORT + select STACKDEPOT + help + Enable allocation tracking of memory manager and leak detection on + shutdown. + + Recommended for driver developers only. + + If in doubt, say "N". + +config DRM_KUNIT_TEST_HELPERS + tristate + depends on DRM && KUNIT + select DRM_KMS_HELPER + help + KUnit Helpers for KMS drivers. + +config DRM_KUNIT_TEST + tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS + depends on DRM && KUNIT && MMU + select DRM_BUDDY + select DRM_DISPLAY_DP_HELPER + select DRM_DISPLAY_HDMI_STATE_HELPER + select DRM_DISPLAY_HELPER + select DRM_EXEC + select DRM_EXPORT_FOR_TESTS if m + select DRM_GEM_SHMEM_HELPER + select DRM_KUNIT_TEST_HELPERS + select DRM_LIB_RANDOM + select PRIME_NUMBERS + default KUNIT_ALL_TESTS + help + This builds unit tests for DRM. This option is not useful for + distributions or general kernels, but only for kernel + developers working on DRM and associated drivers. + + For more information on KUnit and unit tests in general, + please refer to the KUnit documentation in + Documentation/dev-tools/kunit/. + + If in doubt, say "N". + +config DRM_TTM_KUNIT_TEST + tristate "KUnit tests for TTM" if !KUNIT_ALL_TESTS + default n + depends on DRM && KUNIT && MMU && (UML || COMPILE_TEST) + select DRM_TTM + select DRM_BUDDY + select DRM_EXPORT_FOR_TESTS if m + select DRM_KUNIT_TEST_HELPERS + default KUNIT_ALL_TESTS + help + Enables unit tests for TTM, a GPU memory manager subsystem used + to manage memory buffers. This option is mostly useful for kernel + developers. It depends on (UML || COMPILE_TEST) since no other driver + which uses TTM can be loaded while running the tests. + + If in doubt, say "N". + +config DRM_EXPORT_FOR_TESTS + bool
Move some options out into a new debug specific kconfig file in order to make things a bit cleaner. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> Cc: Christian König <christian.koenig@amd.com> Cc: Danilo Krummrich <dakr@kernel.org> Cc: Matthew Brost <matthew.brost@intel.com> Cc: Philipp Stanner <phasta@kernel.org> --- drivers/gpu/drm/Kconfig | 109 ++-------------------------------- drivers/gpu/drm/Kconfig.debug | 103 ++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 104 deletions(-) create mode 100644 drivers/gpu/drm/Kconfig.debug