Re: [PATCH 2/2] drm/amdgpu: allocate entities on demand

[Luben Tuikov <luben.tuikov@amd.com>]

On 2020-01-22 4:25 a.m., Nirmoy wrote:
> Hi Luben,
> 
> Thanks for reviewing.  Was expecting lot from vim auto-indent :/

Yes, no problem. It's a learning experience for me as well.

If you use Emacs, pressing TAB anywhere on a line, will indent
it according to the mode. (It runs the (c-indent-line-or-region)
Lisp function, when the major mode is "C".) Pressing TAB anywhere
in a line which has already been indented according to mode, does
nothing.

You can also indent the whole file (region) by pressing C-M-\.

> 
> On 1/22/20 1:07 AM, Luben Tuikov wrote:
>> On 2020-01-21 11:50 a.m., Nirmoy Das wrote:
>>
>>> -		switch (i) {
>>> +	priority = (ctx->override_priority == DRM_SCHED_PRIORITY_UNSET) ?
>>> +				ctx->init_priority : ctx->override_priority;
>> You don't need parenthesis around the relational equality operator used here.
>> It has higher precedence than the ternary conditional, in which it is embedded.
> 
> Parenthesis makes it more human readable.
> 
> 
>>> +	int r;
>>> +
>>> +	if (priority < 0 || priority >= DRM_SCHED_PRIORITY_MAX)
>>> +		return -EINVAL;
>> This shouldn't be possible since it is an enum...
> What should not be possible ?
>> But why not do this check in "amdgpu_ctx_priority_permit()"
>> which is introduced by this patchset and used in the imediately
>> following line?
> Makes sense.
>>
>> Or perhaps fix up amdgpu_to_sched_priority() where it is massaged
>> from the ioctl argument which is an integer.
>>
>> On a side note: I noticed that the enum drm_sched_priority
>> has no DRM_SCHED_PRIORITY_NONE.
>>
>> I've found value in setting the first value of an enum to
>> "_NONE" (unless zero actually has a meaning as set by HW/etc., anyway).
>> Enum drm_sched_priority starts with "_MIN" and "_LOW" which
>> are both set to the same (zero) value.
>>
>> So having DRM_SCHED_PRIORITY_NONE, could be used to denote
>> that no priority was given and any is fine, or to mean
>> that if the priority was given out of range, set it to "none",
>> to mean pick any.
> 
> Not sure about if HW accepts 0.

Exactly! We use "0" to mean, as I described above,
"that no priority was given and any is fine, or to mean
that if the priority given [by the user] is out of range,
set it to ``none'' to mean pick any", that is so that
we actually input to hardware a meaningful value.

It's just a conveyance.

Using/adding "_NONE" in enums (as usually the first enumerated literal (0))
really changes algorithms, as it makes representing concepts easier.
(Kind of like when we said, let i = sqrt(-1).)

Regards,
Luben
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx