public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH V1] accel/amdxdna: Expose per-client BO memory usage via fdinfo
@ 2026-04-09 15:22 Lizhi Hou
  2026-04-09 17:02 ` Mario Limonciello
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Lizhi Hou @ 2026-04-09 15:22 UTC (permalink / raw)
  To: ogabbay, quic_jhugo, dri-devel, mario.limonciello,
	maciej.falkowski
  Cc: Lizhi Hou, linux-kernel, max.zhen, sonal.santan

Implement amdxdna_show_fdinfo() to report per-client memory usage,
including below driver-specific memory stat:
  - heap allocation (amdxdna-heap-alloc)
  - internal BO allocation (amdxdna-internal-alloc)
  - external BO allocation (amdxdna-external-alloc)

Hook the implementation into the DRM fdinfo infrastructure via
drm_driver.show_fdinfo, while continuing to expose standard DRM
memory stat through drm_show_memory_stats().

This improves observability of per-process memory usage and aligns
with existing fdinfo reporting mechanisms used by other drivers.

Signed-off-by: Max Zhen <max.zhen@amd.com>
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 Documentation/accel/amdxdna/amdnpu.rst  | 25 +++++++++++++++++++
 Documentation/gpu/drm-usage-stats.rst   |  1 +
 drivers/accel/amdxdna/amdxdna_pci_drv.c | 32 ++++++++++++++++++++++++-
 3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/Documentation/accel/amdxdna/amdnpu.rst b/Documentation/accel/amdxdna/amdnpu.rst
index 42e54904f9a8..064973bf4893 100644
--- a/Documentation/accel/amdxdna/amdnpu.rst
+++ b/Documentation/accel/amdxdna/amdnpu.rst
@@ -270,6 +270,31 @@ MERT can report various kinds of telemetry information like the following:
 * Deep Sleep counter
 * etc.
 
+.. _amdxdna-usage-stats:
+
+Amdxdna DRM client usage stats implementation
+=============================================
+
+The amdxdna driver implements the DRM client usage stats specification as
+documented in :ref:`drm-client-usage-stats`.
+
+Example of the output showing the implemented key value pairs:
+
+::
+
+        pos:	0
+        flags:	0100002
+        mnt_id:	29
+        ino:	939
+        drm-driver:	amdxdna_accel_driver
+        drm-client-id:	3219
+        drm-pdev:	0000:c5:00.1
+        amdxdna_accel_driver-heap-alloc:	60 KiB
+        amdxdna_accel_driver-internal-alloc:	67588 KiB
+        amdxdna_accel_driver-external-alloc:	0
+        drm-total-memory:	67632 KiB
+        drm-shared-memory:	0
+
 
 References
 ==========
diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
index 63d6b2abe5ad..24d3012ca7a6 100644
--- a/Documentation/gpu/drm-usage-stats.rst
+++ b/Documentation/gpu/drm-usage-stats.rst
@@ -215,3 +215,4 @@ Driver specific implementations
 * :ref:`panfrost-usage-stats`
 * :ref:`panthor-usage-stats`
 * :ref:`xe-usage-stats`
+* :ref:`amdxdna-usage-stats`
diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c b/drivers/accel/amdxdna/amdxdna_pci_drv.c
index 09d7d88bb6f1..21eddfc538d0 100644
--- a/drivers/accel/amdxdna/amdxdna_pci_drv.c
+++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c
@@ -226,6 +226,35 @@ static const struct drm_ioctl_desc amdxdna_drm_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(AMDXDNA_SET_STATE, amdxdna_drm_set_state_ioctl, DRM_ROOT_ONLY),
 };
 
+static void amdxdna_show_fdinfo(struct drm_printer *p, struct drm_file *filp)
+{
+	struct amdxdna_client *client = filp->driver_priv;
+	size_t heap_usage, external_usage, internal_usage;
+	char *drv_name = filp->minor->dev->driver->name;
+
+	mutex_lock(&client->mm_lock);
+
+	heap_usage = client->heap_usage;
+	internal_usage = client->total_int_bo_usage;
+	external_usage = client->total_bo_usage - internal_usage;
+
+	mutex_unlock(&client->mm_lock);
+
+	/*
+	 * Note for driver specific BO memory usage stat.
+	 * Total memory alloc = amdxdna-internal-alloc + amdxdna-external-alloc
+	 */
+	drm_fdinfo_print_size(p, drv_name, "heap", "alloc", heap_usage);
+	drm_fdinfo_print_size(p, drv_name, "internal", "alloc", internal_usage);
+	drm_fdinfo_print_size(p, drv_name, "external", "alloc", external_usage);
+	/*
+	 * Note for DRM standard BO memory stat.
+	 * drm-total-memory counts both DEV BO and HEAP BO
+	 * drm-shared-memory counts BO imported
+	 */
+	drm_show_memory_stats(p, filp);
+}
+
 static const struct file_operations amdxdna_fops = {
 	.owner		= THIS_MODULE,
 	.open		= accel_open,
@@ -236,6 +265,7 @@ static const struct file_operations amdxdna_fops = {
 	.read		= drm_read,
 	.llseek		= noop_llseek,
 	.mmap		= drm_gem_mmap,
+	.show_fdinfo	= drm_show_fdinfo,
 	.fop_flags	= FOP_UNSIGNED_OFFSET,
 };
 
@@ -251,7 +281,7 @@ const struct drm_driver amdxdna_drm_drv = {
 	.postclose = amdxdna_drm_close,
 	.ioctls = amdxdna_drm_ioctls,
 	.num_ioctls = ARRAY_SIZE(amdxdna_drm_ioctls),
-
+	.show_fdinfo = amdxdna_show_fdinfo,
 	.gem_create_object = amdxdna_gem_create_shmem_object_cb,
 	.gem_prime_import = amdxdna_gem_prime_import,
 };
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH V1] accel/amdxdna: Expose per-client BO memory usage via fdinfo
  2026-04-09 15:22 [PATCH V1] accel/amdxdna: Expose per-client BO memory usage via fdinfo Lizhi Hou
@ 2026-04-09 17:02 ` Mario Limonciello
  2026-04-09 20:46   ` Lizhi Hou
  2026-04-12  1:06 ` Claude review: " Claude Code Review Bot
  2026-04-12  1:06 ` Claude Code Review Bot
  2 siblings, 1 reply; 5+ messages in thread
From: Mario Limonciello @ 2026-04-09 17:02 UTC (permalink / raw)
  To: Lizhi Hou, ogabbay, quic_jhugo, dri-devel, maciej.falkowski
  Cc: linux-kernel, max.zhen, sonal.santan



On 4/9/26 10:22, Lizhi Hou wrote:
> Implement amdxdna_show_fdinfo() to report per-client memory usage,
> including below driver-specific memory stat:
>    - heap allocation (amdxdna-heap-alloc)
>    - internal BO allocation (amdxdna-internal-alloc)
>    - external BO allocation (amdxdna-external-alloc)
> 
> Hook the implementation into the DRM fdinfo infrastructure via
> drm_driver.show_fdinfo, while continuing to expose standard DRM
> memory stat through drm_show_memory_stats().
> 
> This improves observability of per-process memory usage and aligns
> with existing fdinfo reporting mechanisms used by other drivers.
> 
> Signed-off-by: Max Zhen <max.zhen@amd.com>
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>

I think you should add a Suggested-by: tag for the person that suggested 
it (I forget who it was).

Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
>   Documentation/accel/amdxdna/amdnpu.rst  | 25 +++++++++++++++++++
>   Documentation/gpu/drm-usage-stats.rst   |  1 +
>   drivers/accel/amdxdna/amdxdna_pci_drv.c | 32 ++++++++++++++++++++++++-
>   3 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/accel/amdxdna/amdnpu.rst b/Documentation/accel/amdxdna/amdnpu.rst
> index 42e54904f9a8..064973bf4893 100644
> --- a/Documentation/accel/amdxdna/amdnpu.rst
> +++ b/Documentation/accel/amdxdna/amdnpu.rst
> @@ -270,6 +270,31 @@ MERT can report various kinds of telemetry information like the following:
>   * Deep Sleep counter
>   * etc.
>   
> +.. _amdxdna-usage-stats:
> +
> +Amdxdna DRM client usage stats implementation
> +=============================================
> +
> +The amdxdna driver implements the DRM client usage stats specification as
> +documented in :ref:`drm-client-usage-stats`.
> +
> +Example of the output showing the implemented key value pairs:
> +
> +::
> +
> +        pos:	0
> +        flags:	0100002
> +        mnt_id:	29
> +        ino:	939
> +        drm-driver:	amdxdna_accel_driver
> +        drm-client-id:	3219
> +        drm-pdev:	0000:c5:00.1
> +        amdxdna_accel_driver-heap-alloc:	60 KiB
> +        amdxdna_accel_driver-internal-alloc:	67588 KiB
> +        amdxdna_accel_driver-external-alloc:	0
> +        drm-total-memory:	67632 KiB
> +        drm-shared-memory:	0
> +
>   
>   References
>   ==========
> diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
> index 63d6b2abe5ad..24d3012ca7a6 100644
> --- a/Documentation/gpu/drm-usage-stats.rst
> +++ b/Documentation/gpu/drm-usage-stats.rst
> @@ -215,3 +215,4 @@ Driver specific implementations
>   * :ref:`panfrost-usage-stats`
>   * :ref:`panthor-usage-stats`
>   * :ref:`xe-usage-stats`
> +* :ref:`amdxdna-usage-stats`
> diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c b/drivers/accel/amdxdna/amdxdna_pci_drv.c
> index 09d7d88bb6f1..21eddfc538d0 100644
> --- a/drivers/accel/amdxdna/amdxdna_pci_drv.c
> +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c
> @@ -226,6 +226,35 @@ static const struct drm_ioctl_desc amdxdna_drm_ioctls[] = {
>   	DRM_IOCTL_DEF_DRV(AMDXDNA_SET_STATE, amdxdna_drm_set_state_ioctl, DRM_ROOT_ONLY),
>   };
>   
> +static void amdxdna_show_fdinfo(struct drm_printer *p, struct drm_file *filp)
> +{
> +	struct amdxdna_client *client = filp->driver_priv;
> +	size_t heap_usage, external_usage, internal_usage;
> +	char *drv_name = filp->minor->dev->driver->name;
> +
> +	mutex_lock(&client->mm_lock);
> +
> +	heap_usage = client->heap_usage;
> +	internal_usage = client->total_int_bo_usage;
> +	external_usage = client->total_bo_usage - internal_usage;
> +
> +	mutex_unlock(&client->mm_lock);
> +
> +	/*
> +	 * Note for driver specific BO memory usage stat.
> +	 * Total memory alloc = amdxdna-internal-alloc + amdxdna-external-alloc
> +	 */
> +	drm_fdinfo_print_size(p, drv_name, "heap", "alloc", heap_usage);
> +	drm_fdinfo_print_size(p, drv_name, "internal", "alloc", internal_usage);
> +	drm_fdinfo_print_size(p, drv_name, "external", "alloc", external_usage);
> +	/*
> +	 * Note for DRM standard BO memory stat.
> +	 * drm-total-memory counts both DEV BO and HEAP BO
> +	 * drm-shared-memory counts BO imported
> +	 */
> +	drm_show_memory_stats(p, filp);
> +}
> +
>   static const struct file_operations amdxdna_fops = {
>   	.owner		= THIS_MODULE,
>   	.open		= accel_open,
> @@ -236,6 +265,7 @@ static const struct file_operations amdxdna_fops = {
>   	.read		= drm_read,
>   	.llseek		= noop_llseek,
>   	.mmap		= drm_gem_mmap,
> +	.show_fdinfo	= drm_show_fdinfo,
>   	.fop_flags	= FOP_UNSIGNED_OFFSET,
>   };
>   
> @@ -251,7 +281,7 @@ const struct drm_driver amdxdna_drm_drv = {
>   	.postclose = amdxdna_drm_close,
>   	.ioctls = amdxdna_drm_ioctls,
>   	.num_ioctls = ARRAY_SIZE(amdxdna_drm_ioctls),
> -
> +	.show_fdinfo = amdxdna_show_fdinfo,
>   	.gem_create_object = amdxdna_gem_create_shmem_object_cb,
>   	.gem_prime_import = amdxdna_gem_prime_import,
>   };


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH V1] accel/amdxdna: Expose per-client BO memory usage via fdinfo
  2026-04-09 17:02 ` Mario Limonciello
