Re: [PATCH v2 25/28] drm/panthor: Route HW component PM through access windows

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [High] drm/panthor: missing panthor_sched_suspend() in
  panthor_aw_post_yield_work()
- [High] drm/panthor: race between panthor_aw_ensure_gpu_access() and
  post_yield_work
- [High] drm/panthor: endless AW yield-request ping-pong from
  panthor_aw_post_yield_work()
- [Medium] drm/panthor: panthor_device_resume() treats -EAGAIN as
  fast-reset failure
- [Medium] drm/panthor: __panthor_aw_suspend() leaves state as
  SUSPEND_PENDING
- [Medium] drm/panthor: unhandled -EAGAIN from panthor_aw_resume() on PM
  paths
--

commit 4fae2615b4ec401e9748b231f8ea063e488e1f9a
Author: Karunika Choo <karunika.choo@arm.com>

drm/panthor: Route HW component PM through access windows

This commit routes hardware component suspend and resume through the access
window layer when AW support is present, defers post-yield cleanup to a
workqueue, and updates panthor_device_resume_and_get() to ensure GPU access is
granted before returning.

> diff --git a/drivers/gpu/drm/panthor/panthor_aw.c b/drivers/gpu/drm/panthor/panthor_aw.c
> index 5ab1d44f0a22e..3b418c816d824 100644
> --- a/drivers/gpu/drm/panthor/panthor_aw.c
> +++ b/drivers/gpu/drm/panthor/panthor_aw.c
[ ... ]
> @@ -114,6 +117,66 @@ static int panthor_aw_state_wait(struct panthor_aw *aw,
>  	return panthor_aw_wait_cond(aw, atomic_read(&aw->state) == target, timeout_ms);
>  }
>  
> +static int panthor_aw_state_wait_transition(struct panthor_aw *aw, u32 timeout_ms)
> +{
> +	return panthor_aw_wait_cond(
> +		aw,
> +		(atomic_read(&aw->state) == PANTHOR_AW_STATE_READY ||
> +		 atomic_read(&aw->state) == PANTHOR_AW_STATE_GPU_GRANTED),
> +		timeout_ms);
> +}
> +
> +static void panthor_aw_post_yield_work(struct work_struct *work)
> +{
> +	struct panthor_aw *aw =
> +		container_of(work, struct panthor_aw, post_yield_work);
> +	struct panthor_device *ptdev = aw->ptdev;
> +	struct device *dev = ptdev->base.dev;
> +	int ret;
> +
> +	scoped_guard(spinlock_irqsave, &aw->lock) {
> +		/* Something else has progressed the state to READY. */
> +		if (atomic_read(&aw->state) != PANTHOR_AW_STATE_SUSPEND_PENDING)
> +			return;
> +	}

[Severity: Medium]
Does any other path ever transition aw->state from
PANTHOR_AW_STATE_SUSPEND_PENDING to PANTHOR_AW_STATE_READY?

When panthor_aw_suspend() runs after a WINDOW_CLOSED interrupt sets
PANTHOR_AW_STATE_SUSPEND_PENDING, it suspends the hardware components and
calls __panthor_aw_suspend(), which returns 0 on
PANTHOR_AW_STATE_SUSPEND_PENDING without updating aw->state:

drivers/gpu/drm/panthor/panthor_aw.c:__panthor_aw_suspend() {
	scoped_guard(spinlock_irqsave, &aw->lock) {
		switch (atomic_read(&aw->state)) {
		case PANTHOR_AW_STATE_READY:
			return 0;
		case PANTHOR_AW_STATE_SUSPEND_PENDING:
			return 0;
    ...
}

Because aw->state remains PANTHOR_AW_STATE_SUSPEND_PENDING after runtime
suspend finishes, panthor_aw_post_yield_work() does not exit early here and
calls panthor_sched_resume(), which schedules tick_work and wakes the
suspended GPU back up.  If system resume (pm_runtime_force_resume()) runs
before post_yield_work instead, panthor_aw_resume() sees
PANTHOR_AW_STATE_SUSPEND_PENDING and fails system resume with -EAGAIN.

> +
> +	/*
> +	 * Ensure in-progress resume is completed and prevent future RPM suspend
> +	 * to keep the clocks turned on when accessing HW registers.
> +	 */
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_barrier(dev);
> +
> +	ret = panthor_device_suspend_hw_components(ptdev);
> +	if (ret)
> +		dev_warn(dev, "Failed to suspend hardware components.");

[Severity: High]
Doesn't panthor_aw_post_yield_work() need to suspend or unbind scheduler
groups before calling panthor_device_suspend_hw_components()?

Unlike panthor_device_suspend(), which calls panthor_sched_suspend() before
panthor_aw_suspend(), panthor_aw_post_yield_work() leaves groups bound in
sched->csg_slots[] with group->csg_id >= 0 while
panthor_device_suspend_hw_components() stops the MCU and calls
panthor_mmu_suspend().  panthor_mmu_suspend() disables all active address
spaces and calls panthor_vm_release_as_locked(), resetting vm->as.id = -1 and
vm->as.active_cnt = 0 on those still-bound groups.

On the next scheduler tick after hardware components resume, tick_ctx_apply()
sees group->csg_id >= 0 and skips group_bind_locked(), panthor_vm_active(),
and CSG programming, leaving the bound groups unmapped in the MMU and
unconfigured on the rebooted MCU.  When those groups are later unbound in
group_unbind_locked(), panthor_vm_idle() decrements vm->as.active_cnt from 0
(underflowing refcount_t) and hits drm_WARN_ON(&ptdev->base, vm->as.id == -1).
In addition, if runtime suspend runs before the hardware components are
resumed again, panthor_device_suspend() will call panthor_sched_suspend()
while the MCU is stopped and the access window is closed.

> +
> +	scoped_guard(spinlock_irqsave, &aw->lock)
> +		panthor_aw_state_set(aw, PANTHOR_AW_STATE_READY);
> +
> +	/*
> +	 * schedule an immediate tick to re-evaluate if there is any additional
> +	 * work. This will request access to the GPU again and if the driver has
> +	 * gone idle, it will yield GPU access via the runtime suspend path.
> +	 */
> +	panthor_sched_resume(ptdev);
> +
> +	pm_runtime_put_autosuspend(dev);
> +}

[Severity: High]
Can calling panthor_sched_resume() unconditionally here cause an endless
request/yield livelock between two idle access windows sharing a partition?

When AW0 is idle inside its 50ms autosuspend window and AW1 requests the GPU,
the arbiter issues PART_CMD_YIELD_IDLE and FW v2 closes AW0's window,
scheduling panthor_aw_post_yield_work() on AW0.  panthor_sched_resume() then
queues tick_work with delay 0, and tick_work() calls
panthor_device_resume_and_get() -> panthor_aw_ensure_gpu_access() ->
panthor_aw_request() before checking whether AW0 has any runnable groups.

Sending VM_ARB_GPU_REQUEST from idle AW0 causes the arbiter to issue
PART_CMD_YIELD_IDLE to AW1.  Once AW1 finishes its work and yields to AW0,
AW1's panthor_aw_post_yield_work() likewise requests the GPU back from AW0,
while AW0's tick_work() finds no runnable work and resets its 50ms
autosuspend timer via pm_runtime_put_autosuspend().  Both access windows can
then ping-pong GPU ownership and hardware component suspend/resume
indefinitely without ever reaching runtime autosuspend.

[ ... ]

> @@ -463,21 +538,41 @@ int panthor_aw_resume(struct panthor_device *ptdev)
>  	struct panthor_aw *aw = ptdev->aw;
>  	int ret;
>  
> +	/* resume hw components directly if AW is not supported */
>  	if (!aw)
> -		return 0;
> +		return panthor_device_resume_hw_components(ptdev);
>  
>  	panthor_aw_irq_resume(&aw->irq);
>  
> +again:
>  	ret = panthor_aw_request(aw);
> +	if (ret)
> +		goto err_out;
> +
> +	ret = panthor_device_resume_hw_components(ptdev);
>  	if (ret) {
> -		drm_warn(&ptdev->base, "Timedout waiting for GPU to be granted");
> +		/* Yield GPU access if HW components failed to resume */
> +		__panthor_aw_suspend(aw);
>  		goto err_out;
>  	}
>  
> -	return 0;
> +	switch (atomic_read(&aw->state)) {
> +	case PANTHOR_AW_STATE_SUSPEND_PENDING:
> +		/*
> +		 * WINDOW_CLOSED won the race. Suspend components before the
> +		 * caller can disable clocks. post_yield_work will transition
> +		 * state to READY after this PM transition completes.
> +		 */
> +		panthor_device_suspend_hw_components(ptdev);
> +		ret = -EAGAIN;
> +		break;
> +	case PANTHOR_AW_STATE_GPU_GRANTED:
> +		return 0;
> +	default:
> +		goto again;
> +	}
>  
>  err_out:
> -	panthor_aw_irq_suspend(&aw->irq);
>  	return ret;
>  }

[Severity: High]
Can panthor_aw_ensure_gpu_access() -> panthor_aw_resume() race with
panthor_aw_post_yield_work() when the device is already RPM_ACTIVE?

panthor_aw_post_yield_work() uses pm_runtime_barrier(dev) to wait for an
in-progress runtime resume, but panthor_device_resume_and_get() calls
panthor_aw_ensure_gpu_access() after pm_runtime_resume_and_get() has already
set the device to RPM_ACTIVE.  In that state pm_runtime_barrier(dev) returns
immediately, and nothing serializes panthor_aw_resume() against
panthor_aw_post_yield_work() or a second caller of
panthor_aw_ensure_gpu_access():

CPU0                              CPU1 (kworker / second caller)
panthor_aw_ensure_gpu_access()
  panthor_aw_resume()
    panthor_device_resume_hw_components()
    /* WINDOW_CLOSED sets SUSPEND_PENDING */
    switch (atomic_read(&aw->state))
    case PANTHOR_AW_STATE_SUSPEND_PENDING:
                                  panthor_aw_post_yield_work()
                                    pm_runtime_barrier() /* no-op */
                                    panthor_device_suspend_hw_components()
                                    panthor_aw_state_set(READY)
                                  panthor_aw_ensure_gpu_access()
                                    panthor_aw_resume()
                                      panthor_aw_request() /* GRANTED */
                                      panthor_device_resume_hw_components()
                                      return 0
      panthor_device_suspend_hw_components()
      <-- suspends FW/MMU/GPU/PWR while CPU1 is using the GPU

Alternatively, if WINDOW_CLOSED fires after panthor_aw_request() succeeds on
CPU0 but before CPU0 takes hw_component_lock in
panthor_device_resume_hw_components(), panthor_aw_post_yield_work() can finish
and set aw->state to PANTHOR_AW_STATE_READY first; CPU0 then resumes hardware
components while the window is closed, sees PANTHOR_AW_STATE_READY, and takes
default: goto again without suspending them.

> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index ba5252c27410f..d1ef460577c99 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
[ ... ]
> @@ -577,15 +579,17 @@ int panthor_device_resume(struct device *dev)
>  			atomic_set(&ptdev->reset.pending, 0);
>  		}
>  
> -		ret = panthor_device_resume_hw_components(ptdev);
> +		ret = panthor_aw_resume(ptdev);
>  		if (ret && ptdev->reset.fast) {
>  			drm_err(&ptdev->base, "Fast reset failed, trying a slow reset");
>  			ptdev->reset.fast = false;
> -			ret = panthor_device_resume_hw_components(ptdev);
> +			ret = panthor_aw_resume(ptdev);
>  		}

