[PATCH 0/7] drm/panfrost: Resume long jobs instead of resetting, and mapping error path fixes

[PATCH 0/7] 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).

Patches 1-3 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 4-6 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 4 makes sure a soft-stopped job is
never resumed after a GPU reset (its progress is in the L2 that the
reset discards), and patch 5 makes panfrost_reset() actually wait for
the soft-stops (JOB_INT_JS_STATE is a snapshot, not the live state);
both are needed by patch 6 and fix the reset path on their own.

Patch 7 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.

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 4-6 s (timed out before); KHR-GLES31 blend_equation_advanced
  test_coherency.mixedSequence passes (failed before); none of them
  resets the GPU;
- a never-ending OpenCL kernel is hard-stopped after 5 s and the GPU
  keeps working;
- 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;
- no regressions in dEQP-GLES3 draw/occlusion_query/shaders.loops,
  dEQP-GLES31 compute, KHR-GLES31 compute_shader and a set of OpenCL
  CTS groups.
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.

---
Arun Iyengar (7):
      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 resume a soft-stopped job after a GPU 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_device.h |   5 +
 drivers/gpu/drm/panfrost/panfrost_job.c    | 214 +++++++++++++++++++++++------
 drivers/gpu/drm/panfrost/panfrost_job.h    |   8 ++
 drivers/gpu/drm/panfrost/panfrost_mmu.c    | 139 +++++++++++++++++--
 4 files changed, 312 insertions(+), 54 deletions(-)
---
base-commit: 8deaade4a5a4ff635df91e246e42139ab2b33f6d
change-id: 20261110-panfrost-long-jobs-1643287a2ac1

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

[PATCH 0/7] 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).

Patches 1-3 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 4-6 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 4 makes sure a soft-stopped job is
never resumed after a GPU reset (its progress is in the L2 that the
reset discards), and patch 5 makes panfrost_reset() actually wait for
the soft-stops (JOB_INT_JS_STATE is a snapshot, not the live state);
both are needed by patch 6 and fix the reset path on their own.

Patch 7 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.

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 4-6 s (timed out before); KHR-GLES31 blend_equation_advanced
  test_coherency.mixedSequence passes (failed before); none of them
  resets the GPU;
- a never-ending OpenCL kernel is hard-stopped after 5 s and the GPU
  keeps working;
- 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;
- no regressions in dEQP-GLES3 draw/occlusion_query/shaders.loops,
  dEQP-GLES31 compute, KHR-GLES31 compute_shader and a set of OpenCL
  CTS groups.
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.

---
Arun Iyengar (7):
      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 resume a soft-stopped job after a GPU 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_device.h |   5 +
 drivers/gpu/drm/panfrost/panfrost_job.c    | 214 +++++++++++++++++++++++------
 drivers/gpu/drm/panfrost/panfrost_job.h    |   8 ++
 drivers/gpu/drm/panfrost/panfrost_mmu.c    | 139 +++++++++++++++++--
 4 files changed, 312 insertions(+), 54 deletions(-)
---
base-commit: 8deaade4a5a4ff635df91e246e42139ab2b33f6d
change-id: 20261110-panfrost-long-jobs-1643287a2ac1

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

[PATCH 1/7] 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 faa62643c..56e642f73 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 1/7] 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 faa62643c..56e642f73 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 2/7] 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 56e642f73..ba37da499 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 2/7] 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 56e642f73..ba37da499 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 3/7] 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.

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 | 34 +++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index ba37da499..13728b0c4 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -597,6 +597,28 @@ addr_to_mapping(struct panfrost_device *pfdev, int as, u64 addr)
 
 #define NUM_FAULT_PAGES (SZ_2M / PAGE_SIZE)
 
+static void panfrost_heap_put_chunk(struct page **pages, pgoff_t start,
+				    pgoff_t end)
+{
+	pgoff_t pg = start;
+
+	while (pg < end) {
+		struct folio *folio;
+		pgoff_t n;
+
+		if (!pages[pg]) {
+			pg++;
+			continue;
+		}
+
+		folio = page_folio(pages[pg]);
+		n = folio_nr_pages(folio);
+		for (pgoff_t i = 0; i < n && pg < end; i++)
+			pages[pg++] = NULL;
+		folio_put(folio);
+	}
+}
+
 static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 				       u64 addr)
 {
@@ -671,7 +693,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 		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);
@@ -693,7 +715,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 	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)
@@ -720,6 +742,14 @@ 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_chunk(pages, page_offset,
+				min_t(pgoff_t, page_offset + NUM_FAULT_PAGES, nr_pages));
 err_unlock:
 	dma_resv_unlock(obj->resv);
 err_bo:

-- 
2.55.0

[PATCH 3/7] 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.

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 | 34 +++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index ba37da499..13728b0c4 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -597,6 +597,28 @@ addr_to_mapping(struct panfrost_device *pfdev, int as, u64 addr)
 
 #define NUM_FAULT_PAGES (SZ_2M / PAGE_SIZE)
 
+static void panfrost_heap_put_chunk(struct page **pages, pgoff_t start,
+				    pgoff_t end)
+{
+	pgoff_t pg = start;
+
+	while (pg < end) {
+		struct folio *folio;
+		pgoff_t n;
+
+		if (!pages[pg]) {
+			pg++;
+			continue;
+		}
+
+		folio = page_folio(pages[pg]);
+		n = folio_nr_pages(folio);
+		for (pgoff_t i = 0; i < n && pg < end; i++)
+			pages[pg++] = NULL;
+		folio_put(folio);
+	}
+}
+
 static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 				       u64 addr)
 {
@@ -671,7 +693,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 		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);
@@ -693,7 +715,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 	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)
@@ -720,6 +742,14 @@ 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_chunk(pages, page_offset,
+				min_t(pgoff_t, page_offset + NUM_FAULT_PAGES, nr_pages));
 err_unlock:
 	dma_resv_unlock(obj->resv);
 err_bo:

-- 
2.55.0

