[PATCH 0/4] drm/imagination: Multiple enhancement

[PATCH 0/4] drm/imagination: Multiple enhancement

[Brajesh Gupta]

General enchancements to improve driver reliability.

Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
---
Brajesh Gupta (4):
      drm/imagination: Populate FW common context ID before passing to the FW
      drm/imagination: Don't timeout job if its fence has been signaled
      drm/imagination: Rename FW booted to FW initialised
      drm/imagination: Access FW initialised state with READ/WRITE_ONCE

 drivers/gpu/drm/imagination/pvr_context.c | 30 ++++++++++++++++--------------
 drivers/gpu/drm/imagination/pvr_device.c  |  2 +-
 drivers/gpu/drm/imagination/pvr_fw.c      |  4 ++--
 drivers/gpu/drm/imagination/pvr_fw.h      |  7 +++++--
 drivers/gpu/drm/imagination/pvr_mmu.c     |  4 ++--
 drivers/gpu/drm/imagination/pvr_power.c   | 14 +++++++-------
 drivers/gpu/drm/imagination/pvr_queue.c   |  7 ++++++-
 7 files changed, 39 insertions(+), 29 deletions(-)
---
base-commit: ddfbe68a14260aabce25322fac81f2d02501369a
change-id: 20260512-b4-context_reset-f6909e31712d

Best regards,
-- 
Brajesh Gupta <brajesh.gupta@imgtec.com>

[PATCH 1/4] drm/imagination: Populate FW common context ID before passing to the FW

[Brajesh Gupta]

Fix context ID value for the FW common context by moving the context
allocation earlier.
Also an old incorrect comment was removed.

Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
---
 drivers/gpu/drm/imagination/pvr_context.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_context.c b/drivers/gpu/drm/imagination/pvr_context.c
index 8de70c30b9de..eba4694400b5 100644
--- a/drivers/gpu/drm/imagination/pvr_context.c
+++ b/drivers/gpu/drm/imagination/pvr_context.c
@@ -318,10 +318,14 @@ int pvr_context_create(struct pvr_file *pvr_file, struct drm_pvr_ioctl_create_co
 		goto err_put_vm;
 	}
 
-	err = pvr_context_create_queues(ctx, args, ctx->data);
+	err = xa_alloc(&pvr_dev->ctx_ids, &ctx->ctx_id, ctx, xa_limit_32b, GFP_KERNEL);
 	if (err)
 		goto err_free_ctx_data;
 
+	err = pvr_context_create_queues(ctx, args, ctx->data);
+	if (err)
+		goto err_free_ctx_id;
+
 	err = init_fw_objs(ctx, args, ctx->data);
 	if (err)
 		goto err_destroy_queues;
