This series improves performance for the cases when there is not enough VRAM for all buffers.
First of all, I'd like to mention that if you set both VRAM and GTT domains for a buffer, you pretty much say you don't care where the buffer ends up. It usually makes the performance even worse.
This work was largely benchmark-driven and I tried a lot of ideas before I found out which ones work. The patches describe what they do and they're quite simple, so I'll just share the results here.
Card: Evergreen Redwood (HD 5670), 512 MB of VRAM Test: Unigine Heaven 4.0, High settings
1) 1280x720, 4x MSAA, need 525 MB of VRAM
Without patches: 16.6 FPS With patches: 16.6 FPS Improvement: 0 %
2) 1600x900, 4x MSAA, need 642 MB of VRAM
Without patches: 7.1 FPS With patches: 9.7 FPS Improvement: 36 %
3) 1920x1080, 4x MSAA, need 743 MB of VRAM
Without patches: 3.7 FPS With patches: 5.6 FPS Improvement: 51 %
4) 1600x900, 8x MSAA, need 838 MB of VRAM Without patches: 2.9 FPS With patches: 4.6 FPS Improvement: 58 %
These results don't change if you run the benchmark several times, which proves the improvement is stable.
To conclude this, here are ideas for future work:
1) Add virtual memory support for VRAM. Our GPUs support virtual memory, which not only solves fragmentation issues, but it also allows each buffer to be partially in VRAM and partially in GTT, which becomes more important with large buffers like 100 MB. Moving whole buffers back and forth between VRAM and GTT is inefficient if you can do it at page granularity. Also, due to fragmentation, we can never really use all of VRAM, but only about 90-95%.
2) Add support for uncached GTT. I think it should improve performance for dGPUs under memory pressure, but some testing needs to be done to confirm that. Uncached GTT doesn't seem to work for me on Evergreen, but it's said to be working on some later chips.
The patches for Mesa will follow later today. Please review.
Marek
From: Marek Olšák marek.olsak@amd.com
When passing buffers between processes, the receiving process needs to know the original buffer domain, so that it doesn't accidentally move the buffer.
Signed-off-by: Marek Olšák marek.olsak@amd.com --- drivers/gpu/drm/radeon/radeon.h | 3 +++ drivers/gpu/drm/radeon/radeon_drv.c | 3 ++- drivers/gpu/drm/radeon/radeon_gem.c | 30 ++++++++++++++++++++++++++++++ drivers/gpu/drm/radeon/radeon_kms.c | 1 + drivers/gpu/drm/radeon/radeon_object.c | 3 +++ include/uapi/drm/radeon_drm.h | 12 ++++++++++++ 6 files changed, 51 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index a415f8e..3f10782 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -454,6 +454,7 @@ struct radeon_bo { /* Protected by gem.mutex */ struct list_head list; /* Protected by tbo.reserved */ + u32 initial_domain; u32 placements[3]; struct ttm_placement placement; struct ttm_buffer_object tbo; @@ -2114,6 +2115,8 @@ int radeon_gem_wait_idle_ioctl(struct drm_device *dev, void *data, struct drm_file *filp); int radeon_gem_va_ioctl(struct drm_device *dev, void *data, struct drm_file *filp); +int radeon_gem_op_ioctl(struct drm_device *dev, void *data, + struct drm_file *filp); int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp); int radeon_gem_set_tiling_ioctl(struct drm_device *dev, void *data, struct drm_file *filp); diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index 84a1bbb7..4392b7c 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -79,9 +79,10 @@ * 2.35.0 - Add CIK macrotile mode array query * 2.36.0 - Fix CIK DCE tiling setup * 2.37.0 - allow GS ring setup on r6xx/r7xx + * 2.38.0 - RADEON_GEM_OP (GET_INITIAL_DOMAIN, SET_INITIAL_DOMAIN) */ #define KMS_DRIVER_MAJOR 2 -#define KMS_DRIVER_MINOR 37 +#define KMS_DRIVER_MINOR 38 #define KMS_DRIVER_PATCHLEVEL 0 int radeon_driver_load_kms(struct drm_device *dev, unsigned long flags); int radeon_driver_unload_kms(struct drm_device *dev); diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index b96c819..d7890f2 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -533,6 +533,36 @@ out: return r; }
+int radeon_gem_op_ioctl(struct drm_device *dev, void *data, + struct drm_file *filp) +{ + struct drm_radeon_gem_op *args = data; + struct drm_gem_object *gobj; + struct radeon_bo *robj; + + gobj = drm_gem_object_lookup(dev, filp, args->handle); + if (gobj == NULL) { + return -ENOENT; + } + robj = gem_to_radeon_bo(gobj); + + switch (args->op) { + case RADEON_GEM_OP_GET_INITIAL_DOMAIN: + args->value = robj->initial_domain; + break; + case RADEON_GEM_OP_SET_INITIAL_DOMAIN: + robj->initial_domain = args->value & (RADEON_GEM_DOMAIN_VRAM | + RADEON_GEM_DOMAIN_GTT | + RADEON_GEM_DOMAIN_CPU); + break; + default: + ; /* do nothing */ + } + + drm_gem_object_unreference_unlocked(gobj); + return 0; +} + int radeon_mode_dumb_create(struct drm_file *file_priv, struct drm_device *dev, struct drm_mode_create_dumb *args) diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index baff98b..0b631eb 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -814,5 +814,6 @@ const struct drm_ioctl_desc radeon_ioctls_kms[] = { DRM_IOCTL_DEF_DRV(RADEON_GEM_GET_TILING, radeon_gem_get_tiling_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(RADEON_GEM_BUSY, radeon_gem_busy_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(RADEON_GEM_VA, radeon_gem_va_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(RADEON_GEM_OP, radeon_gem_op_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW), }; int radeon_max_kms_ioctl = DRM_ARRAY_SIZE(radeon_ioctls_kms); diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index 08595cf..dd12bb4 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -145,6 +145,9 @@ int radeon_bo_create(struct radeon_device *rdev, bo->surface_reg = -1; INIT_LIST_HEAD(&bo->list); INIT_LIST_HEAD(&bo->va); + bo->initial_domain = domain & (RADEON_GEM_DOMAIN_VRAM | + RADEON_GEM_DOMAIN_GTT | + RADEON_GEM_DOMAIN_CPU); radeon_ttm_placement_from_domain(bo, domain); /* Kernel allocation are uninterruptible */ down_read(&rdev->pm.mclk_lock); diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_drm.h index 1cf18b4..cb5c93a 100644 --- a/include/uapi/drm/radeon_drm.h +++ b/include/uapi/drm/radeon_drm.h @@ -510,6 +510,7 @@ typedef struct { #define DRM_RADEON_GEM_GET_TILING 0x29 #define DRM_RADEON_GEM_BUSY 0x2a #define DRM_RADEON_GEM_VA 0x2b +#define DRM_RADEON_GEM_OP 0x2c
#define DRM_IOCTL_RADEON_CP_INIT DRM_IOW( DRM_COMMAND_BASE + DRM_RADEON_CP_INIT, drm_radeon_init_t) #define DRM_IOCTL_RADEON_CP_START DRM_IO( DRM_COMMAND_BASE + DRM_RADEON_CP_START) @@ -552,6 +553,7 @@ typedef struct { #define DRM_IOCTL_RADEON_GEM_GET_TILING DRM_IOWR(DRM_COMMAND_BASE + DRM_RADEON_GEM_GET_TILING, struct drm_radeon_gem_get_tiling) #define DRM_IOCTL_RADEON_GEM_BUSY DRM_IOWR(DRM_COMMAND_BASE + DRM_RADEON_GEM_BUSY, struct drm_radeon_gem_busy) #define DRM_IOCTL_RADEON_GEM_VA DRM_IOWR(DRM_COMMAND_BASE + DRM_RADEON_GEM_VA, struct drm_radeon_gem_va) +#define DRM_IOCTL_RADEON_GEM_OP DRM_IOWR(DRM_COMMAND_BASE + DRM_RADEON_GEM_OP, struct drm_radeon_gem_op)
typedef struct drm_radeon_init { enum { @@ -884,6 +886,16 @@ struct drm_radeon_gem_pwrite { uint64_t data_ptr; };
+/* Sets or returns a value associated with a buffer. */ +struct drm_radeon_gem_op { + uint32_t handle; /* buffer */ + uint32_t op; /* RADEON_GEM_OP_* */ + uint64_t value; /* input or return value */ +}; + +#define RADEON_GEM_OP_GET_INITIAL_DOMAIN 0 +#define RADEON_GEM_OP_SET_INITIAL_DOMAIN 1 + #define RADEON_VA_MAP 1 #define RADEON_VA_UNMAP 2
Am 24.02.2014 16:20, schrieb Marek Olšák:
When the comment is correct and initial_domain is protected by reserving the BO you should actually reserve/unreserve it here. On the other hand locking might not be necessary.
Should probably return -EINVAL here.
From: Marek Olšák marek.olsak@amd.com
When passing buffers between processes, the receiving process needs to know the original buffer domain, so that it doesn't accidentally move the buffer.
Signed-off-by: Marek Olšák marek.olsak@amd.com --- drivers/gpu/drm/radeon/radeon.h | 3 +++ drivers/gpu/drm/radeon/radeon_drv.c | 3 ++- drivers/gpu/drm/radeon/radeon_gem.c | 36 ++++++++++++++++++++++++++++++++++ drivers/gpu/drm/radeon/radeon_kms.c | 1 + drivers/gpu/drm/radeon/radeon_object.c | 3 +++ include/uapi/drm/radeon_drm.h | 12 ++++++++++++ 6 files changed, 57 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index a415f8e..3f10782 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -454,6 +454,7 @@ struct radeon_bo { /* Protected by gem.mutex */ struct list_head list; /* Protected by tbo.reserved */ + u32 initial_domain; u32 placements[3]; struct ttm_placement placement; struct ttm_buffer_object tbo; @@ -2114,6 +2115,8 @@ int radeon_gem_wait_idle_ioctl(struct drm_device *dev, void *data, struct drm_file *filp); int radeon_gem_va_ioctl(struct drm_device *dev, void *data, struct drm_file *filp); +int radeon_gem_op_ioctl(struct drm_device *dev, void *data, + struct drm_file *filp); int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp); int radeon_gem_set_tiling_ioctl(struct drm_device *dev, void *data, struct drm_file *filp); diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index 84a1bbb7..4392b7c 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -79,9 +79,10 @@ * 2.35.0 - Add CIK macrotile mode array query * 2.36.0 - Fix CIK DCE tiling setup * 2.37.0 - allow GS ring setup on r6xx/r7xx + * 2.38.0 - RADEON_GEM_OP (GET_INITIAL_DOMAIN, SET_INITIAL_DOMAIN) */ #define KMS_DRIVER_MAJOR 2 -#define KMS_DRIVER_MINOR 37 +#define KMS_DRIVER_MINOR 38 #define KMS_DRIVER_PATCHLEVEL 0 int radeon_driver_load_kms(struct drm_device *dev, unsigned long flags); int radeon_driver_unload_kms(struct drm_device *dev); diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index b96c819..9863ca7 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -533,6 +533,42 @@ out: return r; }
+int radeon_gem_op_ioctl(struct drm_device *dev, void *data, + struct drm_file *filp) +{ + struct drm_radeon_gem_op *args = data; + struct drm_gem_object *gobj; + struct radeon_bo *robj; + int r; + + gobj = drm_gem_object_lookup(dev, filp, args->handle); + if (gobj == NULL) { + return -ENOENT; + } + robj = gem_to_radeon_bo(gobj); + r = radeon_bo_reserve(robj, false); + if (unlikely(r)) + goto out; + + switch (args->op) { + case RADEON_GEM_OP_GET_INITIAL_DOMAIN: + args->value = robj->initial_domain; + break; + case RADEON_GEM_OP_SET_INITIAL_DOMAIN: + robj->initial_domain = args->value & (RADEON_GEM_DOMAIN_VRAM | + RADEON_GEM_DOMAIN_GTT | + RADEON_GEM_DOMAIN_CPU); + break; + default: + r = -EINVAL; + } + + radeon_bo_unreserve(robj); +out: + drm_gem_object_unreference_unlocked(gobj); + return r; +} + int radeon_mode_dumb_create(struct drm_file *file_priv, struct drm_device *dev, struct drm_mode_create_dumb *args) diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index baff98b..0b631eb 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -814,5 +814,6 @@ const struct drm_ioctl_desc radeon_ioctls_kms[] = { DRM_IOCTL_DEF_DRV(RADEON_GEM_GET_TILING, radeon_gem_get_tiling_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(RADEON_GEM_BUSY, radeon_gem_busy_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(RADEON_GEM_VA, radeon_gem_va_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(RADEON_GEM_OP, radeon_gem_op_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW), }; int radeon_max_kms_ioctl = DRM_ARRAY_SIZE(radeon_ioctls_kms); diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index 08595cf..dd12bb4 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -145,6 +145,9 @@ int radeon_bo_create(struct radeon_device *rdev, bo->surface_reg = -1; INIT_LIST_HEAD(&bo->list); INIT_LIST_HEAD(&bo->va); + bo->initial_domain = domain & (RADEON_GEM_DOMAIN_VRAM | + RADEON_GEM_DOMAIN_GTT | + RADEON_GEM_DOMAIN_CPU); radeon_ttm_placement_from_domain(bo, domain); /* Kernel allocation are uninterruptible */ down_read(&rdev->pm.mclk_lock); diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_drm.h index 1cf18b4..cb5c93a 100644 --- a/include/uapi/drm/radeon_drm.h +++ b/include/uapi/drm/radeon_drm.h @@ -510,6 +510,7 @@ typedef struct { #define DRM_RADEON_GEM_GET_TILING 0x29 #define DRM_RADEON_GEM_BUSY 0x2a #define DRM_RADEON_GEM_VA 0x2b +#define DRM_RADEON_GEM_OP 0x2c
#define DRM_IOCTL_RADEON_CP_INIT DRM_IOW( DRM_COMMAND_BASE + DRM_RADEON_CP_INIT, drm_radeon_init_t) #define DRM_IOCTL_RADEON_CP_START DRM_IO( DRM_COMMAND_BASE + DRM_RADEON_CP_START) @@ -552,6 +553,7 @@ typedef struct { #define DRM_IOCTL_RADEON_GEM_GET_TILING DRM_IOWR(DRM_COMMAND_BASE + DRM_RADEON_GEM_GET_TILING, struct drm_radeon_gem_get_tiling) #define DRM_IOCTL_RADEON_GEM_BUSY DRM_IOWR(DRM_COMMAND_BASE + DRM_RADEON_GEM_BUSY, struct drm_radeon_gem_busy) #define DRM_IOCTL_RADEON_GEM_VA DRM_IOWR(DRM_COMMAND_BASE + DRM_RADEON_GEM_VA, struct drm_radeon_gem_va) +#define DRM_IOCTL_RADEON_GEM_OP DRM_IOWR(DRM_COMMAND_BASE + DRM_RADEON_GEM_OP, struct drm_radeon_gem_op)
typedef struct drm_radeon_init { enum { @@ -884,6 +886,16 @@ struct drm_radeon_gem_pwrite { uint64_t data_ptr; };
+/* Sets or returns a value associated with a buffer. */ +struct drm_radeon_gem_op { + uint32_t handle; /* buffer */ + uint32_t op; /* RADEON_GEM_OP_* */ + uint64_t value; /* input or return value */ +}; + +#define RADEON_GEM_OP_GET_INITIAL_DOMAIN 0 +#define RADEON_GEM_OP_SET_INITIAL_DOMAIN 1 + #define RADEON_VA_MAP 1 #define RADEON_VA_UNMAP 2
Am 26.02.2014 01:44, schrieb Marek Olšák:
This patch is: Reviewed-by: Christian König christian.koenig@amd.com
Where is the rest of the series? At least I didn't got it in my mailbox, and I would like to apply it to drm-next-3.15.
Cheers, Christian.
I'll send the other patches today.
Marek
On Wed, Feb 26, 2014 at 10:39 AM, Christian König deathsimple@vodafone.de wrote:
From: Marek Olšák marek.olsak@amd.com
The statistics are: - VRAM usage in bytes - GTT usage in bytes - number of bytes moved by TTM
The last one is actually a counter, so you need to sample it before and after command submission and take the difference.
This is useful for finding performance bottlenecks. Userspace queries are also added.
Signed-off-by: Marek Olšák marek.olsak@amd.com --- drivers/gpu/drm/radeon/radeon.h | 5 +++++ drivers/gpu/drm/radeon/radeon_device.c | 1 + drivers/gpu/drm/radeon/radeon_kms.c | 15 ++++++++++++++ drivers/gpu/drm/radeon/radeon_object.c | 38 +++++++++++++++++++++++++++++++++- drivers/gpu/drm/radeon/radeon_object.h | 2 +- drivers/gpu/drm/radeon/radeon_ttm.c | 10 ++++++++- include/uapi/drm/radeon_drm.h | 3 +++ 7 files changed, 71 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 3f10782..d37a57a 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -2307,6 +2307,11 @@ struct radeon_device { /* virtual memory */ struct radeon_vm_manager vm_manager; struct mutex gpu_clock_mutex; + /* memory stats */ + struct mutex memory_stats_mutex; + uint64_t vram_usage; + uint64_t gtt_usage; + uint64_t num_bytes_moved; /* ACPI interface */ struct radeon_atif atif; struct radeon_atcs atcs; diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index b012cbb..6564af7 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -1184,6 +1184,7 @@ int radeon_device_init(struct radeon_device *rdev, mutex_init(&rdev->gem.mutex); mutex_init(&rdev->pm.mutex); mutex_init(&rdev->gpu_clock_mutex); + mutex_init(&rdev->memory_stats_mutex); mutex_init(&rdev->srbm_mutex); init_rwsem(&rdev->pm.mclk_lock); init_rwsem(&rdev->exclusive_lock); diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index 0b631eb..ddc8c74 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -486,6 +486,21 @@ static int radeon_info_ioctl(struct drm_device *dev, void *data, struct drm_file case RADEON_INFO_VCE_FB_VERSION: *value = rdev->vce.fb_version; break; + case RADEON_INFO_NUM_BYTES_MOVED: + value = (uint32_t*)&value64; + value_size = sizeof(uint64_t); + value64 = rdev->num_bytes_moved; + break; + case RADEON_INFO_VRAM_USAGE: + value = (uint32_t*)&value64; + value_size = sizeof(uint64_t); + value64 = rdev->vram_usage; + break; + case RADEON_INFO_GTT_USAGE: + value = (uint32_t*)&value64; + value_size = sizeof(uint64_t); + value64 = rdev->gtt_usage; + break; default: DRM_DEBUG_KMS("Invalid request %d\n", info->request); return -EINVAL; diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index dd12bb4..d676ee2 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -56,11 +56,38 @@ static void radeon_bo_clear_va(struct radeon_bo *bo) } }
+static void radeon_update_memory_usage(struct radeon_bo *bo, + unsigned mem_type, int sign) +{ + struct radeon_device *rdev = bo->rdev; + u64 size = (u64)bo->tbo.num_pages << PAGE_SHIFT; + + mutex_lock(&rdev->memory_stats_mutex); + switch (mem_type) { + case TTM_PL_TT: + if (sign > 0) + rdev->gtt_usage += size; + else + rdev->gtt_usage -= size; + break; + case TTM_PL_VRAM: + if (sign > 0) + rdev->vram_usage += size; + else + rdev->vram_usage -= size; + break; + } + mutex_unlock(&rdev->memory_stats_mutex); +} + static void radeon_ttm_bo_destroy(struct ttm_buffer_object *tbo) { struct radeon_bo *bo;
bo = container_of(tbo, struct radeon_bo, tbo); + + radeon_update_memory_usage(bo, bo->tbo.mem.mem_type, -1); + mutex_lock(&bo->rdev->gem.mutex); list_del_init(&bo->list); mutex_unlock(&bo->rdev->gem.mutex); @@ -567,14 +594,23 @@ int radeon_bo_check_tiling(struct radeon_bo *bo, bool has_moved, }
void radeon_bo_move_notify(struct ttm_buffer_object *bo, - struct ttm_mem_reg *mem) + struct ttm_mem_reg *new_mem) { struct radeon_bo *rbo; + if (!radeon_ttm_bo_is_radeon_bo(bo)) return; + rbo = container_of(bo, struct radeon_bo, tbo); radeon_bo_check_tiling(rbo, 0, 1); radeon_vm_bo_invalidate(rbo->rdev, rbo); + + /* update statistics */ + if (!new_mem) + return; + + radeon_update_memory_usage(rbo, bo->mem.mem_type, -1); + radeon_update_memory_usage(rbo, new_mem->mem_type, 1); }
int radeon_bo_fault_reserve_notify(struct ttm_buffer_object *bo) diff --git a/drivers/gpu/drm/radeon/radeon_object.h b/drivers/gpu/drm/radeon/radeon_object.h index 209b111..a9a8c11 100644 --- a/drivers/gpu/drm/radeon/radeon_object.h +++ b/drivers/gpu/drm/radeon/radeon_object.h @@ -151,7 +151,7 @@ extern void radeon_bo_get_tiling_flags(struct radeon_bo *bo, extern int radeon_bo_check_tiling(struct radeon_bo *bo, bool has_moved, bool force_drop); extern void radeon_bo_move_notify(struct ttm_buffer_object *bo, - struct ttm_mem_reg *mem); + struct ttm_mem_reg *new_mem); extern int radeon_bo_fault_reserve_notify(struct ttm_buffer_object *bo); extern int radeon_bo_get_surface_reg(struct radeon_bo *bo);
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index 77f5b0c..7e2e833 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -406,8 +406,16 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, if (r) { memcpy: r = ttm_bo_move_memcpy(bo, evict, no_wait_gpu, new_mem); + if (r) { + return r; + } } - return r; + + /* update statistics */ + mutex_lock(&rdev->memory_stats_mutex); + rdev->num_bytes_moved += (u64)bo->num_pages << PAGE_SHIFT; + mutex_unlock(&rdev->memory_stats_mutex); + return 0; }
static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem) diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_drm.h index cb5c93a..aefa2f6 100644 --- a/include/uapi/drm/radeon_drm.h +++ b/include/uapi/drm/radeon_drm.h @@ -1004,6 +1004,9 @@ struct drm_radeon_cs { #define RADEON_INFO_VCE_FW_VERSION 0x1b /* version of VCE feedback */ #define RADEON_INFO_VCE_FB_VERSION 0x1c +#define RADEON_INFO_NUM_BYTES_MOVED 0x1d +#define RADEON_INFO_VRAM_USAGE 0x1e +#define RADEON_INFO_GTT_USAGE 0x1f
struct drm_radeon_info {
On Mon, Feb 24, 2014 at 5:20 PM, Christian König deathsimple@vodafone.de wrote:
I'm afraid I cannot use atomic64_t. It doesn't work on x86 32-bit. This seems to be a no-op:
u64 size = (u64)bo->num_pages << PAGE_SHIFT; atomic64_add(size, &rdev->num_bytes_moved);
Marek
Am 26.02.2014 18:56, schrieb Marek Olšák:
Are you sure about this? Haven't tested x86 32-bit in a long time, but we use atomic64 all around the place and they usually work perfectly.
Christian.
Marek
Dammit. I renamed the RADEON_INFO definitions for the new queries to 0xd, e, f in the kernel tree, but I forgot to update the Mesa code, which used 0xc, d, e. Sorry.
Marek
On Wed, Feb 26, 2014 at 7:26 PM, Christian König deathsimple@vodafone.de wrote:
From: Marek Olšák marek.olsak@amd.com
Signed-off-by: Marek Olšák marek.olsak@amd.com --- drivers/gpu/drm/radeon/radeon_gem.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index d7890f2..ccb7d0f 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -344,18 +344,7 @@ int radeon_gem_busy_ioctl(struct drm_device *dev, void *data, } robj = gem_to_radeon_bo(gobj); r = radeon_bo_wait(robj, &cur_placement, true); - switch (cur_placement) { - case TTM_PL_VRAM: - args->domain = RADEON_GEM_DOMAIN_VRAM; - break; - case TTM_PL_TT: - args->domain = RADEON_GEM_DOMAIN_GTT; - break; - case TTM_PL_SYSTEM: - args->domain = RADEON_GEM_DOMAIN_CPU; - default: - break; - } + args->domain = radeon_mem_type_to_domain(cur_placement); drm_gem_object_unreference_unlocked(gobj); r = radeon_gem_handle_lockup(rdev, r); return r;
From: Marek Olšák marek.olsak@amd.com
Signed-off-by: Marek Olšák marek.olsak@amd.com --- drivers/gpu/drm/radeon/radeon_cs.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index f28a8d8..d49a3f7 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -24,6 +24,7 @@ * Authors: * Jerome Glisse glisse@freedesktop.org */ +#include <linux/list_sort.h> #include <drm/drmP.h> #include <drm/radeon_drm.h> #include "radeon_reg.h" @@ -290,6 +291,16 @@ int radeon_cs_parser_init(struct radeon_cs_parser *p, void *data) return 0; }
+static int cmp_size_smaller_first(void *priv, struct list_head *a, + struct list_head *b) +{ + struct radeon_bo_list *la = list_entry(a, struct radeon_bo_list, tv.head); + struct radeon_bo_list *lb = list_entry(b, struct radeon_bo_list, tv.head); + + /* Sort A before B if A is smaller. */ + return (int)la->bo->tbo.num_pages - (int)lb->bo->tbo.num_pages; +} + /** * cs_parser_fini() - clean parser states * @parser: parser structure holding parsing context. @@ -303,6 +314,18 @@ static void radeon_cs_parser_fini(struct radeon_cs_parser *parser, int error, bo unsigned i;
if (!error) { + /* Sort the buffer list from the smallest to largest buffer, + * which affects the order of buffers in the LRU list. + * This assures that the smallest buffers are added first + * to the LRU list, so they are likely to be later evicted + * first, instead of large buffers whose eviction is more + * expensive. + * + * This slightly lowers the number of bytes moved by TTM + * per frame under memory pressure. + */ + list_sort(NULL, &parser->validated, cmp_size_smaller_first); + ttm_eu_fence_buffer_objects(&parser->ticket, &parser->validated, parser->ib.fence);
From: Marek Olšák marek.olsak@amd.com
Userspace should set the first 4 bits of drm_radeon_cs_reloc::flags to a number from 0 to 15. The higher the number, the higher the priority, which means a buffer with a higher number will be validated sooner.
The old behavior is preserved: Buffers used for write are prioritized over read-only buffers if the userspace doesn't set the number.
Signed-off-by: Marek Olšák marek.olsak@amd.com --- drivers/gpu/drm/radeon/radeon.h | 2 +- drivers/gpu/drm/radeon/radeon_cs.c | 53 ++++++++++++++++++++++++++++++++-- drivers/gpu/drm/radeon/radeon_object.c | 10 ------- drivers/gpu/drm/radeon/radeon_object.h | 2 -- 4 files changed, 51 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index d37a57a..f7a3174 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -481,7 +481,7 @@ struct radeon_bo_list { struct ttm_validate_buffer tv; struct radeon_bo *bo; uint64_t gpu_offset; - bool written; + unsigned priority; unsigned domain; unsigned alt_domain; u32 tiling_flags; diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index d49a3f7..1ba1a48 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -31,6 +31,41 @@ #include "radeon.h" #include "radeon_trace.h"
+/* This is a variation of the bucket sort with O(n) time complexity. + * The relocations are sorted from the highest to the lowest priority. */ +static void sort_relocs_for_validation(struct list_head *list) +{ + struct list_head bucket[17], *it, *tmp; + unsigned i, priority; + + for (i = 0; i < 17; i++) + INIT_LIST_HEAD(&bucket[i]); + + /* Move the elements into buckets. An i-th bucket only contains + * elements with priorities i*2 and i*2+1. Odd numbers are added + * at the head of a bucket and even numbers are added at the tail, + * therefore all buckets are always sorted. */ + list_for_each_safe(it, tmp, list) { + priority = list_entry(it, struct radeon_bo_list, + tv.head)->priority; + i = priority / 2; + i = min(i, 16u) ; + + if (priority % 2 == 1) { + list_move(it, &bucket[i]); + } else { + list_move_tail(it, &bucket[i]); + } + } + + INIT_LIST_HEAD(list); + + /* connect the sorted buckets */ + for (i = 0; i < 17; i++) { + list_splice(&bucket[i], list); + } +} + static int radeon_cs_parser_relocs(struct radeon_cs_parser *p) { struct drm_device *ddev = p->rdev->ddev; @@ -80,7 +115,15 @@ static int radeon_cs_parser_relocs(struct radeon_cs_parser *p) p->relocs_ptr[i] = &p->relocs[i]; p->relocs[i].robj = gem_to_radeon_bo(p->relocs[i].gobj); p->relocs[i].lobj.bo = p->relocs[i].robj; - p->relocs[i].lobj.written = !!r->write_domain; + + /* The userspace buffer priorities are from 0 to 15. A higher + * number means the buffer is more important. + * Also, the buffers used for write have a higher priority than + * the buffers used for read only, which doubles the range + * to 0 to 31. Numbers 32 and 33 are reserved for the kernel + * driver. + */ + p->relocs[i].lobj.priority = (r->flags & 0xf) * 2 + !!r->write_domain;
/* the first reloc of an UVD job is the msg and that must be in VRAM, also but everything into VRAM on AGP cards to avoid @@ -94,6 +137,8 @@ static int radeon_cs_parser_relocs(struct radeon_cs_parser *p) p->relocs[i].lobj.alt_domain = RADEON_GEM_DOMAIN_VRAM;
+ /* prioritize this over any other relocation */ + p->relocs[i].lobj.priority = 32; } else { uint32_t domain = r->write_domain ? r->write_domain : r->read_domains; @@ -107,9 +152,11 @@ static int radeon_cs_parser_relocs(struct radeon_cs_parser *p) p->relocs[i].lobj.tv.bo = &p->relocs[i].robj->tbo; p->relocs[i].handle = r->handle;
- radeon_bo_list_add_object(&p->relocs[i].lobj, - &p->validated); + list_add(&p->relocs[i].lobj.tv.head, &p->validated); } + + sort_relocs_for_validation(&p->validated); + return radeon_bo_list_validate(&p->ticket, &p->validated, p->ring); }
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index d676ee2..19042ae 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -368,16 +368,6 @@ void radeon_bo_fini(struct radeon_device *rdev) arch_phys_wc_del(rdev->mc.vram_mtrr); }
-void radeon_bo_list_add_object(struct radeon_bo_list *lobj, - struct list_head *head) -{ - if (lobj->written) { - list_add(&lobj->tv.head, head); - } else { - list_add_tail(&lobj->tv.head, head); - } -} - int radeon_bo_list_validate(struct ww_acquire_ctx *ticket, struct list_head *head, int ring) { diff --git a/drivers/gpu/drm/radeon/radeon_object.h b/drivers/gpu/drm/radeon/radeon_object.h index a9a8c11..6c3ca9e 100644 --- a/drivers/gpu/drm/radeon/radeon_object.h +++ b/drivers/gpu/drm/radeon/radeon_object.h @@ -138,8 +138,6 @@ extern int radeon_bo_evict_vram(struct radeon_device *rdev); extern void radeon_bo_force_delete(struct radeon_device *rdev); extern int radeon_bo_init(struct radeon_device *rdev); extern void radeon_bo_fini(struct radeon_device *rdev); -extern void radeon_bo_list_add_object(struct radeon_bo_list *lobj, - struct list_head *head); extern int radeon_bo_list_validate(struct ww_acquire_ctx *ticket, struct list_head *head, int ring); extern int radeon_bo_fbdev_mmap(struct radeon_bo *bo,
Am 24.02.2014 16:20, schrieb Marek Olšák:
Assuming that we only have 32 different priorities it would probably be better to add the buffers to 32 different lists while evaluating the priorities in radeon_cs_parser_relocs and then concatenate all 32 lists at the end instead of sorting them.
From: Marek Olšák marek.olsak@amd.com
Signed-off-by: Marek Olšák marek.olsak@amd.com --- drivers/gpu/drm/radeon/radeon_cs.c | 2 +- drivers/gpu/drm/radeon/radeon_object.c | 87 +++++++++++++++++++++++++++++++--- drivers/gpu/drm/radeon/radeon_object.h | 3 +- 3 files changed, 84 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 1ba1a48..2338eef 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -157,7 +157,7 @@ static int radeon_cs_parser_relocs(struct radeon_cs_parser *p)
sort_relocs_for_validation(&p->validated);
- return radeon_bo_list_validate(&p->ticket, &p->validated, p->ring); + return radeon_bo_list_validate(p->rdev, &p->ticket, &p->validated, p->ring); }
static int radeon_cs_get_ring(struct radeon_cs_parser *p, u32 ring, s32 priority) diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index 19042ae..e1a5af7 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -368,29 +368,104 @@ void radeon_bo_fini(struct radeon_device *rdev) arch_phys_wc_del(rdev->mc.vram_mtrr); }
-int radeon_bo_list_validate(struct ww_acquire_ctx *ticket, +/* Returns how many bytes TTM can move per IB. + */ +static u64 radeon_bo_get_threshold_for_moves(struct radeon_device *rdev) +{ + u64 real_vram_size = rdev->mc.real_vram_size; + u64 vram_usage = rdev->vram_usage; + + /* This function is based on the current VRAM usage. + * + * - If all of VRAM is free, allow relocating the number of bytes that + * is equal to 1/4 of the size of VRAM for this IB. + + * - If more than one half of VRAM is occupied, only allow relocating + * 1 MB of data for this IB. + * + * - From 0 to one half of used VRAM, the threshold decreases + * linearly. + * __________________ + * 1/4 of -|\ | + * VRAM | \ | + * | \ | + * | \ | + * | \ | + * | \ | + * | \ | + * | ________|1 MB + * |----------------| + * VRAM 0 % 100 % + * used used + * + * Note: It's a threshold, not a limit. The threshold must be crossed + * for buffer relocations to stop, so any buffer of an arbitrary size + * can be moved as long as the threshold isn't crossed before + * the relocation takes place. We don't want to disable buffer + * relocations completely. + * + * The idea is that buffers should be placed in VRAM at creation time + * and TTM should only do a minimum number of relocations during + * command submission. In practice, you need to submit at least + * a dozen IBs to move all buffers to VRAM if they are in GTT. + * + * Also, things can get pretty crazy under memory pressure and actual + * VRAM usage can change a lot, so playing safe even at 50% does + * consistently increase performance. + */ + + u64 half_vram = real_vram_size >> 1; + u64 half_free_vram = vram_usage >= half_vram ? 0 : half_vram - vram_usage; + u64 bytes_moved_threshold = half_free_vram >> 1; + return max(bytes_moved_threshold, 1024*1024ull); +} + +int radeon_bo_list_validate(struct radeon_device *rdev, + struct ww_acquire_ctx *ticket, struct list_head *head, int ring) { struct radeon_bo_list *lobj; struct radeon_bo *bo; - u32 domain; int r; + u64 bytes_moved = 0, initial_bytes_moved; + u64 bytes_moved_threshold = radeon_bo_get_threshold_for_moves(rdev);
r = ttm_eu_reserve_buffers(ticket, head); if (unlikely(r != 0)) { return r; } + list_for_each_entry(lobj, head, tv.head) { bo = lobj->bo; if (!bo->pin_count) { - domain = lobj->domain; - + u32 domain = lobj->domain; + u32 current_domain = + radeon_mem_type_to_domain(bo->tbo.mem.mem_type); + + /* Check if this buffer will be moved and don't move it + * if we have moved too many buffers for this IB already. + * + * Note that this allows moving at least one buffer of + * any size, because it doesn't take the current "bo" + * into account. We don't want to disallow buffer moves + * completely. + */ + if (current_domain != RADEON_GEM_DOMAIN_CPU && + (domain & current_domain) == 0 && /* will be moved */ + bytes_moved > bytes_moved_threshold) { + /* don't move it */ + domain = current_domain; + } + retry: radeon_ttm_placement_from_domain(bo, domain); if (ring == R600_RING_TYPE_UVD_INDEX) radeon_uvd_force_into_uvd_segment(bo); - r = ttm_bo_validate(&bo->tbo, &bo->placement, - true, false); + + initial_bytes_moved = rdev->num_bytes_moved; + r = ttm_bo_validate(&bo->tbo, &bo->placement, true, false); + bytes_moved += rdev->num_bytes_moved - initial_bytes_moved; + if (unlikely(r)) { if (r != -ERESTARTSYS && domain != lobj->alt_domain) { domain = lobj->alt_domain; diff --git a/drivers/gpu/drm/radeon/radeon_object.h b/drivers/gpu/drm/radeon/radeon_object.h index 6c3ca9e..7dff64d 100644 --- a/drivers/gpu/drm/radeon/radeon_object.h +++ b/drivers/gpu/drm/radeon/radeon_object.h @@ -138,7 +138,8 @@ extern int radeon_bo_evict_vram(struct radeon_device *rdev); extern void radeon_bo_force_delete(struct radeon_device *rdev); extern int radeon_bo_init(struct radeon_device *rdev); extern void radeon_bo_fini(struct radeon_device *rdev); -extern int radeon_bo_list_validate(struct ww_acquire_ctx *ticket, +extern int radeon_bo_list_validate(struct radeon_device *rdev, + struct ww_acquire_ctx *ticket, struct list_head *head, int ring); extern int radeon_bo_fbdev_mmap(struct radeon_bo *bo, struct vm_area_struct *vma);
Hi Marek,
Some minor comments on patch 1, 2 and 5, but nothing serious. Patch 3, 4 and 6 are Reviewed-by: christian König christian.koenig@amd.com
See below for a few in line comments.
Am 24.02.2014 16:20, schrieb Marek Olšák:
Yeah, I'm also thinking about this for quite some time now. The basic problem is that while our GPUs support VM they don't support faulting pages in and continuing (at least nobody got that working reliable so far). E.g. when you hit a page fault you can't relocate the page and then continue.
Support for partially resident textures on newer hardware currently works by splitting the buffer up into smaller buffers in userspace and then actively checking in the shader if we hit a buffer that's not currently in memory, but that's not really applicable in the general use case (to much shader overhead).
Did you try to make the whole GTT uncached or just evicted BOs? Making the whole GTT uncached probably won't work out of the box, but avoiding setting the "SNOOPED" flag on those pages might get us better performance while swapping them into VRAM again.
Christian.
On Mon, Feb 24, 2014 at 11:40 AM, Christian König deathsimple@vodafone.de wrote:
Well, for non-scanout buffers we can do scatter gather for vram pages rather than using contiguous buffers. That would at least avoid low memory situations where there is enough vram, just not contiguous. Another option would be to write a ttm defragmentor, but I think we'd have to fix the synchronization issues with bo moves first.
Alex
On Mon, Feb 24, 2014 at 5:40 PM, Christian König deathsimple@vodafone.de wrote:
I was thinking of splitting buffers into smaller chunks and treating them like independent TTM buffers, i.e. one radeon_bo would contain an array of TTM buffers which would be validated independently of each other. The chunks would only be mapped together to make them look like one buffer. This would be hidden from userspace and there would only be one GEM handle for the whole buffer, so that DRI2 sharing works.
I made the whole GTT uncached.
Marek
Am 24.02.2014 20:39, schrieb Marek Olšák:
My thoughts where more to to this on the userspace side, but doing it in the kernel indeed avoids a bunch of problems with sharing the buffer.
Sounds like a plan to me. The only thing I can currently see missing is handling of scanout buffers. Do we have a flag for this while creating the buffer?
I'm not sure if this will work, and at least for the ring buffers it's probably also not a good idea.
Christian.
On Tue, Feb 25, 2014 at 11:11 AM, Christian König deathsimple@vodafone.de wrote:
We have a flag in Mesa, but not in the kernel. We can put it in drm_radeon_gem_create::flags though.
Marek
On Mon, Feb 24, 2014 at 08:39:07PM +0100, Marek Olšák wrote:
This is a bad idea you will waste a lot of memory for all the ttm objects. I think you should just decouple that from ttm. TTM placement would be a hint ie if ttm placement is VRAM than radeon code should try to put as much as possible of it in VRAM.
radeon would manage chunk of VRAM with lightweight structure. Of course if such thing is also usefull for nvidia then it would make sense to do that in ttm.
For the scanout buffer this can be done when the buffer is bound to display at which point a flag is set thus we would be backward compatible with old userspace.
Cheers, Jérôme
On Mon, 2014-02-24 at 16:20 +0100, Marek Olšák wrote:
FWIW, I think the issue is more likely with the code or your system than with the GPU: fglrx has always been using unsnooped GPU mappings with write-combined CPU mappings with PCIe as well. I'm not sure how they choose that vs. snooped and cacheable mappings though.
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Christian König -
Jerome Glisse -
Marek Olšák -
Michel Dänzer