On Wed, 22 Jan 2025 16:14:59 +0000
Tvrtko Ursulin <tursulin@xxxxxxxxxxx> wrote:
> On 22/01/2025 15:51, Boris Brezillon wrote:
> > On Wed, 22 Jan 2025 15:08:20 +0100
> > Philipp Stanner <phasta@xxxxxxxxxx> wrote:
> >
> >> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> >> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> >> @@ -3272,6 +3272,7 @@ group_create_queue(struct panthor_group *group,
> >> const struct drm_panthor_queue_create *args)
> >> {
> >> struct drm_gpu_scheduler *drm_sched;
> >> + struct drm_sched_init_params sched_params;
> >
> > nit: Could we use a struct initializer instead of a
> > memset(0)+field-assignment?
> >
> > struct drm_sched_init_params sched_params = {
Actually, you can even make it const if it's not modified after the
declaration.
> > .ops = &panthor_queue_sched_ops,
> > .submit_wq = group->ptdev->scheduler->wq,
> > .num_rqs = 1,
> > .credit_limit = args->ringbuf_size / sizeof(u64),
> > .hang_limit = 0,
> > .timeout = msecs_to_jiffies(JOB_TIMEOUT_MS),
> > .timeout_wq = group->ptdev->reset.wq,
> > .name = "panthor-queue",
> > .dev = group->ptdev->base.dev,
> > };
>
> +1 on this as a general approach for the whole series. And I'd drop the
> explicit zeros and NULLs. Memsets could then go too.
>
> Regards,
>
> Tvrtko
>
> >
> > The same comment applies the panfrost changes BTW.
> >
> >> struct panthor_queue *queue;
> >> int ret;
> >>
> >> @@ -3289,6 +3290,8 @@ group_create_queue(struct panthor_group *group,
> >> if (!queue)
> >> return ERR_PTR(-ENOMEM);
> >>
> >> + memset(&sched_params, 0, sizeof(struct drm_sched_init_params));
> >> +
> >> queue->fence_ctx.id = dma_fence_context_alloc(1);
> >> spin_lock_init(&queue->fence_ctx.lock);
> >> INIT_LIST_HEAD(&queue->fence_ctx.in_flight_jobs);
> >> @@ -3341,17 +3344,23 @@ group_create_queue(struct panthor_group *group,
> >> if (ret)
> >> goto err_free_queue;
> >>
> >> + sched_params.ops = &panthor_queue_sched_ops;
> >> + sched_params.submit_wq = group->ptdev->scheduler->wq;
> >> + sched_params.num_rqs = 1;
> >> /*
> >> - * Credit limit argument tells us the total number of instructions
> >> + * 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.
> >> */
> >> - 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);
> >> + sched_params.credit_limit = args->ringbuf_size / sizeof(u64);
> >> + sched_params.hang_limit = 0;
> >> + sched_params.timeout = msecs_to_jiffies(JOB_TIMEOUT_MS);
> >> + sched_params.timeout_wq = group->ptdev->reset.wq;
> >> + sched_params.score = NULL;
> >> + sched_params.name = "panthor-queue";
> >> + sched_params.dev = group->ptdev->base.dev;
> >> +
> >> + ret = drm_sched_init(&queue->scheduler, &sched_params);
> >> if (ret)
> >> goto err_free_queue;
