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

[Maíra Canal]

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 at kernel.org>
> ---
> 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.

Best Regards,
- Maíra

> +
> +static int
> +v3d_bin_sched_init(struct v3d_dev *v3d)
> +{
> +	struct drm_sched_init_params params;
> +
> +	v3d_common_sched_init(&params, v3d->drm.dev);
> +	params.ops = &v3d_bin_sched_ops;
> +	params.name = "v3d_bin";
> +
> +	return drm_sched_init(&v3d->queue[V3D_BIN].sched, &params);
> +}
> +
> +static int
> +v3d_render_sched_init(struct v3d_dev *v3d)
> +{
> +	struct drm_sched_init_params params;
> +
> +	v3d_common_sched_init(&params, v3d->drm.dev);
> +	params.ops = &v3d_render_sched_ops;
> +	params.name = "v3d_render";
> +
> +	return drm_sched_init(&v3d->queue[V3D_RENDER].sched, &params);
> +}
> +
> +static int
> +v3d_tfu_sched_init(struct v3d_dev *v3d)
> +{
> +	struct drm_sched_init_params params;
> +
> +	v3d_common_sched_init(&params, v3d->drm.dev);
> +	params.ops = &v3d_tfu_sched_ops;
> +	params.name = "v3d_tfu";
> +
> +	return drm_sched_init(&v3d->queue[V3D_TFU].sched, &params);
> +}
> +
> +static int
> +v3d_csd_sched_init(struct v3d_dev *v3d)
> +{
> +	struct drm_sched_init_params params;
> +
> +	v3d_common_sched_init(&params, v3d->drm.dev);
> +	params.ops = &v3d_csd_sched_ops;
> +	params.name = "v3d_csd";
> +
> +	return drm_sched_init(&v3d->queue[V3D_CSD].sched, &params);
> +}
> +
> +static int
> +v3d_cache_sched_init(struct v3d_dev *v3d)
> +{
> +	struct drm_sched_init_params params;
> +
> +	v3d_common_sched_init(&params, 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, &params);
> +}
> +
> +static int
> +v3d_cpu_sched_init(struct v3d_dev *v3d)
> +{
> +	struct drm_sched_init_params params;
> +
> +	v3d_common_sched_init(&params, v3d->drm.dev);
> +	params.ops = &v3d_cpu_sched_ops;
> +	params.name = "v3d_cpu";
> +
> +	return drm_sched_init(&v3d->queue[V3D_CPU].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_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;
>