[PATCH 1/3] drm/amdgpu: fix VA hole handling on Vega10 v2

[Michel Dänzer]

On 20/11/17 02:20 PM, Christian König wrote:
> Am 20.11.2017 um 11:36 schrieb Michel Dänzer:
>> On 18/11/17 03:31 PM, Christian König wrote:
>>> Am 17.11.2017 um 17:09 schrieb Michel Dänzer:
>>>> On 17/11/17 11:28 AM, Christian König wrote:
>>>>> Ping? Michel, Alex can somebody take a look?
>>>> Patch 2 is
>>>>
>>>> Reviewed-by: Michel Dänzer <michel.daenzer at amd.com>
>>>>
>>>>
>>>> With patches 1 & 3, it's not 100% clear to me what the idea is behind
>>>> the handling of the hole on the kernel and userspace side. Maybe you
>>>> can
>>>> add some explanation in code comments or the commit logs?
>>> Yeah, that is actually a bit of a mess because the hardware
>>> documentation wasn't very clear on how this works.
>>>
>>> How about this as extra code comment on patch 1 to the assignment of
>>> dev_info.virtual_address_max:
>>>
>>> /*
>>>   * Old userspace isn't aware of the VA hole on Vega10. So in theory an
>>> client could get invalid VA addresses assigned.
>>>   * To fix this and keep backward compatibility we limit the VA space
>>> reported in this field to the range below the hole.
>>>   */
>>>
>>> The last patch is then to report the VA space above the hole, cause that
>>> is actually what libdrm should use.
>>>
>>> The crux is when I put the VA space above the hole directly into the old
>>> fields older versions of libdrm would break and we can't do that.
>> That much was actually clear. :) Maybe some questions will expose what
>> I'm missing:
>>
>>
>> Patch 1:
>>
>>> @@ -880,14 +880,14 @@  static int amdgpu_cs_ib_vm_chunk(struct
>>> amdgpu_device *adev,
>>>               if (chunk->chunk_id != AMDGPU_CHUNK_ID_IB)
>>>                   continue;
>>>   -            r = amdgpu_cs_find_mapping(p, chunk_ib->va_start,
>>> -                           &aobj, &m);
>>> +            va_start = chunk_ib->va_start & AMDGPU_VA_HOLE_MASK;
>>> +            r = amdgpu_cs_find_mapping(p, va_start, &aobj, &m);
>> I don't understand how userspace can make use of the address space above
>> the hole, since AMDGPU_VA_HOLE_MASK is 0x0000ffffffffffffULL and
>> AMDGPU_VA_HOLE_END is 0xffff800000000000ULL, so masking with
>> AMDGPU_VA_HOLE_MASK always results in an address below or inside the
>> hole?
> 
> Yes, and exactly that is intended here.
> 
> See for programming the MC and filling the page tables you handle this
> as if there isn't a hole in the address space.
> 
> Only when you start to use the address from userspace you will find that
> you need to sign extend bit 47 into bits 48-63.
> 
> Applying the AMDGPU_VA_HOLE_MASK to and address masks out the upper 16
> bits and so removes the sign extension again.
> 
>>
>>
>>> @@ -566,6 +566,17 @@  int amdgpu_gem_va_ioctl(struct drm_device *dev,
>>> void *data,
>>>           return -EINVAL;
>>>       }
>>>   +    if (args->va_address >= AMDGPU_VA_HOLE_START &&
>>> +        args->va_address < AMDGPU_VA_HOLE_END) {
>>> +        dev_dbg(&dev->pdev->dev,
>>> +            "va_address 0x%LX is in VA hole 0x%LX-0x%LX\n",
>>> +            args->va_address, AMDGPU_VA_HOLE_START,
>>> +            AMDGPU_VA_HOLE_END);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    args->va_address &= AMDGPU_VA_HOLE_MASK;
>> Ignoring the above, I think the masking with AMDGPU_VA_HOLE_MASK would
>> need to be done before checking for the hole, otherwise the masked
>> address could be inside the hole?
> 
> The masking just removes the sign extension which created the hole in
> the first place.
> 
> The VM and MC is programmed as if there isn't a hole.
> 
>> Patch 3:
>>
>>> @@ -577,10 +578,17 @@  static int amdgpu_info_ioctl(struct drm_device
>>> *dev, void *data, struct drm_file
>>>               dev_info.ids_flags |= AMDGPU_IDS_FLAGS_FUSION;
>>>           if (amdgpu_sriov_vf(adev))
>>>               dev_info.ids_flags |= AMDGPU_IDS_FLAGS_PREEMPTION;
>>> +
>>> +        vm_size = adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE;
>>>           dev_info.virtual_address_offset = AMDGPU_VA_RESERVED_SIZE;
>>>           dev_info.virtual_address_max =
>>> -            min(adev->vm_manager.max_pfn * AMDGPU_GPU_PAGE_SIZE,
>>> -                AMDGPU_VA_HOLE_START);
>>> +            min(vm_size, AMDGPU_VA_HOLE_START);
>>> +
>>> +        vm_size -= AMDGPU_VA_RESERVED_SIZE;
>>> +        if (vm_size > AMDGPU_VA_HOLE_START) {
>>> +            dev_info.high_va_offset = AMDGPU_VA_HOLE_END;
>>> +            dev_info.high_va_max = AMDGPU_VA_HOLE_END | vm_size;
>>> +        }
>> This mostly makes sense, except for the
>>
>>         dev_info.high_va_max = AMDGPU_VA_HOLE_END | vm_size;
>>
>> line. Can you explain how simply or'ing together the address of the end
>> of the hole and the VM size works correctly in all cases?
>>
>> As an extreme example, with vm_size == 1, this value would compute as
>> 0xffff800000000001ULL, which seems wrong or at least weird.
> 
> We check above if vm_size is larger than AMDGPU_VA_HOLE_START, in other
> words larger than 0x8000000000.
> 
> Since vm_size is once more in the value range the MC expects it (without
> the hole) that should give us the right result to sign extend bit 47
> into bits 48-63, e.g. 0xffff800000000.
> 
> 
> And yes that initially confused me as well. Especially that you need to
> program the MC and fill the page tables as if there wouldn't be a hole
> in the address space.
> 
> Applying "& AMDGPU_VA_HOLE_MASK" and "| AMDGPU_VA_HOLE_END" just
> converts between the representation with and without the hole in it.

Thanks for the explanation, I get it now.

For the benefit of future readers of this code, it would be nice to have
a short version of this explanation as a comment, e.g. above the
AMDGPU_VA_HOLE_* defines.


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