Re: [PATCH 3/7] 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: partial cleanup of >2MB folios in
  panfrost_heap_put_chunk()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] drm/panfrost: NULL pointer dereference in
  panfrost_gem_sync() on heap BOs [1]
--

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

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

This commit adds panfrost_heap_put_chunk() to release folios and clear page
array entries when mapping a 2 MB heap chunk fails in
panfrost_mmu_map_fault_addr().

> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index ba37da4992307..13728b0c46efc 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -597,6 +597,28 @@ addr_to_mapping(struct panfrost_device *pfdev, int as, u64 addr)
>  
>  #define NUM_FAULT_PAGES (SZ_2M / PAGE_SIZE)
>  
> +static void panfrost_heap_put_chunk(struct page **pages, pgoff_t start,
> +				    pgoff_t end)
> +{
> +	pgoff_t pg = start;
> +
> +	while (pg < end) {
> +		struct folio *folio;
> +		pgoff_t n;
> +
> +		if (!pages[pg]) {
> +			pg++;
> +			continue;
> +		}
> +
> +		folio = page_folio(pages[pg]);
> +		n = folio_nr_pages(folio);
> +		for (pgoff_t i = 0; i < n && pg < end; i++)
> +			pages[pg++] = NULL;
> +		folio_put(folio);
> +	}
> +}
> +
>  static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>  				       u64 addr)
>  {
> @@ -671,7 +693,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>  		folio = shmem_read_folio(mapping, pg);
>  		if (IS_ERR(folio)) {
>  			ret = PTR_ERR(folio);
> -			goto err_unlock;
> +			goto err_put_chunk;
>  		}

[Severity: Critical]
This problem wasn't introduced by this patch, but can calling
panfrost_gem_sync() on a heap BO [1] cause a NULL pointer dereference?

On coherent devices (pfdev->coherent is true), should_map_wc() returns
false, so panfrost_gem_sync() does not return early on shmem->map_wc and
calls drm_gem_shmem_get_pages_sgt():

drivers/gpu/drm/panfrost/panfrost_gem.c:panfrost_gem_sync() {
	...
	if (shmem->map_wc)
		return 0;

	/* Nothing to do if the size is zero. */
	if (size == 0)
		return 0;

	sgt = drm_gem_shmem_get_pages_sgt(shmem);
	...
}

If a heap BO has already faulted in at least one chunk, bo->base.pages is
non-NULL with NULL entries for unfaulted chunks and bo->base.sgt is NULL, so
drm_gem_shmem_get_pages_sgt() passes the sparse bo->base.pages array to
drm_prime_pages_to_sg() and dereferences NULL page pointers.  Conversely, if
panfrost_gem_sync() runs before the first GPU fault on the heap BO,
drm_gem_shmem_get_pages_sgt() populates bo->base.pages while leaving
bo->sgts NULL, and a later fault in panfrost_mmu_map_fault_addr() skips
allocating bo->sgts and dereferences NULL when indexing bo->sgts.

[ ... ]

> @@ -720,6 +742,14 @@ 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_chunk(pages, page_offset,
> +				min_t(pgoff_t, page_offset + NUM_FAULT_PAGES, nr_pages));

[Severity: Medium]
Can panfrost_heap_put_chunk() leave dangling page pointers or drop an active
folio reference when shmem_read_folio() returns a folio larger than 2 MB?

With PAGE_SIZE > 4 KB (such as 16 KB or 64 KB pages on arm64) and
transparent hugepages enabled, shmem_read_folio() on the huge=within_size
mount can return a folio with folio_nr_pages(folio) > NUM_FAULT_PAGES.  The
fill loop in panfrost_mmu_map_fault_addr() aligns pg down to the folio
boundary and populates pages[] across the entire folio:

drivers/gpu/drm/panfrost/panfrost_mmu.c:panfrost_mmu_map_fault_addr() {
		...
		pg &= ~(folio_nr_pages(folio) - 1);
		for (u32 i = 0; i < folio_nr_pages(folio) && pg < nr_pages; i++) {
			if (pages[pg])
				already_owned = true;

			pages[pg++] = folio_page(folio, i);
		}
}

On error, panfrost_heap_put_chunk() only clears pages[] within
[page_offset, page_offset + NUM_FAULT_PAGES) before calling
folio_put(folio).  Any pages[] slots populated for that folio in adjacent
2 MB chunks remain non-NULL after the folio reference is dropped (or if an
adjacent chunk was already mapped and the duplicate reference was dropped via
already_owned, folio_put(folio) drops the mapped chunk's reference), leading
to a second folio_put() in drm_gem_put_pages() when the BO is freed and a
folio use-after-free.

[1] https://sashiko.dev/#/bug/linux-fe4f761b-d268-4774-87c0-3f0de658e362

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

[PATCH 4/7] drm/panfrost: Don't resume a soft-stopped job after a GPU reset

[Arun Iyengar]

panfrost_reset() soft-stops every slot before it resets the GPU. A job
that reaches a task boundary in time reports STOPPED,
panfrost_job_handle_err() moves its jc to JS_TAIL, and
drm_sched_resubmit_jobs() runs it again from there once the GPU has
been reset. But the progress of a stopped job lives in the GPU: the
hardware's update of the job header and the results of the tasks that
completed since the last cache write-back sit in the L2, and
panfrost_device_reset() soft-resets the GPU without cleaning it first.
The resumed job then completes with stale data in the buffers it still
had to write, and no error is reported.

Count the GPU resets and record the count when a job is stopped. A job
that was stopped before the last reset is failed with -ECANCELED in
panfrost_job_run() instead of being resumed, as the Arm driver fails
every atom that was on the hardware when the GPU was reset. A job that
is soft-stopped and resumed without a reset in between is unaffected.

Today the window is small, since panfrost_reset() hardly waits for the
soft-stops (see the next patch), but the next patches make every
soft-stopped job reach STOPPED.

Fixes: 030761e097b4 ("drm/panfrost: Queue jobs on the hardware")
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.h |  5 +++++
 drivers/gpu/drm/panfrost/panfrost_job.c    | 14 ++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_job.h    |  8 ++++++++
 3 files changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 63452febb..fd960a436 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -167,6 +167,11 @@ struct panfrost_device {
 		struct work_struct work;
 		struct rw_semaphore lock;
 		atomic_t pending;
+		/* Bumped by every GPU reset. A soft-stopped job may only be
+		 * resumed while this still reads what it did when the job was
+		 * stopped (see panfrost_job_run()).
+		 */
+		unsigned int count;
 	} reset;
 
 	struct mutex shrinker_lock;
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 3a46dd61c..e1f3e5d71 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -415,6 +415,17 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
 	if (!job->jc)
 		return NULL;
 
+	/* A soft-stopped job carries its progress in the job header and the
+	 * GPU caches, which a GPU reset throws away. Resumed from JS_TAIL after
+	 * a reset it would complete with the results of the tasks since the
+	 * last cache write-back missing, and no error. Fail it instead, as the
+	 * Arm driver fails every atom that was on the hardware at a reset.
+	 */
+	if (job->stopped && job->stop_reset_count != pfdev->reset.count) {
+		dma_fence_set_error(&job->base.s_fence->finished, -ECANCELED);
+		return NULL;
+	}
+
 	fence = panfrost_fence_create(pfdev, slot);
 	if (IS_ERR(fence))
 		return fence;
@@ -475,6 +486,8 @@ static void panfrost_job_handle_err(struct panfrost_device *pfdev,
 		/* Update the job head so we can resume */
 		job->jc = job_read(pfdev, JS_TAIL_LO(js)) |
 			  ((u64)job_read(pfdev, JS_TAIL_HI(js)) << 32);
+		job->stopped = true;
+		job->stop_reset_count = pfdev->reset.count;
 
 		/* The job will be resumed, don't signal the fence */
 		signal_fence = false;
@@ -728,6 +741,7 @@ panfrost_reset(struct panfrost_device *pfdev,
 			panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
 		}
 	}
+	pfdev->reset.count++;
 	memset(pfdev->jobs, 0, sizeof(pfdev->jobs));
 	spin_unlock(&pfdev->js->job_lock);
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
index 3e3b97178..9bbc0c262 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.h
+++ b/drivers/gpu/drm/panfrost/panfrost_job.h
@@ -27,6 +27,14 @@ struct panfrost_job {
 	__u32 requirements;
 	__u32 flush_id;
 
+	/* Set once the job has been soft-stopped: jc then points into a partly
+	 * executed chain whose progress lives in the job header and the GPU
+	 * caches, and is only valid while the GPU has not been reset since
+	 * (pfdev->reset.count at the time of the stop).
+	 */
+	bool stopped;
+	unsigned int stop_reset_count;
+
 	struct panfrost_gem_mapping **mappings;
 	struct drm_gem_object **bos;
 	u32 bo_count;

-- 
2.55.0

[PATCH 4/7] drm/panfrost: Don't resume a soft-stopped job after a GPU reset

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

panfrost_reset() soft-stops every slot before it resets the GPU. A job
that reaches a task boundary in time reports STOPPED,
panfrost_job_handle_err() moves its jc to JS_TAIL, and
drm_sched_resubmit_jobs() runs it again from there once the GPU has
been reset. But the progress of a stopped job lives in the GPU: the
hardware's update of the job header and the results of the tasks that
completed since the last cache write-back sit in the L2, and
panfrost_device_reset() soft-resets the GPU without cleaning it first.
The resumed job then completes with stale data in the buffers it still
had to write, and no error is reported.

Count the GPU resets and record the count when a job is stopped. A job
that was stopped before the last reset is failed with -ECANCELED in
panfrost_job_run() instead of being resumed, as the Arm driver fails
every atom that was on the hardware when the GPU was reset. A job that
is soft-stopped and resumed without a reset in between is unaffected.

Today the window is small, since panfrost_reset() hardly waits for the
soft-stops (see the next patch), but the next patches make every
soft-stopped job reach STOPPED.

Fixes: 030761e097b4 ("drm/panfrost: Queue jobs on the hardware")
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.h |  5 +++++
 drivers/gpu/drm/panfrost/panfrost_job.c    | 14 ++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_job.h    |  8 ++++++++
 3 files changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 63452febb..fd960a436 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -167,6 +167,11 @@ struct panfrost_device {
 		struct work_struct work;
 		struct rw_semaphore lock;
 		atomic_t pending;
+		/* Bumped by every GPU reset. A soft-stopped job may only be
+		 * resumed while this still reads what it did when the job was
+		 * stopped (see panfrost_job_run()).
+		 */
+		unsigned int count;
 	} reset;
 
 	struct mutex shrinker_lock;
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 3a46dd61c..e1f3e5d71 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -415,6 +415,17 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
 	if (!job->jc)
 		return NULL;
 
+	/* A soft-stopped job carries its progress in the job header and the
+	 * GPU caches, which a GPU reset throws away. Resumed from JS_TAIL after
+	 * a reset it would complete with the results of the tasks since the
+	 * last cache write-back missing, and no error. Fail it instead, as the
+	 * Arm driver fails every atom that was on the hardware at a reset.
+	 */
+	if (job->stopped && job->stop_reset_count != pfdev->reset.count) {
+		dma_fence_set_error(&job->base.s_fence->finished, -ECANCELED);
+		return NULL;
+	}
+
 	fence = panfrost_fence_create(pfdev, slot);
 	if (IS_ERR(fence))
 		return fence;
@@ -475,6 +486,8 @@ static void panfrost_job_handle_err(struct panfrost_device *pfdev,
 		/* Update the job head so we can resume */
 		job->jc = job_read(pfdev, JS_TAIL_LO(js)) |
 			  ((u64)job_read(pfdev, JS_TAIL_HI(js)) << 32);
+		job->stopped = true;
+		job->stop_reset_count = pfdev->reset.count;
 
 		/* The job will be resumed, don't signal the fence */
 		signal_fence = false;
@@ -728,6 +741,7 @@ panfrost_reset(struct panfrost_device *pfdev,
 			panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
 		}
 	}
+	pfdev->reset.count++;
 	memset(pfdev->jobs, 0, sizeof(pfdev->jobs));
 	spin_unlock(&pfdev->js->job_lock);
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
index 3e3b97178..9bbc0c262 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.h
+++ b/drivers/gpu/drm/panfrost/panfrost_job.h
@@ -27,6 +27,14 @@ struct panfrost_job {
 	__u32 requirements;
 	__u32 flush_id;
 
+	/* Set once the job has been soft-stopped: jc then points into a partly
+	 * executed chain whose progress lives in the job header and the GPU
+	 * caches, and is only valid while the GPU has not been reset since
+	 * (pfdev->reset.count at the time of the stop).
+	 */
+	bool stopped;
+	unsigned int stop_reset_count;
+
 	struct panfrost_gem_mapping **mappings;
 	struct drm_gem_object **bos;
 	u32 bo_count;

-- 
2.55.0

Re: [PATCH 4/7] drm/panfrost: Don't resume a soft-stopped job after a GPU reset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] drm/panfrost: missing PM runtime reference in
  panfrost_reset_work() [1]
--

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

drm/panfrost: Don't resume a soft-stopped job after a GPU reset

This commit tracks GPU resets and fails soft-stopped jobs with -ECANCELED in
panfrost_job_run() if a GPU reset occurred after the job was stopped.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 3a46dd61cf87f..e1f3e5d717976 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c

[ ... ]

> @@ -728,6 +741,7 @@ panfrost_reset(struct panfrost_device *pfdev,
>  			panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
>  		}
>  	}
> +	pfdev->reset.count++;
>  	memset(pfdev->jobs, 0, sizeof(pfdev->jobs));
>  	spin_unlock(&pfdev->js->job_lock);

[Severity: Critical]
This problem wasn't introduced by this patch, but can panfrost_reset() access
hardware registers while the GPU is runtime suspended [1]?

Between panfrost_device_schedule_reset() queueing reset.work and the
workqueue executing panfrost_reset_work(), the active context can complete
and drop its runtime PM reference, allowing the GPU to suspend and gate its
clocks and power domains.  panfrost_reset_work() then calls panfrost_reset()
without holding a runtime PM reference:

drivers/gpu/drm/panfrost/panfrost_job.c:panfrost_reset_work() {
	struct panfrost_device *pfdev;

	pfdev = container_of(work, struct panfrost_device, reset.work);
	panfrost_reset(pfdev, NULL);
}

When panfrost_reset() writes to MMIO registers such as JOB_INT_MASK on the
powered-down device, it triggers a synchronous external abort.

[1] https://sashiko.dev/#/bug/linux-57c30678-e800-4756-b926-145408a6717d

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

[PATCH 5/7] 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 e1f3e5d71..81efa8a86 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] +=
@@ -647,34 +651,31 @@ 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
 panfrost_reset(struct panfrost_device *pfdev,
 	       struct drm_sched_job *bad)
 {
-	u32 js_state, js_state_mask = 0xffffffff;
 	unsigned int i, j;
-	bool cookie;
+	bool cookie, idle;
 	int ret;
 
 	if (!atomic_read(&pfdev->reset.pending))
@@ -713,10 +714,13 @@ panfrost_reset(struct panfrost_device *pfdev,
 		job_write(pfdev, JS_COMMAND(i), JS_COMMAND_SOFT_STOP);
 	}
 
-	/* Wait at most 10ms for soft-stops to complete */
-	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 5/7] 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 e1f3e5d71..81efa8a86 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] +=
@@ -647,34 +651,31 @@ 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
 panfrost_reset(struct panfrost_device *pfdev,
 	       struct drm_sched_job *bad)
 {
-	u32 js_state, js_state_mask = 0xffffffff;
 	unsigned int i, j;
-	bool cookie;
+	bool cookie, idle;
 	int ret;
 
 	if (!atomic_read(&pfdev->reset.pending))
@@ -713,10 +714,13 @@ panfrost_reset(struct panfrost_device *pfdev,
 		job_write(pfdev, JS_COMMAND(i), JS_COMMAND_SOFT_STOP);
 	}
 
-	/* Wait at most 10ms for soft-stops to complete */
-	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 6/7] 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 GPU is reset when the hard stop has not taken effect after
  JOB_HARD_STOP_TIMEOUT_MS (0.5 s); the job still running is then failed
  with -ECANCELED instead of being restarted from scratch, where it would
  only time out again;
- 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 L2 off under a
  soft-stopped job.

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 | 168 ++++++++++++++++++++++++++------
 1 file changed, 138 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 81efa8a86..e37dd9265 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -27,6 +27,19 @@
 #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
+
 #define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
 #define job_read(dev, reg) readl(dev->iomem + (reg))
 
@@ -165,8 +178,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;
@@ -672,7 +686,7 @@ static bool panfrost_slots_idle(struct panfrost_device *pfdev)
 
 static void
 panfrost_reset(struct panfrost_device *pfdev,
-	       struct drm_sched_job *bad)
+	       struct drm_sched_job *bad, bool job_active)
 {
 	unsigned int i, j;
 	bool cookie, idle;
@@ -681,6 +695,12 @@ panfrost_reset(struct panfrost_device *pfdev,
 	if (!atomic_read(&pfdev->reset.pending))
 		return;
 
+	/* The jobs we stop drop their runtime PM references before we submit
+	 * them again. Keep the GPU powered in between: a suspend would power
+	 * the L2 off and lose the dirty lines of a soft-stopped job.
+	 */
+	pm_runtime_get_noresume(pfdev->base.dev);
+
 	/* Stop the schedulers.
 	 *
 	 * FIXME: We temporarily get out of the dma_fence_signalling section
@@ -699,7 +719,8 @@ panfrost_reset(struct panfrost_device *pfdev,
 
 	cookie = dma_fence_begin_signalling();
 
-	if (bad)
+	/* A job that is merely long is not guilty of anything. */
+	if (bad && !job_active)
 		drm_sched_increase_karma(bad);
 
 	/* Mask job interrupts and synchronize to make sure we won't be
@@ -714,43 +735,110 @@ panfrost_reset(struct panfrost_device *pfdev,
 		job_write(pfdev, JS_COMMAND(i), JS_COMMAND_SOFT_STOP);
 	}
 
-	/* Wait at most 10ms for soft-stops to complete. Every slot must have
-	 * gone idle before we touch the queues: a job that is still stopping
-	 * (or finishing) raises its interrupt later, when its entry in
-	 * pfdev->jobs[] is gone or belongs to its own resubmission.
+	/* Every slot must have gone idle before we touch the queues: a job
+	 * that is still stopping (or finishing) raises its interrupt later,
+	 * when its entry in pfdev->jobs[] is gone or belongs to its own
+	 * resubmission, and the second start of that job then runs on a slot
+	 * we have no job for.
+	 *
+	 * A slot that was idle or faulted gets 10 ms. A job that was still
+	 * active gets the time the Arm driver gives it to reach a task
+	 * boundary (JOB_SOFT_STOP_TIMEOUT_MS). If it has not stopped by then
+	 * it is hard-stopped, which terminates it at once and fails it
+	 * (TERMINATED, see panfrost_job_handle_err()), instead of resetting
+	 * the GPU under every job on it; the GPU is reset only when the hard
+	 * stop does not take effect either.
 	 */
-	ret = read_poll_timeout(panfrost_slots_idle, idle, idle, 10, 10000,
+	ret = read_poll_timeout(panfrost_slots_idle, idle, idle,
+				job_active ? 200 : 10,
+				job_active ? JOB_SOFT_STOP_TIMEOUT_MS * 1000 : 10000,
 				false, pfdev);
 
-	if (ret)
-		dev_err(pfdev->base.dev, "Soft-stop failed\n");
+	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)));
+
+		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);
+	}
+
+	if (ret) {
+		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)));
+		job_active = false;
+	}
 
 	/* Handle the remaining interrupts before we reset. */
 	panfrost_jm_handle_irqs(pfdev);
 
 	/* Remaining interrupts have been handled, but we might still have
-	 * stuck jobs. Let's make sure the PM counters stay balanced by
+	 * stuck jobs, and jobs that were waiting in _NEXT when it was
+	 * cancelled. Let's make sure the PM counters stay balanced by
 	 * manually calling pm_runtime_put_noidle() and
-	 * panfrost_devfreq_record_idle() for each stuck job.
+	 * panfrost_devfreq_record_idle() for each of them.
 	 * Let's also make sure the cycle counting register's refcnt is
-	 * kept balanced to prevent it from running forever
+	 * kept balanced to prevent it from running forever.
 	 */
 	spin_lock(&pfdev->js->job_lock);
 	for (i = 0; i < NUM_JOB_SLOTS; i++) {
 		for (j = 0; j < ARRAY_SIZE(pfdev->jobs[0]) && pfdev->jobs[i][j]; j++) {
-			if (pfdev->jobs[i][j]->requirements & PANFROST_JD_REQ_CYCLE_COUNT ||
-			    pfdev->jobs[i][j]->is_profiled)
-				panfrost_cycle_counter_put(pfdev->jobs[i][j]->pfdev);
+			struct panfrost_job *job = pfdev->jobs[i][j];
+
+			if (job->requirements & PANFROST_JD_REQ_CYCLE_COUNT ||
+			    job->is_profiled)
+				panfrost_cycle_counter_put(job->pfdev);
 			pm_runtime_put_noidle(pfdev->base.dev);
 			panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
+
+			/* The GPU reset wipes the address space assignments;
+			 * without one, release the reference the submission
+			 * took, or the address space can never be reassigned.
+			 */
+			if (job_active)
+				panfrost_mmu_as_put(pfdev, job->mmu);
+
+			/* The job still running on a slot that neither stop
+			 * took effect on is what forces the reset. Fail it,
+			 * as the Arm driver fails every atom on the hardware
+			 * at a reset: restarted from scratch it would hit the
+			 * timeout and the reset again.
+			 */
+			if (!job_active && j == 0 &&
+			    job_read(pfdev, JS_STATUS(i)) ==
+			    DRM_PANFROST_EXCEPTION_ACTIVE)
+				dma_fence_set_error(&job->base.s_fence->finished,
+						    -ECANCELED);
 		}
 	}
