| 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: <e7414579afbfc25d4027471bc265ee48e7d25932.camel@mailbox.org>
Date: Thu, 23 Jan 2025 13:13:25 +0100
From: Philipp Stanner <phasta@...lbox.org>
To: Maíra Canal <mcanal@...lia.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
On Thu, 2025-01-23 at 08:10 -0300, Maíra Canal wrote:
> 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
Is it a queue, though?
How about _v3d_sched_init()?
P.
> 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