[PATCH v3 1/2] drm/xe: Introduce helper to populate userptr
Ghimiray, Himal Prasad
himal.prasad.ghimiray at intel.com
Fri Apr 5 08:05:20 UTC 2024
On 05-04-2024 11:42, Ghimiray, Himal Prasad wrote:
>
>
> On 05-04-2024 06:37, Matthew Brost wrote:
>> On Thu, Apr 04, 2024 at 11:46:30AM +0530, Himal Prasad Ghimiray wrote:
>>> From: Oak Zeng<oak.zeng at intel.com>
>>>
>>> Introduce a helper function xe_userptr_populate_range to populate
>>> a a userptr range. This functions calls hmm_range_fault to read
>>> CPU page tables and populate all pfns/pages of this virtual address
>>> range.
>>>
>>> If the populated page is system memory page, dma-mapping is performed
>>> to get a dma-address which can be used later for GPU to access pages.
>>>
>>> If the populated page is device private page, we calculate the dpa (
>>> device physical address) of the page. This will be handled in future
>>> patches.
>>>
>>> The dma-address or dpa is then saved in userptr's sg table. This is
>>> prepare work to replace the get_user_pages_fast code in userptr code
>>> path.
>>>
>>> v1: Address review comments:
>>> separate a npage_in_range function (Matt)
>>> reparameterize function xe_userptr_populate_range function (Matt)
>>> move mmu_interval_read_begin() call into while loop (Thomas)
>>> s/mark_range_accessed/xe_mark_range_accessed (Thomas)
>>> use set_page_dirty_lock (vs set_page_dirty) (Thomas)
>>> move a few checking in xe_vma_userptr_pin_pages to hmm.c (Matt)
>>> v2: Remove device private page support. Only support system
>>> pages for now. use dma-map-sg rather than dma-map-page (Matt/Thomas)
>>> v3: Address review comments:
>>> Squash patch "drm/xe: Introduce a helper to free sg table" to current
>>> patch (Matt)
>>> start and end addresses are already page aligned (Matt)
>>> Do mmap_read_lock and mmap_read_unlock for hmm_range_fault incase of
>>> non system allocator call. (Matt)
>>> Drop kthread_use_mm and kthread_unuse_mm. (Matt)
>>> No need of kernel-doc for static functions.(Matt)
>>> Modify function names. (Matt)
>>> Free sgtable incase of dma_map_sgtable failure.(Matt)
>>> Modify loop for hmm_range_fault.(Matt)
>>>
>>> Signed-off-by: Oak Zeng<oak.zeng at intel.com>
>>> Co-developed-by: Niranjana Vishwanathapura<niranjana.vishwanathapura at intel.com>
>>> Signed-off-by: Niranjana Vishwanathapura<niranjana.vishwanathapura at intel.com>
>>> Cc: Matthew Brost<matthew.brost at intel.com>
>>> Cc: Thomas Hellström<thomas.hellstrom at intel.com>
>>> Cc: Brian Welty<brian.welty at intel.com>
>>> Signed-off-by: Himal Prasad Ghimiray<himal.prasad.ghimiray at intel.com>
>>> ---
>>> drivers/gpu/drm/xe/Kconfig | 1 +
>>> drivers/gpu/drm/xe/Makefile | 2 +
>>> drivers/gpu/drm/xe/xe_hmm.c | 253 ++++++++++++++++++++++++++++++++++++
>>> drivers/gpu/drm/xe/xe_hmm.h | 18 +++
>>> 4 files changed, 274 insertions(+)
>>> create mode 100644 drivers/gpu/drm/xe/xe_hmm.c
>>> create mode 100644 drivers/gpu/drm/xe/xe_hmm.h
>>>
>>> diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
>>> index 1a556d087e63..449a1ecbc92a 100644
>>> --- a/drivers/gpu/drm/xe/Kconfig
>>> +++ b/drivers/gpu/drm/xe/Kconfig
>>> @@ -41,6 +41,7 @@ config DRM_XE
>>> select MMU_NOTIFIER
>>> select WANT_DEV_COREDUMP
>>> select AUXILIARY_BUS
>>> + select HMM_MIRROR
>>> help
>>> Experimental driver for Intel Xe series GPUs
>>>
>>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>>> index 21316ee47026..07031b5ba977 100644
>>> --- a/drivers/gpu/drm/xe/Makefile
>>> +++ b/drivers/gpu/drm/xe/Makefile
>>> @@ -146,6 +146,8 @@ xe-y += xe_bb.o \
>>> xe_wa.o \
>>> xe_wopcm.o
>>>
>>> +xe-$(CONFIG_HMM_MIRROR) += xe_hmm.o
>>> +
>>> # graphics hardware monitoring (HWMON) support
>>> xe-$(CONFIG_HWMON) += xe_hwmon.o
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_hmm.c b/drivers/gpu/drm/xe/xe_hmm.c
>>> new file mode 100644
>>> index 000000000000..d7a11896ad72
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/xe/xe_hmm.c
>>> @@ -0,0 +1,253 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2024 Intel Corporation
>>> + */
>>> +
>>> +#include <linux/scatterlist.h>
>>> +#include <linux/mmu_notifier.h>
>>> +#include <linux/dma-mapping.h>
>>> +#include <linux/memremap.h>
>>> +#include <linux/swap.h>
>>> +#include <linux/hmm.h>
>>> +#include <linux/mm.h>
>>> +#include "xe_hmm.h"
>>> +#include "xe_vm.h"
>>> +#include "xe_bo.h"
>>> +
>>> +static u64 xe_npages_in_range(unsigned long start, unsigned long end)
>>> +{
>>> + return (end - start) >> PAGE_SHIFT;
>>> +}
>>> +
>>> +/*
>>> + * xe_mark_range_accessed() - mark a range is accessed, so core mm
>>> + * have such information for memory eviction or write back to
>>> + * hard disk
>>> + *
>>> + * @range: the range to mark
>>> + * @write: if write to this range, we mark pages in this range
>>> + * as dirty
>>> + */
>>> +static void xe_mark_range_accessed(struct hmm_range *range, bool write)
>>> +{
>>> + struct page *page;
>>> + u64 i, npages;
>>> +
>>> + npages = xe_npages_in_range(range->start, range->end);
>>> + for (i = 0; i < npages; i++) {
>>> + page = hmm_pfn_to_page(range->hmm_pfns[i]);
>>> + if (write)
>>> + set_page_dirty_lock(page);
>>> +
>>> + mark_page_accessed(page);
>>> + }
>>> +}
>>> +
>>> +/*
>>> + * xe_build_sg() - build a scatter gather table for all the physical pages/pfn
>>> + * in a hmm_range. dma-map pages if necessary. dma-address is save in sg table
>>> + * and will be used to program GPU page table later.
>>> + *
>>> + * @xe: the xe device who will access the dma-address in sg table
>>> + * @range: the hmm range that we build the sg table from. range->hmm_pfns[]
>>> + * has the pfn numbers of pages that back up this hmm address range.
>>> + * @st: pointer to the sg table.
>>> + * @write: whether we write to this range. This decides dma map direction
>>> + * for system pages. If write we map it bi-diretional; otherwise
>>> + * DMA_TO_DEVICE
>>> + *
>>> + * All the contiguous pfns will be collapsed into one entry in
>>> + * the scatter gather table. This is for the purpose of efficiently
>>> + * programming GPU page table.
>>> + *
>>> + * The dma_address in the sg table will later be used by GPU to
>>> + * access memory. So if the memory is system memory, we need to
>>> + * do a dma-mapping so it can be accessed by GPU/DMA.
>>> + *
>>> + * FIXME: This function currently only support pages in system
>>> + * memory. If the memory is GPU local memory (of the GPU who
>>> + * is going to access memory), we need gpu dpa (device physical
>>> + * address), and there is no need of dma-mapping. This is TBD.
>>> + *
>>> + * FIXME: dma-mapping for peer gpu device to access remote gpu's
>>> + * memory. Add this when you support p2p
>>> + *
>>> + * This function allocates the storage of the sg table. It is
>>> + * caller's responsibility to free it calling sg_free_table.
>>> + *
>>> + * Returns 0 if successful; -ENOMEM if fails to allocate memory
>>> + */
>>> +static int xe_build_sg(struct xe_device *xe, struct hmm_range *range,
>>> + struct sg_table *st, bool write)
>>> +{
>>> + struct device *dev = xe->drm.dev;
>>> + struct page **pages;
>>> + u64 i, npages;
>>> + int ret;
>>> +
>>> + npages = xe_npages_in_range(range->start, range->end);
>>> + pages = kvmalloc_array(npages, sizeof(*pages), GFP_KERNEL);
>>> + if (!pages)
>>> + return -ENOMEM;
>>> +
>>> + for (i = 0; i < npages; i++) {
>>> + pages[i] = hmm_pfn_to_page(range->hmm_pfns[i]);
>>> + xe_assert(xe, !is_device_private_page(pages[i]));
>>> + }
>>> +
>>> + ret = sg_alloc_table_from_pages_segment(st, pages, npages, 0, npages << PAGE_SHIFT,
>>> + xe_sg_segment_size(dev), GFP_KERNEL);
>>> + if (ret)
>>> + goto free_pages;
>>> +
>>> + ret = dma_map_sgtable(dev, st, write ? DMA_BIDIRECTIONAL : DMA_TO_DEVICE,
>>> + DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_NO_KERNEL_MAPPING);
>>> + if (ret) {
>>> + sg_free_table(st);
>>> + st = NULL;
>>> + }
>>> +
>>> +free_pages:
>>> + kvfree(pages);
>>> + return ret;
>>> +}
>>> +
>>> +/*
>>> + * xe_hmm_userptr_free_sg() - Free the scatter gather table of userptr
>>> + *
>>> + * @uvma: the userptr vma which hold the scatter gather table
>>> + *
>>> + * With function xe_userptr_populate_range, we allocate storage of
>>> + * the userptr sg table. This is a helper function to free this
>>> + * sg table, and dma unmap the address in the table.
>>> + */
>>> +void xe_hmm_userptr_free_sg(struct xe_userptr_vma *uvma)
>>> +{
>>> + struct xe_userptr *userptr = &uvma->userptr;
>>> + struct xe_vma *vma = &uvma->vma;
>>> + bool write = !xe_vma_read_only(vma);
>>> + struct xe_vm *vm = xe_vma_vm(vma);
>>> + struct xe_device *xe = vm->xe;
>>> + struct device *dev = xe->drm.dev;
>>> +
>>> + xe_assert(xe, userptr->sg);
>>> + dma_unmap_sgtable(dev, userptr->sg,
>>> + write ? DMA_BIDIRECTIONAL : DMA_TO_DEVICE, 0);
>>> +
>>> + sg_free_table(userptr->sg);
>>> + userptr->sg = NULL;
>>> +}
>>> +
>>> +/**
>>> + * xe_hmm_userptr_populate_range() - Populate physical pages of a virtual
>>> + * address range
>>> + *
>>> + * @uvma: userptr vma which has information of the range to populate.
>>> + * @is_mm_mmap_locked: True if mmap_read_lock is already acquired by caller.
>>> + *
>>> + * This function populate the physical pages of a virtual
>>> + * address range. The populated physical pages is saved in
>>> + * userptr's sg table. It is similar to get_user_pages but call
>>> + * hmm_range_fault.
>>> + *
>>> + * This function also read mmu notifier sequence # (
>>> + * mmu_interval_read_begin), for the purpose of later
>>> + * comparison (through mmu_interval_read_retry).
>>> + *
>>> + * This must be called with mmap read or write lock held.
>>> + *
>>> + * This function allocates the storage of the userptr sg table.
>>> + * It is caller's responsibility to free it calling sg_free_table.
>>> + *
>>> + * returns: 0 for succuss; negative error no on failure
>>> + */
>>> +int xe_hmm_userptr_populate_range(struct xe_userptr_vma *uvma,
>>> + bool is_mm_mmap_locked)
>>> +{
>>> + unsigned long timeout =
>>> + jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
>>> + unsigned long *pfns, flags = HMM_PFN_REQ_FAULT;
>>> + struct xe_userptr *userptr;
>>> + struct xe_vma *vma = &uvma->vma;
>>> + u64 start = xe_vma_userptr(vma);
>>> + u64 end = start + xe_vma_size(vma);
>> Nit: xe_vma_end
> Sure. Will address in next patch.
>>> + struct xe_vm *vm = xe_vma_vm(vma);
>>> + struct hmm_range hmm_range;
>>> + bool write = !xe_vma_read_only(vma);
>>> + unsigned long notifier_seq;
>>> + u64 npages;
>>> + int ret;
>>> +
>>> + userptr = &uvma->userptr;
>>> +
>>> + if (is_mm_mmap_locked)
>>> + mmap_assert_locked(userptr->notifier.mm);
>>> +
>>> + if (vma->gpuva.flags & XE_VMA_DESTROYED)
>>> + return 0;
>>> +
>>> + notifier_seq = mmu_interval_read_begin(&userptr->notifier);
>>> + if (notifier_seq == userptr->notifier_seq)
>>> + return 0;
>>> +
>>> + if (userptr->sg)
>>> + xe_hmm_userptr_free_sg(uvma);
>>> +
>>> + npages = xe_npages_in_range(start, end);
>>> + pfns = kvmalloc_array(npages, sizeof(*pfns), GFP_KERNEL);
>>> + if (unlikely(!pfns))
>>> + return -ENOMEM;
>>> +
>>> + if (write)
>>> + flags |= HMM_PFN_REQ_WRITE;
>>> +
>>> + if (!mmget_not_zero(userptr->notifier.mm)) {
>>> + ret = -EFAULT;
>>> + goto free_pfns;
>>> + }
>>> +
>>> + hmm_range.default_flags = flags;
>>> + hmm_range.hmm_pfns = pfns;
>>> + hmm_range.notifier = &userptr->notifier;
>>> + hmm_range.start = start;
>>> + hmm_range.end = end;
>>> + hmm_range.dev_private_owner = vm->xe;
>>> +
>>> + while (true) {
>>> + hmm_range.notifier_seq = mmu_interval_read_begin(&userptr->notifier);
>> Nit: This could be set above in this code:
>>
>> notifier_seq = mmu_interval_read_begin(&userptr->notifier);
>> if (notifier_seq == userptr->notifier_seq)
>> return 0;
>>
>> i.e. drop local notifier_seq and set hmm_range.notifier_seq...
>>
>> Then reset before continue in the if (ret == -EBUSY) function.
> I chose to retain |*mmu_interval_read_begin*| before |hmm_range_fault|
> because I anticipated a scenario where the notifier sequence might be
> invalid between the initial call and just before |hmm_range_fault|.
> In such a case, |hmm_range_fault| would return EBUSY, requiring us to
> unlock the mmap_lock, use a |time_after| routine, lock again, and call
> |hmm_range_fault| once more.
>
> However, by utilizing |mmu_interval_read_begin| before
> |hmm_range_fault|, we might avoid this issue. Any change in sequence
> between the initial call and this point wouldn't impact the process.
> Even if there's no change in sequence, we're only making an extra call
> to |mmu_interval_read_begin|.
>
> Both approaches seem to work, but if you believe my concern about
> changes in the notifier sequence is unfounded, I will proceed with the
> suggested changes.
>
>>> +
>>> + if (!is_mm_mmap_locked)
>>> + mmap_read_lock(userptr->notifier.mm);
>>> +
>>> + ret = hmm_range_fault(&hmm_range);
>>> +
>>> + if (!is_mm_mmap_locked)
>>> + mmap_read_unlock(userptr->notifier.mm);
>>> +
>>> + if (ret == -EBUSY) {
>>> + if (time_after(jiffies, timeout))
>>> + break;
>>> +
>>> + continue;
>>> + }
>>> + break;
>>> + }
>>> +
>>> + mmput(userptr->notifier.mm);
>>> +
>>> + if (ret)
>>> + goto free_pfns;
>>> +
>>> + ret = xe_build_sg(vm->xe, &hmm_range, &userptr->sgt, write);
>>> + if (ret)
>>> + goto free_pfns;
>>> +
>>> + xe_mark_range_accessed(&hmm_range, write);
>>> + userptr->sg = &userptr->sgt;
>>> + userptr->notifier_seq = hmm_range.notifier_seq;
>>> +
>>> +free_pfns:
>>> + kvfree(pfns);
>>> + return ret;
>>> +}
>>> +
>>> diff --git a/drivers/gpu/drm/xe/xe_hmm.h b/drivers/gpu/drm/xe/xe_hmm.h
>>> new file mode 100644
>>> index 000000000000..40250e3f84d1
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/xe/xe_hmm.h
>>> @@ -0,0 +1,18 @@
>>> +/* SPDX-License-Identifier: MIT
>>> + *
>>> + * Copyright © 2024 Intel Corporation
>>> + */
>>> +
>>> +#include <linux/types.h>
>>> +
>>> +struct xe_userptr_vma;
>>> +
>>> +#if IS_ENABLED(CONFIG_HMM_MIRROR)
>>> +int xe_hmm_userptr_populate_range(struct xe_userptr_vma *uvma, bool is_mm_mmap_locked);
>>> +#else
>>> +static inline int xe_hmm_userptr_populate_range(struct xe_userptr_vma *uvma, bool is_mm_mmap_locked)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +#endif
>> Mentioned that I don't think is needed in last rev as our driver require
>> CONFIG_HMM_MIRROR to be built.
>
> I noticed some KUnit failures in earlier patches, so I opted not to
> remove it. The KUnit configuration file doesn't include the HMM_MIRROR
> config. If we want to remove this we need to modify the KUnit configs
> to include this config, as it is essential for our driver.
>
> Please confirm it is OK to add the config to kunit configuration file ?
>
My bad in earlier patch HMM_MIRROR was under config DRM_XE_SVM , this
patch is selecting it under DRM_XE itself which is used by kunit conf
file. This should be safe to remove. Will remove it in next version.
>> Matt
>>
>>> +void xe_hmm_userptr_free_sg(struct xe_userptr_vma *uvma);
>>> --
>>> 2.25.1
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-xe/attachments/20240405/b8774f6c/attachment-0001.htm>
More information about the Intel-xe
mailing list