-	pfdev->reset.count++;
+	if (!job_active)
+		pfdev->reset.count++;
 	memset(pfdev->jobs, 0, sizeof(pfdev->jobs));
 	spin_unlock(&pfdev->js->job_lock);
 
-	/* Proceed with reset now. */
-	panfrost_device_reset(pfdev, false);
+	/* Proceed with reset now, unless the slots were simply busy with long
+	 * jobs that stopped at a task boundary (or a hard stop ended the one
+	 * that did not). The stopped jobs will be resumed from JS_TAIL (see
+	 * panfrost_job_handle_err()). The hardware keeps their resume state in
+	 * the job header and the results of the tasks that already finished
+	 * in the L2: a soft reset throws both away, and a job resumed after it
+	 * completes with stale data in the buffers it had still to write and
+	 * no error reported (panfrost_job_run() fails such a job instead).
+	 */
+	if (!job_active)
+		panfrost_device_reset(pfdev, false);
 
 	/* GPU has been reset, we can clear the reset pending bit. */
 	atomic_set(&pfdev->reset.pending, 0);
@@ -775,6 +863,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
@@ -783,6 +873,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
@@ -807,18 +899,34 @@ static enum drm_gpu_sched_stat panfrost_job_timedout(struct drm_sched_job
 		return DRM_GPU_SCHED_STAT_NO_HANG;
 	}
 
