[PATCH] drm/radeon: adjust default radeon_vm_block_size

[Christian König]

> Hm, that automatic calculation seems strange. Is there any particular
> reason why you haven chosen this? This will use vm_block_size=11 for
> 4096 MB VM space. I have used vm_block_size=12 with good results. This
> manages 16 MB of VM space per page table, which seems reasonable. I also
> see a noticeable CPU overhead reduction between 11 and 12.
Yeah, I'm still playing a bit with this.

You need to consider the page directory size as well. If we have 4GB 
address space (32bits) and 12bits in the page, 12bits in the page tables 
then there are only 8bits for the page directory, right?

Now 8bits for the page directory means we have 256 entries with 8bytes 
for each entry that makes 2048 bytes for the page directory. But since 
we allocate 4096 bytes for the page directory anyway we could support 
8GB address space as well.

How about the v2 I've just send out to the list? It also adjusts the 
vm_size to a default of 8GB and let you get vm_block_sizes according to 
the following table:

vm_size         vm_block_size
1GB        =    9
2GB        =    10
4GB        =    11
8GB        =    12
16GB      =    12
32GB      =    13
64GB      =    13
128GB    =    14
256GB    =    14
512GB    =    15
1TB        =    15

Regards,
Christian.

Am 18.07.2014 22:03, schrieb Grigori Goronzy:
> On 18.07.2014 11:38, Christian König wrote:
>> From: Christian König <christian.koenig at amd.com>
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>>   drivers/gpu/drm/radeon/radeon_device.c | 6 +++++-
>>   drivers/gpu/drm/radeon/radeon_drv.c    | 4 ++--
>>   2 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
>> index 03686fa..a2960db 100644
>> --- a/drivers/gpu/drm/radeon/radeon_device.c
>> +++ b/drivers/gpu/drm/radeon/radeon_device.c
>> @@ -1077,7 +1077,11 @@ static void radeon_check_arguments(struct radeon_device *rdev)
>>   	/* defines number of bits in page table versus page directory,
>>   	 * a page is 4KB so we have 12 bits offset, minimum 9 bits in the
>>   	 * page table and the remaining bits are in the page directory */
>> -	if (radeon_vm_block_size < 9) {
>> +	if (radeon_vm_block_size == -1) {
>> +		radeon_vm_block_size = (ilog2(radeon_vm_size) + 10) / 2;
>> +		radeon_vm_block_size = max(radeon_vm_block_size, 9);
>> +
>> +	} else if (radeon_vm_block_size < 9) {
> Hm, that automatic calculation seems strange. Is there any particular
> reason why you haven chosen this? This will use vm_block_size=11 for
> 4096 MB VM space. I have used vm_block_size=12 with good results. This
> manages 16 MB of VM space per page table, which seems reasonable. I also
> see a noticeable CPU overhead reduction between 11 and 12.
>
> Grigori
>
>>   		dev_warn(rdev->dev, "VM page table size (%d) to small\n",
>>   			 radeon_vm_block_size);
>>   		radeon_vm_block_size = 9;
>> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
>> index cb14213..fef5320 100644
>> --- a/drivers/gpu/drm/radeon/radeon_drv.c
>> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
>> @@ -174,7 +174,7 @@ int radeon_aspm = -1;
>>   int radeon_runtime_pm = -1;
>>   int radeon_hard_reset = 0;
>>   int radeon_vm_size = 4096;
>> -int radeon_vm_block_size = 9;
>> +int radeon_vm_block_size = -1;
>>   int radeon_deep_color = 0;
>>   
>>   MODULE_PARM_DESC(no_wb, "Disable AGP writeback for scratch registers");
>> @@ -246,7 +246,7 @@ module_param_named(hard_reset, radeon_hard_reset, int, 0444);
>>   MODULE_PARM_DESC(vm_size, "VM address space size in megabytes (default 4GB)");
>>   module_param_named(vm_size, radeon_vm_size, int, 0444);
>>   
>> -MODULE_PARM_DESC(vm_block_size, "VM page table size in bits (default 9)");
>> +MODULE_PARM_DESC(vm_block_size, "VM page table size in bits (default depending on vm_size)");
>>   module_param_named(vm_block_size, radeon_vm_block_size, int, 0444);
>>   
>>   MODULE_PARM_DESC(deep_color, "Deep Color support (1 = enable, 0 = disable (default))");
>>
>