[PATCH v2 0/2] drm/imagination: GPU_ID-related cleanups

[PATCH v2 0/2] drm/imagination: GPU_ID-related cleanups

[Matt Coster]

Just a couple quick QoL improvements around GPU_IDs.

The first patch touches the UAPI, but is not intended to introduce any
functional changes; it simply codifies the existing documented layout of
packed BVNC fields as a series of GENMASKed macros.

Signed-off-by: Matt Coster <matt.coster@imgtec.com>
---
Changes in v2:
- Add _CONST variant of PVR_PACKED_BVNC() for use in case statements.
  This fixes the build issue reported by the test bot[1][2] (P1)
- Simplify the change made to pvr_fw_validate() (P2)
- Link to v1: https://patch.msgid.link/20260206-bvnc-cleanup-v1-0-f3c818541fbe@imgtec.com

[1]: https://lore.kernel.org/r/202602070204.PjaKNSpZ-lkp@intel.com/
[2]: https://lore.kernel.org/r/202602070500.zo0DrH6U-lkp@intel.com/

---
Matt Coster (2):
      drm/imagination: Define packed BVNCs in the uapi
      drm/imagination: Add PVR_GPU_ID_FMT to format pvr_gpu_id

 drivers/gpu/drm/imagination/pvr_device.c | 10 +++----
 drivers/gpu/drm/imagination/pvr_device.h | 49 +++++++++++++++-----------------
 drivers/gpu/drm/imagination/pvr_fw.c     |  9 ++----
 include/uapi/drm/pvr_drm.h               | 14 ++++-----
 4 files changed, 38 insertions(+), 44 deletions(-)
---
base-commit: fbef867cf6614178bc6afd15ee15f85cddf19eab
change-id: 20260206-bvnc-cleanup-446e70666747

[PATCH v2 1/2] drm/imagination: Define packed BVNCs in the uapi

[Matt Coster]

Using the uapi-safe __GENMASK_ULL(), we can stably define the layout of
64-bit packed BVNCs. These defs replace the replicated doc comment that
appears all over the place.

Signed-off-by: Matt Coster <matt.coster@imgtec.com>

---
Changes in v2:
- Add _CONST variant of PVR_PACKED_BVNC() for use in case statements.
  This fixes the build issue reported by the test bot[1][2]
- Link to v1: https://patch.msgid.link/20260206-bvnc-cleanup-v1-1-f3c818541fbe@imgtec.com

[1]: https://lore.kernel.org/r/202602070204.PjaKNSpZ-lkp@intel.com/
[2]: https://lore.kernel.org/r/202602070500.zo0DrH6U-lkp@intel.com/
---
 drivers/gpu/drm/imagination/pvr_device.c |  6 ++---
 drivers/gpu/drm/imagination/pvr_device.h | 43 +++++++++++++-------------------
 include/uapi/drm/pvr_drm.h               | 14 +++++------
 3 files changed, 27 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
