Re: [PATCH v8 0/8] Improve test coverage of TTM
On 14.12.2023 11:22, Christian König wrote: Am 14.12.23 um 09:20 schrieb Karolina Stolarek: Hi Christian, On 29.11.2023 13:02, Karolina Stolarek wrote: Karolina Stolarek (8): drm/ttm/tests: Add tests for ttm_resource and ttm_sys_man drm/ttm/tests: Add tests for ttm_tt drm/ttm/tests: Add tests for ttm_bo functions > drm/ttm/tests: Fix argument in ttm_tt_kunit_init() Would it be possible to get these patches merged? They already have r-b's. There are a couple more, but these don't depend on any other patch that is pending review, and should be easy to get in. Sure, just pushed those three to drm-misc-next. Awesome, thanks a lot. I see they went into drm-tip already, so I rebased my series on the top of that. I'm asking for this because I have three final patches ready for submission (eviction, ttm_tt_populate, TODO file) and wondered if I should submit them as v9, or if I could create a new series. I'd prefer creating a new one, this patchset is already big and intimidating :) Having said that, thank you for taking your time to review the tests, I really appreciate it. No problem, you are actually helping getting stuff from my way to long TODO list. By producing 4k+ lines of code for you to look at? ;-) All the best, Karolina Regards, Christian.
Re: [PATCH v8 0/8] Improve test coverage of TTM
Am 14.12.23 um 09:20 schrieb Karolina Stolarek: Hi Christian, On 29.11.2023 13:02, Karolina Stolarek wrote: Karolina Stolarek (8): drm/ttm/tests: Add tests for ttm_resource and ttm_sys_man drm/ttm/tests: Add tests for ttm_tt drm/ttm/tests: Add tests for ttm_bo functions > drm/ttm/tests: Fix argument in ttm_tt_kunit_init() Would it be possible to get these patches merged? They already have r-b's. There are a couple more, but these don't depend on any other patch that is pending review, and should be easy to get in. Sure, just pushed those three to drm-misc-next. I'm asking for this because I have three final patches ready for submission (eviction, ttm_tt_populate, TODO file) and wondered if I should submit them as v9, or if I could create a new series. I'd prefer creating a new one, this patchset is already big and intimidating :) Having said that, thank you for taking your time to review the tests, I really appreciate it. No problem, you are actually helping getting stuff from my way to long TODO list. Regards, Christian. All the best, Karolina drm/ttm/tests: Use an init function from the helpers lib drm/ttm/tests: Test simple BO creation and validation drm/ttm/tests: Add tests with mock resource managers drm/ttm/tests: Add test cases dependent on fence signaling drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/ttm/tests/.kunitconfig | 1 + drivers/gpu/drm/ttm/tests/Makefile | 5 + drivers/gpu/drm/ttm/tests/ttm_bo_test.c | 622 ++ .../gpu/drm/ttm/tests/ttm_bo_validate_test.c | 795 ++ drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 109 ++- drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h | 7 + drivers/gpu/drm/ttm/tests/ttm_mock_manager.c | 207 + drivers/gpu/drm/ttm/tests/ttm_mock_manager.h | 31 + drivers/gpu/drm/ttm/tests/ttm_pool_test.c | 3 +- drivers/gpu/drm/ttm/tests/ttm_resource_test.c | 335 drivers/gpu/drm/ttm/tests/ttm_tt_test.c | 282 +++ drivers/gpu/drm/ttm/ttm_resource.c | 3 + drivers/gpu/drm/ttm/ttm_tt.c | 3 + 14 files changed, 2401 insertions(+), 3 deletions(-) create mode 100644 drivers/gpu/drm/ttm/tests/ttm_bo_test.c create mode 100644 drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c create mode 100644 drivers/gpu/drm/ttm/tests/ttm_mock_manager.c create mode 100644 drivers/gpu/drm/ttm/tests/ttm_mock_manager.h create mode 100644 drivers/gpu/drm/ttm/tests/ttm_resource_test.c create mode 100644 drivers/gpu/drm/ttm/tests/ttm_tt_test.c
Re: [PATCH v8 0/8] Improve test coverage of TTM
Hi Christian, On 29.11.2023 13:02, Karolina Stolarek wrote: Karolina Stolarek (8): drm/ttm/tests: Add tests for ttm_resource and ttm_sys_man drm/ttm/tests: Add tests for ttm_tt drm/ttm/tests: Add tests for ttm_bo functions >drm/ttm/tests: Fix argument in ttm_tt_kunit_init() Would it be possible to get these patches merged? They already have r-b's. There are a couple more, but these don't depend on any other patch that is pending review, and should be easy to get in. I'm asking for this because I have three final patches ready for submission (eviction, ttm_tt_populate, TODO file) and wondered if I should submit them as v9, or if I could create a new series. I'd prefer creating a new one, this patchset is already big and intimidating :) Having said that, thank you for taking your time to review the tests, I really appreciate it. All the best, Karolina drm/ttm/tests: Use an init function from the helpers lib drm/ttm/tests: Test simple BO creation and validation drm/ttm/tests: Add tests with mock resource managers drm/ttm/tests: Add test cases dependent on fence signaling drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/ttm/tests/.kunitconfig| 1 + drivers/gpu/drm/ttm/tests/Makefile| 5 + drivers/gpu/drm/ttm/tests/ttm_bo_test.c | 622 ++ .../gpu/drm/ttm/tests/ttm_bo_validate_test.c | 795 ++ drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 109 ++- drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h | 7 + drivers/gpu/drm/ttm/tests/ttm_mock_manager.c | 207 + drivers/gpu/drm/ttm/tests/ttm_mock_manager.h | 31 + drivers/gpu/drm/ttm/tests/ttm_pool_test.c | 3 +- drivers/gpu/drm/ttm/tests/ttm_resource_test.c | 335 drivers/gpu/drm/ttm/tests/ttm_tt_test.c | 282 +++ drivers/gpu/drm/ttm/ttm_resource.c| 3 + drivers/gpu/drm/ttm/ttm_tt.c | 3 + 14 files changed, 2401 insertions(+), 3 deletions(-) create mode 100644 drivers/gpu/drm/ttm/tests/ttm_bo_test.c create mode 100644 drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c create mode 100644 drivers/gpu/drm/ttm/tests/ttm_mock_manager.c create mode 100644 drivers/gpu/drm/ttm/tests/ttm_mock_manager.h create mode 100644 drivers/gpu/drm/ttm/tests/ttm_resource_test.c create mode 100644 drivers/gpu/drm/ttm/tests/ttm_tt_test.c
Re: [PATCH v8 0/8] Improve test coverage of TTM
Hi Karolina, On Wed, Dec 06, 2023 at 12:28:14PM +0100, Karolina Stolarek wrote: > On 5.12.2023 16:39, Andi Shyti wrote: ... > > Hi Karolina and Christian, ... > > > Karolina Stolarek (8): drm/ttm/tests: Add tests for ttm_resource and > > > ttm_sys_man drm/ttm/tests: Add tests for ttm_tt drm/ttm/tests: > > > Add tests for ttm_bo functions drm/ttm/tests: Fix argument in > > > ttm_tt_kunit_init() drm/ttm/tests: Use an init function from the > > > helpers lib drm/ttm/tests: Test simple BO creation and validation > > > drm/ttm/tests: Add tests with mock resource managers drm/ttm/tests: > > > Add test cases dependent on fence signaling > > > > I see that all these files (and previous patches, as well) don't have > > a consistent prefix system, like kunit_ttm_*. But they all start with > > ttm_*, thread_*, drm_*, etc, which makes it a bit difficult to follow > > and grep through. > > I see what you mean. As for ttm_* part, I decided to keep it as it is > based on what was in DRM KUnit tests and helpers lib. Almost all > functions and subtests start with drm_*, and as I'm working on TTM > tests, ttm_* prefix seemed like a natural choice. With thread_*, I could > add ttm_kunit_* in front of it, but not sure what would be the benefit > of doing this. I do not agree here, ttm_* are related to ttm and not "testing ttm" and we need to be consistent. I'd be more inclined to a kunit_ttm_* or ttm_kunit_*. Anyway, let's hear also what Christian thinks. > To be honest, I haven't considered using kunit_* prefix here. In the > code context, it's obvious these are kunit tests, and repeating that > information would make already long function/struct names even longer. > > I had a quick glance at other KUnit tests in the kernel and this seems > to be the case, no kunit_* prefixes are used. > > > Can we please maintain a consistent prefix system? > > > > I know it looks bad to come out with it after this series (and previous > > work too) has been out for several months already. I need to > > say my "mea culpa" as well as I've been in the review loop, as well. > > After this series, I plan to send a small one to wrap up my work in this > field. How about adding a TODO to clean these up? I mean, that's > just how I see it, I'd like to hear Christian's thoughts on this. nah... a TODO note would be too much... your promise is enough for me. Andi
Re: [PATCH v8 0/8] Improve test coverage of TTM
Hi Andi, On 5.12.2023 16:39, Andi Shyti wrote: Hi Karolina and Christian, Karolina Stolarek (8): drm/ttm/tests: Add tests for ttm_resource and ttm_sys_man drm/ttm/tests: Add tests for ttm_tt drm/ttm/tests: Add tests for ttm_bo functions drm/ttm/tests: Fix argument in ttm_tt_kunit_init() drm/ttm/tests: Use an init function from the helpers lib drm/ttm/tests: Test simple BO creation and validation drm/ttm/tests: Add tests with mock resource managers drm/ttm/tests: Add test cases dependent on fence signaling I see that all these files (and previous patches, as well) don't have a consistent prefix system, like kunit_ttm_*. But they all start with ttm_*, thread_*, drm_*, etc, which makes it a bit difficult to follow and grep through. I see what you mean. As for ttm_* part, I decided to keep it as it is based on what was in DRM KUnit tests and helpers lib. Almost all functions and subtests start with drm_*, and as I'm working on TTM tests, ttm_* prefix seemed like a natural choice. With thread_*, I could add ttm_kunit_* in front of it, but not sure what would be the benefit of doing this. To be honest, I haven't considered using kunit_* prefix here. In the code context, it's obvious these are kunit tests, and repeating that information would make already long function/struct names even longer. I had a quick glance at other KUnit tests in the kernel and this seems to be the case, no kunit_* prefixes are used. Can we please maintain a consistent prefix system? I know it looks bad to come out with it after this series (and previous work too) has been out for several months already. I need to say my "mea culpa" as well as I've been in the review loop, as well. After this series, I plan to send a small one to wrap up my work in this field. How about adding a TODO to clean these up? I mean, that's just how I see it, I'd like to hear Christian's thoughts on this. All the best, Karolina
Re: [PATCH v8 0/8] Improve test coverage of TTM
Hi Karolina and Christian, > Karolina Stolarek (8): > drm/ttm/tests: Add tests for ttm_resource and ttm_sys_man > drm/ttm/tests: Add tests for ttm_tt > drm/ttm/tests: Add tests for ttm_bo functions > drm/ttm/tests: Fix argument in ttm_tt_kunit_init() > drm/ttm/tests: Use an init function from the helpers lib > drm/ttm/tests: Test simple BO creation and validation > drm/ttm/tests: Add tests with mock resource managers > drm/ttm/tests: Add test cases dependent on fence signaling I see that all these files (and previous patches, as well) don't have a consistent prefix system, like kunit_ttm_*. But they all start with ttm_*, thread_*, drm_*, etc, which makes it a bit difficult to follow and grep through. Can we please maintain a consistent prefix system? I know it looks bad to come out with it after this series (and previous work too) has been out for several months already. I need to say my "mea culpa" as well as I've been in the review loop, as well. Thanks, Andi
