Nouveau - Jan 2025 - [PATCH] drm/sched: Use struct for drm_sched_init() params

Hi Philipp,

On 23/01/25 09:13, Philipp Stanner wrote:
> On Thu, 2025-01-23 at 08:10 -0300, Ma?ra Canal wrote:
>> Hi Philipp,
>>
>> On 23/01/25 05:10, Philipp Stanner wrote:
>>> 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 at kernel.org>
>>
>> [...]
>>
>>>>
>>>>> 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.
>>>
>>
>> This is my proposal (just a quick draft, please check if it
>> compiles):
>>
>> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c
>> b/drivers/gpu/drm/v3d/v3d_sched.c
>> index 961465128d80..7cc45a0c6ca0 100644
>> --- a/drivers/gpu/drm/v3d/v3d_sched.c
>> +++ b/drivers/gpu/drm/v3d/v3d_sched.c
>> @@ -820,67 +820,62 @@ static const struct drm_sched_backend_ops
>> v3d_cpu_sched_ops = {
>>  ???????? .free_job = v3d_cpu_job_free
>>  ? };
>>
>> +static int
>> +v3d_sched_queue_init(struct v3d_dev *v3d, enum v3d_queue queue,
>> +??????????????????? const struct drm_sched_backend_ops *ops, const
> 
> Is it a queue, though?
In V3D, we use the abstraction of a queue for everything related to job
submission. For each queue, we have a scheduler instance, a different
IOCTL and such. The queues work independently and the synchronization
between them can be done through syncobjs.
> 
> How about _v3d_sched_init()?
> 
I'd prefer if you use a function name related to "queue", as it
would
make more sense semantically.

Best Regards,
- Ma?ra
> P.
> 
>> char
>> *name)
>> +{
>> +?????? struct drm_sched_init_params params = {
>> +?????????????? .submit_wq = NULL,
>> +?????????????? .num_rqs = DRM_SCHED_PRIORITY_COUNT,
>> +?????????????? .credit_limit = 1,
>> +?????????????? .hang_limit = 0,
>> +?????????????? .timeout = msecs_to_jiffies(500),
>> +?????????????? .timeout_wq = NULL,
>> +?????????????? .score = NULL,
>> +?????????????? .dev = v3d->drm.dev,
>> +?????? };
>> +
>> +?????? params.ops = ops;
>> +?????? params.name = name;
>> +
>> +?????? return drm_sched_init(&v3d->queue[queue].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_sched_queue_init(v3d, V3D_BIN,
&v3d_bin_sched_ops,
>> +????????????????????????????????? "v3d_bin");
>>  ???????? 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_sched_queue_init(v3d, V3D_RENDER,
>> &v3d_render_sched_ops,
>> +????????????????????????????????? "v3d_render");
>>  ???????? if (ret)
>>  ???????????????? goto fail;
>>
>> [...]
>>
>> At least for me, this looks much simpler than one function for each
>> V3D queue.
>>
>> Best Regards,
>> - Ma?ra
>>
>>> P.
>>>
>>>
>>>>
>>>> Best Regards,
>>>> - Ma?ra
>>>>
>>
>