Re: [PATCH v3 13/14] 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 1 potential
issue to consider:
- [Medium] drm/panfrost: runtime suspend race in panfrost_job_timedout()
  before reset
--

commit 3b989ea77e83f04bf915c182d8dabfa8f214f4fc
Author: Arun Iyengar <aiyengar1990@gmail.com>

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

This commit updates the Panfrost job timeout handler to soft-stop and resume
slots whose jobs are still active, falling back to a hard-stop after 5 seconds
and a full GPU reset only if the hard-stop fails or another reset is
requested.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 8235d182d8114..6757d0fbf63c9 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
[ ... ]
> @@ -664,18 +677,35 @@ 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)
>  {
>  	struct panfrost_job *evicted[NUM_JOB_SLOTS] = {};
> +	struct panfrost_job *hung[NUM_JOB_SLOTS] = {};
> +	bool cookie, idle, stop_failed = false, reset_gpu;
>  	unsigned int i, j;
> -	bool cookie, idle;
>  	int ret;
>  
>  	if (!atomic_read(&pfdev->reset.pending))
>  		return;
>  
> +	/* The stopped jobs drop their runtime PM references before they are
> +	 * resubmitted: keep the GPU powered in between.
> +	 */
> +	pm_runtime_get_noresume(pfdev->base.dev);
[ ... ]
> @@ -816,18 +920,46 @@ 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 runs a long job, not a hung one:
> +	 * soft-stop and resume it instead of resetting the GPU.
> +	 */
> +	js_status = job_read(pfdev, JS_STATUS(js));
> +	job_active = js_status == DRM_PANFROST_EXCEPTION_ACTIVE;
> +
> +	/* The job may have completed since the fence check above. */
> +	if (!job_active) {
> +		if (job_read(pfdev, JOB_INT_RAWSTAT) & JOB_INT_MASK_DONE(js))
> +			return DRM_GPU_SCHED_STAT_NO_HANG;
> +
> +		synchronize_irq(pfdev->js->irq);
> +		if (dma_fence_is_signaled(job->done_fence))
> +			return DRM_GPU_SCHED_STAT_NO_HANG;
> +	}
>  
> -	panfrost_core_dump(job);
> +	if (job_active) {
> +		dev_dbg(pfdev->base.dev, "job still active after %u ms, js=%d, head=0x%x, sched_job=%p: soft-stopping it to resume",
> +			JOB_TIMEOUT_MS, js, job_read(pfdev, JS_HEAD_LO(js)),
> +			sched_job);
> +	} else {
> +		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)),
> +			js_status,
> +			job_read(pfdev, JS_HEAD_LO(js)),
> +			job_read(pfdev, JS_TAIL_LO(js)),
> +			sched_job);
>  
> -	atomic_set(&pfdev->reset.pending, 1);
> -	panfrost_reset(pfdev, sched_job);
> +		panfrost_core_dump(job);
> +	}
> +
> +	/* Ask for a reset, or only for the stop if no reset is pending. */
> +	if (job_active)
> +		atomic_cmpxchg(&pfdev->reset.pending, 0,
> +			       PANFROST_RESET_STOP_ONLY);
> +	else
> +		atomic_set(&pfdev->reset.pending, 1);
> +	panfrost_reset(pfdev, sched_job, job_active);

[Severity: Medium]
Can the GPU runtime-autosuspend in panfrost_job_timedout() before
panfrost_reset() calls pm_runtime_get_noresume(), causing MMIO accesses on
a powered-off GPU?

CPU0 (timeout worker)             CPU1 (IRQ thread / PM worker)
panfrost_job_timedout()
  js_status = job_read(JS_STATUS)
  // reads ACTIVE
                                  panfrost_jm_irq_handler_thread()
                                    panfrost_jm_handle_done()
                                      dma_fence_signal_locked(done_fence)
                                        drm_sched_job_done_cb()
                                          // credit_count -> 0
                                      pm_runtime_put_autosuspend()
                                  ...
                                  // 50 ms autosuspend expires
                                  panfrost_device_runtime_suspend()
                                    panfrost_gpu_power_off()
  panfrost_reset()
    pm_runtime_get_noresume()
    job_write(JOB_INT_MASK, 0)    <-- external abort (GPU powered off)

Job interrupts remain enabled until panfrost_reset() masks JOB_INT_MASK,
and the completion callback on job->done_fence remains attached until
drm_sched_stop().  If the only in-flight job completes right after
JS_STATUS(js) is read as DRM_PANFROST_EXCEPTION_ACTIVE (or faults just
before, setting JOB_INT_MASK_ERR(js) rather than JOB_INT_MASK_DONE(js),
and the IRQ thread finishes while panfrost_core_dump() is copying BOs),
the IRQ handler drops sched->credit_count to 0 and releases the job's
runtime PM reference.  With panfrost_jm_is_idle() now true, runtime
autosuspend can power off the GPU and disable its clocks before
panfrost_reset() is reached, and pm_runtime_get_noresume() only increments
usage_count without resuming the device.

Acquiring the runtime PM reference at the start of panfrost_job_timedout()
(or using pm_runtime_resume_and_get()) before checking the fences and
reading registers would keep the GPU powered throughout the timeout and
reset path.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-panfrost-long-jobs-v3-0-80ae78d38f6e@gmail.com?part=13