[PATCH v2 0/4] Introduce Panfrost JM contexts

[PATCH v2 0/4] Introduce Panfrost JM contexts

[Adrián Larumbe]

This patch series brings the notion of JM contexts into Panfrost.
UM will be able to create contexts, get a handle for them and attach
it to a job submission. Contexts describe basic HW resource assignment
to jobs, but at the moment that includes priorities only.

There's a MR for a Mesa commit series that makes use of these changes:
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/37075

Changelog:

v2:
 - Core and L2 cache affinities are not longer part of the context uAPI
 - Individual JS priorites are no longer possible, and the same context
 priority is applied onto all the JS and scheduler entities.
 - Minor changes in the debugfs knob to reflect all the above.

Boris Brezillon (4):
  drm/panfrost: Introduce uAPI for JM context creation
  drm/panfrost: Introduce JM contexts for manging job resources
  drm/panfrost: Expose JM context IOCTLs to UM
  drm/panfrost: Display list of device JM contexts over debugfs

 drivers/gpu/drm/panfrost/panfrost_device.h |   4 +-
 drivers/gpu/drm/panfrost/panfrost_drv.c    | 147 ++++++++++++++-
 drivers/gpu/drm/panfrost/panfrost_job.c    | 203 +++++++++++++++++----
 drivers/gpu/drm/panfrost/panfrost_job.h    |  25 ++-
 include/uapi/drm/panfrost_drm.h            |  50 +++++
 5 files changed, 388 insertions(+), 41 deletions(-)


base-commit: a3ae3384be7704fcf41279f13190bd8a11204bea
--
2.50.0

[PATCH v2 1/4] drm/panfrost: Introduce uAPI for JM context creation

[Adrián Larumbe]

From: Boris Brezillon <boris.brezillon@collabora.com>

The new uAPI lets user space query the KM driver for the available
priorities a job can be given at submit time. These are managed through
the notion of a context, for which we also provide new creation and
destruction ioctls.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 include/uapi/drm/panfrost_drm.h | 50 +++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
index ed67510395bd..e8b47c9f6976 100644
--- a/include/uapi/drm/panfrost_drm.h
+++ b/include/uapi/drm/panfrost_drm.h
@@ -22,6 +22,8 @@ extern "C" {
 #define DRM_PANFROST_PERFCNT_DUMP		0x07
 #define DRM_PANFROST_MADVISE			0x08
 #define DRM_PANFROST_SET_LABEL_BO		0x09
+#define DRM_PANFROST_JM_CTX_CREATE		0x0a
+#define DRM_PANFROST_JM_CTX_DESTROY		0x0b
 
 #define DRM_IOCTL_PANFROST_SUBMIT		DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_SUBMIT, struct drm_panfrost_submit)
 #define DRM_IOCTL_PANFROST_WAIT_BO		DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_WAIT_BO, struct drm_panfrost_wait_bo)
@@ -31,6 +33,8 @@ extern "C" {
 #define DRM_IOCTL_PANFROST_GET_BO_OFFSET	DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_GET_BO_OFFSET, struct drm_panfrost_get_bo_offset)
 #define DRM_IOCTL_PANFROST_MADVISE		DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_MADVISE, struct drm_panfrost_madvise)
 #define DRM_IOCTL_PANFROST_SET_LABEL_BO		DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_SET_LABEL_BO, struct drm_panfrost_set_label_bo)
+#define DRM_IOCTL_PANFROST_JM_CTX_CREATE	DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_JM_CTX_CREATE, struct drm_panfrost_jm_ctx_create)
+#define DRM_IOCTL_PANFROST_JM_CTX_DESTROY	DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_JM_CTX_DESTROY, struct drm_panfrost_jm_ctx_destroy)
 
 /*
  * Unstable ioctl(s): only exposed when the unsafe unstable_ioctls module
@@ -71,6 +75,12 @@ struct drm_panfrost_submit {
 
 	/** A combination of PANFROST_JD_REQ_* */
 	__u32 requirements;
+
+	/** JM context handle. Zero if you want to use the default context. */
+	__u32 jm_ctx_handle;
+
+	/** Padding field. MBZ. */
+	__u32 pad;
 };
 
 /**
@@ -177,6 +187,7 @@ enum drm_panfrost_param {
 	DRM_PANFROST_PARAM_AFBC_FEATURES,
 	DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP,
 	DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP_FREQUENCY,
+	DRM_PANFROST_PARAM_ALLOWED_JM_CTX_PRIORITIES,
 };
 
 struct drm_panfrost_get_param {
@@ -299,6 +310,45 @@ struct panfrost_dump_registers {
 	__u32 value;
 };
 
+enum drm_panfrost_jm_ctx_priority {
+	/**
+	 * @PANFROST_JM_CTX_PRIORITY_LOW: Low priority context.
+	 */
+	PANFROST_JM_CTX_PRIORITY_LOW = 0,
+
+	/**
+	 * @PANFROST_JM_CTX_PRIORITY_MEDIUM: Medium priority context.
+	 */
+	PANFROST_JM_CTX_PRIORITY_MEDIUM,
+
+	/**
+	 * @PANFROST_JM_CTX_PRIORITY_HIGH: High priority context.
+	 *
+	 * Requires CAP_SYS_NICE or DRM_MASTER.
+	 */
+	PANFROST_JM_CTX_PRIORITY_HIGH,
+};
+
+struct drm_panfrost_jm_ctx_create {
+	/** @handle: Handle of the created JM context */
+	__u32 handle;
+
+	/** @priority: Context priority (see enum drm_panfrost_jm_ctx_priority). */
+	__u32 priority;
+};
+
+struct drm_panfrost_jm_ctx_destroy {
+	/**
+	 * @handle: Handle of the JM context to destroy.
+	 *
+	 * Must be a valid context handle returned by DRM_IOCTL_PANTHOR_JM_CTX_CREATE.
+	 */
+	__u32 handle;
+
+	/** @pad: Padding field, MBZ. */
+	__u32 pad;
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.50.0

[PATCH v2 2/4] drm/panfrost: Introduce JM contexts for manging job resources

[Adrián Larumbe]

From: Boris Brezillon <boris.brezillon@collabora.com>

A JM context describes GPU HW resourdce allocation for the jobs bound to
it. At the time of writing this, it only holds the JS and queue
scheduler priorities.

When a context is created, all the scheduler entities created for job
slots will have the same priority.

Until context creation and destruction and attaching a context ID to
a job submission are exposed to UM, all jobs shall be bound to the
default Panfrost file context, which has medium priority.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.h |   4 +-
 drivers/gpu/drm/panfrost/panfrost_drv.c    |  19 +-
 drivers/gpu/drm/panfrost/panfrost_job.c    | 203 +++++++++++++++++----
 drivers/gpu/drm/panfrost/panfrost_job.h    |  25 ++-
 4 files changed, 210 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 077525a3ad68..5b164871eb95 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -15,6 +15,7 @@
 #include <drm/gpu_scheduler.h>
 
 #include "panfrost_devfreq.h"
+#include "panfrost_job.h"
 
 struct panfrost_device;
 struct panfrost_mmu;
@@ -22,7 +23,6 @@ struct panfrost_job_slot;
 struct panfrost_job;
 struct panfrost_perfcnt;
 
-#define NUM_JOB_SLOTS 3
 #define MAX_PM_DOMAINS 5
 
 enum panfrost_drv_comp_bits {
@@ -206,7 +206,7 @@ struct panfrost_engine_usage {
 struct panfrost_file_priv {
 	struct panfrost_device *pfdev;
 
-	struct drm_sched_entity sched_entity[NUM_JOB_SLOTS];
+	struct xarray jm_ctxs;
 
 	struct panfrost_mmu *mmu;
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 1ea6c509a5d5..398c067457d9 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -279,6 +279,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
 	struct panfrost_file_priv *file_priv = file->driver_priv;
 	struct drm_panfrost_submit *args = data;
 	struct drm_syncobj *sync_out = NULL;
+	struct panfrost_jm_ctx *jm_ctx;
 	struct panfrost_job *job;
 	int ret = 0, slot;
 
@@ -294,10 +295,17 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
 			return -ENODEV;
 	}
 
+	/* TODO: Use the default JM context until ctx management IOCTLs are exposed */
+	jm_ctx = panfrost_jm_ctx_from_handle(file, 0);
+	if (!jm_ctx) {
+		ret = -EINVAL;
+		goto out_put_syncout;
+	}
+
 	job = kzalloc(sizeof(*job), GFP_KERNEL);
 	if (!job) {
 		ret = -ENOMEM;
-		goto out_put_syncout;
+		goto out_put_jm_ctx;
 	}
 
 	kref_init(&job->refcount);
@@ -307,12 +315,13 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
 	job->requirements = args->requirements;
 	job->flush_id = panfrost_gpu_get_latest_flush_id(pfdev);
 	job->mmu = file_priv->mmu;
+	job->ctx = panfrost_jm_ctx_get(jm_ctx);
 	job->engine_usage = &file_priv->engine_usage;
 
 	slot = panfrost_job_get_slot(job);
 
 	ret = drm_sched_job_init(&job->base,
-				 &file_priv->sched_entity[slot],
+				 &jm_ctx->slots[slot].sched_entity,
 				 1, NULL, file->client_id);
 	if (ret)
 		goto out_put_job;
@@ -338,6 +347,8 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
 		drm_sched_job_cleanup(&job->base);
 out_put_job:
 	panfrost_job_put(job);
+out_put_jm_ctx:
+	panfrost_jm_ctx_put(jm_ctx);
 out_put_syncout:
 	if (sync_out)
 		drm_syncobj_put(sync_out);
@@ -564,7 +575,7 @@ panfrost_open(struct drm_device *dev, struct drm_file *file)
 		goto err_free;
 	}
 
-	ret = panfrost_job_open(panfrost_priv);
+	ret = panfrost_job_open(file);
 	if (ret)
 		goto err_job;
 
@@ -583,7 +594,7 @@ panfrost_postclose(struct drm_device *dev, struct drm_file *file)
 	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
 
 	panfrost_perfcnt_close(file);
-	panfrost_job_close(panfrost_priv);
+	panfrost_job_close(file);
 
 	panfrost_mmu_ctx_put(panfrost_priv->mmu);
 	kfree(panfrost_priv);
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 82acabb21b27..b6853add307c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -8,6 +8,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/dma-resv.h>
+#include <drm/drm_auth.h>
 #include <drm/gpu_scheduler.h>
 #include <drm/panfrost_drm.h>
 
@@ -22,6 +23,7 @@
 #include "panfrost_mmu.h"
 #include "panfrost_dump.h"
 
+#define MAX_JM_CTX_PER_FILE 128
 #define JOB_TIMEOUT_MS 500
 
 #define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
@@ -222,7 +224,7 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 
 	/* start MMU, medium priority, cache clean/flush on end, clean/flush on
 	 * start */
-	cfg |= JS_CONFIG_THREAD_PRI(8) |
+	cfg |= job->ctx->config |
 		JS_CONFIG_START_FLUSH_CLEAN_INVALIDATE |
 		JS_CONFIG_END_FLUSH_CLEAN_INVALIDATE |
 		panfrost_get_job_chain_flag(job);
@@ -359,6 +361,7 @@ static void panfrost_job_cleanup(struct kref *ref)
 		kvfree(job->bos);
 	}
 
+	panfrost_jm_ctx_put(job->ctx);
 	kfree(job);
 }
 
@@ -917,39 +920,184 @@ void panfrost_job_fini(struct panfrost_device *pfdev)
 	destroy_workqueue(pfdev->reset.wq);
 }
 
