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

[Maíra Canal]

Hi Philipp,

On 23/01/25 05:10, Philipp Stanner wrote:
> On Wed, 2025-01-22 at 19:07 -0300, Maíra Canal wrote:
> > Hi Philipp,
> >
> > On 22/01/25 11:08, 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 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@xxxxxxxxxx>

[...]

> > > diff --git a/drivers/gpu/drm/v3d/v3d_sched.c
> > > b/drivers/gpu/drm/v3d/v3d_sched.c
> > > index 99ac4995b5a1..716e6d074d87 100644
> > > --- a/drivers/gpu/drm/v3d/v3d_sched.c
> > > +++ b/drivers/gpu/drm/v3d/v3d_sched.c
> > > @@ -814,67 +814,124 @@ static const struct drm_sched_backend_ops
> > > v3d_cpu_sched_ops = {
> > >    	.free_job = v3d_cpu_job_free
> > >    };
> > >    
> > > +/*
> > > + * v3d's scheduler instances are all identical, except for ops and
> > > name.
> > > + */
> > > +static void
> > > +v3d_common_sched_init(struct drm_sched_init_params *params, struct
> > > device *dev)
> > > +{
> > > +	memset(params, 0, sizeof(struct drm_sched_init_params));
> > > +
> > > +	params->submit_wq = NULL; /* Use the system_wq. */
> > > +	params->num_rqs = DRM_SCHED_PRIORITY_COUNT;
> > > +	params->credit_limit = 1;
> > > +	params->hang_limit = 0;
> > > +	params->timeout = msecs_to_jiffies(500);
> > > +	params->timeout_wq = NULL; /* Use the system_wq. */
> > > +	params->score = NULL;
> > > +	params->dev = dev;
> > > +}
> >
> > Could we use only one function that takes struct v3d_dev *v3d, enum
> > v3d_queue, and sched_ops as arguments (instead of one function per
> > queue)? You can get the name of the scheduler by concatenating "v3d_"
> > to
> > the return of v3d_queue_to_string().
> >
> > I believe it would make the code much simpler.
>
> Hello,
>
> so just to get that right:
> You'd like to have one universal function that switch-cases over an
> enum, sets the ops and creates the name with string concatenation?
>
> I'm not convinced that this is simpler than a few small functions, but
> it's not my component, so…
>
> Whatever we'll do will be simpler than the existing code, though. Right
> now no reader can see at first glance whether all those schedulers are
> identically parametrized or not.

This is my proposal (just a quick draft, please check if it compiles):

diff --git a/drivers/gpu/drm/v3d/v3d_sched.c 
b/drivers/gpu/drm/v3d/v3d_sched.c
index 961465128d80..7cc45a0c6ca0 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -820,67 +820,62 @@ static const struct drm_sched_backend_ops 
v3d_cpu_sched_ops = {
        .free_job = v3d_cpu_job_free
 };

+static int
+v3d_sched_queue_init(struct v3d_dev *v3d, enum v3d_queue queue,
+                    const struct drm_sched_backend_ops *ops, const char 
*name)
+{
+       struct drm_sched_init_params params = {
+               .submit_wq = NULL,
+               .num_rqs = DRM_SCHED_PRIORITY_COUNT,
+               .credit_limit = 1,
+               .hang_limit = 0,
+               .timeout = msecs_to_jiffies(500),
+               .timeout_wq = NULL,
+               .score = NULL,
+               .dev = v3d->drm.dev,
+       };
+
+       params.ops = ops;
+       params.name = name;
+
+       return drm_sched_init(&v3d->queue[queue].sched, &params);
+}
+
 int
 v3d_sched_init(struct v3d_dev *v3d)
 {
-       int hw_jobs_limit = 1;
-       int job_hang_limit = 0;
-       int hang_limit_ms = 500;
        int ret;

-       ret = drm_sched_init(&v3d->queue[V3D_BIN].sched,
-                            &v3d_bin_sched_ops, NULL,
-                            DRM_SCHED_PRIORITY_COUNT,
-                            hw_jobs_limit, job_hang_limit,
-                            msecs_to_jiffies(hang_limit_ms), NULL,
-                            NULL, "v3d_bin", v3d->drm.dev);
+       ret = v3d_sched_queue_init(v3d, V3D_BIN, &v3d_bin_sched_ops,
+                                  "v3d_bin");
        if (ret)
                return ret;

-       ret = drm_sched_init(&v3d->queue[V3D_RENDER].sched,
-                            &v3d_render_sched_ops, NULL,
-                            DRM_SCHED_PRIORITY_COUNT,
-                            hw_jobs_limit, job_hang_limit,
-                            msecs_to_jiffies(hang_limit_ms), NULL,
-                            NULL, "v3d_render", v3d->drm.dev);
+       ret = v3d_sched_queue_init(v3d, V3D_RENDER, &v3d_render_sched_ops,
+                                  "v3d_render");
        if (ret)
                goto fail;

[...]

At least for me, this looks much simpler than one function for each
V3D queue.

Best Regards,
- Maíra

> P.
>
>
> > Best Regards,
> > - Maíra