[PATCH v2 0/4] drm/panfrost: Fix jobs that outlive their JM context

[PATCH v2 0/4] drm/panfrost: Fix jobs that outlive their JM context

[Jiwoo Kim]

These fix how jobs are handled when their JM context is destroyed,
explicitly or because the file is closed, and the error path of JM
context creation.

Patch 1 makes a job hold a reference to its MMU context. Patch 2 makes
panfrost_jm_ctx_destroy() find the jobs of the context again: it looks
them up by job->base.entity, which the scheduler clears when it pops a
job. Patch 3 stops a job that is queued after its context was
destroyed. Patch 4 keeps panfrost_jm_ctx_create() from destroying
entities that it never initialized when it fails.

Changes in v2:
- New patch 2, and new patch 4 for a related issue, both reported by
  Sashiko on v1.
- Patches 1 and 3: changelogs updated, as panfrost_jm_ctx_destroy() did
  not actually stop any job before patch 2.

v1: https://lore.kernel.org/r/20261114062637.2447-1-arpe1618@gmail.com

Jiwoo Kim (4):
  drm/panfrost: Hold a reference to the MMU context of a job
  drm/panfrost: Find the jobs of a destroyed JM context by their context
  drm/panfrost: Stop a job queued after its JM context was destroyed
  drm/panfrost: Don't destroy uninitialized entities of a new JM context

 drivers/gpu/drm/panfrost/panfrost_drv.c |  2 +-
 drivers/gpu/drm/panfrost/panfrost_job.c | 91 +++++++++++++++++--------
 2 files changed, 62 insertions(+), 31 deletions(-)


base-commit: d48ea86919434695da12d280dccab72b094397e1
-- 
2.43.0

[PATCH v2 1/4] drm/panfrost: Hold a reference to the MMU context of a job

[Jiwoo Kim]

A job stores the MMU context of its file in job->mmu without taking a
reference to it. Once the file is closed, the context is only kept
alive by the BO mappings that still reference it, and a job may be
submitted with no BOs at all.

When the file is closed while such a job is still running,
panfrost_jm_close() does not wait for the job, and panfrost_postclose()
then drops the file's reference to the context, which frees it unless
BO mappings of other jobs still hold it. The job interrupt handling
that follows calls panfrost_mmu_as_put() on the freed job->mmu, and
panfrost_core_dump() also reads job->mmu if the job times out.

Take a reference to the MMU context when the job is created and drop
it when the job is cleaned up.

This was found with an AI coding assistant, which also drafted this
patch and its changelog. It has not been reproduced, and the change
has only been compile-tested.

Fixes: 6e516faf0431 ("drm/panfrost: Job should reference MMU not file_priv")
Cc: stable@vger.kernel.org
Signed-off-by: Jiwoo Kim <arpe1618@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
 drivers/gpu/drm/panfrost/panfrost_job.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 7c348a0bf1ee..afda794e2aeb 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -331,7 +331,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
 	job->jc = args->jc;
 	job->requirements = args->requirements;
 	job->flush_id = panfrost_gpu_get_latest_flush_id(pfdev);
-	job->mmu = file_priv->mmu;
+	job->mmu = panfrost_mmu_ctx_get(file_priv->mmu);
 	job->ctx = panfrost_jm_ctx_get(jm_ctx);
 	job->engine_usage = &file_priv->engine_usage;
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 4ddf4b722ddf..7022b1b2e020 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -377,6 +377,7 @@ static void panfrost_job_cleanup(struct kref *ref)
 		kvfree(job->bos);
 	}
 
+	panfrost_mmu_ctx_put(job->mmu);
 	panfrost_jm_ctx_put(job->ctx);
 	kfree(job);
 }
-- 
2.43.0

[PATCH v2 2/4] drm/panfrost: Find the jobs of a destroyed JM context by their context

[Jiwoo Kim]