-int panfrost_job_open(struct panfrost_file_priv *panfrost_priv)
+int panfrost_job_open(struct drm_file *file)
+{
+	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
+	int ret;
+
+	struct drm_panfrost_jm_ctx_create default_jm_ctx = {
+		.priority = PANFROST_JM_CTX_PRIORITY_MEDIUM,
+	};
+
+	xa_init_flags(&panfrost_priv->jm_ctxs, XA_FLAGS_ALLOC);
+
+	ret = panfrost_jm_ctx_create(file, &default_jm_ctx);
+	if (ret)
+		return ret;
+
+	/* We expect the default context to be assigned handle 0. */
+	if (WARN_ON(default_jm_ctx.handle))
+		return -EINVAL;
+
+	return 0;
+}
+
+void panfrost_job_close(struct drm_file *file)
+{
+	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
+	struct panfrost_jm_ctx *jm_ctx;
+	unsigned long i;
+
+	xa_for_each(&panfrost_priv->jm_ctxs, i, jm_ctx)
+		panfrost_jm_ctx_destroy(file, i);
+
+	xa_destroy(&panfrost_priv->jm_ctxs);
+}
+
+int panfrost_job_is_idle(struct panfrost_device *pfdev)
 {
-	struct panfrost_device *pfdev = panfrost_priv->pfdev;
 	struct panfrost_job_slot *js = pfdev->js;
-	struct drm_gpu_scheduler *sched;
-	int ret, i;
+	int i;
 
 	for (i = 0; i < NUM_JOB_SLOTS; i++) {
-		sched = &js->queue[i].sched;
-		ret = drm_sched_entity_init(&panfrost_priv->sched_entity[i],
-					    DRM_SCHED_PRIORITY_NORMAL, &sched,
-					    1, NULL);
-		if (WARN_ON(ret))
-			return ret;
+		/* If there are any jobs in the HW queue, we're not idle */
+		if (atomic_read(&js->queue[i].sched.credit_count))
+			return false;
 	}
+
+	return true;
+}
+
+static void panfrost_jm_ctx_release(struct kref *kref)
+{
+	struct panfrost_jm_ctx *jm_ctx = container_of(kref, struct panfrost_jm_ctx, refcnt);
+
+	kfree(jm_ctx);
+}
+
+void
+panfrost_jm_ctx_put(struct panfrost_jm_ctx *jm_ctx)
+{
+	if (jm_ctx)
+		kref_put(&jm_ctx->refcnt, panfrost_jm_ctx_release);
+}
+
+struct panfrost_jm_ctx *
+panfrost_jm_ctx_get(struct panfrost_jm_ctx *jm_ctx)
+{
+	if (jm_ctx)
+		kref_get(&jm_ctx->refcnt);
+
+	return jm_ctx;
+}
+
+struct panfrost_jm_ctx *
+panfrost_jm_ctx_from_handle(struct drm_file *file, u32 handle)
+{
+	struct panfrost_file_priv *priv = file->driver_priv;
+	struct panfrost_jm_ctx *jm_ctx;
+
+	xa_lock(&priv->jm_ctxs);
+	jm_ctx = panfrost_jm_ctx_get(xa_load(&priv->jm_ctxs, handle));
+	xa_unlock(&priv->jm_ctxs);
+
+	return jm_ctx;
+}
+
+static int jm_ctx_prio_to_drm_sched_prio(struct drm_file *file,
+					 enum drm_panfrost_jm_ctx_priority in,
+					 enum drm_sched_priority *out)
+{
+	switch (in) {
+	case PANFROST_JM_CTX_PRIORITY_LOW:
+		*out = DRM_SCHED_PRIORITY_LOW;
+		return 0;
+	case PANFROST_JM_CTX_PRIORITY_MEDIUM:
+		*out = DRM_SCHED_PRIORITY_NORMAL;
+		return 0;
+	case PANFROST_JM_CTX_PRIORITY_HIGH:
+		/* Higher priorities require CAP_SYS_NICE or DRM_MASTER */
+		if (!capable(CAP_SYS_NICE) && !drm_is_current_master(file))
+			return -EACCES;
+
+		*out = DRM_SCHED_PRIORITY_HIGH;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+int panfrost_jm_ctx_create(struct drm_file *file,
+			   struct drm_panfrost_jm_ctx_create *args)
+{
+	struct panfrost_file_priv *priv = file->driver_priv;
+	struct panfrost_device *pfdev = priv->pfdev;
+	enum drm_sched_priority sched_prio;
+	struct panfrost_jm_ctx *jm_ctx;
+
+	int ret;
+
+	jm_ctx = kzalloc(sizeof(*jm_ctx), GFP_KERNEL);
+	if (!jm_ctx)
+		return -ENOMEM;
+
+	kref_init(&jm_ctx->refcnt);
+
+	/* Same priority for all JS within a single context */
+	jm_ctx->config = JS_CONFIG_THREAD_PRI(args->priority);
+
+	ret = jm_ctx_prio_to_drm_sched_prio(file, args->priority, &sched_prio);
+	if (ret)
+		goto err_put_jm_ctx;
+
+	for (u32 i = 0; i < NUM_JOB_SLOTS - 1; i++) {
+		struct drm_gpu_scheduler *sched = &pfdev->js->queue[i].sched;
+		struct panfrost_js_ctx *js_ctx = &jm_ctx->slots[i];
+
+		ret = drm_sched_entity_init(&js_ctx->sched_entity, sched_prio,
+					    &sched, 1, NULL);
+		if (ret)
+			goto err_put_jm_ctx;
+
+		js_ctx->enabled = true;
+	}
+
+	ret = xa_alloc(&priv->jm_ctxs, &args->handle, jm_ctx,
+		       XA_LIMIT(0, MAX_JM_CTX_PER_FILE), GFP_KERNEL);
+	if (ret)
+		goto err_put_jm_ctx;
+
 	return 0;
+
+err_put_jm_ctx:
+	panfrost_jm_ctx_put(jm_ctx);
+	return ret;
 }
 
-void panfrost_job_close(struct panfrost_file_priv *panfrost_priv)
+int panfrost_jm_ctx_destroy(struct drm_file *file, u32 handle)
 {
-	struct panfrost_device *pfdev = panfrost_priv->pfdev;
-	int i;
+	struct panfrost_file_priv *priv = file->driver_priv;
+	struct panfrost_device *pfdev = priv->pfdev;
+	struct panfrost_jm_ctx *jm_ctx;
 
-	for (i = 0; i < NUM_JOB_SLOTS; i++)
-		drm_sched_entity_destroy(&panfrost_priv->sched_entity[i]);
+	jm_ctx = xa_erase(&priv->jm_ctxs, handle);
+	if (!jm_ctx)
+		return -EINVAL;
+
+	for (u32 i = 0; i < ARRAY_SIZE(jm_ctx->slots); i++) {
+		if (jm_ctx->slots[i].enabled)
+			drm_sched_entity_destroy(&jm_ctx->slots[i].sched_entity);
+	}
 
 	/* Kill in-flight jobs */
 	spin_lock(&pfdev->js->job_lock);
-	for (i = 0; i < NUM_JOB_SLOTS; i++) {
-		struct drm_sched_entity *entity = &panfrost_priv->sched_entity[i];
-		int j;
+	for (u32 i = 0; i < ARRAY_SIZE(jm_ctx->slots); i++) {
+		struct drm_sched_entity *entity = &jm_ctx->slots[i].sched_entity;
+
+		if (!jm_ctx->slots[i].enabled)
+			continue;
 
-		for (j = ARRAY_SIZE(pfdev->jobs[0]) - 1; j >= 0; j--) {
+		for (int j = ARRAY_SIZE(pfdev->jobs[0]) - 1; j >= 0; j--) {
 			struct panfrost_job *job = pfdev->jobs[i][j];
 			u32 cmd;
 
@@ -980,18 +1128,7 @@ void panfrost_job_close(struct panfrost_file_priv *panfrost_priv)
 		}
 	}
 	spin_unlock(&pfdev->js->job_lock);
-}
 
-int panfrost_job_is_idle(struct panfrost_device *pfdev)
-{
-	struct panfrost_job_slot *js = pfdev->js;
-	int i;
-
-	for (i = 0; i < NUM_JOB_SLOTS; i++) {
-		/* If there are any jobs in the HW queue, we're not idle */
-		if (atomic_read(&js->queue[i].sched.credit_count))
-			return false;
-	}
-
-	return true;
+	panfrost_jm_ctx_put(jm_ctx);
+	return 0;
 }
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
index ec581b97852b..f3e5010e6cf5 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.h
+++ b/drivers/gpu/drm/panfrost/panfrost_job.h
@@ -18,6 +18,7 @@ struct panfrost_job {
 
 	struct panfrost_device *pfdev;
 	struct panfrost_mmu *mmu;
+	struct panfrost_jm_ctx *ctx;
 
 	/* Fence to be signaled by IRQ handler when the job is complete. */
 	struct dma_fence *done_fence;
@@ -39,10 +40,30 @@ struct panfrost_job {
 	u64 start_cycles;
 };
 
+struct panfrost_js_ctx {
+	struct drm_sched_entity sched_entity;
+	bool enabled;
+};
+
+#define NUM_JOB_SLOTS 3
+
+struct panfrost_jm_ctx {
+	struct kref refcnt;
+	u32 config;
+	struct panfrost_js_ctx slots[NUM_JOB_SLOTS];
+};
+
+int panfrost_jm_ctx_create(struct drm_file *file,
+			   struct drm_panfrost_jm_ctx_create *args);
+int panfrost_jm_ctx_destroy(struct drm_file *file, u32 handle);
+void panfrost_jm_ctx_put(struct panfrost_jm_ctx *jm_ctx);
+struct panfrost_jm_ctx *panfrost_jm_ctx_get(struct panfrost_jm_ctx *jm_ctx);
+struct panfrost_jm_ctx *panfrost_jm_ctx_from_handle(struct drm_file *file, u32 handle);
+
 int panfrost_job_init(struct panfrost_device *pfdev);
 void panfrost_job_fini(struct panfrost_device *pfdev);
-int panfrost_job_open(struct panfrost_file_priv *panfrost_priv);
-void panfrost_job_close(struct panfrost_file_priv *panfrost_priv);
+int panfrost_job_open(struct drm_file *file);
+void panfrost_job_close(struct drm_file *file);
 int panfrost_job_get_slot(struct panfrost_job *job);
 int panfrost_job_push(struct panfrost_job *job);
 void panfrost_job_put(struct panfrost_job *job);
-- 
2.50.0

[PATCH v2 3/4] drm/panfrost: Expose JM context IOCTLs to UM

[Adrián Larumbe]

From: Boris Brezillon <boris.brezillon@collabora.com>

Minor revision of the driver must be bumped because this expands the
uAPI. On top of that, let user know the available priorities so that
they can create contexts with legal priority values.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 35 +++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 398c067457d9..02f704ec4961 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -13,6 +13,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <drm/panfrost_drm.h>
+#include <drm/drm_auth.h>
 #include <drm/drm_debugfs.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_ioctl.h>
@@ -109,6 +110,15 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
 #endif
 		break;
 
+	case DRM_PANFROST_PARAM_ALLOWED_JM_CTX_PRIORITIES:
+		param->value = BIT(PANFROST_JM_CTX_PRIORITY_LOW) |
+			       BIT(PANFROST_JM_CTX_PRIORITY_MEDIUM);
+
+		/* High prio require CAP_SYS_NICE or DRM_MASTER */
+		if (capable(CAP_SYS_NICE) || drm_is_current_master(file))
+			param->value |= BIT(PANFROST_JM_CTX_PRIORITY_HIGH);
+		break;
+
 	default:
 		return -EINVAL;
 	}
@@ -295,8 +305,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
 			return -ENODEV;
 	}
 
-	/* TODO: Use the default JM context until ctx management IOCTLs are exposed */
-	jm_ctx = panfrost_jm_ctx_from_handle(file, 0);
+	jm_ctx = panfrost_jm_ctx_from_handle(file, args->jm_ctx_handle);
 	if (!jm_ctx) {
 		ret = -EINVAL;
 		goto out_put_syncout;
@@ -547,6 +556,24 @@ static int panfrost_ioctl_set_label_bo(struct drm_device *ddev, void *data,
 	return ret;
 }
 
+static int panfrost_ioctl_jm_ctx_create(struct drm_device *dev, void *data,
+					struct drm_file *file)
+{
+	return panfrost_jm_ctx_create(file, data);
+}
+
+static int panfrost_ioctl_jm_ctx_destroy(struct drm_device *dev, void *data,
+					 struct drm_file *file)
+{
+	const struct drm_panfrost_jm_ctx_destroy *args = data;
+
+	/* We can't destroy the default context created when the file is opened. */
+	if (!args->handle)
+		return -EINVAL;
+
+	return panfrost_jm_ctx_destroy(file, args->handle);
+}
+
 int panfrost_unstable_ioctl_check(void)
 {
 	if (!unstable_ioctls)
@@ -614,6 +641,8 @@ static const struct drm_ioctl_desc panfrost_drm_driver_ioctls[] = {
 	PANFROST_IOCTL(PERFCNT_DUMP,	perfcnt_dump,	DRM_RENDER_ALLOW),
 	PANFROST_IOCTL(MADVISE,		madvise,	DRM_RENDER_ALLOW),
 	PANFROST_IOCTL(SET_LABEL_BO,	set_label_bo,	DRM_RENDER_ALLOW),
+	PANFROST_IOCTL(JM_CTX_CREATE,	jm_ctx_create,	DRM_RENDER_ALLOW),
+	PANFROST_IOCTL(JM_CTX_DESTROY,	jm_ctx_destroy,	DRM_RENDER_ALLOW),
 };
 
 static void panfrost_gpu_show_fdinfo(struct panfrost_device *pfdev,
@@ -710,6 +739,8 @@ static void panfrost_debugfs_init(struct drm_minor *minor)
  * - 1.3 - adds JD_REQ_CYCLE_COUNT job requirement for SUBMIT
  *       - adds SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY queries
  * - 1.4 - adds SET_LABEL_BO
+ * - 1.5 - adds JM_CTX_{CREATE,DESTROY} ioctls and extend SUBMIT to allow
+ *	   context creation with configurable priorities/affinity
  */
 static const struct drm_driver panfrost_drm_driver = {
 	.driver_features	= DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ,
-- 
2.50.0

[PATCH v2 4/4] drm/panfrost: Display list of device JM contexts over debugfs

[Adrián Larumbe]

From: Boris Brezillon <boris.brezillon@collabora.com>

For DebugFS builds, create a filesystem knob that, for every single open
file of the Panfrost DRM device, shows its command name information and
PID (when applicable), and all of its existing JM contexts.

For every context, show its priority and job config.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 97 +++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 02f704ec4961..b3d14b887da4 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -712,6 +712,48 @@ static int panthor_gems_show(struct seq_file *m, void *data)
 	return 0;
 }
 
+static void show_panfrost_jm_ctx(struct panfrost_jm_ctx *jm_ctx, u32 handle,
+				 struct seq_file *m)
+{
+	struct drm_device *ddev = ((struct drm_info_node *)m->private)->minor->dev;
+	const char *prio = NULL;
+
+	static const char * const prios[] = {
+		[DRM_SCHED_PRIORITY_HIGH] = "HIGH",
+		[DRM_SCHED_PRIORITY_NORMAL] = "NORMAL",
+		[DRM_SCHED_PRIORITY_LOW] = "LOW",
+	};
+
+	if (jm_ctx->slots[0].sched_entity.priority !=
+	    jm_ctx->slots[1].sched_entity.priority)
+		drm_warn(ddev, "Slot priorities should be the same in a single context");
+
+	if (jm_ctx->slots[0].sched_entity.priority < ARRAY_SIZE(prios))
+		prio = prios[jm_ctx->slots[0].sched_entity.priority];
+
+	seq_printf(m, " JM context %u: priority %s config %x\n",
+		   handle, prio ? prio : "UNKNOWN", jm_ctx->config);
+}
+
+static int show_file_jm_ctxs(struct panfrost_file_priv *pfile,
+			     struct seq_file *m)
+{
+	struct panfrost_jm_ctx *jm_ctx;
+	unsigned long i;
+
+	xa_lock(&pfile->jm_ctxs);
+	xa_for_each(&pfile->jm_ctxs, i, jm_ctx) {
+		jm_ctx = panfrost_jm_ctx_get(jm_ctx);
+		xa_unlock(&pfile->jm_ctxs);
+		show_panfrost_jm_ctx(jm_ctx, i, m);
+		panfrost_jm_ctx_put(jm_ctx);
+		xa_lock(&pfile->jm_ctxs);
+	}
+	xa_unlock(&pfile->jm_ctxs);
+
+	return 0;
+}
+
 static struct drm_info_list panthor_debugfs_list[] = {
 	{"gems", panthor_gems_show, 0, NULL},
 };
@@ -725,9 +767,64 @@ static int panthor_gems_debugfs_init(struct drm_minor *minor)
 	return 0;
 }
 
+static int show_each_file(struct seq_file *m, void *arg)
+{
+	struct drm_info_node *node = (struct drm_info_node *)m->private;
+	struct drm_device *ddev = node->minor->dev;
+	int (*show)(struct panfrost_file_priv *, struct seq_file *) =
+		node->info_ent->data;
+	struct drm_file *file;
+	int ret;
+
+	ret = mutex_lock_interruptible(&ddev->filelist_mutex);
+	if (ret)
+		return ret;
+
+	list_for_each_entry(file, &ddev->filelist, lhead) {
+		struct task_struct *task;
+		struct panfrost_file_priv *pfile = file->driver_priv;
+		struct pid *pid;
+
+		/*
+		 * Although we have a valid reference on file->pid, that does
+		 * not guarantee that the task_struct who called get_pid() is
+		 * still alive (e.g. get_pid(current) => fork() => exit()).
+		 * Therefore, we need to protect this ->comm access using RCU.
+		 */
+		rcu_read_lock();
+		pid = rcu_dereference(file->pid);
+		task = pid_task(pid, PIDTYPE_TGID);
+		seq_printf(m, "client_id %8llu pid %8d command %s:\n",
+			   file->client_id, pid_nr(pid),
+			   task ? task->comm : "<unknown>");
+		rcu_read_unlock();
+
+		ret = show(pfile, m);
+		if (ret < 0)
+			break;
+
+		seq_puts(m, "\n");
+	}
+
+	mutex_unlock(&ddev->filelist_mutex);
+	return ret;
+}
+
+static struct drm_info_list panfrost_sched_debugfs_list[] = {
+	{ "sched_ctxs", show_each_file, 0, show_file_jm_ctxs },
+};
+
+static void panfrost_sched_debugfs_init(struct drm_minor *minor)
+{
+	drm_debugfs_create_files(panfrost_sched_debugfs_list,
+				 ARRAY_SIZE(panfrost_sched_debugfs_list),
+				 minor->debugfs_root, minor);
+}
+
 static void panfrost_debugfs_init(struct drm_minor *minor)
 {
 	panthor_gems_debugfs_init(minor);
+	panfrost_sched_debugfs_init(minor);
 }
 #endif
 
-- 
2.50.0

Re: [PATCH v2 1/4] drm/panfrost: Introduce uAPI for JM context creation

[Steven Price]

On 04/09/2025 01:07, Adrián Larumbe wrote:
> From: Boris Brezillon <boris.brezillon@collabora.com>
> 
> The new uAPI lets user space query the KM driver for the available
> priorities a job can be given at submit time. These are managed through
> the notion of a context, for which we also provide new creation and
> destruction ioctls.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  include/uapi/drm/panfrost_drm.h | 50 +++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
> index ed67510395bd..e8b47c9f6976 100644
> --- a/include/uapi/drm/panfrost_drm.h
> +++ b/include/uapi/drm/panfrost_drm.h
> @@ -22,6 +22,8 @@ extern "C" {
>  #define DRM_PANFROST_PERFCNT_DUMP		0x07
>  #define DRM_PANFROST_MADVISE			0x08
>  #define DRM_PANFROST_SET_LABEL_BO		0x09
> +#define DRM_PANFROST_JM_CTX_CREATE		0x0a
> +#define DRM_PANFROST_JM_CTX_DESTROY		0x0b
>  
>  #define DRM_IOCTL_PANFROST_SUBMIT		DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_SUBMIT, struct drm_panfrost_submit)
>  #define DRM_IOCTL_PANFROST_WAIT_BO		DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_WAIT_BO, struct drm_panfrost_wait_bo)
> @@ -31,6 +33,8 @@ extern "C" {
>  #define DRM_IOCTL_PANFROST_GET_BO_OFFSET	DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_GET_BO_OFFSET, struct drm_panfrost_get_bo_offset)
>  #define DRM_IOCTL_PANFROST_MADVISE		DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_MADVISE, struct drm_panfrost_madvise)
>  #define DRM_IOCTL_PANFROST_SET_LABEL_BO		DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_SET_LABEL_BO, struct drm_panfrost_set_label_bo)
> +#define DRM_IOCTL_PANFROST_JM_CTX_CREATE	DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_JM_CTX_CREATE, struct drm_panfrost_jm_ctx_create)
> +#define DRM_IOCTL_PANFROST_JM_CTX_DESTROY	DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_JM_CTX_DESTROY, struct drm_panfrost_jm_ctx_destroy)
>  
>  /*
>   * Unstable ioctl(s): only exposed when the unsafe unstable_ioctls module
> @@ -71,6 +75,12 @@ struct drm_panfrost_submit {
>  
>  	/** A combination of PANFROST_JD_REQ_* */
>  	__u32 requirements;
> +
> +	/** JM context handle. Zero if you want to use the default context. */
> +	__u32 jm_ctx_handle;
> +
> +	/** Padding field. MBZ. */
> +	__u32 pad;
>  };
>  
>  /**
> @@ -177,6 +187,7 @@ enum drm_panfrost_param {
>  	DRM_PANFROST_PARAM_AFBC_FEATURES,
>  	DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP,
>  	DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP_FREQUENCY,
> +	DRM_PANFROST_PARAM_ALLOWED_JM_CTX_PRIORITIES,
>  };
>  
>  struct drm_panfrost_get_param {
> @@ -299,6 +310,45 @@ struct panfrost_dump_registers {
>  	__u32 value;
>  };
>  
> +enum drm_panfrost_jm_ctx_priority {
> +	/**
> +	 * @PANFROST_JM_CTX_PRIORITY_LOW: Low priority context.
> +	 */
> +	PANFROST_JM_CTX_PRIORITY_LOW = 0,
> +
> +	/**
> +	 * @PANFROST_JM_CTX_PRIORITY_MEDIUM: Medium priority context.
> +	 */
> +	PANFROST_JM_CTX_PRIORITY_MEDIUM,
> +
> +	/**
> +	 * @PANFROST_JM_CTX_PRIORITY_HIGH: High priority context.
> +	 *
> +	 * Requires CAP_SYS_NICE or DRM_MASTER.
> +	 */
> +	PANFROST_JM_CTX_PRIORITY_HIGH,
> +};
> +
> +struct drm_panfrost_jm_ctx_create {
> +	/** @handle: Handle of the created JM context */
> +	__u32 handle;
> +
> +	/** @priority: Context priority (see enum drm_panfrost_jm_ctx_priority). */
> +	__u32 priority;
> +};
> +
> +struct drm_panfrost_jm_ctx_destroy {
> +	/**
> +	 * @handle: Handle of the JM context to destroy.
> +	 *
> +	 * Must be a valid context handle returned by DRM_IOCTL_PANTHOR_JM_CTX_CREATE.
> +	 */
> +	__u32 handle;
> +
> +	/** @pad: Padding field, MBZ. */
> +	__u32 pad;
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif

Re: [PATCH v2 2/4] drm/panfrost: Introduce JM contexts for manging job resources

[Steven Price]

On 04/09/2025 01:08, Adrián Larumbe wrote:
> From: Boris Brezillon <boris.brezillon@collabora.com>
> 
> A JM context describes GPU HW resourdce allocation for the jobs bound to
> it. At the time of writing this, it only holds the JS and queue
> scheduler priorities.
> 
> When a context is created, all the scheduler entities created for job
> slots will have the same priority.
> 
> Until context creation and destruction and attaching a context ID to
> a job submission are exposed to UM, all jobs shall be bound to the
> default Panfrost file context, which has medium priority.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.h |   4 +-
>  drivers/gpu/drm/panfrost/panfrost_drv.c    |  19 +-
>  drivers/gpu/drm/panfrost/panfrost_job.c    | 203 +++++++++++++++++----
>  drivers/gpu/drm/panfrost/panfrost_job.h    |  25 ++-
>  4 files changed, 210 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 077525a3ad68..5b164871eb95 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -15,6 +15,7 @@
>  #include <drm/gpu_scheduler.h>
>  
>  #include "panfrost_devfreq.h"
> +#include "panfrost_job.h"
>  
>  struct panfrost_device;
>  struct panfrost_mmu;
> @@ -22,7 +23,6 @@ struct panfrost_job_slot;
>  struct panfrost_job;
>  struct panfrost_perfcnt;
>  
> -#define NUM_JOB_SLOTS 3
>  #define MAX_PM_DOMAINS 5
>  
>  enum panfrost_drv_comp_bits {
> @@ -206,7 +206,7 @@ struct panfrost_engine_usage {
>  struct panfrost_file_priv {
>  	struct panfrost_device *pfdev;
>  
> -	struct drm_sched_entity sched_entity[NUM_JOB_SLOTS];
> +	struct xarray jm_ctxs;
>  
>  	struct panfrost_mmu *mmu;
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 1ea6c509a5d5..398c067457d9 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -279,6 +279,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
>  	struct panfrost_file_priv *file_priv = file->driver_priv;
>  	struct drm_panfrost_submit *args = data;
>  	struct drm_syncobj *sync_out = NULL;
> +	struct panfrost_jm_ctx *jm_ctx;
>  	struct panfrost_job *job;
>  	int ret = 0, slot;
>  
> @@ -294,10 +295,17 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
>  			return -ENODEV;
>  	}
>  
> +	/* TODO: Use the default JM context until ctx management IOCTLs are exposed */
> +	jm_ctx = panfrost_jm_ctx_from_handle(file, 0);
> +	if (!jm_ctx) {
> +		ret = -EINVAL;
> +		goto out_put_syncout;
> +	}
> +
>  	job = kzalloc(sizeof(*job), GFP_KERNEL);
>  	if (!job) {
>  		ret = -ENOMEM;
> -		goto out_put_syncout;
> +		goto out_put_jm_ctx;
>  	}
>  
>  	kref_init(&job->refcount);
> @@ -307,12 +315,13 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
>  	job->requirements = args->requirements;
>  	job->flush_id = panfrost_gpu_get_latest_flush_id(pfdev);
>  	job->mmu = file_priv->mmu;
> +	job->ctx = panfrost_jm_ctx_get(jm_ctx);
>  	job->engine_usage = &file_priv->engine_usage;
>  
>  	slot = panfrost_job_get_slot(job);
>  
>  	ret = drm_sched_job_init(&job->base,
> -				 &file_priv->sched_entity[slot],
> +				 &jm_ctx->slots[slot].sched_entity,
>  				 1, NULL, file->client_id);
>  	if (ret)
>  		goto out_put_job;
> @@ -338,6 +347,8 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
>  		drm_sched_job_cleanup(&job->base);
>  out_put_job:
>  	panfrost_job_put(job);
> +out_put_jm_ctx:
> +	panfrost_jm_ctx_put(jm_ctx);
>  out_put_syncout:
>  	if (sync_out)
>  		drm_syncobj_put(sync_out);
> @@ -564,7 +575,7 @@ panfrost_open(struct drm_device *dev, struct drm_file *file)
>  		goto err_free;
>  	}
>  
> -	ret = panfrost_job_open(panfrost_priv);
> +	ret = panfrost_job_open(file);
>  	if (ret)
>  		goto err_job;
>  
> @@ -583,7 +594,7 @@ panfrost_postclose(struct drm_device *dev, struct drm_file *file)
>  	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
>  
>  	panfrost_perfcnt_close(file);
> -	panfrost_job_close(panfrost_priv);
> +	panfrost_job_close(file);
>  
>  	panfrost_mmu_ctx_put(panfrost_priv->mmu);
>  	kfree(panfrost_priv);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 82acabb21b27..b6853add307c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -8,6 +8,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/dma-resv.h>
> +#include <drm/drm_auth.h>
>  #include <drm/gpu_scheduler.h>
>  #include <drm/panfrost_drm.h>
>  
> @@ -22,6 +23,7 @@
>  #include "panfrost_mmu.h"
>  #include "panfrost_dump.h"
>  
> +#define MAX_JM_CTX_PER_FILE 128

128 seems like a very large number, do we have a reason to set this so high?

>  #define JOB_TIMEOUT_MS 500
>  
>  #define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
> @@ -222,7 +224,7 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  
>  	/* start MMU, medium priority, cache clean/flush on end, clean/flush on
>  	 * start */
> -	cfg |= JS_CONFIG_THREAD_PRI(8) |
> +	cfg |= job->ctx->config |

Note that the thread priority on Midgard/Bifrost is pretty pointless. I
don't believe kbase ever set this to anything other than 8. In theory it
should balance priority between different slots but "How the priority
values affect issue rates from different slots is implementation
dependent" (which I think is wording for 'not implemented'!).

My main concern is that since kbase never used it there's a possibility
for undiscovered hardware bugs.

>  		JS_CONFIG_START_FLUSH_CLEAN_INVALIDATE |
>  		JS_CONFIG_END_FLUSH_CLEAN_INVALIDATE |
>  		panfrost_get_job_chain_flag(job);
> @@ -359,6 +361,7 @@ static void panfrost_job_cleanup(struct kref *ref)
>  		kvfree(job->bos);
>  	}
>  
> +	panfrost_jm_ctx_put(job->ctx);
>  	kfree(job);
>  }
>  
> @@ -917,39 +920,184 @@ void panfrost_job_fini(struct panfrost_device *pfdev)
>  	destroy_workqueue(pfdev->reset.wq);
>  }
>  
> -int panfrost_job_open(struct panfrost_file_priv *panfrost_priv)
> +int panfrost_job_open(struct drm_file *file)
> +{
> +	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
> +	int ret;
> +
> +	struct drm_panfrost_jm_ctx_create default_jm_ctx = {
> +		.priority = PANFROST_JM_CTX_PRIORITY_MEDIUM,
> +	};
> +
> +	xa_init_flags(&panfrost_priv->jm_ctxs, XA_FLAGS_ALLOC);
> +
> +	ret = panfrost_jm_ctx_create(file, &default_jm_ctx);
> +	if (ret)
> +		return ret;
> +
> +	/* We expect the default context to be assigned handle 0. */
> +	if (WARN_ON(default_jm_ctx.handle))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +void panfrost_job_close(struct drm_file *file)
> +{
> +	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
> +	struct panfrost_jm_ctx *jm_ctx;
> +	unsigned long i;
> +
> +	xa_for_each(&panfrost_priv->jm_ctxs, i, jm_ctx)
> +		panfrost_jm_ctx_destroy(file, i);
> +
> +	xa_destroy(&panfrost_priv->jm_ctxs);
> +}
> +
> +int panfrost_job_is_idle(struct panfrost_device *pfdev)
>  {
> -	struct panfrost_device *pfdev = panfrost_priv->pfdev;
>  	struct panfrost_job_slot *js = pfdev->js;
> -	struct drm_gpu_scheduler *sched;
> -	int ret, i;
> +	int i;
>  
>  	for (i = 0; i < NUM_JOB_SLOTS; i++) {
> -		sched = &js->queue[i].sched;
> -		ret = drm_sched_entity_init(&panfrost_priv->sched_entity[i],
> -					    DRM_SCHED_PRIORITY_NORMAL, &sched,
> -					    1, NULL);
> -		if (WARN_ON(ret))
> -			return ret;
> +		/* If there are any jobs in the HW queue, we're not idle */
> +		if (atomic_read(&js->queue[i].sched.credit_count))
> +			return false;
>  	}
> +
> +	return true;
> +}
> +
> +static void panfrost_jm_ctx_release(struct kref *kref)
> +{
> +	struct panfrost_jm_ctx *jm_ctx = container_of(kref, struct panfrost_jm_ctx, refcnt);
> +
> +	kfree(jm_ctx);
> +}
> +
> +void
> +panfrost_jm_ctx_put(struct panfrost_jm_ctx *jm_ctx)
> +{
> +	if (jm_ctx)
> +		kref_put(&jm_ctx->refcnt, panfrost_jm_ctx_release);
> +}
> +
> +struct panfrost_jm_ctx *
> +panfrost_jm_ctx_get(struct panfrost_jm_ctx *jm_ctx)
> +{
> +	if (jm_ctx)
> +		kref_get(&jm_ctx->refcnt);
> +
> +	return jm_ctx;
> +}
> +
> +struct panfrost_jm_ctx *
> +panfrost_jm_ctx_from_handle(struct drm_file *file, u32 handle)
> +{
> +	struct panfrost_file_priv *priv = file->driver_priv;
> +	struct panfrost_jm_ctx *jm_ctx;
> +
> +	xa_lock(&priv->jm_ctxs);
> +	jm_ctx = panfrost_jm_ctx_get(xa_load(&priv->jm_ctxs, handle));
> +	xa_unlock(&priv->jm_ctxs);
> +
> +	return jm_ctx;
> +}
> +
> +static int jm_ctx_prio_to_drm_sched_prio(struct drm_file *file,
> +					 enum drm_panfrost_jm_ctx_priority in,
> +					 enum drm_sched_priority *out)
> +{
> +	switch (in) {
> +	case PANFROST_JM_CTX_PRIORITY_LOW:
> +		*out = DRM_SCHED_PRIORITY_LOW;
> +		return 0;
> +	case PANFROST_JM_CTX_PRIORITY_MEDIUM:
> +		*out = DRM_SCHED_PRIORITY_NORMAL;
> +		return 0;
> +	case PANFROST_JM_CTX_PRIORITY_HIGH:
> +		/* Higher priorities require CAP_SYS_NICE or DRM_MASTER */
> +		if (!capable(CAP_SYS_NICE) && !drm_is_current_master(file))
> +			return -EACCES;
> +
> +		*out = DRM_SCHED_PRIORITY_HIGH;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +int panfrost_jm_ctx_create(struct drm_file *file,
> +			   struct drm_panfrost_jm_ctx_create *args)
> +{
> +	struct panfrost_file_priv *priv = file->driver_priv;
> +	struct panfrost_device *pfdev = priv->pfdev;
> +	enum drm_sched_priority sched_prio;
> +	struct panfrost_jm_ctx *jm_ctx;
> +
> +	int ret;
> +
> +	jm_ctx = kzalloc(sizeof(*jm_ctx), GFP_KERNEL);
> +	if (!jm_ctx)
> +		return -ENOMEM;
> +
> +	kref_init(&jm_ctx->refcnt);
> +
> +	/* Same priority for all JS within a single context */
> +	jm_ctx->config = JS_CONFIG_THREAD_PRI(args->priority);
> +
> +	ret = jm_ctx_prio_to_drm_sched_prio(file, args->priority, &sched_prio);
> +	if (ret)
> +		goto err_put_jm_ctx;
> +
> +	for (u32 i = 0; i < NUM_JOB_SLOTS - 1; i++) {
> +		struct drm_gpu_scheduler *sched = &pfdev->js->queue[i].sched;
> +		struct panfrost_js_ctx *js_ctx = &jm_ctx->slots[i];
> +
> +		ret = drm_sched_entity_init(&js_ctx->sched_entity, sched_prio,
> +					    &sched, 1, NULL);
> +		if (ret)
> +			goto err_put_jm_ctx;
> +
> +		js_ctx->enabled = true;
> +	}
> +
> +	ret = xa_alloc(&priv->jm_ctxs, &args->handle, jm_ctx,
> +		       XA_LIMIT(0, MAX_JM_CTX_PER_FILE), GFP_KERNEL);
> +	if (ret)
> +		goto err_put_jm_ctx;

On error here we just jump down and call panfrost_jm_ctx_put() which
will free jm_ctx but won't destroy any of the drm_sched_entities. There
seems to be something a bit off with the lifetime management here.

Should panfrost_jm_ctx_release() be responsible for tearing down the
context, and panfrost_jm_ctx_destroy() be nothing more than dropping the
reference?

Steve

> +
>  	return 0;
> +
> +err_put_jm_ctx:
> +	panfrost_jm_ctx_put(jm_ctx);
> +	return ret;
>  }
>  
> -void panfrost_job_close(struct panfrost_file_priv *panfrost_priv)
> +int panfrost_jm_ctx_destroy(struct drm_file *file, u32 handle)
>  {
> -	struct panfrost_device *pfdev = panfrost_priv->pfdev;
> -	int i;
> +	struct panfrost_file_priv *priv = file->driver_priv;
> +	struct panfrost_device *pfdev = priv->pfdev;
> +	struct panfrost_jm_ctx *jm_ctx;
>  
> -	for (i = 0; i < NUM_JOB_SLOTS; i++)
> -		drm_sched_entity_destroy(&panfrost_priv->sched_entity[i]);
> +	jm_ctx = xa_erase(&priv->jm_ctxs, handle);
> +	if (!jm_ctx)
> +		return -EINVAL;
> +
> +	for (u32 i = 0; i < ARRAY_SIZE(jm_ctx->slots); i++) {
> +		if (jm_ctx->slots[i].enabled)
> +			drm_sched_entity_destroy(&jm_ctx->slots[i].sched_entity);
> +	}
>  
>  	/* Kill in-flight jobs */
>  	spin_lock(&pfdev->js->job_lock);
> -	for (i = 0; i < NUM_JOB_SLOTS; i++) {
> -		struct drm_sched_entity *entity = &panfrost_priv->sched_entity[i];
> -		int j;
> +	for (u32 i = 0; i < ARRAY_SIZE(jm_ctx->slots); i++) {
> +		struct drm_sched_entity *entity = &jm_ctx->slots[i].sched_entity;
> +
> +		if (!jm_ctx->slots[i].enabled)
> +			continue;
>  
> -		for (j = ARRAY_SIZE(pfdev->jobs[0]) - 1; j >= 0; j--) {
> +		for (int j = ARRAY_SIZE(pfdev->jobs[0]) - 1; j >= 0; j--) {
>  			struct panfrost_job *job = pfdev->jobs[i][j];
>  			u32 cmd;
>  
> @@ -980,18 +1128,7 @@ void panfrost_job_close(struct panfrost_file_priv *panfrost_priv)
>  		}
>  	}
>  	spin_unlock(&pfdev->js->job_lock);
> -}
>  
> -int panfrost_job_is_idle(struct panfrost_device *pfdev)
> -{
> -	struct panfrost_job_slot *js = pfdev->js;
> -	int i;
> -
> -	for (i = 0; i < NUM_JOB_SLOTS; i++) {
> -		/* If there are any jobs in the HW queue, we're not idle */
> -		if (atomic_read(&js->queue[i].sched.credit_count))
> -			return false;
> -	}
> -
> -	return true;
> +	panfrost_jm_ctx_put(jm_ctx);
> +	return 0;
>  }
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
> index ec581b97852b..f3e5010e6cf5 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.h
> @@ -18,6 +18,7 @@ struct panfrost_job {
>  
>  	struct panfrost_device *pfdev;
>  	struct panfrost_mmu *mmu;
> +	struct panfrost_jm_ctx *ctx;
>  
>  	/* Fence to be signaled by IRQ handler when the job is complete. */
>  	struct dma_fence *done_fence;
> @@ -39,10 +40,30 @@ struct panfrost_job {
>  	u64 start_cycles;
>  };
>  
> +struct panfrost_js_ctx {
> +	struct drm_sched_entity sched_entity;
> +	bool enabled;
> +};
> +
> +#define NUM_JOB_SLOTS 3
> +
> +struct panfrost_jm_ctx {
> +	struct kref refcnt;
> +	u32 config;
> +	struct panfrost_js_ctx slots[NUM_JOB_SLOTS];
> +};
> +
> +int panfrost_jm_ctx_create(struct drm_file *file,
> +			   struct drm_panfrost_jm_ctx_create *args);
> +int panfrost_jm_ctx_destroy(struct drm_file *file, u32 handle);
> +void panfrost_jm_ctx_put(struct panfrost_jm_ctx *jm_ctx);
> +struct panfrost_jm_ctx *panfrost_jm_ctx_get(struct panfrost_jm_ctx *jm_ctx);
> +struct panfrost_jm_ctx *panfrost_jm_ctx_from_handle(struct drm_file *file, u32 handle);
> +
>  int panfrost_job_init(struct panfrost_device *pfdev);
>  void panfrost_job_fini(struct panfrost_device *pfdev);
> -int panfrost_job_open(struct panfrost_file_priv *panfrost_priv);
> -void panfrost_job_close(struct panfrost_file_priv *panfrost_priv);
> +int panfrost_job_open(struct drm_file *file);
> +void panfrost_job_close(struct drm_file *file);
>  int panfrost_job_get_slot(struct panfrost_job *job);
>  int panfrost_job_push(struct panfrost_job *job);
>  void panfrost_job_put(struct panfrost_job *job);

Re: [PATCH v2 3/4] drm/panfrost: Expose JM context IOCTLs to UM

[Steven Price]

On 04/09/2025 01:08, Adrián Larumbe wrote:
> From: Boris Brezillon <boris.brezillon@collabora.com>
> 
> Minor revision of the driver must be bumped because this expands the
> uAPI. On top of that, let user know the available priorities so that
> they can create contexts with legal priority values.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 35 +++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 398c067457d9..02f704ec4961 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -13,6 +13,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <drm/panfrost_drm.h>
> +#include <drm/drm_auth.h>
>  #include <drm/drm_debugfs.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_ioctl.h>
> @@ -109,6 +110,15 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
>  #endif
>  		break;
>  
> +	case DRM_PANFROST_PARAM_ALLOWED_JM_CTX_PRIORITIES:
> +		param->value = BIT(PANFROST_JM_CTX_PRIORITY_LOW) |
> +			       BIT(PANFROST_JM_CTX_PRIORITY_MEDIUM);
> +
> +		/* High prio require CAP_SYS_NICE or DRM_MASTER */
> +		if (capable(CAP_SYS_NICE) || drm_is_current_master(file))

NIT: This is repeating the check in jm_ctx_prio_to_drm_sched_prio(). It
would be nice to have this check in one place to ensure that the two
cannot get out of sync.

> +			param->value |= BIT(PANFROST_JM_CTX_PRIORITY_HIGH);
> +		break;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -295,8 +305,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
>  			return -ENODEV;
>  	}
>  
> -	/* TODO: Use the default JM context until ctx management IOCTLs are exposed */
> -	jm_ctx = panfrost_jm_ctx_from_handle(file, 0);
> +	jm_ctx = panfrost_jm_ctx_from_handle(file, args->jm_ctx_handle);

I'm not sure if this should go in this patch or the previous one, but:
we need to add a check that the padding is 0. Personally I'd be tempted
to put it in the previous patch and enforce that jm_ctx_handle is zeroed
too (and drop that check here when you also remove the TODO).

>  	if (!jm_ctx) {
>  		ret = -EINVAL;
>  		goto out_put_syncout;
> @@ -547,6 +556,24 @@ static int panfrost_ioctl_set_label_bo(struct drm_device *ddev, void *data,
>  	return ret;
>  }
>  
> +static int panfrost_ioctl_jm_ctx_create(struct drm_device *dev, void *data,
> +					struct drm_file *file)
> +{
> +	return panfrost_jm_ctx_create(file, data);
> +}
> +
> +static int panfrost_ioctl_jm_ctx_destroy(struct drm_device *dev, void *data,
> +					 struct drm_file *file)
> +{
> +	const struct drm_panfrost_jm_ctx_destroy *args = data;
> +
> +	/* We can't destroy the default context created when the file is opened. */
> +	if (!args->handle)
> +		return -EINVAL;

Also need a check for padding being non-zero.

Thanks,
Steve

> +
> +	return panfrost_jm_ctx_destroy(file, args->handle);
> +}
> +
>  int panfrost_unstable_ioctl_check(void)
>  {
>  	if (!unstable_ioctls)
> @@ -614,6 +641,8 @@ static const struct drm_ioctl_desc panfrost_drm_driver_ioctls[] = {
>  	PANFROST_IOCTL(PERFCNT_DUMP,	perfcnt_dump,	DRM_RENDER_ALLOW),
>  	PANFROST_IOCTL(MADVISE,		madvise,	DRM_RENDER_ALLOW),
>  	PANFROST_IOCTL(SET_LABEL_BO,	set_label_bo,	DRM_RENDER_ALLOW),
> +	PANFROST_IOCTL(JM_CTX_CREATE,	jm_ctx_create,	DRM_RENDER_ALLOW),
> +	PANFROST_IOCTL(JM_CTX_DESTROY,	jm_ctx_destroy,	DRM_RENDER_ALLOW),
>  };
>  
>  static void panfrost_gpu_show_fdinfo(struct panfrost_device *pfdev,
> @@ -710,6 +739,8 @@ static void panfrost_debugfs_init(struct drm_minor *minor)
>   * - 1.3 - adds JD_REQ_CYCLE_COUNT job requirement for SUBMIT
>   *       - adds SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY queries
>   * - 1.4 - adds SET_LABEL_BO
> + * - 1.5 - adds JM_CTX_{CREATE,DESTROY} ioctls and extend SUBMIT to allow
> + *	   context creation with configurable priorities/affinity
>   */
>  static const struct drm_driver panfrost_drm_driver = {
>  	.driver_features	= DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ,

Re: [PATCH v2 4/4] drm/panfrost: Display list of device JM contexts over debugfs

[Steven Price]

On 04/09/2025 01:08, Adrián Larumbe wrote:
> From: Boris Brezillon <boris.brezillon@collabora.com>
> 
> For DebugFS builds, create a filesystem knob that, for every single open
> file of the Panfrost DRM device, shows its command name information and
> PID (when applicable), and all of its existing JM contexts.
> 
> For every context, show its priority and job config.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 97 +++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 02f704ec4961..b3d14b887da4 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -712,6 +712,48 @@ static int panthor_gems_show(struct seq_file *m, void *data)
>  	return 0;
>  }
>  
> +static void show_panfrost_jm_ctx(struct panfrost_jm_ctx *jm_ctx, u32 handle,
> +				 struct seq_file *m)
> +{
> +	struct drm_device *ddev = ((struct drm_info_node *)m->private)->minor->dev;
> +	const char *prio = NULL;
> +
> +	static const char * const prios[] = {
> +		[DRM_SCHED_PRIORITY_HIGH] = "HIGH",
> +		[DRM_SCHED_PRIORITY_NORMAL] = "NORMAL",
> +		[DRM_SCHED_PRIORITY_LOW] = "LOW",
> +	};
> +
> +	if (jm_ctx->slots[0].sched_entity.priority !=
> +	    jm_ctx->slots[1].sched_entity.priority)
> +		drm_warn(ddev, "Slot priorities should be the same in a single context");
> +
> +	if (jm_ctx->slots[0].sched_entity.priority < ARRAY_SIZE(prios))
> +		prio = prios[jm_ctx->slots[0].sched_entity.priority];
> +
> +	seq_printf(m, " JM context %u: priority %s config %x\n",
> +		   handle, prio ? prio : "UNKNOWN", jm_ctx->config);

NIT: If you assign prio to "UNKNOWN" to begin with (rather than NULL)
you can avoid this ?: operator.

> +}
> +
> +static int show_file_jm_ctxs(struct panfrost_file_priv *pfile,
> +			     struct seq_file *m)
> +{
> +	struct panfrost_jm_ctx *jm_ctx;
> +	unsigned long i;
> +
> +	xa_lock(&pfile->jm_ctxs);
> +	xa_for_each(&pfile->jm_ctxs, i, jm_ctx) {
> +		jm_ctx = panfrost_jm_ctx_get(jm_ctx);
> +		xa_unlock(&pfile->jm_ctxs);
> +		show_panfrost_jm_ctx(jm_ctx, i, m);
> +		panfrost_jm_ctx_put(jm_ctx);
> +		xa_lock(&pfile->jm_ctxs);
> +	}
> +	xa_unlock(&pfile->jm_ctxs);

Is it so bad if we just held the xa lock for the whole loop? It just
seems unnecessarily complex.

Thanks,
Steve

> +
> +	return 0;
> +}
> +
>  static struct drm_info_list panthor_debugfs_list[] = {
>  	{"gems", panthor_gems_show, 0, NULL},
>  };
> @@ -725,9 +767,64 @@ static int panthor_gems_debugfs_init(struct drm_minor *minor)
>  	return 0;
>  }
>  
> +static int show_each_file(struct seq_file *m, void *arg)
> +{
> +	struct drm_info_node *node = (struct drm_info_node *)m->private;
> +	struct drm_device *ddev = node->minor->dev;
> +	int (*show)(struct panfrost_file_priv *, struct seq_file *) =
> +		node->info_ent->data;
> +	struct drm_file *file;
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&ddev->filelist_mutex);
> +	if (ret)
> +		return ret;
> +
> +	list_for_each_entry(file, &ddev->filelist, lhead) {
> +		struct task_struct *task;
> +		struct panfrost_file_priv *pfile = file->driver_priv;
> +		struct pid *pid;
> +
> +		/*
> +		 * Although we have a valid reference on file->pid, that does
> +		 * not guarantee that the task_struct who called get_pid() is
> +		 * still alive (e.g. get_pid(current) => fork() => exit()).
> +		 * Therefore, we need to protect this ->comm access using RCU.
> +		 */
> +		rcu_read_lock();
> +		pid = rcu_dereference(file->pid);
> +		task = pid_task(pid, PIDTYPE_TGID);
> +		seq_printf(m, "client_id %8llu pid %8d command %s:\n",
> +			   file->client_id, pid_nr(pid),
> +			   task ? task->comm : "<unknown>");
> +		rcu_read_unlock();
> +
> +		ret = show(pfile, m);
> +		if (ret < 0)
> +			break;
> +
> +		seq_puts(m, "\n");
> +	}
> +
> +	mutex_unlock(&ddev->filelist_mutex);
> +	return ret;
> +}
> +
> +static struct drm_info_list panfrost_sched_debugfs_list[] = {
> +	{ "sched_ctxs", show_each_file, 0, show_file_jm_ctxs },
> +};
> +
> +static void panfrost_sched_debugfs_init(struct drm_minor *minor)
> +{
> +	drm_debugfs_create_files(panfrost_sched_debugfs_list,
> +				 ARRAY_SIZE(panfrost_sched_debugfs_list),
> +				 minor->debugfs_root, minor);
> +}
> +
>  static void panfrost_debugfs_init(struct drm_minor *minor)
>  {
>  	panthor_gems_debugfs_init(minor);
> +	panfrost_sched_debugfs_init(minor);
>  }
>  #endif
>  

Re: [PATCH v2 2/4] drm/panfrost: Introduce JM contexts for manging job resources

[Boris Brezillon]

On Wed, 10 Sep 2025 16:42:32 +0100
Steven Price <steven.price@arm.com> wrote:

> > +int panfrost_jm_ctx_create(struct drm_file *file,
> > +			   struct drm_panfrost_jm_ctx_create *args)
> > +{
> > +	struct panfrost_file_priv *priv = file->driver_priv;
> > +	struct panfrost_device *pfdev = priv->pfdev;
> > +	enum drm_sched_priority sched_prio;
> > +	struct panfrost_jm_ctx *jm_ctx;
> > +
> > +	int ret;
> > +
> > +	jm_ctx = kzalloc(sizeof(*jm_ctx), GFP_KERNEL);
> > +	if (!jm_ctx)
> > +		return -ENOMEM;
> > +
> > +	kref_init(&jm_ctx->refcnt);
> > +
> > +	/* Same priority for all JS within a single context */
> > +	jm_ctx->config = JS_CONFIG_THREAD_PRI(args->priority);
> > +
> > +	ret = jm_ctx_prio_to_drm_sched_prio(file, args->priority, &sched_prio);
> > +	if (ret)
> > +		goto err_put_jm_ctx;
> > +
> > +	for (u32 i = 0; i < NUM_JOB_SLOTS - 1; i++) {
> > +		struct drm_gpu_scheduler *sched = &pfdev->js->queue[i].sched;
> > +		struct panfrost_js_ctx *js_ctx = &jm_ctx->slots[i];
> > +
> > +		ret = drm_sched_entity_init(&js_ctx->sched_entity, sched_prio,
> > +					    &sched, 1, NULL);
> > +		if (ret)
> > +			goto err_put_jm_ctx;
> > +
> > +		js_ctx->enabled = true;
> > +	}
> > +
> > +	ret = xa_alloc(&priv->jm_ctxs, &args->handle, jm_ctx,
> > +		       XA_LIMIT(0, MAX_JM_CTX_PER_FILE), GFP_KERNEL);
> > +	if (ret)
> > +		goto err_put_jm_ctx;  
> 
> On error here we just jump down and call panfrost_jm_ctx_put() which
> will free jm_ctx but won't destroy any of the drm_sched_entities. There
> seems to be something a bit off with the lifetime management here.
> 
> Should panfrost_jm_ctx_release() be responsible for tearing down the
> context, and panfrost_jm_ctx_destroy() be nothing more than dropping the
> reference?

The idea was to kill/cancel any pending jobs as soon as userspace
releases the context, like we were doing previously when the FD was
closed. If we defer this ctx teardown to the release() function, we're
basically waiting for all jobs to complete, which:

1. doesn't encourage userspace to have proper control over the contexts
   lifetime
2. might use GPU/mem resources to execute jobs no one cares about
   anymore

Re: [PATCH v2 2/4] drm/panfrost: Introduce JM contexts for manging job resources

[Steven Price]

On 10/09/2025 16:52, Boris Brezillon wrote:
> On Wed, 10 Sep 2025 16:42:32 +0100
> Steven Price <steven.price@arm.com> wrote:
> 
>>> +int panfrost_jm_ctx_create(struct drm_file *file,
>>> +			   struct drm_panfrost_jm_ctx_create *args)
>>> +{
>>> +	struct panfrost_file_priv *priv = file->driver_priv;
>>> +	struct panfrost_device *pfdev = priv->pfdev;
>>> +	enum drm_sched_priority sched_prio;
>>> +	struct panfrost_jm_ctx *jm_ctx;
>>> +
>>> +	int ret;
>>> +
>>> +	jm_ctx = kzalloc(sizeof(*jm_ctx), GFP_KERNEL);
>>> +	if (!jm_ctx)
>>> +		return -ENOMEM;
>>> +
>>> +	kref_init(&jm_ctx->refcnt);
>>> +
>>> +	/* Same priority for all JS within a single context */
>>> +	jm_ctx->config = JS_CONFIG_THREAD_PRI(args->priority);
>>> +
>>> +	ret = jm_ctx_prio_to_drm_sched_prio(file, args->priority, &sched_prio);
>>> +	if (ret)
>>> +		goto err_put_jm_ctx;
>>> +
>>> +	for (u32 i = 0; i < NUM_JOB_SLOTS - 1; i++) {
>>> +		struct drm_gpu_scheduler *sched = &pfdev->js->queue[i].sched;
>>> +		struct panfrost_js_ctx *js_ctx = &jm_ctx->slots[i];
>>> +
>>> +		ret = drm_sched_entity_init(&js_ctx->sched_entity, sched_prio,
>>> +					    &sched, 1, NULL);
>>> +		if (ret)
>>> +			goto err_put_jm_ctx;
>>> +
>>> +		js_ctx->enabled = true;
>>> +	}
>>> +
>>> +	ret = xa_alloc(&priv->jm_ctxs, &args->handle, jm_ctx,
>>> +		       XA_LIMIT(0, MAX_JM_CTX_PER_FILE), GFP_KERNEL);
>>> +	if (ret)
>>> +		goto err_put_jm_ctx;  
>>
>> On error here we just jump down and call panfrost_jm_ctx_put() which
>> will free jm_ctx but won't destroy any of the drm_sched_entities. There
>> seems to be something a bit off with the lifetime management here.
>>
>> Should panfrost_jm_ctx_release() be responsible for tearing down the
>> context, and panfrost_jm_ctx_destroy() be nothing more than dropping the
>> reference?
> 
> The idea was to kill/cancel any pending jobs as soon as userspace
> releases the context, like we were doing previously when the FD was
> closed. If we defer this ctx teardown to the release() function, we're
> basically waiting for all jobs to complete, which:
> 
> 1. doesn't encourage userspace to have proper control over the contexts
>    lifetime
> 2. might use GPU/mem resources to execute jobs no one cares about
>    anymore

Ah, good point - yes killing the jobs in panfrost_jm_ctx_destroy() makes
sense. But we still need to ensure the clean-up happens in the other
paths ;)

So panfrost_jm_ctx_destroy() should keep the killing jobs part, butthe
drm scheduler entity cleanup should be moved.

Thanks,
Steve

Re: [PATCH v2 2/4] drm/panfrost: Introduce JM contexts for manging job resources

[Boris Brezillon]

On Wed, 10 Sep 2025 16:56:43 +0100
Steven Price <steven.price@arm.com> wrote:

> On 10/09/2025 16:52, Boris Brezillon wrote:
> > On Wed, 10 Sep 2025 16:42:32 +0100
> > Steven Price <steven.price@arm.com> wrote:
> >   
> >>> +int panfrost_jm_ctx_create(struct drm_file *file,
> >>> +			   struct drm_panfrost_jm_ctx_create *args)
> >>> +{
> >>> +	struct panfrost_file_priv *priv = file->driver_priv;
> >>> +	struct panfrost_device *pfdev = priv->pfdev;
> >>> +	enum drm_sched_priority sched_prio;
> >>> +	struct panfrost_jm_ctx *jm_ctx;
> >>> +
> >>> +	int ret;
> >>> +
> >>> +	jm_ctx = kzalloc(sizeof(*jm_ctx), GFP_KERNEL);
> >>> +	if (!jm_ctx)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	kref_init(&jm_ctx->refcnt);
> >>> +
> >>> +	/* Same priority for all JS within a single context */
> >>> +	jm_ctx->config = JS_CONFIG_THREAD_PRI(args->priority);
> >>> +
> >>> +	ret = jm_ctx_prio_to_drm_sched_prio(file, args->priority, &sched_prio);
> >>> +	if (ret)
> >>> +		goto err_put_jm_ctx;
> >>> +
> >>> +	for (u32 i = 0; i < NUM_JOB_SLOTS - 1; i++) {
> >>> +		struct drm_gpu_scheduler *sched = &pfdev->js->queue[i].sched;
> >>> +		struct panfrost_js_ctx *js_ctx = &jm_ctx->slots[i];
> >>> +
> >>> +		ret = drm_sched_entity_init(&js_ctx->sched_entity, sched_prio,
> >>> +					    &sched, 1, NULL);
> >>> +		if (ret)
> >>> +			goto err_put_jm_ctx;
> >>> +
> >>> +		js_ctx->enabled = true;
> >>> +	}
> >>> +
> >>> +	ret = xa_alloc(&priv->jm_ctxs, &args->handle, jm_ctx,
> >>> +		       XA_LIMIT(0, MAX_JM_CTX_PER_FILE), GFP_KERNEL);
> >>> +	if (ret)
> >>> +		goto err_put_jm_ctx;    
> >>
> >> On error here we just jump down and call panfrost_jm_ctx_put() which
> >> will free jm_ctx but won't destroy any of the drm_sched_entities. There
> >> seems to be something a bit off with the lifetime management here.
> >>
> >> Should panfrost_jm_ctx_release() be responsible for tearing down the
> >> context, and panfrost_jm_ctx_destroy() be nothing more than dropping the
> >> reference?  
> > 
> > The idea was to kill/cancel any pending jobs as soon as userspace
> > releases the context, like we were doing previously when the FD was
> > closed. If we defer this ctx teardown to the release() function, we're
> > basically waiting for all jobs to complete, which:
> > 
> > 1. doesn't encourage userspace to have proper control over the contexts
> >    lifetime
> > 2. might use GPU/mem resources to execute jobs no one cares about
> >    anymore  
> 
> Ah, good point - yes killing the jobs in panfrost_jm_ctx_destroy() makes
> sense. But we still need to ensure the clean-up happens in the other
> paths ;)
> 
> So panfrost_jm_ctx_destroy() should keep the killing jobs part, butthe
> drm scheduler entity cleanup should be moved.

The thing is, we need to call drm_sched_entity_fini() if we want all
pending jobs that were not queued to the HW yet to be cancelled
(_fini() calls _flush() + _kill()). This has to happen before we cancel
the jobs at the JM level, otherwise drm_sched might pass us new jobs
while we're trying to get rid of the currently running ones. Once we've
done that, there's basically nothing left to defer, except the kfree().

Re: [PATCH v2 2/4] drm/panfrost: Introduce JM contexts for manging job resources

[Steven Price]

On 10/09/2025 17:50, Boris Brezillon wrote:
> On Wed, 10 Sep 2025 16:56:43 +0100
> Steven Price <steven.price@arm.com> wrote:
> 
>> On 10/09/2025 16:52, Boris Brezillon wrote:
>>> On Wed, 10 Sep 2025 16:42:32 +0100
>>> Steven Price <steven.price@arm.com> wrote:
>>>   
>>>>> +int panfrost_jm_ctx_create(struct drm_file *file,
>>>>> +			   struct drm_panfrost_jm_ctx_create *args)
>>>>> +{
>>>>> +	struct panfrost_file_priv *priv = file->driver_priv;
>>>>> +	struct panfrost_device *pfdev = priv->pfdev;
>>>>> +	enum drm_sched_priority sched_prio;
>>>>> +	struct panfrost_jm_ctx *jm_ctx;
>>>>> +
>>>>> +	int ret;
>>>>> +
>>>>> +	jm_ctx = kzalloc(sizeof(*jm_ctx), GFP_KERNEL);
>>>>> +	if (!jm_ctx)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	kref_init(&jm_ctx->refcnt);
>>>>> +
>>>>> +	/* Same priority for all JS within a single context */
>>>>> +	jm_ctx->config = JS_CONFIG_THREAD_PRI(args->priority);
>>>>> +
>>>>> +	ret = jm_ctx_prio_to_drm_sched_prio(file, args->priority, &sched_prio);
>>>>> +	if (ret)
>>>>> +		goto err_put_jm_ctx;
>>>>> +
>>>>> +	for (u32 i = 0; i < NUM_JOB_SLOTS - 1; i++) {
>>>>> +		struct drm_gpu_scheduler *sched = &pfdev->js->queue[i].sched;
>>>>> +		struct panfrost_js_ctx *js_ctx = &jm_ctx->slots[i];
>>>>> +
>>>>> +		ret = drm_sched_entity_init(&js_ctx->sched_entity, sched_prio,
>>>>> +					    &sched, 1, NULL);
>>>>> +		if (ret)
>>>>> +			goto err_put_jm_ctx;
>>>>> +
>>>>> +		js_ctx->enabled = true;
>>>>> +	}
>>>>> +
>>>>> +	ret = xa_alloc(&priv->jm_ctxs, &args->handle, jm_ctx,
>>>>> +		       XA_LIMIT(0, MAX_JM_CTX_PER_FILE), GFP_KERNEL);
>>>>> +	if (ret)
>>>>> +		goto err_put_jm_ctx;    
>>>>
>>>> On error here we just jump down and call panfrost_jm_ctx_put() which
>>>> will free jm_ctx but won't destroy any of the drm_sched_entities. There
>>>> seems to be something a bit off with the lifetime management here.
>>>>
>>>> Should panfrost_jm_ctx_release() be responsible for tearing down the
>>>> context, and panfrost_jm_ctx_destroy() be nothing more than dropping the
>>>> reference?  
>>>
>>> The idea was to kill/cancel any pending jobs as soon as userspace
>>> releases the context, like we were doing previously when the FD was
>>> closed. If we defer this ctx teardown to the release() function, we're
>>> basically waiting for all jobs to complete, which:
>>>
>>> 1. doesn't encourage userspace to have proper control over the contexts
>>>    lifetime
>>> 2. might use GPU/mem resources to execute jobs no one cares about
>>>    anymore  
>>
>> Ah, good point - yes killing the jobs in panfrost_jm_ctx_destroy() makes
>> sense. But we still need to ensure the clean-up happens in the other
>> paths ;)
>>
>> So panfrost_jm_ctx_destroy() should keep the killing jobs part, butthe
>> drm scheduler entity cleanup should be moved.
> 
> The thing is, we need to call drm_sched_entity_fini() if we want all
> pending jobs that were not queued to the HW yet to be cancelled
> (_fini() calls _flush() + _kill()). This has to happen before we cancel
> the jobs at the JM level, otherwise drm_sched might pass us new jobs
> while we're trying to get rid of the currently running ones. Once we've
> done that, there's basically nothing left to defer, except the kfree().

Ok, I guess that makes sense.

In which case panfrost_jm_ctx_create() just needs fixing to fully tear
down the context in the event the xa_alloc() fails. Although that makes
me wonder whether the reference counting on the JM context really
achieves anything. Are we ever expecting the context to live past the
panfrost_jm_ctx_destroy() call?

Indeed is it even possible to have any in-flight jobs after
drm_sched_entity_destroy() has returned?

Once all the sched entities have been destroyed there doesn't really
seem to be anything left in struct panfrost_jm_ctx.

Thanks,
Steve

Re: [PATCH v2 2/4] drm/panfrost: Introduce JM contexts for manging job resources

[Adrián Larumbe]

Hi Steven,

On 10.09.2025 16:42, Steven Price wrote:
> On 04/09/2025 01:08, Adrián Larumbe wrote:
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> >
> > A JM context describes GPU HW resourdce allocation for the jobs bound to
> > it. At the time of writing this, it only holds the JS and queue
> > scheduler priorities.
> >
> > When a context is created, all the scheduler entities created for job
> > slots will have the same priority.
> >
> > Until context creation and destruction and attaching a context ID to
> > a job submission are exposed to UM, all jobs shall be bound to the
> > default Panfrost file context, which has medium priority.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_device.h |   4 +-
> >  drivers/gpu/drm/panfrost/panfrost_drv.c    |  19 +-
> >  drivers/gpu/drm/panfrost/panfrost_job.c    | 203 +++++++++++++++++----
> >  drivers/gpu/drm/panfrost/panfrost_job.h    |  25 ++-
> >  4 files changed, 210 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> > index 077525a3ad68..5b164871eb95 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> > +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> > @@ -15,6 +15,7 @@
> >  #include <drm/gpu_scheduler.h>
> >
> >  #include "panfrost_devfreq.h"
> > +#include "panfrost_job.h"
> >
> >  struct panfrost_device;
> >  struct panfrost_mmu;
> > @@ -22,7 +23,6 @@ struct panfrost_job_slot;
> >  struct panfrost_job;
> >  struct panfrost_perfcnt;
> >
> > -#define NUM_JOB_SLOTS 3
> >  #define MAX_PM_DOMAINS 5
> >
> >  enum panfrost_drv_comp_bits {
> > @@ -206,7 +206,7 @@ struct panfrost_engine_usage {
> >  struct panfrost_file_priv {
> >  	struct panfrost_device *pfdev;
> >
> > -	struct drm_sched_entity sched_entity[NUM_JOB_SLOTS];
> > +	struct xarray jm_ctxs;
> >
> >  	struct panfrost_mmu *mmu;
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index 1ea6c509a5d5..398c067457d9 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -279,6 +279,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
> >  	struct panfrost_file_priv *file_priv = file->driver_priv;
> >  	struct drm_panfrost_submit *args = data;
> >  	struct drm_syncobj *sync_out = NULL;
> > +	struct panfrost_jm_ctx *jm_ctx;
> >  	struct panfrost_job *job;
> >  	int ret = 0, slot;
> >
> > @@ -294,10 +295,17 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
> >  			return -ENODEV;
> >  	}
> >
> > +	/* TODO: Use the default JM context until ctx management IOCTLs are exposed */
> > +	jm_ctx = panfrost_jm_ctx_from_handle(file, 0);
> > +	if (!jm_ctx) {
> > +		ret = -EINVAL;
> > +		goto out_put_syncout;
> > +	}
> > +
> >  	job = kzalloc(sizeof(*job), GFP_KERNEL);
> >  	if (!job) {
> >  		ret = -ENOMEM;
> > -		goto out_put_syncout;
> > +		goto out_put_jm_ctx;
> >  	}
> >
> >  	kref_init(&job->refcount);
> > @@ -307,12 +315,13 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
> >  	job->requirements = args->requirements;
> >  	job->flush_id = panfrost_gpu_get_latest_flush_id(pfdev);
> >  	job->mmu = file_priv->mmu;
> > +	job->ctx = panfrost_jm_ctx_get(jm_ctx);
> >  	job->engine_usage = &file_priv->engine_usage;
> >
> >  	slot = panfrost_job_get_slot(job);
> >
> >  	ret = drm_sched_job_init(&job->base,
> > -				 &file_priv->sched_entity[slot],
> > +				 &jm_ctx->slots[slot].sched_entity,
> >  				 1, NULL, file->client_id);
> >  	if (ret)
> >  		goto out_put_job;
> > @@ -338,6 +347,8 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
> >  		drm_sched_job_cleanup(&job->base);
> >  out_put_job:
> >  	panfrost_job_put(job);
> > +out_put_jm_ctx:
> > +	panfrost_jm_ctx_put(jm_ctx);
> >  out_put_syncout:
> >  	if (sync_out)
> >  		drm_syncobj_put(sync_out);
> > @@ -564,7 +575,7 @@ panfrost_open(struct drm_device *dev, struct drm_file *file)
> >  		goto err_free;
> >  	}
> >
> > -	ret = panfrost_job_open(panfrost_priv);
> > +	ret = panfrost_job_open(file);
> >  	if (ret)
> >  		goto err_job;
> >
> > @@ -583,7 +594,7 @@ panfrost_postclose(struct drm_device *dev, struct drm_file *file)
> >  	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
> >
> >  	panfrost_perfcnt_close(file);
> > -	panfrost_job_close(panfrost_priv);
> > +	panfrost_job_close(file);
> >
> >  	panfrost_mmu_ctx_put(panfrost_priv->mmu);
> >  	kfree(panfrost_priv);
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> > index 82acabb21b27..b6853add307c 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/dma-resv.h>
> > +#include <drm/drm_auth.h>
> >  #include <drm/gpu_scheduler.h>
> >  #include <drm/panfrost_drm.h>
> >
> > @@ -22,6 +23,7 @@
> >  #include "panfrost_mmu.h"
> >  #include "panfrost_dump.h"
> >
> > +#define MAX_JM_CTX_PER_FILE 128
>
> 128 seems like a very large number, do we have a reason to set this so high?

Maybe I can drop it down to 64, to make sure it remains a power of two.

>
> >  #define JOB_TIMEOUT_MS 500
> >
> >  #define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
> > @@ -222,7 +224,7 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
> >
> >  	/* start MMU, medium priority, cache clean/flush on end, clean/flush on
> >  	 * start */
> > -	cfg |= JS_CONFIG_THREAD_PRI(8) |
> > +	cfg |= job->ctx->config |
>
> Note that the thread priority on Midgard/Bifrost is pretty pointless. I
> don't believe kbase ever set this to anything other than 8. In theory it
> should balance priority between different slots but "How the priority
> values affect issue rates from different slots is implementation
> dependent" (which I think is wording for 'not implemented'!).
>
> My main concern is that since kbase never used it there's a possibility
> for undiscovered hardware bugs.

In that case, to avoid unexpected behaviour, I'll remove this part in the next revision.

> >  		JS_CONFIG_START_FLUSH_CLEAN_INVALIDATE |
> >  		JS_CONFIG_END_FLUSH_CLEAN_INVALIDATE |
> >  		panfrost_get_job_chain_flag(job);
> > @@ -359,6 +361,7 @@ static void panfrost_job_cleanup(struct kref *ref)
> >  		kvfree(job->bos);
> >  	}
> >
> > +	panfrost_jm_ctx_put(job->ctx);
> >  	kfree(job);
> >  }
> >
> > @@ -917,39 +920,184 @@ void panfrost_job_fini(struct panfrost_device *pfdev)
> >  	destroy_workqueue(pfdev->reset.wq);
> >  }
> >
> > -int panfrost_job_open(struct panfrost_file_priv *panfrost_priv)
> > +int panfrost_job_open(struct drm_file *file)
> > +{
> > +	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
> > +	int ret;
> > +
> > +	struct drm_panfrost_jm_ctx_create default_jm_ctx = {
> > +		.priority = PANFROST_JM_CTX_PRIORITY_MEDIUM,
> > +	};
> > +
> > +	xa_init_flags(&panfrost_priv->jm_ctxs, XA_FLAGS_ALLOC);
> > +
> > +	ret = panfrost_jm_ctx_create(file, &default_jm_ctx);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* We expect the default context to be assigned handle 0. */
> > +	if (WARN_ON(default_jm_ctx.handle))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +void panfrost_job_close(struct drm_file *file)
> > +{
> > +	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
> > +	struct panfrost_jm_ctx *jm_ctx;
> > +	unsigned long i;
> > +
> > +	xa_for_each(&panfrost_priv->jm_ctxs, i, jm_ctx)
> > +		panfrost_jm_ctx_destroy(file, i);
> > +
> > +	xa_destroy(&panfrost_priv->jm_ctxs);
> > +}
> > +
> > +int panfrost_job_is_idle(struct panfrost_device *pfdev)
> >  {
> > -	struct panfrost_device *pfdev = panfrost_priv->pfdev;
> >  	struct panfrost_job_slot *js = pfdev->js;
> > -	struct drm_gpu_scheduler *sched;
> > -	int ret, i;
> > +	int i;
> >
> >  	for (i = 0; i < NUM_JOB_SLOTS; i++) {
> > -		sched = &js->queue[i].sched;
> > -		ret = drm_sched_entity_init(&panfrost_priv->sched_entity[i],
> > -					    DRM_SCHED_PRIORITY_NORMAL, &sched,
> > -					    1, NULL);
> > -		if (WARN_ON(ret))
> > -			return ret;
> > +		/* If there are any jobs in the HW queue, we're not idle */
> > +		if (atomic_read(&js->queue[i].sched.credit_count))
> > +			return false;
> >  	}
> > +
> > +	return true;
> > +}
> > +
> > +static void panfrost_jm_ctx_release(struct kref *kref)
> > +{
> > +	struct panfrost_jm_ctx *jm_ctx = container_of(kref, struct panfrost_jm_ctx, refcnt);
> > +
> > +	kfree(jm_ctx);
> > +}
> > +
> > +void
> > +panfrost_jm_ctx_put(struct panfrost_jm_ctx *jm_ctx)
> > +{
> > +	if (jm_ctx)
> > +		kref_put(&jm_ctx->refcnt, panfrost_jm_ctx_release);
> > +}
> > +
> > +struct panfrost_jm_ctx *
> > +panfrost_jm_ctx_get(struct panfrost_jm_ctx *jm_ctx)
> > +{
> > +	if (jm_ctx)
> > +		kref_get(&jm_ctx->refcnt);
> > +
> > +	return jm_ctx;
> > +}
> > +
> > +struct panfrost_jm_ctx *
> > +panfrost_jm_ctx_from_handle(struct drm_file *file, u32 handle)
> > +{
> > +	struct panfrost_file_priv *priv = file->driver_priv;
> > +	struct panfrost_jm_ctx *jm_ctx;
> > +
> > +	xa_lock(&priv->jm_ctxs);
> > +	jm_ctx = panfrost_jm_ctx_get(xa_load(&priv->jm_ctxs, handle));
> > +	xa_unlock(&priv->jm_ctxs);
> > +
> > +	return jm_ctx;
> > +}
> > +
> > +static int jm_ctx_prio_to_drm_sched_prio(struct drm_file *file,
> > +					 enum drm_panfrost_jm_ctx_priority in,
> > +					 enum drm_sched_priority *out)
> > +{
> > +	switch (in) {
> > +	case PANFROST_JM_CTX_PRIORITY_LOW:
> > +		*out = DRM_SCHED_PRIORITY_LOW;
> > +		return 0;
> > +	case PANFROST_JM_CTX_PRIORITY_MEDIUM:
> > +		*out = DRM_SCHED_PRIORITY_NORMAL;
> > +		return 0;
> > +	case PANFROST_JM_CTX_PRIORITY_HIGH:
> > +		/* Higher priorities require CAP_SYS_NICE or DRM_MASTER */
> > +		if (!capable(CAP_SYS_NICE) && !drm_is_current_master(file))
> > +			return -EACCES;
> > +
> > +		*out = DRM_SCHED_PRIORITY_HIGH;
> > +		return 0;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +int panfrost_jm_ctx_create(struct drm_file *file,
> > +			   struct drm_panfrost_jm_ctx_create *args)
> > +{
> > +	struct panfrost_file_priv *priv = file->driver_priv;
> > +	struct panfrost_device *pfdev = priv->pfdev;
> > +	enum drm_sched_priority sched_prio;
> > +	struct panfrost_jm_ctx *jm_ctx;
> > +
> > +	int ret;
> > +
> > +	jm_ctx = kzalloc(sizeof(*jm_ctx), GFP_KERNEL);
> > +	if (!jm_ctx)
> > +		return -ENOMEM;
> > +
> > +	kref_init(&jm_ctx->refcnt);
> > +
> > +	/* Same priority for all JS within a single context */
> > +	jm_ctx->config = JS_CONFIG_THREAD_PRI(args->priority);
> > +
> > +	ret = jm_ctx_prio_to_drm_sched_prio(file, args->priority, &sched_prio);
> > +	if (ret)
> > +		goto err_put_jm_ctx;
> > +
> > +	for (u32 i = 0; i < NUM_JOB_SLOTS - 1; i++) {
> > +		struct drm_gpu_scheduler *sched = &pfdev->js->queue[i].sched;
> > +		struct panfrost_js_ctx *js_ctx = &jm_ctx->slots[i];
> > +
> > +		ret = drm_sched_entity_init(&js_ctx->sched_entity, sched_prio,
> > +					    &sched, 1, NULL);
> > +		if (ret)
> > +			goto err_put_jm_ctx;
> > +
> > +		js_ctx->enabled = true;
> > +	}
> > +
> > +	ret = xa_alloc(&priv->jm_ctxs, &args->handle, jm_ctx,
> > +		       XA_LIMIT(0, MAX_JM_CTX_PER_FILE), GFP_KERNEL);
> > +	if (ret)
> > +		goto err_put_jm_ctx;
>
> On error here we just jump down and call panfrost_jm_ctx_put() which
> will free jm_ctx but won't destroy any of the drm_sched_entities. There
> seems to be something a bit off with the lifetime management here.
>
> Should panfrost_jm_ctx_release() be responsible for tearing down the
> context, and panfrost_jm_ctx_destroy() be nothing more than dropping the
> reference?
>
> Steve
>
> > +
> >  	return 0;
> > +
> > +err_put_jm_ctx:
> > +	panfrost_jm_ctx_put(jm_ctx);
> > +	return ret;
> >  }
> >
> > -void panfrost_job_close(struct panfrost_file_priv *panfrost_priv)
> > +int panfrost_jm_ctx_destroy(struct drm_file *file, u32 handle)
> >  {
> > -	struct panfrost_device *pfdev = panfrost_priv->pfdev;
> > -	int i;
> > +	struct panfrost_file_priv *priv = file->driver_priv;
> > +	struct panfrost_device *pfdev = priv->pfdev;
> > +	struct panfrost_jm_ctx *jm_ctx;
> >
> > -	for (i = 0; i < NUM_JOB_SLOTS; i++)
> > -		drm_sched_entity_destroy(&panfrost_priv->sched_entity[i]);
> > +	jm_ctx = xa_erase(&priv->jm_ctxs, handle);
> > +	if (!jm_ctx)
> > +		return -EINVAL;
> > +
> > +	for (u32 i = 0; i < ARRAY_SIZE(jm_ctx->slots); i++) {
> > +		if (jm_ctx->slots[i].enabled)
> > +			drm_sched_entity_destroy(&jm_ctx->slots[i].sched_entity);
> > +	}
> >
> >  	/* Kill in-flight jobs */
> >  	spin_lock(&pfdev->js->job_lock);
> > -	for (i = 0; i < NUM_JOB_SLOTS; i++) {
> > -		struct drm_sched_entity *entity = &panfrost_priv->sched_entity[i];
> > -		int j;
> > +	for (u32 i = 0; i < ARRAY_SIZE(jm_ctx->slots); i++) {
> > +		struct drm_sched_entity *entity = &jm_ctx->slots[i].sched_entity;
> > +
> > +		if (!jm_ctx->slots[i].enabled)
> > +			continue;
> >
> > -		for (j = ARRAY_SIZE(pfdev->jobs[0]) - 1; j >= 0; j--) {
> > +		for (int j = ARRAY_SIZE(pfdev->jobs[0]) - 1; j >= 0; j--) {
> >  			struct panfrost_job *job = pfdev->jobs[i][j];
> >  			u32 cmd;
> >
> > @@ -980,18 +1128,7 @@ void panfrost_job_close(struct panfrost_file_priv *panfrost_priv)
> >  		}
> >  	}
> >  	spin_unlock(&pfdev->js->job_lock);
> > -}
> >
> > -int panfrost_job_is_idle(struct panfrost_device *pfdev)
> > -{
> > -	struct panfrost_job_slot *js = pfdev->js;
> > -	int i;
> > -
> > -	for (i = 0; i < NUM_JOB_SLOTS; i++) {
> > -		/* If there are any jobs in the HW queue, we're not idle */
> > -		if (atomic_read(&js->queue[i].sched.credit_count))
> > -			return false;
> > -	}
> > -
> > -	return true;
> > +	panfrost_jm_ctx_put(jm_ctx);
> > +	return 0;
> >  }
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
> > index ec581b97852b..f3e5010e6cf5 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_job.h
> > +++ b/drivers/gpu/drm/panfrost/panfrost_job.h
> > @@ -18,6 +18,7 @@ struct panfrost_job {
> >
> >  	struct panfrost_device *pfdev;
> >  	struct panfrost_mmu *mmu;
> > +	struct panfrost_jm_ctx *ctx;
> >
> >  	/* Fence to be signaled by IRQ handler when the job is complete. */
> >  	struct dma_fence *done_fence;
> > @@ -39,10 +40,30 @@ struct panfrost_job {
> >  	u64 start_cycles;
> >  };
> >
> > +struct panfrost_js_ctx {
> > +	struct drm_sched_entity sched_entity;
> > +	bool enabled;
> > +};
> > +
> > +#define NUM_JOB_SLOTS 3
> > +
> > +struct panfrost_jm_ctx {
> > +	struct kref refcnt;
> > +	u32 config;
> > +	struct panfrost_js_ctx slots[NUM_JOB_SLOTS];
> > +};
> > +
> > +int panfrost_jm_ctx_create(struct drm_file *file,
> > +			   struct drm_panfrost_jm_ctx_create *args);
> > +int panfrost_jm_ctx_destroy(struct drm_file *file, u32 handle);
> > +void panfrost_jm_ctx_put(struct panfrost_jm_ctx *jm_ctx);
> > +struct panfrost_jm_ctx *panfrost_jm_ctx_get(struct panfrost_jm_ctx *jm_ctx);
> > +struct panfrost_jm_ctx *panfrost_jm_ctx_from_handle(struct drm_file *file, u32 handle);
> > +
> >  int panfrost_job_init(struct panfrost_device *pfdev);
> >  void panfrost_job_fini(struct panfrost_device *pfdev);
> > -int panfrost_job_open(struct panfrost_file_priv *panfrost_priv);
> > -void panfrost_job_close(struct panfrost_file_priv *panfrost_priv);
> > +int panfrost_job_open(struct drm_file *file);
> > +void panfrost_job_close(struct drm_file *file);
> >  int panfrost_job_get_slot(struct panfrost_job *job);
> >  int panfrost_job_push(struct panfrost_job *job);
> >  void panfrost_job_put(struct panfrost_job *job);


Adrian Larumbe

Re: [PATCH v2 4/4] drm/panfrost: Display list of device JM contexts over debugfs

[Adrián Larumbe]

On 10.09.2025 16:42, Steven Price wrote:
> On 04/09/2025 01:08, Adrián Larumbe wrote:
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> >
> > For DebugFS builds, create a filesystem knob that, for every single open
> > file of the Panfrost DRM device, shows its command name information and
> > PID (when applicable), and all of its existing JM contexts.
> >
> > For every context, show its priority and job config.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_drv.c | 97 +++++++++++++++++++++++++
> >  1 file changed, 97 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index 02f704ec4961..b3d14b887da4 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -712,6 +712,48 @@ static int panthor_gems_show(struct seq_file *m, void *data)
> >  	return 0;
> >  }
> >
> > +static void show_panfrost_jm_ctx(struct panfrost_jm_ctx *jm_ctx, u32 handle,
> > +				 struct seq_file *m)
> > +{
> > +	struct drm_device *ddev = ((struct drm_info_node *)m->private)->minor->dev;
> > +	const char *prio = NULL;
> > +
> > +	static const char * const prios[] = {
> > +		[DRM_SCHED_PRIORITY_HIGH] = "HIGH",
> > +		[DRM_SCHED_PRIORITY_NORMAL] = "NORMAL",
> > +		[DRM_SCHED_PRIORITY_LOW] = "LOW",
> > +	};
> > +
> > +	if (jm_ctx->slots[0].sched_entity.priority !=
> > +	    jm_ctx->slots[1].sched_entity.priority)
> > +		drm_warn(ddev, "Slot priorities should be the same in a single context");
> > +
> > +	if (jm_ctx->slots[0].sched_entity.priority < ARRAY_SIZE(prios))
> > +		prio = prios[jm_ctx->slots[0].sched_entity.priority];
> > +
> > +	seq_printf(m, " JM context %u: priority %s config %x\n",
> > +		   handle, prio ? prio : "UNKNOWN", jm_ctx->config);
>
> NIT: If you assign prio to "UNKNOWN" to begin with (rather than NULL)
> you can avoid this ?: operator.

Acked.

> > +}
> > +
> > +static int show_file_jm_ctxs(struct panfrost_file_priv *pfile,
> > +			     struct seq_file *m)
> > +{
> > +	struct panfrost_jm_ctx *jm_ctx;
> > +	unsigned long i;
> > +
> > +	xa_lock(&pfile->jm_ctxs);
> > +	xa_for_each(&pfile->jm_ctxs, i, jm_ctx) {
> > +		jm_ctx = panfrost_jm_ctx_get(jm_ctx);
> > +		xa_unlock(&pfile->jm_ctxs);
> > +		show_panfrost_jm_ctx(jm_ctx, i, m);
> > +		panfrost_jm_ctx_put(jm_ctx);
> > +		xa_lock(&pfile->jm_ctxs);
> > +	}
> > +	xa_unlock(&pfile->jm_ctxs);
>
> Is it so bad if we just held the xa lock for the whole loop? It just
> seems unnecessarily complex.

xa_unlock() is defined as a spinlock which are fast. I'm often of the view that the
critical region should be as narrow as possible, especially when debug code is clashing
with the normal operation of the driver.

> Thanks,
> Steve
>
> > +
> > +	return 0;
> > +}
> > +
> >  static struct drm_info_list panthor_debugfs_list[] = {
> >  	{"gems", panthor_gems_show, 0, NULL},
> >  };
> > @@ -725,9 +767,64 @@ static int panthor_gems_debugfs_init(struct drm_minor *minor)
> >  	return 0;
> >  }
> >
> > +static int show_each_file(struct seq_file *m, void *arg)
> > +{
> > +	struct drm_info_node *node = (struct drm_info_node *)m->private;
> > +	struct drm_device *ddev = node->minor->dev;
> > +	int (*show)(struct panfrost_file_priv *, struct seq_file *) =
> > +		node->info_ent->data;
> > +	struct drm_file *file;
> > +	int ret;
> > +
> > +	ret = mutex_lock_interruptible(&ddev->filelist_mutex);
> > +	if (ret)
> > +		return ret;
> > +
> > +	list_for_each_entry(file, &ddev->filelist, lhead) {
> > +		struct task_struct *task;
> > +		struct panfrost_file_priv *pfile = file->driver_priv;
> > +		struct pid *pid;
> > +
> > +		/*
> > +		 * Although we have a valid reference on file->pid, that does
> > +		 * not guarantee that the task_struct who called get_pid() is
> > +		 * still alive (e.g. get_pid(current) => fork() => exit()).
> > +		 * Therefore, we need to protect this ->comm access using RCU.
> > +		 */
> > +		rcu_read_lock();
> > +		pid = rcu_dereference(file->pid);
> > +		task = pid_task(pid, PIDTYPE_TGID);
> > +		seq_printf(m, "client_id %8llu pid %8d command %s:\n",
> > +			   file->client_id, pid_nr(pid),
> > +			   task ? task->comm : "<unknown>");
> > +		rcu_read_unlock();
> > +
> > +		ret = show(pfile, m);
> > +		if (ret < 0)
> > +			break;
> > +
> > +		seq_puts(m, "\n");
> > +	}
> > +
> > +	mutex_unlock(&ddev->filelist_mutex);
> > +	return ret;
> > +}
> > +
> > +static struct drm_info_list panfrost_sched_debugfs_list[] = {
> > +	{ "sched_ctxs", show_each_file, 0, show_file_jm_ctxs },
> > +};
> > +
> > +static void panfrost_sched_debugfs_init(struct drm_minor *minor)
> > +{
> > +	drm_debugfs_create_files(panfrost_sched_debugfs_list,
> > +				 ARRAY_SIZE(panfrost_sched_debugfs_list),
> > +				 minor->debugfs_root, minor);
> > +}
> > +
> >  static void panfrost_debugfs_init(struct drm_minor *minor)
> >  {
> >  	panthor_gems_debugfs_init(minor);
> > +	panfrost_sched_debugfs_init(minor);
> >  }
> >  #endif
> >

Adrian Larumbe

Re: [PATCH v2 3/4] drm/panfrost: Expose JM context IOCTLs to UM

[Adrián Larumbe]

On 10.09.2025 16:42, Steven Price wrote:
> On 04/09/2025 01:08, Adrián Larumbe wrote:
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> >
> > Minor revision of the driver must be bumped because this expands the
> > uAPI. On top of that, let user know the available priorities so that
> > they can create contexts with legal priority values.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_drv.c | 35 +++++++++++++++++++++++--
> >  1 file changed, 33 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index 398c067457d9..02f704ec4961 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> >  #include <drm/panfrost_drm.h>
> > +#include <drm/drm_auth.h>
> >  #include <drm/drm_debugfs.h>
> >  #include <drm/drm_drv.h>
> >  #include <drm/drm_ioctl.h>
> > @@ -109,6 +110,15 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
> >  #endif
> >  		break;
> >
> > +	case DRM_PANFROST_PARAM_ALLOWED_JM_CTX_PRIORITIES:
> > +		param->value = BIT(PANFROST_JM_CTX_PRIORITY_LOW) |
> > +			       BIT(PANFROST_JM_CTX_PRIORITY_MEDIUM);
> > +
> > +		/* High prio require CAP_SYS_NICE or DRM_MASTER */
> > +		if (capable(CAP_SYS_NICE) || drm_is_current_master(file))
>
> NIT: This is repeating the check in jm_ctx_prio_to_drm_sched_prio(). It
> would be nice to have this check in one place to ensure that the two
> cannot get out of sync.

I noticed some other drivers are performing the same check, e.g.

- amdgpu: in amdgpu_ctx_priority_permit()
- imagination: remap_priority
- panthor ...

Which makes me wonder, maybe adding a helper to drm_auth.c would be good in this case?
Anyway, that perhaps should be matter for a different patch .


> > +			param->value |= BIT(PANFROST_JM_CTX_PRIORITY_HIGH);
> > +		break;
> > +
> >  	default:
> >  		return -EINVAL;
> >  	}
> > @@ -295,8 +305,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
> >  			return -ENODEV;
> >  	}
> >
> > -	/* TODO: Use the default JM context until ctx management IOCTLs are exposed */
> > -	jm_ctx = panfrost_jm_ctx_from_handle(file, 0);
> > +	jm_ctx = panfrost_jm_ctx_from_handle(file, args->jm_ctx_handle);
>
> I'm not sure if this should go in this patch or the previous one, but:
> we need to add a check that the padding is 0. Personally I'd be tempted
> to put it in the previous patch and enforce that jm_ctx_handle is zeroed
> too (and drop that check here when you also remove the TODO).

Acked.

> >  	if (!jm_ctx) {
> >  		ret = -EINVAL;
> >  		goto out_put_syncout;
> > @@ -547,6 +556,24 @@ static int panfrost_ioctl_set_label_bo(struct drm_device *ddev, void *data,
> >  	return ret;
> >  }
> >
> > +static int panfrost_ioctl_jm_ctx_create(struct drm_device *dev, void *data,
> > +					struct drm_file *file)
> > +{
> > +	return panfrost_jm_ctx_create(file, data);
> > +}
> > +
> > +static int panfrost_ioctl_jm_ctx_destroy(struct drm_device *dev, void *data,
> > +					 struct drm_file *file)
> > +{
> > +	const struct drm_panfrost_jm_ctx_destroy *args = data;
> > +
> > +	/* We can't destroy the default context created when the file is opened. */
> > +	if (!args->handle)
> > +		return -EINVAL;
>
> Also need a check for padding being non-zero.
>
> Thanks,
> Steve
>
> > +
> > +	return panfrost_jm_ctx_destroy(file, args->handle);
> > +}
> > +
> >  int panfrost_unstable_ioctl_check(void)
> >  {
> >  	if (!unstable_ioctls)
> > @@ -614,6 +641,8 @@ static const struct drm_ioctl_desc panfrost_drm_driver_ioctls[] = {
> >  	PANFROST_IOCTL(PERFCNT_DUMP,	perfcnt_dump,	DRM_RENDER_ALLOW),
> >  	PANFROST_IOCTL(MADVISE,		madvise,	DRM_RENDER_ALLOW),
> >  	PANFROST_IOCTL(SET_LABEL_BO,	set_label_bo,	DRM_RENDER_ALLOW),
> > +	PANFROST_IOCTL(JM_CTX_CREATE,	jm_ctx_create,	DRM_RENDER_ALLOW),
> > +	PANFROST_IOCTL(JM_CTX_DESTROY,	jm_ctx_destroy,	DRM_RENDER_ALLOW),
> >  };
> >
> >  static void panfrost_gpu_show_fdinfo(struct panfrost_device *pfdev,
> > @@ -710,6 +739,8 @@ static void panfrost_debugfs_init(struct drm_minor *minor)
> >   * - 1.3 - adds JD_REQ_CYCLE_COUNT job requirement for SUBMIT
> >   *       - adds SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY queries
> >   * - 1.4 - adds SET_LABEL_BO
> > + * - 1.5 - adds JM_CTX_{CREATE,DESTROY} ioctls and extend SUBMIT to allow
> > + *	   context creation with configurable priorities/affinity
> >   */
> >  static const struct drm_driver panfrost_drm_driver = {
> >  	.driver_features	= DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ,

Adrian Larumbe

Re: [PATCH v2 2/4] drm/panfrost: Introduce JM contexts for manging job resources

[Adrián Larumbe]

On 11.09.2025 10:30, Steven Price wrote:
> On 10/09/2025 17:50, Boris Brezillon wrote:
> > On Wed, 10 Sep 2025 16:56:43 +0100
> > Steven Price <steven.price@arm.com> wrote:
> >
> >> On 10/09/2025 16:52, Boris Brezillon wrote:
> >>> On Wed, 10 Sep 2025 16:42:32 +0100
> >>> Steven Price <steven.price@arm.com> wrote:
> >>>
> >>>>> +int panfrost_jm_ctx_create(struct drm_file *file,
> >>>>> +			   struct drm_panfrost_jm_ctx_create *args)
> >>>>> +{
> >>>>> +	struct panfrost_file_priv *priv = file->driver_priv;
> >>>>> +	struct panfrost_device *pfdev = priv->pfdev;
> >>>>> +	enum drm_sched_priority sched_prio;
> >>>>> +	struct panfrost_jm_ctx *jm_ctx;
> >>>>> +
> >>>>> +	int ret;
> >>>>> +
> >>>>> +	jm_ctx = kzalloc(sizeof(*jm_ctx), GFP_KERNEL);
> >>>>> +	if (!jm_ctx)
> >>>>> +		return -ENOMEM;
> >>>>> +
> >>>>> +	kref_init(&jm_ctx->refcnt);
> >>>>> +
> >>>>> +	/* Same priority for all JS within a single context */
> >>>>> +	jm_ctx->config = JS_CONFIG_THREAD_PRI(args->priority);
> >>>>> +
> >>>>> +	ret = jm_ctx_prio_to_drm_sched_prio(file, args->priority, &sched_prio);
> >>>>> +	if (ret)
> >>>>> +		goto err_put_jm_ctx;
> >>>>> +
> >>>>> +	for (u32 i = 0; i < NUM_JOB_SLOTS - 1; i++) {
> >>>>> +		struct drm_gpu_scheduler *sched = &pfdev->js->queue[i].sched;
> >>>>> +		struct panfrost_js_ctx *js_ctx = &jm_ctx->slots[i];
> >>>>> +
> >>>>> +		ret = drm_sched_entity_init(&js_ctx->sched_entity, sched_prio,
> >>>>> +					    &sched, 1, NULL);
> >>>>> +		if (ret)
> >>>>> +			goto err_put_jm_ctx;
> >>>>> +
> >>>>> +		js_ctx->enabled = true;
> >>>>> +	}
> >>>>> +
> >>>>> +	ret = xa_alloc(&priv->jm_ctxs, &args->handle, jm_ctx,
> >>>>> +		       XA_LIMIT(0, MAX_JM_CTX_PER_FILE), GFP_KERNEL);
> >>>>> +	if (ret)
> >>>>> +		goto err_put_jm_ctx;
> >>>>
> >>>> On error here we just jump down and call panfrost_jm_ctx_put() which
> >>>> will free jm_ctx but won't destroy any of the drm_sched_entities. There
> >>>> seems to be something a bit off with the lifetime management here.
> >>>>
> >>>> Should panfrost_jm_ctx_release() be responsible for tearing down the
> >>>> context, and panfrost_jm_ctx_destroy() be nothing more than dropping the
> >>>> reference?
> >>>
> >>> The idea was to kill/cancel any pending jobs as soon as userspace
> >>> releases the context, like we were doing previously when the FD was
> >>> closed. If we defer this ctx teardown to the release() function, we're
> >>> basically waiting for all jobs to complete, which:
> >>>
> >>> 1. doesn't encourage userspace to have proper control over the contexts
> >>>    lifetime
> >>> 2. might use GPU/mem resources to execute jobs no one cares about
> >>>    anymore
> >>
> >> Ah, good point - yes killing the jobs in panfrost_jm_ctx_destroy() makes
> >> sense. But we still need to ensure the clean-up happens in the other
> >> paths ;)
> >>
> >> So panfrost_jm_ctx_destroy() should keep the killing jobs part, butthe
> >> drm scheduler entity cleanup should be moved.
> >
> > The thing is, we need to call drm_sched_entity_fini() if we want all
> > pending jobs that were not queued to the HW yet to be cancelled
> > (_fini() calls _flush() + _kill()). This has to happen before we cancel
> > the jobs at the JM level, otherwise drm_sched might pass us new jobs
> > while we're trying to get rid of the currently running ones. Once we've
> > done that, there's basically nothing left to defer, except the kfree().
>
> Ok, I guess that makes sense.
>
> In which case panfrost_jm_ctx_create() just needs fixing to fully tear
> down the context in the event the xa_alloc() fails. Although that makes
> me wonder whether the reference counting on the JM context really
> achieves anything. Are we ever expecting the context to live past the
> panfrost_jm_ctx_destroy() call?

We still need reference counting, otherwise there would be a racy window between
the submission and context destruction ioctls, in which a context that has just
been released is still owned by a newly created job leading to UAF.

> Indeed is it even possible to have any in-flight jobs after
> drm_sched_entity_destroy() has returned?

My understanding of drm_sched_entity_destroy() is that, after it returns,
no jobs can be in-flight any more and the entity is rendered unusable by
any new jobs. This can lead to the unpleasant situation in which a thread
tries to submit a new job and gets a context reference right before another
thread takes precedence and destroys it, causing the scheduler entities to
be unusable.

Then drm_sched_job_init() would contain a reference to an invalid entity,
which further down the line would cause drm_sched_entity_push_job() to report
a DRM_ERROR warning that te entity is stopped, which should never happen,
because drm_sched_entity_push_job() must always suceed.

> Once all the sched entities have been destroyed there doesn't really
> seem to be anything left in struct panfrost_jm_ctx.

We've thought of a new approach whereby a context would be flagged as destroyed
inside panfrost_jm_ctx_destroy(), destruction of scheduler entities done at context
release time and then cancelling new jobs that had been queued after context
destruction in the .run_job scheduler function if they notice the context
is so flagged.

> Thanks,
> Steve

Cheers,
Adrian Larumbe