| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <24f1c52f-1768-47de-88e3-d4104969d0a9@igalia.com>
Date: Wed, 22 Jan 2025 19:07:38 -0300
From: Maíra Canal <mcanal@...lia.com>
To: Philipp Stanner <phasta@...nel.org>,
Alex Deucher <alexander.deucher@....com>,
Christian König <christian.koenig@....com>,
Xinhui Pan <Xinhui.Pan@....com>, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>, Lucas Stach <l.stach@...gutronix.de>,
Russell King <linux+etnaviv@...linux.org.uk>,
Christian Gmeiner <christian.gmeiner@...il.com>,
Frank Binns <frank.binns@...tec.com>, Matt Coster <matt.coster@...tec.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
Qiang Yu <yuq825@...il.com>, Rob Clark <robdclark@...il.com>,
Sean Paul <sean@...rly.run>, Konrad Dybcio <konradybcio@...nel.org>,
Abhinav Kumar <quic_abhinavk@...cinc.com>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Marijn Suijten <marijn.suijten@...ainline.org>,
Karol Herbst <kherbst@...hat.com>, Lyude Paul <lyude@...hat.com>,
Danilo Krummrich <dakr@...nel.org>,
Boris Brezillon <boris.brezillon@...labora.com>,
Rob Herring <robh@...nel.org>, Steven Price <steven.price@....com>,
Liviu Dudau <liviu.dudau@....com>, Luben Tuikov <ltuikov89@...il.com>,
Matthew Brost <matthew.brost@...el.com>,
Philipp Stanner <pstanner@...hat.com>, Melissa Wen <mwen@...lia.com>,
Lucas De Marchi <lucas.demarchi@...el.com>,
Thomas Hellström <thomas.hellstrom@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>, Sunil Khatri <sunil.khatri@....com>,
Lijo Lazar <lijo.lazar@....com>,
Mario Limonciello <mario.limonciello@....com>, Ma Jun <Jun.Ma2@....com>,
Yunxiang Li <Yunxiang.Li@....com>
Cc: amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, etnaviv@...ts.freedesktop.org,
lima@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
freedreno@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
intel-xe@...ts.freedesktop.org
Subject: Re: [PATCH] drm/sched: Use struct for drm_sched_init() params
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@...nel.org>
> ---
> 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;
>
Powered by blists - more mailing lists