[PATCH v2 00/11] drm/panfrost: Resume long jobs instead of resetting, and mapping error path fixes

[PATCH v2 00/11] drm/panfrost: Resume long jobs instead of resetting, and mapping error path fixes

[Arun Iyengar]

This series fixes problems found running the GLES, Vulkan and OpenCL
conformance tests with Mesa on an Orange Pi Zero2 (Allwinner H616,
Mali-G31 MP2, one shader core, 1 GB of RAM), and problems the Sashiko
review of v1 found in the code around them.

Patches 1-3 are fixes from the v1 review for problems that were already
there: the reset work touching the GPU while it is runtime suspended, a
CPU cache sync of a heap BO walking its sparse page array, and the GPU
fault handler reviving a mapping that is being freed.

Patches 4-6 fix the error paths of BO mapping under memory pressure: a
WARN on an ordinary allocation failure, pages dropped while the cached
sg table still maps them, PTEs left behind by a partial map_pages(),
and the folios of a heap chunk kept after a failed fault, which makes
the next fault in that chunk WARN.

Patches 7 and 8 fix two races in panfrost_reset(): a job waiting in
_NEXT that the reset cancels is taken for done when the running job
completes instead of stopping, and a job that completes while the GPU
is being reset is reported as cancelled. Patch 10 makes both windows
much wider.

Patches 9 and 10 change what happens when a job runs into the 500 ms job
timeout. On a small GPU OpenCL kernels and heavy fragment jobs
legitimately run longer, and resetting the GPU loses their work: the
OpenCL CTS Integer Ops long math and Thread Dimensions tests, the
KHR-GLES31 advanced blend mixedSequence test and several dEQP-VK
spirv_assembly fragment cases fail that way. Following the Arm driver's
job scheduling, a job whose slot is still active is soft-stopped and
resumed from JS_TAIL without a reset, hard-stopped if it does not reach
a task boundary within 5 s, and the GPU is reset only if the hard stop
does not take effect either. Patch 9 makes panfrost_reset() actually
wait for the soft-stops (JOB_INT_JS_STATE is a snapshot, not the live
state); patch 10 needs it, and it fixes the reset path on its own.

Patch 11 adds the BOs around an unhandled page fault to the fault
report, which told a texture over-read apart from a kernel problem.

A job that keeps reaching task boundaries is resumed after every
timeout and is never killed. If a limit on the total run time of a job
is wanted, I can add one.

While a job that outlived the timeout is being stopped, patch 10 keeps
the job interrupts masked and the schedulers of all slots stopped, as
panfrost_reset() does today around a reset, but now for as long as the
job needs to reach a task boundary, 5.5 s at worst (the Arm driver's
JS_HARD_STOP_TICKS_SS and JS_RESET_TICKS_SS). The slot of the long job
is busy with that job either way. What the wait costs is the other slot
(panfrost uses two, one for fragment jobs and one for vertex, tiler and
compute jobs): it starts no new job, and a job that finishes on it is
signalled only when the wait is over. In two runs of the tests below,
the time from stopping the schedulers to starting them again was at
most 85 ms for a compute job (median 0.3 ms, 206 timeouts) and at most
349 ms for a fragment job (6 timeouts). Stopping only the slot that
timed out would avoid that cost, but reset.pending, the interrupt mask
and the scheduler stop are all per device, so that is a larger rework
than I wanted in this series. Mainline resets the GPU at that point,
under the jobs on both slots. I can do the per-slot stop as a follow-up
if you want it.

If you would rather take the independent fixes (1-8, 11) first and look
at the soft-stop rework (9, 10) separately, I can split the series.

Testing: Orange Pi Zero2 (H616, Mali-G31), Mesa 26.2, with this series
backported to v7.3.3:
- OpenCL CTS Integer Ops quick_long_math/quick_ulong_math and Thread
  Dimensions quick_2d_explicit_local pass (they failed before); dEQP-VK
  spirv_assembly vec4 i8/u64 bit_field_insert count64 fragment cases
  pass in 5 s (timed out before); KHR-GLES31 blend_equation_advanced
  test_coherency.mixedSequence passes (failed before); 107 job timeouts
  in these tests, all soft-stopped and resumed, none resets the GPU;
- a never-ending OpenCL kernel is hard-stopped after 5 s, a devcoredump
  of it is taken, and the GPU keeps working;
- with a test-only switch that withholds the hard stop, the GPU is reset
  under that kernel instead: the job ends, a devcoredump of it is taken,
  and the GPU keeps working;
- the reset work, queued with a test-only switch the way the MMU code
  queues it while the GPU is runtime suspended, resumes the GPU before
  it resets it (patch 1), and the GPU keeps working;
- the reset work queued the same way five times while Integer Ops and
  Thread Dimensions run side by side: both pass, and the GPU keeps
  working. Most of these resets found the GPU suspended between two
  jobs; the trace does not show whether a job was on a slot at any of
  them;
- dEQP-GLES31 draw_indirect compute_interop large cases in memory
  cgroups of 450-600 MB: the processes get allocation errors or are
  OOM-killed, without a WARN, and the GPU keeps working;
- a forced page fault prints the fault BO report;
- PANFROST_SYNC_BO fails with -EINVAL on a heap BO and succeeds on a
  plain one;
- no regressions in dEQP-GLES3 draw/occlusion_query/shaders.loops (964
  cases), dEQP-GLES31 compute (195), KHR-GLES31 compute_shader (37, two
  not supported) and 9 OpenCL CTS groups.
The two test-only switches are module parameters on the test kernel,
not part of this series. The problems patches 1-3, 7 and 8 fix were not
reproduced. The fixed paths of patches 1 and 2 run in the tests above;
patches 3, 7 and 8, the larger-folio case of patch 6 and the races
patch 10 now handles rest on code review, as their windows cannot be
forced here.
Each patch builds without warnings with W=1 for arm64 and arm.

These patches were developed with Claude (Anthropic) as a coding assistant:
it investigated the problems, wrote the code and the commit messages. I
reviewed them, and they were tested on an Orange Pi Zero2 (Allwinner H616,
Mali-G31) as described above.

---
Changes in v2:
- New patches 1-3, 7 and 8, for problems the Sashiko review of v1 found
  in the code around the series (1, 2, 7, 8) or by analogy with a
  finding (3):
  https://lore.kernel.org/all/sashiko-outbox-206548@kernel.org/
  https://lore.kernel.org/all/sashiko-outbox-206561@kernel.org/
  https://lore.kernel.org/all/sashiko-outbox-206570@kernel.org/
  https://lore.kernel.org/all/sashiko-outbox-206581@kernel.org/
- Patch 6 (v1 3/7): clear the page array range the fault filled, whole
  folios, which differs from the 2 MB chunk for folios larger than 2 MB,
  and leave folios owned by a neighbouring chunk alone (Sashiko).
- Patch 10 (v1 6/7), after the Sashiko review: a job that does not stop
  within 5 s gets its karma raised and a devcoredump; a reset asked for
  while the slots are being stopped is done instead of dropped; jobs on
  other slots that only had the 10 ms of an ordinary reset are restarted
  as before instead of failed; a job that completes just as its timeout
  fires no longer triggers a reset.
- Patch 11 (v1 7/7): skip mappings whose last reference is gone instead
  of reviving them, drop the "mapped" flag, which said nothing useful,
  and describe what the report can tell apart (Sashiko).
- Dropped v1 4/7 ("Don't resume a soft-stopped job after a GPU reset"),
  and the comments of v1 6/7 that said the same. It assumed that a
  soft-stopped job leaves results in the L2 that a GPU reset then loses.
  I have no evidence of that: it was never observed here, and the Arm
  driver resumes soft-stopped atoms without such a check (its comment in
  kbase_gpu_complete_hw() says the cache is not flushed after a failed
  job chain only on a T60x or with BASE_JD_REQ_SKIP_CACHE_END).
- Cover letter: say which paths the tests did not run, and what stopping
  a long job costs the other slot, with the times measured here; tests
  of the GPU reset path added.
- Patches renumbered: v1 1-3 are now 4-6, v1 5-7 are now 9-11.
- Rebased onto the current drm-misc-next (no conflicts, no code change).
- Link to v1: https://lore.kernel.org/r/20261110-panfrost-long-jobs-v1-0-24508939ae8d@gmail.com

---
Arun Iyengar (11):
      drm/panfrost: Power the GPU up for a reset queued from the MMU code
      drm/panfrost: Refuse to sync the CPU caches of a heap BO
      drm/panfrost: Don't take a reference on a mapping that is being freed
      drm/panfrost: Don't WARN when a GEM mapping cannot get its pages
      drm/panfrost: Unwind a failed GPU mapping without dropping its pages
      drm/panfrost: Give back the pages of a heap chunk that failed to map
      drm/panfrost: Don't signal a job cancelled from _NEXT at a reset as done
      drm/panfrost: Don't cancel a job that completed during a reset
      drm/panfrost: Wait for the soft-stopped slots with the live slot status
      drm/panfrost: Soft-stop and resume a job that outlives the timeout
      drm/panfrost: Name the BOs around an unhandled page fault

 drivers/gpu/drm/panfrost/panfrost_gem.c |   8 +
 drivers/gpu/drm/panfrost/panfrost_job.c | 316 +++++++++++++++++++++++++++-----
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 171 +++++++++++++++--
 3 files changed, 434 insertions(+), 61 deletions(-)
---
base-commit: cb0a63f0c96e8ecb90199eeb99cc2a7fcfa703d4
change-id: 20261110-panfrost-long-jobs-1643287a2ac1

Best regards,
--  
Arun Iyengar <aiyengar1990@gmail.com>

[PATCH v2 00/11] drm/panfrost: Resume long jobs instead of resetting, and mapping error path fixes

[Arun Iyengar via B4 Relay]

This series fixes problems found running the GLES, Vulkan and OpenCL
conformance tests with Mesa on an Orange Pi Zero2 (Allwinner H616,
Mali-G31 MP2, one shader core, 1 GB of RAM), and problems the Sashiko
review of v1 found in the code around them.

Patches 1-3 are fixes from the v1 review for problems that were already
there: the reset work touching the GPU while it is runtime suspended, a
CPU cache sync of a heap BO walking its sparse page array, and the GPU
fault handler reviving a mapping that is being freed.

Patches 4-6 fix the error paths of BO mapping under memory pressure: a
WARN on an ordinary allocation failure, pages dropped while the cached
sg table still maps them, PTEs left behind by a partial map_pages(),
and the folios of a heap chunk kept after a failed fault, which makes
the next fault in that chunk WARN.

Patches 7 and 8 fix two races in panfrost_reset(): a job waiting in
_NEXT that the reset cancels is taken for done when the running job
completes instead of stopping, and a job that completes while the GPU
is being reset is reported as cancelled. Patch 10 makes both windows
much wider.

Patches 9 and 10 change what happens when a job runs into the 500 ms job
timeout. On a small GPU OpenCL kernels and heavy fragment jobs
legitimately run longer, and resetting the GPU loses their work: the
OpenCL CTS Integer Ops long math and Thread Dimensions tests, the
KHR-GLES31 advanced blend mixedSequence test and several dEQP-VK
spirv_assembly fragment cases fail that way. Following the Arm driver's
job scheduling, a job whose slot is still active is soft-stopped and
resumed from JS_TAIL without a reset, hard-stopped if it does not reach
a task boundary within 5 s, and the GPU is reset only if the hard stop
does not take effect either. Patch 9 makes panfrost_reset() actually
wait for the soft-stops (JOB_INT_JS_STATE is a snapshot, not the live
state); patch 10 needs it, and it fixes the reset path on its own.

Patch 11 adds the BOs around an unhandled page fault to the fault
report, which told a texture over-read apart from a kernel problem.

A job that keeps reaching task boundaries is resumed after every
timeout and is never killed. If a limit on the total run time of a job
is wanted, I can add one.

While a job that outlived the timeout is being stopped, patch 10 keeps
the job interrupts masked and the schedulers of all slots stopped, as
panfrost_reset() does today around a reset, but now for as long as the
job needs to reach a task boundary, 5.5 s at worst (the Arm driver's
JS_HARD_STOP_TICKS_SS and JS_RESET_TICKS_SS). The slot of the long job
is busy with that job either way. What the wait costs is the other slot
(panfrost uses two, one for fragment jobs and one for vertex, tiler and
compute jobs): it starts no new job, and a job that finishes on it is
signalled only when the wait is over. In two runs of the tests below,
the time from stopping the schedulers to starting them again was at
most 85 ms for a compute job (median 0.3 ms, 206 timeouts) and at most
349 ms for a fragment job (6 timeouts). Stopping only the slot that
timed out would avoid that cost, but reset.pending, the interrupt mask
and the scheduler stop are all per device, so that is a larger rework
than I wanted in this series. Mainline resets the GPU at that point,
under the jobs on both slots. I can do the per-slot stop as a follow-up
if you want it.

If you would rather take the independent fixes (1-8, 11) first and look
at the soft-stop rework (9, 10) separately, I can split the series.

Testing: Orange Pi Zero2 (H616, Mali-G31), Mesa 26.2, with this series
backported to v7.3.3:
- OpenCL CTS Integer Ops quick_long_math/quick_ulong_math and Thread
  Dimensions quick_2d_explicit_local pass (they failed before); dEQP-VK
  spirv_assembly vec4 i8/u64 bit_field_insert count64 fragment cases
  pass in 5 s (timed out before); KHR-GLES31 blend_equation_advanced
  test_coherency.mixedSequence passes (failed before); 107 job timeouts
  in these tests, all soft-stopped and resumed, none resets the GPU;
- a never-ending OpenCL kernel is hard-stopped after 5 s, a devcoredump
  of it is taken, and the GPU keeps working;
- with a test-only switch that withholds the hard stop, the GPU is reset
  under that kernel instead: the job ends, a devcoredump of it is taken,
  and the GPU keeps working;
- the reset work, queued with a test-only switch the way the MMU code
  queues it while the GPU is runtime suspended, resumes the GPU before
  it resets it (patch 1), and the GPU keeps working;
- the reset work queued the same way five times while Integer Ops and
  Thread Dimensions run side by side: both pass, and the GPU keeps
  working. Most of these resets found the GPU suspended between two
  jobs; the trace does not show whether a job was on a slot at any of
  them;
- dEQP-GLES31 draw_indirect compute_interop large cases in memory
  cgroups of 450-600 MB: the processes get allocation errors or are
  OOM-killed, without a WARN, and the GPU keeps working;
- a forced page fault prints the fault BO report;
- PANFROST_SYNC_BO fails with -EINVAL on a heap BO and succeeds on a
  plain one;
- no regressions in dEQP-GLES3 draw/occlusion_query/shaders.loops (964
  cases), dEQP-GLES31 compute (195), KHR-GLES31 compute_shader (37, two
  not supported) and 9 OpenCL CTS groups.
The two test-only switches are module parameters on the test kernel,
not part of this series. The problems patches 1-3, 7 and 8 fix were not
reproduced. The fixed paths of patches 1 and 2 run in the tests above;
patches 3, 7 and 8, the larger-folio case of patch 6 and the races
patch 10 now handles rest on code review, as their windows cannot be
forced here.
Each patch builds without warnings with W=1 for arm64 and arm.