@ 2026-04-09 20:46   ` Lizhi Hou
  0 siblings, 0 replies; 5+ messages in thread
From: Lizhi Hou @ 2026-04-09 20:46 UTC (permalink / raw)
  To: Mario Limonciello, ogabbay, quic_jhugo, dri-devel,
	maciej.falkowski
  Cc: linux-kernel, max.zhen, sonal.santan


On 4/9/26 10:02, Mario Limonciello wrote:
>
>
> On 4/9/26 10:22, Lizhi Hou wrote:
>> Implement amdxdna_show_fdinfo() to report per-client memory usage,
>> including below driver-specific memory stat:
>>    - heap allocation (amdxdna-heap-alloc)
>>    - internal BO allocation (amdxdna-internal-alloc)
>>    - external BO allocation (amdxdna-external-alloc)
>>
>> Hook the implementation into the DRM fdinfo infrastructure via
>> drm_driver.show_fdinfo, while continuing to expose standard DRM
>> memory stat through drm_show_memory_stats().
>>
>> This improves observability of per-process memory usage and aligns
>> with existing fdinfo reporting mechanisms used by other drivers.
>>
>> Signed-off-by: Max Zhen <max.zhen@amd.com>
>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
>
> I think you should add a Suggested-by: tag for the person that 
> suggested it (I forget who it was).
>
> Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
Added Suggested-by and applied to drm-misc-next
>> ---
>>   Documentation/accel/amdxdna/amdnpu.rst  | 25 +++++++++++++++++++
>>   Documentation/gpu/drm-usage-stats.rst   |  1 +
>>   drivers/accel/amdxdna/amdxdna_pci_drv.c | 32 ++++++++++++++++++++++++-
>>   3 files changed, 57 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/accel/amdxdna/amdnpu.rst 
>> b/Documentation/accel/amdxdna/amdnpu.rst
>> index 42e54904f9a8..064973bf4893 100644
>> --- a/Documentation/accel/amdxdna/amdnpu.rst
>> +++ b/Documentation/accel/amdxdna/amdnpu.rst
>> @@ -270,6 +270,31 @@ MERT can report various kinds of telemetry 
>> information like the following:
>>   * Deep Sleep counter
>>   * etc.
>>   +.. _amdxdna-usage-stats:
>> +
>> +Amdxdna DRM client usage stats implementation
>> +=============================================
>> +
>> +The amdxdna driver implements the DRM client usage stats 
>> specification as
>> +documented in :ref:`drm-client-usage-stats`.
>> +
>> +Example of the output showing the implemented key value pairs:
>> +
>> +::
>> +
>> +        pos:    0
>> +        flags:    0100002
>> +        mnt_id:    29
>> +        ino:    939
>> +        drm-driver:    amdxdna_accel_driver
>> +        drm-client-id:    3219
>> +        drm-pdev:    0000:c5:00.1
>> +        amdxdna_accel_driver-heap-alloc:    60 KiB
>> +        amdxdna_accel_driver-internal-alloc:    67588 KiB
>> +        amdxdna_accel_driver-external-alloc:    0
>> +        drm-total-memory:    67632 KiB
>> +        drm-shared-memory:    0
>> +
>>     References
>>   ==========
>> diff --git a/Documentation/gpu/drm-usage-stats.rst 
>> b/Documentation/gpu/drm-usage-stats.rst
>> index 63d6b2abe5ad..24d3012ca7a6 100644
>> --- a/Documentation/gpu/drm-usage-stats.rst
>> +++ b/Documentation/gpu/drm-usage-stats.rst
>> @@ -215,3 +215,4 @@ Driver specific implementations
>>   * :ref:`panfrost-usage-stats`
>>   * :ref:`panthor-usage-stats`
>>   * :ref:`xe-usage-stats`
>> +* :ref:`amdxdna-usage-stats`
>> diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c 
>> b/drivers/accel/amdxdna/amdxdna_pci_drv.c
>> index 09d7d88bb6f1..21eddfc538d0 100644
>> --- a/drivers/accel/amdxdna/amdxdna_pci_drv.c
>> +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c
>> @@ -226,6 +226,35 @@ static const struct drm_ioctl_desc 
>> amdxdna_drm_ioctls[] = {
>>       DRM_IOCTL_DEF_DRV(AMDXDNA_SET_STATE, 
>> amdxdna_drm_set_state_ioctl, DRM_ROOT_ONLY),
>>   };
>>   +static void amdxdna_show_fdinfo(struct drm_printer *p, struct 
>> drm_file *filp)
>> +{
>> +    struct amdxdna_client *client = filp->driver_priv;
>> +    size_t heap_usage, external_usage, internal_usage;
>> +    char *drv_name = filp->minor->dev->driver->name;
>> +
>> +    mutex_lock(&client->mm_lock);
>> +
>> +    heap_usage = client->heap_usage;
>> +    internal_usage = client->total_int_bo_usage;
>> +    external_usage = client->total_bo_usage - internal_usage;
>> +
>> +    mutex_unlock(&client->mm_lock);
>> +
>> +    /*
>> +     * Note for driver specific BO memory usage stat.
>> +     * Total memory alloc = amdxdna-internal-alloc + 
>> amdxdna-external-alloc
>> +     */
>> +    drm_fdinfo_print_size(p, drv_name, "heap", "alloc", heap_usage);
>> +    drm_fdinfo_print_size(p, drv_name, "internal", "alloc", 
>> internal_usage);
>> +    drm_fdinfo_print_size(p, drv_name, "external", "alloc", 
>> external_usage);
>> +    /*
>> +     * Note for DRM standard BO memory stat.
>> +     * drm-total-memory counts both DEV BO and HEAP BO
>> +     * drm-shared-memory counts BO imported
>> +     */
>> +    drm_show_memory_stats(p, filp);
>> +}
>> +
>>   static const struct file_operations amdxdna_fops = {
>>       .owner        = THIS_MODULE,
>>       .open        = accel_open,
>> @@ -236,6 +265,7 @@ static const struct file_operations amdxdna_fops = {
>>       .read        = drm_read,
>>       .llseek        = noop_llseek,
>>       .mmap        = drm_gem_mmap,
>> +    .show_fdinfo    = drm_show_fdinfo,
>>       .fop_flags    = FOP_UNSIGNED_OFFSET,
>>   };
>>   @@ -251,7 +281,7 @@ const struct drm_driver amdxdna_drm_drv = {
>>       .postclose = amdxdna_drm_close,
>>       .ioctls = amdxdna_drm_ioctls,
>>       .num_ioctls = ARRAY_SIZE(amdxdna_drm_ioctls),
>> -
>> +    .show_fdinfo = amdxdna_show_fdinfo,
>>       .gem_create_object = amdxdna_gem_create_shmem_object_cb,
>>       .gem_prime_import = amdxdna_gem_prime_import,
>>   };
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Claude review: accel/amdxdna: Expose per-client BO memory usage via fdinfo
  2026-04-09 15:22 [PATCH V1] accel/amdxdna: Expose per-client BO memory usage via fdinfo Lizhi Hou
  2026-04-09 17:02 ` Mario Limonciello
  2026-04-12  1:06 ` Claude review: " Claude Code Review Bot
@ 2026-04-12  1:06 ` Claude Code Review Bot
  2 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-04-12  1:06 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: accel/amdxdna: Expose per-client BO memory usage via fdinfo
