Re: [PATCH v1 21/27] drm/panthor: Request AW to yield GPU access on idle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpu: back-to-back command writes and untracked YIELD_IDLE cause spurious resets
--

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

drm/panthor: Request AW to yield GPU access on idle
    
v15 GPU introduces a YIELD_IDLE command/interrupt to signal an access
window to relinquish GPU access as soon as it is idle. With FW v2, the
GPU is immediately yielded when it detects the GPU is idle and the
YIELD_IDLE interrupt is raised.

For latency and CPU scheduling reasons, we want to avoid constantly
yielding the GPU the moment it goes idle when there is only a singular
access window requesting access. As such, we only send YIELD_IDLE when:

- The GPU is granted to an access window and there are other access
  windows requesting GPU access.
- A new GPU_REQUEST was received and the currently active access window
  was the singular requesting AW.

[ ... ]

> diff --git a/drivers/gpu/drm/panthor/arbitration/panthor_arbitration_sched.c b/drivers/gpu/drm/panthor/arbitration/panthor_arbitration_sched.c
> index df0785a80442c..df58fcc56e375 100644
> --- a/drivers/gpu/drm/panthor/arbitration/panthor_arbitration_sched.c
> +++ b/drivers/gpu/drm/panthor/arbitration/panthor_arbitration_sched.c
> @@ -347,6 +347,10 @@ static int arb_sched_next_locked(struct panthor_arbitration_sched *sched)
>  
>  	arb_sched_grant_locked(sched);
>  
> +	if (sched->phase == ARB_SCHED_PHASE_GRANTED &&
> +	    !kfifo_is_empty(&sched->queue))
> +		panthor_arbitration_notify_yield_idle(to_adev(sched), aw_id);

[Severity: High]
Does this sequence cause back-to-back command writes that can trigger spurious
resets?

When multiple AWs are queued, arb_sched_next_locked() grants the GPU to one
AW, which writes the PART_CMD_OPEN_WINDOW command to the hardware register via
arb_sched_grant_locked(), and then immediately issues PART_CMD_YIELD_IDLE here
without waiting for the hardware to process the window state.

The immediate write to the same register can physically overwrite the
PART_CMD_OPEN_WINDOW command before the hardware processes it. If the hardware
instead rejects PART_CMD_YIELD_IDLE (for example, because it's still in the
PART_STATE_WINDOW_OPENING state), it raises PART_INVALID_COMMAND.

Because panthor_arbitration_notify_yield_idle() eventually calls yield_idle(),
which uses an untracked command write:

drivers/gpu/drm/panthor/arbitration/panthor_partition_control.c:yield_idle() {
	...
	partition_command_write_untracked(
		pc, AM_PART_SET_COMMAND(PART_CMD_YIELD_IDLE));
	...
}

The pc->last_cmd remains set to the previously tracked command
(PART_CMD_OPEN_WINDOW). The IRQ handler partition_handle_invalid_cmd() then
mistakenly believes the PART_CMD_OPEN_WINDOW command failed, sees the state is
not PART_STATE_RESET or fully open, and resets the scheduler.

Can we wait for the hardware to process PART_CMD_OPEN_WINDOW or for the window
state to settle before sending PART_CMD_YIELD_IDLE?

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