[PATCH] drm/amdgpu: enable bo priority setting from user space

[Michel Dänzer]

On 2019-03-07 11:48 a.m., zhoucm1 wrote:
> On 2019年03月07日 17:55, Michel Dänzer wrote:
>> On 2019-03-07 10:15 a.m., Chunming Zhou wrote:
>>> Signed-off-by: Chunming Zhou <david1.zhou at amd.com>
>> Please provide corresponding UMD patches showing how this is to be used.
> spec is here:
> https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html,
> please search "|VkMemoryPriorityAllocateInfoEXT|".
> 
> Fortunately, Windows guy already implemented it before, otherwise, I
> cannot find ready code on opensource, I hate this chicken first and egg
> first question :

Whether you like it or not, the rule documented in
Documentation/gpu/drm-uapi.rst is clear: New UAPI cannot go upstream
before corresponding userspace patches have been reviewed. This is
important to avoid adding broken/unmaintainable UAPI.


> https://github.com/GPUOpen-Drivers/pal/blob/dev/src/core/gpuMemory.cpp,
> please search "createInfo.priority".
> https://github.com/GPUOpen-Drivers/pal/blob/dev/inc/core/palGpuMemory.h,
> priority definition is here.

These don't show how the priority field of struct
drm_amdgpu_gem_create_in is used.


>>> @@ -229,6 +231,14 @@ int amdgpu_gem_create_ioctl(struct drm_device
>>> *dev, void *data,
>>>       if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK)
>>>           return -EINVAL;
>>>   +    /* check priority */
>>> +    if (args->in.priority == 0) {
>> Did you verify that this is 0 with old userspace compiled against struct
>> drm_amdgpu_gem_create_in without the priority field?
> Without priority field, I don't think we can check here. Do you mean we
> need to add a new args struct?

I don't think so. Presumably this will be initialized to 0 if userspace
doesn't pass in a value, but this needs to be verified.


>>> @@ -252,6 +262,7 @@ int amdgpu_gem_create_ioctl(struct drm_device
>>> *dev, void *data,
>>>         r = amdgpu_gem_object_create(adev, size, args->in.alignment,
>>>                        (u32)(0xffffffff & args->in.domains),
>>> +                     args->in.priority - 1,
>>>                        flags, ttm_bo_type_device, resv, &gobj);
>> It might be less confusing to subtract 1 after checking against
>> TTM_MAX_BO_PRIORITY instead of here. Still kind of confusing though. How
>> about this instead:
>>
>> Make the priority field of struct drm_amdgpu_gem_create_in signed. In
>> amdgpu_gem_create_ioctl, clamp the priority to the supported range:
>>
>>     args->in.priority += TTM_BO_PRIORITY_NORMAL;
>>     args->in.priority = max(args->in.priority, 0);
>>     args->in.priority = min(args->in.priority,
>>                 TTM_BO_PRIORITY_NORMAL - 1);
>>
>> This way userspace doesn't need to do a weird mapping of the priority
>> values (where 0 and 2 have the same meaning), and the range of supported
>> priorities could at least theoretically be extended without breaking
>> userspace.
> First, I want to explain a bit the priority value from vulkan:
> "    From Vulkan Spec, 0.0 <= value <= 1.0, and the granularity of the
> priorities is implementation-dependent.
>      One thing Spec forced is that if VkMemoryPriority not specified as
> default behavior, it is as if the
>      priority value is 0.5. Our strategy is that map 0.5 to
> GpuMemPriority::Normal-GpuMemPriorityOffset::Offset0,
>      which is consistent to MemoryPriorityDefault. We adopts
> GpuMemPriority::VeryLow, GpuMemPriority::Low,
>      GpuMemPriority::Normal, GpuMemPriority::High, 4 priority grades,
> each of which contains 8 steps of offests.
>      This maps [0.0-1.0) to totally 32 steps. Finally, 1.0 maps to
> GpuMemPriority::VeryHigh.
> "

In that case, I'd suggest making the priority field signed, and mapping
the full integer range to the range of supported priorities:

* 0 maps to TTM_BO_PRIORITY_NORMAL
* INT_MIN maps to TTM_BO_PRIORITY_VERYLOW
* INT_MAX maps to TTM_BO_PRIORITY_VERYHIGH

Intermediate values are interpolated appropriately to the supported
range of priorities.


This way, the Vulkan float priority value can easily be converted to the
integer priority value, and the actual range of priorities used on the
kernel side can be changed arbitrarily without breaking userspace.


> So my original purpose is directly use Priority enum defined in PAL,
> [...]

I don't think it's a good idea to let PAL dictate UAPI.


>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index fd9c4beeaaa4..c85304e03021 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -494,8 +494,9 @@ static int amdgpu_bo_do_create(struct
>>> amdgpu_device *adev,
>>>         bo->tbo.bdev = &adev->mman.bdev;
>>>       amdgpu_bo_placement_from_domain(bo, bp->domain);
>>> +    bo->tbo.priority = bp->priority;
>>>       if (bp->type == ttm_bo_type_kernel)
>>> -        bo->tbo.priority = 1;
>>> +        bo->tbo.priority = TTM_BO_PRIORITY_VERYHIGH;
>>     if (bp->type == ttm_bo_type_kernel)
>>         bo->tbo.priority = TTM_BO_PRIORITY_VERYHIGH;
>>     else
>>         bo->tbo.priority = bp->priority;
>>
>> would be clearer I think.
> Agree.

Another question is whether userspace should be allowed to set the same
priority as the kernel? I'm inclined to say no, there should be a
separate priority for the kernel.


-- 
Earthling Michel Dänzer               |              https://www.amd.com
Libre software enthusiast             |             Mesa and X developer