index f58bb66a6327..9b26585a42bf 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -547,11 +547,11 @@ static enum pvr_gpu_support_level
 pvr_gpu_support_level(const struct pvr_gpu_id *gpu_id)
 {
 	switch (pvr_gpu_id_to_packed_bvnc(gpu_id)) {
-	case PVR_PACKED_BVNC(33, 15, 11, 3):
-	case PVR_PACKED_BVNC(36, 53, 104, 796):
+	case PVR_PACKED_BVNC_CONST(33, 15, 11, 3):
+	case PVR_PACKED_BVNC_CONST(36, 53, 104, 796):
 		return PVR_GPU_SUPPORTED;
 
-	case PVR_PACKED_BVNC(36, 52, 104, 182):
+	case PVR_PACKED_BVNC_CONST(36, 52, 104, 182):
 		return PVR_GPU_EXPERIMENTAL;
 
 	default:
diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
index d51c57cf9332..55d8ff11d507 100644
--- a/drivers/gpu/drm/imagination/pvr_device.h
+++ b/drivers/gpu/drm/imagination/pvr_device.h
@@ -14,7 +14,7 @@
 #include <drm/drm_file.h>
 #include <drm/drm_mm.h>
 
-#include <linux/bits.h>
+#include <linux/bitfield.h>
 #include <linux/compiler_attributes.h>
 #include <linux/compiler_types.h>
 #include <linux/device.h>
@@ -470,6 +470,12 @@ struct pvr_file {
 
 #define to_pvr_file(file) ((file)->driver_priv)
 
+#define __PVR_PACKED_BVNC(prep, b, v, n, c) \
+	(prep(DRM_PVR_BVNC_B, b) | \
+	 prep(DRM_PVR_BVNC_V, v) | \
+	 prep(DRM_PVR_BVNC_N, n) | \
+	 prep(DRM_PVR_BVNC_C, c))
+
 /**
  * PVR_PACKED_BVNC() - Packs B, V, N and C values into a 64-bit unsigned integer
  * @b: Branch ID.
@@ -477,39 +483,24 @@ struct pvr_file {
  * @n: Number of scalable units.
  * @c: Config ID.
  *
- * The packed layout is as follows:
- *
- *    +--------+--------+--------+-------+
- *    | 63..48 | 47..32 | 31..16 | 15..0 |
- *    +========+========+========+=======+
- *    | B      | V      | N      | C     |
- *    +--------+--------+--------+-------+
+ * The packed layout follows the bitfield defined by the DRM_PVR_BVNC_* macros.
  *
  * pvr_gpu_id_to_packed_bvnc() should be used instead of this macro when a
  * &struct pvr_gpu_id is available in order to ensure proper type checking.
  *
  * Return: Packed BVNC.
  */
-/* clang-format off */
-#define PVR_PACKED_BVNC(b, v, n, c) \
-	((((u64)(b) & GENMASK_ULL(15, 0)) << 48) | \
-	 (((u64)(v) & GENMASK_ULL(15, 0)) << 32) | \
-	 (((u64)(n) & GENMASK_ULL(15, 0)) << 16) | \
-	 (((u64)(c) & GENMASK_ULL(15, 0)) <<  0))
-/* clang-format on */
+#define PVR_PACKED_BVNC(b, v, n, c) __PVR_PACKED_BVNC(FIELD_PREP, b, v, n, c)
+
+/** PVR_PACKED_BVNC_CONST() - Compile-time equivalent of PVR_PACKED_BVNC(). */
+#define PVR_PACKED_BVNC_CONST(b, v, n, c) __PVR_PACKED_BVNC(FIELD_PREP_CONST, b, v, n, c)
 
 /**
  * pvr_gpu_id_to_packed_bvnc() - Packs B, V, N and C values into a 64-bit
  * unsigned integer
  * @gpu_id: GPU ID.
  *
- * The packed layout is as follows:
- *
- *    +--------+--------+--------+-------+
- *    | 63..48 | 47..32 | 31..16 | 15..0 |
- *    +========+========+========+=======+
- *    | B      | V      | N      | C     |
- *    +--------+--------+--------+-------+
+ * The packed layout follows the bitfield defined by the DRM_PVR_BVNC_* macros.
  *
  * This should be used in preference to PVR_PACKED_BVNC() when a &struct
  * pvr_gpu_id is available in order to ensure proper type checking.
@@ -525,10 +516,10 @@ pvr_gpu_id_to_packed_bvnc(const struct pvr_gpu_id *gpu_id)
 static __always_inline void
 packed_bvnc_to_pvr_gpu_id(u64 bvnc, struct pvr_gpu_id *gpu_id)
 {
-	gpu_id->b = (bvnc & GENMASK_ULL(63, 48)) >> 48;
-	gpu_id->v = (bvnc & GENMASK_ULL(47, 32)) >> 32;
-	gpu_id->n = (bvnc & GENMASK_ULL(31, 16)) >> 16;
-	gpu_id->c = bvnc & GENMASK_ULL(15, 0);
+	gpu_id->b = FIELD_GET(DRM_PVR_BVNC_B, bvnc);
+	gpu_id->v = FIELD_GET(DRM_PVR_BVNC_V, bvnc);
+	gpu_id->n = FIELD_GET(DRM_PVR_BVNC_N, bvnc);
+	gpu_id->c = FIELD_GET(DRM_PVR_BVNC_C, bvnc);
 }
 
 int pvr_device_init(struct pvr_device *pvr_dev);
diff --git a/include/uapi/drm/pvr_drm.h b/include/uapi/drm/pvr_drm.h
index ccf6c2112468..72f3f90560cf 100644
--- a/include/uapi/drm/pvr_drm.h
+++ b/include/uapi/drm/pvr_drm.h
@@ -6,6 +6,7 @@
 
 #include "drm.h"
 
+#include <linux/bits.h>
 #include <linux/const.h>
 #include <linux/types.h>
 
@@ -113,6 +114,11 @@ struct drm_pvr_obj_array {
  * DOC: PowerVR IOCTL DEV_QUERY interface
  */
 
+#define DRM_PVR_BVNC_B __GENMASK_ULL(63, 48)
+#define DRM_PVR_BVNC_V __GENMASK_ULL(47, 32)
+#define DRM_PVR_BVNC_N __GENMASK_ULL(31, 16)
+#define DRM_PVR_BVNC_C __GENMASK_ULL(15, 0)
+
 /**
  * struct drm_pvr_dev_query_gpu_info - Container used to fetch information about
  * the graphics processor.
@@ -125,13 +131,7 @@ struct drm_pvr_dev_query_gpu_info {
 	 * @gpu_id: GPU identifier.
 	 *
 	 * For all currently supported GPUs this is the BVNC encoded as a 64-bit
-	 * value as follows:
-	 *
-	 *    +--------+--------+--------+-------+
-	 *    | 63..48 | 47..32 | 31..16 | 15..0 |
-	 *    +========+========+========+=======+
-	 *    | B      | V      | N      | C     |
-	 *    +--------+--------+--------+-------+
+	 * value using the DRM_PVR_BVNC_* bitmasks.
 	 */
 	__u64 gpu_id;
 

-- 
2.53.0

[PATCH v2 2/2] drm/imagination: Add PVR_GPU_ID_FMT to format pvr_gpu_id

[Matt Coster]

There are currently two different combinations of format specifiers used to
print a struct pvr_gpu_id, one using %i and one using %d. Both of these
are technically incorrect since the components are stored as u16.

Introduce macros to simplify and correct the formatting of these values:
 - PVR_GPU_ID_FMT: A pre-constructed format string fragment, in the style
   of PRIu32.
 - PVR_GPU_ID_FMT_ARGS(): Accepts a &struct pvr_gpu_id and expands the
   fields into appropriate format arguments to be used with PVR_GPU_ID_FMT.
 - PVR_GPU_ID_FMT_ARGS_PACKED(): Accepts a packed GPU ID as a u64 and
   extracts the components directly into appropriate format arguments to
   be used with PVR_GPU_ID_FMT.

Signed-off-by: Matt Coster <matt.coster@imgtec.com>

---
Changes in v2:
- Simplify the change made to pvr_fw_validate()
- Link to v1: https://patch.msgid.link/20260206-bvnc-cleanup-v1-2-f3c818541fbe@imgtec.com
---
 drivers/gpu/drm/imagination/pvr_device.c | 4 ++--
 drivers/gpu/drm/imagination/pvr_device.h | 6 ++++++
 drivers/gpu/drm/imagination/pvr_fw.c     | 9 +++------
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
index 9b26585a42bf..263d1badacf3 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -362,8 +362,8 @@ pvr_build_firmware_filename(struct pvr_device *pvr_dev, const char *base,
 {
 	struct pvr_gpu_id *gpu_id = &pvr_dev->gpu_id;
 
-	return kasprintf(GFP_KERNEL, "%s_%d.%d.%d.%d_v%d.fw", base, gpu_id->b,
-			 gpu_id->v, gpu_id->n, gpu_id->c, major);
+	return kasprintf(GFP_KERNEL, "%s_" PVR_GPU_ID_FMT "_v%d.fw", base,
+			 PVR_GPU_ID_FMT_ARGS(gpu_id), major);
 }
 
 static void
diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
index 55d8ff11d507..54e3e947e60e 100644
--- a/drivers/gpu/drm/imagination/pvr_device.h
+++ b/drivers/gpu/drm/imagination/pvr_device.h
@@ -53,6 +53,12 @@ struct pvr_gpu_id {
 	u16 b, v, n, c;
 };
 
+#define PVR_GPU_ID_FMT "%u.%u.%u.%u"
+#define PVR_GPU_ID_FMT_ARGS(gpu_id) (gpu_id)->b, (gpu_id)->v, (gpu_id)->n, (gpu_id)->c
+#define PVR_GPU_ID_FMT_ARGS_PACKED(gpu_id) \
+	(u32)FIELD_GET(DRM_PVR_BVNC_B, gpu_id), (u32)FIELD_GET(DRM_PVR_BVNC_V, gpu_id), \
+	(u32)FIELD_GET(DRM_PVR_BVNC_N, gpu_id), (u32)FIELD_GET(DRM_PVR_BVNC_C, gpu_id)
+
 /**
  * struct pvr_fw_version - Firmware version information
  * @major: Major version number.
diff --git a/drivers/gpu/drm/imagination/pvr_fw.c b/drivers/gpu/drm/imagination/pvr_fw.c
index 288516dc2560..0b1c2a0d950c 100644
--- a/drivers/gpu/drm/imagination/pvr_fw.c
+++ b/drivers/gpu/drm/imagination/pvr_fw.c
@@ -126,12 +126,9 @@ pvr_fw_validate(struct pvr_device *pvr_dev)
 	}
 
 	if (pvr_gpu_id_to_packed_bvnc(&pvr_dev->gpu_id) != header->bvnc) {
-		struct pvr_gpu_id fw_gpu_id;
-
-		packed_bvnc_to_pvr_gpu_id(header->bvnc, &fw_gpu_id);
-		drm_err(drm_dev, "FW built for incorrect GPU ID %i.%i.%i.%i (expected %i.%i.%i.%i)\n",
-			fw_gpu_id.b, fw_gpu_id.v, fw_gpu_id.n, fw_gpu_id.c,
-			pvr_dev->gpu_id.b, pvr_dev->gpu_id.v, pvr_dev->gpu_id.n, pvr_dev->gpu_id.c);
+		drm_err(drm_dev, "FW built for incorrect GPU ID " PVR_GPU_ID_FMT " (expected " PVR_GPU_ID_FMT ")\n",
+			PVR_GPU_ID_FMT_ARGS_PACKED(header->bvnc),
+			PVR_GPU_ID_FMT_ARGS(&pvr_dev->gpu_id));
 		return -EINVAL;
 	}
 

-- 
2.53.0

Claude review: drm/imagination: GPU_ID-related cleanups

[Claude Code Review Bot]

Overall Series Review

Subject: drm/imagination: GPU_ID-related cleanups
Author: Matt Coster <matt.coster@imgtec.com>
Patches: 3
Reviewed: 2026-03-22T03:38:20.530354

---

This is a clean, well-structured 2-patch series that makes quality-of-life improvements to the PowerVR GPU ID (BVNC) handling in the Imagination DRM driver. 

**Patch 1** moves the packed BVNC field layout definitions into the UAPI header as `DRM_PVR_BVNC_*` macros using `__GENMASK_ULL()`, replacing duplicated ASCII-art layout comments scattered throughout the code. It also switches the internal packing/unpacking to use `FIELD_PREP`/`FIELD_GET`, and adds a `FIELD_PREP_CONST` variant to fix compile-time constant issues in `case` statements (addressing v1 build failures).

**Patch 2** introduces `PVR_GPU_ID_FMT` / `PVR_GPU_ID_FMT_ARGS()` / `PVR_GPU_ID_FMT_ARGS_PACKED()` macros for consistent printf formatting of GPU IDs, fixing a minor inconsistency where `%i` and `%d` were used for `u16` fields (should be `%u`).

The series is well-motivated, addresses real code duplication and correctness issues, and has no functional changes beyond the format specifier fix. The UAPI change is purely additive (new macros, no ABI breakage). **Recommend merge.**

---
Generated by Claude Code Patch Reviewer

Claude review: drm/imagination: Define packed BVNCs in the uapi

[Claude Code Review Bot]

Patch Review

**Overall:** Good cleanup. The approach of defining the bit layout once in the UAPI and referencing it everywhere is the right pattern.

**UAPI additions look correct:**
```c
#define DRM_PVR_BVNC_B __GENMASK_ULL(63, 48)
#define DRM_PVR_BVNC_V __GENMASK_ULL(47, 32)
#define DRM_PVR_BVNC_N __GENMASK_ULL(31, 16)
#define DRM_PVR_BVNC_C __GENMASK_ULL(15, 0)
```
These correctly match the previously documented layout. Using `__GENMASK_ULL` (the double-underscore UAPI-safe variant) is correct for a UAPI header, and the new `#include <linux/bits.h>` provides it.

**The `__PVR_PACKED_BVNC` helper pattern is clever:**
```c
#define __PVR_PACKED_BVNC(prep, b, v, n, c) \
	(prep(DRM_PVR_BVNC_B, b) | \
	 prep(DRM_PVR_BVNC_V, v) | \
	 prep(DRM_PVR_BVNC_N, n) | \
	 prep(DRM_PVR_BVNC_C, c))
```
Parameterizing on the prep function to support both `FIELD_PREP` (runtime) and `FIELD_PREP_CONST` (compile-time) avoids code duplication while addressing the v1 build issue where `FIELD_PREP` couldn't be used in `case` labels.

**Include change is correct:** Replacing `<linux/bits.h>` with `<linux/bitfield.h>` in `pvr_device.h` is appropriate since `bitfield.h` pulls in `bits.h` transitively and `FIELD_PREP`/`FIELD_GET` are now used.

**`packed_bvnc_to_pvr_gpu_id()` conversion looks correct:**
```c
gpu_id->b = FIELD_GET(DRM_PVR_BVNC_B, bvnc);
```
This is equivalent to the previous `(bvnc & GENMASK_ULL(63, 48)) >> 48` and is more readable.

**Minor nit:** The `__PVR_PACKED_BVNC` macro uses a leading double-underscore name. While this is common for internal helper macros in the kernel, it's technically in the compiler-reserved namespace. This is a style nit, not a blocking issue — the kernel uses this convention extensively.

**No issues found.** Reviewed-by worthy.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/imagination: Add PVR_GPU_ID_FMT to format pvr_gpu_id

[Claude Code Review Bot]

Patch Review

**Overall:** Straightforward and correct. Fixes a real (if minor) bug with format specifiers.

**Format macros are well-designed:**
```c
#define PVR_GPU_ID_FMT "%u.%u.%u.%u"
#define PVR_GPU_ID_FMT_ARGS(gpu_id) (gpu_id)->b, (gpu_id)->v, (gpu_id)->n, (gpu_id)->c
#define PVR_GPU_ID_FMT_ARGS_PACKED(gpu_id) \
	(u32)FIELD_GET(DRM_PVR_BVNC_B, gpu_id), (u32)FIELD_GET(DRM_PVR_BVNC_V, gpu_id), \
	(u32)FIELD_GET(DRM_PVR_BVNC_N, gpu_id), (u32)FIELD_GET(DRM_PVR_BVNC_C, gpu_id)
```
Using `%u` is correct since the struct fields are `u16`. The `(u32)` casts in the packed variant are appropriate because `FIELD_GET` returns `u64` and `%u` expects `unsigned int`.

**The pvr_fw.c simplification is a nice win:**
```c
drm_err(drm_dev, "FW built for incorrect GPU ID " PVR_GPU_ID_FMT " (expected " PVR_GPU_ID_FMT ")\n",
        PVR_GPU_ID_FMT_ARGS_PACKED(header->bvnc),
        PVR_GPU_ID_FMT_ARGS(&pvr_dev->gpu_id));
```
This eliminates the temporary `fw_gpu_id` variable and the intermediate `packed_bvnc_to_pvr_gpu_id()` call in the error path, which is cleaner.

**Minor observation:** `PVR_GPU_ID_FMT_ARGS` evaluates `gpu_id` four times. If someone passed an expression with side effects, it would be evaluated four times. This is a common pattern for such macros in the kernel and all current callers pass simple pointer expressions, so this is fine in practice.

**No issues found.** Reviewed-by worthy.

---
Generated by Claude Code Patch Reviewer