Re: [PATCH v4 03/18] drm/panthor: Make panthor_device::pm::state non-atomic
From: Liviu Dudau
Date: Thu Aug 27 2026 - 11:14:16 EST
On Wed, Aug 26, 2026 at 04:56:02PM +0200, Boris Brezillon wrote:
> Now that the reset logic has been reworked to use disable/enable_work(),
> there's no need for panthor_device::pm::state to be an atomic. It can
> simply be accessed under the same lock we use to touch MMIO mappings.
>
> While at it, rename the lock to make it clear it protects more than just
> the MMIO logic, and transition locked sections to scoped_guard().
>
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/panthor/panthor_device.c | 97 +++++++++++++++++---------------
> drivers/gpu/drm/panthor/panthor_device.h | 16 ++++--
> 2 files changed, 63 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 2974f4bc0bb1..133e3895cd0a 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -147,8 +147,10 @@ static void panthor_device_reset_work(struct work_struct *work)
> /* If the device is entering suspend, we don't reset. A slow reset will
> * be forced at resume time instead.
> */
> - if (atomic_read(&ptdev->pm.state) != PANTHOR_DEVICE_PM_STATE_ACTIVE)
> - return;
> + scoped_guard(mutex, &ptdev->pm.lock) {
> + if (ptdev->pm.state != PANTHOR_DEVICE_PM_STATE_ACTIVE)
> + return;
> + }
>
> if (!drm_dev_enter(&ptdev->base, &cookie))
> return;
> @@ -204,7 +206,7 @@ int panthor_device_init(struct panthor_device *ptdev)
> if (ret)
> return ret;
>
> - ret = drmm_mutex_init(&ptdev->base, &ptdev->pm.mmio_lock);
> + ret = drmm_mutex_init(&ptdev->base, &ptdev->pm.lock);
> if (ret)
> return ret;
>
> @@ -213,7 +215,7 @@ int panthor_device_init(struct panthor_device *ptdev)
> INIT_LIST_HEAD(&ptdev->gems.node);
> #endif
>
> - atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_SUSPENDED);
> + ptdev->pm.state = PANTHOR_DEVICE_PM_STATE_SUSPENDED;
> p = alloc_page(GFP_KERNEL | __GFP_ZERO);
> if (!p)
> return -ENOMEM;
> @@ -432,40 +434,39 @@ static vm_fault_t panthor_mmio_vm_fault(struct vm_fault *vmf)
> {
> struct vm_area_struct *vma = vmf->vma;
> struct panthor_device *ptdev = vma->vm_private_data;
> - u64 offset = (u64)vma->vm_pgoff << PAGE_SHIFT;
> - unsigned long pfn;
> - pgprot_t pgprot;
> vm_fault_t ret;
> - bool active;
> int cookie;
>
> if (!drm_dev_enter(&ptdev->base, &cookie))
> return VM_FAULT_SIGBUS;
>
> - mutex_lock(&ptdev->pm.mmio_lock);
> - active = atomic_read(&ptdev->pm.state) == PANTHOR_DEVICE_PM_STATE_ACTIVE;
> + scoped_guard(mutex, &ptdev->pm.lock) {
> + bool active = ptdev->pm.state == PANTHOR_DEVICE_PM_STATE_ACTIVE;
> + u64 offset = (u64)vma->vm_pgoff << PAGE_SHIFT;
> + unsigned long pfn;
> + pgprot_t pgprot;
>
> - switch (offset) {
> - case DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET:
> + switch (offset) {
> + case DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET:
> + if (active)
> + pfn = __phys_to_pfn(ptdev->phys_addr + CSF_GPU_LATEST_FLUSH_ID);
> + else
> + pfn = page_to_pfn(ptdev->pm.dummy_latest_flush);
> + break;
> +
> + default:
> + ret = VM_FAULT_SIGBUS;
> + goto out_dev_exit;
> + }
> +
> + pgprot = vma->vm_page_prot;
> if (active)
> - pfn = __phys_to_pfn(ptdev->phys_addr + CSF_GPU_LATEST_FLUSH_ID);
> - else
> - pfn = page_to_pfn(ptdev->pm.dummy_latest_flush);
> - break;
> + pgprot = pgprot_noncached(pgprot);
>
> - default:
> - ret = VM_FAULT_SIGBUS;
> - goto out_unlock;
> + ret = vmf_insert_pfn_prot(vma, vmf->address, pfn, pgprot);
> }
>
> - pgprot = vma->vm_page_prot;
> - if (active)
> - pgprot = pgprot_noncached(pgprot);
> -
> - ret = vmf_insert_pfn_prot(vma, vmf->address, pfn, pgprot);
> -
> -out_unlock:
> - mutex_unlock(&ptdev->pm.mmio_lock);
> +out_dev_exit:
> drm_dev_exit(cookie);
> return ret;
> }
> @@ -526,10 +527,13 @@ int panthor_device_resume(struct device *dev)
> struct panthor_device *ptdev = dev_get_drvdata(dev);
> int ret, cookie;
>
> - if (atomic_read(&ptdev->pm.state) != PANTHOR_DEVICE_PM_STATE_SUSPENDED)
> - return -EINVAL;
> + scoped_guard(mutex, &ptdev->pm.lock) {
> + if (ptdev->pm.state != PANTHOR_DEVICE_PM_STATE_SUSPENDED)
> + return -EINVAL;
> +
> + ptdev->pm.state = PANTHOR_DEVICE_PM_STATE_RESUMING;
> + }
>
> - atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_RESUMING);
>
> ret = clk_prepare_enable(ptdev->clks.core);
> if (ret)
> @@ -574,11 +578,11 @@ int panthor_device_resume(struct device *dev)
> * are removed and the real iomem mapping will be restored on next
> * access.
> */
> - mutex_lock(&ptdev->pm.mmio_lock);
> + mutex_lock(&ptdev->pm.lock);
> unmap_mapping_range(ptdev->base.anon_inode->i_mapping,
> DRM_PANTHOR_USER_MMIO_OFFSET, 0, 1);
> - atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_ACTIVE);
> - mutex_unlock(&ptdev->pm.mmio_lock);
> + ptdev->pm.state = PANTHOR_DEVICE_PM_STATE_ACTIVE;
> + mutex_unlock(&ptdev->pm.lock);
Can this also use scoped_guard()?
Otherwise, this looks good to me!
Reviewed-by: Liviu Dudau <liviu.dudau@xxxxxxx>
Best regards,
Liviu
>
> /* Now that everything is resumed, we can re-enable the reset work. */
> enable_resets(ptdev);
> @@ -595,7 +599,9 @@ int panthor_device_resume(struct device *dev)
> clk_disable_unprepare(ptdev->clks.core);
>
> err_set_suspended:
> - atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_SUSPENDED);
> + scoped_guard(mutex, &ptdev->pm.lock)
> + ptdev->pm.state = PANTHOR_DEVICE_PM_STATE_SUSPENDED;
> +
> atomic_set(&ptdev->pm.recovery_needed, 1);
> return ret;
> }
> @@ -605,21 +611,21 @@ int panthor_device_suspend(struct device *dev)
> struct panthor_device *ptdev = dev_get_drvdata(dev);
> int cookie;
>
> - if (atomic_read(&ptdev->pm.state) != PANTHOR_DEVICE_PM_STATE_ACTIVE)
> - return -EINVAL;
> -
> /* Clear all IOMEM mappings pointing to this device before we
> * shutdown the power-domain and clocks. Failing to do that results
> * in external aborts when the process accesses the iomem region.
> * We change the state and call unmap_mapping_range() with the
> - * mmio_lock held to make sure the vm_fault handler won't set up
> + * lock held to make sure the vm_fault handler won't set up
> * invalid mappings.
> */
> - mutex_lock(&ptdev->pm.mmio_lock);
> - atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_SUSPENDING);
> - unmap_mapping_range(ptdev->base.anon_inode->i_mapping,
> - DRM_PANTHOR_USER_MMIO_OFFSET, 0, 1);
> - mutex_unlock(&ptdev->pm.mmio_lock);
> + scoped_guard(mutex, &ptdev->pm.lock) {
> + if (ptdev->pm.state != PANTHOR_DEVICE_PM_STATE_ACTIVE)
> + return -EINVAL;
> +
> + ptdev->pm.state = PANTHOR_DEVICE_PM_STATE_SUSPENDING;
> + unmap_mapping_range(ptdev->base.anon_inode->i_mapping,
> + DRM_PANTHOR_USER_MMIO_OFFSET, 0, 1);
> + }
>
> /* Make sure we're not interrupted by resets after that point
> * until the GPU is resumed.
> @@ -644,6 +650,9 @@ int panthor_device_suspend(struct device *dev)
> clk_disable_unprepare(ptdev->clks.coregroup);
> clk_disable_unprepare(ptdev->clks.stacks);
> clk_disable_unprepare(ptdev->clks.core);
> - atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_SUSPENDED);
> +
> + scoped_guard(mutex, &ptdev->pm.lock)
> + ptdev->pm.state = PANTHOR_DEVICE_PM_STATE_SUSPENDED;
> +
> return 0;
> }
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index f55baa21b25e..217eec811bdb 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -298,18 +298,22 @@ struct panthor_device {
>
> /** @pm: Power management related data. */
> struct {
> - /** @state: Power state. */
> - atomic_t state;
> + /**
> + * @state: Power state.
> + *
> + * Must be accessed with the panthor_device::pm::lock held.
> + */
> + enum panthor_device_pm_state state;
>
> /**
> - * @mmio_lock: Lock protecting MMIO userspace CPU mappings.
> + * @lock: Lock protecting PM related fields.
> *
> * This is needed to ensure we map the dummy IO pages when
> * the device is being suspended, and the real IO pages when
> - * the device is being resumed. We can't just do with the
> - * state atomicity to deal with this race.
> + * the device is being resumed. We can't just do with an
> + * atomic state to deal with this race.
> */
> - struct mutex mmio_lock;
> + struct mutex lock;
>
> /**
> * @dummy_latest_flush: Dummy LATEST_FLUSH page.
>
> --
> 2.55.0
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