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

[Matthew Brost <matthew.brost@intel.com>]

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).

> 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.

>  
>  # 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.

> +
>  };
>  
>  /**
> 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?

>  	}
>  
>  	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.

> +
>  }
>  
>  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?

> + */
> +
> +#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.

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.

> +
> +#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. 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?

> + */
> +
> +#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.

> +#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?

> +/**
> + * 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?

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.

> +{
> +	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.

Matt 

> +}
> +
> -- 
> 2.26.3
>