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

[Michel Dänzer]

On 2017-11-27 06:12 PM, Christian König wrote:
> 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 at 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.

Moreover, checkpatch.pl cannot always be 100% satisfied, let alone in a
way that actually makes sense for the code in question. Its reports
should be considered guidelines, not the law.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer