Re: [PATCH 1/7] drm/amdgpu: fix VM PD addr shift

["Christian König" <christian.koenig-5C7GfCeVMHo@public.gmane.org>]

Am 27.11.2017 um 17:40 schrieb Felix Kuehling:
> On 2017-11-27 11:02 AM, Christian König wrote:
>> The block size only affects the leave nodes, everything else is fixed.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 28 +++++++++++++++++++++++-----
>>   1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 122379dfc7d8..f1e541e9b514 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -139,6 +139,24 @@ struct amdgpu_prt_cb {
>>   };
>>   
>>   /**
>> + * amdgpu_vm_level_shift - return the addr shift for each level
>> + *
>> + * @adev: amdgpu_device pointer
>> + *
>> + * Returns the number of bits the pfn needs to be right shifted for a level.
>> + */
>> +static unsigned amdgpu_vm_level_shift(struct amdgpu_device *adev,
>> +				      unsigned level)
>> +{
>> +	if (level != adev->vm_manager.num_level)
>> +		return 9 * (adev->vm_manager.num_level - level - 1) +
>> +			adev->vm_manager.block_size;
>> +	else
>> +		/* For the page tables on the leaves */
>> +		return 0;
>> +}
>> +
>> +/**
>>    * amdgpu_vm_num_entries - return the number of entries in a PD/PT
>>    *
>>    * @adev: amdgpu_device pointer
>> @@ -288,8 +306,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>>   				  uint64_t saddr, uint64_t eaddr,
>>   				  unsigned level)
>>   {
>> -	unsigned shift = (adev->vm_manager.num_level - level) *
>> -		adev->vm_manager.block_size;
>> +	unsigned shift = amdgpu_vm_level_shift(adev, level);
>>   	unsigned pt_idx, from, to;
>>   	int r;
>>   	u64 flags;
>> @@ -1302,18 +1319,19 @@ void amdgpu_vm_get_entry(struct amdgpu_pte_update_params *p, uint64_t addr,
>>   			 struct amdgpu_vm_pt **entry,
>>   			 struct amdgpu_vm_pt **parent)
>>   {
>> -	unsigned idx, level = p->adev->vm_manager.num_level;
>> +	unsigned level = 0;
> This generates a checkpatch.pl warning.
>
> General question: Do you have a policy for running your patches through
> checkpatch.pl before/during/after code review?

No, I'm using editor settings which generate coding style compliant code 
most of the time and complains/shows when it isn't compliant.

> I've noticed before that some of your patches aren't always 100% compliant.

Yeah, amdgpu inherited a lot of code as well as coding style from radeon 
which isn't compliant.

For example in this case checkpatch.pl most likely complains that we 
should use "unsigned int" instead of just "unsigned".

I've tried to clean this up for years, but simply not enough time to 
handle everything.

Regards,
Christian.

>
> Regards,
>    Felix
>
>>   
>>   	*parent = NULL;
>>   	*entry = &p->vm->root;
>>   	while ((*entry)->entries) {
>> -		idx = addr >> (p->adev->vm_manager.block_size * level--);
>> +		unsigned idx = addr >> amdgpu_vm_level_shift(p->adev, level++);
>> +
>>   		idx %= amdgpu_bo_size((*entry)->base.bo) / 8;
>>   		*parent = *entry;
>>   		*entry = &(*entry)->entries[idx];
>>   	}
>>   
>> -	if (level)
>> +	if (level != p->adev->vm_manager.num_level)
>>   		*entry = NULL;
>>   }
>>   

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx