Re: [PATCH 6/7] drm/panfrost: Soft-stop and resume a job that outlives the timeout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [High] drm/panfrost: jobs finishing during panfrost_reset() get
  -ECANCELED
- [High] drm/panfrost: cancelled _NEXT job marked DONE in
  panfrost_jm_handle_irq()
- [Medium] drm/panfrost: panfrost_reset() clears reset.pending without
  GPU reset
- [Medium] drm/panfrost: hung active jobs skip karma increase and core
  dump
- [Medium] drm/panfrost: panfrost_reset() fails active jobs on sibling
  slots
- [Medium] drm/panfrost: race in panfrost_job_timedout() causes false
  GPU reset
--

commit 459199c0f22ade204c094e9cf2b470689b79061c
Author: Arun Iyengar <aiyengar1990@gmail.com>

drm/panfrost: Soft-stop and resume a job that outlives the timeout

This commit changes the Panfrost scheduler timeout path to soft-stop active
job slots, wait up to 5 seconds for them to reach a task boundary, and
resume their jobs from JS_TAIL without resetting the GPU unless soft-stop
and hard-stop fail.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 81efa8a867890..e37dd92651188 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c

[ ... ]

> @@ -672,7 +686,7 @@ static bool panfrost_slots_idle(struct panfrost_device *pfdev)
>  
>  static void
>  panfrost_reset(struct panfrost_device *pfdev,
> -	       struct drm_sched_job *bad)
> +	       struct drm_sched_job *bad, bool job_active)
>  {
>  	unsigned int i, j;
>  	bool cookie, idle;
> @@ -681,6 +695,12 @@ panfrost_reset(struct panfrost_device *pfdev,
>  	if (!atomic_read(&pfdev->reset.pending))
>  		return;
>  
> +	/* The jobs we stop drop their runtime PM references before we submit
> +	 * them again. Keep the GPU powered in between: a suspend would power
> +	 * the L2 off and lose the dirty lines of a soft-stopped job.
> +	 */
> +	pm_runtime_get_noresume(pfdev->base.dev);
> +
>  	/* Stop the schedulers.
>  	 *
>  	 * FIXME: We temporarily get out of the dma_fence_signalling section
> @@ -699,7 +719,8 @@ panfrost_reset(struct panfrost_device *pfdev,
>  
>  	cookie = dma_fence_begin_signalling();
>  
> -	if (bad)
> +	/* A job that is merely long is not guilty of anything. */
> +	if (bad && !job_active)
>  		drm_sched_increase_karma(bad);

[Severity: Medium]
Does checking !job_active here and in panfrost_job_timedout() skip
drm_sched_increase_karma() and panfrost_core_dump() for shaders that are
genuinely hung in DRM_PANFROST_EXCEPTION_ACTIVE?

A shader stuck in an infinite loop still reports JS_STATUS as
DRM_PANFROST_EXCEPTION_ACTIVE when the 500 ms timeout fires, so job_active
starts out true.  If the job then fails the 5-second soft-stop and has to
be hard-stopped (or fails the hard-stop too and forces a GPU reset),
neither its karma nor a devcoredump is ever recorded.

>  	/* Mask job interrupts and synchronize to make sure we won't be
> @@ -714,43 +735,110 @@ panfrost_reset(struct panfrost_device *pfdev,
>  		job_write(pfdev, JS_COMMAND(i), JS_COMMAND_SOFT_STOP);
>  	}
>  
> -	/* Wait at most 10ms for soft-stops to complete. Every slot must have
> -	 * gone idle before we touch the queues: a job that is still stopping
> -	 * (or finishing) raises its interrupt later, when its entry in
> -	 * pfdev->jobs[] is gone or belongs to its own resubmission.
> +	/* Every slot must have gone idle before we touch the queues: a job
> +	 * that is still stopping (or finishing) raises its interrupt later,
> +	 * when its entry in pfdev->jobs[] is gone or belongs to its own
> +	 * resubmission, and the second start of that job then runs on a slot
> +	 * we have no job for.
> +	 *
> +	 * A slot that was idle or faulted gets 10 ms. A job that was still
> +	 * active gets the time the Arm driver gives it to reach a task
> +	 * boundary (JOB_SOFT_STOP_TIMEOUT_MS). If it has not stopped by then
> +	 * it is hard-stopped, which terminates it at once and fails it
> +	 * (TERMINATED, see panfrost_job_handle_err()), instead of resetting
> +	 * the GPU under every job on it; the GPU is reset only when the hard
> +	 * stop does not take effect either.
>  	 */
> -	ret = read_poll_timeout(panfrost_slots_idle, idle, idle, 10, 10000,
> +	ret = read_poll_timeout(panfrost_slots_idle, idle, idle,
> +				job_active ? 200 : 10,
> +				job_active ? JOB_SOFT_STOP_TIMEOUT_MS * 1000 : 10000,
>  				false, pfdev);

[Severity: High]
If an active job finishes its last tasks during this 5-second soft-stop
wait (raising JOB_INT_MASK_DONE rather than stopping with
DRM_PANFROST_EXCEPTION_STOPPED), won't its scheduler fence be signaled with
-ECANCELED?

drm_sched_stop() has already detached the completion callback from
job->done_fence and cleared s_job->s_fence->parent.  When
panfrost_jm_handle_irqs() then runs panfrost_jm_handle_done(), it sets
job->jc = 0, so panfrost_job_run() returns NULL during
drm_sched_resubmit_jobs():

drivers/gpu/drm/panfrost/panfrost_job.c:panfrost_job_run() {
	...
	if (!job->jc)
		return NULL;
}

When drm_sched_start(&pfdev->js->queue[i].sched, 0) runs afterwards,
s_job->s_fence->parent is still NULL, so drm_sched_start() completes the
job with errno ?: -ECANCELED, failing a job that succeeded on the GPU.

[ ... ]

>  	/* Handle the remaining interrupts before we reset. */
>  	panfrost_jm_handle_irqs(pfdev);

[Severity: High]
What happens to a second job queued in _NEXT (pfdev->jobs[j][1]) when
panfrost_reset() cancels it with JS_COMMAND_NOP and the running job in
pfdev->jobs[j][0] finishes with JOB_INT_MASK_DONE during the soft-stop
poll?

In panfrost_jm_handle_irq(), JOB_INT_MASK_DONE(j) dequeues the running job
into done[j][0] and shifts the cancelled _NEXT job into pfdev->jobs[j][0],
with failed[j] still NULL and both hardware subslots now inactive:

drivers/gpu/drm/panfrost/panfrost_job.c:panfrost_jm_handle_irq() {
	...
		if (failed[j]) {
			panfrost_job_handle_err(pfdev, failed[j], j);
		} else if (pfdev->jobs[j][0] && !(js_state & MK_JS_MASK(j))) {
			if (WARN_ON(!done[j][0]))
				done[j][0] = panfrost_dequeue_job(pfdev, j);
			else
				done[j][1] = panfrost_dequeue_job(pfdev, j);
		}

		for (i = 0; i < ARRAY_SIZE(done[0]) && done[j][i]; i++)
			panfrost_jm_handle_done(pfdev, done[j][i]);
}

Because that branch assumes _NEXT is only cancelled when failed[j] is set,
it dequeues the unstarted _NEXT job into done[j][1] and calls
panfrost_jm_handle_done() on it.  That zeroes its job->jc, so
drm_sched_resubmit_jobs() skips it and the job never runs on the hardware.

>  	/* Remaining interrupts have been handled, but we might still have
> -	 * stuck jobs. Let's make sure the PM counters stay balanced by
> +	 * stuck jobs, and jobs that were waiting in _NEXT when it was
> +	 * cancelled. Let's make sure the PM counters stay balanced by
>  	 * manually calling pm_runtime_put_noidle() and
> -	 * panfrost_devfreq_record_idle() for each stuck job.
> +	 * panfrost_devfreq_record_idle() for each of them.
>  	 * Let's also make sure the cycle counting register's refcnt is
> -	 * kept balanced to prevent it from running forever
> +	 * kept balanced to prevent it from running forever.
>  	 */
>  	spin_lock(&pfdev->js->job_lock);
>  	for (i = 0; i < NUM_JOB_SLOTS; i++) {
>  		for (j = 0; j < ARRAY_SIZE(pfdev->jobs[0]) && pfdev->jobs[i][j]; j++) {
> -			if (pfdev->jobs[i][j]->requirements & PANFROST_JD_REQ_CYCLE_COUNT ||
> -			    pfdev->jobs[i][j]->is_profiled)
> -				panfrost_cycle_counter_put(pfdev->jobs[i][j]->pfdev);
> +			struct panfrost_job *job = pfdev->jobs[i][j];
> +
> +			if (job->requirements & PANFROST_JD_REQ_CYCLE_COUNT ||
> +			    job->is_profiled)
> +				panfrost_cycle_counter_put(job->pfdev);
>  			pm_runtime_put_noidle(pfdev->base.dev);
>  			panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
> +
> +			/* The GPU reset wipes the address space assignments;
> +			 * without one, release the reference the submission
> +			 * took, or the address space can never be reassigned.
> +			 */
> +			if (job_active)
> +				panfrost_mmu_as_put(pfdev, job->mmu);
> +
> +			/* The job still running on a slot that neither stop
> +			 * took effect on is what forces the reset. Fail it,
> +			 * as the Arm driver fails every atom on the hardware
> +			 * at a reset: restarted from scratch it would hit the
> +			 * timeout and the reset again.
> +			 */
> +			if (!job_active && j == 0 &&
> +			    job_read(pfdev, JS_STATUS(i)) ==
> +			    DRM_PANFROST_EXCEPTION_ACTIVE)
> +				dma_fence_set_error(&job->base.s_fence->finished,
> +						    -ECANCELED);

[Severity: Medium]
Can this fail an innocent job on another slot when panfrost_reset() was
entered with job_active == false?

When panfrost_reset() is called from panfrost_reset_work() or from
panfrost_job_timedout() for a non-active slot, job_active is false on
entry, so read_poll_timeout() only gives the other slots 10 ms to reach a
task boundary and skips the hard-stop pass.  Any normal job on another
slot whose current task takes longer than 10 ms will still have
JS_STATUS(i) == DRM_PANFROST_EXCEPTION_ACTIVE here and get failed with
-ECANCELED instead of being restarted after the reset.

>  		}
>  	}
> -	pfdev->reset.count++;
> +	if (!job_active)
> +		pfdev->reset.count++;
>  	memset(pfdev->jobs, 0, sizeof(pfdev->jobs));
>  	spin_unlock(&pfdev->js->job_lock);
>  
> -	/* Proceed with reset now. */
> -	panfrost_device_reset(pfdev, false);
> +	/* Proceed with reset now, unless the slots were simply busy with long
> +	 * jobs that stopped at a task boundary (or a hard stop ended the one
> +	 * that did not). The stopped jobs will be resumed from JS_TAIL (see
> +	 * panfrost_job_handle_err()). The hardware keeps their resume state in
> +	 * the job header and the results of the tasks that already finished
> +	 * in the L2: a soft reset throws both away, and a job resumed after it
> +	 * completes with stale data in the buffers it had still to write and
> +	 * no error reported (panfrost_job_run() fails such a job instead).
> +	 */
> +	if (!job_active)
> +		panfrost_device_reset(pfdev, false);
>  
>  	/* GPU has been reset, we can clear the reset pending bit. */
>  	atomic_set(&pfdev->reset.pending, 0);

[Severity: Medium]
Can clearing pfdev->reset.pending when job_active is true drop a needed
hardware reset?

pfdev->reset.pending is also set by panfrost_device_schedule_reset() (for
example when wait_ready() times out on an MMU command) and by
panfrost_job_handle_err() when panfrost_exception_needs_reset() is true.
If either happens before or during a soft-stop-only panfrost_reset() (including
inside panfrost_jm_handle_irqs() above), skipping panfrost_device_reset()
while unconditionally clearing pfdev->reset.pending causes
panfrost_reset_work() to see !atomic_read(&pfdev->reset.pending) and return
without resetting the GPU.

[ ... ]

> @@ -807,18 +899,34 @@ static enum drm_gpu_sched_stat panfrost_job_timedout(struct drm_sched_job
>  		return DRM_GPU_SCHED_STAT_NO_HANG;
>  	}
>  
> -	dev_err(pfdev->base.dev, "gpu sched timeout, js=%d, config=0x%x, status=0x%x, head=0x%x, tail=0x%x, sched_job=%p",
> -		js,
> -		job_read(pfdev, JS_CONFIG(js)),
> -		job_read(pfdev, JS_STATUS(js)),
> -		job_read(pfdev, JS_HEAD_LO(js)),
> -		job_read(pfdev, JS_TAIL_LO(js)),
> -		sched_job);
> +	/*
> +	 * A slot that is still ACTIVE is not hung, it is running a job that
> +	 * takes longer than JOB_TIMEOUT_MS. Compute kernels legitimately do.
> +	 * Soft-stop such a job and resume it, the way the Arm driver
> +	 * time-slices jobs, instead of resetting the GPU under it; the reset
> +	 * stays for faults and for jobs that do not stop.
> +	 */
> +	js_status = job_read(pfdev, JS_STATUS(js));
> +	job_active = js_status == DRM_PANFROST_EXCEPTION_ACTIVE;

[Severity: Medium]
What if the job finishes on the hardware right after the
dma_fence_is_signaled(job->done_fence) check above and before this
JS_STATUS(js) read?

CPU0 (timeout work)               GPU / IRQ
panfrost_job_timedout()
  dma_fence_is_signaled() -> false
                                  job finishes on slot js
                                  JS_STATUS(js) becomes 0 (OK)
  js_status = job_read(JS_STATUS)
  job_active = false              <-- triggers full GPU reset

Because job interrupts are not masked until inside panfrost_reset(),
JS_STATUS(js) transitions from DRM_PANFROST_EXCEPTION_ACTIVE to 0 (OK) as
soon as the job completes.  job_active then evaluates to false, which logs
a timeout error with status=0x0, takes a devcoredump, increases the
completed job's karma, and performs a full GPU reset (also failing any
soft-stopped jobs on other slots by bumping pfdev->reset.count).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-panfrost-long-jobs-v1-0-24508939ae8d@gmail.com?part=6