@@ -329,23 +333,12 @@ int pvr_context_create(struct pvr_file *pvr_file, struct drm_pvr_ioctl_create_co
 	err = pvr_fw_object_create(pvr_dev, ctx_size, PVR_BO_FW_FLAGS_DEVICE_UNCACHED,
 				   ctx_fw_data_init, ctx, &ctx->fw_obj);
 	if (err)
-		goto err_free_ctx_data;
+		goto err_destroy_queues;
 
-	err = xa_alloc(&pvr_dev->ctx_ids, &ctx->ctx_id, ctx, xa_limit_32b, GFP_KERNEL);
+	err = xa_alloc(&pvr_file->ctx_handles, &args->handle, ctx, xa_limit_32b, GFP_KERNEL);
 	if (err)
 		goto err_destroy_fw_obj;
 
-	err = xa_alloc(&pvr_file->ctx_handles, &args->handle, ctx, xa_limit_32b, GFP_KERNEL);
-	if (err) {
-		/*
-		 * It's possible that another thread could have taken a reference on the context at
-		 * this point as it is in the ctx_ids xarray. Therefore instead of directly
-		 * destroying the context, drop a reference instead.
-		 */
-		pvr_context_put(ctx);
-		return err;
-	}
-
 	spin_lock(&pvr_dev->ctx_list_lock);
 	list_add_tail(&ctx->file_link, &pvr_file->contexts);
 	spin_unlock(&pvr_dev->ctx_list_lock);
@@ -358,6 +351,15 @@ int pvr_context_create(struct pvr_file *pvr_file, struct drm_pvr_ioctl_create_co
 err_destroy_queues:
 	pvr_context_destroy_queues(ctx);
 
+err_free_ctx_id:
+	/*
+	 * Ctx_id is not exposed to userspace and not visible yet within
+	 * the kernel/FW, plus a matching context handle (exposed to userspace)
+	 * hasn't been allocated yet, so it is safe to remove ctx_id
+	 * from the ctx_ids xarray.
+	 */
+	xa_erase(&pvr_dev->ctx_ids, ctx->ctx_id);
+
 err_free_ctx_data:
 	kfree(ctx->data);
 

-- 
2.43.0

[PATCH 2/4] drm/imagination: Don't timeout job if its fence has been signaled

[Brajesh Gupta]

Check for job's fence in timedout handler and report NO HANG if its
is signaled

Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
---
 drivers/gpu/drm/imagination/pvr_queue.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imagination/pvr_queue.c b/drivers/gpu/drm/imagination/pvr_queue.c
index d10a13173f0f..073478919b22 100644
--- a/drivers/gpu/drm/imagination/pvr_queue.c
+++ b/drivers/gpu/drm/imagination/pvr_queue.c
@@ -851,7 +851,8 @@ static void pvr_queue_start(struct pvr_queue *queue)
  * the scheduler, and re-assign parent fences in the middle.
  *
  * Return:
- *  * DRM_GPU_SCHED_STAT_RESET.
+ *  * Fence signaled : DRM_GPU_SCHED_STAT_NO_HANG
+ *  * otherwise      : DRM_GPU_SCHED_STAT_RESET
  */
 static enum drm_gpu_sched_stat
 pvr_queue_timedout_job(struct drm_sched_job *s_job)
@@ -862,6 +863,10 @@ pvr_queue_timedout_job(struct drm_sched_job *s_job)
 	struct pvr_job *job;
 	u32 job_count = 0;
 
+	/* If fence is already signaled then return no hang */
+	if (dma_fence_is_signaled(s_job->s_fence->parent))
+		return DRM_GPU_SCHED_STAT_NO_HANG;
+
 	dev_err(sched->dev, "Job timeout\n");
 
 	/* Before we stop the scheduler, make sure the queue is out of any list, so

-- 
2.43.0

[PATCH 3/4] drm/imagination: Rename FW booted to FW initialised

[Brajesh Gupta]

Update name to FW initialised from booted to better represent FW state.

Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
---
 drivers/gpu/drm/imagination/pvr_device.c |  2 +-
 drivers/gpu/drm/imagination/pvr_fw.c     |  4 ++--
 drivers/gpu/drm/imagination/pvr_fw.h     |  7 +++++--
 drivers/gpu/drm/imagination/pvr_mmu.c    |  4 ++--
 drivers/gpu/drm/imagination/pvr_power.c  | 14 +++++++-------
 5 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
index dbb6f5a8ded1..49696101b547 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -213,7 +213,7 @@ static irqreturn_t pvr_device_irq_thread_handler(int irq, void *data)
 	while (pvr_fw_irq_pending(pvr_dev)) {
 		pvr_fw_irq_clear(pvr_dev);
 
-		if (pvr_dev->fw_dev.booted) {
+		if (pvr_dev->fw_dev.initialised) {
 			pvr_fwccb_process(pvr_dev);
 			pvr_kccb_wake_up_waiters(pvr_dev);
 			pvr_device_process_active_queues(pvr_dev);
diff --git a/drivers/gpu/drm/imagination/pvr_fw.c b/drivers/gpu/drm/imagination/pvr_fw.c
index 288516dc2560..b8ad3f1d222c 100644
--- a/drivers/gpu/drm/imagination/pvr_fw.c
+++ b/drivers/gpu/drm/imagination/pvr_fw.c
@@ -1004,7 +1004,7 @@ pvr_fw_init(struct pvr_device *pvr_dev)
 		goto err_fw_stop;
 	}
 
-	fw_dev->booted = true;
+	fw_dev->initialised = true;
 
 	return 0;
 
@@ -1044,7 +1044,7 @@ pvr_fw_fini(struct pvr_device *pvr_dev)
 {
 	struct pvr_fw_device *fw_dev = &pvr_dev->fw_dev;
 
-	fw_dev->booted = false;
+	fw_dev->initialised = false;
 
 	pvr_fw_destroy_structures(pvr_dev);
 	pvr_fw_object_unmap_and_destroy(pvr_dev->kccb.rtn_obj);
diff --git a/drivers/gpu/drm/imagination/pvr_fw.h b/drivers/gpu/drm/imagination/pvr_fw.h
index 1404dd492d7c..3390c84e4fd3 100644
--- a/drivers/gpu/drm/imagination/pvr_fw.h
+++ b/drivers/gpu/drm/imagination/pvr_fw.h
@@ -295,8 +295,11 @@ struct pvr_fw_device {
 	/** @mem: Structure containing objects representing firmware memory allocations. */
 	struct pvr_fw_mem mem;
 
-	/** @booted: %true if the firmware has been booted, %false otherwise. */
-	bool booted;
+	/**
+	 * @initialised: %true if the firmware has been successfully initialised,
+	 *               %false otherwise.
+	 */
+	bool initialised;
 
 	/**
 	 * @processor_type: FW processor type for this device. Must be one of
diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c b/drivers/gpu/drm/imagination/pvr_mmu.c
index 2e4da5b2c499..e9fefcc4e234 100644
--- a/drivers/gpu/drm/imagination/pvr_mmu.c
+++ b/drivers/gpu/drm/imagination/pvr_mmu.c
@@ -133,8 +133,8 @@ int pvr_mmu_flush_exec(struct pvr_device *pvr_dev, bool wait)
 	if (!drm_dev_enter(from_pvr_device(pvr_dev), &idx))
 		return -EIO;
 
-	/* Can't flush MMU if the firmware hasn't booted yet. */
-	if (!pvr_dev->fw_dev.booted)
+	/* Can't flush MMU if the firmware hasn't been initialised yet. */
+	if (!pvr_dev->fw_dev.initialised)
 		goto err_drm_dev_exit;
 
 	cmd_mmu_cache_data->cache_flags =
diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
index 88fcce433d12..a73a6815306b 100644
--- a/drivers/gpu/drm/imagination/pvr_power.c
+++ b/drivers/gpu/drm/imagination/pvr_power.c
@@ -216,7 +216,7 @@ pvr_watchdog_worker(struct work_struct *work)
 	if (pm_runtime_get_if_in_use(from_pvr_device(pvr_dev)->dev) <= 0)
 		goto out_requeue;
 
-	if (!pvr_dev->fw_dev.booted)
+	if (!pvr_dev->fw_dev.initialised)
 		goto out_pm_runtime_put;
 
 	stalled = pvr_watchdog_kccb_stalled(pvr_dev);
@@ -378,8 +378,8 @@ pvr_power_device_suspend(struct device *dev)
 	if (!drm_dev_enter(drm_dev, &idx))
 		return -EIO;
 
-	if (pvr_dev->fw_dev.booted) {
-		err = pvr_power_fw_disable(pvr_dev, false, true);
+	if (pvr_dev->fw_dev.initialised) {
+		err = pvr_power_fw_disable(pvr_dev, false);
 		if (err)
 			goto err_drm_dev_exit;
 	}
@@ -408,8 +408,8 @@ pvr_power_device_resume(struct device *dev)
 	if (err)
 		goto err_drm_dev_exit;
 
-	if (pvr_dev->fw_dev.booted) {
-		err = pvr_power_fw_enable(pvr_dev, true);
+	if (pvr_dev->fw_dev.initialised) {
+		err = pvr_power_fw_enable(pvr_dev);
 		if (err)
 			goto err_power_off;
 	}
@@ -548,7 +548,7 @@ pvr_power_reset(struct pvr_device *pvr_dev, bool hard_reset)
 		err = pvr_power_fw_disable(pvr_dev, hard_reset, false);
 		if (!err) {
 			if (hard_reset) {
-				pvr_dev->fw_dev.booted = false;
+				pvr_dev->fw_dev.initialised = false;
 				WARN_ON(pvr_power_device_suspend(from_pvr_device(pvr_dev)->dev));
 
 				err = pvr_fw_hard_reset(pvr_dev);
@@ -556,7 +556,7 @@ pvr_power_reset(struct pvr_device *pvr_dev, bool hard_reset)
 					goto err_device_lost;
 
 				err = pvr_power_device_resume(from_pvr_device(pvr_dev)->dev);
-				pvr_dev->fw_dev.booted = true;
+				pvr_dev->fw_dev.initialised = true;
 				if (err)
 					goto err_device_lost;
 			} else {

-- 
2.43.0

[PATCH 4/4] drm/imagination: Access FW initialised state with READ/WRITE_ONCE

[Brajesh Gupta]

Update FW initialised state shared resource access with READ/WRITE_ONCE
to prevent any complier optimization and ensure atomicity of operation.

Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
---
 drivers/gpu/drm/imagination/pvr_device.c |  2 +-
 drivers/gpu/drm/imagination/pvr_fw.c     |  4 ++--
 drivers/gpu/drm/imagination/pvr_mmu.c    |  2 +-
 drivers/gpu/drm/imagination/pvr_power.c  | 10 +++++-----
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
index 49696101b547..2691ef9af0ca 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -213,7 +213,7 @@ static irqreturn_t pvr_device_irq_thread_handler(int irq, void *data)
 	while (pvr_fw_irq_pending(pvr_dev)) {
 		pvr_fw_irq_clear(pvr_dev);
 
-		if (pvr_dev->fw_dev.initialised) {
+		if (READ_ONCE(pvr_dev->fw_dev.initialised)) {
 			pvr_fwccb_process(pvr_dev);
 			pvr_kccb_wake_up_waiters(pvr_dev);
 			pvr_device_process_active_queues(pvr_dev);
diff --git a/drivers/gpu/drm/imagination/pvr_fw.c b/drivers/gpu/drm/imagination/pvr_fw.c
index b8ad3f1d222c..850a3ec8e775 100644
--- a/drivers/gpu/drm/imagination/pvr_fw.c
+++ b/drivers/gpu/drm/imagination/pvr_fw.c
@@ -1004,7 +1004,7 @@ pvr_fw_init(struct pvr_device *pvr_dev)
 		goto err_fw_stop;
 	}
 
-	fw_dev->initialised = true;
+	WRITE_ONCE(fw_dev->initialised, true);
 
 	return 0;
 
@@ -1044,7 +1044,7 @@ pvr_fw_fini(struct pvr_device *pvr_dev)
 {
 	struct pvr_fw_device *fw_dev = &pvr_dev->fw_dev;
 
-	fw_dev->initialised = false;
+	WRITE_ONCE(fw_dev->initialised, false);
 
 	pvr_fw_destroy_structures(pvr_dev);
 	pvr_fw_object_unmap_and_destroy(pvr_dev->kccb.rtn_obj);
diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c b/drivers/gpu/drm/imagination/pvr_mmu.c
index e9fefcc4e234..3cac482e1034 100644
--- a/drivers/gpu/drm/imagination/pvr_mmu.c
+++ b/drivers/gpu/drm/imagination/pvr_mmu.c
@@ -134,7 +134,7 @@ int pvr_mmu_flush_exec(struct pvr_device *pvr_dev, bool wait)
 		return -EIO;
 
 	/* Can't flush MMU if the firmware hasn't been initialised yet. */
-	if (!pvr_dev->fw_dev.initialised)
+	if (!READ_ONCE(pvr_dev->fw_dev.initialised))
 		goto err_drm_dev_exit;
 
 	cmd_mmu_cache_data->cache_flags =
diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
index a73a6815306b..0ed9e7be604b 100644
--- a/drivers/gpu/drm/imagination/pvr_power.c
+++ b/drivers/gpu/drm/imagination/pvr_power.c
@@ -216,7 +216,7 @@ pvr_watchdog_worker(struct work_struct *work)
 	if (pm_runtime_get_if_in_use(from_pvr_device(pvr_dev)->dev) <= 0)
 		goto out_requeue;
 
-	if (!pvr_dev->fw_dev.initialised)
+	if (!READ_ONCE(pvr_dev->fw_dev.initialised))
 		goto out_pm_runtime_put;
 
 	stalled = pvr_watchdog_kccb_stalled(pvr_dev);
@@ -378,7 +378,7 @@ pvr_power_device_suspend(struct device *dev)
 	if (!drm_dev_enter(drm_dev, &idx))
 		return -EIO;
 
-	if (pvr_dev->fw_dev.initialised) {
+	if (READ_ONCE(pvr_dev->fw_dev.initialised)) {
 		err = pvr_power_fw_disable(pvr_dev, false);
 		if (err)
 			goto err_drm_dev_exit;
@@ -408,7 +408,7 @@ pvr_power_device_resume(struct device *dev)
 	if (err)
 		goto err_drm_dev_exit;
 
-	if (pvr_dev->fw_dev.initialised) {
+	if (READ_ONCE(pvr_dev->fw_dev.initialised)) {
 		err = pvr_power_fw_enable(pvr_dev);
 		if (err)
 			goto err_power_off;
@@ -548,7 +548,7 @@ pvr_power_reset(struct pvr_device *pvr_dev, bool hard_reset)
 		err = pvr_power_fw_disable(pvr_dev, hard_reset, false);
 		if (!err) {
 			if (hard_reset) {
-				pvr_dev->fw_dev.initialised = false;
+				WRITE_ONCE(pvr_dev->fw_dev.initialised, false);
 				WARN_ON(pvr_power_device_suspend(from_pvr_device(pvr_dev)->dev));
 
 				err = pvr_fw_hard_reset(pvr_dev);
@@ -556,7 +556,7 @@ pvr_power_reset(struct pvr_device *pvr_dev, bool hard_reset)
 					goto err_device_lost;
 
 				err = pvr_power_device_resume(from_pvr_device(pvr_dev)->dev);
-				pvr_dev->fw_dev.initialised = true;
+				WRITE_ONCE(pvr_dev->fw_dev.initialised, true);
 				if (err)
 					goto err_device_lost;
 			} else {

-- 
2.43.0

Claude review: drm/imagination: Multiple enhancement

[Claude Code Review Bot]

Overall Series Review

Subject: drm/imagination: Multiple enhancement
Author: Brajesh Gupta <brajesh.gupta@imgtec.com>
Patches: 5
Reviewed: 2026-05-16T14:11:24.951867

---

This is a 4-patch series for the PowerVR (Imagination) GPU driver with a mix of a bug fix (patch 1), a timeout-handling improvement (patch 2), and two cleanup/correctness patches (patches 3-4). The series title "Multiple enhancement" undersells that patch 1 is a real bug fix.

**There are two significant issues:**

1. **Patch 2 introduces a NULL pointer dereference risk** — `s_job->s_fence->parent` can be NULL if the job was never run by the scheduler, and `dma_fence_is_signaled()` does not handle NULL.

2. **Patch 3 introduces a build breakage** — it drops arguments from `pvr_power_fw_disable()` and `pvr_power_fw_enable()` call sites without updating the function signatures.

Overall the direction is sound but the series needs a respin to fix these issues.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/imagination: Populate FW common context ID before passing to the FW

[Claude Code Review Bot]

Patch Review

**Verdict: Good fix, minor nit**

This fixes a real bug. Before this patch, `ctx->ctx_id` was allocated via `xa_alloc()` *after* `pvr_context_create_queues()` which calls `pvr_queue_init_fw_context()`, which copies `ctx->ctx_id` into the firmware context at `pvr_queue.c:1097`:

```c
cctx_fw->server_common_context_id = ctx->ctx_id;
```

Since the struct is `kzalloc`'d, the firmware would always see `server_common_context_id = 0` instead of the actual allocated ID. Moving `xa_alloc()` earlier is the correct fix.

The cleanup of the old error path is also good. The old code had a comment about a race where another thread could take a reference on the context via `ctx_ids` between the `ctx_ids` allocation and the `ctx_handles` allocation failure. The new code's comment at `err_free_ctx_id` correctly explains why the erase is safe:

```c
/*
 * Ctx_id is not exposed to userspace and not visible yet within
 * the kernel/FW, plus a matching context handle (exposed to userspace)
 * hasn't been allocated yet, so it is safe to remove ctx_id
 * from the ctx_ids xarray.
 */
xa_erase(&pvr_dev->ctx_ids, ctx->ctx_id);
```

**Nit:** However, note that by moving `xa_alloc` for `ctx_ids` *earlier*, the window during which a partially-initialized context is visible in the `ctx_ids` xarray is now *larger*. If any code path does `xa_load(&pvr_dev->ctx_ids, id)` during this window, it would find a half-built context. The old code acknowledged this race. The commit message should mention why this is safe (e.g., "ctx_ids lookup only happens after the context handle is published to userspace").

---
Generated by Claude Code Patch Reviewer

Claude review: drm/imagination: Don't timeout job if its fence has been signaled

[Claude Code Review Bot]

Patch Review

**Verdict: Bug — NULL pointer dereference**

The added check:

```c
if (dma_fence_is_signaled(s_job->s_fence->parent))
    return DRM_GPU_SCHED_STAT_NO_HANG;
```

`s_job->s_fence->parent` can be NULL. The parent fence is assigned by `drm_sched_fence_set_parent()` during `run_job`. If the timeout fires before the job has been run (e.g., the job was stuck in the pending queue), `parent` is NULL. This will cause a NULL dereference inside `dma_fence_is_signaled()`, which does not check for NULL.

The code later in the same function even has:

```c
WARN_ON(job->base.s_fence->parent);
job->base.s_fence->parent = dma_fence_get(job->done_fence);
```

...which explicitly expects `parent` to be NULL in some cases.

**Fix:** Add a NULL check:

```c
if (s_job->s_fence->parent &&
    dma_fence_is_signaled(s_job->s_fence->parent))
    return DRM_GPU_SCHED_STAT_NO_HANG;
```

The overall idea (avoid false timeouts when the fence has already signaled) is reasonable and used in other DRM scheduler drivers.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/imagination: Rename FW booted to FW initialised

[Claude Code Review Bot]

Patch Review

**Verdict: Build breakage**

This patch is described as a pure rename (`booted` → `initialised`) but it silently changes function call arguments in `pvr_power.c`, creating a **compile error**.

In `pvr_power_device_suspend()`, the diff changes:
```c
-		err = pvr_power_fw_disable(pvr_dev, false, true);
+		err = pvr_power_fw_disable(pvr_dev, false);
```

In `pvr_power_device_resume()`:
```c
-		err = pvr_power_fw_enable(pvr_dev, true);
+		err = pvr_power_fw_enable(pvr_dev);
```

But the function signatures are *not* updated:
```c
pvr_power_fw_disable(struct pvr_device *pvr_dev, bool hard_reset, bool rpm_suspend)  // 3 params
pvr_power_fw_enable(struct pvr_device *pvr_dev, bool rpm_resume)  // 2 params
```

And other call sites in `pvr_power_reset()` still use the original argument count:
```c
err = pvr_power_fw_disable(pvr_dev, hard_reset, false);  // 3 args
err = pvr_power_fw_enable(pvr_dev, false);  // 2 args
```

This means the patch will fail to compile. It looks like the patch was generated against a different tree where `pvr_power_fw_disable`/`pvr_power_fw_enable` already had reduced parameter lists, or unrelated changes were accidentally folded in.

**Fix:** Either keep the original argument counts (just do the `booted` → `initialised` rename), or include the function signature changes and update all call sites consistently. If the intent is to remove the `rpm_suspend`/`rpm_resume` parameters entirely, that should be a separate patch with its own justification.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/imagination: Access FW initialised state with READ/WRITE_ONCE

[Claude Code Review Bot]

Patch Review

**Verdict: Reasonable, minor question**

Using `READ_ONCE`/`WRITE_ONCE` for `fw_dev->initialised` is appropriate since this bool is accessed from multiple contexts (IRQ handler, watchdog worker, suspend/resume, init/fini) without a common lock. This prevents compiler optimizations like repeated loads or store tearing.

The changes are straightforward and mechanical.

**Minor note:** `READ_ONCE`/`WRITE_ONCE` alone do not guarantee ordering with respect to the operations that the `initialised` flag guards. For example, in `pvr_fw_init()`:

```c
WRITE_ONCE(fw_dev->initialised, true);
```

There's no barrier ensuring that the FW initialization steps are visible to other CPUs before `initialised` becomes `true`. If ordering matters (i.e., if another CPU could check `initialised` and then access FW structures), a `smp_store_release()`/`smp_load_acquire()` pair would be more appropriate. The commit message says "ensure atomicity of operation" — but `READ_ONCE`/`WRITE_ONCE` don't provide atomicity guarantees in the hardware sense; they just prevent compiler issues. Worth clarifying the intent.

---
Generated by Claude Code Patch Reviewer