-	dev_err(pfdev->base.dev, "gpu sched timeout, js=%d, config=0x%x, status=0x%x, head=0x%x, tail=0x%x, sched_job=%p",
-		js,
-		job_read(pfdev, JS_CONFIG(js)),
-		job_read(pfdev, JS_STATUS(js)),
-		job_read(pfdev, JS_HEAD_LO(js)),
-		job_read(pfdev, JS_TAIL_LO(js)),
-		sched_job);
+	/*
+	 * A slot that is still ACTIVE is not hung, it is running a job that
+	 * takes longer than JOB_TIMEOUT_MS. Compute kernels legitimately do.
+	 * Soft-stop such a job and resume it, the way the Arm driver
+	 * time-slices jobs, instead of resetting the GPU under it; the reset
+	 * stays for faults and for jobs that do not stop.
+	 */
+	js_status = job_read(pfdev, JS_STATUS(js));
+	job_active = js_status == DRM_PANFROST_EXCEPTION_ACTIVE;
+
+	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);
+	panfrost_reset(pfdev, sched_job, job_active);
 
 	return DRM_GPU_SCHED_STAT_RESET;
 }
@@ -828,7 +936,7 @@ static void panfrost_reset_work(struct work_struct *work)
 	struct panfrost_device *pfdev;
 
 	pfdev = container_of(work, struct panfrost_device, reset.work);
