linux-kernel - Re: [PATCH] drm/sched: Use struct for drm_sched

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z5EBF7W706aGJoVt@pollux>
Date: Wed, 22 Jan 2025 15:30:47 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Philipp Stanner <phasta@...nel.org>
Cc: 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>,
	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>,
	Maíra Canal <mcanal@...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>,
	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 Wed, Jan 22, 2025 at 03:08:20PM +0100, 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/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index cd4fac120834..c1f03eb5f5ea 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2821,6 +2821,9 @@ static int amdgpu_device_init_schedulers(struct amdgpu_device *adev)
>  {
>  	long timeout;
>  	int r, i;
> +	struct drm_sched_init_params params;
> +
> +	memset(&params, 0, sizeof(struct drm_sched_init_params));

I think we should drop the memset() and just write it as:

struct drm_sched_init_params params = {};

<snip>

> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 95e17504e46a..1a834ef43862 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -553,12 +553,37 @@ struct drm_gpu_scheduler {
>  	struct device			*dev;
>  };
>  
> +/**
> + * struct drm_sched_init_params - parameters for initializing a DRM GPU scheduler

Since this is a separate structure now, I think we should point out which fields
are mandatory to set and which of those have a valid default to zero.

> + *
> + * @ops: backend operations provided by the driver
> + * @submit_wq: workqueue to use for submission. If NULL, an ordered wq is
> + *	       allocated and used
> + * @num_rqs: Number of run-queues. This is at most DRM_SCHED_PRIORITY_COUNT,
> + *           as there's usually one run-queue per priority, but could be less.
> + * @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: associated device. Used for debugging
> + */
> +struct drm_sched_init_params {
> +	const struct drm_sched_backend_ops *ops;
> +	struct workqueue_struct *submit_wq;
> +	struct workqueue_struct *timeout_wq;
> +	u32 num_rqs, credit_limit;
> +	unsigned int hang_limit;
> +	long timeout;
> +	atomic_t *score;
> +	const char *name;
> +	struct device *dev;
> +};
> +
>  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);
> +		const struct drm_sched_init_params *params);
>  
>  void drm_sched_fini(struct drm_gpu_scheduler *sched);
>  int drm_sched_job_init(struct drm_sched_job *job,
> -- 
> 2.47.1
>