panfrost_jm_ctx_destroy(), which runs on JM_CTX_DESTROY and when the
file is closed, looks for the jobs of the context in pfdev->jobs[] by
comparing job->base.entity with the entities of the context. Since
commit 96c7c2f4d5bd ("drm/scheduler: set entity to NULL in
drm_sched_entity_pop_job()"), the scheduler clears job->base.entity
when it pops a job, before it calls run_job(), so the entity of every
job in pfdev->jobs[] is NULL and the comparison never matches.

The jobs of a destroyed context are therefore neither stopped nor
detached from the file: they keep running after the file is closed,
and with profiling enabled, panfrost_dequeue_job() writes their
engine_usage to the panfrost_file_priv that panfrost_postclose() has
freed. The same comparison was used in panfrost_job_close() before
commit 6aa8bc58ac88 ("drm/panfrost: Introduce JM contexts for manging
job resources").

Match the jobs by job->ctx instead, which a job holds a reference to
until it is cleaned up.

The patch and its changelog were drafted with an AI coding assistant.
The issue has not been reproduced, and the change has only been
compile-tested.

Fixes: 96c7c2f4d5bd ("drm/scheduler: set entity to NULL in drm_sched_entity_pop_job()")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/sashiko-outbox-214229@kernel.org
Signed-off-by: Jiwoo Kim <arpe1618@gmail.com>
---
Trees from before commit 6aa8bc58ac88 ("drm/panfrost: Introduce JM
contexts for manging job resources") have the same comparison in
panfrost_job_close() and need a separate backport; there, the jobs of
the file can be matched by job->mmu instead.

 drivers/gpu/drm/panfrost/panfrost_job.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 7022b1b2e020..7a3137dacb9d 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -1105,13 +1105,15 @@ int panfrost_jm_ctx_destroy(struct drm_file *file, u32 handle)
 	/* Kill in-flight jobs */
 	spin_lock(&pfdev->js->job_lock);
 	for (u32 i = 0; i < ARRAY_SIZE(jm_ctx->slot_entity); i++) {
-		struct drm_sched_entity *entity = &jm_ctx->slot_entity[i];
-
 		for (int j = ARRAY_SIZE(pfdev->jobs[0]) - 1; j >= 0; j--) {
 			struct panfrost_job *job = pfdev->jobs[i][j];
 			u32 cmd;
 
-			if (!job || job->base.entity != entity)
+			/*
+			 * The scheduler clears job->base.entity when it pops the
+			 * job, so match the job by its context.
+			 */
+			if (!job || job->ctx != jm_ctx)
 				continue;
 
 			if (j == 1) {
-- 
2.43.0

[PATCH v2 3/4] drm/panfrost: Stop a job queued after its JM context was destroyed

[Jiwoo Kim]

panfrost_jm_ctx_destroy(), which runs on JM_CTX_DESTROY and when the
file is closed, sets jm_ctx->destroyed and then stops the jobs of the
context that it finds in pfdev->jobs[]. panfrost_job_run() checks the
flag once, without any lock, and panfrost_job_hw_submit() can then
sleep in pm_runtime_get_sync() before it queues the job in
pfdev->jobs[] under job_lock. A job that is between the check and the
queueing when its context is destroyed is therefore neither cancelled
nor stopped, and can then be started after its context, and possibly
its file, are gone. With profiling enabled, its engine_usage also still
points into the panfrost_file_priv that panfrost_postclose() frees, and
panfrost_dequeue_job() writes to it when the job completes.

Before commit 6aa8bc58ac88 ("drm/panfrost: Introduce JM contexts for
manging job resources"), the close path called
drm_sched_entity_destroy(), which waits for a run_job() call in
progress on the entity, before it looked for the jobs to stop. The
entities are now only destroyed when the last reference to the context
is dropped.

Move the per-job part of panfrost_jm_ctx_destroy() into a helper, and
call it from panfrost_job_hw_submit() on the job it has just queued if
the context has been destroyed by then. The check in
panfrost_job_hw_submit() and the loop in panfrost_jm_ctx_destroy() both
run under job_lock, and panfrost_jm_ctx_destroy() sets the flag before
it takes the lock, so with the previous patch, such a job is either
found by panfrost_jm_ctx_destroy() or sees the flag, and it then goes
through the same stop and completion path in both cases. Cancelling the
job in panfrost_job_hw_submit() instead would leave a gap in the fence
seqnos of the slot, which the job chain flags rely on. Use READ_ONCE()
and WRITE_ONCE() for the flag, as it is written outside job_lock and
read without it in panfrost_job_run().

This was found with an AI coding assistant, which also drafted this
patch and its changelog. It has not been reproduced, and the change
has only been compile-tested.

Fixes: 6aa8bc58ac88 ("drm/panfrost: Introduce JM contexts for manging job resources")
Cc: stable@vger.kernel.org
Signed-off-by: Jiwoo Kim <arpe1618@gmail.com>
---
panfrost_stop_job() is the loop body of panfrost_jm_ctx_destroy() moved
into a helper unchanged, apart from the lockdep assertion.

When the job is in subslot 0, the hard stop is written right after
JS_COMMAND_NEXT=START, as panfrost_jm_ctx_destroy() can already do for
a job it finds just after it was started. Whether the job manager can
miss a stop that early could not be checked on hardware.

Arun Iyengar's "Resume long jobs instead of resetting" series also
changes panfrost_job_hw_submit(); this patch does not depend on it:
https://lore.kernel.org/r/20261113-panfrost-long-jobs-v3-0-80ae78d38f6e@gmail.com

 drivers/gpu/drm/panfrost/panfrost_job.c | 68 ++++++++++++++++---------
 1 file changed, 44 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 7a3137dacb9d..0dbed7a50910 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -202,6 +202,37 @@ panfrost_enqueue_job(struct panfrost_device *pfdev, int slot,
 	return 1;
 }
 
+/* Stop a job of a destroyed JM context, found in pfdev->jobs[slot][subslot]. */
+static void panfrost_stop_job(struct panfrost_device *pfdev, int slot,
+			      unsigned int subslot, struct panfrost_job *job)
+{
+	u32 cmd;
+
+	lockdep_assert_held(&pfdev->js->job_lock);
+
+	if (subslot == 1) {
+		/* Try to cancel the job before it starts */
+		job_write(pfdev, JS_COMMAND_NEXT(slot), JS_COMMAND_NOP);
+		/* Reset the job head so it doesn't get restarted if
+		 * the job in the first slot failed.
+		 */
+		job->jc = 0;
+	}
+
+	if (panfrost_has_hw_feature(pfdev, HW_FEATURE_JOBCHAIN_DISAMBIGUATION)) {
+		cmd = panfrost_get_job_chain_flag(job) ?
+		      JS_COMMAND_HARD_STOP_1 :
+		      JS_COMMAND_HARD_STOP_0;
+	} else {
+		cmd = JS_COMMAND_HARD_STOP;
+	}
+
+	job_write(pfdev, JS_COMMAND(slot), cmd);
+
+	/* Jobs can outlive their file context */
+	job->engine_usage = NULL;
+}
+
 static int panfrost_job_hw_submit(struct panfrost_job *job, int js)
 {
 	struct panfrost_device *pfdev = job->pfdev;
@@ -272,6 +303,16 @@ static int panfrost_job_hw_submit(struct panfrost_job *job, int js)
 			"JS: Submitting atom %p to js[%d][%d] with head=0x%llx AS %d",
 			job, js, subslot, jc_head, cfg & 0xf);
 	}
+
+	/*
+	 * panfrost_jm_ctx_destroy() only stops the jobs it finds in
+	 * pfdev->jobs[], so stop this one here if its context was destroyed
+	 * while we were getting here, e.g. while waiting for the GPU to
+	 * resume. job_lock orders this against panfrost_jm_ctx_destroy().
+	 */
+	if (READ_ONCE(job->ctx->destroyed))
+		panfrost_stop_job(pfdev, js, subslot, job);
+
 	spin_unlock(&pfdev->js->job_lock);
 
 	return 0;
@@ -404,7 +445,7 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
 	struct dma_fence *fence = NULL;
 	int ret;
 
-	if (job->ctx->destroyed)
+	if (READ_ONCE(job->ctx->destroyed))
 		return ERR_PTR(-ECANCELED);
 
 	if (unlikely(job->base.s_fence->finished.error))
@@ -1100,14 +1141,13 @@ int panfrost_jm_ctx_destroy(struct drm_file *file, u32 handle)
 	if (!jm_ctx)
 		return -EINVAL;
 
-	jm_ctx->destroyed = true;
+	WRITE_ONCE(jm_ctx->destroyed, true);
 
 	/* Kill in-flight jobs */
 	spin_lock(&pfdev->js->job_lock);
 	for (u32 i = 0; i < ARRAY_SIZE(jm_ctx->slot_entity); i++) {
 		for (int j = ARRAY_SIZE(pfdev->jobs[0]) - 1; j >= 0; j--) {
 			struct panfrost_job *job = pfdev->jobs[i][j];
-			u32 cmd;
 
 			/*
 			 * The scheduler clears job->base.entity when it pops the
@@ -1116,27 +1156,7 @@ int panfrost_jm_ctx_destroy(struct drm_file *file, u32 handle)
 			if (!job || job->ctx != jm_ctx)
 				continue;
 
-			if (j == 1) {
-				/* Try to cancel the job before it starts */
-				job_write(pfdev, JS_COMMAND_NEXT(i), JS_COMMAND_NOP);
-				/* Reset the job head so it doesn't get restarted if
-				 * the job in the first slot failed.
-				 */
-				job->jc = 0;
-			}
-
-			if (panfrost_has_hw_feature(pfdev, HW_FEATURE_JOBCHAIN_DISAMBIGUATION)) {
-				cmd = panfrost_get_job_chain_flag(job) ?
-				      JS_COMMAND_HARD_STOP_1 :
-				      JS_COMMAND_HARD_STOP_0;
-			} else {
-				cmd = JS_COMMAND_HARD_STOP;
-			}
-
-			job_write(pfdev, JS_COMMAND(i), cmd);
-
-			/* Jobs can outlive their file context */
-			job->engine_usage = NULL;
+			panfrost_stop_job(pfdev, i, j, job);
 		}
 	}
 	spin_unlock(&pfdev->js->job_lock);
-- 
2.43.0

[PATCH v2 4/4] drm/panfrost: Don't destroy uninitialized entities of a new JM context

[Jiwoo Kim]

When panfrost_jm_ctx_create() fails, it drops its reference to the new
context, and panfrost_jm_ctx_release() then calls
drm_sched_entity_destroy() on all the entities of the context. If the
requested priority is invalid or not allowed, or drm_sched_entity_init()
fails, some or all of these entities have never been initialized,
though. Since commit fd177135f0e6 ("drm/sched: Account entity GPU
time"), drm_sched_entity_fini() then dereferences the NULL entity->stats
of such an entity, and since commit 2df5efb45425 ("drm/sched: Remove
redundant entity->rq initialization and checks"),
drm_sched_entity_flush() already dereferences its NULL entity->rq.
Userspace can hit this with JM_CTX_CREATE and an invalid priority.

On these error paths, destroy only the entities that were initialized
and free the context directly. The context is only put once it is fully
set up.

The patch and its changelog were drafted with an AI coding assistant.
The issue has not been reproduced, and the change has only been
compile-tested.

Fixes: 6aa8bc58ac88 ("drm/panfrost: Introduce JM contexts for manging job resources")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-1f36c24e-a878-442e-991c-1b387017bee6
Signed-off-by: Jiwoo Kim <arpe1618@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_job.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 0dbed7a50910..95fd9521016d 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -1097,6 +1097,7 @@ int panfrost_jm_ctx_create(struct drm_file *file,
 	struct panfrost_device *pfdev = priv->pfdev;
 	enum drm_sched_priority sched_prio;
 	struct panfrost_jm_ctx *jm_ctx;
+	u32 i;
 	int ret;
 
 	jm_ctx = kzalloc_obj(*jm_ctx);
@@ -1107,15 +1108,15 @@ int panfrost_jm_ctx_create(struct drm_file *file,
 
 	ret = jm_ctx_prio_to_drm_sched_prio(file, args->priority, &sched_prio);
 	if (ret)
-		goto err_put_jm_ctx;
+		goto err_free_jm_ctx;
 
-	for (u32 i = 0; i < NUM_JOB_SLOTS; i++) {
+	for (i = 0; i < NUM_JOB_SLOTS; i++) {
 		struct drm_gpu_scheduler *sched = &pfdev->js->queue[i].sched;
 
 		ret = drm_sched_entity_init(&jm_ctx->slot_entity[i], sched_prio,
 					    &sched, 1, NULL);
 		if (ret)
-			goto err_put_jm_ctx;
+			goto err_destroy_entities;
 	}
 
 	ret = xa_alloc(&priv->jm_ctxs, &args->handle, jm_ctx,
@@ -1129,6 +1130,13 @@ int panfrost_jm_ctx_create(struct drm_file *file,
 	jm_ctx->destroyed = true;
 	panfrost_jm_ctx_put(jm_ctx);
 	return ret;
+
+err_destroy_entities:
+	while (i--)
+		drm_sched_entity_destroy(&jm_ctx->slot_entity[i]);
+err_free_jm_ctx:
+	kfree(jm_ctx);
+	return ret;
 }
 
 int panfrost_jm_ctx_destroy(struct drm_file *file, u32 handle)
-- 
2.43.0