-	panfrost_reset(pfdev, NULL);
+	panfrost_reset(pfdev, NULL, false);
 }
 
 static const struct drm_sched_backend_ops panfrost_sched_ops = {

-- 
2.55.0

[PATCH 6/7] 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 GPU is reset when the hard stop has not taken effect after
  JOB_HARD_STOP_TIMEOUT_MS (0.5 s); the job still running is then failed
  with -ECANCELED instead of being restarted from scratch, where it would
  only time out again;
- 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 L2 off under a
  soft-stopped job.

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 | 168 ++++++++++++++++++++++++++------
 1 file changed, 138 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 81efa8a86..e37dd9265 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -27,6 +27,19 @@
 #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
+
 #define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
 #define job_read(dev, reg) readl(dev->iomem + (reg))
 
@@ -165,8 +178,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;
@@ -672,7 +686,7 @@ static bool panfrost_slots_idle(struct panfrost_device *pfdev)
 
 static void
 panfrost_reset(struct panfrost_device *pfdev,
-	       struct drm_sched_job *bad)
+	       struct drm_sched_job *bad, bool job_active)
 {
 	unsigned int i, j;
 	bool cookie, idle;
@@ -681,6 +695,12 @@ panfrost_reset(struct panfrost_device *pfdev,
 	if (!atomic_read(&pfdev->reset.pending))
 		return;
 
+	/* The jobs we stop drop their runtime PM references before we submit
+	 * them again. Keep the GPU powered in between: a suspend would power
+	 * the L2 off and lose the dirty lines of a soft-stopped job.
+	 */
+	pm_runtime_get_noresume(pfdev->base.dev);
+
 	/* Stop the schedulers.
 	 *
 	 * FIXME: We temporarily get out of the dma_fence_signalling section
@@ -699,7 +719,8 @@ panfrost_reset(struct panfrost_device *pfdev,
 
 	cookie = dma_fence_begin_signalling();
 
-	if (bad)
+	/* A job that is merely long is not guilty of anything. */
+	if (bad && !job_active)
 		drm_sched_increase_karma(bad);
 
 	/* Mask job interrupts and synchronize to make sure we won't be
@@ -714,43 +735,110 @@ panfrost_reset(struct panfrost_device *pfdev,
 		job_write(pfdev, JS_COMMAND(i), JS_COMMAND_SOFT_STOP);
 	}
 
-	/* Wait at most 10ms for soft-stops to complete. Every slot must have
-	 * gone idle before we touch the queues: a job that is still stopping
-	 * (or finishing) raises its interrupt later, when its entry in
-	 * pfdev->jobs[] is gone or belongs to its own resubmission.
+	/* Every slot must have gone idle before we touch the queues: a job
+	 * that is still stopping (or finishing) raises its interrupt later,
+	 * when its entry in pfdev->jobs[] is gone or belongs to its own
+	 * resubmission, and the second start of that job then runs on a slot
+	 * we have no job for.
+	 *
+	 * A slot that was idle or faulted gets 10 ms. A job that was still
+	 * active gets the time the Arm driver gives it to reach a task
+	 * boundary (JOB_SOFT_STOP_TIMEOUT_MS). If it has not stopped by then
+	 * it is hard-stopped, which terminates it at once and fails it
+	 * (TERMINATED, see panfrost_job_handle_err()), instead of resetting
+	 * the GPU under every job on it; the GPU is reset only when the hard
+	 * stop does not take effect either.
 	 */
-	ret = read_poll_timeout(panfrost_slots_idle, idle, idle, 10, 10000,
+	ret = read_poll_timeout(panfrost_slots_idle, idle, idle,
+				job_active ? 200 : 10,
+				job_active ? JOB_SOFT_STOP_TIMEOUT_MS * 1000 : 10000,
 				false, pfdev);
 
-	if (ret)
-		dev_err(pfdev->base.dev, "Soft-stop failed\n");
+	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)));
+
+		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);
+	}
+
+	if (ret) {
+		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)));
+		job_active = false;
+	}
 
 	/* Handle the remaining interrupts before we reset. */
 	panfrost_jm_handle_irqs(pfdev);
 
 	/* Remaining interrupts have been handled, but we might still have
-	 * stuck jobs. Let's make sure the PM counters stay balanced by
+	 * stuck jobs, and jobs that were waiting in _NEXT when it was
+	 * cancelled. Let's make sure the PM counters stay balanced by
 	 * manually calling pm_runtime_put_noidle() and
-	 * panfrost_devfreq_record_idle() for each stuck job.
+	 * panfrost_devfreq_record_idle() for each of them.
 	 * Let's also make sure the cycle counting register's refcnt is
-	 * kept balanced to prevent it from running forever
+	 * kept balanced to prevent it from running forever.
 	 */
 	spin_lock(&pfdev->js->job_lock);
 	for (i = 0; i < NUM_JOB_SLOTS; i++) {
 		for (j = 0; j < ARRAY_SIZE(pfdev->jobs[0]) && pfdev->jobs[i][j]; j++) {
-			if (pfdev->jobs[i][j]->requirements & PANFROST_JD_REQ_CYCLE_COUNT ||
-			    pfdev->jobs[i][j]->is_profiled)
-				panfrost_cycle_counter_put(pfdev->jobs[i][j]->pfdev);
+			struct panfrost_job *job = pfdev->jobs[i][j];
+
+			if (job->requirements & PANFROST_JD_REQ_CYCLE_COUNT ||
+			    job->is_profiled)
+				panfrost_cycle_counter_put(job->pfdev);
 			pm_runtime_put_noidle(pfdev->base.dev);
 			panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
+
+			/* The GPU reset wipes the address space assignments;
+			 * without one, release the reference the submission
+			 * took, or the address space can never be reassigned.
+			 */
+			if (job_active)
+				panfrost_mmu_as_put(pfdev, job->mmu);
+
+			/* The job still running on a slot that neither stop
+			 * took effect on is what forces the reset. Fail it,
+			 * as the Arm driver fails every atom on the hardware
+			 * at a reset: restarted from scratch it would hit the
+			 * timeout and the reset again.
+			 */
+			if (!job_active && j == 0 &&
+			    job_read(pfdev, JS_STATUS(i)) ==
+			    DRM_PANFROST_EXCEPTION_ACTIVE)
+				dma_fence_set_error(&job->base.s_fence->finished,
+						    -ECANCELED);
 		}
 	}