These patches were developed with Claude (Anthropic) as a coding assistant:
it investigated the problems, wrote the code and the commit messages. I
reviewed them, and they were tested on an Orange Pi Zero2 (Allwinner H616,
Mali-G31) as described above.

---
Changes in v2:
- New patches 1-3, 7 and 8, for problems the Sashiko review of v1 found
  in the code around the series (1, 2, 7, 8) or by analogy with a
  finding (3):
  https://lore.kernel.org/all/sashiko-outbox-206548@kernel.org/
  https://lore.kernel.org/all/sashiko-outbox-206561@kernel.org/
  https://lore.kernel.org/all/sashiko-outbox-206570@kernel.org/
  https://lore.kernel.org/all/sashiko-outbox-206581@kernel.org/
- Patch 6 (v1 3/7): clear the page array range the fault filled, whole
  folios, which differs from the 2 MB chunk for folios larger than 2 MB,
  and leave folios owned by a neighbouring chunk alone (Sashiko).
- Patch 10 (v1 6/7), after the Sashiko review: a job that does not stop
  within 5 s gets its karma raised and a devcoredump; a reset asked for
  while the slots are being stopped is done instead of dropped; jobs on
  other slots that only had the 10 ms of an ordinary reset are restarted
  as before instead of failed; a job that completes just as its timeout
  fires no longer triggers a reset.
- Patch 11 (v1 7/7): skip mappings whose last reference is gone instead
  of reviving them, drop the "mapped" flag, which said nothing useful,
  and describe what the report can tell apart (Sashiko).
- Dropped v1 4/7 ("Don't resume a soft-stopped job after a GPU reset"),
  and the comments of v1 6/7 that said the same. It assumed that a
  soft-stopped job leaves results in the L2 that a GPU reset then loses.
  I have no evidence of that: it was never observed here, and the Arm
  driver resumes soft-stopped atoms without such a check (its comment in
  kbase_gpu_complete_hw() says the cache is not flushed after a failed
  job chain only on a T60x or with BASE_JD_REQ_SKIP_CACHE_END).
- Cover letter: say which paths the tests did not run, and what stopping
  a long job costs the other slot, with the times measured here; tests
  of the GPU reset path added.
- Patches renumbered: v1 1-3 are now 4-6, v1 5-7 are now 9-11.
- Rebased onto the current drm-misc-next (no conflicts, no code change).
- Link to v1: https://lore.kernel.org/r/20261110-panfrost-long-jobs-v1-0-24508939ae8d@gmail.com

---
Arun Iyengar (11):
      drm/panfrost: Power the GPU up for a reset queued from the MMU code
      drm/panfrost: Refuse to sync the CPU caches of a heap BO
      drm/panfrost: Don't take a reference on a mapping that is being freed
      drm/panfrost: Don't WARN when a GEM mapping cannot get its pages
      drm/panfrost: Unwind a failed GPU mapping without dropping its pages
      drm/panfrost: Give back the pages of a heap chunk that failed to map
      drm/panfrost: Don't signal a job cancelled from _NEXT at a reset as done
      drm/panfrost: Don't cancel a job that completed during a reset
      drm/panfrost: Wait for the soft-stopped slots with the live slot status
      drm/panfrost: Soft-stop and resume a job that outlives the timeout
      drm/panfrost: Name the BOs around an unhandled page fault

 drivers/gpu/drm/panfrost/panfrost_gem.c |   8 +
 drivers/gpu/drm/panfrost/panfrost_job.c | 316 +++++++++++++++++++++++++++-----
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 171 +++++++++++++++--
 3 files changed, 434 insertions(+), 61 deletions(-)
---
base-commit: cb0a63f0c96e8ecb90199eeb99cc2a7fcfa703d4
change-id: 20261110-panfrost-long-jobs-1643287a2ac1

Best regards,
--  
Arun Iyengar <aiyengar1990@gmail.com>

[PATCH v2 01/11] drm/panfrost: Power the GPU up for a reset queued from the MMU code

[Arun Iyengar]

panfrost_reset_work() calls panfrost_reset() without a runtime PM
reference. The work is queued by panfrost_device_schedule_reset(), which
wait_ready() calls when the AS_ACTIVE bit is stuck, from MMU operations
whose callers drop their own runtime PM reference right after. The work
shares the ordered reset workqueue with the scheduler timeout handlers, so
it can run well after it was queued. If the GPU has gone idle and been
runtime suspended in between, panfrost_reset() writes JOB_INT_MASK and the
job slot registers of a powered-off GPU, which is a synchronous external
abort on arm64.

Take a runtime PM reference in the work, resuming the GPU if needed, and
drop it after the reset. If the GPU cannot be resumed (runtime PM is
disabled during system sleep), no job is running on it and
panfrost_device_runtime_resume() resets it on the way back up, so just
clear the pending flag. Resuming instead of skipping the reset when the
GPU is suspended keeps jobs that a concurrent resume queued behind the
pending flag from waiting for a reset that never comes.

Found by code review only (Sashiko); not reproduced.

Fixes: f9ab9c66f080 ("drm/panfrost: Reset the GPU when the AS_ACTIVE bit is stuck")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/sashiko-outbox-206548@kernel.org/
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_job.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 3a46dd61c..0c2fda80c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -810,7 +810,21 @@ static void panfrost_reset_work(struct work_struct *work)
 	struct panfrost_device *pfdev;
 
 	pfdev = container_of(work, struct panfrost_device, reset.work);
+
+	/* The work can run long after it was queued, when the GPU has gone
+	 * idle and been runtime suspended. Power it up for the reset: its
+	 * registers are not accessible while it is suspended. If it cannot
+	 * be resumed (runtime PM is disabled during system sleep), there is
+	 * no job on it, and panfrost_device_runtime_resume() resets it when
+	 * it comes back up.
+	 */
+	if (pm_runtime_resume_and_get(pfdev->base.dev)) {
+		atomic_set(&pfdev->reset.pending, 0);
+		return;
+	}
+
 	panfrost_reset(pfdev, NULL);
+	pm_runtime_put_autosuspend(pfdev->base.dev);
 }
 
 static const struct drm_sched_backend_ops panfrost_sched_ops = {

-- 
2.55.0

[PATCH v2 01/11] drm/panfrost: Power the GPU up for a reset queued from the MMU code

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

panfrost_reset_work() calls panfrost_reset() without a runtime PM
reference. The work is queued by panfrost_device_schedule_reset(), which
wait_ready() calls when the AS_ACTIVE bit is stuck, from MMU operations
whose callers drop their own runtime PM reference right after. The work
shares the ordered reset workqueue with the scheduler timeout handlers, so
it can run well after it was queued. If the GPU has gone idle and been
runtime suspended in between, panfrost_reset() writes JOB_INT_MASK and the
job slot registers of a powered-off GPU, which is a synchronous external
abort on arm64.

Take a runtime PM reference in the work, resuming the GPU if needed, and
drop it after the reset. If the GPU cannot be resumed (runtime PM is
disabled during system sleep), no job is running on it and
panfrost_device_runtime_resume() resets it on the way back up, so just
clear the pending flag. Resuming instead of skipping the reset when the
GPU is suspended keeps jobs that a concurrent resume queued behind the
pending flag from waiting for a reset that never comes.

Found by code review only (Sashiko); not reproduced.

Fixes: f9ab9c66f080 ("drm/panfrost: Reset the GPU when the AS_ACTIVE bit is stuck")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/sashiko-outbox-206548@kernel.org/
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_job.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 3a46dd61c..0c2fda80c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -810,7 +810,21 @@ static void panfrost_reset_work(struct work_struct *work)
 	struct panfrost_device *pfdev;
 
 	pfdev = container_of(work, struct panfrost_device, reset.work);
+
+	/* The work can run long after it was queued, when the GPU has gone
+	 * idle and been runtime suspended. Power it up for the reset: its
+	 * registers are not accessible while it is suspended. If it cannot
+	 * be resumed (runtime PM is disabled during system sleep), there is
+	 * no job on it, and panfrost_device_runtime_resume() resets it when
+	 * it comes back up.
+	 */
+	if (pm_runtime_resume_and_get(pfdev->base.dev)) {
+		atomic_set(&pfdev->reset.pending, 0);
+		return;
+	}
+
 	panfrost_reset(pfdev, NULL);
+	pm_runtime_put_autosuspend(pfdev->base.dev);
 }
 
 static const struct drm_sched_backend_ops panfrost_sched_ops = {

-- 
2.55.0

Re: [PATCH v2 01/11] drm/panfrost: Power the GPU up for a reset queued from the MMU code

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/panfrost: race with system suspend in
  panfrost_reset_work()
--

commit 4c2188106dfefcf238af31be2bfb9e60d51e0307
Author: Arun Iyengar <aiyengar1990@gmail.com>

drm/panfrost: Power the GPU up for a reset queued from the MMU code

This commit acquires a runtime PM reference in panfrost_reset_work() before
calling panfrost_reset(), and clears reset.pending if
pm_runtime_resume_and_get() fails.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 3a46dd61cf87f..0c2fda80c51ef 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -810,7 +810,21 @@ static void panfrost_reset_work(struct work_struct *work)
>  	struct panfrost_device *pfdev;
>  
>  	pfdev = container_of(work, struct panfrost_device, reset.work);
> +
> +	/* The work can run long after it was queued, when the GPU has gone
> +	 * idle and been runtime suspended. Power it up for the reset: its
> +	 * registers are not accessible while it is suspended. If it cannot
> +	 * be resumed (runtime PM is disabled during system sleep), there is
> +	 * no job on it, and panfrost_device_runtime_resume() resets it when
> +	 * it comes back up.
> +	 */
> +	if (pm_runtime_resume_and_get(pfdev->base.dev)) {
> +		atomic_set(&pfdev->reset.pending, 0);
> +		return;
> +	}
> +
>  	panfrost_reset(pfdev, NULL);
> +	pm_runtime_put_autosuspend(pfdev->base.dev);
>  }

[Severity: Medium]
Can panfrost_reset_work() still race with panfrost_device_suspend() and
access GPU registers while the GPU is powered off?

pfdev->reset.wq is allocated without WQ_FREEZABLE and
panfrost_device_suspend() does not flush or cancel pfdev->reset.work.
Holding a runtime PM usage reference does not block
pm_runtime_force_suspend(), which ignores usage_count and calls
panfrost_device_runtime_suspend() directly:

kworker                               CPU0
panfrost_reset_work()
  pm_runtime_resume_and_get()
                                      panfrost_device_suspend()
                                        pm_runtime_force_suspend()
                                          panfrost_device_runtime_suspend()
                                            panfrost_jm_is_idle() -> true
                                            panfrost_gpu_power_off()
                                        panfrost_clks_disable()
  panfrost_reset()
    job_write(pfdev, JOB_INT_MASK, 0) <-- external abort (GPU powered off)

In addition, if the GPU is RPM_ACTIVE when pm_runtime_force_suspend()
disables runtime PM and usage_count > 1 (for example if
__pm_runtime_resume() incremented usage_count while
panfrost_device_runtime_suspend() was running, or perfcnt is enabled),
pm_runtime_force_suspend() sets needs_force_resume and leaves both
runtime_status and last_status set to RPM_ACTIVE.  rpm_resume() then
returns 1 even with disable_depth > 0, so pm_runtime_resume_and_get()
returns 0 while the GPU is suspended.

Finally, when pm_runtime_resume_and_get() fails, clearing
pfdev->reset.pending is not serialized against panfrost_device_resume().
If the device resumes concurrently and panfrost_job_hw_submit() observes
reset.pending == 1, it places the job in pfdev->jobs without writing
JS_COMMAND_START, and panfrost_reset_work() then clears reset.pending and
returns without resetting or starting that job.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-panfrost-long-jobs-v2-0-a06830da3d99@gmail.com?part=1

[PATCH v2 02/11] drm/panfrost: Refuse to sync the CPU caches of a heap BO

[Arun Iyengar]

panfrost_gem_sync() calls drm_gem_shmem_get_pages_sgt() on any BO that is
not write-combined. Heap BOs are write-combined only on non-coherent
GPUs; on a coherent one, a PANFROST_SYNC_BO on a heap BO reaches
drm_gem_shmem_get_pages_sgt() with bo->base.pages being the sparse page
array of the GPU fault handler, and drm_prime_pages_to_sg() dereferences
the NULL entries of the chunks not faulted in yet. On a heap BO with no
chunk faulted in yet, it instead pins the whole heap and leaves
bo->base.pages set without bo->sgts, and the next GPU fault on the heap
dereferences the NULL bo->sgts.

Heap BOs are not CPU-mappable (panfrost_ioctl_mmap_bo() refuses them, and
panfrost_gem_create() refuses PANFROST_BO_WB_MMAP for them), so there is
nothing to sync: return -EINVAL, as for imported BOs.

Found by code review (Sashiko). Not tested on a coherent GPU: the
Mali-G31 here is not coherent, so a heap BO is write-combined and the
sync returned early before.

Fixes: 7be45f548976 ("drm/panfrost: Add a PANFROST_SYNC_BO ioctl")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/sashiko-outbox-206561@kernel.org/
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_gem.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 518f24549..5d3925abd 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -569,6 +569,14 @@ panfrost_gem_sync(struct drm_gem_object *obj, u32 type, u32 offset, u32 size)
 	if (drm_gem_is_imported(&shmem->base))
 		return -EINVAL;
 
