[PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init

[Luben Tuikov]

On 2020-03-05 16:13, Nirmoy wrote:
> 
> On 3/5/20 7:42 PM, Luben Tuikov wrote:
>>
>>> A quick search leads me amdgpu_sched_ioctl() which is using 
>>> DRM_SCHED_PRIORITY_INVALID
>>>
>>> to indicate a invalid value from userspace. I don't know much about drm 
>>> api to suggest any useful
>>>
>>> changes regarding this. But again this isn't in the scope of this patch 
>>> series.
>> I'm not asking you to eliminate "DRM_SCHED_PRIORITY_INVALID".
>> Oh wait! You forgot what I suggested in a previous review on
>> how to fix enum drm_sched_priority, did you? :-D Search
>> for that email.
> 
> Let me quote[1]:
> 
> "
> 
> Also consider changing to this:
> 
> 
> enum drm_sched_priority {
>         DRM_SCHED_PRIORITY_UNSET,
> --------DRM_SCHED_PRIORITY_INVALID,--------<--- remove
>         DRM_SCHED_PRIORITY_MIN,
>         DRM_SCHED_PRIORITY_LOW = DRM_SCHED_PRIORITY_MIN,
>         DRM_SCHED_PRIORITY_NORMAL,
>         DRM_SCHED_PRIORITY_HIGH_SW,
>         DRM_SCHED_PRIORITY_HIGH_HW,
>         DRM_SCHED_PRIORITY_KERNEL,
>         DRM_SCHED_PRIORITY_MAX,
> };
> 
> We should never have an "invalid priority", just ignored priority. :-)
> Second, having 0 as UNSET gives you easy priority when you set
> map[0] = normal_priority, as memory usually comes memset to 0.
> IOW, you'd not need to check against UNSET, and simply use
> the default [0] which you'd set to normal priority.
> 
> "
> 

Excellent! You found the email.

Yes, there is no reason to represent and carry around
an "invalid" priority--it simply means that the *input*
was invalid, but the priority would always be set to
something valid and meaningful: the input was invalid,
not the priority.

If you encounter an invalid input, simply set the priority
to unset, or even better, minimum. From then on,
any further map look-ups, would be a trivial linear
transformation.

It is generally a good idea to represent 0, default software
memory initialization, to "*_NONE", in enums. Then start the
meaningful labels at 1, so one can distinguish between
"this data has never been touched by software" and "this was
set to something by someone after memory initialization".
So you should rename "_UNSET" to "_NONE". It *was* set (by memset()),
just not meaningfully so.

To look like this:

enum drm_sched_priority {
	DRM_SCHED_PRIORITY_NONE,
	DRM_SCHED_PRIORITY_LOW,
	DRM_SCHED_PRIORITY_NORMAL,
	DRM_SCHED_PRIORITY_HIGH,
	DRM_SCHED_PRIORITY_HIGHER,
	DRM_SCHED_PRIORITY_KERNEL,

	DRM_SCHED_PRIORITY_SIZE  /* intentionally no comma */
};

Valid values are "*_NONE" to "*_KERNEL".
Meaningful values are "*_LOW" to "*_KERNEL". (i.e. intentional)
"*_SIZE" you use to allocate storage (array, for instance)
of size which can index all valid priorities.

Then you can do:

enum drm_sched_priority some_map[XYZ_DOMAIN_PRIO_SIZE] = {
	[0 ... XYZ_DOMAIN_PRIO_SIZE - 1] = XYZ_DOMAIN_PRIO_DEFAULT,
	[A]                              = XYZ_DOMAIN_PRIO_NORMAL,
	[B]                              = XYZ_DOMAIN_PRIO_HIGH,
};

Where 0 <= A, B < XYZ_DOMAIN_PRIO_SIZE.

This also allows you to chain those transformations, similarly.

Regards,
Luben


> I guess understood it wrong.
> 
> 
> [1]https://www.mail-archive.com/amd-gfx@lists.freedesktop.org/msg45621.html <https://www.mail-archive.com/amd-gfx@lists.freedesktop.org/msg45621.html>
> 
> 
> Regards,
> 
> Nirmoy
>