-	pfdev->reset.count++;
+	if (!job_active)
+		pfdev->reset.count++;
 	memset(pfdev->jobs, 0, sizeof(pfdev->jobs));
 	spin_unlock(&pfdev->js->job_lock);
 
-	/* Proceed with reset now. */
-	panfrost_device_reset(pfdev, false);
+	/* Proceed with reset now, unless the slots were simply busy with long
+	 * jobs that stopped at a task boundary (or a hard stop ended the one
+	 * that did not). The stopped jobs will be resumed from JS_TAIL (see
+	 * panfrost_job_handle_err()). The hardware keeps their resume state in
+	 * the job header and the results of the tasks that already finished
+	 * in the L2: a soft reset throws both away, and a job resumed after it
+	 * completes with stale data in the buffers it had still to write and
+	 * no error reported (panfrost_job_run() fails such a job instead).
+	 */
+	if (!job_active)
+		panfrost_device_reset(pfdev, false);
 
 	/* GPU has been reset, we can clear the reset pending bit. */
 	atomic_set(&pfdev->reset.pending, 0);
@@ -775,6 +863,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
@@ -783,6 +873,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
@@ -807,18 +899,34 @@ static enum drm_gpu_sched_stat panfrost_job_timedout(struct drm_sched_job
 		return DRM_GPU_SCHED_STAT_NO_HANG;
 	}
 
