[PATCH 1/5] drm/xe/svm: Remap and provide memmap backing for GPU vram
Matthew Brost
matthew.brost at intel.com
Thu Mar 14 20:49:07 UTC 2024
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.
> 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.
>
> >
> > > 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?
> >
> > > }
> > >
> > > 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@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