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-5C7GfCeVMHo@public.gmane.org>
> 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 :
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.

>
>
>> @@ -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?

>
>
>> +		/* default is normal */
>> +		args->in.priority = TTM_BO_PRIORITY_NORMAL;
>> +	} else if (args->in.priority > TTM_MAX_BO_PRIORITY) {
>> +		args->in.priority = TTM_MAX_BO_PRIORITY;
>> +		DRM_ERROR("priority specified from user space is over MAX priority\n");
> This must be DRM_DEBUG, or buggy/malicious userspace can spam dmesg.
Will change.

>
>
>> @@ -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.
"

So my original purpose is directly use Priority enum defined in PAL, 
like this:
  "
/// Specifies Base Level priority per GPU memory allocation as a hint to 
the memory manager in the event it needs to
/// select allocations to page out of their preferred heaps.
enum class GpuMemPriority : uint32
{
     Unused    = 0x0,  ///< Indicates that the allocation is not 
currently being used at all, and should be the first
                       ///  choice to be paged out.
     VeryLow   = 0x1,  ///< Lowest priority to keep in its preferred heap.
     Low       = 0x2,  ///< Low priority to keep in its preferred heap.
     Normal    = 0x3,  ///< Normal priority to keep in its preferred heap.
     High      = 0x4,  ///< High priority to keep in its preferred heap 
(e.g., render targets).
     VeryHigh  = 0x5,  ///< Highest priority to keep in its preferred 
heap.  Last choice to be paged out (e.g., page
                       ///  tables, displayable allocations).
     Count
};
"

If according your idea, we will need to convert it again when hooking 
linux implementation.
So what do think we still use unsigned?

>
>
>> @@ -304,6 +315,7 @@ int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>   
>>   	/* create a gem object to contain this object in */
>>   	r = amdgpu_gem_object_create(adev, args->size, 0, AMDGPU_GEM_DOMAIN_CPU,
>> +				     TTM_BO_PRIORITY_NORMAL,
>>   				     0, ttm_bo_type_device, NULL, &gobj);
> Should the userptr ioctl also allow setting the priority?

We can.

>
>
>> 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.

-David

>
>