-	dev_err(pfdev->base.dev, "gpu sched timeout, js=%d, config=0x%x, status=0x%x, head=0x%x, tail=0x%x, sched_job=%p",
-		js,
-		job_read(pfdev, JS_CONFIG(js)),
-		job_read(pfdev, JS_STATUS(js)),
-		job_read(pfdev, JS_HEAD_LO(js)),
-		job_read(pfdev, JS_TAIL_LO(js)),
-		sched_job);
+	/*
+	 * A slot that is still ACTIVE is not hung, it is running a job that
+	 * takes longer than JOB_TIMEOUT_MS. Compute kernels legitimately do.
+	 * Soft-stop such a job and resume it, the way the Arm driver
+	 * time-slices jobs, instead of resetting the GPU under it; the reset
+	 * stays for faults and for jobs that do not stop.
+	 */
+	js_status = job_read(pfdev, JS_STATUS(js));
+	job_active = js_status == DRM_PANFROST_EXCEPTION_ACTIVE;
+
+	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);
+	panfrost_reset(pfdev, sched_job, job_active);
 
 	return DRM_GPU_SCHED_STAT_RESET;
 }
@@ -828,7 +936,7 @@ static void panfrost_reset_work(struct work_struct *work)
 	struct panfrost_device *pfdev;
 
 	pfdev = container_of(work, struct panfrost_device, reset.work);
-	panfrost_reset(pfdev, NULL);
+	panfrost_reset(pfdev, NULL, false);
 }
 
 static const struct drm_sched_backend_ops panfrost_sched_ops = {

-- 
2.55.0

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

[sashiko-bot]

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

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

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

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

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

[ ... ]

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

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

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

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

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

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

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

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

[ ... ]

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

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

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

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

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

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

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

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

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

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

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

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

[ ... ]

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

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

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

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

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

[PATCH 7/7] 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 an access to a buffer that is no longer mapped (a kernel
bug) or from a 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, whether
it is still mapped, heap/noexec, and the label userspace gave it (Mesa
labels its BOs with their use, format and size). For an OpenCL image
read on a Mali-G31 this gave:

  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 | 81 +++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 13728b0c4..b0319d680 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -595,6 +595,86 @@ 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%s, label \"%s\"\n",
+		where, start, end, bo->base.base.size,
+		addr < start ? "-" : addr >= end ? "end+" : "start+", dist,
+		m->active ? "mapped" : "NOT mapped",
+		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), a fault inside a BO whose mapping is gone
+ * points at the kernel, an address far from any BO at a stale or garbage
+ * pointer.
+ */
+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. */
+	drm_mm_for_each_node(node, &mmu->mm) {
+		if (node->start > offset) {
+			above = drm_mm_node_to_panfrost_mapping(node);
+			break;
+		}
+		below = drm_mm_node_to_panfrost_mapping(node);
+	}
+	if (below) {
+		inside = offset < below->mmnode.start + below->mmnode.size;
+		kref_get(&below->refcount);
+		if (inside)
+			above = NULL;
+	}
+	if (above)
+		kref_get(&above->refcount);
+	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)
 
 static void panfrost_heap_put_chunk(struct page **pages, pgoff_t start,
@@ -965,6 +1045,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 7/7] 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 an access to a buffer that is no longer mapped (a kernel
bug) or from a 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, whether
it is still mapped, heap/noexec, and the label userspace gave it (Mesa
labels its BOs with their use, format and size). For an OpenCL image
read on a Mali-G31 this gave:

  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 | 81 +++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 13728b0c4..b0319d680 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -595,6 +595,86 @@ 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%s, label \"%s\"\n",