[Severity: Medium]
Does panthor_device_resume() treat -EAGAIN from panthor_aw_resume() as a
fast-reset failure?

When panthor_aw_resume() returns -EAGAIN because aw->state is
PANTHOR_AW_STATE_SUSPEND_PENDING, panthor_device_resume() logs a false fast
reset error, clears ptdev->reset.fast = false (forcing a full firmware reload
on the next resume), and immediately retries panthor_aw_resume() while the
device is still RPM_RESUMING and panthor_aw_post_yield_work() is still blocked
in pm_runtime_barrier(dev), so the retry is guaranteed to fail with -EAGAIN
as well.  Conversely, when panthor_device_resume_hw_components() is called
from panthor_aw_ensure_gpu_access() -> panthor_aw_resume() while the device
is already RPM_ACTIVE, there is no fallback to retry with
ptdev->reset.fast = false if a fast reset fails.

>  
>  		if (!ret)
>  			panthor_sched_resume(ptdev);
> +		else
> +			panthor_aw_suspend(ptdev);
>  
>  		drm_dev_exit(cookie);

[Severity: Medium]
What happens when panthor_device_resume() returns -EAGAIN to callers other
than panthor_device_resume_and_get()?

panthor_device_resume_and_get() catches -EAGAIN, waits with
panthor_aw_wait_transition_complete(), and retries, but
panthor_device_unplug() calls pm_runtime_get_sync() directly (triggering
drm_WARN_ON() and proceeding with unplug while suspended) and
panthor_device_system_resume() calls pm_runtime_force_resume() directly
(failing system resume and leaving the device suspended).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110153907.3241978-1-karunika.choo@arm.com?part=25