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