RE: [PATCH 1/5] drm/xe/svm: Remap and provide memmap backing for GPU vram

["Zeng, Oak" <oak.zeng@intel.com>]

> -----Original Message-----
> From: Brost, Matthew <matthew.brost@intel.com>
> Sent: Friday, March 15, 2024 9:26 PM
> To: Zeng, Oak <oak.zeng@intel.com>
> Cc: intel-xe@lists.freedesktop.org; Hellstrom, Thomas
> <thomas.hellstrom@intel.com>; airlied@gmail.com; Welty, Brian
> <brian.welty@intel.com>; Ghimiray, Himal Prasad
> <himal.prasad.ghimiray@intel.com>
> Subject: Re: [PATCH 1/5] drm/xe/svm: Remap and provide memmap backing for
> GPU vram
> 
> On Fri, Mar 15, 2024 at 03:31:24PM -0600, Zeng, Oak wrote:
> >
> >
> > > -----Original Message-----
> > > From: Brost, Matthew <matthew.brost@intel.com>
> > > Sent: Friday, March 15, 2024 4:40 PM
> > > To: Zeng, Oak <oak.zeng@intel.com>
> > > Cc: intel-xe@lists.freedesktop.org; Hellstrom, Thomas
> > > <thomas.hellstrom@intel.com>; airlied@gmail.com; Welty, Brian
> > > <brian.welty@intel.com>; Ghimiray, Himal Prasad
> > > <himal.prasad.ghimiray@intel.com>
> > > Subject: Re: [PATCH 1/5] drm/xe/svm: Remap and provide memmap backing
> for
> > > GPU vram
> > >
> > > On Fri, Mar 15, 2024 at 10:00:06AM -0600, Zeng, Oak wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Brost, Matthew <matthew.brost@intel.com>
> > > > > Sent: Thursday, March 14, 2024 4:49 PM
> > > > > To: Zeng, Oak <oak.zeng@intel.com>
> > > > > Cc: intel-xe@lists.freedesktop.org; Hellstrom, Thomas
> > > > > <thomas.hellstrom@intel.com>; airlied@gmail.com; Welty, Brian
> > > > > <brian.welty@intel.com>; Ghimiray, Himal Prasad
> > > > > <himal.prasad.ghimiray@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@intel.com>
> > > > > > > Sent: Thursday, March 14, 2024 1:18 PM
> > > > > > > To: Zeng, Oak <oak.zeng@intel.com>
> > > > > > > Cc: intel-xe@lists.freedesktop.org; Hellstrom, Thomas
> > > > > > > <thomas.hellstrom@intel.com>; airlied@gmail.com; Welty, Brian
> > > > > > > <brian.welty@intel.com>; Ghimiray, Himal Prasad
> > > > > > > <himal.prasad.ghimiray@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.
> > > >
> > >
> > > Thanks for the explaination. After reading your replies, the HMM doc,
> > > and looking at code this all makes sense.
> > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > > 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@intel.com>
> > > > > > > > Co-developed-by: Niranjana Vishwanathapura
> > > > > > > <niranjana.vishwanathapura@intel.com>
> > > > > > > > Signed-off-by: Niranjana Vishwanathapura
> > > > > > > <niranjana.vishwanathapura@intel.com>
> > > > > > > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > > > > > > Cc: Thomas Hellström <thomas.hellstrom@intel.com>
> > > > > > > > Cc: Brian Welty <brian.welty@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?
> > > >
> > >
> > > I think we talking about different things, my bad on wording in the
> > > original question.
> > >
> > > Let me ask again - should be calling xe_svm_devm_add on all *platforms*
> > > that have VRAM. i.e. Should we do this on PVC but not DG2?
> >
> >
> > Oh, I see. Good question. On i915, this feature was only tested on PVC. We
> don't have a plan to enable it on older platform than PVC.
> >
> > Let me add a check here, only enabled it on platform newer than PVC
> >
> 
> Probably actually check 'xe->info.has_usm'.
> 
> We might want to rename field too and drop the 'usm' nomenclature but
> that can be done later.

Good idea. Let me check with "has_usm"

Oak

> 
> Matt
> 
> > Oak
> >
> > >
> > > Matt
> > >
> > > > 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@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
> > > > > > > >