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 <[email protected]> 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/[email protected]?part=21
