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

[Matt Coster <Matt.Coster@imgtec.com>]

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