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>
> > ---
> > Howdy,
> >
> > I have a patch-series in the pipe that will add a `flags` argument
> > to
> > drm_sched_init(). I thought it would be wise to first rework the
> > API as
> > detailed in this patch. It's really a lot of parameters by now, and
> > I
> > would expect that it might get more and more over the years for
> > special
> > use cases etc.
> >
> > Regards,
> > P.
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 21 +++-
> > drivers/gpu/drm/etnaviv/etnaviv_sched.c | 20 ++-
> > drivers/gpu/drm/imagination/pvr_queue.c | 21 +++-
> > drivers/gpu/drm/lima/lima_sched.c | 21 +++-
> > drivers/gpu/drm/msm/msm_ringbuffer.c | 22 ++--
> > drivers/gpu/drm/nouveau/nouveau_sched.c | 20 ++-
> > drivers/gpu/drm/panfrost/panfrost_job.c | 22 ++--
> > drivers/gpu/drm/panthor/panthor_mmu.c | 18 ++-
> > drivers/gpu/drm/panthor/panthor_sched.c | 23 ++--
> > drivers/gpu/drm/scheduler/sched_main.c | 53 +++-----
> > drivers/gpu/drm/v3d/v3d_sched.c | 135 +++++++++++++++-
> > -----
> > drivers/gpu/drm/xe/xe_execlist.c | 20 ++-
> > drivers/gpu/drm/xe/xe_gpu_scheduler.c | 19 ++-
> > include/drm/gpu_scheduler.h | 35 +++++-
> > 14 files changed, 311 insertions(+), 139 deletions(-)
> >
>
> [...]
>
> > 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.
P.
>
> Best Regards,
> - Maíra
>
> > +
> > +static int
> > +v3d_bin_sched_init(struct v3d_dev *v3d)
> > +{
> > + struct drm_sched_init_params params;
> > +
> > + v3d_common_sched_init(¶ms, v3d->drm.dev);
> > + params.ops = &v3d_bin_sched_ops;
> > + params.name = "v3d_bin";
> > +
> > + return drm_sched_init(&v3d->queue[V3D_BIN].sched,
> > ¶ms);
> > +}
> > +
> > +static int
> > +v3d_render_sched_init(struct v3d_dev *v3d)
> > +{
> > + struct drm_sched_init_params params;
> > +
> > + v3d_common_sched_init(¶ms, v3d->drm.dev);
> > + params.ops = &v3d_render_sched_ops;
> > + params.name = "v3d_render";
> > +
> > + return drm_sched_init(&v3d->queue[V3D_RENDER].sched,
> > ¶ms);
> > +}
> > +
> > +static int
> > +v3d_tfu_sched_init(struct v3d_dev *v3d)
> > +{
> > + struct drm_sched_init_params params;
> > +
> > + v3d_common_sched_init(¶ms, v3d->drm.dev);
> > + params.ops = &v3d_tfu_sched_ops;
> > + params.name = "v3d_tfu";
> > +
> > + return drm_sched_init(&v3d->queue[V3D_TFU].sched,
> > ¶ms);
> > +}
> > +
> > +static int
> > +v3d_csd_sched_init(struct v3d_dev *v3d)
> > +{
> > + struct drm_sched_init_params params;
> > +
> > + v3d_common_sched_init(¶ms, v3d->drm.dev);
> > + params.ops = &v3d_csd_sched_ops;
> > + params.name = "v3d_csd";
> > +
> > + return drm_sched_init(&v3d->queue[V3D_CSD].sched,
> > ¶ms);
> > +}
> > +
> > +static int
> > +v3d_cache_sched_init(struct v3d_dev *v3d)
> > +{
> > + struct drm_sched_init_params params;
> > +
> > + v3d_common_sched_init(¶ms, v3d->drm.dev);
> > + params.ops = &v3d_cache_clean_sched_ops;
> > + params.name = "v3d_cache_clean";
> > +
> > + return drm_sched_init(&v3d->queue[V3D_CACHE_CLEAN].sched,
> > ¶ms);
> > +}
> > +
> > +static int
> > +v3d_cpu_sched_init(struct v3d_dev *v3d)
> > +{
> > + struct drm_sched_init_params params;
> > +
> > + v3d_common_sched_init(¶ms, v3d->drm.dev);
> > + params.ops = &v3d_cpu_sched_ops;
> > + params.name = "v3d_cpu";
> > +
> > + return drm_sched_init(&v3d->queue[V3D_CPU].sched,
> > ¶ms);
> > +}
> > +
> > 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_bin_sched_init(v3d);
> > 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_render_sched_init(v3d);
> > if (ret)
> > goto fail;
> >
> > - ret = drm_sched_init(&v3d->queue[V3D_TFU].sched,
> > - &v3d_tfu_sched_ops, NULL,
> > - DRM_SCHED_PRIORITY_COUNT,
> > - hw_jobs_limit, job_hang_limit,
> > - msecs_to_jiffies(hang_limit_ms),
> > NULL,
> > - NULL, "v3d_tfu", v3d->drm.dev);
> > + ret = v3d_tfu_sched_init(v3d);
> > if (ret)
> > goto fail;
> >
> > if (v3d_has_csd(v3d)) {
> > - ret = drm_sched_init(&v3d->queue[V3D_CSD].sched,
> > - &v3d_csd_sched_ops, NULL,
> > - DRM_SCHED_PRIORITY_COUNT,
> > - hw_jobs_limit,
> > job_hang_limit,
> > -
> > msecs_to_jiffies(hang_limit_ms), NULL,
> > - NULL, "v3d_csd", v3d-
> > >drm.dev);
> > + ret = v3d_csd_sched_init(v3d);
> > if (ret)
> > goto fail;
> >
> > - ret = drm_sched_init(&v3d-
> > >queue[V3D_CACHE_CLEAN].sched,
> > - &v3d_cache_clean_sched_ops,
> > NULL,
> > - DRM_SCHED_PRIORITY_COUNT,
> > - hw_jobs_limit,
> > job_hang_limit,
> > -
> > msecs_to_jiffies(hang_limit_ms), NULL,
> > - NULL, "v3d_cache_clean", v3d-
> > >drm.dev);
> > + ret = v3d_cache_sched_init(v3d);
> > if (ret)
> > goto fail;
> > }
> >
> > - ret = drm_sched_init(&v3d->queue[V3D_CPU].sched,
> > - &v3d_cpu_sched_ops, NULL,
> > - DRM_SCHED_PRIORITY_COUNT,
> > - 1, job_hang_limit,
> > - msecs_to_jiffies(hang_limit_ms),
> > NULL,
> > - NULL, "v3d_cpu", v3d->drm.dev);
> > + ret = v3d_cpu_sched_init(v3d);
> > if (ret)
> > goto fail;
> >
>
