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.z...@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 :
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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
