Am 2021-03-26 um 4:50 a.m. schrieb Christian König:
>
>
> Am 25.03.21 um 17:28 schrieb Felix Kuehling:
>> Am 2021-03-25 um 12:22 p.m. schrieb Christian König:
>>>>>>>> My idea is to change the amdgpu_vm_update_mapping interface to use
>>>>>>>> some
>>>>>>>> high-bit in the pages_addr array to indicate the page type. For
>>>>>>>> the
>>>>>>>> default page type (0) nothing really changes for the callers. The
>>>>>>>> "flags" parameter needs to become a pointer to an array that gets
>>>>>>>> indexed by the high bits from the pages_addr array. For existing
>>>>>>>> callers
>>>>>>>> it's as easy as changing flags to &flags (array of size 1). For
>>>>>>>> HMM we
>>>>>>>> would pass a pointer to a real array.
>>>>>>> Yeah, I've thought about stuff like that as well for a while.
>>>>>>>
>>>>>>> Problem is that this won't work that easily. We assume at many
>>>>>>> places
>>>>>>> that the flags doesn't change for the range in question.
>>>>>> I think some lower level functions assume that the flags stay the
>>>>>> same
>>>>>> for physically contiguous ranges. But if you use the high-bits to
>>>>>> encode
>>>>>> the page type, the ranges won't be contiguous any more. So you can
>>>>>> change page flags for different contiguous ranges.
>>>>> Yeah, but then you also get absolutely zero THP and fragment flags
>>>>> support.
>>>> As long as you have a contiguous 2MB page with the same page type, I
>>>> think you can still get a THP mapping in the GPU page table. But if
>>>> one
>>>> page in the middle of a 2MB page has a different page type, that will
>>>> break the THP mapping, as it should.
>>> Yeah, but currently we detect that before we call down into the
>>> functions to update the tables.
>>>
>>> When you give a mixed list the chance for that is just completely gone.
>> Currently the detection of contiguous ranges is in
>> amdgpu_vm_update_mapping:
>>
>>> if (num_entries > AMDGPU_GPU_PAGES_IN_CPU_PAGE) { uint64_t pfn =
>>> cursor.start >> PAGE_SHIFT; uint64_t count; contiguous =
>>> pages_addr[pfn + 1] == pages_addr[pfn] + PAGE_SIZE; tmp = num_entries
>>> / AMDGPU_GPU_PAGES_IN_CPU_PAGE; for (count = 2; count < tmp; ++count)
>>> { uint64_t idx = pfn + count; if (contiguous != (pages_addr[idx] ==
>>> pages_addr[idx - 1] + PAGE_SIZE)) break; } num_entries = count *
>>> AMDGPU_GPU_PAGES_IN_CPU_PAGE; }
>> If a page type changes from one page to the next, it will end a
>> contiguous range and call into the lower level (amdgpu_vm_update_ptes).
>> I don't think anything needs to change in amdgpu_vm_update_ptes, because
>> all pages contiguous passed to it use the same page type, so the same
>> flags. And the existing code in amdgpu_vm_update_mapping already does
>> the right thing. I honestly don't see the problem.
>
> But then you could also just call amdgpu_vm_update_mapping() multiple
> times feeding it from whatever data structure you use in the HMM code.
>
> Using the page array sounds like an intermediate data structure to me
> you only created to feed into amdgpu_vm_update_mapping().
You're right. It could be done. One new call to amdgpu_vm_update_mapping
every time the flags change as we iterate over virtual addresses. It
should work OK as long as the mappings of different memory types aren't
too finely interleaved.
Regards,
Felix
> Regards,
> Christian.
>
>>
>> Regards,
>> Felix
>>
>>
>>> Regards,
>>> Christian.
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
