Message-ID: <b5d0921c-7cbf-4d55-aa47-c35cd7861c02@igalia.com>
Date: Tue, 8 Jul 2025 14:02:20 +0100
From: Tvrtko Ursulin <tvrtko.ursulin@...lia.com>
To: Philipp Stanner <phasta@...nel.org>, Min Ma <min.ma@....com>,
 Lizhi Hou <lizhi.hou@....com>, Oded Gabbay <ogabbay@...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>, 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>, Hawking Zhang <Hawking.Zhang@....com>,
 Mario Limonciello <mario.limonciello@....com>, Ma Jun <Jun.Ma2@....com>,
 Yunxiang Li <Yunxiang.Li@....com>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 amd-gfx@...ts.freedesktop.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, Christian Gmeiner <cgmeiner@...lia.com>
Subject: Re: [PATCH v4] drm/sched: Use struct for drm_sched_init() params



On 11/02/2025 11:14, 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, addressed 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>
> Reviewed-by: Liviu Dudau <liviu.dudau@....com>
> Acked-by: Matthew Brost <matthew.brost@...el.com> # for Xe
> Reviewed-by: Boris Brezillon <boris.brezillon@...labora.com> # for Panfrost and Panthor
> Reviewed-by: Christian Gmeiner <cgmeiner@...lia.com> # for Etnaviv
> Reviewed-by: Frank Binns <frank.binns@...tec.com> # for Imagination
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@...lia.com> # for Sched
> Reviewed-by: Maíra Canal <mcanal@...lia.com> # for v3d
> ---
> Changes in v4:
>    - Add forgotten driver accel/amdxdna. (Me)
>    - Rephrase the "init to NULL" comments. (Tvrtko)
>    - Apply RBs by Tvrtko and Maira.
>    - Terminate the last struct members with a comma, so that future
>      fields can be added with a minimal patch diff. (Me)
> 
> Changes in v3:
>    - Various formatting requirements.
> 
> 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)
> ---

8><

> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 9b8e82fb8bc4..5657106c2f7d 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -836,8 +836,16 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data)
>   
>   int panfrost_job_init(struct panfrost_device *pfdev)
>   {
> +	struct drm_sched_init_args args = {
> +		.ops = &panfrost_sched_ops,
> +		.num_rqs = DRM_SCHED_PRIORITY_COUNT,
> +		.credit_limit = 2,
> +		.timeout = msecs_to_jiffies(JOB_TIMEOUT_MS),
> +		.timeout_wq = pfdev->reset.wq,

^^^

> +		.name = "pan_js",
> +		.dev = pfdev->dev,
> +	};
>   	struct panfrost_job_slot *js;
> -	unsigned int nentries = 2;
>   	int ret, j;
>   
>   	/* All GPUs have two entries per queue, but without jobchain
> @@ -845,7 +853,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
>   	 * so let's just advertise one entry in that case.
>   	 */
>   	if (!panfrost_has_hw_feature(pfdev, HW_FEATURE_JOBCHAIN_DISAMBIGUATION))
> -		nentries = 1;
> +		args.credit_limit = 1;
>   
>   	pfdev->js = js = devm_kzalloc(pfdev->dev, sizeof(*js), GFP_KERNEL);
>   	if (!js)

Stumbled on this while looking at drm_sched_init() workqueue usage.

I think this patch might need a fixup. Because somewhere around here in 
the code there is this:

	pfdev->reset.wq = alloc_ordered_workqueue("panfrost-reset", 0);
	if (!pfdev->reset.wq)
		return -ENOMEM;

Which means that after the patch panfrost is using system_wq for the 
timeout handler instead the one it creates.

> @@ -875,13 +883,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
>   	for (j = 0; j < NUM_JOB_SLOTS; j++) {
>   		js->queue[j].fence_context = dma_fence_context_alloc(1);
>   
> -		ret = drm_sched_init(&js->queue[j].sched,
> -				     &panfrost_sched_ops, NULL,
> -				     DRM_SCHED_PRIORITY_COUNT,
> -				     nentries, 0,
> -				     msecs_to_jiffies(JOB_TIMEOUT_MS),
> -				     pfdev->reset.wq,
> -				     NULL, "pan_js", pfdev->dev);
> +		ret = drm_sched_init(&js->queue[j].sched, &args);

^^^

>   		if (ret) {
>   			dev_err(pfdev->dev, "Failed to create scheduler: %d.", ret);
>   			goto err_sched;

Regards,

Tvrtko

Powered by blists - more mailing lists