+	/* Heap BOs are not CPU-mappable, and their pages are populated by the
+	 * GPU fault handler, one 2 MB chunk at a time, with a page array and
+	 * sg tables of its own: drm_gem_shmem_get_pages_sgt() must not touch
+	 * them.
+	 */
+	if (bo->is_heap)
+		return -EINVAL;
+
 	switch (type) {
 	case PANFROST_BO_SYNC_CPU_CACHE_FLUSH:
 	case PANFROST_BO_SYNC_CPU_CACHE_FLUSH_AND_INVALIDATE:

-- 
2.55.0

[PATCH v2 02/11] drm/panfrost: Refuse to sync the CPU caches of a heap BO

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

panfrost_gem_sync() calls drm_gem_shmem_get_pages_sgt() on any BO that is
not write-combined. Heap BOs are write-combined only on non-coherent
GPUs; on a coherent one, a PANFROST_SYNC_BO on a heap BO reaches
drm_gem_shmem_get_pages_sgt() with bo->base.pages being the sparse page
array of the GPU fault handler, and drm_prime_pages_to_sg() dereferences
the NULL entries of the chunks not faulted in yet. On a heap BO with no
chunk faulted in yet, it instead pins the whole heap and leaves
bo->base.pages set without bo->sgts, and the next GPU fault on the heap
dereferences the NULL bo->sgts.

Heap BOs are not CPU-mappable (panfrost_ioctl_mmap_bo() refuses them, and
panfrost_gem_create() refuses PANFROST_BO_WB_MMAP for them), so there is
nothing to sync: return -EINVAL, as for imported BOs.

Found by code review (Sashiko). Not tested on a coherent GPU: the
Mali-G31 here is not coherent, so a heap BO is write-combined and the
sync returned early before.

Fixes: 7be45f548976 ("drm/panfrost: Add a PANFROST_SYNC_BO ioctl")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/sashiko-outbox-206561@kernel.org/
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_gem.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 518f24549..5d3925abd 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -569,6 +569,14 @@ panfrost_gem_sync(struct drm_gem_object *obj, u32 type, u32 offset, u32 size)
 	if (drm_gem_is_imported(&shmem->base))
 		return -EINVAL;
 
+	/* Heap BOs are not CPU-mappable, and their pages are populated by the
+	 * GPU fault handler, one 2 MB chunk at a time, with a page array and
+	 * sg tables of its own: drm_gem_shmem_get_pages_sgt() must not touch
+	 * them.
+	 */
+	if (bo->is_heap)
+		return -EINVAL;
+
 	switch (type) {
 	case PANFROST_BO_SYNC_CPU_CACHE_FLUSH:
 	case PANFROST_BO_SYNC_CPU_CACHE_FLUSH_AND_INVALIDATE:

-- 
2.55.0

[PATCH v2 03/11] drm/panfrost: Don't take a reference on a mapping that is being freed

[Arun Iyengar]

addr_to_mapping() looks up the mapping that contains a faulting GPU
address in the address space's drm_mm and takes a reference on it with
kref_get(). But panfrost_gem_mapping_put() drops the last reference
before panfrost_gem_mapping_release() -> panfrost_gem_teardown_mapping()
unmaps the BO and removes its node from the drm_mm under mm_lock, so the
lookup can find a mapping whose refcount is already zero. kref_get()
then revives it (refcount_t warns about the addition on zero), the
release goes on and frees the mapping and drops the BO, and the fault
handler maps pages into a BO that is being freed.

This takes a GPU fault on a heap BO's address while the last handle and
the last job reference to that BO go away. Jobs hold a reference on the
mappings of the BOs they list, so it takes a job that uses a heap BO it
does not list; userspace on the render node can submit that.

Use kref_get_unless_zero() and treat a mapping that is being freed as
absent: the fault is then reported as unhandled.

Found by code review: the same pattern in new code of this series was
reported by Sashiko. Not reproduced.

Fixes: bdefca2d8dc0 ("drm/panfrost: Add the panfrost_gem_mapping concept")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/all/sashiko-outbox-206570@kernel.org/
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index faa62643c..eaf94d47d 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -576,7 +576,13 @@ addr_to_mapping(struct panfrost_device *pfdev, int as, u64 addr)
 		    offset < (node->start + node->size)) {
 			mapping = drm_mm_node_to_panfrost_mapping(node);
 
-			kref_get(&mapping->refcount);
+			/* The last reference to a mapping is dropped before
+			 * panfrost_gem_teardown_mapping() takes its node out
+			 * of the address space: don't revive one that is
+			 * being freed.
+			 */
+			if (!kref_get_unless_zero(&mapping->refcount))
+				mapping = NULL;
 			break;
 		}
 	}

-- 
2.55.0

[PATCH v2 03/11] drm/panfrost: Don't take a reference on a mapping that is being freed

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

addr_to_mapping() looks up the mapping that contains a faulting GPU
address in the address space's drm_mm and takes a reference on it with
kref_get(). But panfrost_gem_mapping_put() drops the last reference
before panfrost_gem_mapping_release() -> panfrost_gem_teardown_mapping()
unmaps the BO and removes its node from the drm_mm under mm_lock, so the
lookup can find a mapping whose refcount is already zero. kref_get()
then revives it (refcount_t warns about the addition on zero), the
release goes on and frees the mapping and drops the BO, and the fault
handler maps pages into a BO that is being freed.

This takes a GPU fault on a heap BO's address while the last handle and
the last job reference to that BO go away. Jobs hold a reference on the
mappings of the BOs they list, so it takes a job that uses a heap BO it
does not list; userspace on the render node can submit that.

Use kref_get_unless_zero() and treat a mapping that is being freed as
absent: the fault is then reported as unhandled.

Found by code review: the same pattern in new code of this series was
reported by Sashiko. Not reproduced.

Fixes: bdefca2d8dc0 ("drm/panfrost: Add the panfrost_gem_mapping concept")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/all/sashiko-outbox-206570@kernel.org/
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index faa62643c..eaf94d47d 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -576,7 +576,13 @@ addr_to_mapping(struct panfrost_device *pfdev, int as, u64 addr)
 		    offset < (node->start + node->size)) {
 			mapping = drm_mm_node_to_panfrost_mapping(node);
 
-			kref_get(&mapping->refcount);
+			/* The last reference to a mapping is dropped before
+			 * panfrost_gem_teardown_mapping() takes its node out
+			 * of the address space: don't revive one that is
+			 * being freed.
+			 */
+			if (!kref_get_unless_zero(&mapping->refcount))
+				mapping = NULL;
 			break;
 		}
 	}

-- 
2.55.0

[PATCH v2 04/11] drm/panfrost: Don't WARN when a GEM mapping cannot get its pages

[Arun Iyengar]

panfrost_mmu_map() gets the pages of a BO with
drm_gem_shmem_get_pages_sgt(), which fails with -ENOMEM when shmem
cannot allocate them, for instance when the caller's memory cgroup is at
its limit, or with -EINTR when the caller is being killed. Those are
ordinary failures that reach userspace as an error (Mesa reports
"DRM_IOCTL_PANFROST_CREATE_BO failed (err=12)"), not driver bugs, but
the WARN_ON() turns each of them into a kernel warning with a backtrace:

  WARNING: drivers/gpu/drm/panfrost/panfrost_mmu.c:478 at panfrost_mmu_map+0xcc

dEQP-GLES31.functional.draw_indirect.compute_interop.large.* run in a
600 MB memory cgroup on a 1 GB Mali-G31 board prints it five times.

Return the error without the warning. No pages are held at this point,
so there is nothing to undo.

Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
Cc: stable@vger.kernel.org
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index eaf94d47d..971552c42 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -480,7 +480,7 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
 		prot |= IOMMU_NOEXEC;
 
 	sgt = drm_gem_shmem_get_pages_sgt(shmem);
-	if (WARN_ON(IS_ERR(sgt)))
+	if (IS_ERR(sgt))
 		return PTR_ERR(sgt);
 
 	ret = mmu_map_sg(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT,

-- 
2.55.0

[PATCH v2 04/11] drm/panfrost: Don't WARN when a GEM mapping cannot get its pages

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

panfrost_mmu_map() gets the pages of a BO with
drm_gem_shmem_get_pages_sgt(), which fails with -ENOMEM when shmem
cannot allocate them, for instance when the caller's memory cgroup is at
its limit, or with -EINTR when the caller is being killed. Those are
ordinary failures that reach userspace as an error (Mesa reports
"DRM_IOCTL_PANFROST_CREATE_BO failed (err=12)"), not driver bugs, but
the WARN_ON() turns each of them into a kernel warning with a backtrace:

  WARNING: drivers/gpu/drm/panfrost/panfrost_mmu.c:478 at panfrost_mmu_map+0xcc

dEQP-GLES31.functional.draw_indirect.compute_interop.large.* run in a
600 MB memory cgroup on a 1 GB Mali-G31 board prints it five times.

Return the error without the warning. No pages are held at this point,
so there is nothing to undo.

Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
Cc: stable@vger.kernel.org
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index eaf94d47d..971552c42 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -480,7 +480,7 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
 		prot |= IOMMU_NOEXEC;
 
 	sgt = drm_gem_shmem_get_pages_sgt(shmem);
-	if (WARN_ON(IS_ERR(sgt)))
+	if (IS_ERR(sgt))
 		return PTR_ERR(sgt);
 
 	ret = mmu_map_sg(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT,

-- 
2.55.0

[PATCH v2 05/11] drm/panfrost: Unwind a failed GPU mapping without dropping its pages

[Arun Iyengar]

Two error paths of a BO mapping leave the wrong state behind when an
allocation fails part-way (memory cgroup at its limit, io-pgtable
running out of memory):

- When mmu_map_sg() fails, panfrost_mmu_map() calls
  drm_gem_shmem_put_pages_locked(). The only page reference
  drm_gem_shmem_get_pages_sgt() took belongs to the sg table cached in
  shmem->sgt, which the object releases when it is freed, so this drops
  pages that shmem->sgt still maps (for an imported BO it underflows
  pages_use_count). It also runs without the reservation lock that
  helper asserts. Keep the pages and just return the error.

- When ops->map_pages() fails, mmu_map_sg() unmaps what the earlier
  calls mapped, but not the part the failing call itself had mapped
  before it failed (map_pages() reports it in @mapped). Those PTEs stay
  behind in a VA range that drm_mm hands out again, and the next mapping
  there finds them. Unmap that part too, and flush the unmapped range.

Fixes: 4da352cf8fb1 ("drm/panfrost: Handle page mapping failure")
Cc: stable@vger.kernel.org
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 971552c42..35fb86b6b 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -442,8 +442,11 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
 
 			ret = ops->map_pages(ops, iova, paddr, pgsize, pgcount, prot,
 				       GFP_KERNEL, &mapped);
-			if (ret)
+			if (ret) {
+				/* Unwind the pages this call mapped too. */
+				total_mapped += mapped;
 				goto err_unmap_pages;
+			}
 
 			/* Don't get stuck if things have gone wrong */
 			mapped = max(mapped, pgsize);
@@ -459,7 +462,10 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
 	return 0;
 
 err_unmap_pages:
-	mmu_unmap_range(mmu, start_iova, total_mapped);
+	if (total_mapped) {
+		mmu_unmap_range(mmu, start_iova, total_mapped);
+		panfrost_mmu_flush_range(pfdev, mmu, start_iova, total_mapped);
+	}
 	return ret;
 }
 
@@ -483,18 +489,20 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
 	if (IS_ERR(sgt))
 		return PTR_ERR(sgt);
 
+	/*
+	 * On failure keep the pages: the sg table cached in the shmem object
+	 * owns the only page reference drm_gem_shmem_get_pages_sgt() took,
+	 * and the object frees both. Dropping it here (without the reservation
+	 * lock) released pages that shmem->sgt still maps.
+	 */
 	ret = mmu_map_sg(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT,
 			 prot, sgt);
 	if (ret)
-		goto err_put_pages;
+		return ret;
 
 	mapping->active = true;
 
 	return 0;
-
-err_put_pages:
-	drm_gem_shmem_put_pages_locked(shmem);
-	return ret;
 }
 
 void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping)

-- 
2.55.0

[PATCH v2 05/11] drm/panfrost: Unwind a failed GPU mapping without dropping its pages

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

Two error paths of a BO mapping leave the wrong state behind when an
allocation fails part-way (memory cgroup at its limit, io-pgtable
running out of memory):

- When mmu_map_sg() fails, panfrost_mmu_map() calls
  drm_gem_shmem_put_pages_locked(). The only page reference
  drm_gem_shmem_get_pages_sgt() took belongs to the sg table cached in
  shmem->sgt, which the object releases when it is freed, so this drops
  pages that shmem->sgt still maps (for an imported BO it underflows
  pages_use_count). It also runs without the reservation lock that
  helper asserts. Keep the pages and just return the error.

- When ops->map_pages() fails, mmu_map_sg() unmaps what the earlier
  calls mapped, but not the part the failing call itself had mapped
  before it failed (map_pages() reports it in @mapped). Those PTEs stay
  behind in a VA range that drm_mm hands out again, and the next mapping
  there finds them. Unmap that part too, and flush the unmapped range.

Fixes: 4da352cf8fb1 ("drm/panfrost: Handle page mapping failure")
Cc: stable@vger.kernel.org
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 971552c42..35fb86b6b 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -442,8 +442,11 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
 
 			ret = ops->map_pages(ops, iova, paddr, pgsize, pgcount, prot,
 				       GFP_KERNEL, &mapped);
-			if (ret)
+			if (ret) {
+				/* Unwind the pages this call mapped too. */
+				total_mapped += mapped;
 				goto err_unmap_pages;
+			}
 
 			/* Don't get stuck if things have gone wrong */
 			mapped = max(mapped, pgsize);
@@ -459,7 +462,10 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
 	return 0;
 
 err_unmap_pages:
-	mmu_unmap_range(mmu, start_iova, total_mapped);
+	if (total_mapped) {
+		mmu_unmap_range(mmu, start_iova, total_mapped);
+		panfrost_mmu_flush_range(pfdev, mmu, start_iova, total_mapped);
+	}
 	return ret;
 }
 
@@ -483,18 +489,20 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
 	if (IS_ERR(sgt))
 		return PTR_ERR(sgt);
 
+	/*
+	 * On failure keep the pages: the sg table cached in the shmem object
+	 * owns the only page reference drm_gem_shmem_get_pages_sgt() took,
+	 * and the object frees both. Dropping it here (without the reservation
+	 * lock) released pages that shmem->sgt still maps.
+	 */
 	ret = mmu_map_sg(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT,
 			 prot, sgt);
 	if (ret)
-		goto err_put_pages;
+		return ret;
 
 	mapping->active = true;
 
 	return 0;
-
-err_put_pages:
-	drm_gem_shmem_put_pages_locked(shmem);
-	return ret;
 }
 
 void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping)

-- 
2.55.0

[PATCH v2 06/11] drm/panfrost: Give back the pages of a heap chunk that failed to map

[Arun Iyengar]

panfrost_mmu_map_fault_addr() grows a heap BO by one 2 MB chunk per
fault. If reading the chunk's folios, building its sg table, DMA mapping
it or mapping it on the GPU fails, the folios that were already read stay
in bo->base.pages, while the chunk's sg table stays empty. The next fault
in that chunk reads the same folios again and finds their pages already
owned:

  if (drm_WARN_ON(&pfdev->base, already_owned))
          folio_put(folio);