Author: Lizhi Hou <lizhi.hou@amd.com>
Patches: 3
Reviewed: 2026-04-12T11:06:37.322159

---

This is a single patch adding per-client BO memory usage reporting via the fdinfo interface to the amdxdna accel driver. The approach is sound and follows established patterns used by other drivers (e.g., panthor). The patch hooks `drm_show_fdinfo` into file_operations and provides a custom `show_fdinfo` callback in `drm_driver` that reports driver-specific memory categories before calling `drm_show_memory_stats()` for the standard DRM memory stats.

The implementation is straightforward and mostly correct. There are a few issues worth addressing — one potential unsigned underflow, a type width concern, and minor style/doc nits.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Claude review: accel/amdxdna: Expose per-client BO memory usage via fdinfo
  2026-04-09 15:22 [PATCH V1] accel/amdxdna: Expose per-client BO memory usage via fdinfo Lizhi Hou
  2026-04-09 17:02 ` Mario Limonciello
@ 2026-04-12  1:06 ` Claude Code Review Bot
  2026-04-12  1:06 ` Claude Code Review Bot
  2 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-04-12  1:06 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Positive aspects:**
- Follows the same pattern as panthor's `panthor_show_internal_memory_stats` (even uses the same `char *drv_name = file->minor->dev->driver->name` idiom).
- Correctly locks `mm_lock` while reading the usage counters and unlocks before printing.
- Documentation addition references the standard DRM client usage stats spec.

**Issues:**

1. **Potential unsigned underflow in external_usage calculation (medium severity):**

```c
external_usage = client->total_bo_usage - internal_usage;
```

Both `total_bo_usage` and `internal_usage` are `size_t` (unsigned). If there's ever a bug or race where `total_int_bo_usage > total_bo_usage`, this silently 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 = 0;
else
    external_usage = 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_usage`, `internal_usage`, and `external_usage` are all `size_t`. On 32-bit architectures `size_t` is 32-bit while `u64` is 64-bit. This is not incorrect (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 (NPU hardware probably doesn't target 32-bit), but explicitly casting to `u64` or declaring the local variables as `u64` would be cleaner. Note that panthor avoids this by passing through `struct drm_memory_stats` which uses `u64` fields.

3. **Blank line removal in `drm_driver` struct (nit):**

```c
 	.num_ioctls = ARRAY_SIZE(amdxdna_drm_ioctls),
-
+	.show_fdinfo = amdxdna_show_fdinfo,
 	.gem_create_object = 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_fdinfo` either before the blank line (with the other callbacks like `.open`/`.postclose`) or after a new blank line:

```c
	.num_ioctls = ARRAY_SIZE(amdxdna_drm_ioctls),
	.show_fdinfo = amdxdna_show_fdinfo,

	.gem_create_object = amdxdna_gem_create_shmem_object_cb,
```

4. **Documentation: driver name prefix is rather long and verbose (observation):**

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_driver.name` as the prefix), but the driver name `amdxdna_accel_driver` is unusually 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 need 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 — `drm_fdinfo_print_size` prints `0` without units when the value is zero (the loop breaks at `u=0` when `sz == 0`). Just noting this is intentional and consistent with the implementation.

**Overall: The patch is reasonable and well-structured. The underflow concern (issue 1) is the most important to address; the rest are minor.**

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-04-12  1:06 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-09 15:22 [PATCH V1] accel/amdxdna: Expose per-client BO memory usage via fdinfo Lizhi Hou
2026-04-09 17:02 ` Mario Limonciello
2026-04-09 20:46   ` Lizhi Hou
2026-04-12  1:06 ` Claude review: " Claude Code Review Bot
2026-04-12  1:06 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox