On Wed, Jan 22, 2025 at 03:08:20PM +0100, 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/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index cd4fac120834..c1f03eb5f5ea 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2821,6 +2821,9 @@ static int amdgpu_device_init_schedulers(struct amdgpu_device *adev)
> {
> long timeout;
> int r, i;
> + struct drm_sched_init_params params;
> +
> + memset(¶ms, 0, sizeof(struct drm_sched_init_params));
I think we should drop the memset() and just write it as:
struct drm_sched_init_params params = {};
<snip>
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 95e17504e46a..1a834ef43862 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -553,12 +553,37 @@ struct drm_gpu_scheduler {
> struct device *dev;
> };
>
> +/**
> + * struct drm_sched_init_params - parameters for initializing a DRM GPU scheduler
Since this is a separate structure now, I think we should point out which fields
are mandatory to set and which of those have a valid default to zero.
> + *
> + * @ops: backend operations provided by the driver
> + * @submit_wq: workqueue to use for submission. If NULL, an ordered wq is
> + * allocated and used
> + * @num_rqs: Number of run-queues. This is at most DRM_SCHED_PRIORITY_COUNT,
> + * as there's usually one run-queue per priority, but could be less.
> + * @credit_limit: the number of credits this scheduler can hold from all jobs
> + * @hang_limit: number of times to allow a job to hang before dropping it
> + * @timeout: timeout value in jiffies for the scheduler
> + * @timeout_wq: workqueue to use for timeout work. If NULL, the system_wq is
> + * used
> + * @score: optional score atomic shared with other schedulers
> + * @name: name used for debugging
> + * @dev: associated device. Used for debugging
> + */
> +struct drm_sched_init_params {
> + const struct drm_sched_backend_ops *ops;
> + struct workqueue_struct *submit_wq;
> + struct workqueue_struct *timeout_wq;
> + u32 num_rqs, credit_limit;
> + unsigned int hang_limit;
> + long timeout;
> + atomic_t *score;
> + const char *name;
> + struct device *dev;
> +};
> +
> int drm_sched_init(struct drm_gpu_scheduler *sched,
> - const struct drm_sched_backend_ops *ops,
> - struct workqueue_struct *submit_wq,
> - u32 num_rqs, u32 credit_limit, unsigned int hang_limit,
> - long timeout, struct workqueue_struct *timeout_wq,
> - atomic_t *score, const char *name, struct device *dev);
> + const struct drm_sched_init_params *params);
>
> void drm_sched_fini(struct drm_gpu_scheduler *sched);
> int drm_sched_job_init(struct drm_sched_job *job,
> --
> 2.47.1
>
