GART and VRAM size limits need to be a power of two. Fix values greater than 1GB and simplify those checks a bit.
Signed-off-by: Christian König deathsimple@vodafone.de --- drivers/gpu/drm/radeon/radeon_device.c | 55 ++++++++++++-------------------- 1 file changed, 21 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index bd13ca0..3277aa1 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -835,6 +835,19 @@ static unsigned int radeon_vga_set_decode(void *cookie, bool state) }
/** + * radeon_check_pot_argument - check that argument is a power of two + * + * @arg: value to check + * + * Validates that a certain argument is a power of two (all asics). + * Returns true if argument is valid. + */ +static bool radeon_ckeck_pot_argument(int arg) +{ + return (arg & ((1 << __fls(arg)) - 1)) == 0; +} + +/** * radeon_check_arguments - validate module params * * @rdev: radeon_device pointer @@ -845,52 +858,26 @@ static unsigned int radeon_vga_set_decode(void *cookie, bool state) static void radeon_check_arguments(struct radeon_device *rdev) { /* vramlimit must be a power of two */ - switch (radeon_vram_limit) { - case 0: - case 4: - case 8: - case 16: - case 32: - case 64: - case 128: - case 256: - case 512: - case 1024: - case 2048: - case 4096: - break; - default: + if (!radeon_ckeck_pot_argument(radeon_vram_limit)) { dev_warn(rdev->dev, "vram limit (%d) must be a power of 2\n", radeon_vram_limit); radeon_vram_limit = 0; - break; } - radeon_vram_limit = radeon_vram_limit << 20; + radeon_vram_limit = (uint64_t)radeon_vram_limit << 20; + /* gtt size must be power of two and greater or equal to 32M */ - switch (radeon_gart_size) { - case 4: - case 8: - case 16: + if (radeon_gart_size < 32) { dev_warn(rdev->dev, "gart size (%d) too small forcing to 512M\n", radeon_gart_size); radeon_gart_size = 512; - break; - case 32: - case 64: - case 128: - case 256: - case 512: - case 1024: - case 2048: - case 4096: - break; - default: + + } else if (!radeon_ckeck_pot_argument(radeon_gart_size)) { dev_warn(rdev->dev, "gart size (%d) must be a power of 2\n", radeon_gart_size); radeon_gart_size = 512; - break; } - rdev->mc.gtt_size = radeon_gart_size * 1024 * 1024; + rdev->mc.gtt_size = (uint64_t)radeon_gart_size << 20; + /* AGP mode can only be -1, 1, 2, 4, 8 */ switch (radeon_agpmode) { case -1:
When allocating more than 2GB of GART the array of pages gets to big for kzalloc, use vzmalloc instead.
Signed-off-by: Christian König deathsimple@vodafone.de --- drivers/gpu/drm/radeon/radeon_gart.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_gart.c b/drivers/gpu/drm/radeon/radeon_gart.c index 9a64f8c..631973d 100644 --- a/drivers/gpu/drm/radeon/radeon_gart.c +++ b/drivers/gpu/drm/radeon/radeon_gart.c @@ -355,14 +355,13 @@ int radeon_gart_init(struct radeon_device *rdev) DRM_INFO("GART: num cpu pages %u, num gpu pages %u\n", rdev->gart.num_cpu_pages, rdev->gart.num_gpu_pages); /* Allocate pages table */ - rdev->gart.pages = kzalloc(sizeof(void *) * rdev->gart.num_cpu_pages, - GFP_KERNEL); + rdev->gart.pages = vzalloc(sizeof(void *) * rdev->gart.num_cpu_pages); if (rdev->gart.pages == NULL) { radeon_gart_fini(rdev); return -ENOMEM; } - rdev->gart.pages_addr = kzalloc(sizeof(dma_addr_t) * - rdev->gart.num_cpu_pages, GFP_KERNEL); + rdev->gart.pages_addr = vzalloc(sizeof(dma_addr_t) * + rdev->gart.num_cpu_pages); if (rdev->gart.pages_addr == NULL) { radeon_gart_fini(rdev); return -ENOMEM; @@ -388,8 +387,8 @@ void radeon_gart_fini(struct radeon_device *rdev) radeon_gart_unbind(rdev, 0, rdev->gart.num_cpu_pages); } rdev->gart.ready = false; - kfree(rdev->gart.pages); - kfree(rdev->gart.pages_addr); + vfree(rdev->gart.pages); + vfree(rdev->gart.pages_addr); rdev->gart.pages = NULL; rdev->gart.pages_addr = NULL;
Driver internal users shouldn't be limited in their allocation size.
Signed-off-by: Christian König deathsimple@vodafone.de --- drivers/gpu/drm/radeon/radeon_gem.c | 10 ++++++++++ drivers/gpu/drm/radeon/radeon_object.c | 9 --------- 2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index f38fbcc..dfee7bb 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -53,6 +53,7 @@ int radeon_gem_object_create(struct radeon_device *rdev, int size, struct drm_gem_object **obj) { struct radeon_bo *robj; + unsigned long max_size; int r;
*obj = NULL; @@ -60,6 +61,15 @@ int radeon_gem_object_create(struct radeon_device *rdev, int size, if (alignment < PAGE_SIZE) { alignment = PAGE_SIZE; } + + /* maximun bo size is the minimun btw visible vram and gtt size */ + max_size = min(rdev->mc.visible_vram_size, rdev->mc.gtt_size); + if (size > max_size) { + printk(KERN_WARNING "%s:%d alloc size %dMb bigger than %ldMb limit\n", + __func__, __LINE__, size >> 20, max_size >> 20); + return -ENOMEM; + } + r = radeon_bo_create(rdev, size, alignment, kernel, initial_domain, NULL, &robj); if (r) { if (r != -ERESTARTSYS) diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index 8b27dd6..f404944 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -105,7 +105,6 @@ int radeon_bo_create(struct radeon_device *rdev, struct radeon_bo *bo; enum ttm_bo_type type; unsigned long page_align = roundup(byte_align, PAGE_SIZE) >> PAGE_SHIFT; - unsigned long max_size = 0; size_t acc_size; int r;
@@ -121,14 +120,6 @@ int radeon_bo_create(struct radeon_device *rdev, } *bo_ptr = NULL;
- /* maximun bo size is the minimun btw visible vram and gtt size */ - max_size = min(rdev->mc.visible_vram_size, rdev->mc.gtt_size); - if ((page_align << PAGE_SHIFT) >= max_size) { - printk(KERN_WARNING "%s:%d alloc size %ldM bigger than %ldMb limit\n", - __func__, __LINE__, page_align >> (20 - PAGE_SHIFT), max_size >> 20); - return -ENOMEM; - } - acc_size = ttm_bo_dma_acc_size(&rdev->mman.bdev, size, sizeof(struct radeon_bo));
When internal users want VRAM we shouldn't return GART memory instead.
Signed-off-by: Christian König deathsimple@vodafone.de --- drivers/gpu/drm/radeon/radeon_gem.c | 8 +++++++- drivers/gpu/drm/radeon/radeon_object.c | 10 ---------- 2 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index dfee7bb..fe5c1f6 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -70,11 +70,17 @@ int radeon_gem_object_create(struct radeon_device *rdev, int size, return -ENOMEM; }
+retry: r = radeon_bo_create(rdev, size, alignment, kernel, initial_domain, NULL, &robj); if (r) { - if (r != -ERESTARTSYS) + if (r != -ERESTARTSYS) { + if (initial_domain == RADEON_GEM_DOMAIN_VRAM) { + initial_domain |= RADEON_GEM_DOMAIN_GTT; + goto retry; + } DRM_ERROR("Failed to allocate GEM object (%d, %d, %u, %d)\n", size, initial_domain, alignment, r); + } return r; } *obj = &robj->gem_base; diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index f404944..b91118c 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -123,7 +123,6 @@ int radeon_bo_create(struct radeon_device *rdev, acc_size = ttm_bo_dma_acc_size(&rdev->mman.bdev, size, sizeof(struct radeon_bo));
-retry: bo = kzalloc(sizeof(struct radeon_bo), GFP_KERNEL); if (bo == NULL) return -ENOMEM; @@ -145,15 +144,6 @@ retry: acc_size, sg, &radeon_ttm_bo_destroy); up_read(&rdev->pm.mclk_lock); if (unlikely(r != 0)) { - if (r != -ERESTARTSYS) { - if (domain == RADEON_GEM_DOMAIN_VRAM) { - domain |= RADEON_GEM_DOMAIN_GTT; - goto retry; - } - dev_err(rdev->dev, - "object_init failed for (%lu, 0x%08X)\n", - size, domain); - } return r; } *bo_ptr = bo;
On Die, 2012-10-23 at 14:23 +0200, Christian König wrote:
GART and VRAM size limits need to be a power of two. Fix values greater than 1GB and simplify those checks a bit.
Signed-off-by: Christian König deathsimple@vodafone.de
drivers/gpu/drm/radeon/radeon_device.c | 55 ++++++++++++-------------------- 1 file changed, 21 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index bd13ca0..3277aa1 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -835,6 +835,19 @@ static unsigned int radeon_vga_set_decode(void *cookie, bool state) }
/**
- radeon_check_pot_argument - check that argument is a power of two
- @arg: value to check
- Validates that a certain argument is a power of two (all asics).
- Returns true if argument is valid.
- */
+static bool radeon_ckeck_pot_argument(int arg) +{
- return (arg & ((1 << __fls(arg)) - 1)) == 0;
+}
This could be simplified as
return (arg & (arg - 1)) == 0;
- radeon_vram_limit = radeon_vram_limit << 20;
- radeon_vram_limit = (uint64_t)radeon_vram_limit << 20;
Does this cast have any effect without also changing the type of radeon_vram_limit to something other than int? If the point is to allow the shift to set the sign bit, I think casting to unsigned int or uint32_t would be slightly less confusing, but a comment is probably warranted anyway to prevent this from getting broken accidentally.
The commit message of patch 2 has a typo (vzmalloc instead of vzalloc), other than that patches 2-4 are
Reviewed-by: Michel Dänzer michel.daenzer@amd.com
On 23.10.2012 14:44, Michel Dänzer wrote:
On Die, 2012-10-23 at 14:23 +0200, Christian König wrote:
GART and VRAM size limits need to be a power of two. Fix values greater than 1GB and simplify those checks a bit.
Signed-off-by: Christian König deathsimple@vodafone.de
drivers/gpu/drm/radeon/radeon_device.c | 55 ++++++++++++-------------------- 1 file changed, 21 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index bd13ca0..3277aa1 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -835,6 +835,19 @@ static unsigned int radeon_vga_set_decode(void *cookie, bool state) }
/**
- radeon_check_pot_argument - check that argument is a power of two
- @arg: value to check
- Validates that a certain argument is a power of two (all asics).
- Returns true if argument is valid.
- */
+static bool radeon_ckeck_pot_argument(int arg) +{
- return (arg & ((1 << __fls(arg)) - 1)) == 0;
+}
This could be simplified as
return (arg & (arg - 1)) == 0;
Fixed.
- radeon_vram_limit = radeon_vram_limit << 20;
- radeon_vram_limit = (uint64_t)radeon_vram_limit << 20;
Does this cast have any effect without also changing the type of radeon_vram_limit to something other than int? If the point is to allow the shift to set the sign bit, I think casting to unsigned int or uint32_t would be slightly less confusing, but a comment is probably warranted anyway to prevent this from getting broken accidentally.
Well, I actually focused on the GART limit instead, and just tried to fix that in the same way.
But wait a moment, modifying the radeon_vmram_limit here is quite faulty anyway, cause this code gets executed for each card in the system. So the vram limit won't work for the second card any more (and might produce quite unusable results).
Going to fix that.
The commit message of patch 2 has a typo (vzmalloc instead of vzalloc), other than that patches 2-4 are
Reviewed-by: Michel Dänzer michel.daenzer@amd.com
Thanks for the review, going to send out a v2 of the patchset soon.
Christian.
dri-devel@lists.freedesktop.org
-
Christian König -
Michel Dänzer