Re: [PATCH v4 03/18] drm/panthor: Make panthor_device::pm::state non-atomic

[Adrian Larumbe <adrian.larumbe@collabora.com>]

On 26.08.2026 16:56, 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@collabora.com>
> ---
>  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);

Maybe you could replace it with scoped_guard(mutex, &ptdev->pm.lock) {}
like you've done in other parts of the commit.

Other than that:

Reviewed-by: Adrián Larumbe <adrian.larumbe@collabora.com>

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


Adrian Larumbe