Maíra Canal
2025-Jan-29 14:53 UTC
[PATCH v2] drm/sched: Use struct for drm_sched_init() params
Hi Philipp, On 29/01/25 09:39, Philipp Stanner wrote:> On Wed, 2025-01-29 at 07:53 -0300, Ma?ra Canal wrote: >> Hi Philipp, >> >> On 28/01/25 11:29, 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> >>> --- >>> Changes in v2: >>> ?? - Point out that the hang-limit is deprecated. (Christian) >>> ?? - Initialize the structs to 0 at declaration. (Planet Earth) >>> ?? - Don't set stuff explicitly to 0 / NULL. (Tvrtko) >>> ?? - Make the structs const where possible. (Boris) >>> ?? - v3d: Use just 1, universal, function for sched-init. (Ma?ra) >>> --- >>> ? drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 ++++-- >>> ? drivers/gpu/drm/etnaviv/etnaviv_sched.c??? | 20 +++---- >>> ? drivers/gpu/drm/imagination/pvr_queue.c??? | 18 ++++-- >>> ? drivers/gpu/drm/lima/lima_sched.c????????? | 16 +++-- >>> ? drivers/gpu/drm/msm/msm_ringbuffer.c?????? | 17 +++--- >>> ? drivers/gpu/drm/nouveau/nouveau_sched.c??? | 15 +++-- >>> ? drivers/gpu/drm/panfrost/panfrost_job.c??? | 20 ++++--- >>> ? drivers/gpu/drm/panthor/panthor_mmu.c????? | 16 +++-- >>> ? drivers/gpu/drm/panthor/panthor_sched.c??? | 29 +++++---- >>> ? drivers/gpu/drm/scheduler/sched_main.c???? | 50 ++++++---------- >>> ? drivers/gpu/drm/v3d/v3d_sched.c??????????? | 68 +++++++++-------- >>> ----- >>> ? drivers/gpu/drm/xe/xe_execlist.c?????????? | 16 +++-- >>> ? drivers/gpu/drm/xe/xe_gpu_scheduler.c????? | 17 +++++- >>> ? include/drm/gpu_scheduler.h??????????????? | 37 ++++++++++-- >>> ? 14 files changed, 206 insertions(+), 151 deletions(-) >>> >> >> [...] >> >>> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c >>> b/drivers/gpu/drm/panthor/panthor_sched.c >>> index 5844a7f639e0..44713cfdcd74 100644 >>> --- a/drivers/gpu/drm/panthor/panthor_sched.c >>> +++ b/drivers/gpu/drm/panthor/panthor_sched.c >>> @@ -3284,6 +3284,22 @@ static struct panthor_queue * >>> ? group_create_queue(struct panthor_group *group, >>> ?? ?? const struct drm_panthor_queue_create *args) >>> ? { >>> + const struct drm_sched_init_args sched_args = { >>> + .ops = &panthor_queue_sched_ops, >>> + .submit_wq = group->ptdev->scheduler->wq, >>> + .num_rqs = 1, >>> + /* >>> + * 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. >>> + */ >>> + .credit_limit = args->ringbuf_size / sizeof(u64), >>> + .timeout = msecs_to_jiffies(JOB_TIMEOUT_MS), >>> + .timeout_wq = group->ptdev->reset.wq, >>> + .name = "panthor-queue", >>> + .dev = group->ptdev->base.dev >>> + }; >>> ?? struct drm_gpu_scheduler *drm_sched; >>> ?? struct panthor_queue *queue; >>> ?? int ret; >>> @@ -3354,17 +3370,8 @@ group_create_queue(struct panthor_group >>> *group, >>> ?? if (ret) >>> ?? goto err_free_queue; >>> >>> - /* >>> - * 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); >>> + >> >> Please don't use multiple blank lines. >> >>> + ret = drm_sched_init(&queue->scheduler, &sched_args); >>> ?? if (ret) >>> ?? goto err_free_queue; >>> >>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c >>> b/drivers/gpu/drm/scheduler/sched_main.c >>> index a48be16ab84f..6295b2654a7c 100644 >>> --- a/drivers/gpu/drm/scheduler/sched_main.c >>> +++ b/drivers/gpu/drm/scheduler/sched_main.c >>> @@ -1244,40 +1244,24 @@ static void drm_sched_run_job_work(struct >>> work_struct *w) >>> ?? * drm_sched_init - Init a gpu scheduler instance >>> ?? * >>> ?? * @sched: scheduler instance >>> - * @ops: backend operations for this scheduler >>> - * @submit_wq: workqueue to use for submission. If NULL, an >>> ordered wq is >>> - * ?????? allocated and used >>> - * @num_rqs: number of runqueues, one for each priority, up to >>> DRM_SCHED_PRIORITY_COUNT >>> - * @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: target &struct device >>> + * @args: scheduler initialization arguments >>> ?? * >>> ?? * Return 0 on success, otherwise error code. >>> ?? */ >>> -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) >>> +int drm_sched_init(struct drm_gpu_scheduler *sched, const struct >>> drm_sched_init_args *args) >>> ? { >>> ?? int i; >>> >>> - sched->ops = ops; >>> - sched->credit_limit = credit_limit; >>> - sched->name = name; >>> - sched->timeout = timeout; >>> - sched->timeout_wq = timeout_wq ? : system_wq; >>> - sched->hang_limit = hang_limit; >>> - sched->score = score ? score : &sched->_score; >>> - sched->dev = dev; >>> + sched->ops = args->ops; >>> + sched->credit_limit = args->credit_limit; >>> + sched->name = args->name; >>> + sched->timeout = args->timeout; >>> + sched->timeout_wq = args->timeout_wq ? : system_wq; >>> + sched->hang_limit = args->hang_limit; >>> + sched->score = args->score ? args->score : &sched->_score; >> >> Could we keep it consistent and use the Elvis Operator here as well? >> Just like `sched->timeout_wq`. > > This is literally just the old code. > > And if at all, this insanely stupid GCC extension should not be used. > It's one of the typical compiler people rampages that make the C > language so terrible.Not a problem to me, we can remove the Elvis Operator from `sched- >timeout_wq`. My idea is just to do things consistently in variable assignment. Best Regards, - Ma?ra