Before commit b49c35ba0ce1 ("drm/panfrost: Fix a page leak in
panfrost_mmu_map_fault_addr() when THP is on") the fault path skipped
pages that were already filled in; since then the array is filled a
folio at a time and an already filled page is treated as a bug.

On those error paths, put the folios read for the chunk and clear their
entries, so that a later fault in it starts from an empty range. The
range to clear is the one the fill loop wrote, which is not always the
chunk: the loop aligns down to the start of each folio and fills the
whole folio, so a folio larger than 2 MB (PMD-sized folios with 16K or
64K pages) also fills entries of the neighbouring chunks, and a folio
whose pages the loop found already owned (by such a neighbour) is not
ours to put.

Tested with 4K pages, where the folios are at most 2 MB and never cross
a chunk; the larger-folio case is from reading the code.

Fixes: b49c35ba0ce1 ("drm/panfrost: Fix a page leak in panfrost_mmu_map_fault_addr() when THP is on")
Cc: stable@vger.kernel.org
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 51 ++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 35fb86b6b..684735937 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -603,6 +603,32 @@ addr_to_mapping(struct panfrost_device *pfdev, int as, u64 addr)
 
 #define NUM_FAULT_PAGES (SZ_2M / PAGE_SIZE)
 
+/*
+ * Put the folios that a failed heap fault took and clear the page array
+ * entries it filled for them. [start, end) is what the fault filled: whole
+ * folios, since it fills the array a folio at a time, which can reach past
+ * the 2 MB chunk when a folio is larger than the chunk.
+ */
+static void panfrost_heap_put_pages(struct page **pages, pgoff_t start,
+				    pgoff_t end)
+{
+	while (start < end) {
+		struct folio *folio;
+
+		if (!pages[start]) {
+			start++;
+			continue;
+		}
+
+		folio = page_folio(pages[start]);
+		do {
+			pages[start++] = NULL;
+		} while (start < end && pages[start] &&
+			 page_folio(pages[start]) == folio);
+		folio_put(folio);
+	}
+}
+
 static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 				       u64 addr)
 {
@@ -611,7 +637,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 	struct panfrost_gem_object *bo;
 	struct address_space *mapping;
 	struct drm_gem_object *obj;
-	pgoff_t page_offset, nr_pages;
+	pgoff_t page_offset, nr_pages, got_start, got_end;
 	struct sg_table *sgt;
 	struct page **pages;
 
@@ -670,17 +696,23 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 	mapping = bo->base.base.filp->f_mapping;
 	mapping_set_unevictable(mapping);
 
+	/* The page array range whose folio references this fault takes. */
+	got_start = page_offset;
+	got_end = page_offset;
+
 	for (pgoff_t pg = page_offset; pg < page_offset + NUM_FAULT_PAGES;) {
 		bool already_owned = false;
 		struct folio *folio;
+		pgoff_t first;
 
 		folio = shmem_read_folio(mapping, pg);
 		if (IS_ERR(folio)) {
 			ret = PTR_ERR(folio);
-			goto err_unlock;
+			goto err_put_chunk;
 		}
 
 		pg &= ~(folio_nr_pages(folio) - 1);
+		first = pg;
 		for (u32 i = 0; i < folio_nr_pages(folio) && pg < nr_pages; i++) {
 			if (pages[pg])
 				already_owned = true;
@@ -692,14 +724,18 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 		 * there's no valid reason for a folio range to be partially
 		 * populated.
 		 */
-		if (drm_WARN_ON(&pfdev->base, already_owned))
+		if (drm_WARN_ON(&pfdev->base, already_owned)) {
 			folio_put(folio);
+		} else {
+			got_start = min(got_start, first);
+			got_end = max(got_end, pg);
+		}
 	}
 
 	ret = sg_alloc_table_from_pages(sgt, pages + page_offset,
 					NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL);
 	if (ret)
-		goto err_unlock;
+		goto err_put_chunk;
 
 	ret = dma_map_sgtable(pfdev->base.dev, sgt, DMA_BIDIRECTIONAL, 0);
 	if (ret)
@@ -726,6 +762,13 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 	dma_unmap_sgtable(pfdev->base.dev, sgt, DMA_BIDIRECTIONAL, 0);
 err_map:
 	sg_free_table(sgt);
+err_put_chunk:
+	/*
+	 * Give back the folios read for this chunk, so that the next fault in
+	 * it (the chunk has no sg table yet) starts from an empty page range
+	 * instead of finding it partly owned.
+	 */
+	panfrost_heap_put_pages(pages, got_start, got_end);
 err_unlock:
 	dma_resv_unlock(obj->resv);
 err_bo:

-- 
2.55.0

[PATCH v2 06/11] drm/panfrost: Give back the pages of a heap chunk that failed to map

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

panfrost_mmu_map_fault_addr() grows a heap BO by one 2 MB chunk per
fault. If reading the chunk's folios, building its sg table, DMA mapping
it or mapping it on the GPU fails, the folios that were already read stay
in bo->base.pages, while the chunk's sg table stays empty. The next fault
in that chunk reads the same folios again and finds their pages already
owned:

  if (drm_WARN_ON(&pfdev->base, already_owned))
          folio_put(folio);

Before commit b49c35ba0ce1 ("drm/panfrost: Fix a page leak in
panfrost_mmu_map_fault_addr() when THP is on") the fault path skipped
pages that were already filled in; since then the array is filled a
folio at a time and an already filled page is treated as a bug.

On those error paths, put the folios read for the chunk and clear their
entries, so that a later fault in it starts from an empty range. The
range to clear is the one the fill loop wrote, which is not always the
chunk: the loop aligns down to the start of each folio and fills the
whole folio, so a folio larger than 2 MB (PMD-sized folios with 16K or
64K pages) also fills entries of the neighbouring chunks, and a folio
whose pages the loop found already owned (by such a neighbour) is not
ours to put.

Tested with 4K pages, where the folios are at most 2 MB and never cross
a chunk; the larger-folio case is from reading the code.

Fixes: b49c35ba0ce1 ("drm/panfrost: Fix a page leak in panfrost_mmu_map_fault_addr() when THP is on")
Cc: stable@vger.kernel.org
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 51 ++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 35fb86b6b..684735937 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -603,6 +603,32 @@ addr_to_mapping(struct panfrost_device *pfdev, int as, u64 addr)
 
 #define NUM_FAULT_PAGES (SZ_2M / PAGE_SIZE)
 
+/*
+ * Put the folios that a failed heap fault took and clear the page array
+ * entries it filled for them. [start, end) is what the fault filled: whole
+ * folios, since it fills the array a folio at a time, which can reach past
+ * the 2 MB chunk when a folio is larger than the chunk.
+ */
+static void panfrost_heap_put_pages(struct page **pages, pgoff_t start,
+				    pgoff_t end)
+{
+	while (start < end) {
+		struct folio *folio;
+
+		if (!pages[start]) {
+			start++;
+			continue;
+		}
+
+		folio = page_folio(pages[start]);
+		do {
+			pages[start++] = NULL;
+		} while (start < end && pages[start] &&
+			 page_folio(pages[start]) == folio);
+		folio_put(folio);
+	}
+}
+
 static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 				       u64 addr)
 {
@@ -611,7 +637,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 	struct panfrost_gem_object *bo;
 	struct address_space *mapping;
 	struct drm_gem_object *obj;
-	pgoff_t page_offset, nr_pages;
+	pgoff_t page_offset, nr_pages, got_start, got_end;
 	struct sg_table *sgt;
 	struct page **pages;
 
@@ -670,17 +696,23 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 	mapping = bo->base.base.filp->f_mapping;
 	mapping_set_unevictable(mapping);
 
+	/* The page array range whose folio references this fault takes. */
+	got_start = page_offset;
+	got_end = page_offset;
+
 	for (pgoff_t pg = page_offset; pg < page_offset + NUM_FAULT_PAGES;) {
 		bool already_owned = false;
 		struct folio *folio;
+		pgoff_t first;
 
 		folio = shmem_read_folio(mapping, pg);
 		if (IS_ERR(folio)) {
 			ret = PTR_ERR(folio);
-			goto err_unlock;
+			goto err_put_chunk;
 		}
 
 		pg &= ~(folio_nr_pages(folio) - 1);
+		first = pg;
 		for (u32 i = 0; i < folio_nr_pages(folio) && pg < nr_pages; i++) {
 			if (pages[pg])
 				already_owned = true;
@@ -692,14 +724,18 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 		 * there's no valid reason for a folio range to be partially
 		 * populated.
 		 */
-		if (drm_WARN_ON(&pfdev->base, already_owned))
+		if (drm_WARN_ON(&pfdev->base, already_owned)) {
 			folio_put(folio);
+		} else {
+			got_start = min(got_start, first);
+			got_end = max(got_end, pg);
+		}
 	}
 
 	ret = sg_alloc_table_from_pages(sgt, pages + page_offset,
 					NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL);
 	if (ret)
-		goto err_unlock;
+		goto err_put_chunk;
 
 	ret = dma_map_sgtable(pfdev->base.dev, sgt, DMA_BIDIRECTIONAL, 0);
 	if (ret)
@@ -726,6 +762,13 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 	dma_unmap_sgtable(pfdev->base.dev, sgt, DMA_BIDIRECTIONAL, 0);
 err_map:
 	sg_free_table(sgt);
+err_put_chunk:
+	/*
+	 * Give back the folios read for this chunk, so that the next fault in
+	 * it (the chunk has no sg table yet) starts from an empty page range
+	 * instead of finding it partly owned.
+	 */
+	panfrost_heap_put_pages(pages, got_start, got_end);
 err_unlock:
 	dma_resv_unlock(obj->resv);
 err_bo:

-- 
2.55.0

Re: [PATCH v2 06/11] drm/panfrost: Give back the pages of a heap chunk that failed to map

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/panfrost: false drm_WARN_ON() in
  panfrost_mmu_map_fault_addr()
--

commit 455071c10d7f5d95d3ce18757e41ef79db6b9400
Author: Arun Iyengar <aiyengar1990@gmail.com>

drm/panfrost: Give back the pages of a heap chunk that failed to map

This commit releases the shmem folios read during a heap BO fault and
clears their entries in bo->base.pages if mapping the 2 MB fault chunk
fails.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 35fb86b6b3242..6847359374e53 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
[ ... ]
> @@ -670,17 +696,23 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>  	mapping = bo->base.base.filp->f_mapping;
>  	mapping_set_unevictable(mapping);
>  
> +	/* The page array range whose folio references this fault takes. */
> +	got_start = page_offset;
> +	got_end = page_offset;
> +
>  	for (pgoff_t pg = page_offset; pg < page_offset + NUM_FAULT_PAGES;) {
>  		bool already_owned = false;
>  		struct folio *folio;
> +		pgoff_t first;
>  
>  		folio = shmem_read_folio(mapping, pg);
>  		if (IS_ERR(folio)) {
>  			ret = PTR_ERR(folio);
> -			goto err_unlock;
> +			goto err_put_chunk;
>  		}
>  
>  		pg &= ~(folio_nr_pages(folio) - 1);
> +		first = pg;
>  		for (u32 i = 0; i < folio_nr_pages(folio) && pg < nr_pages; i++) {
>  			if (pages[pg])
>  				already_owned = true;
> @@ -692,14 +724,18 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>  		 * there's no valid reason for a folio range to be partially
>  		 * populated.
>  		 */
> -		if (drm_WARN_ON(&pfdev->base, already_owned))
> +		if (drm_WARN_ON(&pfdev->base, already_owned)) {
>  			folio_put(folio);
> +		} else {
> +			got_start = min(got_start, first);
> +			got_end = max(got_end, pg);
> +		}
>  	}

[Severity: Medium]
Should drm_WARN_ON() be removed when already_owned is true for a folio
larger than 2 MB?

When shmem returns a folio larger than the 2 MB fault chunk (such as a
PMD-sized or multi-megabyte THP folio on 16K or 64K page kernels), the
first chunk fault populates pages[] for the entire folio across
neighbouring 2 MB chunks, while only mapping the sg_table for the faulting
2 MB chunk.  The new else branch relies on already_owned being true when
those neighbouring 2 MB chunks later fault so that [got_start, got_end)
does not claim their shared folio, but keeping drm_WARN_ON(&pfdev->base,
already_owned) in the condition triggers a warning on every normal fault
of those neighbouring chunks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-panfrost-long-jobs-v2-0-a06830da3d99@gmail.com?part=6

[PATCH v2 07/11] drm/panfrost: Don't signal a job cancelled from _NEXT at a reset as done

[Arun Iyengar]

panfrost_reset() cancels the job waiting in each slot's _NEXT registers
with JS_COMMAND_NOP and soft-stops the running one. If the running job
finishes instead of stopping, its DONE interrupt is handled by
panfrost_jm_handle_irqs() before the reset. panfrost_jm_handle_irq()
dequeues it, which moves the cancelled job to the head of the slot, then
finds both subslots inactive and takes that job for done as well: when
a job completes, the job manager starts the _NEXT one without waiting
for the interrupt, so a job left on an inactive slot is normally one
that has finished too. The cancelled job gets its jc cleared and its
done fence signalled without having run: drm_sched_resubmit_jobs() does
not run it again, and it completes with -ECANCELED although nothing was
wrong with it.

The Arm driver checks JS_HEAD_NEXT after writing the NOP: if it still
holds the job, the job has not started and is evicted; if it reads zero,
the job manager had already moved the job to the running subslot. Do the
same: take an evicted job off the slot while the remaining interrupts
are handled, and put it back afterwards, so that it is restarted with
the other jobs.

Found by code review (Sashiko) of the next patches, which make the
soft-stop wait longer and the window wider. Not reproduced.

Fixes: 030761e097b4 ("drm/panfrost: Queue jobs on the hardware")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/sashiko-outbox-206581@kernel.org/
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_job.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 0c2fda80c..418a91a31 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -659,6 +659,7 @@ static void
 panfrost_reset(struct panfrost_device *pfdev,
 	       struct drm_sched_job *bad)
 {
+	struct panfrost_job *evicted[NUM_JOB_SLOTS] = {};
 	u32 js_state, js_state_mask = 0xffffffff;
 	unsigned int i, j;
 	bool cookie;
@@ -694,11 +695,28 @@ panfrost_reset(struct panfrost_device *pfdev,
 	job_write(pfdev, JOB_INT_MASK, 0);
 	synchronize_irq(pfdev->js->irq);
 
+	spin_lock(&pfdev->js->job_lock);
 	for (i = 0; i < NUM_JOB_SLOTS; i++) {
 		/* Cancel the next job and soft-stop the running job. */
 		job_write(pfdev, JS_COMMAND_NEXT(i), JS_COMMAND_NOP);
+
+		/* A job still in the _NEXT registers after the NOP has not
+		 * started and will not. Take it off the slot until the
+		 * remaining interrupts are handled: if the running job
+		 * finishes instead of stopping, panfrost_jm_handle_irq() finds
+		 * both subslots inactive and takes the job left on the slot
+		 * for done too, and it would be signalled without having run.
+		 */
+		if (pfdev->jobs[i][1] &&
+		    (job_read(pfdev, JS_HEAD_NEXT_LO(i)) ||
+		     job_read(pfdev, JS_HEAD_NEXT_HI(i)))) {
+			evicted[i] = pfdev->jobs[i][1];
+			pfdev->jobs[i][1] = NULL;
+		}
+
 		job_write(pfdev, JS_COMMAND(i), JS_COMMAND_SOFT_STOP);
 	}
+	spin_unlock(&pfdev->js->job_lock);
 
 	/* Wait at most 10ms for soft-stops to complete */
 	ret = readl_poll_timeout(pfdev->iomem + JOB_INT_JS_STATE, js_state,
@@ -720,6 +738,12 @@ panfrost_reset(struct panfrost_device *pfdev,
 	 */
 	spin_lock(&pfdev->js->job_lock);
 	for (i = 0; i < NUM_JOB_SLOTS; i++) {
+		/* Put the evicted jobs back, they are restarted like the
+		 * others.
+		 */
+		if (evicted[i])
+			pfdev->jobs[i][pfdev->jobs[i][0] ? 1 : 0] = evicted[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)

-- 
2.55.0

[PATCH v2 07/11] drm/panfrost: Don't signal a job cancelled from _NEXT at a reset as done

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

panfrost_reset() cancels the job waiting in each slot's _NEXT registers
with JS_COMMAND_NOP and soft-stops the running one. If the running job
finishes instead of stopping, its DONE interrupt is handled by
panfrost_jm_handle_irqs() before the reset. panfrost_jm_handle_irq()
dequeues it, which moves the cancelled job to the head of the slot, then
finds both subslots inactive and takes that job for done as well: when
a job completes, the job manager starts the _NEXT one without waiting
for the interrupt, so a job left on an inactive slot is normally one
that has finished too. The cancelled job gets its jc cleared and its
done fence signalled without having run: drm_sched_resubmit_jobs() does
not run it again, and it completes with -ECANCELED although nothing was
wrong with it.

The Arm driver checks JS_HEAD_NEXT after writing the NOP: if it still
holds the job, the job has not started and is evicted; if it reads zero,
the job manager had already moved the job to the running subslot. Do the
same: take an evicted job off the slot while the remaining interrupts
are handled, and put it back afterwards, so that it is restarted with
the other jobs.

Found by code review (Sashiko) of the next patches, which make the
soft-stop wait longer and the window wider. Not reproduced.

Fixes: 030761e097b4 ("drm/panfrost: Queue jobs on the hardware")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/sashiko-outbox-206581@kernel.org/
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_job.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 0c2fda80c..418a91a31 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -659,6 +659,7 @@ static void
 panfrost_reset(struct panfrost_device *pfdev,
 	       struct drm_sched_job *bad)
 {
+	struct panfrost_job *evicted[NUM_JOB_SLOTS] = {};
 	u32 js_state, js_state_mask = 0xffffffff;
 	unsigned int i, j;
 	bool cookie;
@@ -694,11 +695,28 @@ panfrost_reset(struct panfrost_device *pfdev,
 	job_write(pfdev, JOB_INT_MASK, 0);
 	synchronize_irq(pfdev->js->irq);
 
+	spin_lock(&pfdev->js->job_lock);
 	for (i = 0; i < NUM_JOB_SLOTS; i++) {
 		/* Cancel the next job and soft-stop the running job. */
 		job_write(pfdev, JS_COMMAND_NEXT(i), JS_COMMAND_NOP);
+
+		/* A job still in the _NEXT registers after the NOP has not
+		 * started and will not. Take it off the slot until the
+		 * remaining interrupts are handled: if the running job
+		 * finishes instead of stopping, panfrost_jm_handle_irq() finds
+		 * both subslots inactive and takes the job left on the slot
+		 * for done too, and it would be signalled without having run.
+		 */
+		if (pfdev->jobs[i][1] &&
+		    (job_read(pfdev, JS_HEAD_NEXT_LO(i)) ||
+		     job_read(pfdev, JS_HEAD_NEXT_HI(i)))) {
+			evicted[i] = pfdev->jobs[i][1];
+			pfdev->jobs[i][1] = NULL;
+		}
+
 		job_write(pfdev, JS_COMMAND(i), JS_COMMAND_SOFT_STOP);
 	}
+	spin_unlock(&pfdev->js->job_lock);
 
 	/* Wait at most 10ms for soft-stops to complete */
 	ret = readl_poll_timeout(pfdev->iomem + JOB_INT_JS_STATE, js_state,
@@ -720,6 +738,12 @@ panfrost_reset(struct panfrost_device *pfdev,
 	 */
 	spin_lock(&pfdev->js->job_lock);
 	for (i = 0; i < NUM_JOB_SLOTS; i++) {
+		/* Put the evicted jobs back, they are restarted like the
+		 * others.
+		 */
+		if (evicted[i])
+			pfdev->jobs[i][pfdev->jobs[i][0] ? 1 : 0] = evicted[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)

-- 
2.55.0

[PATCH v2 08/11] drm/panfrost: Don't cancel a job that completed during a reset

[Arun Iyengar]

panfrost_reset() stops the schedulers, which detaches them from the done
fences of the jobs on the GPU, then soft-stops the slots and handles the
interrupts that are still pending. A job that finishes in that window
instead of stopping is completed by panfrost_jm_handle_done(): its done
fence is signalled and its jc cleared. drm_sched_resubmit_jobs() then
calls panfrost_job_run(), which returns NULL for a job with no jc, and
since commit 03877d621db0 ("drm/scheduler: mark jobs without fence as
canceled") drm_sched_start() completes a pending job without a hardware
fence with -ECANCELED. The job did complete, but userspace is told it
did not.

Return the job's done fence instead: drm_sched_start() finds it
signalled and completes the job with the fence's status, which is 0 for
a job that finished and the error panfrost_job_handle_err() set for one
that was hard-stopped or faulted.

This depends on the previous patch: before it, a job cancelled from
_NEXT could be taken for done in the same window, and would now be
reported as completed without having run.

Found by code review (Sashiko) of the next patches, which make the
soft-stop wait longer and the window wider. Not reproduced.

Fixes: 03877d621db0 ("drm/scheduler: mark jobs without fence as canceled")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/sashiko-outbox-206581@kernel.org/
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_job.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 418a91a31..7561de67c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -409,11 +409,14 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
 	if (unlikely(job->base.s_fence->finished.error))
 		return NULL;
 
-	/* Nothing to execute: can happen if the job has finished while
-	 * we were resetting the GPU.
+	/* Nothing to execute: the job has finished (or failed) on the GPU
+	 * while we were resetting it, after drm_sched_stop() had detached the
+	 * scheduler from its done fence. Hand that fence back, so that the job
+	 * completes with the status the GPU gave it: with no fence,
+	 * drm_sched_start() would complete it as cancelled.
 	 */
 	if (!job->jc)
-		return NULL;
+		return dma_fence_get(job->done_fence);
 
 	fence = panfrost_fence_create(pfdev, slot);
 	if (IS_ERR(fence))

-- 
2.55.0

[PATCH v2 08/11] drm/panfrost: Don't cancel a job that completed during a reset

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

panfrost_reset() stops the schedulers, which detaches them from the done
fences of the jobs on the GPU, then soft-stops the slots and handles the
interrupts that are still pending. A job that finishes in that window
instead of stopping is completed by panfrost_jm_handle_done(): its done
fence is signalled and its jc cleared. drm_sched_resubmit_jobs() then
calls panfrost_job_run(), which returns NULL for a job with no jc, and
since commit 03877d621db0 ("drm/scheduler: mark jobs without fence as
canceled") drm_sched_start() completes a pending job without a hardware
fence with -ECANCELED. The job did complete, but userspace is told it
did not.

Return the job's done fence instead: drm_sched_start() finds it
signalled and completes the job with the fence's status, which is 0 for
a job that finished and the error panfrost_job_handle_err() set for one
that was hard-stopped or faulted.

This depends on the previous patch: before it, a job cancelled from
_NEXT could be taken for done in the same window, and would now be
reported as completed without having run.

Found by code review (Sashiko) of the next patches, which make the
soft-stop wait longer and the window wider. Not reproduced.

Fixes: 03877d621db0 ("drm/scheduler: mark jobs without fence as canceled")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/sashiko-outbox-206581@kernel.org/
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_job.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 418a91a31..7561de67c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -409,11 +409,14 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
 	if (unlikely(job->base.s_fence->finished.error))
 		return NULL;
 
-	/* Nothing to execute: can happen if the job has finished while
-	 * we were resetting the GPU.
+	/* Nothing to execute: the job has finished (or failed) on the GPU
+	 * while we were resetting it, after drm_sched_stop() had detached the
+	 * scheduler from its done fence. Hand that fence back, so that the job
+	 * completes with the status the GPU gave it: with no fence,
+	 * drm_sched_start() would complete it as cancelled.
 	 */
 	if (!job->jc)
-		return NULL;
+		return dma_fence_get(job->done_fence);
 
 	fence = panfrost_fence_create(pfdev, slot);
 	if (IS_ERR(fence))

-- 
2.55.0

[PATCH v2 09/11] drm/panfrost: Wait for the soft-stopped slots with the live slot status

[Arun Iyengar]

panfrost_reset() soft-stops the slots and waits for them to go idle by
polling JOB_INT_JS_STATE. That register is not live: it is a snapshot of
the slot states taken when JOB_INT_CLEAR was last written (the Arm
driver documents it as "status==active and _next == busy snapshot from
last JOB_IRQ_CLEAR"). For a job that was started into an idle slot, the
usual case for an OpenCL queue that submits one kernel at a time, it
still shows the slot idle, and the wait ends at once while the job is
still running.

The GPU reset that follows hides this, since it kills the job anyway.
Without a reset it does not: with the soft-stopped job resumed instead
(next patch) and this wait, the job was removed from pfdev->jobs[] and
resubmitted while its first run was still stopping. When that first run
reported DONE, the handler signalled the resubmission's fence, the job
manager started the resubmission from _NEXT, and its completion found
no job on the slot (Mali-G31, OpenCL CTS Integer Ops):

  Unable to handle kernel NULL pointer dereference at virtual address 00000000000000f8
  pc : panfrost_dequeue_job+0x2c/0xd8 [panfrost]
  lr : panfrost_jm_handle_irqs+0x268/0x570 [panfrost]

Wait on the live JS_STATUS registers instead: a slot is busy while it
reads ACTIVE, and every slot has to be idle before the queues are
touched, so that the pending DONE or STOPPED interrupt is handled first.
And make panfrost_dequeue_job() return NULL for a slot without a job:
every caller copes with a NULL, so a stray event is a warning rather
than an Oops.

Fixes: 030761e097b4 ("drm/panfrost: Queue jobs on the hardware")
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_job.c | 48 ++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 7561de67c..28842c572 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -165,7 +165,11 @@ panfrost_dequeue_job(struct panfrost_device *pfdev, int slot)
 {
 	struct panfrost_job *job = pfdev->jobs[slot][0];
 
-	WARN_ON(!job);
+	/* An event for a slot we have no job on. Not fatal, as every caller
+	 * copes with a NULL job; a WARN, not an Oops.
+	 */
+	if (WARN_ON(!job))
+		return NULL;
 
 	if (job->is_profiled && job->engine_usage) {
 		job->engine_usage->elapsed_ns[slot] +=
@@ -637,25 +641,23 @@ static void panfrost_jm_handle_irqs(struct panfrost_device *pfdev)
 	}
 }
 
-static u32 panfrost_active_slots(struct panfrost_device *pfdev,
-				 u32 *js_state_mask, u32 js_state)
+/* A slot is busy as long as its status register reads ACTIVE. This has to
+ * be the live JS_STATUS: JOB_INT_JS_STATE is a snapshot taken when
+ * JOB_INT_CLEAR was last written, so for a job that was started into an idle
+ * slot it still shows the slot idle, and a wait on it ends before the
+ * soft-stop has done anything.
+ */
+static bool panfrost_slots_idle(struct panfrost_device *pfdev)
 {
-	u32 rawstat;
-
-	if (!(js_state & *js_state_mask))
-		return 0;
-
-	rawstat = job_read(pfdev, JOB_INT_RAWSTAT);
-	if (rawstat) {
-		unsigned int i;
+	unsigned int i;
 
-		for (i = 0; i < NUM_JOB_SLOTS; i++) {
-			if (rawstat & MK_JS_MASK(i))
-				*js_state_mask &= ~MK_JS_MASK(i);
-		}
+	for (i = 0; i < NUM_JOB_SLOTS; i++) {
+		if (job_read(pfdev, JS_STATUS(i)) ==
+		    DRM_PANFROST_EXCEPTION_ACTIVE)
+			return false;
 	}
 
-	return js_state & *js_state_mask;
+	return true;
 }
 
 static void
@@ -663,9 +665,8 @@ panfrost_reset(struct panfrost_device *pfdev,
 	       struct drm_sched_job *bad)
 {
 	struct panfrost_job *evicted[NUM_JOB_SLOTS] = {};
-	u32 js_state, js_state_mask = 0xffffffff;
 	unsigned int i, j;
-	bool cookie;
+	bool cookie, idle;
 	int ret;
 
 	if (!atomic_read(&pfdev->reset.pending))
@@ -721,10 +722,13 @@ panfrost_reset(struct panfrost_device *pfdev,
 	}
 	spin_unlock(&pfdev->js->job_lock);
 
-	/* Wait at most 10ms for soft-stops to complete */
-	ret = readl_poll_timeout(pfdev->iomem + JOB_INT_JS_STATE, js_state,
-				 !panfrost_active_slots(pfdev, &js_state_mask, js_state),
-				 10, 10000);
+	/* 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.
+	 */
+	ret = read_poll_timeout(panfrost_slots_idle, idle, idle, 10, 10000,
+				false, pfdev);
 
 	if (ret)
 		dev_err(pfdev->base.dev, "Soft-stop failed\n");

-- 
2.55.0

[PATCH v2 09/11] drm/panfrost: Wait for the soft-stopped slots with the live slot status

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

panfrost_reset() soft-stops the slots and waits for them to go idle by
polling JOB_INT_JS_STATE. That register is not live: it is a snapshot of
the slot states taken when JOB_INT_CLEAR was last written (the Arm
driver documents it as "status==active and _next == busy snapshot from
last JOB_IRQ_CLEAR"). For a job that was started into an idle slot, the
usual case for an OpenCL queue that submits one kernel at a time, it
still shows the slot idle, and the wait ends at once while the job is
still running.

The GPU reset that follows hides this, since it kills the job anyway.
Without a reset it does not: with the soft-stopped job resumed instead
(next patch) and this wait, the job was removed from pfdev->jobs[] and
resubmitted while its first run was still stopping. When that first run
reported DONE, the handler signalled the resubmission's fence, the job
manager started the resubmission from _NEXT, and its completion found
no job on the slot (Mali-G31, OpenCL CTS Integer Ops):

  Unable to handle kernel NULL pointer dereference at virtual address 00000000000000f8
  pc : panfrost_dequeue_job+0x2c/0xd8 [panfrost]
  lr : panfrost_jm_handle_irqs+0x268/0x570 [panfrost]

Wait on the live JS_STATUS registers instead: a slot is busy while it
reads ACTIVE, and every slot has to be idle before the queues are
touched, so that the pending DONE or STOPPED interrupt is handled first.
And make panfrost_dequeue_job() return NULL for a slot without a job:
every caller copes with a NULL, so a stray event is a warning rather
than an Oops.

Fixes: 030761e097b4 ("drm/panfrost: Queue jobs on the hardware")
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_job.c | 48 ++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 7561de67c..28842c572 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -165,7 +165,11 @@ panfrost_dequeue_job(struct panfrost_device *pfdev, int slot)
 {
 	struct panfrost_job *job = pfdev->jobs[slot][0];
 
-	WARN_ON(!job);
+	/* An event for a slot we have no job on. Not fatal, as every caller
+	 * copes with a NULL job; a WARN, not an Oops.
+	 */
+	if (WARN_ON(!job))
+		return NULL;
 
 	if (job->is_profiled && job->engine_usage) {
 		job->engine_usage->elapsed_ns[slot] +=
@@ -637,25 +641,23 @@ static void panfrost_jm_handle_irqs(struct panfrost_device *pfdev)
 	}
 }
 
-static u32 panfrost_active_slots(struct panfrost_device *pfdev,
-				 u32 *js_state_mask, u32 js_state)
+/* A slot is busy as long as its status register reads ACTIVE. This has to
+ * be the live JS_STATUS: JOB_INT_JS_STATE is a snapshot taken when
+ * JOB_INT_CLEAR was last written, so for a job that was started into an idle
+ * slot it still shows the slot idle, and a wait on it ends before the
+ * soft-stop has done anything.
+ */
+static bool panfrost_slots_idle(struct panfrost_device *pfdev)
 {
-	u32 rawstat;
-
-	if (!(js_state & *js_state_mask))
-		return 0;
-
-	rawstat = job_read(pfdev, JOB_INT_RAWSTAT);
-	if (rawstat) {
-		unsigned int i;
+	unsigned int i;
 
-		for (i = 0; i < NUM_JOB_SLOTS; i++) {
-			if (rawstat & MK_JS_MASK(i))
-				*js_state_mask &= ~MK_JS_MASK(i);
-		}
+	for (i = 0; i < NUM_JOB_SLOTS; i++) {
+		if (job_read(pfdev, JS_STATUS(i)) ==
+		    DRM_PANFROST_EXCEPTION_ACTIVE)
+			return false;
 	}
 
-	return js_state & *js_state_mask;
+	return true;
 }
 
 static void
@@ -663,9 +665,8 @@ panfrost_reset(struct panfrost_device *pfdev,
 	       struct drm_sched_job *bad)
 {
 	struct panfrost_job *evicted[NUM_JOB_SLOTS] = {};
-	u32 js_state, js_state_mask = 0xffffffff;
 	unsigned int i, j;
-	bool cookie;
+	bool cookie, idle;
 	int ret;
 
 	if (!atomic_read(&pfdev->reset.pending))
@@ -721,10 +722,13 @@ panfrost_reset(struct panfrost_device *pfdev,
 	}
 	spin_unlock(&pfdev->js->job_lock);
 
-	/* Wait at most 10ms for soft-stops to complete */
-	ret = readl_poll_timeout(pfdev->iomem + JOB_INT_JS_STATE, js_state,
-				 !panfrost_active_slots(pfdev, &js_state_mask, js_state),
-				 10, 10000);
+	/* 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.
+	 */
+	ret = read_poll_timeout(panfrost_slots_idle, idle, idle, 10, 10000,
+				false, pfdev);
 
 	if (ret)
 		dev_err(pfdev->base.dev, "Soft-stop failed\n");

-- 
2.55.0

[PATCH v2 10/11] drm/panfrost: Soft-stop and resume a job that outlives the timeout

[Arun Iyengar]

JOB_TIMEOUT_MS is 500 ms, and when it fires panfrost resets the GPU and
the job is lost. But a job that runs longer is not necessarily hung:
OpenCL kernels and heavy fragment jobs legitimately take seconds on a
small GPU. On a Mali-G31 (one shader core) that breaks, among others:

- OpenCL CTS Integer Ops quick_long_math and quick_ulong_math (long16
  64-bit division, > 500 ms per kernel): each subtest logs

    panfrost 1800000.gpu: gpu sched timeout, js=1, config=0x7300, status=0x8, head=0xa0161c0, tail=0xa0161c0

  (status 0x8 = ACTIVE) and fails with the work-items after the reset
  never written; Thread Dimensions quick_2d/3d lose whole rows the same
  way.
- KHR-GLES31.core.blend_equation_advanced.test_coherency.mixedSequence:
  a 44-draw fragment job on a 1024x1024 target needs about 0.7 s, and
  the tiles it had not reached at 500 ms read back as 0,0,0,0.
- dEQP-VK.spirv_assembly.type.vec4.*_frag cases (a loop over the whole
  dataset in every fragment): "Incomplete job or timeout".

The Arm driver time-slices instead: a job is soft-stopped after its
time slice and resumed later, it is hard-stopped only when it has not
stopped after 5 s (JS_HARD_STOP_TICKS_SS = 50 ticks of 100 ms), and the
GPU is reset only when the hard stop has not taken effect 0.5 s after
that (JS_RESET_TICKS_SS = 55). Do the same from the timeout handler:

- a slot whose job is still ACTIVE when the timeout fires is
  soft-stopped and its job resumed from JS_TAIL, without a GPU reset and
  without raising its karma; a soft-stop takes effect at a task boundary
  only, after the tasks already issued have completed, so such a slot
  gets JOB_SOFT_STOP_TIMEOUT_MS (5 s) to go idle, idle or faulted slots
  keep the 10 ms;
- a slot still active after that is hard-stopped, which ends its job at
  once with TERMINATED (-ECANCELED); the stopped jobs on the other slots
  are resumed, nothing is reset. The job that did not stop is hung, so
  it gets its karma raised and a devcoredump, as a hung job did before;
- the GPU is reset when the hard stop has not taken effect after
  JOB_HARD_STOP_TIMEOUT_MS (0.5 s); the job that did not stop is then
  failed with -ECANCELED instead of being restarted from scratch, where
  it would only time out again. Jobs that were only given the 10 ms of a
  reset for another reason are restarted, as before;
- a reset that is asked for while the slots are being stopped (a job
  fault that needs one, the MMU) is not lost: the timeout only marks
  reset.pending for a stop (PANFROST_RESET_STOP_ONLY), a request for a
  reset overrides that, and the GPU is then reset;
- a job that completes between the timeout handler's fence check and its
  JS_STATUS read is not taken for a hung job: with its DONE interrupt
  pending or being handled, the timeout is spurious;
- without a reset, the address space references of the jobs cancelled
  from _NEXT are released (a reset has panfrost_mmu_reset() drop them),
  and a runtime PM reference is held across the stop and the
  resubmission, so that an autosuspend cannot power the GPU off between
  the two.

The timeout of a job that is merely long is now a dev_dbg(); faults,
jobs that do not stop and the other reset paths are reported as before,
with the status of every slot.

With this, on the Mali-G31 the Integer Ops, Thread Dimensions and
mixedSequence cases pass, and the two vec4 i8/u64 bit_field_insert
count64 fragment cases, which need several seconds to reach a task
boundary, pass in 4-6 s.

Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_job.c | 239 +++++++++++++++++++++++++++-----
 1 file changed, 207 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 28842c572..a920d112a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -27,6 +27,25 @@
 #define MAX_JM_CTX_PER_FILE 64
 #define JOB_TIMEOUT_MS 500
 
+/* How long a soft-stopped slot gets to reach a task boundary before its job is
+ * hard-stopped, and how long the hard stop gets before the GPU is reset. These
+ * follow the Arm driver's job slot policy (mali_kbase_config_defaults.h): a job
+ * is soft-stopped after its 100 ms time slice (JS_SOFT_STOP_TICKS = 1 tick),
+ * hard-stopped when it has not stopped by tick 50 (JS_HARD_STOP_TICKS_SS, 5 s)
+ * and the GPU is reset at tick 55 (JS_RESET_TICKS_SS, 5.5 s). A soft-stop
+ * takes effect at a task boundary only, after the tasks already issued (a
+ * compute job's work-groups, a fragment job's tiles) have run to completion,
+ * and a heavy shader needs seconds for that.
+ */
+#define JOB_SOFT_STOP_TIMEOUT_MS 5000
+#define JOB_HARD_STOP_TIMEOUT_MS 500
+
+/* reset.pending while panfrost_reset() is only to stop the slots, to resume
+ * the jobs of a timeout that found its job still running. A request for a
+ * real reset (1) that comes in meanwhile overrides it.
+ */
+#define PANFROST_RESET_STOP_ONLY 2
+
 #define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
 #define job_read(dev, reg) readl(dev->iomem + (reg))
 
@@ -165,8 +184,9 @@ panfrost_dequeue_job(struct panfrost_device *pfdev, int slot)
 {
 	struct panfrost_job *job = pfdev->jobs[slot][0];
 
-	/* An event for a slot we have no job on. Not fatal, as every caller
-	 * copes with a NULL job; a WARN, not an Oops.
+	/* An event for a slot we have no job on: a resubmission raced with
+	 * the first run of the job (see panfrost_reset()). Not fatal, as
+	 * every caller copes with a NULL job; a WARN, not an Oops.
 	 */
 	if (WARN_ON(!job))
 		return NULL;
@@ -660,18 +680,39 @@ static bool panfrost_slots_idle(struct panfrost_device *pfdev)
 	return true;
 }
 
+/* The job running on a slot: the one in _NEXT once the first has completed,
+ * with its interrupt not handled yet.
+ */
+static struct panfrost_job *
+panfrost_running_job(struct panfrost_device *pfdev, unsigned int slot)
+{
+	if (pfdev->jobs[slot][1] &&
+	    (job_read(pfdev, JOB_INT_RAWSTAT) & JOB_INT_MASK_DONE(slot)))
+		return pfdev->jobs[slot][1];
+
+	return pfdev->jobs[slot][0];
+}
+
 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 jobs we stop drop their runtime PM references before we submit
+	 * them again. Keep the GPU powered in between: once they have, nothing
+	 * else keeps it from being runtime suspended under the rest of this
+	 * function.
+	 */
+	pm_runtime_get_noresume(pfdev->base.dev);
+
 	/* Stop the schedulers.
 	 *
 	 * FIXME: We temporarily get out of the dma_fence_signalling section
@@ -690,7 +731,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);
 
 	/* Mask job interrupts and synchronize to make sure we won't be
@@ -722,26 +764,90 @@ panfrost_reset(struct panfrost_device *pfdev,
 	}
 	spin_unlock(&pfdev->js->job_lock);
 
-	/* 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);
 
+	if (ret && job_active) {
+		dev_err(pfdev->base.dev,
+			"Soft-stop failed after %u ms (js status 0x%x 0x%x 0x%x), hard-stopping the active slots\n",
+			JOB_SOFT_STOP_TIMEOUT_MS,
+			job_read(pfdev, JS_STATUS(0)), job_read(pfdev, JS_STATUS(1)),
+			job_read(pfdev, JS_STATUS(2)));
+
+		/* A job that has not reached a task boundary in all that time
+		 * is hung, not long: count it against its context and dump
+		 * it, as a timeout that resets the GPU does.
+		 */
+		for (i = 0; i < NUM_JOB_SLOTS; i++) {
+			if (job_read(pfdev, JS_STATUS(i)) ==
+			    DRM_PANFROST_EXCEPTION_ACTIVE)
+				hung[i] = panfrost_running_job(pfdev, i);
+			if (hung[i])
+				drm_sched_increase_karma(&hung[i]->base);
+		}
+
+		/* The dump allocates memory, which the fence signalling
+		 * section does not allow (see the resubmission below).
+		 */
+		dma_fence_end_signalling(cookie);
+		for (i = 0; i < NUM_JOB_SLOTS; i++) {
+			if (hung[i])
+				panfrost_core_dump(hung[i]);
+		}
+		cookie = dma_fence_begin_signalling();
+
+		for (i = 0; i < NUM_JOB_SLOTS; i++) {
+			if (job_read(pfdev, JS_STATUS(i)) ==
+			    DRM_PANFROST_EXCEPTION_ACTIVE)
+				job_write(pfdev, JS_COMMAND(i), JS_COMMAND_HARD_STOP);
+		}
+
+		ret = read_poll_timeout(panfrost_slots_idle, idle, idle, 200,
+					JOB_HARD_STOP_TIMEOUT_MS * 1000, false,
+					pfdev);
+		stop_failed = ret;
+	}
+
 	if (ret)
-		dev_err(pfdev->base.dev, "Soft-stop failed\n");
+		dev_err(pfdev->base.dev,
+			"%s failed (js status 0x%x 0x%x 0x%x), resetting the GPU\n",
+			job_active ? "Hard-stop" : "Soft-stop",
+			job_read(pfdev, JS_STATUS(0)), job_read(pfdev, JS_STATUS(1)),
+			job_read(pfdev, JS_STATUS(2)));
 
 	/* Handle the remaining interrupts before we reset. */
 	panfrost_jm_handle_irqs(pfdev);
 
+	/* Reset the GPU unless the slots only had to be stopped and did stop,
+	 * and nobody asked for a reset meanwhile: a job fault that needs one
+	 * (in the interrupts just handled) or the MMU.
+	 */
+	reset_gpu = !job_active || stop_failed ||
+		    atomic_read(&pfdev->reset.pending) != PANFROST_RESET_STOP_ONLY;
+
 	/* 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++) {
@@ -752,21 +858,53 @@ panfrost_reset(struct panfrost_device *pfdev,
 			pfdev->jobs[i][pfdev->jobs[i][0] ? 1 : 0] = evicted[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 (!reset_gpu)
+				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. A job that was only
+			 * given the 10 ms is restarted, as before.
+			 */
+			if (stop_failed && job == hung[i])
+				dma_fence_set_error(&job->base.s_fence->finished,
+						    -ECANCELED);
 		}
 	}
 	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()).
+	 */
+	if (reset_gpu) {
+		panfrost_device_reset(pfdev, false);
 
-	/* GPU has been reset, we can clear the reset pending bit. */
-	atomic_set(&pfdev->reset.pending, 0);
+		/* GPU has been reset, we can clear the reset pending bit. */
+		atomic_set(&pfdev->reset.pending, 0);
+	} else {
+		/* Leave a reset that was asked for since we looked to the
+		 * reset work or timeout handler that asked for it.
+		 */
+		atomic_cmpxchg(&pfdev->reset.pending,
+			       PANFROST_RESET_STOP_ONLY, 0);
+	}
 
 	/* Now resubmit jobs that were previously queued but didn't have a
 	 * chance to finish.
@@ -788,6 +926,8 @@ panfrost_reset(struct panfrost_device *pfdev,
 	panfrost_jm_enable_interrupts(pfdev);
 
 	dma_fence_end_signalling(cookie);
+
+	pm_runtime_put_autosuspend(pfdev->base.dev);
 }
 
 static enum drm_gpu_sched_stat panfrost_job_timedout(struct drm_sched_job
@@ -796,6 +936,8 @@ static enum drm_gpu_sched_stat panfrost_job_timedout(struct drm_sched_job
 	struct panfrost_job *job = to_panfrost_job(sched_job);
 	struct panfrost_device *pfdev = job->pfdev;
 	int js = panfrost_job_get_slot(job);
+	bool job_active;
+	u32 js_status;
 
 	/*
 	 * If the GPU managed to complete this jobs fence, the timeout has
@@ -820,18 +962,51 @@ 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;
+
+	/* A job that completed after the checks above is not active either:
+	 * its interrupt is then pending, or being handled.
+	 */
+	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;
+	}
+
+	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);
 
-	panfrost_core_dump(job);
+		panfrost_core_dump(job);
+	}
 
-	atomic_set(&pfdev->reset.pending, 1);
-	panfrost_reset(pfdev, sched_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);
 
 	return DRM_GPU_SCHED_STAT_RESET;
 }
@@ -854,7 +1029,7 @@ static void panfrost_reset_work(struct work_struct *work)
 		return;
 	}
 
-	panfrost_reset(pfdev, NULL);
+	panfrost_reset(pfdev, NULL, false);
 	pm_runtime_put_autosuspend(pfdev->base.dev);
 }
 

-- 
2.55.0

[PATCH v2 10/11] drm/panfrost: Soft-stop and resume a job that outlives the timeout

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

JOB_TIMEOUT_MS is 500 ms, and when it fires panfrost resets the GPU and
the job is lost. But a job that runs longer is not necessarily hung:
OpenCL kernels and heavy fragment jobs legitimately take seconds on a
small GPU. On a Mali-G31 (one shader core) that breaks, among others:

- OpenCL CTS Integer Ops quick_long_math and quick_ulong_math (long16
  64-bit division, > 500 ms per kernel): each subtest logs

    panfrost 1800000.gpu: gpu sched timeout, js=1, config=0x7300, status=0x8, head=0xa0161c0, tail=0xa0161c0

  (status 0x8 = ACTIVE) and fails with the work-items after the reset
  never written; Thread Dimensions quick_2d/3d lose whole rows the same
  way.
- KHR-GLES31.core.blend_equation_advanced.test_coherency.mixedSequence:
  a 44-draw fragment job on a 1024x1024 target needs about 0.7 s, and
  the tiles it had not reached at 500 ms read back as 0,0,0,0.
- dEQP-VK.spirv_assembly.type.vec4.*_frag cases (a loop over the whole
  dataset in every fragment): "Incomplete job or timeout".

The Arm driver time-slices instead: a job is soft-stopped after its
time slice and resumed later, it is hard-stopped only when it has not
stopped after 5 s (JS_HARD_STOP_TICKS_SS = 50 ticks of 100 ms), and the
GPU is reset only when the hard stop has not taken effect 0.5 s after
that (JS_RESET_TICKS_SS = 55). Do the same from the timeout handler:

- a slot whose job is still ACTIVE when the timeout fires is
  soft-stopped and its job resumed from JS_TAIL, without a GPU reset and
  without raising its karma; a soft-stop takes effect at a task boundary
  only, after the tasks already issued have completed, so such a slot
  gets JOB_SOFT_STOP_TIMEOUT_MS (5 s) to go idle, idle or faulted slots
  keep the 10 ms;
- a slot still active after that is hard-stopped, which ends its job at
  once with TERMINATED (-ECANCELED); the stopped jobs on the other slots
  are resumed, nothing is reset. The job that did not stop is hung, so
  it gets its karma raised and a devcoredump, as a hung job did before;
- the GPU is reset when the hard stop has not taken effect after
  JOB_HARD_STOP_TIMEOUT_MS (0.5 s); the job that did not stop is then
  failed with -ECANCELED instead of being restarted from scratch, where
  it would only time out again. Jobs that were only given the 10 ms of a
  reset for another reason are restarted, as before;
- a reset that is asked for while the slots are being stopped (a job
  fault that needs one, the MMU) is not lost: the timeout only marks
  reset.pending for a stop (PANFROST_RESET_STOP_ONLY), a request for a
  reset overrides that, and the GPU is then reset;
- a job that completes between the timeout handler's fence check and its
  JS_STATUS read is not taken for a hung job: with its DONE interrupt
  pending or being handled, the timeout is spurious;
- without a reset, the address space references of the jobs cancelled
  from _NEXT are released (a reset has panfrost_mmu_reset() drop them),
  and a runtime PM reference is held across the stop and the
  resubmission, so that an autosuspend cannot power the GPU off between
  the two.

The timeout of a job that is merely long is now a dev_dbg(); faults,
jobs that do not stop and the other reset paths are reported as before,
with the status of every slot.

With this, on the Mali-G31 the Integer Ops, Thread Dimensions and
mixedSequence cases pass, and the two vec4 i8/u64 bit_field_insert
count64 fragment cases, which need several seconds to reach a task
boundary, pass in 4-6 s.

Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_job.c | 239 +++++++++++++++++++++++++++-----
 1 file changed, 207 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 28842c572..a920d112a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -27,6 +27,25 @@
 #define MAX_JM_CTX_PER_FILE 64
 #define JOB_TIMEOUT_MS 500
 
+/* How long a soft-stopped slot gets to reach a task boundary before its job is
+ * hard-stopped, and how long the hard stop gets before the GPU is reset. These
+ * follow the Arm driver's job slot policy (mali_kbase_config_defaults.h): a job
+ * is soft-stopped after its 100 ms time slice (JS_SOFT_STOP_TICKS = 1 tick),
+ * hard-stopped when it has not stopped by tick 50 (JS_HARD_STOP_TICKS_SS, 5 s)
+ * and the GPU is reset at tick 55 (JS_RESET_TICKS_SS, 5.5 s). A soft-stop
+ * takes effect at a task boundary only, after the tasks already issued (a
+ * compute job's work-groups, a fragment job's tiles) have run to completion,
+ * and a heavy shader needs seconds for that.
+ */
+#define JOB_SOFT_STOP_TIMEOUT_MS 5000
+#define JOB_HARD_STOP_TIMEOUT_MS 500
+
+/* reset.pending while panfrost_reset() is only to stop the slots, to resume
+ * the jobs of a timeout that found its job still running. A request for a
+ * real reset (1) that comes in meanwhile overrides it.
+ */
+#define PANFROST_RESET_STOP_ONLY 2
+
 #define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
 #define job_read(dev, reg) readl(dev->iomem + (reg))
 
@@ -165,8 +184,9 @@ panfrost_dequeue_job(struct panfrost_device *pfdev, int slot)
 {
 	struct panfrost_job *job = pfdev->jobs[slot][0];
 
-	/* An event for a slot we have no job on. Not fatal, as every caller
-	 * copes with a NULL job; a WARN, not an Oops.
+	/* An event for a slot we have no job on: a resubmission raced with
+	 * the first run of the job (see panfrost_reset()). Not fatal, as
+	 * every caller copes with a NULL job; a WARN, not an Oops.
 	 */
 	if (WARN_ON(!job))
 		return NULL;
@@ -660,18 +680,39 @@ static bool panfrost_slots_idle(struct panfrost_device *pfdev)
 	return true;
 }
 
+/* The job running on a slot: the one in _NEXT once the first has completed,
+ * with its interrupt not handled yet.
+ */
+static struct panfrost_job *
+panfrost_running_job(struct panfrost_device *pfdev, unsigned int slot)
+{
+	if (pfdev->jobs[slot][1] &&
+	    (job_read(pfdev, JOB_INT_RAWSTAT) & JOB_INT_MASK_DONE(slot)))
+		return pfdev->jobs[slot][1];
+
+	return pfdev->jobs[slot][0];
+}
+
 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 jobs we stop drop their runtime PM references before we submit
+	 * them again. Keep the GPU powered in between: once they have, nothing
+	 * else keeps it from being runtime suspended under the rest of this
+	 * function.
+	 */
+	pm_runtime_get_noresume(pfdev->base.dev);
+
 	/* Stop the schedulers.
 	 *
 	 * FIXME: We temporarily get out of the dma_fence_signalling section
@@ -690,7 +731,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);
 
 	/* Mask job interrupts and synchronize to make sure we won't be
@@ -722,26 +764,90 @@ panfrost_reset(struct panfrost_device *pfdev,
 	}
 	spin_unlock(&pfdev->js->job_lock);
 
-	/* 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);
 
+	if (ret && job_active) {
+		dev_err(pfdev->base.dev,
+			"Soft-stop failed after %u ms (js status 0x%x 0x%x 0x%x), hard-stopping the active slots\n",
+			JOB_SOFT_STOP_TIMEOUT_MS,
+			job_read(pfdev, JS_STATUS(0)), job_read(pfdev, JS_STATUS(1)),
+			job_read(pfdev, JS_STATUS(2)));
+
+		/* A job that has not reached a task boundary in all that time
+		 * is hung, not long: count it against its context and dump
+		 * it, as a timeout that resets the GPU does.
+		 */
+		for (i = 0; i < NUM_JOB_SLOTS; i++) {
+			if (job_read(pfdev, JS_STATUS(i)) ==
+			    DRM_PANFROST_EXCEPTION_ACTIVE)
+				hung[i] = panfrost_running_job(pfdev, i);
+			if (hung[i])
+				drm_sched_increase_karma(&hung[i]->base);
+		}
+
+		/* The dump allocates memory, which the fence signalling
+		 * section does not allow (see the resubmission below).
+		 */
+		dma_fence_end_signalling(cookie);
+		for (i = 0; i < NUM_JOB_SLOTS; i++) {
+			if (hung[i])
+				panfrost_core_dump(hung[i]);
+		}
+		cookie = dma_fence_begin_signalling();
+
+		for (i = 0; i < NUM_JOB_SLOTS; i++) {
+			if (job_read(pfdev, JS_STATUS(i)) ==
+			    DRM_PANFROST_EXCEPTION_ACTIVE)
+				job_write(pfdev, JS_COMMAND(i), JS_COMMAND_HARD_STOP);
+		}
+
+		ret = read_poll_timeout(panfrost_slots_idle, idle, idle, 200,
+					JOB_HARD_STOP_TIMEOUT_MS * 1000, false,
+					pfdev);
+		stop_failed = ret;
+	}
+
 	if (ret)
-		dev_err(pfdev->base.dev, "Soft-stop failed\n");
+		dev_err(pfdev->base.dev,
+			"%s failed (js status 0x%x 0x%x 0x%x), resetting the GPU\n",
+			job_active ? "Hard-stop" : "Soft-stop",
+			job_read(pfdev, JS_STATUS(0)), job_read(pfdev, JS_STATUS(1)),
+			job_read(pfdev, JS_STATUS(2)));
 
 	/* Handle the remaining interrupts before we reset. */
 	panfrost_jm_handle_irqs(pfdev);
 
+	/* Reset the GPU unless the slots only had to be stopped and did stop,
+	 * and nobody asked for a reset meanwhile: a job fault that needs one
+	 * (in the interrupts just handled) or the MMU.
+	 */
+	reset_gpu = !job_active || stop_failed ||
+		    atomic_read(&pfdev->reset.pending) != PANFROST_RESET_STOP_ONLY;
+
 	/* 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++) {
@@ -752,21 +858,53 @@ panfrost_reset(struct panfrost_device *pfdev,
 			pfdev->jobs[i][pfdev->jobs[i][0] ? 1 : 0] = evicted[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 (!reset_gpu)
+				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. A job that was only
+			 * given the 10 ms is restarted, as before.
+			 */
+			if (stop_failed && job == hung[i])
+				dma_fence_set_error(&job->base.s_fence->finished,
+						    -ECANCELED);
 		}
 	}
 	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()).
+	 */
+	if (reset_gpu) {
+		panfrost_device_reset(pfdev, false);
 
-	/* GPU has been reset, we can clear the reset pending bit. */
-	atomic_set(&pfdev->reset.pending, 0);
+		/* GPU has been reset, we can clear the reset pending bit. */
+		atomic_set(&pfdev->reset.pending, 0);
+	} else {
+		/* Leave a reset that was asked for since we looked to the
+		 * reset work or timeout handler that asked for it.
+		 */
+		atomic_cmpxchg(&pfdev->reset.pending,
+			       PANFROST_RESET_STOP_ONLY, 0);
+	}
 
 	/* Now resubmit jobs that were previously queued but didn't have a
 	 * chance to finish.
@@ -788,6 +926,8 @@ panfrost_reset(struct panfrost_device *pfdev,
 	panfrost_jm_enable_interrupts(pfdev);
 
 	dma_fence_end_signalling(cookie);
+
+	pm_runtime_put_autosuspend(pfdev->base.dev);
 }
 
 static enum drm_gpu_sched_stat panfrost_job_timedout(struct drm_sched_job
@@ -796,6 +936,8 @@ static enum drm_gpu_sched_stat panfrost_job_timedout(struct drm_sched_job
 	struct panfrost_job *job = to_panfrost_job(sched_job);
 	struct panfrost_device *pfdev = job->pfdev;
 	int js = panfrost_job_get_slot(job);
+	bool job_active;
+	u32 js_status;
 
 	/*
 	 * If the GPU managed to complete this jobs fence, the timeout has
@@ -820,18 +962,51 @@ 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;
+
+	/* A job that completed after the checks above is not active either:
+	 * its interrupt is then pending, or being handled.
+	 */
+	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;
+	}
+
+	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);
 
-	panfrost_core_dump(job);
+		panfrost_core_dump(job);
+	}
 
-	atomic_set(&pfdev->reset.pending, 1);
-	panfrost_reset(pfdev, sched_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);
 
 	return DRM_GPU_SCHED_STAT_RESET;
 }
@@ -854,7 +1029,7 @@ static void panfrost_reset_work(struct work_struct *work)
 		return;
 	}
 
-	panfrost_reset(pfdev, NULL);
+	panfrost_reset(pfdev, NULL, false);
 	pm_runtime_put_autosuspend(pfdev->base.dev);
 }
 

-- 
2.55.0

Re: [PATCH v2 10/11] drm/panfrost: Soft-stop and resume a job that outlives the timeout

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/panfrost: cycle counter refcount underflow in
  panfrost_reset()
- [High] drm/panfrost: unstarted _NEXT job marked done in
  panfrost_jm_handle_irq()
--

commit 8e3c3b18f9b16cbaedc76fbae266b58fbe0f8950
Author: Arun Iyengar <aiyengar1990@gmail.com>

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

This commit soft-stops and resumes jobs that are still active when the
scheduler timeout fires, escalating to a hard-stop after 5 seconds and a
full GPU reset only if the hard-stop fails or another reset was requested.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 28842c5724868..a920d112a67dd 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c

[ ... ]

> @@ -752,21 +858,53 @@ panfrost_reset(struct panfrost_device *pfdev,
>  			pfdev->jobs[i][pfdev->jobs[i][0] ? 1 : 0] = evicted[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 (!reset_gpu)
> +				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. A job that was only
> +			 * given the 10 ms is restarted, as before.
> +			 */
> +			if (stop_failed && job == hung[i])
> +				dma_fence_set_error(&job->base.s_fence->finished,
> +						    -ECANCELED);
>  		}
>  	}
>  	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()).
> +	 */
> +	if (reset_gpu) {
> +		panfrost_device_reset(pfdev, false);
>  
> -	/* GPU has been reset, we can clear the reset pending bit. */
> -	atomic_set(&pfdev->reset.pending, 0);
> +		/* GPU has been reset, we can clear the reset pending bit. */
> +		atomic_set(&pfdev->reset.pending, 0);
> +	} else {
> +		/* Leave a reset that was asked for since we looked to the
> +		 * reset work or timeout handler that asked for it.
> +		 */
> +		atomic_cmpxchg(&pfdev->reset.pending,
> +			       PANFROST_RESET_STOP_ONLY, 0);
> +	}
>  
>  	/* Now resubmit jobs that were previously queued but didn't have a
>  	 * chance to finish.

[Severity: High]
Can this underflow pfdev->cycle_counter.use_count when a job is submitted
while pfdev->reset.pending is non-zero?

In panfrost_job_hw_submit(), panfrost_enqueue_job() records the job in
pfdev->jobs[js][subslot] unconditionally, but updating job->is_profiled and
calling panfrost_cycle_counter_get() are skipped when pfdev->reset.pending
is set:

drivers/gpu/drm/panfrost/panfrost_job.c:panfrost_job_hw_submit() {
    ...
	subslot = panfrost_enqueue_job(pfdev, js, job);
	/* Don't queue the job if a reset is in progress */
	if (!atomic_read(&pfdev->reset.pending)) {
		job->is_profiled = pfdev->profile_mode;

		if (job->requirements & PANFROST_JD_REQ_CYCLE_COUNT ||
		    job->is_profiled)
			panfrost_cycle_counter_get(pfdev);
    ...
}

If a reset is requested after reset_gpu is evaluated as false,
atomic_cmpxchg() leaves pfdev->reset.pending set to 1 while
drm_sched_resubmit_jobs() runs (or another slot's scheduler submits a job
after panfrost_job_timedout() sets PANFROST_RESET_STOP_ONLY and before
drm_sched_stop()).  A job with PANFROST_JD_REQ_CYCLE_COUNT, or a
soft-stopped job whose job->is_profiled stayed true after
panfrost_dequeue_job() already dropped its reference, is then queued in
pfdev->jobs[js][subslot] without a matching panfrost_cycle_counter_get().

When panfrost_reset() subsequently drains pfdev->jobs[i][j], it calls
panfrost_cycle_counter_put() for that job and decrements use_count below 0.
Without a GPU reset to reinitialize use_count, the underflowed counter stops
panfrost_cycle_counter_get() from starting the hardware cycle counter and
triggers the drm_WARN_ON() in panfrost_gpu_soft_reset() on the next reset.

[ ... ]

> @@ -820,18 +962,51 @@ 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;
> +
> +	/* A job that completed after the checks above is not active either:
> +	 * its interrupt is then pending, or being handled.
> +	 */
> +	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;
> +	}
> +
> +	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);
>  
> -	panfrost_core_dump(job);
> +		panfrost_core_dump(job);
> +	}
>  
> -	atomic_set(&pfdev->reset.pending, 1);
> -	panfrost_reset(pfdev, sched_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);
>  
>  	return DRM_GPU_SCHED_STAT_RESET;
>  }

