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

[Nirmoy]

Hi Luben,

Thanks for reviewing.  Was expecting lot from vim auto-indent :/

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.


Regards,

Nirmoy