| 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: <55569c2edf754c0e751d272016630528a28e86c8.camel@redhat.com>
Date: Thu, 23 Jan 2025 09:39:31 +0100
From: Philipp Stanner <pstanner@...hat.com>
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 09:10 +0100, 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>
> > > ---
> > > 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.
>
> 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?
Oh, and here's another issue:
The @name string has life time issues to take into account. It must
live as long as the scheduler instance.
In your mind, where should the memory for the strings your
concatenating be and how should their life time be managed?
Currently they're in the TEXT segment, which is fine
P.
>
> 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.
>
> P.
>
>
> >
> > 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