[Severity: High]
Can setting pfdev->reset.pending before stopping the schedulers and masking
interrupts cause a second job queued on another slot to be signaled as done
without ever running on the GPU?

CPU0                              CPU1
panfrost_job_timedout()
  atomic_cmpxchg(&pfdev->reset.pending,
                 0, PANFROST_RESET_STOP_ONLY)
                                  panfrost_job_hw_submit() [job1]
                                    panfrost_enqueue_job()
                                      pfdev->jobs[j][1] = job1
                                    if (!atomic_read(&pfdev->reset.pending))
                                      /* skipped: _NEXT not started */
                                  panfrost_jm_irq_handler_thread()
                                    panfrost_jm_handle_irq()
                                      /* job0 finishes; job1 moves to [0] */
                                      done[j][0] = panfrost_dequeue_job()
                                      if (pfdev->jobs[j][0] &&
                                          !(js_state & MK_JS_MASK(j)))
                                        done[j][1] = panfrost_dequeue_job()
                                      panfrost_jm_handle_done(..., job1)
                                        <-- signals fence without running
  panfrost_reset()
    drm_sched_stop()
    job_write(pfdev, JOB_INT_MASK, 0)

When a 500 ms time-slice timeout fires on one slot, pfdev->reset.pending is
set to PANFROST_RESET_STOP_ONLY before panfrost_reset() stops the schedulers
and masks job interrupts.  If another slot is running job0 and submits job1
in that window, panfrost_job_hw_submit() puts job1 into pfdev->jobs[j][1]
but skips writing JS_COMMAND_START to JS_COMMAND_NEXT(j).

