| 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: <d3ab5057-22fe-43d5-baea-4dec091fb303@igalia.com>
Date: Wed, 29 Jan 2025 11:53:47 -0300
From: Maíra Canal <mcanal@...lia.com>
To: 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@...hat.com>,
Boris Brezillon <boris.brezillon@...labora.com>,
Rob Herring <robh@...nel.org>, Steven Price <steven.price@....com>,
Liviu Dudau <liviu.dudau@....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>, Hawking Zhang <Hawking.Zhang@....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 v2] drm/sched: Use struct for drm_sched_init() params
Hi Philipp,
On 29/01/25 09:39, Philipp Stanner wrote:
> On Wed, 2025-01-29 at 07:53 -0300, Maíra Canal wrote:
>> Hi Philipp,
>>
>> On 28/01/25 11:29, 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>
>>> ---
>>> Changes in v2:
>>> - Point out that the hang-limit is deprecated. (Christian)
>>> - Initialize the structs to 0 at declaration. (Planet Earth)
>>> - Don't set stuff explicitly to 0 / NULL. (Tvrtko)
>>> - Make the structs const where possible. (Boris)
>>> - v3d: Use just 1, universal, function for sched-init. (Maíra)
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 ++++--
>>> drivers/gpu/drm/etnaviv/etnaviv_sched.c | 20 +++----
>>> drivers/gpu/drm/imagination/pvr_queue.c | 18 ++++--
>>> drivers/gpu/drm/lima/lima_sched.c | 16 +++--
>>> drivers/gpu/drm/msm/msm_ringbuffer.c | 17 +++---
>>> drivers/gpu/drm/nouveau/nouveau_sched.c | 15 +++--
>>> drivers/gpu/drm/panfrost/panfrost_job.c | 20 ++++---
>>> drivers/gpu/drm/panthor/panthor_mmu.c | 16 +++--
>>> drivers/gpu/drm/panthor/panthor_sched.c | 29 +++++----
>>> drivers/gpu/drm/scheduler/sched_main.c | 50 ++++++----------
>>> drivers/gpu/drm/v3d/v3d_sched.c | 68 +++++++++--------
>>> -----
>>> drivers/gpu/drm/xe/xe_execlist.c | 16 +++--
>>> drivers/gpu/drm/xe/xe_gpu_scheduler.c | 17 +++++-
>>> include/drm/gpu_scheduler.h | 37 ++++++++++--
>>> 14 files changed, 206 insertions(+), 151 deletions(-)
>>>
>>
>> [...]
>>
>>> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c
>>> b/drivers/gpu/drm/panthor/panthor_sched.c
>>> index 5844a7f639e0..44713cfdcd74 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_sched.c
>>> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
>>> @@ -3284,6 +3284,22 @@ static struct panthor_queue *
>>> group_create_queue(struct panthor_group *group,
>>> const struct drm_panthor_queue_create *args)
>>> {
>>> + const struct drm_sched_init_args sched_args = {
>>> + .ops = &panthor_queue_sched_ops,
>>> + .submit_wq = group->ptdev->scheduler->wq,
>>> + .num_rqs = 1,
>>> + /*
>>> + * The credit limit argument tells us the total
>>> number of
>>> + * instructions across all CS slots in the
>>> ringbuffer, with
>>> + * some jobs requiring twice as many as others,
>>> depending on
>>> + * their profiling status.
>>> + */
>>> + .credit_limit = args->ringbuf_size / sizeof(u64),
>>> + .timeout = msecs_to_jiffies(JOB_TIMEOUT_MS),
>>> + .timeout_wq = group->ptdev->reset.wq,
>>> + .name = "panthor-queue",
>>> + .dev = group->ptdev->base.dev
>>> + };
>>> struct drm_gpu_scheduler *drm_sched;
>>> struct panthor_queue *queue;
>>> int ret;
>>> @@ -3354,17 +3370,8 @@ group_create_queue(struct panthor_group
>>> *group,
>>> if (ret)
>>> goto err_free_queue;
>>>
>>> - /*
>>> - * Credit limit argument tells us the total number of
>>> instructions
>>> - * across all CS slots in the ringbuffer, with some jobs
>>> requiring
>>> - * twice as many as others, depending on their profiling
>>> status.
>>> - */
>>> - ret = drm_sched_init(&queue->scheduler,
>>> &panthor_queue_sched_ops,
>>> - group->ptdev->scheduler->wq, 1,
>>> - args->ringbuf_size / sizeof(u64),
>>> - 0, msecs_to_jiffies(JOB_TIMEOUT_MS),
>>> - group->ptdev->reset.wq,
>>> - NULL, "panthor-queue", group->ptdev-
>>>> base.dev);
>>> +
>>
>> Please don't use multiple blank lines.
>>
>>> + ret = drm_sched_init(&queue->scheduler, &sched_args);
>>> if (ret)
>>> goto err_free_queue;
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index a48be16ab84f..6295b2654a7c 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -1244,40 +1244,24 @@ static void drm_sched_run_job_work(struct
>>> work_struct *w)
>>> * drm_sched_init - Init a gpu scheduler instance
>>> *
>>> * @sched: scheduler instance
>>> - * @ops: backend operations for this scheduler
>>> - * @submit_wq: workqueue to use for submission. If NULL, an
>>> ordered wq is
>>> - * allocated and used
>>> - * @num_rqs: number of runqueues, one for each priority, up to
>>> DRM_SCHED_PRIORITY_COUNT
>>> - * @credit_limit: the number of credits this scheduler can hold
>>> from all jobs
>>> - * @hang_limit: number of times to allow a job to hang before
>>> dropping it
>>> - * @timeout: timeout value in jiffies for the scheduler
>>> - * @timeout_wq: workqueue to use for timeout work. If NULL, the
>>> system_wq is
>>> - * used
>>> - * @score: optional score atomic shared with other schedulers
>>> - * @name: name used for debugging
>>> - * @dev: target &struct device
>>> + * @args: scheduler initialization arguments
>>> *
>>> * Return 0 on success, otherwise error code.
>>> */
>>> -int drm_sched_init(struct drm_gpu_scheduler *sched,
>>> - const struct drm_sched_backend_ops *ops,
>>> - struct workqueue_struct *submit_wq,
>>> - u32 num_rqs, u32 credit_limit, unsigned int
>>> hang_limit,
>>> - long timeout, struct workqueue_struct
>>> *timeout_wq,
>>> - atomic_t *score, const char *name, struct
>>> device *dev)
>>> +int drm_sched_init(struct drm_gpu_scheduler *sched, const struct
>>> drm_sched_init_args *args)
>>> {
>>> int i;
>>>
>>> - sched->ops = ops;
>>> - sched->credit_limit = credit_limit;
>>> - sched->name = name;
>>> - sched->timeout = timeout;
>>> - sched->timeout_wq = timeout_wq ? : system_wq;
>>> - sched->hang_limit = hang_limit;
>>> - sched->score = score ? score : &sched->_score;
>>> - sched->dev = dev;
>>> + sched->ops = args->ops;
>>> + sched->credit_limit = args->credit_limit;
>>> + sched->name = args->name;
>>> + sched->timeout = args->timeout;
>>> + sched->timeout_wq = args->timeout_wq ? : system_wq;
>>> + sched->hang_limit = args->hang_limit;
>>> + sched->score = args->score ? args->score : &sched->_score;
>>
>> Could we keep it consistent and use the Elvis Operator here as well?
>> Just like `sched->timeout_wq`.
>
> This is literally just the old code.
>
> And if at all, this insanely stupid GCC extension should not be used.
> It's one of the typical compiler people rampages that make the C
> language so terrible.
Not a problem to me, we can remove the Elvis Operator from `sched-
>timeout_wq`. My idea is just to do things consistently in variable
assignment.
Best Regards,
- Maíra
Powered by blists - more mailing lists