Re: [PATCH 8/9] accel/rocket: Add job submission IOCTL
From: Jeffrey Hugo
Date: Fri Jun 14 2024 - 12:39:35 EST
On 6/12/2024 7:53 AM, Tomeu Vizoso wrote:
Using the DRM GPU scheduler infrastructure, with a scheduler for each
core.
Userspace can decide for a series of tasks to be executed sequentially
in the same core, so SRAM locality can be taken advantage of.
The job submission code was intially based on Panfrost.
intially -> initially
Signed-off-by: Tomeu Vizoso <tomeu@xxxxxxxxxxxxxxx>
---
drivers/accel/rocket/Makefile | 3 +-
drivers/accel/rocket/rocket_core.c | 6 +
drivers/accel/rocket/rocket_core.h | 16 +
drivers/accel/rocket/rocket_device.c | 2 +
drivers/accel/rocket/rocket_device.h | 2 +
drivers/accel/rocket/rocket_drv.c | 15 +
drivers/accel/rocket/rocket_drv.h | 3 +
drivers/accel/rocket/rocket_job.c | 708 +++++++++++++++++++++++++++++++++++
drivers/accel/rocket/rocket_job.h | 49 +++
include/uapi/drm/rocket_accel.h | 55 +++
10 files changed, 858 insertions(+), 1 deletion(-)
diff --git a/drivers/accel/rocket/Makefile b/drivers/accel/rocket/Makefile
index 875cac2243d9..4d59036af8d9 100644
--- a/drivers/accel/rocket/Makefile
+++ b/drivers/accel/rocket/Makefile
@@ -6,4 +6,5 @@ rocket-y := \
rocket_core.o \
rocket_device.o \
rocket_drv.o \
- rocket_gem.o
+ rocket_gem.o \
+ rocket_job.o
diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
index d6680b00fb2f..2b2d8be38f0a 100644
--- a/drivers/accel/rocket/rocket_core.c
+++ b/drivers/accel/rocket/rocket_core.c
@@ -11,6 +11,7 @@
#include "rocket_core.h"
#include "rocket_device.h"
+#include "rocket_job.h"
#include "rocket_registers.h"
static int rocket_clk_init(struct rocket_core *core)
@@ -122,6 +123,10 @@ int rocket_core_init(struct rocket_core *core)
goto out_pm_domain;
}
+ err = rocket_job_init(core);
+ if (err)
+ goto out_pm_domain;
+
version = rocket_read(core, REG_PC_VERSION) + (rocket_read(core, REG_PC_VERSION_NUM) & 0xffff);
dev_info(rdev->dev, "Rockchip NPU core %d version: %d\n", core->index, version);
@@ -134,6 +139,7 @@ int rocket_core_init(struct rocket_core *core)
void rocket_core_fini(struct rocket_core *core)
{
+ rocket_job_fini(core);
rocket_pmdomain_fini(core);
}
diff --git a/drivers/accel/rocket/rocket_core.h b/drivers/accel/rocket/rocket_core.h
index e5d4c848c9f4..e6401960a9b2 100644
--- a/drivers/accel/rocket/rocket_core.h
+++ b/drivers/accel/rocket/rocket_core.h
@@ -8,6 +8,8 @@
#include <asm/io.h>
#include <asm-generic/io.h>
+#include <drm/gpu_scheduler.h>
What about includes for workqueue or atomic?
+static struct dma_fence *rocket_job_run(struct drm_sched_job *sched_job)
+{
+ struct rocket_job *job = to_rocket_job(sched_job);
+ struct rocket_device *rdev = job->rdev;
+ struct rocket_core *core = sched_to_core(rdev, sched_job->sched);
+ struct dma_fence *fence = NULL;
+ int ret;
+
+ if (unlikely(job->base.s_fence->finished.error))
+ return NULL;
+
+ /* Nothing to execute: can happen if the job has finished while
+ * we were resetting the GPU.
+ */
Not the correct comment style
+ if (job->next_task_idx == job->task_count)
+ return NULL;
+
+ fence = rocket_fence_create(core);
+ if (IS_ERR(fence))
+ return fence;
+
+ if (job->done_fence)
+ dma_fence_put(job->done_fence);
+ job->done_fence = dma_fence_get(fence);
+
+ ret = pm_runtime_get_sync(rdev->dev);
+ if (ret < 0)
+ return fence;
+
+ spin_lock(&core->job_lock);
+
+ core->in_flight_job = job;
+ rocket_job_hw_submit(core, job);
+
+ spin_unlock(&core->job_lock);
+
+ return fence;
+}
+
+static void rocket_job_handle_done(struct rocket_core *core,
+ struct rocket_job *job)
+{
+ if (job->next_task_idx < job->task_count) {
+ rocket_job_hw_submit(core, job);
+ return;
+ }
+
+ core->in_flight_job = NULL;
+ dma_fence_signal_locked(job->done_fence);
+ pm_runtime_put_autosuspend(core->dev->dev);
+}
+
+static void rocket_job_handle_irq(struct rocket_core *core)
+{
+ uint32_t status, raw_status;
+
+ pm_runtime_mark_last_busy(core->dev->dev);
+
+ status = rocket_read(core, REG_PC_INTERRUPT_STATUS);
+ raw_status = rocket_read(core, REG_PC_INTERRUPT_RAW_STATUS);
+
+ rocket_write(core, REG_PC_OPERATION_ENABLE, 0x0);
+ rocket_write(core, REG_PC_INTERRUPT_CLEAR, 0x1ffff);
+
+ spin_lock(&core->job_lock);
+
+ if (core->in_flight_job)
+ rocket_job_handle_done(core, core->in_flight_job);
+
+ spin_unlock(&core->job_lock);
+}
+
+static void
+rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
+{
+ struct rocket_device *rdev = core->dev;
+ bool cookie;
+
+ if (!atomic_read(&core->reset.pending))
+ return;
+
+ /* Stop the scheduler.
Not the correct comment style
+ *
+ * FIXME: We temporarily get out of the dma_fence_signalling section
+ * because the cleanup path generate lockdep splats when taking locks
+ * to release job resources. We should rework the code to follow this
+ * pattern:
+ *
+ * try_lock
+ * if (locked)
+ * release
+ * else
+ * schedule_work_to_release_later
+ */
+ drm_sched_stop(&core->sched, bad);
+
+ cookie = dma_fence_begin_signalling();
+
+ if (bad)
+ drm_sched_increase_karma(bad);
+
+ /* Mask job interrupts and synchronize to make sure we won't be
+ * interrupted during our reset.
+ */
Not the correct comment style, again. This is the last time I'm going
to mention it.
diff --git a/drivers/accel/rocket/rocket_job.h b/drivers/accel/rocket/rocket_job.h
new file mode 100644
index 000000000000..0c3c90e47d39
--- /dev/null
+++ b/drivers/accel/rocket/rocket_job.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright 2024 Tomeu Vizoso <tomeu@xxxxxxxxxxxxxxx> */
+
+#ifndef __ROCKET_JOB_H__
+#define __ROCKET_JOB_H__
+
+#include <drm/gpu_scheduler.h>
+#include <drm/drm_drv.h>
Alphabetical order
diff --git a/include/uapi/drm/rocket_accel.h b/include/uapi/drm/rocket_accel.h
index 8338726a83c3..888c9413e4cd 100644
--- a/include/uapi/drm/rocket_accel.h
+++ b/include/uapi/drm/rocket_accel.h
@@ -12,8 +12,10 @@ extern "C" {
#endif
#define DRM_ROCKET_CREATE_BO 0x00
+#define DRM_ROCKET_SUBMIT 0x01
#define DRM_IOCTL_ROCKET_CREATE_BO DRM_IOWR(DRM_COMMAND_BASE + DRM_ROCKET_CREATE_BO, struct drm_rocket_create_bo)
+#define DRM_IOCTL_ROCKET_SUBMIT DRM_IOW(DRM_COMMAND_BASE + DRM_ROCKET_SUBMIT, struct drm_rocket_submit)
/**
* struct drm_rocket_create_bo - ioctl argument for creating Rocket BOs.
@@ -36,6 +38,59 @@ struct drm_rocket_create_bo {
__u64 offset;
};
+/**
+ * struct drm_rocket_task - A task to be run on the NPU
+ *
+ * A task is the smallest unit of work that can be run on the NPU.
+ */
+struct drm_rocket_task {
+ /** DMA address to NPU mapping of register command buffer */
+ __u64 regcmd;
+
+ /** Number of commands in the register command buffer */
+ __u32 regcmd_count;
+};
+
+/**
+ * struct drm_rocket_job - A job to be run on the NPU
+ *
+ * The kernel will schedule the execution of this job taking into account its
+ * dependencies with other jobs. All tasks in the same job will be executed
+ * sequentially on the same core, to benefit from memory residency in SRAM.
+ */
+struct drm_rocket_job {
+ /** Pointer to an array of struct drm_rocket_task. */
+ __u64 tasks;
+
+ /** Number of tasks passed in. */
+ __u32 task_count;
+
+ /** Pointer to a u32 array of the BOs that are read by the job. */
+ __u64 in_bo_handles;
+
+ /** Number of input BO handles passed in (size is that times 4). */
+ __u32 in_bo_handle_count;
+
+ /** Pointer to a u32 array of the BOs that are written to by the job. */
+ __u64 out_bo_handles;
+
+ /** Number of output BO handles passed in (size is that times 4). */
+ __u32 out_bo_handle_count;
+};
I feels like the mixing of 32-bit and 64-bit fields violates the
guidelines on defining ioctls due to implicit padding that might or
might not be present.
+
+/**
+ * struct drm_rocket_submit - ioctl argument for submitting commands to the NPU.
+ *
+ * The kernel will schedule the execution of these jobs in dependency order.
+ */
+struct drm_rocket_submit {
+ /** Pointer to an array of struct drm_rocket_job. */
+ __u64 jobs;
+
+ /** Number of jobs passed in. */
+ __u32 job_count;
+};
+
#if defined(__cplusplus)
}
#endif