From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: accel/amdxdna: Expose per-client BO memory usage via fdinfo Date: Sun, 12 Apr 2026 11:06:37 +1000 Message-ID: In-Reply-To: <20260409152259.176883-1-lizhi.hou@amd.com> References: <20260409152259.176883-1-lizhi.hou@amd.com> <20260409152259.176883-1-lizhi.hou@amd.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 **Positive aspects:** - Follows the same pattern as panthor's `panthor_show_internal_memory_stats= ` (even uses the same `char *drv_name =3D file->minor->dev->driver->name` i= diom). - Correctly locks `mm_lock` while reading the usage counters and unlocks be= fore printing. - Documentation addition references the standard DRM client usage stats spe= c. **Issues:** 1. **Potential unsigned underflow in external_usage calculation (medium sev= erity):** ```c external_usage =3D client->total_bo_usage - internal_usage; ``` Both `total_bo_usage` and `internal_usage` are `size_t` (unsigned). If ther= e's ever a bug or race where `total_int_bo_usage > total_bo_usage`, this si= lently wraps to a huge value and prints nonsense in fdinfo. While the lock = should prevent races, a defensive check or at minimum a `WARN_ON` would be = appropriate: ```c if (WARN_ON(client->total_bo_usage < client->total_int_bo_usage)) external_usage =3D 0; else external_usage =3D client->total_bo_usage - internal_usage; ``` 2. **Type width mismatch: `size_t` passed where `u64` expected (minor):** `drm_fdinfo_print_size()` takes `u64 sz` as its last parameter, but `heap_u= sage`, `internal_usage`, and `external_usage` are all `size_t`. On 32-bit a= rchitectures `size_t` is 32-bit while `u64` is 64-bit. This is not incorrec= t (implicit promotion), but it does mean the values are silently truncated = to 32 bits on 32-bit builds. This likely doesn't matter for this driver (NP= U hardware probably doesn't target 32-bit), but explicitly casting to `u64`= or declaring the local variables as `u64` would be cleaner. Note that pant= hor avoids this by passing through `struct drm_memory_stats` which uses `u6= 4` fields. 3. **Blank line removal in `drm_driver` struct (nit):** ```c .num_ioctls =3D ARRAY_SIZE(amdxdna_drm_ioctls), - + .show_fdinfo =3D amdxdna_show_fdinfo, .gem_create_object =3D amdxdna_gem_create_shmem_object_cb, ``` The original code had a blank line separating the ioctl-related fields from= the gem-related fields. The patch removes that visual grouping to insert `= .show_fdinfo`. It would be better to keep the blank line and place `.show_f= dinfo` either before the blank line (with the other callbacks like `.open`/= `.postclose`) or after a new blank line: ```c .num_ioctls =3D ARRAY_SIZE(amdxdna_drm_ioctls), .show_fdinfo =3D amdxdna_show_fdinfo, .gem_create_object =3D amdxdna_gem_create_shmem_object_cb, ``` 4. **Documentation: driver name prefix is rather long and verbose (observat= ion):** The fdinfo keys use the full driver name as prefix: ``` amdxdna_accel_driver-heap-alloc: 60 KiB amdxdna_accel_driver-internal-alloc: 67588 KiB amdxdna_accel_driver-external-alloc: 0 ``` This is correct per the `drm_fdinfo_print_size` convention (it uses `drm_dr= iver.name` as the prefix), but the driver name `amdxdna_accel_driver` is un= usually long compared to other drivers. This is arguably an existing issue = with the driver name choice rather than a problem with this patch, but it's= worth noting for readability of fdinfo output. Tools like `gputop` may nee= d to handle this. 5. **Documentation example shows `0` without units for zero external alloc = (observation):** ``` amdxdna_accel_driver-external-alloc: 0 ``` This is actually correct behavior =E2=80=94 `drm_fdinfo_print_size` prints = `0` without units when the value is zero (the loop breaks at `u=3D0` when `= sz =3D=3D 0`). Just noting this is intentional and consistent with the impl= ementation. **Overall: The patch is reasonable and well-structured. The underflow conce= rn (issue 1) is the most important to address; the rest are minor.** --- Generated by Claude Code Patch Reviewer