On Wed, 2025-01-22 at 20:37 -0800, Matthew Brost wrote:
> On Wed, Jan 22, 2025 at 06:04:58PM +0100, Boris Brezillon wrote:
> > 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,
> > > > };
> > >
>
> +2
Yup, getting rid of memset() similar to Danilo's suggestion is surely a
good idea.
I personally don't like mixing initialization and declaration when
possible (readability), but having it const is probably a good
argument.
P.
>
> Matt
>
> > > +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;
> >
