Re: [PATCH v4 02/18] drm/panthor: Revisit the reset logic to avoid reset request loss

[Liviu Dudau]

On Wed, Aug 26, 2026 at 04:56:01PM +0200, Boris Brezillon wrote:
> disable/enable_work() provide a ready to use mechanism to temporarily
> disable a work item, so use that instead of the complex state machinery
> based on the PM state.
> 
> This also allows us to automate the reset resubmission in case a reset
> was received while the work item was disabled.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>

Reviewed-by: Liviu Dudau <liviu.dudau@xxxxxxx>

Best regards,
Liviu

> ---
>  drivers/gpu/drm/panthor/panthor_device.c | 36 +++++++++++++++++++++++++-------
>  drivers/gpu/drm/panthor/panthor_device.h |  3 +--
>  2 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 7c55d0c45cfd..2974f4bc0bb1 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -181,6 +181,15 @@ static void panthor_device_free_page(struct drm_device *ddev, void *data)
>  	__free_page(data);
>  }
>  
> +static void enable_resets(struct panthor_device *ptdev)
> +{
> +	/* If a reset has been queued while the work was
> +	 * disabled, reschedule it.
> +	 */
> +	if (enable_work(&ptdev->reset.work) && atomic_read(&ptdev->reset.pending))
> +		queue_work(ptdev->reset.wq, &ptdev->reset.work);
> +}
> +
>  int panthor_device_init(struct panthor_device *ptdev)
>  {
>  	u32 *dummy_page_virt;
> @@ -256,6 +265,13 @@ int panthor_device_init(struct panthor_device *ptdev)
>  
>  	ptdev->phys_addr = res->start;
>  
> +	/* panthor_device_resume() calls enable_resets(), so we need to disable
> +	 * the reset.work manually before this gets called to keep things
> +	 * balanced. We don't bother re-enabling the work if the resume fails,
> +	 * because the whole initialization will fail in that case, and the work
> +	 * will vanish.
> +	 */
> +	disable_work(&ptdev->reset.work);
>  	ret = devm_pm_runtime_enable(ptdev->base.dev);
>  	if (ret)
>  		return ret;
> @@ -305,9 +321,6 @@ int panthor_device_init(struct panthor_device *ptdev)
>  
>  	panthor_gem_init(ptdev);
>  
> -	/* Now that everything is initialized, we can enable the reset work. */
> -	enable_work(&ptdev->reset.work);
> -
>  	/* ~3 frames */
>  	pm_runtime_set_autosuspend_delay(ptdev->base.dev, 50);
>  	pm_runtime_use_autosuspend(ptdev->base.dev);
> @@ -316,6 +329,9 @@ int panthor_device_init(struct panthor_device *ptdev)
>  	if (ret)
>  		goto err_disable_autosuspend;
>  
> +	/* Now that everything is initialized, we can enable the reset work. */
> +	enable_resets(ptdev);
> +
>  	pm_runtime_put_autosuspend(ptdev->base.dev);
>  	return 0;
>  
> @@ -534,10 +550,8 @@ int panthor_device_resume(struct device *dev)
>  		/* If there was a reset pending at the time we suspended the
>  		 * device, we force a slow reset.
>  		 */
> -		if (atomic_read(&ptdev->reset.pending)) {
> +		if (atomic_cmpxchg(&ptdev->reset.pending, 1, 0))
>  			ptdev->reset.fast = false;
> -			atomic_set(&ptdev->reset.pending, 0);
> -		}
>  
>  		ret = panthor_device_resume_hw_components(ptdev);
>  		if (ret && ptdev->reset.fast) {
> @@ -565,6 +579,9 @@ int panthor_device_resume(struct device *dev)
>  			    DRM_PANTHOR_USER_MMIO_OFFSET, 0, 1);
>  	atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_ACTIVE);
>  	mutex_unlock(&ptdev->pm.mmio_lock);
> +
> +	/* Now that everything is resumed, we can re-enable the reset work. */
> +	enable_resets(ptdev);
>  	return 0;
>  
>  err_suspend_devfreq:
> @@ -604,10 +621,13 @@ int panthor_device_suspend(struct device *dev)
>  			    DRM_PANTHOR_USER_MMIO_OFFSET, 0, 1);
>  	mutex_unlock(&ptdev->pm.mmio_lock);
>  
> +	/* Make sure we're not interrupted by resets after that point
> +	 * until the GPU is resumed.
> +	 */
> +	disable_work_sync(&ptdev->reset.work);
> +
>  	if (panthor_device_is_initialized(ptdev) &&
>  	    drm_dev_enter(&ptdev->base, &cookie)) {
> -		cancel_work_sync(&ptdev->reset.work);
> -
>  		/* We prepare everything as if we were resetting the GPU.
>  		 * The end of the reset will happen in the resume path though.
>  		 */
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 6529e01e838d..f55baa21b25e 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -389,8 +389,7 @@ void panthor_device_unplug(struct panthor_device *ptdev);
>   */
>  static inline void panthor_device_schedule_reset(struct panthor_device *ptdev)
>  {
> -	if (!atomic_cmpxchg(&ptdev->reset.pending, 0, 1) &&
> -	    atomic_read(&ptdev->pm.state) == PANTHOR_DEVICE_PM_STATE_ACTIVE)
> +	if (!atomic_cmpxchg(&ptdev->reset.pending, 0, 1))
>  		queue_work(ptdev->reset.wq, &ptdev->reset.work);
>  }
>  
> 
> -- 
> 2.55.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