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 <[email protected]> --- 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;
