[RFC PATCH 2/4] drm/scheduler: implement a function to modify sched list

[Christian König]

Am 02.03.20 um 21:47 schrieb Luben Tuikov:
> On 2020-02-28 2:47 a.m., Christian König wrote:
>> Am 28.02.20 um 06:08 schrieb Luben Tuikov:
>>> On 2020-02-27 4:40 p.m., Nirmoy Das wrote:
>>>> [SNIP]
>>>> +	if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0])))
>>>> +		return -EINVAL;
>>> This seems unmaintainable. I'd write it in its natural form:
>> This is probably just copy & pasted from the init function and complete
>> overkill here.
> It should be an easy rejection then. Statements like this make
> the code unmaintainable. Regardless of whether it was copy-and-pasted
> I wanted to emphasize the lack of simplification of what
> was being done.

The problem is even deeper. As you noticed as well this is checking for 
in kernel coding error and not application errors.

That check shouldn't have been in the init function in the first place, 
but nobody had time to look into that so far.

> We need to put intention and sense into what we're doing, scrutinizing
> every line we put into a patch. This is why I suggested
> the simplification here:
>
>>> int drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
>>> 				  struct drm_gpu_scheduler **sched_list,
>>> 				  unsigned int num_sched_list)
>>> {
>>> 	if (entity && sched_list && (num_sched_list == 0 || sched_list[0] != NULL)) {
>>> 		entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
>>> 		entity->num_sched_list = num_sched_list;
>>> 		return 0;
>>> 	} else {
>>> 		return -EINVAL;
>>> 	}
>>> }
>> Actually that's a rather bad idea. Error handling should always be in
> I actually don't think that it is a "rather bad idea". At all.
> I actually think that it makes this leaf function more clear to
> understand as the conditional would read like a sentence in prose.

The condition is indeed easier to read, but for the sacrifice of earlier 
return and keeping prerequisite checking out of the code.

> [SNIP]
>> What we should do instead is just: WARN_ON(!num_sched_list || !sched_list);
> Again, what does that *mean*? What does the check mean and what
> does the num_sched_list == 0 or sched_list == NULL mean?
> And how did we get into a situation like this where either or both
> could be nil?

It's an in kernel coding error to do this. The caller should at least 
always provide a list with some entries in it.

A WARN_ON() is appropriate since it helps to narrows down the incorrect 
behavior following from that.

> Wouldn't it be better to simplify or re-architecture this (we only recently
> decided to hide physical rings from user-space), so that the code
> is elegant (meaning no if-else) yet flexible and straightforward?

That was not recently at all, hiding physical rings was done nearly 5 
years ago shortly after the driver was initially released.

>>> Why not fix the architecture so that this is simply copied?
>> We had that and moved away from it because the scheduler list is
>> actually const and shouldn't be allocated with each entity (which we can
>> easily have thousands of).
> I think that peppering the code with if-else conditionals
> everywhere as these patch-series into the DRM scheduler have been,
> would make the code unmaintainable in the long run.

That's something I can agree on. Using a switch to map the priority to 
the backend implementation seems like the best idea to me.

E.g. function amdgpu_to_sched_priority() should not only map the IOCTL 
values to the scheduler values, but also return the array which hw rings 
to use.

Regards,
Christian.