Re: [RFC v2 06/12] drm/i915/svm: Device memory support
On Tue, Dec 17, 2019 at 08:35:47PM +, Jason Gunthorpe wrote:
On Fri, Dec 13, 2019 at 01:56:08PM -0800, Niranjana Vishwanathapura wrote:
@@ -169,6 +170,11 @@ static int i915_range_fault(struct svm_notifier *sn,
return ret;
}
+ /* For dgfx, ensure the range is in device local memory only */
+ regions = i915_dmem_convert_pfn(vm->i915, &range);
+ if (!regions || (IS_DGFX(vm->i915) && (regions & REGION_SMEM)))
+ return -EINVAL;
+
This is not OK, as I said before, the driver cannot de-reference pfns
before doing the retry check, under lock.
Thanks.
Ok, will push it down and do it after validating the range.
+
+int i915_dmem_convert_pfn(struct drm_i915_private *dev_priv,
+ struct hmm_range *range)
+{
+ unsigned long i, npages;
+ int regions = 0;
+
+ npages = (range->end - range->start) >> PAGE_SHIFT;
+ for (i = 0; i < npages; ++i) {
+ struct i915_buddy_block *block;
+ struct intel_memory_region *mem;
+ struct page *page;
+ u64 addr;
+
+ page = hmm_device_entry_to_page(range, range->pfns[i]);
^^
For instance, that cannot be done on a speculatively loaded page.
This also looks like it suffers from the same bug as
Ok.
+ if (!page)
+ continue;
+
+ if (!(range->pfns[i] & range->flags[HMM_PFN_DEVICE_PRIVATE])) {
+ regions |= REGION_SMEM;
+ continue;
+ }
+
+ if (!i915_dmem_page(dev_priv, page)) {
+ WARN(1, "Some unknown device memory !\n");
Why is that a WARN? The user could put other device memory in the
address space. You need to 'segfault' the GPU execution if this happens.
OK, will return an error here if user is trying to bind here.
I agree, we need to segfault the GPU if it is GPU fault handling.
+ range->pfns[i] = 0;
+ continue;
+ }
+
+ regions |= REGION_LMEM;
+ block = page->zone_device_data;
+ mem = block->private;
+ addr = mem->region.start +
+ i915_buddy_block_offset(block);
+ addr += (page_to_pfn(page) - block->pfn_first) << PAGE_SHIFT;
+
+ range->pfns[i] &= ~range->flags[HMM_PFN_DEVICE_PRIVATE];
+ range->pfns[i] &= ((1UL << range->pfn_shift) - 1);
+ range->pfns[i] |= (addr >> PAGE_SHIFT) << range->pfn_shift;
This makes more sense as a direct manipulation of the sgl, not sure
why this routine is split out from the sgl builder?
Ok, yah, let me merge it with sgl building.
Thanks,
Niranjana
Jason
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 06/12] drm/i915/svm: Device memory support
On Fri, Dec 13, 2019 at 01:56:08PM -0800, Niranjana Vishwanathapura wrote:
> @@ -169,6 +170,11 @@ static int i915_range_fault(struct svm_notifier *sn,
> return ret;
> }
>
> + /* For dgfx, ensure the range is in device local memory only */
> + regions = i915_dmem_convert_pfn(vm->i915, &range);
> + if (!regions || (IS_DGFX(vm->i915) && (regions & REGION_SMEM)))
> + return -EINVAL;
> +
This is not OK, as I said before, the driver cannot de-reference pfns
before doing the retry check, under lock.
> +
> +int i915_dmem_convert_pfn(struct drm_i915_private *dev_priv,
> + struct hmm_range *range)
> +{
> + unsigned long i, npages;
> + int regions = 0;
> +
> + npages = (range->end - range->start) >> PAGE_SHIFT;
> + for (i = 0; i < npages; ++i) {
> + struct i915_buddy_block *block;
> + struct intel_memory_region *mem;
> + struct page *page;
> + u64 addr;
> +
> + page = hmm_device_entry_to_page(range, range->pfns[i]);
^^
For instance, that cannot be done on a speculatively loaded page.
This also looks like it suffers from the same bug as
> + if (!page)
> + continue;
> +
> + if (!(range->pfns[i] & range->flags[HMM_PFN_DEVICE_PRIVATE])) {
> + regions |= REGION_SMEM;
> + continue;
> + }
> +
> + if (!i915_dmem_page(dev_priv, page)) {
> + WARN(1, "Some unknown device memory !\n");
Why is that a WARN? The user could put other device memory in the
address space. You need to 'segfault' the GPU execution if this happens.
> + range->pfns[i] = 0;
> + continue;
> + }
> +
> + regions |= REGION_LMEM;
> + block = page->zone_device_data;
> + mem = block->private;
> + addr = mem->region.start +
> +i915_buddy_block_offset(block);
> + addr += (page_to_pfn(page) - block->pfn_first) << PAGE_SHIFT;
> +
> + range->pfns[i] &= ~range->flags[HMM_PFN_DEVICE_PRIVATE];
> + range->pfns[i] &= ((1UL << range->pfn_shift) - 1);
> + range->pfns[i] |= (addr >> PAGE_SHIFT) << range->pfn_shift;
This makes more sense as a direct manipulation of the sgl, not sure
why this routine is split out from the sgl builder?
Jason
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC v2 06/12] drm/i915/svm: Device memory support
Plugin device memory through HMM as DEVICE_PRIVATE.
Add support functions to allocate pages and free pages from device memory.
Implement ioctl to prefetch pages from host to device memory.
For now, only support migrating pages from host memory to device memory.
Cc: Joonas Lahtinen
Cc: Jon Bloomfield
Cc: Daniel Vetter
Cc: Sudeep Dutt
Signed-off-by: Niranjana Vishwanathapura
---
drivers/gpu/drm/i915/Kconfig | 9 +
drivers/gpu/drm/i915/Makefile | 3 +-
drivers/gpu/drm/i915/gem/i915_gem_object.c | 13 -
drivers/gpu/drm/i915/i915_buddy.h | 12 +
drivers/gpu/drm/i915/i915_drv.c| 1 +
drivers/gpu/drm/i915/i915_svm.c| 6 +
drivers/gpu/drm/i915/i915_svm.h| 15 +
drivers/gpu/drm/i915/i915_svm_devmem.c | 400 +
drivers/gpu/drm/i915/intel_memory_region.h | 14 +
drivers/gpu/drm/i915/intel_region_lmem.c | 10 +
include/uapi/drm/i915_drm.h| 22 ++
11 files changed, 491 insertions(+), 14 deletions(-)
create mode 100644 drivers/gpu/drm/i915/i915_svm_devmem.c
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 689e57fe3973..66337f2ca2bf 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -141,9 +141,18 @@ config DRM_I915_SVM
bool "Enable Shared Virtual Memory support in i915"
depends on STAGING
depends on DRM_I915
+ depends on ARCH_ENABLE_MEMORY_HOTPLUG
+ depends on ARCH_ENABLE_MEMORY_HOTREMOVE
+ depends on MEMORY_HOTPLUG
+ depends on MEMORY_HOTREMOVE
+ depends on ARCH_HAS_PTE_DEVMAP
+ depends on SPARSEMEM_VMEMMAP
+ depends on ZONE_DEVICE
+ depends on DEVICE_PRIVATE
depends on MMU
select HMM_MIRROR
select MMU_NOTIFIER
+ select MIGRATE_VMA_HELPER
default n
help
Choose this option if you want Shared Virtual Memory (SVM)
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 7d4cd9eefd12..b574ec31ea2e 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -155,7 +155,8 @@ i915-y += \
# SVM code
i915-$(CONFIG_DRM_I915_SVM) += gem/i915_gem_svm.o \
- i915_svm.o
+ i915_svm.o \
+ i915_svm_devmem.o
# general-purpose microcontroller (GuC) support
obj-y += gt/uc/
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 003d81c171d2..f868a301fc04 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -504,19 +504,6 @@ int __init i915_global_objects_init(void)
return 0;
}
-static enum intel_region_id
-__region_id(u32 region)
-{
- enum intel_region_id id;
-
- for (id = 0; id < INTEL_REGION_UNKNOWN; ++id) {
- if (intel_region_map[id] == region)
- return id;
- }
-
- return INTEL_REGION_UNKNOWN;
-}
-
bool
i915_gem_object_svm_mapped(struct drm_i915_gem_object *obj)
{
diff --git a/drivers/gpu/drm/i915/i915_buddy.h
b/drivers/gpu/drm/i915/i915_buddy.h
index ed41f3507cdc..afc493e6c130 100644
--- a/drivers/gpu/drm/i915/i915_buddy.h
+++ b/drivers/gpu/drm/i915/i915_buddy.h
@@ -9,6 +9,9 @@
#include
#include
+/* 512 bits (one per pages) supports 2MB blocks */
+#define I915_BUDDY_MAX_PAGES 512
+
struct i915_buddy_block {
#define I915_BUDDY_HEADER_OFFSET GENMASK_ULL(63, 12)
#define I915_BUDDY_HEADER_STATE GENMASK_ULL(11, 10)
@@ -32,6 +35,15 @@ struct i915_buddy_block {
*/
struct list_head link;
struct list_head tmp_link;
+
+ unsigned long pfn_first;
+ /*
+* FIXME: There are other alternatives to bitmap. Like splitting the
+* block into contiguous 4K sized blocks. But it is part of bigger
+* issues involving partially invalidating large mapping, freeing the
+* blocks etc., revisit.
+*/
+ unsigned long bitmap[BITS_TO_LONGS(I915_BUDDY_MAX_PAGES)];
};
#define I915_BUDDY_MAX_ORDER I915_BUDDY_HEADER_ORDER
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 866d3cbb1edf..f1b92fd3d234 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2765,6 +2765,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
DRM_IOCTL_DEF_DRV(I915_GEM_VM_CREATE, i915_gem_vm_create_ioctl,
DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(I915_GEM_VM_DESTROY, i915_gem_vm_destroy_ioctl,
DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(I915_GEM_VM_BIND, i915_gem_vm_bind_ioctl,
DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF_DRV(I915_GEM_VM_PREFETCH, i915_gem_vm_prefetch_ioctl,
DRM_RENDER_ALLOW)
};
static struct drm_driver driver = {
diff --git a/drivers/gpu/drm/i915/i915_svm.c b/drivers/gpu/drm/i915/i915_svm.c
index 5941be5b5803..31a80ae0dd45 100644
--- a/drivers/gpu/drm/i915/i915_sv
