From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/ttm/tests: add a test to exercise ttm_bo_swapout Date: Tue, 10 Mar 2026 12:02:40 +1000 Message-ID: In-Reply-To: <20260309-ttm_bo_swapout_test-v2-1-219f64046cfe@igalia.com> References: <20260309-ttm_bo_swapout_test-v2-1-219f64046cfe@igalia.com> <20260309-ttm_bo_swapout_test-v2-1-219f64046cfe@igalia.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Overall assessment: Good patch, minor observations only.** **Positive aspects:** - The test follows the established patterns in `ttm_bo_validate_test.c` wel= l =E2=80=94 using `ttm_mock_manager_init`, `ttm_place_kunit_init`, `ttm_pla= cement_kunit_init`, `kunit_kzalloc`, `drm_gem_private_object_init`, `ttm_bo= _init_reserved`, and `ttm_bo_fini` in the same manner as other tests. - Using `EXPORT_SYMBOL_FOR_TESTS_ONLY` is the correct approach for making `= ttm_bo_swapout` available to the test module, and it already has precedent = (`ttm_tt_swapout` at `ttm_tt.c:368` uses the same macro). - The commit message is well-written with good context about why the previo= us test was removed and why this new approach is better. - The v2 changes (removing redundant asserts, requesting all pages) are goo= d improvements. **Observations:** 1. **`KUNIT_EXPECT_EQ` vs `KUNIT_ASSERT_EQ` for `ttm_bo_init_reserved`**: A= t line 135-136 of the test: ```c KUNIT_EXPECT_EQ(test, err, 0); dma_resv_unlock(bo->base.resv); ``` If `ttm_bo_init_reserved` fails, `err !=3D 0` and the BO may not be prop= erly initialized, yet execution continues to `dma_resv_unlock` and `ttm_bo_= swapout`. This should arguably be `KUNIT_ASSERT_EQ` to bail out on failure,= since the subsequent code depends on successful initialization. That said,= the v2 changelog says "Removed extra asserts that are already internally u= sed," so this may have been a deliberate reviewer-requested change. The exi= sting tests in this file use `KUNIT_EXPECT_EQ` in similar positions, so thi= s is consistent with the file's convention. 2. **Documentation vs implementation mismatch (pre-existing)**: The `ttm_bo= _swapout` doc says `@target: The desired number of bytes to swap out` and t= he return is described as "The number of bytes actually swapped out." Howev= er, `ttm_bo_swapout_cb` returns `ttm->num_pages` (pages, not bytes), and th= e test correctly uses `MANAGER_SIZE / PAGE_SIZE` (i.e., pages) for both tar= get and expected return: ```c swapped =3D ttm_bo_swapout(priv->ttm_dev, &ctx_init, man, GFP_KERNEL, MANAGER_SIZE / PAGE_SIZE); KUNIT_EXPECT_EQ(test, swapped, MANAGER_SIZE / PAGE_SIZE); ``` This is correct behavior matching the actual implementation, but the doc= comment on `ttm_bo_swapout` is misleading (pre-existing issue, not introdu= ced by this patch). 3. **The `#include ` addition**: Adding this include to `tt= m_bo.c` is correct since `EXPORT_SYMBOL_FOR_TESTS_ONLY` is defined in `drm/= drm_util.h`. It's clean and minimal. 4. **Cleanup ordering**: The test calls `ttm_bo_fini(bo)` before `ttm_mock_= manager_fini()`, which is the correct ordering =E2=80=94 the BO's resource = (now in SYSTEM) should be released before tearing down the mock manager. Th= is matches the pattern used elsewhere in the file. **Verdict**: The patch is well-crafted and addresses a real testing gap. Th= e EXPECT vs ASSERT question in point 1 is the only potential concern, and i= t follows the file's existing convention. Reviewed-by from Tvrtko is alread= y present. This looks good to merge. --- Generated by Claude Code Patch Reviewer