+		where, start, end, bo->base.base.size,
+		addr < start ? "-" : addr >= end ? "end+" : "start+", dist,
+		m->active ? "mapped" : "NOT mapped",
+		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), a fault inside a BO whose mapping is gone
+ * points at the kernel, an address far from any BO at a stale or garbage
+ * pointer.
+ */
+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. */
+	drm_mm_for_each_node(node, &mmu->mm) {
+		if (node->start > offset) {
+			above = drm_mm_node_to_panfrost_mapping(node);
+			break;
+		}
+		below = drm_mm_node_to_panfrost_mapping(node);
+	}
+	if (below) {
+		inside = offset < below->mmnode.start + below->mmnode.size;
+		kref_get(&below->refcount);
+		if (inside)
+			above = NULL;
+	}
+	if (above)
+		kref_get(&above->refcount);
+	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)
 
 static void panfrost_heap_put_chunk(struct page **pages, pgoff_t start,
@@ -965,6 +1045,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

Re: [PATCH 7/7] drm/panfrost: Name the BOs around an unhandled page fault

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/panfrost: UAF on zero-refcount mappings in
  panfrost_mmu_report_fault_bos()
- [Low] drm/panfrost: unmapped BOs are removed from mmu->mm before fault
  lookup
--

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

drm/panfrost: Name the BOs around an unhandled page fault

This commit logs the buffer object containing an unhandled GPU page fault
address, or the nearest buffer objects below and above it in the faulting
address space.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 13728b0c46efc..b0319d680b12c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -595,6 +595,86 @@ 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%s, label \"%s\"\n",
> +		where, start, end, bo->base.base.size,
> +		addr < start ? "-" : addr >= end ? "end+" : "start+", dist,
> +		m->active ? "mapped" : "NOT mapped",
> +		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), a fault inside a BO whose mapping is gone
> + * points at the kernel, an address far from any BO at a stale or garbage
> + * pointer.
> + */

[Severity: Low]
Can a BO whose mapping has been torn down ever be reported as "inside" and
"NOT mapped" here?

panfrost_mmu_unmap() is only called from panfrost_gem_teardown_mapping(),
which immediately acquires mapping->mmu->mm_lock and removes
mapping->mmnode from mmu->mm.  Once a mapping is torn down (whether on
handle close or shrinker purge), its node is no longer in mmu->mm, so a
fault at its old VA will be reported between the neighboring "below" and
"above" BOs rather than inside an unmapped BO.  Conversely, for nodes still
in mmu->mm (outside transient setup or teardown windows), m->active is only
false for a heap BO that has not faulted in its first chunk yet, while a
partially mapped heap BO has m->active set to true even when faulting in an
unmapped chunk.

> +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. */
> +	drm_mm_for_each_node(node, &mmu->mm) {
> +		if (node->start > offset) {
> +			above = drm_mm_node_to_panfrost_mapping(node);
> +			break;
> +		}
> +		below = drm_mm_node_to_panfrost_mapping(node);
> +	}
> +	if (below) {
> +		inside = offset < below->mmnode.start + below->mmnode.size;
> +		kref_get(&below->refcount);
> +		if (inside)
> +			above = NULL;
> +	}
> +	if (above)
> +		kref_get(&above->refcount);
> +	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);
> +	}
> +}

[Severity: High]
Can kref_get() in panfrost_mmu_report_fault_bos() race with
panfrost_gem_mapping_release() and cause a refcount addition-on-zero and
use-after-free on below or above?

When the last reference to a mapping is dropped in
panfrost_gem_mapping_put(), mapping->refcount reaches zero before
panfrost_gem_teardown_mapping() unmaps the page tables and acquires
mapping->mmu->mm_lock to remove mapping->mmnode from mmu->mm:

drivers/gpu/drm/panfrost/panfrost_gem.c:panfrost_gem_teardown_mapping() {
	if (mapping->active)
		panfrost_mmu_unmap(mapping);

	spin_lock(&mapping->mmu->mm_lock);
	if (drm_mm_node_allocated(&mapping->mmnode))
		drm_mm_remove_node(&mapping->mmnode);
	spin_unlock(&mapping->mmu->mm_lock);
}

If a GPU job accesses that BO while its GEM handle is being closed,
panfrost_mmu_unmap() triggers the unhandled GPU page fault while
mapping->mmnode is still in mmu->mm with refcount == 0:

CPU0                                  CPU1
panfrost_gem_close()
  panfrost_gem_mapping_put()
    kref_put(&mapping->refcount, ...)
      panfrost_gem_mapping_release()
        panfrost_gem_teardown_mapping()
          panfrost_mmu_unmap(mapping)
                                      panfrost_mmu_irq_handler_thread()
                                        panfrost_mmu_report_fault_bos()
                                          spin_lock(&mmu->mm_lock)
                                          kref_get(&below->refcount)
                                            <-- addition on zero
                                          spin_unlock(&mmu->mm_lock)
          spin_lock(&mmu->mm_lock)
          drm_mm_remove_node(...)
          spin_unlock(&mmu->mm_lock)
        drm_gem_object_put(...)
        kfree(mapping)
                                        panfrost_mmu_print_fault_bo()
                                          mutex_lock(&bo->label.lock)
                                            <-- use-after-free

Using kref_get_unless_zero() (or skipping nodes whose refcount is already
zero while walking mmu->mm) would avoid grabbing a dying mapping.

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