Re: [PATCH v4] drm/sched: Use struct for drm_sched_init() params

[Philipp Stanner <phasta@mailbox.org>]

On Tue, 2025-07-08 at 14:02 +0100, Tvrtko Ursulin wrote:
> 
> 
> 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@kernel.org>
> > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> > Acked-by: Matthew Brost <matthew.brost@intel.com> # for Xe
> > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> # for
> > Panfrost and Panthor
> > Reviewed-by: Christian Gmeiner <cgmeiner@igalia.com> # for Etnaviv
> > Reviewed-by: Frank Binns <frank.binns@imgtec.com> # for Imagination
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> # for Sched
> > Reviewed-by: Maíra Canal <mcanal@igalia.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.

Ouch yes, that's definitely a very subtle bug. AFAICS it comes to be by
pfdev being initialized to 0.

Let me provide a fix..

P.

> 
> > @@ -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
>