If job0 finishes before JOB_INT_MASK is cleared, panfrost_jm_handle_irq()
shifts job1 into pfdev->jobs[j][0], sees both hardware subslots inactive in
js_state, and dequeues job1 into done[j][1] as if the hardware had
auto-started and completed it.  panfrost_jm_handle_done() then sets
job1->jc = 0 and signals job1->done_fence, causing drm_sched_resubmit_jobs()
to treat job1 as finished even though it never ran on the GPU.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-panfrost-long-jobs-v2-0-a06830da3d99@gmail.com?part=10

[PATCH v2 11/11] drm/panfrost: Name the BOs around an unhandled page fault

[Arun Iyengar]

An unhandled GPU page fault only reports the faulting VA. That does not
tell a GPU access just past the end of a buffer (a userspace or hardware
overrun) from a stale or garbage pointer.

After the fault report, look the address up in the faulting address
space and print the BO that contains it, or else the nearest BO below
and above it: VA range, size, the distance of the fault from it,
heap/noexec, and the label userspace gave it (Mesa labels its BOs with
their use, format and size). A BO that has been freed is no longer in the
address space, so an access to it shows up as a hole between its
neighbours. Mappings whose last reference is gone, which stay in the
address space until their teardown takes mm_lock, are skipped. For an
OpenCL image read on a Mali-G31 this gave (with an earlier version of
this patch, which also printed whether the BO was mapped):

  panfrost 1800000.gpu: Unhandled Page fault in AS0 at VA 0x000000000A181000
  ...
  panfrost 1800000.gpu: fault bo: below: VA [0xa041000-0xa181000) size 0x140000, fault at end+0x0, mapped, noexec, label "Texture format=r32g32b32_sint extent=25x128x32 array_size=1 mip_count=0 samples=0 modifier=0x0"

