[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

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

[Matt Coster]

[-- Attachment #1.1: Type: text/plain, Size: 3656 bytes --]

Hi Brajesh,

On 12/05/2026 07:47, Brajesh Gupta wrote:
> Fix context ID value for the FW common context by moving the context
> allocation earlier.

"Fix" is pretty vague here – can you describe the current behaviour, and
why that's wrong?

I also assume this fixes a previous change, so can you please add a
Fixes: tag referencing the commit which introduced the faulty behaviour
(and possibly a Cc: stable tag, if needed)?

> Also an old incorrect comment was removed.

This detail probably doesn't need to be mentioned.

Cheers,
Matt

> 
> 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
> 


-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Matt Coster]

[-- Attachment #1.1: Type: text/plain, Size: 2215 bytes --]

Hi Brajesh,

On 12/05/2026 07:47, Brajesh Gupta wrote:
> Check for job's fence in timedout handler and report NO HANG if its

Nit: s/its/it/

> is signaled

This could use more explanation: _why_ is returning NO_HANG in this
instance the correct behaviour? See the kernel docs section "Describe
your changes"[1] for inspiration.

> 
> 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

The formatting here is off (and won't render nicely in the docs). I
believe it should be something like:

   * Return:
   *  * %DRM_GPU_SCHED_STAT_NO_HANG if the job fence has already been
   *    signaled, or
   *  * %DRM_gPU_SCHED_STAT_RESET otherwise.

>   */
>  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 */

I don't think this comment really adds anything, the function name and
return value are fairly descriptive already.

Cheers,
Matt

[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

> +       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
> 


-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Matt Coster]

[-- Attachment #1.1: Type: text/plain, Size: 6379 bytes --]

Hi Brajesh,

On 12/05/2026 07:47, Brajesh Gupta wrote:
> Update name to FW initialised from booted to better represent FW state.

Can you please add an explanation of the distinction between "booted"
and "initialised" that makes this useful?

Cheers,
Matt

> 
> 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
> 


-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Matt Coster]

[-- Attachment #1.1: Type: text/plain, Size: 6153 bytes --]

Hi Brajesh,

On 12/05/2026 07:47, Brajesh Gupta wrote:
> Update FW initialised state shared resource access with READ/WRITE_ONCE
> to prevent any complier optimization and ensure atomicity of operation.

We're not trying to prevent _any_ compiler optimisations, there are
specific ones that READ_ONCE() prevents. Can you please include an
explanation of exactly what we're trying to avoid here so future readers
can understand the motivation of this change?

My understanding is that (for instance in one case) it's to prevent the
compiler from assuming it only needs to read the value of
fw_dev->initialised once outside the loop instead of on every iteration
(grep "merge successive loads" in [1] for details).

One further thought after skimming[1]: do we actually need to use
READ/WRITE_ONCE() on _every_ read/write of ->initialised? Or can/should
we just use it in critical cases (like the loop body example mentioned
above)?

Cheers,
Matt

[1]: https://www.kernel.org/doc/html/latest/core-api/wrappers/memory-barriers.html

> 
> 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
> 


-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Brajesh Gupta]

On Mon, 2026-05-18 at 15:01 +0000, Matt Coster wrote:
> Hi Brajesh,
> 
> On 12/05/2026 07:47, Brajesh Gupta wrote:
> > Fix context ID value for the FW common context by moving the context
> > allocation earlier.
> 
> "Fix" is pretty vague here – can you describe the current behaviour, and
> why that's wrong?

Updated

> 
> I also assume this fixes a previous change, so can you please add a
> Fixes: tag referencing the commit which introduced the faulty behaviour
> (and possibly a Cc: stable tag, if needed)?
> 
Didn't include fix tag as Context ID is not used currently in driver so nothing
is broken.
> > Also an old incorrect comment was removed.
> 
> This detail probably doesn't need to be mentioned.
> 
Dropped

> Cheers,
> Matt
> 
> > 
> > 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
> > 
> 
> 
Thanks,
Brajesh

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

[Brajesh Gupta]

On Mon, 2026-05-18 at 15:01 +0000, Matt Coster wrote:
> Hi Brajesh,
> 
> On 12/05/2026 07:47, Brajesh Gupta wrote:
> > Check for job's fence in timedout handler and report NO HANG if its
> 
> Nit: s/its/it/
> 
Updated

> > is signaled
> 
> This could use more explanation: _why_ is returning NO_HANG in this
> instance the correct behaviour? See the kernel docs section "Describe
> your changes"[1] for inspiration.
> 
Updated

> > 
> > 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
> 
> The formatting here is off (and won't render nicely in the docs). I
> believe it should be something like:
> 
>    * Return:
>    *  * %DRM_GPU_SCHED_STAT_NO_HANG if the job fence has already been
>    *    signaled, or
>    *  * %DRM_gPU_SCHED_STAT_RESET otherwise.
> 
Updated

> >   */
> >  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 */
> 
> I don't think this comment really adds anything, the function name and
> return value are fairly descriptive already.
> 
Dropped

> Cheers,
> Matt
> 
> [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> 
> > +       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
> > 
> 
> 
Thanks,
Brajesh

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

[Brajesh Gupta]

On Mon, 2026-05-18 at 15:01 +0000, Matt Coster wrote:
> Hi Brajesh,
> 
> On 12/05/2026 07:47, Brajesh Gupta wrote:
> > Update name to FW initialised from booted to better represent FW state.
> 
> Can you please add an explanation of the distinction between "booted"
> and "initialised" that makes this useful?
> 
> Cheers,
> Matt
> 
Updated

> > 
> > 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
> > 
> 
> 
Thanks,
Brajesh

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

[Brajesh Gupta]

On Mon, 2026-05-18 at 15:02 +0000, Matt Coster wrote:
> Hi Brajesh,
> 
> On 12/05/2026 07:47, Brajesh Gupta wrote:
> > Update FW initialised state shared resource access with READ/WRITE_ONCE
> > to prevent any complier optimization and ensure atomicity of operation.
> 
> We're not trying to prevent _any_ compiler optimisations, there are
> specific ones that READ_ONCE() prevents. Can you please include an
> explanation of exactly what we're trying to avoid here so future readers
> can understand the motivation of this change?
> 
> My understanding is that (for instance in one case) it's to prevent the
> compiler from assuming it only needs to read the value of
> fw_dev->initialised once outside the loop instead of on every iteration
> (grep "merge successive loads" in [1] for details).
> 
Updated. Please check.

> One further thought after skimming[1]: do we actually need to use
> READ/WRITE_ONCE() on _every_ read/write of ->initialised? Or can/should
> we just use it in critical cases (like the loop body example mentioned
> above)?
> 
Agree that we don't need READ/WRITE_ONCE for every use but kept it to make
shared variable and synchronisation explicit.

> Cheers,
> Matt
> 
> [1]: https://www.kernel.org/doc/html/latest/core-api/wrappers/memory-barriers.html
> 
> > 
> > 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
> > 
> 
>