[PATCH 1/5] drm/xe/svm: Remap and provide memmap backing for GPU vram
Zeng, Oak
oak.zeng at intel.com
Fri Mar 15 16:00:06 UTC 2024
> -----Original Message-----
> From: Brost, Matthew <matthew.brost at intel.com>
> Sent: Thursday, March 14, 2024 4:49 PM
> To: Zeng, Oak <oak.zeng at intel.com>
> Cc: intel-xe at lists.freedesktop.org; Hellstrom, Thomas
> <thomas.hellstrom at intel.com>; airlied at gmail.com; Welty, Brian
> <brian.welty at intel.com>; Ghimiray, Himal Prasad
> <himal.prasad.ghimiray at intel.com>
> Subject: Re: [PATCH 1/5] drm/xe/svm: Remap and provide memmap backing for
> GPU vram
>
> On Thu, Mar 14, 2024 at 12:32:36PM -0600, Zeng, Oak wrote:
> > Hi Matt,
> >
> > > -----Original Message-----
> > > From: Brost, Matthew <matthew.brost at intel.com>
> > > Sent: Thursday, March 14, 2024 1:18 PM
> > > To: Zeng, Oak <oak.zeng at intel.com>
> > > Cc: intel-xe at lists.freedesktop.org; Hellstrom, Thomas
> > > <thomas.hellstrom at intel.com>; airlied at gmail.com; Welty, Brian
> > > <brian.welty at intel.com>; Ghimiray, Himal Prasad
> > > <himal.prasad.ghimiray at intel.com>
> > > Subject: Re: [PATCH 1/5] drm/xe/svm: Remap and provide memmap backing
> for
> > > GPU vram
> > >
> > > On Wed, Mar 13, 2024 at 11:35:49PM -0400, Oak Zeng wrote:
> > > > Memory remap GPU vram using devm_memremap_pages, so each GPU
> vram
> > > > page is backed by a struct page.
> > > >
> > > > Those struct pages are created to allow hmm migrate buffer b/t
> > > > GPU vram and CPU system memory using existing Linux migration
> > > > mechanism (i.e., migrating b/t CPU system memory and hard disk).
> > > >
> > > > This is prepare work to enable svm (shared virtual memory) through
> > > > Linux kernel hmm framework. The memory remap's page map type is set
> > > > to MEMORY_DEVICE_PRIVATE for now. This means even though each GPU
> > > > vram page get a struct page and can be mapped in CPU page table,
> > > > but such pages are treated as GPU's private resource, so CPU can't
> > > > access them. If CPU access such page, a page fault is triggered
> > > > and page will be migrate to system memory.
> > > >
> > >
> > > Is this really true? We can map VRAM BOs to the CPU without having
> > > migarte back and forth. Admittedly I don't know the inner workings of
> > > how this works but in IGTs we do this all the time.
> > >
> > > 54 batch_bo = xe_bo_create(fd, vm, batch_size,
> > > 55 vram_if_possible(fd, 0),
> > > 56 DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM);
> > > 57 batch_map = xe_bo_map(fd, batch_bo, batch_size);
> > >
> > > The BO is created in VRAM and then mapped to the CPU.
> > >
> > > I don't think there is an expectation of coherence rather caching mode
> > > and exclusive access of the memory based on synchronization.
> > >
> > > e.g.
> > > User write BB/data via CPU to GPU memory
> > > User calls exec
> > > GPU read / write memory
> > > User wait on sync indicating exec done
> > > User reads result
> > >
> > > All of this works without migration. Are we not planing supporting flow
> > > with SVM?
> > >
> > > Afaik this migration dance really only needs to be done if the CPU and
> > > GPU are using atomics on a shared memory region and the GPU device
> > > doesn't support a coherent memory protocol (e.g. PVC).
> >
> > All you said is true. On many of our HW, CPU can actually access device memory,
> cache coherently or not.
> >
> > The problem is, this is not true for all GPU vendors. For example, on some HW
> from some vendor, CPU can only access partially of device memory. The so called
> small bar concept.
> >
> > So when HMM is defined, such factors were considered, and
> MEMORY_DEVICE_PRIVATE is defined. With this definition, CPU can't access
> device memory.
> >
> > So you can think it is a limitation of HMM.
> >
>
> Is it though? I see other type MEMORY_DEVICE_FS_DAX,
> MEMORY_DEVICE_GENERIC, and MEMORY_DEVICE_PCI_P2PDMA. From my
> limited
> understanding it looks to me like one of those modes would support my
> example.
No, above are for other purposes. HMM only support DEVICE_PRIVATE and DEVICE_COHERENT.
>
> > Note this is only part 1 of our system allocator work. We do plan to support
> DEVICE_COHERENT for our newer device, see below. With this option, we don't
> have unnecessary migration back and forth.
> >
> > You can think this is just work out all the code path. 90% of the driver code for
> DEVICE_PRIVATE and DEVICE_COHERENT will be same. Our real use of system
> allocator will be DEVICE_COHERENT mode. While DEVICE_PRIVATE mode allow us
> to exercise the code on old HW.
> >
> > Make sense?
> >
>
> I guess if we want the system allocator to always coherent, then yes you
> need this dynamic migration with faulting on either side.
>
> I was thinking the system allocator would be behave like my example
> above with madvise dictating the coherence rules.
>
> Maybe I missed this in system allocator design but my feeling is we
> shouldn't arbitrarily enforce coherence as that could lead to poor
> performance due to constant migration.
System allocator itself doesn't enforce coherence. Coherence is built in user programming model. So system allocator allow both GPU and CPU access system allocated pointers, but it doesn't necessarily guarantee the data accessed from CPU/GPU is coherent. It is user program's responsibility to maintain data coherence.
Data migration in driver is optional, depending on platform capability, user preference, correctness and performance consideration. Driver internal data migration of course shouldn't break data coherence.
Of course different vendor can have different data coherence scheme. For example, it is completely designer's flexibility to build model that is HW automatic data coherence or software explicit data coherence. On our platform, we allow user program to select different coherence mode by setting pat_index for gpu and cpu_caching mode for CPU. So we have completely give the flexibility to user program. Nothing of this contract is changed in system allocator design.
Going back to the question of what memory type we should use to register our vram to core mm. HMM currently support two types: PRIVATE and COHERENT. The coherent type requires some HW and BIOS support which we don't have right now. So the only available is PRIVATE. We have not other option right now. As said, we plan to support coherent type where we can avoid unnecessary data migration. But that is stage 2.
>
> >
> > >
> > > > For GPU device which supports coherent memory protocol b/t CPU and
> > > > GPU (such as CXL and CAPI protocol), we can remap device memory as
> > > > MEMORY_DEVICE_COHERENT. This is TBD.
> > > >
> > > > 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>
> > > > ---
> > > > drivers/gpu/drm/xe/Makefile | 3 +-
> > > > drivers/gpu/drm/xe/xe_device_types.h | 9 +++
> > > > drivers/gpu/drm/xe/xe_mmio.c | 8 +++
> > > > drivers/gpu/drm/xe/xe_svm.h | 14 +++++
> > > > drivers/gpu/drm/xe/xe_svm_devmem.c | 91
> > > ++++++++++++++++++++++++++++
> > > > 5 files changed, 124 insertions(+), 1 deletion(-)
> > > > create mode 100644 drivers/gpu/drm/xe/xe_svm.h
> > > > create mode 100644 drivers/gpu/drm/xe/xe_svm_devmem.c
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> > > > index c531210695db..840467080e59 100644
> > > > --- a/drivers/gpu/drm/xe/Makefile
> > > > +++ b/drivers/gpu/drm/xe/Makefile
> > > > @@ -142,7 +142,8 @@ xe-y += xe_bb.o \
> > > > xe_vram_freq.o \
> > > > xe_wait_user_fence.o \
> > > > xe_wa.o \
> > > > - xe_wopcm.o
> > > > + xe_wopcm.o \
> > > > + xe_svm_devmem.o
> > >
> > > These should be in alphabetical order.
> >
> > Will fix
> > >
> > > >
> > > > # graphics hardware monitoring (HWMON) support
> > > > xe-$(CONFIG_HWMON) += xe_hwmon.o
> > > > diff --git a/drivers/gpu/drm/xe/xe_device_types.h
> > > b/drivers/gpu/drm/xe/xe_device_types.h
> > > > index 9785eef2e5a4..f27c3bee8ce7 100644
> > > > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > > > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > > > @@ -99,6 +99,15 @@ struct xe_mem_region {
> > > > resource_size_t actual_physical_size;
> > > > /** @mapping: pointer to VRAM mappable space */
> > > > void __iomem *mapping;
> > > > + /** @pagemap: Used to remap device memory as ZONE_DEVICE */
> > > > + struct dev_pagemap pagemap;
> > > > + /**
> > > > + * @hpa_base: base host physical address
> > > > + *
> > > > + * This is generated when remap device memory as ZONE_DEVICE
> > > > + */
> > > > + resource_size_t hpa_base;
> > >
> > > Weird indentation. This goes for the entire series, look at checkpatch.
> >
> > Will fix
> > >
> > > > +
> > > > };
> > > >
> > > > /**
> > > > diff --git a/drivers/gpu/drm/xe/xe_mmio.c
> b/drivers/gpu/drm/xe/xe_mmio.c
> > > > index e3db3a178760..0d795394bc4c 100644
> > > > --- a/drivers/gpu/drm/xe/xe_mmio.c
> > > > +++ b/drivers/gpu/drm/xe/xe_mmio.c
> > > > @@ -22,6 +22,7 @@
> > > > #include "xe_module.h"
> > > > #include "xe_sriov.h"
> > > > #include "xe_tile.h"
> > > > +#include "xe_svm.h"
> > > >
> > > > #define XEHP_MTCFG_ADDR XE_REG(0x101800)
> > > > #define TILE_COUNT REG_GENMASK(15, 8)
> > > > @@ -286,6 +287,7 @@ int xe_mmio_probe_vram(struct xe_device *xe)
> > > > }
> > > >
> > > > io_size -= min_t(u64, tile_size, io_size);
> > > > + xe_svm_devm_add(tile, &tile->mem.vram);
> > >
> > > Do we want to do this probe for all devices with VRAM or only a subset?
> >
> > All
>
> Can you explain why?
It is natural for me to add all device memory to hmm. In hmm design, device memory is used as a special swap out for system memory. I would ask why we only want to add a subset of vram? By a subset, do you mean only vram of one tile instead of all tiles?
Oak
>
> > >
> > > > }
> > > >
> > > > xe->mem.vram.actual_physical_size = total_size;
> > > > @@ -354,10 +356,16 @@ void xe_mmio_probe_tiles(struct xe_device *xe)
> > > > static void mmio_fini(struct drm_device *drm, void *arg)
> > > > {
> > > > struct xe_device *xe = arg;
> > > > + struct xe_tile *tile;
> > > > + u8 id;
> > > >
> > > > pci_iounmap(to_pci_dev(xe->drm.dev), xe->mmio.regs);
> > > > if (xe->mem.vram.mapping)
> > > > iounmap(xe->mem.vram.mapping);
> > > > +
> > > > + for_each_tile(tile, xe, id)
> > > > + xe_svm_devm_remove(xe, &tile->mem.vram);
> > >
> > > This should probably be above existing code. Typical on fini to do
> > > things in reverse order from init.
> >
> > Will fix
> > >
> > > > +
> > > > }
> > > >
> > > > static int xe_verify_lmem_ready(struct xe_device *xe)
> > > > diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h
> > > > new file mode 100644
> > > > index 000000000000..09f9afb0e7d4
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/xe/xe_svm.h
> > > > @@ -0,0 +1,14 @@
> > > > +// SPDX-License-Identifier: MIT
> > > > +/*
> > > > + * Copyright © 2023 Intel Corporation
> > >
> > > 2024?
> >
> > This patch was actually written 2023
> > >
> > > > + */
> > > > +
> > > > +#ifndef __XE_SVM_H
> > > > +#define __XE_SVM_H
> > > > +
> > > > +#include "xe_device_types.h"
> > >
> > > I don't think you need to include this. Rather just forward decl structs
> > > used here.
> >
> > Will fix
> > >
> > > e.g.
> > >
> > > struct xe_device;
> > > struct xe_mem_region;
> > > struct xe_tile;
> > >
> > > > +
> > > > +int xe_svm_devm_add(struct xe_tile *tile, struct xe_mem_region *mem);
> > > > +void xe_svm_devm_remove(struct xe_device *xe, struct xe_mem_region
> > > *mem);
> > >
> > > The arguments here are incongruent here. Typically we want these to
> > > match.
> >
> > Will fix
> > >
> > > > +
> > > > +#endif
> > > > diff --git a/drivers/gpu/drm/xe/xe_svm_devmem.c
> > > b/drivers/gpu/drm/xe/xe_svm_devmem.c
> > >
> > > Incongruent between xe_svm.h and xe_svm_devmem.c.
> >
> > Did you mean mem vs mr? if yes, will fix
> >
> > Again these two
> > > should
> > > match.
> > >
> > > > new file mode 100644
> > > > index 000000000000..63b7a1961cc6
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/xe/xe_svm_devmem.c
> > > > @@ -0,0 +1,91 @@
> > > > +// SPDX-License-Identifier: MIT
> > > > +/*
> > > > + * Copyright © 2023 Intel Corporation
> > >
> > > 2024?
> > It is from 2023
> > >
> > > > + */
> > > > +
> > > > +#include <linux/mm_types.h>
> > > > +#include <linux/sched/mm.h>
> > > > +
> > > > +#include "xe_device_types.h"
> > > > +#include "xe_trace.h"
> > >
> > > xe_trace.h appears to be unused.
> >
> > Will fix
> > >
> > > > +#include "xe_svm.h"
> > > > +
> > > > +
> > > > +static vm_fault_t xe_devm_migrate_to_ram(struct vm_fault *vmf)
> > > > +{
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void xe_devm_page_free(struct page *page)
> > > > +{
> > > > +}
> > > > +
> > > > +static const struct dev_pagemap_ops xe_devm_pagemap_ops = {
> > > > + .page_free = xe_devm_page_free,
> > > > + .migrate_to_ram = xe_devm_migrate_to_ram,
> > > > +};
> > > > +
> > >
> > > Assume these are placeholders that will be populated later?
> >
> >
> > corrrect
> > >
> > > > +/**
> > > > + * xe_svm_devm_add: Remap and provide memmap backing for device
> > > memory
> > > > + * @tile: tile that the memory region blongs to
> > > > + * @mr: memory region to remap
> > > > + *
> > > > + * This remap device memory to host physical address space and create
> > > > + * struct page to back device memory
> > > > + *
> > > > + * Return: 0 on success standard error code otherwise
> > > > + */
> > > > +int xe_svm_devm_add(struct xe_tile *tile, struct xe_mem_region *mr)
> > >
> > > Here I see the xe_mem_region is from tile->mem.vram, wondering rather
> > > than using the tile->mem.vram we should use xe->mem.vram when enabling
> > > svm? Isn't the idea behind svm the entire memory is 1 view?
> >
> > Still need to use tile. The reason is, memory of different tile can have different
> characteristics, such as latency. So we want to differentiate memory b/t tiles also
> in svm. I need to change below " mr->pagemap.owner = tile->xe->drm.dev ". the
> owner should also be tile. This is the way hmm differentiate memory of different
> tile.
> >
> > With svm it is 1 view, from virtual address space perspective and from physical
> struct page perspective. You can think of all the tile's vram is stacked together to
> form a unified view together with system memory. This doesn't prohibit us from
> differentiate memory from different tile. This differentiation allow us to optimize
> performance, i.e., we can wisely place memory in specific tile. If we don't
> differentiate, this is not possible.
> >
>
> Ok makes sense.
>
> Matt
>
> > >
> > > I suppose if we do that we also only use 1 TTM VRAM manager / buddy
> > > allocator too. I thought I saw some patches flying around for that too.
> >
> > Ttm vram manager is not in the picture. We deliberately avoided it per previous
> discussion
> >
> > Yes same buddy allocator. It is in my previous POC: https://lore.kernel.org/dri-
> devel/20240117221223.18540-12-oak.zeng at intel.com/. I didn't put those patches
> in this series because I want to merge this small patches separately.
> > >
> > > > +{
> > > > + struct device *dev = &to_pci_dev(tile->xe->drm.dev)->dev;
> > > > + struct resource *res;
> > > > + void *addr;
> > > > + int ret;
> > > > +
> > > > + res = devm_request_free_mem_region(dev, &iomem_resource,
> > > > + mr->usable_size);
> > > > + if (IS_ERR(res)) {
> > > > + ret = PTR_ERR(res);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + mr->pagemap.type = MEMORY_DEVICE_PRIVATE;
> > > > + mr->pagemap.range.start = res->start;
> > > > + mr->pagemap.range.end = res->end;
> > > > + mr->pagemap.nr_range = 1;
> > > > + mr->pagemap.ops = &xe_devm_pagemap_ops;
> > > > + mr->pagemap.owner = tile->xe->drm.dev;
> > > > + addr = devm_memremap_pages(dev, &mr->pagemap);
> > > > + if (IS_ERR(addr)) {
> > > > + devm_release_mem_region(dev, res->start, resource_size(res));
> > > > + ret = PTR_ERR(addr);
> > > > + drm_err(&tile->xe->drm, "Failed to remap tile %d memory,
> > > errno %d\n",
> > > > + tile->id, ret);
> > > > + return ret;
> > > > + }
> > > > + mr->hpa_base = res->start;
> > > > +
> > > > + drm_info(&tile->xe->drm, "Added tile %d memory [%llx-%llx] to devm,
> > > remapped to %pr\n",
> > > > + tile->id, mr->io_start, mr->io_start + mr->usable_size,
> > > res);
> > > > + return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * xe_svm_devm_remove: Unmap device memory and free resources
> > > > + * @xe: xe device
> > > > + * @mr: memory region to remove
> > > > + */
> > > > +void xe_svm_devm_remove(struct xe_device *xe, struct xe_mem_region
> > > *mr)
> > > > +{
> > > > + /*FIXME: below cause a kernel hange during moduel remove*/
> > > > +#if 0
> > > > + struct device *dev = &to_pci_dev(xe->drm.dev)->dev;
> > > > +
> > > > + if (mr->hpa_base) {
> > > > + devm_memunmap_pages(dev, &mr->pagemap);
> > > > + devm_release_mem_region(dev, mr->pagemap.range.start,
> > > > + mr->pagemap.range.end - mr->pagemap.range.start +1);
> > > > + }
> > > > +#endif
> > >
> > > This would need to be fixed too.
> >
> >
> > Yes...
> >
> > Oak
> > >
> > > Matt
> > >
> > > > +}
> > > > +
> > > > --
> > > > 2.26.3
> > > >
More information about the Intel-xe
mailing list