that is, the texture unit reading past the end of a 96-bit-per-texel
texture, which userspace can avoid by padding such textures.

Only the unhandled fault path is affected; heap growth faults are not.

Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 88 +++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 684735937..9c255daf2 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -601,6 +601,93 @@ addr_to_mapping(struct panfrost_device *pfdev, int as, u64 addr)
 	return mapping;
 }
 
+static void panfrost_mmu_print_fault_bo(struct panfrost_device *pfdev,
+					struct panfrost_gem_mapping *m,
+					u64 addr, const char *where)
+{
+	struct panfrost_gem_object *bo = m->obj;
+	u64 start = m->mmnode.start << PAGE_SHIFT;
+	u64 end = (m->mmnode.start + m->mmnode.size) << PAGE_SHIFT;
+	u64 dist = addr < start ? start - addr : addr >= end ? addr - end : addr - start;
+
+	mutex_lock(&bo->label.lock);
+	dev_err(pfdev->base.dev,
+		"fault bo: %s: VA [0x%llx-0x%llx) size 0x%zx, fault at %s0x%llx%s%s, label \"%s\"\n",
+		where, start, end, bo->base.base.size,
+		addr < start ? "-" : addr >= end ? "end+" : "start+", dist,
+		bo->is_heap ? ", heap" : "", bo->noexec ? ", noexec" : "",
+		bo->label.str ? bo->label.str : "");
+	mutex_unlock(&bo->label.lock);
+}
+
+/*
+ * Report the BOs around an unhandled fault address in the faulting address
+ * space: the one containing it (if any), else the nearest one below and above.
+ * An access just past the end of a BO that a hole follows is a GPU overrun of
+ * that BO (userspace or hardware); an address far from any BO is a garbage
+ * pointer, or a stale one: a BO that has been freed has left the address
+ * space, and its range shows up as a hole between its neighbours.
+ */
+static void panfrost_mmu_report_fault_bos(struct panfrost_device *pfdev,
+					  int as, u64 addr)
+{
+	struct panfrost_gem_mapping *below = NULL, *above = NULL;
+	u64 offset = addr >> PAGE_SHIFT;
+	struct drm_mm_node *node;
+	struct panfrost_mmu *mmu;
+	bool inside = false;
+
+	spin_lock(&pfdev->as_lock);
+	list_for_each_entry(mmu, &pfdev->as_lru_list, list) {
+		if (as == mmu->as)
+			goto found_mmu;
+	}
+	spin_unlock(&pfdev->as_lock);
+	dev_err(pfdev->base.dev, "fault bo: no context holds AS%d\n", as);
+	return;
+
+found_mmu:
+	spin_lock(&mmu->mm_lock);
+	/* The node list is sorted by address. A mapping whose last reference
+	 * is gone is being freed, and only waits for mm_lock to leave the
+	 * list: skip it.
+	 */
+	drm_mm_for_each_node(node, &mmu->mm) {
+		struct panfrost_gem_mapping *m = drm_mm_node_to_panfrost_mapping(node);
+
+		if (!kref_read(&m->refcount))
+			continue;
+		if (node->start > offset) {
+			above = m;
+			break;
+		}
+		below = m;
+	}
+	if (below) {
+		inside = offset < below->mmnode.start + below->mmnode.size;
+		if (inside)
+			above = NULL;
+		if (!kref_get_unless_zero(&below->refcount))
+			below = NULL;
+	}
+	if (above && !kref_get_unless_zero(&above->refcount))
+		above = NULL;
+	spin_unlock(&mmu->mm_lock);
+	spin_unlock(&pfdev->as_lock);
+
+	if (!below && !above)
+		dev_err(pfdev->base.dev, "fault bo: AS%d has no BO\n", as);
+	if (below) {
+		panfrost_mmu_print_fault_bo(pfdev, below, addr,
+					    inside ? "inside" : "below");
+		panfrost_gem_mapping_put(below);
+	}
+	if (above) {
+		panfrost_mmu_print_fault_bo(pfdev, above, addr, "above");
+		panfrost_gem_mapping_put(above);
+	}
+}
+
 #define NUM_FAULT_PAGES (SZ_2M / PAGE_SIZE)
 
 /*
@@ -984,6 +1071,7 @@ static irqreturn_t panfrost_mmu_irq_handler_thread(int irq, void *data)
 				exception_type, panfrost_exception_name(exception_type),
 				access_type, access_type_name(pfdev, fault_status),
 				source_id);
+			panfrost_mmu_report_fault_bos(pfdev, as, addr);
 
 			spin_lock(&pfdev->as_lock);
 			/* Ignore MMU interrupts on this AS until it's been

-- 
2.55.0

[PATCH v2 11/11] drm/panfrost: Name the BOs around an unhandled page fault

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

An unhandled GPU page fault only reports the faulting VA. That does not
tell a GPU access just past the end of a buffer (a userspace or hardware
overrun) from a stale or garbage pointer.

After the fault report, look the address up in the faulting address
space and print the BO that contains it, or else the nearest BO below
and above it: VA range, size, the distance of the fault from it,
heap/noexec, and the label userspace gave it (Mesa labels its BOs with
their use, format and size). A BO that has been freed is no longer in the
address space, so an access to it shows up as a hole between its
neighbours. Mappings whose last reference is gone, which stay in the
address space until their teardown takes mm_lock, are skipped. For an
OpenCL image read on a Mali-G31 this gave (with an earlier version of
this patch, which also printed whether the BO was mapped):

  panfrost 1800000.gpu: Unhandled Page fault in AS0 at VA 0x000000000A181000
  ...
  panfrost 1800000.gpu: fault bo: below: VA [0xa041000-0xa181000) size 0x140000, fault at end+0x0, mapped, noexec, label "Texture format=r32g32b32_sint extent=25x128x32 array_size=1 mip_count=0 samples=0 modifier=0x0"

that is, the texture unit reading past the end of a 96-bit-per-texel
texture, which userspace can avoid by padding such textures.

Only the unhandled fault path is affected; heap growth faults are not.

Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 88 +++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 684735937..9c255daf2 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -601,6 +601,93 @@ addr_to_mapping(struct panfrost_device *pfdev, int as, u64 addr)
 	return mapping;
 }
 
+static void panfrost_mmu_print_fault_bo(struct panfrost_device *pfdev,
+					struct panfrost_gem_mapping *m,
+					u64 addr, const char *where)
+{
+	struct panfrost_gem_object *bo = m->obj;
+	u64 start = m->mmnode.start << PAGE_SHIFT;
+	u64 end = (m->mmnode.start + m->mmnode.size) << PAGE_SHIFT;
+	u64 dist = addr < start ? start - addr : addr >= end ? addr - end : addr - start;
+
+	mutex_lock(&bo->label.lock);
+	dev_err(pfdev->base.dev,
+		"fault bo: %s: VA [0x%llx-0x%llx) size 0x%zx, fault at %s0x%llx%s%s, label \"%s\"\n",
+		where, start, end, bo->base.base.size,
+		addr < start ? "-" : addr >= end ? "end+" : "start+", dist,
+		bo->is_heap ? ", heap" : "", bo->noexec ? ", noexec" : "",
+		bo->label.str ? bo->label.str : "");
+	mutex_unlock(&bo->label.lock);
+}
+
+/*
+ * Report the BOs around an unhandled fault address in the faulting address
+ * space: the one containing it (if any), else the nearest one below and above.
+ * An access just past the end of a BO that a hole follows is a GPU overrun of
+ * that BO (userspace or hardware); an address far from any BO is a garbage
+ * pointer, or a stale one: a BO that has been freed has left the address
+ * space, and its range shows up as a hole between its neighbours.
+ */
+static void panfrost_mmu_report_fault_bos(struct panfrost_device *pfdev,
+					  int as, u64 addr)
+{
+	struct panfrost_gem_mapping *below = NULL, *above = NULL;
+	u64 offset = addr >> PAGE_SHIFT;
+	struct drm_mm_node *node;
+	struct panfrost_mmu *mmu;
+	bool inside = false;
+
+	spin_lock(&pfdev->as_lock);
+	list_for_each_entry(mmu, &pfdev->as_lru_list, list) {
+		if (as == mmu->as)
+			goto found_mmu;
+	}
+	spin_unlock(&pfdev->as_lock);
+	dev_err(pfdev->base.dev, "fault bo: no context holds AS%d\n", as);
+	return;
+
+found_mmu:
+	spin_lock(&mmu->mm_lock);
+	/* The node list is sorted by address. A mapping whose last reference
+	 * is gone is being freed, and only waits for mm_lock to leave the
+	 * list: skip it.
+	 */
+	drm_mm_for_each_node(node, &mmu->mm) {
+		struct panfrost_gem_mapping *m = drm_mm_node_to_panfrost_mapping(node);
+
+		if (!kref_read(&m->refcount))
+			continue;
+		if (node->start > offset) {
+			above = m;
+			break;
+		}
+		below = m;
+	}
+	if (below) {
+		inside = offset < below->mmnode.start + below->mmnode.size;
+		if (inside)
+			above = NULL;
+		if (!kref_get_unless_zero(&below->refcount))
+			below = NULL;
+	}
+	if (above && !kref_get_unless_zero(&above->refcount))
+		above = NULL;
+	spin_unlock(&mmu->mm_lock);
+	spin_unlock(&pfdev->as_lock);
+
+	if (!below && !above)
+		dev_err(pfdev->base.dev, "fault bo: AS%d has no BO\n", as);
+	if (below) {
+		panfrost_mmu_print_fault_bo(pfdev, below, addr,
+					    inside ? "inside" : "below");
+		panfrost_gem_mapping_put(below);
+	}
+	if (above) {
+		panfrost_mmu_print_fault_bo(pfdev, above, addr, "above");
+		panfrost_gem_mapping_put(above);
+	}
+}
+
 #define NUM_FAULT_PAGES (SZ_2M / PAGE_SIZE)
 
 /*
@@ -984,6 +1071,7 @@ static irqreturn_t panfrost_mmu_irq_handler_thread(int irq, void *data)
 				exception_type, panfrost_exception_name(exception_type),
 				access_type, access_type_name(pfdev, fault_status),
 				source_id);
+			panfrost_mmu_report_fault_bos(pfdev, as, addr);
 
 			spin_lock(&pfdev->as_lock);
 			/* Ignore MMU interrupts on this AS until it's been

-- 
2.55.0