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

["Maíra Canal" <mcanal@igalia.com>]

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@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