Hi Philipp,
On 22/01/25 11:08, Philipp Stanner wrote:
drm_sched_init() has a great many parameters and upcoming new
functionality for the scheduler might add even more. Generally, the
great number of parameters reduces readability and has already caused
one missnaming in:
commit 6f1cacf4eba7 ("drm/nouveau: Improve variable name in nouveau_sched_init()").
Introduce a new struct for the scheduler init parameters and port all
users.
Signed-off-by: Philipp Stanner <phasta@xxxxxxxxxx>
---
Howdy,
I have a patch-series in the pipe that will add a `flags` argument to
drm_sched_init(). I thought it would be wise to first rework the API as
detailed in this patch. It's really a lot of parameters by now, and I
would expect that it might get more and more over the years for special
use cases etc.
Regards,
P.
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 21 +++-
drivers/gpu/drm/etnaviv/etnaviv_sched.c | 20 ++-
drivers/gpu/drm/imagination/pvr_queue.c | 21 +++-
drivers/gpu/drm/lima/lima_sched.c | 21 +++-
drivers/gpu/drm/msm/msm_ringbuffer.c | 22 ++--
drivers/gpu/drm/nouveau/nouveau_sched.c | 20 ++-
drivers/gpu/drm/panfrost/panfrost_job.c | 22 ++--
drivers/gpu/drm/panthor/panthor_mmu.c | 18 ++-
drivers/gpu/drm/panthor/panthor_sched.c | 23 ++--
drivers/gpu/drm/scheduler/sched_main.c | 53 +++-----
drivers/gpu/drm/v3d/v3d_sched.c | 135 +++++++++++++++------
drivers/gpu/drm/xe/xe_execlist.c | 20 ++-
drivers/gpu/drm/xe/xe_gpu_scheduler.c | 19 ++-
include/drm/gpu_scheduler.h | 35 +++++-
14 files changed, 311 insertions(+), 139 deletions(-)
[...]
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 99ac4995b5a1..716e6d074d87 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -814,67 +814,124 @@ static const struct drm_sched_backend_ops v3d_cpu_sched_ops = {
.free_job = v3d_cpu_job_free
};
+/*
+ * v3d's scheduler instances are all identical, except for ops and name.
+ */
+static void
+v3d_common_sched_init(struct drm_sched_init_params *params, struct device *dev)
+{
+ memset(params, 0, sizeof(struct drm_sched_init_params));
+
+ params->submit_wq = NULL; /* Use the system_wq. */
+ params->num_rqs = DRM_SCHED_PRIORITY_COUNT;
+ params->credit_limit = 1;
+ params->hang_limit = 0;
+ params->timeout = msecs_to_jiffies(500);
+ params->timeout_wq = NULL; /* Use the system_wq. */
+ params->score = NULL;
+ params->dev = dev;
+}
Could we use only one function that takes struct v3d_dev *v3d, enum
v3d_queue, and sched_ops as arguments (instead of one function per
queue)? You can get the name of the scheduler by concatenating "v3d_" to
the return of v3d_queue_to_string().
I believe it would make the code much simpler.
Best Regards,
- Maíra
+
+static int
+v3d_bin_sched_init(struct v3d_dev *v3d)
+{
+ struct drm_sched_init_params params;
+
+ v3d_common_sched_init(¶ms, v3d->drm.dev);
+ params.ops = &v3d_bin_sched_ops;
+ params.name = "v3d_bin";
+
+ return drm_sched_init(&v3d->queue[V3D_BIN].sched, ¶ms);
+}
+
+static int
+v3d_render_sched_init(struct v3d_dev *v3d)
+{
+ struct drm_sched_init_params params;
+
+ v3d_common_sched_init(¶ms, v3d->drm.dev);
+ params.ops = &v3d_render_sched_ops;
+ params.name = "v3d_render";
+
+ return drm_sched_init(&v3d->queue[V3D_RENDER].sched, ¶ms);
+}
+
+static int
+v3d_tfu_sched_init(struct v3d_dev *v3d)
+{
+ struct drm_sched_init_params params;
+
+ v3d_common_sched_init(¶ms, v3d->drm.dev);
+ params.ops = &v3d_tfu_sched_ops;
+ params.name = "v3d_tfu";
+
+ return drm_sched_init(&v3d->queue[V3D_TFU].sched, ¶ms);
+}
+
+static int
+v3d_csd_sched_init(struct v3d_dev *v3d)
+{
+ struct drm_sched_init_params params;
+
+ v3d_common_sched_init(¶ms, v3d->drm.dev);
+ params.ops = &v3d_csd_sched_ops;
+ params.name = "v3d_csd";
+
+ return drm_sched_init(&v3d->queue[V3D_CSD].sched, ¶ms);
+}
+
+static int
+v3d_cache_sched_init(struct v3d_dev *v3d)
+{
+ struct drm_sched_init_params params;
+
+ v3d_common_sched_init(¶ms, v3d->drm.dev);
+ params.ops = &v3d_cache_clean_sched_ops;
+ params.name = "v3d_cache_clean";
+
+ return drm_sched_init(&v3d->queue[V3D_CACHE_CLEAN].sched, ¶ms);
+}
+
+static int
+v3d_cpu_sched_init(struct v3d_dev *v3d)
+{
+ struct drm_sched_init_params params;
+
+ v3d_common_sched_init(¶ms, v3d->drm.dev);
+ params.ops = &v3d_cpu_sched_ops;
+ params.name = "v3d_cpu";
+
+ return drm_sched_init(&v3d->queue[V3D_CPU].sched, ¶ms);
+}
+
int
v3d_sched_init(struct v3d_dev *v3d)
{
- int hw_jobs_limit = 1;
- int job_hang_limit = 0;
- int hang_limit_ms = 500;
int ret;
- ret = drm_sched_init(&v3d->queue[V3D_BIN].sched,
- &v3d_bin_sched_ops, NULL,
- DRM_SCHED_PRIORITY_COUNT,
- hw_jobs_limit, job_hang_limit,
- msecs_to_jiffies(hang_limit_ms), NULL,
- NULL, "v3d_bin", v3d->drm.dev);
+ ret = v3d_bin_sched_init(v3d);
if (ret)
return ret;
- ret = drm_sched_init(&v3d->queue[V3D_RENDER].sched,
- &v3d_render_sched_ops, NULL,
- DRM_SCHED_PRIORITY_COUNT,
- hw_jobs_limit, job_hang_limit,
- msecs_to_jiffies(hang_limit_ms), NULL,
- NULL, "v3d_render", v3d->drm.dev);
+ ret = v3d_render_sched_init(v3d);
if (ret)
goto fail;
- ret = drm_sched_init(&v3d->queue[V3D_TFU].sched,
- &v3d_tfu_sched_ops, NULL,
- DRM_SCHED_PRIORITY_COUNT,
- hw_jobs_limit, job_hang_limit,
- msecs_to_jiffies(hang_limit_ms), NULL,
- NULL, "v3d_tfu", v3d->drm.dev);
+ ret = v3d_tfu_sched_init(v3d);
if (ret)
goto fail;
if (v3d_has_csd(v3d)) {
- ret = drm_sched_init(&v3d->queue[V3D_CSD].sched,
- &v3d_csd_sched_ops, NULL,
- DRM_SCHED_PRIORITY_COUNT,
- hw_jobs_limit, job_hang_limit,
- msecs_to_jiffies(hang_limit_ms), NULL,
- NULL, "v3d_csd", v3d->drm.dev);
+ ret = v3d_csd_sched_init(v3d);
if (ret)
goto fail;
- ret = drm_sched_init(&v3d->queue[V3D_CACHE_CLEAN].sched,
- &v3d_cache_clean_sched_ops, NULL,
- DRM_SCHED_PRIORITY_COUNT,
- hw_jobs_limit, job_hang_limit,
- msecs_to_jiffies(hang_limit_ms), NULL,
- NULL, "v3d_cache_clean", v3d->drm.dev);
+ ret = v3d_cache_sched_init(v3d);
if (ret)
goto fail;
}
- ret = drm_sched_init(&v3d->queue[V3D_CPU].sched,
- &v3d_cpu_sched_ops, NULL,
- DRM_SCHED_PRIORITY_COUNT,
- 1, job_hang_limit,
- msecs_to_jiffies(hang_limit_ms), NULL,
- NULL, "v3d_cpu", v3d->drm.dev);
+ ret = v3d_cpu_sched_init(v3d);
if (ret)
goto fail;
