| 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: <ec4bb0f6-c366-40e7-a1df-332458b08eec@igalia.com>
Date: Thu, 23 Jan 2025 08:10:01 -0300
From: Maíra Canal <mcanal@...lia.com>
To: Philipp Stanner <pstanner@...hat.com>, 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>, 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 23/01/25 05:10, Philipp Stanner wrote:
> On Wed, 2025-01-22 at 19:07 -0300, Maíra Canal wrote:
>> 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>
[...]
>>
>>> 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.
>
> Hello,
>
> so just to get that right:
> You'd like to have one universal function that switch-cases over an
> enum, sets the ops and creates the name with string concatenation?
>
> I'm not convinced that this is simpler than a few small functions, but
> it's not my component, so…
>
> Whatever we'll do will be simpler than the existing code, though. Right
> now no reader can see at first glance whether all those schedulers are
> identically parametrized or not.
>
This is my proposal (just a quick draft, please check if it compiles):
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c
b/drivers/gpu/drm/v3d/v3d_sched.c
index 961465128d80..7cc45a0c6ca0 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -820,67 +820,62 @@ static const struct drm_sched_backend_ops
v3d_cpu_sched_ops = {
.free_job = v3d_cpu_job_free
};
+static int
+v3d_sched_queue_init(struct v3d_dev *v3d, enum v3d_queue queue,
+ const struct drm_sched_backend_ops *ops, const char
*name)
+{
+ struct drm_sched_init_params params = {
+ .submit_wq = NULL,
+ .num_rqs = DRM_SCHED_PRIORITY_COUNT,
+ .credit_limit = 1,
+ .hang_limit = 0,
+ .timeout = msecs_to_jiffies(500),
+ .timeout_wq = NULL,
+ .score = NULL,
+ .dev = v3d->drm.dev,
+ };
+
+ params.ops = ops;
+ params.name = name;
+
+ return drm_sched_init(&v3d->queue[queue].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_sched_queue_init(v3d, V3D_BIN, &v3d_bin_sched_ops,
+ "v3d_bin");
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_sched_queue_init(v3d, V3D_RENDER, &v3d_render_sched_ops,
+ "v3d_render");
if (ret)
goto fail;
[...]
At least for me, this looks much simpler than one function for each
V3D queue.
Best Regards,
- Maíra
> P.
>
>
>>
>> Best Regards,
>> - Maíra
>>
Powered by blists - more mailing lists